fix(oauth): report ignored callbacks in sign-in timeout

[dgokcin]

Summary

When a user starts the OpenAI browser sign-in flow twice — completing an older, stale browser tab
instead of the newest one — the callback server silently rejected the stale callback (its OAuth
state no longer matches) and the user was left staring at a generic OAuth timeout — browser closed without completing sign-in message with no indication of what actually happened.

Changes

• src/oauth/callback-server.ts: count callbacks whose state parameter doesn't match the
  expected value (previously discarded silently). If the sign-in flow times out and at least one
  such callback was seen, the timeout error now reads:
  OAuth timeout — ignored N callback(s) carrying a different sign-in state; you probably completed an older browser tab. Run the command again and use the newest tab.
  The original generic message is unchanged when no mismatched callback occurred.
• tests/oauth-openai.test.ts: added a test that fires a stale-state callback (confirms the
  server still responds 400) and asserts the timeout rejects with the new count-aware message;
  tightened the existing plain-timeout test to assert the exact original message so the two paths
  stay distinguishable.

Additional Notes

• Verified with a feature-deletion mutation: reverting only the callback-server.ts change turns
  the new test red (OAuth timeout — browser closed without completing sign-in instead of the
  count-aware message) while the rest of the suite still passes — the test is discriminating, not
  theatre.

• pnpm typecheck, pnpm build, and the full pnpm test suite pass under an isolated
  CLODEX_HOME; the only failure seen (credential-helper.test.ts > force-kills a helper that
  exceeds the runtime limit) is a pre-existing flake unrelated to this change and passes in
  isolation.

• No README/docs reference the old error string, so no documentation updates were needed.

• This affects only the browser-based OpenAI sign-in flow (used, for example, by workspaces that
  block device-code auth); the device-code flow is untouched.

  Closes fix(auth): browser sign-in timeout misreports "browser closed" after ignoring a stale-tab callback #142

[dgokcin]

hi @bman654 created this after seeing the bug you filed after my merge :) lmk what you think and thank you for this project. this is best out of both worlds, claude codes ui + codex's models in a native way.🎉

[bman654]

Review — PR #146 (head 48d30d1, base 55d3972)

Thanks for this, and for turning a bug report around the same day. The fix is correct, minimal, live
on the real user path, and the tests genuinely discriminate. Approving with notes — the only
thing that must change before this lands is the commit summary line, and that is something the
maintainer sets at squash time, so nothing is blocked on you.

Every claim below is labeled by how it was established: executed observation, code-path trace, or
policy judgment. Both major findings were put through adversarial refutation, and the whole review
is pinned to 48d30d1 (tests/oauth-openai.test.ts 22/22 confirmed independently at that head).

Verdict: APPROVE-WITH-NOTES — 0 blockers, 1 major (merge-time title), 1 optional minor.

---

Does it fix #142? Yes, proven end to end

Not just at the unit level — a refuter drove the real compiled CLI
(clodex providers auth openai --browser, isolated CLODEX_HOME, real callback server, real
loopback HTTP round trip) and read the actual bytes a stuck user sees. The harness replaced the
external open binary with a URL recorder, used the repository's credential-helper fixture for
isolated credential storage, and shortened only the 300 s timer:

callback_status= 400
callback_body= Invalid OAuth state
■  OAuth timeout — ignored 1 callback(s) carrying a different sign-in state; you probably
   completed an older browser tab. Run the command again and use the newest tab.

Issue #142 records the old message from an executed real-CLI reproduction. The path is
providers-command.ts:131 (--browser) → provider-auth.ts:404 → oauth/openai.ts:185 →
callback-server.ts:72-78, 121-127 → providers-command.ts:450 (prints err.message). Live,
supported configuration, user-visible text.

One design detail worth calling out as correct from the source trace: the count is read inside the
timer callback, so it reflects every mismatch seen right up to the moment the timer fires, not the
moment it was armed.

Your test claims — verified, and you did more than you claimed

You reported the feature-deletion mutation. The panel reproduced it and ran the other direction
too, which you didn't claim:

Mutation	Result
Revert callback-server.ts to main (feature deletion)	new test red, 21/22 pass; full suite 1 failed / 1949 passed — no collateral
ignoredStateMismatchCount += 0 (never increment)	new test red
> 0 → >= 0 (count-aware branch always fires)	old plain-timeout test red

That third row is why tightening the pre-existing test from /OAuth timeout/ to the exact string
mattered — under the loose regex that mutation would have shipped green. Tightening an adjacent test
to close the opposite half of a scoped fix is exactly the discipline this repo asks for, and you did
it unprompted. Credit.

---

MAJOR — the commit summary line, fixed at merge

fix(oauth): report ignored callbacks in sign-in timeout passes commitlint (verified, with a
failing control to prove commitlint was actually running). It does not meet CLAUDE.md's
release-note bar: release-please renders the summary verbatim into CHANGELOG.md (confirmed
against three historical commit→changelog pairs), and this one names the mechanism ("report ignored
callbacks"), never says why it matters, and uses "callbacks" — OAuth protocol vocabulary a
non-technical user won't map to "browser tab".

A refuter attacked this finding hard, including checking whether main is full of the same style.
It is — but CLAUDE.md itself cites several of those exact historical subjects in its "Don't"
column, and the standard post-dates them (01b1ed9, 2026-08-11). So it is acknowledged debt, not
current posture. Finding survives.

Suggested squash subject (measured, both under 100 chars):

fix(auth): stop blaming a closed browser when sign-in was completed in an older tab   (83)
fix(auth): say when browser sign-in timed out because you finished in an older tab    (82)

The first is preferable — it names what the user stops seeing, with zero protocol vocabulary.
No action needed from you; the maintainer sets this with gh pr merge --squash --subject. Your
commit body is fine as-is (all lines ≤100).

---

MINOR — optional; does not hold the merge

1. The counter's arithmetic is unpinned. The test only ever exercises N=1, so
ignoredStateMismatchCount++ → = 1 survives the entire suite (103 files / 1950 tests green,
reproduced independently on two worktrees). A lens filed this as MAJOR; a refuter demoted it, and I
agree: the counter is read in exactly one place (the interpolated integer), nothing branches on it
except > 0 (already pinned), so the worst escapable defect is ignored 1 where the truth is
ignored 2 — inside a sentence whose advice is identical either way. Worth ~5 lines if you want it:
fire two mismatched callbacks, await both 400s, assert ignored 2 callback(s). That probe passed
25/25 serial and 18/18 concurrent full-file runs.

MERGE NOTE — no author action. No open PR touches src/oauth/callback-server.ts; #48
shares only tests/oauth-openai.test.ts and is stale (last updated 2026-07-25). Merge #146 first,
then have #48 rebase onto it.

---

Checked and NOT a problem — so you don't have to re-derive these

• Security probe demoted. An HTTP harness carrying hostile Origin/Referer headers reached
  the fixed callback path and inflated the count. This review did not run that request from a real
  browser, so it does not claim browser delivery as an executed observation. The unchanged mismatch
  branch returns before codeResolve/buffered, so the probe does not consume a later valid
  callback; this PR adds only the eventual timeout explanation. No separate security finding.
• No secret reaches the message. The only ${} in the template is the integer. Asserted
  live that the expected state, the attacker-supplied code/state, and the origin are all absent;
  no console/log call is added anywhere in the diff.
• Counter scope is correctly narrow. The u.pathname !== path 404 branch returns first — five
  wrong-path hits left the zero-mismatch message intact. Good placement.
• The exact wording and signal match #142. The issue explicitly prescribed counting every
  state-rejected callback, the hedged stale-tab explanation, and the literal callback(s) wording.
  A probe from another local process can therefore change the diagnosis, but the advice remains
  safe and this PR implements the requested contract; no contributor ask.
• The test's rejection-order concern is test-only hygiene, not a runtime finding. The submitted
  50 ms test was stable in 49/49 full-file runs; only a forced 0 ms deadline exposed the ordering
  hazard. No merge ask.
• Device-code flow is untouched — startCallbackServer has exactly one call site
  (oauth/openai.ts:185, inside runOpenAiBrowserFlow); the device path never builds a callback
  server. Your claim confirmed.
• Docs sweep confirmed. Grepped the value, not one spelling: "browser closed without completing", "OAuth timeout", and "Invalid OAuth state". Every hit was inside the two
  changed files; no README, docs, help, or changelog text needs updating. Your claim confirmed.
• Skipping the optional FAILURE_HTML nicety was the right call. #142 listed it as optional;
  bundling it would widen a clean 28-line fix, and answering a possibly-hostile mismatched callback
  with a styled page tells the requester more than a bare 400 does. File separately if wanted.
• The credential-helper "pre-existing flake" did not reproduce. 20 full-file runs (10 on your
  head, 10 on main) — force-kills a helper that exceeds the runtime limit passed every time, and
  CI is green. Not a defect in your PR and nothing to fix; noting it only so the caveat isn't
  carried forward as an accepted known-failure.
• Full gate green, independently, under CLODEX_HOME=$(mktemp -d) and
  CLAUDE_CODE_ENTRYPOINT=cli: typecheck clean, 103 files / 1950 tests passed, build success.
  CI agrees (commitlint pass, test pass).

Also checked after the fact — all clean

A second pass went after the things a 28-line diff most often gets wrong. None of it changes
anything for you; recording it so it isn't re-derived later.

• No timer leak. All four arming modes were driven against the real compiled module; the process
  exits in 6–129 ms where a leaked 300 s timer would have held it open. close() with no waiter
  armed cannot leak, because the timer is only ever created inside waitForCallback.
• The never-reset counter is not observable by any production caller. This was the sharpest
  question about the design — the count lives in the startCallbackServer closure and is never
  cleared, so in principle a second waitForCallback on one server would inherit a stale count.
  That caller does not exist: waitForCallback has exactly one call site in src/, and the
  providers retry menu builds a fresh closure per attempt. Proven on the real fixed ports —
  three consecutive retries report 1, 1, 1, never 1, 2, 3. Hygiene at most, no severity.
• N cannot be inflated by stray traffic. 404s and favicon hits return before the counter.
• Fidelity to #142 is byte-identical, checked programmatically against the pre-PR literal: the
  count-aware string and the preserved zero-count string both match what the issue prescribed.

One thing that is our bug, not yours

The header comment at src/oauth/callback-server.ts:1-3 is wrong on all three of its claims, and it
predates your PR:

// CLI fallback local callback server ...
// Primary path: the GUI server handles /oauth/callback when the UI is open.

There is no GUI server — it was removed in d01d0eb when the fork was stripped to the
Claude-to-OpenAI bridge — and no /oauth/callback route exists anywhere; this server serves
/auth/callback. The only occurrences of "GUI" and "/oauth/callback" in src/ are inside that
comment describing them. Calling the file a "CLI fallback ... only used when" is also backwards:
since #141 it is the sole receiver for browser sign-in.

Flagging it because a wrong comment of ours has twice propagated into a contributor's PR, and this
one would have told you the code you were fixing was a secondary path. Not your job — fix it as
a three-line ride-along here if you'd like, or we'll take it separately. It does not hold the merge.

Two possible follow-ups, both pre-existing — not asks on this PR

• oauth/openai.ts:~210's state check is provably unreachable now that the server filters first
  (fuzzed 10 state values, 0 reached it). It's deliberately labeled "defense in depth" and has been
  dead since #141, so this is a note, not a defect.
• The EADDRINUSE message names "another OpenAI sign-in (e.g. codex login)" on the sole evidence of
  a busy port — proven by holding 1455/1457 with a bare TCP server. Same class as #142, but much
  softer: its remedy is hedged and it asserts no false fact. Also pre-existing from #141.

[bman654]

cheers @dgokcin and thanks for the PRs