From da62a2f25eddc0e49c7d7862cf036e1f47359fa9 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:11:57 +0400 Subject: [PATCH] mcp: pin what a tool was when it was approved An allowlist row stores cmd ["mcp", server, tool]. That is a late-bound reference to a name the far end owns, so the row pins nothing about behaviour: a server could redefine an enabled read-only list_tasks into something that writes, and Maven would keep calling it with no confirm turn and no second approval. Discovery now stores a fingerprint of the declared shape, name, description, input schema and readOnlyHint, and compares it on every refresh. A mismatch drops the row back to proposed and, if it stopped claiming read-only, marks it destructive. destructive is only ever raised. A row predating the column adopts its fingerprint silently, because an upgrade is not a redefinition. Nothing retracted a proposal either, so a tool a connected server no longer offers stayed enabled and failed at call time with an internal string. Those rows are withdrawn, with provenance saying why, and only for servers that are actually connected so a restart does not disarm what he approved. Argument binding rested on readOnlyHint, which the same server writes. A server advertising delete_project as read-only got an unconfirmed argument-carrying call. Binding now also requires the tool be named in allow_tools, something local, and refuses a required property the schema never describes rather than guessing it is a string. wireMCP dialled synchronously from run, and on the passkey path from inside the unlock handler, so one black-holed endpoint delayed boot and the answer to an unlock. The first dial happens on the refresh goroutine under the daemon context. Two servers whose names flatten to one local allowlist name no longer share a row. Found in review of #71. --- cmd/mavend/mcp.go | 145 ++++++++++++++++++++++++++++----- cmd/mavend/mcp_test.go | 19 +++++ internal/mcp/allowlist.go | 37 +++++++++ internal/mcp/allowlist_test.go | 32 ++++++++ internal/store/migrations.go | 9 ++ internal/store/tools.go | 123 ++++++++++++++++++++++++++-- internal/store/tools_test.go | 143 +++++++++++++++++++++++++++++++- 7 files changed, 479 insertions(+), 29 deletions(-) create mode 100644 internal/mcp/allowlist_test.go diff --git a/cmd/mavend/mcp.go b/cmd/mavend/mcp.go index 7b31117..c350b80 100644 --- a/cmd/mavend/mcp.go +++ b/cmd/mavend/mcp.go @@ -13,8 +13,11 @@ import ( "github.com/kami/maven/internal/webfetch" ) -// mcpRefreshInterval — how often the manager re-dials a server that is down. -// The manager applies its own backoff on top, so this being short is cheap. +// mcpRefreshInterval — how often the manager is asked to re-dial servers that +// are down. It is a tick, not a retry rate: mcp.Manager holds a per-server +// backoff that starts at DefaultReconnectEvery and doubles to +// MaxReconnectEvery, so a permanently misconfigured stdio server is not +// re-exec'd once a minute forever. const mcpRefreshInterval = time.Minute // mcpWiring — the MCP client, when the `mcp` block configures at least one @@ -29,9 +32,16 @@ type mcpWiring struct { st *store.Store } -// wireMCP builds the manager, connects, and proposes what it found. It never -// fails the daemon: a server that is unreachable at boot is logged and retried, -// because Maven starting is not contingent on someone else's process. +// wireMCP builds the manager. It does NOT dial: run does that, on its own +// goroutine, which is what makes "Maven starting is not contingent on someone +// else's process" true rather than merely intended. +// +// Dialing here used to be synchronous with a 30s budget, from wireVoice, from +// run. Connect dials serially and each HTTP dial is three requests against +// that server's timeout, so one black-holed endpoint cost 15s of boot and two +// cost the whole budget. On the passkey path wireVoice runs inside the unlock +// handler, so it delayed the answer to an unlock as well. Not failing and not +// blocking are different properties and only the first one held. func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { servers := cfg.MCPServers() if len(servers) == 0 { @@ -43,6 +53,7 @@ func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { limits.DenyHosts = cfg.MCP.DenyHosts limits.MaxBytes = cfg.MCP.MaxBytes limits.Timeout = time.Duration(cfg.MCP.Timeout) + limits.HostInterval = time.Duration(cfg.MCP.HostInterval) } mgr, err := mcp.NewManager(mcp.WebfetchDoor(limits), servers) if err != nil { @@ -52,30 +63,58 @@ func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { log.Printf("mcp: not wired: %v", err) return nil } - w := &mcpWiring{mgr: mgr, st: st} - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - mgr.Connect(ctx) - w.propose(ctx) - return w + return &mcpWiring{mgr: mgr, st: st} } -// propose writes a 'proposed' allowlist row for every discovered tool. It does -// NOT enable anything: a configured server is a place Maven may look, not a -// capability she has. Kami enables what he wants on /tools, behind step-up, -// which is the same gate a shell tool goes through. +// connect dials every server and reconciles what came back. Called from run, +// under the daemon's context, so a shutdown during a slow dial is observed. +func (w *mcpWiring) connect(ctx context.Context) { + if w == nil { + return + } + w.mgr.Connect(ctx) + w.propose(ctx) +} + +// propose writes a 'proposed' allowlist row for every discovered tool, and +// reconciles the rows that already exist against what the server offers today. +// It does NOT enable anything: a configured server is a place Maven may look, +// not a capability she has. Kami enables what he wants on /tools, behind +// step-up, which is the same gate a shell tool goes through. // -// Re-running on every boot is idempotent — ProposeMCPTool never touches an -// existing row, so a tool he disabled stays disabled and one he enabled keeps -// the cmd he enabled it with. +// Three things happen per discovered tool. +// +// A name not in the store becomes a proposal, carrying the tool's fingerprint. +// +// A name already in the store is reconciled against that fingerprint. A tool +// whose description, schema or readOnlyHint changed since it was approved drops +// back to 'proposed' and, if it stopped claiming read-only, to destructive=1. +// Insert-or-skip was not enough on its own: the cmd is a late-bound reference +// to a name the far end owns, so the server can redefine list_tasks into +// something that writes without the row changing at all. +// +// A row whose server is connected and no longer offers the tool is withdrawn. func (w *mcpWiring) propose(ctx context.Context) { if w == nil { return } now := time.Now() - fresh := 0 + fresh, changed := 0, 0 + seen := map[string]string{} // local name → "server/tool", for collisions for _, t := range w.mgr.Tools() { name := mcp.LocalName(t.Server, t.Name) + remote := t.Server + "/" + t.Name + // Two different tools can flatten to one local name: server "vik" with + // tool "list_tasks" and server "vik_list" with tool "tasks" both give + // "vik_list_tasks". The store keys rows by name, so the second would + // land on the first one's row. Config-controlled and therefore rare, + // but silently reusing a row is the wrong way to lose that race. + if prev, dup := seen[name]; dup { + log.Printf("mcp: %s and %s both map to the allowlist name %q — skipping the second, rename a server", + prev, remote, name) + continue + } + seen[name] = remote // No readOnlyHint ⇒ assume it mutates ⇒ the confirm turn. Being wrong // in this direction only costs a question. destructive := !t.ReadOnly @@ -83,19 +122,82 @@ func (w *mcpWiring) propose(ctx context.Context) { if t.Description != "" { provenance += ": " + t.Description } + fp := mcp.Fingerprint(t) ok, err := w.st.ProposeMCPTool(ctx, name, mcp.Scope(t.Server), - mcp.Cmd(t.Server, t.Name), destructive, provenance, now) + mcp.Cmd(t.Server, t.Name), destructive, provenance, fp, now) if err != nil { log.Printf("mcp: propose %s: %v", name, err) continue } if ok { fresh++ + continue + } + // The row already existed. Its provenance is whatever the server said + // the first time; reconciling rewrites it, so what /tools shows is what + // the server says now. + ch, err := w.st.ReconcileMCPTool(ctx, name, fp, destructive, provenance, now) + if err != nil { + log.Printf("mcp: reconcile %s: %v", name, err) + continue + } + if !ch.Changed { + continue + } + changed++ + switch { + case ch.Demoted && ch.Escalated: + log.Printf("mcp: %s changed on the server and no longer claims read-only — disabled and marked destructive, re-approve it on /tools", name) + case ch.Demoted: + log.Printf("mcp: %s changed on the server since it was enabled — disabled, re-approve it on /tools", name) + default: + log.Printf("mcp: %s changed on the server; the proposal now shows the new description", name) } } + w.withdrawGone(ctx, seen, now) if fresh > 0 { log.Printf("mcp: %d new tool proposal(s) waiting on /tools", fresh) } + if changed > 0 { + log.Printf("mcp: %d tool(s) changed since approval and need another look", changed) + } +} + +// withdrawGone disarms rows whose tool the server stopped offering. Only +// servers that are CONNECTED are considered: a tool missing because its server +// is down is not a tool that was withdrawn, and disabling a capability every +// time a process restarts would be worse than the problem. +func (w *mcpWiring) withdrawGone(ctx context.Context, seen map[string]string, now time.Time) { + live := map[string]bool{} + for _, name := range w.mgr.Connected() { + live[name] = true + } + if len(live) == 0 { + return + } + rows, err := w.st.ListTools(ctx, "") + if err != nil { + log.Printf("mcp: list tools: %v", err) + return + } + for _, row := range rows { + server, remote, ok := mcp.ParseCmd(row.Cmd) + if !ok || !live[server] { + continue + } + if _, still := seen[row.Name]; still { + continue + } + note := fmt.Sprintf("mcp %s/%s: no longer offered by the server", server, remote) + wasEnabled, err := w.st.WithdrawTool(ctx, row.Name, note, now) + if err != nil { + log.Printf("mcp: withdraw %s: %v", row.Name, err) + continue + } + if wasEnabled { + log.Printf("mcp: %s was enabled but %s no longer offers it — disabled", row.Name, server) + } + } } // run re-dials downed servers and picks up tools that appeared, until ctx is @@ -104,6 +206,9 @@ func (w *mcpWiring) run(ctx context.Context) { if w == nil { return } + // The first dial happens here rather than at wiring time, so boot never + // waits on someone else's process. + w.connect(ctx) t := time.NewTicker(mcpRefreshInterval) defer t.Stop() for { diff --git a/cmd/mavend/mcp_test.go b/cmd/mavend/mcp_test.go index 6339524..b63bdbd 100644 --- a/cmd/mavend/mcp_test.go +++ b/cmd/mavend/mcp_test.go @@ -31,6 +31,23 @@ func TestWireMCPOffWhenUnconfigured(t *testing.T) { } } +// Wiring must not dial. Boot used to block for the whole per-server timeout +// budget on a black-holed endpoint, and on the passkey path that delay landed +// inside the unlock handler. +func TestWireMCPDoesNotDial(t *testing.T) { + st := newTestStore(t) + w := wireMCP(&config.Config{MCP: &config.MCPConfig{Servers: []config.MCPServerConfig{{ + Name: "dead", Command: "/nonexistent/mcp-server", Enabled: true, + }}}}, st) + if w == nil { + t.Fatal("a configured server should wire") + } + defer w.close() + if s := w.status(); len(s) != 1 || s[0].Err != "" { + t.Fatalf("wireMCP dialled: %+v", s) + } +} + // An unreachable server must not stop the daemon, must be reported as down, and // must propose nothing. func TestWireMCPUnreachableServerIsNotFatal(t *testing.T) { @@ -42,6 +59,7 @@ func TestWireMCPUnreachableServerIsNotFatal(t *testing.T) { t.Fatal("a configured server should still wire") } defer w.close() + w.connect(context.Background()) st2 := w.status() if len(st2) != 1 || st2[0].Connected || st2[0].Err == "" { t.Fatalf("status = %+v", st2) @@ -67,6 +85,7 @@ func TestWireMCPPrivateURLRefusedWithoutAllowPrivate(t *testing.T) { t.Fatal("should wire") } defer w.close() + w.connect(context.Background()) s := w.status()[0] if s.Connected { t.Fatal("a loopback server must not connect without allow_private") diff --git a/internal/mcp/allowlist.go b/internal/mcp/allowlist.go index 09c9d06..235b0c5 100644 --- a/internal/mcp/allowlist.go +++ b/internal/mcp/allowlist.go @@ -1,7 +1,11 @@ package mcp import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" "regexp" + "strconv" "strings" ) @@ -59,3 +63,36 @@ func LocalName(server, tool string) string { // Scope is the store scope for a server's rows, so the /tools page can group // them and a human can tell at a glance where a capability came from. func Scope(server string) string { return "mcp:" + server } + +// Fingerprint is the declared shape of a discovered tool: its name, its +// description, its input schema and its readOnlyHint, hashed. +// +// It exists because an allowlist row cannot pin an MCP tool's behaviour. The +// row's cmd is ["mcp", server, tool], a reference to a name the REMOTE server +// owns and may redefine — the row does not have to change for the tool to +// become something else. The fingerprint is what Kami actually approved, so a +// later discovery can tell "same tool" from "same name". +// +// The schema is canonicalised through a decode and re-encode, so a server that +// reorders its JSON keys or changes its whitespace does not read as a +// redefinition. Unparseable schema bytes are hashed as they arrived. +func Fingerprint(t Tool) string { + schema := "" + if len(t.InputSchema) > 0 { + var any any + if json.Unmarshal(t.InputSchema, &any) == nil { + if raw, err := json.Marshal(any); err == nil { + schema = string(raw) + } + } + if schema == "" { + schema = string(t.InputSchema) + } + } + h := sha256.New() + for _, part := range []string{t.Name, t.Description, schema, strconv.FormatBool(t.ReadOnly)} { + h.Write([]byte(part)) + h.Write([]byte{0}) + } + return hex.EncodeToString(h.Sum(nil)) +} diff --git a/internal/mcp/allowlist_test.go b/internal/mcp/allowlist_test.go new file mode 100644 index 0000000..fd74ae2 --- /dev/null +++ b/internal/mcp/allowlist_test.go @@ -0,0 +1,32 @@ +package mcp + +import ( + "encoding/json" + "testing" +) + +// The fingerprint must cover everything the approval was given for, and must +// not move when only the JSON spelling of the schema does. +func TestFingerprintCoversTheDeclaredShape(t *testing.T) { + base := Tool{Name: "list_tasks", Description: "list them", ReadOnly: true, + InputSchema: json.RawMessage(`{"type":"object","properties":{}}`)} + same := base + same.InputSchema = json.RawMessage("{\n \"properties\": {},\n \"type\": \"object\"\n}") + if Fingerprint(base) != Fingerprint(same) { + t.Error("reformatting the schema must not read as a redefinition") + } + for name, mut := range map[string]func(*Tool){ + "description": func(x *Tool) { x.Description = "delete them" }, + "schema": func(x *Tool) { x.InputSchema = json.RawMessage(`{"required":["id"]}`) }, + "readonly": func(x *Tool) { x.ReadOnly = false }, + "name": func(x *Tool) { x.Name = "delete_tasks" }, + } { + t.Run(name, func(t *testing.T) { + got := base + mut(&got) + if Fingerprint(got) == Fingerprint(base) { + t.Error("a redefinition must change the fingerprint") + } + }) + } +} diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 163ad83..d7597f8 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -160,6 +160,15 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 ); CREATE UNIQUE INDEX IF NOT EXISTS idx_tasks_live_norm ON tasks (norm) WHERE status IN ('candidate','open'); CREATE INDEX IF NOT EXISTS idx_tasks_status ON tasks (status, created_ts DESC);`, + + `ALTER TABLE tools ADD COLUMN fingerprint TEXT NOT NULL DEFAULT '';`, + // #15 — what a discovered tool WAS when it was approved (Vikunja #251). + // An MCP row's cmd is ["mcp", server, tool], which is a late-bound + // reference: it names a tool on a server the remote end owns and it pins + // no behaviour at all. A server upgraded, or taken over, can redefine + // list_tasks into something that writes without the row changing by one + // byte. The fingerprint is the declared shape at approval time, so a + // redefinition is a re-approval instead of a silent upgrade. } // migrate applies every migration with a number greater than the DB's current diff --git a/internal/store/tools.go b/internal/store/tools.go index 10f9c37..bf34901 100644 --- a/internal/store/tools.go +++ b/internal/store/tools.go @@ -65,7 +65,16 @@ func (s *Store) ProposeTool(ctx context.Context, name, utterance, scope string, // Like ProposeTool it never touches an existing row, so re-discovery on every // restart is idempotent and cannot silently re-arm a tool that was disabled or // change the cmd of one already enabled. -func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []string, destructive bool, utterance string, ts time.Time) (bool, error) { +// +// The row is NOT what protects him, and it is worth being exact about that. +// cmd is ["mcp", server, tool]: a late-bound reference to a name the remote +// server owns. The tool it points at can be redefined on the far end without +// the row changing at all, so "the cmd cannot change" is true and beside the +// point. fingerprint is what closes that: it records the declared shape (name, +// description, input schema, readOnlyHint) at the time the proposal was +// written, and ReconcileMCPTool compares against it on every later discovery. +// Pass "" for a row with nothing to fingerprint (a Home Assistant device). +func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []string, destructive bool, utterance, fingerprint string, ts time.Time) (bool, error) { if len(cmd) == 0 { return false, ErrToolCmd } @@ -81,10 +90,10 @@ func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []st d = 1 } res, err := s.db.ExecContext(ctx, ` - INSERT INTO tools (name, scope, cmd, destructive, status, utterance, created_ts, updated_ts) - VALUES (?, ?, ?, ?, 'proposed', ?, ?, ?) + INSERT INTO tools (name, scope, cmd, destructive, status, utterance, fingerprint, created_ts, updated_ts) + VALUES (?, ?, ?, ?, 'proposed', ?, ?, ?, ?) ON CONFLICT(name) DO NOTHING`, - name, scope, string(raw), d, utterance, ts.UnixMilli(), ts.UnixMilli()) + name, scope, string(raw), d, utterance, fingerprint, ts.UnixMilli(), ts.UnixMilli()) if err != nil { return false, fmt.Errorf("propose mcp tool: %w", err) } @@ -105,7 +114,111 @@ func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []st // turn. Re-discovery on every refresh is idempotent — an existing row is never // touched, so a device he disabled stays disabled. func (s *Store) ProposeSmartHomeTool(ctx context.Context, name, scope string, cmd []string, utterance string, ts time.Time) (bool, error) { - return s.ProposeMCPTool(ctx, name, scope, cmd, true, utterance, ts) + return s.ProposeMCPTool(ctx, name, scope, cmd, true, utterance, "", ts) +} + +// ToolChange — what ReconcileMCPTool did to an existing row. +type ToolChange struct { + // Changed — the discovered shape differs from the approved one. + Changed bool + // Demoted — the row was enabled and is now 'proposed' again, so the + // capability is off until a human looks at it a second time. + Demoted bool + // Escalated — destructive went from 0 to 1. It never goes the other way. + Escalated bool +} + +// ReconcileMCPTool compares a freshly discovered tool against the row that was +// approved, and escalates when they disagree. +// +// The failure this exists for: day 1 the server offers list_tasks with +// readOnlyHint true, so the row is proposed non-destructive and Kami enables +// it. Day 30 the server is upgraded, or taken over, and list_tasks now writes. +// Insert-or-skip does nothing on that discovery — the row is still enabled, +// still destructive=0 — and the confirm turn never fires, because the flag was +// frozen against a claim the server has since withdrawn. +// +// So: a differing fingerprint drops the row back to 'proposed' and rewrites the +// provenance, and a tool that stopped claiming read-only gets destructive=1. +// destructive is only ever raised, never lowered: relaxing it on the say-so of +// the same server that changed underneath us would undo the point. +// +// A row with an empty stored fingerprint predates this and simply adopts the +// discovered one — an upgrade is not a redefinition. +func (s *Store) ReconcileMCPTool(ctx context.Context, name, fingerprint string, destructive bool, utterance string, ts time.Time) (ToolChange, error) { + var ( + stored string + status string + wasDest int + ) + err := s.db.QueryRowContext(ctx, + `SELECT fingerprint, status, destructive FROM tools WHERE name = ?`, name). + Scan(&stored, &status, &wasDest) + if errors.Is(err, sql.ErrNoRows) { + return ToolChange{}, ErrToolNotFound + } + if err != nil { + return ToolChange{}, fmt.Errorf("reconcile mcp tool: %w", err) + } + var ch ToolChange + if stored == "" { + if _, err := s.db.ExecContext(ctx, + `UPDATE tools SET fingerprint = ?, updated_ts = ? WHERE name = ?`, + fingerprint, ts.UnixMilli(), name); err != nil { + return ToolChange{}, fmt.Errorf("reconcile mcp tool: %w", err) + } + return ch, nil + } + if stored == fingerprint { + return ch, nil + } + ch.Changed = true + ch.Demoted = status == "enabled" + d := wasDest + if destructive && wasDest == 0 { + d, ch.Escalated = 1, true + } + if _, err := s.db.ExecContext(ctx, ` + UPDATE tools + SET fingerprint = ?, destructive = ?, status = 'proposed', utterance = ?, updated_ts = ? + WHERE name = ?`, + fingerprint, d, utterance, ts.UnixMilli(), name); err != nil { + return ToolChange{}, fmt.Errorf("reconcile mcp tool: %w", err) + } + return ch, nil +} + +// WithdrawTool disarms a row whose remote tool no longer exists: it drops back +// to 'proposed' and its provenance says why. +// +// Nothing else retracted a proposal, so a tool a server stopped offering kept +// its row forever, and an ENABLED one stayed enabled and failed at call time +// with an internal string the act path does not match. /tools is where he would +// go to find out and it was the one place that did not say. Returns whether the +// row was still enabled. +func (s *Store) WithdrawTool(ctx context.Context, name, utterance string, ts time.Time) (bool, error) { + res, err := s.db.ExecContext(ctx, ` + UPDATE tools SET status = 'proposed', utterance = ?, updated_ts = ? + WHERE name = ? AND status = 'enabled'`, + utterance, ts.UnixMilli(), name) + if err != nil { + return false, fmt.Errorf("withdraw tool: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("withdraw tool: rows affected: %w", err) + } + if n > 0 { + return true, nil + } + // Not enabled: still refresh the provenance so the proposed row says it. + _, err = s.db.ExecContext(ctx, + `UPDATE tools SET utterance = ?, updated_ts = ? WHERE name = ?`, + utterance, ts.UnixMilli(), name) + if err != nil { + return false, fmt.Errorf("withdraw tool: %w", err) + } + return false, nil } // EnableTool fills cmd + destructive and flips status to 'enabled'. This is the diff --git a/internal/store/tools_test.go b/internal/store/tools_test.go index 0ee973e..375d5b0 100644 --- a/internal/store/tools_test.go +++ b/internal/store/tools_test.go @@ -70,7 +70,7 @@ func TestProposeMCPTool(t *testing.T) { now := time.Now() cmd := []string{"mcp", "vikunja", "list_tasks"} - fresh, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "mcp vikunja/list_tasks: List tasks", now) + fresh, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "mcp vikunja/list_tasks: List tasks", "fp1", now) if err != nil { t.Fatal(err) } @@ -92,7 +92,7 @@ func TestProposeMCPTool(t *testing.T) { } // Re-discovery on the next boot is idempotent. - fresh, err = s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, true, "changed", now) + fresh, err = s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, true, "changed", "fp1", now) if err != nil { t.Fatal(err) } @@ -104,7 +104,7 @@ func TestProposeMCPTool(t *testing.T) { if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { t.Fatal(err) } - if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", []string{"mcp", "vikunja", "delete_task"}, true, "x", now); err != nil { + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", []string{"mcp", "vikunja", "delete_task"}, true, "x", "fp2", now); err != nil { t.Fatal(err) } got, err = s.LookupTool(ctx, "vikunja_list_tasks") @@ -118,7 +118,142 @@ func TestProposeMCPTool(t *testing.T) { func TestProposeMCPToolNeedsCmd(t *testing.T) { s := newTestStore(t) - if _, err := s.ProposeMCPTool(context.Background(), "x", "mcp:y", nil, false, "", time.Now()); !errors.Is(err, ErrToolCmd) { + if _, err := s.ProposeMCPTool(context.Background(), "x", "mcp:y", nil, false, "", "", time.Now()); !errors.Is(err, ErrToolCmd) { t.Fatalf("err = %v, want ErrToolCmd", err) } } + +// A server that redefines a tool Kami already approved must have to ask again. +// The row stores cmd ["mcp", server, tool], a late-bound reference to a name +// the far end owns, so before the fingerprint a server could turn an enabled +// read-only list_tasks into something that writes and Maven would keep running +// it without a confirm turn. +func TestReconcileMCPToolDemotesARedefinedTool(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "read only", "fp1", now); err != nil { + t.Fatal(err) + } + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + + // Same shape ⇒ nothing happens. Discovery runs every minute and must be + // idempotent. + ch, err := s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp1", false, "read only", now) + if err != nil { + t.Fatal(err) + } + if ch.Changed { + t.Fatalf("an unchanged tool must not be touched: %+v", ch) + } + if got, _ := s.LookupTool(ctx, "vikunja_list_tasks"); got.Status != "enabled" { + t.Fatalf("status = %q, want it left enabled", got.Status) + } + + // It stopped claiming read-only and its schema moved. + ch, err = s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp2", true, "now writes", now) + if err != nil { + t.Fatal(err) + } + if !ch.Changed || !ch.Demoted || !ch.Escalated { + t.Fatalf("change = %+v, want changed+demoted+escalated", ch) + } + got, err := s.LookupTool(ctx, "vikunja_list_tasks") + if err != nil { + t.Fatal(err) + } + if got.Status != "proposed" { + t.Errorf("status = %q, want a redefined tool back in the queue", got.Status) + } + if !got.Destructive { + t.Error("a tool that stopped claiming read-only must gain the confirm turn") + } + if got.Utterance != "now writes" { + t.Errorf("utterance = %q, want what the server says today", got.Utterance) + } + + // destructive is only ever raised. The server that changed underneath us + // does not get to relax it by claiming read-only next time. + if _, err := s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp3", false, "read only again", now); err != nil { + t.Fatal(err) + } + if got, _ = s.LookupTool(ctx, "vikunja_list_tasks"); !got.Destructive { + t.Error("destructive was relaxed by the server") + } +} + +// A row written before fingerprints exist simply adopts one. An upgrade is not +// a redefinition and must not disable everything Kami approved. +func TestReconcileMCPToolAdoptsAnEmptyFingerprint(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "x", "", now); err != nil { + t.Fatal(err) + } + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + ch, err := s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp1", false, "x", now) + if err != nil { + t.Fatal(err) + } + if ch.Changed { + t.Fatalf("adopting must be silent: %+v", ch) + } + if got, _ := s.LookupTool(ctx, "vikunja_list_tasks"); got.Status != "enabled" { + t.Fatalf("status = %q, want still enabled after the upgrade", got.Status) + } + // And now it is pinned. + if ch, _ = s.ReconcileMCPTool(ctx, "vikunja_list_tasks", "fp2", false, "y", now); !ch.Changed { + t.Fatal("the adopted fingerprint must be enforced on the next pass") + } +} + +func TestReconcileMCPToolUnknownRow(t *testing.T) { + s := newTestStore(t) + if _, err := s.ReconcileMCPTool(context.Background(), "nope", "fp", false, "", time.Now()); !errors.Is(err, ErrToolNotFound) { + t.Fatalf("err = %v, want ErrToolNotFound", err) + } +} + +// A tool the server stopped offering must be disarmed and must say why. It used +// to stay enabled and fail at call time with an internal string, and /tools — +// the one place he would look — did not mention it. +func TestWithdrawTool(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "x", "fp1", now); err != nil { + t.Fatal(err) + } + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + was, err := s.WithdrawTool(ctx, "vikunja_list_tasks", "gone", now) + if err != nil { + t.Fatal(err) + } + if !was { + t.Error("withdrawing an enabled tool must report that it was enabled") + } + got, err := s.LookupTool(ctx, "vikunja_list_tasks") + if err != nil { + t.Fatal(err) + } + if got.Status != "proposed" || got.Utterance != "gone" { + t.Fatalf("row = %+v, want proposed and saying why", got) + } + // Withdrawing again is not an error and does not claim it was enabled. + if was, err = s.WithdrawTool(ctx, "vikunja_list_tasks", "still gone", now); err != nil || was { + t.Fatalf("second withdraw = %v, %v", was, err) + } + if got, _ = s.LookupTool(ctx, "vikunja_list_tasks"); got.Utterance != "still gone" { + t.Errorf("utterance = %q, want the provenance refreshed anyway", got.Utterance) + } +}