Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 Walkthroughwalkthroughadds a preview-restore workflow for named backups, new assessment and preview types, formatting helpers, and refactors restore flows to use assessment-driven previews and gated restores. changes span manager UI, storage assessment/loading, and tests. changes
sequence diagramsequenceDiagram
actor user as User
participant cli as CLI/Menu
participant mgr as codex-manager
participant assess as assess (storage)
participant store as storage
user->>cli: browse backups
cli->>mgr: showBackupBrowserDetails(entry)
mgr->>store: read backup metadata / candidate
store-->>mgr: candidate metadata
mgr-->>cli: action: preview-restore
cli->>user: prompt preview
user->>mgr: runBackupRestorePreview(entry)
mgr->>assess: assessNamedBackupRestoreCandidate(name)
assess->>store: load rawAccounts, read current storage
store-->>assess: conflicts, active-account preview
assess-->>mgr: BackupRestoreAssessment
mgr->>mgr: buildRestoreAssessmentLines(assessment)
mgr-->>cli: display preview + assessment
cli->>user: confirm restore?
user->>mgr: confirm
mgr->>store: restoreNamedBackup(name, { assessment })
store->>store: importNormalizedAccounts with assessment
store-->>mgr: { imported, skipped, total }
mgr->>mgr: formatNamedBackupRestoreResult(...)
mgr-->>cli: display result
estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes review notesmissing regression tests: tests do not cover a stale-assessment case where assessment computed during preview becomes invalid before restore. add a test that mutates current storage between windows edge case: concurrency risk: current flow re-assesses in 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a “restore preview” flow for named backup restores, including refreshed assessment at confirmation time and support for replace-only restores, with expanded assessment/preview metadata surfaced through the CLI and validated via new storage/CLI tests.
Changes:
- Extends backup restore assessment with account/conflict/active-account preview fields and uses them to gate restore eligibility.
- Adds an interactive CLI “Preview Restore” action that re-assesses just-in-time and confirms with a detailed summary.
- Updates/adds tests covering replace-only restores, limit race reassessment, and new preview messaging.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/storage.ts | Adds richer restore assessment/preview fields, recalculates merge/replace stats, and adds eligibility checks to restoreNamedBackup. |
| lib/codex-manager.ts | Implements the restore preview UI, assessment summary formatting, and confirmation messaging updates. |
| test/storage.test.ts | Adds coverage for preview fields, replace-only restore conflict details, and reassessment when limits change. |
| test/storage-recovery-paths.test.ts | Adjusts recovery-path test timing and loosens an assertion around latestValidPath. |
| test/codex-manager-cli.test.ts | Adds/updates CLI tests to validate preview flow, refreshed assessment confirmation, and failure handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/codex-manager.ts
Outdated
| } | ||
| | { type: "back" }; | ||
|
|
||
| type BackupDetailAction = "back" | "preview-restore" | "restore"; |
| const action = await select<BackupDetailAction>( | ||
| [ | ||
| { label: "Restore This Backup", value: "restore", color: "green" }, | ||
| { label: "Back", value: "back" }, | ||
| { label: "Preview Restore", value: "preview-restore", color: "green" }, | ||
| ], |
lib/codex-manager.ts
Outdated
| const parts: string[] = []; | ||
| if (replacements > 0) { | ||
| parts.push( | ||
| `${replacements} replace current${replacements === 1 ? "" : " accounts"}`, |
| [ | ||
| { label: "Restore This Backup", value: "restore", color: "green" }, | ||
| { label: "Back", value: "back" }, | ||
| { label: "Preview Restore", value: "preview-restore", color: "green" }, | ||
| ], | ||
| { | ||
| message: "Backup Browser", | ||
| message: "Backup Actions", | ||
| subtitle: entry.label, | ||
| help: "Enter Select | Q Back", | ||
| help: "Enter Preview | Q Back", |
lib/storage.ts
Outdated
| name: string, | ||
| ): Promise<{ imported: number; total: number; skipped: number }> { | ||
| const backupPath = await resolveNamedBackupRestorePath(name); | ||
| const assessment = await assessNamedBackupRestore(name); | ||
| if (!assessment.eligibleForRestore || assessment.wouldExceedLimit) { | ||
| throw new Error(assessment.error ?? "Backup is not eligible for restore"); | ||
| } | ||
| return importAccounts(backupPath); |
| const backupPath = await resolveNamedBackupRestorePath(name); | ||
| const assessment = await assessNamedBackupRestore(name); | ||
| if (!assessment.eligibleForRestore || assessment.wouldExceedLimit) { | ||
| throw new Error(assessment.error ?? "Backup is not eligible for restore"); |
test/storage-recovery-paths.test.ts
Outdated
| ); | ||
|
|
||
| await fs.writeFile(`${storagePath}.cache`, "noise", "utf-8"); | ||
| await new Promise((resolve) => setTimeout(resolve, 20)); |
test/storage-recovery-paths.test.ts
Outdated
| const cacheEntries = accountSnapshots.filter((snapshot) => snapshot.path.endsWith(".cache")); | ||
| expect(cacheEntries).toHaveLength(0); | ||
| expect(metadata.accounts.latestValidPath).toBe(`${storagePath}.manual-meta-checkpoint`); | ||
| expect(metadata.accounts.latestValidPath).not.toBe(`${storagePath}.cache`); |
| if (assessment.eligibleForRestore) { | ||
| actionable.push(assessment); | ||
| } |
There was a problem hiding this comment.
getActionableNamedBackupRestores filter broadened — verify startup-restore callers handle imported === 0
the filter changed from:
assessment.eligibleForRestore &&
!assessment.wouldExceedLimit &&
assessment.imported !== null &&
assessment.imported > 0to just assessment.eligibleForRestore, which now includes replace-only restores (imported === 0, replacedExistingCount > 0).
the browser and preview paths were updated, but the startup auto-restore flow (which surfaces prompts when the user has no accounts or after a crash) may display restore hints where 0 accounts would be added — only existing tokens replaced. callers that previously formatted messages as "Importing N accounts…" without checking replacedExistingCount could silently show "Import 0 accounts" to users. trace all consumers of assessments from this function to confirm each surfaces the replace-only message from formatNamedBackupRestoreResult, not an inline string.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 2073-2075
Comment:
**`getActionableNamedBackupRestores` filter broadened — verify startup-restore callers handle `imported === 0`**
the filter changed from:
```ts
assessment.eligibleForRestore &&
!assessment.wouldExceedLimit &&
assessment.imported !== null &&
assessment.imported > 0
```
to just `assessment.eligibleForRestore`, which now includes replace-only restores (`imported === 0, replacedExistingCount > 0`).
the browser and preview paths were updated, but the startup auto-restore flow (which surfaces prompts when the user has no accounts or after a crash) may display restore hints where 0 accounts would be added — only existing tokens replaced. callers that previously formatted messages as `"Importing N accounts…"` without checking `replacedExistingCount` could silently show `"Import 0 accounts"` to users. trace all consumers of `assessments` from this function to confirm each surfaces the replace-only message from `formatNamedBackupRestoreResult`, not an inline string.
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage.ts`:
- Around line 2526-2563: importNormalizedAccounts currently computes skipped as
normalized.accounts.length - imported which treats replaced accounts as skipped;
instead compute how many of the incoming normalized.accounts were actually
applied (either added or replaced) and set skipped = normalized.accounts.length
- appliedCount. Concretely, after producing deduplicatedAccounts, build a lookup
(by account.id or your canonical account key) for existingAccounts and for
deduplicatedAccounts, then compute appliedCount = count of entries in
normalized.accounts that appear in deduplicatedAccounts (regardless of whether
they replaced an existing one); set imported to the number of new additions
(deduplicatedAccounts.length - existingAccounts.length) if you still need that
metric, but compute skipped from appliedCount so replaced items are not flagged
as skipped; update the return to use these computed values (references:
importNormalizedAccounts, existingAccounts, normalized.accounts,
deduplicatedAccounts, deduplicateAccounts).
- Around line 2429-2447: restoreNamedBackup currently always calls
loadImportableBackupCandidate, re-opening the backup even when the caller
already has the candidate, and importNormalizedAccounts (or the restore counting
logic) treats replacements as "skipped", causing preview/restore semantic drift.
Change restoreNamedBackup(name, options) to accept and use an optional preloaded
candidate (e.g., options.candidate) and only call loadImportableBackupCandidate
when that candidate is not provided; keep the existing
buildNamedBackupMetadata/assessNamedBackupRestoreCandidate flow but pass the
supplied candidate into assessment to avoid re-reading the file. Also update the
account import/counting logic in importNormalizedAccounts (the code that
increments skipped/replacements) so that only accounts actually dropped due to
dedup are counted as skipped and replacements are not counted as skipped,
ensuring assessment.skipped matches restore result.
In `@test/codex-manager-cli.test.ts`:
- Around line 927-930: The restoreNamedBackupMock assertion is too loose—replace
the expect.objectContaining({ assessment: expect.anything() }) check with a
deterministic assertion that the passed assessment equals the expected/refreshed
assessment used earlier in the test; locate the call to restoreNamedBackupMock
in this file (and similar occurrences like at lines ~985, 2231, 2623, 3734,
3782, 3912, 4034, 4225) and assert using a deep equality matcher (e.g.,
expect(restoreNamedBackupMock).toHaveBeenCalledWith("named-backup",
expect.objectContaining({ assessment: expectedAssessment })) or the exact
object) where expectedAssessment is the known/refreshed assessment variable
constructed in the test (or extracted right before invocation) so the test fails
if a stale assessment is passed.
In `@test/storage-recovery-paths.test.ts`:
- Around line 783-786: The test is setting newerManualMtime in the future which
can cause flakes on some filesystems; change the mtime values so both are in the
past (e.g., keep olderManualMtime = new Date(Date.now() - 5000) and set
newerManualMtime to a slightly more recent past time like new Date(Date.now() -
1000)) and then call fs.utimes(olderManualPath, olderManualMtime,
olderManualMtime) and fs.utimes(newerManualPath, newerManualMtime,
newerManualMtime) so the ordering is deterministic across Windows and other
filesystems.
In `@test/storage.test.ts`:
- Around line 1269-1291: The test currently asserts a mismatch between the
assessment preview and the actual restore: assessment.skipped is asserted as 0
while restoreNamedBackup("Replace Only") returns skipped: 1; make these
consistent by updating the assessment expectation to
expect(assessment.skipped).toBe(1) (leave assessment.imported and the
restoreResult assertion unchanged) so the preview and apply counts match;
reference the assessment object assertions and the restoreNamedBackup /
restoreResult check to locate the two assertions to adjust.
- Around line 1750-1795: The test "retries transient backup read errors while
restoring backups" currently increments backupReads when the mocked fs.readFile
hits the backupPath but never asserts it; add an explicit assertion that
backupReads equals the expected retry count after calling
restoreNamedBackup("retry-restore-read") (e.g., expect(backupReads).toBe(2) if
one retry was expected), keeping the existing cleanup of
readFileSpy.mockRestore(); ensure the assertion is placed inside the try block
after const result = await restoreNamedBackup(...) and before restoring the spy
so the test deterministically verifies the retry behavior of restoreNamedBackup
and the mocked read path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 712ffd0d-b23c-4239-b773-91b028d18384
📒 Files selected for processing (5)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage-recovery-paths.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-recovery-paths.test.tstest/codex-manager-cli.test.tstest/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/storage.ts
🔇 Additional comments (7)
lib/codex-manager.ts (1)
4673-4757: good preview gating and confirmation flow.
lib/codex-manager.ts:4673blocks non-actionable restores before action selection, andlib/codex-manager.ts:4734adds a clear confirmation that includes replacement/active-account impact.test/storage-recovery-paths.test.ts (1)
751-753: nice assertion hardening here.the exact-path assertion in
test/storage-recovery-paths.test.ts:751prevents the prior weak pass condition and locks this regression to the intended winner snapshot.test/codex-manager-cli.test.ts (2)
991-1164: good replace-only preview regression coveragethis test exercises inspect → preview-restore → confirm → restore and verifies replace-only messaging, which is the right contract for this flow. good coverage against restore-preview regressions in
lib/codex-manager.ts:4695-4755andlib/codex-manager.ts:4912-4950.
4231-4326: good windows ebusy reassessment guard testthis is a solid windows filesystem edge-case regression: reassessment failure exits safely, skips confirm, and avoids restore. it matches the guard behavior in
lib/codex-manager.ts:4704-4718.test/storage.test.ts (3)
1213-1225: good coverage for explicit-null active-account preview state.the assertions in
test/storage.test.ts:1213-test/storage.test.ts:1225pin the active-account transition surface well and protect the restore-preview contract.
1301-1338: nice regression for prevalidated assessment reuse.this is a solid guard for redundant backup reads in the preview→restore path at
test/storage.test.ts:1301-test/storage.test.ts:1338, which is where windowsebusywindows tend to show up.
1477-1548: strong concurrency regression for stale assessment race.
test/storage.test.ts:1477-test/storage.test.ts:1548correctly reproduces the mutation-time limit race and verifies reassessment blocks restore.
| export async function restoreNamedBackup( | ||
| name: string, | ||
| options: { assessment?: BackupRestoreAssessment } = {}, | ||
| ): Promise<{ imported: number; total: number; skipped: number }> { | ||
| const backupPath = await resolveNamedBackupRestorePath(name); | ||
| return importAccounts(backupPath); | ||
| const candidate = await loadImportableBackupCandidate(backupPath); | ||
| if (options.assessment !== undefined) { | ||
| const assessment = assessNamedBackupRestoreCandidate( | ||
| await buildNamedBackupMetadata(name, backupPath, { candidate }), | ||
| candidate, | ||
| await loadAccounts(), | ||
| candidate.rawAccounts, | ||
| ); | ||
| if (!assessment.eligibleForRestore || assessment.wouldExceedLimit) { | ||
| throw new Error(assessment.error ?? "Backup is not eligible for restore"); | ||
| } | ||
| } | ||
| return importNormalizedAccounts(candidate.normalized, backupPath); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# check preview/restore coverage
rg -nP --type=ts -C3 'preview-restore|Restore Preview|restoreNamedBackup\(' test
# check explicit windows lock-error coverage
rg -nP --type=ts -C3 'EBUSY|EPERM|EAGAIN' testRepository: ndycode/codex-multi-auth
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the restoreNamedBackup implementation (lines 2429-2447)
echo "=== restoreNamedBackup function (2429-2447) ==="
sed -n '2429,2447p' lib/storage.ts
echo -e "\n=== loadImportableBackupCandidate definition ==="
rg -nP 'async function loadImportableBackupCandidate' lib/storage.ts -A 5
echo -e "\n=== importNormalizedAccounts definition ==="
rg -nP 'async function importNormalizedAccounts' lib/storage.ts -A 15Repository: ndycode/codex-multi-auth
Length of output: 1850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full importNormalizedAccounts function
sed -n '2526,2573p' lib/storage.ts
echo -e "\n=== deduplicateAccounts definition ==="
rg -nP 'function deduplicateAccounts' lib/storage.ts -A 20Repository: ndycode/codex-multi-auth
Length of output: 3151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the preview path to understand the flow mentioned in comment
echo "=== codex-manager.ts around line 4702 (preview restore) ==="
sed -n '4695,4710p' lib/codex-manager.ts
echo -e "\n=== Check if assessNamedBackupRestoreCandidate requires fresh load ==="
rg -nP 'function assessNamedBackupRestoreCandidate' lib/storage.ts -A 10
echo -e "\n=== Check preview flow in codex-manager ==="
rg -nP 'async function.*Restore.*Preview|preview.*restore' lib/codex-manager.ts -i -C 3 | head -60Repository: ndycode/codex-multi-auth
Length of output: 2264
🏁 Script executed:
#!/bin/bash
set -euo pipefandet
# Check the Replace Only test (line 1289)
echo "=== Replace Only test context ==="
sed -n '1265,1295p' test/storage.test.ts
echo -e "\n=== Check retry-restore-read test ==="
sed -n '1775,1805p' test/storage.test.ts
echo -e "\n=== Check retry-restore-dir test ==="
sed -n '1673,1750p' test/storage.test.tsRepository: ndycode/codex-multi-auth
Length of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the Replace Only test (line 1289)
echo "=== Replace Only test context ==="
sed -n '1265,1295p' test/storage.test.ts
echo -e "\n=== Check retry-restore-read test ==="
sed -n '1775,1805p' test/storage.test.ts
echo -e "\n=== Check retry-restore-dir test ==="
sed -n '1673,1750p' test/storage.test.tsRepository: ndycode/codex-multi-auth
Length of output: 4437
restore re-opens backup file, widening windows lock exposure.
lib/storage.ts:2434 calls loadImportableBackupCandidate unconditionally, re-reading the backup from disk even after preview has already assessed it at lib/codex-manager.ts:4702. this doubles filesystem touchpoints and increases ebusy/eagain exposure during preview→restore flows on windows under concurrent scanners.
separately, lib/storage.ts:2563 counts replacements as "skipped", causing semantic drift between assessment (which reports skipped: 0 for replace-only) and restore result (which reports skipped: 1). this inconsistency confuses preview/result semantics. the fix: only count accounts actually dropped due to dedup as skipped, not replacements.
test/storage.test.ts already has working coverage for ebusy/eagain retries during restore (test/storage.test.ts:1742, 1790).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2429 - 2447, restoreNamedBackup currently always
calls loadImportableBackupCandidate, re-opening the backup even when the caller
already has the candidate, and importNormalizedAccounts (or the restore counting
logic) treats replacements as "skipped", causing preview/restore semantic drift.
Change restoreNamedBackup(name, options) to accept and use an optional preloaded
candidate (e.g., options.candidate) and only call loadImportableBackupCandidate
when that candidate is not provided; keep the existing
buildNamedBackupMetadata/assessNamedBackupRestoreCandidate flow but pass the
supplied candidate into assessment to avoid re-reading the file. Also update the
account import/counting logic in importNormalizedAccounts (the code that
increments skipped/replacements) so that only accounts actually dropped due to
dedup are counted as skipped and replacements are not counted as skipped,
ensuring assessment.skipped matches restore result.
| expect(assessment.backupAccountCount).toBe(1); | ||
| expect(assessment.dedupedBackupAccountCount).toBe(1); | ||
| expect(assessment.imported).toBe(0); | ||
| expect(assessment.skipped).toBe(0); | ||
| expect(assessment.replacedExistingCount).toBe(1); | ||
| expect(assessment.eligibleForRestore).toBe(true); | ||
| expect(assessment.wouldExceedLimit).toBe(false); | ||
| expect(assessment.activeAccountOutcome).toBe("unchanged"); | ||
| expect(assessment.activeAccountChanged).toBe(false); | ||
| expect(assessment.namedBackupRestorePreview?.conflicts).toEqual([ | ||
| expect.objectContaining({ | ||
| conflict: expect.objectContaining({ | ||
| backupIndex: 0, | ||
| currentIndex: 0, | ||
| resolution: "backup-kept", | ||
| reasons: expect.arrayContaining(["accountId", "email"]), | ||
| }), | ||
| }), | ||
| ]); | ||
|
|
||
| const restoreResult = await restoreNamedBackup("Replace Only"); | ||
| expect(restoreResult).toEqual({ imported: 0, skipped: 1, total: 1 }); | ||
|
|
There was a problem hiding this comment.
do not codify a preview/apply skipped-count mismatch.
test/storage.test.ts:1272 expects assessment.skipped to be 0, while test/storage.test.ts:1290 expects restore skipped to be 1 for the same replace-only case. this bakes in inconsistent behavior instead of catching it.
proposed test fix
- expect(assessment.skipped).toBe(0);
+ // preview/apply counters should stay consistent for the same restore intent.
+ // once lib behavior is corrected, this guards against regressions.
+ // test/storage.test.ts:1272
+ expect(assessment.skipped).toBe(1);
@@
- const restoreResult = await restoreNamedBackup("Replace Only");
- expect(restoreResult).toEqual({ imported: 0, skipped: 1, total: 1 });
+ const restoreResult = await restoreNamedBackup("Replace Only");
+ expect(restoreResult).toEqual({ imported: 0, skipped: 1, total: 1 });
+ expect(restoreResult.skipped).toBe(assessment.skipped);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(assessment.backupAccountCount).toBe(1); | |
| expect(assessment.dedupedBackupAccountCount).toBe(1); | |
| expect(assessment.imported).toBe(0); | |
| expect(assessment.skipped).toBe(0); | |
| expect(assessment.replacedExistingCount).toBe(1); | |
| expect(assessment.eligibleForRestore).toBe(true); | |
| expect(assessment.wouldExceedLimit).toBe(false); | |
| expect(assessment.activeAccountOutcome).toBe("unchanged"); | |
| expect(assessment.activeAccountChanged).toBe(false); | |
| expect(assessment.namedBackupRestorePreview?.conflicts).toEqual([ | |
| expect.objectContaining({ | |
| conflict: expect.objectContaining({ | |
| backupIndex: 0, | |
| currentIndex: 0, | |
| resolution: "backup-kept", | |
| reasons: expect.arrayContaining(["accountId", "email"]), | |
| }), | |
| }), | |
| ]); | |
| const restoreResult = await restoreNamedBackup("Replace Only"); | |
| expect(restoreResult).toEqual({ imported: 0, skipped: 1, total: 1 }); | |
| expect(assessment.backupAccountCount).toBe(1); | |
| expect(assessment.dedupedBackupAccountCount).toBe(1); | |
| expect(assessment.imported).toBe(0); | |
| // preview/apply counters should stay consistent for the same restore intent. | |
| // once lib behavior is corrected, this guards against regressions. | |
| // test/storage.test.ts:1272 | |
| expect(assessment.skipped).toBe(1); | |
| expect(assessment.replacedExistingCount).toBe(1); | |
| expect(assessment.eligibleForRestore).toBe(true); | |
| expect(assessment.wouldExceedLimit).toBe(false); | |
| expect(assessment.activeAccountOutcome).toBe("unchanged"); | |
| expect(assessment.activeAccountChanged).toBe(false); | |
| expect(assessment.namedBackupRestorePreview?.conflicts).toEqual([ | |
| expect.objectContaining({ | |
| conflict: expect.objectContaining({ | |
| backupIndex: 0, | |
| currentIndex: 0, | |
| resolution: "backup-kept", | |
| reasons: expect.arrayContaining(["accountId", "email"]), | |
| }), | |
| }), | |
| ]); | |
| const restoreResult = await restoreNamedBackup("Replace Only"); | |
| expect(restoreResult).toEqual({ imported: 0, skipped: 1, total: 1 }); | |
| expect(restoreResult.skipped).toBe(assessment.skipped); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 1269 - 1291, The test currently asserts a
mismatch between the assessment preview and the actual restore:
assessment.skipped is asserted as 0 while restoreNamedBackup("Replace Only")
returns skipped: 1; make these consistent by updating the assessment expectation
to expect(assessment.skipped).toBe(1) (leave assessment.imported and the
restoreResult assertion unchanged) so the preview and apply counts match;
reference the assessment object assertions and the restoreNamedBackup /
restoreResult check to locate the two assertions to adjust.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
3127-3148:⚠️ Potential issue | 🔴 Criticalpin transaction persists to the storage path captured at transaction start.
lib/storage.ts:3134andlib/storage.ts:3161capturestate.storagePath, but the transaction persist and rollback closures atlib/storage.ts:3141-3143andlib/storage.ts:3174-3181still route throughsaveAccountsUnlocked()/saveFlaggedAccountsUnlocked(), which resolve paths from the live globals atlib/storage.ts:2999andlib/storage.ts:3446-3447. a concurrentsetStoragePath()can make a transaction read repo a and commit into repo b, which is straight data corruption.test/storage.test.ts:369-395already proves path drift is a real scenario; please add a vitest that flips the path beforepersist()and asserts only the original files change. 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: 3151-3202
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage.ts`:
- Around line 2082-2090: The fast-path in
getActionableNamedBackupRestores()/scannedBackups currently drops
entry.candidate.rawAccounts which causes inconsistent
skipped/conflict/replacedExistingCount math between
assessNamedBackupRestoreCandidate(), restoreNamedBackup(), and preview paths;
update the fast assessment loop to thread entry.candidate.rawAccounts (or the
exact transaction inputs) into assessNamedBackupRestoreCandidate() and use those
inputs when computing skipped/conflict/replacedExistingCount (instead of
normalized.accounts.length or preview-time replacedExistingCount) so counters
are computed from actual transaction inputs; add vitest tests that cover
duplicate raw backup rows and a preview→apply storage race to assert counts
remain identical, and ensure any new queuing/code handles EBUSY/429 retries and
that logging in these changes does not leak tokens/emails.
- Around line 2429-2448: restoreNamedBackup currently trusts a caller-provided
options.assessment without proving the freshly loaded candidate (via
loadImportableBackupCandidate) is the same backup the assessment was built
against; to fix, always compute and compare a stable fingerprint/metadata of the
loaded candidate (use buildNamedBackupMetadata and any stable fields used by
assessNamedBackupRestoreCandidate such as checksum/mtime/size or an explicit id)
against the assessment passed in options, and if they differ re-run
assessNamedBackupRestoreCandidate (or reject with a clear error) before calling
importNormalizedAccounts; update restoreNamedBackup to reference
loadImportableBackupCandidate, buildNamedBackupMetadata,
assessNamedBackupRestoreCandidate and importNormalizedAccounts accordingly and
add a vitest that simulates a file-modify-between-preview-and-apply case (same
name, changed valid JSON) to assert the operation is rejected or re-assessed.
In `@test/storage.test.ts`:
- Around line 1424-1478: The tests "throws when a named backup is deleted after
assessment" and "throws when a named backup becomes invalid JSON after
assessment" call assessNamedBackupRestore(...) to get an assessment but then
call restoreNamedBackup(...) without passing that assessment; update both
restoreNamedBackup calls to pass the previewed result (e.g.
restoreNamedBackup("deleted-after-assessment", { assessment }) and
restoreNamedBackup("invalid-after-assessment", { assessment })) so the tests
exercise the prevalidated/previewed restore path used in lib/codex-manager.ts
(restoreNamedBackup and assessNamedBackupRestore).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17d66d4c-8201-4556-9326-9e58f7786fd9
📒 Files selected for processing (2)
lib/storage.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.ts
| if (usesFastPath) { | ||
| for (const entry of scannedBackups) { | ||
| try { | ||
| const assessment = assessNamedBackupRestoreCandidate( | ||
| entry.backup, | ||
| entry.candidate, | ||
| currentStorage, | ||
| ); | ||
| recordAssessment(assessment); |
There was a problem hiding this comment.
thread raw backup rows through every preview/apply path.
lib/storage.ts:2085-2089 drops entry.candidate.rawAccounts on the fast assessment path, and lib/storage.ts:2563-2569 derives skipped from normalized.accounts.length. backups with duplicate raw rows will therefore report different conflict/skipped counts between getActionableNamedBackupRestores(), assessNamedBackupRestore(), and restoreNamedBackup(). restoreNamedBackup() also feeds preview-time replacedExistingCount into this math from lib/storage.ts:2446-2448, so a current-storage race can skew the apply result even when the file is unchanged. the existing replace-only case at test/storage.test.ts:1228-1293 will not catch either drift. please compute these counters from the actual transaction inputs and add vitest coverage for duplicate backup rows plus preview→apply storage races. 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: 2528-2570
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2082 - 2090, The fast-path in
getActionableNamedBackupRestores()/scannedBackups currently drops
entry.candidate.rawAccounts which causes inconsistent
skipped/conflict/replacedExistingCount math between
assessNamedBackupRestoreCandidate(), restoreNamedBackup(), and preview paths;
update the fast assessment loop to thread entry.candidate.rawAccounts (or the
exact transaction inputs) into assessNamedBackupRestoreCandidate() and use those
inputs when computing skipped/conflict/replacedExistingCount (instead of
normalized.accounts.length or preview-time replacedExistingCount) so counters
are computed from actual transaction inputs; add vitest tests that cover
duplicate raw backup rows and a preview→apply storage race to assert counts
remain identical, and ensure any new queuing/code handles EBUSY/429 retries and
that logging in these changes does not leak tokens/emails.
| export async function restoreNamedBackup( | ||
| name: string, | ||
| options: { assessment?: BackupRestoreAssessment } = {}, | ||
| ): Promise<{ imported: number; total: number; skipped: number }> { | ||
| const backupPath = await resolveNamedBackupRestorePath(name); | ||
| const candidate = await loadImportableBackupCandidate(backupPath); | ||
| const assessment = | ||
| options.assessment ?? | ||
| assessNamedBackupRestoreCandidate( | ||
| await buildNamedBackupMetadata(name, backupPath, { candidate }), | ||
| candidate, | ||
| await loadAccounts(), | ||
| candidate.rawAccounts, | ||
| ); | ||
| if (!assessment.eligibleForRestore) { | ||
| throw new Error(assessment.error ?? "Backup is not eligible for restore"); | ||
| } | ||
| return importNormalizedAccounts(candidate.normalized, backupPath, { | ||
| replacedExistingCount: assessment.replacedExistingCount ?? 0, | ||
| }); |
There was a problem hiding this comment.
revalidate the preview against the file you actually import.
lib/storage.ts:2433-2444 will honor options.assessment without proving the candidate loaded at lib/storage.ts:2434 is still the same backup the user previewed. if that file is rewritten with new valid contents between preview and apply, restore can import different accounts under stale eligibility and counter data. test/storage.test.ts:1424-1478 only covers missing and invalid files, so please add a vitest for the { assessment } path where the backup keeps the same name but new valid json. 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 2429 - 2448, restoreNamedBackup currently trusts
a caller-provided options.assessment without proving the freshly loaded
candidate (via loadImportableBackupCandidate) is the same backup the assessment
was built against; to fix, always compute and compare a stable
fingerprint/metadata of the loaded candidate (use buildNamedBackupMetadata and
any stable fields used by assessNamedBackupRestoreCandidate such as
checksum/mtime/size or an explicit id) against the assessment passed in options,
and if they differ re-run assessNamedBackupRestoreCandidate (or reject with a
clear error) before calling importNormalizedAccounts; update restoreNamedBackup
to reference loadImportableBackupCandidate, buildNamedBackupMetadata,
assessNamedBackupRestoreCandidate and importNormalizedAccounts accordingly and
add a vitest that simulates a file-modify-between-preview-and-apply case (same
name, changed valid JSON) to assert the operation is rejected or re-assessed.
| it("throws when a named backup is deleted after assessment", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "deleted-backup", | ||
| refreshToken: "ref-deleted-backup", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| const backup = await createNamedBackup("deleted-after-assessment"); | ||
| await clearAccounts(); | ||
|
|
||
| const assessment = await assessNamedBackupRestore("deleted-after-assessment"); | ||
| expect(assessment.eligibleForRestore).toBe(true); | ||
|
|
||
| await removeWithRetry(backup.path, { force: true }); | ||
|
|
||
| await expect( | ||
| restoreNamedBackup("deleted-after-assessment"), | ||
| ).rejects.toThrow(/Import file not found/); | ||
| expect((await loadAccounts())?.accounts ?? []).toHaveLength(0); | ||
| }); | ||
|
|
||
| it("throws when a named backup becomes invalid JSON after assessment", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "invalid-backup", | ||
| refreshToken: "ref-invalid-backup", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| const backup = await createNamedBackup("invalid-after-assessment"); | ||
| await clearAccounts(); | ||
|
|
||
| const assessment = await assessNamedBackupRestore("invalid-after-assessment"); | ||
| expect(assessment.eligibleForRestore).toBe(true); | ||
|
|
||
| await fs.writeFile(backup.path, "not valid json {[", "utf-8"); | ||
|
|
||
| await expect( | ||
| restoreNamedBackup("invalid-after-assessment"), | ||
| ).rejects.toThrow(/Invalid JSON in import file/); | ||
| expect((await loadAccounts())?.accounts ?? []).toHaveLength(0); | ||
| }); |
There was a problem hiding this comment.
exercise the prevalidated restore path in these stale-backup tests.
lib/codex-manager.ts:4700-4710 and lib/codex-manager.ts:4952-4987 pass the previewed assessment into restoreNamedBackup(), but test/storage.test.ts:1446-1476 does not. as written, both tests only cover a fresh restore, not the preview→apply path this pr adds. pass { assessment } into each restore so the suite actually proves stale previews still fail after deletion or file corruption.
proposed test fix
- await expect(
- restoreNamedBackup("deleted-after-assessment"),
- ).rejects.toThrow(/Import file not found/);
+ await expect(
+ restoreNamedBackup("deleted-after-assessment", { assessment }),
+ ).rejects.toThrow(/Import file not found/);
@@
- await expect(
- restoreNamedBackup("invalid-after-assessment"),
- ).rejects.toThrow(/Invalid JSON in import file/);
+ await expect(
+ restoreNamedBackup("invalid-after-assessment", { assessment }),
+ ).rejects.toThrow(/Invalid JSON in import file/);as per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 1424 - 1478, The tests "throws when a
named backup is deleted after assessment" and "throws when a named backup
becomes invalid JSON after assessment" call assessNamedBackupRestore(...) to get
an assessment but then call restoreNamedBackup(...) without passing that
assessment; update both restoreNamedBackup calls to pass the previewed result
(e.g. restoreNamedBackup("deleted-after-assessment", { assessment }) and
restoreNamedBackup("invalid-after-assessment", { assessment })) so the tests
exercise the prevalidated/previewed restore path used in lib/codex-manager.ts
(restoreNamedBackup and assessNamedBackupRestore).
| } | ||
|
|
||
| export async function appendSyncHistoryEntry( | ||
| entry: SyncHistoryEntry, | ||
| ): Promise<void> { | ||
| const writePromise = withHistoryLock(async () => { | ||
| const paths = getSyncHistoryPaths(); | ||
| lastAppendPaths = paths; | ||
| await ensureHistoryDir(paths.directory); | ||
| await fs.appendFile(paths.historyPath, `${serializeEntry(entry)}\n`, { | ||
| encoding: "utf8", | ||
| mode: 0o600, | ||
| }); | ||
| await trimHistoryFileIfNeeded(paths); | ||
| await fs.writeFile(paths.latestPath, `${JSON.stringify(entry, null, 2)}\n`, { | ||
| encoding: "utf8", | ||
| mode: 0o600, |
There was a problem hiding this comment.
trimHistoryFileIfNeeded is not atomic — windows crash between truncation and write corrupts history
fs.writeFile with O_WRONLY | O_CREAT | O_TRUNC first truncates the file to zero bytes, then writes the trimmed content. if the process is killed (or windows EPERM/EBUSY interrupts the write) between the truncation and the full write completing, the history file is left empty or partially written with no way to recover the entries.
this is inside the historyMutex lock so concurrent access is safe, but the lock offers no protection against mid-write process termination on windows.
prefer the temp-file + rename pattern used elsewhere in this codebase for atomic writes:
const tempPath = `${paths.historyPath}.${Date.now()}.tmp`;
await fs.writeFile(tempPath, trimmedContent, { encoding: "utf8", mode: 0o600 });
await fs.rename(tempPath, paths.historyPath);fs.rename is atomic on windows (within the same volume) and means the old history is never lost — either the old file persists or the trimmed file is complete.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/sync-history.ts
Line: 190-206
Comment:
**`trimHistoryFileIfNeeded` is not atomic — windows crash between truncation and write corrupts history**
`fs.writeFile` with `O_WRONLY | O_CREAT | O_TRUNC` first truncates the file to zero bytes, then writes the trimmed content. if the process is killed (or windows EPERM/EBUSY interrupts the write) between the truncation and the full write completing, the history file is left empty or partially written with no way to recover the entries.
this is inside the `historyMutex` lock so concurrent access is safe, but the lock offers no protection against mid-write process termination on windows.
prefer the temp-file + rename pattern used elsewhere in this codebase for atomic writes:
```ts
const tempPath = `${paths.historyPath}.${Date.now()}.tmp`;
await fs.writeFile(tempPath, trimmedContent, { encoding: "utf8", mode: 0o600 });
await fs.rename(tempPath, paths.historyPath);
```
`fs.rename` is atomic on windows (within the same volume) and means the old history is never lost — either the old file persists or the trimmed file is complete.
How can I resolve this? If you propose a fix, please make it concise.
|
Closing this preserved |
Summary
#86fresh/12-backup-browserWhat Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
17cec7eand5ddee4dAdditional Notes
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 lands the restore-preview slice from archived #86 on top of
fresh/12-backup-browser, adding a full named-backup assessment and browser UI flow, a replace-only restore path, a newsync-historyNDJSON module, and adestructive-actionsextraction with proper index-rebasing and rollback. the storage layer gains atomic-delete retries (unlinkWithRetry), transaction path-anchoring (storagePathinAsyncLocalStoragecontext), and a boolean return fromclearAccounts/clearFlaggedAccounts. coverage is broad (17k line diff, 43 files, all checklist gates passed).key findings:
loadBackupBrowserEntries— when the backup browser opens without startup assessments,listNamedBackups()viascanNamedBackups()reads and parses every.jsonfile, thenassessNamedBackupRestore()reads each file again; on windows this doubles the EBUSY/antivirus lock window and silently produces unrestorable entries with no user-visible errortrimHistoryFileIfNeededis non-atomic — usesfs.writeFile(truncate-then-write) instead of temp+rename; a windows process kill between truncation and write completion leavessync-history.ndjsonempty with no recovery pathrecoveryState.allAssessments(the startup snapshot) is re-passed asstartupAssessmentson every retry after a cancelled restore, so users see stale data if they added or modified accounts between visitsscanNamedBackupsfailure — no test simulates one file being EBUSY mid-chunk, so the mismatch betweentotalBackupsandallAssessments.lengthon windows is not regression-testedreadFileSyncimport insync-history.ts— dead code, drop itConfidence Score: 3/5
storagePathanchor in transactions is a solid correctness fix. the two P1 issues (double-read EBUSY window inloadBackupBrowserEntries, non-atomic trim insync-history.ts) are real but narrow — they only manifest under windows antivirus/vss contention and process termination respectively, not under normal operation. the stale-assessment UX issue is low-severity. overall the PR is solid foundational work with two targeted fixes needed before it's fully windows-safe.lib/codex-manager.ts(double-read inloadBackupBrowserEntries) andlib/sync-history.ts(non-atomictrimHistoryFileIfNeeded) need the most attention before mergeImportant Files Changed
BackupRestoreAssessment,NamedBackupMetadata, and the fullassessNamedBackupRestoreCandidatepipeline; introducesimportNormalizedAccounts,scanNamedBackups,listNamedBackupsWithoutLoading, andrestoreNamedBackup; also refactorsclearAccounts/clearFlaggedAccountsto returnbooleanand addsunlinkWithRetrywith windows EBUSY/EPERM backoff. thestoragePathanchor added to both transaction contexts closes a subtle TOCTOU for path changes mid-transaction. theeligibleForRestorebroadening (now includes replace-only) is intentional and covered by new tests.runBackupBrowserManager,runBackupRestorePreview,loadBackupBrowserEntries, and therestore-backupCLI command; integratesdestructiveActionInFlightguard forfreshandresetmodes; refactorshandleManageActionto delegate todeleteAccountAtIndexfor proper index rebasing and flagged-account cleanup. the double-read issue inloadBackupBrowserEntries(listNamedBackups + per-backup assessNamedBackupRestore) is a new windows EBUSY risk. startup recovery flow withallowEmptyStorageMenuandpendingRecoveryStateis complex but the logic appears correct.MAX_HISTORY_ENTRIEStrim;trimHistoryFileIfNeededuses a destructivewriteFilepattern instead of temp+rename, creating a window for history corruption on windows if the process is killed mid-write; also contains a stalereadFileSyncimport that is never used.deleteSavedAccounts,resetLocalState,deleteAccountAtIndex, andclampActiveIndicesfrom codex-manager; thedeleteAccountAtIndexrollback path (revertsaveAccountsifsaveFlaggedAccountsfails) andAggregateErrorpropagation on double-failure are well-structured;rebaseActiveIndicesAfterDeletecorrectly shifts all family indices above the removed slot.restore-backupandresetlogin modes,isInteractiveLoginMenuAvailablehelper, andpromptResetTypedConfirmwith RESET-typed confirmation guard; the fallback path now correctly gatesfreshbehindpromptDeleteAllTypedConfirm(previously missing); changes are low-risk formatting/refactor with two meaningful new mode branches.listNamedBackups/restoreNamedBackupend-to-end flows, andclearFlaggedAccountsboolean return assertions; good coverage of the new APIs though no test simulates a partial scan failure (one file EBUSY duringscanNamedBackups).deleteAccountAtIndexrollback,clampActiveIndices,rebaseActiveIndicesAfterDelete, andresetLocalState; rollback behavior tested via injected save errors; looks thorough.resolveStartupRecoveryActionunit tests, and reset/fresh mode confirmation gate coverage; the scale of the test file makes navigation difficult but content appears thorough.getActionableNamedBackupRestoreseligibleForRestorebroadening; covers both eligible and ineligible cases and startup recovery action resolution; the concurrency/limit-race tests added for the guard check are valuable.appendSyncHistoryEntry,readSyncHistory, andgetSyncHistoryPaths; covers trim behavior at >200 entries and kind filtering; does not test partial-write corruption scenarios or mutex contention on windows.Sequence Diagram
sequenceDiagram participant CLI as runCodexMultiAuthCli participant Login as runAuthLogin participant Browser as runBackupBrowserManager participant Preview as runBackupRestorePreview participant Storage as lib/storage.ts CLI->>Login: restore-backup command Login->>Storage: getActionableNamedBackupRestores()<br/>(startup recovery path) Storage-->>Login: ActionableNamedBackupRecoveries Login->>Browser: runBackupBrowserManager(displaySettings, allAssessments?) Browser->>Storage: listNamedBackups() → scanNamedBackups()<br/>⚠️ reads all .json files (read #1) Storage-->>Browser: NamedBackupMetadata[] loop per backup (chunked by NAMED_BACKUP_LIST_CONCURRENCY) Browser->>Storage: assessNamedBackupRestore(name)<br/>⚠️ reads same .json file again (read #2) Storage-->>Browser: BackupRestoreAssessment end Browser->>Preview: runBackupRestorePreview(entry) Preview->>Storage: assessNamedBackupRestore(name)<br/>(read #3 — pre-confirm re-assess) Storage-->>Preview: BackupRestoreAssessment Preview->>Storage: restoreNamedBackup(name, {assessment})<br/>⚠️ re-assesses internally (ignores passed assessment) Storage->>Storage: withAccountStorageTransaction → importNormalizedAccounts Storage-->>Preview: {imported, total, skipped} Preview-->>Browser: "restored" | "dismissed" Browser-->>Login: BackupRestoreManagerResultComments Outside Diff (4)
test/storage-recovery-paths.test.ts, line 1182 (link)Weakened assertion no longer verifies the expected winner
the assertion was narrowed from
toBe(manual-meta-checkpoint)tonot.toBe(.cache). this only confirms the wrong file isn't picked; it doesn't confirm the right file is. if the path resolution logic regresses and picks some third unexpected file, this test will silently pass.the
setTimeout(resolve, 20)delay addresses the mtime race — that's fine — but the assertion should still pin the correct winner:if this is still flaky after the mtime nudge, consider bumping the delay or using a deterministic mtime offset rather than weakening the contract.
Prompt To Fix With AI
lib/codex-manager.ts, line 752-756 (link)result.skipped === replacedCountcondition never fires for standard replace-only restoresfor the typical single-account replace-only scenario (
backup = [A'(new_token)],existing = [A(old_token)]),importNormalizedAccountscomputes:so
result.skipped === replacedCountresolves to0 === 1→false, and the fallback message"Imported 0 accounts. Skipped 0. Total accounts: 1."is shown instead of the intended"Replaced 1 current account.". the condition only becomes truthy whenbackup_count === 2 × replacedCount(i.e. exactly half the backup entries are within-backup duplicates), which is an obscure edge case.the test at line 680–682 masks this by mocking
restoreNamedBackupto return{ imported: 0, skipped: 1, total: 1 }— a value the real function never produces for this scenario.the condition should just be:
also needs a companion vitest that calls the real
importNormalizedAccounts(not mocked) and passes the output toformatNamedBackupRestoreResultto guard against a recurrence.Prompt To Fix With AI
lib/codex-manager.ts, line 1002-1035 (link)loadBackupBrowserEntriesdouble-reads every backup file on the non-fast path — widens EBUSY window on windowswhen
startupAssessmentsis not provided, the function first callslistNamedBackups()→scanNamedBackups()→loadBackupCandidate(path)for every backup, then immediately callsassessNamedBackupRestore(backup.name, ...)which callsloadBackupCandidate(path)again for every backup. each backup JSON is therefore read twice from disk in rapid succession. on windows, an antivirus or shadow-copy lock between the two reads converts a valid scan into an EBUSY failure at assessment time, returningassessmentErrorentries in the browser UI that were fine a millisecond earlier.getActionableNamedBackupRestoresavoids this by reusing theNamedBackupScanEntrydata from the scan.loadBackupBrowserEntriesshould do the same — callscanNamedBackups()directly and pass the already-loaded candidates toassessNamedBackupRestoreCandidateinstead of fetching them twice:no additional vitest for the double-read race exists in the new tests; worth adding a test that stubs
fs.readFileto fail on the second call per path and verifies graceful degradation.Prompt To Fix With AI
lib/codex-manager.ts, line 928-960 (link)when
startupAssessmentsis not provided,loadBackupBrowserEntriescallslistNamedBackups()which internally callsscanNamedBackups()— that already reads and fully parses every.jsonbackup file to build the metadata +LoadedBackupCandidate. the parsed candidate is then discarded. immediately after,assessNamedBackupRestore(backup.name, { currentStorage })reads every same file a second time.on windows, antivirus scanners or vss shadow-copy can hold an exclusive handle between those two reads, causing the second read to fail with
EBUSY. the error is caught and the entry gets anassessmentErrorlabel making it unrestorable from the browser — no user-visible explanation is shown.the fast path already solves this:
getActionableNamedBackupRestores()usesscanNamedBackups()internally and callsassessNamedBackupRestoreCandidatedirectly from the already-loaded candidate (single read per file). consider replacing thelistNamedBackups+ per-backupassessNamedBackupRestoreloop with a singlegetActionableNamedBackupRestores()call:this eliminates the second per-file disk read and the doubled windows EBUSY window.
Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "fix(auth): bind rest..."