From effdcd99155a9235856b245b7f49a37e0632db08 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Sat, 15 Aug 2026 10:57:00 +0200 Subject: [PATCH] fix: recover from a refused mcp read assertion, drop stale discovery (#10710) * fix: recover from a refused mcp read assertion, drop stale discovery Co-Authored-By: Claude Opus 5 (1M context) * fix: drop the stale listing from the raw error, not the bounded payload Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../copilot/chat/global/mcpTools.test.ts | 24 +++++++++++++++++++ .../copilot/chat/global/mcpTools.ts | 23 +++++++++++++++++- .../mcp/McpServerOAuthConnect.svelte | 12 +++++++++- 3 files changed, 57 insertions(+), 2 deletions(-) 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 79871eacf8..a155115d28 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts @@ -127,6 +127,30 @@ describe('read/write split', () => { expect(getTool('call_mcp_write_tool').requiresConfirmation).toBe(true) }) + // The rejection sends the model to the write tool, which classifies from the + // same cached listing: without dropping it, that retry is refused too and the + // model has nowhere to go until the entry expires. + it('reclassifies after the backend refuses the read-only assertion', async () => { + callMcpToolMock.mockRejectedValueOnce({ + status: 400, + body: 'Bad request: MCP tool get_issue is not marked read-only by the server, it must be called as a tool that modifies data' + }) + const refused = await run('call_mcp_read_tool', { + server: 'u/hugo/github_mcp', + tool: 'get_issue' + }) + expect(refused.success).toBe(false) + + // The server now reports the same name as mutating. + getMcpToolsMock.mockResolvedValue([{ ...TOOLS[0], annotations: { readOnlyHint: false } }]) + callMcpToolMock.mockResolvedValue({ content: [] }) + const retried = await run('call_mcp_write_tool', { + server: 'u/hugo/github_mcp', + tool: 'get_issue' + }) + expect(retried.success).toBe(true) + }) + // This classification comes from a listing that can predate a resource edited // mid-turn, so the backend re-checks it against the server it is calling — but // only knows to when the unconfirmed path says it assumed read-only. diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index 4784c06d7e..d5f16d5c54 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -66,6 +66,20 @@ export function clearMcpToolsCache() { toolsCache = {} } +/** + * Drop one server's listing after the backend refused the read-only assertion it + * produced. Without this the write call the model is being sent to would be + * rejected by the same stale `readOnlyHint`, leaving it with nowhere to go until + * the entry expires. + */ +function forgetServerTools(workspace: string, path: string) { + cacheGeneration++ + const prefix = `${workspace}:${path}:` + for (const key of Object.keys(toolsCache)) { + if (key.startsWith(prefix)) delete toolsCache[key] + } +} + /** * The `mcp` resources the user turned on for this workspace. Readable is not * enough: a shared resource would otherwise put a server the user never chose @@ -208,10 +222,17 @@ async function executeTool( }) } catch (e: any) { const status = e?.status + const error = errorMessage(e) + // The server disagrees with the listing this call was classified from, so the + // write tool the model is sent to must not be handed the same answer. Matching + // loosely is safe: the worst a false positive costs is one extra listing. + if (skippedConfirmation && status === 400 && /read-only/i.test(error)) { + forgetServerTools(workspace, server.path) + } return bounded({ success: false, ...(status ? { status } : {}), - error: errorMessage(e), + error, // Wrong arguments are the common failure: echo the schema so the model // can self-correct on the next call without a separate schema tool. ...(status >= 400 && status < 500 ? { schema: tool.inputSchema } : {}) diff --git a/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte b/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte index e086012166..bafbf4b7b4 100644 --- a/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte +++ b/frontend/src/lib/components/mcp/McpServerOAuthConnect.svelte @@ -38,6 +38,7 @@ let noOAuth = $state(false) let pending: { workspace: string; path: string; serverUrl: string } | undefined = undefined let popup: Window | null = null + let destroyed = false async function discoverOAuth() { status = 'discovering' @@ -46,11 +47,17 @@ discoveryResult = await McpOauthService.discoverMcpOauth({ requestBody: { mcp_server_url: serverUrl } }) + // The caller keys this component on the url, so editing it replaces this + // instance while its request is still in flight. Reporting the answer then + // would describe the previous server: a slow failure would take the new + // connector down with it. + if (destroyed) return selectedScopes = discoveryResult?.scopes_supported ?? [] noOAuth = false status = 'discovered' onDiscovered?.(true) } catch (e) { + if (destroyed) return console.error('Error discovering OAuth settings', e) noOAuth = true status = 'idle' @@ -187,7 +194,10 @@ onMount(discoverOAuth) - onDestroy(cleanup) + onDestroy(() => { + destroyed = true + cleanup() + })