fix(codex): stop selecting accounts that cannot serve the request (#4768, #4778) - #4797
Conversation
…es the model A pool holding a Plus account and a Free account handed `gpt-5.6-sol` and `gpt-6-astra` to whichever account rotation reached first, and the Free account answered with the upstream unsupported-model 400. The roster evidence needed to avoid that already existed; selection never consulted it, because entitlement is only activated for `ACCOUNT_GATED_NATIVE_OPENAI_MODELS` and the flagships were deliberately removed from that set on 2026-09-04. Adding them back is not the fix. That set fails closed on ABSENCE of evidence -- membership hides the row from the catalog and refuses before dispatch -- so a timed-out fetch or a lagging shard would make the model vanish from the picker, which is #3022. So this adds evidence of the opposite polarity. `cachedDeniedCodexAccountIdsForModel` reports only accounts whose own confirmed roster definitively omits the model, and `getEligiblePoolAccounts` drops them before the priority tier -- unless that would leave no candidate, in which case the full list is restored. Unknown, stale, expired and too-old-client rosters stay unknown and change nothing. Nothing refuses, so the existing bounded alternate-account retry on an exact unsupported-model 400 remains the safety net, and it now picks an entitled alternate for the same reason. The read is synchronous and cache-only. The gated path may await discovery because a gated model is rare; the flagships are the most requested models in the product, and an authenticated fetch per account on that path would trade one occasional 400 for latency on every turn. Background catalog sync already warms the full roster per account, so the evidence is there without asking for it. Worst case on the happy path is unchanged selection: a single-account pool, an install with no cached rosters, and a pool where every account is denied all resolve exactly as they do today. The source-oracle assertion in the fallback-preview test is relaxed from one pinned property to "both sites still forward `modelEligibleAccountIds`", which is what it says it is protecting; the options object now also carries the per-candidate preference. Closes #4768
…loaded files #4710 made an orphaning account change fail closed with `409 account_change_file_scope`. That stops the invalid request but does not restore the conversation: the file reference stays in history, so every later turn presents it and is refused the same way. Pool rotation is automatic, so any conversation with an attachment was one rotation away from being permanently blocked, and both escapes the error names -- re-upload, or start over -- ask the user to absorb a routing decision they never made and cannot see. The refusal was the middle step. This is the other half: retain the binding so the rotation that causes it happens less often. `conversationCarriesUploadedFiles` already answers from the body alone, and it is the same predicate the refusal guard uses, so routing and refusal cannot disagree about which conversations are in scope. `resolveResponsesCodexAuth` carries the answer into `retainAccountForUploadedFiles`, and `mayRebindAffinityForQuota` applies the default cache-affinity bar even when `pool.cacheAffinity` is false. That flag trades cache locality for capacity; it was never asking to trade correctness for capacity, and under the default it already retains a healthy bound account, so this is inert there. Voluntary moves only. Genuine exhaustion and an unusable account still release the binding, and every involuntary release that runs earlier -- quota refusal, failover streak, pause, cooldown, lost generation, expiry -- is untouched. A pinned conversation therefore cannot be wedged on an account that cannot serve it, which is why the #4710 refusal stays: this reduces how often it fires and can never replace it. The preview path carries the same flag so subagent fallback is not scored against a move the request will not make. Structure: adds "Uploaded-file account retention" to openai-tiers.md and resolves the forward reference in transports/responses.md, which reserved this to routing. Also records the #4768 always-visible-flagship selection contract in the account-gated natives section, which that commit changed without documenting. Closes #4778
The entitlement ordering added for #4768 runs before `selectPriorityTier`, so a pinned account that a roster denies was removed from the candidate list. That is more than a demotion: `selectPriorityTier` reads the pin to lower the tier ceiling, so a pin filtered out beforehand stops acting as a ceiling at all and silently re-enables the tiers the operator had excluded. Roster evidence orders the pool's own discretion; it does not overrule an explicit human choice. An operator who pins an account that upstream will refuse still gets the bounded alternate-account retry -- what they do not get is the pool quietly deciding they were wrong. Eligibility is unchanged either way. `isCodexAccountSelectable` remains the sole authority for pause, plan exclusion, quota cooldown and avoidance, soft avoidance, refresh cooling and usability, and `codexAccountBlockReason` still reports which guard fired; this rule only ever narrows a list those guards already produced, and still restores it in full when narrowing would leave nothing. Refs #4768
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe change adds cached model-entitlement evidence to Codex account selection and retains accounts for conversations with uploaded files during voluntary quota moves. It updates request wiring, routing predicates, tests, documentation, and layout metadata. ChangesAccount routing behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ResponsesRequest
participant EntitlementCache
participant AccountSelection
participant AuthContext
participant AccountPool
ResponsesRequest->>EntitlementCache: read cached denial evidence for model
EntitlementCache-->>ResponsesRequest: denied account IDs or no evidence
ResponsesRequest->>AccountSelection: preview with denial and retention options
AccountSelection->>AccountPool: choose entitled account or retain bound account
AccountPool-->>AccountSelection: selected account
AccountSelection->>AuthContext: resolve final account with same options
AuthContext-->>ResponsesRequest: resolved account
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. |
CI caught this, and the diagnosis matters more than the patch: ordering `getEligiblePoolAccounts` was not enough, because that list is not the only door into selection. An account that is already ACTIVE is served straight from `isCodexAccountSelectable` and never passes through it, so once the denied account became the shared cursor every request kept going to it -- which is precisely the case #4768 reports. `pickPriorityPreemption` does not rescue it either: it refuses to move toward a tier that does not strictly outrank the active one, and a Free account sitting in the lower tier is exactly that shape. `preferModelEntitledAccount` closes it, and stays inside "order the already-eligible set" by construction. It admits nothing, because the replacement is drawn from `getEligiblePoolAccounts` and every eligibility guard has already passed on it. It cannot fail, because with no entitled alternative it returns the active account unchanged, so a served request can never become `none`. It changes nothing without evidence, so absent `deniedModelAccountIds` it is the identity function. The result is deliberately NOT persisted -- one request's correction for one model, like a model detour -- and a pinned active account is exempt outright. Preview applies the same correction so subagent fallback cannot score a model against an account the request will not use. `mayRebindAffinityForQuota` and `retainsBoundAccountForQuota` move to the new `src/codex/routing/cache-affinity.ts`. The retention policy is a question two call sites ask and it reads better owned by one module than inlined in `routing.ts`; it also returns that file to its file-size ratchet cap, which the previous head had exceeded. Refs #4768 #4778
Three files had grown past their file-size ratchet caps. The ratchet only ever lowers a baseline, so the cases move rather than shrink. `tests/codex-integration/codex-routing.test.ts` returns byte-for-byte to its `dev` contents, and the seven cases it had gained now live in `codex-account-selection-preferences.test.ts` alongside two new ones. It is registered in both `scripts/test-layout/layout.json` and `tests/fixtures/test-layout-expected.json` with identical values, which `test-layout-tooling` compares directly. `subagent-fallback-handle-responses.test.ts` was over by exactly the comment lines added with the relaxed source-oracle regex, so the comment is rewritten to say the same thing in the space it already had. The two new cases cover the gap that let the active-cursor defect ship. Every existing case supplied denial evidence, so none of them pinned the shared-cursor path or what happens with NO evidence at all -- which is why ten green tests sat beside a broken one. One now pins that a denial moves the request off the cursor without persisting the move, and its companion pins that absent evidence, and evidence about an account the cursor does not name, both leave that path exactly as it was. The baseline file is untouched. Refs #4768 #4778
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: fe3d006db5
ℹ️ 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".
| const identity = currentCredentialIdentity(accountId); | ||
| if (identity !== undefined && identity !== entry.credentialIdentity) continue; |
There was a problem hiding this comment.
When a flagship request is resolved while native-main reads are forbidden—such as during profile switching/recovery or for a caller-owned bearer—this loop still processes a cached __main__ entry and currentCredentialIdentity() calls getMainAccountToken(), reading auth.json inside the fence. The gated entitlement path explicitly excludes main before credential access, but this projection has no equivalent exclusion, so pass the excluded account IDs or validate from already-cached identity state without touching the physical credential.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| const identity = currentCredentialIdentity(accountId); | ||
| if (identity !== undefined && identity !== entry.credentialIdentity) continue; |
There was a problem hiding this comment.
For every Sol/Terra/Luna/Astra request with warm roster evidence, this loop invokes currentCredentialIdentity() once per cached account-version entry; for pool accounts that calls readCodexAccountRecord(), which synchronously hardens, reads, and parses the entire account store. With the documented bounds of four versions per account and 64 accounts, one request can perform up to 256 full synchronous store reads on Bun's request thread. Snapshot each distinct account identity once, or retain an epoch-backed identity alongside the cache, rather than doing filesystem work per entry.
Useful? React with 👍 / 👎.
| const finalAuth = await resolveResponsesCodexAuth( | ||
| req, | ||
| config, | ||
| route, | ||
| options, | ||
| credentialDomainWasRewritten, | ||
| conversationCarriesUploadedFiles(parsed._rawBody), |
There was a problem hiding this comment.
The retention flag is wired only through the regular Responses authentication path. Native compact dispatch resolves the same pool/thread context directly in src/server/responses/compact.ts:731 without retainAccountForUploadedFiles; therefore, with pool.cacheAffinity: false and an over-threshold issuing account, a compact request containing file_id can still voluntarily move to another account and immediately fail the existing account-change guard. Thread the same body predicate into compact's initial auth resolution.
Useful? React with 👍 / 👎.
| { | ||
| ...recoverySelectionOptions, | ||
| modelEligibleAccountIds, | ||
| deniedModelAccountIds: cachedDeniedCodexAccountIdsForModel(modelId, previewNow), | ||
| }, |
There was a problem hiding this comment.
When encrypted task recovery reruns subagent fallback, this reconstructed preview options object includes model eligibility and denial evidence but drops retainAccountForUploadedFiles. For a file-carrying bound thread with cache affinity disabled and quota above the threshold, the recovery preview can therefore move to account B and choose a fallback model based on B, while final authentication retains account A. Carry the uploaded-file retention bit into recoverySelectionOptions so preview and final resolution remain consistent.
Useful? React with 👍 / 👎.
1 participant
Summary
Two pool-selection failures users hit today, fixed inside the existing eligibility gate.
A pooled account whose plan cannot serve the requested model could be selected, and the upstream then returned an unsupported-model 400 (#4768). The entitlement machinery already existed but only ran for ACCOUNT_GATED_NATIVE_OPENAI_MODELS, which holds Daybreak alone. Adding the flagships to that set would have failed closed: a timed-out fetch or a lagging shard would make Sol or Astra vanish from the picker, which is #3022 and which the 2026-09-04 owner decision deliberately removed them from. Instead this reports evidence of the opposite polarity, listing only accounts whose own confirmed roster definitively omits the model and dropping those inside getEligiblePoolAccounts ahead of the priority tier. Unknown, unconfirmed, expired and too-old-client rosters stay unknown, a grant under any client version clears a denial recorded under another, and the full list is restored whenever filtering would leave no candidate. The read is synchronous and cache-only, so no upstream fetch joins the request path.
A conversation carrying uploaded files was refused after rotation instead of staying on the account that issued the files (#4778). conversationCarriesUploadedFiles is the predicate the existing refusal already uses, so it is reused rather than duplicated, and the retention applies the default cache-affinity bar even when pool.cacheAffinity is false. That flag trades cache locality for capacity; it was never asking to trade correctness for capacity, and under the default it already retains a healthy bound account, so this is inert there.
The third commit fixes a regression found while auditing against the release contract: the denial filter ran before selectPriorityTier, so a pinned account a roster denied was removed from the list entirely. That is worse than a demotion, because selectPriorityTier reads the pin to lower the tier ceiling, so a filtered-out pin silently re-enables the tiers the operator excluded. The pin is now exempt, with a regression that fails on the pre-fix code.
isCodexAccountSelectable and codexAccountBlockReason are byte-identical to dev. Both new fields are ignored by account-usability.ts and absent from sharedStateSelectionOptions, so neither can become an eligibility boundary or leak into shared routing state.
Closes #4768
Closes #4778
Verification
Checklist
Summary by CodeRabbit
New Features
Documentation