feat(auth): add backup browser and rotating restore support#111
feat(auth): add backup browser and rotating restore support#111
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. |
📝 WalkthroughWalkthroughintroduces a browser-driven backup manager in Changes
Sequence Diagram(s)sequenceDiagram
participant user as User
participant cli as CLI
participant browser as BackupBrowserManager
participant storage as Storage
participant ui as UI
user->>cli: run restore-backup
cli->>browser: runBackupBrowserManager()
browser->>storage: listNamedBackups()
storage-->>browser: named metadata
browser->>storage: listRotatingBackups()
storage-->>browser: rotating metadata
browser->>browser: assess entries (per-entry)
browser->>ui: present menu of entries
ui-->>user: show entries
user->>ui: select inspect -> confirm restore
ui-->>browser: selected entry + confirm
browser->>storage: assess (re-check) entry
storage-->>browser: assessment result
alt eligible
browser->>storage: restoreNamedBackup/restoreRotatingBackup
storage-->>browser: restore result
browser-->>user: restore complete
else ineligible
browser-->>user: show eligibility failure
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes detailed observations
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ 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 backup browser UI replacing the old restore manager, with support for both named and rotating backups, plus a detail/inspect flow for each backup entry.
Changes:
- Introduces
listRotatingBackups()and supporting helpers (parseRotatingBackupSlot,formatRotatingBackupLabel,normalizeBackupLookupName) in storage - Replaces
runBackupRestoreManagerwithrunBackupBrowserManagerfeaturing a two-section menu (named + rotating) and per-backup detail view - Updates tests for the new backup browser behavior and rotating backup listing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/storage.ts | Adds RotatingBackupMetadata, listRotatingBackups, backup name normalization, and refactors metadata building |
| lib/codex-manager.ts | Replaces restore manager with backup browser, adds detail view, file size formatting, and logger |
| test/storage.test.ts | Tests for rotating backup listing including invalid metadata and ENOENT/EPERM handling |
| test/codex-manager-cli.test.ts | Updates CLI tests for backup browser rename and new menu structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/storage.ts
Outdated
| const requested = normalizedName; | ||
| const backupRoot = getNamedBackupRoot(getStoragePath()); | ||
| const requestedWithExtension = requested.toLowerCase().endsWith(".json") | ||
| ? requested | ||
| : `${requested}.json`; |
lib/codex-manager.ts
Outdated
| const legacySelection = selection as unknown as LegacyBackupRestoreSelection; | ||
| if (legacySelection.type === "restore" && legacySelection.assessment) { | ||
| entry = { | ||
| kind: "named", | ||
| label: legacySelection.assessment.backup.name, | ||
| backup: legacySelection.assessment.backup, | ||
| assessment: legacySelection.assessment, | ||
| }; | ||
| action = "restore"; | ||
| } else if (selection.type === "inspect") { |
| lines.push( | ||
| `${stylePromptText("Current accounts:", "muted")} ${assessment.currentAccountCount}`, | ||
| `${stylePromptText("Merged after dedupe:", "muted")} ${assessment.mergedAccountCount ?? "unknown"}`, | ||
| `${stylePromptText("Would import:", "muted")} ${assessment.imported ?? "unknown"}`, | ||
| `${stylePromptText("Would skip:", "muted")} ${assessment.skipped ?? "unknown"}`, | ||
| `${stylePromptText("Eligibility:", "muted")} ${assessment.wouldExceedLimit ? `Would exceed ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts` : assessment.eligibleForRestore ? "Recoverable" : (assessment.error ?? "Unavailable")}`, |
| const candidate = await loadBackupCandidate(candidatePath); | ||
| if (!candidate.normalized && candidate.errorCode === "ENOENT") { | ||
| continue; | ||
| } | ||
| const metadata = await buildBackupFileMetadata(candidatePath, { | ||
| candidate, | ||
| }); | ||
| backups.push({ | ||
| label: formatRotatingBackupLabel(slot), | ||
| slot, | ||
| ...metadata, | ||
| }); | ||
| } | ||
|
|
||
| return backups.sort((a, b) => a.slot - b.slot); |
There was a problem hiding this comment.
loadBackupCandidate and the subsequent buildBackupFileMetadata (which calls fs.stat) both hit the filesystem with no retry. the rest of this codebase wraps retryable paths with retryTransientFilesystemOperation (EBUSY/EAGAIN/EPERM on win32). on windows, an antivirus scanner that briefly locks a .bak file will cause loadBackupCandidate to return an error with errorCode === "EBUSY", which does NOT match the === "ENOENT" skip guard, so the slot is included but surfaces as valid: false in the browser. the user sees a corrupt backup when it's just temporarily locked — and a second fs.stat on the same locked path is then issued by buildBackupFileMetadata, adding more contention.
const candidate = await retryTransientFilesystemOperation(() =>
loadBackupCandidate(candidatePath),
);without this, the storage.test.ts test "keeps rotating backups visible when a non-ENOENT error mentions an ENOENT path segment" only covers the steady-state EPERM case, not the transient EBUSY race that's common on windows desktop deployments.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 1831-1845
Comment:
**no retry on EBUSY/EAGAIN — windows antivirus will show valid slots as broken**
`loadBackupCandidate` and the subsequent `buildBackupFileMetadata` (which calls `fs.stat`) both hit the filesystem with no retry. the rest of this codebase wraps retryable paths with `retryTransientFilesystemOperation` (EBUSY/EAGAIN/EPERM on win32). on windows, an antivirus scanner that briefly locks a `.bak` file will cause `loadBackupCandidate` to return an error with `errorCode === "EBUSY"`, which does NOT match the `=== "ENOENT"` skip guard, so the slot is included but surfaces as `valid: false` in the browser. the user sees a corrupt backup when it's just temporarily locked — and a second `fs.stat` on the same locked path is then issued by `buildBackupFileMetadata`, adding more contention.
```ts
const candidate = await retryTransientFilesystemOperation(() =>
loadBackupCandidate(candidatePath),
);
```
without this, the `storage.test.ts` test "keeps rotating backups visible when a non-ENOENT error mentions an ENOENT path segment" only covers the steady-state EPERM case, not the transient EBUSY race that's common on windows desktop deployments.
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/codex-manager-cli.test.ts (1)
2202-2271:⚠️ Potential issue | 🟠 Majordo not lock in the extra storage reread here.
test/codex-manager-cli.test.ts:2267-2270makes this pass only ifloadAccounts()has already been called twice when startup assessments are reused. that bakes in the redundant disk i/o path instead of catching it, and on windows it increases the chance ofebusywhile the backup browser is opening.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.proposed fix
selectMock.mockImplementationOnce(async (_items, options) => { expect(options).toMatchObject({ message: "Backup Browser" }); - expect(loadAccountsCallCount).toBe(2); + expect(loadAccountsCallCount).toBe(1); return { type: "back" }; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 2202 - 2271, The test is wrongly asserting that loadAccounts() was called twice, which bakes in an extra storage reread; remove or relax the assertion inside the selectMock (remove expect(loadAccountsCallCount).toBe(2) or change it to expect(loadAccountsCallCount).toBeGreaterThanOrEqual(1)) so the test does not require the redundant disk I/O. Update the test in test/codex-manager-cli.test.ts where loadAccountsMock, selectMock and getActionableNamedBackupRestoresMock are used (specifically the selectMock implementation) to avoid locking in the extra read.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4544-4569: The UI only offers restore for named backups and the
restore dispatcher always calls restoreNamedBackup(), leaving rotating backups
(.bak/.bak.N) view-only; update the backup browser flow in the function that
builds the select menu (the block returning "restore" vs "back") to include a
Restore action for rotating entries (check entry.kind or assessment properties)
and wire the restore handler at the higher-level dispatcher (the routine that
currently invokes restoreNamedBackup()) to call the new rotating restore
function (e.g., restoreRotatingBackup or restoreRotatingFlow) when the entry
indicates a rotating backup; add/adjust vitest coverage (referenced
test/storage.test.ts:4028-4242) to exercise the browser restore path for
rotating entries and ensure the new restore queue implements retry/backoff for
EBUSY/429 and concurrency controls per project guidelines.
- Around line 4578-4588: Wrap the initial call to listRotatingBackups() in a
try/catch so any rotating-backup I/O error is treated as non-fatal: on error set
rotatingBackups = [] (not null/undefined), log the error via
normalizeBackupAssessmentError, and continue to build named entries using
loadAccounts() as before; additionally, ensure the later logic that maps
rotating-backup failures to the "dismissed" state only annotates the rotating
source and does not suppress or change the outcome of named-recovery paths (so
codex auth restore-backup reflects named results rather than exiting 0 due to a
dismissed rotating source). Update/extend tests (test/storage.test.ts around the
per-file tests) to add cases for EBUSY/429 and Windows lock scenarios to verify
the new try/catch behavior and that named backups still restore when rotating
listing fails.
In `@lib/storage.ts`:
- Around line 197-223: The normalizeBackupLookupName function currently strips
both .json and .bak extensions which breaks named backups like "demo.bak" —
change the normalization to only remove a trailing .json (case-insensitive) and
keep .bak intact; update any duplicate logic that resolves filenames (the same
backup-lookup/restore resolution code near the file restore path) to match this
behavior so named backups can round-trip and won't collide with rotating
backups; refer to normalizeBackupLookupName and the restore/resolution routine
that uses getNamedBackupRoot/getStoragePath to find where to apply the fix.
- Around line 1817-1845: listRotatingBackups currently treats any candidate path
ending with ".bak.N" as a rotation slot which allows stray files like
`${storagePath}.notes.bak.1`; update the filtering logic in listRotatingBackups
(after calling getAccountsBackupRecoveryCandidatesWithDiscovery and before
parseRotatingBackupSlot/loadBackupCandidate) to only accept candidates whose
filename stem exactly matches the storage stem (`<storagePath>.bak` or
`<storagePath>.bak.<n>`), performing a case-insensitive comparison on Windows
(use OS detection or path normalization) so only true
`${storagePath}.bak`/`${storagePath}.bak.<n>` files are processed; keep the
remaining flow (parseRotatingBackupSlot, loadBackupCandidate,
buildBackupFileMetadata, formatRotatingBackupLabel) unchanged.
In `@test/codex-manager-cli.test.ts`:
- Around line 14-15: The tests wire listRotatingBackupsMock and
assessNamedBackupRestoreMock but never simulate rotating backups, so add
deterministic vitest-based regression cases that mock rotating entries (e.g.,
filenames ending with ".bak" and ".bak.N") and mixed named+rotating sections and
assert the CLI chooses rotating restore entries correctly; update the mocks used
in test/codex-manager-cli.test.ts (the listRotatingBackupsMock and any calls to
assessNamedBackupRestoreMock) to return representative rotating arrays and
ensure the test exercise the rotating-restore code path in lib/storage.ts (the
restore selection flow that handles rotating backups) while keeping the tests
deterministic (use vi mocks/timers and explicit ordering) so the new flow at the
rotating-restore logic is covered.
---
Outside diff comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 2202-2271: The test is wrongly asserting that loadAccounts() was
called twice, which bakes in an extra storage reread; remove or relax the
assertion inside the selectMock (remove expect(loadAccountsCallCount).toBe(2) or
change it to expect(loadAccountsCallCount).toBeGreaterThanOrEqual(1)) so the
test does not require the redundant disk I/O. Update the test in
test/codex-manager-cli.test.ts where loadAccountsMock, selectMock and
getActionableNamedBackupRestoresMock are used (specifically the selectMock
implementation) to avoid locking in the extra read.
🪄 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: ff32201c-f6e0-449d-81e0-2c82cc64b958
📒 Files selected for processing (4)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/codex-manager-cli.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/storage.ts
| const rotatingBackups = await listRotatingBackups(); | ||
| let currentStorage: Awaited<ReturnType<typeof loadAccounts>> = null; | ||
| if (!startupAssessments) { | ||
| try { | ||
| currentStorage = await loadAccounts(); | ||
| } catch (error) { | ||
| log.warn("Failed to load current storage for backup browser", { | ||
| error: normalizeBackupAssessmentError(error), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
don't let rotating-backup i/o failures take down named recovery or return exit 0.
lib/codex-manager.ts:4578-4588 awaits listRotatingBackups() before it builds any named entries. if that throws, lib/codex-manager.ts:4660-4672 turns the error into "dismissed", which means codex auth restore-backup still exits successfully at lib/codex-manager.ts:4823-4826 and a win32-style lock on the rotating side can block recovery from already-restorable named backups. the provided storage coverage at test/storage.test.ts:4028-4242 only covers per-file listing behavior, not this manager-level fallback. As per coding guidelines, lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.
Also applies to: 4660-4672, 4823-4826
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4578 - 4588, Wrap the initial call to
listRotatingBackups() in a try/catch so any rotating-backup I/O error is treated
as non-fatal: on error set rotatingBackups = [] (not null/undefined), log the
error via normalizeBackupAssessmentError, and continue to build named entries
using loadAccounts() as before; additionally, ensure the later logic that maps
rotating-backup failures to the "dismissed" state only annotates the rotating
source and does not suppress or change the outcome of named-recovery paths (so
codex auth restore-backup reflects named results rather than exiting 0 due to a
dismissed rotating source). Update/extend tests (test/storage.test.ts around the
per-file tests) to add cases for EBUSY/429 and Windows lock scenarios to verify
the new try/catch behavior and that named backups still restore when rotating
listing fails.
| assessment, | ||
| })); | ||
| } else { | ||
| const namedBackups = await listNamedBackups(); |
There was a problem hiding this comment.
listNamedBackups() error dismisses the whole browser even when rotating backups exist
listRotatingBackups() is called first (line 4588) and its result is stored. but listNamedBackups() here has no try-catch — if the named-backup directory is inaccessible (e.g. EPERM on the backups folder), the exception escapes loadBackupBrowserEntries, the outer catch in runBackupBrowserManager fires, logs "Could not read backup directory", and returns "dismissed". the rotating backups already fetched at line 4588 are silently discarded.
the old loadBackupRestoreManagerAssessments explicitly caught listNamedBackups() errors and returned [] so the flow could continue. this PR regresses that: a user whose named-backup directory is locked by windows explorer or an AV scan will see a cryptic "Could not read backup directory" message and lose access to their rotating backups entirely.
fix by wrapping listNamedBackups() in its own try-catch (matching how loadAccounts() is handled above at line 4591):
let namedBackups: Awaited<ReturnType<typeof listNamedBackups>> = [];
try {
namedBackups = await listNamedBackups();
} catch (error) {
log.warn("Failed to list named backups for backup browser", {
error: normalizeBackupAssessmentError(error),
});
}Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4608
Comment:
**`listNamedBackups()` error dismisses the whole browser even when rotating backups exist**
`listRotatingBackups()` is called first (line 4588) and its result is stored. but `listNamedBackups()` here has no try-catch — if the named-backup directory is inaccessible (e.g. EPERM on the backups folder), the exception escapes `loadBackupBrowserEntries`, the outer catch in `runBackupBrowserManager` fires, logs "Could not read backup directory", and returns `"dismissed"`. the rotating backups already fetched at line 4588 are silently discarded.
the old `loadBackupRestoreManagerAssessments` explicitly caught `listNamedBackups()` errors and returned `[]` so the flow could continue. this PR regresses that: a user whose named-backup directory is locked by windows explorer or an AV scan will see a cryptic "Could not read backup directory" message and lose access to their rotating backups entirely.
fix by wrapping `listNamedBackups()` in its own try-catch (matching how `loadAccounts()` is handled above at line 4591):
```ts
let namedBackups: Awaited<ReturnType<typeof listNamedBackups>> = [];
try {
namedBackups = await listNamedBackups();
} catch (error) {
log.warn("Failed to list named backups for backup browser", {
error: normalizeBackupAssessmentError(error),
});
}
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
lib/codex-manager.ts (1)
4670-4682:⚠️ Potential issue | 🟠 Majordon't collapse backup-browser load failures into a successful exit.
lib/codex-manager.ts:4670-4682converts real i/o errors into"dismissed", andlib/codex-manager.ts:4878-4881maps"dismissed"to exit 0. ifcodex auth restore-backuphitsepremorebusywhile reading backups, the command still reports success. return"failed"for non-user-cancel errors here, and cover it with a vitest intest/codex-manager-cli.test.ts:1.smallest safe fix
} catch (error) { const message = error instanceof Error ? error.message : String(error); console.error( `Could not read backup directory: ${ collapseWhitespace(message) || "unknown error" }`, ); - return "dismissed"; + return "failed"; }As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 4878-4881
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4670 - 4682, The catch in the loadBackupBrowserEntries call (function loadBackupBrowserEntries) currently converts any error into the "dismissed" result and logs a collapsed message via collapseWhitespace, causing real I/O failures to be treated as successful exits; change the catch to detect user-cancel/intentional aborts (if any) and otherwise return "failed" instead of "dismissed", preserve the detailed error message in the log, and update the code-path that maps "dismissed" to exit 0 (the mapping around the CLI exit handling) so that "failed" produces a non-zero exit; add a vitest in test/codex-manager-cli.test.ts that simulates an EBUSY/EPERM read error from loadBackupBrowserEntries and asserts the CLI returns a failing exit (non-zero) rather than success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4419-4428: The rotating-backup assessment isn't being carried
through the browser model, so canRestoreBackupBrowserEntry (function) and the UI
preview paths rely on entry.backup.valid instead of assessRotatingBackupRestore
results; update the code that builds the BackupBrowserEntry/its assessment in
codex-manager (the code paths around where rotating backups are loaded and where
entries are constructed—search for the rotating-load logic and the functions
that populate assessment fields around lines referenced) to call
assessRotatingBackupRestore (from lib/storage.ts) and store its result in
entry.assessment for rotating entries, then change canRestoreBackupBrowserEntry
and the restore-action gating logic (the preview/preview rendering code
referenced at the other locations) to require assessment.eligibleForRestore and
!assessment.wouldExceedLimit for rotating entries the same as named backups, and
add a Vitest regression test in test/codex-manager-cli.test.ts that creates a
rotating backup which exceeds ACCOUNT_LIMITS.MAX_ACCOUNTS and asserts the
preview denies restore (and that restore is blocked), also ensure any
concurrency/IO error handling (EBUSY/429) in the updated code paths follows
existing queue/backoff patterns per lib/** guidelines.
- Around line 4804-4808: latestAssessment is declared without a type causing
implicit any under strict mode; declare an explicit type for latestAssessment
matching the return type of assessRotatingBackupRestore (e.g., the interface or
type alias returned by assessRotatingBackupRestore) before the try block and use
that type when assigning await assessRotatingBackupRestore(entry.backup.slot, {
currentStorage: await loadAccounts() }); reference the symbol latestAssessment
and the function assessRotatingBackupRestore to find where to apply the
annotation so subsequent property accesses on latestAssessment are type-checked.
In `@lib/storage.ts`:
- Around line 2113-2126: The rotating restore applies
retryTransientFilesystemOperation but named restores call importAccounts without
it; move the EBUSY/ebusy retry into the shared restore path by wrapping the
file-IO logic inside importAccounts (or create a new shared helper used by
importAccounts and restoreRotatingBackup) so all restore flows
(restoreRotatingBackup and restoreNamedBackup in codex-manager.ts ->
importAccounts) get the same retry behavior; add a vitest regression that
simulates an EBUSY during fs.readFile in either test/storage.test.ts or
test/codex-manager-cli.test.ts to assert the retry logic succeeds, and update
any affected tests to account for EBUSY/429 retry handling.
In `@test/codex-manager-cli.test.ts`:
- Around line 1118-1162: Add a deterministic vitest regression that simulates an
EBUSY on the rotating-backup restore path: create a new test (mirroring the
existing rotating-backup account-limit test) that sets up the same
rotatingBackup fixture, queues it with queueRotatingBackupBrowserSelection,
stubs restoreRotatingBackupMock to reject once with makeErrnoError("resource
busy","EBUSY"), spies on console.warn, runs
runCodexMultiAuthCli(["auth","restore-backup"]), asserts exitCode is 1, that
restoreRotatingBackupMock was called with the rotating slot (1) and that
console.warn was called with a message containing "EBUSY"; place the test
adjacent to the other rotating-backup tests in test/codex-manager-cli.test.ts so
it covers the EBUSY Windows regression.
In `@test/storage.test.ts`:
- Around line 4300-4420: The current tests around restoreRotatingBackup only
cover a single transient EBUSY retry; add deterministic vitest cases that cover
(1) account-limit-exceeded filesystem errors (simulate EMFILE/ENFILE on
fs.readFile or fs.open when reading rotating backups and assert
restoreRotatingBackup rejects with a limit-specific error/label), (2) concurrent
restores under tight limits (invoke restoreRotatingBackup concurrently for
multiple slots and assert correct imported/skipped totals and that
retries/backoffs occur), and (3) win32-specific filename behavior (mock
process.platform = "win32" and ensure listRotatingBackups/restoreRotatingBackup
treat files like `${testStoragePath}.notes.bak.1` as stray while accepting
`${testStoragePath}.bak` and `${testStoragePath}.bak.1`); reuse existing helpers
and spies (restoreRotatingBackup, listRotatingBackups, testStoragePath,
rotatingBackupPath, and vi.spyOn(fs, "readFile")) and always restore spies after
each test.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4670-4682: The catch in the loadBackupBrowserEntries call
(function loadBackupBrowserEntries) currently converts any error into the
"dismissed" result and logs a collapsed message via collapseWhitespace, causing
real I/O failures to be treated as successful exits; change the catch to detect
user-cancel/intentional aborts (if any) and otherwise return "failed" instead of
"dismissed", preserve the detailed error message in the log, and update the
code-path that maps "dismissed" to exit 0 (the mapping around the CLI exit
handling) so that "failed" produces a non-zero exit; add a vitest in
test/codex-manager-cli.test.ts that simulates an EBUSY/EPERM read error from
loadBackupBrowserEntries and asserts the CLI returns a failing exit (non-zero)
rather than success.
🪄 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: 1b90f176-584b-43e2-8d01-8806ad9cdaef
📒 Files selected for processing (4)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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/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
🪛 Biome (2.4.7)
lib/codex-manager.ts
[error] 4804-4804: This variable implicitly has the any type.
(lint/suspicious/noImplicitAnyLet)
🔇 Additional comments (9)
test/storage.test.ts (3)
1296-1349: good collision regression for.bak-suffixed named backups.this test cleanly proves
demo,demo.bak, anddemo.bak.jsonresolve to distinct restore targets and avoids sibling-path ambiguity. ref:test/storage.test.ts:1296-1349.
1411-1415: good assertion update for normalized invalid-name errors.the case-insensitive
/invalid backup name/iexpectation makes the restore and assess assertions resilient to message casing changes. ref:test/storage.test.ts:1411-1415.
4084-4298: solid rotating-backup listing coverage.the new cases for invalid json, disappearing files, and non-enoent read failures are strong and deterministic. they protect the browser listing path well. ref:
test/storage.test.ts:4084-4298.test/codex-manager-cli.test.ts (6)
1085-1116: good: rotating backup restore path is now exercised.the new test at
test/codex-manager-cli.test.ts:1085-1116covers the happy path for rotating backup restore via the direct cli command. this addresses the prior gap wherelistRotatingBackupsMockwas wired but never used with actual rotating entries.
407-429: lgtm: rotating backup browser helpers follow the named backup pattern.
buildRotatingBackupBrowserEntryandqueueRotatingBackupBrowserSelectionattest/codex-manager-cli.test.ts:407-429mirror the named backup helpers cleanly. the type constraints (T extends { slot: number; label: string }) are appropriate for the rotating backup shape.
699-767: lgtm: rotating backup mocks properly reset in beforeEach.the mock resets at
test/codex-manager-cli.test.ts:699-704and default return values attest/codex-manager-cli.test.ts:713,735-767follow the same pattern as named backup mocks. test isolation is maintained.
1016-1027: lgtm: test refactored to use browser selection helper.the test at
test/codex-manager-cli.test.ts:989-1028now usesqueueNamedBackupBrowserSelectioninstead of direct mock setup. assertions remain unchanged and the test still verifies the full restore flow.
1103-1115: past review addressed: rotating backup browser selection is now tested.the test correctly exercises the rotating backup restore flow with actual rotating backup data at
test/codex-manager-cli.test.ts:1103. the assertions verifyassessRotatingBackupRestoreMockis called with the correct slot (1) andrestoreRotatingBackupMockis invoked. this addresses the prior gap where rotating backups were mocked but never exercised.
3991-4016: lgtm: assessment failure handling test updated for browser pattern.the test at
test/codex-manager-cli.test.ts:3949-4034correctly verifies that when one backup assessment fails, the backup browser still shows it (with error hint) while keeping healthy backups selectable. the mock at line 3992-4016 properly validates the error entry structure includingassessmentErrorandcolor: "yellow".
| function canRestoreBackupBrowserEntry(entry: BackupBrowserEntry): boolean { | ||
| if (entry.kind === "rotating") { | ||
| return entry.backup.valid; | ||
| } | ||
| if (backups.length === 0) { | ||
| return []; | ||
|
|
||
| return ( | ||
| entry.assessment !== null && | ||
| entry.assessment.eligibleForRestore && | ||
| !entry.assessment.wouldExceedLimit | ||
| ); |
There was a problem hiding this comment.
assess rotating backups before you offer restore.
lib/codex-manager.ts:4581-4658 never loads a rotating assessment, so lib/codex-manager.ts:4419-4428 and lib/codex-manager.ts:4481-4493 fall back to backup.valid, and lib/codex-manager.ts:4518-4533 only shows preview data for named backups. that means assessRotatingBackupRestore() from lib/storage.ts:2028-2056 is only consulted after the user picks restore, which hides max-account overflow and other assessment results from the preview flow. please carry the rotating assessment through the browser model and gate the action off eligibleForRestore the same way the named path does. add a vitest regression in test/codex-manager-cli.test.ts:1 for a rotating backup that exceeds ACCOUNT_LIMITS.MAX_ACCOUNTS.
As per coding guidelines, lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.
Also applies to: 4481-4493, 4518-4533, 4581-4658
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4419 - 4428, The rotating-backup
assessment isn't being carried through the browser model, so
canRestoreBackupBrowserEntry (function) and the UI preview paths rely on
entry.backup.valid instead of assessRotatingBackupRestore results; update the
code that builds the BackupBrowserEntry/its assessment in codex-manager (the
code paths around where rotating backups are loaded and where entries are
constructed—search for the rotating-load logic and the functions that populate
assessment fields around lines referenced) to call assessRotatingBackupRestore
(from lib/storage.ts) and store its result in entry.assessment for rotating
entries, then change canRestoreBackupBrowserEntry and the restore-action gating
logic (the preview/preview rendering code referenced at the other locations) to
require assessment.eligibleForRestore and !assessment.wouldExceedLimit for
rotating entries the same as named backups, and add a Vitest regression test in
test/codex-manager-cli.test.ts that creates a rotating backup which exceeds
ACCOUNT_LIMITS.MAX_ACCOUNTS and asserts the preview denies restore (and that
restore is blocked), also ensure any concurrency/IO error handling (EBUSY/429)
in the updated code paths follows existing queue/backoff patterns per lib/**
guidelines.
| let latestAssessment; | ||
| try { | ||
| latestAssessment = await assessRotatingBackupRestore(entry.backup.slot, { | ||
| currentStorage: await loadAccounts(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the exact code at lines 4804-4808
sed -n '4800,4820p' lib/codex-manager.tsRepository: ndycode/codex-multi-auth
Length of output: 716
🏁 Script executed:
# Find where assessRotatingBackupRestore is imported/defined
rg "assessRotatingBackupRestore" --type ts -B 2 -A 2Repository: ndycode/codex-multi-auth
Length of output: 3041
🏁 Script executed:
# Check for any Biome configuration and related errors
find . -name "biome.json" -o -name ".biomeignore" -o -name "biome.jsonc" | head -5Repository: ndycode/codex-multi-auth
Length of output: 81
🏁 Script executed:
# Search for related vitest tests that cover this rotating restore path
rg "latestAssessment|assessRotatingBackupRestore" test/ --type ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 589
🏁 Script executed:
# Get the full assessRotatingBackupRestore definition from lib/storage.ts
rg "export async function assessRotatingBackupRestore" lib/storage.ts -A 30Repository: ndycode/codex-multi-auth
Length of output: 1036
🏁 Script executed:
# Check biome.jsonc for configuration and any type checking rules
cat biome.jsoncRepository: ndycode/codex-multi-auth
Length of output: 570
🏁 Script executed:
# Look at full latestAssessment usage in this function
sed -n '4800,4850p' lib/codex-manager.tsRepository: ndycode/codex-multi-auth
Length of output: 1549
🏁 Script executed:
# Check if there are any windows filesystem or concurrency concerns in the rotating restore path
sed -n '4790,4870p' lib/codex-manager.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 2808
🏁 Script executed:
# Check for any sensitive data logging in this section
rg "console\.(log|warn|error|debug)" lib/codex-manager.ts --context 3 | grep -A 3 -B 3 "4800\|4810\|4820"Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Find BackupRestoreAssessment type definition
rg "type BackupRestoreAssessment|interface BackupRestoreAssessment" lib/ --type ts -B 2 -A 8Repository: ndycode/codex-multi-auth
Length of output: 503
🏁 Script executed:
# Check if there are windows-specific concerns in loadAccounts or restoreRotatingBackup
rg "loadAccounts|restoreRotatingBackup" lib/storage.ts -A 15 | head -50Repository: ndycode/codex-multi-auth
Length of output: 1646
🏁 Script executed:
# Check test/codex-manager-cli.test.ts for specific test coverage of this path
rg "rotating.*restore|assessRotatingBackup" test/codex-manager-cli.test.ts -B 3 -A 8 | head -60Repository: ndycode/codex-multi-auth
Length of output: 2185
🏁 Script executed:
# Verify if there are any queue/retry mechanisms for EBUSY/429 in this code path
rg "EBUSY|429|queue|retry" lib/codex-manager.ts | grep -i "rotating\|backup\|restore"Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Check if there's a specific Biome error report for this location
rg "4804|latestAssessment" . --type ts --type json | grep -i "error\|warn\|biome"Repository: ndycode/codex-multi-auth
Length of output: 253
🏁 Script executed:
# Get the full BackupRestoreAssessment interface to verify the exact structure
rg "export interface BackupRestoreAssessment" lib/storage.ts -A 15Repository: ndycode/codex-multi-auth
Length of output: 505
🏁 Script executed:
# Check if loadAccounts has any concurrency/windows concerns that might affect the rotating restore path
rg "export.*function.*loadAccounts" lib/storage.ts -A 20 | head -30Repository: ndycode/codex-multi-auth
Length of output: 920
🏁 Script executed:
# Verify the complete usage pattern of latestAssessment properties
rg "latestAssessment\." lib/codex-manager.ts -B 2 -A 2Repository: ndycode/codex-multi-auth
Length of output: 446
🏁 Script executed:
# Check if there are any type-checking issues already flagged in CI or tests
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "latestAssessment\|BackupRestoreAssessment" 2>/dev/nullRepository: ndycode/codex-multi-auth
Length of output: 52
🏁 Script executed:
# Check the actual TypeScript strict mode settings
find . -name "tsconfig.json" | xargs cat | grep -A 20 '"strict"'Repository: ndycode/codex-multi-auth
Length of output: 532
🏁 Script executed:
# Look for any Biome linter errors related to type inference
rg "noImplicitAny|implicit" biome.jsonc lib/Repository: ndycode/codex-multi-auth
Length of output: 200
🏁 Script executed:
# Check if there's a windows-specific filesystem concern in loadAccounts
rg "windows|win32|EBUSY|EAGAIN" lib/storage.ts lib/codex-manager.tsRepository: ndycode/codex-multi-auth
Length of output: 729
🏁 Script executed:
# Verify the exact return type by checking if RotatingBackupMetadata is correct
rg "RotatingBackupMetadata" lib/storage.ts -B 2 -A 5Repository: ndycode/codex-multi-auth
Length of output: 1653
add explicit type to latestAssessment.
with strict mode enabled in tsconfig.json, lib/codex-manager.ts:4804 leaves latestAssessment implicitly any. add the type annotation to preserve type safety before property access on lines 4827-4829:
suggested fix
- let latestAssessment;
+ let latestAssessment: Awaited<
+ ReturnType<typeof assessRotatingBackupRestore>
+ >;🧰 Tools
🪛 Biome (2.4.7)
[error] 4804-4804: This variable implicitly has the any type.
(lint/suspicious/noImplicitAnyLet)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4804 - 4808, latestAssessment is declared
without a type causing implicit any under strict mode; declare an explicit type
for latestAssessment matching the return type of assessRotatingBackupRestore
(e.g., the interface or type alias returned by assessRotatingBackupRestore)
before the try block and use that type when assigning await
assessRotatingBackupRestore(entry.backup.slot, { currentStorage: await
loadAccounts() }); reference the symbol latestAssessment and the function
assessRotatingBackupRestore to find where to apply the annotation so subsequent
property accesses on latestAssessment are type-checked.
| export async function restoreRotatingBackup( | ||
| slot: number, | ||
| ): Promise<{ imported: number; total: number; skipped: number }> { | ||
| if (!Number.isInteger(slot) || slot < 0) { | ||
| throw new StorageError( | ||
| `Invalid rotating backup slot: ${slot}`, | ||
| "EINVALID", | ||
| getAccountsBackupPath(getStoragePath()), | ||
| "Rotating backup restore operations only accept non-negative numeric slots.", | ||
| ); | ||
| } | ||
| return retryTransientFilesystemOperation(() => | ||
| importAccounts(getAccountsBackupPathAtIndex(getStoragePath(), slot)), | ||
| ); |
There was a problem hiding this comment.
move the ebusy retry to the shared restore path.
lib/storage.ts:2113-2126 makes rotating restores retryable, but named restores in the same browser still go through restoreNamedBackup() at lib/codex-manager.ts:4786, then lib/storage.ts:2106-2110 -> lib/storage.ts:3182-3193 without the same protection. on windows, a transient ebusy during fs.readFile will still fail named restores while rotating restores retry. please move the retryTransientFilesystemOperation() wrapper into importAccounts() or a shared restore helper, and add a vitest regression in test/storage.test.ts:1 or test/codex-manager-cli.test.ts:1 that forces ebusy on restore.
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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2113 - 2126, The rotating restore applies
retryTransientFilesystemOperation but named restores call importAccounts without
it; move the EBUSY/ebusy retry into the shared restore path by wrapping the
file-IO logic inside importAccounts (or create a new shared helper used by
importAccounts and restoreRotatingBackup) so all restore flows
(restoreRotatingBackup and restoreNamedBackup in codex-manager.ts ->
importAccounts) get the same retry behavior; add a vitest regression that
simulates an EBUSY during fs.readFile in either test/storage.test.ts or
test/codex-manager-cli.test.ts to assert the retry logic succeeds, and update
any affected tests to account for EBUSY/429 retry handling.
| it("blocks rotating backup restore when re-assessment says it would exceed the account limit", async () => { | ||
| setInteractiveTTY(true); | ||
| loadAccountsMock.mockResolvedValue(null); | ||
| const rotatingBackup = { | ||
| label: "Rotating backup 1 (.bak.1)", | ||
| slot: 1, | ||
| path: "/mock/openai-codex-accounts.json.bak.1", | ||
| createdAt: null, | ||
| updatedAt: Date.now(), | ||
| sizeBytes: 256, | ||
| version: 3, | ||
| accountCount: 11, | ||
| schemaErrors: [], | ||
| valid: true, | ||
| loadError: undefined, | ||
| }; | ||
| listNamedBackupsMock.mockResolvedValue([]); | ||
| listRotatingBackupsMock.mockResolvedValue([rotatingBackup]); | ||
| queueRotatingBackupBrowserSelection(rotatingBackup); | ||
| assessRotatingBackupRestoreMock.mockResolvedValueOnce({ | ||
| backup: rotatingBackup, | ||
| currentAccountCount: 0, | ||
| mergedAccountCount: ACCOUNT_LIMITS.MAX_ACCOUNTS + 1, | ||
| imported: null, | ||
| skipped: null, | ||
| wouldExceedLimit: true, | ||
| eligibleForRestore: false, | ||
| error: `Restore would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts`, | ||
| }); | ||
| const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); | ||
|
|
||
| try { | ||
| const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); | ||
| const exitCode = await runCodexMultiAuthCli(["auth", "restore-backup"]); | ||
|
|
||
| expect(exitCode).toBe(1); | ||
| expect(confirmMock).not.toHaveBeenCalled(); | ||
| expect(restoreRotatingBackupMock).not.toHaveBeenCalled(); | ||
| expect(logSpy).toHaveBeenCalledWith( | ||
| `Restore would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts`, | ||
| ); | ||
| } finally { | ||
| logSpy.mockRestore(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
missing: ebusy regression test for rotating backup restore path.
the test at test/codex-manager-cli.test.ts:1118-1162 covers the account-limit-exceeded error path for rotating backups, which is good. however, there's no corresponding ebusy test for restoreRotatingBackupMock.
the named backup restore path has ebusy coverage at test/codex-manager-cli.test.ts:2612-2631, but rotating backup restore lacks this. on windows, EBUSY can occur when another process holds a lock on the storage file during restore. add a regression test that makes restoreRotatingBackupMock reject with makeErrnoError("resource busy", "EBUSY") and verify the cli handles it gracefully (returns to menu, logs warning).
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.
suggested test skeleton
it("catches rotating backup restore failures from EBUSY and returns to the login menu", async () => {
setInteractiveTTY(true);
loadAccountsMock.mockResolvedValue(null);
const rotatingBackup = {
label: "Rotating backup 1 (.bak.1)",
slot: 1,
path: "/mock/openai-codex-accounts.json.bak.1",
createdAt: null,
updatedAt: Date.now(),
sizeBytes: 256,
version: 3,
accountCount: 1,
schemaErrors: [],
valid: true,
loadError: undefined,
};
listNamedBackupsMock.mockResolvedValue([]);
listRotatingBackupsMock.mockResolvedValue([rotatingBackup]);
queueRotatingBackupBrowserSelection(rotatingBackup);
restoreRotatingBackupMock.mockRejectedValueOnce(
makeErrnoError("resource busy", "EBUSY"),
);
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
try {
const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js");
const exitCode = await runCodexMultiAuthCli(["auth", "restore-backup"]);
expect(exitCode).toBe(1);
expect(restoreRotatingBackupMock).toHaveBeenCalledWith(1);
expect(warnSpy).toHaveBeenCalledWith(
expect.stringContaining("EBUSY"),
);
} finally {
warnSpy.mockRestore();
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` around lines 1118 - 1162, Add a deterministic
vitest regression that simulates an EBUSY on the rotating-backup restore path:
create a new test (mirroring the existing rotating-backup account-limit test)
that sets up the same rotatingBackup fixture, queues it with
queueRotatingBackupBrowserSelection, stubs restoreRotatingBackupMock to reject
once with makeErrnoError("resource busy","EBUSY"), spies on console.warn, runs
runCodexMultiAuthCli(["auth","restore-backup"]), asserts exitCode is 1, that
restoreRotatingBackupMock was called with the rotating slot (1) and that
console.warn was called with a message containing "EBUSY"; place the test
adjacent to the other rotating-backup tests in test/codex-manager-cli.test.ts so
it covers the EBUSY Windows regression.
| it("retries rotating backup restore after a transient EBUSY read failure", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "primary", | ||
| refreshToken: "ref-primary", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
| const rotatingBackupPath = `${testStoragePath}.bak`; | ||
| await fs.writeFile( | ||
| rotatingBackupPath, | ||
| JSON.stringify({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "rotating-restore", | ||
| refreshToken: "ref-rotating-restore", | ||
| addedAt: 2, | ||
| lastUsed: 2, | ||
| }, | ||
| ], | ||
| }), | ||
| "utf-8", | ||
| ); | ||
|
|
||
| const originalReadFile = fs.readFile.bind(fs) as typeof fs.readFile; | ||
| let backupReadAttempts = 0; | ||
| const readFileSpy = vi | ||
| .spyOn(fs, "readFile") | ||
| .mockImplementation((async ( | ||
| ...args: Parameters<typeof fs.readFile> | ||
| ) => { | ||
| const [filePath] = args; | ||
| if (String(filePath) === rotatingBackupPath) { | ||
| backupReadAttempts += 1; | ||
| if (backupReadAttempts === 1) { | ||
| throw Object.assign(new Error("backup busy"), { | ||
| code: "EBUSY", | ||
| }); | ||
| } | ||
| } | ||
| return originalReadFile(...args); | ||
| }) as typeof fs.readFile); | ||
|
|
||
| try { | ||
| await expect(restoreRotatingBackup(0)).resolves.toEqual({ | ||
| imported: 1, | ||
| skipped: 0, | ||
| total: 2, | ||
| }); | ||
| expect(backupReadAttempts).toBe(2); | ||
| } finally { | ||
| readFileSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it("ignores stray files that only resemble rotating backups", async () => { | ||
| await fs.writeFile( | ||
| `${testStoragePath}.bak`, | ||
| JSON.stringify({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "rotating-latest", | ||
| refreshToken: "ref-rotating-latest", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }), | ||
| "utf-8", | ||
| ); | ||
| await fs.writeFile( | ||
| `${testStoragePath}.bak.1`, | ||
| JSON.stringify({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "rotating-history", | ||
| refreshToken: "ref-rotating-history", | ||
| addedAt: 2, | ||
| lastUsed: 2, | ||
| }, | ||
| ], | ||
| }), | ||
| "utf-8", | ||
| ); | ||
| await fs.writeFile( | ||
| `${testStoragePath}.notes.bak.1`, | ||
| JSON.stringify({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "stray-lookalike", | ||
| refreshToken: "ref-stray-lookalike", | ||
| addedAt: 3, | ||
| lastUsed: 3, | ||
| }, | ||
| ], | ||
| }), | ||
| "utf-8", | ||
| ); | ||
|
|
||
| const rotatingBackups = await listRotatingBackups(); | ||
|
|
||
| expect(rotatingBackups).toHaveLength(2); | ||
| expect(rotatingBackups.map((backup) => backup.slot)).toEqual([0, 1]); | ||
| expect(rotatingBackups.map((backup) => backup.path)).toEqual([ | ||
| `${testStoragePath}.bak`, | ||
| `${testStoragePath}.bak.1`, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
missing rotating-restore regressions for limit failures, concurrent restores, and win32 behavior.
line [4300] only validates a single restore retry after one transient ebusy. it does not cover the account-limit-exceeded failure path or concurrent rotating restores under tight limits, and it does not pin win32-specific filesystem behavior. these are the exact paths likely to regress in restore eligibility and error labeling.
proposed test additions
+ it("surfaces account-limit-exceeded when restoring a rotating backup", async () => {
+ await saveAccounts({
+ version: 3,
+ activeIndex: 0,
+ accounts: Array.from({ length: ACCOUNT_LIMITS.MAX_ACCOUNTS }, (_, index) => ({
+ accountId: `current-${index}`,
+ refreshToken: `ref-current-${index}`,
+ addedAt: index + 1,
+ lastUsed: index + 1,
+ })),
+ });
+ await fs.writeFile(
+ `${testStoragePath}.bak`,
+ JSON.stringify({
+ version: 3,
+ activeIndex: 0,
+ accounts: [{ accountId: "overflow", refreshToken: "ref-overflow", addedAt: 999, lastUsed: 999 }],
+ }),
+ "utf-8",
+ );
+ await expect(restoreRotatingBackup(0)).rejects.toThrow(/exceed maximum/i);
+ });
+
+ it("serializes concurrent rotating restores when limit allows only one import", async () => {
+ await saveAccounts({
+ version: 3,
+ activeIndex: 0,
+ accounts: Array.from({ length: ACCOUNT_LIMITS.MAX_ACCOUNTS - 1 }, (_, index) => ({
+ accountId: `current-${index}`,
+ refreshToken: `ref-current-${index}`,
+ addedAt: index + 1,
+ lastUsed: index + 1,
+ })),
+ });
+ await fs.writeFile(`${testStoragePath}.bak`, JSON.stringify({
+ version: 3, activeIndex: 0,
+ accounts: [{ accountId: "rot-a", refreshToken: "ref-rot-a", addedAt: 100, lastUsed: 100 }],
+ }), "utf-8");
+ await fs.writeFile(`${testStoragePath}.bak.1`, JSON.stringify({
+ version: 3, activeIndex: 0,
+ accounts: [{ accountId: "rot-b", refreshToken: "ref-rot-b", addedAt: 101, lastUsed: 101 }],
+ }), "utf-8");
+
+ const results = await Promise.allSettled([restoreRotatingBackup(0), restoreRotatingBackup(1)]);
+ const ok = results.filter((r) => r.status === "fulfilled");
+ const fail = results.filter((r) => r.status === "rejected");
+ expect(ok).toHaveLength(1);
+ expect(fail).toHaveLength(1);
+ expect(String((fail[0] as PromiseRejectedResult).reason)).toContain("exceed maximum");
+ });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." ref: test/storage.test.ts:4300-4420.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 4300 - 4420, The current tests around
restoreRotatingBackup only cover a single transient EBUSY retry; add
deterministic vitest cases that cover (1) account-limit-exceeded filesystem
errors (simulate EMFILE/ENFILE on fs.readFile or fs.open when reading rotating
backups and assert restoreRotatingBackup rejects with a limit-specific
error/label), (2) concurrent restores under tight limits (invoke
restoreRotatingBackup concurrently for multiple slots and assert correct
imported/skipped totals and that retries/backoffs occur), and (3) win32-specific
filename behavior (mock process.platform = "win32" and ensure
listRotatingBackups/restoreRotatingBackup treat files like
`${testStoragePath}.notes.bak.1` as stray while accepting
`${testStoragePath}.bak` and `${testStoragePath}.bak.1`); reuse existing helpers
and spies (restoreRotatingBackup, listRotatingBackups, testStoragePath,
rotatingBackupPath, and vi.spyOn(fs, "readFile")) and always restore spies after
each test.
Summary
#85fresh/11-sync-history-logWhat 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
56dac78Additional 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 replaces the single-step named-backup restore manager with a two-panel backup browser that surfaces both named and rotating backups in a unified inspect→restore flow. the storage layer gets
listRotatingBackups,assessRotatingBackupRestore, andrestoreRotatingBackup(withretryTransientFilesystemOperationcorrectly applied to restore), plus path-traversal hardening innormalizeBackupLookupNameand windows-safe path comparison. the CLI layer gets the newrunBackupBrowserManagerwith theBackupBrowserEntrytype hierarchy and detail-view helpers.key concerns:
loadBackupBrowserEntrieswrapsloadAccounts()in a try-catch but leaveslistNamedBackups()(line 4608) unwrapped. a transient EPERM on the named-backup folder on windows will cause the outer catch inrunBackupBrowserManagerto fire, dismiss the whole browser with "Could not read backup directory", and discard the rotating backups that were already fetched. the old code explicitly handled this error and continued with an empty named list.assessRotatingBackupRestoreunit test instorage.test.ts: the six new rotating backup tests cover listing and restore, but no test exercises the realassessRotatingBackupRestoreimplementation (limit-exceeded, eligibility, bad slot). the CLI tests mock it, so a concurrency bug there goes undetected.settledEntriesname onPromise.allat line 4619 — cosmetic but confusing for reviewers.listNamedBackupsMockcall-count assertions incodex-manager-cli.test.tswere loosened fromtoHaveBeenCalledTimes(1)totoHaveBeenCalled(), reducing signal if the browser loop re-fetches more than intended.Confidence Score: 3/5
listNamedBackups()error path inloadBackupBrowserEntriesis a real regression that silently drops rotating backup access on windows when the named-backup directory is temporarily lockedrestoreRotatingBackupwith retry,normalizeBackupLookupNamepath-traversal guard, windows path comparison) are solid. the logic regression inloadBackupBrowserEntriesis a medium-severity UX/correctness bug — it won't lose data, but on windows with AV activity it degrades the backup browser to a dismissal instead of showing rotating backups. the missingassessRotatingBackupRestoreunit test is a coverage gap given the custom concurrency requirements of this codebase. both issues are quick to fix.loadBackupBrowserEntriesat line 4608 needs a try-catch aroundlistNamedBackups(); test/storage.test.ts — needs a direct unit test forassessRotatingBackupRestoreImportant Files Changed
runBackupRestoreManagerwithrunBackupBrowserManager— a two-step inspect+restore UI that shows both named and rotating backups; adds helper types and formatting functions; contains a logic regression wherelistNamedBackups()errors dismiss the whole browser even when rotating backups are available, and a misleadingsettledEntriesvariable nameRotatingBackupMetadata, genericBackupRestoreAssessment<TBackup>,listRotatingBackups,assessRotatingBackupRestore, andrestoreRotatingBackup(correctly wrapped inretryTransientFilesystemOperation); adds path-traversal guardnormalizeBackupLookupNameand windows-safenormalizeFilesystemPathForComparison;loadBackupCandidateandbuildBackupFileMetadatastill lack retry for the listing/assessment paths (flagged in prior threads)listNamedBackupsMockcall-count assertions are relaxed fromtoHaveBeenCalledTimes(1)totoHaveBeenCalled(), which hides the regression wherelistNamedBackupsis now called an extra time per browser loop iterationassessRotatingBackupRestore(e.g. limit-exceeded, eligibility)Sequence Diagram
sequenceDiagram participant U as User participant BBM as runBackupBrowserManager participant LBE as loadBackupBrowserEntries participant ST as storage.ts participant UI as select / confirm BBM->>LBE: loadBackupBrowserEntries({ startupAssessments? }) LBE->>ST: listRotatingBackups() ST-->>LBE: RotatingBackupMetadata[] LBE->>ST: listNamedBackups() [no try-catch ⚠️] ST-->>LBE: NamedBackupMetadata[] LBE->>ST: assessNamedBackupRestore(name) per chunk ST-->>LBE: NamedBackupBrowserEntry[] LBE-->>BBM: { namedEntries, rotatingEntries } BBM->>UI: select(items) — "Backup Browser" U-->>UI: inspect entry UI-->>BBM: { type: "inspect", entry } BBM->>UI: showBackupBrowserDetails(entry) U-->>UI: "Restore This Backup" UI-->>BBM: "restore" alt entry.kind === "named" BBM->>ST: assessNamedBackupRestore(name, { currentStorage }) ST-->>BBM: NamedBackupAssessment BBM->>UI: confirm("Restore backup?") U-->>UI: true BBM->>ST: restoreNamedBackup(name) ST-->>BBM: { imported, skipped, total } BBM-->>U: "restored" else entry.kind === "rotating" BBM->>ST: assessRotatingBackupRestore(slot, { currentStorage }) ST-->>BBM: BackupRestoreAssessment<RotatingBackupMetadata> BBM->>UI: confirm("Restore backup?") U-->>UI: true BBM->>ST: restoreRotatingBackup(slot) Note over ST: retryTransientFilesystemOperation wraps importAccounts ✅ ST-->>BBM: { imported, skipped, total } BBM-->>U: "restored" endPrompt To Fix All With AI
Last reviewed commit: ee665ec