feat(auth): add backup restore manager#76
Open
ndycode wants to merge 5 commits intogit-plan/01-reset-safetyfrom
Open
feat(auth): add backup restore manager#76ndycode wants to merge 5 commits intogit-plan/01-reset-safetyfrom
ndycode wants to merge 5 commits intogit-plan/01-reset-safetyfrom
Conversation
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
✨ 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}
test/storage.test.ts
Outdated
Comment on lines
+330
to
+333
| afterEach(async () => { | ||
| setStoragePathDirect(null); | ||
| await fs.rm(testWorkDir, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
afterEach missing windows retry for EBUSY/EPERM
the outer export/import suite's afterEach (lines 128-147) was updated in this PR to retry fs.rm on EBUSY, EPERM, and ENOTEMPTY — exactly the errors antivirus or the WAL rename logic can leave behind on windows. the new "named backups" suite's afterEach does plain fs.rm with no retry, so named-backup integration tests will flake on windows CI under the same conditions.
Suggested change
| afterEach(async () => { | |
| setStoragePathDirect(null); | |
| await fs.rm(testWorkDir, { recursive: true, force: true }); | |
| }); | |
| afterEach(async () => { | |
| setStoragePathDirect(null); | |
| for (let attempt = 0; attempt < 5; attempt += 1) { | |
| try { | |
| await fs.rm(testWorkDir, { recursive: true, force: true }); | |
| break; | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if ( | |
| (code !== "EBUSY" && code !== "EPERM" && code !== "ENOTEMPTY") || | |
| attempt === 4 | |
| ) { | |
| throw error; | |
| } | |
| await new Promise((resolve) => | |
| setTimeout(resolve, 25 * 2 ** attempt), | |
| ); | |
| } | |
| } | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/storage.test.ts
Line: 330-333
Comment:
**`afterEach` missing windows retry for `EBUSY`/`EPERM`**
the outer export/import suite's `afterEach` (lines 128-147) was updated in this PR to retry `fs.rm` on `EBUSY`, `EPERM`, and `ENOTEMPTY` — exactly the errors antivirus or the WAL rename logic can leave behind on windows. the new "named backups" suite's `afterEach` does plain `fs.rm` with no retry, so named-backup integration tests will flake on windows CI under the same conditions.
```suggestion
afterEach(async () => {
setStoragePathDirect(null);
for (let attempt = 0; attempt < 5; attempt += 1) {
try {
await fs.rm(testWorkDir, { recursive: true, force: true });
break;
} catch (error) {
const code = (error as NodeJS.ErrnoException).code;
if (
(code !== "EBUSY" && code !== "EPERM" && code !== "ENOTEMPTY") ||
attempt === 4
) {
throw error;
}
await new Promise((resolve) =>
setTimeout(resolve, 25 * 2 ** attempt),
);
}
}
});
```
How can I resolve this? If you propose a fix, please make it concise.
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.
| return { mode: "deep-check" }; | ||
| case "verify-flagged": | ||
| return { mode: "verify-flagged" }; | ||
| case "restore-backup": |
There was a problem hiding this comment.
P2: restore-backup is wired only in the TUI path; the fallback login prompt cannot select this new mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/cli.ts, line 283:
<comment>`restore-backup` is wired only in the TUI path; the fallback login prompt cannot select this new mode.</comment>
<file context>
@@ -279,6 +280,8 @@ export async function promptLoginMode(
return { mode: "deep-check" };
case "verify-flagged":
return { mode: "verify-flagged" };
+ case "restore-backup":
+ return { mode: "restore-backup" };
case "select-account": {
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by cubic
Adds a backup restore manager to the auth flow so users can list, assess, and restore named backups with clear status, confirmation, and safer path handling. Also hardens restore paths and makes the CLI catch and report restore failures.
New Features
listNamedBackups,assessNamedBackupRestore,createNamedBackup,restoreNamedBackup, andgetNamedBackupsDirectoryPath. Supports manual filenames already in the backups dir, includes metadata (created/updated, size, version), and enforces limits withimportAccounts.Safety
Written for commit 7443074. Summary will update on new commits.
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
adds a first-class backup restore manager to the auth login flow: named backups are discovered by scanning
~/.codex/multi-auth/backups/, assessed against the current account limit (pre-dedup merge count shown in the confirm prompt), and restored via the existingimportAccountstransaction path with a newtry/catchinrunAuthLogin. thetransactionSnapshotContextis also patched to capturestoragePathat transaction start soexportAccountsno longer blindly reuses a stale snapshot from a different storage scope.lib/storage.ts— new exported apislistNamedBackups,assessNamedBackupRestore,createNamedBackup,restoreNamedBackup,getNamedBackupsDirectoryPath;storagePathguard added toexportAccountsto prevent cross-scope snapshot reuse; path traversal is fully blocked vianormalizeNamedBackupFileName+assertWithinDirectoryinnamed-backup-export.tslib/codex-manager.ts—runBackupRestoreManagerbatches assessments concurrently, shows a selection menu with per-backup status/count/age hints, confirms before callingrestoreNamedBackup, and is wrapped in a top-leveltry/catchso errors return to the login loop rather than crashing the sessionlib/ui/auth-menu.ts/lib/ui/copy.ts— "Recovery → Restore From Backup" section added above "Danger Zone"; existing menu focus-key routing updatedtest/storage.test.ts— new named-backup tests correctly inherit theremoveWithRetryafterEach; path-traversal rejection is covered; a concurrent-restore race test is missingassertWithinDirectoryuseslstatSync+realpathSyncwith case-insensitive comparison on win32 and refuses symlinked backup roots; the storage mutex serialises concurrent restores; redaction policy unchanged (paths logged vialog.warn, tokens not included in backup metadata)Confidence Score: 3/5
Important Files Changed
listNamedBackups,assessNamedBackupRestore,createNamedBackup,restoreNamedBackup, andgetNamedBackupsDirectoryPath; fixes stale transaction snapshot bug viastoragePathcapture — but the new path-equality guard inexportAccountscreates a potential deadlock ifsetStoragePathis ever called while a transaction is active (theoretical on prod, realistic in windows CI tests)runBackupRestoreManagerwith batch assessment, confirm prompt, and proper try/catch around the whole menu entry;formatRelativeDateShorthandles clock-skew future timestamps correctly by collapsing them to "today"restore-backupaction type and "Recovery" section to the auth menu; change is straightforward and consistent with existing menu construction patternsconfirm; two new integration tests cover the happy-path restore and error-recovery flows; good coverage of the new menu branchremoveWithRetryafterEach; path-traversal rejection test is present; missing a concurrent-restore regression testSequence Diagram
sequenceDiagram participant User participant AuthMenu participant BackupRestoreManager participant Storage User->>AuthMenu: select "Restore From Backup" AuthMenu->>BackupRestoreManager: runBackupRestoreManager() BackupRestoreManager->>Storage: listNamedBackups() Storage-->>BackupRestoreManager: NamedBackupMetadata[] BackupRestoreManager->>Storage: loadAccounts() Storage-->>BackupRestoreManager: currentStorage BackupRestoreManager->>Storage: assessNamedBackupRestore(name, {currentStorage}) [parallel] Storage-->>BackupRestoreManager: BackupRestoreAssessment[] BackupRestoreManager->>User: select(backups with status/count/age hints) User-->>BackupRestoreManager: { type: "restore", assessment } BackupRestoreManager->>User: confirm("merge N accounts, M after dedupe?") User-->>BackupRestoreManager: true BackupRestoreManager->>Storage: restoreNamedBackup(name) Storage->>Storage: resolveNamedBackupRestorePath(name) Storage->>Storage: importAccounts(backupPath) [inside storageMutex] Storage-->>BackupRestoreManager: { imported, skipped, total } BackupRestoreManager-->>User: "Restored backup. Imported N, skipped M, total T."Comments Outside Diff (1)
lib/storage.ts, line 2597-2604 (link)deadlock when
storagePathchanges inside an active transactionthe new path-equality guard falls through to
withAccountStorageTransactionwhentransactionState.active === truebuttransactionState.storagePath !== currentStoragePath.withAccountStorageTransactioncallswithStorageLock, which chains ontostorageMutex. if we are already inside awithStorageLockinvocation (which is how we got here with an active transaction), the new work item is queued behind the lock we are currently holding — the outer lock waits for the inner lock that can never run → deadlock.this matters in windows CI where tests call
setStoragePathDirectbetween operations and have occasionally raced into cross-path export paths.the safer fallback is to throw or return early with an explicit error rather than silently re-entering the lock:
Prompt To Fix All With AI
Last reviewed commit: 7443074
Context used: