From acef3e614e4e834bb1c5da378444a54bd04462c6 Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" <207016121+credence-the-bot[bot]@users.noreply.github.com> Date: Tue, 5 May 2026 22:48:17 +0000 Subject: [PATCH 1/4] fix(oauth): drop in-process loopback fallback from `assistant oauth connect` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI's BYO OAuth connect path used to fall through to an in-process `orchestrateOAuthConnect` invocation when the daemon IPC was unreachable. That fallback was buggy and dead code in practice: - It re-introduced the heap-split bug for `--callback-transport=gateway` (platform redirect lands in a different process from the loopback waiter — see #29596). - The OAuth tokens it would acquire need the daemon running before they're usable, so "daemon unreachable" is already a fatal precondition for the user's flow. Now we surface a clear "Is the assistant running?" error and exit 1 when the daemon is unreachable, matching the MCP CLI consolidation in #29484. The daemon-orchestrated IPC path remains the canonical implementation; nothing changes when the daemon is up. --- assistant/src/cli/commands/oauth/connect.ts | 68 +++++---------------- 1 file changed, 16 insertions(+), 52 deletions(-) diff --git a/assistant/src/cli/commands/oauth/connect.ts b/assistant/src/cli/commands/oauth/connect.ts index 2028c3d3f6a..b3e4890ce4a 100644 --- a/assistant/src/cli/commands/oauth/connect.ts +++ b/assistant/src/cli/commands/oauth/connect.ts @@ -4,7 +4,6 @@ import type { Command } from "commander"; import { getIsContainerized } from "../../../config/env-registry.js"; import { cliIpcCall } from "../../../ipc/cli-client.js"; -import { orchestrateOAuthConnect } from "../../../oauth/connect-orchestrator.js"; import { getAppByProviderAndClientId, getMostRecentAppByProvider, @@ -514,57 +513,22 @@ Examples: return; } - // IPC unavailable (daemon unreachable, older daemon without this route, socket missing). - // Fall through to the existing in-process flow. This still carries the heap-split bug - // for gateway transport, but if the daemon is unreachable we have a worse problem; - // the fallback preserves existing behavior as a regression guard. - // e. Call the orchestrator (in-process fallback) - const result = await orchestrateOAuthConnect({ - service: provider, - clientId, - clientSecret, - callbackTransport: opts.callbackTransport, - isInteractive: opts.browser !== false, - openUrl: opts.browser !== false ? openInHostBrowser : undefined, - ...(opts.scopes ? { requestedScopes: opts.scopes } : {}), - }); - - // f. Handle results - if (!result.success) { - writeError(result.error ?? "OAuth connect failed"); - return; - } - - if (result.deferred) { - if (jsonMode) { - writeOutput(cmd, { - ok: true, - deferred: true, - // Wire key stays `authUrl` for backward compatibility with - // existing CLI script consumers; the internal field on - // `result` is `authorizeUrl`. - authUrl: result.authorizeUrl, - service: result.service, - }); - } else { - process.stdout.write( - `\nAuthorize with ${provider}:\n\n${result.authorizeUrl}\n\nThe connection will complete automatically once you authorize.\n`, - ); - } - return; - } - - // Interactive mode completed - if (jsonMode) { - writeOutput(cmd, { - ok: true, - grantedScopes: result.grantedScopes, - accountInfo: result.accountInfo, - }); - } else { - const msg = `Connected to ${provider}${result.accountInfo ? ` as ${result.accountInfo}` : ""}`; - process.stdout.write(msg + "\n"); - } + // IPC unavailable (daemon unreachable, older daemon without this route, socket + // missing). Surface a clear error and exit 1. + // + // We previously fell through to an in-process invocation of + // `orchestrateOAuthConnect`, but that fallback re-introduced the heap-split bug + // for `--callback-transport=gateway` (the platform redirect would land in a + // different process from the loopback waiter — see #29596). The fallback is also + // dead code in practice: the OAuth tokens it would acquire need the daemon to be + // running before they're useful, so "daemon unreachable" is already a fatal + // precondition. Mirrors the MCP CLI consolidation in #29484. + writeError( + startResult.error + ? `Could not reach the assistant daemon: ${startResult.error}. Is the assistant running?` + : "Could not reach the assistant daemon. Is the assistant running?", + ); + return; } } catch (err) { const message = err instanceof Error ? err.message : String(err); From ddf1251e746945bd813f2a0ad21804c421585f3d Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" <207016121+credence-the-bot[bot]@users.noreply.github.com> Date: Tue, 5 May 2026 22:51:22 +0000 Subject: [PATCH 2/4] test(oauth-cli): add daemon-unreachable regression coverage for BYO connect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three new tests in `oauth-cli.test.ts` lock in the post-cleanup invariant that the BYO `oauth connect` flow MUST exit 1 when the daemon is unreachable, never silently fall back to in-process orchestration: 1. ECONNREFUSED → exit 1, error mentions 'Is the assistant running?' 2. 'Unknown method' → exit 1; `orchestrateOAuthConnect` is NOT called 3. Daemon HTTP error (statusCode set) → surfaces error verbatim Tests (2) and (3) explicitly count invocations of the in-process orchestrator mock and assert zero calls — this is the same regression guard pattern PR #29484 used for the MCP CLI consolidation. --- assistant/src/__tests__/oauth-cli.test.ts | 121 ++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/assistant/src/__tests__/oauth-cli.test.ts b/assistant/src/__tests__/oauth-cli.test.ts index f1e89988102..27e497c3720 100644 --- a/assistant/src/__tests__/oauth-cli.test.ts +++ b/assistant/src/__tests__/oauth-cli.test.ts @@ -62,6 +62,13 @@ let mockUpsertAppImpl: let mockOrchestrateOAuthConnect: ( opts: Record, ) => Promise>; +let mockCliIpcCall: ( + operationId: string, + params?: Record, +) => Promise> = async () => ({ + ok: false, + error: "Could not connect to assistant daemon (test default)", +}); let mockGetAppByProviderAndClientId: ( provider: string, clientId: string, @@ -335,6 +342,15 @@ mock.module("../oauth/connect-orchestrator.js", () => ({ mockOrchestrateOAuthConnect(opts), })); +// --------------------------------------------------------------------------- +// Mock cli-client (IPC) — used by `oauth connect` for daemon-orchestrated flow +// --------------------------------------------------------------------------- + +mock.module("../ipc/cli-client.js", () => ({ + cliIpcCall: (operationId: string, params?: Record) => + mockCliIpcCall(operationId, params), +})); + mock.module("../oauth/seed-providers.js", () => ({ SEEDED_PROVIDER_KEYS: new Set([ "google", @@ -1220,6 +1236,111 @@ describe("assistant oauth connect managed mode — platform 401/403 errors", () }); }); +// --------------------------------------------------------------------------- +// `assistant oauth connect ` BYO mode — daemon-unreachable behavior. +// +// We deleted the in-process `orchestrateOAuthConnect` fallback (the same +// pattern as the MCP CLI consolidation in #29484). When the daemon is +// unreachable, the CLI must surface a clear error and exit 1 — never +// silently fall back to in-process flow. +// --------------------------------------------------------------------------- + +describe("assistant oauth connect — daemon unreachable (BYO mode)", () => { + beforeEach(() => { + // BYO provider with a registered app and no managed-mode wiring. + mockGetProvider = () => ({ + provider: "github", + authorizeUrl: "https://github.com/login/oauth/authorize", + tokenExchangeUrl: "https://github.com/login/oauth/access_token", + defaultScopes: "[]", + availableScopes: null, + authorizeParams: null, + managedServiceConfigKey: null, + requiresClientSecret: false, + createdAt: Date.now(), + updatedAt: Date.now(), + }); + mockGetMostRecentAppByProvider = () => ({ + provider: "github", + clientId: "test-client-id", + clientSecretCredentialPath: "oauth_app/github/test/client_secret", + }); + mockGetSecureKey = () => "test-secret"; + mockGetConfig = () => ({ services: {} }); + }); + + test("daemon connect-refused → exit 1 with 'Is the assistant running?'", async () => { + mockCliIpcCall = async () => ({ + ok: false, + error: "Could not connect to assistant daemon: ECONNREFUSED", + }); + + const { exitCode, stdout } = await runCli([ + "connect", + "github", + "--no-browser", + "--json", + ]); + + expect(exitCode).toBe(1); + const parsed = JSON.parse(stdout); + expect(parsed.ok).toBe(false); + expect(parsed.error).toContain("Could not reach the assistant daemon"); + expect(parsed.error).toContain("Is the assistant running?"); + }); + + test("daemon route missing (Unknown method) → exit 1, never falls through to in-process", async () => { + let orchestratorCalls = 0; + mockOrchestrateOAuthConnect = async () => { + orchestratorCalls++; + return { success: true, deferred: false, grantedScopes: [] }; + }; + mockCliIpcCall = async () => ({ + ok: false, + error: "Unknown method: internal_oauth_connect_start", + }); + + const { exitCode } = await runCli([ + "connect", + "github", + "--no-browser", + "--json", + ]); + + expect(exitCode).toBe(1); + // Critical regression guard: the in-process `orchestrateOAuthConnect` + // must NOT be invoked from the CLI. The daemon-orchestrated path is + // the sole code path; this is the same invariant #29484 established + // for the MCP CLI. + expect(orchestratorCalls).toBe(0); + }); + + test("daemon HTTP error (statusCode set) → surfaces error verbatim, no fallback", async () => { + let orchestratorCalls = 0; + mockOrchestrateOAuthConnect = async () => { + orchestratorCalls++; + return { success: true, deferred: false, grantedScopes: [] }; + }; + mockCliIpcCall = async () => ({ + ok: false, + statusCode: 400, + error: "service must be registered before connecting", + }); + + const { exitCode, stdout } = await runCli([ + "connect", + "github", + "--no-browser", + "--json", + ]); + + expect(exitCode).toBe(1); + const parsed = JSON.parse(stdout); + expect(parsed.error).toContain("service must be registered"); + expect(orchestratorCalls).toBe(0); + }); +}); + // --------------------------------------------------------------------------- // requirePlatformClient — improved error messages // --------------------------------------------------------------------------- From 54dda7f703edf55be1010b183be2aacf86bd9b8c Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" Date: Wed, 6 May 2026 00:26:45 +0000 Subject: [PATCH 3/4] fix(oauth-cli): address review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Codex P1s + Devin findings on PR #29756: 1. **Use 'assistant' instead of 'daemon' in user-facing CLI errors.** Per assistant/AGENTS.md, 'daemon' is an internal implementation detail and CLI output must use 'assistant' for user-facing language. The new error string in connect.ts violated this. 2. **Drop historical-narrative comment block.** AGENTS.md prohibits comments that reference removed code or prior implementations ('We previously...'). Replaced the block with a description of current behavior only — why an unreachable assistant is a fatal precondition, with no reference to the removed loopback fallback or PR history. 3. **Delete 6 tests that pinned the removed in-process fallback.** These tests mocked `mockOrchestrateOAuthConnect` and asserted the orchestrator was called or its result surfaced. With the loopback fallback removed, every CLI path now goes through IPC, so those tests assert removed behavior. The IPC-first path tests (added in the PR's first commit) cover every JSON shape and error case the deleted tests asserted on: - 'BYO mode --no-browser prints auth URL' → 'IPC start + --no-browser without json → prints URL' - 'BYO default calls orchestrator with isInteractive: true' → deleted; orchestrator no longer in CLI heap - 'JSON output for deferred case' → 'IPC start + --no-browser + json → returns deferred JSON' - 'JSON output for completed case' → 'IPC start succeeds + polling returns complete' - 'IPC ok:false → falls back to in-process orchestrator' → opposite is now true; covered by 'IPC ok:false with statusCode → does NOT fall back' - 'orchestrator error propagates correctly' → 'IPC poll returns ok:false with statusCode → breaks early' 18 tests pass (down from 18 surviving + 6 broken = 24 total). --- .../commands/oauth/__tests__/connect.test.ts | 254 ------------------ assistant/src/cli/commands/oauth/connect.ts | 18 +- 2 files changed, 6 insertions(+), 266 deletions(-) diff --git a/assistant/src/cli/commands/oauth/__tests__/connect.test.ts b/assistant/src/cli/commands/oauth/__tests__/connect.test.ts index c535f4fc3f7..0776642e27b 100644 --- a/assistant/src/cli/commands/oauth/__tests__/connect.test.ts +++ b/assistant/src/cli/commands/oauth/__tests__/connect.test.ts @@ -395,106 +395,6 @@ describe("assistant oauth connect", () => { ); }); - // ------------------------------------------------------------------------- - // BYO mode with --no-browser: prints auth URL (deferred) - // ------------------------------------------------------------------------- - - test("BYO mode with --no-browser: prints auth URL", async () => { - mockGetProvider = () => ({ - provider: "google", - authorizeUrl: "https://accounts.google.com/o/oauth2/v2/auth", - tokenExchangeUrl: "https://oauth2.googleapis.com/token", - tokenExchangeBodyFormat: "form", - managedServiceConfigKey: null, - }); - mockIsManagedMode = () => false; - - mockGetMostRecentAppByProvider = () => ({ - id: "app-1", - clientId: "byo-client-id", - clientSecretCredentialPath: "oauth_app/app-1/client_secret", - provider: "google", - createdAt: 0, - updatedAt: 0, - }); - - mockOrchestrateOAuthConnect = async () => ({ - success: true, - deferred: true, - authorizeUrl: "https://accounts.google.com/o/oauth2/v2/auth?state=abc", - state: "abc", - service: "google", - }); - - const { exitCode, stdout } = await runCommand([ - "connect", - "google", - "--no-browser", - "--json", - ]); - expect(exitCode).toBe(0); - const parsed = JSON.parse(stdout); - expect(parsed.ok).toBe(true); - expect(parsed.deferred).toBe(true); - expect(parsed.authUrl).toBe( - "https://accounts.google.com/o/oauth2/v2/auth?state=abc", - ); - expect(parsed.service).toBe("google"); - }); - - // ------------------------------------------------------------------------- - // BYO mode default: orchestrator called with isInteractive true - // ------------------------------------------------------------------------- - - test("BYO mode default calls orchestrator with isInteractive: true", async () => { - mockGetProvider = () => ({ - provider: "google", - authorizeUrl: "https://accounts.google.com/o/oauth2/v2/auth", - tokenExchangeUrl: "https://oauth2.googleapis.com/token", - tokenExchangeBodyFormat: "form", - managedServiceConfigKey: null, - }); - mockIsManagedMode = () => false; - - mockGetAppByProviderAndClientId = () => ({ - id: "app-1", - clientId: "test-id", - clientSecretCredentialPath: "oauth_app/app-1/client_secret", - provider: "google", - createdAt: 0, - updatedAt: 0, - }); - - let capturedOpts: Record | undefined; - mockOrchestrateOAuthConnect = async (opts) => { - capturedOpts = opts; - return { - success: true, - deferred: false, - grantedScopes: ["email"], - accountInfo: "user@example.com", - }; - }; - - const { exitCode, stdout } = await runCommand([ - "connect", - "google", - "--client-id", - "test-id", - "--json", - ]); - expect(exitCode).toBe(0); - expect(capturedOpts).toBeDefined(); - expect(capturedOpts!.isInteractive).toBe(true); - // openUrl should be provided by default (browser opens automatically) - expect(typeof capturedOpts!.openUrl).toBe("function"); - - const parsed = JSON.parse(stdout); - expect(parsed.ok).toBe(true); - expect(parsed.grantedScopes).toEqual(["email"]); - expect(parsed.accountInfo).toBe("user@example.com"); - }); - // ------------------------------------------------------------------------- // BYO missing app: error with hint // ------------------------------------------------------------------------- @@ -590,96 +490,6 @@ describe("assistant oauth connect", () => { ); }); - // ------------------------------------------------------------------------- - // JSON output format for deferred case (BYO) - // ------------------------------------------------------------------------- - - test("JSON output for deferred case includes ok, deferred, authUrl, service", async () => { - mockGetProvider = () => ({ - provider: "slack", - authorizeUrl: "https://slack.com/oauth/v2/authorize", - tokenExchangeUrl: "https://slack.com/api/oauth.v2.access", - tokenExchangeBodyFormat: "form", - managedServiceConfigKey: null, - }); - mockIsManagedMode = () => false; - - mockGetMostRecentAppByProvider = () => ({ - id: "app-slack", - clientId: "slack-client-id", - clientSecretCredentialPath: "oauth_app/app-slack/client_secret", - provider: "slack", - createdAt: 0, - updatedAt: 0, - }); - - mockOrchestrateOAuthConnect = async () => ({ - success: true, - deferred: true, - authorizeUrl: "https://slack.com/oauth/v2/authorize?state=xyz", - state: "xyz", - service: "slack", - }); - - const { exitCode, stdout } = await runCommand([ - "connect", - "slack", - "--no-browser", - "--json", - ]); - expect(exitCode).toBe(0); - const parsed = JSON.parse(stdout); - expect(parsed).toHaveProperty("ok", true); - expect(parsed).toHaveProperty("deferred", true); - expect(parsed).toHaveProperty("authUrl"); - expect(parsed).toHaveProperty("service", "slack"); - }); - - // ------------------------------------------------------------------------- - // JSON output format for completed case (BYO) - // ------------------------------------------------------------------------- - - test("JSON output for completed case includes ok, grantedScopes, accountInfo", async () => { - mockGetProvider = () => ({ - provider: "google", - authorizeUrl: "https://accounts.google.com/o/oauth2/v2/auth", - tokenExchangeUrl: "https://oauth2.googleapis.com/token", - tokenExchangeBodyFormat: "form", - managedServiceConfigKey: null, - }); - mockIsManagedMode = () => false; - - mockGetMostRecentAppByProvider = () => ({ - id: "app-1", - clientId: "completed-client-id", - clientSecretCredentialPath: "oauth_app/app-1/client_secret", - provider: "google", - createdAt: 0, - updatedAt: 0, - }); - - mockOrchestrateOAuthConnect = async () => ({ - success: true, - deferred: false, - grantedScopes: ["email", "profile"], - accountInfo: "test@gmail.com", - }); - - const { exitCode, stdout } = await runCommand([ - "connect", - "google", - "--json", - ]); - expect(exitCode).toBe(0); - const parsed = JSON.parse(stdout); - expect(parsed).toHaveProperty("ok", true); - expect(parsed).toHaveProperty("grantedScopes"); - expect(parsed.grantedScopes).toEqual(["email", "profile"]); - expect(parsed).toHaveProperty("accountInfo", "test@gmail.com"); - // Should NOT have deferred - expect(parsed.deferred).toBeUndefined(); - }); - // ------------------------------------------------------------------------- // BYO mode: client_secret required but missing // ------------------------------------------------------------------------- @@ -911,31 +721,6 @@ describe("assistant oauth connect", () => { expect(mockOpenInBrowserCalls.length).toBe(0); }); - test("IPC returns ok:false → falls back to in-process orchestrateOAuthConnect", async () => { - // Default mockCliIpcCallFn already returns ok: false - let orchestratorCalled = false; - mockOrchestrateOAuthConnect = async () => { - orchestratorCalled = true; - return { - success: true, - deferred: false, - grantedScopes: ["email"], - accountInfo: "fallback@example.com", - }; - }; - - const { exitCode, stdout } = await runCommand([ - "connect", - "google", - "--json", - ]); - expect(exitCode).toBe(0); - expect(orchestratorCalled).toBe(true); - const parsed = JSON.parse(stdout); - expect(parsed.ok).toBe(true); - expect(parsed.accountInfo).toBe("fallback@example.com"); - }); - test("IPC returns ok:false with statusCode → surfaces daemon error, does NOT fall back", async () => { // Daemon was reachable but returned an error (e.g. 500) mockCliIpcCallFn = async (method) => { @@ -1159,43 +944,4 @@ describe("assistant oauth connect", () => { expect(parsed.accountInfo).toBe("gw-user@example.com"); }); }); - - // ------------------------------------------------------------------------- - // Orchestrator error propagation - // ------------------------------------------------------------------------- - - test("BYO mode: orchestrator error propagates correctly", async () => { - mockGetProvider = () => ({ - provider: "google", - authorizeUrl: "https://accounts.google.com/o/oauth2/v2/auth", - tokenExchangeUrl: "https://oauth2.googleapis.com/token", - tokenExchangeBodyFormat: "form", - managedServiceConfigKey: null, - }); - mockIsManagedMode = () => false; - - mockGetMostRecentAppByProvider = () => ({ - id: "app-1", - clientId: "client-id", - clientSecretCredentialPath: "oauth_app/app-1/client_secret", - provider: "google", - createdAt: 0, - updatedAt: 0, - }); - - mockOrchestrateOAuthConnect = async () => ({ - success: false, - error: "Token exchange failed: invalid_grant", - }); - - const { exitCode, stdout } = await runCommand([ - "connect", - "google", - "--json", - ]); - expect(exitCode).toBe(1); - const parsed = JSON.parse(stdout); - expect(parsed.ok).toBe(false); - expect(parsed.error).toBe("Token exchange failed: invalid_grant"); - }); }); diff --git a/assistant/src/cli/commands/oauth/connect.ts b/assistant/src/cli/commands/oauth/connect.ts index b3e4890ce4a..755fb26c4b2 100644 --- a/assistant/src/cli/commands/oauth/connect.ts +++ b/assistant/src/cli/commands/oauth/connect.ts @@ -513,20 +513,14 @@ Examples: return; } - // IPC unavailable (daemon unreachable, older daemon without this route, socket - // missing). Surface a clear error and exit 1. - // - // We previously fell through to an in-process invocation of - // `orchestrateOAuthConnect`, but that fallback re-introduced the heap-split bug - // for `--callback-transport=gateway` (the platform redirect would land in a - // different process from the loopback waiter — see #29596). The fallback is also - // dead code in practice: the OAuth tokens it would acquire need the daemon to be - // running before they're useful, so "daemon unreachable" is already a fatal - // precondition. Mirrors the MCP CLI consolidation in #29484. + // IPC unavailable: the assistant must be running for OAuth connect. The + // gateway-routed callback lands in the assistant's process, and any tokens + // acquired need the assistant to store and use them — so an unreachable + // assistant is a fatal precondition. Surface a clear error and exit 1. writeError( startResult.error - ? `Could not reach the assistant daemon: ${startResult.error}. Is the assistant running?` - : "Could not reach the assistant daemon. Is the assistant running?", + ? `Could not reach the assistant: ${startResult.error}. Is the assistant running?` + : "Could not reach the assistant. Is the assistant running?", ); return; } From c2dc087063d5630a5752988cf2f29c4f7167abeb Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" Date: Wed, 6 May 2026 10:24:33 +0000 Subject: [PATCH 4/4] fix(test): update oauth-cli.test.ts assertion to new error wording Sibling test file at assistant/src/__tests__/oauth-cli.test.ts:1288 still asserted on the previous 'Could not reach the assistant daemon' string. Updated to match the new wording in connect.ts (per assistant/AGENTS.md: 'daemon' is internal implementation detail). --- assistant/src/__tests__/oauth-cli.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assistant/src/__tests__/oauth-cli.test.ts b/assistant/src/__tests__/oauth-cli.test.ts index 27e497c3720..f3284665caf 100644 --- a/assistant/src/__tests__/oauth-cli.test.ts +++ b/assistant/src/__tests__/oauth-cli.test.ts @@ -1285,7 +1285,7 @@ describe("assistant oauth connect — daemon unreachable (BYO mode)", expect(exitCode).toBe(1); const parsed = JSON.parse(stdout); expect(parsed.ok).toBe(false); - expect(parsed.error).toContain("Could not reach the assistant daemon"); + expect(parsed.error).toContain("Could not reach the assistant"); expect(parsed.error).toContain("Is the assistant running?"); });