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. |
📝 WalkthroughWalkthroughadds a transactional, snapshot-first destructive actions module and a large expansion of storage snapshot/backup APIs (named + 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 | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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
| it("clears the pre-delete snapshot before releasing the lock to newer saves", async () => { | ||
| const originalWriteFile = fs.writeFile.bind(fs); | ||
| let releaseSnapshotWrite: (() => void) | undefined; | ||
| const snapshotWriteBlocked = new Promise<void>((resolve) => { | ||
| releaseSnapshotWrite = resolve; | ||
| }); | ||
| let snapshotWriteStarted = false; | ||
| const writeFileSpy = vi | ||
| .spyOn(fs, "writeFile") | ||
| .mockImplementation(async (path, data, options) => { | ||
| if ( | ||
| !snapshotWriteStarted && | ||
| String(path).includes("accounts-delete-saved-accounts-snapshot-") | ||
| ) { | ||
| snapshotWriteStarted = true; | ||
| await snapshotWriteBlocked; | ||
| } | ||
| return originalWriteFile(path, data as never, options as never); | ||
| }); | ||
|
|
||
| try { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "delete-original", | ||
| refreshToken: "ref-delete-original", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| const deletePromise = deleteSavedAccounts(); | ||
| await vi.waitFor(() => { | ||
| expect(snapshotWriteStarted).toBe(true); | ||
| }); | ||
|
|
||
| const concurrentSave = saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "saved-after-delete", | ||
| refreshToken: "ref-saved-after-delete", | ||
| addedAt: 2, | ||
| lastUsed: 2, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| releaseSnapshotWrite?.(); | ||
|
|
||
| await expect(deletePromise).resolves.toMatchObject({ | ||
| accountsCleared: true, | ||
| }); | ||
| await expect(concurrentSave).resolves.toBeUndefined(); | ||
|
|
||
| expect((await loadAccounts())?.accounts).toEqual([ | ||
| expect.objectContaining({ accountId: "saved-after-delete" }), | ||
| ]); | ||
|
|
||
| const entries = await fs.readdir(getNamedBackupsDirectoryPath()); | ||
| expect( | ||
| entries.some((name) => | ||
| name.startsWith("accounts-delete-saved-accounts-snapshot-"), | ||
| ), | ||
| ).toBe(true); | ||
| } finally { | ||
| releaseSnapshotWrite?.(); | ||
| writeFileSpy.mockRestore(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
add the win32 snapshot regression here.
test/storage.test.ts:2439-2540 proves the lock ordering, but it never drives the new snapshot path through the windows eprem/ebusy branch. that leaves lib/storage.ts:2175-2278 and lib/storage.ts:3409-3422 unpinned on the exact filesystem behavior this pr is hardening. please add a deterministic vitest that flips process.platform to win32 and forces a transient named-backup write failure before deleteSavedAccounts() or resetLocalState() continue.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Also applies to: 2514-2540
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 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).
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.
| const result = await deleteSavedAccounts(); | ||
| invalidateAccountManagerCache(); | ||
| console.log( | ||
| "\nCleared saved accounts from active storage. Recovery snapshots remain available. Starting fresh.\n", | ||
| `\n${ | ||
| result.accountsCleared | ||
| ? DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.completed | ||
| : "Delete saved accounts completed with warnings. Some saved account artifacts could not be removed; see logs." | ||
| }\n`, | ||
| ); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| if (menuResult.mode === "reset") { | ||
| startFresh = true; | ||
| const result = await resetLocalState(); |
There was a problem hiding this comment.
deleteSavedAccounts() and resetLocalState() both call snapshotAndClearAccounts, which now uses failurePolicy: "error". that means an EPERM/EBUSY from antivirus or a locked file on windows will throw before the clear even runs. in the TUI path (lib/codex-manager.ts), those calls are inside runActionPanel + try/finally so errors are caught and displayed. here in the fallback (non-TUI) path there is no try/catch, so the error propagates out of the login loop entirely and can crash the process.
this is a regression: the old clearAccounts() call only threw on fundamental FS errors; the new path adds the snapshot-write failure as a new throw surface.
| const result = await deleteSavedAccounts(); | |
| invalidateAccountManagerCache(); | |
| console.log( | |
| "\nCleared saved accounts from active storage. Recovery snapshots remain available. Starting fresh.\n", | |
| `\n${ | |
| result.accountsCleared | |
| ? DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.completed | |
| : "Delete saved accounts completed with warnings. Some saved account artifacts could not be removed; see logs." | |
| }\n`, | |
| ); | |
| } | |
| break; | |
| } | |
| if (menuResult.mode === "reset") { | |
| startFresh = true; | |
| const result = await resetLocalState(); | |
| if (menuResult.mode === "fresh") { | |
| startFresh = true; | |
| if (menuResult.deleteAll) { | |
| try { | |
| const result = await deleteSavedAccounts(); | |
| invalidateAccountManagerCache(); | |
| console.log( | |
| `\n${ | |
| result.accountsCleared | |
| ? DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.completed | |
| : "Delete saved accounts completed with warnings. Some saved account artifacts could not be removed; see logs." | |
| }\n`, | |
| ); | |
| } catch (error) { | |
| console.error( | |
| `\nDelete saved accounts failed: ${error instanceof Error ? error.message : String(error)}\n`, | |
| ); | |
| } | |
| } | |
| break; | |
| } | |
| if (menuResult.mode === "reset") { | |
| startFresh = true; | |
| try { | |
| const result = await resetLocalState(); | |
| invalidateAccountManagerCache(); | |
| console.log( | |
| `\n${ | |
| result.accountsCleared && | |
| result.flaggedCleared && | |
| result.quotaCacheCleared | |
| ? DESTRUCTIVE_ACTION_COPY.resetLocalState.completed | |
| : "Reset local state completed with warnings. Some local artifacts could not be removed; see logs." | |
| }\n`, | |
| ); | |
| } catch (error) { | |
| console.error( | |
| `\nReset local state failed: ${error instanceof Error ? error.message : String(error)}\n`, | |
| ); | |
| } | |
| break; | |
| } |
no vitest coverage for this failure path either — there is no test that verifies the fallback menu survives a snapshot write error without crashing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: index.ts
Line: 3149-3164
Comment:
**uncaught snapshot failure crashes fallback path on windows**
`deleteSavedAccounts()` and `resetLocalState()` both call `snapshotAndClearAccounts`, which now uses `failurePolicy: "error"`. that means an `EPERM`/`EBUSY` from antivirus or a locked file on windows will throw before the clear even runs. in the TUI path (`lib/codex-manager.ts`), those calls are inside `runActionPanel` + `try/finally` so errors are caught and displayed. here in the fallback (non-TUI) path there is no try/catch, so the error propagates out of the login loop entirely and can crash the process.
this is a regression: the old `clearAccounts()` call only threw on fundamental FS errors; the new path adds the snapshot-write failure as a new throw surface.
```suggestion
if (menuResult.mode === "fresh") {
startFresh = true;
if (menuResult.deleteAll) {
try {
const result = await deleteSavedAccounts();
invalidateAccountManagerCache();
console.log(
`\n${
result.accountsCleared
? DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.completed
: "Delete saved accounts completed with warnings. Some saved account artifacts could not be removed; see logs."
}\n`,
);
} catch (error) {
console.error(
`\nDelete saved accounts failed: ${error instanceof Error ? error.message : String(error)}\n`,
);
}
}
break;
}
if (menuResult.mode === "reset") {
startFresh = true;
try {
const result = await resetLocalState();
invalidateAccountManagerCache();
console.log(
`\n${
result.accountsCleared &&
result.flaggedCleared &&
result.quotaCacheCleared
? DESTRUCTIVE_ACTION_COPY.resetLocalState.completed
: "Reset local state completed with warnings. Some local artifacts could not be removed; see logs."
}\n`,
);
} catch (error) {
console.error(
`\nReset local state failed: ${error instanceof Error ? error.message : String(error)}\n`,
);
}
break;
}
```
no vitest coverage for this failure path either — there is no test that verifies the fallback menu survives a snapshot write error without crashing.
How can I resolve this? If you propose a fix, please make it concise.
| 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
| if (snapshotReason) { | ||
| await snapshotAccountStorage({ | ||
| reason: snapshotReason, | ||
| failurePolicy: snapshotFailurePolicy, | ||
| storage: existing, | ||
| storagePath: getStoragePath(), | ||
| }); |
There was a problem hiding this comment.
pin transaction snapshots and writes to the captured storage path.
lib/storage.ts:3306-3318 and lib/storage.ts:3333-3349 capture state.storagePath, but the transaction still snapshots and persists through helpers that resolve getStoragePath() again. lib/storage.ts:2707-2713 has the same issue for the pre-import snapshot. if setStoragePath() or setStoragePathDirect() changes during an active transaction, the code can read one profile and commit or snapshot another. please thread the captured path through the unlocked save/snapshot helpers and add a vitest that flips the storage path mid-transaction for both transaction helpers.
As per coding guidelines, lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Also applies to: 3306-3318, 3333-3349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 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.
| it("creates a snapshot before restoring a named backup", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "current", | ||
| refreshToken: "ref-current", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
| await createNamedBackup("restore-with-snapshot"); | ||
|
|
||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "replacement", | ||
| refreshToken: "ref-replacement", | ||
| addedAt: 2, | ||
| lastUsed: 2, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| await restoreNamedBackup("restore-with-snapshot"); | ||
|
|
||
| const entries = await fs.readdir(getNamedBackupsDirectoryPath()); | ||
| const restoreSnapshot = entries.find((name) => | ||
| name.startsWith("accounts-import-accounts-snapshot-"), | ||
| ); | ||
| expect(restoreSnapshot).toBeTruthy(); | ||
| const snapshotContent = JSON.parse( | ||
| await fs.readFile( | ||
| join(getNamedBackupsDirectoryPath(), restoreSnapshot!), | ||
| "utf-8", | ||
| ), | ||
| ); | ||
| expect(snapshotContent.accounts).toEqual([ | ||
| expect.objectContaining({ | ||
| accountId: "replacement", | ||
| refreshToken: "ref-replacement", | ||
| }), | ||
| ]); | ||
| expect(await loadAccounts()).toMatchObject({ | ||
| accounts: expect.arrayContaining([ | ||
| expect.objectContaining({ | ||
| accountId: "current", | ||
| refreshToken: "ref-current", | ||
| }), | ||
| ]), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
add the restore snapshot failure regression here.
test/storage.test.ts:2631-2686 only proves the happy path. add the companion case that forces the accounts-import-accounts-snapshot-... write to fail on win32 and asserts restoreNamedBackup() rejects before it mutates the current pool. mirroring the abort coverage you already added at test/storage.test.ts:2777-2839 would pin the destructive edge this pr is hardening.
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 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).
| it("creates the reset marker parent directory when the storage path is brand new", async () => { | ||
| const nestedStoragePath = join( | ||
| testWorkDir, | ||
| "brand-new", | ||
| "nested", | ||
| "accounts.json", | ||
| ); | ||
| setStoragePathDirect(nestedStoragePath); | ||
|
|
||
| await expect(clearAccounts()).resolves.toBe(true); | ||
| }); |
There was a problem hiding this comment.
this branch is still effectively untested.
test/storage.test.ts:3624-3634 only checks the boolean return. if clearAccounts() stops creating the brand-new parent/reset-marker directory, this still passes, so the behavior named by the test never gets pinned.
suggested fix
await expect(clearAccounts()).resolves.toBe(true);
+ expect(existsSync(dirname(nestedStoragePath))).toBe(true);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.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("creates the reset marker parent directory when the storage path is brand new", async () => { | |
| const nestedStoragePath = join( | |
| testWorkDir, | |
| "brand-new", | |
| "nested", | |
| "accounts.json", | |
| ); | |
| setStoragePathDirect(nestedStoragePath); | |
| await expect(clearAccounts()).resolves.toBe(true); | |
| }); | |
| it("creates the reset marker parent directory when the storage path is brand new", async () => { | |
| const nestedStoragePath = join( | |
| testWorkDir, | |
| "brand-new", | |
| "nested", | |
| "accounts.json", | |
| ); | |
| setStoragePathDirect(nestedStoragePath); | |
| await expect(clearAccounts()).resolves.toBe(true); | |
| expect(existsSync(dirname(nestedStoragePath))).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 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.
| it("retries on EAGAIN and cleans up the WAL after rename succeeds", async () => { | ||
| const now = Date.now(); | ||
| const storage = { | ||
| version: 3 as const, | ||
| activeIndex: 0, | ||
| accounts: [{ refreshToken: "token", addedAt: now, lastUsed: now }], | ||
| }; | ||
| const walPath = `${testStoragePath}.wal`; | ||
|
|
||
| const originalRename = fs.rename.bind(fs); | ||
| let attemptCount = 0; | ||
| const renameSpy = vi | ||
| .spyOn(fs, "rename") | ||
| .mockImplementation(async (oldPath, newPath) => { | ||
| attemptCount++; | ||
| if (attemptCount === 1) { | ||
| const err = new Error("EAGAIN error") as NodeJS.ErrnoException; | ||
| err.code = "EAGAIN"; | ||
| throw err; | ||
| } | ||
| return originalRename(oldPath as string, newPath as string); | ||
| }); | ||
|
|
||
| await saveAccounts(storage); | ||
| expect(attemptCount).toBe(2); | ||
| expect(existsSync(testStoragePath)).toBe(true); | ||
| expect(existsSync(walPath)).toBe(false); | ||
|
|
||
| renameSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual code at the specified lines
sed -n '4680,4709p' test/storage.test.tsRepository: ndycode/codex-multi-auth
Length of output: 985
🏁 Script executed:
# Search for other mockRestore calls in the test file to identify similar patterns
rg "mockRestore\(\)" test/storage.test.ts -B 5 -A 2Repository: ndycode/codex-multi-auth
Length of output: 6870
🏁 Script executed:
# Check if there are other tests in this describe block that might be affected
sed -n '4600,4750p' test/storage.test.ts | grep -E "it\(|describe\(|mockRestore"Repository: ndycode/codex-multi-auth
Length of output: 585
restore renameSpy in a finally block.
test/storage.test.ts:4680-4709 calls renameSpy.mockRestore() after assertions without a finally block. if saveAccounts() or any expect() throws, the spy stays patched and cascades failures through subsequent retry tests in this describe block.
the codebase uses try/finally for spy cleanup everywhere else—this test stands out. it's a good regression case for EAGAIN retry logic with WAL cleanup, but the cleanup pattern needs to match.
the same issue exists in the other four tests in "saveAccounts EPERM/EBUSY retry logic" describe block.
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();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("retries on EAGAIN and cleans up the WAL after rename succeeds", async () => { | |
| const now = Date.now(); | |
| const storage = { | |
| version: 3 as const, | |
| activeIndex: 0, | |
| accounts: [{ refreshToken: "token", addedAt: now, lastUsed: now }], | |
| }; | |
| const walPath = `${testStoragePath}.wal`; | |
| const originalRename = fs.rename.bind(fs); | |
| let attemptCount = 0; | |
| const renameSpy = vi | |
| .spyOn(fs, "rename") | |
| .mockImplementation(async (oldPath, newPath) => { | |
| attemptCount++; | |
| if (attemptCount === 1) { | |
| const err = new Error("EAGAIN error") as NodeJS.ErrnoException; | |
| err.code = "EAGAIN"; | |
| throw err; | |
| } | |
| return originalRename(oldPath as string, newPath as string); | |
| }); | |
| await saveAccounts(storage); | |
| expect(attemptCount).toBe(2); | |
| expect(existsSync(testStoragePath)).toBe(true); | |
| expect(existsSync(walPath)).toBe(false); | |
| renameSpy.mockRestore(); | |
| }); | |
| it("retries on EAGAIN and cleans up the WAL after rename succeeds", async () => { | |
| const now = Date.now(); | |
| const storage = { | |
| version: 3 as const, | |
| activeIndex: 0, | |
| accounts: [{ refreshToken: "token", addedAt: now, lastUsed: now }], | |
| }; | |
| const walPath = `${testStoragePath}.wal`; | |
| const originalRename = fs.rename.bind(fs); | |
| let attemptCount = 0; | |
| const renameSpy = vi | |
| .spyOn(fs, "rename") | |
| .mockImplementation(async (oldPath, newPath) => { | |
| attemptCount++; | |
| if (attemptCount === 1) { | |
| const err = new Error("EAGAIN error") as NodeJS.ErrnoException; | |
| err.code = "EAGAIN"; | |
| throw err; | |
| } | |
| return originalRename(oldPath as string, newPath as string); | |
| }); | |
| try { | |
| await saveAccounts(storage); | |
| expect(attemptCount).toBe(2); | |
| expect(existsSync(testStoragePath)).toBe(true); | |
| expect(existsSync(walPath)).toBe(false); | |
| } finally { | |
| renameSpy.mockRestore(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 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.
Summary
Add automatic pre-destructive snapshots for account mutations.
Current head
dev5628dc4Latest follow-up on this head
Validation
npm run lintnpm run typechecknpm run buildnpm testNotes
Remaining current-head bot feedback is either stale in substance or hardening-level follow-up rather than a verified blocker on this PR.