fix(auth): preserve visible dashboard row numbers#116
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. |
|
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 selectedAccountNumber to login menu results and makes manage-mode actions validate targets against freshly reloaded storage before performing switch/delete/toggle/refresh. new helpers and options were added in changes
sequence diagram(s)sequenceDiagram
participant user as user
participant login as login menu\nlib/cli.ts
participant manager as codex manager\nlib/codex-manager.ts
participant storage as storage/fs\nlib/storage.ts
user->>login: pick manage action for visible row
login->>login: resolveAccountDisplayNumber(account)
login-->>user: return { mode, selectedAccountNumber, ... }
user->>manager: handleManageAction(result)
manager->>manager: resolveManageActionAccountNumber(result)
manager->>storage: load latest accounts snapshot
storage-->>manager: return current accounts list
manager->>manager: loadManageActionStorage(validate display->index)
alt target valid
manager->>manager: runSwitch / delete / toggle / refresh (with preloadedStorage)
manager-->>user: success message (includes displayAccountNumber)
else target invalid/stale
manager->>manager: reportUnavailableManageActionAccount(displayAccountNumber, targetIndex, total)
manager-->>user: error/cancel message
end
estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes suggested labels
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 docstrings
🧪 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}
There was a problem hiding this comment.
Pull request overview
Preserves user-visible (dashboard) row numbering for interactive manage actions while keeping the underlying stored/source index semantics used by codex auth switch <index> intact.
Changes:
- Adds
selectedAccountNumberto manage-mode menu results to preserve visible row numbering. - Threads that visible number through manage actions (switch/delete/toggle/refresh) so logs/errors reference what the user saw.
- Adds/updates CLI and manager-CLI tests covering reordered/sorted selection behavior and stale-selection errors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/codex-manager-cli.test.ts | Adds coverage ensuring manage-mode actions/logs use visible row numbers (including stale-selection error cases). |
| test/cli-auth-menu.test.ts | Updates expectations and adds a test to ensure visible row numbers are preserved separately from source indexes. |
| lib/codex-manager.ts | Accepts a “display account number” for runSwitch and uses it for manage-mode messaging/logging. |
| lib/cli.ts | Extends LoginMenuResult and builds manage results that carry the visible row number. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| refreshAccountIndex?: number; | ||
| toggleAccountIndex?: number; | ||
| switchAccountIndex?: number; | ||
| selectedAccountNumber?: number; |
lib/codex-manager.ts
Outdated
| function resolveManageActionAccountNumber( | ||
| menuResult: Awaited<ReturnType<typeof promptLoginMode>>, | ||
| fallbackIndex: number, | ||
| ): number { | ||
| if ( | ||
| typeof menuResult.selectedAccountNumber === "number" && | ||
| Number.isFinite(menuResult.selectedAccountNumber) | ||
| ) { | ||
| return Math.max(1, Math.floor(menuResult.selectedAccountNumber)); | ||
| } | ||
| return fallbackIndex + 1; | ||
| } | ||
|
|
| const displayAccountNumber = | ||
| typeof options.displayAccountNumber === "number" && | ||
| Number.isFinite(options.displayAccountNumber) | ||
| ? Math.max(1, Math.floor(options.displayAccountNumber)) | ||
| : parsed; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
ndycode
changed the base branch from
fresh/16-retention-doctor
to
fresh/19-first-run-wizard
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 4199-4256: Redundant final null-check: inside
loadManageActionStorage, remove the unreachable defensive block that checks if
(!freshStorage.accounts[targetIndex]) and calls
reportUnavailableManageActionAccount, because findMatchingAccountIndex already
guarantees the account exists when it returns targetIndex; simply delete that
if-block so the function returns freshStorage directly after the matchingIndex
check (refer to loadManageActionStorage and findMatchingAccountIndex to locate
the code).
In `@test/codex-manager-cli.test.ts`:
- Around line 2604-2836: Add two deterministic vitest regression tests in the
same test file that reproduce the pre-loop first-run wizard skip and the Windows
opencode probe race; locate the CLI entry used in existing tests via
runCodexMultiAuthCli and the mocked helpers promptLoginModeMock,
loadAccountsMock, and saveAccountsMock. For the first-run wizard skip, mock the
initial load path to simulate the wizard being skipped before the selection loop
(e.g., make loadAccountsMock return an empty/new storage on first call and a
populated one on subsequent calls, then assert the CLI does not hang and returns
expected exit code/logs). For the Windows opencode probe race, simulate the
filesystem probe race by having the opencode/open-file check (the module or
helper invoked by runCodexMultiAuthCli) flip state between calls (mock it to
return “not found” then “found” or throw then succeed) while exercising
manage/switch flows, then assert deterministic error handling and no flakiness
(check console.error/log expectations and that saveAccountsMock behavior is
correct). Ensure both tests use vitest mocks/spies and explicit sequence control
(mockImplementationOnce / flags) so they are deterministic and fail fast on
regressions.
🪄 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: 21aef969-a36f-4f8d-a14a-7368fee03bf2
📒 Files selected for processing (4)
lib/cli.tslib/codex-manager.tstest/cli-auth-menu.test.tstest/codex-manager-cli.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/cli-auth-menu.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/cli.tslib/codex-manager.ts
🔇 Additional comments (13)
lib/codex-manager.ts (6)
4167-4178: lgtm.the helper correctly derives a 1-based display number from menu result or falls back to index+1. floor + max(1, ...) guards negative/fractional inputs.
4180-4183: lgtm.simple options bag for passing preloaded storage and display number through the switch flow.
4185-4197: lgtm.error messaging is clear and avoids leaking tokens or emails—only numeric indices shown.
4258-4369: manage actions now reload storage before executing—good fix for stale-state bugs.the pattern of calling
loadManageActionStoragethen operating onfreshStorageis consistently applied across switch, delete, toggle, and refresh flows. user-facing messages usedisplayAccountNumbercorrectly without leaking tokens or emails.
4937-5046: runSwitch now accepts preloadedStorage and displayAccountNumber—clean api extension.
lib/codex-manager.ts:5043shows email viaformatAccountLabelin the switch confirmation. this is user-facing cli output (not telemetry), so it's acceptable. no token leakage detected.one note: the cli path
codex auth switch <index>at line 5774 still callsrunSwitch(rest)without options, preserving the original semantics where the cli arg is both storage index and display number. this matches the pr objective of keeping direct cli semantics unchanged.
4199-4256: test coverage confirmed for stale-storage validation.tests at
test/codex-manager-cli.test.ts:2604,test/codex-manager-cli.test.ts:2707, andtest/codex-manager-cli.test.ts:2761exercise all three scenarios via the manage action flow:
- successful reload path (line 2604: storage consistent, account identity preserved, operation succeeds)
- index out of bounds (line 2707: shrinking storage, error reports correct bounds)
- account identity mismatch (line 2761: same index holds different account, error reports unavailability)
additional stale scenarios tested for toggle (
test/codex-manager-cli.test.ts:7721,test/codex-manager-cli.test.ts:7777) and refresh (test/codex-manager-cli.test.ts:7930) operations via the same validation pattern.lib/cli.ts (5)
103-111: lgtm.
selectedAccountNumbercleanly extends the result interface to carry visible row numbers through manage flows.
150-165: lgtm.
resolveAccountDisplayNumbercorrectly prefersquickSwitchNumberwhen available, falling back toindex + 1. defensive guards against invalid values are in place.
175-185: lgtm.
buildManageResultcentralizes manage-mode result construction, ensuringselectedAccountNumberis always populated. good dryness improvement.
344-427: lgtm.all manage action cases now consistently use
buildManageResult, ensuringselectedAccountNumberflows through every path. the pattern is uniformly applied across delete, switch, refresh, and toggle actions.
175-185: test coverage for buildManageResult is adequate.test/cli-auth-menu.test.ts:70-86 directly validates both scenarios: unsorted menu where quickSwitchNumber = index + 1, and sorted menu where sourceIndex differs from index. coverage extends across all action types (switch, refresh, toggle, delete) with consistent selectedAccountNumber assertions.
test/codex-manager-cli.test.ts (1)
7552-7986: selectedAccountNumber propagation in manage delete/toggle/refresh looks solid.these additions verify user-visible numbering is preserved across success and stale-storage failure paths for delete, toggle, and refresh. references:
test/codex-manager-cli.test.ts:7560,test/codex-manager-cli.test.ts:7666,test/codex-manager-cli.test.ts:7854.test/cli-auth-menu.test.ts (1)
47-323: manage action result-shape updates are consistent and well-covered.the new expectations for
selectedAccountNumber, plus the visible-row and invalid-display normalization cases, are clear and enforce the intended cli contract. references:test/cli-auth-menu.test.ts:47,test/cli-auth-menu.test.ts:70,test/cli-auth-menu.test.ts:88.
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.ts`:
- Around line 4332-4340: The null-check for account is redundant because
loadManageActionStorage ensures the account at idx exists when freshStorage is
non-null; remove the unreachable block that fetches const account =
freshStorage.accounts[idx] and the subsequent if (!account) {
reportUnavailableManageActionAccount(...) ; return; } so the function proceeds
directly using freshStorage.accounts[idx] (or assign account without guarding)
and eliminate the call to reportUnavailableManageActionAccount here; keep
references to freshStorage, loadManageActionStorage, idx, displayAccountNumber
and reportUnavailableManageActionAccount in mind when removing to avoid leaving
dangling references.
- Around line 4167-4178: Add unit tests for the new manage-action helpers: write
focused tests for resolveManageActionAccountNumber,
reportUnavailableManageActionAccount, and loadManageActionStorage that
explicitly exercise stale-storage validation (simulate storage reload returning
different accounts and ensure findMatchingAccountIndex path is exercised); mock
prompt responses that include selectedAccountNumber to verify
resolveManageActionAccountNumber returns floor/1-floors and fallback behavior,
assert reportUnavailableManageActionAccount logs/returns the expected message
for missing accounts, and for loadManageActionStorage ensure the fresh reload +
validation branch rejects or reports unavailable accounts when
findMatchingAccountIndex fails. Ensure each helper has direct unit coverage
rather than relying only on integration tests.
- Around line 4205-4262: Add an inline comment inside loadManageActionStorage
documenting the TOCTOU window: explain that although loadManageActionStorage
re-validates the target against a fresh reload, the AccountStorageV3 returned
may become stale by the time the caller calls saveAccounts; note that
saveAccounts uses withStorageLock which only serializes within the same process
and does not prevent races from other concurrent CLI instances or Windows EBUSY
contention, and that existing tests (e.g., codex-manager-cli tests) exercise
stale-detection but do not simulate cross-process contention—this comment should
reference the functions loadManageActionStorage, saveAccounts and
withStorageLock and recommend considering an advisory-lock across processes for
future work.
🪄 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: 29157c1a-9469-4795-9cf2-3ffc52690898
📒 Files selected for processing (2)
lib/codex-manager.tstest/codex-manager-cli.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/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/codex-manager.ts
🔇 Additional comments (9)
lib/codex-manager.ts (4)
4167-4178: lgtm, straightforward helper.simple extraction with reasonable defensive clamping. would be good to confirm tests cover the
NaN/undefinedfallback path.
4185-4203: error messaging looks safe, guard at line 4190 prevents degenerate "1-0" output.the early return for
totalAccounts <= 0ensures the range message at line 4198 always has a valid upper bound. no token/email leakage in console output.
4252-4259: redundant final null-check is unreachable.
findMatchingAccountIndexreturningtargetIndexat line 4238 guarantees the account exists at that index. this block cannot execute unlessfindMatchingAccountIndexhas a bug. past review already flagged this.
4943-5052: runSwitch correctly propagates displayAccountNumber through error/success paths.the preloaded storage path at line 4965 skips a redundant load when caller already validated. messaging is consistent with user-visible row numbers.
one consideration: if
preloadedStorageis supplied but another process modified the file betweenloadManageActionStorageandsaveAccountsat line 5032, changes could be lost. this is the same toctou gap noted above. the pr summary acknowledges this for future advisory-lock work.test/codex-manager-cli.test.ts (5)
2604-2890: strong visible-row regression coverage for switch flows.good depth here. these cases validate reordered targets, sparse-hole stale slots, shrunk storage, empty storage, and replaced-slot drift while keeping user messaging pinned to the visible row. references:
test/codex-manager-cli.test.ts:2604-2890.
7606-7629: delete flow now correctly asserts visible-row messaging.this locks the expected user-facing confirmation to
selectedAccountNumberwhile still deleting by storage index. references:test/codex-manager-cli.test.ts:7606-7629.
7638-7718: toggle and re-enable paths preserve visible-row semantics.both disable and re-enable assertions are clear and deterministic, and they verify visible-row text independently from storage position. references:
test/codex-manager-cli.test.ts:7638-7718.
7720-7906: stale delete/toggle regressions are well targeted and deterministic.good coverage for shrink/disappeared-slot/replaced-slot conditions with explicit no-write assertions. this is the right shape for concurrency-window protection. references:
test/codex-manager-cli.test.ts:7720-7906.
7908-8040: refresh manage-action regressions are solid.the reordered refresh case and stale-slot rejection both assert the intended behavior and prevent accidental oauth invocation on stale selections. references:
test/codex-manager-cli.test.ts:7908-8040.
| async function loadManageActionStorage( | ||
| storage: AccountStorageV3, | ||
| displayAccountNumber: number, | ||
| targetIndex: number, | ||
| ): Promise<AccountStorageV3 | null> { | ||
| if (targetIndex < 0) { | ||
| reportUnavailableManageActionAccount(displayAccountNumber, targetIndex, 0); | ||
| return null; | ||
| } | ||
| const selectedAccount = storage.accounts[targetIndex]; | ||
| if (!selectedAccount) { | ||
| reportUnavailableManageActionAccount( | ||
| displayAccountNumber, | ||
| targetIndex, | ||
| storage.accounts.length, | ||
| ); | ||
| return null; | ||
| } | ||
| const freshStorage = await loadAccounts(); | ||
| if (!freshStorage || freshStorage.accounts.length === 0) { | ||
| reportUnavailableManageActionAccount(displayAccountNumber, targetIndex, 0); | ||
| return null; | ||
| } | ||
|
|
||
| if (targetIndex >= freshStorage.accounts.length) { | ||
| reportUnavailableManageActionAccount( | ||
| displayAccountNumber, | ||
| targetIndex, | ||
| freshStorage.accounts.length, | ||
| ); | ||
| return null; | ||
| } | ||
|
|
||
| const matchingIndex = findMatchingAccountIndex( | ||
| freshStorage.accounts, | ||
| selectedAccount, | ||
| { allowUniqueAccountIdFallbackWithoutEmail: true }, | ||
| ); | ||
| if (matchingIndex !== targetIndex) { | ||
| reportUnavailableManageActionAccount( | ||
| displayAccountNumber, | ||
| targetIndex, | ||
| freshStorage.accounts.length, | ||
| ); | ||
| return null; | ||
| } | ||
|
|
||
| if (!freshStorage.accounts[targetIndex]) { | ||
| reportUnavailableManageActionAccount( | ||
| displayAccountNumber, | ||
| targetIndex, | ||
| freshStorage.accounts.length, | ||
| ); | ||
| return null; | ||
| } | ||
|
|
||
| return freshStorage; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# verify tests cover stale-storage / reordered-switch scenarios for loadManageActionStorage
rg -n --type=ts 'loadManageActionStorage|stale.*storage|reordered.*switch' --glob '*test*'Repository: ndycode/codex-multi-auth
Length of output: 836
🏁 Script executed:
# Check if inline comments exist in the function
sed -n '4205,4262p' lib/codex-manager.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1889
🏁 Script executed:
# Look at the specific test implementations mentioned
sed -n '2707,2750p' test/codex-manager-cli.test.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1846
🏁 Script executed:
# Check for Windows EBUSY/429 handling in queue or retry logic
rg -n --type=ts 'EBUSY|429|windows.*retry|retry.*windows' lib/Repository: ndycode/codex-multi-auth
Length of output: 3188
🏁 Script executed:
# Find where loadManageActionStorage is called and what follows (saveAccounts pattern)
rg -n --type=ts 'loadManageActionStorage' lib/ | head -20Repository: ndycode/codex-multi-auth
Length of output: 453
🏁 Script executed:
# Check if saveAccounts is called immediately after loadManageActionStorage returns
rg -B2 -A5 'loadManageActionStorage' lib/codex-manager.ts | head -60Repository: ndycode/codex-multi-auth
Length of output: 829
🏁 Script executed:
# Verify if storage write operations (saveAccounts) handle EBUSY retries
rg -n --type=ts 'saveAccounts.*await|await.*saveAccounts' lib/ | head -10Repository: ndycode/codex-multi-auth
Length of output: 761
🏁 Script executed:
# Look at the actual caller implementation to see saveAccounts pattern
sed -n '4274,4330p' lib/codex-manager.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1827
🏁 Script executed:
# Check if saveAccounts in codex-manager.ts uses retry queue or direct I/O
sed -n '1720,1735p' lib/codex-manager.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 637
🏁 Script executed:
# Verify saveAccounts function signature and retry handling
rg -n --type=ts 'export.*function saveAccounts|export.*const saveAccounts' lib/Repository: ndycode/codex-multi-auth
Length of output: 123
🏁 Script executed:
# Check saveAccounts implementation and its retry handling
sed -n '3450,3500p' lib/storage.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1953
🏁 Script executed:
# Look at the callers of loadManageActionStorage to understand the full race window
# Specifically check if there's any time between load and save where data can change
sed -n '4274,4320p' lib/codex-manager.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1591
🏁 Script executed:
# Check if there are Windows-specific tests for EBUSY in concurrent scenarios
rg -n --type=ts 'windows|concurrent.*EBUSY|EBUSY.*concurrent' test/ | head -20Repository: ndycode/codex-multi-auth
Length of output: 2055
🏁 Script executed:
# Check one of the stale storage tests to see if it covers Windows EBUSY retry scenarios
sed -n '2707,2850p' test/codex-manager-cli.test.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 5517
🏁 Script executed:
# Look for withStorageLock implementation to understand the locking mechanism
rg -n --type=ts 'withStorageLock|export.*withStorageLock' lib/ | head -5Repository: ndycode/codex-multi-auth
Length of output: 382
🏁 Script executed:
# Check the full withStorageLock implementation
sed -n '484,530p' lib/storage.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1699
🏁 Script executed:
# Check if there are concurrent/parallel load+save tests for Windows EBUSY
rg -n --type=ts 'test.*concurrent|test.*parallel' test/codex-manager-cli.test.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Check if there are any concurrent test scenarios mocking EBUSY + stale storage
rg -n --type=ts 'EBUSY.*stale|stale.*EBUSY|concurrent.*loadManageActionStorage' test/Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Verify if the race is documented elsewhere (in comments, issues, or PR notes)
rg -n --type=ts 'toctou|race.*condition|advisory.*lock' lib/codex-manager.ts | head -10Repository: ndycode/codex-multi-auth
Length of output: 50
add inline comment documenting the toctou window for future maintainers.
loadManageActionStorage validates target identity against a fresh reload, but the returned storage can be stale by the time the caller performs saveAccounts. on windows this is especially risky under concurrent cli invocations or EBUSY retries. the withStorageLock in saveAccounts (lib/storage.ts:3450) protects only within a single process; it does not prevent races from other cli instances.
tests at test/codex-manager-cli.test.ts:2707, 2815, 7775, 7831, 7984 cover stale data detection but do not exercise concurrent cross-process contention under Windows EBUSY retry scenarios.
pr summary notes this limitation for future advisory-lock work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4205 - 4262, Add an inline comment inside
loadManageActionStorage documenting the TOCTOU window: explain that although
loadManageActionStorage re-validates the target against a fresh reload, the
AccountStorageV3 returned may become stale by the time the caller calls
saveAccounts; note that saveAccounts uses withStorageLock which only serializes
within the same process and does not prevent races from other concurrent CLI
instances or Windows EBUSY contention, and that existing tests (e.g.,
codex-manager-cli tests) exercise stale-detection but do not simulate
cross-process contention—this comment should reference the functions
loadManageActionStorage, saveAccounts and withStorageLock and recommend
considering an advisory-lock across processes for future work.
There was a problem hiding this comment.
Actionable comments posted: 2
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)
4417-4442:⚠️ Potential issue | 🟡 Minorrefresh flow logs misleading account number when oauth returns mismatched identity.
when user refreshes account N, the oauth flow could return tokens for a different account identity (e.g., wrong chatgpt account).
persistAccountPoolatlib/codex-manager.ts:1439matches tokens by email/refresh-token/accountId and updates whichever account matches—potentially a different one than the selected index. the log atlib/codex-manager.ts:1441still reports"Refreshed account ${displayAccountNumber}."based on the user's selection, not which account was actually persisted.pr summary flags missing vitest coverage for this path. add a test case in
test/codex-manager-cli.test.tsthat mocks oauth returning a different account's tokens and verify either:
- the account-mismatch is detected and rejected before persist
- a warning is logged indicating identity changed
- the correct account number is reported in the log message
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4417 - 4442, The refresh flow currently logs the user-selected displayAccountNumber but persistAccountPool(resolveAccountSelection(...)) may update a different account when runOAuthFlow returns a mismatched identity; update the code in the refresh block (the code that calls runOAuthFlow, resolveAccountSelection, persistAccountPool, and syncSelectionToCodex) to detect when the resolved/persisted account identity differs from displayAccountNumber and either reject the refresh, emit a clear warning showing both the selected and actual persisted account numbers, or update the log to report the actual persisted account number (use the resolved value returned by resolveAccountSelection/persistAccountPool to determine the actual account index/identifier before logging), and add a Vitest test in test/codex-manager-cli.test.ts that mocks runOAuthFlow to return tokens for a different identity and asserts the chosen behavior (rejection, warning, or correct account-number log) by inspecting console output and persistAccountPool interactions.
🤖 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 5078-5082: The error text for the out-of-range check in
codex-manager.ts currently says "is no longer available" which implies prior
existence; update the message in the block handling targetIndex < 0 ||
targetIndex >= storage.accounts.length to a neutral phrasing when the user
supplied the index directly (use the parsed variable or detect
displayAccountNumber === parsed) — e.g. "Selected account {parsed} is not
available (storage position {parsed} is outside 1-{storage.accounts.length})" or
"No account exists at position {parsed} (valid range
1-{storage.accounts.length})". Modify the string that uses displayAccountNumber
so it picks parsed for direct-CLI cases (or uses a generic "No account exists"
wording) and keep references to targetIndex, displayAccountNumber, parsed, and
storage.accounts.length to locate the change.
In `@test/codex-manager-cli.test.ts`:
- Around line 8145-8201: Add two deterministic regression tests to the same
suite around runCodexMultiAuthCli: one that simulates an identity-swap
stale-refresh (storage slot index still present but the account object at
refreshAccountIndex has a different accountId/refreshToken than before) by
making loadAccountsMock return an alteredStorage where
accounts[refreshAccountIndex] has changed and asserting createAuthorizationFlow
is not called, saveAccountsMock not called, and console.error logs the "Selected
account ... identity changed" message; and another that simulates a reload/save
failing with Windows-style concurrency errors by having loadAccountsMock or the
save/save-validate sequence throw an error object with code 'EBUSY' (and a
separate case 'EPERM'), then assert the CLI handles the error deterministically
(returns expected exit code and logs the EBUSY/EPERM-aware message) — reuse
promptLoginModeMock to trigger the refresh path and reference
runCodexMultiAuthCli, loadAccountsMock, saveAccountsMock, promptLoginModeMock,
and createAuthorizationFlow to locate where to inject these behaviors.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 4417-4442: The refresh flow currently logs the user-selected
displayAccountNumber but persistAccountPool(resolveAccountSelection(...)) may
update a different account when runOAuthFlow returns a mismatched identity;
update the code in the refresh block (the code that calls runOAuthFlow,
resolveAccountSelection, persistAccountPool, and syncSelectionToCodex) to detect
when the resolved/persisted account identity differs from displayAccountNumber
and either reject the refresh, emit a clear warning showing both the selected
and actual persisted account numbers, or update the log to report the actual
persisted account number (use the resolved value returned by
resolveAccountSelection/persistAccountPool to determine the actual account
index/identifier before logging), and add a Vitest test in
test/codex-manager-cli.test.ts that mocks runOAuthFlow to return tokens for a
different identity and asserts the chosen behavior (rejection, warning, or
correct account-number log) by inspecting console output and persistAccountPool
interactions.
🪄 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: 87ad2ddb-0462-46ac-bc2c-cf7fe7bb7415
📒 Files selected for processing (2)
lib/codex-manager.tstest/codex-manager-cli.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/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/codex-manager.ts
🔇 Additional comments (10)
lib/codex-manager.ts (7)
4235-4246: helper function lacks direct unit test coverage.per past review,
resolveManageActionAccountNumberis exercised only through integration tests intest/codex-manager-cli.test.tsandtest/cli-auth-menu.test.ts. coding guidelines forlib/**require citing affected tests for new functions. consider adding focused unit tests that verify theMath.floorandMath.max(1,...)boundary behavior, especially for edge cases likeselectedAccountNumber: 0.5or negative values.
4248-4251: interface definition looks good.
RunSwitchOptionscleanly captures the preloaded storage pattern for the validate-then-act flow. the optional fields allow backward-compatible usage in direct CLIcodex auth switch <n>calls.
4253-4271: error reporting helper also lacks direct unit tests.same concern as past review:
reportUnavailableManageActionAccountis called byloadManageActionStoragebut has no focused unit tests verifying the three message branches (no accounts, out-of-range, generic unavailable). per coding guidelines forlib/**, new functions should cite affected tests.
4273-4330: missing toctou documentation as flagged in past review.
loadManageActionStoragere-validates the target against a fresh reload, but the returnedAccountStorageV3can become stale by the time the caller callssaveAccounts. past review requested an inline comment documenting this window for future maintainers. the comment should note thatwithStorageLockinlib/storage.ts:3450only serializes within the same process and does not prevent races from concurrent cli invocations or windows EBUSY retry scenarios.suggested inline comment
async function loadManageActionStorage( storage: AccountStorageV3, displayAccountNumber: number, targetIndex: number, ): Promise<AccountStorageV3 | null> { + // NOTE: TOCTOU window - the returned storage may become stale between this + // validation and the caller's saveAccounts(). withStorageLock in storage.ts + // only serializes within a single process; concurrent CLI invocations or + // Windows EBUSY retries can still cause races. See tests at + // test/codex-manager-cli.test.ts:2707,2815,7775,7831,7984 for stale-detection + // coverage. Cross-process advisory locking is a future improvement. if (targetIndex < 0) {
4400-4408: redundant null-check is unreachable as flagged in past review.
loadManageActionStorageatlib/codex-manager.ts:4320already validates thatfreshStorage.accounts[targetIndex]exists before returning non-null. since line 4397 returns early on!freshStorage, the guard at lines 4401-4407 can never execute.suggested removal
const account = freshStorage.accounts[idx]; - if (!account) { - reportUnavailableManageActionAccount( - displayAccountNumber, - idx, - freshStorage.accounts.length, - ); - return; - } account.enabled = account.enabled === false;
4332-4354: validate-then-act flow is well-structured for manage actions.the refactored
handleManageActionproperly resolvesdisplayAccountNumberviaresolveManageActionAccountNumberand validates against fresh storage vialoadManageActionStoragebefore delegating torunSwitch. this preserves visible row semantics while mitigating stale-storage issues.one note: all four action branches (switch, delete, toggle, refresh) follow the same pattern, which is good for consistency. the
displayAccountNumberflows through to final log messages correctly.
5156-5158: email is logged in formatAccountLabel, but only to tty in success message.lib/codex-manager.ts:5157 calls
formatAccountLabel(account, displayAccountNumber - 1)which returns full email when available (lib/accounts.ts:836). this is intentional for cli user feedback—tests confirm the expected output includes email (test/codex-manager-cli.test.ts:2763:"Switched to account 1: 1. account-29@example.com"). no structured logging or error tracking captures console output, so no inadvertent pii leakage to external systems.test/codex-manager-cli.test.ts (3)
2724-3010: good visible-row switch coverage for reordered and stale storage paths.the scenarios in
test/codex-manager-cli.test.ts:2724-3010are focused and deterministic, and they correctly lock messaging to visible row numbers while validating storage-index behavior.
7767-7879: delete/toggle manage flows now align visible-row numbering with user output.
test/codex-manager-cli.test.ts:7767-7879correctly verifies that delete/disable/enable messaging uses the selected visible row rather than raw storage slot.
7881-8067: stale delete/toggle guards are solid and deterministic.the stale-list and replaced-slot cases in
test/codex-manager-cli.test.ts:7881-8067are good regressions and correctly assert no write on invalidated selections.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/codex-manager-cli.test.ts (1)
8405-8483:⚠️ Potential issue | 🟠 Majorthe
epermwindows retry regression is still host-platform gated.in
test/codex-manager-cli.test.ts:8405-8483, the expected retry depth forEPERMis conditional onprocess.platform. on non-windows runners this does not validate the windowsEPERMretry behavior, so a windows-only regression can slip through linux/mac ci.please make this deterministic by isolating platform detection behind a mockable seam and asserting the windows path explicitly in ci.
As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 8405 - 8483, The test currently gates the EPERM retry expectation on process.platform, making the Windows retry behavior untested on non-Windows CI; introduce (or use) a mockable platform-seam (e.g., export and use an isWindows() helper that returns process.platform === "win32") and update the test to stub that helper to true when asserting the Windows EPERM retry depth: modify the calculation of remainingReloadFailures and the expect(loadCount) branch in the test to call the helper (or import and mock it) instead of reading process.platform directly, and add an explicit mocked assertion for the EPERM path so CI deterministically verifies the Windows retry behavior (refer to symbols loadAccountsMock, remainingReloadFailures, failSelectedReload, and runCodexMultiAuthCli to locate the test logic to change).
🤖 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 4347-4354: The if-check guarding
freshStorage.accounts[targetIndex] is dead because targetIndex was validated
(targetIndex >= 0, targetIndex < freshStorage.accounts.length) and
findMatchingAccountIndex already confirmed matchingIndex === targetIndex; remove
the entire guard and its body (the reportUnavailableManageActionAccount(...)
call and return null) from the code path in codex-manager.ts so execution
continues to the intended logic that assumes the account exists (references:
freshStorage.accounts, targetIndex, findMatchingAccountIndex,
reportUnavailableManageActionAccount).
- Around line 4296-4298: The trailing throw "Manage action storage reload retry
loop exhausted unexpectedly" is unreachable because the retry loop either
returns on success or throws on non-retryable/final retry; remove that
unreachable throw statement or replace it with a defensive assertion (e.g., an
invariant/assertion helper) if you want explicit defensive checks. Locate the
retry loop in the manage action storage reload logic (the block that returns on
success and throws on retryable/non-retryable errors) and either delete the
final throw or swap it for a call to your project's assertion utility (so it's
clear and intentional rather than unreachable).
In `@test/codex-manager-cli.test.ts`:
- Around line 2762-3048: Add a new vitest case that simulates the
validate-then-save race: use the existing patterns with runCodexMultiAuthCli,
promptLoginModeMock, and loadAccountsMock to return a fresh reload after
selection (like returnShrunken/returnStale/returnReplaced flags), then have
saveAccountsMock.mockRejectedValueOnce(Object.assign(new Error("save failed"), {
code: "EBUSY" })) to force a failing save; call
runCodexMultiAuthCli(["auth","login"]) and assert exitCode is 0, that
saveAccountsMock was called and rejected, that no success log like "Switched to
account" was emitted (spy console.log), and that no persistent side-effect
(activeIndex/activeIndexByFamily) was applied via saveAccountsMock's saved
payload; reference runCodexMultiAuthCli, promptLoginModeMock, loadAccountsMock,
saveAccountsMock, and console.log/console.error to locate and mirror the other
tests' structure.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 8405-8483: The test currently gates the EPERM retry expectation on
process.platform, making the Windows retry behavior untested on non-Windows CI;
introduce (or use) a mockable platform-seam (e.g., export and use an isWindows()
helper that returns process.platform === "win32") and update the test to stub
that helper to true when asserting the Windows EPERM retry depth: modify the
calculation of remainingReloadFailures and the expect(loadCount) branch in the
test to call the helper (or import and mock it) instead of reading
process.platform directly, and add an explicit mocked assertion for the EPERM
path so CI deterministically verifies the Windows retry behavior (refer to
symbols loadAccountsMock, remainingReloadFailures, failSelectedReload, and
runCodexMultiAuthCli to locate the test logic to change).
🪄 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: 7e9b24e7-20a5-4a92-bdb5-1631e0a08b72
📒 Files selected for processing (2)
lib/codex-manager.tstest/codex-manager-cli.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/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/codex-manager.ts
🔇 Additional comments (17)
lib/codex-manager.ts (14)
4235-4246: lgtm —resolveManageActionAccountNumbercorrectly normalizesselectedAccountNumberwith NaN-safe handling viaNumber.isFiniteand floors to integer.
4248-4251: lgtm —RunSwitchOptionsinterface is straightforward.
4253-4271: lgtm — error reporting function logs only numeric identifiers, no sensitive data leakage.
4273-4280: lgtm — retry gate matchesisRetryableFilesystemErrorCodeinlib/storage.ts:2039-2046for consistent EBUSY/EAGAIN/EPERM handling.
4300-4318: add inline comment documenting the toctou window for maintainers.
loadManageActionStoragereloads storage and validates target identity, but the returnedAccountStorageV3can become stale by the time the caller invokessaveAccounts. thewithStorageLockinlib/storage.ts:3450serializes only within a single process — concurrent cli instances (especially under windows EBUSY retries) can still race. existing tests attest/codex-manager-cli.test.ts:2707,2815,7775,7831,7984cover stale-data detection but not cross-process contention.this was flagged in a previous review and remains unaddressed.
4363-4381: lgtm — switch path correctly resolvesdisplayAccountNumber, reloads vialoadManageActionStorage, and threads both intorunSwitch.
4384-4410: lgtm — delete path follows the reload-validate-act pattern and logs usingdisplayAccountNumberfor consistent user-facing messaging.
4427-4435: redundant null-check —loadManageActionStoragealready validated account existence.
loadManageActionStoragereturnsnullif the account atidxdoesn't exist (guards at lines 4324-4330 and 4333-4344). since line 4424 returns on!freshStorage, the account is guaranteed present here. this was flagged in a previous review and marked as addressed, but the code remains.suggested removal
if (!freshStorage) { return; } - const account = freshStorage.accounts[idx]; - if (!account) { - reportUnavailableManageActionAccount( - displayAccountNumber, - idx, - freshStorage.accounts.length, - ); - return; - } - account.enabled = account.enabled === false; + const account = freshStorage.accounts[idx]!; + account.enabled = !account.enabled; await saveAccounts(freshStorage);
4444-4478: lgtm — refresh path wraps storage reload in try-catch and provides a clear error message on failure before proceeding with oauth.
5087-5108: lgtm —runSwitchsignature update correctly handles optionaldisplayAccountNumberwith fallback toparsedand usespreloadedStoragewhen provided.
5125-5129: lgtm — account-not-found error correctly usesdisplayAccountNumberfor consistent user messaging.
5171-5175: lgtm — refresh failure warning usesdisplayAccountNumberconsistently.
5190-5198: lgtm — success and sync-failure messages consistently usedisplayAccountNumberfor user-facing output.
5114-5122: test coverage for direct-cli out-of-range exists attest/codex-manager-cli.test.ts:2629and verifies the expected error message "Index out of range. Valid range: 1-2." the error differentiation is correct:lib/codex-manager.ts:5114-5122uses "Index out of range" for direct cli (no preloadedStorage) and "Selected account X is no longer available" for menu path. no further action needed.test/codex-manager-cli.test.ts (3)
2594-2630: good direct-cli out-of-range regression coverage.
test/codex-manager-cli.test.ts:2594-2630cleanly locks the directauth switch <n>range error wording and verifies no storage/codex sync side effects.
7805-8318: visible-row manage regressions for delete/toggle/refresh look strong.
test/codex-manager-cli.test.ts:7805-8318correctly threadsselectedAccountNumberthrough delete/toggle/refresh paths and validates stale-slot and identity-swap failures without mutating storage.
8320-8403: good transient reload race regression for refresh.
test/codex-manager-cli.test.ts:8320-8403is a solid deterministic race repro: it injects a transient reload failure, verifies retry continuation, and confirms refresh completion/logging.
ndycode
force-pushed
the
fresh/20-dashboard-visible-row-semantics
branch
from
70a804e to
e518abd
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
lib/quota-cache.ts (1)
245-256:⚠️ Potential issue | 🟠 Majorserialize
saveQuotaCache()andclearQuotaCache().
clearQuotaCache()inlib/quota-cache.ts:277-301can returntrue, then an already-startedsaveQuotaCache()inlib/quota-cache.ts:245-256can finish its delayed rename and recreatequota-cache.json. that resurrects stale quota data right after auth rotation, and theEBUSY/EPERMbackoff makes the interleave wider on windows. current vitest coverage intest/quota-cache.test.ts:108-184andtest/quota-cache.test.ts:231-304only checks retry behavior, not this race. put both mutations behind the same in-process queue or generation guard, and add a deterministic vitest that blocksfs.rename()until afterclearQuotaCache()resolves.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.Also applies to: 277-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/quota-cache.ts` around lines 245 - 256, saveQuotaCache() and clearQuotaCache() race: saveQuotaCache's delayed fs.rename(tempPath, quotaCachePath) can recreate quota-cache.json after clearQuotaCache() returns, resurrecting stale data; serialize their mutations by introducing a single in-process queue or generation guard shared by saveQuotaCache and clearQuotaCache (use the existing function names saveQuotaCache and clearQuotaCache and the quotaCachePath/tempPath symbols) so only one mutation runs at a time, keep the existing EBUSY/EPERM retry/backoff inside the serialized task, update tests in test/quota-cache.test.ts to add a deterministic case that blocks fs.rename until after clearQuotaCache resolves to assert no resurrection, and ensure no logs emitted by these paths leak tokens or emails.test/cli.test.ts (1)
745-797: 🧹 Nitpick | 🔵 Trivialwindows filesystem behavior not covered.
per coding guidelines for
test/**, tests should cover windows filesystem behavior. the TTY and env var tests here manipulatestdin.isTTYandstdout.isTTYbut don't test windows-specific readline quirks or EPERM/EBUSY scenarios that the pr objectives mention as noted risks.consider adding a deterministic test that simulates the windows readline edge case or at minimum document why it's deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 745 - 797, The TTY-focused test for isNonInteractiveMode() doesn't cover Windows-specific readline quirks (EPERM/EBUSY) described in the PR; add a deterministic unit test that mocks/simulates Windows behavior by stubbing the readline/open behavior or process.stdin.readableState to throw EPERM/EBUSY when constructing the CLI module (targeting the same import("../lib/cli.js") and isNonInteractiveMode() calls), asserting the module still resolves non-interactive detection or handles the error as intended; if adding such a mock is infeasible, add a short test case that documents the deferred coverage and references isNonInteractiveMode() and the TTY branches so reviewers know the gap is intentional.lib/storage.ts (1)
3890-3923:⚠️ Potential issue | 🟠 Majorpre-import snapshot is outside the storage lock.
line 3892 in
lib/storage.ts:3892snapshots beforewithAccountStorageTransactionstarts at line 3898 (lib/storage.ts:3898). a concurrent writer can commit between these steps, so rollback context may not represent the real pre-import state. take the snapshot inside the locked transaction path (before persist) and add a vitest that races import against a concurrent save.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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 3890 - 3923, The pre-import snapshot call snapshotAccountStorage is currently outside the storage lock and can be invalidated by a concurrent writer; move the snapshotAccountStorage call into the withAccountStorageTransaction callback just before calling persist (so snapshot reflects the locked pre-persist state), keep the existing dedup/limit checks and persist(newStorage) flow, and update callers accordingly; add a Vitest that races importAccounts (the function containing this logic) against a concurrent save operation to assert the snapshot/rollback state is correct and deterministic, and ensure the test names reference the file (lib/storage.ts) and the import flow; also verify logging around this path does not leak sensitive data and that any new queuing/retry logic used for concurrent writers accounts for EBUSY/429 semantics.docs/troubleshooting.md (1)
62-170:⚠️ Potential issue | 🟠 Majoradd upgrade-note pointers for the new recovery workflows.
line 62 and line 125 in
docs/troubleshooting.mddocument new restore/sync/reset flows tied tolib/codex-manager/settings-hub.ts:2708andlib/destructive-actions.ts:176, but this section does not point to upgrade notes or npm-script updates. please add a short upgrade-note reference so rollout changes are discoverable.As per coding guidelines, "
docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/troubleshooting.md` around lines 62 - 170, The troubleshooting doc mentions new restore/sync/reset flows but lacks upgrade-note pointers and npm-script calls; update the two spots around the "Restore From Backup" and "Reset Local State" passages to add a short upgrade-note reference that directs readers to the release upgrade notes and the new npm scripts (e.g., the deploy/rollout or migration scripts added for the change), and add a brief parenthetical mentioning the code changes in settings-hub.ts (settings-hub module, relevant flow at settings-hub.ts:2708) and destructive-actions.ts (destructive-actions module, reset logic at destructive-actions.ts:176) so operators can trace behavior, ensuring the README/SECURITY/docs consistency requirement is satisfied.test/accounts-edge.test.ts (1)
108-125:⚠️ Potential issue | 🟡 Minorthis test still misses the new failure-handling branch
test/accounts-edge.test.ts:108-125makessaveAccountsreject, but the mocked sync result attest/accounts-edge.test.ts:113-116never includes apendingRun. that means the new calls inlib/accounts.ts:123-125are still unexecuted here, so the regression does not provecommitCodexCliSyncRunFailure()fires on persistence failure. please return a non-null pending run and assert both commit mocks explicitly.As per coding guidelines,
test/**: 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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/accounts-edge.test.ts` around lines 108 - 125, The test currently simulates saveAccounts failing but doesn't include a pendingRun in the mocked sync result, so the new failure-handling branch in AccountManager.loadFromDisk (which calls commitCodexCliSyncRunFailure when persistence fails) is never executed; update the mock for mockSyncAccountStorageFromCodexCli to return a non-null pendingRun (e.g., an object with an id/timestamp) and then assert that the commit mock for the failure path (commitCodexCliSyncRunFailure or its test mock) was called once and the success-commit mock (commitCodexCliSyncRunSuccess) was not called; specifically change the mockResolvedValue for mockSyncAccountStorageFromCodexCli to include pendingRun and add explicit expect(...) assertions for the commit mocks after calling AccountManager.loadFromDisk() so the new branch in lib/accounts.ts:AccountManager.loadFromDisk is exercised.test/cli-auth-menu.test.ts (1)
104-119:⚠️ Potential issue | 🟠 Majorthe account-details branch still is not pinned for divergent display numbers.
test/cli-auth-menu.test.ts:70proves the direct dashboard action keepsquickSwitchNumber, but theselect-account -> showAccountDetailscases attest/cli-auth-menu.test.ts:104,test/cli-auth-menu.test.ts:153, andtest/cli-auth-menu.test.ts:292only run withselectedAccountNumber === index + 1. if that branch falls back to array position, these tests still pass. add at least one refresh, toggle, or delete case with something like{ index: 0, sourceIndex: 28, quickSwitchNumber: 7 }and assert the manage result keepsselectedAccountNumber: 7while targeting storage index28.test/cli-auth-menu.test.ts:70,test/cli-auth-menu.test.ts:104,test/cli-auth-menu.test.ts:153,test/cli-auth-menu.test.ts:292Also applies to: 153-168, 292-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli-auth-menu.test.ts` around lines 104 - 119, Add a test variant for the select-account -> showAccountDetails path that uses an account object with divergent display numbering (e.g., { index: 0, sourceIndex: 28, quickSwitchNumber: 7 }) to ensure the branch uses the account's pinned quickSwitchNumber for selectedAccountNumber while still targeting the storage index for switchAccountIndex; specifically, in the test that calls showAuthMenu.mockResolvedValueOnce({ type: "select-account", account: { index: 0, sourceIndex: 28, quickSwitchNumber: 7 } }) and showAccountDetails.mockResolvedValueOnce("set-current"), assert the promptLoginMode result returns mode: "manage", switchAccountIndex: 28, and selectedAccountNumber: 7 so the code paths in promptLoginMode, showAuthMenu, and showAccountDetails honor quickSwitchNumber rather than falling back to index+1.README.md (1)
101-119:⚠️ Potential issue | 🟡 Minordocument the new split between dashboard row numbers and cli indices.
the new behavior exercised in
test/cli-auth-menu.test.ts:54andtest/cli-auth-menu.test.ts:70makes visible dashboard numbers authoritative for interactive manage actions while directcodex auth switch <n>still uses the saved storage index. this section now shows both workflows side by side, but it never says they can diverge after sorting or sparse rows. add one note here, then mirror it indocs/getting-started.mdanddocs/upgrade.md, so users do not map a visible row number back to the wrong cli index.test/cli-auth-menu.test.ts:54,test/cli-auth-menu.test.ts:70suggested doc note
Day-1 command set: ```bash codex auth switch 2 codex auth report --live --json codex auth fix --dry-run codex auth doctor --fixInteractive dashboard paths:
+- dashboard quick-switch numbers follow the visible row order in the current view. direct
codex auth switch <n>still uses the saved storage index.
- restore named backups:
codex auth login->Restore From Backup- preview Codex CLI sync:
codex auth login->Settings->Codex CLI Sync- adjust stable dashboard preferences:
codex auth login->Settings->Everyday Settings</details> as per coding guidelines, `docs/**`: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 101 - 119, Update the README (and mirror the same
paragraph into docs/getting-started.md and docs/upgrade.md) to explicitly
document the new split between visible dashboard row numbers and stored CLI
indices: state that dashboard quick-switch numbers reflect the current visible
row order in the interactive view while direct commands like "codex auth switch
" use the saved storage index and therefore can diverge after sorting,
filtering, or when rows are sparse; reference the behavior exercised by tests
test/cli-auth-menu.test.ts:54 and test/cli-auth-menu.test.ts:70 as confirmation,
and add a short upgrade note mentioning this behavioral change plus any new npm
scripts introduced so users update their workflows accordingly.</details> </blockquote></details> <details> <summary>test/storage-recovery-paths.test.ts (1)</summary><blockquote> `782-795`: _⚠️ Potential issue_ | _🟠 Major_ **replace the sleep-based mtime ordering with explicit mtimes.** test/storage-recovery-paths.test.ts:782-795 depends on a 20ms `setTimeout()` to make `newerManualPath` win. that flakes on windows and slower filesystems where mtimes are coarser or coalesced, so this regression can pass or fail without any code change. set the mtimes with `fs.utimes()` instead. <details> <summary>suggested change</summary> ```diff await fs.writeFile( olderManualPath, JSON.stringify({ version: 3, activeIndex: 0, accounts: [{ refreshToken: "older-refresh", accountId: "older", addedAt: 1, lastUsed: 1 }], }), "utf-8", ); - await new Promise((resolve) => setTimeout(resolve, 20)); await fs.writeFile( newerManualPath, JSON.stringify({ version: 3, activeIndex: 0, accounts: [{ refreshToken: "newer-refresh", accountId: "newer", addedAt: 2, lastUsed: 2 }], }), "utf-8", ); + const olderMtime = new Date("2026-03-10T00:00:00.000Z"); + const newerMtime = new Date("2026-03-10T00:00:05.000Z"); + await fs.utimes(olderManualPath, olderMtime, olderMtime); + await fs.utimes(newerManualPath, newerMtime, newerMtime);As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage-recovery-paths.test.ts` around lines 782 - 795, The test currently relies on a 20ms sleep to make newerManualPath have a later mtime, which flakes on some filesystems; instead after writing newerManualPath call fs.utimes (or fsPromises.utimes) to set its mtime explicitly to a time later than the other file so getBackupMetadata deterministically picks newerManualPath; update the test in storage-recovery-paths.test.ts to replace the setTimeout-based ordering with an fs.utimes call (referencing newerManualPath and the existing file paths and the getBackupMetadata assertion) to set a later atime/mtime before calling getBackupMetadata.lib/codex-manager.ts (1)
5219-5227:⚠️ Potential issue | 🟡 Minoruse visible row number in switch warning to preserve ui semantics.
lib/codex-manager.ts:5219-5222still logsparsedin the warning path. when switching from sorted dashboard rows, this can leak storage index semantics and contradict the visible-row contract. usedisplayAccountNumberconsistently.proposed fix
- console.warn( - `Switched account ${parsed} locally, but Codex auth sync did not complete. Multi-auth routing will still use this account.`, - ); + console.warn( + `Switched account ${displayAccountNumber} locally, but Codex auth sync did not complete. Multi-auth routing will still use this account.`, + );please add a regression test for this exact message path when
displayAccountNumber !== parsed(sorted/visible-row manage flow). current stale-path coverage intest/codex-manager-cli.test.ts:2707-2750is adjacent but does not prove this warning text behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 5219 - 5227, Replace the use of `parsed` in the warning log with `displayAccountNumber` so the message uses the visible row number (change the `console.warn` line to reference `displayAccountNumber`), and keep the success `console.log` using `displayAccountNumber` and `formatAccountLabel(account, displayAccountNumber - 1)` as-is; then add a regression test that simulates a switch where `displayAccountNumber !== parsed` (the sorted/visible-row flow) and asserts the warning string contains `displayAccountNumber` (not `parsed`) to ensure the visible-row semantics are preserved.
♻️ Duplicate comments (2)
lib/codex-manager.ts (2)
4499-4507:⚠️ Potential issue | 🟡 Minorremove dead guard after
loadManageActionStoragevalidation.
lib/codex-manager.ts:4499-4507re-checksfreshStorage.accounts[idx]even thoughloadManageActionStoragealready rejects invalid/missing targets before returning. this branch is unreachable and adds noise.proposed cleanup
const account = freshStorage.accounts[idx]; - if (!account) { - reportUnavailableManageActionAccount( - displayAccountNumber, - idx, - freshStorage.accounts.length, - ); - return; - } account.enabled = account.enabled === false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4499 - 4507, Remove the redundant null-check for freshStorage.accounts[idx] that follows loadManageActionStorage's validation: since loadManageActionStorage already rejects invalid/missing targets, delete the unreachable branch that calls reportUnavailableManageActionAccount and returns; simply assume account exists and proceed using the retrieved account variable (account / freshStorage.accounts[idx]) in the surrounding logic, ensuring no other references rely on that guard.
4318-4344:⚠️ Potential issue | 🟠 Majoradd direct vitest coverage for retryable manage-action reload codes.
lib/codex-manager.ts:4318-4344introduces windows-specific retry logic (EPERMon win32 +EBUSY/EAGAIN) with backoff. this is a concurrency-sensitive path, but i don’t see direct regression coverage in the provided context for retry success/final throw behavior.#!/bin/bash # verify direct test coverage for manage-action reload retry logic rg -n --type=ts -C3 'isRetryableManageActionReloadCode|loadManageActionStorageSnapshot|EPERM|EBUSY|EAGAIN' test/ rg -n --type=ts -C3 'loadManageActionStorage|handleManageAction|selectedAccountNumber' test/expected result:
- at least one vitest case that stubs throw codes and asserts retry/throw behavior for
loadManageActionStorageSnapshot.- at least one vitest case that exercises windows
EPERMbranch semantics.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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4318 - 4344, Add direct vitest tests for loadManageActionStorageSnapshot and isRetryableManageActionReloadCode: stub/mock loadAccounts to throw NodeJS.ErrnoException with codes "EBUSY" and "EAGAIN" and assert the function retries and eventually returns on a subsequent success; add a test that stubs loadAccounts to always throw non-retryable code and assert loadManageActionStorageSnapshot rethrows; add a Windows-specific test that temporarily set process.platform to "win32" (or mock isRetryableManageActionReloadCode) and verify that "EPERM" is treated as retryable and that the backoff/retry loop behaves (succeeds after a retry or exhausts and throws); ensure tests measure retry attempts (e.g., by counting calls to loadAccounts) rather than relying on timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/storage-paths.md`:
- Line 25: Add regression tests that exercise project-scoped backup path
resolution by calling getProjectGlobalConfigDir(projectPath) and asserting
produced backup paths match the documented project-scoped pattern; update
test/storage.test.ts to include these cases. Replace the in-memory concurrency
guard (inFlightNamedBackupExports) in lib/named-backup-export.ts with a
cross-process-safe write pattern (e.g., atomic write via temp file + fs.rename
or OS file-locking) to prevent concurrent-export corruption. Add
unit/integration tests that simulate Windows-style paths and case-insensitive
comparisons to validate normalizePathForComparison() is applied before backup
path comparisons and actual write operations, ensuring mixed separators and case
variants still map to the same backup file. Ensure tests clean up temp files and
cover both canonical and project-scoped backup flows.
In `@docs/upgrade.md`:
- Around line 65-70: Add a short cross-reference to the direct entrypoint for
recovery by appending one sentence to the "Startup Recovery Prompt" section that
mentions the explicit command users can run to restore backups without going
through the interactive login flow (refer to the command "codex auth
restore-backup"), and link or point readers to the corresponding auth reference
page or restore-backup doc entry for full usage details; ensure the note
clarifies it is an alternative to waiting for the interactive prompt and that it
bypasses login flow.
In `@lib/cli.ts`:
- Around line 150-165: resolveAccountDisplayNumber currently synthesizes a
display number from account.index when quickSwitchNumber is absent; change it to
fail closed by returning undefined (or leaving selectedAccountNumber unset)
instead of deriving index+1: update resolveAccountDisplayNumber(account:
ExistingAccountInfo) to only return a number when account.quickSwitchNumber is a
finite number (Math.floor and Math.max(1,...)), otherwise return undefined;
update any callsites that assign to selectedAccountNumber to handle the
undefined case (do not silently convert from account.index), and ensure
downstream confirmation UI relies on quickSwitchNumber being present rather than
a synthesized value.
- Around line 167-173: The warnUnresolvableAccountSelection function currently
logs PII by printing account.email and account.accountId; change it to only emit
a stable, non-identifying row reference such as `row ${account.index + 1}` or a
generic label like `saved account row ${account.index + 1}` and remove any use
of account.email/account.accountId in the message; update any affected vitest
cases to expect the new generic label and ensure no tests rely on printing
emails/accountIds.
In `@lib/codex-cli/sync.ts`:
- Around line 36-38: Add "EACCES" to the RETRYABLE_SELECTION_TIMESTAMP_CODES set
so transient permission errors are retried (update the constant alongside
RETRYABLE_SELECTION_TIMESTAMP_CODES and ensure
SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS logic still applies), then update the
retry/error handling in the selection-timestamp read logic around the block that
currently checks for "EBUSY" and "EPERM" (the code handling fs.stat() /
persistedLocalTimestamp around the logic starting near the retry loop at the
section referenced by the diff around lines 629-658) to treat "EACCES" the same
as the other retryable codes; finally add a vitest regression that simulates
fs.stat throwing an EACCES error and asserts the code retries and does not set
persistedLocalTimestamp to null and that the active-selection import path still
proceeds.
In `@lib/codex-manager.ts`:
- Around line 4289-4291: The current return of "fallbackIndex + 1" can yield 0
when fallbackIndex is negative; update the calculation to clamp the visible row
number to at least 1 (e.g., use Math.max(fallbackIndex + 1, 1) or equivalent) so
the UI remains 1-based. Locate the function that computes the visible/fallback
display number (the code that returns "fallbackIndex + 1") and replace that
return with a clamped value ensuring the minimum is 1 while preserving the
existing behavior for positive indices.
- Around line 3758-3805: Wrap the calls to listRotatingBackups() and
listAccountSnapshots() in try/catch blocks so runDoctor does not abort on
transient Windows FS errors (EBUSY/EPERM) or rate-like errors (429); on catch,
log a non-sensitive warning and add the corresponding check ("rotating-backups"
or "snapshot-backups") with severity "warn" and a details string that includes
error.code (not full error message or any tokens/emails) and the fallback path
(dirname(storagePath) or getNamedBackupsDirectoryPath()); optionally implement a
small retry/backoff (a few ms delays) before giving up for those specific error
codes; update tests (vitest) to assert that runDoctor returns diagnostics when
listRotatingBackups/listAccountSnapshots throw EBUSY/EPERM/429.
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2947-2953: In the apply/save catch path update the
preview.statusDetail text so it clearly indicates an apply failure instead of
"Failed to refresh sync center": locate the preview object update in the apply
error handler (the block that sets preview = { ...preview, status: "error",
lastSync: getLastCodexCliSyncRun(), statusDetail: ... } in
lib/codex-manager/settings-hub.ts) and replace the generic message with an
apply-specific message such as "Failed to apply/save settings" (including the
error message as currently done via error instanceof Error ? error.message :
String(error)) so operators can triage apply failures correctly.
- Around line 2910-2926: The current flow computes synced via
withQueuedRetry(preview.targetPath) then does a separate withQueuedRetry to
save, creating a TOCTOU gap; refactor so loadAccounts,
applyCodexCliSyncToStorage, saveAccounts and commitPendingCodexCliSyncRun all
run inside a single withQueuedRetry(preview.targetPath) critical section (remove
the intermediate external call and use the synced payload locally), preserve
passing storageBackupEnabled (from getStorageBackupEnabled(config)) into
saveAccounts, and ensure errors like EBUSY/429 are retried by the queue; add a
vitest that interleaves a concurrent write during manual apply to assert no lost
updates and update tests to avoid logging tokens/emails.
In `@lib/destructive-actions.ts`:
- Around line 114-161: The delete flow performs two separate writes
(saveAccounts in rebaseActiveIndicesAfterDelete branch and saveFlaggedAccounts
when flagged changed), allowing interleaving; change this to a single atomic
storage transaction that writes both account storage and flagged storage
together (introduce or use a transaction API so the operations that modify
nextStorage and updatedFlagged are committed in one transactional call and roll
back together on failure), ensure rollback occurs if either save fails (use
previousStorage/flagged for restore), add a vitest regression that simulates
concurrent/interleaving writers during delete to assert atomicity, and make the
transaction resilient to filesystem concurrency errors (retry/backoff for
EBUSY/429) while avoiding logging tokens or emails in error messages; key
symbols to update: rebaseActiveIndicesAfterDelete, clampActiveIndices,
saveAccounts, saveFlaggedAccounts, nextStorage, previousStorage, and the
delete-account snapshot logic.
- Around line 108-113: The deleteAccountAtIndex function accepts unchecked index
values; validate the index before mutating by ensuring options.index is a
Number.isInteger and is >= 0 and < options.storage.accounts.length, and reject
invalid indices (return null or throw consistent with existing API) before
calling options.storage.accounts.at or performing deletion; update
deleteAccountAtIndex to perform this bounds check using
options.storage.accounts.length and only then select the target and proceed with
deletion. Include references to deleteAccountAtIndex, AccountStorageV3, and
DeleteAccountResult in your change.
In `@lib/index.ts`:
- Line 20: The barrel export in lib/index.ts re-exports everything from
"./sync-history.js", exposing test-only mutable helpers (e.g. symbols matching
__*fortests and configure*fortests) to consumers; remove the export * from
"./sync-history.js" and instead add explicit named exports for only the stable
public API items from sync-history (list the intended functions/classes from
sync-history) while leaving any __*fortests / configure*fortests symbols only
inside the sync-history module (do not re-export them). Ensure the new explicit
exports replace the export * line so test hooks remain module-private.
In `@lib/quota-cache.ts`:
- Around line 201-204: The warning logs call logWarn with raw error.message
which can include absolute filesystem paths and leak usernames/emails; before
calling logWarn in the quota cache functions that reference
getQuotaCacheLabel(quotaCachePath) and tempPath (the occurrences around the
logWarn calls that include quotaCachePath and tempPath), sanitize the nested fs
error text by removing or redacting quotaCachePath and tempPath from
error.message (and from String(error) fallback) — e.g., replace occurrences of
quotaCachePath and tempPath with a redacted token — then pass the sanitized
message to logWarn; apply the same redaction logic to all three sites noted (the
blocks that call getQuotaCacheLabel(quotaCachePath) and any logWarn that
includes tempPath) so tests (vitest) can assert no absolute paths are emitted.
In `@lib/storage.ts`:
- Around line 3478-3482: The code writes reset marker files to resetMarkerPath
without ensuring the parent directory exists; before calling fs.writeFile for
resetMarkerPath (and the analogous call for the other reset marker), create the
parent directory using path.dirname(resetMarkerPath) and fs.mkdir(..., {
recursive: true }) and handle/propagate filesystem errors appropriately; add a
vitest regression that simulates an empty/nonexistent directory path and
verifies the reset path write no longer throws ENOENT; ensure you reference the
resetMarkerPath variable and the write logic you updated in tests and keep
logging free of sensitive tokens/emails.
In `@lib/sync-history.ts`:
- Around line 289-295: Wrap the file writes in a retry-with-backoff loop: when
calling fs.appendFile(paths.historyPath, `${serializeEntry(entry)}\n`, ...) and
the subsequent calls to trimHistoryFileIfNeeded(paths) and
rewriteLatestEntry(...), retry on transient filesystem errors (EPERM, EBUSY)
with exponential backoff and a sensible max attempts, propagating non-transient
errors immediately; on success clear lastAppendError, on final failure set
lastAppendError and surface/log a safe message (no tokens/emails). Update the
logic inside the same block that currently calls serializeEntry,
trimHistoryFileIfNeeded, and rewriteLatestEntry so each step is retried (or the
whole sequence retried) atomically, and add Vitest tests that mock fs errors to
throw EPERM/EBUSY transiently then succeed to verify retries occur and that
permanent errors still fail; ensure tests reference the functions
serializeEntry, trimHistoryFileIfNeeded, rewriteLatestEntry, and variable
lastAppendError.
- Around line 255-258: The current in-place write to paths.latestPath can
produce torn JSON on concurrent reads; modify the save logic that writes
`latest` so it writes JSON to a temporary file in the same directory (e.g.,
`${paths.latestPath}.tmp-${pid}-${timestamp}`) with the same encoding and mode
(0o600), fsyncs the file, then atomically replaces the target via fs.rename (and
optionally fsync the directory on POSIX) to ensure readers of `paths.latestPath`
never see partial content; on Windows or EBUSY errors add a small retry/backoff
for rename to handle transient file-locks, preserve logging that does not leak
tokens/emails, and update the vitest unit tests covering this behavior to assert
atomic replacement and handle the EBUSY/rename-retry case.
In `@test/auth-menu-hotkeys.test.ts`:
- Around line 288-301: The current test strips ANSI codes into strippedLabels
before asserting, which masks ANSI leakage; instead assert against the raw
labels (the labels array derived from selectMock.mock.calls[0][0]) to ensure no
escape sequences are present, or add an additional expectation such as
expect(labels.join("")).not.toMatch(/\\u001b\\[[0-?]*[ -/]*[`@-`~]/) (or a
dedicated ansiPattern) while keeping the existing content checks on
strippedLabels, referencing the variables selectMock, labels, and strippedLabels
in the test to pin the sanitizer regression.
In `@test/destructive-actions.test.ts`:
- Around line 24-44: The test currently fully mocks ../lib/storage.js so the new
delete/reset flows never exercise real snapshot/clear behavior; add a
deterministic vitest that does not mock snapshotAccountStorage (or that restores
the real implementation) and instead uses an fs spy or helper to simulate a
Windows busy-file error (throwing an Error with code 'EBUSY' or 'EPERM') when
snapshotAccountStorage runs, then exercise the delete/reset path (calling the
same entrypoint tested in "destructive actions") and assert the code
tolerates/retries the busy error and still calls
clearAccounts/clearFlaggedAccounts/clearQuotaCache as expected; reference the
symbols snapshotAccountStorage, clearAccounts, clearFlaggedAccounts,
clearQuotaCache and the test suite "destructive actions" so the new test runs
end-to-end against the real storage helpers while keeping determinism via
controlled fs spies in vitest.
In `@test/recovery.test.ts`:
- Around line 49-65: The cleanup retry logic in removeWithRetry currently only
treats "EBUSY", "EPERM", and "ENOTEMPTY" as transient; update removeWithRetry to
also treat "EACCES" and "ETIMEDOUT" as retryable error codes so Windows temp-dir
cleanup and timeout races are retried like other tests (preserve existing
attempt/backoff behavior and final throw on attempt === 4 or non-retryable
codes).
In `@test/storage-flagged.test.ts`:
- Around line 254-341: Add a deterministic regression test that simulates a
concurrent recreate-during-clear: after saveFlaggedAccounts(...) write a file,
spyOn fs.unlink inside the clearFlaggedAccounts() path (the same way other tests
do) but on the first successful unlink call recreate the flagged file (using
getFlaggedAccountsPath() and writeFileSync or saveFlaggedAccounts) before
allowing subsequent unlink calls to proceed; then assert clearFlaggedAccounts()
still returns true (or the expected boolean) and that loadFlaggedAccounts()
returns an empty store (no revived accounts). Reference the existing test
helpers and symbols: saveFlaggedAccounts, clearFlaggedAccounts,
getFlaggedAccountsPath, loadFlaggedAccounts, and the unlink spy used in the
tests to hook the race and restore the mock in finally.
---
Outside diff comments:
In `@docs/troubleshooting.md`:
- Around line 62-170: The troubleshooting doc mentions new restore/sync/reset
flows but lacks upgrade-note pointers and npm-script calls; update the two spots
around the "Restore From Backup" and "Reset Local State" passages to add a short
upgrade-note reference that directs readers to the release upgrade notes and the
new npm scripts (e.g., the deploy/rollout or migration scripts added for the
change), and add a brief parenthetical mentioning the code changes in
settings-hub.ts (settings-hub module, relevant flow at settings-hub.ts:2708) and
destructive-actions.ts (destructive-actions module, reset logic at
destructive-actions.ts:176) so operators can trace behavior, ensuring the
README/SECURITY/docs consistency requirement is satisfied.
In `@lib/codex-manager.ts`:
- Around line 5219-5227: Replace the use of `parsed` in the warning log with
`displayAccountNumber` so the message uses the visible row number (change the
`console.warn` line to reference `displayAccountNumber`), and keep the success
`console.log` using `displayAccountNumber` and `formatAccountLabel(account,
displayAccountNumber - 1)` as-is; then add a regression test that simulates a
switch where `displayAccountNumber !== parsed` (the sorted/visible-row flow) and
asserts the warning string contains `displayAccountNumber` (not `parsed`) to
ensure the visible-row semantics are preserved.
In `@lib/quota-cache.ts`:
- Around line 245-256: saveQuotaCache() and clearQuotaCache() race:
saveQuotaCache's delayed fs.rename(tempPath, quotaCachePath) can recreate
quota-cache.json after clearQuotaCache() returns, resurrecting stale data;
serialize their mutations by introducing a single in-process queue or generation
guard shared by saveQuotaCache and clearQuotaCache (use the existing function
names saveQuotaCache and clearQuotaCache and the quotaCachePath/tempPath
symbols) so only one mutation runs at a time, keep the existing EBUSY/EPERM
retry/backoff inside the serialized task, update tests in
test/quota-cache.test.ts to add a deterministic case that blocks fs.rename until
after clearQuotaCache resolves to assert no resurrection, and ensure no logs
emitted by these paths leak tokens or emails.
In `@lib/storage.ts`:
- Around line 3890-3923: The pre-import snapshot call snapshotAccountStorage is
currently outside the storage lock and can be invalidated by a concurrent
writer; move the snapshotAccountStorage call into the
withAccountStorageTransaction callback just before calling persist (so snapshot
reflects the locked pre-persist state), keep the existing dedup/limit checks and
persist(newStorage) flow, and update callers accordingly; add a Vitest that
races importAccounts (the function containing this logic) against a concurrent
save operation to assert the snapshot/rollback state is correct and
deterministic, and ensure the test names reference the file (lib/storage.ts) and
the import flow; also verify logging around this path does not leak sensitive
data and that any new queuing/retry logic used for concurrent writers accounts
for EBUSY/429 semantics.
In `@README.md`:
- Around line 101-119: Update the README (and mirror the same paragraph into
docs/getting-started.md and docs/upgrade.md) to explicitly document the new
split between visible dashboard row numbers and stored CLI indices: state that
dashboard quick-switch numbers reflect the current visible row order in the
interactive view while direct commands like "codex auth switch <n>" use the
saved storage index and therefore can diverge after sorting, filtering, or when
rows are sparse; reference the behavior exercised by tests
test/cli-auth-menu.test.ts:54 and test/cli-auth-menu.test.ts:70 as confirmation,
and add a short upgrade note mentioning this behavioral change plus any new npm
scripts introduced so users update their workflows accordingly.
In `@test/accounts-edge.test.ts`:
- Around line 108-125: The test currently simulates saveAccounts failing but
doesn't include a pendingRun in the mocked sync result, so the new
failure-handling branch in AccountManager.loadFromDisk (which calls
commitCodexCliSyncRunFailure when persistence fails) is never executed; update
the mock for mockSyncAccountStorageFromCodexCli to return a non-null pendingRun
(e.g., an object with an id/timestamp) and then assert that the commit mock for
the failure path (commitCodexCliSyncRunFailure or its test mock) was called once
and the success-commit mock (commitCodexCliSyncRunSuccess) was not called;
specifically change the mockResolvedValue for mockSyncAccountStorageFromCodexCli
to include pendingRun and add explicit expect(...) assertions for the commit
mocks after calling AccountManager.loadFromDisk() so the new branch in
lib/accounts.ts:AccountManager.loadFromDisk is exercised.
In `@test/cli-auth-menu.test.ts`:
- Around line 104-119: Add a test variant for the select-account ->
showAccountDetails path that uses an account object with divergent display
numbering (e.g., { index: 0, sourceIndex: 28, quickSwitchNumber: 7 }) to ensure
the branch uses the account's pinned quickSwitchNumber for selectedAccountNumber
while still targeting the storage index for switchAccountIndex; specifically, in
the test that calls showAuthMenu.mockResolvedValueOnce({ type: "select-account",
account: { index: 0, sourceIndex: 28, quickSwitchNumber: 7 } }) and
showAccountDetails.mockResolvedValueOnce("set-current"), assert the
promptLoginMode result returns mode: "manage", switchAccountIndex: 28, and
selectedAccountNumber: 7 so the code paths in promptLoginMode, showAuthMenu, and
showAccountDetails honor quickSwitchNumber rather than falling back to index+1.
In `@test/cli.test.ts`:
- Around line 745-797: The TTY-focused test for isNonInteractiveMode() doesn't
cover Windows-specific readline quirks (EPERM/EBUSY) described in the PR; add a
deterministic unit test that mocks/simulates Windows behavior by stubbing the
readline/open behavior or process.stdin.readableState to throw EPERM/EBUSY when
constructing the CLI module (targeting the same import("../lib/cli.js") and
isNonInteractiveMode() calls), asserting the module still resolves
non-interactive detection or handles the error as intended; if adding such a
mock is infeasible, add a short test case that documents the deferred coverage
and references isNonInteractiveMode() and the TTY branches so reviewers know the
gap is intentional.
In `@test/storage-recovery-paths.test.ts`:
- Around line 782-795: The test currently relies on a 20ms sleep to make
newerManualPath have a later mtime, which flakes on some filesystems; instead
after writing newerManualPath call fs.utimes (or fsPromises.utimes) to set its
mtime explicitly to a time later than the other file so getBackupMetadata
deterministically picks newerManualPath; update the test in
storage-recovery-paths.test.ts to replace the setTimeout-based ordering with an
fs.utimes call (referencing newerManualPath and the existing file paths and the
getBackupMetadata assertion) to set a later atime/mtime before calling
getBackupMetadata.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4499-4507: Remove the redundant null-check for
freshStorage.accounts[idx] that follows loadManageActionStorage's validation:
since loadManageActionStorage already rejects invalid/missing targets, delete
the unreachable branch that calls reportUnavailableManageActionAccount and
returns; simply assume account exists and proceed using the retrieved account
variable (account / freshStorage.accounts[idx]) in the surrounding logic,
ensuring no other references rely on that guard.
- Around line 4318-4344: Add direct vitest tests for
loadManageActionStorageSnapshot and isRetryableManageActionReloadCode: stub/mock
loadAccounts to throw NodeJS.ErrnoException with codes "EBUSY" and "EAGAIN" and
assert the function retries and eventually returns on a subsequent success; add
a test that stubs loadAccounts to always throw non-retryable code and assert
loadManageActionStorageSnapshot rethrows; add a Windows-specific test that
temporarily set process.platform to "win32" (or mock
isRetryableManageActionReloadCode) and verify that "EPERM" is treated as
retryable and that the backoff/retry loop behaves (succeeds after a retry or
exhausts and throws); ensure tests measure retry attempts (e.g., by counting
calls to loadAccounts) rather than relying on timing.
🪄 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: eae7ecca-b921-4165-bd96-791d1d70c93d
📒 Files selected for processing (43)
README.mddocs/README.mddocs/features.mddocs/getting-started.mddocs/index.mddocs/reference/commands.mddocs/reference/settings.mddocs/reference/storage-paths.mddocs/troubleshooting.mddocs/upgrade.mdindex.tslib/accounts.tslib/cli.tslib/codex-cli/sync.tslib/codex-manager.tslib/codex-manager/settings-hub.tslib/destructive-actions.tslib/index.tslib/live-account-sync.tslib/quota-cache.tslib/storage.tslib/sync-history.tslib/ui/auth-menu.tslib/ui/copy.tstest/accounts-edge.test.tstest/accounts-load-from-disk.test.tstest/accounts.test.tstest/auth-menu-hotkeys.test.tstest/cli-auth-menu.test.tstest/cli.test.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/destructive-actions.test.tstest/documentation.test.tstest/live-account-sync.test.tstest/quota-cache.test.tstest/recovery.test.tstest/release-main-prs-regression.test.tstest/settings-hub-utils.test.tstest/storage-flagged.test.tstest/storage-recovery-paths.test.tstest/storage.test.tstest/sync-history.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 (3)
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/cli.test.tstest/live-account-sync.test.tstest/accounts-edge.test.tstest/quota-cache.test.tstest/cli-auth-menu.test.tstest/auth-menu-hotkeys.test.tstest/settings-hub-utils.test.tstest/destructive-actions.test.tstest/release-main-prs-regression.test.tstest/accounts.test.tstest/storage-recovery-paths.test.tstest/documentation.test.tstest/storage-flagged.test.tstest/recovery.test.tstest/codex-cli-sync.test.tstest/accounts-load-from-disk.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/index.tslib/accounts.tslib/quota-cache.tslib/codex-manager/settings-hub.tslib/codex-manager.tslib/live-account-sync.tslib/cli.tslib/destructive-actions.tslib/codex-cli/sync.tslib/ui/auth-menu.tslib/storage.tslib/sync-history.tslib/ui/copy.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/index.mddocs/reference/storage-paths.mddocs/getting-started.mddocs/reference/settings.mddocs/features.mddocs/upgrade.mddocs/reference/commands.mddocs/troubleshooting.mddocs/README.md
🪛 LanguageTool
docs/getting-started.md
[style] ~79-~79: To make your writing flow more naturally, try moving the adverb ‘already’ closer to the verb ‘named’.
Context: ...rt Fresh Use the restore path when you already have named backup files and want to recover accoun...
(PERF_TENS_ADV_PLACEMENT)
🔇 Additional comments (14)
test/cli.test.ts (2)
1-11: formatting and mock setup look good.the mock setup for readline/promises is clean and properly resets between tests. no concerns with the test scaffolding.
707-743: tests correctly assert only mode since restore-backup and import-opencode don't return selectedAccountNumber in these paths.the fallback paths in
lib/cli.ts:280andlib/cli.ts:283return only{ mode: "restore-backup" }and{ mode: "import-opencode" }respectively. selectedAccountNumber is only returned for account management actions (like set-current-account), which are tested separately intest/cli-auth-menu.test.ts. the assertions in your tests attest/cli.test.ts:707-743correctly match what the functions actually return.the tty property manipulation at
test/cli.test.ts:219-321has proper try/finally restoration logic withObject.definePropertyrestoration—no concurrency risk.docs/reference/storage-paths.md (1)
105-115: restore workflow documentation is clear.the four-step restore workflow and direct entrypoint are well documented. this aligns with the backup browser UI and
codex auth restore-backupcommand added in this pr.docs/index.md (1)
15-16: interactive workflow documentation is well-structured.the new dashboard workflows section at lines 44-49 documents the three key interactive paths (backup restore, sync preview/apply, settings split) with clear navigation. the recoverable backups prompt note at line 15-16 correctly describes the conditional behavior.
Also applies to: 44-49
docs/reference/settings.md (2)
77-103: codex cli sync documentation is thorough.the sync center documentation covers prerequisites, pre-sync state details, workflow notes, and validation steps. the one-way sync clarification at line 95 is important for user understanding.
per coding guidelines, this correctly mentions npm scripts (
npm run typecheck,npm run build,npm test) in the validation section.
22-25: section rename from "account list view" to "everyday settings" is clear.the intro note at line 24 explaining the separation between everyday settings and advanced operator controls provides good context for users navigating the settings surface.
docs/README.md (2)
20-26: documentation portal updates align with content changes.the updated focus descriptions for index.md, getting-started.md, features.md, troubleshooting.md, commands.md, and settings.md accurately reflect the expanded scope covering restore, sync, and diagnostics workflows.
Also applies to: 43-44
29-35: archived release notes references are well organized.the release notes archive from v0.1.0-beta.0 through v0.1.9 provides good historical reference for users tracking version changes.
docs/reference/commands.md (3)
23-23: new restore-backup command properly documented.the login description update at line 23 correctly reflects the expanded entry points, and the new
codex auth restore-backupcommand at line 29 is consistent withdocs/reference/storage-paths.md:112-114.Also applies to: 29-29
104-111: interactive dashboard workflows section is well organized.the five workflow paths (backup restore, startup recovery prompt, sync preview/apply, stable settings, advanced settings) provide clear navigation guidance. the TTY-only note for startup recovery prompt at line 107 is accurate per pr objectives.
94-99: hotkey documentation matches ui implementation.all settings screen hotkey sequences in docs/reference/commands.md lines 94-99 match the corresponding help text in lib/ui/copy.ts:line 113-129 (accountListHelp, summaryHelp, behaviorHelp, themeHelp, backendHelp). section titles align correctly across both files.
test/release-main-prs-regression.test.ts (1)
192-212: good regression coverage for windows EPERM handling.the test exercises the persistent failure path: unlink fails with EPERM on every retry, clearFlaggedAccounts returns false, but flagged accounts are cleared from memory. the retry-success path is already covered by
test/storage-flagged.test.ts:254-295which verifies the first call fails with EPERM and the second succeeds. these two tests together provide solid coverage for the windows permission error scenario mentioned in pr objectives.test/live-account-sync.test.ts (1)
321-632: good regression coverage on the new live-sync race paths
test/live-account-sync.test.ts:321-632exercises queued reloads, stale completion drops, and stop-during-switch behavior with vitest-controlled timing. that is the right shape of regression coverage for the new generation guards inlib/live-account-sync.ts:313-386.As per coding guidelines,
test/**: 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.lib/codex-cli/sync.ts (1)
312-348: revision allocation at publish time ensures "last sync" matches disk state; test coverage exists for the ordering scenariolib/codex-cli/sync.ts:312-348 both
commitPendingCodexCliSyncRun()andcommitCodexCliSyncRunFailure()allocate a fresh revision at commit time (not reusing the pending revision). combined with the revision check inpublishCodexCliSyncRun()at line 223 (if (revision <= lastCodexCliSyncRunRevision)), whichever commit executes last gets the higher revision and wins.lib/accounts.ts:119-129 saves accounts first (awaited), then commits synchronously, ensuring the commit follows disk completion. lib/storage.ts:484-490 the global
withStorageLockmutex serializes all account storage writes.result: an older failure cannot overwrite a newer success in "last sync" because the success commit will always get a higher revision if it happens after the failure.
regression test coverage exists: test/codex-cli-sync.test.ts line 2242 ("publishes the completion that finishes last even when it started earlier") verifies this exact scenario—second run fails, first run succeeds, first wins because it commits with higher revision.
no issue found.
| | --- | --- | | ||
| | Unified settings | `~/.codex/multi-auth/settings.json` | | ||
| | Accounts | `~/.codex/multi-auth/openai-codex-accounts.json` | | ||
| | Named backups | `~/.codex/multi-auth/backups/<name>.json` | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# verify backup directory path matches documented paths
ast-grep --pattern 'getNamedBackupsDirectoryPath($$$)'
rg -nA5 'backups.*\.json' lib/storage.tsRepository: ndycode/codex-multi-auth
Length of output: 1085
🏁 Script executed:
#!/bin/bash
# Find getNamedBackupsDirectoryPath implementation
rg -nB2 -A10 'function getNamedBackupsDirectoryPath|export.*getNamedBackupsDirectoryPath' lib/Repository: ndycode/codex-multi-auth
Length of output: 734
🏁 Script executed:
#!/bin/bash
# Search entire codebase for getNamedBackupsDirectoryPath definition
rg -nB2 -A10 'getNamedBackupsDirectoryPath\s*='Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check the actual backup directory construction in lib/storage.ts
rg -n 'backups' lib/storage.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 1056
🏁 Script executed:
#!/bin/bash
# Find getNamedBackupRoot implementation
rg -nB2 -A5 'function getNamedBackupRoot|getNamedBackupRoot\s*=|export.*getNamedBackupRoot'Repository: ndycode/codex-multi-auth
Length of output: 646
🏁 Script executed:
#!/bin/bash
# Also search for project-scoped backup paths mentioned at line 60
rg -n 'projects.*backups|project.*backup.*json' docs/reference/storage-paths.mdRepository: ndycode/codex-multi-auth
Length of output: 221
🏁 Script executed:
#!/bin/bash
# Find BACKUP_EXPORT_DIR_NAME constant
rg -n 'BACKUP_EXPORT_DIR_NAME\s*=' lib/Repository: ndycode/codex-multi-auth
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Find getStoragePath implementation
rg -nB2 -A8 'export function getStoragePath' lib/storage.tsRepository: ndycode/codex-multi-auth
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Check actual directory structure in tests to see what path is actually used
rg -n 'multi-auth.*backups' test/ | head -20Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find getConfigDir implementation
rg -nB2 -A8 'export function getConfigDir|function getConfigDir' lib/storage.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Also verify the project-scoped backup path construction
rg -nB2 -A10 'projects.*backup|project.*backup' lib/storage.ts | head -40Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Look for any test that shows actual constructed paths
rg -n 'multi-auth.*backups' test/storage.test.ts | head -5Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find getConfigDir definition - may be in a different file
rg -n 'getConfigDir\s*=' lib/ | grep -v testRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for where project-scoped backups are built
rg -n 'getProjectBackupRoot|project.*backup.*path' lib/Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check storage.ts for CONFIG_DIR or home directory references
rg -n 'CONFIG_DIR|\.codex|expandHome' lib/storage.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 461
🏁 Script executed:
#!/bin/bash
# Search for CONFIG_DIR constant and home directory handling
rg -n 'CONFIG_DIR|expandTilde|expandHome|homedir' lib/ | head -20Repository: ndycode/codex-multi-auth
Length of output: 1643
🏁 Script executed:
#!/bin/bash
# Look at the beginning of storage.ts for imports and constants
head -100 lib/storage.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 3591
🏁 Script executed:
#!/bin/bash
# Find where storage path is actually set to ~/.codex/multi-auth
rg -n '\.codex.*multi-auth|multi-auth.*accounts' lib/storage.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Read lib/storage/paths.ts to see getConfigDir and getCodexMultiAuthDir implementations
cat lib/storage/paths.ts | head -200Repository: ndycode/codex-multi-auth
Length of output: 7434
🏁 Script executed:
#!/bin/bash
# Look for getCodexMultiAuthDir function
rg -nB2 -A8 'getCodexMultiAuthDir'Repository: ndycode/codex-multi-auth
Length of output: 17583
🏁 Script executed:
#!/bin/bash
# Find project storage path construction
rg -nB2 -A10 'getProjectStoragePath|resolveProjectStorageIdentityRoot' lib/storage.ts | head -40Repository: ndycode/codex-multi-auth
Length of output: 834
🏁 Script executed:
#!/bin/bash
# Search for how project backups directory is constructed
rg -n 'projects.*backups|backup.*project' lib/named-backup-export.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify the full path construction for project backups
rg -nB5 -A5 'buildProjectNamedBackupPath|getProjectNamedBackup' lib/Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find where project storage gets set
rg -nB2 -A10 'currentStoragePath = join.*getProjectGlobalConfigDir' lib/storage.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Get the definition of getProjectGlobalConfigDir
rg -nB2 -A10 'export function getProjectGlobalConfigDir'Repository: ndycode/codex-multi-auth
Length of output: 954
🏁 Script executed:
#!/bin/bash
# Search for the test that shows actual backup paths
rg -n 'backups.*json' test/storage.test.ts | grep -i project | head -10Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify the actual file is at the right location in getConfigDir
rg -n 'ACCOUNTS_FILE_NAME' lib/storage.ts | head -5Repository: ndycode/codex-multi-auth
Length of output: 342
🏁 Script executed:
#!/bin/bash
# Check if there are test cases covering the backup path scenarios
rg -n 'getNamedBackupsDirectoryPath|multi-auth.*backups' test/storage.test.ts | head -15Repository: ndycode/codex-multi-auth
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Check for any concurrency or windows-specific issues in backup path code
rg -n 'windows|win32|concurrent|race|lock' lib/named-backup-export.tsRepository: ndycode/codex-multi-auth
Length of output: 1555
🏁 Script executed:
#!/bin/bash
# Search for any TODOs or FIXMEs related to backup paths
rg -n 'TODO|FIXME' lib/named-backup-export.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Get more context on the test cases for backup paths
rg -nB3 -A8 'getNamedBackupsDirectoryPath' test/storage.test.ts | head -50Repository: ndycode/codex-multi-auth
Length of output: 1467
🏁 Script executed:
#!/bin/bash
# Check if tests cover both canonical and project-scoped backup scenarios
rg -n 'describe.*backup|it.*backup' test/storage.test.ts | grep -i namedRepository: ndycode/codex-multi-auth
Length of output: 2083
🏁 Script executed:
#!/bin/bash
# Trace the full path construction: getStoragePath() -> dirname -> dirname + /backups
python3 << 'EOF'
from pathlib import Path
# Simulate the path construction
config_dir = Path.home() / ".codex" / "multi-auth"
accounts_file = config_dir / "openai-codex-accounts.json"
# getStoragePath() returns accounts_file path
storage_path = accounts_file
# getNamedBackupRoot does: dirname(storagePath) / "backups"
backup_root = storage_path.parent / "backups"
print(f"config_dir (getCodexMultiAuthDir): {config_dir}")
print(f"storage_path (getStoragePath): {storage_path}")
print(f"dirname(storage_path): {storage_path.parent}")
print(f"backup_root (getNamedBackupsDirectoryPath): {backup_root}")
# For project-scoped
project_key = "abc123"
project_storage_dir = config_dir / "projects" / project_key
project_accounts_file = project_storage_dir / "openai-codex-accounts.json"
project_backup_root = project_accounts_file.parent / "backups"
print(f"\nproject_storage_dir: {project_storage_dir}")
print(f"project_backup_root: {project_backup_root}")
EOFRepository: ndycode/codex-multi-auth
Length of output: 516
🏁 Script executed:
#!/bin/bash
# Check for concurrency safety in backup operations
rg -nB3 -A10 'createNamedBackup|exportNamedBackup' lib/storage.ts | head -60Repository: ndycode/codex-multi-auth
Length of output: 1815
🏁 Script executed:
#!/bin/bash
# Check for file locking or mutex patterns in backup code
rg -n 'lock|mutex|atomic|flock' lib/ | grep -i backupRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify test coverage for windows paths
rg -n 'win32|windows' test/storage.test.ts | head -10Repository: ndycode/codex-multi-auth
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Check exportNamedBackupFile implementation for concurrency safety
rg -nB5 -A20 'export.*function exportNamedBackupFile' lib/named-backup-export.tsRepository: ndycode/codex-multi-auth
Length of output: 1342
🏁 Script executed:
#!/bin/bash
# Look for windows path edge cases in tests
rg -nB2 -A8 'win32.*backup|windows.*backup' test/Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify if project-scoped backup tests exist
rg -n 'project.*backup|backup.*project' test/storage.test.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for any tests involving project-scoped backups
rg -n 'project.*backup|getProjectGlobalConfigDir.*backup|projects.*backups' test/storage.test.tsRepository: ndycode/codex-multi-auth
Length of output: 50
backup paths documented correctly, but missing regression tests and concurrency safeguards.
the canonical paths ~/.codex/multi-auth/backups/<name>.json and project-scoped ~/.codex/multi-auth/projects/<project-key>/backups/<name>.json match the implementation in lib/named-backup-export.ts:176-180 and lib/storage/paths.ts:242-243. however:
- no regression tests for project-scoped backups: test/storage.test.ts has "named backup helpers" tests but only exercises canonical paths. need tests covering
getProjectGlobalConfigDir(projectPath)backup scenarios. - concurrency risk: lib/named-backup-export.ts:237 uses
inFlightNamedBackupExports(in-memory Set) to prevent concurrent overwrites. this won't prevent cross-process races if multiple codex processes export the same backup name simultaneously. concurrent file writes could corrupt backups. - missing windows edge case tests: no tests for backup paths on windows with case-insensitive filesystem or mixed path separators. normalizePathForComparison() exists but isn't validated against actual backup operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/storage-paths.md` at line 25, Add regression tests that
exercise project-scoped backup path resolution by calling
getProjectGlobalConfigDir(projectPath) and asserting produced backup paths match
the documented project-scoped pattern; update test/storage.test.ts to include
these cases. Replace the in-memory concurrency guard
(inFlightNamedBackupExports) in lib/named-backup-export.ts with a
cross-process-safe write pattern (e.g., atomic write via temp file + fs.rename
or OS file-locking) to prevent concurrent-export corruption. Add
unit/integration tests that simulate Windows-style paths and case-insensitive
comparisons to validate normalizePathForComparison() is applied before backup
path comparisons and actual write operations, ensuring mixed separators and case
variants still map to the same backup file. Ensure tests clean up temp files and
cover both canonical and project-scoped backup flows.
| ### Startup Recovery Prompt | ||
|
|
||
| Interactive `codex auth login` now offers named-backup recovery before OAuth only when the session starts with zero saved accounts and at least one recoverable named backup. | ||
|
|
||
| The prompt is intentionally skipped in fallback/non-interactive login paths and after same-session `fresh` or `reset` actions so an intentional wipe does not immediately re-offer restore state. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
startup recovery prompt documentation is accurate but could cross-reference direct entrypoint.
this section correctly describes when the recovery prompt appears. consider adding a note about codex auth restore-backup as the direct entrypoint for users who want to skip the login flow, similar to how docs/reference/storage-paths.md:112-114 documents it.
The prompt is intentionally skipped in fallback/non-interactive login paths and after same-session `fresh` or `reset` actions so an intentional wipe does not immediately re-offer restore state.
+
+For direct access outside the login flow, use `codex auth restore-backup`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/upgrade.md` around lines 65 - 70, Add a short cross-reference to the
direct entrypoint for recovery by appending one sentence to the "Startup
Recovery Prompt" section that mentions the explicit command users can run to
restore backups without going through the interactive login flow (refer to the
command "codex auth restore-backup"), and link or point readers to the
corresponding auth reference page or restore-backup doc entry for full usage
details; ensure the note clarifies it is an alternative to waiting for the
interactive prompt and that it bypasses login flow.
| function resolveAccountDisplayNumber( | ||
| account: ExistingAccountInfo, | ||
| ): number { | ||
| if ( | ||
| typeof account.quickSwitchNumber === "number" && | ||
| Number.isFinite(account.quickSwitchNumber) | ||
| ) { | ||
| return Math.max(1, Math.floor(account.quickSwitchNumber)); | ||
| } | ||
| return Math.max( | ||
| 1, | ||
| typeof account.index === "number" && Number.isFinite(account.index) | ||
| ? Math.floor(account.index) + 1 | ||
| : 1, | ||
| ); | ||
| } |
There was a problem hiding this comment.
do not synthesize a display number from the storage index here.
lib/cli.ts:150-165 falls back to account.index + 1 whenever quickSwitchNumber is missing. that quietly restores the old storage-index semantics for the exact manage path this pr is fixing, so delete/refresh/toggle confirmations downstream can show the wrong visible row number. fail closed here, or leave selectedAccountNumber unset, so a missing quickSwitchNumber is caught instead of masked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/cli.ts` around lines 150 - 165, resolveAccountDisplayNumber currently
synthesizes a display number from account.index when quickSwitchNumber is
absent; change it to fail closed by returning undefined (or leaving
selectedAccountNumber unset) instead of deriving index+1: update
resolveAccountDisplayNumber(account: ExistingAccountInfo) to only return a
number when account.quickSwitchNumber is a finite number (Math.floor and
Math.max(1,...)), otherwise return undefined; update any callsites that assign
to selectedAccountNumber to handle the undefined case (do not silently convert
from account.index), and ensure downstream confirmation UI relies on
quickSwitchNumber being present rather than a synthesized value.
| function warnUnresolvableAccountSelection(account: ExistingAccountInfo): void { | ||
| const label = account.email?.trim() || account.accountId?.trim() || `index ${account.index + 1}`; | ||
| const label = | ||
| account.email?.trim() || | ||
| account.accountId?.trim() || | ||
| `index ${account.index + 1}`; | ||
| console.log(`Unable to resolve saved account for action: ${label}`); | ||
| } |
There was a problem hiding this comment.
avoid logging account identifiers in the unresolved-selection path.
lib/cli.ts:167-173 currently prints account.email or account.accountId when selection resolution fails. this path only needs a stable row reference, and emitting user identifiers here leaks pii into shell captures and support logs for no real benefit. prefer the visible row number or a generic label instead.
suggested change
function warnUnresolvableAccountSelection(account: ExistingAccountInfo): void {
const label =
- account.email?.trim() ||
- account.accountId?.trim() ||
- `index ${account.index + 1}`;
+ typeof account.quickSwitchNumber === "number" &&
+ Number.isFinite(account.quickSwitchNumber)
+ ? `row ${Math.max(1, Math.floor(account.quickSwitchNumber))}`
+ : `index ${account.index + 1}`;
console.log(`Unable to resolve saved account for action: ${label}`);
}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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/cli.ts` around lines 167 - 173, The warnUnresolvableAccountSelection
function currently logs PII by printing account.email and account.accountId;
change it to only emit a stable, non-identifying row reference such as `row
${account.index + 1}` or a generic label like `saved account row ${account.index
+ 1}` and remove any use of account.email/account.accountId in the message;
update any affected vitest cases to expect the new generic label and ensure no
tests rely on printing emails/accountIds.
| const RETRYABLE_SELECTION_TIMESTAMP_CODES = new Set(["EBUSY", "EPERM"]); | ||
| export const SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS = 4; | ||
|
|
There was a problem hiding this comment.
retry eacces here too
lib/codex-cli/sync.ts:36-38 and lib/codex-cli/sync.ts:629-658 retry ebusy and eperm, but they still treat transient eacces as terminal. on windows, fs.stat() can surface eacces during antivirus/indexer interference. that sends persistedLocalTimestamp to null, and lib/codex-cli/sync.ts:755-768 can then skip a legitimate codex-cli active-selection import. please add eacces to the retry set and cover it with a vitest regression.
suggested fix
-const RETRYABLE_SELECTION_TIMESTAMP_CODES = new Set(["EBUSY", "EPERM"]);
+const RETRYABLE_SELECTION_TIMESTAMP_CODES = new Set([
+ "EBUSY",
+ "EPERM",
+ "EACCES",
+]);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.
Also applies to: 629-658
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-cli/sync.ts` around lines 36 - 38, Add "EACCES" to the
RETRYABLE_SELECTION_TIMESTAMP_CODES set so transient permission errors are
retried (update the constant alongside RETRYABLE_SELECTION_TIMESTAMP_CODES and
ensure SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS logic still applies), then update
the retry/error handling in the selection-timestamp read logic around the block
that currently checks for "EBUSY" and "EPERM" (the code handling fs.stat() /
persistedLocalTimestamp around the logic starting near the retry loop at the
section referenced by the diff around lines 629-658) to treat "EACCES" the same
as the other retryable codes; finally add a vitest regression that simulates
fs.stat throwing an EACCES error and asserts the code retries and does not set
persistedLocalTimestamp to null and that the active-selection import path still
proceeds.
| await fs.appendFile(paths.historyPath, `${serializeEntry(entry)}\n`, { | ||
| encoding: "utf8", | ||
| mode: 0o600, | ||
| }); | ||
| const prunedHistory = await trimHistoryFileIfNeeded(paths); | ||
| await rewriteLatestEntry(prunedHistory.latest ?? entry, paths); | ||
| lastAppendError = null; |
There was a problem hiding this comment.
add retry/backoff around history append + trim writes.
line 289 in lib/sync-history.ts:289 appends and then rewrites history/latest without retry wrappers. on windows, transient EPERM/EBUSY locks will fail this flow and drop sync-history updates. wrap these file writes with retry/backoff and add vitest coverage that mocks transient lock errors.
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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sync-history.ts` around lines 289 - 295, Wrap the file writes in a
retry-with-backoff loop: when calling fs.appendFile(paths.historyPath,
`${serializeEntry(entry)}\n`, ...) and the subsequent calls to
trimHistoryFileIfNeeded(paths) and rewriteLatestEntry(...), retry on transient
filesystem errors (EPERM, EBUSY) with exponential backoff and a sensible max
attempts, propagating non-transient errors immediately; on success clear
lastAppendError, on final failure set lastAppendError and surface/log a safe
message (no tokens/emails). Update the logic inside the same block that
currently calls serializeEntry, trimHistoryFileIfNeeded, and rewriteLatestEntry
so each step is retried (or the whole sequence retried) atomically, and add
Vitest tests that mock fs errors to throw EPERM/EBUSY transiently then succeed
to verify retries occur and that permanent errors still fail; ensure tests
reference the functions serializeEntry, trimHistoryFileIfNeeded,
rewriteLatestEntry, and variable lastAppendError.
| const items = (selectMock.mock.calls[0]?.[0] as Array<{ | ||
| label: string; | ||
| value: { type: string }; | ||
| }>).filter((item) => item.value.type === "select-account"); | ||
| const labels = items.map((item) => item.label); | ||
| const strippedLabels = labels.map((label) => | ||
| label.replace(new RegExp("\\u001b\\[[0-?]*[ -/]*[@-~]", "g"), ""), | ||
| ); | ||
|
|
||
| expect(strippedLabels[0]).toContain("1. first@example.com"); | ||
| expect(strippedLabels[1]).toContain("2. Friendly Label"); | ||
| expect(strippedLabels[2]).toContain("3. acc-id-42"); | ||
| // biome-ignore lint/suspicious/noControlCharactersInRegex: intentional test assertion | ||
| expect(strippedLabels.join("")).not.toMatch(/[\u0000\u0007\u007f]/); |
There was a problem hiding this comment.
this assertion masks ansi leaks instead of catching them.
the normalization at test/auth-menu-hotkeys.test.ts:293 strips escape sequences before the expectations run, so this still passes if showAuthMenu() leaves ansi codes in the rendered labels. assert against the unmodified labels, or add a separate not.toMatch(ansiPattern) on the raw output, so the sanitizer regression is actually pinned. test/auth-menu-hotkeys.test.ts:293
suggested test fix
- const strippedLabels = labels.map((label) =>
- label.replace(new RegExp("\\u001b\\[[0-?]*[ -/]*[`@-`~]", "g"), ""),
- );
-
- expect(strippedLabels[0]).toContain("1. first@example.com");
- expect(strippedLabels[1]).toContain("2. Friendly Label");
- expect(strippedLabels[2]).toContain("3. acc-id-42");
+ const ansiPattern = /\u001b\[[0-?]*[ -/]*[`@-`~]/g;
+ expect(labels.join("")).not.toMatch(ansiPattern);
+ expect(labels[0]).toContain("1. first@example.com");
+ expect(labels[1]).toContain("2. Friendly Label");
+ expect(labels[2]).toContain("3. acc-id-42");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/auth-menu-hotkeys.test.ts` around lines 288 - 301, The current test
strips ANSI codes into strippedLabels before asserting, which masks ANSI
leakage; instead assert against the raw labels (the labels array derived from
selectMock.mock.calls[0][0]) to ensure no escape sequences are present, or add
an additional expectation such as
expect(labels.join("")).not.toMatch(/\\u001b\\[[0-?]*[ -/]*[`@-`~]/) (or a
dedicated ansiPattern) while keeping the existing content checks on
strippedLabels, referencing the variables selectMock, labels, and strippedLabels
in the test to pin the sanitizer regression.
| vi.mock("../lib/storage.js", () => ({ | ||
| clearAccounts: clearAccountsMock, | ||
| clearFlaggedAccounts: clearFlaggedAccountsMock, | ||
| loadFlaggedAccounts: loadFlaggedAccountsMock, | ||
| saveAccounts: saveAccountsMock, | ||
| saveFlaggedAccounts: saveFlaggedAccountsMock, | ||
| snapshotAccountStorage: snapshotAccountStorageMock, | ||
| })); | ||
|
|
||
| describe("destructive actions", () => { | ||
| beforeEach(() => { | ||
| vi.resetModules(); | ||
| vi.clearAllMocks(); | ||
| clearAccountsMock.mockResolvedValue(true); | ||
| clearFlaggedAccountsMock.mockResolvedValue(true); | ||
| clearQuotaCacheMock.mockResolvedValue(true); | ||
| loadFlaggedAccountsMock.mockResolvedValue({ version: 1, accounts: [] }); | ||
| saveAccountsMock.mockResolvedValue(undefined); | ||
| saveFlaggedAccountsMock.mockResolvedValue(undefined); | ||
| snapshotAccountStorageMock.mockResolvedValue(null); | ||
| }); |
There was a problem hiding this comment.
add a real windows busy-file regression for these destructive flows.
test/destructive-actions.test.ts:24-44 fully mocks ../lib/storage.js, so the new delete/reset paths never exercise the ebusy/eperm snapshot and clear behavior that the corresponding lib/** changes are supposed to tolerate. right now the suite only proves call ordering. please add at least one vitest that runs against the real storage helpers or fs spies so a windows busy-file failure can reproduce end to end.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/destructive-actions.test.ts` around lines 24 - 44, The test currently
fully mocks ../lib/storage.js so the new delete/reset flows never exercise real
snapshot/clear behavior; add a deterministic vitest that does not mock
snapshotAccountStorage (or that restores the real implementation) and instead
uses an fs spy or helper to simulate a Windows busy-file error (throwing an
Error with code 'EBUSY' or 'EPERM') when snapshotAccountStorage runs, then
exercise the delete/reset path (calling the same entrypoint tested in
"destructive actions") and assert the code tolerates/retries the busy error and
still calls clearAccounts/clearFlaggedAccounts/clearQuotaCache as expected;
reference the symbols snapshotAccountStorage, clearAccounts,
clearFlaggedAccounts, clearQuotaCache and the test suite "destructive actions"
so the new test runs end-to-end against the real storage helpers while keeping
determinism via controlled fs spies in vitest.
| async function removeWithRetry(targetPath: string): Promise<void> { | ||
| for (let attempt = 0; attempt < 5; attempt += 1) { | ||
| try { | ||
| await fs.rm(targetPath, { recursive: true, force: true }); | ||
| return; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if ( | ||
| (code !== "EBUSY" && code !== "EPERM" && code !== "ENOTEMPTY") || | ||
| attempt === 4 | ||
| ) { | ||
| throw error; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
treat eacces and etimedout as retryable during cleanup.
test/recovery.test.ts:49-65 only retries ebusy, eperm, and enotempty. windows temp-dir cleanup failures commonly surface as eacces, and other suites here already account for that broader retry set in test/codex-cli-sync.test.ts:34-55. leaving those codes out will keep this suite flaky on the exact platform the recovery code is trying to support.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/recovery.test.ts` around lines 49 - 65, The cleanup retry logic in
removeWithRetry currently only treats "EBUSY", "EPERM", and "ENOTEMPTY" as
transient; update removeWithRetry to also treat "EACCES" and "ETIMEDOUT" as
retryable error codes so Windows temp-dir cleanup and timeout races are retried
like other tests (preserve existing attempt/backoff behavior and final throw on
attempt === 4 or non-retryable codes).
| it.each(["EPERM", "EBUSY"] as const)( | ||
| "retries transient %s when clearing flagged storage", | ||
| async (code) => { | ||
| await saveFlaggedAccounts({ | ||
| version: 1, | ||
| accounts: [ | ||
| { | ||
| refreshToken: "stale-primary", | ||
| flaggedAt: 1, | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| await expect(clearFlaggedAccounts()).rejects.toThrow( | ||
| "EPERM primary delete", | ||
| ); | ||
| const flaggedPath = getFlaggedAccountsPath(); | ||
| const realUnlink = fs.unlink.bind(fs); | ||
| let failedOnce = false; | ||
| const unlinkSpy = vi | ||
| .spyOn(fs, "unlink") | ||
| .mockImplementation(async (targetPath) => { | ||
| if (targetPath === flaggedPath && !failedOnce) { | ||
| failedOnce = true; | ||
| const error = new Error("locked") as NodeJS.ErrnoException; | ||
| error.code = code; | ||
| throw error; | ||
| } | ||
| return realUnlink(targetPath); | ||
| }); | ||
|
|
||
| try { | ||
| await expect(clearFlaggedAccounts()).resolves.toBe(true); | ||
| expect(existsSync(flaggedPath)).toBe(false); | ||
| expect( | ||
| unlinkSpy.mock.calls.filter( | ||
| ([targetPath]) => targetPath === flaggedPath, | ||
| ), | ||
| ).toHaveLength(2); | ||
| } finally { | ||
| unlinkSpy.mockRestore(); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| const flagged = await loadFlaggedAccounts(); | ||
| expect(existsSync(flaggedPath)).toBe(true); | ||
| expect(flagged.accounts).toHaveLength(0); | ||
| it.each(["EPERM", "EBUSY"] as const)( | ||
| "returns false when clearing flagged storage exhausts retryable %s failures", | ||
| async (code) => { | ||
| await saveFlaggedAccounts({ | ||
| version: 1, | ||
| accounts: [ | ||
| { | ||
| refreshToken: "stuck-flagged", | ||
| flaggedAt: 1, | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| unlinkSpy.mockRestore(); | ||
| }); | ||
| const flaggedPath = getFlaggedAccountsPath(); | ||
| const unlinkSpy = vi | ||
| .spyOn(fs, "unlink") | ||
| .mockImplementation(async (targetPath) => { | ||
| if (targetPath === flaggedPath) { | ||
| const error = new Error( | ||
| "still locked", | ||
| ) as NodeJS.ErrnoException; | ||
| error.code = code; | ||
| throw error; | ||
| } | ||
| const error = new Error("missing") as NodeJS.ErrnoException; | ||
| error.code = "ENOENT"; | ||
| throw error; | ||
| }); | ||
|
|
||
| try { | ||
| await expect(clearFlaggedAccounts()).resolves.toBe(false); | ||
| expect(existsSync(flaggedPath)).toBe(true); | ||
| expect( | ||
| unlinkSpy.mock.calls.filter( | ||
| ([targetPath]) => targetPath === flaggedPath, | ||
| ), | ||
| ).toHaveLength(5); | ||
| } finally { | ||
| unlinkSpy.mockRestore(); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
add a recreate-during-clear regression here.
test/storage-flagged.test.ts:254 and test/storage-flagged.test.ts:298 cover transient EPERM/EBUSY, but they never recreate getFlaggedAccountsPath() or a backup artifact after one delete succeeds. that means a windows cross-process rewrite regression can still go green: clearFlaggedAccounts() could clear the original file, another process could recreate it mid-flight, and the suite would not catch the revival on the next load. add a case that recreates the primary during clear and asserts loadFlaggedAccounts() still returns an empty store afterward. test/storage-flagged.test.ts:254, test/storage-flagged.test.ts:298
as per coding guidelines, test/**: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage-flagged.test.ts` around lines 254 - 341, Add a deterministic
regression test that simulates a concurrent recreate-during-clear: after
saveFlaggedAccounts(...) write a file, spyOn fs.unlink inside the
clearFlaggedAccounts() path (the same way other tests do) but on the first
successful unlink call recreate the flagged file (using getFlaggedAccountsPath()
and writeFileSync or saveFlaggedAccounts) before allowing subsequent unlink
calls to proceed; then assert clearFlaggedAccounts() still returns true (or the
expected boolean) and that loadFlaggedAccounts() returns an empty store (no
revived accounts). Reference the existing test helpers and symbols:
saveFlaggedAccounts, clearFlaggedAccounts, getFlaggedAccountsPath,
loadFlaggedAccounts, and the unlink spy used in the tests to hook the race and
restore the mock in finally.
Summary
fresh/19-first-run-wizard#91ff69952What Changed
codex auth switch <n>semantics unchanged while making manage-action switch confirmations and stale-selection handling follow the visible row number.fresh/19-first-run-wizardhead so it inherits the latest first-run wizard, import redaction, and sync-history fixes from#103.Validation
npm run lintnpm run typechecknpm run buildnpm testDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
#91and the stack-refresh mergeff69952if neededAdditional Notes
107files,2727tests passed.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 preserves visible dashboard row numbers separately from storage indexes for manage-action confirmations (switch, delete, toggle, refresh), so users see consistent numbering in all messages. it also adds
sync-history.ts,lib/destructive-actions.ts, a first-run wizard surface inauth-menu.ts, backup/restore plumbing incodex-manager.ts, and a write-ahead journal check incodex auth doctor.key changes:
resolveManageActionAccountNumberreadsmenuResult.selectedAccountNumber(the visible row) and falls back tofallbackIndex + 1, ensuring all manage-action log lines use the visible row numberrunSwitchnow acceptsRunSwitchOptions.displayAccountNumberandpreloadedStorage;formatAccountLabelis called withdisplayAccountNumber - 1so the "Account N" label is consistent with the visible row prefixloadManageActionStoragereloads storage before each manage action and validates the account identity viafindMatchingAccountIndex, catching stale-selection racesdestructiveActionInFlightmodule-level boolean prevents re-entrant delete-all / reset actionslib/destructive-actions.tsextractsdeleteAccountAtIndex,deleteSavedAccounts, andresetLocalState, each preceded bysnapshotAccountStoragefor safetysync-history.tsadds an ndjson-based history log with an async mutex (withHistoryLock) and a 200-entry rolling capsanitizeTerminalTextinauth-menu.tsnow uses pre-compiled regex constants instead of inline literalsgetReadOnlyDoctorRefreshTokenFingerprinthashes tokens with sha-256 (truncated to 12 hex chars) to detect duplicate refresh-token entries without logging raw tokensConfidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant AuthMenu as showAuthMenu participant CLI as promptLoginMode participant Manager as handleManageAction participant Loader as loadManageActionStorage participant Storage as AccountStorageV3 participant Switch as runSwitch User->>AuthMenu: selects account row N (quickSwitchNumber) AuthMenu-->>CLI: AuthMenuAction {select-account, account.quickSwitchNumber=N} CLI-->>Manager: LoginMenuResult {switchAccountIndex=storageIdx, selectedAccountNumber=N} Manager->>Manager: resolveManageActionAccountNumber(menuResult, storageIdx) → displayAccountNumber=N Manager->>Loader: loadManageActionStorage(storage, N, storageIdx) Loader->>Storage: loadManageActionStorageSnapshot() Storage-->>Loader: freshStorage Loader->>Loader: findMatchingAccountIndex(freshStorage, selectedAccount) alt account identity mismatch or out of range Loader-->>Manager: null (reportUnavailableManageActionAccount logged) Manager-->>User: error: "Selected account N is no longer available" else account valid Loader-->>Manager: freshStorage Manager->>Switch: runSwitch([storageIdx+1], {displayAccountNumber: N, preloadedStorage}) Switch->>Switch: displayAccountNumber = N (from options) Switch->>Storage: saveAccounts / setCodexCliActiveSelection Switch-->>User: "Switched to account N: Account N (email)" endComments Outside Diff (12)
lib/codex-manager.ts, line 4185-4197 (link)degenerate "1-0" range when storage is empty
reportUnavailableManageActionAccountis called withtotalAccounts = 0from both thetargetIndex < 0guard (line 4205) and the empty-freshStorage guard (line 4219). whentargetIndex >= 0andtotalAccounts = 0, the conditiontargetIndex >= totalAccountsis always true, so the output is:"1-0" is a degenerate, confusing range. the function should special-case
totalAccounts <= 0and emit a message like "no accounts configured" instead:lib/codex-manager.ts, line 4280-4288 (link)stale-write window between
loadManageActionStorageandsaveAccountsloadManageActionStoragecallsloadAccounts()and returnsfreshStorage. that reference is then passed aspreloadedStorageintorunSwitch, which uses it directly forsaveAccounts— skipping a second reload. on windows, another codex process or the user themselves could write accounts.json between those two await points (oauth refresh, token sync), causing the finalsaveAccountsto clobber interleaved changes silently.before this PR,
runSwitchloaded its own fresh copy right before writing, minimizing that window. now the window spans the entire oauth token-refresh path (potentially several seconds).worth adding a note in the code and consider a re-read-before-write pattern (compare-and-swap on the file) or at minimum flagging this in a comment so it's not an invisible assumption. no vitest covers concurrent-write scenarios here.
lib/codex-manager.ts, line 4252-4259 (link)unreachable guard — dead code
by line 4252, two earlier conditions already guarantee this branch can never fire:
targetIndex < freshStorage.accounts.lengthmatchingIndex === targetIndex—findMatchingAccountIndexonly returns an index when the slot is truthy, sofreshStorage.accounts[targetIndex]is guaranteed non-null herethe dead check won't be exercised by any test, and its presence misleads future readers into thinking there's a case where
matchingIndex === targetIndexyet the slot is still falsy. remove it:lib/codex-manager.ts, line 4319-4327 (link)unreachable guard after
matchingIndex !== targetIndexcheckreaching line 4319 requires
matchingIndex === targetIndex.findMatchingAccountIndexreturningtargetIndexmeansfreshStorage.accounts[targetIndex]was the matched entry — it cannot be falsy. this block is dead code and silently hides any future logic added between the two checks.remove the redundant
!freshStorage.accounts[targetIndex]guard entirely.lib/codex-manager.ts, line 4283-4295 (link)loadManageActionStoragethrows go unhandled in switch/delete/toggle branchesloadManageActionStorageSnapshotcan throw two ways: immediately on a non-retryable filesystem error (e.g.EACCES), or after exhausting all 5 retries on a persistentEBUSY/EAGAIN(orEPERMon windows). only therefreshAccountIndexbranch wrapsloadManageActionStoragein a try/catch — theswitchAccountIndex,deleteAccountIndex, andtoggleAccountIndexbranches call it bare. a filesystem error in any of those three paths propagates uncaught throughhandleManageActionand crashes the CLI process.the retry-exhaustion and non-retryable error paths are also not covered by vitest for those three branches.
alternatively, keep the try/catch at each call-site so branch-specific messages can differ, but the switch/delete/toggle branches must be covered too.
lib/codex-manager.ts, line 4363-4402 (link)switch / delete / toggle don't catch storage-reload exhaustion — inconsistent with refresh branch
only the
refreshAccountIndexbranch wrapsloadManageActionStoragein a try/catch and surfaces a user-facing error. theswitchAccountIndex,deleteAccountIndex, andtoggleAccountIndexbranches callloadManageActionStoragewithout any error handling:if
loadManageActionStorageSnapshotexhausts all five retries (EBUSY on linux/mac, EBUSY or EPERM on windows), the exception propagates throughhandleManageActionuncaught and crashes the entirerunAuthLoginloop. on windows, a locked token file can produce repeated EPERM, making this a realistic failure path.the three branches should wrap
loadManageActionStorageidentically to the refresh branch:there are also no vitest tests for switch/delete/toggle exhaustion of the retry loop — the retry tests in the new suite only cover the
refreshAccountIndexpath.lib/codex-manager.ts, line 4282-4299 (link)unreachable sentinel throw and misleading return type
the
throw new Error("Manage action storage reload retry loop exhausted unexpectedly")after the loop is dead code. the loop is guaranteed to exit viareturnorthrowbefore the condition becomes false:return await loadAccounts()exits immediatelyattempt === 4forces re-throw in the catch, so the loop body always exits viareturnorthrowthe loop condition
attempt < 5can never be false without the function having already returned or thrown. this also means the function never actually returnsnull, so the declared return typePromise<AccountStorageV3 | null>is misleading — it either returnsAccountStorageV3or throws. the!freshStoragenull-guard inloadManageActionStorageis therefore always false.if you want to keep the sentinel for TypeScript's exhaustive-return checker, mark it with a comment so it's clear it's intentionally unreachable — but also update the return type to drop
| null.lib/cli.ts, line 147-159 (link)selectedAccountNumbernever set in fallback manage actionspromptLoginModeFallbackreturns manage-action results directly (e.g.{ mode: "manage", switchAccountIndex: index }) without going throughbuildManageResult, soselectedAccountNumberis alwaysundefinedon that code path.resolveManageActionAccountNumberthen falls back tofallbackIndex + 1(storage index + 1), meaning the non-interactive terminal path silently bypasses the visible-row semantics this PR is meant to introduce.if
promptLoginModeFallbackcan ever return per-account manage indexes (e.g. from a numeric account picker), those results should also be wrapped withbuildManageResult. if it truly never does, add an explicit comment and a test asserting the absence ofselectedAccountNumberin all its returned shapes so the invariant is documented.lib/codex-manager.ts, line 4334-4348 (link)TOCTOU gap between storage validation and write
loadManageActionStoragevalidates identity (matchingIndex === targetIndex) then returnsfreshStorage. all four callers — switch, delete, toggle, refresh — then perform an async write (e.g.saveAccounts,deleteAccountAtIndex) on that snapshot. another concurrent process (e.g. a parallelcodex auth switch) can mutate storage on disk between theloadManageActionStorageSnapshot()read and the subsequent write, silently operating on stale data.this is a concurrency issue on all platforms but is especially risky on windows where file-lock semantics differ. the existing WAL-based save logic provides some protection for the write side, but the read → validate → write path has no lock. recommend documenting the known race or wrapping the validate + write in a single exclusive operation.
lib/destructive-actions.ts, line 139 (link)undefined refreshToken causes over-broad flagged-account removal
if
target.refreshTokenisundefined(account was stored without a token), the filteraccount.refreshToken !== target.refreshTokenevaluates toaccount.refreshToken !== undefined, which removes every flagged account that also lacks arefreshToken. on windows, token writes can fail silently and leave stub entries with norefreshToken, making this realistic.guard the filter so it only runs when the token is a non-empty string:
no vitest coverage for this path exists in
test/destructive-actions.test.ts.lib/codex-manager.ts, line 809-812 (link)toggle mutates freshStorage directly then saves — concurrent writes possible
account.enabled = account.enabled === falsemutates the object insidefreshStorage.accountsin-place.loadManageActionStoragereloads a snapshot, but between that reload andsaveAccounts(freshStorage)there is anawaitgap (theloadManageActionStorageretry loop itself). a second manage action triggered from a different shell process can write to the same file during that gap, and this save will silently win and overwrite it.this is a concurrency issue on all platforms but is especially likely on windows where background sync or the quota-cache writer can hold the file open (
EBUSY). consider using an atomic compare-and-swap via the WAL path, or at minimum document that the last-write-wins behaviour is intentional.lib/codex-manager.ts, line 1524 (link)formatAccountLabel index still diverges from targetIndex for direct
codex auth switch <n>callswhen
runSwitchis invoked directly from the CLI (nooptions.displayAccountNumber),displayAccountNumber === parsedandtargetIndex === parsed - 1, sodisplayAccountNumber - 1 === targetIndex— the two are equivalent and this line is correct.however, when called from a manage action where
displayAccountNumberdiffers fromtargetIndex + 1(sparse visible rows),formatAccountLabel(account, displayAccountNumber - 1)passes the visible-row index rather than the actual storage slot.formatAccountLabeluses this index only to build the human-readable "Account N" prefix, so the label is consistent with the visible row, which is the intent of this PR.the
test/codex-manager-cli.test.tsmock stubsformatAccountLabelas`${index + 1}. ${account.email}`, so the assertion checks"Switched to account 1: 1. account-29@example.com"— confirming visible-row consistency. if that mock is ever removed, ensure an integration test covers the sparse-row case so a regression here is caught.Prompt To Fix All With AI
Last reviewed commit: "fix(auth): restore P..."