fix(codex): let an operator account selection outrank the pool cursor - #4284
Conversation
|
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. |
리뷰 · 우선순위 74 / 80이 PR은 Codex 계정 풀이 이미 활성 계정을 B로 옮긴 뒤에, 운영자가 대시보드나 원인을 짧게 말하면 이렇습니다. GUI와 CLI는 둘 다 같은 고치는 모양은 Anthropic 쪽에 이미 있는 문서 쪽도 같이 옵니다. 지금 src/codex/routing.ts / 베이스 브랜치 - 베이스가 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: 7260439d35
ℹ️ 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".
| if (commitSharedActive) { | ||
| if (!isIndependentCodexQuotaScope(quotaScope)) rememberActiveCodexAccount(config, picked); | ||
| if (!isIndependentCodexQuotaScope(quotaScope) | ||
| && !manualPreferenceBlocks(codexPoolKeyForScope(quotaScope), picked)) { | ||
| rememberActiveCodexAccount(config, picked); |
There was a problem hiding this comment.
When a round-robin dispatch on the preferred account fails, the preference remains unspent, but this guard only suppresses rememberActiveCodexAccount; it does not change the picked account returned to the caller. Because notePoolRotationSuccess advances the ring when the account is selected rather than when its upstream outcome succeeds, later unbound requests can route to another account while the preference is still live, contradicting the consume-on-success behavior. Select the live preferred account before consulting the ring while it remains eligible, and mirror that choice in preview, instead of merely hiding the automatic pick from the effective cursor.
Useful? React with 👍 / 👎.
| // | ||
| // Seeding happens ONLY here. A pool-driven promote must never create or move a preference, | ||
| // or the pool would manufacture an operator intent nobody expressed. | ||
| manualPreference.set(POOL_KEY_CODEX, accountId); |
There was a problem hiding this comment.
The active-account API calls this function with MAIN_CODEX_ACCOUNT_ID even when the request body contains accountId: null, which explicitly clears rather than creates a selection. This unconditional insertion therefore manufactures a main-account preference; if the Desktop/main account is unavailable and a pool account serves subsequent requests, no success can consume that entry and manualPreferenceBlocks suppresses automatic cursor updates indefinitely, leaving GET /api/codex-auth/active null or stale. Pass the clear operation separately or allow this reset to skip preference creation.
Useful? React with 👍 / 👎.
| function forgetManualPreference(accountId: string): void { | ||
| for (const [poolKey, preferred] of manualPreference) { | ||
| if (preferred === accountId) manualPreference.delete(poolKey); |
There was a problem hiding this comment.
This cleanup is only invoked by reconcileCodexActiveAfterExclusion, while deleteCodexAccount instead calls purgeCodexAccountRuntimeState and clearCodexUpstreamHealthForAccount, neither of which removes the new preference. Deleting a manually selected account before its successful dispatch therefore leaves an impossible-to-consume entry that continues blocking every replacement account from becoming the runtime cursor. Invoke this cleanup from per-account purge/reconciliation as well as the pause path.
Useful? React with 👍 / 👎.
46df5f9 to
e1e8b25
Compare
7260439 to
63217d1
Compare
The audit drove the first version of these tests red on the parent branch: all three passed with the production change reverted, because they only re-asserted what resetCodexRoutingForManualSelection and the deliberately exempt failover promote already did. Three defects fall out of that. Deletion reaches routing through clearCodexUpstreamHealthForAccount, which did not revoke the preference, so a preference could outlive its account and suppress every later write. The generation sweep had the same hole. The model-detour promote wrote over the operator's selection while preemption next to it yielded. The independent-scope preference entries were written and consumed but never read by any guard. Co-authored-by: Heisenberg <audit@localhost>
Guarding it failed 8 cases in codex-routing.test.ts. A model detour runs because the operator account cannot serve the model at all, and under a rotating strategy the promote moves only the process-local cursor, never the persisted selection.
reconcileCodexRoutingHealth had no test at all, so the preference sweep added for the delete-path blocker was verified by reading rather than by running. Both halves are covered now: an account the generation no longer lists loses its preference, and one that is still listed keeps it. Red control: removing the four sweep lines makes the first case read undefined.
…ards clearCodexUpstreamHealth dropped every health map but kept lastReconciledGeneration and liveHealthAccountIds, so after a full reset recordCodexUpstreamOutcome would still discard a writer whose generation predated the watermark for any account missing from the equally stale live set. Found because the watermark leaked between test files: the new generation-sweep coverage failed two unrelated cases in codex-routing.test.ts when both files ran in one process.
2 participants
Summary
Fixes the reported defect: the pool moves the active Codex account to B, the operator then selects A through the dashboard or
ocx account use, and the runtime keeps serving B.The cause was not the write path the operator takes. GUI and CLI both issue the same
PUT /api/codex-auth/active, and that handler already clears the runtime cursor. The cause is that nothing stopped the next automatic pick from immediately overwriting it again:rememberActiveCodexAccountis reached from the round-robin commit, the fill-first commit, the failover promote and priority preemption, and none of them consulted the operator's choice. The pin was only ever a priority-tier ceiling, not a selection input.This adds an unspent one-shot preference, mirroring the
manualPreferenceshape Anthropic already has and Codex lacked.Three things make it safe, and each was forced by a measured failure rather than guessed:
recordCodexUpstreamOutcomeconsumes it on a successful dispatch for that scope. Wiring the guard without a consume site first makes the preference permanent and freezes the automatic cursor: that shape fails 15 of 69 rotation tests.getEffectiveActiveCodexAccountIdkeeps its existing contract. Overlaying the preference there breaks the callers that now treat it as the live shared cursor, including health evidence and the management snapshots.The user-facing rule, stated plainly: an account past its switch threshold is temporarily spent, not wrong. The pool serves the request from another account, and the operator's selection stays pointed where the operator put it, so the window rolling over returns routing to it without a second manual pick.
What the last audit round changed
The first version of this PR shipped three tests that all passed with the production change reverted. They re-asserted what
resetCodexRoutingForManualSelectionand the deliberately exempt failover promote already did, so the guard, the preference map and the consume site had no coverage at all. Driving them red first surfaced three real defects, now fixed here:reconcileCodexActiveAfterExclusion; delete does not. It reaches routing throughclearCodexUpstreamHealthForAccount, which cleared two health maps and left the preference behind. Once the named account is gone nothing can succeed on it, so the one-shot could never be spent and every later automatic write stayed suppressed until process restart. The generation sweep inreconcileCodexRoutingHealthhad the same hole.Each replacement case is red against the variant that removes the piece it covers:
b, expecteda63217d161: readsundefined, expectedbDesign and the full audit trail:
devlog/_plan/260911_account_pool_unification/010_phase1_manual_selection.md. Six audit rounds and three measured implementation passes are recorded there, including the two failing shapes above and the empty-coverage round.Verification
bun x tsc --noEmit- passbun test tests/codex-integration/codex-pool-rotation.test.ts- 74 pass, 0 fail63217d161- 1 fail (the delete case)bun run privacy:scan- passedChecklist