8d5e357b57
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
188 lines
6.5 KiB
Go
188 lines
6.5 KiB
Go
package tool
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"reflect"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/kami/maven/internal/ipc"
|
|
)
|
|
|
|
// fakeAPI — an in-memory tool store for the executor/matcher tests.
|
|
type fakeAPI struct{ tools map[string]ipc.Tool }
|
|
|
|
func (f fakeAPI) LookupTool(_ context.Context, name string) (ipc.Tool, error) {
|
|
t, ok := f.tools[name]
|
|
if !ok {
|
|
return ipc.Tool{}, ipc.ErrToolNotFound
|
|
}
|
|
return t, nil
|
|
}
|
|
func (f fakeAPI) ListTools(_ context.Context, status string) ([]ipc.Tool, error) {
|
|
var out []ipc.Tool
|
|
for _, t := range f.tools {
|
|
if status == "" || t.Status == status {
|
|
out = append(out, t)
|
|
}
|
|
}
|
|
return out, nil
|
|
}
|
|
func (f fakeAPI) ProposeTool(_ context.Context, _, _, _ string, _ time.Time) (bool, error) {
|
|
return true, nil
|
|
}
|
|
|
|
// TestExec covers the allowlist boundary: enabled runs, unknown/proposed refuse,
|
|
// destructive needs confirm, and args land as argv (no shell) after the prefix.
|
|
func TestExec(t *testing.T) {
|
|
api := fakeAPI{tools: map[string]ipc.Tool{
|
|
"restart": {Name: "restart", Scope: "homelab", Cmd: []string{"systemctl", "restart"}, Status: "enabled"},
|
|
"drop": {Name: "drop", Scope: "homelab", Cmd: []string{"dropdb"}, Destructive: true, Status: "enabled"},
|
|
"draft": {Name: "draft", Scope: "homelab", Cmd: []string{"x"}, Status: "proposed"},
|
|
}}
|
|
|
|
var gotArgv []string
|
|
e := NewExecutor(api, 0)
|
|
e.run = func(_ context.Context, argv []string) (string, error) { gotArgv = argv; return "ok", nil }
|
|
|
|
// enabled → runs, args appended to the fixed prefix as argv.
|
|
out, err := e.Exec(context.Background(), "restart", []string{"nginx; rm -rf /"}, false)
|
|
if err != nil || out != "ok" {
|
|
t.Fatalf("enabled: out=%q err=%v", out, err)
|
|
}
|
|
want := []string{"systemctl", "restart", "nginx; rm -rf /"}
|
|
if !reflect.DeepEqual(gotArgv, want) {
|
|
t.Fatalf("argv=%v want %v (injection must stay one argv element)", gotArgv, want)
|
|
}
|
|
|
|
// unknown → refuse.
|
|
if _, err := e.Exec(context.Background(), "nope", nil, false); !errors.Is(err, ErrNotEnabled) {
|
|
t.Fatalf("unknown: err=%v want ErrNotEnabled", err)
|
|
}
|
|
// proposed (not enabled) → refuse.
|
|
if _, err := e.Exec(context.Background(), "draft", nil, false); !errors.Is(err, ErrNotEnabled) {
|
|
t.Fatalf("proposed: err=%v want ErrNotEnabled", err)
|
|
}
|
|
// destructive unconfirmed → needs confirm; confirmed → runs.
|
|
if _, err := e.Exec(context.Background(), "drop", nil, false); !errors.Is(err, ErrNeedsConfirm) {
|
|
t.Fatalf("destructive: err=%v want ErrNeedsConfirm", err)
|
|
}
|
|
if _, err := e.Exec(context.Background(), "drop", []string{"db"}, true); err != nil {
|
|
t.Fatalf("destructive confirmed: err=%v", err)
|
|
}
|
|
if want := []string{"dropdb", "db"}; !reflect.DeepEqual(gotArgv, want) {
|
|
t.Fatalf("confirmed argv=%v want %v", gotArgv, want)
|
|
}
|
|
|
|
// matcher allowlist = enabled names only (proposed excluded).
|
|
m := NewMatcher(api)
|
|
fn, args, ok := m.Match("restart nginx")
|
|
if !ok || fn != "restart" || !reflect.DeepEqual(args, []string{"nginx"}) {
|
|
t.Fatalf("match: fn=%q args=%v ok=%v", fn, args, ok)
|
|
}
|
|
if _, _, ok := m.Match("draft something"); ok {
|
|
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`)
|
|
}
|
|
}
|