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 | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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).
| } | ||
| const backupPaths = | ||
| await getAccountsBackupRecoveryCandidatesWithDiscovery(path); | ||
| for (const candidate of [path, ...backupPaths, markerPath]) { | ||
| let hadError = false; | ||
| for (const candidate of [path, ...backupPaths]) { | ||
| try { | ||
| await fs.unlink(candidate); | ||
| await unlinkWithRetry(candidate); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| hadError = true; | ||
| log.error("Failed to clear flagged account storage", { | ||
| path: candidate, | ||
| error: String(error), | ||
| }); | ||
| if (candidate === path) { | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if (!hadError) { | ||
| try { | ||
| await unlinkWithRetry(markerPath); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.error("Failed to clear flagged reset marker", { | ||
| path, | ||
| markerPath, | ||
| error: String(error), | ||
| }); | ||
| hadError = true; |
There was a problem hiding this comment.
old clearFlaggedAccounts deleted markerPath inside the same loop as [path, ...backupPaths]; if a backup delete failed it continued and still cleaned up the marker. new code gates marker deletion on !hadError, so a single EBUSY/EPERM on any .bak file leaves the reset intent marker on disk even though the primary flagged-accounts file was successfully removed.
on the next startup the lingering marker may suppress WAL replay or trigger "reset in progress" recovery logic for flagged accounts that are actually clean, which silently discards valid recovery state. the same pattern was applied in clearAccounts (the hadError + unlinkWithRetry refactor is good), but flagged accounts are a distinct concern because their reset marker is separate.
consider deleting the marker unconditionally if the primary path delete succeeded, regardless of backup cleanup failures:
let primaryCleared = false;
for (const candidate of [path, ...backupPaths]) { ... }
// always attempt marker cleanup if main file is gone
if (primaryCleared || !hadError) {
try { await unlinkWithRetry(markerPath); } catch { ... }
}no vitest covers the scenario where unlinkWithRetry succeeds on path but fails on a backup path and the marker state is verified.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 3511-3540
Comment:
**reset marker kept alive when only a backup file fails to delete — false-positive intent signal on next startup**
old `clearFlaggedAccounts` deleted `markerPath` inside the same loop as `[path, ...backupPaths]`; if a backup delete failed it continued and still cleaned up the marker. new code gates marker deletion on `!hadError`, so a single EBUSY/EPERM on any `.bak` file leaves the reset intent marker on disk even though the primary flagged-accounts file was successfully removed.
on the next startup the lingering marker may suppress WAL replay or trigger "reset in progress" recovery logic for flagged accounts that are actually clean, which silently discards valid recovery state. the same pattern was applied in `clearAccounts` (the `hadError` + `unlinkWithRetry` refactor is good), but flagged accounts are a distinct concern because their reset marker is separate.
consider deleting the marker unconditionally if the primary `path` delete succeeded, regardless of backup cleanup failures:
```ts
let primaryCleared = false;
for (const candidate of [path, ...backupPaths]) { ... }
// always attempt marker cleanup if main file is gone
if (primaryCleared || !hadError) {
try { await unlinkWithRetry(markerPath); } catch { ... }
}
```
no vitest covers the scenario where `unlinkWithRetry` succeeds on `path` but fails on a backup path and the marker state is verified.
How can I resolve this? If you propose a fix, please make it concise.
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 ports the restore-preview slice from archived PR #86 onto the fresh/12-backup-browser base, wiring up
runBackupRestorePreview,assessNamedBackupRestoreCandidate,importNormalizedAccounts, and the backup browser manager into a cohesive replace-only + merge restore flow. thedestructive-actions.tsextraction andsync-history.tsaddition are clean, and the assessment type model is thorough. three issues need attention before merge:formatNamedBackupRestoreResultguards the "Replaced N account(s)" branch withresult.skipped === replacedCount, a condition that evaluates tofalsefor the standard single-account replacement (skipped = 0, replacedCount = 1). users always see "Imported 0 accounts. Skipped 0." instead. the test at line 680–682 masks this by mockingrestoreNamedBackupwithskipped: 1, a value the realimportNormalizedAccountsnever produces for that scenario. fix: drop theresult.skipped === replacedCountguard.loadBackupBrowserEntriesdouble-reads every backup on the non-fast path —listNamedBackups()loads all backup files viascanNamedBackups, thenassessNamedBackupRestoreloads each one again. on windows this doubles the EBUSY exposure window per file.getActionableNamedBackupRestoresavoids this by reusing the scanned candidates;loadBackupBrowserEntriesshould do the same.clearFlaggedAccountskeeps the reset-intent marker alive when any backup file fails to delete — if a.bakauxiliary file gets an EPERM the main flagged file may already be gone, buthadError = trueskips marker cleanup. on next startup the lingering marker can suppress WAL replay or trigger spurious recovery prompts. the marker should be cleaned up whenever the primary path deletion succeeded.Confidence Score: 3/5
formatNamedBackupRestoreResult) means the primary new user-facing feature (replace-only result message) never renders correctly, and the test suite doesn't catch it due to incorrect mock values. the windows double-read and marker-cleanup gaps are lower severity but worth fixing before promoting to stable.lib/codex-manager.ts(formatNamedBackupRestoreResult condition, loadBackupBrowserEntries double-read) andlib/storage.ts(clearFlaggedAccounts marker gating)Important Files Changed
importNormalizedAccounts. reset-marker gating bug inclearFlaggedAccountsand pre-existing double-read concerns flagged.runBackupRestorePreviewandrunBackupBrowserManagerare the main new flows.formatNamedBackupRestoreResulthas a logic bug in the replace-only branch;loadBackupBrowserEntriesreads each backup file twice on the non-fast path (EBUSY risk on windows).deleteAccountAtIndex,deleteSavedAccounts, andresetLocalStatefromcodex-manager.ts. logic is clean;rebaseActiveIndicesAfterDeletecorrectly shifts family indices; rollback on flagged-save failure is thorough.restoreNamedBackupreturningskipped: 1— a value the real function never produces for that scenario — which masks theformatNamedBackupRestoreResultcondition bug.importNormalizedAccounts→formatNamedBackupRestoreResultfor replace-only.destructive-actions.ts. thorough including flagged-save rollback scenario.removeAllAccountslabel corrected to "Delete Saved Accounts", "Restore From Backup" added, settings section hints added. no logic.restore-backupmenu entry andisInteractiveLoginMenuAvailablehelper; allows empty-storage login menu when backups exist. looks correct.Sequence Diagram
sequenceDiagram participant User participant CLI as runBackupBrowserManager participant Preview as runBackupRestorePreview participant Storage as storage.ts participant FS as Filesystem User->>CLI: codex auth restore-backup CLI->>Storage: listNamedBackups() Storage->>FS: readdir(backupRoot) Storage->>FS: readFile(backup.json) ×N [scanNamedBackups] Storage-->>CLI: NamedBackupMetadata[] CLI->>Storage: assessNamedBackupRestore(name) ×N Storage->>FS: readFile(backup.json) ×N [double-read] Storage->>Storage: loadAccounts() Storage-->>CLI: BackupRestoreAssessment[] User->>CLI: select backup → preview-restore CLI->>Preview: runBackupRestorePreview(entry) Preview->>Storage: assessNamedBackupRestore(name) [re-assess] Storage->>FS: readFile(backup.json) Storage-->>Preview: BackupRestoreAssessment Preview->>User: show preview + confirm prompt User->>Preview: confirm = true Preview->>Storage: restoreNamedBackup(name, {assessment}) Storage->>FS: readFile(backup.json) [loadImportableBackupCandidate] Storage->>Storage: withAccountStorageTransaction Storage->>Storage: deduplicateAccounts([existing, backup]) Storage->>FS: writeFile(accounts.json) Storage-->>Preview: {imported, skipped, total} Preview->>User: formatNamedBackupRestoreResult(result, assessment)Comments Outside Diff (3)
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
Prompt To Fix All With AI
Last reviewed commit: "test(restore): tight..."