feat(codex): add account pause controls and bulk exhaustion action - #565
feat(codex): add account pause controls and bulk exhaustion action#565Alvin0412 wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds persisted Codex account pausing, quota-confirmed bulk pausing, routing exclusions, API endpoints, GUI controls, localized text, lifecycle cleanup, configuration documentation, and automated coverage. ChangesCodex account pausing
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AccountPoolUI
participant useCodexAccountPool
participant CodexAuthAPI
participant Config
AccountPoolUI->>useCodexAccountPool: pause account or pause exhausted accounts
useCodexAccountPool->>CodexAuthAPI: send pause mutation
CodexAuthAPI->>Config: persist pausedCodexAccountIds
CodexAuthAPI-->>useCodexAccountPool: return paused state and active account
useCodexAccountPool-->>AccountPoolUI: render updated account state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@lidge-jun The PR Labeler is failing before it reaches any branch code. Run 30292826165 reports |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@gui/src/hooks/useCodexAccountPool.ts`:
- Around line 242-266: Translate the "__main__" sentinel when reconciling
main-account pause state: in gui/src/hooks/useCodexAccountPool.ts:242-266,
update setAccountPaused’s setAccounts predicate to match either the requested id
or "__main__" with account.isMain; in
gui/src/components/codex-account-pool-main-card.tsx:96-113, compare
pauseUpdatingId to "__main__" for the Saving indicator; in
gui/tests/codex-account-pool-behaviour.test.tsx:128-157, add a regression test
using a main account whose real id differs from "__main__" and assert
setAccountPaused("__main__", true) updates its paused flag.
In `@gui/tests/codex-account-pool-behaviour.test.tsx`:
- Around line 128-157: Add a test for setAccountPaused("__main__", true) using a
fixture whose isMain account has a distinct real id, then assert the matching
account’s paused flag becomes true locally and the operation succeeds. Place it
alongside the existing direct-id pause tests to cover the sentinel path used by
CodexAccountPoolMainCard.
🪄 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: 7d3f2b55-7d88-465f-a6a0-5a10ee59a03c
📒 Files selected for processing (34)
README.mddocs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdgui/src/components/CodexAccountPool.tsxgui/src/components/codex-account-pool-cards.tsxgui/src/components/codex-account-pool-main-card.tsxgui/src/hooks/useCodexAccountPool.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/icons.tsxgui/tests/codex-account-pool-behaviour.test.tsxgui/tests/codex-account-pool-controller.test.tsgui/tests/codex-account-pool-toast-tone.test.tsxsrc/codex/account-lifecycle.tssrc/codex/account-pause.tssrc/codex/auth-api.tssrc/codex/quota.tssrc/codex/routing.tssrc/codex/subagent-model-fallback.tssrc/config.tssrc/types.tsstructure/08_openai-provider-tiers.mdtests/codex-auth-api.test.tstests/codex-auth-context.test.tstests/codex-routing.test.tstests/config.test.tstests/rate-limit-reset-credits.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gui/src/hooks/useCodexAccountPool.ts (1)
259-262: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize active-account reconciliation across pause and switch mutations.
setAccountPausedandpauseExhaustedAccountsunconditionally overwritependingActiveIdRefandactiveId, whileswitchAccountcan run concurrently becausepauseMutationRefdoes not cover it. If a pause response arrives after a newer switch response, it can replace the accepted active account with a stale value. Subsequent loads treat the mismatch as a stale read and keep the incorrect pending value, leaving the GUI out of sync with the management API.Serialize these mutations or use a shared monotonic mutation/request token so only the latest operation commits
activeIdandpendingActiveIdRef. Add a delayed-response regression test covering switch-versus-pause ordering.As per path instructions, GUI state changes must stay consistent with management API responses.
Also applies to: 286-289
🤖 Prompt for 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. In `@gui/src/hooks/useCodexAccountPool.ts` around lines 259 - 262, Serialize active-account mutations across switchAccount, setAccountPaused, and pauseExhaustedAccounts, or introduce a shared monotonic request token, so only the latest management API response updates activeId and pendingActiveIdRef. Ensure stale pause responses cannot overwrite a newer switch result, while preserving GUI state consistency with the accepted API response, and add a delayed switch-versus-pause regression test.Source: Path instructions
gui/src/components/codex-account-pool-main-card.tsx (1)
146-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDisable header actions during individual pause mutations.
The header buttons only check
refreshingQuota || pausingExhausted. WhilesetAccountPausedis in flight,pauseUpdatingIdis non-null but “Pause exhausted” remains clickable and immediately receives{ ok: false, reason: "busy" }; refresh can also run concurrently with the pause reconciliation.Pass a unified pause-busy value (for example,
pauseUpdatingId !== null || pausingExhausted) intoCodexAccountPoolPageHeadand use it for both buttons. Add a UI test asserting both controls are disabled during an individual pause.As per path instructions, GUI state changes must stay consistent with management API responses.
🤖 Prompt for 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. In `@gui/src/components/codex-account-pool-main-card.tsx` around lines 146 - 159, Update CodexAccountPoolPageHead and its caller to derive a unified pause-busy state from pauseUpdatingId being non-null or pausingExhausted, then use that state alongside refreshingQuota to disable both header actions. Preserve management API response handling and add a UI test verifying both controls are disabled while an individual pause mutation is in flight.Source: Path instructions
🤖 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.
Outside diff comments:
In `@gui/src/components/codex-account-pool-main-card.tsx`:
- Around line 146-159: Update CodexAccountPoolPageHead and its caller to derive
a unified pause-busy state from pauseUpdatingId being non-null or
pausingExhausted, then use that state alongside refreshingQuota to disable both
header actions. Preserve management API response handling and add a UI test
verifying both controls are disabled while an individual pause mutation is in
flight.
In `@gui/src/hooks/useCodexAccountPool.ts`:
- Around line 259-262: Serialize active-account mutations across switchAccount,
setAccountPaused, and pauseExhaustedAccounts, or introduce a shared monotonic
request token, so only the latest management API response updates activeId and
pendingActiveIdRef. Ensure stale pause responses cannot overwrite a newer switch
result, while preserving GUI state consistency with the accepted API response,
and add a delayed switch-versus-pause regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 417f282b-2dfb-4773-ad42-909aeee379d2
📒 Files selected for processing (3)
gui/src/components/codex-account-pool-main-card.tsxgui/src/hooks/useCodexAccountPool.tsgui/tests/codex-account-pool-behaviour.test.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84247cd904
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36946b9ec3
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gui/src/hooks/useCodexAccountPool.ts (1)
272-297: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBulk "pause exhausted" doesn't handle the
"__main__"sentinel, and no test catches it.pauseExhaustedAccountswas added without the sentinel-translation logic already present in its siblingsetAccountPaused, and the new bulk-pause test never exercises the main-account path to catch the gap.
gui/src/hooks/useCodexAccountPool.ts#L272-L297: extend thesetAccountspredicate at Line 285 to also matchpausedIds.has("__main__") && account.isMain, mirroring Line 255'ssetAccountPausedlogic.gui/tests/codex-account-pool-behaviour.test.tsx#L194-L209: add a bulk-pause regression test where thepause-exhaustedmock returnspausedAccountIds: ["__main__"]against a fixture whoseisMainrow has a real id distinct from"__main__", asserting that row'spausedflag flips locally (mirroring the existingsetAccountPaused("__main__", true)test at Lines 149-166).🤖 Prompt for 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. In `@gui/src/hooks/useCodexAccountPool.ts` around lines 272 - 297, Update pauseExhaustedAccounts in gui/src/hooks/useCodexAccountPool.ts lines 272-297 so its setAccounts predicate marks an account paused when pausedIds contains "__main__" and account.isMain, matching setAccountPaused behavior. Add a regression test in gui/tests/codex-account-pool-behaviour.test.tsx lines 194-209 using a distinct real ID for the isMain fixture, returning pausedAccountIds: ["__main__"], and asserting the main row’s local paused flag changes.Source: Path instructions
src/codex/auth-api.ts (1)
751-766: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winNormalize the target account before the pause check
Insrc/codex/auth-api.ts:751-766,body.accountId: nullskipsisCodexAccountPaused(...), but the same handler later treatsnullasMAIN_CODEX_ACCOUNT_IDfor routing. That lets a paused main account be reselected through/api/codex-auth/active. Normalize first, then check the pause state, and add a regression test for thenullbody path.🤖 Prompt for 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. In `@src/codex/auth-api.ts` around lines 751 - 766, Normalize body.accountId to the effective target account, using MAIN_CODEX_ACCOUNT_ID when it is null, before calling isCodexAccountPaused in the PUT /api/codex-auth/active handler. Use that normalized target consistently for pause validation and resetCodexRoutingForManualSelection, while preserving the response’s existing accountId value; add a regression test covering a null accountId when the main account is paused.
🤖 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.
Outside diff comments:
In `@gui/src/hooks/useCodexAccountPool.ts`:
- Around line 272-297: Update pauseExhaustedAccounts in
gui/src/hooks/useCodexAccountPool.ts lines 272-297 so its setAccounts predicate
marks an account paused when pausedIds contains "__main__" and account.isMain,
matching setAccountPaused behavior. Add a regression test in
gui/tests/codex-account-pool-behaviour.test.tsx lines 194-209 using a distinct
real ID for the isMain fixture, returning pausedAccountIds: ["__main__"], and
asserting the main row’s local paused flag changes.
In `@src/codex/auth-api.ts`:
- Around line 751-766: Normalize body.accountId to the effective target account,
using MAIN_CODEX_ACCOUNT_ID when it is null, before calling isCodexAccountPaused
in the PUT /api/codex-auth/active handler. Use that normalized target
consistently for pause validation and resetCodexRoutingForManualSelection, while
preserving the response’s existing accountId value; add a regression test
covering a null accountId when the main account is paused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 624e6bba-d379-4b4f-9d0e-06c5b514a7b9
📒 Files selected for processing (5)
gui/src/hooks/useCodexAccountPool.tsgui/tests/active-account-reauth-aggregate.test.tsgui/tests/codex-account-pool-behaviour.test.tsxsrc/codex/auth-api.tstests/codex-auth-api.test.ts
|
Addressed both outside-diff findings from the latest CodeRabbit review in c1be956: bulk pause now maps the |
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 `@gui/tests/codex-account-pool-behaviour.test.tsx`:
- Around line 240-243: Update the gated reload test around releaseReload and the
subsequent act block to assert the reconciled GUI state after the reload
completes. Verify that the main account remains paused and that activeId still
matches the value selected by the management API, covering both state fields
rather than only awaiting the reload.
🪄 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: 75b47cdb-c11a-4cf9-9a85-fa6824f4cdc5
📒 Files selected for processing (4)
gui/src/hooks/useCodexAccountPool.tsgui/tests/codex-account-pool-behaviour.test.tsxsrc/codex/auth-api.tstests/codex-auth-api.test.ts
|
Please resolve the Conflicts. After that I will review your PR. Thanks. |
|
Thanks — the conflicts are resolved in edf04e6. The PR is mergeable again and the current checks pass. I also added regression coverage for pause behavior with the new quota, round-robin, and fill-first account-pool strategies. Ready for your review. |
Wibias
left a comment
There was a problem hiding this comment.
Maintainer review (bug + security)
Verdict: APPROVE
Security
No medium+ issues in the pause/control-plane surface. Pause endpoints sit on the existing /api/codex-auth management auth model; list/bulk responses keep the masked account DTO (no tokens). PUT /active rejects paused accounts (409). Pause is enforced in isCodexAccountSelectable and related failover/affinity/subagent paths.
Bug review / open Codex + CodeRabbit
| Item | Status |
|---|---|
Rabbit: __main__ sentinel UI desync |
Fixed (Rabbit confirmed) |
Rabbit: __main__ / gated-reload test coverage |
Fixed (Rabbit confirmed) |
| Codex P2: classify pool exhaustion with fresh plan | Fixed (freshPlan from WHAM) |
| Codex P2: require known main plan before pause | Fixed (omit plan_type → fail/skip, not mis-pause) |
| Codex P2: align next/active after pausing current | Fixed (reconcileCodexActiveAfterExclusion) |
| Codex P2: recheck existence before persisting bulk pauses | Fixed (delete/gen race → discard) |
| Codex P2: report bulk refresh total failure | Fixed (502 when nothing checked) |
| Codex P2: exclude paused from active reauth projection | Fixed (!activeAccount?.paused) |
Focused regressions exercised here: 15 pass (pause / exhausted filters in codex-auth-api + codex-routing).
Notes (non-blocking)
- Fork head currently shows only label/CodeRabbit checks — run/wait for full cross-platform CI before merge.
- Residual edge case: if WHAM omits
plan_typefor a pool account, classification falls back to storedcurrentAccount.plan(stricter than inventing a plan). Acceptable given fail-closed when no plan can be established; optional follow-up is stronger plan cache parity with main. - Partial bulk refresh (
complete: false) still uses the success toast path — UX polish only.
|
Will get merged after interal code work lands on dev. |
|
Superseded by maintainer takeover #667 (conflict resolve + CI fix). |
Resolve conflicts between lidge-jun#565 pause work and account namespace foundation on current dev. Keep both pausedCodexAccountIds and codexAccountNamespaces; align pause id validation with CODEX_ACCOUNT_ID_RE / isValidCodexAccountId.
Maintainer takeover of #565. Persisted pause exclusion, bulk pause-exhausted, GUI controls, docs. Integrated with account namespaces and Spark quota scopes on current dev.
What changed
Why
A dedicated eligibility control is more general than special-casing the main account as a last resort. It lets users explicitly reserve any account, including the main login, without allowing retries, resets, or fallback paths to silently reintroduce it.
The bulk action removes repetitive manual work while keeping the decision strict: routing's conservative unknown-quota score is not reused, so an unknown or stale quota cannot be mistaken for exhaustion.
Behavior
Pause affects future Pool selections. In-flight requests keep the credential captured when they started. Quota metadata remains visible and refreshable. Direct mode continues to use its existing single-main-account semantics.
Validation
server-authtests fail with BunECONNRESETserver-authfailures reproduce unchanged on the untouched7ba0fecbase checkout (52 passed, 2 failed)Summary by CodeRabbit
New Features
pausedCodexAccountIdsto persist pause exclusions across restarts.Bug Fixes
Documentation