feat(codex): add account picker lifecycle controls - #1303
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:
📝 WalkthroughWalkthroughThis change adds an opt-in Codex account picker, durable selector mappings, bounded catalog-convergence reporting, management API updates, GUI and CLI feedback, tests, and multilingual documentation. ChangesCodex account picker and catalog convergence
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs-site/src/content/docs/ja/reference/management-api.md`:
- Line 209: Update the table row containing GET, POST, DELETE
/api/codex-auth/accounts by adding the missing trailing pipe delimiter,
preserving the existing row content.
In `@docs-site/src/content/docs/ko/reference/cli/providers-accounts.md`:
- Around line 188-190: Update the generic success response shape in the CLI
account reference to make catalogRefreshPending optional, matching
src/cli/account-extended.ts where it is emitted only for codex credentials. Keep
the documented OAuth and API-key response examples consistent with their
omission of this field.
In `@docs-site/src/content/docs/reference/cli/providers-accounts.md`:
- Around line 246-247: Update the generic success response documentation to mark
catalogRefreshPending as optional or explicitly Codex-only, matching the
implementation for OAuth and API-key removals. Apply the same change in
docs-site/src/content/docs/reference/cli/providers-accounts.md (lines 246-247)
and docs-site/src/content/docs/ru/reference/cli/providers-accounts.md (lines
244-245); both sites require direct updates.
🪄 Autofix
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: eebc30f6-1469-4d1e-9dc3-0f3a174b0b50
📒 Files selected for processing (63)
docs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/ja/guides/web-dashboard.mddocs-site/src/content/docs/ja/reference/cli/providers-accounts.mddocs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ja/reference/configuration/routing.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/guides/web-dashboard.mddocs-site/src/content/docs/ko/reference/cli/providers-accounts.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/routing.mddocs-site/src/content/docs/ko/reference/management-api.mddocs-site/src/content/docs/reference/cli/providers-accounts.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/routing.mddocs-site/src/content/docs/reference/management-api.mddocs-site/src/content/docs/ru/guides/web-dashboard.mddocs-site/src/content/docs/ru/reference/cli/providers-accounts.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/routing.mddocs-site/src/content/docs/ru/reference/management-api.mddocs-site/src/content/docs/zh-cn/guides/web-dashboard.mddocs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/routing.mddocs-site/src/content/docs/zh-cn/reference/management-api.mdgui/src/codex-account-mutation.tsgui/src/components/AddCodexAccountModal.tsxgui/src/components/CodexAccountPickerSetting.tsxgui/src/components/CodexAccountPool.tsxgui/src/components/codex-account-pool-main-card.tsxgui/src/components/use-add-codex-account-oauth.tsgui/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/pages/CodexAuth.tsxgui/src/pages/Providers.tsxgui/src/pages/providers-page-modals.tsxgui/src/styles.cssgui/src/ui.tsxgui/tests/add-codex-account-oauth.test.tsxgui/tests/codex-account-picker-setting.test.tsxgui/tests/codex-account-pool-behaviour.test.tsxgui/tests/codex-account-pool-toast-tone.test.tsxgui/tests/providers-codex-completion-toast.test.tsxsrc/cli/account-auth.tssrc/cli/account-catalog-refresh.tssrc/cli/account-extended.tssrc/codex/account-lifecycle.tssrc/codex/auth-api.tssrc/codex/catalog-refresh-status.tssrc/server/management-api.tssrc/server/management/config-routes.tsstructure/02_config-and-codex-home.mdstructure/05_gui-and-management-api.mdtests/cli-account.test.tstests/codex-auth-api.test.tstests/codex-catalog-refresh-status.test.tstests/helpers/catalog-convergence.tstests/settings-stream-mode.test.ts
eb0b038 to
02f0414
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/auth-api.ts`:
- Around line 411-440: The account persistence flow must roll back credentials
and related runtime state when saveRuntimeConfig fails, not only restore
runtimeConfig. Update persistNewCodexAccount and the manual and OAuth callers to
stage credential changes until configuration persistence succeeds, or snapshot
and restore the prior credential, validation, and quota state in the failure
path; add failure coverage for both new-account flows.
🪄 Autofix
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: ef1d1133-f01a-499d-981c-47b2932c876c
📒 Files selected for processing (11)
docs-site/src/content/docs/ja/reference/cli/providers-accounts.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/reference/cli/providers-accounts.mddocs-site/src/content/docs/reference/cli/providers-accounts.mddocs-site/src/content/docs/ru/reference/cli/providers-accounts.mddocs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.mdgui/src/codex-account-mutation.tsgui/src/ui.tsxsrc/codex/account-lifecycle.tssrc/codex/auth-api.tssrc/codex/catalog-refresh-status.ts
02f0414 to
929c8de
Compare
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 `@docs-site/src/content/docs/zh-cn/reference/configuration/routing.md`:
- Around line 47-50: Update the Chinese model-resolution order in the routing
reference to include policy/<id> before account selectors, matching the English
routing documentation; alternatively, explicitly state that the numbered list
excludes policy-profile routes. Ensure the surrounding translated routing
behavior remains consistent with the English source.
In `@src/codex/auth-api.ts`:
- Around line 1370-1376: Update handleCodexAuthAPI’s publication-failure path so
persistence.error is converted into an explicit JSON recovery response rather
than thrown through the management dispatcher. Preserve the successful
catalogRefresh response, and return a client-safe signal such as
needsReauth/credential-missing without exposing credential-store details or file
paths.
🪄 Autofix
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: 6ef7de30-3c47-4a53-b965-f36ddca610c2
📒 Files selected for processing (5)
docs-site/src/content/docs/reference/configuration/routing.mddocs-site/src/content/docs/zh-cn/reference/configuration/routing.mdsrc/codex/auth-api.tstests/codex-auth-api.test.tstests/provider-workspace-auth.test.ts
929c8de to
0038dc1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tests/codex-auth-api.test.ts`:
- Around line 3826-3828: Remove the duplicate const data declaration in the
affected test block, keeping a single response-body read before the status
assertion.
- Around line 4310-4312: Replace the source-text assertions in the auth API test
with behavioral cases alongside the existing `isAccountNeedsReauth` HTTP
coverage: verify `reauth=1` with a valid credential and no flow row returns
`status: "expired"` rather than `done`, and verify no `reauth` parameter with a
valid credential and no reauth marker returns `status: "done"`. Remove the
assertions inspecting `handleCodexAuthAPI` source text.
- Around line 2546-2550: Add the required email field to the mocked
codexAccounts entry in the makeConfig fixture, matching the CodexAccount shape
and the neighboring fixture while leaving the other account properties
unchanged.
🪄 Autofix
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: 6cd938f6-4a9e-4872-b436-f5fe942da995
📒 Files selected for processing (12)
docs-site/src/content/docs/ja/reference/configuration/routing.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/reference/configuration/routing.mddocs-site/src/content/docs/ko/reference/management-api.mddocs-site/src/content/docs/reference/configuration/routing.mddocs-site/src/content/docs/reference/management-api.mddocs-site/src/content/docs/ru/reference/configuration/routing.mddocs-site/src/content/docs/ru/reference/management-api.mddocs-site/src/content/docs/zh-cn/reference/configuration/routing.mddocs-site/src/content/docs/zh-cn/reference/management-api.mdsrc/codex/auth-api.tstests/codex-auth-api.test.ts
0038dc1 to
dea62e4
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/codex-auth-api.test.ts (1)
3826-3828: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThe duplicated
const datadeclaration is still present.Line 3827 repeats the same statement twice in one block:
const data = await resp!.json() as { status: string; accountId?: string }; const data = await resp!.json() as { status: string; accountId?: string };This breaks the file two ways. First,
const datais redeclared in the same lexical block, which is a TypeScript and JavaScript error. Second,resp!.json()consumes theResponsebody, so the second read throwsTypeError: Body already used.The retrieved learning about repeated
constdeclarations covers separatetest()callbacks. Both declarations here sit inside the same callback block, so that exemption does not apply.🐛 Proposed fix
const resp = await handleCodexAuthAPI(req, new URL(req.url), {} as any); const data = await resp!.json() as { status: string; accountId?: string }; - const data = await resp!.json() as { status: string; accountId?: string }; expect(data).toEqual({ status: "expired" });🤖 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 `@tests/codex-auth-api.test.ts` around lines 3826 - 3828, Remove the duplicate const data declaration in the test callback around handleCodexAuthAPI, leaving a single response-body read and preserving the existing expired-status assertion.Source: Learnings
🤖 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.
Duplicate comments:
In `@tests/codex-auth-api.test.ts`:
- Around line 3826-3828: Remove the duplicate const data declaration in the test
callback around handleCodexAuthAPI, leaving a single response-body read and
preserving the existing expired-status assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: acd867ce-bf6d-406f-bdf7-db69c4705508
📒 Files selected for processing (1)
tests/codex-auth-api.test.ts
|
CI's I reran the exact shard locally: 2,416 passed, 2 skipped, 0 failed in 96s. I also ran Could someone with Actions permission rerun the failed jobs? |
|
Synced current Post-sync validation is green:
I also confirmed the same 15-minute |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
I reviewed the current head (b62a59b2) against dev for functional bugs, privacy/data leaks, security regressions, failure handling, concurrency/lifecycle edge cases, and GUI/CLI behaviour.
I like that this is an opt-in feature. That is the right default for account-qualified picker rows: users who want exact account targeting can enable it, while existing Pool/Direct behaviour and ordinary GPT picker rows remain unchanged for everyone else.
What I checked
- Picker enable/disable semantics and backward compatibility with existing/manual selector maps.
- Generated selector privacy: account IDs/emails are not used as generated public picker labels.
- Deleted-account selector retention: stale bindings stay in config for fail-closed exact routing, but deleted accounts are filtered from visible picker rows and the same selector is restored on re-add.
- New-account persistence ordering for manual import and OAuth login.
- Credential publication failures after config commit: the durable account remains recoverable, is marked
needsReauth, quota is not published prematurely, and private storage/token/path errors are not reflected to API clients. - Catalog convergence failures and malformed outcomes: management/API/GUI/CLI surfaces collapse them to the privacy-safe
catalogRefreshPendingbit and fixedocx syncrecovery guidance. - Login-status recovery so a partially published credential cannot be reported as a completed login.
- Settings validation, rollback on persistence/allocation failure, one bounded post-persistence convergence, and stale-response protection in the dashboard toggle.
- Account add/remove completion propagation through the dashboard and CLI.
- Current review threads and current-head CI. Cross-platform CI and React Doctor are green, and CodeRabbit is green at this head.
The earlier major concern around account/config atomicity and the later credential-store error exposure path are both addressed in the current code. I do not see a current blocking security, privacy, correctness, or edge-case regression in this PR.
Non-blocking, pre-existing hardening note
deleteCodexAccount() still removes credential/runtime state before the caller persists the config deletion. If that config save fails, the durable config can still contain the account while its credential has already been removed. This ordering existed before this PR, so I would not block this change on it, but it is worth fixing separately with the same config-first/compensating approach now used for new-account creation.
Approved.
2 participants
Summary
catalogRefreshPendingcompletion bit; CLI and dashboard recovery guidance uses the fixedocx synccommand.Verification
bun run typecheckbun test tests/codex-catalog-refresh-status.test.ts tests/settings-stream-mode.test.ts tests/codex-auth-api.test.ts tests/cli-account.test.ts— 320 passedcd gui && bun run lint && bun run lint:i18n && bun run build && bun run test— 696 passedcd docs-site && bun run build— 221 pages builtbun run privacy:scanbun run audit:high— no vulnerabilities foundbun test tests/repo-hygiene.test.tsnode --test .github/scripts/pr-sponsored-surface.test.cjsbun run test: 10,066 passed, 10 skipped, with one existing suite-order failure innative-profile-drain-server.test.ts; the same aggregate assertion fails on cleanupstream/dev, while the exact test passes 5/5 in isolation.Screenshot
Checklist
Summary by CodeRabbit
New Features
ocx syncrecovery guidance.Documentation