Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions assistant/src/__tests__/oauth-cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ let mockUpsertAppImpl:
let mockOrchestrateOAuthConnect: (
opts: Record<string, unknown>,
) => Promise<Record<string, unknown>>;
let mockCliIpcCall: (
operationId: string,
params?: Record<string, unknown>,
) => Promise<Record<string, unknown>> = async () => ({
ok: false,
error: "Could not connect to assistant daemon (test default)",
});
Comment on lines +68 to +71
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 IPC default mock in integration test uses 'daemon' in error string that flows to user-facing output

The default mockCliIpcCall at oauth-cli.test.ts:68-71 returns error: "Could not connect to assistant daemon (test default)". This mirrors the real cliIpcCall error strings at cli-client.ts:102,169 which also say "assistant daemon". Since this PR's new code at connect.ts:521-522 now surfaces the IPC error directly to users via Could not reach the assistant: ${startResult.error}, the word "daemon" appears in user-facing CLI output. The AGENTS.md rule states user-facing text should say "assistant" not "daemon". While the "daemon" text originates from pre-existing cli-client.ts code, this PR creates the new code path that surfaces it. A follow-up could either sanitize the embedded error or update cli-client.ts error strings.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

let mockGetAppByProviderAndClientId: (
provider: string,
clientId: string,
Expand Down Expand Up @@ -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<string, unknown>) =>
mockCliIpcCall(operationId, params),
}));

mock.module("../oauth/seed-providers.js", () => ({
SEEDED_PROVIDER_KEYS: new Set([
"google",
Expand Down Expand Up @@ -1220,6 +1236,111 @@ describe("assistant oauth connect managed mode — platform 401/403 errors", ()
});
});

// ---------------------------------------------------------------------------
// `assistant oauth connect <provider>` 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 <provider> — 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
// ---------------------------------------------------------------------------
Expand Down
Loading
Loading