feat(auth): snapshot before destructive actions#113
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthroughwalkthroughadds a transactional, snapshot-first destructive-actions module and a large expansion of storage snapshot/backup APIs (named and rotating backups, snapshot/restore, export/import, helpers). see changes
sequence diagram(s)sequenceDiagram
participant client as Client/App
participant snap as SnapshotSystem
participant trans as TransactionHandler
participant storage as AccountStorage
participant fs as BackupFS
client->>snap: snapshotAccountStorage(reason)
snap->>storage: read current storage
snap->>fs: write named backup
snap-->>client: NamedBackupMetadata
client->>trans: withAccountAndFlaggedStorageTransaction()
trans->>storage: clone current state (snapshot)
trans->>trans: apply destructive mutation (delete/reset)
trans->>storage: saveAccounts() and saveFlaggedAccounts()
trans-->>client: commit or error (rollback if failed)
estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes flags for review:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
ndycode
force-pushed
the
fresh/14-auto-snapshots
branch
from
0fd695e to
97a02c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/destructive-actions.test.ts (1)
135-193:⚠️ Potential issue | 🟠 Majoradd the duplicate-refresh-token delete regression.
test/destructive-actions.test.ts:135-193 only exercises unique-token accounts. lib/destructive-actions.ts:109-117 now re-resolves the target inside the transaction, while lib/storage.ts:1538-1562 explicitly treats shared refresh tokens as ambiguous when
accountIdorAs 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 135 - 193, Add a deterministic vitest that covers the duplicate-refresh-token regression by creating a storage fixture with two accounts sharing the same refreshToken and a third distinct account, then call deleteAccountAtIndex for the intended duplicate index (use the same structuredClone/transactionCurrentStorage pattern as the existing test), and assert that the function deletes only the targeted account (check deleted.storage.accounts refreshToken order and lengths), that activeIndex and activeIndexByFamily are re-based correctly, and that snapshotAccountStorageMock and saveAccountsMock are called in the same order and with the expected updated storage; ensure the test uses only fixed values (no real secrets), uses vitest assertions, and fails if a wrong-row delete occurs so the concurrent/ambiguous-token path in deleteAccountAtIndex and lib/storage.ts behavior is exercised.
🤖 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 109-117: The transaction re-finds the target account by
refreshToken only (in the withAccountAndFlaggedStorageTransaction callback),
which can delete the wrong row when refresh-token collisions exist; replace that
lookup with the storage-safe identity matcher by importing and using
findMatchingAccountIndex from ./storage.js to locate the exact account (use the
same identity object/criteria the UI passed rather than refreshToken alone),
ensure you still return null when not found, and avoid logging any tokens or
emails; also add a vitest in test/destructive-actions.test.ts next to the
existing delete tests that simulates two rows sharing a refreshToken but
different identities to assert the correct row is deleted.
In `@lib/storage.ts`:
- Around line 2175-2194: The named-snapshot write path uses bare
fs.mkdir/fs.writeFile and can fail transiently (EBUSY/EPERM) allowing
destructive flows to proceed without a backup; update the snapshot creation code
that builds backupPath (uses getStoragePath and resolveNamedBackupPath) to reuse
the existing retry/backoff logic found in the other filesystem retry block (the
code at lib/storage.ts:2028-2046) so both mkdir and writeFile are retried on
transient errors (handle EBUSY/EPERM and similar), ensure retries use the same
backoff and max attempts, log failures without leaking sensitive data, and
crucially propagate a thrown error on exhaustion (do not swallow and return
null) so callers like deleteAccountAtIndex and snapshotAndClearAccounts fail
closed; run/adjust the vitests referenced (storage.test.ts and
destructive-actions.test.ts) to expect failure when snapshot retries are
exhausted.
- Around line 3363-3380: clearAccountsUnlocked currently writes the reset marker
file (via getIntentionalResetMarkerPath) without ensuring the parent directory
exists, causing ENOENT on fresh storage paths; before calling fs.writeFile in
clearAccountsUnlocked (and similar flows used by clearAccounts(),
snapshotAndClearAccounts(), deleteSavedAccounts(), resetLocalState(), and
setStoragePathDirect()), create the parent directory
(dirname(getIntentionalResetMarkerPath(storagePath))) with fs.mkdir(..., {
recursive: true, mode: 0o700 }) and then write the file, preserving the intended
file mode (0o600) and avoiding races; add a Vitest that invokes clearAccounts()
or snapshotAndClearAccounts() against a storagePath whose parent directory does
not exist to assert no ENOENT is thrown and update/mention test/storage.test.ts
for the new test; ensure logging changes do not leak tokens/emails and consider
retry/backoff for transient EBUSY/429 in related IO queues.
In `@test/storage.test.ts`:
- Around line 2439-2512: Add a deterministic Vitest regression that flips
process.platform to "win32" and reproduces the transient named-backup write
failure: in the existing test that spies on fs.writeFile (the mock around
saveAccounts/deleteSavedAccounts), add a branch that for the first call whose
path includes "accounts-delete-saved-accounts-snapshot-" throws an error with
code "EPERM" or "EBUSY" (simulating Windows transient lock) and on subsequent
attempts delegates to originalWriteFile; ensure you restore process.platform and
the writeFile spy in finally; keep using saveAccounts, deleteSavedAccounts,
loadAccounts and getNamedBackupsDirectoryPath so the test verifies the same
assertions after the transient failure (backup created after retry and
concurrent save succeeds).
---
Outside diff comments:
In `@test/destructive-actions.test.ts`:
- Around line 135-193: Add a deterministic vitest that covers the
duplicate-refresh-token regression by creating a storage fixture with two
accounts sharing the same refreshToken and a third distinct account, then call
deleteAccountAtIndex for the intended duplicate index (use the same
structuredClone/transactionCurrentStorage pattern as the existing test), and
assert that the function deletes only the targeted account (check
deleted.storage.accounts refreshToken order and lengths), that activeIndex and
activeIndexByFamily are re-based correctly, and that snapshotAccountStorageMock
and saveAccountsMock are called in the same order and with the expected updated
storage; ensure the test uses only fixed values (no real secrets), uses vitest
assertions, and fails if a wrong-row delete occurs so the
concurrent/ambiguous-token path in deleteAccountAtIndex and lib/storage.ts
behavior is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4253248a-4960-45c4-a3d4-0591cabf28cb
📒 Files selected for processing (4)
lib/destructive-actions.tslib/storage.tstest/destructive-actions.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/destructive-actions.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.tslib/destructive-actions.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
3668-3701:⚠️ Potential issue | 🟠 Majorclear the legacy flagged store during reset.
lib/storage.ts:3668-3701only unlinks the primary flagged file and its backups.loadFlaggedAccounts()inlib/storage.ts:3583-3606will still migrategetLegacyFlaggedAccountsPath()back on the next read, soresetLocalState()can resurrect flagged/problem accounts on older installs.test/destructive-actions.test.ts:102-137mocksclearFlaggedAccounts(), so this regression still has no real coverage; please add a vitest intest/storage.test.tsthat starts with only the legacy flagged file present.proposed fix
export async function clearFlaggedAccounts(): Promise<boolean> { return withStorageLock(async () => { const path = getFlaggedAccountsPath(); + const legacyPath = getLegacyFlaggedAccountsPath(); const markerPath = getIntentionalResetMarkerPath(path); @@ - for (const candidate of [path, ...backupPaths]) { + for (const candidate of [path, legacyPath, ...backupPaths]) {As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 3668 - 3701, clearFlaggedAccounts currently removes only the primary flagged file and backups, but loadFlaggedAccounts can migrate the legacy file (getLegacyFlaggedAccountsPath) back on next read; update clearFlaggedAccounts() to also unlink the legacy flagged path returned by getLegacyFlaggedAccountsPath() (and any backup candidates discovered for it) using the same unlinkWithRetry/error handling pattern, ensure logs (in clearFlaggedAccounts and related error branches) do not contain tokens/emails, and add a vitest in test/storage.test.ts that creates only the legacy flagged file, calls resetLocalState()/clearFlaggedAccounts(), then asserts the legacy file is removed and loadFlaggedAccounts no longer migrates it; reference functions: clearFlaggedAccounts, loadFlaggedAccounts, getLegacyFlaggedAccountsPath, unlinkWithRetry, and resetLocalState for locating code and tests.
♻️ Duplicate comments (1)
test/storage.test.ts (1)
2478-2598:⚠️ Potential issue | 🟠 Majormissing a combined win32 regression for transient snapshot failure under concurrency.
test/storage.test.ts:2478-2598 validates lock ordering and EPERM retry separately, but it still does not pin the windows-specific transient snapshot-write branch under concurrent save pressure in lib/storage.ts:3409-3422. please add one deterministic vitest that sets win32, forces first snapshot write to fail with EPERM/EBUSY, then verifies retry + concurrent save ordering in the same flow.
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 2478 - 2598, Add a deterministic vitest that reproduces the Windows transient snapshot-failure under concurrency: stub process.platform to "win32", spy/mock fs.writeFile so the first write to the "accounts-delete-saved-accounts-snapshot-" path throws a NodeJS.ErrnoException with code "EPERM" (or "EBUSY") and subsequent attempts succeed, then start deleteSavedAccounts() and while its snapshot write is failing start a concurrent saveAccounts() call; assert deleteSavedAccounts resolves with accountsCleared true, the snapshot write was retried (attempts count == 2), the concurrent save resolves, loadAccounts() contains the saved-after-delete account, and a snapshot file exists via getNamedBackupsDirectoryPath(); restore process.platform and mocked fs.writeFile in finally. Reference deleteSavedAccounts, saveAccounts, loadAccounts, getNamedBackupsDirectoryPath, and the fs.writeFile spy in your test.
🤖 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 110-130: The transactional handler uses current ?? options.storage
which allows falling back to caller memory; change the flow in
withAccountAndFlaggedStorageTransaction callback so if current is null the
delete path aborts immediately (return null or throw) and do not call
snapshotAccountStorage, loadFlaggedAccounts, findMatchingAccountIndex or mutate
accounts; update the delete-account branch in destructive-actions.ts to check
for current === null before computing sourceStorage/targetIndex and bail out,
and add a vitest in test/destructive-actions.test.ts that simulates the
transaction providing null for current and asserts the delete operation aborts
(no snapshot or write occurs).
In `@lib/storage.ts`:
- Around line 2672-2693: The pre-import snapshot currently defaults
snapshotFailurePolicy to "warn", allowing importNormalizedAccounts to proceed
after snapshot write errors; change the behavior so snapshot failures abort the
import (fail-closed) by making importNormalizedAccounts enforce failure on
snapshot errors (either default snapshotFailurePolicy to "fail" or detect
eprem/ebusy/EBUSY/429 errors from snapshotAccountStorage and throw to abort the
transaction) so no state is mutated without a recovery point; update references
in importNormalizedAccounts and ensure snapshotAccountStorage error handling is
adjusted accordingly, and add a Vitest in test/storage.test.ts that simulates
the Win32 pre-import snapshot failure (EBUSY/eprem) to assert the import is
aborted and no accounts are merged.
- Around line 2892-2894: The cleanup call before every load is deleting live
rotate temp files from concurrent rotations; modify the logic so
cleanupStaleRotatingBackupArtifacts(path) is NOT run unconditionally inside
loadAccounts — instead run cleanup only at startup or when the temp-file is
older than a safe threshold and skip files matching the in-progress pattern
(e.g., ".rotate.*.tmp"); update the code around getStoragePath,
getIntentionalResetMarkerPath and the loadAccounts callsite to only invoke
cleanup under those conditions (preserve createRotatingAccountsBackup()
behavior), and add a new real-FS vitest at test/storage.test.ts (referencing
test/destructive-actions.test.ts for context) that creates a fresh
".rotate.*.tmp" next to storage and asserts loadAccounts() does not remove it;
ensure changes consider Windows/EBUSY file semantics and mention in the test
that EBUSY/429 retries are handled and that no logs leak tokens/emails.
In `@test/storage.test.ts`:
- Around line 2861-2872: Replace the placeholder expect(true) test with a
deterministic vitest that exercises the migration path in lib/storage.ts:
arrange by creating a temporary test storage directory and writing the old
filename "openai-codex-accounts.json" with sample account data, ensure
getStoragePath()/loadAccounts() (the functions under test) are invoked to
trigger migration to "codex-accounts.json", then assert the new file exists with
correct contents, the old file has been removed, and that subsequent calls read
from the new filename (persisted migration); ensure the test uses vitest's
tempfile or tmpdir helpers, avoids mocking real secrets, runs synchronously (or
uses Promise.all to reproduce concurrency edge cases if needed), and includes a
Windows-safe path/assertion strategy to remain deterministic.
- Around line 2902-2926: The tests override process.platform directly without a
safe restore; wrap the platform mutation and the call to formatStorageErrorHint
inside a try/finally so the originalPlatform is always restored even if an
assertion fails. Update the two tests titled "should return permission hint for
EACCES on Windows" and "should return chmod hint for EACCES on Unix" to capture
originalPlatform, set the platform, run the err/hint assertions, and restore
originalPlatform in a finally block (or use vitest's vi.spyOn/vi.stub if
preferred) to prevent leaking the mocked platform across tests.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 3668-3701: clearFlaggedAccounts currently removes only the primary
flagged file and backups, but loadFlaggedAccounts can migrate the legacy file
(getLegacyFlaggedAccountsPath) back on next read; update clearFlaggedAccounts()
to also unlink the legacy flagged path returned by
getLegacyFlaggedAccountsPath() (and any backup candidates discovered for it)
using the same unlinkWithRetry/error handling pattern, ensure logs (in
clearFlaggedAccounts and related error branches) do not contain tokens/emails,
and add a vitest in test/storage.test.ts that creates only the legacy flagged
file, calls resetLocalState()/clearFlaggedAccounts(), then asserts the legacy
file is removed and loadFlaggedAccounts no longer migrates it; reference
functions: clearFlaggedAccounts, loadFlaggedAccounts,
getLegacyFlaggedAccountsPath, unlinkWithRetry, and resetLocalState for locating
code and tests.
---
Duplicate comments:
In `@test/storage.test.ts`:
- Around line 2478-2598: Add a deterministic vitest that reproduces the Windows
transient snapshot-failure under concurrency: stub process.platform to "win32",
spy/mock fs.writeFile so the first write to the
"accounts-delete-saved-accounts-snapshot-" path throws a NodeJS.ErrnoException
with code "EPERM" (or "EBUSY") and subsequent attempts succeed, then start
deleteSavedAccounts() and while its snapshot write is failing start a concurrent
saveAccounts() call; assert deleteSavedAccounts resolves with accountsCleared
true, the snapshot write was retried (attempts count == 2), the concurrent save
resolves, loadAccounts() contains the saved-after-delete account, and a snapshot
file exists via getNamedBackupsDirectoryPath(); restore process.platform and
mocked fs.writeFile in finally. Reference deleteSavedAccounts, saveAccounts,
loadAccounts, getNamedBackupsDirectoryPath, and the fs.writeFile spy in your
test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02170ba3-bb60-43cb-9fdc-a713d63aff3e
📒 Files selected for processing (4)
lib/destructive-actions.tslib/storage.tstest/destructive-actions.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/destructive-actions.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/destructive-actions.tslib/storage.ts
| const path = getStoragePath(); | ||
| const resetMarkerPath = getIntentionalResetMarkerPath(path); | ||
| await cleanupStaleRotatingBackupArtifacts(path); |
There was a problem hiding this comment.
don't reap fresh rotating temp files on every read.
lib/storage.ts:2892-2894 now runs rotating-backup cleanup before every load, but createRotatingAccountsBackup() in lib/storage.ts:712-763 stages live writes in .rotate.*.tmp files. a second codex process can be in the middle of that rotation and lose its staged copy here, which means backup history can disappear under normal cross-process concurrency. test/destructive-actions.test.ts:43-82 mocks the storage layer away, so please add a real-fs vitest in test/storage.test.ts that leaves a fresh rotate tmp file beside storage and asserts loadAccounts() does not delete 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2892 - 2894, The cleanup call before every load
is deleting live rotate temp files from concurrent rotations; modify the logic
so cleanupStaleRotatingBackupArtifacts(path) is NOT run unconditionally inside
loadAccounts — instead run cleanup only at startup or when the temp-file is
older than a safe threshold and skip files matching the in-progress pattern
(e.g., ".rotate.*.tmp"); update the code around getStoragePath,
getIntentionalResetMarkerPath and the loadAccounts callsite to only invoke
cleanup under those conditions (preserve createRotatingAccountsBackup()
behavior), and add a new real-FS vitest at test/storage.test.ts (referencing
test/destructive-actions.test.ts for context) that creates a fresh
".rotate.*.tmp" next to storage and asserts loadAccounts() does not remove it;
ensure changes consider Windows/EBUSY file semantics and mention in the test
that EBUSY/429 retries are handled and that no logs leak tokens/emails.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/storage.test.ts (2)
2928-2939:⚠️ Potential issue | 🟠 Majorplaceholder test hides migration regressions.
test/storage.test.ts:2928-2939still usesexpect(true).toBe(true), so filename migration behavior inlib/storage.tsremains effectively untested. per the prior review, replace this with a real arrange/act/assert migration case that verifies old-file detection, persisted migration, and cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 2928 - 2939, Replace the placeholder test with a real arrange/act/assert that simulates an existing legacy file and verifies migration: create a temp test storage dir, write the old filename "openai-codex-accounts.json" with sample account JSON, ensure "codex-accounts.json" does not exist, then call the code path that triggers getStoragePath()/loadAccounts() (the same function under test), assert that loadAccounts() returns the migrated account data, assert that "codex-accounts.json" now exists with the same contents, and assert that the old file "openai-codex-accounts.json" has been removed; clean up temp files after the test. Ensure you reference getStoragePath(), loadAccounts(), oldName ("openai-codex-accounts.json") and newName ("codex-accounts.json") in the test to locate the intended migration logic.
2969-2993:⚠️ Potential issue | 🟡 Minorplatform override is not failure-safe and can leak across tests.
test/storage.test.ts:2969-2993mutatesprocess.platformwithouttry/finally. if an assertion fails before restore, later cases inherit the wrong platform and become flaky. the past review comment was marked addressed but the code still shows the vulnerable pattern.proposed fix
it("should return permission hint for EACCES on Windows", () => { - const originalPlatform = process.platform; - Object.defineProperty(process, "platform", { value: "win32" }); - - const err = { code: "EACCES" } as NodeJS.ErrnoException; - const hint = formatStorageErrorHint(err, testPath); - - expect(hint).toContain("antivirus"); - expect(hint).toContain(testPath); - - Object.defineProperty(process, "platform", { value: originalPlatform }); + const originalPlatform = process.platform; + Object.defineProperty(process, "platform", { value: "win32" }); + try { + const err = { code: "EACCES" } as NodeJS.ErrnoException; + const hint = formatStorageErrorHint(err, testPath); + + expect(hint).toContain("antivirus"); + expect(hint).toContain(testPath); + } finally { + Object.defineProperty(process, "platform", { value: originalPlatform }); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 2969 - 2993, The tests "should return permission hint for EACCES on Windows" and "should return chmod hint for EACCES on Unix" mutate process.platform unsafely; fix by mocking/restoring platform in a failure-safe way—either use try/finally around the Object.defineProperty calls to ensure originalPlatform is restored in the finally block, or replace the manual mutation with a scoped mock (e.g., jest.spyOn(process, 'platform', 'get').mockReturnValue(...)) and restore the mock after the assertion; apply this change around the calls that exercise formatStorageErrorHint so the platform is always reset even if assertions fail.
🤖 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/storage.ts`:
- Around line 2381-2408: The code performs duplicate deduplication twice because
deduplicateAccounts and deduplicateAccountsByEmail both delegate to
deduplicateAccountsByIdentity; remove the redundant pass by calling only one of
them (prefer deduplicateAccountsByEmail for clarity) when computing
dedupedBackupAccounts and mergedTagged. Update the creation of
dedupedBackupAccounts to call deduplicateAccountsByEmail(backupAccounts)
(instead of deduplicateAccounts(deduplicateAccountsByEmail(...)) or vice versa),
and similarly compute mergedTagged by first building taggedCurrent and
taggedBackup then calling a single deduplication function
(deduplicateAccountsByEmail) on [...taggedCurrent, ...taggedBackup]; leave
TaggedAccount, taggedCurrent, taggedBackup, and conflict-count logic unchanged.
- Around line 2248-2283: Add a new test that simulates exhausting all
BACKUP_COPY_MAX_ATTEMPTS when creating a snapshot: mock the file write/backup
operation used by the snapshot retry loop (the same fs/writeFile or internal
helper used by createNamedBackup / the backup copy logic referenced by
BACKUP_COPY_MAX_ATTEMPTS) to reject on every attempt for the full number of
retries, then call snapshotAccountStorage(options) with failurePolicy: "error"
and assert it throws the final error; additionally ensure any destructive flow
that should abort after a snapshot failure (the test in
destructive-actions.test.ts) observes the abort behavior. Target symbols:
snapshotAccountStorage, createNamedBackup (or the backup write helper), and
BACKUP_COPY_MAX_ATTEMPTS, and place the test alongside existing
snapshot/destructive tests to mirror the existing EPERM test pattern. Ensure the
mock verifies the write was attempted BACKUP_COPY_MAX_ATTEMPTS times and that
the thrown error is the last rejection.
---
Duplicate comments:
In `@test/storage.test.ts`:
- Around line 2928-2939: Replace the placeholder test with a real
arrange/act/assert that simulates an existing legacy file and verifies
migration: create a temp test storage dir, write the old filename
"openai-codex-accounts.json" with sample account JSON, ensure
"codex-accounts.json" does not exist, then call the code path that triggers
getStoragePath()/loadAccounts() (the same function under test), assert that
loadAccounts() returns the migrated account data, assert that
"codex-accounts.json" now exists with the same contents, and assert that the old
file "openai-codex-accounts.json" has been removed; clean up temp files after
the test. Ensure you reference getStoragePath(), loadAccounts(), oldName
("openai-codex-accounts.json") and newName ("codex-accounts.json") in the test
to locate the intended migration logic.
- Around line 2969-2993: The tests "should return permission hint for EACCES on
Windows" and "should return chmod hint for EACCES on Unix" mutate
process.platform unsafely; fix by mocking/restoring platform in a failure-safe
way—either use try/finally around the Object.defineProperty calls to ensure
originalPlatform is restored in the finally block, or replace the manual
mutation with a scoped mock (e.g., jest.spyOn(process, 'platform',
'get').mockReturnValue(...)) and restore the mock after the assertion; apply
this change around the calls that exercise formatStorageErrorHint so the
platform is always reset even if assertions fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dfdd708d-b0de-4b02-8e41-bf355335ae27
📒 Files selected for processing (4)
lib/destructive-actions.tslib/storage.tstest/destructive-actions.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/destructive-actions.test.tstest/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/destructive-actions.tslib/storage.ts
🔇 Additional comments (26)
test/destructive-actions.test.ts (4)
1-82: lgtm on test setup and mock structure.the mock implementation for
withAccountAndFlaggedStorageTransactionat lines 55-81 correctly simulates transactional rollback withAggregateErrorhandling. thestructuredCloneusage for snapshot isolation is appropriate.
231-255: good coverage for null current storage abort path.this test at
test/destructive-actions.test.ts:231-255addresses the prior review concern about falling back to caller memory when transactional read fails. the assertion that no snapshot or save occurs whencurrentis null is correct.
257-298: collision handling by full identity test is solid.
test/destructive-actions.test.ts:257-298validates that when refresh tokens collide, the correct account is deleted by matching full identity (email + accountId). this addresses the prior review aboutfindMatchingAccountIndexusage.
402-464: aggregate error preservation test is comprehensive.the test correctly validates that both the original flagged-save error and rollback error are preserved in the
AggregateError.errorsarray. good coverage for the double-failure scenario.test/storage.test.ts (6)
2064-2133: good concurrency test for restore serialization.
test/storage.test.ts:2064-2133validates that concurrent restores are serialized when the account limit is tight, with one succeeding and one failing. thePromise.allSettledpattern and assertion on fulfilled/rejected counts is correct.
3650-3684: solid retry coverage for windows filesystem transient errors.
test/storage.test.ts:3650-3684tests retry on EPERM/EBUSY/EAGAIN forclearAccounts, which addresses the windows filesystem behavior requirement. the mock correctly simulates transient failure then success.
5401-5498: comprehensive quota cache clear tests.
test/storage.test.ts:5401-5498coversclearQuotaCachethoroughly: removal, missing file, transient retry on EPERM/EBUSY, and retry exhaustion. good alignment with windows filesystem behavior requirements.
2248-2313: good security test for path redaction in snapshot logs.
test/storage.test.ts:2248-2313validates that filesystem paths are redacted from warning logs when snapshot creation fails. this is important for avoiding leakage of user directory paths. the test correctly verifies the backup name is logged buttestWorkDiris not.
2688-2732: critical safety test for snapshot-failure abort.
test/storage.test.ts:2688-2732validates that accounts are not cleared when the pre-delete snapshot fails. this is essential for the fail-closed behavior mentioned in PR objectives.
2553-2601:⚠️ Potential issue | 🟡 Minorwin32 snapshot retry test addresses past review but platform override still leaks.
test/storage.test.ts:2553-2601adds the requested win32 EPERM retry regression test, which is good. however, theObject.defineProperty(process, "platform", ...)at line 2586 is inside the try block but the assertions at 2587-2596 happen before the finally restore at 2598. ifexpect(snapshotWriteAttempts).toBe(2)fails, the platform stayswin32.proposed fix
try { Object.defineProperty(process, "platform", { value: "win32" }); - await expect(deleteSavedAccounts()).resolves.toMatchObject({ - accountsCleared: true, - }); - expect(snapshotWriteAttempts).toBe(2); - const entries = await fs.readdir(getNamedBackupsDirectoryPath()); - expect( - entries.some((name) => - name.startsWith("accounts-delete-saved-accounts-snapshot-"), - ), - ).toBe(true); + try { + await expect(deleteSavedAccounts()).resolves.toMatchObject({ + accountsCleared: true, + }); + expect(snapshotWriteAttempts).toBe(2); + const entries = await fs.readdir(getNamedBackupsDirectoryPath()); + expect( + entries.some((name) => + name.startsWith("accounts-delete-saved-accounts-snapshot-"), + ), + ).toBe(true); + } finally { + Object.defineProperty(process, "platform", { value: originalPlatform }); + } } finally { - Object.defineProperty(process, "platform", { value: originalPlatform }); writeFileSpy.mockRestore(); }> Likely an incorrect or invalid review comment.lib/destructive-actions.ts (6)
110-113: fail-closed behavior on null current storage is correct.
lib/destructive-actions.ts:110-113returnsnullwhencurrentisnull, which prevents falling back to caller memory. this addresses the prior review concern about clobbering data after concurrent reset. test coverage is attest/destructive-actions.test.ts:231-255.
115-118: identity matching uses findMatchingAccountIndex correctly.
lib/destructive-actions.ts:115-118usesfindMatchingAccountIndexwith the fullrequestedTargetobject, which includesaccountId,refreshToken. this addresses the prior review about refresh token collision handling. test coverage is attest/destructive-actions.test.ts:257-298.
127-133: snapshot-before-mutate ordering is correct.
lib/destructive-actions.ts:127-133creates the snapshot after loading flagged accounts but before any mutations. withfailurePolicy: "error", a snapshot failure will abort the entire delete operation. test coverage is attest/destructive-actions.test.ts:200-229.
73-88: rebase logic handles deleted-active-account edge case correctly.
lib/destructive-actions.ts:73-88only decrements indices greater thanremovedIndex. if the deleted account was the active one, the index stays (now pointing to the next account), thenclampActiveIndicesat line 143 handles out-of-bounds. test coverage is attest/destructive-actions.test.ts:139-198.
145-153: flagged account cleanup uses refreshToken intentionally.
lib/destructive-actions.ts:145-147filters flagged accounts byrefreshTokenrather than full identity. this is correct because flagged accounts are keyed by refreshToken and the original account'saccountIdmay have changed since flagging. test coverage is attest/destructive-actions.test.ts:300-353.
182-192: resetLocalState order is correct with sequential awaits.
lib/destructive-actions.ts:182-192runs operations sequentially. ifclearFlaggedAccountsthrows (line 184),clearQuotaCacheandclearCodexCliStateCacheare not called. the snapshot is already created at this point, which is intentional per fail-closed design. test coverage is attest/destructive-actions.test.ts:122-137.lib/storage.ts (10)
2892-2894: cleanup on every load can race with concurrent rotations.
lib/storage.ts:2894runscleanupStaleRotatingBackupArtifacts(path)unconditionally before each load. the function atlib/storage.ts:791-826deletes any.rotate.*.tmpfile without checking age. if another codex process is mid-rotation viacreateRotatingAccountsBackup(lib/storage.ts:712-763), its staged write can be reaped.consider gating cleanup to startup only or adding a staleness threshold (e.g., files older than 60s).
119-248: lgtm on the new backup metadata interfaces.the types for
NamedBackupMetadata,BackupRestoreAssessment, and conflict details are well-structured. theAccountSnapshotFailurePolicytype with"warn" | "error"gives callers explicit control over fail-open vs fail-closed behavior.
478-496: retry utilities handle windows transient errors correctly.
unlinkWithRetryatlib/storage.ts:478andretryTransientFilesystemOperationatlib/storage.ts:2028both use exponential backoff forEBUSY/EPERM/EAGAIN. the win32-specificEPERMcheck atlib/storage.ts:2025is appropriate since unixEPERMusually signals a real permission problem.minor:
unlinkWithRetrylacks jitter whileretryTransientFilesystemOperationadds it. consider harmonizing for consistency under heavy concurrency.Also applies to: 2019-2046
2672-2693: import snapshot failure policy now enforced by callers.
lib/storage.ts:2680still defaultssnapshotFailurePolicyto"warn", but the public entry pointsimportAccounts(lib/storage.ts:3787) andrestoreNamedBackup(lib/storage.ts:2589) both pass"error". this addresses the past review concern for user-facing flows.if new internal callers are added later without explicit policy, they'll inherit warn behavior. consider flipping the internal default to
"error"to fail closed by default, but this is low risk given current call sites.
2175-2198: snapshot write path now retries transient errors.
lib/storage.ts:2180-2198wrapsfs.mkdirandfs.writeFileinretryTransientFilesystemOperation, addressing the prior concern about bare writes failing on win32EBUSY/EPERM.
3374-3392: reset marker directory creation now precedes write.
lib/storage.ts:3387addsfs.mkdir(dirname(resetMarkerPath), { recursive: true })before writing the marker. this fixes theENOENTon fresh paths flagged in the prior review.
463-467: transaction context now tracks storage path.
lib/storage.ts:466addsstoragePathto the transaction snapshot context, andlib/storage.ts:3740-3747validates it during export. this prevents exporting stale state ifsetStoragePathDirectis called mid-transaction.Also applies to: 3739-3748
3421-3435: snapshotAndClearAccounts fails closed on snapshot error.
lib/storage.ts:3427-3432passesfailurePolicy: "error"so a failed snapshot aborts beforeclearAccountsUnlockedruns. the whole flow is insidewithStorageLock, ensuring atomicity.
2222-2246: logging redacts filesystem paths, no tokens or emails leaked.
formatSnapshotErrorForLogatlib/storage.ts:2234andredactFilesystemDetailsatlib/storage.ts:2227strip full paths from error messages. log statements likelib/storage.ts:2724-2729only emit counts and redacted paths.Also applies to: 2724-2729
2755-2759: case-insensitive backup name matching on windows is correct.
equalsNamedBackupEntryatlib/storage.ts:2755-2758usestoLowerCase()comparison on win32, matching ntfs behavior. this prevents "not found" errors when users specify names with different casing.
| export async function snapshotAccountStorage( | ||
| options: AccountSnapshotOptions, | ||
| ): Promise<NamedBackupMetadata | null> { | ||
| const { | ||
| reason, | ||
| now = Date.now(), | ||
| force = true, | ||
| failurePolicy = "warn", | ||
| createBackup = createNamedBackup, | ||
| storagePath, | ||
| } = options; | ||
| const currentStorage = | ||
| options.storage !== undefined ? options.storage : await loadAccounts(); | ||
| if (!currentStorage || currentStorage.accounts.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| const backupName = buildAccountSnapshotName(reason, now); | ||
| try { | ||
| return await createBackup(backupName, { | ||
| force, | ||
| storage: currentStorage, | ||
| storagePath, | ||
| }); | ||
| } catch (error) { | ||
| if (failurePolicy === "error") { | ||
| throw error; | ||
| } | ||
| log.warn("Failed to create account storage snapshot", { | ||
| reason, | ||
| backupName, | ||
| error: formatSnapshotErrorForLog(error), | ||
| }); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# search for tests exercising snapshot failure scenarios
rg -nC3 'snapshotAccountStorage|snapshotFailurePolicy|failurePolicy.*error' --type=ts --glob '*test*'Repository: ndycode/codex-multi-auth
Length of output: 7379
🏁 Script executed:
# Check the actual EPERM test case in destructive-actions
sed -n '200,260p' test/destructive-actions.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1826
🏁 Script executed:
# Search for retry logic in storage.ts for backup/snapshot operations
rg -n 'retry|EBUSY|EPERM' lib/storage.ts -A 2 -B 2Repository: ndycode/codex-multi-auth
Length of output: 2979
🏁 Script executed:
# Search for tests in storage.test.ts that simulate multiple failures or retry exhaustion
rg -n 'EBUSY|EPERM|mockRejected|attempt|retry' test/storage.test.ts -B 2 -A 5 | head -100Repository: ndycode/codex-multi-auth
Length of output: 3266
🏁 Script executed:
# Also check what BACKUP_COPY_MAX_ATTEMPTS is set to
rg -n 'BACKUP_COPY_MAX_ATTEMPTS' lib/storage.tsRepository: ndycode/codex-multi-auth
Length of output: 360
🏁 Script executed:
# Look for tests that specifically test retry exhaustion or multiple failures
rg -n 'retry-list-dir|mockRejected.*Times|callCount|toHaveBeenCalledTimes.*[2-9]' test/storage.test.ts -B 3 -A 10Repository: ndycode/codex-multi-auth
Length of output: 2167
🏁 Script executed:
# Check if there are Windows-specific snapshot tests
rg -n 'win32|platform.*win|windows' test/storage.test.ts -B 2 -A 5 | head -80Repository: ndycode/codex-multi-auth
Length of output: 2125
🏁 Script executed:
# Search for tests that specifically test snapshot creation retry exhaustion
rg -n 'snapshotWriteAttempts|createNamedBackup.*retry|createBackup.*EBUSY' test/storage.test.ts -B 5 -A 10Repository: ndycode/codex-multi-auth
Length of output: 4218
🏁 Script executed:
# Look around line 2590 to see the full snapshot retry test
sed -n '2560,2620p' test/storage.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1743
verify retry exhaustion test coverage for snapshot creation.
snapshotAccountStorage at lib/storage.ts:2248 supports failurePolicy: "error" for fail-closed behavior. win32 EPERM retry is tested (test/storage.test.ts:2590 verifies 2 writeFile attempts on first failure), and destructive abort on snapshot failure is tested (test/destructive-actions.test.ts:207). however, retry exhaustion scenarios are not covered:
- no test exercises all 5
BACKUP_COPY_MAX_ATTEMPTSretries failing for snapshot creation (lib/storage.ts:48) test/destructive-actions.test.ts:207mocks one rejection only; doesn't verify actual exhaustion behavior- snapshot creation retry logic exists at
lib/storage.ts:489forEPERM/EBUSY, but exhaustion path untested
add a test case where snapshot writeFile exhausts retries (fails all 5 attempts), then verify snapshotAccountStorage with failurePolicy: "error" throws the final error and destructive flows properly abort.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2248 - 2283, Add a new test that simulates
exhausting all BACKUP_COPY_MAX_ATTEMPTS when creating a snapshot: mock the file
write/backup operation used by the snapshot retry loop (the same fs/writeFile or
internal helper used by createNamedBackup / the backup copy logic referenced by
BACKUP_COPY_MAX_ATTEMPTS) to reject on every attempt for the full number of
retries, then call snapshotAccountStorage(options) with failurePolicy: "error"
and assert it throws the final error; additionally ensure any destructive flow
that should abort after a snapshot failure (the test in
destructive-actions.test.ts) observes the abort behavior. Target symbols:
snapshotAccountStorage, createNamedBackup (or the backup write helper), and
BACKUP_COPY_MAX_ATTEMPTS, and place the test alongside existing
snapshot/destructive tests to mirror the existing EPERM test pattern. Ensure the
mock verifies the write was attempted BACKUP_COPY_MAX_ATTEMPTS times and that
the thrown error is the last rejection.
| const dedupedBackupAccounts = deduplicateAccountsByEmail( | ||
| deduplicateAccounts(backupAccounts), | ||
| ); | ||
| const dedupedBackupAccountCount = dedupedBackupAccounts.length; | ||
| const conflictsWithinBackup = Math.max( | ||
| 0, | ||
| backupAccountCount - dedupedBackupAccountCount, | ||
| ); | ||
|
|
||
| type TaggedAccount = AccountLike & { | ||
| __source: "current" | "backup"; | ||
| __index: number; | ||
| }; | ||
|
|
||
| const taggedCurrent: TaggedAccount[] = currentAccounts.map((account, index) => ({ | ||
| ...account, | ||
| __source: "current", | ||
| __index: index, | ||
| })); | ||
| const taggedBackup: TaggedAccount[] = backupAccounts.map((account, index) => ({ | ||
| ...account, | ||
| __source: "backup", | ||
| __index: index, | ||
| })); | ||
|
|
||
| const mergedTagged = deduplicateAccountsByEmail( | ||
| deduplicateAccounts([...taggedCurrent, ...taggedBackup]), | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
redundant deduplication passes.
deduplicateAccounts and deduplicateAccountsByEmail both resolve to deduplicateAccountsByIdentity (see lib/storage.ts:2602-2624). chaining them at lib/storage.ts:2381 and lib/storage.ts:2406 iterates the array twice without additional benefit.
suggested simplification
- const dedupedBackupAccounts = deduplicateAccountsByEmail(
- deduplicateAccounts(backupAccounts),
- );
+ const dedupedBackupAccounts = deduplicateAccounts(backupAccounts);- const mergedTagged = deduplicateAccountsByEmail(
- deduplicateAccounts([...taggedCurrent, ...taggedBackup]),
- );
+ const mergedTagged = deduplicateAccounts([...taggedCurrent, ...taggedBackup]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2381 - 2408, The code performs duplicate
deduplication twice because deduplicateAccounts and deduplicateAccountsByEmail
both delegate to deduplicateAccountsByIdentity; remove the redundant pass by
calling only one of them (prefer deduplicateAccountsByEmail for clarity) when
computing dedupedBackupAccounts and mergedTagged. Update the creation of
dedupedBackupAccounts to call deduplicateAccountsByEmail(backupAccounts)
(instead of deduplicateAccounts(deduplicateAccountsByEmail(...)) or vice versa),
and similarly compute mergedTagged by first building taggedCurrent and
taggedBackup then calling a single deduplication function
(deduplicateAccountsByEmail) on [...taggedCurrent, ...taggedBackup]; leave
TaggedAccount, taggedCurrent, taggedBackup, and conflict-count logic unchanged.
| if (destructiveActionInFlight) { | ||
| console.log("Another destructive action is already running. Wait for it to finish."); | ||
| continue; | ||
| } | ||
| if (menuResult.mode === "forecast") { | ||
| await runActionPanel("Best Account", "Comparing accounts", async () => { | ||
| await runForecast(["--live"]); | ||
| }, displaySettings); | ||
| continue; | ||
| destructiveActionInFlight = true; | ||
| try { | ||
| await runActionPanel( | ||
| DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.label, | ||
| DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.stage, | ||
| async () => { | ||
| const result = await deleteSavedAccounts(); | ||
| console.log( | ||
| result.accountsCleared | ||
| ? DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.completed | ||
| : "Delete saved accounts completed with warnings. Some saved account artifacts could not be removed; see logs.", | ||
| ); | ||
| }, | ||
| displaySettings, | ||
| ); | ||
| } finally { | ||
| destructiveActionInFlight = false; | ||
| } | ||
| if (menuResult.mode === "fix") { | ||
| await runActionPanel("Auto-Fix", "Checking and fixing common issues", async () => { | ||
| await runFix(["--live"]); | ||
| }, displaySettings); | ||
| suppressRecoveryPrompt = true; | ||
| continue; | ||
| } | ||
| if (menuResult.mode === "reset") { | ||
| if (destructiveActionInFlight) { | ||
| console.log("Another destructive action is already running. Wait for it to finish."); | ||
| continue; | ||
| } | ||
| if (menuResult.mode === "settings") { | ||
| await configureUnifiedSettings(displaySettings); | ||
| continue; | ||
| destructiveActionInFlight = true; |
There was a problem hiding this comment.
destructiveActionInFlight guard is unreachable dead code
destructiveActionInFlight is set to true before await runActionPanel(...) and reset to false in a finally block. because runAuthLogin is a single async function that awaits every action panel before returning to the menu loop, the menu can only prompt for the next action after the previous finally has run. there is no parallel execution path that could leave destructiveActionInFlight === true when these checks are reached.
the guard reads as meaningful concurrency protection, but it cannot fire in the current single-threaded async flow. concurrent sessions (two terminals running codex auth login) live in separate processes with separate module state, so the flag doesn't protect against that either. consider removing it or replacing it with a comment that explains under what future scenario it would become relevant (e.g., background refresh tasks that share the same process).
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4004-4034
Comment:
**`destructiveActionInFlight` guard is unreachable dead code**
`destructiveActionInFlight` is set to `true` before `await runActionPanel(...)` and reset to `false` in a `finally` block. because `runAuthLogin` is a single async function that `await`s every action panel before returning to the menu loop, the menu can only prompt for the next action after the previous `finally` has run. there is no parallel execution path that could leave `destructiveActionInFlight === true` when these checks are reached.
the guard reads as meaningful concurrency protection, but it cannot fire in the current single-threaded async flow. concurrent sessions (two terminals running `codex auth login`) live in separate processes with separate module state, so the flag doesn't protect against that either. consider removing it or replacing it with a comment that explains under what future scenario it would become relevant (e.g., background refresh tasks that share the same process).
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/storage-recovery-paths.test.ts (1)
628-665:⚠️ Potential issue | 🟡 Minoradd the win32 locked-file regression next to this cleanup case.
test/storage-recovery-paths.test.ts:628-665covers stale-vs-fresh selection, but it still does not exercise theEPERM/EBUSYbranch inlib/storage.ts:825-836. that cleanup runs on everyloadAccounts()now, so please add a vitest that forces one stale.rotate.*.tmpunlink to fail with a windows-style lock and asserts the primary storage still loads.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-recovery-paths.test.ts` around lines 628 - 665, Add a new vitest in the same file beside the stale-vs-fresh case that creates the same stale/fresh/unrelated artifact files, then spy/mock fs.promises.unlink (or fs.unlink) so that when unlink is invoked for one specific stale artifact it throws an Error with code "EPERM" (or "EBUSY") to simulate a Windows locked-file; call loadAccounts() and assert it still returns the primary account (use loadAccounts symbol) and that the unlink mock was called for the targeted stale file, and finally assert the process still behaved as expected (the storage loaded successfully and other stale artifacts were cleaned or the locked-stale file remains as appropriate). Reference the cleanup branch in lib/storage.ts (the unlink/EPERM handling) when locating where to simulate the error.lib/storage.ts (1)
1742-1759:⚠️ Potential issue | 🟠 Majorpreserve the sparse
activeIndexByFamilyshape.
lib/storage.ts:1742-1759still assigns an entry for everyMODEL_FAMILIESkey. that means a sparse map gets densified on any load/save round-trip, which changes auth-rotation semantics for families that were supposed to inherit the default index. the new serializers atlib/storage.ts:2208-2212,lib/storage.ts:2731-2735, andlib/storage.ts:3782-3785will now faithfully persist that mutated shape. please only clamp keys that were present in the source object, and add a vitest in the storage round-trip suite for the sparse-family case.possible fix
const activeIndexByFamily: Partial<Record<ModelFamily, number>> = {}; const rawFamilyIndices = isRecord(baseStorage.activeIndexByFamily) ? (baseStorage.activeIndexByFamily as Record<string, unknown>) : {}; for (const family of MODEL_FAMILIES) { + if (!Object.prototype.hasOwnProperty.call(rawFamilyIndices, family)) { + continue; + } const rawIndexValue = rawFamilyIndices[family]; const rawIndex = typeof rawIndexValue === "number" && Number.isFinite(rawIndexValue) ? rawIndexValue : rawActiveIndex;As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1742 - 1759, The loop over MODEL_FAMILIES currently always writes an entry into activeIndexByFamily, densifying a previously sparse map; update the loop to first check whether the source rawFamilyIndices actually contains that family (e.g., using Object.prototype.hasOwnProperty.call(rawFamilyIndices, family) or rawFamilyIndices[family] !== undefined) and only compute/clamp (via clampIndex) and assign activeIndexByFamily[family] when the key was present in the input; keep using extractActiveAccountRef and resolveAccountSelectionIndex/deduplicatedAccounts for the value calculation but do not create entries for absent families so the sparse shape is preserved, and add a vitest in the storage round-trip test suite that asserts a sparse activeIndexByFamily survives load/save unchanged.
♻️ Duplicate comments (2)
lib/storage.ts (1)
3689-3704:⚠️ Potential issue | 🟠 Majorcreate the flagged reset-marker directory before writing it.
lib/storage.ts:3691-3697still writes the flagged reset marker before ensuring the parent directory exists. on a fresh storage root,clearFlaggedAccounts()throwsENOENTand aborts before cleanup.lib/storage.ts:3408-3413already fixed the same pattern for account storage; please mirror it here and add a vitest for the missing-parent case.possible fix
export async function clearFlaggedAccounts(): Promise<boolean> { return withStorageLock(async () => { const path = getFlaggedAccountsPath(); const markerPath = getIntentionalResetMarkerPath(path); try { + await fs.mkdir(dirname(markerPath), { recursive: true }); await fs.writeFile(markerPath, "reset", { encoding: "utf-8", mode: 0o600, });As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 3689 - 3704, clearFlaggedAccounts writes the reset marker before ensuring the parent directory exists, causing ENOENT on fresh storage; update clearFlaggedAccounts (the withStorageLock block that uses getFlaggedAccountsPath and getIntentionalResetMarkerPath) to create the marker parent directory first (use fs.mkdir on the marker's dirname with { recursive: true, appropriate mode } like the account-storage fix at lib/storage.ts lines ~3408–3413), then write the marker with fs.writeFile; also add a vitest that simulates a missing-parent storage root to assert clearFlaggedAccounts creates the directory and succeeds, and ensure logs do not leak sensitive data.test/storage.test.ts (1)
2955-2979:⚠️ Potential issue | 🟠 Majormake the platform override failure-safe.
test/storage.test.ts:2955-2979still mutatesprocess.platformdirectly and restores it only on the happy path. if either assertion fails, later cases inherit the wrong platform and the suite gets flaky.suggested fix
it("should return permission hint for EACCES on Windows", () => { - const originalPlatform = process.platform; - Object.defineProperty(process, "platform", { value: "win32" }); - - const err = { code: "EACCES" } as NodeJS.ErrnoException; - const hint = formatStorageErrorHint(err, testPath); - - expect(hint).toContain("antivirus"); - expect(hint).toContain(testPath); - - Object.defineProperty(process, "platform", { value: originalPlatform }); + const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); + try { + const err = { code: "EACCES" } as NodeJS.ErrnoException; + const hint = formatStorageErrorHint(err, testPath); + + expect(hint).toContain("antivirus"); + expect(hint).toContain(testPath); + } finally { + platformSpy.mockRestore(); + } });apply the same pattern to the unix case below.
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 2955 - 2979, Both tests ("should return permission hint for EACCES on Windows" and "should return chmod hint for EACCES on Unix") mutate process.platform and only restore it on the happy path; make the override failure-safe by storing originalPlatform, setting Object.defineProperty(process, "platform", { value: ... }) and wrapping the assertions in a try/finally block that restores process.platform in finally; apply this pattern to the Unix test as well and keep references to the test names and the helper under test formatStorageErrorHint so the platform is always restored even if expect(...) throws.
🤖 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/storage.ts`:
- Around line 2707-2713: The pre-import snapshot and transaction paths call
getStoragePath() during snapshot/write which allows
setStoragePath()/setStoragePathDirect() to change the target mid-transaction;
modify snapshotAccountStorage usage and the unlocked save/snapshot helpers (the
functions that currently call getStoragePath() internally) to accept a captured
storagePath argument and use that value instead of calling getStoragePath()
again (update callers at the places using snapshotAccountStorage and the
transaction write helpers referenced in lib/storage.ts around the existing
pre-import snapshot and the sections at 3306-3318 and 3333-3349); add Vitest
unit tests that start a transaction, capture the path, flip the storage path
mid-transaction, and assert the snapshot/write used the original captured path
for both transaction helpers, and ensure tests exercise EBUSY/429 retry behavior
and that logging does not leak tokens/emails.
In `@test/storage.test.ts`:
- Around line 2631-2686: Add a deterministic regression test alongside the
existing "creates a snapshot before restoring a named backup" case that forces
the snapshot write to fail on win32 and asserts restoreNamedBackup rejects
without changing the current accounts; specifically, stub or intercept the
filesystem write used by restoreNamedBackup (the write of files named like
"accounts-import-accounts-snapshot-...") to throw on win32, then call
saveAccounts(...) / createNamedBackup(...) / saveAccounts(...) as in the happy
path and assert await
expect(restoreNamedBackup("restore-with-snapshot")).rejects.toThrow(), and
finally assert loadAccounts() still returns the original "replacement" pool was
not lost (use loadAccounts(), getNamedBackupsDirectoryPath() and mirror the
abort-style assertions and cleanup pattern used in the abort coverage around the
restore tests at the other block to keep determinism and vitest usage).
- Around line 3624-3634: The test only asserts clearAccounts() returns true but
doesn't verify the side-effect it claims (creating the parent/reset-marker
directory); update the test that uses setStoragePathDirect(nestedStoragePath)
and clearAccounts() to also assert the expected filesystem state—e.g., use Node
fs (fs.existsSync or fs.promises.stat) to confirm the parent directory
(join(testWorkDir, "brand-new", "nested") or the intended reset-marker file) now
exists after clearAccounts() resolves; keep the vitest async pattern and clean
up any created files/dirs in test teardown.
- Around line 4680-4709: The test patches fs.rename with renameSpy but calls
renameSpy.mockRestore() only after assertions, risking leaked spy if
saveAccounts() or expects throw; wrap the exercise and assertions in a
try/finally and call renameSpy.mockRestore() in the finally so the spy is always
restored; apply the same try/finally cleanup pattern to the other four tests in
the "saveAccounts EPERM/EBUSY retry logic" describe block that also create a
renameSpy for saveAccounts.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 1742-1759: The loop over MODEL_FAMILIES currently always writes an
entry into activeIndexByFamily, densifying a previously sparse map; update the
loop to first check whether the source rawFamilyIndices actually contains that
family (e.g., using Object.prototype.hasOwnProperty.call(rawFamilyIndices,
family) or rawFamilyIndices[family] !== undefined) and only compute/clamp (via
clampIndex) and assign activeIndexByFamily[family] when the key was present in
the input; keep using extractActiveAccountRef and
resolveAccountSelectionIndex/deduplicatedAccounts for the value calculation but
do not create entries for absent families so the sparse shape is preserved, and
add a vitest in the storage round-trip test suite that asserts a sparse
activeIndexByFamily survives load/save unchanged.
In `@test/storage-recovery-paths.test.ts`:
- Around line 628-665: Add a new vitest in the same file beside the
stale-vs-fresh case that creates the same stale/fresh/unrelated artifact files,
then spy/mock fs.promises.unlink (or fs.unlink) so that when unlink is invoked
for one specific stale artifact it throws an Error with code "EPERM" (or
"EBUSY") to simulate a Windows locked-file; call loadAccounts() and assert it
still returns the primary account (use loadAccounts symbol) and that the unlink
mock was called for the targeted stale file, and finally assert the process
still behaved as expected (the storage loaded successfully and other stale
artifacts were cleaned or the locked-stale file remains as appropriate).
Reference the cleanup branch in lib/storage.ts (the unlink/EPERM handling) when
locating where to simulate the error.
---
Duplicate comments:
In `@lib/storage.ts`:
- Around line 3689-3704: clearFlaggedAccounts writes the reset marker before
ensuring the parent directory exists, causing ENOENT on fresh storage; update
clearFlaggedAccounts (the withStorageLock block that uses getFlaggedAccountsPath
and getIntentionalResetMarkerPath) to create the marker parent directory first
(use fs.mkdir on the marker's dirname with { recursive: true, appropriate mode }
like the account-storage fix at lib/storage.ts lines ~3408–3413), then write the
marker with fs.writeFile; also add a vitest that simulates a missing-parent
storage root to assert clearFlaggedAccounts creates the directory and succeeds,
and ensure logs do not leak sensitive data.
In `@test/storage.test.ts`:
- Around line 2955-2979: Both tests ("should return permission hint for EACCES
on Windows" and "should return chmod hint for EACCES on Unix") mutate
process.platform and only restore it on the happy path; make the override
failure-safe by storing originalPlatform, setting Object.defineProperty(process,
"platform", { value: ... }) and wrapping the assertions in a try/finally block
that restores process.platform in finally; apply this pattern to the Unix test
as well and keep references to the test names and the helper under test
formatStorageErrorHint so the platform is always restored even if expect(...)
throws.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 336af532-b427-48ed-8ad1-dbce6c9ca8be
📒 Files selected for processing (3)
lib/storage.tstest/storage-recovery-paths.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-recovery-paths.test.tstest/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
792-847:⚠️ Potential issue | 🟠 Majortest exists, but cleanup lacks EBUSY retry on windows.
the test "cleans up only stale staged backup artifacts during load" at
test/storage-recovery-paths.test.ts:628already covers your request—it verifies fresh rotate tmp files survive cleanup while stale ones (epoch-dated) are deleted. the 60-second threshold atlib/storage.ts:50is solid for protecting in-progress rotations.however,
cleanupStaleRotatingBackupArtifactsatlib/storage.ts:827callsfs.unlink()without retry logic. the codebase already hasunlinkWithRetry()atlib/storage.ts:479that handles EBUSY/EPERM/EAGAIN with exponential backoff—windows users can hit file locks during cleanup, causing immediate failure instead of retrying. replace barefs.unlink()withunlinkWithRetry()and add a vitest covering EBUSY during artifact removal.
🤖 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/storage.ts`:
- Around line 3420-3425: The mkdir and writeFile calls in clearAccountsUnlocked
are vulnerable to transient filesystem errors; wrap both
fs.mkdir(dirname(resetMarkerPath), { recursive: true }) and the
fs.writeFile(resetMarkerPath, JSON.stringify(...), { encoding: "utf-8", mode:
0o600 }) inside retryTransientFilesystemOperation so transient EBUSY/lock errors
are retried; call retryTransientFilesystemOperation with async closures that
perform the mkdir and the write (preserving the original options and payload)
and surface errors if retries exhaust, referencing clearAccountsUnlocked,
resetMarkerPath, and retryTransientFilesystemOperation to locate the spots to
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08c5aeae-08a9-4817-b5a7-3991435cf953
📒 Files selected for processing (2)
lib/storage.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.ts
🔇 Additional comments (32)
lib/storage.ts (20)
2269-2308:snapshotAccountStoragerespects transaction storagePath and redacts errors.the storagePath resolution at
lib/storage.ts:2281-2283correctly falls back to the transaction context's captured path when active. error logging atlib/storage.ts:2301-2305usesformatSnapshotErrorForLogwhich redacts filesystem paths — no token/email leakage.a past review comment flagged missing test coverage for retry exhaustion scenarios. that concern remains open per the existing comment thread.
2406-2433: redundant deduplication passes remain.the chained
deduplicateAccountsByEmail(deduplicateAccounts(...))atlib/storage.ts:2406-2408andlib/storage.ts:2431-2433iterates the array twice unnecessarily since both functions delegate todeduplicateAccountsByIdentity. this was flagged in a past review and remains unaddressed — low priority but a minor perf overhead.
120-258: lgtm on the new type definitions.the backup metadata types and snapshot options are well-structured. pluggable
createBackupinAccountSnapshotOptionsatlib/storage.ts:222is good for test injection.
260-297: good path traversal protection innormalizeBackupLookupName.the validation at
lib/storage.ts:271-297properly rejects path separators, drive prefixes, and..segments. this prevents directory traversal attacks on named backup restore operations.
464-468: transaction context now captures storagePath — good fix.the addition of
storagePathatlib/storage.ts:467addresses the race condition wheresetStoragePath()during an active transaction could cause reads from one profile and writes to another. this was flagged in a past review and is now resolved.
479-497: solid retry logic for unlink operations.
unlinkWithRetryatlib/storage.ts:479-497handlesEPERM/EBUSY/EAGAINwith exponential backoff, which is essential for windows where files can be transiently locked. the pattern matches other retry helpers in this file.
2040-2067: retry helper correctly scopesEPERMto win32.
isRetryableFilesystemErrorCodeatlib/storage.ts:2040-2047only treatsEPERMas retryable on windows, which is correct — on unixEPERMtypically indicates a permission issue, not a transient lock.EBUSYandEAGAINare retried on all platforms.the retry helper properly throws after exhaustion at
lib/storage.ts:2058, ensuring fail-closed behavior.
2188-2221: snapshot writes now use retry logic — past issue addressed.
writeNamedBackupFromStorageatlib/storage.ts:2201-2219now wraps bothfs.mkdirandfs.writeFileinretryTransientFilesystemOperation, addressing the past review concern about transientEPERM/EBUSYfailures during destructive flows.
2697-2717: call sites enforce fail-closed snapshot policy.while
importNormalizedAccountsdefaults tosnapshotFailurePolicy: "warn"atlib/storage.ts:2705, the actual call sites atlib/storage.ts:2614-2617(restoreNamedBackup) andlib/storage.ts:3824-3827(importAccounts) explicitly pass"error", ensuring pre-import snapshots fail-closed as required.the default allows internal flexibility (e.g., for dry-run assessment paths that don't mutate), which is reasonable.
3303-3327: transaction path pinning implemented correctly.
withAccountStorageTransactionatlib/storage.ts:3310capturesstoragePathbefore the transaction begins and threads it through the persist callback atlib/storage.ts:3318-3320. this ensures that ifsetStoragePath()is called mid-transaction, the transaction still operates on the original path.
3329-3387: flagged storage transaction also pins path.
withAccountAndFlaggedStorageTransactionatlib/storage.ts:3339-3343follows the same pattern, ensuring both account and flagged storage operations use the captured path. the rollback logic atlib/storage.ts:3362-3364correctly usesstate.storagePath.
3407-3452: reset marker directory creation fixed.
clearAccountsUnlockedatlib/storage.ts:3420now callsfs.mkdir(dirname(resetMarkerPath), { recursive: true })before writing the reset marker, addressing the past review concern aboutENOENTon fresh storage paths.error logging at
lib/storage.ts:3435-3436usesextractPathTailandformatSnapshotErrorForLogto avoid leaking full filesystem paths.
3454-3468:snapshotAndClearAccountsenforces fail-closed behavior.the function at
lib/storage.ts:3460-3465passesfailurePolicy: "error", so if the pre-destructive snapshot fails (e.g.,EBUSYexhausts retries), the clear operation is never reached. this is the correct fail-closed pattern.
3705-3756:clearFlaggedAccountsuses retry-aware unlink.the function at
lib/storage.ts:3725-3738usesunlinkWithRetryfor artifact cleanup, handling transient windows file locks. the marker is only removed if all artifacts were successfully cleared atlib/storage.ts:3739-3753.
3776-3785:exportAccountsvalidates transaction storage path.at
lib/storage.ts:3778-3784, the function throws if called inside an active transaction that's operating on a different storage path than the current one. this prevents accidental export from a stale or mismatched path.
3819-3828:importAccountsenforces fail-closed pre-import snapshot.the function at
lib/storage.ts:3824-3827passessnapshotFailurePolicy: "error", ensuring the import aborts if the pre-import snapshot cannot be written.
3649-3656:saveFlaggedAccountsUnlockednow accepts storagePath override.at
lib/storage.ts:3651-3656, the function derives the flagged accounts path from the providedstoragePathoption, enabling proper path pinning within transactions.
1510-1510: minor type annotation fix.the explicit type annotation
const newest: T = selectNewestAccount(...)atlib/storage.ts:1510is a harmless clarification — typescript would infer this anyway but the explicit annotation improves readability.
2243-2267: path redaction utilities prevent leakage in logs.
extractPathTailatlib/storage.ts:2243-2246andredactFilesystemDetailsatlib/storage.ts:2248-2253strip full paths from error messages.formatSnapshotErrorForLogatlib/storage.ts:2255-2267uses these to sanitize errors before logging, preventing filesystem path disclosure without leaking any tokens or emails.
1875-1942:scanNamedBackupsuses bounded concurrency for fs operations.the function at
lib/storage.ts:1886-1922processes backup entries in chunks ofNAMED_BACKUP_LIST_CONCURRENCY(8), limiting concurrentfs.statand file reads.retryTransientFilesystemOperationatlib/storage.ts:1878-1880handles transient readdir failures.since this is read-only scanning, no storage lock is needed.
test/storage.test.ts (12)
3782-3792: test title claims directory creation but assertion is missing.
test/storage.test.ts:3791only checks the boolean return fromclearAccounts(). the test says it "creates the reset marker parent directory" but never verifiesexistsSync(dirname(nestedStoragePath)). without the fs assertion the behavior isn't pinned.suggested fix
await expect(clearAccounts()).resolves.toBe(true); + expect(existsSync(dirname(nestedStoragePath))).toBe(true);
4838-4867: wrap spy cleanup in try/finally to prevent test pollution.
test/storage.test.ts:4866callsrenameSpy.mockRestore()after assertions. ifsaveAccounts()or anyexpect()throws, the spy stays patched and cascades into subsequent tests.suggested fix
- await saveAccounts(storage); - expect(attemptCount).toBe(2); - expect(existsSync(testStoragePath)).toBe(true); - expect(existsSync(walPath)).toBe(false); - - renameSpy.mockRestore(); + try { + await saveAccounts(storage); + expect(attemptCount).toBe(2); + expect(existsSync(testStoragePath)).toBe(true); + expect(existsSync(walPath)).toBe(false); + } finally { + renameSpy.mockRestore(); + }
2789-2844: add the restore snapshot failure regression test.
test/storage.test.ts:2789-2844proves the happy path for pre-restore snapshots, but there's no companion case that forces theaccounts-import-accounts-snapshot-*write to fail on win32 and assertsrestoreNamedBackup()rejects before mutating the current account pool. this leaveslib/storage.tssnapshot-failure abort behavior unpinned for the restore flow.mirror the pattern at
test/storage.test.ts:2935-2997("keeps accounts intact when the pre-import snapshot cannot be written") but drive it throughrestoreNamedBackup()instead ofimportAccounts().as per coding guidelines,
test/**: demand regression cases that reproduce ... windows filesystem behavior.#!/bin/bash # check if a restoreNamedBackup snapshot-failure test exists anywhere rg -n "restoreNamedBackup.*snapshot.*fail|pre-restore.*snapshot.*fail" test/
2711-2759: good win32 retry regression coverage.test properly simulates transient EPERM on win32 during pre-delete snapshot write and verifies retry succeeds. cleanup is wrapped in try/finally. this addresses the windows filesystem behavior regression requirement.
5363-5391: nice race-condition coverage for disappearing backup files.test correctly simulates a backup file being deleted between readdir and readFile, and verifies the listing gracefully excludes the missing entry rather than throwing. spy cleanup is in try/finally.
5584-5611: well-structured retry tests for quota cache clearing.
test/storage.test.ts:5584-5611andtest/storage.test.ts:5613-5641correctly exercise transient EPERM/EBUSY retries and exhaustion scenarios with proper try/finally cleanup.
2222-2291: solid concurrency regression for restore serialization.test at
test/storage.test.ts:2222-2291correctly exercises the race between two concurrentrestoreNamedBackup()calls when the limit is tight, verifying exactly one wins and the other rejects with limit error. this pins the lock-based serialization behavior.
2169-2220: good concurrency-limiter coverage.test at
test/storage.test.ts:2169-2220properly verifies thatlistNamedBackups()respects the concurrency limit by tracking peak simultaneous reads. spy cleanup in try/finally.
627-694: transaction path-pinning coverage looks good.test at
test/storage.test.ts:627-694correctly verifies that changingsetStoragePathDirect()mid-transaction doesn't affect where accounts and snapshots are written — they stay pinned to the captured path.
2406-2471: path redaction test is well-structured.test at
test/storage.test.ts:2406-2471properly verifies that snapshot failure warnings don't leak filesystem paths. module mock cleanup is in try/finally withvi.doUnmock.
5267-5315: rotating backup listing test looks good.
test/storage.test.ts:5267-5315correctly exercises mixed valid/invalid rotating backups and verifies stable labels and metadata.
4522-4550: good coverage for legacy file cleanup in worktree scenarios.test at
test/storage.test.ts:4522-4550properly verifies thatclearAccounts()removes canonical, legacy project, and legacy worktree account files.
| } | ||
| throw error; | ||
| }); | ||
| if (!content) { | ||
| return; | ||
| } | ||
| const lines = content.split(/\r?\n/).filter(Boolean); | ||
| if (lines.length <= MAX_HISTORY_ENTRIES) { | ||
| return; | ||
| } | ||
| const trimmedContent = `${lines.slice(-MAX_HISTORY_ENTRIES).join("\n")}\n`; | ||
| await fs.writeFile(paths.historyPath, trimmedContent, { | ||
| encoding: "utf8", | ||
| mode: 0o600, |
There was a problem hiding this comment.
trimHistoryFileIfNeeded reads the full NDJSON file into memory and then calls fs.writeFile directly on paths.historyPath. writeFile is NOT atomic — it truncates first, then streams bytes. on windows, if antivirus or another process holds an exclusive lock (EPERM/EBUSY), the write can fail partway through, leaving a partially-truncated file. subsequent appendFile calls will then append valid JSON lines after a corrupted prefix, and every future parseEntry / readHistoryTail call on the head of the file will fail silently (returning null), gradually eroding history.
the safe fix is the same atomic pattern used everywhere else in the codebase — write to a .tmp file and rename into place:
const trimmedContent = `${lines.slice(-MAX_HISTORY_ENTRIES).join("\n")}\n`;
const tmpPath = `${paths.historyPath}.${Date.now()}.tmp`;
await fs.writeFile(tmpPath, trimmedContent, { encoding: "utf8", mode: 0o600 });
await fs.rename(tmpPath, paths.historyPath);no vitest coverage exists that mocks a mid-write EPERM and verifies the history file is left intact, so this gap won't be caught by the existing suite.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/sync-history.ts
Line: 175-188
Comment:
**non-atomic trim can silently corrupt the history file on windows**
`trimHistoryFileIfNeeded` reads the full NDJSON file into memory and then calls `fs.writeFile` directly on `paths.historyPath`. `writeFile` is NOT atomic — it truncates first, then streams bytes. on windows, if antivirus or another process holds an exclusive lock (`EPERM`/`EBUSY`), the write can fail partway through, leaving a partially-truncated file. subsequent `appendFile` calls will then append valid JSON lines after a corrupted prefix, and every future `parseEntry` / `readHistoryTail` call on the head of the file will fail silently (returning `null`), gradually eroding history.
the safe fix is the same atomic pattern used everywhere else in the codebase — write to a `.tmp` file and `rename` into place:
```typescript
const trimmedContent = `${lines.slice(-MAX_HISTORY_ENTRIES).join("\n")}\n`;
const tmpPath = `${paths.historyPath}.${Date.now()}.tmp`;
await fs.writeFile(tmpPath, trimmedContent, { encoding: "utf8", mode: 0o600 });
await fs.rename(tmpPath, paths.historyPath);
```
no vitest coverage exists that mocks a mid-write `EPERM` and verifies the history file is left intact, so this gap won't be caught by the existing suite.
How can I resolve this? If you propose a fix, please make it concise.| const flagged = await loadFlaggedAccounts(); | ||
| await snapshotAccountStorage({ | ||
| reason: "delete-account", | ||
| failurePolicy: "error", | ||
| storage: sourceStorage, | ||
| }); |
There was a problem hiding this comment.
loadFlaggedAccounts() is called at line 126 before snapshotAccountStorage at line 127. if the snapshot throws (which it will on windows EPERM/EBUSY since failurePolicy: "error"), the transaction propagates the exception and nothing is persisted — so no data is lost. however, the loaded flagged data may be stale relative to the sourceStorage snapshot if a concurrent writer mutated flagged accounts between the two reads.
more importantly, there is no vitest test that verifies deleteAccountAtIndex correctly propagates the snapshot error and leaves the accounts file fully intact on failurePolicy: "error". the existing test/destructive-actions.test.ts suite covers the happy path but not the "snapshot write fails → transaction aborts → disk state unchanged" invariant. this should be asserted explicitly, particularly for the windows lock scenario.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/destructive-actions.ts
Line: 126-131
Comment:
**flagged accounts loaded before snapshot; stale on snapshot failure**
`loadFlaggedAccounts()` is called at line 126 before `snapshotAccountStorage` at line 127. if the snapshot throws (which it will on windows `EPERM`/`EBUSY` since `failurePolicy: "error"`), the transaction propagates the exception and nothing is persisted — so no data is lost. however, the loaded `flagged` data may be stale relative to the `sourceStorage` snapshot if a concurrent writer mutated flagged accounts between the two reads.
more importantly, there is no vitest test that verifies `deleteAccountAtIndex` correctly propagates the snapshot error and leaves the accounts file fully intact on `failurePolicy: "error"`. the existing `test/destructive-actions.test.ts` suite covers the happy path but not the "snapshot write fails → transaction aborts → disk state unchanged" invariant. this should be asserted explicitly, particularly for the windows lock scenario.
How can I resolve this? If you propose a fix, please make it concise.
|
Closing this preserved |
Summary
Add automatic pre-destructive snapshots for account mutations.
Current head
dev552909aLatest follow-up on this head
clearAccountsUnlocked()on transient Windows filesystem locksValidation
npm run lintnpm run typechecknpm run buildnpm test -- test/storage.test.tsnpm testNotes
Current live actionable feedback on this head is addressed. Any remaining comments should be re-evaluated against
552909a, not older review snapshots.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr introduces automatic pre-destructive snapshots for all account mutation paths (
deleteAccountAtIndex,deleteSavedAccounts,resetLocalState,importAccounts,restoreNamedBackup) and extracts them into a dedicatedlib/destructive-actions.tsmodule. it also addslib/sync-history.tsfor NDJSON-backed sync run history, extendslib/storage.tswith named-backup scanning/assessment/restore APIs, and pins transaction-scoped writes to the storage path captured at transaction start — directly addressing the core class of storage-path drift issues from prior review cycles.key changes:
snapshotAccountStoragenow resolvesstoragePathfromtransactionSnapshotContextwhen inside a transaction, eliminating the stale-path risk on concurrentsetStoragePathDirect()callsimportNormalizedAccountsmoves the snapshot call to after the limit check but still insidewithAccountStorageTransaction, closing the TOCTOU window from the previous headrestoreNamedBackupandimportAccountsboth passsnapshotFailurePolicy: "error", so a failed backup write aborts the import rather than silently proceedingsnapshotAndClearAccountsuseswithStorageLock+failurePolicy: "error", hardening the clear path against concurrent mutationstrimHistoryFileIfNeededinlib/sync-history.tswrites back the trimmed content with a barefs.writeFileon the live file — not atomic. on windows,EPERM/EBUSYfrom AV during that write can leave the NDJSON file partially truncated, corrupting all subsequent line parsing; no vitest case covers this failure pathsnapshotAndClearAccountssilently skips the snapshot (returnsnull) when the accounts file is empty or missing, then still callsclearAccountsUnlocked— thefailurePolicy: "error"contract is not enforced for the empty-store case, leaving no audit trail; no vitest assertion locks this behavior downdeleteAccountAtIndexloads flagged accounts before taking the snapshot; on a snapshotEPERMthe snapshot throws and the transaction aborts correctly, but there is no test verifying the disk-state-unchanged invariant after that failure pathConfidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller participant DestructiveActions as lib/destructive-actions.ts participant Storage as lib/storage.ts participant Disk Note over Caller,Disk: deleteAccountAtIndex Caller->>DestructiveActions: deleteAccountAtIndex({storage, index}) DestructiveActions->>Storage: withAccountAndFlaggedStorageTransaction() Storage->>Storage: withStorageLock → loadAccountsInternal()<br/>pins storagePath in transactionSnapshotContext Storage->>Storage: findMatchingAccountIndex(sourceStorage, requestedTarget) DestructiveActions->>Storage: loadFlaggedAccounts() DestructiveActions->>Storage: snapshotAccountStorage({failurePolicy:"error", storage:sourceStorage}) Storage->>Disk: createNamedBackup(accounts-delete-account-snapshot-*.json) alt snapshot EPERM/EBUSY on Windows Disk-->>Storage: throws Storage-->>DestructiveActions: throws → transaction aborts else snapshot OK Disk-->>Storage: NamedBackupMetadata DestructiveActions->>Storage: persist(nextStorage, updatedFlagged) Storage->>Disk: saveAccountsUnlocked (pinned storagePath) Storage->>Disk: saveFlaggedAccountsUnlocked (pinned storagePath) Storage-->>Caller: DeleteAccountResult end Note over Caller,Disk: deleteSavedAccounts / resetLocalState Caller->>DestructiveActions: deleteSavedAccounts() DestructiveActions->>Storage: snapshotAndClearAccounts("delete-saved-accounts") Storage->>Storage: withStorageLock → loadAccountsInternal() alt currentStorage null or empty Storage->>Storage: snapshotAccountStorage returns null (no snapshot written) Storage->>Disk: clearAccountsUnlocked(storagePath) else accounts exist Storage->>Disk: createNamedBackup(accounts-delete-saved-accounts-snapshot-*.json) alt snapshot fails Disk-->>Storage: throws → clear aborted else snapshot OK Storage->>Disk: clearAccountsUnlocked(storagePath) end endComments Outside Diff (2)
lib/storage.ts, line 1240-1261 (link)restoreNamedBackupwithout assessment skips email deduplication gatewhen called without
options.assessment,restoreNamedBackupgoes directly toimportNormalizedAccounts, which deduplicates only by token viadeduplicateAccounts. butassessNamedBackupRestoreCandidatealso appliesdeduplicateAccountsByEmail. this creates a split:eligibleForRestore=false, "Backup would not change any accounts"restoreNamedBackup(name)(no assessment) callsimportNormalizedAccounts, which treats different tokens as distinct and imports the backup copy alongside the existing one → two accounts with the same emailall current callers in
codex-manager.tspass{ assessment }, so this path is not reachable through the UI today. but the function is exported and the gap will silently widen as new entry points are added. applying email deduplication insideimportNormalizedAccountswould match the assessment behavior:missing vitest: no test calls
restoreNamedBackup(name)without assessment where the backup shares an email but differs in token from an existing account, then asserts the email-duplicate account is not added.Prompt To Fix With AI
lib/storage.ts, line 3507-3525 (link)snapshotAndClearAccounts— empty-storage fast path bypassesfailurePolicy: "error"intentwhen
currentStorageis null or has zero accounts,snapshotAccountStorageshort-circuits and returnsnullwithout attempting a write (line ~2230 in the function body). this meanssnapshotAndClearAccountswill proceed directly toclearAccountsUnlockedwith no snapshot on disk and no error — even though the caller passedfailurePolicy: "error"expecting a fail-closed guarantee.in practice, deleting an empty store is harmless, but the contract of
deleteSavedAccounts/resetLocalStateimplies "snapshot or abort". a user who sees theaccounts-delete-saved-accounts-snapshot-*file exist in a previous run but NOT in the current one (because the store was empty when they triggered the action) has no audit trail, and support tooling that relies on snapshot presence for diagnostics will silently miss these events.no vitest case asserts the behavior of
snapshotAndClearAccountswhen the accounts file is missing or empty; adding one would lock down the current intent and catch regressions.Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "fix(storage): harden..."