From 718b2fda254ecfffe6309eef88f5bc746f2191a3 Mon Sep 17 00:00:00 2001 From: seraf Date: Mon, 19 Oct 2026 12:52:17 +0300 Subject: [PATCH 1/2] fix(sdk): surface the authenticate tool for a needs-auth MCP server An embedder hands mcpServers to a v2 session, a server answers 401, and system:init reports needs-auth. That is where it ended: connectSdkMcpServers kept tools only for a connected client, so the host was told the server needs authentication and given nothing that could act on it. The interactive CLI's /mcp is not available to an embedder. The CLI path already attaches the authenticate pseudo-tool for exactly this case (getMcpToolsCommandsAndResources). This adds the same branch to the SDK path, with the same expression: the model is told the server exists, calls mcp____authenticate, and receives a URL to hand the user. Nothing about discovery, PKCE, the loopback listener or token storage changes. The test uses a real local server rather than a stub of connectToServer, because needs-auth is not decided by the 401 alone: the MCP SDK walks RFC 9728 resource metadata, RFC 8414 authorization-server metadata and dynamic client registration first, and only then reports that a redirect is needed. A stub would assert our guess about that chain instead of the behaviour. Refs #2294 Co-Authored-By: Claude Opus 5 --- src/entrypoints/sdk/mcpNeedsAuth.test.ts | 160 +++++++++++++++++++++++ src/entrypoints/sdk/permissions.ts | 11 ++ 2 files changed, 171 insertions(+) create mode 100644 src/entrypoints/sdk/mcpNeedsAuth.test.ts diff --git a/src/entrypoints/sdk/mcpNeedsAuth.test.ts b/src/entrypoints/sdk/mcpNeedsAuth.test.ts new file mode 100644 index 0000000000..e0bea84643 --- /dev/null +++ b/src/entrypoints/sdk/mcpNeedsAuth.test.ts @@ -0,0 +1,160 @@ +/** + * A server that wants OAuth must not be a dead end for an embedder. + * + * The CLI already surfaces the `authenticate` pseudo-tool for a `needs-auth` + * client, so the model can start the flow and hand the user a URL. The SDK + * path kept tools only for a connected client, which left a host with a + * status it could read (`system:init.mcp_servers`) and nothing it could do. + * + * The server here is a real local one rather than a stub of `connectToServer`: + * `needs-auth` is not decided by the 401 alone — the MCP SDK walks RFC 9728 + * resource metadata, RFC 8414 authorization-server metadata and dynamic client + * registration first, and only then reports that a redirect is needed. A stub + * would assert our guess about that chain instead of the behaviour. + */ + +import { afterAll, afterEach, beforeAll, beforeEach, expect, test } from 'bun:test' +import { mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { acquireSharedMutationLock, releaseSharedMutationLock } from '../../test/sharedMutationLock.js' +import { + getClaudeConfigHomeDirOverrideForTesting, + setClaudeConfigHomeDirForTesting, +} from '../../utils/envUtils.js' + +import { connectSdkMcpServers } from './permissions.js' + +let server: ReturnType | null = null +let configHome = '' +let originalConfigOverride: string | undefined +let hadMacro = false +let savedMacro: unknown + +/** + * A server that asks to be signed into, and answers the discovery the MCP + * SDK does on the way there. It never issues a token: the point is to reach + * "a redirect is needed", which is what `needs-auth` means. + */ +function serveNeedingAuth(): string { + let base = '' + server = Bun.serve({ + port: 0, + fetch: request => { + const path = new URL(request.url).pathname + if (path.startsWith('/.well-known/oauth-protected-resource')) { + return Response.json({ resource: `${base}/mcp`, authorization_servers: [base] }) + } + if ( + path.startsWith('/.well-known/oauth-authorization-server') || + path.startsWith('/.well-known/openid-configuration') + ) { + return Response.json({ + issuer: base, + authorization_endpoint: `${base}/authorize`, + token_endpoint: `${base}/token`, + registration_endpoint: `${base}/register`, + response_types_supported: ['code'], + code_challenge_methods_supported: ['S256'], + }) + } + if (path === '/register') { + return Response.json( + { + client_id: 'stand-in-client', + client_secret: 'not-a-secret', + redirect_uris: ['http://127.0.0.1:1/callback'], + client_id_issued_at: Math.floor(Date.now() / 1000), + }, + { status: 201 }, + ) + } + return Response.json( + { error: 'invalid_token', error_description: 'sign in first' }, + { + status: 401, + headers: { + 'WWW-Authenticate': `Bearer resource_metadata="${base}/.well-known/oauth-protected-resource"`, + }, + }, + ) + }, + }) + base = `http://127.0.0.1:${server.port}` + return `${base}/mcp` +} + +/** A server that is simply broken, with nothing to sign into. */ +function serveBroken(): string { + server = Bun.serve({ port: 0, fetch: () => new Response('nope', { status: 500 }) }) + return `http://127.0.0.1:${server.port}/mcp` +} + +beforeAll(async () => { + await acquireSharedMutationLock('entrypoints/sdk/mcpNeedsAuth.test.ts') + // Build-time constants the connection path reads through. Absent, the + // failure is "MACRO is not defined" and says nothing about MCP. + hadMacro = Object.hasOwn(globalThis, 'MACRO') + savedMacro = (globalThis as Record).MACRO + ;(globalThis as Record).MACRO = { + VERSION: '0.0.0-test', + DISPLAY_VERSION: '0.0.0-test', + BUILD_TIME: 'test', + ISSUES_EXPLAINER: 'test', + PACKAGE_URL: 'test', + NATIVE_PACKAGE_URL: undefined, + } +}) + +afterAll(() => { + if (hadMacro) (globalThis as Record).MACRO = savedMacro + else delete (globalThis as Record).MACRO + releaseSharedMutationLock() +}) + +beforeEach(async () => { + originalConfigOverride = getClaudeConfigHomeDirOverrideForTesting() + // Both the 15-minute needs-auth cache and the client registration the SDK + // performs are keyed to the config home. Somewhere disposable, so this + // touches neither the developer's own store nor the next test. + configHome = await mkdtemp(join(tmpdir(), 'openclaude-mcp-auth-')) + setClaudeConfigHomeDirForTesting(configHome) +}) + +afterEach(async () => { + server?.stop(true) + server = null + setClaudeConfigHomeDirForTesting(originalConfigOverride) + await rm(configHome, { recursive: true, force: true }) +}) + +test('A SERVER THAT NEEDS SIGNING INTO ARRIVES WITH THE AUTHENTICATE TOOL', async () => { + const url = serveNeedingAuth() + + const { clients, tools } = await connectSdkMcpServers({ gated: { type: 'http', url } }) + + expect(clients.map(one => one.type)).toEqual(['needs-auth']) + expect(tools.map(one => one.name)).toEqual(['mcp__gated__authenticate']) + // The description is what the model reads to decide whether to call it, so + // it has to name the server and say what calling it does. + // Both arguments are ignored by this tool — the text is fixed when it is + // built — but the Tool interface asks for them. + const said = await tools[0]!.description({} as never, {} as never) + expect(said).toContain('gated') + expect(said).toContain('authentication') +}, 30_000) + +/** + * The branch is about authentication, not about failure in general. A server + * that is simply broken has nothing to sign into, and offering the model a + * sign-in for it would invite a wasted turn. + */ +test('a server that fails for another reason still gets no tool', async () => { + const url = serveBroken() + + const { clients, tools } = await connectSdkMcpServers({ broken: { type: 'http', url } }) + + expect(clients.map(one => one.type)).toEqual(['failed']) + expect(tools).toEqual([]) +}, 30_000) diff --git a/src/entrypoints/sdk/permissions.ts b/src/entrypoints/sdk/permissions.ts index 204bcf5ed0..e654fe8335 100644 --- a/src/entrypoints/sdk/permissions.ts +++ b/src/entrypoints/sdk/permissions.ts @@ -19,6 +19,7 @@ import { type ToolPermissionContext, type Tool, } from '../../Tool.js' +import { createMcpAuthTool } from '../../tools/McpAuthTool/McpAuthTool.js' import { MCPTool } from '../../tools/MCPTool/MCPTool.js' import type { MCPServerConnection, ScopedMcpServerConfig } from '../../services/mcp/types.js' import { connectToServer, fetchToolsForClient } from '../../services/mcp/client.js' @@ -590,6 +591,16 @@ export async function connectSdkMcpServers( return { client, tools: serverTools } } + // A server that answered 401 is not broken — it is waiting to be + // signed into, and the model is the only one here who can ask. The + // CLI path already surfaces the pseudo-tool for exactly this case + // (client.ts, getMcpToolsCommandsAndResources); without it an + // embedder is told the server needs auth and handed no way to act on + // that, since `system:init` reports the status and nothing else can. + if (client.type === 'needs-auth') { + return { client, tools: [createMcpAuthTool(name, scopedConfig as unknown as ScopedMcpServerConfig)] } + } + // Return failed/pending client with no tools return { client, tools: [] } } catch (error) { From 03a6d5a8b1060f3b68466bd4921952e5476379f6 Mon Sep 17 00:00:00 2001 From: seraf Date: Wed, 21 Oct 2026 18:16:45 +0300 Subject: [PATCH 2/2] fix(sdk): tell the model a new session is needed after MCP auth Address review on #2295. createMcpAuthTool's text promised that a needs-auth server's real tools "become available automatically" once the user authorizes. That is true on the CLI path, where the background continuation swaps them into appState.mcp.tools, but not on the SDK path: a v2 session connects its MCP servers once (v2.ts, self.mcpConnected) and never re-reads the tool pool, so the model would look for tools the session cannot offer. Add an optional realToolsInCurrentContext to createMcpAuthTool (default true, CLI unchanged) and pass false from connectSdkMcpServers, so the SDK tool tells the model to start a new session. The test asserts the wording. Refs #2294 --- src/entrypoints/sdk/mcpNeedsAuth.test.ts | 4 +++ src/entrypoints/sdk/permissions.ts | 14 +++++++++- src/tools/McpAuthTool/McpAuthTool.ts | 35 ++++++++++++++++++++++-- 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/entrypoints/sdk/mcpNeedsAuth.test.ts b/src/entrypoints/sdk/mcpNeedsAuth.test.ts index e0bea84643..42789a5e1b 100644 --- a/src/entrypoints/sdk/mcpNeedsAuth.test.ts +++ b/src/entrypoints/sdk/mcpNeedsAuth.test.ts @@ -143,6 +143,10 @@ test('A SERVER THAT NEEDS SIGNING INTO ARRIVES WITH THE AUTHENTICATE TOOL', asyn const said = await tools[0]!.description({} as never, {} as never) expect(said).toContain('gated') expect(said).toContain('authentication') + // The embedder's session connects its MCP servers once and does not pick + // the real tools up after auth (v2.ts, self.mcpConnected), so the SDK path + // must not promise that it will. Raised in review on the PR. + expect(said).toContain('new session') }, 30_000) /** diff --git a/src/entrypoints/sdk/permissions.ts b/src/entrypoints/sdk/permissions.ts index e654fe8335..4213040dea 100644 --- a/src/entrypoints/sdk/permissions.ts +++ b/src/entrypoints/sdk/permissions.ts @@ -598,7 +598,19 @@ export async function connectSdkMcpServers( // embedder is told the server needs auth and handed no way to act on // that, since `system:init` reports the status and nothing else can. if (client.type === 'needs-auth') { - return { client, tools: [createMcpAuthTool(name, scopedConfig as unknown as ScopedMcpServerConfig)] } + // The embedder's session connects its MCP servers once and does not + // re-read the tool pool after a reconnect (v2.ts, self.mcpConnected), + // so tell the model a new session is needed rather than promising + // tools this one will not offer. Reconnecting in place is proposal 2 + // in #2294. + return { + client, + tools: [ + createMcpAuthTool(name, scopedConfig as unknown as ScopedMcpServerConfig, { + realToolsInCurrentContext: false, + }), + ], + } } // Return failed/pending client with no tools diff --git a/src/tools/McpAuthTool/McpAuthTool.ts b/src/tools/McpAuthTool/McpAuthTool.ts index 7ea96795fe..f157e2959e 100644 --- a/src/tools/McpAuthTool/McpAuthTool.ts +++ b/src/tools/McpAuthTool/McpAuthTool.ts @@ -45,19 +45,42 @@ function getConfigUrl(config: ScopedMcpServerConfig): string | undefined { * are swapped into appState.mcp.tools via the existing prefix-based * replacement (useManageMCPConnections.updateServer wipes anything matching * mcp____*, so this pseudo-tool is removed automatically). + * + * That last step only happens where appState feeds the tool pool — the CLI. + * A v2 SDK session connects its MCP servers once and does not re-read the + * pool after a reconnect, so `realToolsInCurrentContext` lets that caller + * tell the model the truth instead of promising tools the session cannot + * offer. See issue #2294. */ +export type McpAuthToolOptions = { + /** + * Whether the host exposes the server's real tools in the current context + * once authorization completes. Defaults to `true` (the CLI behavior). + * Pass `false` from an embedder whose session will not pick them up, so the + * model is told to start a new session rather than promised automatic + * availability. + */ + realToolsInCurrentContext?: boolean +} + export function createMcpAuthTool( serverName: string, config: ScopedMcpServerConfig, + options: McpAuthToolOptions = {}, ): Tool { const url = getConfigUrl(config) const transport = config.type ?? 'stdio' const location = url ? `${transport} at ${url}` : transport + const realToolsInCurrentContext = options.realToolsInCurrentContext ?? true + + const afterAuth = realToolsInCurrentContext + ? `Once the user completes authorization in their browser, the server's real tools will become available automatically.` + : `Once the user completes authorization in their browser, start a new session to use the server's real tools — this one connects its MCP servers once and will not pick them up.` const description = `The \`${serverName}\` MCP server (${location}) is installed but requires authentication. ` + `Call this tool to start the OAuth flow — you'll receive an authorization URL to share with the user. ` + - `Once the user completes authorization in their browser, the server's real tools will become available automatically.` + afterAuth return { name: buildMcpToolName(serverName, 'authenticate'), @@ -184,7 +207,11 @@ export function createMcpAuthTool( data: { status: 'auth_url' as const, authUrl, - message: `Ask the user to open this URL in their browser to authorize the ${serverName} MCP server:\n\n${authUrl}\n\nOnce they complete the flow, the server's tools will become available automatically.`, + message: + `Ask the user to open this URL in their browser to authorize the ${serverName} MCP server:\n\n${authUrl}\n\n` + + (realToolsInCurrentContext + ? `Once they complete the flow, the server's tools will become available automatically.` + : `Once they complete the flow, start a new session to use the server's tools.`), }, } } @@ -192,7 +219,9 @@ export function createMcpAuthTool( return { data: { status: 'auth_url' as const, - message: `Authentication completed silently for ${serverName}. The server's tools should now be available.`, + message: realToolsInCurrentContext + ? `Authentication completed silently for ${serverName}. The server's tools should now be available.` + : `Authentication completed silently for ${serverName}. Start a new session to use the server's tools.`, }, } } catch (err) {