diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 9beb49a3a2..530582b915 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -145,8 +145,8 @@ import { forgetLoadedMcpTools, invalidateMcpRegistrations, isRequestBodyRejection, + reconcileMcpRegistry, withdrawMcpToolsAfterRejection, - loadedMcpServers, loadedMcpTools, loadMcpServers, type McpServer @@ -2261,16 +2261,10 @@ export class AIChatManager { // workspace's servers installed would go on advertising its paths against // the workspace switched to. this.mcpServers = workspace === (this.operatingWorkspace ?? '') ? servers : [] - // A tool registered from a server that has since been turned off, deleted, or - // left behind by a workspace switch would still be callable, and would run - // against whichever workspace the chat is on now. The revision is checked too: - // a registered call bypasses the listing cache, so a connection edited elsewhere - // would otherwise keep running against the schema it was frozen with. - const live = new Map(this.mcpServers.map((s) => [s.path, s.editedAt])) - // The reconcile below only reaches servers that already registered something. A - // search still awaiting its listing has registered nothing yet, so a server turned - // off during that await would register after the fact and be advertised on the next - // iteration — bumping the generation makes that search drop its results instead. + // A search still awaiting its listing has registered nothing yet, so the reconcile + // below cannot reach it: a server turned off during that await would register after + // the fact and be advertised on the next iteration. Bumping the generation makes + // that search drop its results instead. const signature = this.mcpServers.map((s) => `${s.path}@${s.editedAt ?? ''}`).join(',') if (signature !== this.mcpServersSignature) { this.mcpServersSignature = signature @@ -2278,17 +2272,14 @@ export class AIChatManager { } // A registered call runs against the workspace the chat is on when it is made, not // the one it was registered in, and a fork carries the same resource path and - // `edited_at` as its parent — so the per-path reconcile below cannot tell those two + // `edited_at` as its parent — so the per-path reconcile cannot tell those two // servers apart. A workspace change drops the lot instead. if (this.mcpRegistryWorkspace !== undefined && this.mcpRegistryWorkspace !== workspace) { forgetLoadedMcpTools(this.mcpOwnerId) } this.mcpRegistryWorkspace = workspace - for (const { path, editedAt } of loadedMcpServers(this.mcpOwnerId)) { - if (!live.has(path) || live.get(path) !== editedAt) { - forgetLoadedMcpTools(this.mcpOwnerId, path) - } - } + // Tools and refusals frozen against a server that is now gone or has changed. + reconcileMcpRegistry(this.mcpOwnerId, this.mcpServers) if (this.mode === AIMode.GLOBAL) { this.configureGlobalMode() } diff --git a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte index 5e856f4754..aeeb167064 100644 --- a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte +++ b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte @@ -30,6 +30,7 @@ switch that decides whether this chat carries its tools. import { untrack } from 'svelte' import { getAiChatManager } from './aiChatManagerContext' import { clearMcpToolsCache } from './global/mcpTools' + import { forgetMcpServerMarks } from '$lib/components/mcp/serverMark' let { ws, @@ -390,8 +391,10 @@ switch that decides whether this chat carries its tools. async function refresh(target = ws) { // A path can be reconnected to a different server, so the cached tool list - // (and the readOnlyHint the confirmation gate reads) must not survive. + // (and the readOnlyHint the confirmation gate reads) must not survive — nor the + // provider mark the transcript's call rows show. clearMcpToolsCache() + forgetMcpServerMarks() await loadServers(target) // `refreshMcpServers` blanks the list when the workspace it is handed is not // the one the chat is on, so a refresh landing after a switch would take B's 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 d2c44e153f..27a9359048 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts @@ -37,6 +37,7 @@ import { forgetLoadedMcpTools, invalidateMcpRegistrations, isRequestBodyRejection, + reconcileMcpRegistry, withdrawMcpToolsAfterRejection, loadedMcpServers, mcpRegistryGeneration, @@ -480,18 +481,62 @@ describe('request rejections that withdraw registered tools', () => { expect(loadedMcpTools(OWNER)).toEqual([]) }) - it('registers a refused tool again once its server is reconnected', () => { + // Two servers whose schemas the provider will not take: keeping only the latest + // refusal lets them take turns poisoning the conversation forever. + it('keeps refusing every tool a rejection withdrew, not just the last', () => { + const [a, b] = [{ path: 'u/hugo/a_mcp' }, { path: 'u/hugo/b_mcp' }] + registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), a, [TOOLS[0]]) + withdrawMcpToolsAfterRejection(OWNER) + registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), b, [TOOLS[0]]) + withdrawMcpToolsAfterRejection(OWNER) + + expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), a, [TOOLS[0]])).toEqual([ + undefined + ]) + expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), b, [TOOLS[0]])).toEqual([ + undefined + ]) + }) + + // A refused tool is not registered, so the reconcile has to lift its refusal from the + // connected list rather than from the registry — turning the server off is the escape. + it('registers a refused tool again once its server is turned off and back on', () => { const server = SERVERS[0] registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]]) withdrawMcpToolsAfterRejection(OWNER) - forgetLoadedMcpTools(OWNER, server.path) + reconcileMcpRegistry(OWNER, []) + reconcileMcpRegistry(OWNER, [server]) expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])).toEqual([ 'mcp_u_hugo_github_mcp__get_issue' ]) }) + it('registers a refused tool again once its server is edited', () => { + const server = { path: 'u/hugo/github_mcp', editedAt: '2026-01-01T00:00:00Z' } + registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]]) + withdrawMcpToolsAfterRejection(OWNER) + + reconcileMcpRegistry(OWNER, [{ ...server, editedAt: '2026-01-02T00:00:00Z' }]) + + expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])).toEqual([ + 'mcp_u_hugo_github_mcp__get_issue' + ]) + }) + + it('keeps refusing while its server is unchanged', () => { + const server = SERVERS[0] + registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]]) + withdrawMcpToolsAfterRejection(OWNER) + + reconcileMcpRegistry(OWNER, [server]) + + expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])).toEqual([ + undefined + ]) + }) + it('separates a refused body from a refused account', () => { expect(isRequestBodyRejection(400)).toBe(true) expect(isRequestBodyRejection(422)).toBe(true) diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index 4cd0e47a06..5d0968ca3d 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -125,8 +125,12 @@ const registries = new Map() * otherwise a rotation or disposal during that await is silently undone. */ const generations = new Map() -/** Registry keys `withdrawMcpToolsAfterRejection` will not let an owner register again. */ -const refused = new Map>() +/** + * Registry keys `withdrawMcpToolsAfterRejection` will not let an owner register again, + * against the server revision each was refused at — so the refusal lifts when that + * server is edited, turned off, or reconnected, and holds otherwise. + */ +const refused = new Map>() /** Bumped by the all-owner clear. Folded into the token below so it also invalidates an * owner that has registered nothing yet, and is therefore in neither map. */ let allGeneration = 0 @@ -227,13 +231,46 @@ export function isRequestBodyRejection(status: number | undefined): boolean { * was just refused, and the next request fails the same way. Which schema was at fault * is not knowable from the error, so every tool that was loaded goes on the list — each * falls back to the free-form wrapper, which is how they were reached before they could - * be registered at all. A server turned off, edited, or reconnected clears its entries, - * as does a new conversation. + * be registered at all. Refusals accumulate across rejections: dropping the earlier ones + * would let two bad schemas take turns poisoning the conversation forever. + * + * `reconcileMcpRegistry` lifts a refusal when its server changes, and a new conversation + * clears the lot. */ export function withdrawMcpToolsAfterRejection(owner: string) { - const keys = [...(registries.get(owner)?.tools.keys() ?? [])] + const registry = registries.get(owner) + const frozen = [...(registry?.tools.keys() ?? [])].map( + (key) => [key, registry?.editedAt.get(key)] as const + ) + // Held across the drop, which clears the owner's refusals along with its tools. + const carried = refused.get(owner) ?? new Map() forgetLoadedMcpTools(owner) - if (keys.length > 0) refused.set(owner, new Set(keys)) + for (const [key, editedAt] of frozen) carried.set(key, editedAt) + if (carried.size > 0) refused.set(owner, carried) +} + +/** + * Drop what a change in the connected servers makes stale: tools registered from a server + * that is gone or has been edited since, and the refusals recorded against it. Both are + * frozen copies of what a server said — a registered call bypasses the listing cache, and + * a refusal outlives the registry it was taken from, so neither may survive its server. + * + * The refusals are reconciled here rather than through the registry: a refused tool is by + * construction not registered, so nothing else in the chat can reach it. + */ +export function reconcileMcpRegistry(owner: string, servers: McpServer[]) { + const live = new Map(servers.map((s) => [s.path, s.editedAt])) + const stale = (path: string, editedAt: string | undefined) => + !live.has(path) || live.get(path) !== editedAt + for (const { path, editedAt } of loadedMcpServers(owner)) { + if (stale(path, editedAt)) forgetLoadedMcpTools(owner, path) + } + const keys = refused.get(owner) + if (!keys) return + for (const [key, editedAt] of [...keys]) { + if (stale(serverPathOfKey(key), editedAt)) keys.delete(key) + } + if (keys.size === 0) refused.delete(owner) } /** Drop one owner's loaded tools, or only those belonging to one server. */ @@ -270,7 +307,7 @@ function clearRefused(owner: string, serverPath?: string) { const keys = refused.get(owner) if (!keys) return const prefix = `${serverPath}::` - for (const key of [...keys]) { + for (const key of [...keys.keys()]) { if (key.startsWith(prefix)) keys.delete(key) } if (keys.size === 0) refused.delete(owner) diff --git a/frontend/src/lib/components/mcp/serverMark.ts b/frontend/src/lib/components/mcp/serverMark.ts index 6542c558a9..c728abdead 100644 --- a/frontend/src/lib/components/mcp/serverMark.ts +++ b/frontend/src/lib/components/mcp/serverMark.ts @@ -26,6 +26,15 @@ export function resolveMcpServerMark(workspace: string, path: string): Promise { const cached = cachedProviderMark(workspace, path) if (cached) return { icon: await loadProviderIcon(cached.key) }