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.
| const deleted = await deleteAccountAtIndex({ | ||
| storage: workingStorage, | ||
| index: menuResult.deleteAccountIndex, | ||
| flaggedStorage, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# inspect helper contract and returned shape
rg -nP -C4 '\bdeleteAccountAtIndex\b' lib/destructive-actions.ts
# inspect all call sites and how return values are consumed
rg -nP -C4 '\bdeleteAccountAtIndex\s*\(' index.ts lib/codex-manager.tsRepository: ndycode/codex-multi-auth
Length of output: 1286
🏁 Script executed:
# Get the full deleteAccountAtIndex function implementation and return type definition
rg -A 30 'export async function deleteAccountAtIndex' lib/destructive-actions.ts
# Check for DeleteAccountResult type definition
rg -B 5 -A 10 'type DeleteAccountResult|interface DeleteAccountResult' lib/destructive-actions.ts
# See how the deleted return value is actually used after line 3112
sed -n '3108,3150p' index.tsRepository: ndycode/codex-multi-auth
Length of output: 3331
🏁 Script executed:
# Check the codex-manager.ts call site more thoroughly
sed -n '3750,3780p' lib/codex-manager.tsRepository: ndycode/codex-multi-auth
Length of output: 965
🏁 Script executed:
# See what happens after line 3112 in the manage mode block
sed -n '3108,3135p' index.tsRepository: ndycode/codex-multi-auth
Length of output: 1288
rebind workingStorage with the updated storage after deleteAccountAtIndex returns.
the helper returns an updated storage object in deleted.storage (lib/destructive-actions.ts:107-138), but the caller ignores it and continues using the stale workingStorage. this leaves in-memory state out of sync with disk state. the function's design—explicitly returning the updated storage—signals the caller should use it. without rebinding, subsequent loop iterations operate on outdated state.
add workingStorage = deleted.storage; after the delete block (before line 3123).
same issue exists in lib/codex-manager.ts:3757. also add regression test for state consistency across delete operations in a loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.ts` around lines 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.
| if (menuResult.mode === "reset") { | ||
| startFresh = true; | ||
| const result = await resetLocalState(); | ||
| invalidateAccountManagerCache(); | ||
| console.log( | ||
| `\n${ | ||
| result.accountsCleared && | ||
| result.flaggedCleared && | ||
| result.quotaCacheCleared | ||
| ? DESTRUCTIVE_ACTION_COPY.resetLocalState.completed | ||
| : "Reset local state completed with warnings. Some local artifacts could not be removed; see logs." | ||
| }\n`, | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# verify current test coverage around reset/delete menu wiring
rg -nP -C3 'mode === "reset"|resetLocalState|deleteSavedAccounts|deleteAccountAtIndex'
# verify whether index-side destructive in-flight guard exists
rg -nP -C3 'destructiveActionInFlight|inFlight.*destructive|reset.*in.?flight' index.ts lib/codex-manager.tsRepository: ndycode/codex-multi-auth
Length of output: 25751
🏁 Script executed:
# Check which tests import/use index.ts reset path vs lib/codex-manager.ts
rg -n 'import.*index' test/ | grep -E 'test\.ts'
# Look for any tests that directly test index.ts menu flow
fd -e test.ts -path test/ -exec grep -l 'index\.ts\|runCodexMultiAuthCli.*reset' {} \;
# Check if index.ts and lib/codex-manager.ts reset paths are both active or conditional
rg -B5 -A5 'menuResult\.mode === "reset"' index.tsRepository: ndycode/codex-multi-auth
Length of output: 2973
🏁 Script executed:
# Check what test/codex-manager-cli.test.ts actually tests
rg -n 'it\(|describe\(' test/codex-manager-cli.test.ts | grep -i reset
# Search for all test cases with "reset" in the name
rg -n "it\(['\"].*reset.*['\"]" test/
# Check the actual imports in test/codex-manager-cli.test.ts
head -200 test/codex-manager-cli.test.ts | grep -E '^import|^const|runCodexMultiAuth'Repository: ndycode/codex-multi-auth
Length of output: 8768
🏁 Script executed:
# Check what runCodexMultiAuthCli actually tests - which code path does it use?
rg -B20 'describe.*auth.*login|describe.*reset' test/codex-manager-cli.test.ts | head -50
# Look for the actual definition/imports of runCodexMultiAuthCli
rg -n 'runCodexMultiAuthCli\s*=' test/codex-manager-cli.test.ts | head -5
# Check whether index.ts is ever called in tests or if it's lib/codex-manager.ts only
rg -n 'lib/codex-manager|index\.ts' test/codex-manager-cli.test.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 1583
🏁 Script executed:
# Verify if index.ts is actually exported or used as an entry point
rg -n 'export.*runCodexMultiAuthCli|export.*function.*login' index.ts | head -10
# Check if index.ts is the main entry point
cat package.json | grep -A3 '"main"'
# Look for tests that directly test index.ts auth/login flow (not via lib/codex-manager)
rg -n 'reset.*mode|menuResult.*reset' test/index.test.tsRepository: ndycode/codex-multi-auth
Length of output: 164
🏁 Script executed:
# Check what index.ts actually exports and if the reset path at 3163+ is reachable
sed -n '1,150p' index.ts | grep -E '^export|^import' | head -20
# Look at the context around line 3163 in index.ts to understand the function/context
sed -n '3100,3200p' index.ts | head -100Repository: ndycode/codex-multi-auth
Length of output: 4221
add regression tests and concurrency guard for index.ts reset path.
index.ts:3163-3177 implements a destructive reset branch without the in-flight guard present in lib/codex-manager.ts:3922-3950. add tests for both success + warning messaging and double-invocation protection. the reset path is currently untested; test/codex-manager-cli.test.ts exercises only the lib/codex-manager.ts flow. this matters if index.ts is reachable via plugin initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.ts` around lines 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.
| if (!(await promptDeleteAllTypedConfirm())) { | ||
| console.log("\nDelete saved accounts cancelled.\n"); | ||
| continue; | ||
| } | ||
| return { mode: "fresh", deleteAll: true }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
minor: cancellation message inconsistency between fallback and tty.
fallback at line 211 logs "Delete saved accounts cancelled." while tty at line 274 logs "Delete all cancelled.". consider aligning these for consistency — the pr elsewhere standardizes on "saved accounts" terminology.
🧹 align cancellation messages
case "fresh":
if (!(await promptDeleteAllTypedConfirm())) {
- console.log("\nDelete all cancelled.\n");
+ console.log("\nDelete saved accounts cancelled.\n");
continue;
}Also applies to: 273-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/cli.ts` around lines 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.
| 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") && attempt < 4) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt)); | ||
| continue; | ||
| } | ||
| throw unlinkError; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check if there's a vitest covering transient EAGAIN on unlink
rg -n "EAGAIN" --type ts test/Repository: ndycode/codex-multi-auth
Length of output: 731
🏁 Script executed:
# check the actual implementation of unlinkWithRetry and renameFileWithRetry
sed -n '182,200p' lib/storage.ts
echo "---"
sed -n '385,410p' lib/storage.ts
echo "---"
# also check around line 396 mentioned in review
sed -n '390,420p' lib/storage.tsRepository: ndycode/codex-multi-auth
Length of output: 2314
🏁 Script executed:
# search for test cases covering unlinkWithRetry
rg -n "unlinkWithRetry" test/ -A 5 -B 2Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# search for the exact test case mentioned in results for EAGAIN rename
sed -n '2800,2830p' test/storage.test.tsRepository: ndycode/codex-multi-auth
Length of output: 986
add EAGAIN to unlinkWithRetry retry conditions and add test coverage.
unlinkWithRetry at lib/storage.ts:182 only retries on EPERM or EBUSY, but renameFileWithRetry at lib/storage.ts:393 includes EAGAIN. while EAGAIN is less common for unlink than rename, it can occur on NFS and windows network drives. this creates an inconsistency: the rename path handles transient EAGAIN errors (with test coverage at test/storage.test.ts:2802) but the unlink path doesn't.
per guidelines, every change must cite affected tests. add both the EAGAIN condition and a regression test covering unlinkWithRetry with transient EAGAIN errors.
🔧 proposed fix
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") && attempt < 4) {
+ if ((code === "EPERM" || code === "EBUSY" || code === "EAGAIN") && attempt < 4) {
await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt));
continue;
}
throw unlinkError;
}
}
}📝 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.
| 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") && attempt < 4) { | |
| await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt)); | |
| continue; | |
| } | |
| throw unlinkError; | |
| } | |
| } | |
| } | |
| 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; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 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).
| const resolveStatusMessage = (): string | undefined => { | ||
| const raw = typeof options.statusMessage === "function" | ||
| ? options.statusMessage() | ||
| : options.statusMessage; | ||
| return typeof raw === "string" && raw.trim().length > 0 ? raw.trim() : undefined; | ||
| const raw = | ||
| typeof options.statusMessage === "function" | ||
| ? options.statusMessage() | ||
| : options.statusMessage; | ||
| return typeof raw === "string" && raw.trim().length > 0 | ||
| ? raw.trim() | ||
| : undefined; |
There was a problem hiding this comment.
sanitize subtitle input too.
lib/ui/auth-menu.ts:683-697 echoes both statusMessage and normalizedSearch directly into the subtitle, while promptSearchQuery() at lib/ui/auth-menu.ts:488-497 preserves pasted control bytes. that bypasses sanitizeTerminalText() at lib/ui/auth-menu.ts:99-105, so a caller-provided status string or pasted escape sequence can still corrupt the menu chrome.
proposed fix
- const statusText = resolveStatusMessage();
+ const statusText = sanitizeTerminalText(resolveStatusMessage());
if (statusText) {
parts.push(statusText);
}
@@
- parts.push(
- `${UI_COPY.mainMenu.searchSubtitlePrefix} ${normalizedSearch}`,
- );
+ const safeSearch = sanitizeTerminalText(normalizedSearch);
+ if (safeSearch) {
+ parts.push(
+ `${UI_COPY.mainMenu.searchSubtitlePrefix} ${safeSearch}`,
+ );
+ }Also applies to: 692-697
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/auth-menu.ts` around lines 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.
| expect(strippedLabels[0]).toContain("1. first@example.com"); | ||
| expect(strippedLabels[1]).toContain("2. Friendly Label"); | ||
| expect(strippedLabels[2]).toContain("3. acc-id-42"); | ||
| expect(strippedLabels.join("")).not.toMatch(/[\u0000\u0007\u007f]/); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
biome flags control characters in regex — suppress or escape.
the regex /[\u0000\u0007\u007f]/ is intentional here to assert sanitization removed control chars. biome's noControlCharactersInRegex rule flags this as suspicious.
options:
- add
// biome-ignore lint/suspicious/noControlCharactersInRegex: intentional test assertion - use hex escapes like
/[\x00\x07\x7f]/(biome may still flag this)
recommend option 1 since the intent is clear from context.
🧹 suppress biome lint warning
- expect(strippedLabels.join("")).not.toMatch(/[\u0000\u0007\u007f]/);
+ // biome-ignore lint/suspicious/noControlCharactersInRegex: intentional assertion that control chars are absent
+ expect(strippedLabels.join("")).not.toMatch(/[\u0000\u0007\u007f]/);📝 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.
| expect(strippedLabels.join("")).not.toMatch(/[\u0000\u0007\u007f]/); | |
| // biome-ignore lint/suspicious/noControlCharactersInRegex: intentional assertion that control chars are absent | |
| expect(strippedLabels.join("")).not.toMatch(/[\u0000\u0007\u007f]/); |
🧰 Tools
🪛 Biome (2.4.6)
[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)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/auth-menu-hotkeys.test.ts` at 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.
| vi.mock("../lib/destructive-actions.js", () => ({ | ||
| DESTRUCTIVE_ACTION_COPY: { | ||
| deleteSavedAccounts: { | ||
| label: "Delete Saved Accounts", | ||
| stage: "Deleting saved accounts only", | ||
| completed: | ||
| "Deleted saved accounts. Saved accounts deleted; flagged/problem accounts, settings, and Codex CLI sync state kept.", | ||
| }, | ||
| resetLocalState: { | ||
| label: "Reset Local State", | ||
| stage: | ||
| "Clearing saved accounts, flagged/problem accounts, and quota cache", | ||
| completed: | ||
| "Reset local state. Saved accounts, flagged/problem accounts, and quota cache cleared; settings and Codex CLI sync state kept.", | ||
| }, | ||
| }, | ||
| deleteSavedAccounts: deleteSavedAccountsMock, | ||
| resetLocalState: resetLocalStateMock, | ||
| deleteAccountAtIndex: deleteAccountAtIndexMock, | ||
| })); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
mock is missing typedConfirm and confirm fields from the real export.
the actual DESTRUCTIVE_ACTION_COPY at lib/destructive-actions.ts:15-36 includes typedConfirm and confirm strings used for typed confirmation prompts. if any code path starts relying on those fields during the test, this mock will return undefined instead of the expected string.
consider adding the missing fields to keep the mock in sync with the real structure, or explicitly document that this mock is intentionally minimal.
🔧 proposed fix
vi.mock("../lib/destructive-actions.js", () => ({
DESTRUCTIVE_ACTION_COPY: {
deleteSavedAccounts: {
label: "Delete Saved Accounts",
+ typedConfirm:
+ "Type DELETE to delete saved accounts only (saved accounts: delete; flagged/problem accounts, settings, and Codex CLI sync state: keep): ",
+ confirm:
+ "Delete saved accounts? (Saved accounts: delete. Flagged/problem accounts: keep. Settings: keep. Codex CLI sync state: keep.)",
stage: "Deleting saved accounts only",
completed:
"Deleted saved accounts. Saved accounts deleted; flagged/problem accounts, settings, and Codex CLI sync state kept.",
},
resetLocalState: {
label: "Reset Local State",
+ typedConfirm:
+ "Type RESET to reset local state (saved accounts + flagged/problem accounts: delete; settings + Codex CLI sync state: keep; quota cache: clear): ",
+ confirm:
+ "Reset local state? (Saved accounts: delete. Flagged/problem accounts: delete. Settings: keep. Codex CLI sync state: keep. Quota cache: clear.)",
stage:
"Clearing saved accounts, flagged/problem accounts, and quota cache",
completed:
"Reset local state. Saved accounts, flagged/problem accounts, and quota cache cleared; settings and Codex CLI sync state kept.",
},
},
deleteSavedAccounts: deleteSavedAccountsMock,
resetLocalState: resetLocalStateMock,
deleteAccountAtIndex: deleteAccountAtIndexMock,
}));📝 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.
| vi.mock("../lib/destructive-actions.js", () => ({ | |
| DESTRUCTIVE_ACTION_COPY: { | |
| deleteSavedAccounts: { | |
| label: "Delete Saved Accounts", | |
| stage: "Deleting saved accounts only", | |
| completed: | |
| "Deleted saved accounts. Saved accounts deleted; flagged/problem accounts, settings, and Codex CLI sync state kept.", | |
| }, | |
| resetLocalState: { | |
| label: "Reset Local State", | |
| stage: | |
| "Clearing saved accounts, flagged/problem accounts, and quota cache", | |
| completed: | |
| "Reset local state. Saved accounts, flagged/problem accounts, and quota cache cleared; settings and Codex CLI sync state kept.", | |
| }, | |
| }, | |
| deleteSavedAccounts: deleteSavedAccountsMock, | |
| resetLocalState: resetLocalStateMock, | |
| deleteAccountAtIndex: deleteAccountAtIndexMock, | |
| })); | |
| vi.mock("../lib/destructive-actions.js", () => ({ | |
| DESTRUCTIVE_ACTION_COPY: { | |
| deleteSavedAccounts: { | |
| label: "Delete Saved Accounts", | |
| typedConfirm: | |
| "Type DELETE to delete saved accounts only (saved accounts: delete; flagged/problem accounts, settings, and Codex CLI sync state: keep): ", | |
| confirm: | |
| "Delete saved accounts? (Saved accounts: delete. Flagged/problem accounts: keep. Settings: keep. Codex CLI sync state: keep.)", | |
| stage: "Deleting saved accounts only", | |
| completed: | |
| "Deleted saved accounts. Saved accounts deleted; flagged/problem accounts, settings, and Codex CLI sync state kept.", | |
| }, | |
| resetLocalState: { | |
| label: "Reset Local State", | |
| typedConfirm: | |
| "Type RESET to reset local state (saved accounts + flagged/problem accounts: delete; settings + Codex CLI sync state: keep; quota cache: clear): ", | |
| confirm: | |
| "Reset local state? (Saved accounts: delete. Flagged/problem accounts: delete. Settings: keep. Codex CLI sync state: keep. Quota cache: clear.)", | |
| stage: | |
| "Clearing saved accounts, flagged/problem accounts, and quota cache", | |
| completed: | |
| "Reset local state. Saved accounts, flagged/problem accounts, and quota cache cleared; settings and Codex CLI sync state kept.", | |
| }, | |
| }, | |
| deleteSavedAccounts: deleteSavedAccountsMock, | |
| resetLocalState: resetLocalStateMock, | |
| deleteAccountAtIndex: deleteAccountAtIndexMock, | |
| })); |
🤖 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 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.
| describe("destructive actions", () => { | ||
| beforeEach(() => { | ||
| vi.resetModules(); | ||
| vi.clearAllMocks(); | ||
| clearAccountsMock.mockResolvedValue(true); | ||
| clearFlaggedAccountsMock.mockResolvedValue(true); | ||
| clearQuotaCacheMock.mockResolvedValue(true); | ||
| loadFlaggedAccountsMock.mockResolvedValue({ version: 1, accounts: [] }); | ||
| saveAccountsMock.mockResolvedValue(undefined); | ||
| saveFlaggedAccountsMock.mockResolvedValue(undefined); | ||
| }); | ||
|
|
||
| it("returns delete-only results without pretending kept data was cleared", async () => { | ||
| const { deleteSavedAccounts } = await import( | ||
| "../lib/destructive-actions.js" | ||
| ); | ||
|
|
||
| await expect(deleteSavedAccounts()).resolves.toEqual({ | ||
| accountsCleared: true, | ||
| flaggedCleared: false, | ||
| quotaCacheCleared: false, | ||
| }); | ||
| expect(clearAccountsMock).toHaveBeenCalledTimes(1); | ||
| expect(clearFlaggedAccountsMock).not.toHaveBeenCalled(); | ||
| expect(clearQuotaCacheMock).not.toHaveBeenCalled(); | ||
| expect(clearCodexCliStateCacheMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("returns reset results and clears Codex CLI state", async () => { | ||
| clearAccountsMock.mockResolvedValueOnce(true); | ||
| clearFlaggedAccountsMock.mockResolvedValueOnce(false); | ||
| clearQuotaCacheMock.mockResolvedValueOnce(true); | ||
|
|
||
| const { resetLocalState } = await import("../lib/destructive-actions.js"); | ||
|
|
||
| await expect(resetLocalState()).resolves.toEqual({ | ||
| accountsCleared: true, | ||
| flaggedCleared: false, | ||
| quotaCacheCleared: true, | ||
| }); | ||
| expect(clearAccountsMock).toHaveBeenCalledTimes(1); | ||
| expect(clearFlaggedAccountsMock).toHaveBeenCalledTimes(1); | ||
| expect(clearQuotaCacheMock).toHaveBeenCalledTimes(1); | ||
| expect(clearCodexCliStateCacheMock).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("does not clear Codex CLI state when resetLocalState aborts on an exception", async () => { | ||
| const resetError = Object.assign(new Error("flagged clear failed"), { | ||
| code: "EPERM", | ||
| }); | ||
| clearFlaggedAccountsMock.mockRejectedValueOnce(resetError); | ||
|
|
||
| const { resetLocalState } = await import("../lib/destructive-actions.js"); | ||
|
|
||
| await expect(resetLocalState()).rejects.toBe(resetError); | ||
| expect(clearAccountsMock).toHaveBeenCalledTimes(1); | ||
| expect(clearFlaggedAccountsMock).toHaveBeenCalledTimes(1); | ||
| expect(clearQuotaCacheMock).not.toHaveBeenCalled(); | ||
| expect(clearCodexCliStateCacheMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("re-bases active indices before clamping when deleting an earlier account", async () => { | ||
| const { deleteAccountAtIndex } = await import( | ||
| "../lib/destructive-actions.js" | ||
| ); | ||
|
|
||
| const storage = { | ||
| version: 3, | ||
| activeIndex: 1, | ||
| activeIndexByFamily: { codex: 2, "gpt-5.x": 1 }, | ||
| accounts: [ | ||
| { | ||
| refreshToken: "refresh-remove", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| { | ||
| refreshToken: "refresh-active", | ||
| addedAt: 2, | ||
| lastUsed: 2, | ||
| }, | ||
| { | ||
| refreshToken: "refresh-other", | ||
| addedAt: 3, | ||
| lastUsed: 3, | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| const deleted = await deleteAccountAtIndex({ storage, index: 0 }); | ||
|
|
||
| expect(deleted).not.toBeNull(); | ||
| expect(deleted?.storage.accounts.map((account) => account.refreshToken)).toEqual([ | ||
| "refresh-active", | ||
| "refresh-other", | ||
| ]); | ||
| expect(deleted?.storage.activeIndex).toBe(0); | ||
| expect(deleted?.storage.activeIndexByFamily).toEqual({ | ||
| codex: 1, | ||
| "gpt-5.x": 0, | ||
| }); | ||
| expect(saveAccountsMock).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 1, "gpt-5.x": 0 }, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it("rethrows the original flagged-save failure after a successful rollback", async () => { | ||
| const flaggedSaveError = Object.assign(new Error("flagged save failed"), { | ||
| code: "EPERM", | ||
| }); | ||
| 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, | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| await expect(deleteAccountAtIndex({ storage, index: 1 })).rejects.toBe( | ||
| flaggedSaveError, | ||
| ); | ||
| expect(saveAccountsMock).toHaveBeenCalledTimes(2); | ||
| expect(storage.accounts).toHaveLength(2); | ||
| }); | ||
|
|
||
| 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.
cover the new in-flight guard in the cli path.
test/destructive-actions.test.ts:31-248 only exercises lib/destructive-actions.ts; it never hits the serialization guard added at lib/codex-manager.ts:3896-3949. there is still no deterministic vitest proving a second delete/reset request is rejected while the first action is pending, or that the flag is cleared after an exception.
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/destructive-actions.test.ts` around lines 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.
| describe("clearQuotaCache", () => { | ||
| const tmpRoot = join( | ||
| tmpdir(), | ||
| `quota-cache-test-${Math.random().toString(36).slice(2)}`, | ||
| ); | ||
| let originalDir: string | undefined; | ||
|
|
||
| beforeEach(async () => { | ||
| originalDir = process.env.CODEX_MULTI_AUTH_DIR; | ||
| process.env.CODEX_MULTI_AUTH_DIR = tmpRoot; | ||
| await fs.mkdir(tmpRoot, { recursive: true }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| if (originalDir === undefined) delete process.env.CODEX_MULTI_AUTH_DIR; | ||
| else process.env.CODEX_MULTI_AUTH_DIR = originalDir; | ||
| await fs.rm(tmpRoot, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it("removes only the quota cache file", async () => { | ||
| const quotaPath = getQuotaCachePath(); | ||
| const accountsPath = join(tmpRoot, "openai-codex-accounts.json"); | ||
| await fs.mkdir(dirname(quotaPath), { recursive: true }); | ||
| await fs.writeFile(quotaPath, "{}", "utf-8"); | ||
| await fs.writeFile(accountsPath, "{}", "utf-8"); | ||
|
|
||
| expect(existsSync(quotaPath)).toBe(true); | ||
| expect(existsSync(accountsPath)).toBe(true); | ||
|
|
||
| await expect(clearQuotaCache()).resolves.toBe(true); | ||
|
|
||
| expect(existsSync(quotaPath)).toBe(false); | ||
| expect(existsSync(accountsPath)).toBe(true); | ||
| }); | ||
|
|
||
| it("ignores missing quota cache file", async () => { | ||
| await expect(clearQuotaCache()).resolves.toBe(true); | ||
| }); | ||
|
|
||
| it("retries transient EPERM when clearing the quota cache", async () => { | ||
| 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 = "EPERM"; | ||
| 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.
add the missing ebusy and exhausted-retry cases for quota-cache clears.
test/storage.test.ts:3293-3356 only covers a missing file and a single eperm recovery. the new quota-cache delete path still has no windows ebusy regression and no assertion that exhausted retries resolve false, even though the same file already treats those as required behavior for account and flagged storage at test/storage.test.ts:1788-1910.
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 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.
1 participant
Summary
Reset Local Stateclears local files so stale quota writes cannot recreatequota-cache.jsonAccount N, and aligning destructive-action mocks with the shipped contractDELETE/RESETas the single destructive safety gate in the TUI and add regression coverage forresetLocalState(),clearQuotaCache()retries, flagged-reset suppression, and terminal-text sanitizationValidation
npm run typechecknpm run lintnpm testnote: 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 flows into
lib/destructive-actions.ts, wires up the typedDELETE/RESETgate as the single safety gate in the tui, and closes the windows filesystem concurrency gaps that were flagged in previous reviews.key changes and their safety reasoning:
deleteAccountAtIndex— creates a deep copy (nextStorage) before mutating, callssaveAccountsthensaveFlaggedAccountsin that order. onsaveFlaggedAccountsfailure, rollback callssaveAccounts(previousStorage); both errors are preserved in anAggregateErrorif rollback also fails (eperm/ebusy race on windows). the originalstoragearg is never mutated, so callers that reload from disk on the next loop iteration (as bothindex.tsandcodex-manager.tsinner loops do) are safe.clearQuotaCache— now has the same 5-attempt ebusy/eperm exponential backoff assaveQuotaCache, closing the asymmetry that was previously flagged. returnsbooleansoresetLocalStatecan surface partial-reset warnings.clearAccounts/clearFlaggedAccounts— both now useunlinkWithRetryand returnbooleaninstead of throwing; the reset marker is only deleted from disk when all artifact removals succeed, preserving the "soft clear" suppression on locked files.resetLocalStateincodex-manager.tscaptures and awaitspendingMenuQuotaRefreshbefore callingresetLocalState(), preventing stale quota writes from recreatingquota-cache.jsonafter the reset.destructiveActionInFlightguard — module-level flag prevents a second concurrent destructive action from racing through while the first is running; reset infinallyso it can never stick.confirm()dialog was removed fromshowAuthMenufordelete-all/reset-all; the typedDELETE/RESETprompt inpromptLoginModeis now the only gate, consistent across tui and fallback paths.Account Ninstead of the email, preventing token/email leakage in console output or logs.Confidence Score: 4/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant TUI as auth-menu (TUI) participant CLI as cli.ts (promptLoginMode) participant CM as codex-manager.ts participant DA as destructive-actions.ts participant ST as storage.ts participant QC as quota-cache.ts User->>TUI: selects "Reset Local State" Note over TUI: no confirm() — returns reset-all action TUI-->>CLI: { type: "reset-all" } CLI->>User: "Type RESET to confirm" User-->>CLI: "RESET" CLI-->>CM: { mode: "reset" } alt destructiveActionInFlight = true CM-->>User: "Another destructive action running. Wait." else destructiveActionInFlight = false CM->>CM: destructiveActionInFlight = true CM->>CM: await pendingMenuQuotaRefresh (if in-flight) CM->>DA: resetLocalState() DA->>ST: clearAccounts() → boolean Note over ST: unlinkWithRetry x5 (EPERM/EBUSY) DA->>ST: clearFlaggedAccounts() → boolean Note over ST: unlinkWithRetry x5 (EPERM/EBUSY) DA->>QC: clearQuotaCache() → boolean Note over QC: unlink retry x5 (EPERM/EBUSY) DA->>DA: clearCodexCliStateCache() DA-->>CM: { accountsCleared, flaggedCleared, quotaCacheCleared } CM->>User: success message (or warnings if any false) CM->>CM: destructiveActionInFlight = false end User->>TUI: selects "Delete Saved Accounts" TUI-->>CLI: { type: "delete-all" } CLI->>User: "Type DELETE to confirm" User-->>CLI: "DELETE" CLI-->>CM: { mode: "fresh", deleteAll: true } CM->>DA: deleteSavedAccounts() DA->>ST: clearAccounts() → boolean DA-->>CM: { accountsCleared, flaggedCleared: false, quotaCacheCleared: false } User->>TUI: selects account → "Delete Account" TUI->>User: confirm("Delete Account N?") User-->>TUI: yes TUI-->>CLI: { type: "delete-account" } CLI-->>CM: { mode: "manage", deleteAccountIndex: N } CM->>DA: deleteAccountAtIndex({ storage, index, flaggedStorage }) DA->>ST: saveAccounts(nextStorage) alt removedFlaggedCount > 0 DA->>ST: saveFlaggedAccounts(updatedFlagged) alt saveFlaggedAccounts throws DA->>ST: saveAccounts(previousStorage) [rollback] alt rollback also throws DA-->>CM: throw AggregateError([original, rollback]) else rollback ok DA-->>CM: throw original error end end end DA-->>CM: { storage, flagged, removedAccount, removedFlaggedCount }Comments Outside Diff (2)
index.ts, line 3112-3127 (link)Stale in-memory
workingStorageafter deletiondeleteAccountAtIndexcreates a freshnextStorageobject internally and saves it, but it does NOT mutateoptions.storage. the old code calledworkingStorage.accounts.splice(...)directly, keeping the caller's reference up-to-date. now, aftercontinue,workingStorage.accountsstill contains the deleted account — disk is correct but the in-memory state is stale for the rest of this loop iteration.fix: propagate
deleted.storageback after the call:same gap applies to
codex-manager.tsinsidehandleManageAction(see that file). the bug surfaces whenever the enclosing loop re-renders the menu usingworkingStorage/currentStoragewithout reloading from disk first.lib/codex-manager.ts, line 3756-3768 (link)handleManageActionleaves caller'sstoragestale after deletionsame root cause as
index.ts:deleteAccountAtIndexwrites a freshnextStorageto disk without mutatingoptions.storage. the old code didstorage.accounts.splice(idx, 1)and updatedactiveIndex/activeIndexByFamilyin-place, socurrentStorageinrunAuthLoginwas always up-to-date. now it isn't.handleManageActionreturnsvoid, so the caller has no way to getdeleted.storageback. one safe fix is to mutate the object that was passed in so the caller's reference stays current:alternatively, change
handleManageActionto return{ storage, flagged }and letrunAuthLoginassign both back — that's the cleaner contract and it would also fix the pre-existingflaggedStoragestale-display window noted on this PR.Prompt To Fix All With AI
Last reviewed commit: b0c83b9
Context used: