fix(codex): correct managed auth checks and browser login completion - #490
fix(codex): correct managed auth checks and browser login completion#490morluto wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
baron
left a comment
There was a problem hiding this comment.
Exact head 5530d1de92cd2357a5c47dda15ffabe9b20acd8c conflicts with the current managed-auth authority. The PR's plain recovered/authenticated results and three-task model predate current account projection, logout, device-code, mutation-generation fencing, and richer cancellation state. Please deliberately port the passive account/read(refreshToken:false) and correlated login-completion behavior while preserving those current semantics, drop obsolete ledger edits, update call sites, and rerun focused auth plus exact-head CI.
5530d1d to
2af1613
Compare
baron
left a comment
There was a problem hiding this comment.
Blocking on rewritten head 2af1613d9a0726061e4b0bc2797281347c876a58.
The passive account-read direction has merit, but the exact head does not compile: CodexManagedAuthRecoveryServiceTests.swift:674 places await inside XCTAssertEqual's non-async autoclosure. Style also fails. Please evaluate the async value before the assertion, cover interaction between the new inFlightCheck and login/refresh ownership, then rerun the full exact-head lanes.
Audit disposition — authentication concurrency blocker (2026-08-14)Passive account checks and forced refreshes use separate in-flight tasks. Because the actor is reentrant across awaits, they can overlap and publish in completion order rather than authority order; a slower passive check can overwrite the result of a successful forced refresh. Please place both operations behind one publication generation/authentication lane and add deterministic tests for both completion orders. Also distinguish a transient passive transport failure from an authenticated-state transition: “unable to determine” should not clear a known-good account or automatically become “user must log in.” The OAuth completion correlation work is good, but I would not merge with this remaining state race. |
Deep-review assessment — 2026-08-14Disposition: block pending unified auth publication fencing. Separating passive checks from forced refresh is correct at the RPC level, but the two operations have independent in-flight tasks. Because the actor is reentrant across Please put check/refresh/login/logout behind one authentication operation generation or one publication lane, and test both completion orders for simultaneous passive check + forced refresh. Also distinguish transient passive transport failure from confirmed sign-out: an inability to determine account state should not clear a known-good snapshot or automatically become “user must log in.” |
Summary
Fixes #479.
Corrects RepoPrompt's Codex managed-auth state handling so connection checks no longer invalidate otherwise usable sessions, and browser login cannot report success before OAuth actually completes.
Changes
account/read(refreshToken: false)for Test Connection and cached connection-status checks.account/login/completednotification matching the returnedloginId.account/logoutfallback when external authentication is active.User impact
Previously, a user could see this sequence:
401 refresh_token_reused.After this change:
Validation
CodexManagedAuthRecoveryServiceTests: 7 passedPromptContextPreAssemblyServiceTests: 16 passed