diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 21e3ba775..387655959 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,6 +19,10 @@ - Changed search truncation metadata/renderer output from match/result-based limits to file-based limits (`fileLimitReached`, `perFileLimitReached`) and updated truncation labels accordingly - Lowered `read.defaultLimit` default from `500` to `300` lines, and split the per-range context padding into asymmetric `RANGE_LEADING_CONTEXT_LINES = 1` / `RANGE_TRAILING_CONTEXT_LINES = 3` (was symmetric `RANGE_CONTEXT_LINES = 3`). Replay analysis over post-summarizer sessions (`scripts/session-stats/optimize_read_config.py`) showed that bare-path reads are over-provisioned at the median (file p50 = 220 lines) and that most follow-up reads are disjoint hops rather than adjacent extensions — so a smaller default plus narrower leading context reclaims tokens without measurably changing first-cover rate. Trailing context stays at 3 lines to keep anchor-stale recovery on narrow reads. Explicit `read.defaultLimit` overrides in settings are honoured unchanged. +### Fixed + +- Fixed MCP OAuth refresh failing with `HTTP 401 invalid_client` for servers that require Dynamic Client Registration (RFC 7591) and have no `oauth.clientId` configured (e.g. `mcp.linear.app`). `MCPOAuthFlow` registered a fresh public PKCE client on each authorize and discarded the issued `client_id` once the flow object went out of scope; refresh then called the provider's `/token` endpoint without a `client_id`. The flow now exposes `resolvedClientId` / `registeredClientSecret` getters, `MCPCommandController#handleOAuthFlow` returns them alongside `credentialId`, and both the initial-connect and `/mcp reauth` paths persist them into `auth.{clientId,clientSecret}` (used at refresh) and `oauth.{clientId,clientSecret}` (used by subsequent `/mcp reauth` to skip re-registration). The `MCPAddWizard` `onOAuth` callback type is now `Promise` and `#launchOAuthFlow` folds the registered credentials into wizard state. Servers with a statically-configured `oauth.clientId` (Notion, Slack, Datadog) are unaffected — `#tryRegisterClient` short-circuits and the write-back is a no-op. ([#1061](https://github.com/can1357/oh-my-pi/pull/1061) by [@ldx](https://github.com/ldx)). + ## [15.0.0] - 2026-05-13 ### Breaking Changes diff --git a/packages/coding-agent/src/mcp/oauth-flow.ts b/packages/coding-agent/src/mcp/oauth-flow.ts index ed90f3c27..71ae572d9 100644 --- a/packages/coding-agent/src/mcp/oauth-flow.ts +++ b/packages/coding-agent/src/mcp/oauth-flow.ts @@ -133,6 +133,26 @@ export class MCPOAuthFlow extends OAuthCallbackFlow { this.#resolvedClientId = this.#resolveClientId(config); } + /** + * Client id used during the authorization request. Returns the value supplied + * via {@link MCPOAuthConfig.clientId} or, when the server required dynamic + * client registration, the id issued during registration. `undefined` until + * {@link generateAuthUrl} (or {@link login}) has run for a server that needs + * a client id. + */ + get resolvedClientId(): string | undefined { + return this.#resolvedClientId; + } + + /** + * Client secret issued by dynamic client registration, if any. Always + * `undefined` for PKCE-only/public clients and when the caller supplies the + * client id via config. + */ + get registeredClientSecret(): string | undefined { + return this.#registeredClientSecret; + } + async generateAuthUrl(state: string, redirectUri: string): Promise<{ url: string; instructions?: string }> { if (!this.#resolvedClientId) { await this.#tryRegisterClient(redirectUri); diff --git a/packages/coding-agent/src/modes/components/mcp-add-wizard.ts b/packages/coding-agent/src/modes/components/mcp-add-wizard.ts index 88a3dc199..36087d77e 100644 --- a/packages/coding-agent/src/modes/components/mcp-add-wizard.ts +++ b/packages/coding-agent/src/modes/components/mcp-add-wizard.ts @@ -47,6 +47,18 @@ type WizardStep = | "scope" | "confirm"; +/** + * Result of the wizard's OAuth callback. `credentialId` is mandatory; + * `clientId`/`clientSecret` are populated when the OAuth provider performed + * dynamic client registration (or when the caller pre-supplied them) so the + * wizard can fold them into the final `mcp.json` entry for refresh. + */ +export interface MCPAddWizardOAuthResult { + credentialId: string; + clientId?: string; + clientSecret?: string; +} + interface WizardState { name: string; transport: TransportType | null; @@ -104,7 +116,13 @@ export class MCPAddWizard extends Container { #onCompleteCallback: (name: string, config: MCPServerConfig, scope: Scope) => void; #onCancelCallback: () => void; #onOAuthCallback: - | ((authUrl: string, tokenUrl: string, clientId: string, clientSecret: string, scopes: string) => Promise) + | (( + authUrl: string, + tokenUrl: string, + clientId: string, + clientSecret: string, + scopes: string, + ) => Promise) | null = null; #onTestConnectionCallback: ((config: MCPServerConfig) => Promise) | null = null; #onRenderCallback: (() => void) | null = null; @@ -118,7 +136,7 @@ export class MCPAddWizard extends Container { clientId: string, clientSecret: string, scopes: string, - ) => Promise, + ) => Promise, onTestConnection?: (config: MCPServerConfig) => Promise, onRender?: () => void, initialName?: string, @@ -1120,7 +1138,7 @@ export class MCPAddWizard extends Container { try { // Call OAuth handler - const credentialId = await this.#onOAuthCallback( + const oauthResult = await this.#onOAuthCallback( this.#state.oauthAuthUrl, this.#state.oauthTokenUrl, this.#state.oauthClientId, @@ -1128,8 +1146,11 @@ export class MCPAddWizard extends Container { this.#state.oauthScopes, ); - // Store credential ID - this.#state.oauthCredentialId = credentialId; + // Store credential ID + any dynamically-registered client credentials, + // so the final mcp.json entry persists everything needed for refresh. + this.#state.oauthCredentialId = oauthResult.credentialId; + if (oauthResult.clientId) this.#state.oauthClientId = oauthResult.clientId; + if (oauthResult.clientSecret) this.#state.oauthClientSecret = oauthResult.clientSecret; // Show success message this.#contentContainer.clear(); diff --git a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts index dc28c7248..d6dbe2972 100644 --- a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts @@ -49,6 +49,22 @@ function withTimeout(promise: Promise, timeoutMs: number, message: string) return Promise.race([promise, timeoutPromise]).finally(() => clearTimeout(timer)); } +/** + * Outcome of {@link MCPCommandController}'s OAuth handler. + * + * `clientId`/`clientSecret` are populated when the OAuth provider required (or + * accepted) dynamic client registration; callers MUST persist them alongside + * `credentialId` so subsequent token refreshes and reauthorizations can reuse + * the same registered client. Both are also set when the caller pre-supplied a + * client id via the wizard or `oauth.clientId` in `mcp.json`, in which case the + * write-back is a no-op. + */ +interface OAuthFlowResult { + credentialId: string; + clientId?: string; + clientSecret?: string; +} + type MCPAddScope = "user" | "project"; type MCPAddTransport = "http" | "sse"; @@ -406,7 +422,7 @@ export class MCPCommandController { try { const oauthClientSecret = finalConfig.oauth?.clientSecret ?? ""; - const credentialId = await this.#handleOAuthFlow( + const oauthResult = await this.#handleOAuthFlow( oauth.authorizationUrl, oauth.tokenUrl, oauth.clientId ?? finalConfig.oauth?.clientId ?? "", @@ -416,14 +432,21 @@ export class MCPCommandController { finalConfig.oauth?.callbackPath, finalConfig.oauth?.redirectUri, ); + const persistedClientId = oauthResult.clientId ?? oauth.clientId ?? finalConfig.oauth?.clientId; + const persistedClientSecret = oauthResult.clientSecret ?? finalConfig.oauth?.clientSecret; finalConfig = { ...finalConfig, auth: { type: "oauth", - credentialId, + credentialId: oauthResult.credentialId, tokenUrl: oauth.tokenUrl, - clientId: oauth.clientId ?? finalConfig.oauth?.clientId, - clientSecret: finalConfig.oauth?.clientSecret, + clientId: persistedClientId, + clientSecret: persistedClientSecret, + }, + oauth: { + ...finalConfig.oauth, + clientId: persistedClientId ?? finalConfig.oauth?.clientId, + clientSecret: persistedClientSecret ?? finalConfig.oauth?.clientSecret, }, }; } catch (oauthError) { @@ -488,7 +511,7 @@ export class MCPCommandController { callbackPort?: number, callbackPath?: string, redirectUri?: string, - ): Promise { + ): Promise { const authStorage = this.ctx.session.modelRegistry.authStorage; let parsedAuthUrl: URL; @@ -600,7 +623,11 @@ export class MCPCommandController { // Store under a synthetic provider name await authStorage.set(credentialId, oauthCredential); - return credentialId; + return { + credentialId, + clientId: flow.resolvedClientId, + clientSecret: flow.registeredClientSecret, + }; } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error); @@ -1348,7 +1375,7 @@ export class MCPCommandController { this.#showMessage(["", theme.fg("muted", `Reauthorizing "${name}"...`), ""].join("\n")); - const credentialId = await this.#handleOAuthFlow( + const oauthResult = await this.#handleOAuthFlow( oauth.authorizationUrl, oauth.tokenUrl, oauth.clientId ?? found.config.oauth?.clientId ?? "", @@ -1359,14 +1386,22 @@ export class MCPCommandController { found.config.oauth?.redirectUri, ); + const persistedClientId = oauthResult.clientId ?? oauth.clientId ?? found.config.oauth?.clientId; + const persistedClientSecret = oauthResult.clientSecret ?? (oauthClientSecret || undefined); + const updated: MCPServerConfig = { ...baseConfig, auth: { type: "oauth", - credentialId, + credentialId: oauthResult.credentialId, tokenUrl: oauth.tokenUrl, - clientId: oauth.clientId ?? found.config.oauth?.clientId, - clientSecret: oauthClientSecret || undefined, + clientId: persistedClientId, + clientSecret: persistedClientSecret, + }, + oauth: { + ...found.config.oauth, + clientId: persistedClientId ?? found.config.oauth?.clientId, + clientSecret: persistedClientSecret ?? found.config.oauth?.clientSecret, }, }; await updateMCPServer(found.filePath, name, updated); diff --git a/packages/coding-agent/test/oauth-flow.test.ts b/packages/coding-agent/test/oauth-flow.test.ts index c81afd5ea..6e31daa09 100644 --- a/packages/coding-agent/test/oauth-flow.test.ts +++ b/packages/coding-agent/test/oauth-flow.test.ts @@ -309,4 +309,74 @@ describe("mcp oauth flow", () => { await expect(flow.login()).rejects.toThrow("cannot fall back to a random port when oauth.redirectUri is set"); }); + + it("exposes the dynamically registered client_id and client_secret after generateAuthUrl", async () => { + using _hook = hookFetch(input => { + const url = String(input); + if (url === "https://www.figma.com/.well-known/oauth-authorization-server") { + return new Response( + JSON.stringify({ registration_endpoint: "https://api.figma.com/v1/oauth/mcp/register" }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ); + } + if (url === "https://api.figma.com/v1/oauth/mcp/register") { + return new Response( + JSON.stringify({ + client_id: "registered-client-id", + client_secret: "registered-client-secret", + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ); + } + return new Response("not found", { status: 404 }); + }); + + const flow = new MCPOAuthFlow( + { + authorizationUrl: "https://www.figma.com/oauth/mcp", + tokenUrl: "https://api.figma.com/v1/oauth/token", + }, + {}, + ); + + expect(flow.resolvedClientId).toBeUndefined(); + expect(flow.registeredClientSecret).toBeUndefined(); + + await flow.generateAuthUrl("test-state", "http://127.0.0.1:53173/callback"); + + expect(flow.resolvedClientId).toBe("registered-client-id"); + expect(flow.registeredClientSecret).toBe("registered-client-secret"); + }); + + it("returns the configured client_id from resolvedClientId without triggering registration", async () => { + let registrationCalled = false; + using _hook = hookFetch(input => { + const url = String(input); + if (url.includes("/.well-known/")) { + return new Response("{}", { status: 200, headers: { "Content-Type": "application/json" } }); + } + if (url.endsWith("/register")) { + registrationCalled = true; + } + return new Response("not found", { status: 404 }); + }); + + const flow = new MCPOAuthFlow( + { + authorizationUrl: "https://provider.example/authorize", + tokenUrl: "https://provider.example/token", + clientId: "configured-client-id", + }, + {}, + ); + + expect(flow.resolvedClientId).toBe("configured-client-id"); + expect(flow.registeredClientSecret).toBeUndefined(); + + await flow.generateAuthUrl("test-state", "http://127.0.0.1:53174/callback"); + + expect(flow.resolvedClientId).toBe("configured-client-id"); + expect(flow.registeredClientSecret).toBeUndefined(); + expect(registrationCalled).toBe(false); + }); });