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