diff --git a/.changeset/auth-login-error-codes.md b/.changeset/auth-login-error-codes.md new file mode 100644 index 00000000..57018d3f --- /dev/null +++ b/.changeset/auth-login-error-codes.md @@ -0,0 +1,6 @@ +--- +"clerk": patch +--- + +Report why `clerk auth login` failed instead of collapsing every failure into one error. +The browser sign-in wait, OAuth provider errors, state mismatches, missing authorization codes, and callback bind failures now carry distinct error codes, and login records which step of the flow it reached. diff --git a/packages/cli-core/src/commands/auth/login.test.ts b/packages/cli-core/src/commands/auth/login.test.ts index 639895a1..e04b0174 100644 --- a/packages/cli-core/src/commands/auth/login.test.ts +++ b/packages/cli-core/src/commands/auth/login.test.ts @@ -97,6 +97,7 @@ mock.module("../../lib/autoclaim.ts", () => ({ })); const { setLogLevel } = await import("../../lib/log.ts"); +const telemetryMod = await import("../../lib/telemetry.ts"); const { login } = await import("./login.ts"); describe("login", () => { @@ -609,4 +610,84 @@ describe("login", () => { expect(captured.err).not.toContain("Next steps:"); }); + + describe("telemetry stages", () => { + let stageSpy: ReturnType | undefined; + let callerStageSpy: ReturnType | undefined; + + afterEach(() => { + stageSpy?.mockRestore(); + callerStageSpy?.mockRestore(); + stageSpy = undefined; + callerStageSpy = undefined; + }); + + function trackStages() { + const spy = spyOn(telemetryMod, "setTelemetryStage"); + stageSpy = spy; + return { calls: () => spy.mock.calls.map((call) => call[0]) }; + } + + test("a completed login reports the terminal stage", async () => { + mockGetValidToken.mockResolvedValue(null); + mockOAuthSuccess(); + const stages = trackStages(); + + await runLogin(); + + expect(stages.calls().at(-1)).toBe("done"); + }); + + // The browser wait is where the flow is known to lose people; a run that + // stops there must be attributable to that step and no other. + test("a login abandoned during the browser wait stops at awaiting_callback", async () => { + mockGetValidToken.mockResolvedValue(null); + mockBunSpawn(); + mockStartAuthServer.mockReturnValue({ + port: 54321, + waitForCallback: mock().mockRejectedValue(new Error("Authentication timed out.")), + stop: mock(), + }); + const stages = trackStages(); + + await expect(runLogin()).rejects.toThrow("Authentication timed out"); + + expect(stages.calls().at(-1)).toBe("awaiting_callback"); + }); + + // `init` and `link` invoke login mid-flow; a clean return must hand the + // caller's stage back instead of leaving the outer command at "done". + test("a nested login hands the caller's stage back on success", async () => { + mockGetValidToken.mockResolvedValue(null); + mockOAuthSuccess(); + callerStageSpy = spyOn(telemetryMod, "currentTelemetryStage").mockReturnValue("link"); + const stages = trackStages(); + + await runLogin({ showNextSteps: false }); + + expect(stages.calls().at(-1)).toBe("link"); + }); + + test("a failure persisting the token stops at store", async () => { + mockGetValidToken.mockResolvedValue(null); + mockOAuthSuccess(); + mockStoreToken.mockRejectedValue(new Error("keychain unavailable")); + const stages = trackStages(); + + await expect(runLogin()).rejects.toThrow("keychain unavailable"); + + expect(stages.calls().at(-1)).toBe("store"); + }); + + test("a failure exchanging the code stops at token_exchange", async () => { + mockGetValidToken.mockResolvedValue(null); + mockOAuthSuccess(); + mockExchangeCodeForToken.mockRejectedValue(new Error("exchange failed")); + const stages = trackStages(); + + await expect(runLogin()).rejects.toThrow("exchange failed"); + + expect(stages.calls().at(-1)).toBe("token_exchange"); + }); + }); }); diff --git a/packages/cli-core/src/commands/auth/login.ts b/packages/cli-core/src/commands/auth/login.ts index b036985a..29a80d12 100644 --- a/packages/cli-core/src/commands/auth/login.ts +++ b/packages/cli-core/src/commands/auth/login.ts @@ -25,6 +25,7 @@ import { attemptAutoclaim, type AutoclaimResult } from "../../lib/autoclaim.ts"; import { openBrowser } from "../../lib/open.ts"; import { cyan, dim } from "../../lib/color.ts"; import { log } from "../../lib/log.ts"; +import { currentTelemetryStage, setTelemetryStage } from "../../lib/telemetry.ts"; import { ensureFirstApplication } from "../../lib/first-application.ts"; interface LoginOptions { @@ -92,6 +93,7 @@ async function performOAuthFlow(): Promise { const timeoutMinutes = Math.round(AUTH_TIMEOUT_MS / 60_000); log.info(`Waiting for authentication (timeout in ${timeoutMinutes}m)...`); + setTelemetryStage("awaiting_callback"); const { code } = await withSpinner("Waiting for authentication...", async () => authServer.waitForCallback().catch((error: unknown) => { authServer.stop(); @@ -113,6 +115,7 @@ async function performOAuthFlow(): Promise { log.debug(`credentials: could not read outgoing session — ${errorMessage(error)}`); } + setTelemetryStage("token_exchange"); const tokenResponse = await withSpinner("Completing authentication...", async () => exchangeCodeForToken({ code, @@ -121,6 +124,7 @@ async function performOAuthFlow(): Promise { }), ); + setTelemetryStage("store"); await storeToken(createOAuthSession(tokenResponse)); const userInfo = await fetchUserInfo(tokenResponse.access_token); @@ -130,13 +134,27 @@ async function performOAuthFlow(): Promise { } export async function login(options: LoginOptions = {}): Promise { + // `init` and `link` call this mid-flow and share the one process-global + // telemetry stage. Login's own markers are worth having while it runs, but + // on a clean return the caller's stage comes back so the rest of *their* + // work isn't reported as `done`. On a throw the login stage stands: that is + // genuinely where the run stopped. + const callerStage = currentTelemetryStage(); + const userInfo = await runLogin(options); + if (callerStage) setTelemetryStage(callerStage); + return userInfo; +} + +async function runLogin(options: LoginOptions = {}): Promise { const { showNextSteps = true, yes } = options; intro("Signing in"); + setTelemetryStage("session_check"); const existingSession = await withSpinner("Checking session...", async () => getExistingSession(), ); if (existingSession && !isHuman()) { + setTelemetryStage("done"); log.success(`Logged in as ${existingSession.email}`); const claimResult = await handleAutoclaim(process.cwd()); if (showNextSteps) { @@ -180,9 +198,11 @@ export async function login(options: LoginOptions = {}): Promise { // Best-effort: ensure the user has at least one application so downstream // commands (clerk link, clerk init) have something to operate on. + setTelemetryStage("first_application"); await withSpinner("Setting up your default application...", async () => ensureFirstApplication()); bar(); + setTelemetryStage("done"); log.success(`Logged in as ${userInfo.email}`); const claimResult = await handleAutoclaim(process.cwd()); diff --git a/packages/cli-core/src/lib/auth-server.test.ts b/packages/cli-core/src/lib/auth-server.test.ts index 00a53d7c..a2d1ab6b 100644 --- a/packages/cli-core/src/lib/auth-server.test.ts +++ b/packages/cli-core/src/lib/auth-server.test.ts @@ -1,17 +1,25 @@ import { afterEach, describe, expect, spyOn, test } from "bun:test"; import { startAuthServer } from "./auth-server.ts"; +import { AUTH_TIMEOUT_MS } from "./constants.ts"; +import { ERROR_CODE } from "./errors.ts"; import { useCaptureLog } from "../test/lib/stubs.ts"; describe("auth-server", () => { let serveSpy: ReturnType | undefined; let clearTimeoutSpy: ReturnType | undefined; + let timeoutSpy: ReturnType | undefined; + let openServer: { stop: () => void } | undefined; useCaptureLog(); afterEach(() => { serveSpy?.mockRestore(); clearTimeoutSpy?.mockRestore(); + timeoutSpy?.mockRestore(); + openServer?.stop(); serveSpy = undefined; clearTimeoutSpy = undefined; + timeoutSpy = undefined; + openServer = undefined; }); test("starts on a random port", () => { @@ -31,6 +39,48 @@ describe("auth-server", () => { expect(clearTimeoutSpy).toHaveBeenCalled(); }); + // A host that forbids binding loopback fails every login on the machine — + // worth telling apart from anything the user did. + test("a bind failure carries the callback_bind_failed code", () => { + serveSpy = spyOn(Bun, "serve").mockImplementation(() => { + throw new Error("listen failed"); + }); + + expect(() => startAuthServer("test-state")).toThrowError( + expect.objectContaining({ code: ERROR_CODE.CALLBACK_BIND_FAILED }), + ); + }); + + // The wait expiring is the single most common way login ends without a + // session; it must not look like a crash. + test("the callback wait timing out carries the auth_timeout code", async () => { + let fire: (() => void) | undefined; + const realSetTimeout = globalThis.setTimeout; + timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation((( + cb: () => void, + ms?: number, + ...rest: unknown[] + ) => { + // Capture only the login deadline; everything else keeps real timing. + if (ms === AUTH_TIMEOUT_MS) { + fire = cb; + return 0 as unknown as ReturnType; + } + return realSetTimeout(cb, ms, ...rest); + }) as typeof setTimeout); + + const server = startAuthServer("test-state"); + openServer = server; + const errorPromise = server.waitForCallback().catch((e: unknown) => e); + + expect(fire).toBeDefined(); + fire?.(); + + const error = await errorPromise; + expect(error).toMatchObject({ code: ERROR_CODE.AUTH_TIMEOUT }); + expect((error as Error).message).toContain("timed out"); + }); + test("callback resolves with code on valid request", async () => { const state = "my-test-state"; const server = startAuthServer(state); @@ -60,6 +110,7 @@ describe("auth-server", () => { const error = await errorPromise; expect(error).toBeInstanceOf(Error); expect((error as Error).message).toContain("Invalid state"); + expect(error).toMatchObject({ code: ERROR_CODE.OAUTH_STATE_MISMATCH }); }); test("callback rejects on missing code", async () => { @@ -73,6 +124,7 @@ describe("auth-server", () => { const error = await errorPromise; expect(error).toBeInstanceOf(Error); expect((error as Error).message).toContain("No authorization code"); + expect(error).toMatchObject({ code: ERROR_CODE.OAUTH_NO_CODE }); }); test("callback rejects on OAuth error", async () => { @@ -88,6 +140,7 @@ describe("auth-server", () => { const error = await errorPromise; expect(error).toBeInstanceOf(Error); expect((error as Error).message).toContain("User denied access"); + expect(error).toMatchObject({ code: ERROR_CODE.OAUTH_PROVIDER_ERROR }); }); test("root path returns waiting message", async () => { diff --git a/packages/cli-core/src/lib/auth-server.ts b/packages/cli-core/src/lib/auth-server.ts index 47d3ac4b..cfde3424 100644 --- a/packages/cli-core/src/lib/auth-server.ts +++ b/packages/cli-core/src/lib/auth-server.ts @@ -4,6 +4,7 @@ */ import { AUTH_TIMEOUT_MS, CALLBACK_PATH } from "./constants.ts"; +import { CliError, ERROR_CODE, errorMessage } from "./errors.ts"; import { observeHostCapabilityFailure } from "./host-execution.ts"; import { log } from "./log.ts"; import { whileAwaitingUser } from "./signals.ts"; @@ -188,8 +189,9 @@ export function startAuthServer(expectedState: string): AuthServerResult { const timeout = setTimeout(() => { log.debug(`auth-server: timed out after ${AUTH_TIMEOUT_MS}ms`); rejectCallback( - new Error( + new CliError( "Authentication timed out. Run `clerk auth login` to try again — if your browser did not open, copy the printed URL into any browser on this machine.", + { code: ERROR_CODE.AUTH_TIMEOUT }, ), ); // `stop()` is a promise nothing can wait on: the login flow has already @@ -213,7 +215,11 @@ export function startAuthServer(expectedState: string): AuthServerResult { if (error) { const description = url.searchParams.get("error_description") || error; log.debug(`auth-server: OAuth error in callback — ${error}: ${description}`); - rejectCallback(new Error(`OAuth error: ${description}`)); + rejectCallback( + new CliError(`OAuth error: ${description}`, { + code: ERROR_CODE.OAUTH_PROVIDER_ERROR, + }), + ); clearTimeout(timeout); setTimeout(() => void server?.stop(), 100); return new Response(ERROR_HTML(description), { @@ -223,7 +229,11 @@ export function startAuthServer(expectedState: string): AuthServerResult { if (state !== expectedState) { log.debug(`auth-server: state mismatch (expected=${expectedState}, got=${state})`); - rejectCallback(new Error("Invalid state parameter. Possible CSRF attack.")); + rejectCallback( + new CliError("Invalid state parameter. Possible CSRF attack.", { + code: ERROR_CODE.OAUTH_STATE_MISMATCH, + }), + ); clearTimeout(timeout); setTimeout(() => void server?.stop(), 100); return new Response(ERROR_HTML("Invalid state parameter."), { @@ -234,7 +244,11 @@ export function startAuthServer(expectedState: string): AuthServerResult { if (!code) { log.debug("auth-server: callback received with no authorization code"); - rejectCallback(new Error("No authorization code received.")); + rejectCallback( + new CliError("No authorization code received.", { + code: ERROR_CODE.OAUTH_NO_CODE, + }), + ); clearTimeout(timeout); setTimeout(() => void server?.stop(), 100); return new Response(ERROR_HTML("No authorization code received."), { @@ -266,7 +280,17 @@ export function startAuthServer(expectedState: string): AuthServerResult { target: "127.0.0.1:0", label: CALLBACK_PATH, }); - throw error; + // A sandbox or firewall that forbids binding loopback fails every login on + // the machine; it is a distinct condition from anything the user did. + log.debug( + `auth-server: bind failed — ${error instanceof Error ? (error.stack ?? error.message) : String(error)}`, + ); + throw new CliError( + `Could not start the local sign-in callback server: ${errorMessage(error)}`, + { + code: ERROR_CODE.CALLBACK_BIND_FAILED, + }, + ); } const activeServer = server; diff --git a/packages/cli-core/src/lib/errors.ts b/packages/cli-core/src/lib/errors.ts index 8711d9df..0c7dcd5c 100644 --- a/packages/cli-core/src/lib/errors.ts +++ b/packages/cli-core/src/lib/errors.ts @@ -104,6 +104,17 @@ export const ERROR_CODE = { NO_INTERACTIVE_TERMINAL: "no_interactive_terminal", /** Named environment isn't configured. */ INVALID_ENVIRONMENT: "invalid_environment", + + /** The browser sign-in wait expired before a callback arrived. */ + AUTH_TIMEOUT: "auth_timeout", + /** The authorization server redirected back with an `error` parameter. */ + OAUTH_PROVIDER_ERROR: "oauth_provider_error", + /** Callback `state` did not match what this process generated. */ + OAUTH_STATE_MISMATCH: "oauth_state_mismatch", + /** Callback arrived without an authorization code. */ + OAUTH_NO_CODE: "oauth_no_code", + /** The loopback callback server could not bind a local port. */ + CALLBACK_BIND_FAILED: "callback_bind_failed", } as const; export type ErrorCode = (typeof ERROR_CODE)[keyof typeof ERROR_CODE]; diff --git a/packages/cli-core/src/lib/telemetry.ts b/packages/cli-core/src/lib/telemetry.ts index 505e5631..5880a435 100644 --- a/packages/cli-core/src/lib/telemetry.ts +++ b/packages/cli-core/src/lib/telemetry.ts @@ -48,10 +48,12 @@ export type TelemetryResult = { * instead of silently splitting the funnel into two buckets in the warehouse, * and so no interpolated value (a path, a project name) can reach the payload. * - * Declared in execution order: this is the funnel, so a new stage goes where it - * runs, not at the end. `already_set_up` is a terminal branch off `scaffold`. + * Declared in execution order, grouped per command: each group is that + * command's funnel, so a new stage goes where it runs, not at the end. + * `already_set_up` is a terminal branch off `scaffold`. */ export type TelemetryStage = + // `clerk init` | "flags" | "detect" | "bootstrap" @@ -62,6 +64,13 @@ export type TelemetryStage = | "already_set_up" | "keys" | "skills" + // `clerk auth login` + | "session_check" + | "awaiting_callback" + | "token_exchange" + | "store" + | "first_application" + // shared terminal marker | "done"; /** Structural slice of Commander's Command — avoids its generic types. */ @@ -194,6 +203,11 @@ export function setTelemetryStage(stage: TelemetryStage): void { if (context) context.stage = stage; } +/** Read the stage a caller had set, so a nested flow can hand it back. */ +export function currentTelemetryStage(): TelemetryStage | null { + return context?.stage ?? null; +} + export function telemetryResultForError(error: unknown): TelemetryResult { if (error instanceof UserAbortError) { return { outcome: "abort", exitCode: EXIT_CODE.SUCCESS };