feat(codex): reserve main account as last resort#495
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
89b94a6 to
670d7f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/routing.ts`:
- Line 27: Persist manual-selection provenance alongside the persisted active
account, including an explicit marker for manual selection of the main account,
instead of relying on the process-local manualAccountSelections WeakMap. Update
setActiveCodexAccount() to clear the marker when routing changes the active
account, and ensure preferAddedPoolAccount() honors the marker after
configuration reloads. Add a regression test covering save/reload after manually
selecting main with a selectable added account.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ee9415f-e216-453e-9f2a-e0ea341a82c2
📒 Files selected for processing (12)
README.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdreadme/README.zh-CN.mdsrc/codex/auth-api.tssrc/codex/routing.tssrc/config.tssrc/types.tsstructure/08_openai-provider-tiers.mdtests/codex-main-rotation.test.tstests/codex-routing.test.tstests/config.test.ts
670d7f4 to
6e3d519
Compare
There was a problem hiding this comment.
Verdict
Request changes ÔÇö do not merge until provenance/upgrade contract, docs/comment accuracy, and process gates below are fixed in this PR. Optional follow-ups are not accepted for this PR; all quality items below must ship in this PR before merge.
Summary
Opt-in mainAccountLastResort correctly prefers added pool accounts for new/re-bound work, keeps quota rebalancing off main while an added account is healthy, and only admits main after added candidates are exhausted. Manual selection provenance via manualCodexAccountSelectionId (including __main__) is the right fix for the earlier WeakMap restart gap, and EN/ZH/structure docs are largely in sync. Base branch is correctly dev.
Blocking findings
None on merge conflicts.
Merge conflict status: mergeable=MERGEABLE, mergeStateStatus=UNSTABLE. Not CONFLICTING ÔÇö no conflicted files. Branch is diverged from dev (ahead_by=3, behind_by=6); rebase onto latest dev in this PR before merge (freshness / avoid landing a stale tip). Conflicts are not currently present, but divergence is not an acceptable leave-behind.
Additional required quality changes (fix in this PR before merge)
-
Pre-existing explicit main selection is not treated as manual under the new flag ÔÇö
src/codex/routing.tspreferAddedPoolAccount(~L505ÔÇô511)- Failure mode: Protection is only
manualCodexAccountSelectionId === active. A config that already hasactiveCodexAccountId: "__main__"from a prior GUI/CLI selection (no provenance field ÔÇö every install before this PR) and then enablesmainAccountLastResort: truewill have the next non-affinityresolveCodexAccountForThreadDetailedcall vacate main viasetActiveCodexAccounteven though no quota/failure event occurred. That contradicts the PR/docs contract that explicit manual selection ÔÇ£survives proxy restartsÔÇØ / ÔÇ£remains until normal quota or failure routing changes it.ÔÇØ - Why this is not free to grandfather-naively: the recovery test correctly relies on
active === __main__without a pin meaning ÔÇ£automatic last-resort occupancy,ÔÇØ which should yield to a recovered added account. Blindly treating every MAIN active as manual would break that. - Fix (pick one, implement + test):
- One-shot config migration when the provenance field is introduced (versioned / first-seen marker) that copies a pre-existing
activeCodexAccountIdintomanualCodexAccountSelectionIdexactly once; or - Document the upgrade caveat in EN + ZH config docs / README and structure SoT: enabling the flag does not inherit a pre-flag
__main__active id as a pin ÔÇö re-select main via dashboard/CLI. Add a focused regression or explicit negative test so the contract stays honest.
- One-shot config migration when the provenance field is introduced (versioned / first-seen marker) that copies a pre-existing
- Current docs claim the stronger behavior without the caveat (
docs-site/.../configuration.mdmainAccountLastResort/manualCodexAccountSelectionIdrows;structure/08_openai-provider-tiers.md).
- Failure mode: Protection is only
-
Stale
activeCodexAccountIdtype comment ÔÇösrc/types.ts(~L643ÔÇô644)- Still says
undefined = main (passthrough as-is). UndermainAccountLastResort, cleared/undefinedactive means automatic no-pin selection (often an added account), not ÔÇ£use main.ÔÇØ Update the comment to matchstructure/08and the API null/__main__contract so future callers do not reintroduce Option-A assumptions.
- Still says
-
Deduplicate main-exclusion predicates ÔÇö mild duplication between main-exclusion in
getEligiblePoolAccountsvsgetQuotaAutoSwitchCandidates. Consolidate or share the predicate in this PR so the last-resort / quota paths cannot drift; different call sites may keep distinct filters, but the shared ÔÇ£exclude main under last-resortÔÇØ rule must live in one place with coverage that both paths exercise. -
Rebase onto latest
devÔÇö tip is 6 commits behinddev. Rebase (or mergedev) in this PR and re-resolve any new conflicts before merge; do not leave divergence for a follow-up. -
Re-run / land green required CI on the rebased tip ÔÇö Cross-platform CI and React Doctor are currently
action_required(fork PR ÔÇö maintainer must approve workflow runs) and have not produced a green required suite on HEAD7e0351d. After rebase, ensure required CI is approved and green on the mergeable SHA.
Note (already addressed on current HEAD ÔÇö no further code change required beyond verification after rebase): prior CodeRabbit/Codex threads on WeakMap persistence and __main__ vs null were addressed (6e3d519, 7e0351d) and look correct.
Optional / non-blocking
None. Do not defer quality work to a follow-up PR; ship the items above in this PR.
Standards / Spec / Security
Standards
- Branch targeting:
devÔÇö OK (AGENTS.md/MAINTAINERS.md). - Bun-native / privacy: No Node-only runtime additions; no new token/account-identifier logging ÔÇö OK.
- Docs sync: EN + zh-CN configuration + README/zh README +
structure/08updated together ÔÇö OK aside from the upgrade-caveat gap above. - Tests: Solid focused coverage in
tests/codex-main-rotation.test.ts,tests/codex-auth-api.test.ts,tests/config.test.tsfor last-resort ordering, preview, affinity, save/reload pin, and__main__vsnullAPI ÔÇö OK. - Security boundary (process): Touches Codex account selection / auth-api active PUT and persisted selection provenance. Per
MAINTAINERS.md/AGENTS.md, explicit maintainer security review is required before merge. Diff does not appear to alter credential storage, token loading, or secret logging; default remainsfalse.
Spec (PR body as spec)
- Claims 1ÔÇô4, 6ÔÇô8 (prefer added, quota stays on added, failover order, main as final fallback, affinity authoritative, default-off, Direct/API-key untouched): implemented correctly on review of
preferAddedPoolAccount,getEligiblePoolAccounts,getQuotaAutoSwitchCandidates, affinity early-return before prefer, and non-mutatingpreviewCodexAccountForRequest. - Claim 5 (manual selection survives / remains until quota/failure): partial ÔÇö holds for selections made through the updated
PUT /api/codex-auth/activepath; incomplete for pre-provenance configs (finding #1). - Affinity / preview / pin-clearing / caller signature probes: no additional defects found.
setActiveCodexAccountclears the pin; delete clears a matching pin; only callers ofresetCodexRoutingForManualSelectionappear updated.
CI / process gates
| Gate | Status |
|---|---|
| Merge conflicts | MERGEABLE / not CONFLICTING |
mergeStateStatus |
UNSTABLE |
| Cross-platform CI | action_required (fork PR ÔÇö maintainer must approve workflow runs); has not produced a green required suite on HEAD 7e0351d |
| React Doctor | action_required (same) |
| enforce-target / label / CodeRabbit | green |
| Divergence | 3 ahead / 6 behind dev ÔÇö rebase required in this PR |
| Maintainer approval + security review | still required (MAINTAINERS.md) |
Do not merge until: (1) all Additional required quality changes above are addressed in this PR (no follow-up PRs), (2) required CI is approved and green on current HEAD, (3) maintainer security review is recorded.
After fixes
Once all blocking + required quality changes above are addressed (and conflicts/rebase done), mark this PR Ready for review (leave draft only until then). Then ping for re-review / CI.
Summary
mainAccountLastResortpool policy, defaulting tofalseMotivation
Pool mode currently treats the main Codex login as a first-class rotation candidate. Because it is inserted ahead of added accounts and participates in lowest-usage selection, new work can move onto the main login even while an added account is still usable.
Some users want the opposite operational policy: consume added pool accounts first and preserve the main login as a final reserve.
Behavior
The policy is explicit and backward compatible:
{ "mainAccountLastResort": true }When omitted or
false, routing is unchanged.When
true:Direct mode, the OpenAI API-key provider, credential storage, and account-qualified model selection are unchanged.
Root cause
getEligiblePoolAccounts()always inserted the main login as the first eligible candidate, while quota selection compared it directly with added accounts. There was no policy boundary separating routine quota balancing from true last-resort failover.This change keeps one eligible-account engine but applies two explicit rules: omit main while another added failover candidate exists, and exclude main from quota moves away from a still-healthy added account.
The rebase onto current
devalso preserves its immediate manual-selection contract: the selected account id is persisted as explicit provenance across proxy restarts, then cleared when ordinary quota or failure routing changes the active account.Security review
This touches Codex account selection at an authentication boundary and should receive the explicit maintainer security review required by
MAINTAINERS.md.The change does not alter token loading, credential persistence, API responses, account identifiers, or logging. The option is disabled by default.
Verification
bun test tests/codex-main-rotation.test.ts tests/codex-routing.test.ts tests/codex-auth-api.test.ts tests/config.test.ts— 245 passed, 0 failedbun run typecheck— passedbun run lint:gui— passedcd docs-site && bun run build— 131 pages builtbun run privacy:scan— passedECONNRESETfailuresorigin/dev@5ff20dcreproduction — the same 2 tests fail at the same pre-assertion fetches:websocket upgrade rejects hostile Origin even with a valid API tokenOpenAI option auth matrix keeps direct, pool, and API credentials independentChecklist
Summary by CodeRabbit
mainAccountLastResortto keep work on selectable added pool accounts, using the main login only as a final fallback after added accounts are exhausted.