Expose discovered MCP tools through the act allowlist (#251)

Second half of the MCP client: the tools the manager discovers become rows in the existing act allowlist instead of a parallel capability system. An MCP tool is encoded in the columns that already exist — cmd ["mcp",<server>,<tool>], scope mcp:<server> — so no migration, and ProposeTool/EnableTool/DisableTool, tool.Matcher and the confirm turn need no changes. One branch in Executor.Exec routes such a row to the manager instead of exec, and "mcp" is never run as a binary. Discovery only ever PROPOSES. destructive comes from the inverse of the MCP readOnlyHint, so a tool that does not promise to be read-only inherits the confirm turn, and enabling stays on /tools behind step-up. Voice args are positional and MCP args are named, so CallPositional binds only what it can defend: no required properties runs bare, and a read-only tool with exactly one required string or number gets the tail. Everything else refuses with ErrNeedsArgs rather than guessing. The read-only condition was learned against the live Vikunja server: update_task requires only task_id and takes the rest as optional, so one guessed argument blanked the fields it did not mention. A partially-filled write destroys what it omits, so a mutating tool never receives a guessed argument. Also: a read-only mcp_servers IPC method and an "MCP servers" card on /tools showing transport, target and state, with the trust level of a local target spelled out. There is deliberately no call-a-tool IPC method and no run button, so mutation keeps exactly one path. Vikunja #251
2026-08-01 04:36:40 +04:00
parent 95ae900a58
commit 8d5e357b57
20 changed files with 863 additions and 4 deletions
@@ -13,6 +13,11 @@
 //   - Args are passed as argv, NEVER through a shell. STT text lands as
 //     positional arguments to Cmd; there is no `sh -c`, so "restart nginx;
 //     rm -rf" can't inject — the tail is one argv element to the named binary.
+//   - An enabled row whose cmd is ["mcp", "<server>", "<tool>"] is a call to a
+//     configured MCP server instead of a process (Vikunja #251). It goes
+//     through every rule above unchanged — enabled, and confirmed if it
+//     mutates — because the store is still the allowlist; only the dispatch at
+//     the bottom of Exec differs.
 //   - Destructive tools don't run on first hearing: Exec returns ErrNeedsConfirm
 //     and the handler runs a confirm turn ("выполнить X? да/нет"); only a
 //     confirmed re-Exec runs them. A gate assumes a fully-formed action, which
@@ -31,6 +36,7 @@ import (
 	"time"

 	"github.com/kami/maven/internal/ipc"
+	"github.com/kami/maven/internal/mcp"
 	"github.com/kami/maven/internal/router"
 )

@@ -50,12 +56,21 @@ var (
 	ErrNeedsConfirm = errors.New("destructive tool needs confirmation")
 )

+// MCPCaller is the seam for an act that is an MCP tool call rather than a
+// process (Vikunja #251). internal/mcp.Manager satisfies it via CallPositional.
+// nil ⇒ MCP is not configured, and an MCP row refuses to run rather than
+// silently doing nothing.
+type MCPCaller interface {
+	CallPositional(ctx context.Context, server, tool string, args []string) (string, error)
+}
+
 // Executor runs enabled tools. run is the exec seam (default: real process);
 // tests swap it. timeout bounds each invocation.
 type Executor struct {
 	api     API
 	timeout time.Duration
 	run     func(ctx context.Context, argv []string) (string, error)
+	mcp     MCPCaller
 }

 // NewExecutor builds the executor. timeout<=0 defaults to 30s.
@@ -66,6 +81,13 @@ func NewExecutor(api API, timeout time.Duration) *Executor {
 	return &Executor{api: api, timeout: timeout, run: runProcess}
 }

+// WithMCP attaches the MCP caller. Called once at wiring time when the mcp
+// config block is present; without it, a row whose cmd is ["mcp", …] refuses.
+func (e *Executor) WithMCP(m MCPCaller) *Executor {
+	e.mcp = m
+	return e
+}
+
 // Exec looks up name in the store and runs Cmd+args as argv (no shell).
 // confirmed=true is the second turn of a destructive act (the user said "да");
 // it bypasses the ErrNeedsConfirm gate. Non-enabled ⇒ ErrNotEnabled; a
@@ -84,6 +106,17 @@ func (e *Executor) Exec(ctx context.Context, name string, args []string, confirm
 	if t.Destructive && !confirmed {
 		return "", ErrNeedsConfirm
 	}
+	// An MCP row is a call to a configured server, not a process. Everything
+	// above still applied: it had to be enabled, and a mutating one had to be
+	// confirmed. Only the dispatch differs.
+	if server, remote, ok := mcp.ParseCmd(t.Cmd); ok {
+		if e.mcp == nil {
+			return "", ErrNotEnabled
+		}
+		ctx, cancel := context.WithTimeout(ctx, e.timeout)
+		defer cancel()
+		return e.mcp.CallPositional(ctx, server, remote, args)
+	}
 	argv := append(append([]string(nil), t.Cmd...), args...)
 	if len(argv) == 0 {
 		return "", ErrNotEnabled
@@ -85,3 +85,103 @@ func TestExec(t *testing.T) {
 		t.Fatal("proposed tool must not match (not enabled)")
 	}
 }
+
+// fakeMCP records what the executor asked it to call.
+type fakeMCP struct {
+	server, tool string
+	args         []string
+	out          string
+	err          error
+	calls        int
+}
+
+func (f *fakeMCP) CallPositional(_ context.Context, server, tool string, args []string) (string, error) {
+	f.calls++
+	f.server, f.tool, f.args = server, tool, args
+	return f.out, f.err
+}
+
+// An MCP row dispatches to the caller instead of a process, and the process
+// seam is never touched.
+func TestExecMCPRowDispatchesToMCP(t *testing.T) {
+	api := fakeAPI{tools: map[string]ipc.Tool{
+		"vikunja_list_tasks": {
+			Name: "vikunja_list_tasks", Status: "enabled", Scope: "mcp:vikunja",
+			Cmd: []string{"mcp", "vikunja", "list_tasks"},
+		},
+	}}
+	m := &fakeMCP{out: "две задачи"}
+	ran := false
+	e := NewExecutor(api, time.Second).WithMCP(m)
+	e.run = func(context.Context, []string) (string, error) { ran = true; return "", nil }
+
+	out, err := e.Exec(context.Background(), "vikunja_list_tasks", []string{"мавен"}, false)
+	if err != nil {
+		t.Fatalf("exec: %v", err)
+	}
+	if out != "две задачи" {
+		t.Fatalf("out = %q", out)
+	}
+	if ran {
+		t.Fatal("an MCP row must not be executed as a process")
+	}
+	if m.server != "vikunja" || m.tool != "list_tasks" || len(m.args) != 1 || m.args[0] != "мавен" {
+		t.Fatalf("dispatched wrong: %+v", m)
+	}
+}
+
+// The allowlist rules still apply to an MCP row: destructive means a confirm
+// turn first, and nothing is called until the second turn.
+func TestExecMCPRowStillNeedsConfirm(t *testing.T) {
+	api := fakeAPI{tools: map[string]ipc.Tool{
+		"vikunja_delete_task": {
+			Name: "vikunja_delete_task", Status: "enabled", Destructive: true,
+			Cmd: []string{"mcp", "vikunja", "delete_task"},
+		},
+	}}
+	m := &fakeMCP{out: "удалила"}
+	e := NewExecutor(api, time.Second).WithMCP(m)
+	if _, err := e.Exec(context.Background(), "vikunja_delete_task", nil, false); !errors.Is(err, ErrNeedsConfirm) {
+		t.Fatalf("err = %v, want ErrNeedsConfirm", err)
+	}
+	if m.calls != 0 {
+		t.Fatal("a destructive MCP tool must not reach the server before confirmation")
+	}
+	if _, err := e.Exec(context.Background(), "vikunja_delete_task", nil, true); err != nil {
+		t.Fatalf("confirmed exec: %v", err)
+	}
+	if m.calls != 1 {
+		t.Fatalf("calls = %d", m.calls)
+	}
+}
+
+// A proposed MCP row does not run, exactly like a proposed shell tool.
+func TestExecMCPRowNotEnabled(t *testing.T) {
+	api := fakeAPI{tools: map[string]ipc.Tool{
+		"vikunja_list_tasks": {Name: "vikunja_list_tasks", Status: "proposed", Cmd: []string{"mcp", "vikunja", "list_tasks"}},
+	}}
+	m := &fakeMCP{}
+	e := NewExecutor(api, time.Second).WithMCP(m)
+	if _, err := e.Exec(context.Background(), "vikunja_list_tasks", nil, false); !errors.Is(err, ErrNotEnabled) {
+		t.Fatalf("err = %v", err)
+	}
+	if m.calls != 0 {
+		t.Fatal("a proposal must not call anything")
+	}
+}
+
+// With MCP unconfigured, an MCP row refuses rather than trying to exec "mcp".
+func TestExecMCPRowWithoutCallerRefuses(t *testing.T) {
+	api := fakeAPI{tools: map[string]ipc.Tool{
+		"vikunja_list_tasks": {Name: "vikunja_list_tasks", Status: "enabled", Cmd: []string{"mcp", "vikunja", "list_tasks"}},
+	}}
+	ran := false
+	e := NewExecutor(api, time.Second)
+	e.run = func(context.Context, []string) (string, error) { ran = true; return "", nil }
+	if _, err := e.Exec(context.Background(), "vikunja_list_tasks", nil, false); !errors.Is(err, ErrNotEnabled) {
+		t.Fatalf("err = %v, want ErrNotEnabled", err)
+	}
+	if ran {
+		t.Fatal(`"mcp" must never be run as a binary`)
+	}
+}