feat(codex): let an operator keep a downgraded account out of pool rotation (#4211) - #4238
Conversation
…tation When a ChatGPT subscription lapses, the account is downgraded to Free and keeps taking production traffic until requests start failing. codexPool.excludedPlans lets an operator name the plan keys automatic selection skips. It is absent by default, so an existing install rotates exactly as before. The filter goes where pause already goes, in both isCodexAccountSelectable and the getEligiblePoolAccounts pool-row chain. That is not redundancy: the eligible list is only consulted when routing picks a NEW account, while an account that is already active or already bound to a thread is served straight out of isCodexAccountSelectable. A lapsed subscription leaves behind exactly that account, so filtering only the eligible list would have shipped a config key that reads correctly and changes nothing for the reporter. Two deliberate limits. The main account is exempt, because selection-only routing withholds its plan rather than reading the fenced native credential, so a rule covering it would answer differently under drain than under ordinary routing. And unlike pause, an excluded account still answers when no unexcluded candidate remains: #4211 asks for a selection policy rather than a hard block, and pausing every account is still how you stop serving entirely. No minimumPlan. Ranking ChatGPT plans against each other needs a total ordering this repository does not have. A malformed policy degrades to no policy rather than failing the parse, so a hand-edited typo cannot trip the backup-and-defaults repair path. Because that makes it a successful parse, the write path rejects it and loadConfig warns on all three success paths instead of letting it disappear in silence. Refs #4211
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. |
|
✅ Deterministic PR hygiene checks passed. |
|
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 |
리뷰 · 우선순위 70 / 80이 PR은 지금 지금 설정은 기본값이 없는 설정 파싱도 pause/알림 블록과 같은 패턴입니다.
베이스 브랜치 로컬 스위트 NOT RUN - 패킷 규칙상 정상이다. 다만 라우팅 이중 게이트·플랜 키 정규화·설정 degrade warn은 회귀 면적이 크니, 호스티드 CI의 codex-integration 샤드가 초록인지 머지 직전에 한 번 더 확인하자. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 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: 46e2a3774c
ℹ️ 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".
| .filter(account => isSelectableCodexPoolAccount(account) | ||
| && account.id !== excludeId | ||
| && !isCodexAccountPaused(config, account.id) | ||
| && !isCodexAccountPlanExcluded(config, account.id, excludedPlans) |
There was a problem hiding this comment.
When the operator uses the supported auto mode (activeCodexAccountId is unset), the main account is unavailable, and every configured pool account has an excluded plan, this filter removes every candidate; resolveCodexAccountForThreadDetailed then returns none from its no-active branch. Requests therefore fail authentication instead of honoring the documented guarantee that an excluded account still answers when no unexcluded account remains. Preserve a valid excluded candidate as the last resort and add a regression test with no active account.
Useful? React with 👍 / 👎.
|
|
||
| ### Keeping a downgraded account out of rotation | ||
|
|
||
| `codexPool.excludedPlans` lists plan keys that automatic pool selection skips, matched case-insensitively against the plan stored on each account. It is absent by default, so an existing install rotates exactly as before. |
There was a problem hiding this comment.
This introduces a top-level persisted configuration section, but docs-site/src/content/docs/reference/configuration/providers.md still lists the adjacent pool fields without codexPool or excludedPlans. Users relying on the configuration reference therefore cannot discover the field's type, default, validation, or main-account exception; update that directly affected field table alongside this guide.
AGENTS.md reference: docs-site/AGENTS.md:L13-L16
Useful? React with 👍 / 👎.
1 participant
Summary
codexPool.excludedPlansnames the plan keys automatic pool selection skips, matched case-insensitively against the plan already stored on each account. It is absent by default, so an existing install rotates exactly as before.isCodexAccountSelectableand thegetEligiblePoolAccountspool-row chain. That is not redundancy, and it is the one place this PR departs from the plan of record, which named onlygetEligiblePoolAccounts. The eligible list is consulted only when routing picks a new account; an account that is already active or already bound to a thread by affinity is served straight out ofisCodexAccountSelectable, atsrc/codex/routing.ts:2126and:2198, without the eligible list being built at all. Priority preemption cannot rescue it either, because every account defaults to priority 0. A lapsed subscription leaves behind precisely that account, so filtering only the eligible list would have shipped a config key that reads correctly and changes nothing for the reporter. Pause is checked in both places for the same reason, and pause is the manual workaround the issue itself names.Two deliberate limits, both pinned by tests:
getPoolAccountPlanForSelectionwithholds the main plan during a selection-only drain so routing never reads the fenced native credential for it. A rule covering main would exclude it under ordinary routing and not under drain — the same account, two answers. The audit confirmedisCodexAccountPlanExcludedreturns at the__main__check before any plan lookup, so no new native-credential read exists on any routing path.No
minimumPlan: ranking ChatGPT plans against each other needs a total ordering this repository does not have. A malformed policy degrades to no policy rather than failing the parse, so a hand-edited typo cannot trip the backup-and-defaults repair path and wipe providers or pool accounts — and because that makes it a successful parse, the write path rejects it andloadConfigwarns on all three success paths rather than letting the policy disappear in silence.Out of scope.
docs-site/src/content/docs/reference/configuration/providers.mdcarries the field table wherepausedCodexAccountIdsandcodexAccountPrioritiesare listed, and this key belongs beside them. That file is not in this lane's owned path list, so the key is documented in the Codex integration guide instead and the reference-table row is reported as a follow-up. The dashboard and CLI display halves the issue also asks for needsrc/cli/account.tsand a GUI component, which are likewise out of scope — henceRefs, notCloses.Refs #4211
Stacked on #4233, which this branch targets. Retarget down the chain as each parent lands.
Verification
tests/codex-integration/codex-pool-plan-exclusion.test.ts, registered in bothscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json. It pins that no policy and an empty list both leave rotation unchanged, that an excluded plan loses a new pick it would otherwise win on usage, that an account already serving a thread stops serving it once its plan is excluded (the reported case), that matching ignores casing and whitespace, that an account with no recorded plan is never excluded, and that the last remaining account still serves rather than stranding the operator.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 reviewed this. Two returned fail and both were folded in. The routing audit found four of the eight tests would fail: three because afreeplan is thirty-day-only and scores on the monthly window while the fixture recorded weekly only, so the account scoredCODEX_UNKNOWN_USAGE_SCOREand lost the ranking even with no policy; and one becausepreviewCodexAccountForRequesttakes(threadId, config)and was called with one argument. The config audit found the schema comment claimedloadConfigwarns when the warning had only been wired into the diagnostics arrayocx config show --sourceprints — which, given.catch(undefined)makes a malformed policy a successful parse, meant the proxy would start, rotate onto the excluded accounts, and print nothing. Both fixed, plus a hoisted per-rowSetrebuild. The re-audit returned pass on every point: all eight tests predicted to pass, both gates matching pause,__main__exempt before any plan read, and an absentcodexPoola total no-op.Checklist
This changes account selection, so the failure modes were reviewed rather than the secrets. The policy cannot widen access: it only removes candidates from automatic rotation, never adds one. It introduces no new credential read — the
__main__exemption is checked before any plan lookup specifically so the fenced native credential is not touched.excludedPlansholds plan labels, not identifiers, and nothing new is logged.