diff --git a/assistant/src/__tests__/oauth-cli.test.ts b/assistant/src/__tests__/oauth-cli.test.ts index f1e89988102..f3284665caf 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"); + 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 // --------------------------------------------------------------------------- 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 2028c3d3f6a..755fb26c4b2 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,16 @@ 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: 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: ${startResult.error}. Is the assistant running?` + : "Could not reach the assistant. Is the assistant running?", + ); + return; } } catch (err) { const message = err instanceof Error ? err.message : String(err);