fix(auth): define reset and delete safety flows#104
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:
📝 WalkthroughWalkthroughintroduces a destructive-actions framework and a guarded reset flow; replaces in-place storage mutations with coordinated helpers; adds retryable unlink/rename and quota-cache APIs; surfaces typed confirmations in cli/ui; and expands tests for destructive actions, retry semantics, sanitization, and rollback behaviors. Changes
Sequence DiagramsequenceDiagram
participant user as user/cli
participant menu as auth menu
participant mgr as codex manager
participant actions as destructive actions
participant storage as storage handlers
participant fs as filesystem
user->>menu: select delete or reset
menu->>menu: require typed confirmation
menu->>mgr: invoke login flow (mode=delete|reset)
mgr->>mgr: check destructiveActionInFlight
mgr->>actions: call deleteAccountAtIndex / deleteSavedAccounts / resetLocalState
actions->>storage: perform saveAccounts / saveFlagged / clearQuotaCache / unlink targets
storage->>fs: unlink/rename with retry/backoff
fs-->>storage: operation result or error
storage-->>actions: aggregated result (success/partial/failure)
actions-->>mgr: result payload (accountsCleared, flaggedCleared, quotaCacheCleared, removedFlaggedCount)
mgr-->>user: log completion or sanitized error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes additional reviewer flags:
🚥 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
This PR hardens destructive auth/storage actions by introducing explicit “Delete Saved Accounts” vs “Reset Local State” flows, adding retryable cleanup for transient filesystem errors, and aligning the TUI + fallback CLI prompts and docs with the new behavior.
Changes:
- Add
lib/destructive-actions.tsto centralize delete/reset behaviors and ensure safe rollback when deleting a single account. - Make account/flagged/quota-cache cleanup more resilient (retry transient unlink/rename failures; return success/failure booleans instead of throwing in some cases).
- Update auth menu/CLI prompts, and expand tests/docs to cover the new destructive flows.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/storage.test.ts | Adds coverage for retryable deletes, legacy artifact cleanup, and quota-cache clearing. |
| test/storage-flagged.test.ts | Updates expectations for flagged clear to return boolean outcomes and exercise retry behavior. |
| test/release-main-prs-regression.test.ts | Aligns regression expectation with new “return false” behavior. |
| test/quota-cache.test.ts | Adds tests for dynamic quota-cache path resolution and clear retries. |
| test/destructive-actions.test.ts | New unit tests for delete/reset flows and delete rollback semantics. |
| test/codex-manager-cli.test.ts | Updates CLI tests to use destructive-actions entrypoints and adds concurrency/waiting scenarios. |
| test/cli.test.ts | Extends CLI prompt tests for typed confirmations and reset flow. |
| test/cli-auth-menu.test.ts | Adds reset-all flow tests and updates cancellation copy. |
| test/auth-menu-hotkeys.test.ts | Adds tests for reset/delete actions, plus sanitization assertions. |
| lib/ui/copy.ts | Updates danger-zone copy and adds reset option label. |
| lib/ui/auth-menu.ts | Adds reset-all action, removes extra confirm for delete-all/reset-all, and sanitizes terminal text. |
| lib/storage.ts | Introduces unlink retry helper; changes clear APIs to return boolean and expands artifact deletion set. |
| lib/quota-cache.ts | Makes quota cache path dynamic per call and adds clearQuotaCache() with retries + boolean result. |
| lib/destructive-actions.ts | New module defining delete/reset flows and safe delete-with-rollback for single accounts. |
| lib/codex-manager.ts | Switches to destructive-actions and adds in-flight destructive-action gating. |
| lib/cli.ts | Adds typed confirm prompts wired to destructive-action copy and supports reset mode. |
| index.ts | Switches to destructive-actions for delete/reset and single-account delete behavior. |
| docs/troubleshooting.md | Documents new reset options and manual equivalents. |
| README.md | Updates install + usage examples (adds codex auth list and Codex install). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/codex-manager.ts
Outdated
| const deleted = await deleteAccountAtIndex({ | ||
| storage, | ||
| index: idx, | ||
| }); | ||
| if (deleted) { | ||
| const label = `Account ${idx + 1}`; | ||
| const flaggedNote = | ||
| deleted.removedFlaggedCount > 0 | ||
| ? ` Removed ${deleted.removedFlaggedCount} matching problem account${deleted.removedFlaggedCount === 1 ? "" : "s"}.` | ||
| : ""; | ||
| console.log(`Deleted ${label}.${flaggedNote}`); | ||
| } |
index.ts
Outdated
| const deleted = await deleteAccountAtIndex({ | ||
| storage: workingStorage, | ||
| index: menuResult.deleteAccountIndex, | ||
| }); | ||
| if (deleted) { | ||
| invalidateAccountManagerCache(); | ||
| console.log(`\nDeleted ${target.email ?? `Account ${menuResult.deleteAccountIndex + 1}`}.\n`); | ||
| const label = `Account ${menuResult.deleteAccountIndex + 1}`; | ||
| const flaggedNote = | ||
| deleted.removedFlaggedCount > 0 | ||
| ? ` Removed ${deleted.removedFlaggedCount} matching problem account${deleted.removedFlaggedCount === 1 ? "" : "s"}.` | ||
| : ""; | ||
| console.log(`\nDeleted ${label}.${flaggedNote}\n`); | ||
| } |
lib/storage.ts
Outdated
| * Deletes the account storage file from disk. | ||
| * Silently ignores if file doesn't exist. |
| * Silently ignores if file doesn't exist. | ||
| */ | ||
| export async function clearAccounts(): Promise<void> { | ||
| export async function clearAccounts(): Promise<boolean> { |
| } | ||
|
|
||
| export async function clearFlaggedAccounts(): Promise<void> { | ||
| export async function clearFlaggedAccounts(): Promise<boolean> { |
| async function unlinkWithRetry(path: string): Promise<void> { | ||
| for (let attempt = 0; attempt < 5; attempt += 1) { | ||
| try { | ||
| await fs.unlink(path); | ||
| return; | ||
| } catch (error) { | ||
| const unlinkError = error as NodeJS.ErrnoException; | ||
| const code = unlinkError.code; | ||
| if (code === "ENOENT") { | ||
| return; | ||
| } | ||
| if ((code === "EPERM" || code === "EBUSY" || code === "EAGAIN") && attempt < 4) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt)); | ||
| continue; | ||
| } | ||
| throw unlinkError; | ||
| } | ||
| } | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/cli-auth-menu.test.ts (1)
282-295:⚠️ Potential issue | 🟡 Minorassert readline cleanup on the cancel paths.
test/cli-auth-menu.test.ts:282-323,325-337covers the negative typed-confirm flows, but it never checksmockRl.close.test/cli-auth-menu.test.ts:279already locks the success path down; the cancel paths should do the same, or a leaked readline handle can slip through and hang interactive exits.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.Also applies to: 308-323, 325-337
🤖 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 282 - 295, The cancel-path tests for promptLoginMode (e.g., the "cancels delete-all when typed confirmation is not DELETE" case that stubs showAuthMenu and uses mockRl.question) do not assert that the readline handle was closed; add an assertion that mockRl.close was called (e.g., expect(mockRl.close).toHaveBeenCalled()) after awaiting promptLoginMode in each negative/ cancel flow test, and ensure any console spies or mocks are restored afterwards so the readline mock cannot leak across tests.test/storage-flagged.test.ts (1)
15-16: 🧹 Nitpick | 🔵 TrivialRETRYABLE_REMOVE_CODES includes ENOTEMPTY but no test coverage for it.
the set at line 15 includes
ENOTEMPTYbut the parameterized tests only coverEPERMandEBUSY. consider addingENOTEMPTYto the test matrix for completeness, though it's less common in this context.🤖 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 15 - 16, The test matrix is missing coverage for the ENOTEMPTY case even though RETRYABLE_REMOVE_CODES includes it; update the parameterized tests that iterate over retryable error codes (the test using RETRYABLE_REMOVE_CODES or the array of codes in the parameterized test) to include "ENOTEMPTY" alongside "EPERM" and "EBUSY" so the behavior for ENOTEMPTY is asserted the same as the other retryable codes; locate the parameterized test in storage-flagged.test.ts that references EPERM/EBUSY and add ENOTEMPTY to that list so the test exercises the same retry logic as the RETRYABLE_REMOVE_CODES constant.test/codex-manager-cli.test.ts (2)
456-567:⚠️ Potential issue | 🟠 Majorforce non-interactive tty in the default setup.
test/codex-manager-cli.test.ts:564restores the host descriptors but never forces them off. on a real tty, the new destructive-action cases can enterrunActionPanel()/waitForMenuReturn()vialib/codex-manager.ts:3966and change logging behavior or hang on input. default this suite tosetInteractiveTTY(false), and letsetupInteractiveSettingsLogin()remain the only opt-in path totrue.proposed fix
selectMock.mockResolvedValue(undefined); restoreTTYDescriptors(); + setInteractiveTTY(false); setStoragePathMock.mockReset(); getStoragePathMock.mockReturnValue("/mock/openai-codex-accounts.json"); });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/codex-manager-cli.test.ts` around lines 456 - 567, The test suite leaves the process TTY in an interactive state causing non-deterministic hangs via runActionPanel()/waitForMenuReturn; add a deterministic default by calling setInteractiveTTY(false) in the beforeEach setup (alongside the existing restoreTTYDescriptors() call) so tests default to non-interactive; keep setupInteractiveSettingsLogin() as the only place that opts into true. Reference: the test file's beforeEach block and the helpers setInteractiveTTY and setupInteractiveSettingsLogin (and the runtime paths runActionPanel / waitForMenuReturn) to locate where to insert the call.
3391-3449:⚠️ Potential issue | 🟠 Majormake the delete test assert the rebased pool, not just the helper call.
test/codex-manager-cli.test.ts:3393pinsloadAccountsMockto the pre-delete pool for every read, so this case still passes even iflib/codex-manager.ts:3825stops refreshing local state or the helper never persists the deletion. back the mock with mutable state and assert the next menu render sees the one-account pool.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/codex-manager-cli.test.ts` around lines 3391 - 3449, The test currently pins loadAccountsMock to a static pre-delete pool so it only verifies deleteAccountAtIndexMock was called; change the mock to use mutable state (e.g., a local in-test variable representing current storage) and have loadAccountsMock return that state on each call, update that state when deleteAccountAtIndexMock is invoked (or make deleteAccountAtIndexMock resolve to the new storage), then after runCodexMultiAuthCli(["auth","login"]) assert the next menu/render reflects the rebased one-account pool (verify loadAccountsMock returned the post-deletion storage and the UI/menu contains only the remaining email) instead of only checking deleteAccountAtIndexMock call; keep using vitest-style mocks and assertions (loadAccountsMock, deleteAccountAtIndexMock, promptLoginModeMock, runCodexMultiAuthCli).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 3108-3121: The advanced-tool deletion path still mutates storage
inline and bypasses the safety/rollback/in-flight/flagged-sync guarantees
implemented in lib/destructive-actions.ts; change that path to call the same
deleteAccountAtIndex helper used in the manage path (preserving its call
signature: { storage, index }) and then apply the returned deleted.storage and
deleted.flagged via Object.assign, call invalidateAccountManagerCache(), and
surface deleted.removedFlaggedCount in the same console message flow; remove the
inline mutation logic so all deletions go through deleteAccountAtIndex to ensure
consistency with test/destructive-actions.test.ts.
- Around line 3164-3178: The reset/fresh branches call resetLocalState and set
startFresh without coordinating with the existing destructive-action guard; add
the same concurrent protection used by lib/codex-manager.ts (the
destructiveActionInFlight guard) around the index-level handlers that perform
destructive operations (the code paths that set startFresh and call
resetLocalState) so a second dispatch is serialized or rejected, and update
invalidateAccountManagerCache usage to run only after the guarded operation
completes; then add regression tests that double-trigger the index-level "reset"
and "fresh" paths to assert serialized/rejected behavior and that simulate
Windows EPERM/EBUSY during concurrent destructive operations to validate the
same lock handling seen in codex-manager and existing
destructive-actions.test.ts.
In `@lib/codex-manager.ts`:
- Around line 3817-3831: Before calling deleteAccountAtIndex in the menu
handling branch that checks menuResult.deleteAccountIndex, guard with the
destructiveActionInFlight flag: check it and set it true before invoking
deleteAccountAtIndex, then use try/finally to ensure destructiveActionInFlight
is reset to false after the call (or on error). Locate the menu branch that
reads menuResult.deleteAccountIndex and wrap the delete flow (the call to
deleteAccountAtIndex and subsequent Object.assign/logging) inside that
check/try/finally using the existing destructiveActionInFlight boolean used by
resetLocalState and deleteSavedAccounts so concurrent writes are prevented;
update tests to add a concurrency scenario exercising this race window.
In `@lib/destructive-actions.ts`:
- Around line 193-199: deleteSavedAccounts currently lets exceptions from
clearAccounts bubble up; wrap the call to clearAccounts in a try/catch so the
function returns a DestructiveActionResult with accountsCleared set to true on
success and false on failure (do not rethrow), matching the semantics used by
resetLocalState; keep the other fields (flaggedCleared, quotaCacheCleared)
unchanged and log or capture the error if needed for diagnostics inside the
catch block.
- Around line 205-215: resetLocalState currently swallows errors for each step
via .catch(() => false) which hides which operation failed; change it to call
each operation (clearAccounts, clearFlaggedAccounts, clearQuotaCache) inside its
own try/catch so failures are isolated, log the thrown error with context (e.g.,
"failed to clear accounts", "failed to clear flagged accounts", "failed to clear
quota cache") using your project's logger (e.g., processLogger) or
console.warn/error, ensure clearCodexCliStateCache still runs unconditionally,
and return the same DestructiveActionResult booleans based on success/failure of
each step.
- Around line 120-142: The manage-mode delete path is not protected by the
destructiveActionInFlight gate which allows concurrent calls to
deleteAccountAtIndex to race; update the manage-mode branch in
lib/codex-manager.ts (the block that calls handleManageAction()) to
acquire/check the destructiveActionInFlight flag the same way the "fresh" and
"reset" branches do, ensuring the flag is set before calling
handleManageAction() and cleared after, or alternatively add a concurrency test
in test/destructive-actions.test.ts that invokes deleteAccountAtIndex
concurrently to assert the race is prevented; reference
destructiveActionInFlight, handleManageAction, and deleteAccountAtIndex when
making the change or writing the test.
In `@lib/storage.ts`:
- Around line 2004-2023: The clearAccounts boolean helper is writing the reset
marker before ensuring the parent storage directory exists which can throw
ENOENT on empty/fresh installs; update clearAccounts (and the other boolean
clear helper around 2272-2287) to either short-circuit and return false when
there is nothing to clear (no WAL, backups, or legacy paths) or ensure the
parent directory exists (mkdirp/getStoragePath parent) before writing
getIntentionalResetMarkerPath; locate code using getStoragePath,
getIntentionalResetMarkerPath, getAccountsWalPath, and
getAccountsBackupRecoveryCandidatesWithDiscovery to implement the fix, add a
Vitest regression that runs the helper against an empty temp dir to assert no
exception and correct boolean return, and keep logging free of tokens/emails and
robust to Windows EBUSY semantics.
- Around line 2289-2294: The destructive sweep currently only deletes the
current flagged file and its backups (uses
getAccountsBackupRecoveryCandidatesWithDiscovery(path)) but misses the legacy
flagged file, so update the sweep in lib/storage.ts (the loop that calls
unlinkWithRetry on each candidate) to also include
getLegacyFlaggedAccountsPath() in the candidates array, ensuring the legacy path
is cleared alongside path and backupPaths; update the corresponding test
test/release-main-prs-regression.test.ts to add a case that creates the legacy
file (openai-codex-blocked-accounts.json) and asserts it is removed after the
sweep, and while touching unlinkWithRetry ensure retry/queue logic still handles
EBUSY/429 scenarios without leaking tokens or emails in logs.
- Around line 1857-1869: The cleanup after rename currently calls
fs.unlink(resetMarkerPath) and swallows errors, which leaves reset markers on
Windows; replace that bare unlink with the project's retry helper
unlinkWithRetry(resetMarkerPath) (mirroring the retry pattern used in
renameFileWithRetry) so failures like EPERM/EBUSY are retried before giving up,
preserving the existing "best effort" semantics; do the same for the walPath
removal (fs.unlink(walPath) -> unlinkWithRetry(walPath)) if you want symmetric
handling, and ensure tests exercise this behavior similarly to
test/quota-cache.test.ts (the retry-style coverage at lines ~108-184). Also keep
lastAccountsSaveTimestamp update and surrounding return logic unchanged.
In `@test/auth-menu-hotkeys.test.ts`:
- Around line 240-258: The tests "returns delete-all without an extra confirm
prompt" and "returns reset-all without an extra confirm prompt" only assert
confirmMock, which misses regressions that reintroduce typed confirmation via
the mocked node:readline/promises; update both specs to also assert that
searchQuestionMock and searchCloseMock were not called (e.g.,
expect(searchQuestionMock).not.toHaveBeenCalled();
expect(searchCloseMock).not.toHaveBeenCalled()) after calling showAuthMenu with
createAccounts(), referencing the existing mocks and the showAuthMenu import to
ensure typed-confirmation helpers in lib/ui/auth-menu.ts remain unused for these
destructive actions.
In `@test/codex-manager-cli.test.ts`:
- Around line 3506-3568: Add deterministic vitest cases to cover the
partial-cleanup warning branches in lib/codex-manager.ts by simulating cleanup
failure (the Windows ebusy/eperm path): create one test for the "delete saved
accounts" flow and one for the "reset local state" flow that mirror the existing
tests (use runCodexMultiAuthCli from lib/codex-manager.js, mock loadAccountsMock
and promptLoginModeMock the same way) but have deleteSavedAccountsMock or
resetLocalStateMock resolve/return false instead of success, then assert
exitCode === 0 and that console.log was called with the specific partial-cleanup
warning message emitted by the code paths (the messages around the
partial-cleanup branches referenced in lib/codex-manager.ts). Ensure tests
remain deterministic by using vitest mocks (no real secrets, no timers/races)
and restore spies after assertions.
In `@test/destructive-actions.test.ts`:
- Around line 299-360: The test for deleteAccountAtIndex currently asserts the
original flaggedSaveError and rollbackError are preserved, but the
implementation runs errors through asSafeAggregateError which may sanitize
path-like messages; update the test by making the mocked errors
(flaggedSaveError and rollbackError) include path characters (e.g. a string with
"/" or "\") so sanitization is exercised, and change the expectation on
aggregateError.errors to assert the sanitized errors/messages produced by
asSafeAggregateError (instead of the original Error objects) while keeping the
rest of the test (calls to saveAccountsMock and storage assertions) unchanged.
In `@test/storage.test.ts`:
- Around line 3362-3389: The test's mock relies on unlinkSpy.mock.calls.length
to detect the first call which is brittle; change the mock to use an independent
counter variable (e.g., callCount) captured in the test scope and increment it
inside the mockImplementation, then throw the transient error when callCount ===
1; update the test around unlinkSpy and its mock (the mockImplementation for
fs.unlink used in the "retries transient %s when clearing the quota cache" case
that exercises clearQuotaCache and getQuotaCachePath) and restore behavior the
same otherwise.
---
Outside diff comments:
In `@test/cli-auth-menu.test.ts`:
- Around line 282-295: The cancel-path tests for promptLoginMode (e.g., the
"cancels delete-all when typed confirmation is not DELETE" case that stubs
showAuthMenu and uses mockRl.question) do not assert that the readline handle
was closed; add an assertion that mockRl.close was called (e.g.,
expect(mockRl.close).toHaveBeenCalled()) after awaiting promptLoginMode in each
negative/ cancel flow test, and ensure any console spies or mocks are restored
afterwards so the readline mock cannot leak across tests.
In `@test/codex-manager-cli.test.ts`:
- Around line 456-567: The test suite leaves the process TTY in an interactive
state causing non-deterministic hangs via runActionPanel()/waitForMenuReturn;
add a deterministic default by calling setInteractiveTTY(false) in the
beforeEach setup (alongside the existing restoreTTYDescriptors() call) so tests
default to non-interactive; keep setupInteractiveSettingsLogin() as the only
place that opts into true. Reference: the test file's beforeEach block and the
helpers setInteractiveTTY and setupInteractiveSettingsLogin (and the runtime
paths runActionPanel / waitForMenuReturn) to locate where to insert the call.
- Around line 3391-3449: The test currently pins loadAccountsMock to a static
pre-delete pool so it only verifies deleteAccountAtIndexMock was called; change
the mock to use mutable state (e.g., a local in-test variable representing
current storage) and have loadAccountsMock return that state on each call,
update that state when deleteAccountAtIndexMock is invoked (or make
deleteAccountAtIndexMock resolve to the new storage), then after
runCodexMultiAuthCli(["auth","login"]) assert the next menu/render reflects the
rebased one-account pool (verify loadAccountsMock returned the post-deletion
storage and the UI/menu contains only the remaining email) instead of only
checking deleteAccountAtIndexMock call; keep using vitest-style mocks and
assertions (loadAccountsMock, deleteAccountAtIndexMock, promptLoginModeMock,
runCodexMultiAuthCli).
In `@test/storage-flagged.test.ts`:
- Around line 15-16: The test matrix is missing coverage for the ENOTEMPTY case
even though RETRYABLE_REMOVE_CODES includes it; update the parameterized tests
that iterate over retryable error codes (the test using RETRYABLE_REMOVE_CODES
or the array of codes in the parameterized test) to include "ENOTEMPTY"
alongside "EPERM" and "EBUSY" so the behavior for ENOTEMPTY is asserted the same
as the other retryable codes; locate the parameterized test in
storage-flagged.test.ts that references EPERM/EBUSY and add ENOTEMPTY to that
list so the test exercises the same retry logic as the RETRYABLE_REMOVE_CODES
constant.
🪄 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: a7b63af9-aa4b-4696-8a99-59988db5789a
📒 Files selected for processing (19)
README.mddocs/troubleshooting.mdindex.tslib/cli.tslib/codex-manager.tslib/destructive-actions.tslib/quota-cache.tslib/storage.tslib/ui/auth-menu.tslib/ui/copy.tstest/auth-menu-hotkeys.test.tstest/cli-auth-menu.test.tstest/cli.test.tstest/codex-manager-cli.test.tstest/destructive-actions.test.tstest/quota-cache.test.tstest/release-main-prs-regression.test.tstest/storage-flagged.test.tstest/storage.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-auth-menu.test.tstest/release-main-prs-regression.test.tstest/auth-menu-hotkeys.test.tstest/storage-flagged.test.tstest/destructive-actions.test.tstest/cli.test.tstest/codex-manager-cli.test.tstest/quota-cache.test.tstest/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.tslib/quota-cache.tslib/destructive-actions.tslib/codex-manager.tslib/ui/copy.tslib/ui/auth-menu.tslib/cli.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/troubleshooting.md
🔇 Additional comments (23)
lib/destructive-actions.ts (3)
174-178: path leakage on successful rollback path.when
saveFlaggedAccountsfails and rollback succeeds,asSafeAggregateErroris called at line 174, but the original error is already captured at line 153 viaasError. if the original error message contains a path, that path is preserved inoriginalErrorand only sanitized when re-wrapping at line 174.however, looking closer:
asSafeAggregateErrorcreates a new error withfallbackMessagewhen paths are detected, so the sanitized version is what's thrown. this looks correct.one edge case: if
originalError.messagecontains a path butasSafeAggregateErroris called with a differentfallbackMessagethat doesn't match the original context, the user loses the original error code context. the code preserveserrorCodethough, so this is fine.
38-69: clampActiveIndices handles edge cases well.the function correctly:
- handles empty accounts array (lines 46-50)
- validates
activeIndexis finite (lines 41-44)- clamps per-family indices with proper fallback (lines 54-68)
looks solid for the index management logic.
71-86: rebaseActiveIndicesAfterDelete only decrements indices above removedIndex.this is correct behavior - indices at or below the removed position stay the same (except the removed one is gone), indices above shift down by 1.
note: if
activeIndex === removedIndex, it stays unchanged here butclampActiveIndicesis called after (line 140) to fix it. that's the right pattern.docs/troubleshooting.md (2)
92-104: documentation accurately reflects the new destructive action semantics.the exact effects table clearly documents what each action does. this matches the implementation in
lib/destructive-actions.ts:
deleteSavedAccountsonly clears accounts (line 195 setsflaggedCleared: false)resetLocalStateclears accounts, flagged, and quota cache (lines 206-208)good alignment between docs and code.
110-120: manual cleanup instructions are consistent with implementation.the bash/powershell commands now include
.waland.bak*artifacts, which matches theclearAccountsimplementation that removes these files. as per coding guidelines, docs should stay consistent with actual CLI behavior - this looks aligned.test/storage-flagged.test.ts (2)
235-277: good coverage for transient EPERM/EBUSY retry behavior.the parameterized tests at
lib/storage-flagged.test.ts:235-277properly verify:
- first unlink fails with transient error
- second attempt succeeds
- file is removed
- exactly 2 unlink calls on the primary path
this covers the windows filesystem edge cases mentioned in coding guidelines.
279-322: exhausted retry tests verify correct failure semantics.tests at
lib/storage-flagged.test.ts:279-322confirm that when all 5 retry attempts fail:
clearFlaggedAccounts()returnsfalse(not throws)- primary file remains on disk
- exactly 5 unlink attempts recorded
this matches the boolean-return API contract.
test/storage.test.ts (3)
2516-2544: good test for clearing legacy worktree storage files.
test/storage.test.ts:2516-2544verifies thatclearAccountsremoves:
- canonical path
- legacy project path
- legacy worktree path
this is important for the migration path and prevents orphaned files.
2832-2861: EAGAIN retry test with WAL cleanup is a good addition.this test at
test/storage.test.ts:2832-2861covers:
- EAGAIN as retryable error (distinct from EPERM/EBUSY)
- WAL file is cleaned up after successful save
- exactly 2 rename attempts
solid coverage for the retry matrix.
3322-3420: comprehensive clearQuotaCache test coverage.the new test block properly covers:
- basic file removal
- tolerance for missing files
- EPERM/EBUSY retry behavior
- exhausted retry returning false
this aligns with the boolean-return API established for clear operations.
test/destructive-actions.test.ts (3)
243-297: path redaction test verifies windows paths are sanitized.the test at
test/destructive-actions.test.ts:243-297correctly verifies that:
- error message doesn't contain user-identifying paths (alice)
- error code is preserved
- fallback message is used
this is important for privacy on windows systems.
96-142: rebase index test covers the critical deletion edge case.
test/destructive-actions.test.ts:96-142verifies that when deleting account at index 0:
- activeIndex shifts from 1 to 0
- activeIndexByFamily values shift correctly (codex: 2→1, gpt-5.x: 1→0)
- remaining accounts are correct
this is the key logic that prevents stale indices after deletion.
77-94: best-effort reset test validates partial failure handling.
test/destructive-actions.test.ts:77-94confirms that whenclearFlaggedAccountsthrows:
resetLocalStatestill returns (doesn't throw)flaggedClearedisfalse- other steps complete normally
this matches the best-effort semantics documented in the PR.
lib/cli.ts (4)
161-171: promptResetTypedConfirm follows established pattern.the new function at
lib/cli.ts:161-171mirrorspromptDeleteAllTypedConfirmstructure:
- creates readline interface
- prompts with DESTRUCTIVE_ACTION_COPY text
- checks exact match ("RESET")
- closes readline in finally
consistent and correct.
278-283: reset-all flow integrated correctly into promptLoginMode.the new case at
lib/cli.ts:278-283handles the TTY menureset-allaction:
- prompts for typed confirmation
- logs cancellation message if not confirmed
- returns
{ mode: "reset" }on successthis follows the same pattern as the
freshcase.
23-24: TERM_PROGRAM check uses case-insensitive comparison.
lib/cli.ts:23-24normalizes TERM_PROGRAM with.trim().toLowerCase()before comparing to "codex". this handles variations like "Codex", " codex ", etc.
49-61: LoginMode type extended with "reset".the type union at
lib/cli.ts:49-61now includes "reset" alongside existing modes. the union is exhaustive for the switch cases inpromptLoginMode.test/cli.test.ts (4)
176-185: reset flow test covers fallback confirmation.
test/cli.test.ts:176-185verifies:
- "reset" input triggers reset flow
- "RESET" typed confirmation proceeds
- returns
{ mode: "reset" }good coverage for the non-TTY fallback path.
203-217: reset cancellation test verifies user-facing message.
test/cli.test.ts:203-217confirms that when typed confirmation doesn't match:
- flow continues to next prompt
- cancellation message is logged:
"\nReset local state cancelled.\n"this matches the UX established for delete-all cancellation.
219-266: TTY reset-all test mocks showAuthMenu correctly.
test/cli.test.ts:219-266sets up:
- stdin/stdout isTTY overrides
- showAuthMenu mock returning
{ type: "reset-all" }- readline mock for "RESET" confirmation
- proper cleanup in finally block
the test isolation is good - vi.doMock/vi.doUnmock pattern prevents leakage.
268-321: TTY reset-all cancellation test verifies correct copy.
test/cli.test.ts:268-321confirms that when TTY reset-all is selected but typed confirmation fails:
- cancellation message
"\nReset local state cancelled.\n"is logged- flow returns to menu (mock returns
{ type: "add" }on second call)this ensures the UX is consistent between fallback and TTY paths.
index.ts (2)
104-109: good consolidation of destructive-action imports.this keeps destructive behavior anchored to the shared safety layer in
lib/destructive-actions.ts:1, which is the right direction for this flow.
3151-3158: good use of centralized delete flow for fresh mode.delegating to
deleteSavedAccountsand reusingDESTRUCTIVE_ACTION_COPYkeeps behavior and user copy consistent withlib/destructive-actions.ts:1.
| it("returns delete-all without an extra confirm prompt", async () => { | ||
| selectMock.mockResolvedValueOnce({ type: "delete-all" }); | ||
|
|
||
| const { showAuthMenu } = await import("../lib/ui/auth-menu.js"); | ||
| const result = await showAuthMenu(createAccounts()); | ||
|
|
||
| expect(result).toEqual({ type: "delete-all" }); | ||
| expect(confirmMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("returns reset-all without an extra confirm prompt", async () => { | ||
| selectMock.mockResolvedValueOnce({ type: "reset-all" }); | ||
|
|
||
| const { showAuthMenu } = await import("../lib/ui/auth-menu.js"); | ||
| const result = await showAuthMenu(createAccounts()); | ||
|
|
||
| expect(result).toEqual({ type: "reset-all" }); | ||
| expect(confirmMock).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
assert that the menu never opens typed confirmation.
test/auth-menu-hotkeys.test.ts:240-258 only checks confirmMock. this file already mocks node:readline/promises at test/auth-menu-hotkeys.test.ts:17-22, so a regression that moves typed confirmation back into lib/ui/auth-menu.ts would still pass. assert searchQuestionMock and searchCloseMock stay untouched for both destructive actions.
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/auth-menu-hotkeys.test.ts` around lines 240 - 258, The tests "returns
delete-all without an extra confirm prompt" and "returns reset-all without an
extra confirm prompt" only assert confirmMock, which misses regressions that
reintroduce typed confirmation via the mocked node:readline/promises; update
both specs to also assert that searchQuestionMock and searchCloseMock were not
called (e.g., expect(searchQuestionMock).not.toHaveBeenCalled();
expect(searchCloseMock).not.toHaveBeenCalled()) after calling showAuthMenu with
createAccounts(), referencing the existing mocks and the showAuthMenu import to
ensure typed-confirmation helpers in lib/ui/auth-menu.ts remain unused for these
destructive actions.
| it("preserves both the flagged-save failure and rollback failure", async () => { | ||
| const flaggedSaveError = Object.assign(new Error("flagged save failed"), { | ||
| code: "EPERM", | ||
| }); | ||
| const rollbackError = Object.assign(new Error("rollback failed"), { | ||
| code: "EPERM", | ||
| }); | ||
| saveAccountsMock | ||
| .mockResolvedValueOnce(undefined) | ||
| .mockRejectedValueOnce(rollbackError); | ||
| saveFlaggedAccountsMock.mockRejectedValueOnce(flaggedSaveError); | ||
| loadFlaggedAccountsMock.mockResolvedValue({ | ||
| version: 1, | ||
| accounts: [ | ||
| { | ||
| refreshToken: "refresh-remove", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| flaggedAt: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| const { deleteAccountAtIndex } = await import( | ||
| "../lib/destructive-actions.js" | ||
| ); | ||
|
|
||
| const storage = { | ||
| version: 3, | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| accounts: [ | ||
| { | ||
| refreshToken: "refresh-keep", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| { | ||
| refreshToken: "refresh-remove", | ||
| addedAt: 2, | ||
| lastUsed: 2, | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| try { | ||
| await deleteAccountAtIndex({ storage, index: 1 }); | ||
| throw new Error("expected deleteAccountAtIndex to throw"); | ||
| } catch (error) { | ||
| expect(error).toBeInstanceOf(AggregateError); | ||
| const aggregateError = error as AggregateError; | ||
| expect(aggregateError.message).toBe( | ||
| "Deleting the account partially failed and rollback also failed.", | ||
| ); | ||
| expect(aggregateError.errors).toEqual([ | ||
| flaggedSaveError, | ||
| rollbackError, | ||
| ]); | ||
| } | ||
| expect(saveAccountsMock).toHaveBeenCalledTimes(2); | ||
| expect(storage.accounts).toHaveLength(2); | ||
| }); |
There was a problem hiding this comment.
test expects original errors in AggregateError but implementation sanitizes them.
at test/destructive-actions.test.ts:353-356, the test expects:
expect(aggregateError.errors).toEqual([
flaggedSaveError,
rollbackError,
]);but looking at lib/destructive-actions.ts:160-170, the implementation wraps errors with asSafeAggregateError, which creates new Error objects when paths are detected. the original errors shouldn't be in the array.
compare with the test at lines 362-431 which correctly expects sanitized error messages. this test at 299-360 may be passing because the mock errors don't contain path separators, so asSafeAggregateError returns them unchanged.
consider adding path characters to the mock errors to verify sanitization:
suggested fix
-const flaggedSaveError = Object.assign(new Error("flagged save failed"), {
+const flaggedSaveError = Object.assign(new Error("flagged save failed at /path/to/file"), {
code: "EPERM",
});
-const rollbackError = Object.assign(new Error("rollback failed"), {
+const rollbackError = Object.assign(new Error("rollback failed at /path/to/backup"), {
code: "EPERM",
});then update the assertion to check for sanitized messages.
🤖 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 299 - 360, The test for
deleteAccountAtIndex currently asserts the original flaggedSaveError and
rollbackError are preserved, but the implementation runs errors through
asSafeAggregateError which may sanitize path-like messages; update the test by
making the mocked errors (flaggedSaveError and rollbackError) include path
characters (e.g. a string with "/" or "\") so sanitization is exercised, and
change the expectation on aggregateError.errors to assert the sanitized
errors/messages produced by asSafeAggregateError (instead of the original Error
objects) while keeping the rest of the test (calls to saveAccountsMock and
storage assertions) unchanged.
| it.each(["EPERM", "EBUSY"] as const)( | ||
| "retries transient %s when clearing the quota cache", | ||
| async (code) => { | ||
| const quotaPath = getQuotaCachePath(); | ||
| await fs.mkdir(dirname(quotaPath), { recursive: true }); | ||
| await fs.writeFile(quotaPath, "{}", "utf-8"); | ||
|
|
||
| const realUnlink = fs.unlink.bind(fs); | ||
| const unlinkSpy = vi | ||
| .spyOn(fs, "unlink") | ||
| .mockImplementation(async (target) => { | ||
| if (target === quotaPath && unlinkSpy.mock.calls.length === 1) { | ||
| const err = new Error("locked") as NodeJS.ErrnoException; | ||
| err.code = code; | ||
| throw err; | ||
| } | ||
| return realUnlink(target); | ||
| }); | ||
|
|
||
| try { | ||
| await expect(clearQuotaCache()).resolves.toBe(true); | ||
| expect(existsSync(quotaPath)).toBe(false); | ||
| expect(unlinkSpy).toHaveBeenCalledTimes(2); | ||
| } finally { | ||
| unlinkSpy.mockRestore(); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
fragile first-call check in retry test.
at test/storage.test.ts:3373, the condition unlinkSpy.mock.calls.length === 1 checks if this is the first call, but this relies on the mock being called exactly once before the check. if the implementation changes to make multiple unlink calls per attempt, this could break unexpectedly.
consider using a counter variable similar to the pattern in other tests:
suggested approach
const realUnlink = fs.unlink.bind(fs);
+let failedOnce = false;
const unlinkSpy = vi
.spyOn(fs, "unlink")
.mockImplementation(async (target) => {
- if (target === quotaPath && unlinkSpy.mock.calls.length === 1) {
+ if (target === quotaPath && !failedOnce) {
+ failedOnce = true;
const err = new Error("locked") as NodeJS.ErrnoException;
err.code = code;
throw err;
}
return realUnlink(target);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.each(["EPERM", "EBUSY"] as const)( | |
| "retries transient %s when clearing the quota cache", | |
| async (code) => { | |
| const quotaPath = getQuotaCachePath(); | |
| await fs.mkdir(dirname(quotaPath), { recursive: true }); | |
| await fs.writeFile(quotaPath, "{}", "utf-8"); | |
| const realUnlink = fs.unlink.bind(fs); | |
| const unlinkSpy = vi | |
| .spyOn(fs, "unlink") | |
| .mockImplementation(async (target) => { | |
| if (target === quotaPath && unlinkSpy.mock.calls.length === 1) { | |
| const err = new Error("locked") as NodeJS.ErrnoException; | |
| err.code = code; | |
| throw err; | |
| } | |
| return realUnlink(target); | |
| }); | |
| try { | |
| await expect(clearQuotaCache()).resolves.toBe(true); | |
| expect(existsSync(quotaPath)).toBe(false); | |
| expect(unlinkSpy).toHaveBeenCalledTimes(2); | |
| } finally { | |
| unlinkSpy.mockRestore(); | |
| } | |
| }, | |
| ); | |
| it.each(["EPERM", "EBUSY"] as const)( | |
| "retries transient %s when clearing the quota cache", | |
| async (code) => { | |
| const quotaPath = getQuotaCachePath(); | |
| await fs.mkdir(dirname(quotaPath), { recursive: true }); | |
| await fs.writeFile(quotaPath, "{}", "utf-8"); | |
| const realUnlink = fs.unlink.bind(fs); | |
| let failedOnce = false; | |
| const unlinkSpy = vi | |
| .spyOn(fs, "unlink") | |
| .mockImplementation(async (target) => { | |
| if (target === quotaPath && !failedOnce) { | |
| failedOnce = true; | |
| const err = new Error("locked") as NodeJS.ErrnoException; | |
| err.code = code; | |
| throw err; | |
| } | |
| return realUnlink(target); | |
| }); | |
| try { | |
| await expect(clearQuotaCache()).resolves.toBe(true); | |
| expect(existsSync(quotaPath)).toBe(false); | |
| expect(unlinkSpy).toHaveBeenCalledTimes(2); | |
| } finally { | |
| unlinkSpy.mockRestore(); | |
| } | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 3362 - 3389, The test's mock relies on
unlinkSpy.mock.calls.length to detect the first call which is brittle; change
the mock to use an independent counter variable (e.g., callCount) captured in
the test scope and increment it inside the mockImplementation, then throw the
transient error when callCount === 1; update the test around unlinkSpy and its
mock (the mockImplementation for fs.unlink used in the "retries transient %s
when clearing the quota cache" case that exercises clearQuotaCache and
getQuotaCachePath) and restore behavior the same otherwise.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/storage.ts (2)
2273-2323:⚠️ Potential issue | 🟠 Majorhonor the reset marker before legacy flagged migration.
when
unlinkWithRetry(legacyPath)exhausts retries here, this branch intentionally keepsmarkerPath, butloadFlaggedAccounts()only consults that marker after a successful primary read inlib/storage.ts:2164-2218. if the primary is gone and the legacy file survives,lib/storage.ts:2188-2210migratesopenai-codex-blocked-accounts.jsonright back on the next load.test/storage-flagged.test.ts:395-423only proves the happy path, so this windows failure mode is still untested.possible fix
export async function loadFlaggedAccounts(): Promise<FlaggedAccountStorageV1> { const path = getFlaggedAccountsPath(); const resetMarkerPath = getIntentionalResetMarkerPath(path); const empty: FlaggedAccountStorageV1 = { version: 1, accounts: [] }; + if (existsSync(resetMarkerPath)) { + return empty; + } try { const content = await fs.readFile(path, "utf-8"); const data = JSON.parse(content) as unknown; const loaded = normalizeFlaggedStorage(data); - if (existsSync(resetMarkerPath)) { - return empty; - } return loaded;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 2273 - 2323, The current bug is that clearFlaggedAccounts may leave the reset marker when deleting legacyPath fails, but loadFlaggedAccounts only checks the marker after a successful primary read which allows a surviving legacy file to be re-migrated; fix by making loadFlaggedAccounts consult the intentional reset marker (getIntentionalResetMarkerPath(path) / markerPath) before performing any legacy migration or fallback reads (i.e., check marker existence early in loadFlaggedAccounts and abort/mask migration if present), update references to unlinkWithRetry/legacyPath/path within that logic, and add a vitest in test/storage-flagged.test.ts that simulates primary missing + legacy present + marker present to assert no migration occurs.
2004-2049:⚠️ Potential issue | 🟠 Majorhonor the reset marker before legacy account migration.
if one of the
legacyPathsdeletes fails here,clearAccounts()keeps the reset marker and returnsfalse, butloadAccountsInternal()still runsmigrateLegacyProjectStorageIfNeeded()first inlib/storage.ts:856-952andlib/storage.ts:1618-1627. in project-scoped mode that migration persists the surviving legacy file throughsaveAccountsUnlocked()and removes the marker atlib/storage.ts:1857-1866, so the next load can silently recreate the account pool you just tried to delete. please skip legacy migration whenever the reset marker exists, and add a vitest regression intest/storage.test.tsthat forces one legacy unlink to stay locked.possible fix
async function migrateLegacyProjectStorageIfNeeded( persist: (storage: AccountStorageV3) => Promise<void> = saveAccounts, ): Promise<AccountStorageV3 | null> { if (!currentStoragePath) { return null; } + if (existsSync(getIntentionalResetMarkerPath(currentStoragePath))) { + return null; + } const candidatePaths = [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 2004 - 2049, clearAccounts writes an intentional reset marker then tries to remove legacyPaths, but if a legacy unlink fails the marker can persist while loadAccountsInternal still runs migrateLegacyProjectStorageIfNeeded and resurrects accounts; change migrateLegacyProjectStorageIfNeeded (called from loadAccountsInternal) to first check for the intentional reset marker (use getIntentionalResetMarkerPath(getStoragePath())) and early-return / skip any legacy migration when the marker exists, and keep clearAccounts behavior so failures still return false; add a vitest regression that simulates one legacy unlink staying locked (causing unlink to fail) and asserts that migration is skipped, the reset marker remains, and no account pool is recreated.
♻️ Duplicate comments (2)
test/codex-manager-cli.test.ts (1)
477-490:⚠️ Potential issue | 🟠 Majorcover the partial-cleanup warning branches for delete and reset.
test/codex-manager-cli.test.ts:477-490 hard-wires both destructive actions to all-success results, and the new flows at test/codex-manager-cli.test.ts:3607-3669 never override that.
lib/destructive-actions.ts:193-215exposesaccountsCleared,flaggedCleared, andquotaCacheClearedspecifically so the cli can surface partial-cleanup warnings. without onefalsecase fordeleteSavedAccounts()and one forresetLocalState(), the windowseperm/ebusywarning copy can still regress unnoticed.As per coding guidelines
test/**: demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.Also applies to: 3607-3669
🤖 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 477 - 490, The tests currently stub deleteSavedAccountsMock and resetLocalStateMock to always return all-true results, so the CLI partial-cleanup warning branches in lib/destructive-actions.ts (which check accountsCleared, flaggedCleared, quotaCacheCleared) are not exercised; update the test cases around deleteSavedAccountsMock, resetLocalStateMock and deleteAccountAtIndexMock so that at least one test simulates deleteSavedAccounts() returning one false (e.g., flaggedCleared:false or quotaCacheCleared:false) and another simulates resetLocalState() returning one false, to force the partial-cleanup warning paths to run (keep using the same mocks deleteSavedAccountsMock, resetLocalStateMock, deleteAccountAtIndexMock and adjust mockResolvedValue returns accordingly).test/storage.test.ts (1)
3456-3483:⚠️ Potential issue | 🟡 Minormake the first retry failure independent of total unlink calls.
test/storage.test.ts:3467 still keys the injected failure off
unlinkSpy.mock.calls.length === 1. that makes the case brittle: any extrafs.unlinkbefore the quota-path attempt will stop reproducing the retry path.suggested fix
const realUnlink = fs.unlink.bind(fs); + let failedOnce = false; const unlinkSpy = vi .spyOn(fs, "unlink") .mockImplementation(async (target) => { - if (target === quotaPath && unlinkSpy.mock.calls.length === 1) { + if (target === quotaPath && !failedOnce) { + failedOnce = true; const err = new Error("locked") as NodeJS.ErrnoException; err.code = code; throw err; }As per coding guidelines
test/**: tests must stay deterministic and use vitest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 3456 - 3483, The injected transient failure in the test is keyed to unlinkSpy.mock.calls.length which makes the failure depend on total unlink calls; change the mock in the test for unlink (the unlinkSpy mock around fs.unlink used in the "retries transient %s when clearing the quota cache" case) to detect the specific target path (quotaPath) and a per-target first-call state (e.g., a local boolean like `failedOnceForQuotaPath`) rather than global mock.calls length, so the mock throws the ErrnoException for the first attempt to unlink quotaPath and succeeds thereafter, keeping the test deterministic for clearQuotaCache and expectations about unlinkSpy call behavior.
🤖 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 3817-3841: After deleting an account and merging deleted.storage
into storage in the deleteAccountIndex branch, ensure codex is resynced: call
autoSyncActiveAccountToCodex() (or explicitly clear codex credentials if the
deleted index was the active account) immediately after Object.assign(storage,
deleted.storage) and before logging/return, so codex won't reference removed
credentials; update lib/codex-manager.ts around the deleteAccountAtIndex
handling to perform this resync/clear and ensure the call is awaited. Add a
vitest regression in test/codex-manager-cli.test.ts that deletes the currently
active account and asserts codex state is cleared or updated appropriately. When
implementing, follow lib/** guidelines: avoid logging sensitive tokens/emails,
handle concurrent destructiveActionInFlight correctly, and ensure any new queues
or retries used by autoSyncActiveAccountToCodex tolerate EBUSY/429 conditions.
In `@test/codex-manager-cli.test.ts`:
- Around line 3671-3753: The test currently only delays
fetchCodexQuotaSnapshotMock but never delays the quota write; to reproduce the
race, make saveQuotaCacheMock return a deferred promise (like fetchDeferred) so
the save stays in-flight when resetLocalStateMock is invoked, resolve that
deferred only after observing reset started, and then assert that the save's
invocation completed before clearQuotaCache() / resetLocalStateMock clears the
cache; update the test setup to create a saveDeferred, have
saveQuotaCacheMock.mockImplementation(async () => saveDeferred.promise),
coordinate fetchDeferred and saveDeferred ordering around runCodexMultiAuthCli
and then assert invocationCallOrder between saveQuotaCacheMock and
resetLocalStateMock (and/or that clearQuotaCache was not overlapped).
In `@test/storage-flagged.test.ts`:
- Around line 235-322: The tests exercise unlinkWithRetry's EPERM/EBUSY retry
branch but miss the EAGAIN branch; update both it.each matrices in
test/storage-flagged.test.ts that currently use ["EPERM","EBUSY"] to include
"EAGAIN" (i.e. ["EPERM","EBUSY","EAGAIN"]) so clearFlaggedAccounts() triggers
unlinkWithRetry()'s EAGAIN retry logic; keep the same mockImplementation
behavior and assertions (unlinkSpy, existsSync, call counts) so the tests remain
deterministic and cover the additional branch.
In `@test/storage.test.ts`:
- Around line 2563-2591: The new test case for clearAccounts is missing a
Windows-style linked-worktree variant, so add a regression that uses the Windows
`.git` pointer form (like the other windows fixtures around lines 2393-2450 and
2698-2733) to exercise path normalization; specifically, create or reuse a
fixture that produces a win32 linked worktree (e.g., prepareWorktreeFixture or a
new prepareWindowsLinkedWorktreeFixture), call setStoragePath(worktreeRepo) and
build/write the same canonical, legacy project and legacy worktree files, then
call clearAccounts() and assert all three files were removed; ensure you
reference the same helper functions used elsewhere (prepareWorktreeFixture,
setStoragePath, getProjectStorageKey, getConfigDir, clearAccounts) so the test
hits the Windows pointer code path and catches the regression.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 2273-2323: The current bug is that clearFlaggedAccounts may leave
the reset marker when deleting legacyPath fails, but loadFlaggedAccounts only
checks the marker after a successful primary read which allows a surviving
legacy file to be re-migrated; fix by making loadFlaggedAccounts consult the
intentional reset marker (getIntentionalResetMarkerPath(path) / markerPath)
before performing any legacy migration or fallback reads (i.e., check marker
existence early in loadFlaggedAccounts and abort/mask migration if present),
update references to unlinkWithRetry/legacyPath/path within that logic, and add
a vitest in test/storage-flagged.test.ts that simulates primary missing + legacy
present + marker present to assert no migration occurs.
- Around line 2004-2049: clearAccounts writes an intentional reset marker then
tries to remove legacyPaths, but if a legacy unlink fails the marker can persist
while loadAccountsInternal still runs migrateLegacyProjectStorageIfNeeded and
resurrects accounts; change migrateLegacyProjectStorageIfNeeded (called from
loadAccountsInternal) to first check for the intentional reset marker (use
getIntentionalResetMarkerPath(getStoragePath())) and early-return / skip any
legacy migration when the marker exists, and keep clearAccounts behavior so
failures still return false; add a vitest regression that simulates one legacy
unlink staying locked (causing unlink to fail) and asserts that migration is
skipped, the reset marker remains, and no account pool is recreated.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 477-490: The tests currently stub deleteSavedAccountsMock and
resetLocalStateMock to always return all-true results, so the CLI
partial-cleanup warning branches in lib/destructive-actions.ts (which check
accountsCleared, flaggedCleared, quotaCacheCleared) are not exercised; update
the test cases around deleteSavedAccountsMock, resetLocalStateMock and
deleteAccountAtIndexMock so that at least one test simulates
deleteSavedAccounts() returning one false (e.g., flaggedCleared:false or
quotaCacheCleared:false) and another simulates resetLocalState() returning one
false, to force the partial-cleanup warning paths to run (keep using the same
mocks deleteSavedAccountsMock, resetLocalStateMock, deleteAccountAtIndexMock and
adjust mockResolvedValue returns accordingly).
In `@test/storage.test.ts`:
- Around line 3456-3483: The injected transient failure in the test is keyed to
unlinkSpy.mock.calls.length which makes the failure depend on total unlink
calls; change the mock in the test for unlink (the unlinkSpy mock around
fs.unlink used in the "retries transient %s when clearing the quota cache" case)
to detect the specific target path (quotaPath) and a per-target first-call state
(e.g., a local boolean like `failedOnceForQuotaPath`) rather than global
mock.calls length, so the mock throws the ErrnoException for the first attempt
to unlink quotaPath and succeeds thereafter, keeping the test deterministic for
clearQuotaCache and expectations about unlinkSpy call behavior.
🪄 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: f3edf33b-d27f-4bd5-bb93-cdfc8e9afc5e
📒 Files selected for processing (5)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage-flagged.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-flagged.test.tstest/storage.test.tstest/codex-manager-cli.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.tslib/codex-manager.ts
🔇 Additional comments (1)
lib/storage.ts (1)
182-200: good windows cleanup hardening.centralizing retryable unlinks and reusing them for the post-rename reset-marker and wal cleanup is the right fix for the stale-artifact window on windows. the transient lock handling shape is now at least partially exercised by
test/storage-flagged.test.ts:235-322.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: 1857-1869
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 3970-3993: The delete-all branch doesn't await
pendingMenuQuotaRefresh before calling deleteSavedAccounts, risking a
quota-cache race; update the destructive "fresh && deleteAll" path (inside the
runActionPanel callback where deleteSavedAccounts() is invoked) to first await
pendingMenuQuotaRefresh (same as the reset path does) before calling
deleteSavedAccounts(), keeping the existing destructiveActionInFlight flag and
try/finally behavior intact.
In `@lib/quota-cache.ts`:
- Around line 41-50: The logs for filesystem errors in quota cache functions are
currently using raw error.message and may leak absolute paths; replace those
usages so all filesystem errors are sanitized through the existing helper
formatSafeQuotaCacheClearError(error). Specifically, update the error logging in
the functions that call fs.readFile/fs.rename/clearQuotaCache (eg. the
clearQuotaCache function and the load/save quota cache blocks that currently
append error.message) to call formatSafeQuotaCacheClearError(error) instead of
using error.message directly, and ensure any log lines or thrown messages use
that sanitized string; update related tests to simulate real path-bearing fs
errors so the sanitizer is exercised.
- Around line 34-39: Add "EAGAIN" to the RETRYABLE_FS_CODES set and ensure
isRetryableFsError recognizes it (update the RETRYABLE_FS_CODES constant used by
isRetryableFsError), then update the quota-cache tests that cover the
rename/unlink retry loops (the tests in test/quota-cache.test.ts that currently
assert behavior for EBUSY/EPERM) to also simulate and assert the same retry
behavior for EAGAIN so the quota-cache rename/unlink logic mirrors
lib/storage.ts's retry handling.
🪄 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: 2a5a1651-464b-46d9-8442-ae6964474437
📒 Files selected for processing (7)
index.tslib/codex-manager.tslib/destructive-actions.tslib/quota-cache.tstest/codex-manager-cli.test.tstest/destructive-actions.test.tstest/quota-cache.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/quota-cache.tslib/codex-manager.tslib/destructive-actions.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-cli.test.tstest/quota-cache.test.tstest/destructive-actions.test.ts
🔇 Additional comments (11)
test/codex-manager-cli.test.ts (2)
3722-3750: add the partial-cleanup warning regressions here.
test/codex-manager-cli.test.ts:3722-3784(Lines 3722-3784) only asserts the all-success copy. ifdeleteSavedAccountsMockorresetLocalStateMockreturns afalsecleanup flag, the windows best-effort branch still has no assertion and the warning copy can regress unnoticed.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.Also applies to: 3754-3784
3786-3868: this still does not reproduce the quota-cache write/reset race.
test/codex-manager-cli.test.ts:3786-3868(Lines 3786-3868) blocksfetchCodexQuotaSnapshotMock, butsaveQuotaCacheMockresolves immediately. that only proves reset waits for the fetch. it does not prove reset waits for the cache write that can overlapclearQuotaCache(). keepsaveQuotaCacheMockpending on a deferred promise and release it only after assertingresetLocalStateMockhas not started.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-manager.ts (4)
3820-3843: lgtm - guard and resync look correct.the
destructiveActionInFlightguard atlib/codex-manager.ts:3820-3824with try/finally at 3825-3843 serializes the delete flow.autoSyncActiveAccountToCodex()at line 3832 ensures codex points at valid credentials after removal, which addresses the earlier resync concern.
3995-4024: lgtm - reset flow correctly awaits quota refresh before clearing state.the await at
lib/codex-manager.ts:4006-4008ensures the quota refresh completes beforeresetLocalStateclears files, avoiding mid-write corruption. the guard pattern and finally block are correct.
33-38: lgtm - imports align with usage.
95-96: module-level guard prevents concurrent actions within single cli invocation; add regression test for guard behavior.
destructiveActionInFlightatlib/codex-manager.ts:95serializes destructive actions within a single process, but concurrent cli processes can still race (documented gap in pr objectives).however, two real issues need addressing:
missing regression test for concurrent guard: coding guidelines require "verify every change cites affected tests (vitest)."
test/destructive-actions.test.tsandtest/codex-manager-cli.test.tstest individual function behavior but do not test thatdestructiveActionInFlightactually blocks a second concurrent call. add a test that attempts concurrent invocation and confirms the second call is rejected.inconsistent pendingMenuQuotaRefresh handling: reset mode at
lib/codex-manager.ts:4006-4008awaitspendingMenuQuotaRefreshbefore callingresetLocalState(), but fresh-mode deleteAll atlib/codex-manager.ts:3970-3993does not await it before callingdeleteSavedAccounts(). either both should await (safer) or both should skip (if quota refresh is not critical for account deletion). align these paths.index.ts (5)
104-109: lgtm — imports align with new destructive-action api.imports
DESTRUCTIVE_ACTION_COPY,deleteAccountAtIndex,deleteSavedAccounts, andresetLocalStatefromlib/destructive-actions.js:1. matches the new api surface documented in the pr summary.
3108-3124: lgtm — manage path now uses deleteAccountAtIndex with proper state sync.the call to
deleteAccountAtIndexatindex.ts:3108returns{ storage, flagged, removedAccount, removedFlaggedCount }. applying viaObject.assignat lines 3113-3114 updates the in-memory working copies correctly, and the log message now surfaces the removed flagged count per pr objectives.
3150-3161: lgtm — fresh path uses deleteSavedAccounts with completion messaging.
deleteSavedAccounts()atindex.ts:3153returns{ accountsCleared }, and the ternary at lines 3157-3159 correctly chooses the standard copy or the warning message.
3166-3180: lgtm — reset handler uses resetLocalState and surfaces partial-failure warnings.the new
menuResult.mode === "reset"branch atindex.ts:3166correctly callsresetLocalState()fromlib/destructive-actions.js:1and checks all three flags (accountsCleared,flaggedCleared,quotaCacheCleared) before declaring full success.the missing
destructiveActionInFlightguard and regression tests for concurrent dispatch were already flagged in a prior review — see past comment on lines 3166-3180.
3943-3967: codex-remove tool still mutates storage inline — same gap flagged in prior review.the
codex-removetool atindex.ts:3943splices the account array directly and manually adjusts indices at lines 3949-3966. this bypasses the rollback, in-flight guard, and flagged-sync guarantees provided bydeleteAccountAtIndexinlib/destructive-actions.ts:1.this is the same issue noted in the past review comment on lines 3108-3123: the advanced tool path needs migration to
deleteAccountAtIndex.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/codex-manager-cli.test.ts (1)
3391-3465:⚠️ Potential issue | 🟠 Majoradd the windows delete-failure regression for manage mode.
test/codex-manager-cli.test.ts:3391-3551only covers successful deletes. it still never drivesdeleteAccountAtIndexMockthrough anEPERMorEBUSYrejection, so the manage-path crash this pr calls out can regress without a failing test. add one deterministic case that rejects the helper and asserts the session stays alive and surfaces the user-facing warning instead of bubbling the error.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.Also applies to: 3467-3551
♻️ Duplicate comments (2)
test/storage.test.ts (1)
3496-3506:⚠️ Potential issue | 🟡 Minorstop keying the first transient failure off spy call count.
test/storage.test.ts:3496-3506still usesunlinkSpy.mock.calls.length === 1inside the mock body. any extra unlink added toclearQuotaCache()will break this regression for the wrong reason. use the local boolean/counter pattern already used by the other retry tests in this file.diff
const realUnlink = fs.unlink.bind(fs); + let failedOnce = false; const unlinkSpy = vi .spyOn(fs, "unlink") .mockImplementation(async (target) => { - if (target === quotaPath && unlinkSpy.mock.calls.length === 1) { + if (target === quotaPath && !failedOnce) { + failedOnce = true; const err = new Error("locked") as NodeJS.ErrnoException; err.code = code; throw err; } return realUnlink(target); });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.test.ts` around lines 3496 - 3506, The mock for fs.unlink should not rely on unlinkSpy.mock.calls.length to trigger the transient failure; replace that check with a local boolean or counter captured in the test (e.g., let firstAttempt = true) so the mock for unlink (used in the clearQuotaCache() test, referencing quotaPath and unlinkSpy) throws the locked error only on the intended first invocation and then delegates to realUnlink thereafter; update the mockImplementation to use this local flag/counter pattern (as used in the other retry tests) instead of reading unlinkSpy.mock.calls.length.test/codex-manager-cli.test.ts (1)
3722-3752:⚠️ Potential issue | 🟠 Majorcover the partial-cleanup warning branches, not just the completed copy.
test/codex-manager-cli.test.ts:3722-3752andtest/codex-manager-cli.test.ts:3847-3877only assert the all-success messages. the windows partial-cleanup branches inlib/codex-manager.ts:3971andlib/codex-manager.ts:4000still have no regression, so a helper result with a failed cleanup flag can slip through without failing this suite. add one case per command that returns a partial cleanup result and asserts the warning text.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.Also applies to: 3847-3877
🤖 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 3722 - 3752, Add deterministic Vitest cases that exercise the partial-cleanup branches by having the deleteSavedAccountsMock (or whichever helper returns cleanup results) resolve with an object indicating partialCleanup: true (or the existing failed-cleanup flag shape used by lib/codex-manager.ts) for both commands exercised by runCodexMultiAuthCli (the "auth login" case at 3722-3752 and the other case at 3847-3877). Specifically, add one test per command that sets loadAccountsMock and promptLoginModeMock as in the existing tests but make deleteSavedAccountsMock return the partial-cleanup result, then assert the CLI exit code and that console.log was called with the exact warning message emitted by the partial-cleanup branches in lib/codex-manager.ts (the warning text around the code handling failed cleanup at ~3971 and ~4000); keep tests deterministic by using vi.spyOn/vi.mockResolvedValue and restore spies.
🤖 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 3820-3843: The delete-account flow can throw and bubble out of
runActionPanel causing the session to exit; wrap the await deleteAccountAtIndex
call (and surrounding post-delete logic that accesses deleted.*) in a try/catch
so failures (e.g. EPERM/EAGAIN, sync failures) are handled locally: set
destructiveActionInFlight back to false in finally (keep that), log a
non-sensitive error via console.error (avoid leaking tokens/emails), and return
to the menu instead of rethrowing. Ensure autoSyncActiveAccountToCodex is only
awaited if deleteAccountAtIndex resolved, and add a vitest regression in
test/codex-manager-cli.test.ts that mocks deleteAccountAtIndex to reject and
asserts the menu/session continues (mock EBUSY/EPERM style errors); reference
destructiveActionInFlight, deleteAccountAtIndex, autoSyncActiveAccountToCodex,
and runActionPanel when updating code and tests.
In `@test/codex-manager-cli.test.ts`:
- Around line 3553-3665: Add a deterministic mixed-action test that proves the
destructive-action guard is global by starting one run that triggers
resetLocalStateMock and holds it open (use a deferred) while a second concurrent
run attempts a delete (use deleteAccountAtIndexMock or deleteSavedAccountsMock);
mock promptLoginModeMock to return the appropriate modes for each run, resolve a
deferred when the second menu is attempted, assert the skip message ("Another
destructive action is already running. Wait for it to finish.") was logged and
that the delete mock was not called while reset was in progress, then release
the reset deferred and assert both runs exit normally; reference and reuse the
existing helpers and symbols from the file like runCodexMultiAuthCli,
resetLocalStateMock, deleteAccountAtIndexMock/deleteSavedAccountsMock,
promptLoginModeMock and the skip message to locate where to add this test.
In `@test/storage.test.ts`:
- Around line 2563-2591: Add explicit non-alias checks to ensure the canonical
storage file and the two legacy paths are distinct before seeding the fixture:
after computing canonicalPath, legacyProjectPath and legacyWorktreePath (in the
test that calls prepareWorktreeFixture, setStoragePath, getStoragePath,
getConfigDir and getProjectStorageKey) assert that canonicalPath is not equal to
legacyProjectPath and not equal to legacyWorktreePath (e.g.
expect(canonicalPath).not.toBe(legacyProjectPath) and
expect(canonicalPath).not.toBe(legacyWorktreePath)) immediately before creating
directories and writing the files, so the test fails if normalization regresses
and the same file would be written multiple times.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 3722-3752: Add deterministic Vitest cases that exercise the
partial-cleanup branches by having the deleteSavedAccountsMock (or whichever
helper returns cleanup results) resolve with an object indicating
partialCleanup: true (or the existing failed-cleanup flag shape used by
lib/codex-manager.ts) for both commands exercised by runCodexMultiAuthCli (the
"auth login" case at 3722-3752 and the other case at 3847-3877). Specifically,
add one test per command that sets loadAccountsMock and promptLoginModeMock as
in the existing tests but make deleteSavedAccountsMock return the
partial-cleanup result, then assert the CLI exit code and that console.log was
called with the exact warning message emitted by the partial-cleanup branches in
lib/codex-manager.ts (the warning text around the code handling failed cleanup
at ~3971 and ~4000); keep tests deterministic by using
vi.spyOn/vi.mockResolvedValue and restore spies.
In `@test/storage.test.ts`:
- Around line 3496-3506: The mock for fs.unlink should not rely on
unlinkSpy.mock.calls.length to trigger the transient failure; replace that check
with a local boolean or counter captured in the test (e.g., let firstAttempt =
true) so the mock for unlink (used in the clearQuotaCache() test, referencing
quotaPath and unlinkSpy) throws the locked error only on the intended first
invocation and then delegates to realUnlink thereafter; update the
mockImplementation to use this local flag/counter pattern (as used in the other
retry tests) instead of reading unlinkSpy.mock.calls.length.
🪄 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: ba40e3e1-e49b-4091-a88e-ada914e7e53d
📒 Files selected for processing (7)
lib/codex-manager.tslib/quota-cache.tslib/storage.tstest/codex-manager-cli.test.tstest/quota-cache.test.tstest/storage-flagged.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/quota-cache.tslib/storage.tslib/codex-manager.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/quota-cache.test.tstest/storage-flagged.test.tstest/codex-manager-cli.test.tstest/storage.test.ts
🔇 Additional comments (4)
test/storage-flagged.test.ts (1)
235-322: good retry matrix coverage for windows fs races.
test/storage-flagged.test.ts:235andtest/storage-flagged.test.ts:279now cover EPERM/EBUSY/EAGAIN on both retry-success and retry-exhaustion paths with stable assertions.lib/storage.ts (1)
182-200: retry + boolean cleanup contract looks solid.
lib/storage.ts:line 182,lib/storage.ts:line 2005, andlib/storage.ts:line 2280consistently apply retryable unlink behavior and return explicit success flags for destructive cleanup paths.Also applies to: 2005-2050, 2280-2331
lib/quota-cache.ts (1)
33-48: good hardening on retry and log sanitization.
lib/quota-cache.ts:line 34andlib/quota-cache.ts:line 286now align transient fs handling with windows lock behavior while keeping warning output path-safe.Also applies to: 286-310
test/quota-cache.test.ts (1)
83-224: nice regression expansion for windows fs behavior.
test/quota-cache.test.ts:line 108,test/quota-cache.test.ts:line 142, andtest/quota-cache.test.ts:line 186add deterministic vitest coverage for transient retry and path-redacted warnings, including EAGAIN.Also applies to: 271-311
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
lib/codex-manager.ts (2)
3842-3845:⚠️ Potential issue | 🟠 Majordon't print raw delete exceptions to the menu.
lib/codex-manager.ts:3842-3845interpolateserror.messagedirectly into output. that bypasses the redaction work in this pr: any unsanitized storage/quota error will leak local paths here. log a generic or code-only failure instead, and add a vitest regression intest/codex-manager-cli.test.tsthat injects a path-bearing delete error and asserts the path never appears.proposed fix
} catch (error) { - console.log( - `Failed to delete account: ${error instanceof Error ? error.message : String(error)}`, - ); + const code = (error as NodeJS.ErrnoException | undefined)?.code; + console.error( + code + ? `Failed to delete account (${code}). Please retry.` + : "Failed to delete account. Please retry.", + ); }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 3842 - 3845, The catch block that currently logs the raw delete error in lib/codex-manager.ts (the catch handling account deletion around the deleteAccount/deleteUser flow) must stop interpolating error.message into console output; replace the raw output with a generic failure message or an error code/token (e.g., "Failed to delete account: ERROR_DELETE_FAILED") and log the full error only to a secure logger that redacts paths, not to the menu/console. Update the delete-account catch handling in the function where the current console.log(`Failed to delete account: ${...}`) appears to emit a non-sensitive message, and add a vitest regression in test/codex-manager-cli.test.ts that injects a thrown Error containing a filesystem path (simulate delete throwing EBUSY/path) and asserts the rendered menu/console output does NOT contain that path while still signaling failure.
3832-3840:⚠️ Potential issue | 🟠 Majordon't report delete success when codex resync failed.
lib/codex-manager.ts:3832-3840ignores the boolean returned byautoSyncActiveAccountToCodex(). when the deleted row was current and the follow-up sync fails — including the empty-pool early return atlib/codex-manager.ts:4191-4194— this branch still prints a success message and leaves codex cli pointed at removed credentials. handle thefalseresult explicitly here, and add a vitest regression intest/codex-manager-cli.test.tsfor deleting the final active account.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 3832 - 3840, The code prints a success message regardless of whether autoSyncActiveAccountToCodex() succeeded; change the post-delete flow in the block handling deleted (references: deleted, deleted.removedAccount, deleted.removedFlaggedCount) to await autoSyncActiveAccountToCodex(), check its boolean result, and only log the "Deleted ..." success via console.log when it returns true; if it returns false, log a non-sensitive error message (do not include tokens/emails) and ensure the Codex CLI is not assumed to point at removed credentials. Additionally add a vitest regression in test/codex-manager-cli.test.ts that deletes the final active account and asserts that the sync failure prevents a success message and leaves active account selection unchanged.test/storage.test.ts (1)
3500-3510:⚠️ Potential issue | 🟡 Minormake the transient quota-cache retry mock deterministic.
test/storage.test.ts:3500-3510usesunlinkSpy.mock.calls.length === 1to decide when to inject the first failure. that couples the regression to the current internal call ordering inclearQuotaCache(). the same file already switched the storage-clear tests to a stablefailedOnceflag attest/storage.test.ts:1803-1815; mirror that pattern here so this test does not break if the implementation adds another unlink before the quota file.proposed fix
const realUnlink = fs.unlink.bind(fs); + let failedOnce = false; const unlinkSpy = vi .spyOn(fs, "unlink") .mockImplementation(async (target) => { - if (target === quotaPath && unlinkSpy.mock.calls.length === 1) { + if (target === quotaPath && !failedOnce) { + failedOnce = true; const err = new Error("locked") as NodeJS.ErrnoException; err.code = code; throw err; }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.test.ts` around lines 3500 - 3510, The test injection that throws on the first unlink call is brittle because it checks unlinkSpy.mock.calls.length; change this to a deterministic closure flag like failedOnce: declare let failedOnce = false before creating unlinkSpy, then in the mock implementation use if (target === quotaPath && !failedOnce) { failedOnce = true; const err = new Error("locked") as NodeJS.ErrnoException; err.code = code; throw err; } else return realUnlink(target); — update the mock that references unlinkSpy, fs.unlink, quotaPath and ensure this mirrors the earlier stable pattern used around clearQuotaCache() tests.test/codex-manager-cli.test.ts (1)
3772-3801:⚠️ Potential issue | 🟠 Majorthe windows partial-cleanup warning path is still untested.
test/codex-manager-cli.test.ts:3772-3801andtest/codex-manager-cli.test.ts:3897-3927only pin the completed-copy path. they never makedeleteSavedAccountsMockorresetLocalStateMockreport a cleanup miss, so theebusy/epermwarning branch can still regress green. add one deterministic delete-saved-accounts case and one reset case that assert the warning copy instead of the success copy.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.Also applies to: 3897-3927
🤖 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 3772 - 3801, Add two deterministic test cases in test/codex-manager-cli.test.ts that exercise the "partial-cleanup" warning branch by making the mocked cleanup helpers indicate a cleanup miss: one where deleteSavedAccountsMock resolves to a value that signals failure (e.g., false or a non-success result) while resetLocalStateMock remains uncalled, and one where resetLocalStateMock resolves to indicate failure while deleteSavedAccountsMock succeeds. In each case, call runCodexMultiAuthCli(["auth", "login"]) with promptLoginModeMock returning the same flows used in the existing tests, then assert exitCode === 0 and that console.log was called with the windows partial-cleanup warning message instead of the success message; ensure you use vitest mocks (vi.spyOn/ mockResolvedValueOnce) so tests remain deterministic and do not touch real secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 3113-3116: The Object.assign calls that copy deleted.storage into
workingStorage and deleted.flagged into flaggedStorage (followed by
invalidateAccountManagerCache()) are dead code because deleteAccountAtIndex
performs a two-phase write and the loop immediately continues and reloads
workingStorage/flaggedStorage; either remove those Object.assign lines and the
subsequent invalidateAccountManagerCache() call, or keep them but add a clear
comment in the block (referencing deleted, workingStorage, flaggedStorage,
invalidateAccountManagerCache, deleteAccountAtIndex) explaining that these
assignments are intentionally retained to support a future optimization to skip
the reload on the next iteration; implement one of those two options.
In `@test/cli.test.ts`:
- Around line 227-273: The test mutates stdin.isTTY/stdout.isTTY by replacing
only boolean values which can leave altered property descriptors behind; change
the setup/teardown in the "returns reset for TTY reset-all confirmation" case
(and the other similar blocks) to save the full property descriptors via
Object.getOwnPropertyDescriptor(stdin, "isTTY") and
Object.getOwnPropertyDescriptor(stdout, "isTTY") before mocking, then restore
those saved descriptors with Object.defineProperty in the finally block (instead
of writing back a simple value). Update the block that manipulates isTTY (and
corresponding blocks around promptLoginMode, mockRl.question, and
showAuthMenuMock) to use descriptor snapshot/restore so inherited or
non-writable descriptor shapes are preserved.
In `@test/codex-manager-cli.test.ts`:
- Around line 3553-3600: The test currently queues a second prompt response but
doesn't verify it was consumed; add an assertion that the menu prompt was
invoked again to prove the loop continued after delete failure (e.g., assert
promptLoginModeMock was called twice or that the second "cancel" branch was
taken). Update the test near the end (after the existing expectations) to
include something like expect(promptLoginModeMock).toHaveBeenCalledTimes(2) or
an assertion that the cancel result was used, referencing the
promptLoginModeMock and runCodexMultiAuthCli symbols.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 3842-3845: The catch block that currently logs the raw delete
error in lib/codex-manager.ts (the catch handling account deletion around the
deleteAccount/deleteUser flow) must stop interpolating error.message into
console output; replace the raw output with a generic failure message or an
error code/token (e.g., "Failed to delete account: ERROR_DELETE_FAILED") and log
the full error only to a secure logger that redacts paths, not to the
menu/console. Update the delete-account catch handling in the function where the
current console.log(`Failed to delete account: ${...}`) appears to emit a
non-sensitive message, and add a vitest regression in
test/codex-manager-cli.test.ts that injects a thrown Error containing a
filesystem path (simulate delete throwing EBUSY/path) and asserts the rendered
menu/console output does NOT contain that path while still signaling failure.
- Around line 3832-3840: The code prints a success message regardless of whether
autoSyncActiveAccountToCodex() succeeded; change the post-delete flow in the
block handling deleted (references: deleted, deleted.removedAccount,
deleted.removedFlaggedCount) to await autoSyncActiveAccountToCodex(), check its
boolean result, and only log the "Deleted ..." success via console.log when it
returns true; if it returns false, log a non-sensitive error message (do not
include tokens/emails) and ensure the Codex CLI is not assumed to point at
removed credentials. Additionally add a vitest regression in
test/codex-manager-cli.test.ts that deletes the final active account and asserts
that the sync failure prevents a success message and leaves active account
selection unchanged.
In `@test/codex-manager-cli.test.ts`:
- Around line 3772-3801: Add two deterministic test cases in
test/codex-manager-cli.test.ts that exercise the "partial-cleanup" warning
branch by making the mocked cleanup helpers indicate a cleanup miss: one where
deleteSavedAccountsMock resolves to a value that signals failure (e.g., false or
a non-success result) while resetLocalStateMock remains uncalled, and one where
resetLocalStateMock resolves to indicate failure while deleteSavedAccountsMock
succeeds. In each case, call runCodexMultiAuthCli(["auth", "login"]) with
promptLoginModeMock returning the same flows used in the existing tests, then
assert exitCode === 0 and that console.log was called with the windows
partial-cleanup warning message instead of the success message; ensure you use
vitest mocks (vi.spyOn/ mockResolvedValueOnce) so tests remain deterministic and
do not touch real secrets.
In `@test/storage.test.ts`:
- Around line 3500-3510: The test injection that throws on the first unlink call
is brittle because it checks unlinkSpy.mock.calls.length; change this to a
deterministic closure flag like failedOnce: declare let failedOnce = false
before creating unlinkSpy, then in the mock implementation use if (target ===
quotaPath && !failedOnce) { failedOnce = true; const err = new Error("locked")
as NodeJS.ErrnoException; err.code = code; throw err; } else return
realUnlink(target); — update the mock that references unlinkSpy, fs.unlink,
quotaPath and ensure this mirrors the earlier stable pattern used around
clearQuotaCache() tests.
🪄 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: b3c9d6fa-a653-422e-b4ec-6344bddfcb24
📒 Files selected for processing (6)
index.tslib/cli.tslib/codex-manager.tstest/cli.test.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/cli.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/cli.test.tstest/codex-manager-cli.test.tstest/storage.test.ts
🔇 Additional comments (3)
index.ts (3)
104-109: imports fromlib/destructive-actions.tslook correct.these align with the new destructive-action api surface.
deleteAccountAtIndex,deleteSavedAccounts,resetLocalState, andDESTRUCTIVE_ACTION_COPYare all documented in the enriched summary and match the interface definitions atlib/destructive-actions.ts:88-99,193-215.
3172-3186: index.ts reset/fresh dispatch still lacks concurrent protection.previous review flagged that lines 3164-3178 (now 3172-3186) execute destructive actions without the
destructiveActionInFlightguard enforced bylib/codex-manager.ts:3960,3984. the fresh path at lines 3156-3169 has the same gap.while this runs inside an interactive menu loop (low concurrency risk in practice), the index-level entry points diverge from the guarded lib-level equivalents, leaving a consistency gap vs what
test/destructive-actions.test.tsvalidates.consider:
- importing and reusing the module-level guard from
lib/codex-manager.ts, or- adding a local guard in
index.tsthat serializes/rejects concurrent reset/fresh/delete calls.
3156-3169: fresh path integration looks correct.
deleteSavedAccounts()returnsDestructiveActionResultperlib/destructive-actions.ts:193-199. the conditional message handling forresult.accountsClearedproperly surfaces partial failures. cache invalidation follows at line 3160.(concurrent protection gap covered in separate comment.)
| if (deleted) { | ||
| Object.assign(workingStorage, deleted.storage); | ||
| Object.assign(flaggedStorage, deleted.flagged); | ||
| invalidateAccountManagerCache(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Object.assign calls are dead code in current control flow.
deleteAccountAtIndex persists changes internally (per lib/destructive-actions.ts two-phase write). after this block, line 3131 hits continue, jumping back to the top of the while(true) loop at line 3029 where workingStorage and flaggedStorage are reloaded from disk anyway. these assignments update local vars that are immediately discarded.
either remove the Object.assign calls or, if keeping them for future use (e.g., skipping reload on the next iteration), add a comment explaining why.
🧹 suggested cleanup
if (deleted) {
- Object.assign(workingStorage, deleted.storage);
- Object.assign(flaggedStorage, deleted.flagged);
invalidateAccountManagerCache();
const label =
deleted.removedAccount.email?.trim() ||🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.ts` around lines 3113 - 3116, The Object.assign calls that copy
deleted.storage into workingStorage and deleted.flagged into flaggedStorage
(followed by invalidateAccountManagerCache()) are dead code because
deleteAccountAtIndex performs a two-phase write and the loop immediately
continues and reloads workingStorage/flaggedStorage; either remove those
Object.assign lines and the subsequent invalidateAccountManagerCache() call, or
keep them but add a clear comment in the block (referencing deleted,
workingStorage, flaggedStorage, invalidateAccountManagerCache,
deleteAccountAtIndex) explaining that these assignments are intentionally
retained to support a future optimization to skip the reload on the next
iteration; implement one of those two options.
| it("returns reset for TTY reset-all confirmation", async () => { | ||
| const { stdin, stdout } = await import("node:process"); | ||
| const origInputTTY = stdin.isTTY; | ||
| const origOutputTTY = stdout.isTTY; | ||
| const showAuthMenuMock = vi.fn().mockResolvedValue({ type: "reset-all" }); | ||
|
|
||
| Object.defineProperty(stdin, "isTTY", { | ||
| value: true, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| Object.defineProperty(stdout, "isTTY", { | ||
| value: true, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| try { | ||
| vi.resetModules(); | ||
| vi.doMock("../lib/ui/auth-menu.js", async () => { | ||
| const actual = await vi.importActual("../lib/ui/auth-menu.js"); | ||
| return { | ||
| ...(actual as Record<string, unknown>), | ||
| isTTY: vi.fn(() => true), | ||
| showAuthMenu: showAuthMenuMock, | ||
| }; | ||
| }); | ||
| mockRl.question.mockResolvedValueOnce("RESET"); | ||
|
|
||
| const { promptLoginMode } = await import("../lib/cli.js"); | ||
| const result = await promptLoginMode([{ index: 0 }]); | ||
|
|
||
| expect(result).toEqual({ mode: "reset" }); | ||
| expect(showAuthMenuMock).toHaveBeenCalledTimes(1); | ||
| } finally { | ||
| vi.doUnmock("../lib/ui/auth-menu.js"); | ||
| Object.defineProperty(stdin, "isTTY", { | ||
| value: origInputTTY, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| Object.defineProperty(stdout, "isTTY", { | ||
| value: origOutputTTY, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
restore the original tty descriptors.
test/cli.test.ts:227-273, test/cli.test.ts:276-329, test/cli.test.ts:498-527, and test/cli.test.ts:715-766 only stash the boolean isTTY values and then write back own data properties. on node runtimes where isTTY is inherited or has a different descriptor shape, that leaves mutated globals behind and can bleed into later cases. please snapshot and restore the full descriptor here, like test/codex-manager-cli.test.ts:219-250 already does.
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.
Also applies to: 276-329, 498-527, 715-766
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cli.test.ts` around lines 227 - 273, The test mutates
stdin.isTTY/stdout.isTTY by replacing only boolean values which can leave
altered property descriptors behind; change the setup/teardown in the "returns
reset for TTY reset-all confirmation" case (and the other similar blocks) to
save the full property descriptors via Object.getOwnPropertyDescriptor(stdin,
"isTTY") and Object.getOwnPropertyDescriptor(stdout, "isTTY") before mocking,
then restore those saved descriptors with Object.defineProperty in the finally
block (instead of writing back a simple value). Update the block that
manipulates isTTY (and corresponding blocks around promptLoginMode,
mockRl.question, and showAuthMenuMock) to use descriptor snapshot/restore so
inherited or non-writable descriptor shapes are preserved.
Summary
#75mainWhat Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs 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
af7f137Additional Notes
npm run lint,npm run typecheck,npm run build, andnpm test.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 extracts all destructive account operations (single-account delete, delete-saved-accounts, reset-local-state) into a new
lib/destructive-actions.tsmodule, replacing the old inline splice+save patterns with snapshot-before/rollback-on-failure semantics.clearQuotaCacheis added tolib/quota-cache.tswith EAGAIN/EBUSY/EPERM retry logic consistent with the existing storage helpers,unlinkWithRetryis promoted to a shared helper inlib/storage.ts, and bothclearAccounts/clearFlaggedAccountsare hardened to return booleans instead of throwing. theindex.tsandcodex-manager.tscallers are updated to useObject.assignin-place mutation so in-memory state stays coherent after a deletion, addressing the stale-storage issues from the previous review round. a module-leveldestructiveActionInFlightflag incodex-manager.tsprevents concurrent destructive actions within a single session; theauth-menu.tstyped-confirm gating (DELETE/RESET) replaces the earlier single-click confirm dialog for destructive danger-zone actions.key items:
deleteAccountAtIndexuses a pre-deletion snapshot (previousStorage) to roll back the accounts file ifsaveFlaggedAccountssubsequently fails;asSafeAggregateErrorstrips filesystem paths from error messages before they reach logs to avoid leaking home-directory paths on windowsclearQuotaCacheinlib/quota-cache.tscontains a deadreturn falseafter the retry loop — theattempt >= 4branch inside the loop always returns before the loop exits, making line 309 unreachablesaveAccountsfail,deleteAccountAtIndexthrows anAggregateError; both callers incodex-manager.tsandindex.tsextract.codefrom the thrown value, butAggregateErrorcarries no.code— the user always sees the generic retry message in this (windows-likely) double-failure path; the code is available on the sub-errors insideerror.errors[]test/destructive-actions.test.tsandtest/quota-cache.test.tsis thorough, including theAggregateErrorstructure, path-redaction, and EAGAIN/EBUSY/EPERM retry exhaustion; the one gap is that no test verifies what the cli prints to the user when the double-failureAggregateErroris caught byhandleManageActionConfidence Score: 4/5
deleteAccountAtIndexis sound, storage operations are correctly serialized behindwithStorageLock, thedestructiveActionInFlightmutex prevents concurrent destructive actions, and the typed-confirm gates are appropriately wired. the deadreturn falseinclearQuotaCacheis cosmetic. theAggregateErrorcode-extraction issue only surfaces on windows in the specific scenario where both the flagged-accounts save and its rollback both fail, which is uncommon — it produces a generic user message rather than a crash.lib/quota-cache.ts(dead code at line 309) andlib/codex-manager.ts/index.tscatch blocks fordeleteAccountAtIndex(AggregateErrorcode extraction)Important Files Changed
deleteAccountAtIndexsnapshot+rollback pattern is solid, but the double-failureAggregateErrorpath doesn't expose.codeto callersclearQuotaCachewith EAGAIN/EBUSY/EPERM retry logic; path-redaction helper added for windows safety; unreachablereturn falseafter the for loop is dead codeunlinkWithRetry(with EAGAIN added);clearAccountsnow returns boolean and handles legacy paths;clearFlaggedAccountssimilarly returns boolean — solid improvementdestructiveActionInFlightmutex guarding manage/fresh/reset paths;handleManageActionusesdeleteAccountAtIndexabstraction but theAggregateErrorerror-code extraction in the catch block doesn't surface a code to the userdeleteAccountAtIndexandObject.assignin-place refresh for bothworkingStorageandflaggedStorage; sameAggregateErrorcode-extraction gap ascodex-manager.tspromptTypedConfirmhelper with optional shared-rl support;resetandreset-allflows are wired correctly; cancellation copy is consistentreset-allmenu action; removes the extraconfirmprompt fordelete-all(handled by typed confirm in caller); ANSI/control-char regex promoted to module-level constants;sanitizeTerminalTextapplied to search subtitle and status message — good security hardeningAggregateError), path-redaction, andclampActiveIndicesedge cases; good windows-path redaction assertionsclearQuotaCacheAggregateErrorerror-display path through the menu handlerSequence Diagram
sequenceDiagram participant U as User (menu) participant CLI as cli.ts promptTypedConfirm participant DA as destructive-actions.ts participant ST as storage.ts participant QC as quota-cache.ts U->>CLI: selects "Delete Account" (delete-account confirm) CLI-->>U: confirm prompt U->>DA: deleteAccountAtIndex({storage, index}) DA->>ST: loadFlaggedAccounts() ST-->>DA: flagged DA->>ST: saveAccounts(nextStorage) alt saveAccounts succeeds ST-->>DA: ok DA->>ST: saveFlaggedAccounts(updatedFlagged) alt saveFlaggedAccounts succeeds ST-->>DA: ok DA-->>U: DeleteAccountResult {storage, flagged, removedAccount} else saveFlaggedAccounts fails DA->>ST: saveAccounts(previousStorage) [rollback] alt rollback succeeds DA-->>U: throw asSafeAggregateError (code preserved) else rollback fails DA-->>U: throw AggregateError (no .code on root error) end end end U->>CLI: selects "Delete Saved Accounts" CLI-->>U: typed confirm (type DELETE) U->>DA: deleteSavedAccounts() DA->>ST: clearAccounts() [boolean result] ST-->>DA: accountsCleared U->>CLI: selects "Reset Local State" CLI-->>U: typed confirm (type RESET) U->>DA: resetLocalState() DA->>ST: clearAccounts() DA->>ST: clearFlaggedAccounts() DA->>QC: clearQuotaCache() [EAGAIN/EBUSY/EPERM retry] DA->>DA: clearCodexCliStateCache() DA-->>U: DestructiveActionResult {accountsCleared, flaggedCleared, quotaCacheCleared}Comments Outside Diff (18)
lib/codex-manager.ts, line 3752-3536 (link)handleManageActiondelete not guarded bydestructiveActionInFlighthandleManageActioncallsdeleteAccountAtIndexdirectly without checking or settingdestructiveActionInFlight. in a programmatic or multi-tab session whererunAuthLogincould run on two concurrent paths, a single-account delete and aresetLocalStatecan overlap. the storage-level mutex (withStorageLock) protects individual file writes, but the two-phase commit indeleteAccountAtIndex(write-accounts → write-flagged or rollback) can interleave withclearAccountsinresetLocalState, leaving accounts or flagged entries in a partially-reset state.the same guard used for
fresh/resetshould wrap the individual-delete path, or at minimum the function should bail out early when a destructive action is already in flight:Prompt To Fix With AI
lib/destructive-actions.ts, line 780-782 (link)throw originalErrorbypasses path sanitization on successful rollbackwhen
saveFlaggedAccountsfails but the rollback write succeeds,throw originalErroris reached.originalErrorwas constructed byasError(), notasSafeAggregateError(), so its.messagestill contains the raw fs error — which on Windows can include the full%HOMEPATH%path (e.g.EPERM: …, rename 'C:\Users\alice\.codex\…'). only theAggregateErrorbranch (rollback also fails) sanitizes both messages. this inconsistency means the successful-rollback path still leaks the username.lib/storage.ts, line 2288-2325 (link)clearFlaggedAccountsreturnsfalseeven when data was successfully deletedwhen the data files (
path+ backups) are deleted without error but the marker cleanup at the end fails,hadErroris set totrueand the function returnsfalse. the callerresetLocalStatethen setsflaggedCleared: falseand surfaces the "completed with warnings" message to the user — even though the flagged accounts are already gone from disk.there's a second consequence: the stale marker file remains. while the next call to
clearFlaggedAccountsoverwrites it immediately (thefs.writeFilehappens before any deletion), recovery code that inspects the marker without first callingclearcould mis-classify state.the marker cleanup failure should be tracked separately from data deletion failure so callers can distinguish "data not deleted" from "data deleted but cleanup artifact remained":
the windows EPERM/EBUSY window on the marker file is real; separating the two failure categories prevents a benign transient lock on the marker from reporting a false failure to the user.
lib/codex-manager.ts, line 3817-3837 (link)deleteAccountAtIndexbypasses thedestructiveActionInFlightguarddeleteSavedAccountsandresetLocalStateboth check and set the module-leveldestructiveActionInFlightflag, buthandleManageAction(which callsdeleteAccountAtIndex) does not. if tworunCodexMultiAuthCliinvocations run concurrently — common on windows desktops where a user may open two terminals — both could enter the manage-delete path simultaneously.deleteAccountAtIndexcallssaveAccounts(which holds the storage lock), so the final write is serialized, but the read-modify-write cycle is not atomic across invocations: both callers snapshot the same original storage, both splice the same index, and the second write silently wins with a stale view.wrap the call with the same guard used for the other destructive actions:
there's no vitest coverage for concurrent manage-delete to catch this race; a
test/codex-manager-cli.test.tscase similar to "skips a second destructive action while reset is already running" would help verify the guard.lib/codex-manager.ts, line 3988-3992 (link)invalidateAccountManagerCache()missing afterdeleteSavedAccountsafter
runActionPanelcompletes and the function hitscontinue, the account manager cache still holds stale data from before the delete.index.tscallsinvalidateAccountManagerCache()immediately after the same destructive action (line 3152 inindex.ts), butcodex-manager.tsdoes not. the next menu loop iteration may re-render with old account entries still in the in-memory cache.same pattern repeats for the
resetmode block (line ~4021). both need the invalidation beforecontinue.lib/codex-manager.ts, line 4019-4023 (link)invalidateAccountManagerCache()missing afterresetLocalStatesame issue as the
fresh+deleteAllblock above: afterresetLocalState()the account manager cache is not invalidated beforecontinue.index.tscallsinvalidateAccountManagerCache()at line 3167, but this path does not. the next menu render can show ghost accounts from the pre-reset cache state.lib/quota-cache.ts, line 975-980 (link)error.messagemay leak Windows path in logsclearQuotaCachelogserror.messagedirectly intologWarn. on windows,fs.unlinkerrors look like"EPERM: operation not permitted, unlink 'C:\Users\johndoe\.codex\multi-auth\quota-cache.json'"— that embeds the windows username. theasSafeAggregateErrorhelper indestructive-actions.tsexists exactly for this reason (it strips messages containing path separators), but this new function doesn't use it.logWarnoutput can end up in telemetry sinks per the project's risk policy.same issue exists in the catch block of
saveQuotaCacheandloadQuotaCache, but those are pre-existing.clearQuotaCacheis new code added in this PR and should follow the redaction strategy from day one.or, pull in
asSafeAggregateErrorfromdestructive-actions.ts(or extract the redaction to a shared util) and use it here.Rule Used: What: Every code change must explain how it defend... (source)
index.ts, line 3154-3168 (link)index.tsdelete flow has nodestructiveActionInFlightguardlib/codex-manager.tscarefully wraps every destructive action with a module-leveldestructiveActionInFlightflag to prevent concurrent double-deletes, but thisindex.tscode path has no equivalent guard.two simultaneous terminal sessions both showing the auth menu (both running the
index.tsloop) can each independently calldeleteAccountAtIndexwith the same originalworkingStoragesnapshot. both calls computenextStorageby splicing the same index out of the same accounts array, both callsaveAccounts(nextStorage), and the second write silently overwrites the first — restoring the account that the first session just deleted. the underlyingwithStorageLockmutex serialises the writes, but it doesn't protect against the read-compute-write race at the application layer.this is the same concurrency contract that
codex-manager.tsexplicitly defends against with the in-flight flag and its accompanying test ("skips a second manage delete while another destructive action is already running"). theindex.tspath should either adopt the same flag (declared at module scope inindex.ts) or delegate fully tocodex-manager.tsso the guard is not duplicated.index.ts, line 3163-3168 (link)Email dropped from delete confirmation log
deleteAccountAtIndexreturnsdeleted.removedAccount(typeAccountMetadataV3) which includes the originalemailfield, but the caller always constructslabelfrom the index number instead. the old code loggedtarget.email ?? \Account N`` — this PR regresses to always showing "Account N", so users lose the confirmation that they deleted the right account.same pattern also appears in
lib/codex-manager.tsaround line 3533 (const label = \Account ${idx + 1}``).lib/codex-manager.ts, line 3533 (link)Email dropped from delete log (manager path)
same regression as
index.ts:deleted.removedAccount.emailis available but ignored — users see "Account 2" instead of"user@example.com".DeleteAccountResult.removedAccountis typed to carry the fullAccountMetadataV3includingemail, use it here.lib/destructive-actions.ts, line 811-817 (link)deleteSavedAccountspropagates uncaughtclearAccountsthrowsclearAccounts()can throw before it reaches the best-effort artifact loop — specifically at thefs.mkdirandfs.writeFilecalls for the reset-marker, both of which are bare awaits with no enclosing try-catch. on windows, AV or an OS file-lock on%HOMEPATH%can produce EPERM here, causingdeleteSavedAccountsto reject.the caller in
index.ts(const result = await deleteSavedAccounts()) has no surrounding try-catch, so that exception escapes thewhilemenu loop entirely and crashes the process.resetLocalStatehandles this correctly with.catch(() => false). apply the same guard here:test/destructive-actions.test.ts, line 3905-3947 (link)AggregateError rollback path has no path-redaction coverage
the existing test covers the single-failure path (flagged-save fails, rollback succeeds → rethrows original error). but there is zero test for the double-failure path:
saveFlaggedAccountsthrows with a windows-style path in the message, ANDsaveAccounts(previousStorage)also throws with a path-containing message. in that case the code reaches:both inner errors are wrapped by
asSafeAggregateError, which creates sanitisedErrorobjects, but there is no test asserting that (a) theAggregateErroris thrown rather than a plain error, and (b) the inner errors do not contain the windows username path. given rule 637a42e6 requires explicit concurrency reasoning with a matching regression test for every windows filesystem path, this gap should be closed.a minimal addition to
test/destructive-actions.test.ts:Rule Used: What: Every code change must explain how it defend... (source)
lib/destructive-actions.ts, line 775-803 (link)Uncaught throw from
deleteAccountAtIndexcrashes both callerswhen
saveFlaggedAccountsthrows (e.g. windows EPERM while antivirus holds the file) and rollback succeeds, this function re-throws a safe error at line 799. both callers —index.tsandcodex-manager.ts— have notry-catcharounddeleteAccountAtIndex, so the exception propagates unhandled and terminates the interactive session entirely.lib/codex-manager.tshas atry-finallythat resetsdestructiveActionInFlight, but the error still bubbles torunAuthLoginwithout being caught.index.tshas no try protection at all around the call.the rollback guarantees data integrity, but the UX result is a crash. add a
try-catchin each caller to display a friendly error message and let the menu loop continue:same pattern needed in
index.tsaround thedeleteAccountAtIndexcall (lines ~3154-3172).lib/cli.ts, line 336-393 (link)Nested readline interfaces race on the same stdin
promptLoginModeFallbackholds an outerrlopen for the main loop (created at the top of the function) and then callspromptDeleteAllTypedConfirm()/promptResetTypedConfirm(), each of which creates a secondcreateInterface({ input, output })on the sameprocess.stdin. While the outerrlis only passive (not mid-question()) during the inner call, Node.js readline attaches its ownkeypress/dataevent listeners as soon as the interface is created. Two active interfaces on the same stream compete for input events; on Windows in particular, the second interface can miss the newline terminator that the first interface re-emits when it resumes, causing the next outerrl.question()call to return immediately with an empty string and loop unexpectedly.the TTY path (
showAuthMenu→promptLoginMode) avoids this because the interactive selector fully completes before a readline interface is opened. the fallback path does not — consider closing and reopeningrlaround each destructive-action confirmation, or hoisting all readline interactions into the outer loop so only one interface is ever live at a time.lib/destructive-actions.ts, line 772-782 (link)Path-detection regex fires on non-path slashes
asSafeAggregateErrorredacts the whole error message whenever it contains\or/. that heuristic also triggers on completely legitimate messages like"read/write error","n/a","operation x/y failed", etc. — stripping the only useful diagnostic context.node.js filesystem errors reliably start with
ERRNO: description, syscall '/full/path'so a tighter check like/(^|[ "'])(?:[a-zA-Z]:)?[/\\]/.test(msg)would catch real path-bearing messages without destroying generic ones. the existing rollback tests don't exercise this case, so a false-positive here would silently swallow the true error code.lib/codex-manager.ts, line 3562-3590 (link)deleted.flaggedis silently dropped — flaggedStorage stays stale incodex-manager.tspathdeleteAccountAtIndexinternally callsloadFlaggedAccounts(), filters out the matching entry, and persists the result. it returns the updated flagged snapshot asdeleted.flagged. here,Object.assign(storage, deleted.storage)correctly updates accounts in memory, butdeleted.flaggedis never applied to any localflaggedStoragereference in the caller.the
index.tsflow was fixed for this exact issue (previous thread on line 3168 there), buthandleManageActionincodex-manager.tsdoesn't receive or propagateflaggedStorageat all. ifrunAuthLoginpasses a localflaggedStoragecount toshowAuthMenufor the "verify-flagged" badge, deleting an account with a matching flagged entry will leave the badge count inflated until a full re-entry into the menu re-reads from disk.handleManageActionshould either accept aflaggedStorageparameter andObject.assignit likeindex.tsdoes, or return thedeleted.flaggedvalue so the caller can update its local copy.(or thread
flaggedStorageas a parameter tohandleManageActionand assign unconditionally)lib/destructive-actions.ts, line 800-806 (link)Accounts written to disk before flagged save — rollback window still exists
saveAccounts(nextStorage)commits to disk unconditionally (line 805), thensaveFlaggedAccountsis attempted. on Windows, the OS file lock from the firstsaveAccountsrename may still be held when the rollbacksaveAccounts(previousStorage)is issued — EPERM/EBUSY during rollback is a real failure mode on user desktops.saveAccountshas its own retry loop, but a retry-exhausted rollback surfaces theAggregateErrorwith both the original and rollback errors.this was already noted in previous review thread on this file. the existing tests cover the
AggregateErrorpath but there is no vitest regression that exercises an EBUSY rollback write that eventually succeeds after the lock clears — i.e. a scenario wheresaveFlaggedAccountsfails with EPERM butsaveAccounts(previousStorage)succeeds only on the 3rd retry. consider adding a test that stubssaveAccountsto throw EBUSY on the rollback attempt once, then resolve — confirming the final on-disk state matchespreviousStorageand noAggregateErroris thrown.lib/codex-manager.ts, line 3848-3854 (link)AggregateErrorswallows the error code; double-failure shows generic messagewhen both
saveFlaggedAccountsand the rollbacksaveAccountsfail insidedeleteAccountAtIndex, the function throws anAggregateError.AggregateErrordoesn't carry a.codeproperty, so(error as NodeJS.ErrnoException)?.codereturnsundefinedand the user always sees the generic "Failed to delete account. Please retry." with no code — even on windows where this double-failure scenario is most likely (EPERM/EBUSY on the rename + another EPERM on the rollback unlink). the individual sub-errors insideerror.errors[]do have.codeset viaasSafeAggregateError, but they're never inspected here.the same pattern repeats in
index.tsat lines 3127-3132.consider probing the sub-errors for a code when the top-level
.codeis absent:Prompt To Fix All With AI
Last reviewed commit: "fix(auth): harden re..."