diff --git a/docs/contributing/architecture/mcp-client-servers.md b/docs/contributing/architecture/mcp-client-servers.md index 2216eb2a8d..376d31e6db 100644 --- a/docs/contributing/architecture/mcp-client-servers.md +++ b/docs/contributing/architecture/mcp-client-servers.md @@ -42,9 +42,16 @@ the `/mcp` endpoint (where Kody is the server) and complements remote connectors the browser session cookie, forwards the full callback URL to that user's hub DO, and the SDK exchanges the code (matching the `state` parameter to the pending authorization) and establishes the connection. -5. The route redirects to `/account/mcp-servers?auth=success|error` for user - feedback. Tokens live only in the DO storage; they never reach D1 or the - client. +5. The hub only treats the callback as successful when the connection reaches + `ready`. If the Agents SDK reports `authSuccess` but the connection stays in + `authenticating` (including the stuck case with no stored auth URL after the + SDK clears it), the route redirects with `auth=error` and a concrete reason. + Reconnect also recovers that stuck state by invalidating unusable tokens and + requesting a fresh authorization URL. +6. The route redirects to `/account/mcp-servers/:serverId?auth=success|error` + when the callback resolves to a server (including failures), or + `/account/mcp-servers?auth=error` when it does not, for user feedback. Tokens + live only in the DO storage; they never reach D1 or the client. Because the callback is resolved through the session cookie, the OAuth state is always looked up in the hub belonging to the signed-in user — cross-user diff --git a/packages/worker/client/routes/account-mcp-servers.tsx b/packages/worker/client/routes/account-mcp-servers.tsx index 8c9c2e7976..3b70ef1d7c 100644 --- a/packages/worker/client/routes/account-mcp-servers.tsx +++ b/packages/worker/client/routes/account-mcp-servers.tsx @@ -631,6 +631,14 @@ export function AccountMcpServersRoute(handle: Handle) { ) : null} + {server.state === 'authenticating' && !server.authUrl ? ( + + Authorization is incomplete and no authorization link is + available. Click Reconnect to restart OAuth, or remove and + add the server again. + + ) : null} + {server.authUrl && server.state === 'authenticating' ? (
{ } as never) expect(failureResponse.status).toBe(303) const failureLocation = new URL(failureResponse.headers.get('Location') ?? '') + expect(failureLocation.pathname).toBe('/account/mcp-servers') expect(failureLocation.searchParams.get('auth')).toBe('error') expect(failureLocation.searchParams.get('reason')).toBe('Invalid state.') + + mockModule.handleOAuthCallback.mockResolvedValueOnce({ + serverId: 'server-1', + authSuccess: false, + authError: 'Authorization completed, but no authorization link.', + serverName: 'linear', + }) + const incompleteResponse = await handler.handler({ + request: new Request( + 'https://example.com/account/mcp-servers/oauth/callback?code=abc&state=xyz.server-1', + ), + params: {}, + } as never) + expect(incompleteResponse.status).toBe(303) + const incompleteLocation = new URL( + incompleteResponse.headers.get('Location') ?? '', + ) + expect(incompleteLocation.pathname).toBe('/account/mcp-servers/server-1') + expect(incompleteLocation.searchParams.get('auth')).toBe('error') + expect(incompleteLocation.searchParams.get('reason')).toContain( + 'no authorization link', + ) }) diff --git a/packages/worker/src/app/handlers/account-mcp-servers.ts b/packages/worker/src/app/handlers/account-mcp-servers.ts index 6f6a76de5b..9ffaeb92d7 100644 --- a/packages/worker/src/app/handlers/account-mcp-servers.ts +++ b/packages/worker/src/app/handlers/account-mcp-servers.ts @@ -143,13 +143,12 @@ export function createAccountMcpServersOauthCallbackHandler(env: Env) { authError = getErrorMessage(error) } - const target = - authSuccess && serverId - ? new URL( - `/account/mcp-servers/${encodeURIComponent(serverId)}`, - request.url, - ) - : new URL('/account/mcp-servers', request.url) + const target = serverId + ? new URL( + `/account/mcp-servers/${encodeURIComponent(serverId)}`, + request.url, + ) + : new URL('/account/mcp-servers', request.url) if (authSuccess) { target.searchParams.set('auth', 'success') if (serverName) { diff --git a/packages/worker/src/mcp-client/hub.ts b/packages/worker/src/mcp-client/hub.ts index 6ad18a4a0d..1555f5eed9 100644 --- a/packages/worker/src/mcp-client/hub.ts +++ b/packages/worker/src/mcp-client/hub.ts @@ -4,6 +4,10 @@ import { MCPClientManager } from 'agents/mcp/client' import { DurableObjectOAuthClientProvider } from 'agents/mcp/do-oauth-client-provider' import { type CallToolResult } from '@modelcontextprotocol/sdk/types.js' import { buildSentryOptions } from '#worker/sentry-options.ts' +import { + isStuckMcpAuthenticatingWithoutAuthUrl, + resolveMcpOAuthCallbackOutcome, +} from './oauth-callback-outcome.ts' import { type McpClientHubSnapshot, type McpServerConnectResult, @@ -182,7 +186,11 @@ class McpClientHubBase extends DurableObject { }) } } - return this.buildConnectResult(input.serverId) + const connected = this.buildConnectResult(input.serverId) + if (isStuckMcpAuthenticatingWithoutAuthUrl(connected)) { + return await this.recoverStuckAuthenticating(input.serverId) + } + return connected } /** Re-discover tools for a connected server. */ @@ -208,6 +216,11 @@ class McpClientHubBase extends DurableObject { * Complete an OAuth authorization redirect. The worker forwards the * callback URL (including `code` and `state`) after authenticating the * browser session that owns this hub. + * + * Success is reported only when the MCP connection reaches `ready`. The + * Agents SDK `authSuccess` flag alone is not enough: it can clear the + * stored auth URL and leave the connection in `authenticating` with no + * error after a provider (e.g. Clerk) authorization redirect. */ async handleOAuthCallback(input: { url: string @@ -233,13 +246,54 @@ class McpClientHubBase extends DurableObject { await this.manager.waitForConnections({ timeout: connectionSettleTimeoutMs, }) + let connection = this.buildConnectResult(serverId) + if (isStuckMcpAuthenticatingWithoutAuthUrl(connection)) { + connection = await this.recoverStuckAuthenticating(serverId) + } + return resolveMcpOAuthCallbackOutcome({ + sdkAuthSuccess: true, + sdkAuthError: result.authError ?? null, + serverId, + serverName, + connection, + }) } - return { + return resolveMcpOAuthCallbackOutcome({ + sdkAuthSuccess: result.authSuccess, + sdkAuthError: result.authError ?? null, serverId, - authSuccess: result.authSuccess, - authError: result.authError ?? null, serverName, + connection: serverId ? this.buildConnectResult(serverId) : null, + }) + } + + /** + * Clear unusable tokens and reconnect so the Agents SDK can mint a fresh + * auth URL. Needed when SQL `auth_url` was cleared on callback success but + * the live connection never reached `ready`. + */ + private async recoverStuckAuthenticating( + serverId: string, + ): Promise { + const connection = this.manager.mcpConnections[serverId] + const authProvider = connection?.options.transport.authProvider + if ( + authProvider && + typeof authProvider.invalidateCredentials === 'function' + ) { + try { + await authProvider.invalidateCredentials('tokens') + } catch { + // Best-effort: reconnect below still regenerates OAuth when possible. + } + } + const result = await this.manager.connectToServer(serverId) + if (result.state === 'connected') { + await this.manager.discoverIfConnected(serverId, { + timeoutMs: discoverTimeoutMs, + }) } + return this.buildConnectResult(serverId) } async getSnapshot(): Promise { diff --git a/packages/worker/src/mcp-client/oauth-callback-outcome.node.test.ts b/packages/worker/src/mcp-client/oauth-callback-outcome.node.test.ts new file mode 100644 index 0000000000..5e03c0cdfa --- /dev/null +++ b/packages/worker/src/mcp-client/oauth-callback-outcome.node.test.ts @@ -0,0 +1,115 @@ +import { expect, test } from 'vitest' +import { + describeIncompleteMcpOAuthConnection, + isStuckMcpAuthenticatingWithoutAuthUrl, + resolveMcpOAuthCallbackOutcome, +} from './oauth-callback-outcome.ts' + +test('OAuth callback success requires the MCP connection to be ready', () => { + expect( + resolveMcpOAuthCallbackOutcome({ + sdkAuthSuccess: true, + sdkAuthError: null, + serverId: 'server-1', + serverName: 'recipe-keeper', + connection: { + state: 'ready', + authUrl: null, + error: null, + }, + }), + ).toEqual({ + serverId: 'server-1', + authSuccess: true, + authError: null, + serverName: 'recipe-keeper', + }) +}) + +test('OAuth callback does not report success when left authenticating without authUrl', () => { + const outcome = resolveMcpOAuthCallbackOutcome({ + sdkAuthSuccess: true, + sdkAuthError: null, + serverId: 'server-1', + serverName: 'recipe-keeper', + connection: { + state: 'authenticating', + authUrl: null, + error: null, + }, + }) + + expect(outcome.authSuccess).toBe(false) + expect(outcome.serverId).toBe('server-1') + expect(outcome.serverName).toBe('recipe-keeper') + expect(outcome.authError).toContain('no authorization link') + expect( + isStuckMcpAuthenticatingWithoutAuthUrl({ + state: 'authenticating', + authUrl: null, + }), + ).toBe(true) +}) + +test('OAuth callback surfaces connection errors after SDK authSuccess', () => { + expect( + resolveMcpOAuthCallbackOutcome({ + sdkAuthSuccess: true, + sdkAuthError: null, + serverId: 'server-1', + serverName: 'recipe-keeper', + connection: { + state: 'failed', + authUrl: null, + error: 'Token exchange failed.', + }, + }), + ).toEqual({ + serverId: 'server-1', + authSuccess: false, + authError: 'Token exchange failed.', + serverName: 'recipe-keeper', + }) +}) + +test('OAuth callback keeps SDK failures as failures', () => { + expect( + resolveMcpOAuthCallbackOutcome({ + sdkAuthSuccess: false, + sdkAuthError: 'Invalid state', + serverId: 'server-1', + serverName: 'recipe-keeper', + connection: { + state: 'authenticating', + authUrl: 'https://auth.example/authorize', + error: null, + }, + }), + ).toEqual({ + serverId: 'server-1', + authSuccess: false, + authError: 'Invalid state', + serverName: 'recipe-keeper', + }) +}) + +test('incomplete OAuth connection messages distinguish missing auth URLs', () => { + expect( + describeIncompleteMcpOAuthConnection({ + state: 'authenticating', + authUrl: 'https://auth.example/authorize', + }), + ).toContain('still requires authorization') + expect( + describeIncompleteMcpOAuthConnection({ + state: 'authenticating', + authUrl: null, + }), + ).toContain('no authorization link') + expect( + describeIncompleteMcpOAuthConnection({ + state: 'connecting', + authUrl: null, + }), + ).toContain('"connecting"') +}) diff --git a/packages/worker/src/mcp-client/oauth-callback-outcome.ts b/packages/worker/src/mcp-client/oauth-callback-outcome.ts new file mode 100644 index 0000000000..5063c661e1 --- /dev/null +++ b/packages/worker/src/mcp-client/oauth-callback-outcome.ts @@ -0,0 +1,91 @@ +import { + type McpServerConnectionState, + type McpServerOAuthCallbackOutcome, +} from './types.ts' + +/** + * Decide the user-facing OAuth callback outcome from the Agents SDK result + * plus the post-establish connection snapshot. + * + * The SDK's `authSuccess` only means the authorization code was accepted (or + * treated as already accepted). Kody must not report success until the MCP + * connection is actually `ready`. Otherwise users can land on + * "Authorization required" with no auth URL and no error — the Clerk / Convex + * MCP failure mode reported by Bernardo. + */ +export function resolveMcpOAuthCallbackOutcome(input: { + sdkAuthSuccess: boolean + sdkAuthError: string | null + serverId: string | null + serverName: string | null + connection: { + state: McpServerConnectionState + authUrl: string | null + error: string | null + } | null +}): McpServerOAuthCallbackOutcome { + const { serverId, serverName } = input + + if (!input.sdkAuthSuccess || !serverId) { + return { + serverId, + authSuccess: false, + authError: input.sdkAuthError ?? 'Authorization failed.', + serverName, + } + } + + if (!input.connection) { + return { + serverId, + authSuccess: false, + authError: + 'Authorization completed, but Kody lost the MCP server connection. Try reconnecting from /account/mcp-servers.', + serverName, + } + } + + if (input.connection.state === 'ready') { + return { + serverId, + authSuccess: true, + authError: null, + serverName, + } + } + + return { + serverId, + authSuccess: false, + authError: + input.connection.error ?? + describeIncompleteMcpOAuthConnection(input.connection), + serverName, + } +} + +export function describeIncompleteMcpOAuthConnection(connection: { + state: McpServerConnectionState + authUrl: string | null +}): string { + if (connection.state === 'authenticating' && connection.authUrl) { + return 'Authorization completed at the identity provider, but the MCP server still requires authorization. Open the authorization link again from /account/mcp-servers.' + } + if (connection.state === 'authenticating') { + return 'Authorization completed at the identity provider, but Kody could not finish connecting and has no authorization link to offer. Reconnect the server from /account/mcp-servers, or remove and add it again.' + } + return `Authorization completed at the identity provider, but the MCP server is still "${connection.state}". Reconnect it from /account/mcp-servers.` +} + +/** + * Agents SDK quirk: `connectToServer` can leave `connectionState` as + * `authenticating` while returning FAILED when `authUrl` is missing, and + * `oauthCallbackSuccess` clears the stored auth URL. Detect that stuck state + * so callers can recover before surfacing success/error to the user. + */ +export function isStuckMcpAuthenticatingWithoutAuthUrl(connection: { + state: McpServerConnectionState + authUrl: string | null +}): boolean { + return connection.state === 'authenticating' && !connection.authUrl +}