fix(codex): name the account that left the pool instead of only dropping it (#4212) - #4233
Conversation
…ing it isAccountNeedsReauth() made an account unselectable and told callers nothing, so every surface that wanted to explain a refusal had to re-derive the cause somewhere else. That is how an operator ends up reading a healthy account card while routing is quietly dropping the account and the model it backed. codexAccountUnusableReason() now holds every branch of the decision and returns the cause, and isCodexAccountUsable() is its boolean projection rather than a second copy of the same checks. A reason cannot name a cause routing did not use, and routing cannot refuse an account for a cause no surface can name. The Codex account surface carries that through: poolAccountDto and the main row emit reauthReason alongside needsReauth, separating a credential that was never stored from a refresh that keeps failing and from a rejected usage lookup. /api/oauth/accounts already used that field name. nativeMainRefreshFailureResponse stays a retryable 503 with Retry-After, because the refresh may still succeed, but no longer reads as a pure transient fault: it now says that a persistent failure means the main account needs reauthentication. The pool-account 401 is deliberately unchanged. server-search.test.ts asserts that message must not carry the account id, alias, or email. Refs #4212
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfe2b5ee50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : runtimeReauth | ||
| ? "refresh_failed" | ||
| : quotaResult.needsReauth | ||
| ? "quota_unauthorized" |
There was a problem hiding this comment.
When getValidCodexToken throws TokenRefreshError before the WHAM request, fetchFreshPoolAccountQuota returns needsReauth: true without setting the runtime flag, so this code reports quota_unauthorized even though credential refresh failed. Conversely, a terminal WHAM 401 marks the runtime flag before returning, causing this precedence order to report refresh_failed for a quota authorization rejection. The new public field therefore reverses the two documented causes in normal failure paths; carry the originating reason through PoolQuotaResult or the runtime state instead of inferring it from overlapping booleans.
Useful? React with 👍 / 👎.
| "Codex main credential refresh did not complete; retry this request. " | ||
| + "If it keeps failing, the main Codex account needs reauthentication.", |
There was a problem hiding this comment.
This message also handles every retryable NativeProfileError, including contention failures such as PROFILE_LOCK_UNAVAILABLE, NATIVE_PROFILE_BUSY, and NATIVE_MAIN_CLAIM_BUSY, as well as MainAuthJsonChangedDuringRefreshError. Those conditions can persist while another profile operation owns the lock, but reauthentication cannot resolve them and may unnecessarily replace a valid credential. Keep the reauthentication advice for errors that establish a failed or invalid refresh grant, while retaining retry/busy guidance for profile coordination errors.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 68 / 80이 PR은 지금 지금 계정 목록 API 쪽도 맞춰 갑니다. 이 PR이 #4212를
PR 본문 - 아직 “Stacked on #4230 … Retarget to
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
isAccountNeedsReauth(accountId)makes the account unselectable "with no reason carried to callers". So the reason now comes from the same function as the decision.codexAccountUnusableReason()holds every branch and returns the cause;isCodexAccountUsable()is its boolean projection rather than a second copy of the same checks. A reason cannot name a cause routing did not use, and routing cannot refuse an account for a cause no surface can name.poolAccountDtocomputedneedsReauthas an OR of three independent causes plus a persisted verdict resolved inside the health projection, and emitted only the boolean. It now also emitsreauthReason, separatingmissing_credential(a credential that was never stored) fromrefresh_failed(the reporter's case) andquota_unauthorized(the usage lookup itself was rejected). The main row carries it too, so the field's contract holds for every row./api/oauth/accountsalready used that field name.nativeMainRefreshFailureResponsesaid "retry this request" and nothing else. It stays a retryable503withRetry-After, because the refresh genuinely may succeed, and now adds that a failure which persists means the main account needs reauthentication.docs-site/src/content/docs/guides/codex-integration.mdand its seven locale copies.Deliberately not changed: the pool-account request-time 401.
tests/server/server-search.test.ts:344asserts that message must not contain the account id, alias, or email — naming the account there would reverse a privacy decision this repository already made, which is not this change's to make.Out of scope, recorded as follow-ups. The 503 the reporter quoted is inlined separately at
src/server/responses/core.ts:2336andcompact.ts:383, and the model-list drop is published fromsrc/codex/catalog/sync.ts:1777. None of the three is in this lane's owned path list, so this isRefs, notCloses.Refs #4212
Stacked on #4230, which this branch targets. Retarget to
devonce #4230 lands.Verification
tests/codex-integration/codex-account-unusable-reason.test.ts, registered in bothscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json. It pins each reason (needs_reauth,missing_credential,not_in_pool,model_not_entitled,main_credential_unavailable), asserts the boolean projection never disagrees with the reason, and checks the 503 keeps its status andRetry-Afterwhile naming reauthentication.tests/codex-integration/codex-auth-api.test.tsto assertreauthReasonat the DTO layer: present asrefresh_failedwhen the account genuinely needs the operator, and absent otherwise.bun run typecheck, andbun run build:guiwere NOT RUN, by operator instruction for this round. Hosted CI on the exact pushed head is the only product evidence this change is offered on.xai/grok-4.6subagents audited this in parallel before push. The equivalence audit on theisCodexAccountUsablesplit returned pass: no input changes the truth value, helper call count, call order, or throw set;readCodexAccountRecordis still called exactly once and only after the existence and reauth checks, theisMainAccountTokenLiveseam still fires 0 or 1 times rather than 2, and the expanded pool tail is truth-equivalent for a null record, a record without a credential,deletedAtset, andcodexValidationPending. The DTO/error audit returned pass with five non-blocking findings; three were folded in (the main row now carriesreauthReason, the union comment no longer overclaims what the health projection can currently produce, and the DTO-layer assertion was added), and a re-audit of the fold-in returned pass.Checklist
This touches an authentication surface, so it was read for disclosure specifically.
reauthReasonis a closed enum with no free text, the new 503 names "the main Codex account" and carries no id, email, or token, and the account-naming that would have leaked is the one change deliberately left out. No new logging.