From bcc51e3b311824ca49760168f75ce20eb47f9258 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Thu, 10 Sep 2026 16:34:05 +0200 Subject: [PATCH] fix: make the transcript's MCP mark survive a reload without a warm cache Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JgzuxyafKNF2uaEeL35XQw --- .../copilot/chat/ToolExecutionDisplay.svelte | 40 +++++-------------- .../copilot/chat/global/mcpTools.test.ts | 25 ++++++++++-- .../copilot/chat/global/mcpTools.ts | 10 +++-- 3 files changed, 38 insertions(+), 37 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte index f514b36347..5ac02974d5 100644 --- a/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/ToolExecutionDisplay.svelte @@ -41,7 +41,6 @@ import ExpandableImage from '$lib/components/common/image/ExpandableImage.svelte' import McpServerIcon from '$lib/components/mcp/McpServerIcon.svelte' import { resolveMcpServerMark } from '$lib/components/mcp/serverMark' - import { cachedProviderMark } from '$lib/components/mcp/iconCache' import { mcpServerForToolName } from './global/mcpTools' interface Props { @@ -50,34 +49,17 @@ let { message }: Props = $props() - // A call records its server on the row, which is what survives a reload. The rest - // is for rows written before it did: the generic wrappers name their server in the - // arguments, and a registered tool is resolved through the in-memory registry. - const MCP_CALL_TOOLS = ['call_mcp_read_tool', 'call_mcp_write_tool'] - const mcpServerPath = $derived.by(() => { - const claimed = (() => { - if (message.mcpServer) return message.mcpServer - const name = message.toolName - if (!name) return undefined - if (MCP_CALL_TOOLS.includes(name)) { - const server = message.parameters?.server - return typeof server === 'string' ? server : undefined - } - return mcpServerForToolName(aiChatManager.mcpOwnerId, name) - })() - if (!claimed) return undefined - // Both sources are ultimately a path the model wrote, and resolving one reads - // that resource — so it has to name a server the user connected, not any - // workspace resource the model can point at. - if (aiChatManager.mcpServers.some((s) => s.path === claimed)) return claimed - // A session runtime does not populate `mcpServers` until its first send, so on a - // reloaded transcript the live list is empty and the rows this path was persisted - // for would go unmarked. The local icon cache is the second witness: it only ever - // holds servers this user's own MCP list resolved, and answering from it costs - // neither a read nor a request. - const workspace = aiChatManager.operatingWorkspace - return workspace && cachedProviderMark(workspace, claimed) ? claimed : undefined - }) + const mcpServerPath = $derived( + // `mcpServer` is written by the call itself, from the connected-server list rather + // than from the model's arguments, so it needs no second opinion here — which is + // what lets a reloaded transcript resolve it. Nothing else is consulted: the live + // server list is empty on a session until its first send, and the model's own + // `parameters.server` is a path it could point anywhere. + message.mcpServer ?? + (message.toolName + ? mcpServerForToolName(aiChatManager.mcpOwnerId, message.toolName) + : undefined) + ) const isPlanReview = $derived(message.toolName === EXIT_PLAN_MODE_TOOL) const isPlanCard = $derived(isPlanCardTool(message.toolName)) diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts index 5192dd8d2c..087543624f 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts @@ -173,10 +173,9 @@ describe('loaded remote tools', () => { expect(registered?.def.function.parameters).toEqual(TOOLS[0].inputSchema) }) - // The row's provider icon is resolved from this, and the registry of loaded tools - // lives only in memory — so without it on the message, a reloaded transcript loses - // every mark. Recorded before the server is resolved, so an unreachable server - // still marks its own failure row. + // The row's provider mark is resolved from this alone — the registry lives only in + // memory and the live server list is empty on a reloaded session — so it has to land + // on the message, and before the listing, which an unreachable server never answers. it('records the server on the row, even when it cannot be reached', async () => { getMcpToolsMock.mockRejectedValue(new Error('connection refused')) const callbacks = createToolCallbacks() @@ -194,6 +193,24 @@ describe('loaded remote tools', () => { }) }) + // The transcript trusts this field without re-checking it, so it must come from the + // connected list rather than from the arguments — otherwise a path the model made up + // would be resolved, and resolving one reads that workspace resource. + it('records nothing for a server the user has not connected', async () => { + const callbacks = createToolCallbacks() + + await getTool('call_mcp_read_tool').fn({ + args: { server: 'u/hugo/not_connected', tool: 'get_issue', arguments: {} }, + workspace: 'test-ws', + helpers: {}, + toolCallbacks: callbacks, + toolId: 'tool-1' + }) + + const recorded = callbacks.setToolStatus.mock.calls.map(([, meta]: [string, any]) => meta) + expect(recorded.some((m: any) => m?.mcpServer !== undefined)).toBe(false) + }) + // Several chats are live at once — the docked one plus a warm runtime per session, // all in GLOBAL mode. A shared registry would put one chat's remote tools into // another's request, and let either one's "New chat" drop the other's. diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index e96c668094..029970cbe4 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -523,10 +523,12 @@ function createCallTool(owner: string, servers: McpServer[], mode: 'read' | 'wri }), fn: async ({ args, workspace, toolId, toolCallbacks }) => { const parsed = callMcpToolSchema.parse(args) - // Recorded from the arguments, before resolution: resolving needs a live - // listing, so a server that cannot be reached would otherwise leave its own - // failure row unmarked. - toolCallbacks.setToolStatus(toolId, { mcpServer: parsed.server }) + // Matched against the connected list rather than taken from the arguments, so + // what lands on the row is a server the user connected and the transcript can + // resolve it without re-deciding that. Done before the listing, which needs the + // server to be reachable — an unreachable one still marks its own failure row. + const named = servers.find((s) => s.path === parsed.server) + if (named) toolCallbacks.setToolStatus(toolId, { mcpServer: named.path }) // Listing is a live call to a third party: a server that has gone away // must fail this tool, not the chat loop around it. let resolved: Awaited>