feat(cli): add codex auth best account switching#120
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSeverity: Minor — This PR adds a low-risk new feature ( Key Changes:
Risk Assessment:
Architectural Notes:
Validation: Typecheck, build, lint, and test suite passed; low rollback complexity (revert commit e44e0d7). Walkthroughadds a new Changes
Sequence Diagram(s)sequenceDiagram
actor user
participant cli as codex cli
participant manager as lib/codex-manager.ts
participant storage as lib/...loader
participant prober as quota prober
participant forecast as forecast engine
participant switcher as persistAndSyncSelectedAccount
user->>cli: codex auth best [--live] [--json]
cli->>manager: runBest(args)
manager->>storage: load accounts (storage)
storage-->>manager: accounts[]
alt --live flag
manager->>prober: probe live quotas
prober-->>manager: quota snapshot / errors
end
manager->>forecast: compute forecast + recommendation
forecast-->>manager: recommended account index
alt recommended != current
manager->>switcher: persistAndSyncSelectedAccount(targetIndex, switchReason="best")
switcher-->>manager: { synced, wasDisabled }
manager-->>user: output json/human result + probe notes
else already best
manager-->>user: report already-on-best
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes additional notes / concerns
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4008-4155: The runbest flow reads storage via loadAccounts, does
network refreshes, mutates bestAccount and then calls saveAccounts, creating a
TOCTOU race; fix by implementing an atomic switch helper (e.g.,
atomicSwitchToBest or commitAtomicSwitch) that reloads the latest storage just
before persisting, merges only the intended fields (activeIndex,
activeIndexByFamily entries,
bestAccount.enabled/refresh/access/expires/lastUsed/lastSwitchReason/accountId/accountIdSource/email)
and retries on conflict (use optimistic compare-and-replace or a short retry
loop), and route the existing logic to use queuedRefresh and this helper instead
of calling saveAccounts directly; ensure the helper handles filesystem EBUSY and
transient 429 errors with backoff and surface safe logs that never include raw
tokens/emails, and add/update vitest cases to cover concurrent runs and retry
behavior.
- Around line 3981-4182: Add vitest tests targeting runBest to cover missing
edge cases: create unit tests that invoke runBest with invalid args (missing
value after "--model", "--help" and unknown flags) and both Windows argument
forms ("--model=value" and "--model value") to ensure parsing sets options.model
correctly; add a test where resolveActiveIndex returns the best index to assert
idempotence (no switch, checks for the expected JSON/plain outputs); simulate
live probe failures by mocking fetchCodexQuotaSnapshot to throw (including an
error object with status 429) and assert errors are caught and recorded in
liveQuotaByIndex/refreshFailures without leaking tokens/emails in logs; create
concurrent-invocation tests that call runBest in parallel and mock queuedRefresh
to simulate EBUSY/429 and ensure the refresh queue serializes access and avoids
races (verify saveAccounts/setCodexCliActiveSelection called once as
appropriate); in mocks/stubs ensure queuedRefresh, fetchCodexQuotaSnapshot,
setCodexCliActiveSelection, resolveActiveIndex, and saveAccounts are referenced
so tests exercise auth rotation, concurrency, and syncing behavior and assert no
sensitive tokens/emails are emitted to console/logs.
- Around line 4125-4151: The refresh branch in runbest (inside the
hasUsableAccessToken check) currently ignores non-success results from
queuedRefresh; update it to mirror runswitch's behavior by detecting when
queuedRefresh returns a non-"success" type and emitting a clear warning (include
bestAccount.accountId and/or bestAccount.email and any available error details
from the refreshResult) so refresh failures are visible; keep the existing
token-update logic only for refreshResult.type === "success" and do not
overwrite credentials on failure.
- Around line 4153-4155: The persisted lastSwitchReason value "best" is being
overwritten on load because the storage deserialization/validator only allows
"rate-limit" | "initial" | "rotation"; update the storage-side type/guard that
validates account objects (the AccountSwitchReason union or the
deserialize/validateAccount function used during loadAccounts) to include "best"
as an allowed value and ensure the deserializer preserves unknown-but-valid
strings (or maps legacy values if needed); after updating the validator, run
existing saveAccounts/loadAccounts flows to confirm bestAccount.lastSwitchReason
("best") survives round-trip.
- Around line 3981-4006: The runBest function's args parsing currently ignores
unknown flags and accepts a bare "--model" without a value; update the loop in
runBest to validate flags: accept only the known flags ("--live"/"-l",
"--json"/"-j", "--model"/"-m" and "--model=..."), treat any arg starting with
"-" that isn't one of those as an error (reject and fail-fast), and require a
non-empty value for "--model" (if "--model" is followed by nothing or "--model="
yields an empty string, fail immediately). Implement the failure by returning a
non-zero exit code (or throwing) from runBest so malformed commands don't
silently succeed; adjust references to options.model accordingly when setting
the model value.
- Around line 4046-4055: The empty catch swallowing probe failures in the block
that calls fetchCodexQuotaSnapshot causes lost errors and biased account
selection; change the catch to capture the thrown error (catch (err)) and append
it to the same probeErrors collection used elsewhere (the one referenced by the
"Live check notes" logic and forecast paths), and also optionally call the
existing logger (or processLogger) with context including
probeAccountId/probeAccessToken and i so failures are recorded; ensure you still
skip setting liveQuotaByIndex on error but do not silently discard the error.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48ef9380-2250-4958-b435-7730747b1214
📒 Files selected for processing (7)
lib/accounts.tslib/codex-manager.tslib/schemas.tslib/storage/migrations.tsscripts/codex-multi-auth.jsscripts/codex-routing.jsscripts/codex.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/accounts.tslib/storage/migrations.tslib/schemas.tslib/codex-manager.ts
🔇 Additional comments (5)
lib/accounts.ts (1)
94-94:"best"is correctly added to managed account switch reasons.
lib/accounts.ts:94is consistent with the new command intent and keeps account state typing aligned.lib/storage/migrations.ts (1)
28-29: migration metadata unions are updated consistently.
lib/storage/migrations.ts:28andlib/storage/migrations.ts:53correctly allow"best"so migrated records don’t get narrowed by type declarations.Also applies to: 53-54
lib/schemas.ts (1)
85-85: switch reason schema update looks correct.
lib/schemas.ts:85now accepts"best", which matches the new switch path behavior.lib/codex-manager.ts (1)
4303-4305: dispatcher wiring forbestlooks correct.
lib/codex-manager.ts:4303(Line 4303) correctly routes torunBest(rest).scripts/codex-routing.js (1)
1-7: routing table update is correct for the new subcommand.
scripts/codex-routing.js:6now includes"best", so auth routing will recognizecodex auth best.
…witching Adds a new CLI command that automatically switches to the best standing account based on the forecast algorithm. Supports --live, --json, and --model flags. - Add 'best' to AUTH_SUBCOMMANDS in codex-routing.js - Implement runBest() function in codex-manager.ts - Add 'best' switch reason to schemas and types - Update help text and feature list Usage: codex auth best [--live] [--json] [--model <model>]
Microck
force-pushed
the
feat/auth-best-command
branch
from
3247e6b to
57fd770
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4105-4162: The duplicated account-switch logic in runBest()
(around bestIndex handling, token refresh and calling
setCodexCliActiveSelection) diverges from runSwitch() by not emitting the
refresh-failure warning; replace the duplicated block with a single reuse of
runSwitch() or extract a shared helper (e.g., switchToAccount(storage, index,
options)) and call it from both runBest() and runSwitch(); ensure the helper
preserves behavior from runSwitch() including calls to queuedRefresh(),
hasUsableAccessToken(), updating bestAccount fields (accessToken, refreshToken,
expiresAt, email, accountId, lastUsed, lastSwitchReason), saveAccounts(), and
finally setCodexCliActiveSelection(), and keep the refresh-failure warning path
identical to runSwitch().
- Around line 4028-4041: The live-probe refresh flow calls
queuedRefresh(account.refreshToken) but doesn't persist rotated tokens back into
storage, so subsequent refreshes use a stale refresh token; when
refreshResult.type === "success" (the block that sets probeAccessToken and
probeAccountId) persist the rotated tokens into the accounts store (e.g., update
storage.accounts[i].refreshToken = refreshResult.refresh and
storage.accounts[i].accessToken = refreshResult.access and accountId if
provided) and flush/save the storage or update the in-memory account used later
(so bestAccount.refreshToken won't remain stale); locate the logic around
hasUsableAccessToken, queuedRefresh, refreshResult and mirror the persistence
approach used by runFix().
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47a71baa-7ca0-4104-aba8-60f6d9da3c8f
📒 Files selected for processing (7)
lib/accounts.tslib/codex-manager.tslib/schemas.tslib/storage/migrations.tsscripts/codex-multi-auth.jsscripts/codex-routing.jsscripts/codex.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/schemas.tslib/storage/migrations.tslib/accounts.tslib/codex-manager.ts
🔇 Additional comments (12)
lib/accounts.ts (1)
94-94: lgtm — type union extended consistently.the
"best"literal aligns withlib/schemas.ts:85andlib/storage/migrations.ts:28,53. no runtime impact.scripts/codex-routing.js (1)
6-6: lgtm — subcommand registered.enables
codex auth bestrouting. consistent withlib/codex-manager.ts:4301-4303dispatcher.lib/storage/migrations.ts (1)
28-28: lgtm — storage metadata types extended.both v1 and v3 interfaces now accept
"best". migration at line 92 passes through the value unchanged.Also applies to: 53-53
lib/schemas.ts (1)
85-85: lgtm — zod schema updated.
SwitchReasonSchemanow validates"best". bothAccountMetadataV1Schema:154andAccountMetadataV3Schema:110inherit this via.optional().lib/codex-manager.ts (8)
295-295: lgtm — help text and feature registry updated.help at
lib/codex-manager.ts:295documents the command syntax. feature 41 at line 357 tracks implementation.Also applies to: 357-357
3981-4006: args parsing still accepts invalid flags silently.past review flagged this at
lib/codex-manager.ts:3981-4006.parseForecastArgs()at line 1744 already handles validation, unknown-flag rejection, and--modelvalue checks — reuse it.
4052-4054: probe errors still silently swallowed.past review at
lib/codex-manager.ts:4045-4054flagged the empty catch. compare withrunForecast()at lines 2095-2100 which collects errors intoprobeErrorsand surfaces them.
4123-4149: refresh failure during switch still silent.past review at
lib/codex-manager.ts:4123-4149flagged missing warning.runSwitch():3951emitsconsole.warn()on failure; this path doesn't.
4008-4009: toctou race window — concurrent cli runs can clobber each other.past review at
lib/codex-manager.ts:4008-4153flagged the load→network→save pattern. on windows with slow ntfs, concurrentcodex auth bestinvocations risk EBUSY or lost writes. as per coding guidelines,lib/**changes should handle EBUSY/429 scenarios.Also applies to: 4153-4153
3981-4180: no vitest coverage forrunBest()edge cases.past review at
lib/codex-manager.ts:3981-4180flagged missing tests. as per coding guidelines,lib/**: "verify every change cites affected tests (vitest)". need coverage for:
--helpflag--modelwithout value- idempotence (already on best account)
- probe 429 handling
- concurrent invocations / EBUSY
4301-4303: lgtm — dispatcher routesbestcommand.correctly calls
runBest(rest)and returns exit code.
4151-4153: verifylib/storage.tsguard accepts"best".past review at
lib/codex-manager.ts:4151-4153notedlib/storage.ts:1829had a guard that stripped unknown switch reasons. the schemas are updated, but the storage module may have a separate runtime check.#!/bin/bash # check if lib/storage.ts has a switch-reason guard and whether it includes "best" rg -n "isSwitchReason|lastSwitchReason" lib/storage.ts -B 2 -A 5
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/codex-manager.ts (1)
1768-1808: 🧹 Nitpick | 🔵 Trivialdedupe forecast/best arg parsing to avoid drift.
lib/codex-manager.ts:1768-1808 and lib/codex-manager.ts:1810-1849 duplicate nearly identical parsing logic. extracting a shared parser will keep behavior aligned and reduce future regression risk.
Also applies to: 1810-1849
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4079-4182: The CLI currently accepts options.model but only uses
it when options.live is true, causing silent no-ops; after parsing args
(parseBestArgs) and obtaining options, add an explicit check: if options.model
is set and options.live is falsy, either print a clear warning (e.g.,
console.error(...) or console.log(...)) or return a non-zero error code so the
user knows --model requires --live; implement this check near where options is
read (immediately after const options = parsedArgs.options) and make sure
messaging references both options.model and options.live so behavior is
explicit.
In `@test/codex-manager-cli.test.ts`:
- Around line 944-1070: Add a deterministic regression test that verifies
parsing and behavior of "--model <value>" when "--live" is NOT passed by
invoking runCodexMultiAuthCli(["auth","best","--model","<model>"]) with
controlled mocks (loadAccountsMock, fetchCodexQuotaSnapshotMock,
setCodexCliActiveSelectionMock, saveAccountsMock); specifically, simulate
account(s) via loadAccountsMock, stub fetchCodexQuotaSnapshotMock to return a
200 snapshot for the given model, assert fetchCodexQuotaSnapshotMock was called
with expect.objectContaining({ model: "<model>" }), and then assert expected
side effects: when current account is already best ensure saveAccountsMock and
setCodexCliActiveSelectionMock are not called and a console.log message like
"Already on best account" was emitted, and when another account is better ensure
setCodexCliActiveSelectionMock is called once and exit code is 0—use vitest
spies and deterministic timestamps similar to the existing tests to avoid
flakiness.
- Around line 864-943: Add a deterministic JSON-path assertion for the success
switch branch by invoking runCodexMultiAuthCli with the --json flag (e.g.,
runCodexMultiAuthCli(["auth","best","--live","--json"])) and reusing the
existing console.log spy (logSpy) to capture output; parse the logged JSON and
assert it contains a success status and the switched-account object with
accountId, email, accessToken, refreshToken, and expiresAt matching
storageState.accounts[1] (and the previously asserted now + 3_600_000 / idToken
values), ensuring the machine-readable contract is verified alongside the
human-readable log checks in the test for runCodexMultiAuthCli.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82ee9235-37e4-4ec2-bfde-d3927fa08d14
📒 Files selected for processing (4)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage-flagged.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-flagged.test.tstest/codex-manager-cli.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.tslib/codex-manager.ts
🔇 Additional comments (5)
lib/storage.ts (1)
1832-1835: good guard update for the new switch reason.lib/storage.ts:1832-1835 correctly preserves
lastSwitchReason: "best"during flagged storage normalization, so round-trips no longer drop this value.lib/codex-manager.ts (1)
4073-4297: please confirm vitest regressions for the newbestpaths.i can’t verify test coverage for this flow from the provided files. please confirm cases exist for zero-account json errors, already-on-best idempotence, and live probe failure handling (including 429/concurrency) for lib/codex-manager.ts:4073-4297.
run this read-only check:
#!/bin/bash set -u fd "codex-manager-cli.test.ts" test || true rg -n --type ts -C2 "auth best|runBest|already on best|No accounts configured|--model|429|concurrent|ebusy" test || trueexpected result: explicit vitest cases covering runBest edge paths and failure/concurrency behavior.
as per coding guidelines,lib/**: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.test/storage-flagged.test.ts (1)
116-133: good targeted regression for"best"persistence.test/storage-flagged.test.ts:116-133 adds a deterministic vitest check that
lastSwitchReason: "best"survives save/load, which matches the runtime guard update in lib/storage.ts.test/codex-manager-cli.test.ts (2)
531-601: good early-guard coverage forauth bestcli parsing.these tests in
test/codex-manager-cli.test.ts: Line 531throughLine 601correctly lock help, malformed args, unknown args, and no-account json error before mutation. this is solid defensive coverage for the new command surface.
1237-1342: nice concurrency regression for queued refresh reuse.
test/codex-manager-cli.test.ts: Line 1237throughLine 1342is a good targeted race test. asserting one underlying refresh network call with two concurrentauth best --live --jsonruns directly covers the token refresh race class.
ndycode
changed the title
feat(cli): add codex auth best account switching
2 participants
Summary
codex auth bestfor forecast-driven account switching--jsonsync state when the Codex CLI config update succeeds or failsauth bestedge casesValidation
npm run typechecknpm run buildnpm run lintnpm testDocs
auth bestbehavior already under reviewRisk
Rollback
04e6b3efrom the PR branch if the recency timestamp update or added test assertions cause downstream issuesNotes
04e6b3elastUsedbefore savingloadAccounts() === nullis covered inauth best --json