feat(auth): add backup restore manager#76
feat(auth): add backup restore manager#76ndycode wants to merge 39 commits intogit-plan/01-reset-safetyfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughthis pr adds named-backup restore support: new storage APIs for create/list/assess/restore, cli/ui wiring for a Changes
Sequence diagramsequenceDiagram
participant user as User
participant auth as Auth Menu
participant mgr as BackupRestoreManager
participant storage as Storage
participant fs as File System
rect rgba(200,200,255,0.5)
user->>auth: select "restore from backup"
auth->>mgr: runBackupRestoreManager()
end
rect rgba(200,255,200,0.5)
mgr->>storage: listNamedBackups()
storage->>fs: read backups dir
fs-->>storage: backup entries
storage-->>mgr: NamedBackupMetadata[]
end
rect rgba(255,230,200,0.5)
mgr->>storage: assessNamedBackupRestore(name) (batched)
storage->>fs: read backup file(s)
fs-->>storage: backup content
storage-->>mgr: BackupRestoreAssessment[]
mgr->>user: display menu with hints
user->>mgr: choose backup
mgr->>storage: reassessNamedBackupRestore(name)
mgr->>user: prompt confirm()
user->>mgr: confirm
mgr->>storage: restoreNamedBackup(name)
storage->>fs: merge/import accounts
fs-->>storage: write results
storage-->>mgr: {imported, skipped, total}
mgr->>user: show result / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes reviewable concerns
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:316">
P2: Use retry-based cleanup instead of bare `fs.rm` in test teardown to avoid Windows EBUSY/EPERM/ENOTEMPTY flakiness.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:475">
P2: Backup name normalization in path resolution breaks restore for listed backup files whose filenames contain spaces or other non-normalized characters.</violation>
</file>
<file name="test/codex-manager-cli.test.ts">
<violation number="1" location="test/codex-manager-cli.test.ts:1609">
P3: `vi.doMock` here is never unmocked. `vi.resetModules()` does not clear the mock registry, so this confirm mock will leak into later tests and can change their behavior. Add a `vi.doUnmock`/`vi.unmock` cleanup (e.g., in `afterEach`) to keep tests isolated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:132">
P2: Replace the bare `fs.rm` in test cleanup with retry/backoff to avoid Windows CI flakes on transient file locks.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:476">
P2: `resolveNamedBackupPath` allows path traversal via unsanitized backup names before directory confinement.</violation>
</file>
<file name="lib/cli.ts">
<violation number="1" location="lib/cli.ts:283">
P2: `restore-backup` is wired only in the TUI path; the fallback login prompt cannot select this new mode.</violation>
</file>
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4171">
P2: Handle restoreNamedBackup errors so a missing/invalid backup or limit breach doesn’t crash the auth menu.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/cli.ts (1)
49-61:⚠️ Potential issue | 🟠 Majorthe new mode is still unreachable in the readline fallback menu.
this only wires
restore-backupthrough the tty path.promptLoginModeFallback()inlib/cli.ts:174-243and its prompt copy inlib/ui/copy.ts:135-137still never advertise or parse the mode, so users in fallback environments cannot reach the restore flow even thoughrunAuthLogin()handles it atlib/codex-manager.ts:3909-3919. please add a vitest for the fallback branch.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 291-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/cli.ts` around lines 49 - 61, The fallback readline menu never exposes or parses the new "restore-backup" LoginMode, so add it to promptLoginModeFallback() (ensure the option label and value include "restore-backup") and update the fallback prompt text in lib/ui/copy.ts to advertise the restore flow (matching how runAuthLogin() in lib/codex-manager.ts handles "restore-backup"); add a vitest that exercises the fallback branch to assert the prompt lists and correctly parses "restore-backup", and update any affected tests to reference the new behavior; while modifying prompts/tests, verify any updated auth rotation or queue logic touched by this change properly retries or surfaces EBUSY/429 errors per existing concurrency and Windows IO guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/storage-paths.md`:
- Line 25: Update docs/reference/storage-paths.md to document the restore entry
point and user workflow for the new backup paths: add a short step-by-step
showing "codex auth login" → select recovery → choose 'restore from backup' and
point readers to the interactive restore implementation in lib/codex-manager.ts
(see the interactive flow around the blocks at ~3909-3919 and the restore logic
at ~4211-4291) so users know how to consume files under
~/.codex/multi-auth/backups/<name>.json; also add or link an upgrade note that
mentions the changed CLI workflow and any new npm scripts introduced so README,
SECURITY, and docs remain consistent with the shipped flags and behavior.
In `@lib/codex-manager.ts`:
- Around line 3909-3920: The recovery menu (restore-backup) is never reachable
because runAuthLogin() only enters the menu loop when
existingStorage.accounts.length > 0; hoist the recovery entry so
runBackupRestoreManager(...) can be invoked even when existingStorage.accounts
is empty: move the restore-backup handling (call to runBackupRestoreManager and
its try/catch) to execute before the non-empty accounts guard in runAuthLogin,
ensuring the menu can present the recovery path on fresh/reset/missing storage;
add a vitest that simulates zero-account storage and asserts the restore flow is
offered and that runBackupRestoreManager is called; update any affected tests
referenced in the auth/login suite and ensure new queue logic (if added) handles
EBUSY and 429 retries per concurrency guidelines.
In `@lib/storage.ts`:
- Around line 1712-1728: loadBackupCandidate currently converts any read error
into a permanent invalid-backup result; change it to detect transient Windows/IO
errors (EPERM, EBUSY, EAGAIN) and retry the read (call to loadAccountsFromPath)
with an exponential/backoff loop (same retry envelope used elsewhere in this
module) before returning an error, and apply the same retry logic to the paired
fs.stat call in this file that checks backup file metadata; add a vitest that
simulates a transient busy/EPERM/EBUSY/EAGAIN failure on the first N attempts
and succeeds thereafter to assert the retry path is exercised and cite/update
the affected tests.
- Around line 1580-1609: The function listNamedBackups currently swallows all
readdir errors except ENOENT by logging and returning an empty array; change the
catch in listNamedBackups so that only ENOENT returns [] while any other error
is re-thrown (preserve the original error object) so callers like
runBackupRestoreManager surface real filesystem errors (EPERM/EBUSY). Update
tests: add a vitest that simulates an unreadable backup directory (make readdir
throw EPERM/EBUSY) and assert that listNamedBackups rejects/throws rather than
returning an empty array. Keep references to getNamedBackupRoot/getStoragePath
for locating the path and ensure loadBackupCandidate/buildNamedBackupMetadata
logic is untouched.
In `@test/codex-manager-cli.test.ts`:
- Around line 2365-2428: Add a regression test that covers the confirm=false
path so restore is blocked: duplicate the existing "restores a named backup..."
test setup (use setInteractiveTTY, mock loadAccountsMock, listNamedBackupsMock,
assessNamedBackupRestoreMock, promptLoginModeMock and selectMock) but make
confirmMock resolve to false and then call
runCodexMultiAuthCli(["auth","login"]); assert exitCode is non-zero (or
indicates cancellation) and that restoreNamedBackupMock was not called; keep
other expectations similar (list/assess called once, select used) to ensure the
safety gate implemented by confirmMock is exercised.
In `@test/storage.test.ts`:
- Around line 1111-1185: The tests cover happy paths but lack regression cases
that ensure restoreNamedBackup and assessNamedBackupRestore properly surface
failures for invalid or missing backup payloads; add deterministic vitest tests
in test/storage.test.ts that (1) create a named backup file with invalid JSON
and assert assessNamedBackupRestore(...) and restoreNamedBackup(...) both
reject/throw, (2) create a backup JSON missing required fields (e.g., no
"accounts") and assert both functions reject with a clear error, and (3) assert
behavior is consistent when a backup file is empty or truncated; use the same
helper functions (createNamedBackup, listNamedBackups, assessNamedBackupRestore,
restoreNamedBackup, clearAccounts, loadAccounts) and filesystem writes to inject
the malformed payloads so the tests reproduce the error-path regressions
deterministically under vitest.
---
Outside diff comments:
In `@lib/cli.ts`:
- Around line 49-61: The fallback readline menu never exposes or parses the new
"restore-backup" LoginMode, so add it to promptLoginModeFallback() (ensure the
option label and value include "restore-backup") and update the fallback prompt
text in lib/ui/copy.ts to advertise the restore flow (matching how
runAuthLogin() in lib/codex-manager.ts handles "restore-backup"); add a vitest
that exercises the fallback branch to assert the prompt lists and correctly
parses "restore-backup", and update any affected tests to reference the new
behavior; while modifying prompts/tests, verify any updated auth rotation or
queue logic touched by this change properly retries or surfaces EBUSY/429 errors
per existing concurrency and Windows IO guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8fa6ad5-4c6c-4110-af7b-d6c8d1c2f62c
📒 Files selected for processing (8)
docs/reference/storage-paths.mdlib/cli.tslib/codex-manager.tslib/storage.tslib/ui/auth-menu.tslib/ui/copy.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/ui/auth-menu.tslib/codex-manager.tslib/ui/copy.tslib/storage.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/storage-paths.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (3)
test/storage.test.ts (1)
333-336: good windows-safe teardown change.line 335 in
test/storage.test.ts:335usesremoveWithRetry, which reduces eperm/ebusy teardown flakes on windows.test/codex-manager-cli.test.ts (2)
506-539: good deterministic mock reset/default setup.line 506 through line 539 in
test/codex-manager-cli.test.tscorrectly resets and reseeds new restore-related mocks, which prevents cross-test state bleed.
2430-2500: good restore-failure recovery coverage.line 2430 through line 2500 in
test/codex-manager-cli.test.tsverifies restore failure is surfaced and the login flow continues, which is the right user-safety behavior.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1208">
P3: Duplicate test case: this restore-after-deletion scenario is already covered earlier in the same file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@cubic-dev-ai review this PR |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:1351">
P2: Use `removeWithRetry` instead of bare `fs.rm` to avoid transient Windows file-lock failures in tests.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:1807">
P2: Do not swallow non-ENOENT errors when scanning named backups; rethrow them so restore reports the real filesystem failure instead of a misleading “file not found.”</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai review this PR |
|
@coderabbitai full review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:2066">
P2: `process.platform` is mocked outside the guarded `try/finally`, so setup failures can leak the win32 mock into later tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
2867-2873:⚠️ Potential issue | 🟠 Majorthe final restore read still skips the transient-fs retry envelope.
listing and assessment now retry transient
EPERM/EBUSY/EAGAINfailures in lib/storage.ts:1606-1704 and lib/storage.ts:1869-1890, but the actual import here still does a one-shotfs.readFile(). that means a backup can pass the menu and then fail during the final restore under the same windows lock conditions. wrap this read inretryTransientFilesystemOperation()and add a vitest that forces one busy read during import.patch sketch
- const content = await fs.readFile(resolvedPath, "utf-8"); + const content = await retryTransientFilesystemOperation(() => + fs.readFile(resolvedPath, "utf-8"), + );As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
♻️ Duplicate comments (1)
lib/storage.ts (1)
2819-2827:⚠️ Potential issue | 🟠 Majornormalize both storage paths before comparing them.
this guard still does a raw string comparison between
transactionState.storagePathandgetStoragePath(). on windows, the same location can come through with different casing or normalization, soexportNamedBackupFile()in lib/named-backup-export.ts:223-250 can now fail with a false "different storage path" error from the same logical root.patch sketch
const transactionState = transactionSnapshotContext.getStore(); - const currentStoragePath = getStoragePath(); + const normalizeStoragePath = (value: string): string => { + const resolved = resolvePath(value); + return process.platform === "win32" ? resolved.toLowerCase() : resolved; + }; + const currentStoragePath = normalizeStoragePath(getStoragePath()); const storage = transactionState?.active - ? transactionState.storagePath === currentStoragePath + ? normalizeStoragePath(transactionState.storagePath) === currentStoragePath ? transactionState.snapshot : (() => {As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 2819 - 2827, The comparison between transactionState.storagePath and getStoragePath() in the exportAccounts path does a raw string compare and can fail on Windows due to differing casing/normalization; update the guard in the exportAccounts logic to normalize both sides before comparing (e.g., use a shared normalize/resolve approach such as path.resolve + path.normalize or fs.realpathSync where appropriate) so transactionState.storagePath and getStoragePath() are compared canonicalized, and ensure exportNamedBackupFile() still receives the original intended path after the check; modify the code around transactionState.storagePath, getStoragePath(), and the exportAccounts guard to use that normalizer function.
🤖 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/cli.ts`:
- Around line 237-244: Add test assertions to cover the remaining fallback
aliases so the CLI mapping is fully exercised: in the existing CLI unit tests
that already check "u" and "restore" (near the current test block around the
other alias assertions), add cases asserting that passing "backup" and
"restore-backup" also produce { mode: "restore-backup" }. Target the same test
helper that parses CLI input (the test invoking the code path that reaches the
normalized checks in lib/cli.ts) so both "backup" and "restore-backup" are
explicitly asserted to return mode "restore-backup".
In `@lib/codex-manager.ts`:
- Around line 4353-4375: The restore path that calls
importAccounts(latestAssessment.backup.path) updates multi-auth storage but does
not push the restored active account into the codex CLI state; after detecting a
changed restore (i.e., result.changed is true and after you log the success
message), call autoSyncActiveAccountToCodex() to sync the active account back
into the codex CLI state (referencing importAccounts and
autoSyncActiveAccountToCodex in lib/codex-manager.ts); also add/extend the
restore vitest to cover the changed-restore case so the zero-account recovery
path no longer leaves downstream auth empty, and ensure any added logs do not
leak tokens or emails and that any new queue logic respects EBUSY/429 retry
behavior per project guidelines.
In `@lib/ui/copy.ts`:
- Around line 19-37: The UI copy for restoreBackupConfirm and
restoreBackupRefreshSuccess misleadingly shows exact numeric "refreshed" counts
while the restore flow does not compute updated-row counts—lib/storage.ts
currently computes `skipped` as every non-imported row and lib/codex-manager.ts
feeds that into the "refreshed" message; fix by changing the restore flow to
return an explicit updated/refreshed count from the storage layer (e.g., add an
`updated` or `refreshed` field to the result returned by the storage method used
in lib/storage.ts and use that in lib/codex-manager.ts) and then wire that
accurate count into restoreBackupRefreshSuccess and the zero-import branch used
by restoreBackupConfirm, or alternatively change those two copy strings to
non-numeric wording (e.g., "refreshed existing accounts" without a number) if
you cannot compute an exact count; also add a vitest regression that covers a
mixed case (some duplicates updated, some unchanged) to ensure the UI shows the
correct semantics moving forward.
In `@test/cli.test.ts`:
- Around line 707-719: The test should assert that the other two accepted
aliases also map to mode "restore-backup": update the "returns restore-backup
for fallback restore aliases" case that imports promptLoginMode to also call
mockRl.question.mockResolvedValueOnce("backup") and expect(promptLoginMode([{
index: 0 }])).resolves.toEqual({ mode: "restore-backup" }), and likewise
mockRl.question.mockResolvedValueOnce("restore-backup") with the same
expectation; keep using the existing mockRl.question flow and promptLoginMode
function to lock the full alias mapping.
In `@test/storage.test.ts`:
- Around line 454-466: The test's broad assertion
expect(writeFileSpy).not.toHaveBeenCalled() can fail due to unrelated fs writes;
instead scope assertions to the active storage target by checking
writeFileSpy.mock.calls (or using
expect(writeFileSpy).not.toHaveBeenCalledWith(...)) for the specific path(s)
used by importAccounts/exportPath (and the other case around lines 539–551).
Replace the global not-to-have-been-called with an assertion that no call's
first argument equals or contains exportPath (or the exact storage filename(s)
used by the storage layer), e.g. iterate writeFileSpy.mock.calls and assert none
reference the active storage path(s) so only writes to the target files cause
failures.
- Around line 2048-2081: The test currently only asserts that
restoreNamedBackup("Manual Backup") throws after the backup file is removed;
also assert that the active storage state remained unchanged to guarantee
atomicity—after the expect(...rejects.toThrow(...)) add assertions that check
the same state you had before restore was attempted (e.g., call whatever reads
active accounts/storage used elsewhere in this file such as
clearAccounts/readActiveAccounts/getAccounts or inspect testStoragePath
contents) to ensure no accounts were added/modified; use the existing helpers in
this test file (clearAccounts, assessNamedBackupRestore, restoreNamedBackup,
removeWithRetry) to capture pre-restore state, then re-read and assert equality
after the failed restore so the manual-backup deletion path is deterministic and
verifies restore atomicity.
---
Duplicate comments:
In `@lib/storage.ts`:
- Around line 2819-2827: The comparison between transactionState.storagePath and
getStoragePath() in the exportAccounts path does a raw string compare and can
fail on Windows due to differing casing/normalization; update the guard in the
exportAccounts logic to normalize both sides before comparing (e.g., use a
shared normalize/resolve approach such as path.resolve + path.normalize or
fs.realpathSync where appropriate) so transactionState.storagePath and
getStoragePath() are compared canonicalized, and ensure exportNamedBackupFile()
still receives the original intended path after the check; modify the code
around transactionState.storagePath, getStoragePath(), and the exportAccounts
guard to use that normalizer function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ecccd83-d873-4ff5-9403-ab6206faf358
📒 Files selected for processing (9)
docs/reference/storage-paths.mdlib/cli.tslib/codex-manager.tslib/storage.tslib/ui/auth-menu.tslib/ui/copy.tstest/cli.test.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/ui/copy.tslib/ui/auth-menu.tslib/codex-manager.tslib/storage.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/storage-paths.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/cli.test.tstest/storage.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (19)
lib/cli.ts (2)
49-61: lgtm, new login mode variant added correctly.the
restore-backupmode variant integrates cleanly with the existing union. downstream handling inlib/codex-manager.tswires this torunBackupRestoreManager.
299-300: tty path wiring looks correct.the
case "restore-backup"delegates cleanly; corresponding ui action type is introduced inlib/ui/auth-menu.ts. no token or email logging here.test/codex-manager-cli.test.ts (15)
9-12: mock wiring for named backup apis looks correct.
test/codex-manager-cli.test.ts:9-12declares mocks forlistNamedBackups,assessNamedBackupRestore,getNamedBackupsDirectoryPath, andimportAccounts. these are properly wired into the storage mock at lines 110-113. the setup aligns with the public api surface inlib/storage.ts.Also applies to: 110-113
197-201: confirm mock setup is clean.
test/codex-manager-cli.test.ts:197-201introducesconfirmMockfor the ui confirmation dialog. this enables testing the confirm-decline path at line 2523 and the confirm-proceed path throughout the restore tests.
506-540: beforeeach reset for backup mocks covers defaults well.
test/codex-manager-cli.test.ts:506-540resets all new mocks and provides sensible defaults. the defaultassessNamedBackupRestoreMockreturns a valid, eligible assessment witheligibleForRestore: true. this prevents test pollution across cases.
2366-2431: restore happy path test is solid.
test/codex-manager-cli.test.ts:2366-2431validates the full restore flow: list → assess → select → confirm → import. assertions verifylistNamedBackupsMock,assessNamedBackupRestoreMock,confirmMock, andimportAccountsMockinteractions. the backup path is correctly passed through.
2480-2545: confirm-decline regression is now complete.
test/codex-manager-cli.test.ts:2480-2545addresses the past review comment. it setsconfirmMock.mockResolvedValueOnce(false)at line 2523 and asserts both thatimportAccountsMockis not called and thatpromptLoginModeMock.toHaveBeenCalledTimes(2)at line 2543, proving the flow returns to the menu.
2547-2620: restore failure regression proves menu loop resumes.
test/codex-manager-cli.test.ts:2547-2620addresses the past review comment by assertingpromptLoginModeMock.toHaveBeenCalledTimes(2)at line 2605. this locks in the "stay in menu flow" contract whenimportAccountsMockrejects.
2696-2726: eperm regression for backup listing is present.
test/codex-manager-cli.test.ts:2696-2726covers theEPERMerror code fromlistNamedBackupsMockand verifies the cli returns to the login menu. this addresses the past review comment about manager-levelEPERMregression.
2796-2871: concurrency bounding test is well-designed.
test/codex-manager-cli.test.ts:2796-2871importsNAMED_BACKUP_ASSESS_CONCURRENCYfrom the actual storage module, creates more backups than the limit, and tracksmaxInFlightusing deferred promises. the assertion at line 2868 confirms bounded concurrency is enforced. deterministic and covers the concurrency concern.
3138-3224: ebusy reassessment rejection regression is present.
test/codex-manager-cli.test.ts:3138-3224addresses the past review comment. the secondassessNamedBackupRestoreMockcall rejects withEBUSYat line 3182, and the test assertsconfirmMockandimportAccountsMockare not called, with the error logged.
3226-3264: epoch timestamp hint test avoids locale brittleness.
test/codex-manager-cli.test.ts:3226-3264addresses the past review comment about locale-specific date strings. line 3263 now assertsnot.toContain("updated ")forupdatedAt: 0, avoiding the hardcoded locale-dependent date.
3266-3358: recent timestamp formatting test uses date.now spy.
test/codex-manager-cli.test.ts:3266-3358spies onDate.nowat line 3270 to control the "now" reference, then asserts relative hints like"updated today","updated yesterday","updated 3d ago". the spy is restored in the finally block. deterministic.
3537-3657: quick check waits for in-flight quota refresh.
test/codex-manager-cli.test.ts:3537-3657uses deferred promises to control fetch ordering and verifies the second fetch occurs after the first completes. the test ensures quota cache is populated for both accounts before the check proceeds. this covers a potential race condition.
2728-2794: partial assessment failure test is comprehensive.
test/codex-manager-cli.test.ts:2728-2794simulates one backup throwing during assessment while another succeeds. theselectMockimplementation at line 2770 verifies only healthy backups appear in the menu. the warning is asserted at line 2786.
2873-2959: reassessment freshness test validates toctou guard.
test/codex-manager-cli.test.ts:2873-2959ensuresassessNamedBackupRestoreMockis called twice (initial + pre-confirm) and that the confirmation prompt uses the refreshed assessment data. this locks in the toctou re-assessment behavior.
3041-3136: reassessment ineligibility test proves early exit.
test/codex-manager-cli.test.ts:3041-3136covers the case where reassessment returnseligibleForRestore: false. the test confirmsconfirmMockandimportAccountsMockare not called, and the appropriate message is logged.test/storage.test.ts (2)
373-399: good regression for cross-path transaction guard
test/storage.test.ts:373is a strong guardrail for the storage-path transaction boundary and catches a high-risk misuse path early.
1857-2365: solid windows retry and bounded-concurrency coveragethe regressions in
test/storage.test.ts:1857throughtest/storage.test.ts:2365do a good job pinning transient fs retry behavior andNAMED_BACKUP_LIST_CONCURRENCYthrottling.
|
@coderabbitai full review |
|
✅ Actions performedFull review triggered. |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:2017">
P1: Restore path containment is lexical-only and can be bypassed through symlinked backup roots.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@greptileai |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
@coderabbitai full review |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:2255">
P3: Avoid hardcoding `chunk-boundary-08`; derive the expected backup name from `NAMED_BACKUP_LIST_CONCURRENCY` so the test remains valid if concurrency changes.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:2029">
P2: Reject symlinked backup roots in restore path validation. The current containment check follows `backupRoot` symlinks, so a symlinked backups directory can redirect restores outside the intended backups boundary.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Adds a named-backup restore manager to the codex auth login flow, with stronger backup-path containment and transient filesystem retry behavior, plus expanded regression coverage.
Changes:
- Introduces a “Restore From Backup” recovery path in the auth menu + CLI fallback aliases.
- Adds named-backup listing/assessment/restore helpers in
lib/storage.ts, including containment validation and transient filesystem retries. - Expands vitest coverage for restore safety, cache reuse, Windows FS flakiness (cleanup), and login recovery scenarios.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/storage.test.ts | Adds coverage for import/restore behaviors, named backups, containment checks, and retry scenarios; switches cleanup to removeWithRetry. |
| test/codex-manager-cli.test.ts | Adds extensive CLI orchestration coverage for the restore manager and login recovery paths. |
| test/cli.test.ts | Verifies fallback aliases map to restore-backup. |
| lib/ui/copy.ts | Adds menu copy and prompts for the restore flow. |
| lib/ui/auth-menu.ts | Adds “Recovery” section + restore action in the interactive menu. |
| lib/storage.ts | Implements named backup listing/assessment/restore, containment checks, retry helper, and import result “changed” tracking. |
| lib/codex-manager.ts | Wires restore manager into auth login, syncs Codex CLI state after restore, and flattens login loop to support empty-storage recovery. |
| lib/cli.ts | Extends login mode union and fallback parsing for restore aliases. |
| docs/reference/storage-paths.md | Documents named backup paths and restore workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/storage.ts
Outdated
| if (code === "EBUSY" || code === "ENOTEMPTY" || code === "EAGAIN") { | ||
| return true; | ||
| } | ||
| return code === "EPERM" && process.platform === "win32"; |
test/storage.test.ts
Outdated
| it("retries transient backup directory errors while listing backups", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "retry-list-dir", | ||
| refreshToken: "ref-retry-list-dir", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
| await createNamedBackup("retry-list-dir"); | ||
| const backupRoot = join(dirname(testStoragePath), "backups"); | ||
| const originalReaddir = fs.readdir.bind(fs); | ||
| let busyFailures = 0; | ||
| const readdirSpy = vi | ||
| .spyOn(fs, "readdir") | ||
| .mockImplementation(async (...args) => { | ||
| const [path] = args; | ||
| if (String(path) === backupRoot && busyFailures === 0) { | ||
| busyFailures += 1; | ||
| const error = new Error( | ||
| "backup directory busy", | ||
| ) as NodeJS.ErrnoException; | ||
| error.code = "EBUSY"; | ||
| throw error; | ||
| } | ||
| return originalReaddir(...(args as Parameters<typeof fs.readdir>)); | ||
| }); |
| const mergedAccounts = deduplicateAccounts([ | ||
| ...currentAccounts, | ||
| ...candidate.normalized.accounts, | ||
| ]); | ||
| const wouldExceedLimit = mergedAccounts.length > ACCOUNT_LIMITS.MAX_ACCOUNTS; | ||
| const imported = wouldExceedLimit | ||
| ? null | ||
| : mergedAccounts.length - currentAccounts.length; | ||
| const skipped = wouldExceedLimit | ||
| ? null | ||
| : Math.max(0, candidate.normalized.accounts.length - (imported ?? 0)); | ||
| const changed = !haveEquivalentAccountRows(mergedAccounts, currentAccounts); | ||
| const nothingToImport = !wouldExceedLimit && !changed; |
lib/storage.ts
Outdated
| const merged = [...existingAccounts, ...normalized.accounts]; | ||
|
|
||
| if (merged.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) { | ||
| const deduped = deduplicateAccounts(merged); | ||
| if (deduped.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) { | ||
| throw new Error( | ||
| `Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduped.length})`, | ||
| ); | ||
| } | ||
| const deduplicatedAccounts = deduplicateAccounts(merged); | ||
| if (deduplicatedAccounts.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) { | ||
| throw new Error( | ||
| `Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduplicatedAccounts.length})`, | ||
| ); | ||
| } | ||
| const imported = deduplicatedAccounts.length - existingAccounts.length; | ||
| const skipped = Math.max(0, normalized.accounts.length - imported); | ||
| const changed = !haveEquivalentAccountRows( | ||
| deduplicatedAccounts, | ||
| existingAccounts, | ||
| ); |
Summary
Why
These follow-up fixes close the remaining review gaps around backup-path safety, Windows-heavy retry semantics, and restore accounting accuracy.
Validation
npx eslint lib/storage.ts lib/codex-manager.ts test/storage.test.tsnpx vitest run test/storage.test.ts --maxWorkers=1npx vitest run test/codex-manager-cli.test.ts --maxWorkers=1Risk
Rollback
16cc8d3ongit-plan/02-backup-restore-managernote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr wires a named-backup restore flow into
codex auth login, hardening the path containment layer (assertNamedBackupRestorePath+resolvePathForNamedBackupContainment), adding windows-aware retry logic (retryTransientFilesystemOperationforEAGAIN/EBUSY/EPERM), and keeping CLI auth state in sync after a successful restore viaautoSyncActiveAccountToCodex. the newrunBackupRestoreManagerfunction inlib/codex-manager.tscorrectly drains any in-flight quota refresh before entering the restore menu, re-assesses the selected backup with a fresh storage snapshot before confirmation, and propagates containment errors as hard failures while gracefully skipping other assessment failures.key areas to check:
retryTransientFilesystemOperationcoversreaddir,readFile, andstatin the listing/assessment path;importAccountsadds a retry onreadFile; the test suite verifies retry exhaustion (EAGAIN× 7 attempts) and partial-chunk retry onEBUSY.ENOTEMPTYis only needed for test-cleanup (removeWithRetry) and is correctly absent from the production retry list.pathand stringified error only — no account data or token fields are included.collapseWhitespaceon error messages further reduces leakage surface in user-facing output.test/storage.test.tscovers symlinked backup roots, symlinked subdirectories, injectedreaddirentries with path traversal,EPERMfromrealpathSync, and the happy path for leading-dot names that stay inside the boundary.resolvePathForNamedBackupContainmentfalls back to the rawresolvePathresult for non-existent files, meaning symlinks in intermediate path components are not resolved. not exploitable today (backup name separators are blocked upstream), but a risk surface ifassertNamedBackupRestorePathis called with compound paths in new code.skippedcount semantics:max(0, normalized.accounts.length - imported)overstates skipped when a backup file has internal duplicates;imported + skipped > totalbecomes possible, which is confusing in the success message.test/codex-manager-cli.test.tsdoesn't assert thatautoSyncActiveAccountToCodexfires for theimported=0, changed=truepath.Confidence Score: 3/5
resolvePathForNamedBackupContainmentfallback is a latent gap that isn't exploitable today but is an exported function, making it a regression risk. theskippedovercount is a cosmetic bug that will show confusing output to users. the missing sync assertion in the metadata-refresh test path leaves a behavioural guarantee unchecked.Important Files Changed
resolvePathForNamedBackupContainmentfalls back to unresolved path for non-existent files, creating a latent containment gap;skippedcount in importAccounts can exceed total when the backup itself has internal duplicatesSequence Diagram
sequenceDiagram participant U as User participant M as runAuthLogin loop participant R as runBackupRestoreManager participant S as storage.ts participant C as Codex CLI U->>M: selects "Restore From Backup" M->>M: drain pendingMenuQuotaRefresh M->>R: runBackupRestoreManager(displaySettings) R->>S: listNamedBackups({ candidateCache }) S-->>R: NamedBackupMetadata[] R->>S: assessNamedBackupRestore(name, { currentStorage, candidateCache }) Note over S: cache evicted after first use S-->>R: BackupRestoreAssessment[] R->>U: select(items) — show backup menu U->>R: selects backup entry R->>S: assessNamedBackupRestore(name, { currentStorage: fresh }) — re-assess S-->>R: latestAssessment R->>U: confirm(message) U->>R: confirmed R->>S: assertNamedBackupRestorePath(path, backupDir) S-->>R: validatedBackupPath R->>S: importAccounts(validatedBackupPath) Note over S: retryTransientFilesystemOperation on EAGAIN/EBUSY/EPERM(win32) S-->>R: { imported, skipped, total, changed } alt changed === false R->>U: "All accounts already exist" else changed === true R->>C: autoSyncActiveAccountToCodex() C-->>R: synced R->>U: success message end R-->>M: return M->>U: back to login loopComments Outside Diff (1)
lib/storage.ts, line 1826-1833 (link)symlink traversal gap for non-existent paths in containment fallback
resolvePathForNamedBackupContainmentfalls back to the rawresolvePathresult when the target doesn't exist yet:if a symlinked subdirectory exists inside
backupRoot(e.g.backups/linked/ -> /tmp/) but the target file hasn't been created yet, a call likeassertNamedBackupRestorePath("backups/linked/steal.json", backupRoot)would pass therelative()containment check — the path looks like"linked/steal.json"relative tobackupRooteven though its real destination is/tmp/steal.json.this isn't reachable today because
buildNamedBackupPathrejects names with separators andfindExistingNamedBackupPathonly matches direct directory entries. butassertNamedBackupRestorePathis exported, so a future caller passing a compound path for a not-yet-existent file could bypass containment.safer fallback: use
realpathSync.nativeon the longest existing prefix of the path, or resolve each path component individually.Prompt To Fix All With AI
Last reviewed commit: 16cc8d3
Context used: