feat(auth): add backup restore manager#76
feat(auth): add backup restore manager#76ndycode wants to merge 24 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 | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
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.
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
|
@coderabbitai full review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedFull review triggered. |
|
@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 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4348">
P2: Calling `importAccounts` directly bypasses the final restore eligibility check, so a backup that becomes duplicate-only after confirmation can still be persisted as a no-op write.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
lib/storage.ts
Outdated
| )), | ||
| ); | ||
| } | ||
| return backups.sort((left, right) => (right.updatedAt ?? 0) - (left.updatedAt ?? 0)); |
There was a problem hiding this comment.
The backup sort will break if updatedAt is NaN. The nullish coalescing operator (??) only handles null and undefined, not NaN. When updatedAt is NaN, the expression (NaN ?? 0) returns NaN, and NaN - NaN also returns NaN, causing unpredictable sort behavior. The test at line 3115 confirms NaN timestamps are expected. Fix:
return backups.sort((left, right) => {
const leftTime = Number.isFinite(left.updatedAt) ? left.updatedAt : 0;
const rightTime = Number.isFinite(right.updatedAt) ? right.updatedAt : 0;
return rightTime - leftTime;
});| return backups.sort((left, right) => (right.updatedAt ?? 0) - (left.updatedAt ?? 0)); | |
| return backups.sort((left, right) => { | |
| const leftTime = Number.isFinite(left.updatedAt) ? left.updatedAt : 0; | |
| const rightTime = Number.isFinite(right.updatedAt) ? right.updatedAt : 0; | |
| return rightTime - leftTime; | |
| }); |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
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="lib/storage.ts">
<violation number="1" location="lib/storage.ts:2825">
P1: The new `imported === 0` early return can skip valid account updates. Duplicate-only imports may still replace existing entries with newer metadata, but this branch returns before `persist(newStorage)`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@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 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/storage.ts">
<violation number="1" location="lib/storage.ts:1728">
P2: Restore assessment incorrectly blocks metadata-only backup restores by treating `imported === 0` as a hard no-op.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const skipped = wouldExceedLimit | ||
| ? null | ||
| : Math.max(0, candidate.normalized.accounts.length - (imported ?? 0)); | ||
| const nothingToImport = !wouldExceedLimit && imported === 0; |
There was a problem hiding this comment.
P2: Restore assessment incorrectly blocks metadata-only backup restores by treating imported === 0 as a hard no-op.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/storage.ts, line 1728:
<comment>Restore assessment incorrectly blocks metadata-only backup restores by treating `imported === 0` as a hard no-op.</comment>
<file context>
@@ -1551,6 +1582,179 @@ export async function getRestoreAssessment(): Promise<RestoreAssessment> {
+ const skipped = wouldExceedLimit
+ ? null
+ : Math.max(0, candidate.normalized.accounts.length - (imported ?? 0));
+ const nothingToImport = !wouldExceedLimit && imported === 0;
+
+ return {
</file context>
| const nothingToImport = !wouldExceedLimit && imported === 0; | |
| const changed = !haveEquivalentAccountRows(mergedAccounts, currentAccounts); | |
| const nothingToImport = !wouldExceedLimit && !changed; |
1 participant
Summary
codex auth login, including the zero-account recovery path and fallback prompt aliasesbackups/participate in restore flowsValidation
npm test -- test/storage.test.ts test/codex-manager-cli.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
codex auth login— covering the zero-account recovery path, concurrent assessment fan-out withPromise.allSettled, a fresh re-assessment before confirmation to keep the merge summary current, and arestore-backupfallback alias in the non-TTY prompt. path traversal is hardened viaassertNamedBackupRestorePathusingpath.relative, symlinks are rejected, and windows filesystem contention is handled with aretryTransientFilesystemOperationhelper (EBUSY/ENOTEMPTY on all platforms, EPERM/EAGAIN on win32).key concerns:
importAccountsnever enforcesACCOUNT_LIMITS.MAX_ACCOUNTSinside the storage transaction — the limit check inassessNamedBackupRestoreis advisory. two concurrent interactive sessions on windows can both pass the assessment independently, serialise at the mutex, and the second import silently pushes the account count above the cap. adding the check beforepersist()(and a matching vitest case) would close this TOCTOU window.haveEquivalentAccountRowsusesJSON.stringifyfor order-sensitive comparison — safe today sincededuplicateAccountsis order-preserving, but deserves an inline comment to protect future refactors.storagePathfield added totransactionSnapshotContextand the guard inexportAccountscorrectly prevent stale-snapshot leakage when the active path changes mid-transaction — good hardening.removeWithRetryapplied to the outer export/importafterEachcloses the windows CI flake path noted in prior review rounds.Confidence Score: 3/5
importAccountsshould be fixed before the restore path ships to avoid silent cap breaches on Windows under concurrent use.importAccountsdoes not enforceACCOUNT_LIMITS.MAX_ACCOUNTSinside the storage transaction, so the assessment-level limit gate can be raced past by concurrent sessions — a real concern given the PR's own focus on windows concurrency safety.lib/storage.ts—importAccountstransaction block needs adeduplicatedAccounts.length > ACCOUNT_LIMITS.MAX_ACCOUNTSguard beforepersist().test/storage.test.ts— needs a companion test for the concurrent-exceed scenario.Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User (codex auth login) participant M as runAuthLogin loop participant BRM as runBackupRestoreManager participant S as storage.ts participant FS as Filesystem (backups/) M->>BRM: mode === "restore-backup" BRM->>S: listNamedBackups() S->>FS: readdir(backupRoot) [retryTransientFsOp] FS-->>S: [entries] S-->>BRM: NamedBackupMetadata[] BRM->>S: assessNamedBackupRestore(name, {currentStorage})\n[chunked, NAMED_BACKUP_ASSESS_CONCURRENCY=8] S->>FS: readdir + stat + loadBackupCandidate [retryTransientFsOp] S->>S: assertNamedBackupRestorePath (traversal check) S->>S: deduplicateAccounts → check wouldExceedLimit S-->>BRM: BackupRestoreAssessment[] BRM->>U: select(assessments menu) U-->>BRM: { type: "restore", assessment } BRM->>S: assessNamedBackupRestore(name, {currentStorage: fresh}) [re-assess] S-->>BRM: latestAssessment (eligibleForRestore check) BRM->>U: confirm(restoreBackupConfirm(...)) U-->>BRM: true BRM->>S: importAccounts(latestAssessment.backup.path) S->>S: withAccountStorageTransaction (storageMutex acquired) S->>FS: read backup file [retryTransientFsOp] S->>S: deduplicateAccounts → haveEquivalentAccountRows alt changed S->>FS: persist newStorage (WAL write) S-->>BRM: ImportAccountsResult { changed: true } else no-op S-->>BRM: ImportAccountsResult { changed: false } end BRM-->>M: return (errors caught, continue loop)Comments Outside Diff (1)
lib/storage.ts, line 2842-2865 (link)TOCTOU race:
importAccountsdoesn't enforceACCOUNT_LIMITS.MAX_ACCOUNTSinside the transactionassessNamedBackupRestorecheckswouldExceedLimitand setseligibleForRestore: falsewhen the merge would push over the cap.runBackupRestoreManagergates on that flag before callingimportAccounts. But the actual import executes insidewithAccountStorageTransaction, reads the current accounts at that point, and writes the merged result without ever consultingACCOUNT_LIMITS.MAX_ACCOUNTS.on windows, two concurrent
codex auth loginsessions can both pass the assessment check independently (both see the current count as below the cap), both receive user confirmation, and then both enterwithAccountStorageTransactionin sequence. the second import operates on the already-merged state from the first and can silently push the stored account count aboveMAX_ACCOUNTSwith no error returned to the caller.the fix is to check the merged count inside the transaction before calling
persist:this matches the error message already used in
assessNamedBackupRestore, and all callers ofimportAccountsalready wrap in try/catch.Prompt To Fix All With AI
Last reviewed commit: da40e70
Context used: