From 470fcfea8b0fe041b687abc0511f06ae04c4baf9 Mon Sep 17 00:00:00 2001 From: yhh Date: Mon, 12 Oct 2026 07:42:27 -0700 Subject: [PATCH] feat(studio): approve MCP tools whose definition changed A server whose tool definitions differ from what the user approved has those tools withheld from the model, and the kernel already had an approval route for them, but the settings row never said so and offered no way to approve. The row now carries the withheld tools with how each differs, and a button that sends back the digest of exactly that set, then reconnects. --- desktop/frontend-next/src/actions.ts | 1 + desktop/frontend-next/src/i18n/en.ts | 7 ++ desktop/frontend-next/src/port/mcp.ts | 14 ++++ desktop/frontend-next/src/port/mock_mcp.ts | 10 +++ desktop/frontend-next/src/port/port.ts | 3 + desktop/frontend-next/src/port/sse.ts | 11 +++ desktop/frontend-next/src/port/sse_mcp.ts | 11 +++ desktop/frontend-next/src/styles/mcpheld.css | 6 ++ desktop/frontend-next/src/ui/ServerRow.tsx | 39 ++++++++++- .../src/ui/mcp-held.interaction.test.tsx | 67 +++++++++++++++++++ internal/frontend/serve/catalog.go | 30 +++++++++ internal/frontend/serve/mcp_held_test.go | 48 +++++++++++++ tools/repolint/wireparity.go | 1 + 13 files changed, 246 insertions(+), 2 deletions(-) create mode 100644 desktop/frontend-next/src/styles/mcpheld.css create mode 100644 desktop/frontend-next/src/ui/mcp-held.interaction.test.tsx create mode 100644 internal/frontend/serve/mcp_held_test.go diff --git a/desktop/frontend-next/src/actions.ts b/desktop/frontend-next/src/actions.ts index 2ec7c09c91..76aeefe9a1 100644 --- a/desktop/frontend-next/src/actions.ts +++ b/desktop/frontend-next/src/actions.ts @@ -166,6 +166,7 @@ export const ACTIONS: UIAction[] = [ { id: "skill.enabled", kind: "kernel-mutation", target: "entity", proof: "authority-effect" }, { id: "mcp.enabled", kind: "kernel-mutation", target: "entity", proof: "interaction" }, { id: "mcp.retry", kind: "repeatable", target: "entity", proof: "interaction" }, + { id: "mcp.trust", kind: "kernel-mutation", target: "entity", proof: "interaction" }, { id: "mcp.remove", kind: "destructive", target: "entity", proof: "interaction" }, { id: "mcp.load", kind: "kernel-mutation", target: "entity", proof: "interaction" }, diff --git a/desktop/frontend-next/src/i18n/en.ts b/desktop/frontend-next/src/i18n/en.ts index 7b4578d7e2..95a7d61216 100644 --- a/desktop/frontend-next/src/i18n/en.ts +++ b/desktop/frontend-next/src/i18n/en.ts @@ -549,6 +549,13 @@ export const EN: Record = { "忘记": "Forget", "重连": "Reconnect", "立即连接": "Connect now", + "定义有改动": "Definition changed", + "无法核对": "Cannot be checked", + "等待你批准的工具": "Tools waiting for your approval", + "这个服务有 {n} 个工具的定义与你批准过的不一致，已暂时不交给模型：": "{n} of this service's tools differ from what you approved and are withheld from the model for now:", + "还有 {n} 个没有列出，批准时会一并生效。": "{n} more are not listed; approving covers them too.", + "批准并重新连接": "Approve and reconnect", + "批准中…": "Approving…", "记的是 {had}，但它答的是 {got}。": "Recorded as {had}, but it answers as {got}.", // 收工回执 "没有未经验证的部分": "nothing left unverified", diff --git a/desktop/frontend-next/src/port/mcp.ts b/desktop/frontend-next/src/port/mcp.ts index 5d53a3eb61..ce6f8a1606 100644 --- a/desktop/frontend-next/src/port/mcp.ts +++ b/desktop/frontend-next/src/port/mcp.ts @@ -28,6 +28,20 @@ export interface McpEntry { launch?: string; // Why a pending server waits for the user, as the kernel's code. pendingReason?: string; + // Tools this connection withheld from the model because their definition is + // not the one the user approved. heldDigest names exactly that set, and + // approving it hands the digest back so what is approved is what was shown. + // heldMore counts withheld tools past the list's cap. + held?: McpHeld[]; + heldDigest?: string; + heldMore?: number; +} + +// One withheld tool and how it differs from what was approved: added, changed, +// or unverifiable when the approval record could not be read. +export interface McpHeld { + name: string; + class: string; } // always puts a server's tools in every request; deferred reaches them through diff --git a/desktop/frontend-next/src/port/mock_mcp.ts b/desktop/frontend-next/src/port/mock_mcp.ts index 04277cdc0e..207148efb9 100644 --- a/desktop/frontend-next/src/port/mock_mcp.ts +++ b/desktop/frontend-next/src/port/mock_mcp.ts @@ -107,6 +107,16 @@ export class MockMcp extends MockProvider { ]; } + async trustMcp(name: string, digest: string) { + const s = this.servers.find((x) => x.name === name); + if (!s) return { state: "failed", error: "no configured MCP server named " + name }; + if (!s.held?.length || s.heldDigest !== digest) return { state: "failed", error: "the held MCP tool definitions changed since they were shown" }; + s.held = undefined; + s.heldDigest = undefined; + s.tools = s.tools + 1; + return { state: "ready", tools: s.tools }; + } + async reconnectMcp(name: string) { const s = this.servers.find((x) => x.name === name); if (!s) return { state: "failed", error: "no configured MCP server named " + name }; diff --git a/desktop/frontend-next/src/port/port.ts b/desktop/frontend-next/src/port/port.ts index e75520881c..980afbac27 100644 --- a/desktop/frontend-next/src/port/port.ts +++ b/desktop/frontend-next/src/port/port.ts @@ -300,6 +300,9 @@ export interface AgentPort { // Retries a failed or disconnected server and answers with its new state, so // the caller never has to race a follow-up GET against the connect. reconnectMcp(name: string): Promise<{ state: string; tools?: number; error?: string }>; + // Approves the withheld definitions the digest names, then reconnects. The + // kernel refuses a digest that no longer matches what it withholds. + trustMcp(name: string, digest: string): Promise<{ state: string; tools?: number; error?: string }>; setMcpEnabled(name: string, enabled: boolean, scope?: ScopeLayer, root?: string): Promise; clearMcpOverride(name: string, root?: string): Promise; setMcpLoad(name: string, load: McpLoad): Promise; diff --git a/desktop/frontend-next/src/port/sse.ts b/desktop/frontend-next/src/port/sse.ts index 3b956594b9..ac9bd22ea2 100644 --- a/desktop/frontend-next/src/port/sse.ts +++ b/desktop/frontend-next/src/port/sse.ts @@ -138,6 +138,17 @@ export class SsePort extends SseWorkspace implements AgentPort { return this.post("/memory/restore", { name, revision }); } + async trustMcp(name: string, digest: string) { + const res = await fetch(this.base + "/mcp/trust", { + method: "POST", + headers: { "content-type": "application/json" }, + credentials: "same-origin", + body: JSON.stringify({ name, digest }), + }); + const body = (await res.json().catch(() => ({}))) as { state?: string; tools?: number | unknown[]; error?: string }; + if (!res.ok && !body.error) throw new Error(`/mcp/trust: ${res.status}`); + return { state: body.state ?? (res.ok ? "ready" : "failed"), tools: typeof body.tools === "number" ? body.tools : body.tools?.length, error: body.error }; + } async reconnectMcp(name: string) { const res = await fetch(this.base + "/mcp/reconnect", { method: "POST", diff --git a/desktop/frontend-next/src/port/sse_mcp.ts b/desktop/frontend-next/src/port/sse_mcp.ts index 4b444d77f7..29a3e762f1 100644 --- a/desktop/frontend-next/src/port/sse_mcp.ts +++ b/desktop/frontend-next/src/port/sse_mcp.ts @@ -21,6 +21,17 @@ export class SseMcp extends SseProvider { // whole point of the button, so it is read rather than thrown away. // 502 carries the diagnosis in the body — the reason the retry failed is the // whole point of the button, so it is read rather than thrown away. + async trustMcp(name: string, digest: string) { + const res = await fetch(this.base + "/mcp/trust", { + method: "POST", + headers: { "content-type": "application/json" }, + credentials: "same-origin", + body: JSON.stringify({ name, digest }), + }); + const body = (await res.json().catch(() => ({}))) as { state?: string; tools?: number | unknown[]; error?: string }; + if (!res.ok && !body.error) throw new Error(`/mcp/trust: ${res.status}`); + return { state: body.state ?? (res.ok ? "ready" : "failed"), tools: typeof body.tools === "number" ? body.tools : body.tools?.length, error: body.error }; + } async reconnectMcp(name: string) { const res = await fetch(this.base + "/mcp/reconnect", { method: "POST", diff --git a/desktop/frontend-next/src/styles/mcpheld.css b/desktop/frontend-next/src/styles/mcpheld.css new file mode 100644 index 0000000000..53cfc51dd3 --- /dev/null +++ b/desktop/frontend-next/src/styles/mcpheld.css @@ -0,0 +1,6 @@ +.srv-held { margin: 6px 0 0 15px; padding: 8px 10px; border: 1px solid var(--border); border-radius: var(--r-md); background: var(--overlay); font: var(--w-reg) var(--fs-ui)/1.55 var(--ui); color: var(--text); display: grid; gap: 6px; justify-items: start } +.srv-held ul { margin: 0; padding: 0; list-style: none; display: grid; gap: 2px; max-width: 100% } +.srv-held li { display: flex; gap: 8px; align-items: baseline; min-width: 0 } +.srv-held li b { font-weight: var(--w-med); overflow-wrap: anywhere } +.srv-held li i { font-style: normal; color: var(--muted); white-space: nowrap } +.srv-held .hn { color: var(--muted) } diff --git a/desktop/frontend-next/src/ui/ServerRow.tsx b/desktop/frontend-next/src/ui/ServerRow.tsx index 2a2450f069..93d199912c 100644 --- a/desktop/frontend-next/src/ui/ServerRow.tsx +++ b/desktop/frontend-next/src/ui/ServerRow.tsx @@ -5,6 +5,7 @@ import { Exception } from "./CapabilityScope"; import { Clip } from "./Clip"; import { Switch } from "./Switch"; import { reason } from "../i18n/kernel"; +import "../styles/mcpheld.css"; // One external service: whether it is answering, what it brought, and the two // acts that change that — switching it off and letting it try again. @@ -28,6 +29,14 @@ const PENDING_WHY: Record = { changed_since_enabled: "启用之后它要启动的内容变了，或是由旧版本启用的。确认下面的命令后重新启用。", }; +// How a withheld tool differs from what the user approved, keyed by the kernel's +// class. A class with no entry is shown as the kernel wrote it. +const HELD_CLASS: Record = { + added: "新增", + changed: "定义有改动", + unverifiable: "无法核对", +}; + // A tag only when the schema carries the server's tools or config asks it to: // deferred is the default, and a badge on every row is a badge on none. function loadTag(m: McpEntry): { text: string; pending: boolean } | null { @@ -131,6 +140,30 @@ export function ServerRow({ ); + const heldTools = m.held ?? []; + // The approval is for the exact set the kernel withheld; the digest it sent is + // handed back, and a set that changed meanwhile is refused rather than waved through. + const held = heldTools.length > 0 && !!m.heldDigest && ( +
+ + {t("这个服务有 {n} 个工具的定义与你批准过的不一致，已暂时不交给模型：", { n: heldTools.length + (m.heldMore ?? 0) })} + +
    + {heldTools.map((h) => ( +
  • + {h.name} + {t(HELD_CLASS[h.class] ?? h.class)} +
    • + ))} +
+ {!!m.heldMore && {t("还有 {n} 个没有列出，批准时会一并生效。", { n: m.heldMore })}} + {live && ( + + )} +
+ ); const tools = m.toolList ?? []; const tag = loadTag(m); // The tool list is what gets loaded, so the choice sits above it; a server @@ -194,6 +227,7 @@ export function ServerRow({
{head}
{about} {launch} + {held} {why &&
{why}
} {confirm} @@ -201,11 +235,12 @@ export function ServerRow({ } return ( // Asking to remove has to open the row: the confirmation lives inside the - // fold, and what the server contributes is worth seeing before dropping it. -
+ // fold. A row with withheld tools opens too, so the approval is not behind a click. +
0 || undefined}> {head} {about} {launch} + {held} {why &&
{why}
} {confirm} {load} diff --git a/desktop/frontend-next/src/ui/mcp-held.interaction.test.tsx b/desktop/frontend-next/src/ui/mcp-held.interaction.test.tsx new file mode 100644 index 0000000000..9ab0e5ea60 --- /dev/null +++ b/desktop/frontend-next/src/ui/mcp-held.interaction.test.tsx @@ -0,0 +1,67 @@ +// @vitest-environment jsdom +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import "./testkit"; +import { ServerRow } from "./ServerRow"; +import type { AgentPort, McpEntry } from "../port/port"; + +afterEach(cleanup); + +const withheld = (over: Partial = {}): McpEntry => ({ + name: "notes", state: "ready", enabled: true, transport: "stdio", tools: 1, + held: [{ name: "mcp__notes__search", class: "changed" }, { name: "mcp__notes__add", class: "added" }], + heldDigest: "digest-1", ...over, +}); + +function draw(m: McpEntry, live = true) { + const trustMcp = vi.fn(async () => ({ state: "ready", tools: 2 })); + const onDone = vi.fn(); + render(); + return { trustMcp, onDone }; +} + +describe("tools a server changed since the user approved them", () => { + it("lists each withheld tool with how it differs", () => { + draw(withheld()); + const group = screen.getByRole("group", { name: "等待你批准的工具" }); + expect(group.textContent).toContain("mcp__notes__search"); + expect(group.textContent).toContain("定义有改动"); + expect(group.textContent).toContain("mcp__notes__add"); + expect(group.textContent).toContain("2 个工具"); + }); + + it("opens a row that has tools, so the approval is not hidden behind a fold", () => { + draw(withheld({ toolList: [{ name: "add", description: "Add a note" }] })); + expect(document.querySelector("details.mcp-server")?.hasAttribute("open")).toBe(true); + }); + + it("approves exactly the digest it was shown, then refreshes the list", async () => { + const { trustMcp, onDone } = draw(withheld()); + await userEvent.click(screen.getByRole("button", { name: "批准并重新连接" })); + await waitFor(() => expect(onDone).toHaveBeenCalled()); + expect(trustMcp).toHaveBeenCalledTimes(1); + expect(trustMcp).toHaveBeenCalledWith("notes", "digest-1"); + }); + + it("shows the kernel's refusal instead of claiming success when the set changed meanwhile", async () => { + const trustMcp = vi.fn(async () => ({ state: "failed", error: "the held MCP tool definitions changed since they were shown" })); + render( {}} root="/w" live />); + await userEvent.click(screen.getByRole("button", { name: "批准并重新连接" })); + expect(await screen.findByText(/changed since they were shown/)).toBeTruthy(); + }); + + it("says how many more are withheld than listed", () => { + draw(withheld({ heldMore: 7 })); + expect(screen.getByRole("group", { name: "等待你批准的工具" }).textContent).toContain("9 个工具"); + expect(screen.getByText(/还有 7 个没有列出/)).toBeTruthy(); + }); + + it("offers no approval from a row the person cannot edit, and nothing when none is withheld", () => { + draw(withheld(), false); + expect(screen.queryByRole("button", { name: "批准并重新连接" })).toBeNull(); + cleanup(); + draw(withheld({ held: undefined, heldDigest: undefined })); + expect(screen.queryByRole("group", { name: "等待你批准的工具" })).toBeNull(); + }); +}); diff --git a/internal/frontend/serve/catalog.go b/internal/frontend/serve/catalog.go index cac8c7d00e..0e82873f60 100644 --- a/internal/frontend/serve/catalog.go +++ b/internal/frontend/serve/catalog.go @@ -262,8 +262,25 @@ type mcpEntry struct { Launch string `json:"launch,omitempty"` // PendingReason says why a pending server waits for the user, as a code. PendingReason string `json:"pendingReason,omitempty"` + // Held lists tools withheld from the model for a definition the user has not + // approved. HeldDigest names that whole set and comes back with the approval. + // HeldMore counts the withheld tools past the list's cap. + Held []mcpHeld `json:"held,omitempty"` + HeldDigest string `json:"heldDigest,omitempty"` + HeldMore int `json:"heldMore,omitempty"` } +// mcpHeld is one withheld tool: its visible name and how it differs from what +// was approved (added, changed, or unverifiable when the record cannot be read). +type mcpHeld struct { + Name string `json:"name"` + Class string `json:"class"` +} + +// mcpHeldLimit bounds how many withheld tools one row lists; the rest are only +// counted, so a server that withholds thousands cannot make the page heavy. +const mcpHeldLimit = 50 + // mcpTool is one tool as the server describes it, plus the two hints that // decide how a call is classified. Error carries a schema the host rejected: // the tool is listed but not callable, which is a different thing from absent. @@ -356,12 +373,25 @@ func (s *Server) mcp(w http.ResponseWriter, r *http.Request) { } if configured { row.Launch = launchText(st.Entry) + row.Held, row.HeldDigest, row.HeldMore = mcpHeldViews(st.Held) } out = append(out, row) } writeJSON(w, map[string]any{"servers": out, "scope": scopeView(ctl)}) } +func mcpHeldViews(held []plugin.HeldTool) ([]mcpHeld, string, int) { + if len(held) == 0 { + return nil, "", 0 + } + shown := held[:min(len(held), mcpHeldLimit)] + out := make([]mcpHeld, len(shown)) + for i, h := range shown { + out[i] = mcpHeld{Name: displayText(h.VisibleName, mcpServerTextLimit), Class: string(h.Class)} + } + return out, plugin.HeldDigest(held), len(held) - len(shown) +} + // remembered fills a row that has no live connection to ask with what the last // successful handshake recorded. It is marked as remembered rather than merged // silently: a row that cannot tell the two apart claims a server that is off or diff --git a/internal/frontend/serve/mcp_held_test.go b/internal/frontend/serve/mcp_held_test.go new file mode 100644 index 0000000000..5144c87226 --- /dev/null +++ b/internal/frontend/serve/mcp_held_test.go @@ -0,0 +1,48 @@ +package serve + +import ( + "fmt" + "strings" + "testing" + + "reasonix/internal/contract/tool" + "reasonix/internal/ext/plugin" +) + +func TestMCPHeldViewsNameTheSetTheDigestCovers(t *testing.T) { + held := []plugin.HeldTool{ + {RawName: "a", VisibleName: "mcp__s__a", Class: tool.HeldMCPAdded, Digest: "d1"}, + {RawName: "b", VisibleName: "mcp__s__b", Class: tool.HeldMCPChanged, Digest: "d2", Approved: "d0"}, + } + views, digest, more := mcpHeldViews(held) + if len(views) != 2 || more != 0 || views[0].Class != "added" || views[1].Class != "changed" { + t.Fatalf("views = %+v more = %d", views, more) + } + if digest != plugin.HeldDigest(held) { + t.Fatalf("digest %q is not the digest of the held set", digest) + } +} + +func TestMCPHeldViewsBoundTheRowButNotTheDigest(t *testing.T) { + var held []plugin.HeldTool + for i := range mcpHeldLimit + 7 { + held = append(held, plugin.HeldTool{RawName: fmt.Sprint("t", i), VisibleName: fmt.Sprint("t", i), Class: tool.HeldMCPAdded, Digest: "d"}) + } + views, digest, more := mcpHeldViews(held) + if len(views) != mcpHeldLimit || more != 7 { + t.Fatalf("listed %d, more %d", len(views), more) + } + if digest != plugin.HeldDigest(held) { + t.Fatal("the digest must cover every withheld tool, including the ones past the cap") + } +} + +func TestMCPHeldViewsSanitiseServerWrittenNames(t *testing.T) { + views, _, _ := mcpHeldViews([]plugin.HeldTool{{RawName: "x", VisibleName: "evil\x1b[31m" + strings.Repeat("n", mcpServerTextLimit*2), Class: tool.HeldMCPChanged, Digest: "d"}}) + if strings.ContainsRune(views[0].Name, 0x1b) || len([]rune(views[0].Name)) > mcpServerTextLimit+1 { + t.Fatalf("name was not sanitised and bounded: %q", views[0].Name) + } + if v, d, m := mcpHeldViews(nil); v != nil || d != "" || m != 0 { + t.Fatal("no held tools must produce no row fields") + } +} diff --git a/tools/repolint/wireparity.go b/tools/repolint/wireparity.go index 13193600c7..940b7bbf08 100644 --- a/tools/repolint/wireparity.go +++ b/tools/repolint/wireparity.go @@ -89,6 +89,7 @@ var mirroredWireTypes = []wireMirror{ // The MCP row: a status the host answered with and the page cannot read is a // row that goes back to reading the server's own prose for it. {"internal/frontend/serve/catalog.go", "mcpEntry", tsMcpFile, "McpEntry"}, + {"internal/frontend/serve/catalog.go", "mcpHeld", tsMcpFile, "McpHeld"}, // The fold bounds and the one in force: a field the panel cannot read is a // threshold a user sets and never sees applied. {"internal/session/control/compaction_settings.go", "CompactionSettings", tsModelFile, "CompactionSettings"},