From a3eebf84282ce148453a517546132f219f810567 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Thu, 10 Sep 2026 20:46:33 +0200 Subject: [PATCH] fix: keep a refused MCP schema out of the conversation it broke Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JgzuxyafKNF2uaEeL35XQw --- .../copilot/chat/AIChatManager.svelte.ts | 21 +++-- .../copilot/chat/global/mcpTools.test.ts | 28 +++++++ .../copilot/chat/global/mcpTools.ts | 82 +++++++++++++++---- .../src/lib/components/copilot/chat/shared.ts | 6 +- 4 files changed, 115 insertions(+), 22 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 32227d7f0c..9beb49a3a2 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -145,6 +145,7 @@ import { forgetLoadedMcpTools, invalidateMcpRegistrations, isRequestBodyRejection, + withdrawMcpToolsAfterRejection, loadedMcpServers, loadedMcpTools, loadMcpServers, @@ -1217,6 +1218,8 @@ export class AIChatManager { private mcpServersRefreshId = 0 /** The connected set the last refresh settled on, to notice when it changes. */ private mcpServersSignature = '' + /** The workspace the registered tools were frozen against. */ + private mcpRegistryWorkspace: string | undefined // The GLOBAL prompt's path conventions and folder ACLs, for this chat's operating // workspace (`GlobalPromptIdentity`). Resolved asynchronously alongside skills, never @@ -2273,6 +2276,14 @@ export class AIChatManager { this.mcpServersSignature = signature invalidateMcpRegistrations(this.mcpOwnerId) } + // 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 + // 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) @@ -2286,15 +2297,15 @@ export class AIChatManager { /** * A registered MCP tool carries a schema a third party wrote, and a provider that * refuses it refuses every later request in the conversation the same way — with an - * error naming the request, not the tool. So a rejection drops the registered tools: - * the send that follows goes out with the search tool and the wrappers only, and the - * model can register again. A false positive costs one re-search. + * error naming the request, not the tool. So a rejection withdraws those tools: the + * send that follows goes out with the search tool and the wrappers only, and a later + * search leaves them to the wrappers rather than registering the same schema again. */ private dropMcpToolsOnRejectedRequest = (err: unknown) => { if (!isRequestBodyRejection(getErrorStatus(err))) return if (loadedMcpTools(this.mcpOwnerId).length === 0) return - console.warn('Dropping registered MCP tools after a rejected request', err) - forgetLoadedMcpTools(this.mcpOwnerId) + console.warn('Withdrawing registered MCP tools after a rejected request', err) + withdrawMcpToolsAfterRejection(this.mcpOwnerId) if (this.mode === AIMode.GLOBAL) { this.configureGlobalMode() } 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 e4feaee099..d2c44e153f 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, + withdrawMcpToolsAfterRejection, loadedMcpServers, mcpRegistryGeneration, loadedMcpTools, @@ -464,6 +465,33 @@ describe('loaded remote tools', () => { // provider refuses, and it must not fire on an account problem: a quota error would // then cost the user every tool they had searched for. describe('request rejections that withdraw registered tools', () => { + // Dropping alone only moves the failure: the model is told to search again, registers + // the schema that was just refused, and the next request fails the same way. + it('will not register a refused tool again in the same conversation', () => { + const server = SERVERS[0] + registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]]) + + withdrawMcpToolsAfterRejection(OWNER) + + expect(loadedMcpTools(OWNER)).toEqual([]) + expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])).toEqual([ + undefined + ]) + expect(loadedMcpTools(OWNER)).toEqual([]) + }) + + it('registers a refused tool again once its server is reconnected', () => { + const server = SERVERS[0] + registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]]) + withdrawMcpToolsAfterRejection(OWNER) + + forgetLoadedMcpTools(OWNER, server.path) + + expect(registerMcpTools(OWNER, mcpRegistryGeneration(OWNER), server, [TOOLS[0]])).toEqual([ + 'mcp_u_hugo_github_mcp__get_issue' + ]) + }) + 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 585b0dd70f..4cd0e47a06 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -125,6 +125,8 @@ 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>() /** 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 @@ -207,12 +209,42 @@ export function mcpServerForToolName(owner: string, toolName: string): string | return undefined } +/** + * Whether a failed chat request was the provider refusing the body rather than the + * account. A remote schema this provider will not accept refuses every later request + * in the conversation the same way, so the registered tools are withdrawn on the first + * of these; auth and quota statuses say nothing about the schemas. + */ +export function isRequestBodyRejection(status: number | undefined): boolean { + if (status === undefined || status < 400 || status >= 500) return false + return status !== 401 && status !== 403 && status !== 429 +} + +/** + * Drop an owner's registered tools after the provider refused the request, and refuse + * to register those same tools again for the rest of the conversation. Dropping alone + * only moves the failure: the model is told to search again, registers the schema that + * 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. + */ +export function withdrawMcpToolsAfterRejection(owner: string) { + const keys = [...(registries.get(owner)?.tools.keys() ?? [])] + forgetLoadedMcpTools(owner) + if (keys.length > 0) refused.set(owner, new Set(keys)) +} + /** Drop one owner's loaded tools, or only those belonging to one server. */ export function forgetLoadedMcpTools(owner: string, serverPath?: string) { // Before the empty-registry check: a conversation can rotate while its first search // is still awaiting a listing, with nothing registered yet, and that search must // still be rejected when it comes back. invalidateOwner(owner) + // Whatever drops a tool also lifts its refusal: a new conversation, and a server + // turned off or edited, both mean the next listing is worth registering again. + clearRefused(owner, serverPath) const registry = registries.get(owner) if (!registry) return if (serverPath === undefined) { @@ -230,6 +262,20 @@ export function forgetLoadedMcpTools(owner: string, serverPath?: string) { if (registry.tools.size === 0) registries.delete(owner) } +function clearRefused(owner: string, serverPath?: string) { + if (serverPath === undefined) { + refused.delete(owner) + return + } + const keys = refused.get(owner) + if (!keys) return + const prefix = `${serverPath}::` + for (const key of [...keys]) { + if (key.startsWith(prefix)) keys.delete(key) + } + if (keys.size === 0) refused.delete(owner) +} + /** * Drop every owner's loaded tools. Used when a server resource itself changed, which * makes the frozen copy every chat holds stale at once — not for one chat rotating. @@ -237,6 +283,7 @@ export function forgetLoadedMcpTools(owner: string, serverPath?: string) { function forgetAllLoadedMcpTools() { allGeneration++ registries.clear() + refused.clear() } /** @@ -591,17 +638,6 @@ function shortHash(text: string): string { * `list_issues` stay apart, and nothing parses it back: the registry keys on the * pair, so truncation only has to stay unique. */ -/** - * Whether a failed chat request was the provider refusing the body rather than the - * account. A remote schema this provider will not accept refuses every later request - * in the conversation the same way, so the registered tools are dropped on the first - * of these; auth and quota statuses say nothing about the schemas. - */ -export function isRequestBodyRejection(status: number | undefined): boolean { - if (status === undefined || status < 400 || status >= 500) return false - return status !== 401 && status !== 403 && status !== 429 -} - function registeredToolName(serverPath: string, toolName: string): string { const full = `${MCP_TOOL_NAME_PREFIX}${sanitizeToolNamePart(serverPath)}__${sanitizeToolNamePart(toolName)}` if (full.length <= MAX_TOOL_NAME_CHARS) return full @@ -624,12 +660,24 @@ function uniqueRegisteredName( toolName: string ): string { const name = registeredToolName(serverPath, toolName) - for (const [otherKey, tool] of registry.tools) { - if (otherKey !== key && tool.def.function.name === name) { - return `${name.slice(0, MAX_TOOL_NAME_CHARS - 8)}_${shortHash(key)}` + const taken = (candidate: string) => { + for (const [otherKey, tool] of registry.tools) { + if (otherKey !== key && tool.def.function.name === candidate) return true } + return false } - return name + if (!taken(name)) return name + // A remote names its own tools, so the suffixed candidate can be taken too — by a + // tool that sanitizes to exactly it, or by another key whose hash collided. Salt + // until it is free; the registry holds MAX_LOADED_TOOLS entries, so this ends. + const stem = name.slice(0, MAX_TOOL_NAME_CHARS - 8) + for (let salt = 0; salt <= MAX_LOADED_TOOLS; salt++) { + const candidate = `${stem}_${shortHash(`${key}#${salt}`)}` + if (!taken(candidate)) return candidate + } + // More collisions than there are entries to collide with: the counter is unique + // within the registry by construction. + return `${stem}_${registry.counter}` } /** @@ -721,8 +769,12 @@ export function registerMcpTools( // conversation's tools into the new one, or resurrect a registry nothing can reach. if (generation !== mcpRegistryGeneration(owner)) return tools.map(() => undefined) const registry = registryFor(owner) + const refusedKeys = refused.get(owner) const names = tools.map((tool) => { const key = `${server.path}::${tool.name}` + // Registering this again is what made the last request fail; the wrapper still + // reaches it (see `withdrawMcpToolsAfterRejection`). + if (refusedKeys?.has(key)) return undefined const name = uniqueRegisteredName(registry, key, server.path, tool.name) const readOnly = isReadOnly(tool) const parameters = safeInputSchema(tool.inputSchema) diff --git a/frontend/src/lib/components/copilot/chat/shared.ts b/frontend/src/lib/components/copilot/chat/shared.ts index e542968373..9d4aeba176 100644 --- a/frontend/src/lib/components/copilot/chat/shared.ts +++ b/frontend/src/lib/components/copilot/chat/shared.ts @@ -722,10 +722,12 @@ async function callTool({ // A registered MCP tool is withdrawn whenever its registration is dropped — a // server turned off, an eviction, a schema the provider refused — while the search // result that advertised its name stays in the transcript. Searching again is the - // way back; the mode advice below would send the model after a tool it has not got. + // way back, through whichever name that search returns: the tool may come back + // registered, or only through the wrappers. The mode advice below would send the + // model after a tool it has not got. if (functionName.startsWith(MCP_TOOL_NAME_PREFIX)) { throw new Error( - `Unknown tool call: ${functionName}. That MCP tool is no longer loaded — call search_mcp_tools again to load it.` + `Unknown tool call: ${functionName}. That MCP tool is no longer loaded — call search_mcp_tools again and use the name it returns.` ) } throw new Error(