Uh oh!
There was an error while loading. Please reload this page.
fix(auth): give each login failure its own error code and stage - #442
Conversation
🦋 Changeset detectedLatest commit: 8a0416a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
a3d8784 to
d3ff139Compare| await withSpinner("Setting up your default application...", async () => ensureFirstApplication()); | ||
| bar(); | ||
| setTelemetryStage("done"); |
There was a problem hiding this comment.
login() isn't only the clerk auth login command body — init calls it from resolveAuthLabel (init/index.ts:460), link from ensureAuth (link/index.ts:119), and doctor from its context fixer. The telemetry context is process-global (telemetry.ts:87), created once per run by the preAction hook against the top-level command, so a stage set in here lands on that command's event.
The concrete failure: clerk link in a logged-out directory calls login(), which reaches this line and sets stage: "done". Control returns to ensureAuth, and link then does the work it actually exists to do — resolveProfile, app selection, writing the profile. If any of that throws, the event is command: "link", outcome: "error", stage: "done". link sets no stages of its own, so this is the only stage it will ever report, and it reports it exclusively on runs that failed after completing auth.
init has the same shape: setTelemetryStage("link"), then authenticateAndLink → login() → "done", then the rest of authenticateAndLink plus detectAndInstall all run before init's next marker ("keys" at index.ts:179). Every failure in that window is attributed to "done".
That inverts what the stage field is for. The same hazard applies to the whole flat TelemetryStage union — a future command adding a "link" or "keys" stage will silently collide with init's.
Suggested fix — restore the caller's stage on a clean return
Restoring only on a clean return keeps the useful half: a run that dies inside the browser wait still reports awaiting_callback, correctly attributed to whichever command was running. It's the successful return that has to hand the stage back.
In lib/telemetry.ts:
/** Read the stage a caller had set, so a nested flow can hand it back. */exportfunctioncurrentTelemetryStage(): TelemetryStage|null{returncontext?.stage??null;}In commands/auth/login.ts — rename the existing body to runLogin and wrap it:
exportasyncfunctionlogin(options: LoginOptions={}): Promise<UserInfo>{// `init`, `link`, and `doctor` call this mid-flow and share the one// process-global stage. Its 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.constcallerStage=currentTelemetryStage();constuserInfo=awaitrunLogin(options);if(callerStage)setTelemetryStage(callerStage);returnuserInfo;}Gating the stage calls on login being the invoked command works too, and is more explicit — but it needs a signal login doesn't currently have (showNextSteps: false correlates with nested calls today, which is a coincidence worth not depending on).
Sent from Claude
- Reviewed by @wyattjoh
There was a problem hiding this comment.
Fixed in bf122db — took the wrapper: currentTelemetryStage() reader, and login() hands the caller's stage back on a clean return (login's own stage stands on a throw). Mutation-checked: the nested test fails with the restore removed.
| }), | ||
| ); | ||
| setTelemetryStage("store"); |
There was a problem hiding this comment.
"store" is the last stage set before performOAuthFlow returns, so it covers storeToken, fetchUserInfo, setAuth, and then back in login(): revokeToken for the superseded session and ensureFirstApplication.
ensureFirstApplication is the one that matters — it's a PLAPI round trip that creates an application, and it's the slowest and most failure-prone step in the tail of this flow. A user whose login dies there produces stage: "store", which reads as a credential-store problem and points debugging at the keychain instead of the API.
Suggested fix — give the app-creation step its own marker
In lib/telemetry.ts:
// `clerk auth login`
| "session_check"
| "awaiting_callback"
| "token_exchange"
| "store"
+ | "first_application" // 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());Sent from Claude
- Reviewed by @wyattjoh
There was a problem hiding this comment.
Fixed in bf122db — first_application stage added before ensureFirstApplication.
| expect(error).toMatchObject({ code: ERROR_CODE.AUTH_TIMEOUT }); | ||
| expect((error as Error).message).toContain("timed out"); | ||
| timeoutSpy.mockRestore(); |
There was a problem hiding this comment.
timeoutSpy.mockRestore() and server.stop() run after the assertions, so any failing expect above them skips both. The consequences are asymmetric with the usual leaked-mock nit: a live spyOn(globalThis, "setTimeout") stays installed for the rest of the file, and every later test that calls startAuthServer gets its deadline swallowed and its fire callback captured into a stale closure. One real assertion failure here cascades into a set of confusing unrelated ones.
The file already has the right pattern for this — serveSpy and clearTimeoutSpy are let-declared at describe scope and restored in afterEach.
Suggested fix — move both into the existing afterEach
describe("auth-server", () => {
let serveSpy: ReturnType<typeof spyOn> | undefined;
let clearTimeoutSpy: ReturnType<typeof spyOn> | undefined;
+ let timeoutSpy: ReturnType<typeof spyOn> | undefined;+ let openServer: { stop: () => void } | undefined;
useCaptureLog();
afterEach(() => {
serveSpy?.mockRestore();
clearTimeoutSpy?.mockRestore();
+ timeoutSpy?.mockRestore();+ openServer?.stop();
serveSpy = undefined;
clearTimeoutSpy = undefined;
+ timeoutSpy = undefined;+ openServer = undefined;
});Then in the timeout test, assign instead of declaring, and drop the trailing cleanup:
- const timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation(((+ timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation(((
cb: () => void,
ms?: number,
...rest: unknown[]
) => {
@@
const server = startAuthServer("test-state");
+ openServer = server;
const errorPromise = server.waitForCallback().catch((e: unknown) => e);
@@
expect((error as Error).message).toContain("timed out");
-- timeoutSpy.mockRestore();- server.stop();
});Sent from Claude
- Reviewed by @wyattjoh
There was a problem hiding this comment.
Fixed in bf122db — timeoutSpy and the open server moved into the existing afterEach.
| await runLogin(); | ||
| expect(stages.calls().at(-1)).toBe("done"); | ||
| stages.restore(); |
There was a problem hiding this comment.
Same shape as the auth-server.test.ts teardown, lower blast radius: stages.restore() is unreachable if expect(stages.calls().at(-1)) fails, leaving the module spy installed for the two tests that follow — each of which then calls spyOn on an already-spied property.
While you're here: the three tests cover done, awaiting_callback, and token_exchange, but nothing pins session_check or store, and nothing covers login() invoked as a subroutine — which is where the stage bookkeeping actually goes wrong.
Suggested fix — register the restore in a teardown hook
describe("telemetry stages", () => {
+ let stageSpy: ReturnType<typeof spyOn> | undefined;++ afterEach(() => {+ stageSpy?.mockRestore();+ stageSpy = undefined;+ });+
function trackStages() {
- const stage = spyOn(telemetryMod, "setTelemetryStage");- return {- calls: () => stage.mock.calls.map((call) => call[0]),- restore: () => stage.mockRestore(),- };+ const spy = spyOn(telemetryMod, "setTelemetryStage");+ stageSpy = spy;+ return { calls: () => spy.mock.calls.map((call) => call[0]) };
}Each test then drops its trailing stages.restore() line.
A fourth case worth adding, which is what LOG-001 is about:
test("a nested login leaves the caller's stage in place",async()=>{mockGetValidToken.mockResolvedValue(null);mockOAuthSuccess();telemetryMod.setTelemetryStage("link");conststages=trackStages();awaitrunLogin({showNextSteps: false});expect(stages.calls().at(-1)).toBe("link");});Sent from Claude
- Reviewed by @wyattjoh
There was a problem hiding this comment.
Fixed in bf122db — spy restore moved to afterEach, and added the nested-login handback test plus a store pin. Skipped a session_check pin: getExistingSession swallows every failure, so no run can terminate at that stage.
| 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. | ||
| throw new CliError( |
There was a problem hiding this comment.
errorMessage(error) preserves the message text, but the thrown CliError carries no reference to the original — CliErrorOptions has no cause, and observeHostCapabilityFailure only logs when isAgent() and the failure matches its sandbox heuristic. In a normal human-mode run the errno and stack are gone before anyone can look at them, and EACCES (sandbox/firewall) versus EADDRINUSE versus something else is exactly the distinction the new callback_bind_failed code exists to help diagnose.
Suggested fix — keep the original recoverable under --verbose
Every other branch in this file already debug-logs before rejecting; this one is the outlier.
| thrownewCliError( | |
| log.debug(`auth-server: bind failed — ${errorinstanceofError ? (error.stack??error.message) : String(error)}`); | |
| thrownewCliError( |
Sent from Claude
- Reviewed by @wyattjoh
There was a problem hiding this comment.
Fixed in bf122db — original error (stack included) debug-logged before the CliError wraps it.
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| "clerk": minor | |||
There was a problem hiding this comment.
The commit is fix(auth): ... and the change is a diagnostics fix rather than new CLI surface. The repo's bump table maps fix: to patch and reserves minor for new user-facing features, commands, or flags, with an explicit default-to-patch rule when the split is unclear. The parent PR's minor is correct for its own feat(telemetry): commit, but that doesn't carry over.
Suggested fix
| "clerk": minor | |
| "clerk": patch |
Sent from Claude
- Reviewed by @wyattjoh
| import { openBrowser } from "../../lib/open.ts"; | ||
| import { cyan, dim } from "../../lib/color.ts"; | ||
| import { log } from "../../lib/log.ts"; | ||
| import { setTelemetryStage } from "../../lib/telemetry.js"; |
There was a problem hiding this comment.
Every other import in this file — including ../../lib/errors.ts and ../../lib/log.ts a few lines up — uses a .ts specifier, as do ~1100 imports across cli-core. There's a small pocket of .js specifiers (lib/skills.ts:10, init/scan.ts:2) and the parent commit added one more at init/index.ts:24, so this is drifting rather than isolated. Bun resolves both, but it's worth keeping the new ones consistent.
Suggested fix
| import{setTelemetryStage}from"../../lib/telemetry.js"; | |
| import{setTelemetryStage}from"../../lib/telemetry.ts"; |
Sent from Claude
- Reviewed by @wyattjoh
d3ff139 to
7330d8cCompareEvery failure in the browser-callback phase of `clerk auth login` was a plain `Error`, and `telemetryResultForError` only reads a code off `CliError` and `ApiError` — so a timed-out wait, an OAuth provider error, a state mismatch, a missing authorization code, and a loopback bind failure all landed in the warehouse as `unexpected_error`, indistinguishable from each other and from every other uncaught throw. Types those five sites as `CliError` with distinct codes, and instruments login with stage markers (session_check → awaiting_callback → token_exchange → store → done) so an abandoned browser wait is attributable to the step it stopped at. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Nested login (init, link) hands the caller's telemetry stage back on a clean return, so their post-auth work stops reporting stage "done" - ensureFirstApplication gets its own first_application stage; failures there no longer read as credential-store problems - Debug-log the original bind error before wrapping it in CliError - Restore test spies in afterEach so a failed assertion can't leak them - Changeset minor -> patch per the bump policy for fix: changes - .ts import specifier Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bf122db to
8a0416aCompareNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: 6 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughWalkthroughThe login command now records telemetry stages for session checks, OAuth callback waiting, token exchange, credential storage, application setup, and completion. Nested login calls restore the caller’s stage after successful execution. Authentication-server failures now use typed Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk:⚪ Minimal · up to This PR gives auth login failures specific error codes and records the login stage reached, with no actionable merge-blocking risk remaining beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Stacked on #441 — retarget to
mainonce it merges.Every failure in the
auth loginbrowser flow reported as one genericunexpected_error. Each now has its own code —auth_timeout,oauth_provider_error,oauth_state_mismatch,oauth_no_code,callback_bind_failed— and login records which step it reached (session_check → awaiting_callback → token_exchange → store → done), so a walked-away browser wait is distinguishable from real breakage.🤖 Generated with Claude Code