feat(settings): productize stable and advanced controls#79
Open
ndycode wants to merge 6 commits intogit-plan/04-sync-centerfrom
Open
feat(settings): productize stable and advanced controls#79ndycode wants to merge 6 commits intogit-plan/04-sync-centerfrom
ndycode wants to merge 6 commits intogit-plan/04-sync-centerfrom
Conversation
…ttings-productization # Conflicts: # test/codex-manager-cli.test.ts
Contributor
📝 WalkthroughWalkthroughintroduces a named backup restore feature enabling users to interactively select, assess eligibility, and restore backed-up account credentials from designated directories. includes new storage apis, cli login modes, manager workflows, and updated ui surfaces. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant Manager as Backup Restore<br/>Manager
participant Storage
participant UI
User->>CLI: auth login
CLI->>Manager: select restore-backup mode
Manager->>Storage: listNamedBackups()
Storage-->>Manager: backups[]
Manager->>UI: show backup options
User->>UI: select backup
UI-->>Manager: backup selection
Manager->>Storage: assessNamedBackupRestore(name)
Storage-->>Manager: BackupRestoreAssessment
Manager->>UI: confirm restoration?
User->>UI: confirm
UI-->>Manager: confirmed
Manager->>Storage: restoreNamedBackup(name)
Storage-->>Manager: {imported, total, skipped}
Manager->>CLI: restoration complete
CLI-->>User: success log
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes notes for reviewerslib/storage.ts:1231/-785 lines — substantial changes warrant careful examination of:
lib/codex-manager.ts:+109 lines — verify:
test/storage.test.ts:+109/-1 lines — note:
test/codex-manager-cli.test.ts:+182 lines — flag:
lib/ui/copy.ts — settings text rebranding is extensive; confirm all ui surfaces correctly reference updated keys, especially 🚥 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}
Comment on lines
4085
to
+4110
| } | ||
| } | ||
|
|
||
| type BackupMenuAction = | ||
| | { | ||
| type: "restore"; | ||
| assessment: Awaited<ReturnType<typeof assessNamedBackupRestore>>; | ||
| } | ||
| | { type: "back" }; | ||
|
|
||
| async function runBackupRestoreManager( | ||
| displaySettings: DashboardDisplaySettings, | ||
| ): Promise<void> { | ||
| const backupDir = getNamedBackupsDirectoryPath(); | ||
| const backups = await listNamedBackups(); | ||
| if (backups.length === 0) { | ||
| console.log(`No named backups found. Place backup files in ${backupDir}.`); | ||
| return; | ||
| } | ||
|
|
||
| const currentStorage = await loadAccounts(); | ||
| const assessments = await Promise.all( | ||
| backups.map((backup) => | ||
| assessNamedBackupRestore(backup.name, { currentStorage }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Double I/O per backup — antivirus lock risk on Windows
runBackupRestoreManager calls listNamedBackups() first, which already reads and stats every backup file. then it immediately calls assessNamedBackupRestore(backup.name, ...) for each one — which calls loadBackupCandidate and fs.stat again. that's 4 I/O operations per backup file to build the menu. on Windows where antivirus rescans files on every open, you can get EBUSY errors or multi-second stalls proportional to backup count.
since listNamedBackups already returns full NamedBackupMetadata (including valid, accountCount, etc.), you can build the assessment directly from what you already have rather than re-reading from disk:
const backups = await listNamedBackups();
const currentStorage = await loadAccounts();
const currentAccounts = currentStorage?.accounts ?? [];
const assessments = backups.map((backup) => {
if (!backup.valid || !backup.accountCount || backup.accountCount <= 0) {
return { backup, currentAccountCount: currentAccounts.length, mergedAccountCount: null, imported: null, skipped: null, wouldExceedLimit: false, valid: false, error: backup.loadError ?? "Backup is empty or invalid" };
}
// build merged count inline without re-reading disk
...
});this avoids the second round of file I/O entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4085-4110
Comment:
**Double I/O per backup — antivirus lock risk on Windows**
`runBackupRestoreManager` calls `listNamedBackups()` first, which already reads and stats every backup file. then it immediately calls `assessNamedBackupRestore(backup.name, ...)` for each one — which calls `loadBackupCandidate` and `fs.stat` again. that's 4 I/O operations per backup file to build the menu. on Windows where antivirus rescans files on every open, you can get `EBUSY` errors or multi-second stalls proportional to backup count.
since `listNamedBackups` already returns full `NamedBackupMetadata` (including `valid`, `accountCount`, etc.), you can build the assessment directly from what you already have rather than re-reading from disk:
```ts
const backups = await listNamedBackups();
const currentStorage = await loadAccounts();
const currentAccounts = currentStorage?.accounts ?? [];
const assessments = backups.map((backup) => {
if (!backup.valid || !backup.accountCount || backup.accountCount <= 0) {
return { backup, currentAccountCount: currentAccounts.length, mergedAccountCount: null, imported: null, skipped: null, wouldExceedLimit: false, valid: false, error: backup.loadError ?? "Backup is empty or invalid" };
}
// build merged count inline without re-reading disk
...
});
```
this avoids the second round of file I/O entirely.
How can I resolve this? If you propose a fix, please make it concise.
![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 10 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 Windows-safe retry cleanup in teardown instead of a bare `fs.rm` to avoid flaky `EBUSY`/`EPERM`/`ENOTEMPTY` failures.</violation>
</file>
<file name="lib/cli.ts">
<violation number="1" location="lib/cli.ts:283">
P2: The new `restore-backup` login mode is not supported in `promptLoginModeFallback`, so it becomes unreachable when the UI falls back from `showAuthMenu` to text input.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:475">
P2: Backup restore path resolution is inconsistent with listed backup names, so manually placed backups with non-canonical filenames can be listed but not restored.</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.
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="lib/storage.ts">
<violation number="1" location="lib/storage.ts:477">
P1: `resolveNamedBackupPath` now returns an unsanitized existing path, allowing path traversal outside the backups directory.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+477
to
+485
| const directPath = join( | ||
| backupDir, | ||
| trimmedName.endsWith(NAMED_BACKUP_EXTENSION) | ||
| ? trimmedName | ||
| : `${trimmedName}${NAMED_BACKUP_EXTENSION}`, | ||
| ); | ||
| if (existsSync(directPath)) { | ||
| return directPath; | ||
| } |
There was a problem hiding this comment.
P1: resolveNamedBackupPath now returns an unsanitized existing path, allowing path traversal outside the backups directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/storage.ts, line 477:
<comment>`resolveNamedBackupPath` now returns an unsanitized existing path, allowing path traversal outside the backups directory.</comment>
<file context>
@@ -472,11 +472,19 @@ async function ensureNamedBackupsDirectory(): Promise<string> {
- `${normalizedName}${NAMED_BACKUP_EXTENSION}`,
+ const backupDir = getNamedBackupsDirectory();
+ const trimmedName = name.trim();
+ const directPath = join(
+ backupDir,
+ trimmedName.endsWith(NAMED_BACKUP_EXTENSION)
</file context>
Suggested change
| const directPath = join( | |
| backupDir, | |
| trimmedName.endsWith(NAMED_BACKUP_EXTENSION) | |
| ? trimmedName | |
| : `${trimmedName}${NAMED_BACKUP_EXTENSION}`, | |
| ); | |
| if (existsSync(directPath)) { | |
| return directPath; | |
| } | |
| const directFileName = trimmedName.endsWith(NAMED_BACKUP_EXTENSION) | |
| ? trimmedName | |
| : `${trimmedName}${NAMED_BACKUP_EXTENSION}`; | |
| if (directFileName === basename(directFileName)) { | |
| const directPath = join(backupDir, directFileName); | |
| if (existsSync(directPath)) { | |
| return directPath; | |
| } | |
| } |
Comment on lines
+742
to
+756
| candidate: T, | ||
| ): T { | ||
| if (!current) return candidate; | ||
| const currentLastUsed = current.lastUsed || 0; | ||
| const candidateLastUsed = candidate.lastUsed || 0; | ||
| if (candidateLastUsed > currentLastUsed) return candidate; | ||
| if (candidateLastUsed < currentLastUsed) return current; | ||
| const currentAddedAt = current.addedAt || 0; | ||
| const candidateAddedAt = candidate.addedAt || 0; | ||
| return candidateAddedAt >= currentAddedAt ? candidate : current; | ||
| if (!current) return candidate; | ||
| const currentLastUsed = current.lastUsed || 0; | ||
| const candidateLastUsed = candidate.lastUsed || 0; | ||
| if (candidateLastUsed > currentLastUsed) return candidate; | ||
| if (candidateLastUsed < currentLastUsed) return current; | ||
| const currentAddedAt = current.addedAt || 0; | ||
| const candidateAddedAt = candidate.addedAt || 0; | ||
| return candidateAddedAt >= currentAddedAt ? candidate : current; | ||
| } | ||
|
|
||
| function deduplicateAccountsByKey<T extends AccountLike>(accounts: T[]): T[] { | ||
| const keyToIndex = new Map<string, number>(); | ||
| const indicesToKeep = new Set<number>(); | ||
|
|
||
| for (let i = 0; i < accounts.length; i += 1) { | ||
| const account = accounts[i]; | ||
| if (!account) continue; | ||
| const key = account.accountId || account.refreshToken; | ||
| if (!key) continue; | ||
|
|
||
| const existingIndex = keyToIndex.get(key); | ||
| if (existingIndex === undefined) { | ||
| keyToIndex.set(key, i); | ||
| continue; | ||
| } | ||
|
|
||
| const existing = accounts[existingIndex]; | ||
| const newest = selectNewestAccount(existing, account); | ||
| keyToIndex.set(key, newest === account ? i : existingIndex); | ||
| } | ||
|
|
||
| for (const idx of keyToIndex.values()) { | ||
| indicesToKeep.add(idx); | ||
| } | ||
|
|
||
| const result: T[] = []; | ||
| for (let i = 0; i < accounts.length; i += 1) { | ||
| if (indicesToKeep.has(i)) { | ||
| const account = accounts[i]; | ||
| if (account) result.push(account); | ||
| } | ||
| } | ||
| return result; | ||
| const keyToIndex = new Map<string, number>(); | ||
| const indicesToKeep = new Set<number>(); |
There was a problem hiding this comment.
path traversal via directPath before normalization
resolveNamedBackupPath builds directPath from the raw trimmedName before any sanitization, then calls existsSync on it. if trimmedName contains .. (e.g., "../../settings.json"), node:path join will silently resolve outside backupDir. if that path happens to exist, the function returns it — and callers like restoreNamedBackup and assessNamedBackupRestore never normalize the name first (createNamedBackup does, but those two don't).
worst case on a user desktop: restoreNamedBackup("../openai-codex-accounts.json") reads the live accounts file as an "import source", potentially corrupting active storage after dedup.
fix: normalize the name before the existsSync probe, or validate that directPath is still under backupDir:
function resolveNamedBackupPath(name: string): string {
const backupDir = getNamedBackupsDirectory();
const normalizedName = normalizeBackupName(name.trim());
const candidatePath = join(backupDir, `${normalizedName}${NAMED_BACKUP_EXTENSION}`);
if (existsSync(candidatePath)) {
return candidatePath;
}
return candidatePath;
}same guard should be applied in assessNamedBackupRestore and restoreNamedBackup — both should call normalizeBackupName on the incoming name before forwarding to resolveNamedBackupPath.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 742-756
Comment:
**path traversal via `directPath` before normalization**
`resolveNamedBackupPath` builds `directPath` from the raw `trimmedName` before any sanitization, then calls `existsSync` on it. if `trimmedName` contains `..` (e.g., `"../../settings.json"`), `node:path join` will silently resolve outside `backupDir`. if that path happens to exist, the function returns it — and callers like `restoreNamedBackup` and `assessNamedBackupRestore` never normalize the name first (`createNamedBackup` does, but those two don't).
worst case on a user desktop: `restoreNamedBackup("../openai-codex-accounts.json")` reads the live accounts file as an "import source", potentially corrupting active storage after dedup.
fix: normalize the name before the `existsSync` probe, or validate that `directPath` is still under `backupDir`:
```ts
function resolveNamedBackupPath(name: string): string {
const backupDir = getNamedBackupsDirectory();
const normalizedName = normalizeBackupName(name.trim());
const candidatePath = join(backupDir, `${normalizedName}${NAMED_BACKUP_EXTENSION}`);
if (existsSync(candidatePath)) {
return candidatePath;
}
return candidatePath;
}
```
same guard should be applied in `assessNamedBackupRestore` and `restoreNamedBackup` — both should call `normalizeBackupName` on the incoming `name` before forwarding to `resolveNamedBackupPath`.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/storage-paths.md (1)
25-47:⚠️ Potential issue | 🟡 Minoradd named backups restore workflow to upgrade guide and README.
the storage-paths.md updates are good, but docs/upgrade.md and README.md don't mention how to use the new named backup restore feature. per coding guidelines ("whenever behavior changes, require updated upgrade notes"), you need to document the restore workflow in docs/upgrade.md:45+ area and update README.md with a backup/restore example. CHANGELOG.md covers internal backup improvements, but the public-facing restore API usage should be clear in user-facing docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/storage-paths.md` around lines 25 - 47, Add a new subsection to docs/upgrade.md (around the existing upgrade steps near the 45+ area) that explains the named backups restore workflow: describe the restore command or API endpoint, required arguments (backup name, project-key if project-scoped), where backups live (`~/.codex/multi-auth/backups/<name>.json` and project-scoped `~/.codex/multi-auth/projects/<project-key>/backups/<name>.json`), and any safety steps (WAL/backup validation and recommended pre-restore backup of current accounts file). Also update README.md to include a brief example showing how to list available named backups and perform a restore (include the same path conventions and example flags/arguments), and mention when to use project-scoped vs global restore; keep wording consistent with CHANGELOG.md’s internal note but focus on user-facing CLI/API usage.
🤖 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 216-223: The new fallback branch in lib/cli.ts that maps
normalized values ("u", "restore", "backup", "restore-backup") to { mode:
"restore-backup" } lacks a non-TTY regression test; add a vitest that calls the
same fallback/argument-parsing entry used by the CLI (the function that computes
normalized and returns the mode) with each alias
("u","restore","backup","restore-backup") in a non-tty scenario and assert the
returned object equals { mode: "restore-backup" }; add the test to
test/codex-manager-cli.test.ts near the existing tty coverage (the test around
line ~1636) so future changes to the fallback parser (the code using normalized
and returning mode "restore-backup") are explicitly verified.
In `@lib/codex-manager.ts`:
- Around line 4099-4110: The code currently calls listNamedBackups() to get
backup metadata and then calls assessNamedBackupRestore(backup.name, ...) which
re-reads the same backup files, causing double I/O; fix by reading each backup
once and passing the read data into the assessor (either change
assessNamedBackupRestore signature to accept a backup object/content or add a
new helper assessNamedBackupRestoreFromData), i.e., replace the backups.map call
to pass the already-loaded backup data (from listNamedBackups) into the assessor
and update assessNamedBackupRestore (or add the new function) to use that data
instead of re-opening the file; keep function names listNamedBackups and
assessNamedBackupRestore referenced so you can locate and update both places.
- Around line 4171-4174: The restoreNamedBackup call can throw and should be
wrapped in a try/catch so interactive login/menu flow doesn't abort; inside the
same block where restoreNamedBackup(assessment.backup.name) is invoked (and
where result is used), add a try { const result = await
restoreNamedBackup(assessment.backup.name); ... } catch (err) {
processLogger.error or console.error with the backup name and err, and set any
local status variables (e.g. treat imported/skipped/total as zero or mark
restore failed) so the code continues the menu/login path instead of throwing.
Ensure you reference restoreNamedBackup and assessment.backup.name and keep the
downstream logic that prints the restored/imported/skipped/total behavior
conditional on success vs failure.
In `@lib/storage.ts`:
- Around line 474-487: resolveNamedBackupPath currently returns a directPath
built from unsanitized name which allows path traversal; fix by
normalizing/sanitizing the input and ensuring any candidate path (directPath and
the fallback built from normalizedName) is resolved into an absolute path and
verified to live inside backupDir (use path.resolve and confirm
path.relative(backupDir, candidatePath) does not start with '..' or that
candidatePath.startsWith(backupDir) after resolving) before returning it,
otherwise reject or fall back to the safe normalizedName-based path; update the
existence check to only accept paths that both exist and are contained within
backupDir (references: resolveNamedBackupPath, backupDir, directPath,
normalizedName, normalizeBackupName).
- Around line 1294-1304: The loop that reads entries and awaits
loadBackupCandidate/buildNamedBackupMetadata serially (the block using entries,
deriveBackupNameFromFile, loadBackupCandidate, buildNamedBackupMetadata) should
be converted to a parallelized mapping: filter entries for isFile() and
NAMED_BACKUP_EXTENSION, map each to an async function that derives name/path,
calls loadBackupCandidate(path) and then buildNamedBackupMetadata(name, path, {
candidate }), collect the resulting promises and await Promise.all to produce
the backups array; ensure you preserve ordering if needed and surface errors as
before.
In `@lib/ui/copy.ts`:
- Around line 17-18: The fallback non-TTY prompt in lib/ui/copy.ts is missing
the new restore/backup option, causing a mismatch with the parser in lib/cli.ts
that accepts restore commands; update the fallback prompt text (the string(s)
that list available commands in this module—look for the fallback prompt
generation around the existing recovery/restoreBackup labels) to include the
restore/restoreBackup wording so non-tty users see the same "Restore From
Backup" option that the parser (lib/cli.ts handling at the restore command
branch) already accepts.
In `@test/codex-manager-cli.test.ts`:
- Around line 1636-1698: The test uses the real confirm() making restore flow
non-deterministic; mock ../lib/ui/confirm.js with a confirmMock (vi.fn(async ()
=> true)) at the top of test/codex-manager-cli.test.ts, reset it in beforeEach,
and update the existing "restores a named backup..." test to assert
expect(confirmMock).toHaveBeenCalled() and that restoreNamedBackupMock was
invoked; then add a regression test where confirmMock resolves to false and
verify that restoreNamedBackupMock is NOT called (while listNamedBackupsMock and
assessNamedBackupRestoreMock still run) to cover the declined-confirmation
branch for runCodexMultiAuthCli -> restoreNamedBackupMock control flow.
In `@test/storage.test.ts`:
- Around line 335-403: The test block only covers happy-path backup/restore and
invalid names; add deterministic vitest regression tests that reproduce
concurrency and edge-path failures by (1) adding a concurrent-restore race test
that spawns multiple parallel calls to restoreNamedBackup and asserts
deterministic outcome/no data corruption (use Promise.all with controlled delays
or mocked fs), (2) a restore-limit failure test that seeds accounts up to the
limit and verifies assessNamedBackupRestore.wouldExceedLimit and
restoreNamedBackup rejects or returns the expected error/partial result, and (3)
Windows-path filename handling tests that create a backup name containing
backslashes or reserved chars and ensure
createNamedBackup/listNamedBackups/store file names are normalized (refer to
createNamedBackup, listNamedBackups, assessNamedBackupRestore,
restoreNamedBackup, saveAccounts, clearAccounts, loadAccounts to locate code
under test); keep tests deterministic (avoid timers unless mocked) and use
vitest async utilities and filesystem mocking where needed.
- Around line 330-333: The afterEach teardown using a single fs.rm call can
flake on Windows due to transient EBUSY/EPERM/ENOTEMPTY errors; update the
afterEach (the teardown that calls setStoragePathDirect(null) and
fs.rm(testWorkDir, ...)) to perform a resilient removal: implement a short retry
loop with a few attempts and small delays (exponential backoff) catching
transient errors (EPERM, EBUSY, ENOTEMPTY) and retrying before ultimately
failing, or use a cross-platform recursive delete helper if available; ensure
retries only wrap the fs.rm call and still clear setStoragePathDirect(null)
immediately.
---
Outside diff comments:
In `@docs/reference/storage-paths.md`:
- Around line 25-47: Add a new subsection to docs/upgrade.md (around the
existing upgrade steps near the 45+ area) that explains the named backups
restore workflow: describe the restore command or API endpoint, required
arguments (backup name, project-key if project-scoped), where backups live
(`~/.codex/multi-auth/backups/<name>.json` and project-scoped
`~/.codex/multi-auth/projects/<project-key>/backups/<name>.json`), and any
safety steps (WAL/backup validation and recommended pre-restore backup of
current accounts file). Also update README.md to include a brief example showing
how to list available named backups and perform a restore (include the same path
conventions and example flags/arguments), and mention when to use project-scoped
vs global restore; keep wording consistent with CHANGELOG.md’s internal note but
focus on user-facing CLI/API usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42441cd8-bd6a-40fc-b2c1-0324c8e5af39
📒 Files selected for processing (10)
docs/reference/settings.mddocs/reference/storage-paths.mdlib/cli.tslib/codex-manager.tslib/codex-manager/settings-hub.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). (2)
- GitHub Check: cubic · AI code reviewer
- 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/codex-manager/settings-hub.tslib/cli.tslib/codex-manager.tslib/ui/auth-menu.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.mddocs/reference/settings.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 (6)
lib/codex-manager/settings-hub.ts (1)
2744-2784: nice settings hub wiring.the new hints and advanced entries are internally consistent, and the new sync-center action is fully wired through selection and handling (
lib/codex-manager/settings-hub.ts:2773,lib/codex-manager/settings-hub.ts:2835). coverage is present intest/codex-manager-cli.test.ts:1700andtest/codex-manager-cli.test.ts:1778.lib/ui/auth-menu.ts (1)
58-69: restore-backup menu action integration looks good.the action type, focus-key persistence, and rendered recovery entry are aligned (
lib/ui/auth-menu.ts:58,lib/ui/auth-menu.ts:502,lib/ui/auth-menu.ts:665), and this matches the login-mode mapping inlib/cli.ts:291.Also applies to: 519-520, 658-668
lib/ui/copy.ts (1)
65-115: settings copy refresh is clean and consistent.the new everyday/advanced taxonomy and hint keys line up with settings-hub consumers (
lib/ui/copy.ts:68,lib/codex-manager/settings-hub.ts:2745).lib/cli.ts (1)
291-292: restore-backup action mapping is correct.the menu action-to-mode bridge is wired correctly for the new flow (
lib/cli.ts:291).test/codex-manager-cli.test.ts (1)
1946-1962: good regression matrix for windows/concurrency/token-race scenarios.the parameterized no-save-on-cancel matrix is strong coverage for the failure modes we care about (
test/codex-manager-cli.test.ts:1946).docs/reference/settings.md (1)
22-139: lgtm: the settings taxonomy update is clear and consistent.the split between everyday settings and advanced/operator controls reads cleanly and maps well to the new surface language. reference:
docs/reference/settings.md:22-139.
Comment on lines
+216
to
+223
| if ( | ||
| normalized === "u" || | ||
| normalized === "restore" || | ||
| normalized === "backup" || | ||
| normalized === "restore-backup" | ||
| ) { | ||
| return { mode: "restore-backup" }; | ||
| } |
Contributor
There was a problem hiding this comment.
add fallback-parser regression coverage for restore aliases.
this adds new fallback commands (lib/cli.ts:216), but there is no explicit non-tty regression test proving u/restore/backup routes to restore-backup. current coverage shown is tty flow (test/codex-manager-cli.test.ts:1636).
As per coding guidelines, "lib/**: ... verify every change cites affected tests (vitest) ..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/cli.ts` around lines 216 - 223, The new fallback branch in lib/cli.ts
that maps normalized values ("u", "restore", "backup", "restore-backup") to {
mode: "restore-backup" } lacks a non-TTY regression test; add a vitest that
calls the same fallback/argument-parsing entry used by the CLI (the function
that computes normalized and returns the mode) with each alias
("u","restore","backup","restore-backup") in a non-tty scenario and assert the
returned object equals { mode: "restore-backup" }; add the test to
test/codex-manager-cli.test.ts near the existing tty coverage (the test around
line ~1636) so future changes to the fallback parser (the code using normalized
and returning mode "restore-backup") are explicitly verified.
Comment on lines
+4099
to
+4110
| const backups = await listNamedBackups(); | ||
| if (backups.length === 0) { | ||
| console.log(`No named backups found. Place backup files in ${backupDir}.`); | ||
| return; | ||
| } | ||
|
|
||
| const currentStorage = await loadAccounts(); | ||
| const assessments = await Promise.all( | ||
| backups.map((backup) => | ||
| assessNamedBackupRestore(backup.name, { currentStorage }), | ||
| ), | ||
| ); |
Contributor
There was a problem hiding this comment.
avoid double backup reads while constructing the restore menu.
this reads each backup once in listNamedBackups() and again in assessNamedBackupRestore(). latency scales linearly with file count and can stall the menu. reference: lib/codex-manager.ts:4099-4110.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4099 - 4110, The code currently calls
listNamedBackups() to get backup metadata and then calls
assessNamedBackupRestore(backup.name, ...) which re-reads the same backup files,
causing double I/O; fix by reading each backup once and passing the read data
into the assessor (either change assessNamedBackupRestore signature to accept a
backup object/content or add a new helper assessNamedBackupRestoreFromData),
i.e., replace the backups.map call to pass the already-loaded backup data (from
listNamedBackups) into the assessor and update assessNamedBackupRestore (or add
the new function) to use that data instead of re-opening the file; keep function
names listNamedBackups and assessNamedBackupRestore referenced so you can locate
and update both places.
Comment on lines
+4171
to
+4174
| const result = await restoreNamedBackup(assessment.backup.name); | ||
| console.log( | ||
| `Restored backup "${assessment.backup.name}". Imported ${result.imported}, skipped ${result.skipped}, total ${result.total}.`, | ||
| ); |
Contributor
There was a problem hiding this comment.
catch restore failures so login mode does not abort on i/o or schema errors.
restoreNamedBackup can throw for invalid/corrupt backups. right now that exception escapes the menu path and can terminate the interactive flow. reference: lib/codex-manager.ts:4171-4174.
proposed fix
- const result = await restoreNamedBackup(assessment.backup.name);
- console.log(
- `Restored backup "${assessment.backup.name}". Imported ${result.imported}, skipped ${result.skipped}, total ${result.total}.`,
- );
+ try {
+ const result = await restoreNamedBackup(assessment.backup.name);
+ console.log(
+ `Restored backup "${assessment.backup.name}". Imported ${result.imported}, skipped ${result.skipped}, total ${result.total}.`,
+ );
+ } catch (error) {
+ console.error(
+ `Restore failed for "${assessment.backup.name}": ${error instanceof Error ? error.message : String(error)}`,
+ );
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const result = await restoreNamedBackup(assessment.backup.name); | |
| console.log( | |
| `Restored backup "${assessment.backup.name}". Imported ${result.imported}, skipped ${result.skipped}, total ${result.total}.`, | |
| ); | |
| try { | |
| const result = await restoreNamedBackup(assessment.backup.name); | |
| console.log( | |
| `Restored backup "${assessment.backup.name}". Imported ${result.imported}, skipped ${result.skipped}, total ${result.total}.`, | |
| ); | |
| } catch (error) { | |
| console.error( | |
| `Restore failed for "${assessment.backup.name}": ${error instanceof Error ? error.message : String(error)}`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4171 - 4174, The restoreNamedBackup call
can throw and should be wrapped in a try/catch so interactive login/menu flow
doesn't abort; inside the same block where
restoreNamedBackup(assessment.backup.name) is invoked (and where result is
used), add a try { const result = await
restoreNamedBackup(assessment.backup.name); ... } catch (err) {
processLogger.error or console.error with the backup name and err, and set any
local status variables (e.g. treat imported/skipped/total as zero or mark
restore failed) so the code continues the menu/login path instead of throwing.
Ensure you reference restoreNamedBackup and assessment.backup.name and keep the
downstream logic that prints the restored/imported/skipped/total behavior
conditional on success vs failure.
Comment on lines
+474
to
+487
| function resolveNamedBackupPath(name: string): string { | ||
| const backupDir = getNamedBackupsDirectory(); | ||
| const trimmedName = name.trim(); | ||
| const directPath = join( | ||
| backupDir, | ||
| trimmedName.endsWith(NAMED_BACKUP_EXTENSION) | ||
| ? trimmedName | ||
| : `${trimmedName}${NAMED_BACKUP_EXTENSION}`, | ||
| ); | ||
| if (existsSync(directPath)) { | ||
| return directPath; | ||
| } | ||
| const normalizedName = normalizeBackupName(trimmedName); | ||
| return join(backupDir, `${normalizedName}${NAMED_BACKUP_EXTENSION}`); |
Contributor
There was a problem hiding this comment.
block path traversal in named-backup path resolution.
resolveNamedBackupPath builds directPath from unsanitized input and returns it when it exists. a crafted name like ../... can target files outside the backups directory. reference: lib/storage.ts:474-487.
proposed fix
function resolveNamedBackupPath(name: string): string {
const backupDir = getNamedBackupsDirectory();
- const trimmedName = name.trim();
+ const trimmedName = basename(name.trim());
+ if (!trimmedName) {
+ throw new Error(`Invalid backup name: ${name}`);
+ }
const directPath = join(
backupDir,
trimmedName.endsWith(NAMED_BACKUP_EXTENSION)
? trimmedName
: `${trimmedName}${NAMED_BACKUP_EXTENSION}`,
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 474 - 487, resolveNamedBackupPath currently
returns a directPath built from unsanitized name which allows path traversal;
fix by normalizing/sanitizing the input and ensuring any candidate path
(directPath and the fallback built from normalizedName) is resolved into an
absolute path and verified to live inside backupDir (use path.resolve and
confirm path.relative(backupDir, candidatePath) does not start with '..' or that
candidatePath.startsWith(backupDir) after resolving) before returning it,
otherwise reject or fall back to the safe normalizedName-based path; update the
existence check to only accept paths that both exist and are contained within
backupDir (references: resolveNamedBackupPath, backupDir, directPath,
normalizedName, normalizeBackupName).
Comment on lines
+1294
to
+1304
| const entries = await fs.readdir(backupDir, { withFileTypes: true }); | ||
| const backups: NamedBackupMetadata[] = []; | ||
| for (const entry of entries) { | ||
| if (!entry.isFile()) continue; | ||
| if (!entry.name.endsWith(NAMED_BACKUP_EXTENSION)) continue; | ||
|
|
||
| const name = deriveBackupNameFromFile(entry.name); | ||
| const path = join(backupDir, entry.name); | ||
| const candidate = await loadBackupCandidate(path); | ||
| backups.push(await buildNamedBackupMetadata(name, path, { candidate })); | ||
| } |
Contributor
There was a problem hiding this comment.
parallelize named-backup metadata loading to reduce menu latency.
the current loop awaits each backup read/stat serially. this becomes a hot path when many backups exist and slows restore ui responsiveness. reference: lib/storage.ts:1294-1304.
proposed fix
const entries = await fs.readdir(backupDir, { withFileTypes: true });
- const backups: NamedBackupMetadata[] = [];
- for (const entry of entries) {
- if (!entry.isFile()) continue;
- if (!entry.name.endsWith(NAMED_BACKUP_EXTENSION)) continue;
-
- const name = deriveBackupNameFromFile(entry.name);
- const path = join(backupDir, entry.name);
- const candidate = await loadBackupCandidate(path);
- backups.push(await buildNamedBackupMetadata(name, path, { candidate }));
- }
+ const backups = await Promise.all(
+ entries
+ .filter((entry) => entry.isFile() && entry.name.endsWith(NAMED_BACKUP_EXTENSION))
+ .map(async (entry) => {
+ const name = deriveBackupNameFromFile(entry.name);
+ const path = join(backupDir, entry.name);
+ const candidate = await loadBackupCandidate(path);
+ return buildNamedBackupMetadata(name, path, { candidate });
+ }),
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 1294 - 1304, The loop that reads entries and
awaits loadBackupCandidate/buildNamedBackupMetadata serially (the block using
entries, deriveBackupNameFromFile, loadBackupCandidate,
buildNamedBackupMetadata) should be converted to a parallelized mapping: filter
entries for isFile() and NAMED_BACKUP_EXTENSION, map each to an async function
that derives name/path, calls loadBackupCandidate(path) and then
buildNamedBackupMetadata(name, path, { candidate }), collect the resulting
promises and await Promise.all to produce the backups array; ensure you preserve
ordering if needed and surface errors as before.
Comment on lines
+17
to
+18
| recovery: "Recovery", | ||
| restoreBackup: "Restore From Backup", |
Contributor
There was a problem hiding this comment.
fallback command copy is now out of sync with parser behavior.
you added recovery labels for tty flows (lib/ui/copy.ts:17), but the fallback prompt still omits restore commands (lib/ui/copy.ts:143-144) while parsing already accepts them (lib/cli.ts:216-223). non-tty users won’t discover the new mode.
proposed fix
- selectModePrompt:
- "(a) add, (c) check, (b) best, fi(x), (s) settings, (d) deep, (g) problem, (f) fresh, (r) reset, (q) back [a/c/b/x/s/d/g/f/r/q]: ",
- invalidModePrompt: "Use one of: a, c, b, x, s, d, g, f, r, q.",
+ selectModePrompt:
+ "(a) add, (c) check, (b) best, fi(x), (s) settings, (d) deep, (g) problem, (f) fresh, (r) reset, (u) restore backup, (q) back [a/c/b/x/s/d/g/f/r/u/q]: ",
+ invalidModePrompt: "Use one of: a, c, b, x, s, d, g, f, r, u, q.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ui/copy.ts` around lines 17 - 18, The fallback non-TTY prompt in
lib/ui/copy.ts is missing the new restore/backup option, causing a mismatch with
the parser in lib/cli.ts that accepts restore commands; update the fallback
prompt text (the string(s) that list available commands in this module—look for
the fallback prompt generation around the existing recovery/restoreBackup
labels) to include the restore/restoreBackup wording so non-tty users see the
same "Restore From Backup" option that the parser (lib/cli.ts handling at the
restore command branch) already accepts.
Comment on lines
+1636
to
+1698
| it("restores a named backup from the login recovery menu", async () => { | ||
| setInteractiveTTY(true); | ||
| const now = Date.now(); | ||
| const assessment = { | ||
| backup: { | ||
| name: "named-backup", | ||
| path: "/mock/backups/named-backup.json", | ||
| createdAt: now - 1_000, | ||
| updatedAt: now - 1_000, | ||
| sizeBytes: 512, | ||
| version: 3, | ||
| accountCount: 1, | ||
| schemaErrors: [], | ||
| valid: true, | ||
| loadError: undefined, | ||
| }, | ||
| currentAccountCount: 1, | ||
| mergedAccountCount: 2, | ||
| imported: 1, | ||
| skipped: 0, | ||
| wouldExceedLimit: false, | ||
| valid: true, | ||
| error: undefined, | ||
| }; | ||
| listNamedBackupsMock.mockResolvedValue([assessment.backup]); | ||
| assessNamedBackupRestoreMock.mockResolvedValue(assessment); | ||
| loadAccountsMock.mockResolvedValue({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| accounts: [ | ||
| { | ||
| email: "settings@example.com", | ||
| accountId: "acc_settings", | ||
| refreshToken: "refresh-settings", | ||
| accessToken: "access-settings", | ||
| expiresAt: now + 3_600_000, | ||
| addedAt: now - 1_000, | ||
| lastUsed: now - 1_000, | ||
| enabled: true, | ||
| }, | ||
| ], | ||
| }); | ||
| promptLoginModeMock | ||
| .mockResolvedValueOnce({ mode: "restore-backup" }) | ||
| .mockResolvedValueOnce({ mode: "cancel" }); | ||
| selectMock.mockResolvedValueOnce({ type: "restore", assessment }); | ||
| const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); | ||
|
|
||
| const exitCode = await runCodexMultiAuthCli(["auth", "login"]); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| expect(listNamedBackupsMock).toHaveBeenCalledTimes(1); | ||
| expect(assessNamedBackupRestoreMock).toHaveBeenCalledWith( | ||
| "named-backup", | ||
| expect.objectContaining({ | ||
| currentStorage: expect.objectContaining({ | ||
| accounts: expect.any(Array), | ||
| }), | ||
| }), | ||
| ); | ||
| expect(restoreNamedBackupMock).toHaveBeenCalledWith("named-backup"); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# check whether restore manager path calls confirm
rg -n "runBackupRestoreManager|restoreNamedBackup|confirm\\(" lib/codex-manager.ts -C3
# check whether confirm is mocked/controlled in this test file
rg -n "vi\\.mock\\(\"../lib/ui/confirm\\.js\"|confirmMock|\\bconfirm\\b" test/codex-manager-cli.test.ts -C3
# show local restore test block for context
rg -n "restores a named backup from the login recovery menu" test/codex-manager-cli.test.ts -C25Repository: ndycode/codex-multi-auth
Length of output: 1295
🏁 Script executed:
# check imports and mocking setup in test file
head -100 test/codex-manager-cli.test.ts | grep -n "import\|vi.mock\|confirmMock"
# check if confirm is imported anywhere in test file
rg -n "confirm" test/codex-manager-cli.test.ts | head -20
# check if there's any test for declined confirmation
rg -n "does not restore|confirmation.*decline|confirm.*false" test/codex-manager-cli.test.ts
# check how other UI functions are mocked in test file
rg -n "vi.mock.*ui\|selectMock\|promptLoginModeMock" test/codex-manager-cli.test.ts | head -15Repository: ndycode/codex-multi-auth
Length of output: 484
🏁 Script executed:
# search entire test file for confirm.js mocking
rg -n "confirm" test/codex-manager-cli.test.ts | grep -E "mock|Mock"
# check all vi.mock calls for UI-related modules
rg -n "vi.mock.*ui" test/codex-manager-cli.test.ts
# get count of tests with "restore" in their name
rg -n "it\\(.*restore" test/codex-manager-cli.test.ts
# check if confirm function from lib is imported anywhere
rg -n "from.*confirm|import.*confirm" test/codex-manager-cli.test.ts
# check select and prompt mock setup to understand UI mocking pattern
rg -n "selectMock|promptLoginModeMock|restoreNamedBackupMock" test/codex-manager-cli.test.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 1101
🏁 Script executed:
# check all UI mocks in test file to understand pattern
rg -n "vi.mock.*lib/ui" test/codex-manager-cli.test.ts
# verify confirm.js exists as a real module
fd -e js "confirm" lib/
# check if confirm is used in codex-manager.ts
rg -n "import.*confirm|from.*confirm" lib/codex-manager.tsRepository: ndycode/codex-multi-auth
Length of output: 157
🏁 Script executed:
# check confirm.js implementation to ensure mock will work correctly
cat lib/ui/confirm.js
# verify the pattern of how other test files mock similar UI functions
rg -n "vi.mock.*ui/.*js" test/ -A2 | head -30
# check if there's a pattern for how mocked functions return values
rg -n "confirmMock|selectMock" test/codex-manager-cli.test.ts | grep -E "mockResolved|mockReturnValue" | head -10Repository: ndycode/codex-multi-auth
Length of output: 1150
🏁 Script executed:
# check if confirm is exported from somewhere else or if it's a different file
rg -n "export.*confirm" lib/ -t ts -t js
# find confirm implementation
fd "confirm" lib/ui/
# check the actual import in codex-manager.ts more carefully
rg -n "import.*confirm" lib/codex-manager.ts -A2 -B2
# check what auth-menu-hotkeys test looks like for confirmMock pattern
rg -n "confirmMock" test/auth-menu-hotkeys.test.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 623
🏁 Script executed:
# verify confirm is NOT mocked in codex-manager-cli.test.ts
grep -n "confirmMock" test/codex-manager-cli.test.ts
# verify there's no vi.mock for confirm.js in codex-manager-cli.test.ts
grep -n "vi.mock.*confirm" test/codex-manager-cli.test.ts
# check what the restore test currently asserts
sed -n '1636,1698p' test/codex-manager-cli.test.ts | grep -n "expect\|assert"Repository: ndycode/codex-multi-auth
Length of output: 414
mock confirm.js and add regression test for declined confirmation.
test/codex-manager-cli.test.ts doesn't mock ../lib/ui/confirm.js, so the restore test runs with real confirm() which makes it non-deterministic. lib/codex-manager.ts:4168-4171 gates restoreNamedBackup on the confirm result—this is critical flow control that needs explicit assertion and regression coverage.
add confirmMock following the pattern in test/auth-menu-hotkeys.test.ts:
- mock confirm.js at file top with
vi.fn(async () => true) - reset in beforeEach
- assert
expect(confirmMock).toHaveBeenCalled()in the restore happy path test - add regression test case where confirmMock resolves false and verify restoreNamedBackupMock is not called
violates coding guideline: "demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior"—at minimum this needs the declined confirmation regression case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` around lines 1636 - 1698, The test uses the
real confirm() making restore flow non-deterministic; mock ../lib/ui/confirm.js
with a confirmMock (vi.fn(async () => true)) at the top of
test/codex-manager-cli.test.ts, reset it in beforeEach, and update the existing
"restores a named backup..." test to assert
expect(confirmMock).toHaveBeenCalled() and that restoreNamedBackupMock was
invoked; then add a regression test where confirmMock resolves to false and
verify that restoreNamedBackupMock is NOT called (while listNamedBackupsMock and
assessNamedBackupRestoreMock still run) to cover the declined-confirmation
branch for runCodexMultiAuthCli -> restoreNamedBackupMock control flow.
Comment on lines
+330
to
+333
| afterEach(async () => { | ||
| setStoragePathDirect(null); | ||
| await fs.rm(testWorkDir, { recursive: true, force: true }); | ||
| }); |
Contributor
There was a problem hiding this comment.
make named-backup teardown resilient on windows to avoid flaky cleanup.
this teardown is single-attempt fs.rm, so transient eprem/ebusy/enotempty can leak temp dirs and cascade failures in later tests. reference: test/storage.test.ts:330-333.
proposed fix
afterEach(async () => {
setStoragePathDirect(null);
- await fs.rm(testWorkDir, { recursive: true, force: true });
+ 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),
+ );
+ }
+ }
});As per coding guidelines test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
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 for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 330 - 333, The afterEach teardown using a
single fs.rm call can flake on Windows due to transient EBUSY/EPERM/ENOTEMPTY
errors; update the afterEach (the teardown that calls setStoragePathDirect(null)
and fs.rm(testWorkDir, ...)) to perform a resilient removal: implement a short
retry loop with a few attempts and small delays (exponential backoff) catching
transient errors (EPERM, EBUSY, ENOTEMPTY) and retrying before ultimately
failing, or use a cross-platform recursive delete helper if available; ensure
retries only wrap the fs.rm call and still clear setStoragePathDirect(null)
immediately.
Comment on lines
+335
to
+403
| it("creates and lists named backups with metadata", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "acct-backup", | ||
| refreshToken: "ref-backup", | ||
| addedAt: 1, | ||
| lastUsed: 2, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| const backup = await createNamedBackup("My Backup 1"); | ||
| expect(backup.name).toBe("My-Backup-1"); | ||
| const backups = await listNamedBackups(); | ||
| expect(backups.length).toBeGreaterThan(0); | ||
| expect(backups[0]?.accountCount).toBe(1); | ||
| const backupPath = join(testWorkDir, "backups", `${backup.name}.json`); | ||
| expect(existsSync(backupPath)).toBe(true); | ||
| }); | ||
|
|
||
| it("assesses eligibility and restores a named backup", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "primary", | ||
| refreshToken: "ref-primary", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| await createNamedBackup("restore-me"); | ||
| await clearAccounts(); | ||
|
|
||
| const assessment = await assessNamedBackupRestore("restore-me"); | ||
| expect(assessment.valid).toBe(true); | ||
| expect(assessment.wouldExceedLimit).toBe(false); | ||
|
|
||
| const restoreResult = await restoreNamedBackup("restore-me"); | ||
| expect(restoreResult.total).toBe(1); | ||
|
|
||
| const restored = await loadAccounts(); | ||
| expect(restored?.accounts[0]?.accountId).toBe("primary"); | ||
| }); | ||
|
|
||
| it("rejects invalid backup names", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "primary", | ||
| refreshToken: "ref-primary", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
| await expect(createNamedBackup(" ")).rejects.toThrow( | ||
| /Invalid backup name/, | ||
| ); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
add regression tests for backup-restore concurrency and windows edge paths.
this block validates only happy-path restore and invalid names. it misses regressions for concurrent restore races, restore-limit failures, and windows-style filename/path behavior. reference: test/storage.test.ts:335-403.
As per coding guidelines test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 335 - 403, The test block only covers
happy-path backup/restore and invalid names; add deterministic vitest regression
tests that reproduce concurrency and edge-path failures by (1) adding a
concurrent-restore race test that spawns multiple parallel calls to
restoreNamedBackup and asserts deterministic outcome/no data corruption (use
Promise.all with controlled delays or mocked fs), (2) a restore-limit failure
test that seeds accounts up to the limit and verifies
assessNamedBackupRestore.wouldExceedLimit and restoreNamedBackup rejects or
returns the expected error/partial result, and (3) Windows-path filename
handling tests that create a backup name containing backslashes or reserved
chars and ensure createNamedBackup/listNamedBackups/store file names are
normalized (refer to createNamedBackup, listNamedBackups,
assessNamedBackupRestore, restoreNamedBackup, saveAccounts, clearAccounts,
loadAccounts to locate code under test); keep tests deterministic (avoid timers
unless mocked) and use vitest async utilities and filesystem mocking where
needed.
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
Productized the Settings hub with clear Everyday vs Advanced controls and shipped a guided backup-restore flow with a new Recovery menu and CLI, including a fallback “restore-backup” command for non-interactive prompts.
restore-backupCLI mode and fallback text commands (u,restore,backup,restore-backup).~/.codex/multi-auth/backups/<name>.json.Written for commit e251c79. 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
this pr productizes the settings hub (everyday vs advanced/operator split) and adds a named-backup restore flow —
listNamedBackups,createNamedBackup,assessNamedBackupRestore,restoreNamedBackupin storage, arunBackupRestoreManagerui in codex-manager, and arestore-backupcli mode. the bulk of the storage diff is whitespace reformatting. the feature itself is well-scoped: backup files get0o600mode, dedup runs before limit checks, and the WAL/rename path is reused for saves.issues found:
resolveNamedBackupPathprobesexistsSyncwith the raw (unnormalized)trimmedNamebefore sanitization. a crafted name like../../settings.jsonresolves outsidebackupsDirviapath.join.restoreNamedBackupandassessNamedBackupRestoreboth skip normalization before calling this helper —createNamedBackupnormalizes first but the other two don't. in the current call sites names come fromdirent.name(safe), but as exported api surface this is a defense gap.named backupsdescribe block intest/storage.test.tsadds its ownafterEachwith a barefs.rm, missing the 5-attempt exponential-backoff retry loop the outer describe added specifically for windowsEBUSY/EPERMon freshly-written json files. this will flake on windows ci.ensureNamedBackupsDirectorycreates the backups folder with no explicitmode, so the directory may be world-readable on unix (0o755with typical umask) even though individual files are0o600. addingmode: 0o700closes that gap at no cost on windows.runBackupRestoreManagercalls the newly-importedconfirm()beforerestoreNamedBackup; the test for the restore flow expectsrestoreNamedBackupMockto be called, which requiresconfirmto returntrue. novi.mock("../lib/ui/confirm.js")appears in the pr diff — worth confirming the mock already exists in the test file.Confidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant AuthMenu participant BackupManager as runBackupRestoreManager participant Storage as storage.ts participant FS as Filesystem User->>AuthMenu: select "Restore From Backup" AuthMenu->>BackupManager: runBackupRestoreManager(displaySettings) BackupManager->>Storage: listNamedBackups() Storage->>FS: readdir(backupsDir) loop each .json file FS-->>Storage: dirent Storage->>FS: readFile(path) [loadBackupCandidate] Storage->>FS: stat(path) Storage-->>Storage: NamedBackupMetadata end Storage-->>BackupManager: NamedBackupMetadata[] BackupManager->>Storage: loadAccounts() Storage->>FS: readFile(accounts.json) Storage-->>BackupManager: currentStorage loop each backup BackupManager->>Storage: assessNamedBackupRestore(name, {currentStorage}) Storage->>FS: readFile(backup.json) [loadBackupCandidate — 2nd read] Storage->>FS: stat(backup.json) [2nd stat] Storage-->>BackupManager: BackupRestoreAssessment end BackupManager->>User: show select menu User->>BackupManager: pick backup BackupManager->>User: confirm(message) User->>BackupManager: confirmed = true BackupManager->>Storage: restoreNamedBackup(name) Storage->>Storage: resolveNamedBackupPath(name) Storage->>Storage: importAccounts(backupPath) Storage->>FS: readFile(backup.json) Storage->>FS: writeFile(accounts.json via WAL+rename) Storage-->>BackupManager: {imported, skipped, total} BackupManager->>User: print resultComments Outside Diff (2)
test/codex-manager-cli.test.ts, line 2963-2982 (link)confirmis not mocked — test may hang or failrunBackupRestoreManagernow callsconfirm(confirmMessage)(imported from./ui/confirm.js) between the backup menu selection and therestoreNamedBackupMockcall. the diff doesn't add avi.mock("../lib/ui/confirm.js", ...)entry, and i don't seeconfirmMockin the existing test setup.if
confirmtries to read from stdin in a non-interactive test environment it will either block indefinitely or throw. the test expectsrestoreNamedBackupMockto have been called, which only happens ifconfirmreturnstrue.you need something like:
without this the test is non-deterministic.
Prompt To Fix With AI
test/storage.test.ts, line 3153-3156 (link)named backup
afterEachmissing windows-safe retry loopthe outer export/import describe block added a 5-attempt retry with exponential backoff for
EBUSY/EPERM/ENOTEMPTYon windows (lines 3116-3132), but this inner named-backupsafterEachdoes a barefs.rmwith no retry. the backup json files were just written and stat'd, so windows antivirus will hold a read lock on them. this will flake on windows ci.Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: e251c79
Context used: