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) <noreply@anthropic.com>

* fix: drop the stale listing from the raw error, not the bounded payload

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

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.

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 } : {})

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()
+	})
 </script>
 
 <div class="flex flex-col gap-4">