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.
2026-08-01 14:11:57 +04:00
parent 52f56947bb
commit da62a2f25e
7 changed files with 479 additions and 29 deletions
@@ -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 {
@@ -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")