fix(auth): define reset and delete safety flows#75
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthroughwalkthroughadds a reset-local-state option and typed-confirm flows, introduces a destructive-actions module, wires delete/reset flows into cli/ui, and adds quota-cache clearing and robust file-retry helpers across storage. (50 words) changes
sequence diagram(s)sequenceDiagram
participant user as user
participant cli as cli
participant menu as auth menu
participant da as destructive actions
participant storage as account storage
participant flagged as flagged accounts
participant quota as quota cache
participant codex as codex cli state
user->>cli: choose "reset-all"
cli->>menu: show typed-confirm using DESTRUCTIVE_ACTION_COPY (`lib/cli.ts`:line)
menu->>user: request typed confirmation (`lib/ui/auth-menu.ts`:line)
user->>menu: confirm
menu->>da: call resetLocalState() (`lib/destructive-actions.ts`:line)
par remove saved accounts
da->>storage: delete saved accounts / rebase indices (`lib/destructive-actions.ts`:line)
storage->>storage: persist storage (`lib/storage.ts`:line)
and remove flagged accounts
da->>flagged: load flagged, filter by refreshToken, persist if changed (`lib/destructive-actions.ts`:line)
and clear caches
da->>quota: clearQuotaCache() (`lib/quota-cache.ts`:line)
da->>codex: clear codex cli state (`lib/destructive-actions.ts`:line)
end
da-->>menu: completion message (`lib/destructive-actions.ts`:line)
menu-->>user: display "reset complete" (`lib/ui/auth-menu.ts`:line)
estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes detailed review flags
suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 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}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/cli.ts">
<violation number="1" location="lib/cli.ts:213">
P2: Require typed confirmation for `reset` in fallback mode as well; currently it bypasses the destructive-action safety check.</violation>
</file>
<file name="lib/quota-cache.ts">
<violation number="1" location="lib/quota-cache.ts:274">
P2: `clearQuotaCache` should retry transient `EBUSY`/`EPERM` unlink failures; a single attempt can leave stale quota cache after reset/delete flows.</violation>
</file>
<file name="docs/troubleshooting.md">
<violation number="1" location="docs/troubleshooting.md:106">
P2: The manual reset/delete instructions are not equivalent to the CLI destructive actions because they omit account WAL/backup cleanup. Leaving `.wal`/`.bak*` artifacts can cause deleted accounts to be recovered on next load.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/codex-manager.ts (1)
4120-4130:⚠️ Potential issue | 🟡 Minorpotential stale
flaggedStorageafter delete operations.
flaggedStorageis loaded atlib/codex-manager.ts:4120inside the menu loop, but afterdeleteAccountAtIndexmodifies it atlib/codex-manager.ts:4022-4026, the next menu iteration may use stale data since the loopcontinues without reloading.if
deleteAccountAtIndexmutates the passedflaggedStorageobject in place and persists it, subsequent iterations could show inconsistent counts in the menu (e.g.,flaggedCountat line 4125).#!/bin/bash # check if deleteAccountAtIndex mutates flaggedStorage in place or returns new data echo "=== checking deleteAccountAtIndex implementation ===" rg -A 30 "export.*function deleteAccountAtIndex|export async function deleteAccountAtIndex" lib/Also applies to: 4225-4240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4120 - 4130, The menu uses flaggedStorage loaded by loadFlaggedAccounts() but deleteAccountAtIndex may mutate/persist it and the loop continues without reloading, causing stale flaggedCount shown to promptLoginMode; fix by reloading or replacing flaggedStorage after any deleteAccountAtIndex call (or change deleteAccountAtIndex to return the new storage and assign it). Update the loop so that after calling deleteAccountAtIndex you either call flaggedStorage = await loadFlaggedAccounts() or assign the returned updated storage before continuing, and ensure promptLoginMode/toExistingAccountInfo always receives the fresh flaggedStorage for the flaggedCount.test/codex-manager-cli.test.ts (1)
51-54:⚠️ Potential issue | 🟠 Majorthese cases do not exercise the new accept/cancel safety flow.
with
promptLoginModemocked attest/codex-manager-cli.test.ts:51-54and the destructive-actions module mocked attest/codex-manager-cli.test.ts:138-157, the new tests attest/codex-manager-cli.test.ts:2161-2248only verify howrunCodexMultiAuthCli()reacts to a pre-baked result. they never hit the real confirm/cancel behavior inlib/cli.ts:173-235orlib/ui/auth-menu.ts:777-788, which is the safety surface this pr is adding. please add focused vitest coverage against the real prompt/menu modules, including the fallback branch. 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: 138-157, 2161-2248
🤖 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 51 - 54, The current tests mock promptLoginMode and destructive-actions so they never exercise the real accept/cancel safety flow; add focused Vitest tests that import the real prompt/menu modules (do not vi.mock promptLoginMode or destructive-actions in these new tests) and call runCodexMultiAuthCli to drive the confirm/cancel branches in lib/cli.ts and the accept/cancel + fallback in lib/ui/auth-menu.ts, asserting both acceptance and cancellation behavior. Use Vitest utilities (fake timers, controlled async helpers) to keep tests deterministic and add targeted regression scenarios that simulate concurrency/token-refresh races by orchestrating parallel promises around runCodexMultiAuthCli and stub the token refresh entry points, and simulate Windows filesystem behavior by stubbing fs/path functions (e.g., path.sep or fs.stat) to trigger the Windows branch; keep tests isolated and use vitest mocks/stubs only for external side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/troubleshooting.md`:
- Around line 106-129: The troubleshooting doc's manual cleanup commands omit
the write-ahead log and backup/recovery files that clearAccounts() in
lib/storage.ts removes, so update docs/troubleshooting.md to either list and
include removal commands for the WAL and backup-recovery artifacts (matching
clearAccounts() behavior) or replace the manual steps with a clear instruction
to run the CLI destructive command instead; reference clearAccounts(), the
existing JSON filenames (openai-codex-accounts.json,
openai-codex-flagged-accounts.json, quota-cache.json), and mention the
WAL/backup-recovery artifacts so the doc text and commands exactly mirror the
CLI behavior.
In `@lib/cli.ts`:
- Around line 205-214: The fallback readline path in promptLoginModeFallback is
returning "fresh" or "reset" immediately (see the branch that returns { mode:
"fresh", deleteAll: true } and { mode: "reset" }) which bypasses the
typed-confirm destructive guard implemented in promptLoginMode (the
DESTRUCTIVE_ACTION_COPY flow used in the normal TTY path). Update
promptLoginModeFallback to invoke the same DESTRUCTIVE_ACTION_COPY confirm flow
(reusing the promptLoginMode or the underlying confirm routine) so non-TTY users
must explicitly type the confirm text to proceed, and make sure the user-facing
wording matches lib/ui/copy.ts semantics (clarify delete vs reset). Add vitest
coverage for the fallback path (unmock promptLoginMode in
test/codex-manager-cli.test.ts and add tests to cover both accept and cancel
outcomes) and update tests referenced in the review to assert both branches.
In `@lib/codex-manager.ts`:
- Around line 4195-4219: Add a re-entry guard around destructive operations so
deleteSavedAccounts() and resetLocalState() cannot run concurrently: introduce a
module-level boolean (e.g., isDestructiveInFlight) and check it before invoking
runActionPanel; if true, skip or show a message. When running runActionPanel for
DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts or
DESTRUCTIVE_ACTION_COPY.resetLocalState, set isDestructiveInFlight = true before
starting the async action and reset it in a finally block after the await
completes (or fails) to ensure the flag is always cleared; alternatively
integrate a simple mutex/lock helper used by the callers to serialize these
actions and/or ensure runActionPanel exposes an option to block input while the
action runs. Ensure references include runActionPanel, deleteSavedAccounts,
resetLocalState, DESTRUCTIVE_ACTION_COPY, and displaySettings so the change is
applied at the correct call sites.
- Around line 4028-4034: The console.log in codex-manager.ts leaks full emails
because it calls formatAccountLabel (from lib/accounts.ts) which embeds the raw
email; update the logging to use a masked email variant instead of the raw one:
either modify formatAccountLabel to return a safe/masked string for log usage or
add a new helper (e.g., maskEmail or formatAccountLabelForLogs) and use that
when building the label for deleted.removedAccount in the Deleted log path (the
code that reads deleted.removedAccount and deleted.removedFlaggedCount); ensure
masking keeps enough context (e.g., first char and domain or last 6 chars) but
never logs the full address.
In `@lib/destructive-actions.ts`:
- Around line 38-69: clampActiveIndices currently only clamps values which
causes deleting an account before the current one to leave stored indices
pointing one-too-high; before calling clampActiveIndices you must decrement
storage.activeIndex and every entry in storage.activeIndexByFamily that is
greater than the removed index (the deleted position) so indices rebase
correctly, then call clampActiveIndices(storage) to bound them; update logic
around the removal/splice site that deletes an account to perform this decrement
(referencing storage.activeIndex, storage.activeIndexByFamily and
MODEL_FAMILIES/AccountStorageV3) and add a Vitest that deletes an account at a
lower index than the active one and asserts activeIndex and each
activeIndexByFamily entry were shifted down (replace the current mock of this
module in the test with a real-module exercise so the regression is caught).
In `@lib/ui/auth-menu.ts`:
- Around line 97-102: sanitizeTerminalText currently embeds the control char
\u001b in a regex literal which triggers the noControlCharactersInRegex lint
rule; change the ANSI escape regex to be built via the RegExp constructor using
a string (e.g. new RegExp("\\u001b\\[[0-?]*[ -/]*[`@-`~]", "g")) and keep the
subsequent control-character removal and trim logic intact in
sanitizeTerminalText so behavior is unchanged. After fixing the regex, add
Vitest regression tests for sanitizeTerminalText that cover: stripping ANSI
color sequences, removing other control characters (e.g. null and DEL), handling
empty/undefined input, already-sanitized strings, and Windows CRLF line endings;
also include tests exercising the callers that sanitize account.email,
account.accountLabel, and account.accountId to ensure no regression.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 4120-4130: The menu uses flaggedStorage loaded by
loadFlaggedAccounts() but deleteAccountAtIndex may mutate/persist it and the
loop continues without reloading, causing stale flaggedCount shown to
promptLoginMode; fix by reloading or replacing flaggedStorage after any
deleteAccountAtIndex call (or change deleteAccountAtIndex to return the new
storage and assign it). Update the loop so that after calling
deleteAccountAtIndex you either call flaggedStorage = await
loadFlaggedAccounts() or assign the returned updated storage before continuing,
and ensure promptLoginMode/toExistingAccountInfo always receives the fresh
flaggedStorage for the flaggedCount.
In `@test/codex-manager-cli.test.ts`:
- Around line 51-54: The current tests mock promptLoginMode and
destructive-actions so they never exercise the real accept/cancel safety flow;
add focused Vitest tests that import the real prompt/menu modules (do not
vi.mock promptLoginMode or destructive-actions in these new tests) and call
runCodexMultiAuthCli to drive the confirm/cancel branches in lib/cli.ts and the
accept/cancel + fallback in lib/ui/auth-menu.ts, asserting both acceptance and
cancellation behavior. Use Vitest utilities (fake timers, controlled async
helpers) to keep tests deterministic and add targeted regression scenarios that
simulate concurrency/token-refresh races by orchestrating parallel promises
around runCodexMultiAuthCli and stub the token refresh entry points, and
simulate Windows filesystem behavior by stubbing fs/path functions (e.g.,
path.sep or fs.stat) to trigger the Windows branch; keep tests isolated and use
vitest mocks/stubs only for external side effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d39fe25-7aee-4de0-ae2d-a44182768739
📒 Files selected for processing (10)
docs/troubleshooting.mdindex.tslib/cli.tslib/codex-manager.tslib/destructive-actions.tslib/quota-cache.tslib/ui/auth-menu.tslib/ui/copy.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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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/ui/copy.tslib/quota-cache.tslib/destructive-actions.tslib/ui/auth-menu.tslib/cli.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/codex-manager-cli.test.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
🪛 ast-grep (0.41.0)
lib/codex-manager.ts
[warning] 755-755: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((?:^|\\|)\\s*${windowLabel}\\s+(\\d{1,3})%, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.4.6)
lib/ui/auth-menu.ts
[error] 100-100: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (3)
lib/codex-manager.ts (3)
4008-4011: lgtm on the signature change.the optional
flaggedStorage?: FlaggedAccountStorageV1parameter is correctly typed and maintains backward compatibility. callers that don't need flagged account handling can omit it.
4195-4219: destructive action error scenarios lack test coverage; add tests for transient failures.the cancel and accept flows are already tested at
test/codex-manager-cli.test.ts:2161(cancel),test/codex-manager-cli.test.ts:2186(delete success), andtest/codex-manager-cli.test.ts:2218(reset success). however, there are no tests for error recovery whendeleteSavedAccounts()orresetLocalState()throw.add regression tests for:
- transient EBUSY errors on windows during account/flagged account deletion
- recovery from partial failures (e.g., accounts deleted but flagged accounts delete fails)
- retries for rate-limited quota cache clears
the
runActionPanelatlib/codex-manager.ts:4100+captures errors but has no visible retry logic. compare this to the EBUSY/429 retry pattern already in the settings flow tests to ensure parity.
47-52: destructive action imports verified—proper windows fs handling and test coverage in place.the new imports from lib/destructive-actions.ts (deleteSavedAccounts, resetLocalState, deleteAccountAtIndex) have solid backing: all delegate through storage helpers wrapped in withStorageLock mutex. lib/storage.ts:~1500 implements explicit EBUSY/EPERM retry with 5 attempts and exponential backoff (10ms * 2^N), directly addressing windows filesystem locking. test/storage.test.ts contains dedicated test suites ("saveAccounts EPERM/EBUSY retry logic") covering transient failures and all five retry exhaustion. integration layer tested in test/codex-manager-cli.test.ts confirms deleteSavedAccounts and resetLocalState flows. no token or email leaks in logging.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/cli.test.ts">
<violation number="1" location="test/cli.test.ts:615">
P2: These new fallback-cancel tests are outside the suite setup, so they can pass via `promptLoginMode`'s non-interactive early return instead of testing typed-confirm behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/cli.ts`:
- Around line 278-283: The "reset-all" tty branch prints "Reset cancelled."
which is inconsistent with the fallback branch's message "Reset local state
cancelled."; update the console.log in the case "reset-all" (the block that runs
when promptResetTypedConfirm() returns false) to use the exact same cancellation
message ("Reset local state cancelled.") or extract a shared constant/string
used by the fallback to ensure both branches print the same text.
In `@test/cli.test.ts`:
- Around line 615-639: Move the two orphaned it(...) tests that start with
"cancels fallback delete-all..." and "cancels fallback reset..." into the
existing describe block for the CLI Module (or into the nested describe for
promptLoginMode); update them to rely on the shared beforeEach/afterEach setup
(remove the duplicated vi.mocked(createInterface) and mockRl setup if already
handled by the suite) and keep references to promptLoginMode, createInterface
and mockRl intact so they run with the same test fixtures and are grouped with
the other CLI tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91f4c2ce-2e54-4d7a-986b-72f682fdc18c
📒 Files selected for processing (3)
docs/troubleshooting.mdlib/cli.tstest/cli.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/cli.test.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
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.ts
🔇 Additional comments (4)
lib/cli.ts (2)
149-171: typed confirmation guards now cover both destructive paths.both
promptDeleteAllTypedConfirmandpromptResetTypedConfirmfollow the same try/finally pattern ensuring readline cleanup. the fallback path atlib/cli.ts:205-222and tty path atlib/cli.ts:272-283both gate destructive actions behind these confirms.minor observation: each confirm creates a new readline interface. if a user cancels and retries multiple times in a session, this creates/destroys interfaces repeatedly. not a bug, but reusing the existing
rlfrom the caller might be cleaner. low priority.
216-222: fallback reset path correctly guarded.the reset branch at lines 216-222 properly calls
promptResetTypedConfirm()and only returns{ mode: "reset" }on success. the continue on cancel loops back to mode selection. this addresses the past review comment about fallback bypassing safety flows.docs/troubleshooting.md (1)
92-137: documentation correctly reflects cli behavior without unnecessary file removals.the exact effects table and manual steps are accurate. flagged accounts don't use the wal/backup pattern that saved accounts do—
saveFlaggedAccountsatlib/storage.ts:1324-1343uses a simple atomic write (temp file → rename), not the writeFile wrapper with wal/backup thatsaveAccountsuses. so the docs are right to excludeopenai-codex-flagged-accounts.json.waland.bak*files from the reset local state commands.clearFlaggedAccountsatlib/storage.ts:1347-1357only unlinks the main file, which matches the documented behavior.test/cli.test.ts (1)
615-639: missing test coverage for reset-all path and successful reset confirmations.the tests at lines 615-639 cover the fallback "reset" input cancel flow, which is good. however, the tty path where
showAuthMenureturns{ type: "reset-all" }(lib/cli.ts:79-84) has zero test coverage. additionally, even the fallback path lacks a test for successful reset confirmation (where user types "RESET" correctly), unlike the "fresh" path which has successful confirmation tests at lines 154-174.add tests that:
- mock
showAuthMenuto return{ type: "reset-all" }and verify the typed confirmation flow with valid "RESET" input- verify fallback path for successful reset confirmation with "r"/"reset" input followed by "RESET" confirmation
per coding guidelines, regression tests should cover windows readline edge cases (EBUSY on file conflicts, readline interface lifecycle on windows during rapid create/close cycles).
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/quota-cache.ts">
<violation number="1" location="lib/quota-cache.ts:275">
P2: `clearQuotaCache()` still uses a module-initialized `QUOTA_CACHE_PATH`, so runtime `CODEX_MULTI_AUTH_DIR` changes are ignored and the wrong cache file can be targeted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/quota-cache.test.ts">
<violation number="1" location="test/quota-cache.test.ts:103">
P2: Use retry-based cleanup instead of single-shot `fs.rm` to avoid Windows EBUSY/EPERM test flakes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/codex-manager.ts (1)
4093-4117:⚠️ Potential issue | 🔴 Criticalwait for the background quota refresh before running destructive or manage actions.
pendingMenuQuotaRefreshcan still be insiderefreshQuotaCacheForMenu()and callsaveQuotaCache()fromlib/codex-manager.ts:700-702afterresetLocalState()clears the cache inlib/destructive-actions.ts:146-155. if the user picks reset while the menu auto-fetch is in flight,quota-cache.jsoncan be recreated after the flow reports success. the same race can also repopulate stale entries after delete/manage actions. await or cancel the pending refresh before these paths, and add a vitest regression intest/codex-manager-cli.test.tsthat reproduces it. 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: 4195-4251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4093 - 4117, pendingMenuQuotaRefresh can race with destructive/manage flows and recreate quota-cache after resetLocalState; fix by ensuring destructive/manage entry points await or cancel the in-flight pendingMenuQuotaRefresh Promise returned by refreshQuotaCacheForMenu (or expose an abort mechanism from refreshQuotaCacheForMenu) before calling saveQuotaCache or resetLocalState; update the code paths that perform reset/manage/delete to check pendingMenuQuotaRefresh (and await .finally or call an abort API) so cache writes cannot win the race, add a vitest regression in test/codex-manager-cli.test.ts that triggers reset/manage while refreshQuotaCacheForMenu is in flight to reproduce the issue, and when introducing the queue/abort handling ensure retries handle EBUSY/429 and that logging around these changes avoids leaking tokens/emails.
♻️ Duplicate comments (2)
lib/destructive-actions.ts (1)
103-105:⚠️ Potential issue | 🔴 Criticalrebase active indices before clamping after a delete.
after
splice(),clampActiveIndices()only bounds values. ifoptions.indexis belowactiveIndexor anyactiveIndexByFamilyentry, those pointers stay one too high and silently retarget the next account instead of the previously selected one. decrement indices greater than the removed slot beforelib/destructive-actions.ts:104, and add a vitest for that path becausetest/destructive-actions.test.tsdoes not cover it today. 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/destructive-actions.ts` around lines 103 - 105, After removing an account with nextStorage.accounts.splice(options.index, 1) in lib/destructive-actions.ts, adjust any stored pointers that point past the removed slot (e.g., activeIndex and entries in activeIndexByFamily) by decrementing values greater than options.index before calling clampActiveIndices(nextStorage) and saveAccounts(nextStorage); update the code paths that mutate storage (the splice block around nextStorage/accounts) to perform this rebase, add a vitest in test/destructive-actions.test.ts that deletes an earlier index and asserts activeIndex and activeIndexByFamily were moved to the previous account, and ensure your changes follow the repo guidelines by updating any affected queues to handle EBUSY/429 and avoiding logs that leak tokens or emails.lib/codex-manager.ts (1)
4028-4033:⚠️ Potential issue | 🟠 Majormask account labels in deletion logs.
formatAccountLabel()is written straight to stdout here.test/codex-manager-cli.test.ts:63-65models that label with the raw email address, so this delete path prints pii to the console. use a log-safe or masked label instead. 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 4028 - 4033, Log currently prints raw account labels using formatAccountLabel(deleted.removedAccount,...), leaking PII; replace that call with a masked/log-safe label generator (e.g., maskEmailLabel or formatAccountLabelSafe) that redacts identifiable parts of deleted.removedAccount while preserving the idx context, then use the masked label in the console output instead of formatAccountLabel. Update the affected test (test/codex-manager-cli.test.ts lines ~63-65) to assert the masked form is logged (not the raw email), and run the vitest suite; also scan lib/codex-manager.ts for any other console/log calls that use full emails or tokens and switch them to the same masking helper.
🤖 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/destructive-actions.ts`:
- Around line 114-118: When saveFlaggedAccounts(updatedFlagged) fails, we must
not lose that original error if the rollback saveAccounts(previousStorage) also
throws; wrap the rollback call in its own try/catch inside the catch block for
saveFlaggedAccounts and if the rollback throws create and throw an
AggregateError that includes both the original error and the rollback error
(preserving both). Update the catch around saveFlaggedAccounts in the function
handling updatedFlagged/previousStorage to catch rollback errors separately and
throw new AggregateError([originalError, rollbackError], 'saving flagged
accounts failed and rollback failed').
- Around line 135-139: The return object from deleteSavedAccounts incorrectly
claims flaggedCleared and quotaCacheCleared are true even though the function
only calls clearAccounts(); update deleteSavedAccounts so it only sets
flaggedCleared and quotaCacheCleared to true when those specific clear
operations actually occur: either call the appropriate helpers (e.g.,
clearFlagged, clearQuotaCache) and await their results and use those booleans,
or set those fields to false if you didn't perform those clears; ensure
clearAccounts() result is used for accountsCleared and do not assert
cleared=true for flaggedCleared/quotaCacheCleared unless verified.
In `@lib/quota-cache.ts`:
- Around line 272-295: The module currently uses the module-scoped
QUOTA_CACHE_PATH which does not update when CODEX_MULTI_AUTH_DIR changes; update
clearQuotaCache() (and similarly saveQuotaCache() and any other helpers) to
resolve the path lazily by calling getQuotaCachePath() at runtime instead of
referencing QUOTA_CACHE_PATH, then use that returned path for
fs.unlink/fs.writeFile/etc., preserving the existing retry/backoff and ENOENT
handling; remove reliance on the stale QUOTA_CACHE_PATH variable so tests that
change CODEX_MULTI_AUTH_DIR target the correct file.
In `@lib/storage.ts`:
- Around line 1307-1339: clearAccounts currently unlinks only the primary path,
wal and fixed backup slots; update clearAccounts to enumerate and delete every
recovery surface that loadAccountsInternal can read (use
getAccountsBackupRecoveryCandidates result plus legacy project/worktree store
paths referenced in loadAccountsInternal and the legacy path helpers), and
perform each delete via the same backoff retry helper used elsewhere for
filesystem deletes so transient EPERM/EBUSY on Windows is retried; keep
per-artifact logging and preserve the hadError semantics, and update/add vitest
cases that assert “delete saved accounts” stays empty when only legacy or extra
backup artifacts exist.
- Around line 1534-1552: clearFlaggedAccounts currently only removes the new
flagged-store and can allow the legacy openai-codex-blocked-accounts.json to be
migrated back by loadFlaggedAccounts; update clearFlaggedAccounts to unlink both
getFlaggedAccountsPath() and the legacy filename
("openai-codex-blocked-accounts.json") using the same windows-safe retry/backoff
logic used elsewhere for unlink (handle EBUSY, EACCES, and transient 429-like
conditions), ensure operations are serialized via withStorageLock, and return
true only when both removals either succeed or are ENOENT; add a vitest that
simulates the legacy file present after clear (including a transient EBUSY on
unlink to validate retry/backoff) to prevent legacy-file resurrection and
reference the adjusted functions clearFlaggedAccounts and loadFlaggedAccounts in
the test description.
- Around line 1227-1249: saveAccountsUnlocked currently performs its own
temp→final rename loop that only retries EPERM/EBUSY, missing EAGAIN handling;
route the rename in saveAccountsUnlocked to use the existing renameFileWithRetry
helper (or at minimum add EAGAIN to the retry set) so transient EAGAIN is
handled the same as EPERM/EBUSY, and update/add a vitest that simulates one
transient EAGAIN on fs.rename before a successful rename to assert the save
succeeds and cleanup (wal unlink) still runs; reference renameFileWithRetry and
saveAccountsUnlocked when implementing and updating tests.
In `@test/destructive-actions.test.ts`:
- Around line 31-167: Add deterministic vitest coverage for resetLocalState by
exercising the exported function resetLocalState (and the existing
deleteSavedAccounts and deleteAccountAtIndex flows) to assert the new three-flag
result shape (accountsCleared, flaggedCleared, quotaCacheCleared) and that
clearCodexCliStateCacheMock is invoked when appropriate; create tests that
simulate successful clears, partial failures (e.g., make
clearFlaggedAccountsMock or clearQuotaCacheMock reject once) and ensure
deleteSavedAccounts returns correct flags without pretending others were
cleared, and simulate rollback scenarios for deleteAccountAtIndex where
saveFlaggedAccountsMock rejects then ensure resetLocalState's cache-clear side
effect (clearCodexCliStateCacheMock) is called or not as expected; keep all
mocks deterministic via vi.resetModules/clearAllMocks, avoid using real secrets
or non-deterministic timers, and assert storage mutation (or lack thereof) and
error aggregation behavior (AggregateError) for concurrent/rollback failure
cases.
In `@test/quota-cache.test.ts`:
- Around line 82-105: Add deterministic tests for the windows retry branches in
clearQuotaCache by mocking the filesystem unlink/rm calls to reproduce transient
EBUSY/EPERM and exhausted-retry behavior: import clearQuotaCache,
saveQuotaCache, and getQuotaCachePath from "../lib/quota-cache.js" as in the
existing test, then write one test that stubs the fs.unlink/fs.rm (same function
used by clearQuotaCache) to throw an { code: 'EBUSY' } or { code: 'EPERM' } on
the first N-1 calls and succeed on the Nth call, asserting clearQuotaCache
eventually removes the file and resolves; write another test that stubs the same
function to always throw EBUSY/EPERM (or to exceed the retry count) and assert
clearQuotaCache rejects/throws after retries are exhausted; ensure you use
vitest spies/mocks (vi.spyOn or vi.fn) and restore mocks between tests and still
call saveQuotaCache/getQuotaCachePath to create the target file so tests remain
deterministic and consistent with the existing test patterns.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 4093-4117: pendingMenuQuotaRefresh can race with
destructive/manage flows and recreate quota-cache after resetLocalState; fix by
ensuring destructive/manage entry points await or cancel the in-flight
pendingMenuQuotaRefresh Promise returned by refreshQuotaCacheForMenu (or expose
an abort mechanism from refreshQuotaCacheForMenu) before calling saveQuotaCache
or resetLocalState; update the code paths that perform reset/manage/delete to
check pendingMenuQuotaRefresh (and await .finally or call an abort API) so cache
writes cannot win the race, add a vitest regression in
test/codex-manager-cli.test.ts that triggers reset/manage while
refreshQuotaCacheForMenu is in flight to reproduce the issue, and when
introducing the queue/abort handling ensure retries handle EBUSY/429 and that
logging around these changes avoids leaking tokens/emails.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4028-4033: Log currently prints raw account labels using
formatAccountLabel(deleted.removedAccount,...), leaking PII; replace that call
with a masked/log-safe label generator (e.g., maskEmailLabel or
formatAccountLabelSafe) that redacts identifiable parts of
deleted.removedAccount while preserving the idx context, then use the masked
label in the console output instead of formatAccountLabel. Update the affected
test (test/codex-manager-cli.test.ts lines ~63-65) to assert the masked form is
logged (not the raw email), and run the vitest suite; also scan
lib/codex-manager.ts for any other console/log calls that use full emails or
tokens and switch them to the same masking helper.
In `@lib/destructive-actions.ts`:
- Around line 103-105: After removing an account with
nextStorage.accounts.splice(options.index, 1) in lib/destructive-actions.ts,
adjust any stored pointers that point past the removed slot (e.g., activeIndex
and entries in activeIndexByFamily) by decrementing values greater than
options.index before calling clampActiveIndices(nextStorage) and
saveAccounts(nextStorage); update the code paths that mutate storage (the splice
block around nextStorage/accounts) to perform this rebase, add a vitest in
test/destructive-actions.test.ts that deletes an earlier index and asserts
activeIndex and activeIndexByFamily were moved to the previous account, and
ensure your changes follow the repo guidelines by updating any affected queues
to handle EBUSY/429 and avoiding logs that leak tokens or emails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04f1d77f-c129-4fb0-9317-50bef940da6d
📒 Files selected for processing (9)
index.tslib/codex-manager.tslib/destructive-actions.tslib/quota-cache.tslib/storage.tstest/codex-manager-cli.test.tstest/destructive-actions.test.tstest/quota-cache.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). (2)
- GitHub Check: cubic · AI code reviewer
- 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/quota-cache.test.tstest/destructive-actions.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/quota-cache.tslib/destructive-actions.tslib/storage.tslib/codex-manager.ts
🪛 ast-grep (0.41.1)
lib/codex-manager.ts
[warning] 755-755: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((?:^|\\|)\\s*${windowLabel}\\s+(\\d{1,3})%, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
# Conflicts: # index.ts # lib/codex-manager.ts # lib/storage.ts # test/codex-manager-cli.test.ts # test/storage-flagged.test.ts # test/storage.test.ts
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/codex-manager.ts (1)
3743-3761:⚠️ Potential issue | 🔴 Criticaldo not pass a stale flagged snapshot into account deletion.
lib/codex-manager.ts:3743-3761andlib/codex-manager.ts:3955-3960thread theflaggedStoragesnapshot loaded before the menu render atlib/codex-manager.ts:3848intodeleteAccountAtIndex(). if another process updates flagged storage while the menu is open, this path writes the stale snapshot back and drops the newer flagged entries. letdeleteAccountAtIndex()load flagged storage at delete time, or reload it immediately before the call, and add a vitest for that race.proposed fix
-async function handleManageAction( - storage: AccountStorageV3, - menuResult: Awaited<ReturnType<typeof promptLoginMode>>, - flaggedStorage?: FlaggedAccountStorageV1, -): Promise<void> { +async function handleManageAction( + storage: AccountStorageV3, + menuResult: Awaited<ReturnType<typeof promptLoginMode>>, +): Promise<void> { @@ - const deleted = await deleteAccountAtIndex({ - storage, - index: idx, - flaggedStorage, - }); + const deleted = await deleteAccountAtIndex({ + storage, + index: idx, + });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: 3953-3960
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 3743 - 3761, The delete path is passing a stale flaggedStorage snapshot into deleteAccountAtIndex from handleManageAction (flaggedStorage loaded before menu render), which can overwrite newer changes; change the logic so deleteAccountAtIndex always reads current flagged storage itself or reload flaggedStorage immediately before calling deleteAccountAtIndex (remove the flaggedStorage argument from the stale call site in handleManageAction and/or update deleteAccountAtIndex to load fresh flagged data internally), and add a vitest that simulates concurrent flaggedStorage update while the menu is open to ensure the newer entries are not lost; reference functions/variables: handleManageAction, deleteAccountAtIndex, flaggedStorage, and the menu deleteAccountIndex code path.lib/storage.ts (1)
2004-2043:⚠️ Potential issue | 🟡 Minorlegacy paths aren't explicitly cleared, though reset marker blocks resurrection.
clearAccounts clears primary, wal, and backup candidates but not
currentLegacyProjectStoragePathorcurrentLegacyWorktreeStoragePath. if legacy files exist, migration deletes them (lib/storage.ts:922), but only during load. clearAccounts itself doesn't clean them up.the reset marker at lib/storage.ts:1652 prevents loading stale data, so physical legacy files pose no practical risk—but test coverage is missing. add a regression test in test/storage.test.ts that creates legacy files, calls clearAccounts, and verifies they're handled correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 2004 - 2043, clearAccounts currently removes primary, wal, and backup candidates but omits the legacy storage files referenced by currentLegacyProjectStoragePath and currentLegacyWorktreeStoragePath; update clearAccounts to also compute those two legacy paths for the same storage root (alongside getStoragePath(), getAccountsWalPath(), and getAccountsBackupRecoveryCandidatesWithDiscovery()) and call unlinkWithRetry on them with the same per-artifact error handling (preserve hadError logic and logging). Additionally add a regression test in test/storage.test.ts that creates files at currentLegacyProjectStoragePath and currentLegacyWorktreeStoragePath, invokes clearAccounts(), and asserts the legacy files no longer exist (or that clearAccounts returns true and no legacy artifacts remain).
🤖 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-3112: The call to deleteAccountAtIndex returns an updated
storage object in deleted.storage but the caller continues using the stale
workingStorage; after the deleteAccountAtIndex call (where you assign const
deleted = await deleteAccountAtIndex(...)), rebind the in-memory state by
setting workingStorage = deleted.storage so subsequent iterations use the
updated storage; apply the same fix at the other call site where
deleteAccountAtIndex is used (the call in codex-manager around the other loop)
and add a regression test that performs multiple sequential deletes in a loop
and asserts in-memory workingStorage matches the returned storage/disk state
after each delete.
- Around line 3163-3177: The reset branch in index.ts (the block that checks
menuResult.mode === "reset" and calls resetLocalState(),
invalidateAccountManagerCache(), and logs
DESTRUCTIVE_ACTION_COPY.resetLocalState) lacks the in-flight concurrency guard
used in lib/codex-manager.ts and has no tests; add the same "in-flight" guard
semantics used around the reset flow in lib/codex-manager.ts to prevent double
invocation, and update the index.ts reset path to return/skip when a reset is
already running. Then add regression tests in test/codex-manager-cli.test.ts
that cover: (1) successful reset logging (all flags true ->
DESTRUCTIVE_ACTION_COPY.resetLocalState.completed), (2) warning logging when
some flags are false, and (3) double-invocation protection (attempting a second
reset while the first is in-flight should be prevented/short-circuited). Ensure
the tests exercise resetLocalState(), invalidateAccountManagerCache(), and the
printed messages.
In `@lib/cli.ts`:
- Around line 210-214: The cancellation message for the delete-all flow is
inconsistent: the branch that uses promptDeleteAllTypedConfirm() logs "Delete
saved accounts cancelled." but the TTY branch logs "Delete all cancelled.";
update the TTY branch to use the same standardized wording ("Delete saved
accounts cancelled.") so both branches (the promptDeleteAllTypedConfirm() branch
and the TTY handling around the return { mode: "fresh", deleteAll: true } / the
other cancellation at lines ~273-277) report the identical cancellation message.
In `@lib/storage.ts`:
- Around line 182-200: The unlinkWithRetry function only retries on EPERM and
EBUSY; update its retry condition to also include EAGAIN to match
renameFileWithRetry, i.e., in lib/storage.ts add "EAGAIN" alongside "EPERM" and
"EBUSY" in the conditional that waits-and-retries, and ensure the backoff logic
remains the same; then add a regression test in test/storage.test.ts that
simulates a transient EAGAIN error on fs.unlink (flaky first few attempts, then
succeed) to verify unlinkWithRetry retries and eventually succeeds (mirror the
approach used for the rename EAGAIN test).
In `@lib/ui/auth-menu.ts`:
- Around line 683-690: The subtitle is built from caller-provided statusMessage
and normalizedSearch without sanitization, allowing control bytes/escape
sequences to corrupt the UI; update resolveStatusMessage and any place that
returns/uses normalizedSearch (e.g., promptSearchQuery and the code around
normalizedSearch at lib/ui/auth-menu.ts) to pass the raw string through
sanitizeTerminalText() before trimming/returning, ensuring both statusMessage()
results and normalizedSearch are sanitized (use the existing
sanitizeTerminalText function name as the sanitizer) so the subtitle only
contains cleaned text.
In `@test/auth-menu-hotkeys.test.ts`:
- Line 261: The test assertion using the regex /[\u0000\u0007\u007f]/ is
intentionally checking control-character removal but trips Biome's
noControlCharactersInRegex rule; fix it by adding a suppression comment
immediately above the assertion line containing the regex (the expect on the
variable strippedLabels) using: // biome-ignore
lint/suspicious/noControlCharactersInRegex: intentional test assertion; this
keeps the intent clear and preserves the original Unicode escapes used in the
test.
In `@test/codex-manager-cli.test.ts`:
- Around line 156-175: The test mock for DESTRUCTIVE_ACTION_COPY is missing the
typedConfirm and confirm string fields present in the real export; update the
mock inside the vi.mock call to include typedConfirm and confirm keys (matching
the real structure used by code paths that render typed confirmation prompts)
alongside the existing deleteSavedAccounts and resetLocalState entries so that
DESTRUCTIVE_ACTION_COPY.typedConfirm and DESTRUCTIVE_ACTION_COPY.confirm return
the expected prompt strings when referenced by the code under test.
In `@test/destructive-actions.test.ts`:
- Around line 31-248: Add a deterministic vitest that hits the codex-manager
serialization guard by invoking the CLI path that routes to the destructive
actions (use the codex-manager's in-flight guard / "in-flight" flag around
performing destructive actions) instead of importing lib/destructive-actions
directly; mock the underlying clear* helpers (clearAccountsMock,
clearFlaggedAccountsMock, clearQuotaCacheMock) with controlled deferred promises
so the first call remains pending, then call the same CLI entrypoint a second
time and assert it immediately rejects (or returns the guard error) while the
first is in-flight, finally resolve/reject the deferred promise to simulate
completion/exception and assert the in-flight flag is cleared so subsequent
calls succeed; reference the CLI entry functions you call (the manager's CLI
handlers that call deleteSavedAccounts and resetLocalState) and the mocked
helpers (clearAccountsMock, clearFlaggedAccountsMock, clearQuotaCacheMock) to
locate where to hook the deferred promises.
In `@test/storage.test.ts`:
- Around line 3293-3356: Add two deterministic vitest cases for clearQuotaCache:
one that simulates a transient Windows EBUSY (mock fs.unlink to throw a
NodeJS.ErrnoException with code "EBUSY" once then succeed) and assert
clearQuotaCache resolves true and unlink called twice; and one that simulates
repeated transient failures until retries are exhausted (mock fs.unlink to
always throw EPERM/EBUSY) and assert clearQuotaCache resolves false and unlink
was called the expected retry count; target the existing test suite around
clearQuotaCache/getQuotaCachePath and use vi.spyOn(fs, "unlink") with
mockImplementation and proper mockRestore to keep tests deterministic.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 3743-3761: The delete path is passing a stale flaggedStorage
snapshot into deleteAccountAtIndex from handleManageAction (flaggedStorage
loaded before menu render), which can overwrite newer changes; change the logic
so deleteAccountAtIndex always reads current flagged storage itself or reload
flaggedStorage immediately before calling deleteAccountAtIndex (remove the
flaggedStorage argument from the stale call site in handleManageAction and/or
update deleteAccountAtIndex to load fresh flagged data internally), and add a
vitest that simulates concurrent flaggedStorage update while the menu is open to
ensure the newer entries are not lost; reference functions/variables:
handleManageAction, deleteAccountAtIndex, flaggedStorage, and the menu
deleteAccountIndex code path.
In `@lib/storage.ts`:
- Around line 2004-2043: clearAccounts currently removes primary, wal, and
backup candidates but omits the legacy storage files referenced by
currentLegacyProjectStoragePath and currentLegacyWorktreeStoragePath; update
clearAccounts to also compute those two legacy paths for the same storage root
(alongside getStoragePath(), getAccountsWalPath(), and
getAccountsBackupRecoveryCandidatesWithDiscovery()) and call unlinkWithRetry on
them with the same per-artifact error handling (preserve hadError logic and
logging). Additionally add a regression test in test/storage.test.ts that
creates files at currentLegacyProjectStoragePath and
currentLegacyWorktreeStoragePath, invokes clearAccounts(), and asserts the
legacy files no longer exist (or that clearAccounts returns true and no legacy
artifacts remain).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 614264cc-2d12-4a08-b225-4bea65461b59
📒 Files selected for processing (17)
index.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
🧰 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/auth-menu-hotkeys.test.tstest/cli-auth-menu.test.tstest/release-main-prs-regression.test.tstest/quota-cache.test.tstest/storage.test.tstest/codex-manager-cli.test.tstest/cli.test.tstest/destructive-actions.test.tstest/storage-flagged.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/quota-cache.tslib/ui/copy.tslib/storage.tslib/destructive-actions.tslib/ui/auth-menu.tslib/codex-manager.ts
🪛 Biome (2.4.6)
test/auth-menu-hotkeys.test.ts
[error] 261-261: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
[error] 261-261: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (26)
test/release-main-prs-regression.test.ts (1)
202-202: lgtm — test correctly expects false on eperm.the updated assertion aligns with the new
clearFlaggedAccounts()return semantics where permission errors resolve tofalseinstead of rejecting. the subsequent assertions at lines 207-210 confirm the file persists and sync doesn't pick up stale data.lib/ui/copy.ts (1)
18-19: ui copy updated for reset flow.new
resetLocalStatelabel and updatedremoveAllAccountswording align with the destructive-actions module. the fallback prompt at line 134 correctly adds the(r) resetoption.lib/quota-cache.ts (1)
277-301: clearQuotaCache looks solid, but the trailing return is unreachable.the 5-retry loop with exponential backoff matches the pattern in
saveQuotaCacheatlib/quota-cache.ts:246-254. the lazy path resolution viagetQuotaCachePath()fixes the stale-path issue from earlier reviews.one nit: line 300
return falseis unreachable — the loop always returns inside (success, ENOENT, or exhausted retries on line 294). harmless but dead code.test coverage confirmed in
test/quota-cache.test.ts:108-184.test/quota-cache.test.ts (3)
108-140: good regression coverage for transient unlink failures.the parameterized test properly exercises both
EBUSYandEPERMretry paths. the mock at line 118-130 fails the first 2 attempts then delegates to the real unlink, verifying the retry loop eventually succeeds. assertion at line 135 confirms exactly 3 attempts.
142-184: exhausted-retry test correctly validates warning and return value.the mock at line 158-166 forces all 5 attempts to fail. assertions at lines 169-176 verify the function returns
false, attempts = 5, file still exists (because unlink was mocked), and the warning includes the error message.one note: line 171 asserts the file still exists via
fs.access, which is correct since the mock prevents actual deletion. the real behavior would be file-still-exists on exhausted retries.
83-106: lazy path resolution test addresses prior review feedback.this validates that
getQuotaCachePath()reflects env changes at runtime, fixing the staleQUOTA_CACHE_PATHmodule-scope issue flagged in earlier reviews. the test creates a second temp dir at line 87-89, verifies the path changes at line 95-96, and confirms save/clear target the new location.test/cli.test.ts (2)
176-217: good coverage for fallback reset confirmation and cancellation.tests at lines 176-185 verify the happy path where typing "RESET" confirms the reset action. lines 203-217 verify cancellation when confirmation doesn't match, including the correct cancellation message
"Reset local state cancelled.". these align with the fallback flow inlib/cli.ts:216-222.
219-321: tty reset-all flow tested with proper mock setup.the tests correctly mock
isTTYandshowAuthMenuto exercise the tty path. line 246 mocks the typed confirm, line 299 tests the cancellation path. both restore stdin/stdout iTTY in the finally block.test/auth-menu-hotkeys.test.ts (1)
201-219: confirm prompt removal validated for delete-all and reset-all.these tests verify the pr objective: typed confirmations are now the single safety gate in the cli layer, so the tui's
showAuthMenureturns the action directly without callingconfirm(). line 208 and 218 assertconfirmMockis not called.test/cli-auth-menu.test.ts (1)
297-323: reset-all tty flow tested for both confirm and cancel paths.lines 297-306 verify typing "RESET" confirms the action and closes readline. lines 308-323 verify mismatched confirmation logs
"\nReset local state cancelled.\n"and falls back to cancel. these align withlib/cli.ts:278-283.lib/cli.ts (3)
161-171: promptResetTypedConfirm follows the established pattern.mirrors
promptDeleteAllTypedConfirmat lines 149-158. usesDESTRUCTIVE_ACTION_COPY.resetLocalState.typedConfirmfor the prompt, checks for exact "RESET" match, and properly closes the readline interface infinally.
216-222: fallback reset path now requires typed confirmation.this addresses the past review comment about the fallback bypassing destructive safety. line 217 calls
promptResetTypedConfirm(), and line 218 uses consistent cancellation message"Reset local state cancelled.".test coverage in
test/cli.test.ts:203-217.
278-283: tty reset-all path uses consistent cancellation message.line 280 now logs
"Reset local state cancelled."which matches the fallback at line 218, addressing the past review comment about message inconsistency.test/codex-manager-cli.test.ts (3)
3209-3240: test correctly validates cli call site.the mock-based approach is appropriate for verifying the cli layer passes the correct index to
deleteAccountAtIndex. the actual index rebase regression test should live intest/destructive-actions.test.tswhere the real module logic is exercised.
3446-3517: good concurrency regression test.the deferred-promise approach makes the test deterministic while exercising the
destructiveActionInFlightguard. this validates that concurrent destructive actions are properly serialized at the cli layer.
3362-3444: solid regression test for quota refresh ordering.validates that
resetLocalStatewaits for in-flight quota refresh to complete before clearing the cache. the invocation order assertion at lines 3436-3439 is a good check for the race condition fix.lib/storage.ts (3)
1857-1869: good refactor using shared rename helper.this addresses the past review comment about the main save path missing EAGAIN retries. using
renameFileWithRetrycentralizes the retry logic and ensures consistent handling of windows filesystem transients.
2267-2317: reset marker approach correctly prevents legacy flagged resurrection.even though the legacy flagged path (
openai-codex-blocked-accounts.json) isn't explicitly deleted,loadFlaggedAccountsatlib/storage.ts:2167returns empty when the reset marker exists. the conditional marker cleanup at lines 2301-2315 ensures the marker persists on partial failure, maintaining protection.
1947-1948: correct fix for transaction state consistency.
exportAccountsatlib/storage.ts:2338checkstransactionState?.activeto determine if it's running inside a transaction. addingactive: truehere ensureswithAccountAndFlaggedStorageTransactionbehaves consistently withwithAccountStorageTransaction.lib/destructive-actions.ts (5)
107-169: deleteAccountAtIndex handles rollback correctly.the shallow copy at line 117 is sufficient since we're only removing an account from the array, not mutating individual account objects. the
AggregateErrorat lines 148-157 preserves both failures when rollback also fails, addressing the past review comment.note:
saveAccountsinternally callswithStorageLock(lib/storage.ts:1994-1998), so the direct calls here are safe from concurrent storage mutations.
175-181: correct result contract.returns
flaggedCleared: falseandquotaCacheCleared: falsesince those operations aren't performed. this addresses the past review comment about lying result contracts.
15-36: clear user-facing copy.the typed confirmation prompts clearly enumerate what will be deleted vs kept, reducing accidental data loss. the
as constassertion enables proper type inference for the string literals.
187-197: [rewritten review comment]
[classification tag]
71-86: index rebase logic is correct and tested.
rebaseActiveIndicesAfterDeleteatlib/destructive-actions.ts:71-86decrements indices greater than the removed position beforeclampActiveIndicesbounds them. this prevents silent mis-targeting after splice. test coverage attest/destructive-actions.test.ts:92-138verifies the exact scenario: deleting index 0 withactiveIndex: 1andactiveIndexByFamily: { codex: 2, "gpt-5.x": 1 }correctly adjusts toactiveIndex: 0and{ codex: 1, "gpt-5.x": 0 }. rollback with AggregateError atlib/destructive-actions.ts:140-160handles double-failure correctly.index.ts (2)
104-109: good consolidation of destructive action imports.this keeps destructive flows and copy centralized, which reduces drift between menu paths. ref: lib/destructive-actions.ts:1-15.
3150-3157: good partial-failure messaging for delete-all.line 3153 reports warnings when cleanup is incomplete instead of claiming full success. this is the right behavior for windows file-lock edge cases. ref: lib/destructive-actions.ts:1-15.
|
@greptile @cubic review |
ndycode
changed the title
ndycode
changed the title
1 participant
Summary
lib/destructive-actions.tsand keep typedDELETE/RESETas the single destructive confirmation path for both TTY and fallback CLI flowsValidation
npm run typechecknpm run lintnpx vitest run --exclude test/oauth-server.integration.test.ts