fix(auth): stop blaming another sign-in when a busy port blocks browser sign-in - #148
Merged
Merged
Conversation
…er sign-in The busy-port error asserted a cause it never observed: it told the user to "close any other OpenAI sign-in (e.g. codex login)" on the sole evidence that ports 1455/1457 were occupied. Any process holding those ports produces the same bind failure, so the advice could send a user hunting for a codex login that was never running. The message now leads with what was actually observed, offers that sign-in as the first thing to check rather than the cause, and says any other process holding the ports will do it. Same defect class as the timeout message fixed in #146/#142, found while reviewing that change. Also corrects two comments that made false claims about this code: - callback-server.ts's header described itself as a "CLI fallback" whose "primary path" is a GUI server handling /oauth/callback. No GUI server exists and no /oauth/callback route exists anywhere; this server serves the callback path its caller registers and, since #141, is the sole receiver for browser sign-in. The only occurrences of "GUI" and "/oauth/callback" in src/ were inside that comment. The UI server that owned this module's job went with the strip to the Claude-to-OpenAI bridge (d01d0eb) — for the since-removed Antigravity flow, not for OpenAI, which used device code at the time. - openai.ts's post-callback state check was labelled "defense in depth", which understates it: this flow always passes expectedState, so the server rejects a mismatch with 400 before it can reach that branch. The guard is kept as a backstop, but the comment no longer implies it is what protects the flow today. The existing busy-port test asserted only a loose regex on the port numbers, which passed unchanged against the new wording. It now compares the Error for equality: toThrow(string) is a substring match in Vitest, so an assertion written that way still lets appended copy through unasserted — verified by appending prose to the message and watching the test stay green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When browser sign-in can't start because its ports are already taken, clodex used to tell you to
"close any other OpenAI sign-in (e.g.
codex login)" — even though all it had actually observed wasthat the ports were busy. Any program holding port 1455 or 1457 causes the same failure, so that
advice could send you hunting for a
codex loginthat was never running. The error now leads withwhat was actually observed, offers another OpenAI sign-in as the first thing to check rather than
the cause, and says plainly that any other process holding those ports will do it.
This is the same defect as #142 — an error message asserting a cause it never observed — on a
neighbouring path. It was found while reviewing the fix for #142 in #146.
Changes
src/oauth/openai.ts— the busy-port message. Before / after, as the real CLI prints them withboth ports held:
The remedy is now correct whoever holds the port. It renders at 170 columns, down from an
intermediate 218-column draft.
Two comments that made false claims. Neither changes behaviour; both were actively misleading.
src/oauth/callback-server.tsopened by describing itself as a "CLI fallback" whose "primarypath" is a GUI server handling
/oauth/callback. There is no GUI server, and no/oauth/callbackroute exists anywhere — this server serves the callback path its caller registers, and since feat(auth): add browser sign-in for workspaces that disable device codes #141
it is the only thing that receives a browser sign-in callback. At the parent commit, the only
occurrences of "GUI" and "/oauth/callback" under
src/were inside that comment describing them.A reviewer following it would conclude the code they were changing was a secondary path.
src/oauth/openai.ts's post-callback state check was labelled "defense in depth", whichunderstates it: this flow always passes
expectedState, so the callback server answers 400 andkeeps waiting rather than ever delivering a mismatched state to that branch. The guard is
deliberately kept — removing a redundant check in auth code trades a real safety margin for a
cosmetic cleanup — but the comment no longer implies it is what protects the flow today.
Tests
tests/oauth-openai.test.tsasserted only a loose regex on the port numbers, so it passed unchangedagainst the new wording. It now compares the
Errorfor equality.That distinction is the substantive test change here.
expect(...).rejects.toThrow('<string>')is asubstring match in Vitest, so an assertion written that way lets appended copy through
unasserted. Verified by appending
ARBITRARY UNPINNED SUFFIXto the production message: undertoThrow(string)the file stayed green at 22/22; undertoThrowError(new Error(...))it fails.Mutations run, full-file (not
-tisolation):Verification
pnpm typecheck && pnpm test && pnpm buildgreen under an isolatedCLODEX_HOME—103 files / 1950 tests. Re-run 5× consecutively for stability.
clodex providers auth openai --browserwith both ports heldby a bare TCP server and read the bytes on fd 1 (exit 1). Also drove it holding only 1455 and
confirmed the flow binds 1457 and completes — so "needs one free" is accurate, not a guess.
Notes and limits
one falls through to the generic listener error. Both need the internal
portsoption; thesupported CLI always supplies the two fixed ports. Hygiene, not fixed here.
callback-server.ts's own timeout message says the browser "closed" without observing that. It isthe same class again, but fix(oauth): report ignored callbacks in sign-in timeout #146 addressed the reachable case and this PR does not reopen it.