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 | ✅ 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 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") { |
lib/codex-manager.ts
Outdated
| 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.
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".
test/codex-manager-cli.test.ts
Outdated
| 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.
| const previousStorage: AccountStorageV3 = { | ||
| ...options.storage, | ||
| accounts: options.storage.accounts.map((account) => ({ ...account })), | ||
| activeIndexByFamily: { ...(options.storage.activeIndexByFamily ?? {}) }, | ||
| }; | ||
|
|
||
| nextStorage.accounts.splice(options.index, 1); | ||
| rebaseActiveIndicesAfterDelete(nextStorage, options.index); | ||
| clampActiveIndices(nextStorage); | ||
| await saveAccounts(nextStorage); | ||
|
|
||
| const remainingFlagged = flagged.accounts.filter( | ||
| (account) => account.refreshToken !== target.refreshToken, | ||
| ); | ||
| const removedFlaggedCount = flagged.accounts.length - remainingFlagged.length; | ||
| let updatedFlagged = flagged; | ||
| if (removedFlaggedCount > 0) { | ||
| updatedFlagged = { ...flagged, accounts: remainingFlagged }; | ||
| try { | ||
| await saveFlaggedAccounts(updatedFlagged); | ||
| } catch (error) { | ||
| const originalError = asError( | ||
| error, | ||
| "Failed to save flagged account storage after deleting an account", | ||
| ); | ||
| try { | ||
| await saveAccounts(previousStorage); | ||
| } catch (rollbackError) { | ||
| throw new AggregateError( | ||
| [ | ||
| originalError, | ||
| asError( | ||
| rollbackError, | ||
| "Failed to roll back account storage after flagged save failure", | ||
| ), | ||
| ], | ||
| "Deleting the account partially failed and rollback also failed.", |
There was a problem hiding this comment.
previousStorage is a shallow clone of options.storage as it was when deleteAccountAtIndex was called. if saveAccounts(nextStorage) succeeds but saveFlaggedAccounts then fails, the rollback writes back this pre-call snapshot. any writes made to the accounts file between the caller's last loadAccounts() and the point of rollback are silently overwritten.
the safe fix is to reload from disk inside the rollback path:
} catch (error) {
const originalError = asError(error, "Failed to save flagged account storage after deleting an account");
try {
// reload from disk so rollback doesn't clobber concurrent writes
const freshStorage = await loadAccounts();
const restoredStorage = freshStorage ?? previousStorage;
// re-apply the deletion undo by adding the target account back
// or, if using withAccountAndFlaggedStorageTransaction, the whole
// function becomes atomic and rollback is implicit.
await saveAccounts(restoredStorage);
} catch (rollbackError) { ... }the cleanest long-term fix is to replace the manual lock-and-restore with withAccountAndFlaggedStorageTransaction, which already provides atomic rollback across both the accounts and flagged-accounts files. windows filesystem token safety risk: if the accounts file is temporarily inaccessible during the rollback saveAccounts, AggregateError is thrown and the storage is in an undefined state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/destructive-actions.ts
Line: 119-155
Comment:
**Rollback restores a caller-captured snapshot, not a fresh load**
`previousStorage` is a shallow clone of `options.storage` as it was when `deleteAccountAtIndex` was called. if `saveAccounts(nextStorage)` succeeds but `saveFlaggedAccounts` then fails, the rollback writes back this pre-call snapshot. any writes made to the accounts file between the caller's last `loadAccounts()` and the point of rollback are silently overwritten.
the safe fix is to reload from disk inside the rollback path:
```ts
} catch (error) {
const originalError = asError(error, "Failed to save flagged account storage after deleting an account");
try {
// reload from disk so rollback doesn't clobber concurrent writes
const freshStorage = await loadAccounts();
const restoredStorage = freshStorage ?? previousStorage;
// re-apply the deletion undo by adding the target account back
// or, if using withAccountAndFlaggedStorageTransaction, the whole
// function becomes atomic and rollback is implicit.
await saveAccounts(restoredStorage);
} catch (rollbackError) { ... }
```
the cleanest long-term fix is to replace the manual lock-and-restore with `withAccountAndFlaggedStorageTransaction`, which already provides atomic rollback across both the accounts and flagged-accounts files. windows filesystem token safety risk: if the accounts file is temporarily inaccessible during the rollback `saveAccounts`, `AggregateError` is thrown and the storage is in an undefined state.
How can I resolve this? If you propose a fix, please make it concise.
|
Closing this preserved |
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 lands the backup browser and rotating restore support extracted from PR #85, stacked on the sync-history log branch. it adds an interactive
runBackupBrowserManagerUI, exposeslistNamedBackups/listRotatingBackups/assessNamedBackupRestore/assessRotatingBackupRestoreas first-class storage APIs, wraps all key filesystem paths withretryTransientFilesystemOperation(EBUSY/EAGAIN/EPERM on win32), fixes a pre-existing bug wherewithAccountAndFlaggedStorageTransactionwas missingactive: true(transaction snapshots were never used in that path), and refactors destructive ops (deleteAccountAtIndex,deleteSavedAccounts,resetLocalState) into a newlib/destructive-actions.tsmodule.key findings:
deleteAccountAtIndex— the rollback path inlib/destructive-actions.ts:145restorespreviousStorage, which was cloned from the caller-suppliedoptions.storageat function entry. ifsaveAccounts(nextStorage)succeeds butsaveFlaggedAccountsthen fails, the rollback writes back this potentially stale snapshot, silently overwriting any accounts written between the caller'sloadAccounts()and the rollback. the safe fix is to usewithAccountAndFlaggedStorageTransaction(which provides implicit rollback) or reload from disk inside the catch block.loadBackupBrowserEntries— whenstartupAssessmentsis absent, the function callslistNamedBackups()(full scan) thenassessNamedBackupRestorefor each entry (re-reads and re-stats each file).getActionableNamedBackupRestoresalready has a fast path using pre-loaded candidates fromscanNamedBackups(); the same pattern should be applied here to halve I/O and reduce the window for windows AV lock races between the two scans.ENOTEMPTYinRETRYABLE_REMOVE_CODES— this constant inlib/sync-history.tsis only used in the test reset helper for file removal;ENOTEMPTYis not returned for regular file removal and is dead code.Confidence Score: 3/5
deleteAccountAtIndexis a real correctness issue (stale snapshot on flagged-save failure), and the double filesystem scan inloadBackupBrowserEntriescreates an AV-lock race window on windows; neither is blocking but both should be resolved before a wider rolloutactive: truebug fix inwithAccountAndFlaggedStorageTransactionis a genuine correctness improvement. test coverage is broad but misses the rollback-under-concurrent-write scenario and the double-scan I/O path. score reflects the rollback correctness issue indeleteAccountAtIndexand the windows-relevant double-scan race.lib/destructive-actions.ts(rollback correctness),lib/codex-manager.tsloadBackupBrowserEntries(double-scan I/O),lib/sync-history.tsRETRYABLE_REMOVE_CODES(deadENOTEMPTYentry)Important Files Changed
NamedBackupMetadata,RotatingBackupMetadata,BackupRestoreAssessmenttypes, newscanNamedBackups,listNamedBackups,listRotatingBackups,assessNamedBackupRestore,assessRotatingBackupRestore,restoreRotatingBackup,retryTransientFilesystemOperation,unlinkWithRetry— all key filesystem paths now retry EBUSY/EAGAIN/EPERM;importAccountsread path wrapped with retry;clearAccounts/clearFlaggedAccountsreturnboolean;withAccountAndFlaggedStorageTransactionfixed to includeactive: true(pre-existing bug);storagePathadded to transaction context to guard cross-path savesrunBackupBrowserManager,loadBackupBrowserEntries,showBackupBrowserDetails, rotating and named restore flows, startup recovery prompt withresolveStartupRecoveryAction;handleManageActiondelegates todeleteAccountAtIndex/deleteSavedAccounts/resetLocalStatefrom newdestructive-actions.ts;destructiveActionInFlightmodule-level guard prevents concurrent destructive ops;loadBackupBrowserEntriesperforms double filesystem scan for named backups whenstartupAssessmentsis absentdeleteAccountAtIndex,deleteSavedAccounts,resetLocalState,clampActiveIndices; rollback indeleteAccountAtIndexrestores a pre-call snapshot of the caller-supplied storage rather than reloading from disk, risking overwriting concurrent writes if flagged-account save failsreadHistoryTailreads backward with chunk buffering;trimHistoryFileIfNeededkeeps file at ≤200 entries;RETRYABLE_REMOVE_CODESincludesENOTEMPTYwhich is unreachable for file removal in the only caller (__resetSyncHistoryForTests)loadBackupBrowserEntriesor fordeleteAccountAtIndexrollback under concurrent writedeleteAccountAtIndex,deleteSavedAccounts,resetLocalState,clampActiveIndices; rollback path aftersaveFlaggedAccountsfailure is tested at a basic level but does not verify that the rollback avoids overwriting concurrent writeslistRotatingBackups,assessRotatingBackupRestore,retryTransientFilesystemOperation,clearAccountsboolean return,clearFlaggedAccountsbehaviour change — good breadth; EBUSY on the import path is not currently simulatedSequence Diagram
sequenceDiagram participant User participant runAuthLogin participant runBackupBrowserManager participant loadBackupBrowserEntries participant storage as storage.ts participant fs as Filesystem (retried) User->>runAuthLogin: codex auth login (empty storage) runAuthLogin->>storage: getActionableNamedBackupRestores() storage->>fs: scanNamedBackups() → readdir + N×readFile+stat storage-->>runAuthLogin: recoveryState {assessments[]} alt assessments.length > 0 runAuthLogin->>User: confirm "Open backup browser?" User-->>runAuthLogin: yes runAuthLogin->>runBackupBrowserManager: (displaySettings, startupAssessments) runBackupBrowserManager->>loadBackupBrowserEntries: (startupAssessments) loadBackupBrowserEntries->>storage: listRotatingBackups() storage->>fs: getAccountsBackupRecoveryCandidatesWithDiscovery + N×loadBackupCandidate loadBackupBrowserEntries-->>runBackupBrowserManager: {namedEntries, rotatingEntries} runBackupBrowserManager->>User: select<BackupMenuAction> menu User-->>runBackupBrowserManager: inspect entry runBackupBrowserManager->>User: showBackupBrowserDetails() User-->>runBackupBrowserManager: restore runBackupBrowserManager->>storage: assessNamedBackupRestore / assessRotatingBackupRestore storage->>fs: loadBackupCandidate (retried) runBackupBrowserManager->>User: confirm "Restore backup X?" User-->>runBackupBrowserManager: yes runBackupBrowserManager->>storage: restoreNamedBackup / restoreRotatingBackup storage->>fs: importAccounts → readFile (retried) + saveAccounts (rename retried) runBackupBrowserManager-->>runAuthLogin: "restored" else no actionable assessments runAuthLogin->>User: OAuth flow endPrompt To Fix All With AI
Last reviewed commit: "fix(auth): harden ba..."