Files
Maven/internal/tool/tool_test.go
kami 8d5e357b57 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

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`)
}
}