refactor: split auth CLI help and command flows#142
refactor: split auth CLI help and command flows#142ndycode wants to merge 7 commits intoplan/01-beginner-onboarding-docsfrom
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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthroughwalkthroughrefactors auth command logic out of changes
sequence diagram(s)mermaid estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes review notes
🚥 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
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: 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/auth-commands.ts`:
- Around line 290-294: The doc comment warns of a race when concurrent `codex
auth best --live` runs refresh the same account and both call `saveAccounts`;
add a runtime guard by acquiring the same refresh-lease/file-lock mechanism used
in `lib/codex-manager.ts` (the refresh-lease dedupe) around the live-probe +
refresh path in `lib/codex-manager/auth-commands.ts` so that the live probe
acquires the lease before mutating tokens and releases it after `saveAccounts`;
also add a regression test that spawns two concurrent invocations of the `auth
best --live` code path and asserts that only one process performs the
refresh/save while the other waits or skips, documenting expected serialized
behavior.
In `@test/codex-manager-auth-commands.test.ts`:
- Around line 455-540: Add a regression test that reproduces concurrent
live-probe writes by invoking runBest(["--live"]) twice in parallel, using the
existing mocks (loadAccountsMock, saveAccountsMock,
setCodexCliActiveSelectionMock, createStorage) and delaying one save via a
Promise to force an interleaving; assert both runBest calls return successfully
and that storage is not corrupted (e.g., final storage.activeIndex and account
fields match an expected consistent state or that saveAccountsMock calls
preserve order/atomicity), or alternatively assert saveAccountsMock was
effectively serialized (no overlapping save starts) to prevent race conditions;
place this test alongside the other runBest tests and reference runBest and
saveAccounts in the test to make the concurrency regression explicit.
🪄 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: 15d3dd4e-403a-4917-85f6-6594b7f8a020
📒 Files selected for processing (6)
lib/codex-manager.tslib/codex-manager/auth-commands.tslib/codex-manager/help.tstest/codex-manager-auth-commands.test.tstest/codex-manager-help.test.tstest/documentation.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/documentation.test.tstest/codex-manager-help.test.tstest/codex-manager-auth-commands.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/codex-manager/help.tslib/codex-manager.tslib/codex-manager/auth-commands.ts
🔇 Additional comments (14)
test/documentation.test.ts (1)
244-269: lgtm, the doc integrity test correctly tracks the new module locations.the assertions now check
lib/codex-manager/help.tsandlib/codex-manager/auth-commands.tsfor usage text alignment, which matches the refactor's extraction of help and auth command logic.test/codex-manager-help.test.ts (1)
1-63: good coverage of the pure arg parsers.the test suite validates flag parsing for
parseAuthLoginArgsandparseBestArgsincluding edge cases like missing model values and unknown flags. since these are synchronous, deterministic parsers, no concurrency tests are needed here.lib/codex-manager.ts (3)
82-88: clean import wiring for the extracted modules.the imports from
./codex-manager/help.jsand./codex-manager/auth-commands.jsalign with the refactor goals.
4239-4249: delegation to extracted command functions looks correct.each of
runAuthLogin,runSwitch,runBestnow forwards to the corresponding module function with the appropriate dependency object.
4320-4349: dependency objects correctly wire the required helpers.
authCommandHelpersincludesresolveActiveIndex,hasUsableAccessToken,applyTokenAccountIdentity, andnormalizeFailureDetailas required byAuthCommandHelpersinterface inlib/codex-manager/auth-commands.ts:62-76.
authLoginCommandDepsspreadsauthCommandHelpersand adds all the required fields fromAuthLoginCommandDepsinterface atlib/codex-manager/auth-commands.ts:78-135.lib/codex-manager/help.ts (1)
1-153: the extracted help module is clean and well-typed.
parseAuthLoginArgsandparseBestArgsreturn discriminated unions that clearly separate success, help, and error states. no token or sensitive data handling here.test/codex-manager-auth-commands.test.ts (2)
272-421: solid coverage of the core persistence and sync flows.the tests validate:
- re-enabling disabled accounts and refreshing stale tokens at
test/codex-manager-auth-commands.test.ts:273-316- family index clamping for restored pools at
test/codex-manager-auth-commands.test.ts:318-377- warning surfacing when refresh fails at
test/codex-manager-auth-commands.test.ts:379-421
542-620: test covers restore-backup family index clamping well.the assertions at lines 612-618 verify that out-of-bounds indices like
9and-3are clamped to valid bounds (2and0). this matches the clamping logic inclampPreservedActiveIndexByFamilyatlib/codex-manager/auth-commands.ts:137-156.lib/codex-manager/auth-commands.ts (6)
1-49: imports look correct for the extracted module.the module imports the required dependencies from internal modules without any circular dependency risks.
137-156: clamping logic correctly bounds family indices.
clampPreservedActiveIndexByFamilyatlib/codex-manager/auth-commands.ts:141-155iteratesMODEL_FAMILIESand clamps each index to[0, maxIndex]. the test attest/codex-manager-auth-commands.test.ts:318-377verifies this handles negative and out-of-range values.
158-236: persistAndSyncSelectedAccount handles token refresh and persistence correctly.the function:
- re-enables disabled accounts at lines 180-183
- updates activeIndex and activeIndexByFamily at lines 185-191
- refreshes tokens if unusable at lines 199-221
- persists via saveAccounts at line 225
- syncs to codex cli at lines 227-234
the warning at line 217-219 uses
normalizeFailureDetailwhich truncates and sanitizes error messages perlib/codex-manager.ts:177-187, so no token leakage.
674-692: backup discovery error handling is safe.the debug log at lines 679-682 only logs
codeanderror.message, not tokens or sensitive data. the warning at lines 684-686 is a generic user-facing message without secrets.
730-788: restore backup confirmation flow is well-structured.the flow:
- prompts user confirmation at lines 731-736
- restores without persisting at line 749-752
- clamps family indices via
preserveActiveIndexByFamily: trueat line 759- persists via
persistAndSyncSelectedAccountat lines 754-761- handles StorageError with formatted hints at lines 776-777
this addresses the behavioral regression mentioned in the pr summary regarding multi-family index bounds clamping.
343-420: saveAccounts already handles EBUSY with exponential backoff retry—concern addressed.the probe loop at lib/codex-manager/auth-commands.ts:349-420 accumulates changes and calls
persistProbeChangesIfNeededto write them once. this is efficient. concurrent writes are already protected:saveAccountsat lib/storage.ts:2157 wraps the operation withwithStorageLockfor in-process serialization. the underlyingsaveAccountsUnlockedat lib/storage.ts:1997-2019 explicitly retries EBUSY on the final rename with exponential backoff (up to 5 attempts).renameFileWithRetryat lib/storage.ts:456-481 does the same for backup operations. test/storage.test.ts:2646 verifies the EBUSY retry behavior. the concern is valid but already mitigated.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/auth-commands.ts`:
- Around line 824-837: Add direct vitest unit tests for runAuthLogin that
simulate its error branches by mocking the restore/persistence and storage
modules: create tests that (1) throw a StorageError with code "EBUSY" when the
restore is attempted and assert formatStorageErrorHint is used and flow
continues, (2) throw a StorageError with code "EACCES" and assert the same
hint-handling path, (3) throw a rate/429-equivalent error from the
persistence/restore call and assert the generic error path is taken, and (4)
simulate a failure followed by loadAccounts resolving to a non-empty accounts
array to verify the recovery branch (uses loadAccounts mock to return accounts
after the failed restore); reference runAuthLogin, StorageError,
formatStorageErrorHint, loadAccounts, and loadNamedBackupsForOnboarding when
locating code to mock and assert.
In `@test/codex-manager-auth-commands.test.ts`:
- Around line 272-700: Add regression tests that simulate Windows EBUSY
filesystem errors by making saveAccountsMock (and where relevant
persistProbeChangesIfNeeded/restoreAccountsFromBackupMock) reject with an Error
that has code "EBUSY" and an appropriate message; for
persistAndSyncSelectedAccount assert the promise rejects with the EBUSY error
and that setCodexCliActiveSelectionMock is not called, for runBest --live
simulate a mid-probe failure of persistProbeChangesIfNeeded and assert the
command handles/logs the failure and returns the expected exit code while not
corrupting concurrent saves, and for the backup-restore flow add a test where
restoreAccountsFromBackupMock rejects with code "EBUSY" and assert runAuthLogin
surfaces the error (or returns non-zero) and does not call saveAccountsMock;
reference test helpers and mocks: persistAndSyncSelectedAccount, runBest,
runAuthLogin, saveAccountsMock, persistProbeChangesIfNeeded,
restoreAccountsFromBackupMock, setCodexCliActiveSelectionMock when adding these
tests.
- Around line 608-686: Add a new test that simulates restoreAccountsFromBackup
throwing a StorageError and asserts the CLI handles it: mock
restoreAccountsFromBackupMock to reject with new StorageError("msg") (use the
same StorageError type imported in the code), call runAuthLogin with deps that
choose the restore flow (promptOAuthSignInMode -> "restore-backup",
promptBackupRestoreMode -> "latest"), then expect runAuthLogin to return a
non-zero error code, expect confirmMock to have been invoked, expect
saveAccountsMock not to be called, and assert the error was logged (spy on
console.error or the logger used). This tests the error path around
restoreAccountsFromBackup in runAuthLogin / auth-commands restore flow.
🪄 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: 78b7071d-a17a-418b-8870-f70f4a541894
📒 Files selected for processing (2)
lib/codex-manager/auth-commands.tstest/codex-manager-auth-commands.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)
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/codex-manager/auth-commands.ts
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/codex-manager-auth-commands.test.ts
🔇 Additional comments (11)
lib/codex-manager/auth-commands.ts (7)
65-66: lgtm: module-level serialization state.the combination of
liveBestLeaseCoordinatorfor cross-process lease coordination andserializedLiveBestByStoragefor in-process queue serialization addresses the concurrency concern from the past review. tests attest/codex-manager-auth-commands.test.ts:542-606validate the in-process serialization path.
143-179: solid serialization implementation.the two-layer approach (in-process queue at line 147-158 + cross-process lease at line 162) correctly prevents concurrent
--liveprobes from racing on the same storage path. the nested finally blocks at lines 165-177 ensure cleanup even when actions throw.
181-200: index clamping logic is correct.properly handles out-of-bounds indices including negative values (clamped to 0) and values exceeding account count (clamped to maxIndex). tested at
test/codex-manager-auth-commands.test.ts:318-377.
282-332: runSwitch validation flow is correct.validates input before mutating storage, bounds-checks after loading, and warns on sync failure without blocking the local switch. tested at
test/codex-manager-auth-commands.test.ts:423-438.
504-526: conditional sync for already-best accounts looks correct.when the current account is already best but was refreshed during live probe, the code syncs updated tokens to codex cli at lines 513-522 without unnecessarily re-persisting unchanged storage. good optimization.
267-279: saveAccounts already implements EBUSY retry with exponential backoff.lib/storage.ts:2001-2020 contains a 5-attempt retry loop for windows EPERM/EBUSY on the rename operation, with exponential backoff (10ms × 2^attempt). regression tests at test/storage.test.ts:2597 ("saveAccounts EPERM/EBUSY retry logic") explicitly cover this path—including success cases on second/third attempt and exhaustion after 5 failures. the coding guidelines requirement for windows filesystem IO EBUSY handling is already met.
> Likely an incorrect or invalid review comment.
383-387: saveAccounts already retries EBUSY; the gap is test coverage for exhaustion in live best mode.
saveAccountswrapssaveAccountsUnlocked(lib/storage.ts:2078-2081), which implements EBUSY/EPERM retry with exponential backoff over 5 attempts. if all retries fail, the error propagates. persistProbeChangesIfNeeded at lib/codex-manager/auth-commands.ts:383-387 doesn't catch this, so a persistent EBUSY after exhausted retries would terminate the live best command.regression test is missing: add a test in test/codex-manager-auth-commands.test.ts that mocks saveAccounts to reject with EBUSY after 5 attempts and verifies graceful failure or recovery in live mode. test/storage.test.ts already covers saveAccounts retry exhaustion (test/storage.test.ts:2646), but live best integration test coverage for this path is absent.
test/codex-manager-auth-commands.test.ts (4)
89-100: partial storage mock preserves real types.using
vi.importActualto keepStorageErrorand other types while mocking i/o functions is the right approach for these tests.
124-166: test factories are well-designed.
createStorageandcreateHelpersprovide sensible defaults that tests can override. reduces boilerplate while keeping tests readable.
222-270: deterministic test setup with fake timers.fixed system time at line 224 and
vi.clearAllMocks()at line 225 ensure tests are reproducible and isolated.
542-606: concurrent serialization test is solid.this test directly exercises the serialization added to address the past review comment. the
maxActiveSaves === 1assertion at line 602 confirms no overlapping saves occur. good coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-manager-auth-commands.test.ts`:
- Around line 279-285: Add a test that verifies runBest(["--live"]) propagates
lease acquisition failures: mock leaseAcquireMock to reject with an Error (e.g.,
with code "EACCES"), ensure loadAccountsMock resolves normally, then assert that
await runBest(["--live"], createHelpers()) rejects with that same error and that
saveAccountsMock and setCodexCliActiveSelectionMock were not called; target the
mocks and functions named leaseAcquireMock, RefreshLeaseCoordinator.acquire,
runBest, loadAccountsMock, saveAccountsMock, and setCodexCliActiveSelectionMock
when adding the test.
🪄 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: 7c60887f-863f-4a70-b1d2-ba7d0ef2c400
📒 Files selected for processing (1)
test/codex-manager-auth-commands.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 (1)
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/codex-manager-auth-commands.test.ts
🔇 Additional comments (6)
test/codex-manager-auth-commands.test.ts (6)
107-125: good partial mock strategy for storage.js and refresh-lease.js.preserving real
StorageErrorandformatStorageErrorHintexports viavi.importActualattest/codex-manager-auth-commands.test.ts:107-108lets error-path tests assert on actual hint formatting. lease coordinator mock attest/codex-manager-auth-commands.test.ts:121-125with bypass role prevents filesystem writes during test runs.
149-191: helper factories are cleanly typed and avoid real secrets.
createStorageandcreateHelpersattest/codex-manager-auth-commands.test.ts:149-191use placeholder tokens ("refresh-token-1","access-token-1") rather than real credentials. the typed overrides pattern keeps test setup DRY while allowing per-test customization.
247-306: deterministic timer setup with proper cleanup.fake timers pinned to
2026-03-22T19:30:00.000Zattest/codex-manager-auth-commands.test.ts:248-249ensure reproducible behavior for expiry checks and lastUsed timestamps.vi.clearAllMocks()andvi.restoreAllMocks()maintain test isolation.
459-478: good regression coverage for windows EBUSY errors.
test/codex-manager-auth-commands.test.ts:459-478verifiespersistAndSyncSelectedAccountpropagates the EBUSY error and skips the codex sync call. combined with theit.eachat lines 788-839 covering both EBUSY and EACCES for restore paths, this addresses the past review concern about missing windows filesystem error tests.
612-676: concurrency regression test properly verifies serialized saves.
test/codex-manager-auth-commands.test.ts:612-676blocks the first save, starts a secondrunBest(["--live"]), and assertsmaxActiveSaves === 1at line 672 proving no interleaved writes. this addresses the prior review concern about missing regression tests for concurrent live-probe writes perlib/codex-manager/auth-commands.ts:290-294.
708-786: backup-restore flow tested with clamping and error paths.
test/codex-manager-auth-commands.test.ts:708-786validates the happy path withactiveIndexByFamilyclamping (out-of-range indices snapped to pool size). error paths at lines 788-923 coverStorageErrorhint formatting, generic failures, and branching back to the existing-account menu when restore fails post-login.
ndycode
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
All review threads are resolved and follow-up commits addressed this stale automated change request.
1 participant
Summary
lib/codex-manager.tsauth login,auth switch, andauth bestinto a focused auth-command moduleWhat Changed
lib/codex-manager/help.tsfor CLI usage text plus login/best arg parsinglib/codex-manager/auth-commands.tsforrunAuthLogin,runSwitch,runBest, and shared selected-account persistence/synclib/codex-manager.tsdown to orchestration and shared helperstest/codex-manager-auth-commands.test.tsto coverrunBest --help, hermetic live-best lease mocking,EBUSYsave failures, and backup-restore error handlingtest/documentation.test.tsso CLI usage assertions read from the extracted modules instead of the monolithValidation
npm test -- test/codex-manager-auth-commands.test.tsnpm run typechecknpm run lintnpm run buildNotes
npm run audit:cistill fails on inherited dependency advisories inhonoandflatted. No dependency files changed in this slice.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this PR completes a focused extraction:
runAuthLogin,runSwitch,runBest, andpersistAndSyncSelectedAccountmove out of the monolith intolib/codex-manager/auth-commands.ts, while help text and arg parsers land inlib/codex-manager/help.ts. the previous concern about droppedactiveIndexByFamilybounds clamping on backup restore is fully addressed by the newclampPreservedActiveIndexByFamilyhelper, with a passing hermetic test confirming clamping to[0, maxIndex]for allMODEL_FAMILIESentries.key points:
clampPreservedActiveIndexByFamilyiteratesMODEL_FAMILIES, clamps out-of-range indices, and always writescodex = targetIndex; the restore-flow test at line 723 validates end-to-endwithSerializedBestLiveRuncorrectly serializes concurrent--livewrites per storage path via a mutable tail-promise queue; the concurrent test (line 627) verifiesmaxActiveSaves === 1; windows EBUSY/EPERM during the live-best save propagates correctly and blockssetCodexCliActiveSelection(tested at line 693)saveAccountsis always called beforesetCodexCliActiveSelection; if the save throws (EBUSY, EPERM), the codex CLI state is never updated with a partially-written token — this is explicitly testedisBrowserLaunchSuppressed = true(headless / windows no-browser path) is mockedfalsein all tests; the branch that skipspromptOAuthSignInModeand forces"manual"mode goes unexercised; see inline commentConfidence Score: 4/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant CM as codex-manager.ts participant AC as auth-commands.ts participant H as help.ts participant S as storage.ts participant CW as codex-cli/writer.ts CM->>H: printUsage() CM->>AC: runAuthLoginCommand(args, deps) AC->>H: parseAuthLoginArgs(args) H-->>AC: ParsedAuthLoginArgs AC->>S: loadAccounts() AC->>S: getNamedBackups() AC->>S: restoreAccountsFromBackup(path, {persist:false}) AC->>AC: persistAndSyncSelectedAccount(storage, targetIndex) AC->>AC: clampPreservedActiveIndexByFamily(storage, targetIndex) AC->>S: saveAccounts(storage) AC->>CW: setCodexCliActiveSelection(...) CM->>AC: runBestCommand(args, helpers) AC->>H: parseBestArgs(args) H-->>AC: ParsedBestArgs AC->>AC: withSerializedBestLiveRun(storagePath, execute) note over AC: lease acquired, queue serialized AC->>S: loadAccounts() AC->>S: saveAccounts(storage) AC->>AC: persistAndSyncSelectedAccount(...) AC->>CW: setCodexCliActiveSelection(...) CM->>AC: runSwitchCommand(args, helpers) AC->>S: loadAccounts() AC->>AC: persistAndSyncSelectedAccount(...) AC->>S: saveAccounts(storage) AC->>CW: setCodexCliActiveSelection(...)Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "test: cover live lease failures" | Re-trigger Greptile