feat(auth): add backup restore manager#76
feat(auth): add backup restore manager#76ndycode wants to merge 37 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 | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 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.
| const resolvedBackupRoot = resolvePathForNamedBackupContainment(backupRoot); | ||
| const containedPath = resolvePathForNamedBackupContainment(resolvedPath); | ||
| const relativePath = relative(resolvedBackupRoot, containedPath); |
There was a problem hiding this comment.
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.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/storage.ts, line 2029:
<comment>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.</comment>
<file context>
@@ -1574,6 +1922,165 @@ async function loadAccountsFromPath(path: string): Promise<{
+ backupRoot: string,
+): string {
+ const resolvedPath = resolvePath(path);
+ const resolvedBackupRoot = resolvePathForNamedBackupContainment(backupRoot);
+ const containedPath = resolvePathForNamedBackupContainment(resolvedPath);
+ const relativePath = relative(resolvedBackupRoot, containedPath);
</file context>
| const resolvedBackupRoot = resolvePathForNamedBackupContainment(backupRoot); | |
| const containedPath = resolvePathForNamedBackupContainment(resolvedPath); | |
| const relativePath = relative(resolvedBackupRoot, containedPath); | |
| const resolvedBackupRoot = resolvePath(backupRoot); | |
| const canonicalBackupRoot = resolvePathForNamedBackupContainment( | |
| resolvedBackupRoot, | |
| ); | |
| if ( | |
| existsSync(resolvedBackupRoot) && | |
| normalizeStoragePathForComparison(canonicalBackupRoot) !== | |
| normalizeStoragePathForComparison(resolvedBackupRoot) | |
| ) { | |
| throw new Error("Backup path escapes backup directory"); | |
| } | |
| const containedPath = resolvePathForNamedBackupContainment(resolvedPath); | |
| const relativePath = relative(canonicalBackupRoot, containedPath); |
| expect(listedBackups).toEqual( | ||
| expect.arrayContaining([ | ||
| expect.objectContaining({ | ||
| name: "chunk-boundary-08", |
There was a problem hiding this comment.
P3: Avoid hardcoding chunk-boundary-08; derive the expected backup name from NAMED_BACKUP_LIST_CONCURRENCY so the test remains valid if concurrency changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/storage.test.ts, line 2255:
<comment>Avoid hardcoding `chunk-boundary-08`; derive the expected backup name from `NAMED_BACKUP_LIST_CONCURRENCY` so the test remains valid if concurrency changes.</comment>
<file context>
@@ -1073,6 +1403,1346 @@ describe("storage", () => {
+ expect(listedBackups).toEqual(
+ expect.arrayContaining([
+ expect.objectContaining({
+ name: "chunk-boundary-08",
+ valid: true,
+ }),
</file context>
| name: "chunk-boundary-08", | |
| name: `chunk-boundary-${String(NAMED_BACKUP_LIST_CONCURRENCY).padStart(2, "0")}`, |
1 participant
Summary
codex auth login, including the recovery path when no accounts are savedWhy
The restore-manager flow needs to stay inside the backups boundary, survive transient filesystem contention on Windows, and keep Codex CLI auth state aligned after successful restores.
Docs / Governance
docs/reference/storage-paths.mdfor the named-backup restore path and workflowValidation
npx vitest run test/storage.test.ts test/codex-manager-cli.test.ts test/cli.test.ts --maxWorkers=1npx eslint lib/storage.ts lib/codex-manager.ts test/storage.test.ts test/codex-manager-cli.test.tsRisk
npm run typecheckstill has pre-existing unrelated failures outside this PR scopeRollback
#76commits fromgit-plan/02-backup-restore-manager94c18b8d297663be987dfd1fb6079d88751b0398to drop the latest restore containment and retry hardeningnote: 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 manager into
codex auth login, adds path containment hardening, transient filesystem retry logic, and syncs codex cli auth state after a successful restore. the login loop is also flattened so the restore option is reachable even when no accounts are saved.key changes:
runBackupRestoreManagerinlib/codex-manager.ts: lists backups, assesses each with concurrency cap (NAMED_BACKUP_ASSESS_CONCURRENCY=4), re-assesses selected backup before confirmation (TOCTOU guard), callsassertNamedBackupRestorePatha second time beforeimportAccounts, then syncs codex cli stateassertNamedBackupRestorePath/resolveNamedBackupRestorePathinlib/storage.ts: path containment viarealpathSync.native+relative()check; symlinks filtered out earlier bylistNamedBackupsandfindExistingNamedBackupPathretryTransientFilesystemOperation: 7-attempt exponential backoff with jitter forEBUSY,EAGAIN,ENOTEMPTY, andEPERM(win32 only for the last)importAccountsnow returns achangedboolean and always deduplicates before checking the account limit; supports a metadata-refresh path (imported=0, changed=true) when backup values are newertest/storage.test.tsafterEachnow usesremoveWithRetry— the previously flagged windows flakiness issue is addressedissues noted:
EBUSYandENOTEMPTYare retried on all platforms, not scoped towin32— on linux these errors fromreadFile/readdirare typically non-transient and silently adding 2-3s of retry delay could mask real errorsassertNamedBackupRestorePathreturns the pre-realpathresolvedPathrather than the symlink-resolvedcontainedPath— creates a narrow TOCTOU window between containment validation andimportAccountsread, though symlink filtering earlier in the pipeline largely mitigates thisretryTransientFilesystemOperationexhausting all 7 attempts — the PR description lists retry exhaustion as a regression target but the new test only validates a single-failure recoveryConfidence Score: 3/5
Important Files Changed
assertNamedBackupRestorePathandretryTransientFilesystemOperationare the two key safety mechanisms; both have minor issues flagged above (return value of realpath-resolved path, cross-platform EBUSY retry scope). logic forimportAccountsis correct and now carries achangedfield.runBackupRestoreManagerand flattens therunAuthLoginlogin loop to expose the restore path when no accounts are saved. re-assessment + doubleassertNamedBackupRestorePathcall beforeimportAccountsis correct defense-in-depth. error handling aroundrunBackupRestoreManageris wrapped in try/catch so no crash risk in the menu loop. previously flagged unhandled error issue appears addressed.afterEachnow usesremoveWithRetry(addresses prior EBUSY/EPERM flakiness concern). new named backup tests cover listing, assessment, restore, and containment. missing coverage:retryTransientFilesystemOperationexhaustion (all 7 attempts fail).restore-backupaction type and inserts a "Recovery" heading section with the restore menu item. change is minimal and low-risk.(u) restore backup. copy is consistent with the restore flow logic inrunBackupRestoreManager.restore-backuptoLoginModeunion and fallback input aliases (u,backup,restore,restore-backup). straightforward extension with no risk surface.restore-backup. clean and correct.Sequence Diagram
sequenceDiagram participant User participant AuthMenu participant BackupRestoreManager participant Storage participant ImportAccounts participant CodexCLI User->>AuthMenu: select "Restore From Backup" AuthMenu->>BackupRestoreManager: runBackupRestoreManager() BackupRestoreManager->>Storage: listNamedBackups({ candidateCache }) Storage-->>BackupRestoreManager: NamedBackupMetadata[] loop for each chunk (ASSESS_CONCURRENCY=4) BackupRestoreManager->>Storage: assessNamedBackupRestore(name, { currentStorage, candidateCache }) Storage-->>BackupRestoreManager: BackupRestoreAssessment end BackupRestoreManager->>User: select() — show backup menu User-->>BackupRestoreManager: { type: "restore", assessment } Note over BackupRestoreManager: re-assess with fresh loadAccounts() (TOCTOU guard) BackupRestoreManager->>Storage: assessNamedBackupRestore(name, { currentStorage: await loadAccounts() }) Storage-->>BackupRestoreManager: latestAssessment alt latestAssessment.eligibleForRestore === false BackupRestoreManager->>User: show ineligible message, return else eligible BackupRestoreManager->>User: confirm(merge summary) User-->>BackupRestoreManager: confirmed BackupRestoreManager->>Storage: assertNamedBackupRestorePath(path, backupDir) Note over Storage: containment check via realpathSync Storage-->>BackupRestoreManager: validatedPath BackupRestoreManager->>ImportAccounts: importAccounts(validatedPath) Note over ImportAccounts: retryTransientFilesystemOperation (7 attempts)<br/>withAccountStorageTransaction → dedup → persist ImportAccounts-->>BackupRestoreManager: { imported, skipped, total, changed } alt result.changed === false BackupRestoreManager->>User: "All accounts already exist" else result.imported === 0 (metadata refresh) BackupRestoreManager->>User: restoreBackupRefreshSuccess else new accounts imported BackupRestoreManager->>User: restoreBackupSuccess end BackupRestoreManager->>CodexCLI: autoSyncActiveAccountToCodex() endPrompt To Fix All With AI
Last reviewed commit: 94c18b8
Context used: