feat(codex): add account picker lifecycle settings - #1019
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 PR adds configurable Codex account-picker visibility, preserves exact selector routing, persists account changes before catalog refresh, retries refreshes, and reports ChangesCodex account picker lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountAPI
participant OcxConfig
participant CatalogRefresh
participant Catalog
Client->>AccountAPI: create, delete, or complete OAuth login
AccountAPI->>OcxConfig: persist account and selector bindings
AccountAPI->>CatalogRefresh: refresh catalog with retry
CatalogRefresh->>Catalog: write and synchronize catalog
Catalog-->>CatalogRefresh: completion result
CatalogRefresh-->>AccountAPI: catalogRefreshPending
AccountAPI-->>Client: mutation result and refresh status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb44b7d353
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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 `@src/server/management-api.ts`:
- Around line 105-109: Update refreshCodexCatalogStrict and auth-api’s
refreshAccountNamespaceCatalog to inspect the refresh result and reject when
catalogExists is false, while preserving successful refresh handling. Ensure the
existing retry and catalogRefreshPending flow receives this failure, and add a
regression covering a non-throwing incomplete refresh.
In `@tests/settings-stream-mode.test.ts`:
- Around line 359-367: Update the response assertions in the settings
stream-mode test to decode the response body once, reuse it for the existing
field checks, and assert that its serialized payload does not contain “private
refresh failure detail” in any field. Keep the existing status and refresh-count
assertions 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: 7040a690-d68b-483f-83f6-82f68682bbfd
📒 Files selected for processing (19)
src/codex/account-lifecycle.tssrc/codex/account-namespaces.tssrc/codex/auth-api.tssrc/codex/catalog-refresh-status.tssrc/codex/catalog/account-models.tssrc/config.tssrc/server/management-api.tssrc/server/management/config-routes.tssrc/server/management/context.tssrc/types.tsstructure/02_config-and-codex-home.mdstructure/03_catalog-and-subagents.mdstructure/05_gui-and-management-api.mdtests/codex-account-namespaces.test.tstests/codex-auth-api.test.tstests/config.test.tstests/native-model-toggle.test.tstests/router.test.tstests/settings-stream-mode.test.ts
fb44b7d to
eae13eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 204: Update the table row for GET, POST, DELETE /api/codex-auth/accounts
to add the missing trailing pipe and replace the redundant 「することができます」 phrasing
with the concise equivalent, preserving the row’s meaning and table formatting.
In `@src/codex/account-lifecycle.ts`:
- Around line 73-83: Extend the focused Bun regression coverage for
deleteCodexAccount and its auth-api refresh path: verify enabled pickers with a
stored pool account and matching namespace return true, while disabled pickers
or orphaned namespaces return false. Also verify deleting and re-adding the same
account preserves the namespace and triggers refresh when visibility is enabled,
using the existing lifecycle/catalog test symbols under tests/ rather than
relying only on router.test.ts.
In `@src/codex/auth-api.ts`:
- Around line 1316-1318: Make the deletion flow around deleteCodexAccount and
saveRuntimeConfig atomic: ensure the credential tombstone and runtimeConfig
account removal occur within one mutation critical section, or add compensating
rollback that restores both when saveRuntimeConfig throws
ConfigMutationLockError. Preserve the 503 response while preventing partial
deletion, and add a delete-specific test covering save failure and state
restoration.
In `@src/codex/catalog-refresh-status.ts`:
- Around line 28-36: In the retry loop surrounding refresh, add a short delay
between the failed first attempt and the second attempt, using the existing
retry-delay convention from renameAtomicFile where practical. Keep the current
two-attempt limit, immediate success return, and generic failure handling
unchanged; only pause before retrying after a caught error.
- Around line 32-35: Update the catch block in the catalog refresh retry flow to
bind the caught error and call debugProviderDiagnostic("codex",
"catalog-refresh-failed", ...) for every failed attempt. Extract the error
message explicitly rather than serializing Error directly, sanitize it with
redactSecretString and redactUserPath before recording, and preserve the
existing generic terminal warning.
In `@src/server/management/context.ts`:
- Around line 12-16: Export a shared CodexCatalogRefreshCompletion type from
catalog-refresh-status.ts and use it as the return result type of
refreshCodexCatalog in the management context. Update
assertCodexCatalogRefreshComplete to accept void | CodexCatalogRefreshCompletion
while preserving its existing behavior, and remove the duplicated inline object
shape.
🪄 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: 32d4ce77-5ce2-40da-9ea6-96e424668bb6
📒 Files selected for processing (39)
docs-site/src/content/docs/guides/codex-app-models.mddocs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/ja/guides/codex-app-models.mddocs-site/src/content/docs/ja/guides/web-dashboard.mddocs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/guides/codex-app-models.mddocs-site/src/content/docs/ko/guides/web-dashboard.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/management-api.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/reference/management-api.mddocs-site/src/content/docs/ru/guides/codex-app-models.mddocs-site/src/content/docs/ru/guides/web-dashboard.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/management-api.mddocs-site/src/content/docs/zh-cn/guides/codex-app-models.mddocs-site/src/content/docs/zh-cn/guides/web-dashboard.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/management-api.mdsrc/codex/account-lifecycle.tssrc/codex/account-namespaces.tssrc/codex/auth-api.tssrc/codex/catalog-refresh-status.tssrc/codex/catalog/account-models.tssrc/config.tssrc/server/management-api.tssrc/server/management/config-routes.tssrc/server/management/context.tssrc/types.tsstructure/02_config-and-codex-home.mdstructure/03_catalog-and-subagents.mdstructure/05_gui-and-management-api.mdtests/codex-account-namespaces.test.tstests/codex-auth-api.test.tstests/config.test.tstests/native-model-toggle.test.tstests/router.test.tstests/settings-stream-mode.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eae13eb170
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…-lifecycle-settings
…-lifecycle-settings
…-lifecycle-settings
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (1)
docs-site/src/content/docs/reference/configuration/providers.md (1)
20-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument exact account-route retention and pool management.
The new text says that exact selector routing uses the binding map. It does not state that an account-qualified route keeps its selected account and never falls through to another credential. It also omits the CLI commands that manage the same account pools.
docs-site/src/content/docs/reference/configuration/providers.md#L20-L21: State that exact<selector>/<native-openai-model>routes retain the selected account without Pool fallback. Distinguish Pool routing from Direct routing. Addocx account list,ocx account current, andocx account use.docs-site/src/content/docs/ja/reference/configuration/providers.md#L19-L20: Add the equivalent Japanese wording.docs-site/src/content/docs/ko/reference/configuration/providers.md#L19-L20: Add the equivalent Korean wording.As per path instructions, “exact account-qualified routes retain their selected account and do not silently fall through to other credentials; users can manage the same account pools with
ocx account list/current/use.”🤖 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 `@docs-site/src/content/docs/reference/configuration/providers.md` around lines 20 - 21, Update the provider configuration documentation to state that exact account-qualified routes retain their selected account and never silently fall back to other credentials, while distinguishing Pool routing from Direct routing. Add the account-pool management commands ocx account list, ocx account current, and ocx account use to docs-site/src/content/docs/reference/configuration/providers.md lines 20-21, and add equivalent wording in docs-site/src/content/docs/ja/reference/configuration/providers.md lines 19-20 and docs-site/src/content/docs/ko/reference/configuration/providers.md lines 19-20.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 `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Around line 20-21: Update the provider configuration documentation to state
that exact account-qualified routes retain their selected account and never
silently fall back to other credentials, while distinguishing Pool routing from
Direct routing. Add the account-pool management commands ocx account list, ocx
account current, and ocx account use to
docs-site/src/content/docs/reference/configuration/providers.md lines 20-21, and
add equivalent wording in
docs-site/src/content/docs/ja/reference/configuration/providers.md lines 19-20
and docs-site/src/content/docs/ko/reference/configuration/providers.md lines
19-20.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 60b52466-2be5-4b6d-98bb-a1e08cdf8dd5
📒 Files selected for processing (23)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/management-api.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/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/management-api.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/management-api.mdgui/src/styles.csssrc/cli/account-auth.tssrc/codex/account-namespaces.tssrc/config.tssrc/routing/profile-namespace.tssrc/routing/profile.tssrc/server/management-api.tssrc/types.tstests/cli-account.test.tstests/codex-account-namespaces.test.tstests/config.test.tstests/settings-stream-mode.test.ts
💤 Files with no reviewable changes (1)
- gui/src/styles.css
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (1)
docs-site/src/content/docs/reference/configuration/providers.md (1)
20-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the catalog recovery action in all locales.
The
codexAccountPickerEnabledrows describe persisted picker changes but omit the recovery action when catalog refresh remains pending. Add a localized instruction or link to the existing guidance: runocx sync.
docs-site/src/content/docs/reference/configuration/providers.md:20-21docs-site/src/content/docs/ja/reference/configuration/providers.md:19-20docs-site/src/content/docs/ko/reference/configuration/providers.md:19-20docs-site/src/content/docs/ru/reference/configuration/providers.md:20-21docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:19-20🤖 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 `@docs-site/src/content/docs/reference/configuration/providers.md` around lines 20 - 21, Update the codexAccountPickerEnabled documentation to include a localized recovery instruction or link to the existing guidance to run “ocx sync” when catalog refresh remains pending. Apply this change in docs-site/src/content/docs/reference/configuration/providers.md:20-21, docs-site/src/content/docs/ja/reference/configuration/providers.md:19-20, docs-site/src/content/docs/ko/reference/configuration/providers.md:19-20, docs-site/src/content/docs/ru/reference/configuration/providers.md:20-21, and docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:19-20, translating the instruction appropriately for each locale.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 `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Around line 20-21: Update the codexAccountPickerEnabled documentation to
include a localized recovery instruction or link to the existing guidance to run
“ocx sync” when catalog refresh remains pending. Apply this change in
docs-site/src/content/docs/reference/configuration/providers.md:20-21,
docs-site/src/content/docs/ja/reference/configuration/providers.md:19-20,
docs-site/src/content/docs/ko/reference/configuration/providers.md:19-20,
docs-site/src/content/docs/ru/reference/configuration/providers.md:20-21, and
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:19-20,
translating the instruction appropriately for each locale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f1d057d-b60d-4f3d-a05e-e66c4a474897
📒 Files selected for processing (5)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
…-lifecycle-settings
…-lifecycle-settings
…-lifecycle-settings
…-lifecycle-settings
Summary
codexAccountPickerEnabledlifecycle setting for account-qualified Codex picker entries.policynamespace and slash-qualified routing-profile alias prefixes so picker enable and account add cannot create ambiguous or unloadable combined configs.catalogRefreshPendingwithout leaking internal errors when the catalog is missing, unwritten, or its models cache cannot be invalidated.ocx syncrecovery warning in the dashboard and CLI when an account change succeeds but its catalog refresh remains pending; JSON clients retain the boolean completion field.Dashboard feedback
Verification
bun run typecheckbun run privacy:scancd docs-site && bun run build(216 pages)git diff upstream/dev...HEAD --check96c33aa8): 8,504 pass and 10 skip; the same unrelated native-main sideband admission timing test failed only under aggregate load. Its exact regression then passed 10 consecutive isolated runs.dev, with no remaining findings.devsync (ebcfff44): tri-state CodexfastModeinjection is merged; its 52 affected tests plus 193 picker/config regressions, typecheck, privacy scan, and diff-check pass.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation