feat(auth): add backup restore manager#76
feat(auth): add backup restore manager#76ndycode wants to merge 17 commits intogit-plan/01-reset-safetyfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughthis pr adds a named-backup restore feature: storage APIs to create/list/assess/restore named backups, cli/login support for a new Changes
Sequence diagramsequenceDiagram
participant user as User
participant auth as Auth Menu
participant mgr as BackupRestoreManager
participant storage as Storage
participant fs as File System
user->>auth: select "restore from backup"
auth->>mgr: runBackupRestoreManager()
mgr->>storage: listNamedBackups()
storage->>fs: read backups dir
fs-->>storage: backup entries
storage-->>mgr: NamedBackupMetadata[]
mgr->>storage: assessNamedBackupRestore(name) (batched)
storage->>fs: read backup file
fs-->>storage: backup content
storage-->>mgr: BackupRestoreAssessment
mgr->>user: display menu with hints
user->>mgr: choose backup
mgr->>storage: reassess assessNamedBackupRestore(name)
mgr->>user: prompt confirm()
user->>mgr: confirm
mgr->>storage: restoreNamedBackup(name)
storage->>fs: merge/import accounts
fs-->>storage: write results
storage-->>mgr: {imported, total, skipped}
mgr->>user: show result / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes reviewable concerns
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:316">
P2: Use retry-based cleanup instead of bare `fs.rm` in test teardown to avoid Windows EBUSY/EPERM/ENOTEMPTY flakiness.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:475">
P2: Backup name normalization in path resolution breaks restore for listed backup files whose filenames contain spaces or other non-normalized characters.</violation>
</file>
<file name="test/codex-manager-cli.test.ts">
<violation number="1" location="test/codex-manager-cli.test.ts:1609">
P3: `vi.doMock` here is never unmocked. `vi.resetModules()` does not clear the mock registry, so this confirm mock will leak into later tests and can change their behavior. Add a `vi.doUnmock`/`vi.unmock` cleanup (e.g., in `afterEach`) to keep tests isolated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:132">
P2: Replace the bare `fs.rm` in test cleanup with retry/backoff to avoid Windows CI flakes on transient file locks.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:476">
P2: `resolveNamedBackupPath` allows path traversal via unsanitized backup names before directory confinement.</violation>
</file>
<file name="lib/cli.ts">
<violation number="1" location="lib/cli.ts:283">
P2: `restore-backup` is wired only in the TUI path; the fallback login prompt cannot select this new mode.</violation>
</file>
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4171">
P2: Handle restoreNamedBackup errors so a missing/invalid backup or limit breach doesn’t crash the auth menu.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/cli.ts (1)
49-61:⚠️ Potential issue | 🟠 Majorthe new mode is still unreachable in the readline fallback menu.
this only wires
restore-backupthrough the tty path.promptLoginModeFallback()inlib/cli.ts:174-243and its prompt copy inlib/ui/copy.ts:135-137still never advertise or parse the mode, so users in fallback environments cannot reach the restore flow even thoughrunAuthLogin()handles it atlib/codex-manager.ts:3909-3919. please add a vitest for the fallback branch.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.Also applies to: 291-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/cli.ts` around lines 49 - 61, The fallback readline menu never exposes or parses the new "restore-backup" LoginMode, so add it to promptLoginModeFallback() (ensure the option label and value include "restore-backup") and update the fallback prompt text in lib/ui/copy.ts to advertise the restore flow (matching how runAuthLogin() in lib/codex-manager.ts handles "restore-backup"); add a vitest that exercises the fallback branch to assert the prompt lists and correctly parses "restore-backup", and update any affected tests to reference the new behavior; while modifying prompts/tests, verify any updated auth rotation or queue logic touched by this change properly retries or surfaces EBUSY/429 errors per existing concurrency and Windows IO guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/storage-paths.md`:
- Line 25: Update docs/reference/storage-paths.md to document the restore entry
point and user workflow for the new backup paths: add a short step-by-step
showing "codex auth login" → select recovery → choose 'restore from backup' and
point readers to the interactive restore implementation in lib/codex-manager.ts
(see the interactive flow around the blocks at ~3909-3919 and the restore logic
at ~4211-4291) so users know how to consume files under
~/.codex/multi-auth/backups/<name>.json; also add or link an upgrade note that
mentions the changed CLI workflow and any new npm scripts introduced so README,
SECURITY, and docs remain consistent with the shipped flags and behavior.
In `@lib/codex-manager.ts`:
- Around line 3909-3920: The recovery menu (restore-backup) is never reachable
because runAuthLogin() only enters the menu loop when
existingStorage.accounts.length > 0; hoist the recovery entry so
runBackupRestoreManager(...) can be invoked even when existingStorage.accounts
is empty: move the restore-backup handling (call to runBackupRestoreManager and
its try/catch) to execute before the non-empty accounts guard in runAuthLogin,
ensuring the menu can present the recovery path on fresh/reset/missing storage;
add a vitest that simulates zero-account storage and asserts the restore flow is
offered and that runBackupRestoreManager is called; update any affected tests
referenced in the auth/login suite and ensure new queue logic (if added) handles
EBUSY and 429 retries per concurrency guidelines.
In `@lib/storage.ts`:
- Around line 1712-1728: loadBackupCandidate currently converts any read error
into a permanent invalid-backup result; change it to detect transient Windows/IO
errors (EPERM, EBUSY, EAGAIN) and retry the read (call to loadAccountsFromPath)
with an exponential/backoff loop (same retry envelope used elsewhere in this
module) before returning an error, and apply the same retry logic to the paired
fs.stat call in this file that checks backup file metadata; add a vitest that
simulates a transient busy/EPERM/EBUSY/EAGAIN failure on the first N attempts
and succeeds thereafter to assert the retry path is exercised and cite/update
the affected tests.
- Around line 1580-1609: The function listNamedBackups currently swallows all
readdir errors except ENOENT by logging and returning an empty array; change the
catch in listNamedBackups so that only ENOENT returns [] while any other error
is re-thrown (preserve the original error object) so callers like
runBackupRestoreManager surface real filesystem errors (EPERM/EBUSY). Update
tests: add a vitest that simulates an unreadable backup directory (make readdir
throw EPERM/EBUSY) and assert that listNamedBackups rejects/throws rather than
returning an empty array. Keep references to getNamedBackupRoot/getStoragePath
for locating the path and ensure loadBackupCandidate/buildNamedBackupMetadata
logic is untouched.
In `@test/codex-manager-cli.test.ts`:
- Around line 2365-2428: Add a regression test that covers the confirm=false
path so restore is blocked: duplicate the existing "restores a named backup..."
test setup (use setInteractiveTTY, mock loadAccountsMock, listNamedBackupsMock,
assessNamedBackupRestoreMock, promptLoginModeMock and selectMock) but make
confirmMock resolve to false and then call
runCodexMultiAuthCli(["auth","login"]); assert exitCode is non-zero (or
indicates cancellation) and that restoreNamedBackupMock was not called; keep
other expectations similar (list/assess called once, select used) to ensure the
safety gate implemented by confirmMock is exercised.
In `@test/storage.test.ts`:
- Around line 1111-1185: The tests cover happy paths but lack regression cases
that ensure restoreNamedBackup and assessNamedBackupRestore properly surface
failures for invalid or missing backup payloads; add deterministic vitest tests
in test/storage.test.ts that (1) create a named backup file with invalid JSON
and assert assessNamedBackupRestore(...) and restoreNamedBackup(...) both
reject/throw, (2) create a backup JSON missing required fields (e.g., no
"accounts") and assert both functions reject with a clear error, and (3) assert
behavior is consistent when a backup file is empty or truncated; use the same
helper functions (createNamedBackup, listNamedBackups, assessNamedBackupRestore,
restoreNamedBackup, clearAccounts, loadAccounts) and filesystem writes to inject
the malformed payloads so the tests reproduce the error-path regressions
deterministically under vitest.
---
Outside diff comments:
In `@lib/cli.ts`:
- Around line 49-61: The fallback readline menu never exposes or parses the new
"restore-backup" LoginMode, so add it to promptLoginModeFallback() (ensure the
option label and value include "restore-backup") and update the fallback prompt
text in lib/ui/copy.ts to advertise the restore flow (matching how
runAuthLogin() in lib/codex-manager.ts handles "restore-backup"); add a vitest
that exercises the fallback branch to assert the prompt lists and correctly
parses "restore-backup", and update any affected tests to reference the new
behavior; while modifying prompts/tests, verify any updated auth rotation or
queue logic touched by this change properly retries or surfaces EBUSY/429 errors
per existing concurrency and Windows IO guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8fa6ad5-4c6c-4110-af7b-d6c8d1c2f62c
📒 Files selected for processing (8)
docs/reference/storage-paths.mdlib/cli.tslib/codex-manager.tslib/storage.tslib/ui/auth-menu.tslib/ui/copy.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/ui/auth-menu.tslib/codex-manager.tslib/ui/copy.tslib/storage.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/storage-paths.md
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/codex-manager-cli.test.ts
🔇 Additional comments (3)
test/storage.test.ts (1)
333-336: good windows-safe teardown change.line 335 in
test/storage.test.ts:335usesremoveWithRetry, which reduces eperm/ebusy teardown flakes on windows.test/codex-manager-cli.test.ts (2)
506-539: good deterministic mock reset/default setup.line 506 through line 539 in
test/codex-manager-cli.test.tscorrectly resets and reseeds new restore-related mocks, which prevents cross-test state bleed.
2430-2500: good restore-failure recovery coverage.line 2430 through line 2500 in
test/codex-manager-cli.test.tsverifies restore failure is surfaced and the login flow continues, which is the right user-safety behavior.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1208">
P3: Duplicate test case: this restore-after-deletion scenario is already covered earlier in the same file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@cubic-dev-ai review this PR |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1351">
P2: Use `removeWithRetry` instead of bare `fs.rm` to avoid transient Windows file-lock failures in tests.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:1807">
P2: Do not swallow non-ENOENT errors when scanning named backups; rethrow them so restore reports the real filesystem failure instead of a misleading “file not found.”</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai review this PR |
|
@coderabbitai full review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/codex-manager-cli.test.ts">
<violation number="1" location="test/codex-manager-cli.test.ts:2639">
P2: The new restore-menu concurrency test can deadlock when the implementation uses fewer than 8 workers, because deferred assessments are only released at `pending.length === 8` or on the final backup.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1351">
P2: Use `removeWithRetry` instead of direct `fs.rm` here to avoid Windows EBUSY/EPERM test flakes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4224-4230: The current fan-out uses Promise.all in the chunk
mapping (the chunk variable building assessments into assessments.push(...await
Promise.all(...))) which causes one rejected assessNamedBackupRestore to abort
the whole chunk; change it to use Promise.allSettled (or individually catch
per-assessment) so successful assessments still return while failures are logged
and skipped, ensuring assessNamedBackupRestore(backup.name, { currentStorage })
rejections don't throw the whole flow. Add two vitest cases in
test/codex-manager-cli.test.ts: (1) a regression where one
assessNamedBackupRestore rejects but others are selectable/rendered, asserting
the restore menu still shows healthy backups; (2) a fan-out test that validates
chunking honors the configured cap and does not exceed parallel requests. Also
ensure any retry/queue logic around assessNamedBackupRestore handles transient
EBUSY/429 with backoff, that logging avoids leaking tokens/emails, and update
assertions in affected tests to reference the new non-fail-fast behavior.
In `@lib/storage.ts`:
- Around line 1581-1624: The directory reads in listNamedBackups and
findExistingNamedBackupPath must be retried on transient Windows filesystem
errors: replace the direct fs.readdir calls inside listNamedBackups and the
confirm-time lookup in findExistingNamedBackupPath with calls wrapped by
retryTransientFilesystemOperation(async () => fs.readdir(...)), so transient
EBUSY/EPERM/EAGAIN are retried but genuine ENOENT still returns [] (in
listNamedBackups) or bubbles after retries (in findExistingNamedBackupPath);
preserve existing behavior around loadBackupCandidate and
buildNamedBackupMetadata, keep logging but avoid leaking secrets, and add a
vitest that injects transient errors (EBUSY/EAGAIN) for the backup directory
read to assert retries happen and final behavior matches current tests (cite the
restore tests in test/codex-manager-cli.test.ts).
In `@test/codex-manager-cli.test.ts`:
- Around line 2541-2611: The test currently verifies the error log and exit code
but doesn't assert the UI loop resumed; update the test that exercises
runCodexMultiAuthCli(["auth","login"]) to assert promptLoginModeMock was invoked
twice to prove control returned to the login menu (use
expect(promptLoginModeMock).toHaveBeenCalledTimes(2)); locate the failing test
around the it("catches restore failures and returns to the login menu"...) block
and add that assertion (you can also assert selectMock was called for the
restore selection if helpful) so the "stay in menu" flow is locked in.
In `@test/storage.test.ts`:
- Around line 24-25: The test currently hardcodes the concurrency limit value
(the magic number 8) in the concurrency regression assertion; replace that
literal with the canonical constant NAMED_BACKUP_LIST_CONCURRENCY so the test
tracks the real contract if the limit changes, update assertions around
listNamedBackups (and any related assertions between lines referenced) to
reference NAMED_BACKUP_LIST_CONCURRENCY rather than 8, and keep the test
deterministic under vitest by relying on the exported constant used by the
implementation instead of a hardcoded value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b96886a9-579e-418c-bdc7-dc80336851df
📒 Files selected for processing (8)
docs/reference/storage-paths.mdlib/cli.tslib/codex-manager.tslib/storage.tslib/ui/copy.tstest/cli.test.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/ui/copy.tslib/codex-manager.tslib/storage.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/cli.test.tstest/codex-manager-cli.test.tstest/storage.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/storage-paths.md
🔇 Additional comments (2)
test/storage.test.ts (1)
334-337: good windows-safe teardown update.using
removeWithRetryattest/storage.test.ts:336(line 336) is the right move for transient windows file locks and keeps this suite less flaky.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.lib/codex-manager.ts (1)
3826-3830: good fix: restore menu is now reachable with zero accounts.
lib/codex-manager.ts:3827(line 3827) now falls back tocreateEmptyAccountStorage(), so recovery actions can render even when storage is missing/empty.
|
@cubic-dev-ai review this PR |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4227">
P2: The restore menu does redundant filesystem work by loading every backup in `listNamedBackups()` and then reloading every backup again in `assessNamedBackupRestore()`, which can noticeably slow menu rendering with many backups.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:1799">
P2: Restore lookup can fail for backups that are listed with uppercase `.JSON` extensions because matching is stricter than listing on non-Windows filesystems.</violation>
</file>
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1268">
P2: Use `removeWithRetry` instead of `fs.unlink` here to avoid transient Windows file-lock flakes in this restore test.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
lib/codex-manager.ts
Outdated
| assessments.push( | ||
| ...(await Promise.all( | ||
| chunk.map((backup) => | ||
| assessNamedBackupRestore(backup.name, { currentStorage }), |
There was a problem hiding this comment.
P2: The restore menu does redundant filesystem work by loading every backup in listNamedBackups() and then reloading every backup again in assessNamedBackupRestore(), which can noticeably slow menu rendering with many backups.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/codex-manager.ts, line 4227:
<comment>The restore menu does redundant filesystem work by loading every backup in `listNamedBackups()` and then reloading every backup again in `assessNamedBackupRestore()`, which can noticeably slow menu rendering with many backups.</comment>
<file context>
@@ -4173,6 +4196,115 @@ export async function autoSyncActiveAccountToCodex(): Promise<boolean> {
+ assessments.push(
+ ...(await Promise.all(
+ chunk.map((backup) =>
+ assessNamedBackupRestore(backup.name, { currentStorage }),
+ ),
+ )),
</file context>
|
@coderabbitai full review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/codex-manager.ts (1)
4209-4213:⚠️ Potential issue | 🟡 Minoradd manager-level vitest for unreadable backup directory path
lib/codex-manager.ts:4209-4213depends onlistNamedBackups()surfacing real fs failures. please add a manager-path regression intest/codex-manager-cli.test.ts:2365-2799that mockslistNamedBackupsrejecting witheperm/ebusyand asserts the restore flow reports failure (not a misleading empty-backup state).i can draft the vitest case and expected assertions if you want.
As per coding guidelines,lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4209 - 4213, The restore flow currently treats errors from listNamedBackups() as "no backups" — update tests to ensure real FS errors surface: add a manager-level vitest in test/codex-manager-cli.test.ts that mocks listNamedBackups to reject with an EPERM/EBUSY-like error and asserts the CLI/restore command (the code path that calls getNamedBackupsDirectoryPath() and listNamedBackups()) returns a failure exit/throws and prints an error message (not the "No named backups found..." message); reference mocking the listNamedBackups function and exercising the restore invocation so that the implementation for listNamedBackups/restore surfaces filesystem errors instead of treating them as empty result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 2365-2881: Add a deterministic regression test that simulates a
manager-level EPERM from backup discovery by having listNamedBackupsMock reject
once with an error object whose code === "EPERM" (e.g. Object.assign(new
Error(...), { code: "EPERM" }) ), then call
runCodexMultiAuthCli(["auth","login"]) with interactive TTY and
promptLoginModeMock set to enter/exit the restore-backup flow; assert the CLI
handled the EPERM (e.g. promptLoginModeMock was called again / the flow returned
to the login menu) and that a warning or error was logged (spy on console.warn
or console.error), ensuring this suite covers the EPERM path alongside the
existing success/failure cases referenced by listNamedBackupsMock,
promptLoginModeMock, selectMock, and runCodexMultiAuthCli.
In `@test/storage.test.ts`:
- Around line 1209-1309: Add a deterministic regression test that creates a
symlink/junction inside the backups directory that points outside the backups
root and asserts that listNamedBackups() does not surface that symlink entry and
that assessNamedBackupRestore()/restoreNamedBackup() reject attempts to use its
name; specifically, in the new test use the same helpers around
createNamedBackup/assessNamedBackupRestore/restoreNamedBackup/listNamedBackups
to create a real file outside the backups folder, create a symlink (or Windows
junction) to it inside the backups folder, then call listNamedBackups()
expecting the symlink entry to be absent and call
assessNamedBackupRestore(symlinkName) and restoreNamedBackup(symlinkName)
expecting them to throw; ensure the test is deterministic (use vitest
utilities), cleans up the symlink/junction, and does not rely on real secrets or
external network.
- Around line 1249-1300: The tests for deleted/corrupt backups currently only
assert that restoreNamedBackup(...) throws, but do not verify it didn't mutate
active storage; after the rejection add assertions that loadAccounts() (and/or
getActive accounts via the existing storage API such as loadAccounts()) still
returns an empty state to ensure restoreNamedBackup() is atomic and leaves
active storage untouched; update both test cases that call
restoreNamedBackup("deleted-after-assessment") and
restoreNamedBackup("invalid-after-assessment") to await loadAccounts() and
expect the returned accounts/state to remain empty (use the same helpers
clearAccounts() and loadAccounts() used elsewhere in the file).
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4209-4213: The restore flow currently treats errors from
listNamedBackups() as "no backups" — update tests to ensure real FS errors
surface: add a manager-level vitest in test/codex-manager-cli.test.ts that mocks
listNamedBackups to reject with an EPERM/EBUSY-like error and asserts the
CLI/restore command (the code path that calls getNamedBackupsDirectoryPath() and
listNamedBackups()) returns a failure exit/throws and prints an error message
(not the "No named backups found..." message); reference mocking the
listNamedBackups function and exercising the restore invocation so that the
implementation for listNamedBackups/restore surfaces filesystem errors instead
of treating them as empty result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4009a67a-dfe7-4ea8-8a2b-d3537be183d8
📒 Files selected for processing (4)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/storage.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-cli.test.tstest/storage.test.ts
🔇 Additional comments (2)
lib/storage.ts (1)
1581-1650: good hardening on backup listing and transient fs retries
lib/storage.ts:1581-1650now bounds fan-out and retries transienteperm/ebusy/eagainfailures before bubbling errors. this is the right direction for windows filesystem stability and concurrency control.lib/codex-manager.ts (1)
4218-4245: nice fail-open assessment fan-out with bounded concurrency
lib/codex-manager.ts:4218-4245uses chunkedpromise.allsettledwithNAMED_BACKUP_LIST_CONCURRENCY, so one bad backup assessment no longer blocks restore options for healthy backups.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1348">
P3: The traversal test matcher is still too broad (`not found`), so it can pass on generic missing-file failures instead of validating the security guard.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/storage.ts (1)
2682-2692: 🧹 Nitpick | 🔵 Trivialtransaction path guard works but lacks windows path normalization.
the comparison at
lib/storage.ts:2685is a direct string equality check. on windows, equivalent paths can differ by case (e.g.,C:\Users\...vsc:\users\...). the past review at these lines noted this concern.test at
test/storage.test.ts:373-399exercises the different-path scenario but doesn't cover case-variant paths on windows.consider adding path normalization for windows:
+ const normalizePath = (p: string): string => + process.platform === "win32" ? p.toLowerCase() : p; const currentStoragePath = getStoragePath(); const storage = transactionState?.active - ? transactionState.storagePath === currentStoragePath + ? normalizePath(transactionState.storagePath) === normalizePath(currentStoragePath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 2682 - 2692, The direct string equality check comparing transactionState.storagePath to getStoragePath() fails on Windows due to case-insensitive paths; update the guard in the transaction snapshot logic (where transactionSnapshotContext, getStoragePath, transactionState.storagePath, snapshot, and withAccountStorageTransaction are used around exportAccounts) to normalize both paths before comparing: import/require Node's path, resolve/normalize both storage paths and, when process.platform === 'win32', compare their lowercased forms (or use path.win32.normalize) so equivalent Windows paths match; keep the thrown Error message ("exportAccounts called inside an active transaction for a different storage path") intact when the normalized paths differ.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Line 4210: The variable "backups" is declared without a type causing a
noImplicitAnyLet lint error; change its declaration to explicitly type it as the
resolved value of listNamedBackups(): use "let backups: NamedBackupMetadata[] |
undefined;" (or "const backups: NamedBackupMetadata[]" if you can await
immediately) and ensure the code that assigns "backups = await
listNamedBackups(...)" (reference listNamedBackups and the backups variable in
lib/codex-manager.ts) matches the new type so the linter no longer reports an
implicit any.
In `@lib/storage.ts`:
- Around line 1628-1650: The retryTransientFilesystemOperation backoff uses
strict exponential delays without jitter, which can cause thundering-herd under
contention; update retryTransientFilesystemOperation to add randomized jitter to
the sleep before retry (similar to renameFileWithRetry), e.g. compute baseDelay
= 10 * 2 ** attempt and apply a small random factor (like multiply by 0.5–1.5 or
add +/- jitter) when awaiting the sleep in the existing await new Promise(...)
call, preserve the same max attempts and keep isRetryableFilesystemErrorCode and
error handling unchanged.
In `@test/codex-manager-cli.test.ts`:
- Around line 2788-2872: Add a new deterministic vitest case mirroring the
existing "reassesses a backup..." test but make the second call to
assessNamedBackupRestoreMock reject with an error whose code is 'EBUSY' or
'EPERM' (simulate the reassessment rejection path in lib/codex-manager.ts at the
reassess branch), then assert that runCodexMultiAuthCli(["auth","login"])
returns to the menu (promptLoginModeMock resolves back to menu) and that
restoreNamedBackupMock is not called; reference and reuse the same mocks used in
the file (assessNamedBackupRestoreMock, listNamedBackupsMock,
promptLoginModeMock, selectMock, confirmMock, restoreNamedBackupMock) and assert
appropriate expectations: assessNamedBackupRestoreMock called twice with
"named-backup" (second call rejected), promptLoginModeMock was invoked to return
to menu, and restoreNamedBackupMock was not invoked.
In `@test/storage.test.ts`:
- Around line 1345-1355: The test uses an overly broad alternation for
path-traversal errors; update the expectations in the two calls to
assessNamedBackupRestore and restoreNamedBackup so they assert the specific
error text thrown by the implementation (use the exact message or a tighter
regex from lib/storage.ts lines ~1861-1863 or the error/message exported by
lib/named-backup-export.ts) instead of the current wide /invalid|not
allowed|escape|traversal|not found|path separators/i pattern; either import the
exported error/message constant from lib/named-backup-export.ts (or copy the
exact string used in lib/storage.ts) and assert against that value (or a narrow
regex like /contains path separator/i) to make the test precise.
---
Duplicate comments:
In `@lib/storage.ts`:
- Around line 2682-2692: The direct string equality check comparing
transactionState.storagePath to getStoragePath() fails on Windows due to
case-insensitive paths; update the guard in the transaction snapshot logic
(where transactionSnapshotContext, getStoragePath, transactionState.storagePath,
snapshot, and withAccountStorageTransaction are used around exportAccounts) to
normalize both paths before comparing: import/require Node's path,
resolve/normalize both storage paths and, when process.platform === 'win32',
compare their lowercased forms (or use path.win32.normalize) so equivalent
Windows paths match; keep the thrown Error message ("exportAccounts called
inside an active transaction for a different storage path") intact when the
normalized paths differ.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df6cb1b4-fc90-4962-b71a-828f6781fb52
📒 Files selected for processing (4)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (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/codex-manager-cli.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/storage.ts
🪛 Biome (2.4.6)
lib/codex-manager.ts
[error] 4210-4210: This variable implicitly has the any type.
(lint/suspicious/noImplicitAnyLet)
🔇 Additional comments (26)
lib/storage.ts (9)
119-141: interfaces look solid.the nullable fields are typed correctly, and the structure captures all needed restore assessment info without exposing sensitive account data.
1581-1626: listing implementation handles windows edge cases well.symlink filtering at
lib/storage.ts:1588, case-insensitive.jsonmatching atlib/storage.ts:1589, and bounded concurrency via chunks are all correct. the retry wrapper handles transient EBUSY/EPERM.tests at
test/storage.test.ts:1357-1411(symlink),test/storage.test.ts:1249-1287(uppercase .JSON), andtest/storage.test.ts:1645-1696(concurrency) cover these paths.
1669-1723: assessment logic handles edge cases correctly.explicit
nullvsundefineddistinction atlib/storage.ts:1680-1683is important for zero-account recovery flows. test attest/storage.test.ts:1169-1207verifies this behavior.
1725-1730: restore delegation is correct.the function properly delegates to
importAccountswhich handles the storage mutex internally. test attest/storage.test.ts:1289-1315verifies the TOCTOU edge case where backup is deleted after assessment but before restore.
1755-1773: backup candidate loading with retry is correct.the retry wrapper at
lib/storage.ts:1762-1764handles transient windows errors. tests attest/storage.test.ts:1563-1603verify retry behavior for both read and stat operations.
1787-1837: directory-listing-based path resolution is secure.using
readdirand matching entries atlib/storage.ts:1805-1816prevents path traversal attacks. symlink rejection atlib/storage.ts:1817-1821is a good defense. tests attest/storage.test.ts:1345-1355verify traversal rejection andtest/storage.test.ts:1357-1411verifies symlink handling.
1839-1867: path resolution fallback logic is correct.the function correctly distinguishes between "valid manual backup name that doesn't exist" (file-not-found at
lib/storage.ts:1861-1863) vs "invalid name format" (rethrow atlib/storage.ts:1865). this allows manual backups with spaces like "Manual Backup.json" while still validating programmatic names.
2077-2119: metadata construction handles platform differences.the fallback from
birthtimeMstoctimeMsatlib/storage.ts:2104handles platforms where birthtime isn't available. retry wrapper atlib/storage.ts:2090handles transient windows errors.
2256-2261: transaction state correctly captures storagePath at transaction start.both
withAccountStorageTransactionandwithAccountAndFlaggedStorageTransactioncapturestoragePathbefore entering the transaction context, ensuring the path is consistent throughout.Also applies to: 2283-2288
test/storage.test.ts (14)
335-338: cleanup using removeWithRetry is correct.this prevents flaky test failures on windows where temp directories may be briefly locked.
373-399: transaction path guard test is correct.the test properly exercises the error path when
exportAccountsis called inside a transaction for a different storage path. thefinallyblock ensures cleanup even if the test fails.
1112-1139: create and list test is deterministic with proper assertions.
1169-1207: explicit null currentStorage test is important for zero-account recovery.this verifies the distinction between
undefined(load from disk) andnull(treat as empty), which is critical for the recovery flow.
1209-1287: manual backup tests cover important edge cases.space-in-name at
test/storage.test.ts:1213and uppercase.JSONattest/storage.test.ts:1253are critical for windows compatibility where users may create backups manually.
1289-1343: post-assessment failure tests verify atomicity.assertions at
test/storage.test.ts:1314andtest/storage.test.ts:1342verify thatloadAccounts()returns empty after failed restores, ensuring the restore operation is atomic.
1345-1355: path traversal tests cover both unix and windows styles.
1357-1411: symlink filtering test uses mock appropriately.the mock approach tests the code path in
lib/storage.ts:1588andlib/storage.ts:1817. for real symlink/junction behavior on windows, theisSymbolicLink()method should work, but that's a node.js behavior rather than something we can unit test.
1413-1439: unreadable directory tests verify error propagation.these tests ensure that EPERM errors bubble up rather than being swallowed, addressing the past review concern about masking real errors.
1441-1526: transient directory error retry tests are correct.tests at
test/storage.test.ts:1441-1484(EBUSY) andtest/storage.test.ts:1486-1526(EAGAIN) verify the retry logic inretryTransientFilesystemOperation.
1528-1561: TOCTOU test for manual backups is correct.this verifies the edge case where a user deletes a manual backup between assessment and restore.
1563-1643: read and stat retry tests verify candidate loading resilience.these tests cover the retry logic in
loadBackupCandidate(readFile) andbuildNamedBackupMetadata(stat).
1645-1696: concurrency limit test uses exported constant.this addresses the past review concern about hardcoding the magic number 8. the test now references
NAMED_BACKUP_LIST_CONCURRENCYattest/storage.test.ts:1690-1691.
1698-1767: concurrent restore serialization test is critical.this test verifies that the storage mutex properly serializes concurrent restore attempts, preventing the account limit from being exceeded. the assertion at
test/storage.test.ts:1766confirms exactlyMAX_ACCOUNTSafter both attempts.lib/codex-manager.ts (2)
3825-3865: good recovery-flow hoist for zero-account login.
lib/codex-manager.ts:3825-3865now drives the menu fromexistingStorage ?? createEmptyAccountStorage(), so restore actions are reachable even when account storage is empty. this closes the lockout path cleanly.
4235-4256: good non-fail-fast assessment fan-out.
lib/codex-manager.ts:4235-4256usespromise.allsettledwith chunking, so one backup assessment failure no longer aborts the whole restore list. this is the right concurrency behavior for windows/transient fs failures.test/codex-manager-cli.test.ts (1)
2365-2913: strong restore-flow regression coverage here.
test/codex-manager-cli.test.ts:2365-2913now covers confirm decline, restore failure recovery, windowsepermlisting failure, partial assessment failure isolation, chunked concurrency cap, zero-account entry, and pre-confirm reassessment. this is a solid safety net for the new recovery path.
| function isRetryableFilesystemErrorCode( | ||
| code: string | undefined, | ||
| ): code is "EPERM" | "EBUSY" | "EAGAIN" { | ||
| return code === "EPERM" || code === "EBUSY" || code === "EAGAIN"; | ||
| } | ||
|
|
||
| async function retryTransientFilesystemOperation<T>( | ||
| operation: () => Promise<T>, | ||
| ): Promise<T> { | ||
| for (let attempt = 0; attempt < 5; attempt += 1) { | ||
| try { | ||
| return await operation(); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (!isRetryableFilesystemErrorCode(code) || attempt === 4) { | ||
| throw error; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt)); | ||
| } | ||
| } | ||
|
|
||
| throw new Error("Retry loop exhausted unexpectedly"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
retry logic is correct but consider adding jitter for heavily contested paths.
the exponential backoff at lib/storage.ts:1645 is good, but unlike renameFileWithRetry at lib/storage.ts:428-432 which adds jitter, this function doesn't. under heavy contention (e.g., multiple cli instances listing backups), synchronized retries could cause thundering herd.
low priority since listing is read-only, but worth noting for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 1628 - 1650, The
retryTransientFilesystemOperation backoff uses strict exponential delays without
jitter, which can cause thundering-herd under contention; update
retryTransientFilesystemOperation to add randomized jitter to the sleep before
retry (similar to renameFileWithRetry), e.g. compute baseDelay = 10 * 2 **
attempt and apply a small random factor (like multiply by 0.5–1.5 or add +/-
jitter) when awaiting the sleep in the existing await new Promise(...) call,
preserve the same max attempts and keep isRetryableFilesystemErrorCode and error
handling unchanged.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
lib/storage.ts (2)
1847-1861:⚠️ Potential issue | 🟠 Majorpreserve validation errors from restore-path resolution.
at
lib/storage.ts:1847-1861(line [1847]), the catch path can replacebuildNamedBackupPathvalidation errors withimport file not found. this hides the real cause for invalid names and makes diagnosis harder.please add a regression in
test/storage.test.ts:1that passes an invalid backup name and asserts the original validation error is surfaced.proposed fix
async function resolveNamedBackupRestorePath(name: string): Promise<string> { const existingPath = await findExistingNamedBackupPath(name); if (existingPath) { return existingPath; } - const requested = (name ?? "").trim(); - const backupRoot = getNamedBackupRoot(getStoragePath()); - const requestedWithExtension = requested.toLowerCase().endsWith(".json") - ? requested - : `${requested}.json`; - try { - return buildNamedBackupPath(name); - } catch (error) { - const baseName = requestedWithExtension.toLowerCase().endsWith(".json") - ? requestedWithExtension.slice(0, -".json".length) - : requestedWithExtension; - if ( - requested.length > 0 && - basename(requestedWithExtension) === requestedWithExtension && - !requestedWithExtension.includes("..") && - !/^[A-Za-z0-9_-]+$/.test(baseName) - ) { - throw new Error( - `Import file not found: ${resolvePath(join(backupRoot, requestedWithExtension))}`, - ); - } - throw error; - } + return buildNamedBackupPath(name); }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 1847 - 1861, The catch block around buildNamedBackupPath is swallowing validation errors and replacing them with a generic "Import file not found" message; change the handler in the catch to rethrow the original error for validation-related failures (i.e. when buildNamedBackupPath(name) throws a validation error) and only fall back to the existing requestedWithExtension/file-not-found logic when the original error is specifically a missing-file resolution scenario; update the catch to inspect the thrown error and either throw it (preserving the validation message) or proceed with the requestedWithExtension basename/../ checks and the existing resolvePath(join(backupRoot, requestedWithExtension)) error; also add a vitest regression in test/storage.test.ts that calls the restore path resolution with an invalid backup name and asserts the original validation error is surfaced (reference buildNamedBackupPath, requestedWithExtension, requested, and the file-not-found branch).
2681-2689:⚠️ Potential issue | 🟠 Majornormalize transaction/storage path comparison on windows.
at
lib/storage.ts:2681-2689(line [2681]), raw string comparison can fail for same-path case variants on windows and incorrectly throw"different storage path".please add a windows-style case-variant regression in
test/storage.test.ts:1forexportAccountstransaction guard.proposed fix
const transactionState = transactionSnapshotContext.getStore(); - const currentStoragePath = getStoragePath(); + const normalizeStoragePathForComparison = (value: string): string => { + const resolved = resolvePath(value); + return process.platform === "win32" ? resolved.toLowerCase() : resolved; + }; + const currentStoragePath = normalizeStoragePathForComparison(getStoragePath()); const storage = transactionState?.active - ? transactionState.storagePath === currentStoragePath + ? normalizeStoragePathForComparison(transactionState.storagePath) === currentStoragePath ? transactionState.snapshot : (() => { throw new Error( "exportAccounts called inside an active transaction for a different storage path", ); })() : await withAccountStorageTransaction((current) => Promise.resolve(current));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 2681 - 2689, The transaction path comparison in exportAccounts uses raw string equality between transactionState.storagePath and getStoragePath(), which can fail on Windows due to case and separator variants; update the guard in lib/storage.ts to compare normalized, resolved paths (use path.resolve + path.normalize) and, when process.platform === 'win32', compare the lowercased normalized paths to ensure case-insensitive equality for Windows; update references to transactionState.storagePath and getStoragePath() in that block to use the normalized/resolved values and throw only if those normalized values differ; also add a vitest regression in test/storage.test.ts that creates a transaction with a Windows-style case-variant path and asserts exportAccounts does not throw for the same logical path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 3834-3854: The pendingMenuQuotaRefresh background task can race
with foreground quota actions and overwrite fresher entries; modify the code
paths that start runHealthCheck, runForecast, and runFix to first serialize with
pendingMenuQuotaRefresh (e.g., if pendingMenuQuotaRefresh is non-null await it
or acquire a simple mutex before starting any read-modify-write on quotaCache)
so that refreshQuotaCacheForMenu and those functions never run concurrent
writers against quotaCache; update logic that uses pendingMenuQuotaRefresh,
countMenuQuotaRefreshTargets, and refreshQuotaCacheForMenu to ensure writers are
awaited/queued and still clear pendingMenuQuotaRefresh in finally; then add a
regression test in test/codex-manager-cli.test.ts that starts a
pendingMenuQuotaRefresh (mock or stub refreshQuotaCacheForMenu to delay) and
concurrently invokes runHealthCheck/runForecast/runFix, asserting the quotaCache
keys are preserved (no lost/stale keys) after both operations complete.
In `@test/codex-manager-cli.test.ts`:
- Around line 2475-2539: Add an assertion that the login-mode prompt was invoked
twice to verify the decline path returns to the recovery menu: after calling
runCodexMultiAuthCli([...]) add
expect(promptLoginModeMock).toHaveBeenCalledTimes(2) (reference mocks:
promptLoginModeMock, confirmMock, selectMock and the entry function
runCodexMultiAuthCli used in the test case "does not restore a named backup when
confirmation is declined"); this ensures the second mockResolvedValue for
promptLoginModeMock is consumed and the flow loops back rather than exiting
early.
- Around line 2958-2997: The test is brittle because it asserts a
locale-specific formatted date (new Date(0).toLocaleDateString()); update the
assertion in the test that checks selectMock's hint (backupItems?.[0]?.hint) to
avoid exact localized matching — instead assert that the hint contains the
stable prefix "updated " (e.g., expect(...).toContain("updated ")) or otherwise
verify that the updatedAt value is rendered (not dropped) by checking presence
of the word "updated" and a numeric year/epoch token, leaving
runCodexMultiAuthCli, selectMock and the backup assessment structure unchanged.
In `@test/storage.test.ts`:
- Around line 1645-1696: The test currently creates a fixed 12 backups which can
fail to saturate the limiter if NAMED_BACKUP_LIST_CONCURRENCY >= 12; change the
workload to be dynamic by deriving the number of backups from
NAMED_BACKUP_LIST_CONCURRENCY (e.g. const total = NAMED_BACKUP_LIST_CONCURRENCY
+ X) and use that total when creating backups with
saveAccounts/createNamedBackup and when asserting
expect(backups).toHaveLength(...); apply the same change in the other test
referenced (test/codex-manager-cli.test.ts around the create loop at line ~2719)
so both tests always exceed NAMED_BACKUP_LIST_CONCURRENCY and reliably exercise
the concurrency limiter used by listNamedBackups.
- Around line 1413-1526: Add a deterministic test that exercises the transient
"EPERM" retry path by modifying the existing retry tests: in either the "retries
transient backup directory errors while listing backups" or "while restoring
backups" block, change the readdir mock (the vi.spyOn(fs,
"readdir").mockImplementation used with backupRoot and originalReaddir) to throw
a NodeJS.ErrnoException with error.code = "EPERM" exactly once (increment
busyFailures) and then return originalReaddir for subsequent calls, then assert
the operation still succeeds and busyFailures === 1; ensure you reference
listNamedBackups or restoreNamedBackup and restore the spy with
readdirSpy.mockRestore() in finally so the test stays deterministic.
---
Duplicate comments:
In `@lib/storage.ts`:
- Around line 1847-1861: The catch block around buildNamedBackupPath is
swallowing validation errors and replacing them with a generic "Import file not
found" message; change the handler in the catch to rethrow the original error
for validation-related failures (i.e. when buildNamedBackupPath(name) throws a
validation error) and only fall back to the existing
requestedWithExtension/file-not-found logic when the original error is
specifically a missing-file resolution scenario; update the catch to inspect the
thrown error and either throw it (preserving the validation message) or proceed
with the requestedWithExtension basename/../ checks and the existing
resolvePath(join(backupRoot, requestedWithExtension)) error; also add a vitest
regression in test/storage.test.ts that calls the restore path resolution with
an invalid backup name and asserts the original validation error is surfaced
(reference buildNamedBackupPath, requestedWithExtension, requested, and the
file-not-found branch).
- Around line 2681-2689: The transaction path comparison in exportAccounts uses
raw string equality between transactionState.storagePath and getStoragePath(),
which can fail on Windows due to case and separator variants; update the guard
in lib/storage.ts to compare normalized, resolved paths (use path.resolve +
path.normalize) and, when process.platform === 'win32', compare the lowercased
normalized paths to ensure case-insensitive equality for Windows; update
references to transactionState.storagePath and getStoragePath() in that block to
use the normalized/resolved values and throw only if those normalized values
differ; also add a vitest regression in test/storage.test.ts that creates a
transaction with a Windows-style case-variant path and asserts exportAccounts
does not throw for the same logical path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72c679b6-32f2-4246-b6ef-7822d19589ee
📒 Files selected for processing (4)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/storage.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (2)
lib/codex-manager.ts (1)
4235-4256: good fail-soft assessment fan-out.
lib/codex-manager.ts:4235-4256(line [4235]) usespromise.allsettledwith per-item warnings, so one bad backup does not block other restore candidates. this is the right resilience behavior for recovery UX.lib/storage.ts (1)
1581-1626: good windows retry + error surfacing in named-backup listing.
lib/storage.ts:1581-1626andlib/storage.ts:1634-1650(line [1581], line [1634]) correctly retryeperm/ebusy/eagain, return[]only forenoent, and rethrow other failures. this keeps transient windows behavior robust without masking real directory faults.Also applies to: 1634-1650
| if (shouldAutoFetchLimits && !pendingMenuQuotaRefresh) { | ||
| const staleCount = countMenuQuotaRefreshTargets(currentStorage, quotaCache, quotaTtlMs); | ||
| if (staleCount > 0) { | ||
| if (showFetchStatus) { | ||
| menuQuotaRefreshStatus = `${UI_COPY.mainMenu.loadingLimits} [0/${staleCount}]`; | ||
| } | ||
| pendingMenuQuotaRefresh = refreshQuotaCacheForMenu( | ||
| currentStorage, | ||
| quotaCache, | ||
| quotaTtlMs, | ||
| (current, total) => { | ||
| if (!showFetchStatus) return; | ||
| menuQuotaRefreshStatus = `${UI_COPY.mainMenu.loadingLimits} [${current}/${total}]`; | ||
| }, | ||
| ) | ||
| .then(() => undefined) | ||
| .catch(() => undefined) | ||
| .finally(() => { | ||
| menuQuotaRefreshStatus = undefined; | ||
| pendingMenuQuotaRefresh = null; | ||
| }); |
There was a problem hiding this comment.
serialize quota-cache writers before launching quota-touching actions.
at lib/codex-manager.ts:3834-3854 and lib/codex-manager.ts:3871-3893 (line [3834], line [3871]), pendingMenuQuotaRefresh can still be writing while runHealthCheck / runForecast / runFix starts another read-modify-write cycle on quota cache. this can clobber fresher entries with stale state.
add a guard and a regression in test/codex-manager-cli.test.ts:1 that simulates a pending menu refresh plus a foreground quota action, then asserts no key loss.
proposed fix
@@
if (menuResult.mode === "cancel") {
console.log("Cancelled.");
return 0;
}
+ const modeTouchesQuotaCache =
+ menuResult.mode === "check" ||
+ menuResult.mode === "deep-check" ||
+ menuResult.mode === "forecast" ||
+ menuResult.mode === "fix";
+ if (modeTouchesQuotaCache && pendingMenuQuotaRefresh) {
+ await pendingMenuQuotaRefresh;
+ }
if (menuResult.mode === "check") {
await runActionPanel("Quick Check", "Checking local session + live status", async () => {
await runHealthCheck({ forceRefresh: false, liveProbe: true });
}, displaySettings);
continue;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: 3871-3893
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 3834 - 3854, The pendingMenuQuotaRefresh
background task can race with foreground quota actions and overwrite fresher
entries; modify the code paths that start runHealthCheck, runForecast, and
runFix to first serialize with pendingMenuQuotaRefresh (e.g., if
pendingMenuQuotaRefresh is non-null await it or acquire a simple mutex before
starting any read-modify-write on quotaCache) so that refreshQuotaCacheForMenu
and those functions never run concurrent writers against quotaCache; update
logic that uses pendingMenuQuotaRefresh, countMenuQuotaRefreshTargets, and
refreshQuotaCacheForMenu to ensure writers are awaited/queued and still clear
pendingMenuQuotaRefresh in finally; then add a regression test in
test/codex-manager-cli.test.ts that starts a pendingMenuQuotaRefresh (mock or
stub refreshQuotaCacheForMenu to delay) and concurrently invokes
runHealthCheck/runForecast/runFix, asserting the quotaCache keys are preserved
(no lost/stale keys) after both operations complete.
| it("does not restore a named backup when confirmation is declined", async () => { | ||
| setInteractiveTTY(true); | ||
| const now = Date.now(); | ||
| loadAccountsMock.mockResolvedValue({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| accounts: [ | ||
| { | ||
| email: "settings@example.com", | ||
| accountId: "acc_settings", | ||
| refreshToken: "refresh-settings", | ||
| accessToken: "access-settings", | ||
| expiresAt: now + 3_600_000, | ||
| addedAt: now - 1_000, | ||
| lastUsed: now - 1_000, | ||
| enabled: true, | ||
| }, | ||
| ], | ||
| }); | ||
| const assessment = { | ||
| backup: { | ||
| name: "named-backup", | ||
| path: "/mock/backups/named-backup.json", | ||
| createdAt: null, | ||
| updatedAt: now, | ||
| sizeBytes: 128, | ||
| version: 3, | ||
| accountCount: 1, | ||
| schemaErrors: [], | ||
| valid: true, | ||
| loadError: undefined, | ||
| }, | ||
| currentAccountCount: 1, | ||
| mergedAccountCount: 2, | ||
| imported: 1, | ||
| skipped: 0, | ||
| wouldExceedLimit: false, | ||
| eligibleForRestore: true, | ||
| error: undefined, | ||
| }; | ||
| listNamedBackupsMock.mockResolvedValue([assessment.backup]); | ||
| assessNamedBackupRestoreMock.mockResolvedValue(assessment); | ||
| confirmMock.mockResolvedValueOnce(false); | ||
| promptLoginModeMock | ||
| .mockResolvedValueOnce({ mode: "restore-backup" }) | ||
| .mockResolvedValueOnce({ mode: "cancel" }); | ||
| selectMock.mockResolvedValueOnce({ type: "restore", assessment }); | ||
|
|
||
| const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); | ||
| const exitCode = await runCodexMultiAuthCli(["auth", "login"]); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| expect(listNamedBackupsMock).toHaveBeenCalledTimes(1); | ||
| expect(assessNamedBackupRestoreMock).toHaveBeenCalledWith( | ||
| "named-backup", | ||
| expect.objectContaining({ | ||
| currentStorage: expect.objectContaining({ | ||
| accounts: expect.any(Array), | ||
| }), | ||
| }), | ||
| ); | ||
| expect(confirmMock).toHaveBeenCalledOnce(); | ||
| expect(restoreNamedBackupMock).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
assert the decline path loops back to login.
test/codex-manager-cli.test.ts:2475 currently only proves restoreNamedBackup() is not called. if the confirm=false branch starts exiting auth login instead of returning to the recovery menu, this still passes without consuming the second promptLoginModeMock response. add expect(promptLoginModeMock).toHaveBeenCalledTimes(2).
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` around lines 2475 - 2539, Add an assertion
that the login-mode prompt was invoked twice to verify the decline path returns
to the recovery menu: after calling runCodexMultiAuthCli([...]) add
expect(promptLoginModeMock).toHaveBeenCalledTimes(2) (reference mocks:
promptLoginModeMock, confirmMock, selectMock and the entry function
runCodexMultiAuthCli used in the test case "does not restore a named backup when
confirmation is declined"); this ensures the second mockResolvedValue for
promptLoginModeMock is consumed and the flow loops back rather than exiting
early.
| it("shows epoch backup timestamps in restore hints", async () => { | ||
| setInteractiveTTY(true); | ||
| loadAccountsMock.mockResolvedValue(null); | ||
| const assessment = { | ||
| backup: { | ||
| name: "epoch-backup", | ||
| path: "/mock/backups/epoch-backup.json", | ||
| createdAt: null, | ||
| updatedAt: 0, | ||
| sizeBytes: 128, | ||
| version: 3, | ||
| accountCount: 1, | ||
| schemaErrors: [], | ||
| valid: true, | ||
| loadError: undefined, | ||
| }, | ||
| currentAccountCount: 0, | ||
| mergedAccountCount: 1, | ||
| imported: 1, | ||
| skipped: 0, | ||
| wouldExceedLimit: false, | ||
| eligibleForRestore: true, | ||
| error: undefined, | ||
| }; | ||
| listNamedBackupsMock.mockResolvedValue([assessment.backup]); | ||
| assessNamedBackupRestoreMock.mockResolvedValue(assessment); | ||
| promptLoginModeMock | ||
| .mockResolvedValueOnce({ mode: "restore-backup" }) | ||
| .mockResolvedValueOnce({ mode: "cancel" }); | ||
| selectMock.mockResolvedValueOnce({ type: "back" }); | ||
|
|
||
| const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); | ||
| const exitCode = await runCodexMultiAuthCli(["auth", "login"]); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| const backupItems = selectMock.mock.calls[0]?.[0]; | ||
| expect(backupItems?.[0]?.hint).toContain( | ||
| `updated ${new Date(0).toLocaleDateString()}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
avoid matching an exact localized date string here.
test/codex-manager-cli.test.ts:2994 hardcodes the result of toLocaleDateString(), which couples the regression to runner locale/time zone and to the current hint formatting in lib/codex-manager.ts:4258. the behavior that matters is that updatedAt: 0 is rendered instead of dropped. assert the "updated " prefix, or compare against the same shared formatter, instead of an exact locale-specific string.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` around lines 2958 - 2997, The test is brittle
because it asserts a locale-specific formatted date (new
Date(0).toLocaleDateString()); update the assertion in the test that checks
selectMock's hint (backupItems?.[0]?.hint) to avoid exact localized matching —
instead assert that the hint contains the stable prefix "updated " (e.g.,
expect(...).toContain("updated ")) or otherwise verify that the updatedAt value
is rendered (not dropped) by checking presence of the word "updated" and a
numeric year/epoch token, leaving runCodexMultiAuthCli, selectMock and the
backup assessment structure unchanged.
| it("rethrows unreadable backup directory errors while listing backups", async () => { | ||
| const readdirSpy = vi.spyOn(fs, "readdir"); | ||
| const error = new Error("backup directory locked") as NodeJS.ErrnoException; | ||
| error.code = "EPERM"; | ||
| readdirSpy.mockRejectedValue(error); | ||
|
|
||
| try { | ||
| await expect(listNamedBackups()).rejects.toMatchObject({ code: "EPERM" }); | ||
| } finally { | ||
| readdirSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it("rethrows unreadable backup directory errors while restoring backups", async () => { | ||
| const readdirSpy = vi.spyOn(fs, "readdir"); | ||
| const error = new Error("backup directory locked") as NodeJS.ErrnoException; | ||
| error.code = "EPERM"; | ||
| readdirSpy.mockRejectedValue(error); | ||
|
|
||
| try { | ||
| await expect(restoreNamedBackup("Manual Backup")).rejects.toMatchObject({ | ||
| code: "EPERM", | ||
| }); | ||
| } finally { | ||
| readdirSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it("retries transient backup directory errors while listing backups", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "retry-list-dir", | ||
| refreshToken: "ref-retry-list-dir", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
| await createNamedBackup("retry-list-dir"); | ||
| const backupRoot = join(dirname(testStoragePath), "backups"); | ||
| const originalReaddir = fs.readdir.bind(fs); | ||
| let busyFailures = 0; | ||
| const readdirSpy = vi | ||
| .spyOn(fs, "readdir") | ||
| .mockImplementation(async (...args) => { | ||
| const [path] = args; | ||
| if (String(path) === backupRoot && busyFailures === 0) { | ||
| busyFailures += 1; | ||
| const error = new Error( | ||
| "backup directory busy", | ||
| ) as NodeJS.ErrnoException; | ||
| error.code = "EBUSY"; | ||
| throw error; | ||
| } | ||
| return originalReaddir(...(args as Parameters<typeof fs.readdir>)); | ||
| }); | ||
|
|
||
| try { | ||
| const backups = await listNamedBackups(); | ||
| expect(backups).toEqual( | ||
| expect.arrayContaining([ | ||
| expect.objectContaining({ name: "retry-list-dir", valid: true }), | ||
| ]), | ||
| ); | ||
| expect(busyFailures).toBe(1); | ||
| } finally { | ||
| readdirSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it("retries transient backup directory errors while restoring backups", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "retry-restore-dir", | ||
| refreshToken: "ref-retry-restore-dir", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
| await createNamedBackup("retry-restore-dir"); | ||
| await clearAccounts(); | ||
| const backupRoot = join(dirname(testStoragePath), "backups"); | ||
| const originalReaddir = fs.readdir.bind(fs); | ||
| let busyFailures = 0; | ||
| const readdirSpy = vi | ||
| .spyOn(fs, "readdir") | ||
| .mockImplementation(async (...args) => { | ||
| const [path] = args; | ||
| if (String(path) === backupRoot && busyFailures === 0) { | ||
| busyFailures += 1; | ||
| const error = new Error( | ||
| "backup directory busy", | ||
| ) as NodeJS.ErrnoException; | ||
| error.code = "EAGAIN"; | ||
| throw error; | ||
| } | ||
| return originalReaddir(...(args as Parameters<typeof fs.readdir>)); | ||
| }); | ||
|
|
||
| try { | ||
| const result = await restoreNamedBackup("retry-restore-dir"); | ||
| expect(result.total).toBe(1); | ||
| expect(busyFailures).toBe(1); | ||
| } finally { | ||
| readdirSpy.mockRestore(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
cover the transient eperm retry path too.
test/storage.test.ts:1413 only locks the permanent-eperm failure case, while test/storage.test.ts:1441 and test/storage.test.ts:1486 only lock successful retries for ebusy and eagain. if the named-backup retry path stops treating eperm as transient, windows backup discovery/restore can regress and this suite still stays green. add one deterministic success-after-eperm case on the list or restore path.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 1413 - 1526, Add a deterministic test that
exercises the transient "EPERM" retry path by modifying the existing retry
tests: in either the "retries transient backup directory errors while listing
backups" or "while restoring backups" block, change the readdir mock (the
vi.spyOn(fs, "readdir").mockImplementation used with backupRoot and
originalReaddir) to throw a NodeJS.ErrnoException with error.code = "EPERM"
exactly once (increment busyFailures) and then return originalReaddir for
subsequent calls, then assert the operation still succeeds and busyFailures ===
1; ensure you reference listNamedBackups or restoreNamedBackup and restore the
spy with readdirSpy.mockRestore() in finally so the test stays deterministic.
| it("limits concurrent backup reads while listing backups", async () => { | ||
| const backupPaths: string[] = []; | ||
| for (let index = 0; index < 12; index += 1) { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: `concurrency-${index}`, | ||
| refreshToken: `ref-concurrency-${index}`, | ||
| addedAt: index + 1, | ||
| lastUsed: index + 1, | ||
| }, | ||
| ], | ||
| }); | ||
| const backup = await createNamedBackup(`concurrency-${index}`); | ||
| backupPaths.push(backup.path); | ||
| } | ||
|
|
||
| const originalReadFile = fs.readFile.bind(fs); | ||
| const delayedPaths = new Set(backupPaths); | ||
| let activeReads = 0; | ||
| let peakReads = 0; | ||
| const readFileSpy = vi | ||
| .spyOn(fs, "readFile") | ||
| .mockImplementation(async (...args) => { | ||
| const [path] = args; | ||
| if (delayedPaths.has(String(path))) { | ||
| activeReads += 1; | ||
| peakReads = Math.max(peakReads, activeReads); | ||
| try { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| return await originalReadFile( | ||
| ...(args as Parameters<typeof fs.readFile>), | ||
| ); | ||
| } finally { | ||
| activeReads -= 1; | ||
| } | ||
| } | ||
| return originalReadFile(...(args as Parameters<typeof fs.readFile>)); | ||
| }); | ||
|
|
||
| try { | ||
| const backups = await listNamedBackups(); | ||
| expect(backups).toHaveLength(12); | ||
| expect(peakReads).toBeLessThanOrEqual( | ||
| NAMED_BACKUP_LIST_CONCURRENCY, | ||
| ); | ||
| } finally { | ||
| readFileSpy.mockRestore(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
make the workload scale with the exported limit.
test/storage.test.ts:1647 always creates 12 backups. if NAMED_BACKUP_LIST_CONCURRENCY moves to 12 or higher, line 1690 still passes even with no throttling because the test never saturates the queue. derive the backup count from NAMED_BACKUP_LIST_CONCURRENCY + n here, and in test/codex-manager-cli.test.ts:2719, so the concurrency regressions keep exercising the limiter.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 1645 - 1696, The test currently creates a
fixed 12 backups which can fail to saturate the limiter if
NAMED_BACKUP_LIST_CONCURRENCY >= 12; change the workload to be dynamic by
deriving the number of backups from NAMED_BACKUP_LIST_CONCURRENCY (e.g. const
total = NAMED_BACKUP_LIST_CONCURRENCY + X) and use that total when creating
backups with saveAccounts/createNamedBackup and when asserting
expect(backups).toHaveLength(...); apply the same change in the other test
referenced (test/codex-manager-cli.test.ts around the create loop at line ~2719)
so both tests always exceed NAMED_BACKUP_LIST_CONCURRENCY and reliably exercise
the concurrency limiter used by listNamedBackups.
1 participant
Summary
Validation
npm test -- test/codex-manager-cli.test.ts test/storage.test.ts test/cli.test.tsnpm run lintNotes
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 wires a named backup restore manager into the
codex auth loginrecovery flow, enabling users to list, assess, and merge backup files from thebackups/directory — including when no accounts are currently saved. the restore path is properly serialised through the existing storage mutex, errors are contained inside the menu loop, and a pre-confirm re-assessment catches concurrent account changes between the list display and the import.lib/storage.ts— addslistNamedBackups,assessNamedBackupRestore,restoreNamedBackup,createNamedBackup,getNamedBackupsDirectoryPath, and aretryTransientFilesystemOperationhelper; bounded concurrency viaNAMED_BACKUP_LIST_CONCURRENCY; case-insensitive.JSON/.jsonextension matching;storagePathadded to transaction context to detect cross-pathexportAccountsmisuselib/codex-manager.ts— addsrunBackupRestoreManagerwith chunked assessments, stale-backup re-check before confirmation, and full error containment; flattens therunAuthLogindouble-loop so the menu is accessible with zero accountslib/cli.ts/lib/ui/auth-menu.ts/lib/ui/copy.ts— addsrestore-backupmode with fallback aliases (u,backup,restore) and a Recovery section in the UItest/storage.test.ts— 15 new integration tests covering the full restore surface including TOCTOU, path traversal rejection, symlink blocking, transient retry, concurrency cap, and concurrent-restore serialisation;afterEachupgraded toremoveWithRetryfor windows CItest/codex-manager-cli.test.ts— 8 new unit tests covering zero-account entry, confirmation decline, restore failure containment, partial assessment skip, concurrency limit, re-assessment before confirm, and epoch timestamp hinttwo issues flagged:
EPERMis included in the retryable error set unconditionally — on linux/macOS it is a permanent permission denial, not a transient lock, so retrying wastes ~150 ms before propagating the real error (should be win32-only); and a symlink-detection error infindExistingNamedBackupPathleaks through the sharedcatchblock, logging a misleading "Failed to read named backup directory" message instead of the actual symlink rejection.Confidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant runAuthLogin participant runBackupRestoreManager participant listNamedBackups participant assessNamedBackupRestore participant select participant confirm participant restoreNamedBackup participant importAccounts User->>runAuthLogin: codex auth login runAuthLogin->>runAuthLogin: promptLoginMode → restore-backup runAuthLogin->>runBackupRestoreManager: call (displaySettings) runBackupRestoreManager->>listNamedBackups: list backup dir (retry EBUSY/EAGAIN/EPERM) listNamedBackups-->>runBackupRestoreManager: NamedBackupMetadata[] loop chunks of NAMED_BACKUP_LIST_CONCURRENCY runBackupRestoreManager->>assessNamedBackupRestore: assess(name, {currentStorage}) assessNamedBackupRestore-->>runBackupRestoreManager: BackupRestoreAssessment end runBackupRestoreManager->>select: show backup menu User->>select: pick backup select-->>runBackupRestoreManager: {type: "restore", assessment} Note over runBackupRestoreManager: re-assess with fresh loadAccounts() to detect concurrent writes runBackupRestoreManager->>assessNamedBackupRestore: re-assess(name, {currentStorage: fresh}) assessNamedBackupRestore-->>runBackupRestoreManager: latestAssessment runBackupRestoreManager->>confirm: "Merge N accounts into M current (K after dedupe)?" User->>confirm: yes confirm-->>runBackupRestoreManager: true runBackupRestoreManager->>restoreNamedBackup: restore(name) restoreNamedBackup->>importAccounts: importAccounts(backupPath) Note over importAccounts: runs inside withAccountStorageTransaction (mutex-serialised) importAccounts-->>restoreNamedBackup: {imported, skipped, total} restoreNamedBackup-->>runBackupRestoreManager: result runBackupRestoreManager-->>runAuthLogin: return (errors caught, stay in menu) runAuthLogin->>runAuthLogin: continue loop → promptLoginModeComments Outside Diff (1)
lib/storage.ts, line 1628-1632 (link)EPERMretry is platform-unsafeEPERMis included in the retryable set unconditionally. on windows, antivirus/file-indexing tools can transiently deny access to a file — retryingEPERMthere is correct. on linux and macOS,EPERMfromreaddir,readFile, orstatmeans the process genuinely lacks the needed permission; that condition does not resolve on retry, so the function wastes up to 150 ms (10 + 20 + 40 + 80 ms) before propagating what was a permanent error from the start.Prompt To Fix All With AI
Last reviewed commit: ee7a402
Context used: