diff --git a/src/entrypoints/sdk/mcpNeedsAuth.test.ts b/src/entrypoints/sdk/mcpNeedsAuth.test.ts new file mode 100644 index 0000000000..42789a5e1b --- /dev/null +++ b/src/entrypoints/sdk/mcpNeedsAuth.test.ts @@ -0,0 +1,164 @@ +/** + * 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') + // 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) + +/** + * 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..4213040dea 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,28 @@ 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') { + // 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 return { client, tools: [] } } catch (error) { 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) {