Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
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:
📝 WalkthroughWalkthroughintroduces named backups and an interactive restore workflow via a new Changes
Sequence Diagram(s)sequenceDiagram
actor user
participant cli as "cli / lib/codex-manager.ts"
participant ui as "ui / lib/ui/*"
participant storage as "storage / lib/storage.ts"
participant sync as "codex sync"
user->>cli: trigger restore-backup
cli->>storage: listNamedBackups()
storage-->>cli: backup metadata list
cli->>ui: render backup menu
user->>ui: select backup
cli->>storage: assessNamedBackupRestore(name)
storage-->>cli: assessment (eligible, impact, errors)
cli->>ui: show assessment & confirm
user->>ui: confirm restore
cli->>storage: restoreAssessedNamedBackup(assessment)
storage-->>cli: ImportAccountsResult { imported, skipped, total, changed? }
alt sync enabled
cli->>sync: attempt sync
sync-->>cli: success or error
end
cli->>ui: show result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes flags:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a “backup restore manager” flow to the auth CLI, including a new direct codex auth restore-backup entrypoint, named-backup discovery/assessment/restore logic, and associated docs/tests.
Changes:
- Introduces named-backup listing/assessment/restore APIs in
lib/storage.ts(plus containment/path validation and transient FS retry handling). - Adds UI/menu & CLI wiring for “Restore From Backup” (interactive login menu + direct command).
- Expands
importAccounts()runtime result with achangedflag and updates tests/docs accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/storage.test.ts | Adds coverage for import/export edge-cases and extensive named-backup restore scenarios (containment, transient FS errors, concurrency). |
| test/codex-manager-cli.test.ts | Adds CLI-level tests for interactive restore menu + direct restore command behavior and error messaging. |
| test/cli.test.ts | Verifies fallback input aliases map to the new restore-backup mode. |
| lib/ui/copy.ts | Adds UI copy for recovery/restore prompts and updates fallback prompt text to include restore. |
| lib/ui/auth-menu.ts | Adds a new “Recovery” section with “Restore From Backup” menu item. |
| lib/storage.ts | Implements named-backup metadata, listing, containment validation, restore assessment/execution, import result changed, and transient FS retries. |
| lib/codex-manager.ts | Wires restore manager into auth login flow and adds codex auth restore-backup command + interactive restore picker. |
| lib/cli.ts | Extends login modes and fallback readline input parsing to support restore-backup. |
| docs/reference/storage-paths.md | Documents named-backup paths and restore workflow/entrypoint. |
| docs/reference/public-api.md | Documents the additive changed field on importAccounts() results. |
| docs/reference/commands.md | Adds codex auth restore-backup to command reference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/cli.ts
Outdated
| if ( | ||
| normalized === "u" || | ||
| normalized === "backup" || | ||
| normalized === "restore" || | ||
| normalized === "restore-backup" | ||
| ) { | ||
| return { mode: "restore-backup" }; | ||
| } |
| const ui = getUiRuntimeOptions(); | ||
| const selection = await select(items, { | ||
| message: "Restore From Backup", | ||
| subtitle: backupDir, | ||
| help: UI_COPY.mainMenu.helpCompact, | ||
| clearScreen: true, | ||
| selectedEmphasis: "minimal", | ||
| focusStyle: displaySettings.menuFocusStyle ?? "row-invert", | ||
| theme: ui.theme, | ||
| }); |
| try { | ||
| builtPath = buildNamedBackupPath(requested); | ||
| } catch (error) { | ||
| // buildNamedBackupPath rejects names with special characters even when the | ||
| // requested backup name is a plain filename inside the backups directory. | ||
| // In that case, reporting ENOENT is clearer than surfacing the filename | ||
| // validator, but only when no separator/traversal token is present. | ||
| if ( | ||
| requested.length > 0 && | ||
| basename(requestedWithExtension) === requestedWithExtension && | ||
| !requestedWithExtension.includes("..") && | ||
| !/^[A-Za-z0-9_-]+$/.test(baseName) | ||
| ) { | ||
| throw new Error( | ||
| `Import file not found: ${resolvePath(join(backupRoot, requestedWithExtension))}`, | ||
| ); | ||
| } | ||
| throw error; | ||
| } |
| function stableStringifyForComparison(value: unknown): string { | ||
| return JSON.stringify(canonicalizeComparisonValue(value)); | ||
| } | ||
|
|
||
| function haveEquivalentAccountRows( | ||
| left: readonly unknown[], | ||
| right: readonly unknown[], | ||
| ): boolean { | ||
| // deduplicateAccounts() keeps the last occurrence of duplicates, so incoming | ||
| // rows win when we compare merged restore data against the current snapshot. | ||
| // That keeps index-aligned comparison correct for restore no-op detection. | ||
| if (left.length !== right.length) { | ||
| return false; | ||
| } | ||
| for (let index = 0; index < left.length; index += 1) { | ||
| if ( | ||
| stableStringifyForComparison(left[index]) !== | ||
| stableStringifyForComparison(right[index]) | ||
| ) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
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)
154-156:⚠️ Potential issue | 🟡 Minorfallback prompt lists restore-backup option but handler is missing.
the fallback prompt at
lib/ui/copy.ts:155shows(u) restore backupbutpromptLoginModeFallbackin this file (lines 174-244) doesn't handle theuinput. the tty-basedshowAuthMenupath at line 291-292 handles it, but non-tty users see the option without being able to use it.either add a handler for
uhere or remove the(u)option from the fallback copy.proposed fix to add handler
if (normalized === "q" || normalized === "quit") return { mode: "cancel" }; + if (normalized === "u" || normalized === "restore-backup" || normalized === "restore") { + return { mode: "restore-backup" }; + } console.log(UI_COPY.fallback.invalidModePrompt);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/cli.ts` around lines 154 - 156, The fallback prompt presents a "(u) restore backup" option but promptLoginModeFallback does not handle input "u", causing non-TTY users to see an unavailable choice; update promptLoginModeFallback (the function that parses the fallback input) to recognize "u" (and "U") and route it to the same restore-backup handler used by the tty path (the same code invoked by showAuthMenu for the restore action), or alternatively remove the "(u) restore backup" option from the fallback prompt in lib/ui/copy.ts so the option is not shown to non-tty users; ensure you reference and reuse the existing restore logic rather than duplicating behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 4430-4438: The restore error message should call out
Windows-specific EPERM/EBUSY transient-file-lock behavior and point users to the
retry logic in storage.ts (retryTransientFilesystemOperation) or troubleshooting
docs; update the catch block around collapseWhitespace/error handling in
codex-manager (the restore failure message) to append a parenthetical note like
"on Windows this may be caused by EPERM/EBUSY file locks — try closing other
Codex instances or see [troubleshooting]" and/or include a link to the existing
docs that explain retryTransientFilesystemOperation for Windows users.
- Around line 4394-4397: Update the boolean guard around the import result to be
explicit about the optional ImportAccountsResult.changed field: instead of
relying on falsy semantics (as seen in the block that logs "All accounts in this
backup already exist"), check the value explicitly (e.g., test result.changed
=== false or result.changed !== true depending on intended semantics) so
TypeScript strictNullChecks won't treat undefined the same as false; refer to
the ImportAccountsResult.changed definition (lib/storage.ts:1922) and update the
conditional in the function/method in codex-manager.ts where result.changed is
used.
- Around line 4228-4440: Add two regression tests exercising
runBackupRestoreManager: one where listNamedBackups returns an empty array to
hit the "No named backups found" branch (assert console.log contains the
backupDir message) and another where a successful restore path is followed but
autoSyncActiveAccountToCodex is mocked to throw or return false so the restore
branch logs the expected warning; stub/mocks should target listNamedBackups,
assessNamedBackupRestore/restoreAssessedNamedBackup (to simulate a restorable
backup), confirm (auto-confirm), and autoSyncActiveAccountToCodex, and your
tests should run the CLI entry that calls runBackupRestoreManager and assert the
appropriate console output was produced for both the empty-list and sync-failure
cases.
In `@lib/storage.ts`:
- Around line 1971-1978: namedBackupContainmentFs is exported as a live mutable
singleton (used where the module hands it out) allowing test or external code to
monkey-patch lstat/realpath and weaken containment; change the API to stop
exposing the raw object: implement and export either (a) a factory that returns
a fresh cloned facade object with lstat/realpath bound to
lstatSync/realpathSync.native, or (b) explicit setter/reset hooks (e.g.,
setBackupContainmentFs and resetBackupContainmentFs) that control a private
internal instance; update the code paths that currently return
namedBackupContainmentFs to call the factory or use the controlled accessor so
callers cannot mutate the module's live functions directly (refer to
namedBackupContainmentFs and the site that currently returns it).
- Around line 1721-1739: The sort comparator in backups.sort uses only computed
leftTime/rightTime so equal (including "unknown") mtimes are non-deterministic;
modify the comparator used in lib/storage.ts (the backups.sort closure
referencing left.updatedAt and right.updatedAt) to add a deterministic
tie-breaker (e.g., compare a stable key such as backup.id, backup.name, or
backup.path) when rightTime === leftTime so order is stable across filesystems;
update/extend the related vitest (test/codex-manager-cli.test.ts) to assert the
new deterministic ordering and mention the affected test ranges, and ensure the
change follows lib/** guidelines (auth rotation, Windows IO/concurrency
concerns) and that any new queuing behavior handles EBUSY/429 scenarios in
tests.
- Around line 2184-2215: Add regression tests in the CLI restore test suite that
call the restore flow using both a bare backup name (e.g., "named-backup") and
the dotted form ("named-backup.json") to exercise resolveNamedBackupRestorePath;
assert it resolves to the same valid path and that invalid dotted names are
rejected the same way as normalizeNamedBackupFileName enforces (cover allowed
chars and rejection of special chars/traversal). Specifically, add tests that
invoke the code path which uses resolveNamedBackupRestorePath (so
buildNamedBackupPath and normalizeNamedBackupFileName validations run) and
assert successful resolution for valid base names with and without the ".json"
suffix and proper ENOENT-style error behavior for invalid dotted names.
In `@test/cli.test.ts`:
- Around line 707-735: The test currently only checks that promptLoginMode([{
index: 0 }]) eventually returns { mode: "cancel" } after a second "quit" prompt,
which lets alias inputs (u/restore/backup/restore-backup) hide behind the
generic invalid-input loop; update the assertions so each alias-specific case
asserts the branch-specific rejection returned by promptLoginMode (for the
inputs "u", "restore", "backup", "restore-backup") instead of only the final
cancel — e.g. after calling promptLoginMode with mockRl.question mocked to
return the alias then "quit", assert the resolved value contains the
alias-rejection indicator (a specific property or message returned by
promptLoginMode that states the alias requires the interactive backup browser),
using promptLoginMode and mockRl.question to locate the behavior and ensuring
each alias is checked for that exact rejection text/property rather than only {
mode: "cancel" }.
In `@test/codex-manager-cli.test.ts`:
- Around line 4046-4066: The test currently only verifies
runNamedBackupExportMock was called with "../bad-name" but not that the CLI
rejected the invalid/colliding name; update the "rejects invalid or colliding
experimental backup filenames" test to assert the user-visible rejection or
collision outcome by checking one or more of: the CLI exit code reflects failure
(e.g., expect(exitCode).not.toBe(0) or specific code), a user-facing message was
printed via the logger
(expect(mockLogger.error/print).toHaveBeenCalledWith(expect.stringContaining("invalid")
or "collision")), or that the prompt flow continued/re-prompted
(expect(promptQuestionMock).toHaveBeenCalledTimes(>1) or
selectSequence.remaining() changed accordingly). Use the existing symbols
promptQuestionMock, runNamedBackupExportMock, runCodexMultiAuthCli,
selectSequence to locate where to add these assertions and make the test
deterministic under Vitest.
In `@test/storage.test.ts`:
- Around line 2159-2190: The test mixes two conflicting contracts:
assessNamedBackupRestore("../escaped-entry") and
restoreNamedBackup("../escaped-entry") are already rejected by the
name/separator validator, so the mocked readdir branch with escapedEntry is
never exercised; fix by either (A) removing the high-level assertions against
assessNamedBackupRestore and restoreNamedBackup and only asserting that the
readdir-mocked code path triggers an "escapes backup directory" error (i.e.,
keep the readdirSpy and test the lower-level containment behavior), or (B)
change the test input to a name that passes the separator validator (e.g.,
"escaped-entry" or another valid token) so that assessNamedBackupRestore and
restoreNamedBackup reach the readdir logic and then assert they throw the
"escapes backup directory" error; reference assessNamedBackupRestore,
restoreNamedBackup, readdirSpy and escapedEntry to locate the lines to modify.
- Around line 2060-2088: Add a deterministic regression test that reproduces the
stale-assessment TOCTOU on the public API by concurrently invoking
restoreNamedBackup (not just restoreAssessedNamedBackup) so the code path that
re-assesses inside restoreNamedBackup is exercised; specifically, create a named
backup via createNamedBackup after saving accounts up to
ACCOUNT_LIMITS.MAX_ACCOUNTS - 1, then spawn two parallel flows that call
restoreNamedBackup for the same name (one should succeed and change storage, the
other must detect the changed state and behave as a no-op/fail
deterministically), use the existing helpers saveAccounts,
assessNamedBackupRestore, removeWithRetry and loadAccounts to assert
post-conditions, implement synchronization (promises/await barriers) to force
the race deterministically under vitest, and assert the second call does not
cause account overflow or leave partial state (e.g., expect appropriate throw or
no-op and that loadAccounts shows correct account count).
- Around line 2447-2501: The test currently fabricates a Dirent rather than
creating a real symlink/junction, so lib functions listNamedBackups,
assessNamedBackupRestore, and restoreNamedBackup never exercise the
lstat/realpath containment checks; replace the Dirent mock with an actual
filesystem symlink (use fs.symlink or on Windows create a junction) named
"escaped-link.json" inside the backup root that points to the
externalBackupPath, run the three calls (listNamedBackups,
assessNamedBackupRestore, restoreNamedBackup) and assert they reject as before,
and ensure proper cleanup of the symlink in a finally block so the test is
deterministic and triggers the real lstat/realpath logic in lib/storage.ts.
---
Outside diff comments:
In `@lib/cli.ts`:
- Around line 154-156: The fallback prompt presents a "(u) restore backup"
option but promptLoginModeFallback does not handle input "u", causing non-TTY
users to see an unavailable choice; update promptLoginModeFallback (the function
that parses the fallback input) to recognize "u" (and "U") and route it to the
same restore-backup handler used by the tty path (the same code invoked by
showAuthMenu for the restore action), or alternatively remove the "(u) restore
backup" option from the fallback prompt in lib/ui/copy.ts so the option is not
shown to non-tty users; ensure you reference and reuse the existing restore
logic rather than duplicating behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e768d2ed-7019-473a-adbd-03f461773e60
📒 Files selected for processing (11)
docs/reference/commands.mddocs/reference/public-api.mddocs/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)
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
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/commands.mddocs/reference/storage-paths.mddocs/reference/public-api.md
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
🔇 Additional comments (16)
docs/reference/commands.md (1)
29-29: documentation looks good for the new restore-backup command.the command entry at line 29 and workflow example at line 115 are consistent with the implementation in
lib/codex-manager.ts:4499-4512. no flags are documented, which matches the implementation that takes no arguments.Also applies to: 115-115
lib/cli.ts (1)
291-292: switch case for restore-backup is correct.the handler properly returns
{ mode: "restore-backup" }matching theLoginMenuResultinterface. wiring is complete withlib/ui/auth-menu.ts:67and consumed atlib/codex-manager.ts:3927.docs/reference/public-api.md (1)
40-44: api contract documentation is accurate.the additive compatibility note correctly documents that
changedis optional in the type (seelib/storage.ts:1916-1923) while runtime always includes it. this preserves backward compatibility for callers modeling the legacy{ imported, total, skipped }shape.docs/reference/storage-paths.md (1)
25-25: storage paths documentation is complete and accurate.the named backup paths at lines 25 and 60 match the implementation. the restore workflow at lines 105-114 correctly documents both the dashboard flow and the direct
codex auth restore-backupentrypoint wired atlib/codex-manager.ts:4499.Also applies to: 60-60, 105-114
lib/ui/copy.ts (2)
17-37: ui copy for restore-backup flows is well-structured.the conditional message in
restoreBackupConfirm(lines 26-28) clearly distinguishes metadata-only refreshes (netNewAccounts === 0) from actual imports. parameter order matches the call site atlib/codex-manager.ts:4382-4388.
155-156: fallback prompt updated but handler missing in cli.ts.the
(u) restore backupoption is now listed in the prompt, but the corresponding input handler inlib/cli.ts:174-244(promptLoginModeFallback) doesn't parseuinput. see related comment onlib/cli.ts.lib/ui/auth-menu.ts (2)
67-67: auth menu action type and focus key handler are correct.the
restore-backupvariant at line 67 aligns withLoginModeinlib/cli.ts:60. the focus key handler at line 521 follows the existing pattern for action-type keys.Also applies to: 521-521
660-670: recovery section placement and styling look good.placing the restore-backup item in a dedicated "Recovery" section between accounts and danger zone is sensible ux. the yellow color appropriately signals a cautious action without being as alarming as danger-zone red.
lib/codex-manager.ts (8)
141-152: formatRelativeDateShort handles edge cases correctly.the helper properly guards against
null,undefined,0, and non-finite values at lines 144-146. the date formatting logic is straightforward and consistent with similar helpers elsewhere.
4231-4236: non-interactive mode guard is correct and necessary.blocking restore in non-interactive mode (
isNonInteractiveMode()) at line 4231 prevents automated or ci-triggered restores without user confirmation. this aligns with the pr objective "block non-interactive backup restore mode".
4270-4304: chunked assessment with Promise.allSettled is resilient.using
Promise.allSettledat line 4276 ensures one failing assessment doesn't abort the entire list. containment errors are correctly re-thrown at line 4290 since they indicate a security boundary violation. the concurrency limit fromNAMED_BACKUP_ASSESS_CONCURRENCYprevents overwhelming the filesystem.
4363-4378: re-assessment before restore mitigates toctou race.the re-assessment at lines 4365-4368 checks current storage state immediately before restore, addressing the time-of-check-to-time-of-use concern noted in pr objectives. if eligibility changed between menu selection and confirmation, the user sees the updated error at line 4377.
4414-4428: codex cli sync failures are appropriately handled as warnings.the sync at line 4415 is wrapped in try/catch and failures are logged as warnings rather than errors. this is correct since the restore itself succeeded - sync failure is a degraded state, not a fatal error.
4499-4512: cli command handler is correctly wired.the
restore-backupcommand at line 4499 follows the same pattern as other commands: set storage path, run manager, handle errors. exit codes are correct (0 for success, 1 for failure).
3881-3892: restore-backup correctly drains pending quota refresh.adding
restore-backuptomodeRequiresDrainedQuotaRefreshat line 3886 ensures the pending quota cache refresh completes before attempting restore. this prevents potential race conditions where restore modifies account storage while quota refresh is still reading it.
4228-4440: no sensitive data leakage in restore manager logging.reviewed all console output in
runBackupRestoreManager: only backup names, account counts, and sanitized error messages are logged. no tokens, emails, or other pii are exposed.
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| const collapsedMessage = collapseWhitespace(message) || "unknown error"; | ||
| console.error( | ||
| /exceed maximum/i.test(collapsedMessage) | ||
| ? `Restore failed: ${collapsedMessage}. Close other Codex instances and try again.` | ||
| : `Restore failed: ${collapsedMessage}`, | ||
| ); | ||
| return false; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
error message for concurrent access could mention windows specifically.
the "Close other Codex instances" suggestion at line 4435 is helpful but doesn't explain windows-specific EPERM/EBUSY behavior. per pr notes, retryTransientFilesystemOperation handles windows retry in storage.ts. consider expanding the message for windows users or linking to troubleshooting docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4430 - 4438, The restore error message
should call out Windows-specific EPERM/EBUSY transient-file-lock behavior and
point users to the retry logic in storage.ts (retryTransientFilesystemOperation)
or troubleshooting docs; update the catch block around collapseWhitespace/error
handling in codex-manager (the restore failure message) to append a
parenthetical note like "on Windows this may be caused by EPERM/EBUSY file locks
— try closing other Codex instances or see [troubleshooting]" and/or include a
link to the existing docs that explain retryTransientFilesystemOperation for
Windows users.
| const namedBackupContainmentFs = { | ||
| lstat(path: string) { | ||
| return lstatSync(path); | ||
| }, | ||
| realpath(path: string) { | ||
| return realpathSync.native(path); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
don't export the live containment fs singleton.
lib/storage.ts:1971-1978 is handed out directly through lib/storage.ts:3189-3191. any vitest or internal caller can patch lstat or realpath and silently weaken containment checks for later restores in the same process. that can bleed across the containment regressions in test/codex-manager-cli.test.ts:2672-2738. expose explicit set/reset hooks or a cloned facade instead of the mutable object itself.
Also applies to: 3189-3191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 1971 - 1978, namedBackupContainmentFs is
exported as a live mutable singleton (used where the module hands it out)
allowing test or external code to monkey-patch lstat/realpath and weaken
containment; change the API to stop exposing the raw object: implement and
export either (a) a factory that returns a fresh cloned facade object with
lstat/realpath bound to lstatSync/realpathSync.native, or (b) explicit
setter/reset hooks (e.g., setBackupContainmentFs and resetBackupContainmentFs)
that control a private internal instance; update the code paths that currently
return namedBackupContainmentFs to call the factory or use the controlled
accessor so callers cannot mutate the module's live functions directly (refer to
namedBackupContainmentFs and the site that currently returns it).
| it("rejects invalid or colliding experimental backup filenames", async () => { | ||
| const now = Date.now(); | ||
| setupInteractiveSettingsLogin(createSettingsStorage(now)); | ||
| promptQuestionMock.mockResolvedValueOnce("../bad-name"); | ||
| runNamedBackupExportMock.mockResolvedValueOnce({ kind: "collision", path: "/mock/backups/bad-name.json" }); | ||
| const selectSequence = queueSettingsSelectSequence([ | ||
| { type: "experimental" }, | ||
| { type: "backup" }, | ||
| { type: "back" }, | ||
| { type: "back" }, | ||
| { type: "back" }, | ||
| ]); | ||
|
|
||
| const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); | ||
| const exitCode = await runCodexMultiAuthCli(["auth", "login"]); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| expect(selectSequence.remaining()).toBe(0); | ||
| expect(promptQuestionMock).toHaveBeenCalledOnce(); | ||
| expect(runNamedBackupExportMock).toHaveBeenCalledWith({ name: "../bad-name" }); | ||
| }); |
There was a problem hiding this comment.
this doesn't verify the rejection path yet.
test/codex-manager-cli.test.ts:4046-4066 only proves that runNamedBackupExportMock was called with ../bad-name. it still passes if the ui accepts the invalid name, swallows the collision warning, or exits the flow early. assert the user-visible rejection or collision outcome as well.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
🤖 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 4046 - 4066, The test currently
only verifies runNamedBackupExportMock was called with "../bad-name" but not
that the CLI rejected the invalid/colliding name; update the "rejects invalid or
colliding experimental backup filenames" test to assert the user-visible
rejection or collision outcome by checking one or more of: the CLI exit code
reflects failure (e.g., expect(exitCode).not.toBe(0) or specific code), a
user-facing message was printed via the logger
(expect(mockLogger.error/print).toHaveBeenCalledWith(expect.stringContaining("invalid")
or "collision")), or that the prompt flow continued/re-prompted
(expect(promptQuestionMock).toHaveBeenCalledTimes(>1) or
selectSequence.remaining() changed accordingly). Use the existing symbols
promptQuestionMock, runNamedBackupExportMock, runCodexMultiAuthCli,
selectSequence to locate where to add these assertions and make the test
deterministic under Vitest.
test/storage.test.ts
Outdated
| it("re-resolves an assessed named backup before the final import", async () => { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "deleted-helper", | ||
| refreshToken: "ref-deleted-helper", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| const backup = await createNamedBackup("deleted-helper-assessment"); | ||
| await clearAccounts(); | ||
|
|
||
| const assessment = await assessNamedBackupRestore( | ||
| "deleted-helper-assessment", | ||
| ); | ||
| expect(assessment.eligibleForRestore).toBe(true); | ||
|
|
||
| await removeWithRetry(backup.path, { force: true }); | ||
|
|
||
| await expect(restoreAssessedNamedBackup(assessment)).rejects.toThrow( | ||
| /Import file not found/, | ||
| ); | ||
| expect((await loadAccounts())?.accounts ?? []).toHaveLength(0); | ||
| }); |
There was a problem hiding this comment.
the stale-assessment race is still untested on the public restore api.
test/storage.test.ts:2060-2088 only covers restoreAssessedNamedBackup() when the backup file disappears. the concurrency regression in test/storage.test.ts:3214-3283 races restoreNamedBackup(), which re-assesses internally. that leaves the real toctou surface unpinned: two precomputed assessments can both look eligible at ACCOUNT_LIMITS.MAX_ACCOUNTS - 1, then one import changes storage before the second restoreAssessedNamedBackup() runs. add that regression so the mutex/no-op behavior is locked down on the exported api, not just the wrapper.
As per coding guidelines, "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions."
Also applies to: 3214-3283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 2060 - 2088, Add a deterministic
regression test that reproduces the stale-assessment TOCTOU on the public API by
concurrently invoking restoreNamedBackup (not just restoreAssessedNamedBackup)
so the code path that re-assesses inside restoreNamedBackup is exercised;
specifically, create a named backup via createNamedBackup after saving accounts
up to ACCOUNT_LIMITS.MAX_ACCOUNTS - 1, then spawn two parallel flows that call
restoreNamedBackup for the same name (one should succeed and change storage, the
other must detect the changed state and behave as a no-op/fail
deterministically), use the existing helpers saveAccounts,
assessNamedBackupRestore, removeWithRetry and loadAccounts to assert
post-conditions, implement synchronization (promises/await barriers) to force
the race deterministically under vitest, and assert the second call does not
cause account overflow or leave partial state (e.g., expect appropriate throw or
no-op and that loadAccounts shows correct account count).
| it("ignores symlink-like named backup entries that point outside the backups root", async () => { | ||
| const backupRoot = join(dirname(testStoragePath), "backups"); | ||
| const externalBackupPath = join(testWorkDir, "outside-backup.json"); | ||
| await fs.mkdir(backupRoot, { recursive: true }); | ||
| await fs.writeFile( | ||
| externalBackupPath, | ||
| JSON.stringify({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "outside-manual-backup", | ||
| refreshToken: "ref-outside-manual-backup", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }), | ||
| "utf-8", | ||
| ); | ||
|
|
||
| const originalReaddir = fs.readdir.bind(fs); | ||
| const readdirSpy = vi.spyOn(fs, "readdir"); | ||
| const escapedEntry = { | ||
| name: "escaped-link.json", | ||
| isFile: () => false, | ||
| isSymbolicLink: () => true, | ||
| } as unknown as Awaited< | ||
| ReturnType<typeof fs.readdir> | ||
| >[number]; | ||
| readdirSpy.mockImplementation(async (...args) => { | ||
| const [path, options] = args; | ||
| if ( | ||
| String(path) === backupRoot && | ||
| typeof options === "object" && | ||
| options?.withFileTypes === true | ||
| ) { | ||
| return [escapedEntry] as Awaited<ReturnType<typeof fs.readdir>>; | ||
| } | ||
| return originalReaddir(...(args as Parameters<typeof fs.readdir>)); | ||
| }); | ||
|
|
||
| try { | ||
| const backups = await listNamedBackups(); | ||
| expect(backups).toEqual([]); | ||
| await expect(assessNamedBackupRestore("escaped-link")).rejects.toThrow( | ||
| /not a regular backup file/i, | ||
| ); | ||
| await expect(restoreNamedBackup("escaped-link")).rejects.toThrow( | ||
| /not a regular backup file/i, | ||
| ); | ||
| } finally { | ||
| readdirSpy.mockRestore(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
this does not reproduce the symlink escape you are naming here.
test/storage.test.ts:2449-2466 creates externalBackupPath, but test/storage.test.ts:2470-2487 never links it into backupRoot; the test only feeds a fabricated Dirent with isSymbolicLink() === true. that misses the real lstat/realpath containment path in lib/storage.ts:2126-2160, and it will not catch win32 junction regressions. please drive this through an actual symlink/junction under backupRoot and assert listNamedBackups, assessNamedBackupRestore, and restoreNamedBackup reject it end to end.
As per coding guidelines, "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage.test.ts` around lines 2447 - 2501, The test currently fabricates
a Dirent rather than creating a real symlink/junction, so lib functions
listNamedBackups, assessNamedBackupRestore, and restoreNamedBackup never
exercise the lstat/realpath containment checks; replace the Dirent mock with an
actual filesystem symlink (use fs.symlink or on Windows create a junction) named
"escaped-link.json" inside the backup root that points to the
externalBackupPath, run the three calls (listNamedBackups,
assessNamedBackupRestore, restoreNamedBackup) and assert they reject as before,
and ensure proper cleanup of the symlink in a finally block so the test is
deterministic and triggers the real lstat/realpath logic in lib/storage.ts.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/codex-manager-cli.test.ts (1)
4133-4153:⚠️ Potential issue | 🟡 Minortest still doesn't verify the rejection outcome.
this test only asserts that
runNamedBackupExportMockwas called with../bad-name(line 4152). it passes even if the ui swallows thecollisionresult silently or exits without user feedback. add an assertion for the user-visible outcome:
- assert that a warning/error was logged (e.g.,
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("collision")))- or verify the prompt re-prompts / flow continues appropriately
as per coding guidelines:
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🛠️ suggested fix
it("rejects invalid or colliding experimental backup filenames", async () => { const now = Date.now(); setupInteractiveSettingsLogin(createSettingsStorage(now)); promptQuestionMock.mockResolvedValueOnce("../bad-name"); runNamedBackupExportMock.mockResolvedValueOnce({ kind: "collision", path: "/mock/backups/bad-name.json" }); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const selectSequence = queueSettingsSelectSequence([ { type: "experimental" }, { type: "backup" }, { type: "back" }, { type: "back" }, { type: "back" }, ]); - const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); - const exitCode = await runCodexMultiAuthCli(["auth", "login"]); + try { + const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); + const exitCode = await runCodexMultiAuthCli(["auth", "login"]); - expect(exitCode).toBe(0); - expect(selectSequence.remaining()).toBe(0); - expect(promptQuestionMock).toHaveBeenCalledOnce(); - expect(runNamedBackupExportMock).toHaveBeenCalledWith({ name: "../bad-name" }); + expect(exitCode).toBe(0); + expect(selectSequence.remaining()).toBe(0); + expect(promptQuestionMock).toHaveBeenCalledOnce(); + expect(runNamedBackupExportMock).toHaveBeenCalledWith({ name: "../bad-name" }); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("collision"), + ); + } finally { + warnSpy.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 4133 - 4153, The test "rejects invalid or colliding experimental backup filenames" currently only verifies runNamedBackupExportMock was called but not that the UI handled the "collision" result; update the test to assert the user-visible outcome by adding a check that the warning logger (e.g., warnSpy) was called with a message containing "collision" or that the prompt re-ran (e.g., promptQuestionMock called more than once or selectSequence progressed accordingly); reference runNamedBackupExportMock and promptQuestionMock in the new assertion so the test deterministically verifies the collision path and user feedback.
🤖 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/storage.ts`:
- Around line 2077-2165: resolvePathForNamedBackupContainment and
assertNamedBackupRestorePath currently convert EPERM/EBUSY into
BackupPathValidationTransientError but are not retried like
readdir/readFile/stat, causing one-off Windows AV/lock failures to surface as
hard errors; wrap the containment validation calls in the same retry/backoff
helper used for directory reads (the retry used around readdir/readFile/stat) or
refactor resolvePathForNamedBackupContainment to call that helper for
lstat/realpath operations and ensure assertNamedBackupRestorePath invokes the
retryable variant; add a vitest regression that simulates a one-shot EBUSY/EPERM
on lstat/realpath during a list+restore flow to assert the operation succeeds
after a single retry and update related tests to reference the new retryable
containment functions.
- Around line 1882-1900: The current restoreAssessedNamedBackup re-resolves the
backup by name, creating a TOCTOU risk; change its API and callers so
restoreAssessedNamedBackup receives the assessed backup path and a stable
fingerprint (use fields from BackupRestoreAssessment like backup.path and a
digest/timestamp from the assessment) and call importAccounts with that resolved
path instead of calling resolveNamedBackupRestorePath again; update
restoreNamedBackup to forward assessNamedBackupRestore()'s backup.path and
fingerprint into restoreAssessedNamedBackup, add a vitest that performs an
assessment, swaps the file contents (simulate replace) before restore and
asserts the fingerprint prevents restoring the new content, and ensure the
change accounts for Windows EBUSY and concurrency by retrying path reads on
transient errors (EBUSY/429) per IO retry policy.
In `@test/codex-manager-cli.test.ts`:
- Around line 530-567: The default mock setup creates an implicit dependency
chain where restoreAssessedNamedBackupMock calls
resolveNamedBackupRestorePathMock and then importAccountsMock (see
restoreAssessedNamedBackupMock, resolveNamedBackupRestorePathMock,
importAccountsMock and assessNamedBackupRestoreMock); add a brief inline comment
next to this mock implementation explaining that restoreAssessedNamedBackupMock
delegates to importAccountsMock via resolveNamedBackupRestorePathMock and that
some tests override importAccountsMock directly (so they depend on this chain),
so maintainers know not to break the linkage when changing mocks.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 4133-4153: The test "rejects invalid or colliding experimental
backup filenames" currently only verifies runNamedBackupExportMock was called
but not that the UI handled the "collision" result; update the test to assert
the user-visible outcome by adding a check that the warning logger (e.g.,
warnSpy) was called with a message containing "collision" or that the prompt
re-ran (e.g., promptQuestionMock called more than once or selectSequence
progressed accordingly); reference runNamedBackupExportMock and
promptQuestionMock in the new assertion so the test deterministically verifies
the collision path and user feedback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9decb3d0-27f9-47c8-b9c9-d8e9aee26dee
📒 Files selected for processing (6)
lib/codex-manager.tslib/storage.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 (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/cli.test.tstest/codex-manager-cli.test.tstest/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/storage.tslib/ui/copy.ts
🔇 Additional comments (20)
lib/ui/copy.ts (1)
17-37: lgtm — ui copy for restore-backup flow.new copy entries for recovery/restore-backup are straightforward. template functions correctly interpolate counts and names. no sensitive data exposure risk here.
test/cli.test.ts (1)
707-744: test still passes on the generic invalid-input loop rather than the alias-specific rejection.this test verifies that
u,restore,backup,restore-backupall eventually resolve to{ mode: "cancel" }after a secondquitinput, and thatUI_COPY.fallback.invalidModePromptis logged 4 times. however, it does not assert the branch-specific rejection message (e.g., "restore-backup requires the interactive backup browser") that distinguishes these aliases from any other invalid input. if the implementation just falls through to the generic invalid loop, the test passes; if someone later removes the alias handling entirely, the test still passes.add an assertion on the alias-specific rejection copy (or a distinct property in the resolved value) so this test actually guards the new alias handling. as per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.test/storage.test.ts (12)
1-44: imports look correct for new named-backup test coverage.added
resolve,ACCOUNT_LIMITS,removeWithRetry, and the named-backup api surface fromlib/storage.js. these are all exercised in the new tests.
379-440: good transaction boundary tests including win32 case-insensitive path handling.
test/storage.test.ts:407-440correctly mocksprocess.platformtowin32and verifies that case-only path differences are allowed inside a transaction. cleanup is handled in finally block.
2223-2254: these assertions encode conflicting contracts for the same input shape.
test/storage.test.ts:2170-2179already establishes thatassessNamedBackupRestore("../...")andrestoreNamedBackup("../...")fail fast on the separator validator with "must not contain path separators". attest/storage.test.ts:2245-2250, the test expects a different error ("escapes backup directory") for another../...input, but the separator check rejects first, so the mockedreaddirbranch is never reached.use an input that survives the name validator if you want to exercise the containment check on directory entries returned by readdir, or drop the high-level assertions here and keep only the lower-level containment coverage.
2511-2565: this does not reproduce the symlink escape you are naming here.
test/storage.test.ts:2511-2530createsexternalBackupPathbut never links it intobackupRoot. the test only feeds a fabricatedDirentwithisSymbolicLink() === true, which misses the reallstat/realpathcontainment path inlib/storage.ts. lines 2256-2346 testassertNamedBackupRestorePathwith real symlinks, butlistNamedBackups,assessNamedBackupRestore, andrestoreNamedBackupare not driven through an actual symlink/junction underbackupRoot. this won't catch win32 junction regressions.drive this through an actual symlink/junction under
backupRootand assert the end-to-end api calls reject it.
2112-2140: the stale-assessment race is still untested on the publicrestoreAssessedNamedBackupapi.
test/storage.test.ts:2112-2140covers the case where the backup file disappears after assessment.test/storage.test.ts:3278-3347racesrestoreNamedBackup(), which re-assesses internally. but the real toctou surface is not pinned: two precomputed assessments can both look eligible atACCOUNT_LIMITS.MAX_ACCOUNTS - 1, then one import changes storage before the secondrestoreAssessedNamedBackup()runs.add that regression so the mutex/no-op behavior is locked down on the exported api, not just the wrapper. as per coding guidelines,
test/**: demand regression cases that reproduce concurrency bugs.
3278-3347: good concurrent restore serialization test.
test/storage.test.ts:3278-3347correctly races tworestoreNamedBackupcalls when limit is tight and verifies one succeeds while the other fails with "Import would exceed maximum". this exercises the internal mutex behavior. (the stale-assessment race onrestoreAssessedNamedBackupis flagged separately.)
2256-2346: good real-symlink containment tests forassertNamedBackupRestorePath.
test/storage.test.ts:2256-2346creates actual symlinks/junctions and validates thatassertNamedBackupRestorePathrejects paths escaping the backup root. platform-specific symlink types are handled (junctionon win32). this covers the low-level containment logic well.
2605-2623: good retry behavior test for EAGAIN on non-windows.
test/storage.test.ts:2605-2623mocks platform tolinux, injects EAGAIN errors on readdir, and verifies 7 retry attempts. deterministic and properly restored in finally block.
2774-2860: good chunked backup read retry test.
test/storage.test.ts:2774-2860creates backups exceeding the chunk boundary, injects a transient EBUSY on the last backup read, and verifies retry succeeds. properly cleans up spies in finally block.
3081-3119: good candidate cache reuse test.
test/storage.test.ts:3081-3119verifies thatlistNamedBackupspopulates the candidate cache andassessNamedBackupRestorereuses it, avoiding duplicate readFile calls. proper spy cleanup.
3224-3276: good concurrency limit test for backup reads.
test/storage.test.ts:3224-3276creates backups exceeding the concurrency limit, adds delays to readFile, and verifies peak concurrent reads stay withinNAMED_BACKUP_LIST_CONCURRENCY. the timing-based approach is reasonable for this type of test.
1950-2002: good metadata-only restore eligibility test.
test/storage.test.ts:1950-2002verifies that a backup with fresher metadata is markedeligibleForRestore = trueeven whenimported = 0, and that the restore actually updates stored metadata (accessToken, lastUsed).test/codex-manager-cli.test.ts (6)
3388-3463: concurrency test is well-designed.imports the actual
NAMED_BACKUP_ASSESS_CONCURRENCYconstant fromlib/storage.js:3391-3394and verifiesmaxInFlightnever exceeds it. the deferred-gate pattern withqueueMicrotaskbatching is deterministic since all gates release together before assertion. good coverage for the chunked-concurrency behavior mentioned in the PR summary.
2701-2757: EBUSY error path tested correctly.the test at
test/codex-manager-cli.test.ts:2701covers transient filesystem errors withmakeErrnoError("backup busy", "EBUSY")and verifies the cli returns non-zero with proper error messaging. this addresses one of the windows-related concerns from the greptile review.
2393-2459: restore flow tests are comprehensive.both "restores a named backup from the login recovery menu" and "restores a named backup from the direct restore-backup command" cover:
listNamedBackupsMockcalled onceassessNamedBackupRestoreMockcalled with expected args includingcurrentStorageconfirmMockcalled onceimportAccountsMockcalled with resolved pathsetCodexCliActiveSelectionMockcalled for post-restore syncassertions are specific and deterministic.
Also applies to: 2461-2526
3649-3744: reassessment eligibility change is tested.the test "returns to the login menu when backup reassessment becomes ineligible" at
test/codex-manager-cli.test.ts:3649correctly verifies that wheneligibleForRestoreflips tofalseon reassessment, the flow aborts without callingconfirmMockorimportAccountsMock. this addresses theeligibleForRestoreguard concern from the PR summary.
5145-5265: race condition tests for quota refresh coordination.both "waits for an in-flight menu quota refresh before starting quick check" and "waits for an in-flight menu quota refresh before starting backup restore" use the deferred pattern to verify that:
- menu actions wait for in-flight fetches to complete
saveQuotaCacheMock.mock.invocationCallOrder[0]precedes the action's mock callthese are solid concurrency regression tests per coding guidelines.
Also applies to: 5267-5355
1-10: symlink regression tests already exist throughout the test suite and address windows filesystem edge cases.test/named-backup-export.test.ts:186 ("rejects backup roots that escape through a symlinked backups directory") and test/storage.test.ts:2256 ("rejects backup paths whose real path escapes the backups directory through symlinked directories") already cover the exact scenario you suggested—symlinks that resolve outside the backup directory. both tests handle windows by using junction points (
process.platform === "win32" ? "junction" : "dir"). further, test/storage.test.ts:2313 validates symlinked backup roots during restore, and test/paths.test.ts covers realpathSync fallback behavior on windows.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
♻️ Duplicate comments (6)
lib/storage.ts (3)
3200-3202:⚠️ Potential issue | 🟠 Majordo not export the live containment fs singleton.
lib/storage.ts:3200-3202exposes a mutable process-wide object. tests or callers can monkey-patchlstat/realpathand weaken containment checks for later restores in the same process. expose controlled set/reset hooks (test-only) or return a frozen clone instead.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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 3200 - 3202, The export is exposing the live mutable singleton namedBackupContainmentFs via __testOnly; instead, stop exporting the live object and replace it with controlled test-only accessors: provide functions like getNamedBackupContainmentFs() that return a deep-frozen clone and setNamedBackupContainmentFsForTest(fs) + resetNamedBackupContainmentFsForTest() to install/reset a mock in a controlled way; update references to call the getter where needed, mark the new functions as test-only, and update vitest tests to use set/reset hooks so no test can monkey-patch the real process-wide object (also verify related tests handle Windows FS IO, auth rotation, and concurrency scenarios including EBUSY/429 retries).
1977-1984:⚠️ Potential issue | 🟠 Majorcontainment checks are still sync and transient path-validation errors are not retried.
lib/storage.ts:1977-1984useslstatSync/realpathSync.nativeon interactive restore/list paths, andlib/storage.ts:2083-2171converts transient lock errors intoBackupPathValidationTransientErrorwithout a retry loop at the containment layer. on windows, one-shot av/file-lock races can still fail list/restore, and sync calls can stall the menu thread.#!/bin/bash set -euo pipefail echo "== containment fs sync usage ==" rg -n "lstatSync|realpathSync\\.native" lib/storage.ts echo "== transient containment error paths ==" rg -n "BackupPathValidationTransientError|resolvePathForNamedBackupContainment|assertNamedBackupRestorePath" lib/storage.ts echo "== retry helper callsites in named-backup flow ==" rg -n "retryTransientFilesystemOperation\\(" lib/storage.tsAs 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: 2083-2171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1977 - 1984, The containment FS currently uses blocking lstatSync/realpathSync.native in namedBackupContainmentFs and does not retry transient lock races: replace those sync calls with async fs.promises equivalents inside namedBackupContainmentFs (use async lstat and realpath), and wrap them with the existing retryTransientFilesystemOperation helper so transient errors (EBUSY/EPERM/ETIMEDOUT/429-like conditions) are retried; ensure resolvePathForNamedBackupContainment and assertNamedBackupRestorePath propagate BackupPathValidationTransientError when the retry helper exhausts attempts, update affected callsites to await the async containment methods, and add/update vitest cases to cover Windows EBUSY/file-lock races and that retries occur.
1895-1906:⚠️ Potential issue | 🟠 Majorcarry assessed backup identity into restore to close the assess/restore toctou window.
lib/storage.ts:1903-1905resolves byassessment.backup.nameagain, so a file swap after confirmation can restore different content than what was assessed. this is still the same race surfaced in earlier feedback. also add a vitest that swaps a backup to a different valid payload between assess andrestoreAssessedNamedBackup(); the current tests aroundtest/storage.test.ts:2084-2168only cover delete/invalid-json paths.possible fix
export async function restoreAssessedNamedBackup( assessment: Pick<BackupRestoreAssessment, "backup" | "eligibleForRestore" | "error">, ): Promise<ImportAccountsResult> { @@ - const resolvedPath = await resolveNamedBackupRestorePath( - assessment.backup.name, - ); + const backupRoot = getNamedBackupRoot(getStoragePath()); + const resolvedPath = assertNamedBackupRestorePath( + assessment.backup.path, + backupRoot, + ); + const currentMetadata = await buildNamedBackupMetadata( + assessment.backup.name, + resolvedPath, + ); + if ( + currentMetadata.updatedAt !== assessment.backup.updatedAt || + currentMetadata.sizeBytes !== assessment.backup.sizeBytes + ) { + throw new Error( + "backup changed after confirmation. re-assess and retry restore.", + ); + } return importAccounts(resolvedPath); }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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1895 - 1906, The restoreAssessedNamedBackup function currently re-resolves the backup by name (calling resolveNamedBackupRestorePath inside restoreAssessedNamedBackup), opening a TOCTOU where the file at that name could be swapped after assessment; change the flow to carry the assessed identity into the restore call (e.g., include the resolved path or a content hash in the BackupRestoreAssessment and use that stored value instead of re-resolving) so restoreAssessedNamedBackup uses assessment's vetted identifier (use symbols: restoreAssessedNamedBackup, BackupRestoreAssessment, resolveNamedBackupRestorePath, importAccounts); update tests by adding a vitest that swaps the backup file to a different valid payload between assess and restore (extend test/storage.test.ts around the existing tests at 2084-2168) to assert the restore uses the original assessed content and fails if swapped.test/storage.test.ts (3)
2511-2565:⚠️ Potential issue | 🟠 Majorreplace dirent fabrication with a real symlink/junction regression.
test/storage.test.ts:2534-2551mocks a symlink-likeDirent, so it does not exercise reallstat/realpathcontainment behavior end-to-end. create an actual symlink/junction under backup root and runlistNamedBackups,assessNamedBackupRestore, andrestoreNamedBackupagainst it.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 2511 - 2565, Replace the fabricated Dirent hack with a real filesystem symlink/junction: instead of mocking fs.readdir, create an actual link file named "escaped-link.json" inside backupRoot that points to externalBackupPath (use fs.symlink with type 'junction' on Windows) before calling listNamedBackups, assessNamedBackupRestore("escaped-link"), and restoreNamedBackup("escaped-link"); assert listNamedBackups returns [] and the two restore/assess calls reject with /not a regular backup file/i, then remove the symlink in a finally block to clean up. Ensure the test creates backupRoot and externalBackupPath as shown, uses the same identifiers used in the diff (backupRoot, externalBackupPath, escaped-link.json, listNamedBackups, assessNamedBackupRestore, restoreNamedBackup), and avoids mocking filesystem APIs so lstat/realpath containment checks run end-to-end.
2223-2254:⚠️ Potential issue | 🟠 Majorthis assertion path uses an impossible filename shape and conflicts with separator-validator coverage.
test/storage.test.ts:2228fabricates aDirentnamed"../escaped-entry.json", which cannot exist as a real directory entry. it also conflicts with the explicit separator-validator expectations intest/storage.test.ts:2170-2180. use a valid input that reaches containment checks, or keep this as a lower-level containment-only test without contradictory high-level name-validation assertions.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 2223 - 2254, The test currently fabricates a Dirent with an impossible name ("../escaped-entry.json") which conflicts with the higher-level separator validation; change the fake entry so its name is a valid leaf (e.g. "escaped-entry.json") and simulate an escape via resolution (make the Dirent report isSymbolicLink() true and mock the follow-up resolution calls—e.g., fs.readlink or the code path that resolves the entry to an absolute path—to return a path outside backupRoot) so assessNamedBackupRestore and restoreNamedBackup exercise the containment checks without violating name-validator expectations; update references in the test to the same symbols (readdirSpy, escapedEntry, backupRoot) and keep the restore/assert logic intact.
2112-2140:⚠️ Potential issue | 🟠 Majorthis test locks in stale-assessment behavior instead of guarding against a changed backup payload.
test/storage.test.ts:2112-2140verifies re-resolution after deletion, but it does not pin the higher-risk case where the backup remains present and changes to different valid json between assess and restore. add a deterministic regression for that race onrestoreAssessedNamedBackup().As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 2112 - 2140, The test currently only checks deletion between assess and restore; add a deterministic regression where the backup file is still present but mutated to a different valid JSON between assessNamedBackupRestore() and restoreAssessedNamedBackup() to ensure restore re-resolves and fails appropriately. Specifically, in the same test (or a new one), after creating the named backup via createNamedBackup("...") and calling assessNamedBackupRestore(...) (store the returned assessment and backup.path), overwrite the backup file at backup.path with a different valid JSON payload (using fs.writeFileSync or the test helper used elsewhere) before calling restoreAssessedNamedBackup(assessment), then assert that restoreAssessedNamedBackup rejects (or returns the expected error) and that loadAccounts() remains unchanged; reference functions createNamedBackup, assessNamedBackupRestore, restoreAssessedNamedBackup, removeWithRetry, saveAccounts, and loadAccounts so the change is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/storage.ts`:
- Around line 3200-3202: The export is exposing the live mutable singleton
namedBackupContainmentFs via __testOnly; instead, stop exporting the live object
and replace it with controlled test-only accessors: provide functions like
getNamedBackupContainmentFs() that return a deep-frozen clone and
setNamedBackupContainmentFsForTest(fs) + resetNamedBackupContainmentFsForTest()
to install/reset a mock in a controlled way; update references to call the
getter where needed, mark the new functions as test-only, and update vitest
tests to use set/reset hooks so no test can monkey-patch the real process-wide
object (also verify related tests handle Windows FS IO, auth rotation, and
concurrency scenarios including EBUSY/429 retries).
- Around line 1977-1984: The containment FS currently uses blocking
lstatSync/realpathSync.native in namedBackupContainmentFs and does not retry
transient lock races: replace those sync calls with async fs.promises
equivalents inside namedBackupContainmentFs (use async lstat and realpath), and
wrap them with the existing retryTransientFilesystemOperation helper so
transient errors (EBUSY/EPERM/ETIMEDOUT/429-like conditions) are retried; ensure
resolvePathForNamedBackupContainment and assertNamedBackupRestorePath propagate
BackupPathValidationTransientError when the retry helper exhausts attempts,
update affected callsites to await the async containment methods, and add/update
vitest cases to cover Windows EBUSY/file-lock races and that retries occur.
- Around line 1895-1906: The restoreAssessedNamedBackup function currently
re-resolves the backup by name (calling resolveNamedBackupRestorePath inside
restoreAssessedNamedBackup), opening a TOCTOU where the file at that name could
be swapped after assessment; change the flow to carry the assessed identity into
the restore call (e.g., include the resolved path or a content hash in the
BackupRestoreAssessment and use that stored value instead of re-resolving) so
restoreAssessedNamedBackup uses assessment's vetted identifier (use symbols:
restoreAssessedNamedBackup, BackupRestoreAssessment,
resolveNamedBackupRestorePath, importAccounts); update tests by adding a vitest
that swaps the backup file to a different valid payload between assess and
restore (extend test/storage.test.ts around the existing tests at 2084-2168) to
assert the restore uses the original assessed content and fails if swapped.
In `@test/storage.test.ts`:
- Around line 2511-2565: Replace the fabricated Dirent hack with a real
filesystem symlink/junction: instead of mocking fs.readdir, create an actual
link file named "escaped-link.json" inside backupRoot that points to
externalBackupPath (use fs.symlink with type 'junction' on Windows) before
calling listNamedBackups, assessNamedBackupRestore("escaped-link"), and
restoreNamedBackup("escaped-link"); assert listNamedBackups returns [] and the
two restore/assess calls reject with /not a regular backup file/i, then remove
the symlink in a finally block to clean up. Ensure the test creates backupRoot
and externalBackupPath as shown, uses the same identifiers used in the diff
(backupRoot, externalBackupPath, escaped-link.json, listNamedBackups,
assessNamedBackupRestore, restoreNamedBackup), and avoids mocking filesystem
APIs so lstat/realpath containment checks run end-to-end.
- Around line 2223-2254: The test currently fabricates a Dirent with an
impossible name ("../escaped-entry.json") which conflicts with the higher-level
separator validation; change the fake entry so its name is a valid leaf (e.g.
"escaped-entry.json") and simulate an escape via resolution (make the Dirent
report isSymbolicLink() true and mock the follow-up resolution calls—e.g.,
fs.readlink or the code path that resolves the entry to an absolute path—to
return a path outside backupRoot) so assessNamedBackupRestore and
restoreNamedBackup exercise the containment checks without violating
name-validator expectations; update references in the test to the same symbols
(readdirSpy, escapedEntry, backupRoot) and keep the restore/assert logic intact.
- Around line 2112-2140: The test currently only checks deletion between assess
and restore; add a deterministic regression where the backup file is still
present but mutated to a different valid JSON between assessNamedBackupRestore()
and restoreAssessedNamedBackup() to ensure restore re-resolves and fails
appropriately. Specifically, in the same test (or a new one), after creating the
named backup via createNamedBackup("...") and calling
assessNamedBackupRestore(...) (store the returned assessment and backup.path),
overwrite the backup file at backup.path with a different valid JSON payload
(using fs.writeFileSync or the test helper used elsewhere) before calling
restoreAssessedNamedBackup(assessment), then assert that
restoreAssessedNamedBackup rejects (or returns the expected error) and that
loadAccounts() remains unchanged; reference functions createNamedBackup,
assessNamedBackupRestore, restoreAssessedNamedBackup, removeWithRetry,
saveAccounts, and loadAccounts so the change is easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d93ca5a2-d608-48c4-a3cb-5ff5c61b390a
📒 Files selected for processing (2)
lib/storage.tstest/storage.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/storage.ts
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.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/storage.test.ts (1)
5830-5859: 🧹 Nitpick | 🔵 Trivialstandalone test outside main describe block.
test/storage.test.ts:5830-5859is a standaloneitblock outside the maindescribe("storage", ...)block. this works in vitest but is slightly inconsistent with the file's organization. consider moving it inside thedescribe("clearAccounts edge cases", ...)block at line 5669 for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 5830 - 5859, The test "clearAccounts removes discovered backup artifacts as well as fixed slots" is declared as a standalone it block outside the main storage suite; move this it block into the existing describe("clearAccounts edge cases", ...) block so it sits with related tests for consistency, keeping the same test body and using the same references to clearAccounts, getStoragePath, and the filesystem helpers; ensure imports/setup from the surrounding describe still apply and remove the orphaned it to avoid duplication.
♻️ Duplicate comments (5)
lib/storage.ts (2)
1919-1928:⚠️ Potential issue | 🟠 Majorpreserve the assessed backup fingerprint through import.
lib/storage.ts:1919-1928still has a post-confirm to import race. the path is reused, but the file can still be replaced after the last assessment and beforeimportAccounts(), so the cli may restore different contents than the user approved.test/codex-manager-cli.test.ts:2752-2820only injects an import failure and does not cover that replace-after-confirm case. carry a stable fingerprint fromassessNamedBackupRestore()and reject if it changes before import.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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1919 - 1928, The assessed backup fingerprint produced by assessNamedBackupRestore must be preserved and verified before import to prevent replace-after-confirm races: modify BackupRestoreAssessment to include the stable fingerprint/hash and change restoreAssessedNamedBackup to accept/use that fingerprint (or a new assessedBackupFingerprint field) and, before calling importAccounts(assessment.backup.path), recompute the current file fingerprint and throw/reject if it differs; alternatively pass the fingerprint into importAccounts so it validates internally. Update or add vitest cases (e.g., test/codex-manager-cli.test.ts around the earlier injection tests) to cover the replace-after-confirm scenario and ensure the new branch handles filesystem concurrency/EBUSY semantics per lib/** guidelines.
1998-2005:⚠️ Potential issue | 🟠 Majordo not export the live containment fs hooks.
lib/storage.ts:1998-2005pluslib/storage.ts:3226-3228still hand out the mutable singleton that enforces named-backup containment.test/codex-manager-cli.test.ts:108-130spreads the reallib/storage.tsexports into one shared vitest module instance, so one test patch can silently weaken later restore coverage. expose controlled set/reset hooks or a cloned facade instead.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: 3226-3228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1998 - 2005, The current export namedBackupContainmentFs is handing out the live mutable singleton (namedBackupContainmentFs) which tests can modify; change the module API so you do not export the live object directly—export a factory or a function that returns a shallow-cloned facade (e.g., getNamedBackupContainmentFs() or createNamedBackupContainmentFs()) and add explicit setNamedBackupContainmentFsForTest/resetNamedBackupContainmentFs hooks for controlled test injection; update any uses that directly import namedBackupContainmentFs to call the factory/getter, and ensure the original internal singleton (the one referenced in namedBackupContainmentFs and the other export sites around the same area) remains private within the module.test/codex-manager-cli.test.ts (1)
4128-4148:⚠️ Potential issue | 🟡 Minorassert the rejection outcome here.
test/codex-manager-cli.test.ts:4128-4148still passes if the ui accepts"../bad-name", swallows the collision, or exits before showing feedback. add an assertion on the user-visible rejection path—reprompt, warning text, or collision guidance—so this regression stays meaningful.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 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 4128 - 4148, The test currently only verifies runNamedBackupExportMock was called but doesn't assert the user-visible rejection flow; update the "rejects invalid or colliding experimental backup filenames" test to assert the rejection path by checking that promptQuestionMock was invoked again (reprompt) or that the UI/logger was called with a collision/invalid filename message (e.g., expect(consoleMock/yourLoggerMock).toHaveBeenCalledWith(expect.stringMatching(/collision|invalid|reject/i))), referencing promptQuestionMock, runNamedBackupExportMock, runCodexMultiAuthCli, and selectSequence to locate the test logic and mock hooks.test/storage.test.ts (2)
2595-2649:⚠️ Potential issue | 🟡 Minortest still mocks Dirent instead of creating a real symlink for listing.
test/storage.test.ts:2595-2649fabricates aDirentwithisSymbolicLink() => truerather than creating an actual symlink insidebackupRoot. while tests attest/storage.test.ts:2255-2345coverassertNamedBackupRestorePathwith real symlinks, this test exerciseslistNamedBackups/assessNamedBackupRestore/restoreNamedBackupbut doesn't hit the reallstat/realpathcontainment code path for symlinked entries discovered during listing.consider creating an actual symlink file inside the backup directory pointing to
externalBackupPath, then verifying thatlistNamedBackupsfilters it out andassessNamedBackupRestore("escaped-link")rejects it through the real containment checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 2595 - 2649, The test fabricates a Dirent instead of creating a real symlink so it never exercises the real lstat/realpath containment path; replace the Dirent mock with an actual filesystem symlink: create backupRoot, write externalBackupPath as done, then create a symlink inside backupRoot named "escaped-link.json" that points to externalBackupPath (using fs.symlink/await), remove the readdir spy/mocking, call listNamedBackups to assert the symlinked entry is filtered out and then call assessNamedBackupRestore("escaped-link") and restoreNamedBackup("escaped-link") to assert they reject via the real containment checks; finally remove the symlink in the test cleanup (or use try/finally) so tests are isolated.
3413-3482:⚠️ Potential issue | 🟠 Majorconcurrent restore test is good but stale-assessment toctou remains untested.
test/storage.test.ts:3413-3482tests concurrentrestoreNamedBackupcalls which re-assess internally. this validates serialization at the import layer.however, the past review flagged a different race: two calls to
assessNamedBackupRestoreboth returneligibleForRestore: trueatMAX_ACCOUNTS - 1, then both callrestoreAssessedNamedBackupwith their precomputed assessments. one import changes storage, but the secondrestoreAssessedNamedBackupstill has a stale assessment. this toctou surface on the exportedrestoreAssessedNamedBackupapi is not pinned down here.add a regression test that:
- saves accounts up to
ACCOUNT_LIMITS.MAX_ACCOUNTS - 1- creates a backup with 1 new account
- calls
assessNamedBackupRestoretwice concurrently to get two assessments- both assessments show
eligibleForRestore: true- calls
restoreAssessedNamedBackup(assessment1)andrestoreAssessedNamedBackup(assessment2)concurrently- asserts one succeeds, one fails or is a no-op, and final account count equals
MAX_ACCOUNTSas 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 3413 - 3482, Add a deterministic vitest regression that reproduces the TOCTOU: save accounts up to ACCOUNT_LIMITS.MAX_ACCOUNTS - 1, create a named backup containing one extra account, then call assessNamedBackupRestore concurrently twice (Promise.all) to produce two assessments and assert both report eligibleForRestore: true; next call restoreAssessedNamedBackup concurrently with those two assessments (Promise.allSettled) and assert exactly one fulfillment and one rejection or no-op, and finally loadAccounts and assert restored accounts length === ACCOUNT_LIMITS.MAX_ACCOUNTS; reference the existing helpers used elsewhere in the file (saveAccounts, createNamedBackup, assessNamedBackupRestore, restoreAssessedNamedBackup, loadAccounts) and follow surrounding test style for deterministic timings and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 2609-2628: Add a new test case that exercises the env-driven
non-interactive guard by setting TTYs to interactive and forcing
isNonInteractiveMode() to true: call setInteractiveTTY(true), set
isNonInteractiveModeMock.mockReturnValue(true), import and run
runCodexMultiAuthCli(["auth","restore-backup"]) and assert it returns exit code
1, that promptLoginModeMock/listNamedBackupsMock/selectMock/confirmMock are not
called, and that console.error was called with the same "Backup restore manager
requires an interactive TTY..." message; this pins the lib/codex-manager.ts
branch handling isNonInteractiveMode() while keeping tests deterministic under
Vitest.
---
Outside diff comments:
In `@test/storage.test.ts`:
- Around line 5830-5859: The test "clearAccounts removes discovered backup
artifacts as well as fixed slots" is declared as a standalone it block outside
the main storage suite; move this it block into the existing
describe("clearAccounts edge cases", ...) block so it sits with related tests
for consistency, keeping the same test body and using the same references to
clearAccounts, getStoragePath, and the filesystem helpers; ensure imports/setup
from the surrounding describe still apply and remove the orphaned it to avoid
duplication.
---
Duplicate comments:
In `@lib/storage.ts`:
- Around line 1919-1928: The assessed backup fingerprint produced by
assessNamedBackupRestore must be preserved and verified before import to prevent
replace-after-confirm races: modify BackupRestoreAssessment to include the
stable fingerprint/hash and change restoreAssessedNamedBackup to accept/use that
fingerprint (or a new assessedBackupFingerprint field) and, before calling
importAccounts(assessment.backup.path), recompute the current file fingerprint
and throw/reject if it differs; alternatively pass the fingerprint into
importAccounts so it validates internally. Update or add vitest cases (e.g.,
test/codex-manager-cli.test.ts around the earlier injection tests) to cover the
replace-after-confirm scenario and ensure the new branch handles filesystem
concurrency/EBUSY semantics per lib/** guidelines.
- Around line 1998-2005: The current export namedBackupContainmentFs is handing
out the live mutable singleton (namedBackupContainmentFs) which tests can
modify; change the module API so you do not export the live object
directly—export a factory or a function that returns a shallow-cloned facade
(e.g., getNamedBackupContainmentFs() or createNamedBackupContainmentFs()) and
add explicit setNamedBackupContainmentFsForTest/resetNamedBackupContainmentFs
hooks for controlled test injection; update any uses that directly import
namedBackupContainmentFs to call the factory/getter, and ensure the original
internal singleton (the one referenced in namedBackupContainmentFs and the other
export sites around the same area) remains private within the module.
In `@test/codex-manager-cli.test.ts`:
- Around line 4128-4148: The test currently only verifies
runNamedBackupExportMock was called but doesn't assert the user-visible
rejection flow; update the "rejects invalid or colliding experimental backup
filenames" test to assert the rejection path by checking that promptQuestionMock
was invoked again (reprompt) or that the UI/logger was called with a
collision/invalid filename message (e.g.,
expect(consoleMock/yourLoggerMock).toHaveBeenCalledWith(expect.stringMatching(/collision|invalid|reject/i))),
referencing promptQuestionMock, runNamedBackupExportMock, runCodexMultiAuthCli,
and selectSequence to locate the test logic and mock hooks.
In `@test/storage.test.ts`:
- Around line 2595-2649: The test fabricates a Dirent instead of creating a real
symlink so it never exercises the real lstat/realpath containment path; replace
the Dirent mock with an actual filesystem symlink: create backupRoot, write
externalBackupPath as done, then create a symlink inside backupRoot named
"escaped-link.json" that points to externalBackupPath (using fs.symlink/await),
remove the readdir spy/mocking, call listNamedBackups to assert the symlinked
entry is filtered out and then call assessNamedBackupRestore("escaped-link") and
restoreNamedBackup("escaped-link") to assert they reject via the real
containment checks; finally remove the symlink in the test cleanup (or use
try/finally) so tests are isolated.
- Around line 3413-3482: Add a deterministic vitest regression that reproduces
the TOCTOU: save accounts up to ACCOUNT_LIMITS.MAX_ACCOUNTS - 1, create a named
backup containing one extra account, then call assessNamedBackupRestore
concurrently twice (Promise.all) to produce two assessments and assert both
report eligibleForRestore: true; next call restoreAssessedNamedBackup
concurrently with those two assessments (Promise.allSettled) and assert exactly
one fulfillment and one rejection or no-op, and finally loadAccounts and assert
restored accounts length === ACCOUNT_LIMITS.MAX_ACCOUNTS; reference the existing
helpers used elsewhere in the file (saveAccounts, createNamedBackup,
assessNamedBackupRestore, restoreAssessedNamedBackup, loadAccounts) and follow
surrounding test style for deterministic timings and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25a8fec1-534b-4359-8d39-579368ad250b
📒 Files selected for processing (3)
lib/storage.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-cli.test.tstest/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.ts
🔇 Additional comments (13)
test/storage.test.ts (13)
1-44: imports and setup look correct.the new imports for named backup APIs (
assessNamedBackupRestore,createNamedBackup,listNamedBackups,restoreNamedBackup, etc.) align with the feature being tested.removeWithRetryhelper attest/helpers/remove-with-retry.jsis a good pattern for windows cleanup reliability.
341-344: good use ofremoveWithRetryfor windows cleanup reliability.switching from
fs.rmtoremoveWithRetryin afterEach handles EBUSY/EPERM transient errors that occur on windows when AV or indexers hold file handles. this makes tests more reliable cross-platform.
379-440: transaction + storage path tests cover important edge cases.the win32 case-insensitivity test at
test/storage.test.ts:407-440is a good addition for windows path comparison behavior. the platform mock is properly restored in finally block.
478-714: import deduplication tests are thorough and verify side effects.the
changedfield assertions andwriteFileSpychecks ensure no unnecessary disk writes occur for no-op imports. this is important for performance and avoiding wear on SSDs. tests attest/storage.test.ts:478-714cover edge cases like key order differences and metadata-only refreshes.
1251-1316: max accounts limit test properly validates sequential imports.test at
test/storage.test.ts:1251-1316verifies that hittingACCOUNT_LIMITS.MAX_ACCOUNTS(20) on first import still rejects subsequent imports that would exceed the limit. the assertion at line 1314 confirms no partial state from the failed second import.
1576-1643: chunk boundary test attest/storage.test.ts:1605-1643is good for pagination coverage.testing with
NAMED_BACKUP_LIST_CONCURRENCY + 1backups ensures the chunked listing logic handles boundary conditions. the test verifies all backups are returned with correct metadata.
2255-2345: symlink containment tests now use real filesystem symlinks.tests at
test/storage.test.ts:2255-2345create actual symlinks/junctions usingfs.symlinkwith the correct type for win32 (junction). this exercises the reallstat/realpathcontainment logic inlib/storage.tsrather than fabricated Dirents. this addresses the containment validation gap noted in prior review.
2169-2253: path containment tests cover both input validation and directory traversal layers.tests at
test/storage.test.ts:2169-2179validate the separator rejection in input names, whiletest/storage.test.ts:2222-2253tests the containment check when readdir returns a traversal entry. the past review concern about conflicting contracts was marked as addressed.
3216-3357: candidate cache tests are thorough and verify isolation.
test/storage.test.ts:3308-3357testing isolated caches across concurrent listings is important for correctness. the read count assertions validate that caching behaves as expected without cross-contamination.
2651-2987: transient error retry tests cover platform differences well.tests at
test/storage.test.ts:2651-2667verify non-windows platforms rethrow EPERM immediately, whiletest/storage.test.ts:2806-2856verify win32 retries EPERM. this matches the windows-specific AV/indexer behavior patterns. the EAGAIN tests are platform-agnostic as expected.
3359-3411: concurrency limit test validates throttling behavior.
test/storage.test.ts:3359-3411tracks peak concurrent reads to ensureNAMED_BACKUP_LIST_CONCURRENCYis respected. the delay-based tracking is a reasonable approach for this kind of concurrency verification.
1803-2002: restore eligibility tests distinguish between no-op and metadata refresh scenarios.
test/storage.test.ts:1950-2002is a key test: backups with only metadata differences (likeaccessTokenorlastUsed) remain eligible for restore. this allows users to refresh stale tokens from backups. the assertions verify both the assessment and the final restored state.
2084-2167: toctou tests for file changes after assessment are well designed.
test/storage.test.ts:2112-2139is a subtle test: it mutatesassessment.backup.nameafter assessment but before restore, verifying thatrestoreAssessedNamedBackupuses the validated path captured at assessment time (assessment.backup.path), not the name. this is important for security.
| it("returns a non-zero exit code for the direct restore-backup command when stdin/stdout are not TTYs", async () => { | ||
| setInteractiveTTY(false); | ||
| const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); | ||
|
|
||
| it("exports named pool backup from experimental settings", async () => { | ||
| try { | ||
| const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); | ||
| const exitCode = await runCodexMultiAuthCli(["auth", "restore-backup"]); | ||
|
|
||
| expect(exitCode).toBe(1); | ||
| expect(promptLoginModeMock).not.toHaveBeenCalled(); | ||
| expect(listNamedBackupsMock).not.toHaveBeenCalled(); | ||
| expect(selectMock).not.toHaveBeenCalled(); | ||
| expect(confirmMock).not.toHaveBeenCalled(); | ||
| expect(errorSpy).toHaveBeenCalledWith( | ||
| "Backup restore manager requires an interactive TTY. Run this command in an interactive terminal.", | ||
| ); | ||
| } finally { | ||
| errorSpy.mockRestore(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
cover the env-driven non-interactive guard.
test/codex-manager-cli.test.ts:2609-2628 only exercises the !input.isTTY || !output.isTTY branch. lib/codex-manager.ts:4228-4236 also blocks when isNonInteractiveMode() is true, so add a case with setInteractiveTTY(true) and isNonInteractiveModeMock.mockReturnValue(true) to keep that guard pinned.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
🤖 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 2609 - 2628, Add a new test case
that exercises the env-driven non-interactive guard by setting TTYs to
interactive and forcing isNonInteractiveMode() to true: call
setInteractiveTTY(true), set isNonInteractiveModeMock.mockReturnValue(true),
import and run runCodexMultiAuthCli(["auth","restore-backup"]) and assert it
returns exit code 1, that
promptLoginModeMock/listNamedBackupsMock/selectMock/confirmMock are not called,
and that console.error was called with the same "Backup restore manager requires
an interactive TTY..." message; this pins the lib/codex-manager.ts branch
handling isNonInteractiveMode() while keeping tests deterministic under Vitest.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/storage.test.ts (1)
2639-2693:⚠️ Potential issue | 🟠 Majorreplace this dirent mock with a real symlink/junction escape regression.
test/storage.test.ts:2639-2693fabricatesDirentflags (isFile/isSymbolicLink) instead of creating an actual link on disk. that bypasses the real containment path (lib/storage.ts:2108-2192) and can miss windows junction edge cases.possible fix
- it("ignores symlink-like named backup entries that point outside the backups root", async () => { + it("rejects symlinked named backup entries that resolve outside the backups root", async () => { const backupRoot = join(dirname(testStoragePath), "backups"); - const externalBackupPath = join(testWorkDir, "outside-backup.json"); + const outsideRoot = join(testWorkDir, "outside-backup-root"); + const linkedRoot = join(backupRoot, "escaped-link"); + const externalBackupPath = join(outsideRoot, "escaped-link.json"); await fs.mkdir(backupRoot, { recursive: true }); + await fs.mkdir(outsideRoot, { recursive: true }); await fs.writeFile( externalBackupPath, JSON.stringify({ @@ "utf-8", ); - - const originalReaddir = fs.readdir.bind(fs); - const readdirSpy = vi.spyOn(fs, "readdir"); - const escapedEntry = { - name: "escaped-link.json", - isFile: () => false, - isSymbolicLink: () => true, - } as unknown as Awaited< - ReturnType<typeof fs.readdir> - >[number]; - readdirSpy.mockImplementation(async (...args) => { - const [path, options] = args; - if ( - String(path) === backupRoot && - typeof options === "object" && - options?.withFileTypes === true - ) { - return [escapedEntry] as Awaited<ReturnType<typeof fs.readdir>>; - } - return originalReaddir(...(args as Parameters<typeof fs.readdir>)); - }); + await fs.symlink( + resolve(outsideRoot), + linkedRoot, + process.platform === "win32" ? "junction" : "dir", + ); try { - const backups = await listNamedBackups(); - expect(backups).toEqual([]); - await expect(assessNamedBackupRestore("escaped-link")).rejects.toThrow( + await expect( + assertNamedBackupRestorePath(join(linkedRoot, "escaped-link.json"), backupRoot), + ).rejects.toThrow(/escapes backup directory/i); + await expect(assessNamedBackupRestore("escaped-link")).rejects.toThrow( /not a regular backup file/i, ); await expect(restoreNamedBackup("escaped-link")).rejects.toThrow( /not a regular backup file/i, ); } finally { - readdirSpy.mockRestore(); + await removeWithRetry(linkedRoot, { force: true, recursive: true }); } });As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 2639 - 2693, The test fakes Dirent flags instead of creating a real filesystem link, so update the test to create an actual symlink/junction that points outside the backups root (rather than mocking fs.readdir); create the external backup file (externalBackupPath) as already done, then create a symlink inside backupRoot named "escaped-link.json" that targets externalBackupPath (use fs.symlink and on Windows pass type: "junction" when necessary), call listNamedBackups(), assessNamedBackupRestore("escaped-link") and restoreNamedBackup("escaped-link") to assert failures, and finally clean up by removing the symlink and external file; remove the readdirSpy/mockImplementation and ensure teardown always runs (try/finally) so the created symlink and file are unlinked even on test failure.lib/storage.ts (2)
1919-1931:⚠️ Potential issue | 🟠 Majoradd content fingerprint validation before restoring an assessed backup.
lib/storage.ts:1919-1931revalidates containment, but it still allows a same-path file swap after assessment. that meansrestoreAssessedNamedBackup()can import content different from what was assessed. this is a residual assessment→restore toctou risk. please also pin this with a regression aroundtest/storage.test.ts:2112-2139(swap file contents between assess and restore-assessed).possible fix
export async function restoreAssessedNamedBackup( assessment: Pick<BackupRestoreAssessment, "backup" | "eligibleForRestore" | "error">, ): Promise<ImportAccountsResult> { if (!assessment.eligibleForRestore) { throw new Error( assessment.error ?? "Backup is not eligible for restore.", ); } const backupRoot = getNamedBackupRoot(getStoragePath()); const resolvedPath = await retryTransientNamedBackupPathValidation(() => assertNamedBackupRestorePath(assessment.backup.path, backupRoot), ); + const currentStats = await retryTransientFilesystemOperation(() => + fs.stat(resolvedPath), + ); + if ( + (typeof assessment.backup.updatedAt === "number" && + Number.isFinite(assessment.backup.updatedAt) && + currentStats.mtimeMs !== assessment.backup.updatedAt) || + (typeof assessment.backup.sizeBytes === "number" && + currentStats.size !== assessment.backup.sizeBytes) + ) { + throw new Error("Backup changed after assessment. Re-assess before restoring."); + } return importAccounts(resolvedPath); }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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1919 - 1931, The assessed backup restore currently only revalidates containment and is vulnerable to TOCTOU swaps; update the flow so the assessment includes a content fingerprint (e.g., SHA256) stored on the BackupRestoreAssessment record and then have restoreAssessedNamedBackup verify that fingerprint before importing: compute the fingerprint during assessment (where BackupRestoreAssessment is created), persist it on the assessment (e.g., assessment.backup.fingerprint), and in restoreAssessedNamedBackup (alongside assertNamedBackupRestorePath / retryTransientNamedBackupPathValidation) recompute the fingerprint for the resolvedPath and reject if it differs from the assessment fingerprint, then call importAccounts only after the match; also add a regression test (based on test/storage.test.ts:2112-2139) that swaps file contents between assess and restore-assessed to ensure detection, and ensure the change respects Windows IO/concurrency (EBUSY/429) and any auth-rotation concerns per lib/** guidelines.
3225-3227:⚠️ Potential issue | 🟠 Majordo not export the live containment fs singleton.
lib/storage.ts:3225-3227exposes a mutable process-wide object (__testOnly.namedBackupContainmentFs). any caller can monkey-patchlstat/realpathand weaken containment checks for later restores in the same process.possible fix
-const namedBackupContainmentFs = { +type NamedBackupContainmentFs = { + lstat(path: string): Promise<Awaited<ReturnType<typeof fs.lstat>>>; + realpath(path: string): Promise<string>; +}; + +function createNamedBackupContainmentFs(): NamedBackupContainmentFs { + return { + lstat(path: string) { + return fs.lstat(path); + }, + realpath(path: string) { + return fs.realpath(path); + }, + }; +} + +let namedBackupContainmentFs = createNamedBackupContainmentFs(); - lstat(path: string) { - return fs.lstat(path); - }, - realpath(path: string) { - return fs.realpath(path); - }, -}; @@ export const __testOnly = { - namedBackupContainmentFs, + setNamedBackupContainmentFs(overrides: Partial<NamedBackupContainmentFs>) { + namedBackupContainmentFs = { ...namedBackupContainmentFs, ...overrides }; + }, + resetNamedBackupContainmentFs() { + namedBackupContainmentFs = createNamedBackupContainmentFs(); + }, };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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 3225 - 3227, The export __testOnly exposes the live singleton namedBackupContainmentFs which allows callers to monkey-patch lstat/realpath and weaken containment checks; change the export so tests get a safe handle instead of the mutable singleton — e.g., remove exporting the raw namedBackupContainmentFs and provide either a factory/getter that returns a fresh instance or a read-only wrapper/API (e.g., getTestContainmentFs(), createNamedBackupContainmentFs(), or an immutableProxy) that does not expose mutating methods or underlying functions; update references to use the new accessor and ensure lstat/realpath cannot be overridden via the exported symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/storage.ts`:
- Around line 1919-1931: The assessed backup restore currently only revalidates
containment and is vulnerable to TOCTOU swaps; update the flow so the assessment
includes a content fingerprint (e.g., SHA256) stored on the
BackupRestoreAssessment record and then have restoreAssessedNamedBackup verify
that fingerprint before importing: compute the fingerprint during assessment
(where BackupRestoreAssessment is created), persist it on the assessment (e.g.,
assessment.backup.fingerprint), and in restoreAssessedNamedBackup (alongside
assertNamedBackupRestorePath / retryTransientNamedBackupPathValidation)
recompute the fingerprint for the resolvedPath and reject if it differs from the
assessment fingerprint, then call importAccounts only after the match; also add
a regression test (based on test/storage.test.ts:2112-2139) that swaps file
contents between assess and restore-assessed to ensure detection, and ensure the
change respects Windows IO/concurrency (EBUSY/429) and any auth-rotation
concerns per lib/** guidelines.
- Around line 3225-3227: The export __testOnly exposes the live singleton
namedBackupContainmentFs which allows callers to monkey-patch lstat/realpath and
weaken containment checks; change the export so tests get a safe handle instead
of the mutable singleton — e.g., remove exporting the raw
namedBackupContainmentFs and provide either a factory/getter that returns a
fresh instance or a read-only wrapper/API (e.g., getTestContainmentFs(),
createNamedBackupContainmentFs(), or an immutableProxy) that does not expose
mutating methods or underlying functions; update references to use the new
accessor and ensure lstat/realpath cannot be overridden via the exported symbol.
In `@test/storage.test.ts`:
- Around line 2639-2693: The test fakes Dirent flags instead of creating a real
filesystem link, so update the test to create an actual symlink/junction that
points outside the backups root (rather than mocking fs.readdir); create the
external backup file (externalBackupPath) as already done, then create a symlink
inside backupRoot named "escaped-link.json" that targets externalBackupPath (use
fs.symlink and on Windows pass type: "junction" when necessary), call
listNamedBackups(), assessNamedBackupRestore("escaped-link") and
restoreNamedBackup("escaped-link") to assert failures, and finally clean up by
removing the symlink and external file; remove the readdirSpy/mockImplementation
and ensure teardown always runs (try/finally) so the created symlink and file
are unlinked even on test failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5c52290-5447-4ad0-8980-d01ec97961bf
📒 Files selected for processing (2)
lib/storage.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
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/storage.ts
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.ts
Summary
#76fresh/01-reset-safetyWhat Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
f48ee06Additional Notes
npm run lint,npm run typecheck,npm run build, and targeted Vitest coverage for the restore-manager slice.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 reopens the backup-restore-manager slice from archived pr #76, adding a
codex auth restore-backupdirect command and a "Restore From Backup" entry under a new "Recovery" section in the login menu. it also refactorsrunAuthLoginto remove the empty-account gate so the menu is always shown (enabling restore when no accounts exist yet), and upgradesimportAccountswith achangedfield and a no-op skip path that avoids redundant writes when backup data is already identical to current storage.key changes and observations:
lib/storage.ts— addslistNamedBackups,assessNamedBackupRestore,restoreAssessedNamedBackup, and supporting helpers. path containment is enforced vialstat+realpathwithBackupContainmentErrorfor hard violations andBackupPathValidationTransientError(+ retry) for transient windows AV/file-lock cases.NAMED_BACKUP_LIST_CONCURRENCY=8andNAMED_BACKUP_ASSESS_CONCURRENCY=4bound concurrent i/o.importAccountsis refactored to pre-deduplicate both the existing and incoming account lists before merging — this fixes a pre-existing negative import count bug when the current storage has duplicate rows.lib/codex-manager.ts—runBackupRestoreManagercorrectly drainspendingMenuQuotaRefreshbefore starting the restore flow. it does a fresh re-assessment (no cache) before the confirm dialog to reflect any concurrent storage changes. a two-phasePromise.allSettledbatch assessment with containment error propagation is implemented correctly.codex-manager-cli.test.tscover all major paths: happy path from login menu and direct command, TTY guard, no-op, metadata-refresh wording, concurrent assessment limit, quota-drain ordering, and containment error propagation.storage.test.tsadds real-fs integration tests for the no-op skip, metadata-refresh persist, and theexistsSyncfalse-negative regression.log.info("Imported accounts", ...)fires even on the no-op (changed === false) path;__testOnly.namedBackupContainmentFsis exported unconditionally into the production module surface; the comment onhaveEquivalentAccountRowsslightly mischaracterises the dedup ordering guarantee; the pre-confirm re-assessment reads the directory and file fresh even though a warmcandidateCachecould avoid the extra file read on windows.Confidence Score: 3/5
Important Files Changed
changedfield, fixes negative import count bug, adds no-op skip). key issues: log.info fires on no-op path, __testOnly exports internal containment shim into production build.Sequence Diagram
sequenceDiagram participant User participant CLI as runCodexMultiAuthCli participant Manager as runBackupRestoreManager participant Storage as storage.ts participant FS as filesystem User->>CLI: codex auth restore-backup (or login menu) CLI->>CLI: setStoragePath(null) CLI->>Manager: runBackupRestoreManager(displaySettings) Manager->>Manager: isNonInteractiveMode / TTY check Manager->>Storage: listNamedBackups({ candidateCache }) Storage->>FS: readdir(backupRoot) Storage->>FS: retryTransientFilesystemOperation(readFile) × N (chunked, NAMED_BACKUP_LIST_CONCURRENCY=8) Storage-->>Manager: NamedBackupMetadata[] Manager->>Storage: assessNamedBackupRestore(name, { currentStorage, candidateCache }) × M (chunked, NAMED_BACKUP_ASSESS_CONCURRENCY=4) Note over Manager,Storage: cache hit from listNamedBackups — avoids re-read Storage-->>Manager: BackupRestoreAssessment[] Manager->>User: select(backupItems) — menu with eligibility hints User-->>Manager: { type: "restore", assessment } Manager->>Storage: assessNamedBackupRestore(name, { currentStorage: fresh loadAccounts() }) Note over Manager,Storage: fresh re-assessment — no cache — reflects concurrent changes Storage->>FS: readdir + readFile (fresh) Storage-->>Manager: latestAssessment Manager->>User: confirm(mergedAccountCount diff message) User-->>Manager: true Manager->>Storage: restoreAssessedNamedBackup(latestAssessment) Storage->>Storage: assertNamedBackupRestorePath(path, backupRoot) Note over Storage: containment check via lstat + realpath Storage->>Storage: importAccounts(resolvedPath) Storage->>FS: retryTransientFilesystemOperation(readFile) Storage->>Storage: withAccountStorageTransaction → haveEquivalentAccountRows → persist if changed Storage-->>Manager: ImportAccountsResult { imported, skipped, total, changed } Manager->>CLI: autoSyncActiveAccountToCodex() Manager-->>User: success messageComments Outside Diff (14)
lib/ui/copy.ts, line 1739-1741 (link)"u"in fallback prompt is never handled — causes an infinite re-prompt loopthe fallback readline handler in
lib/cli.ts(around line 188–239) has explicit mappings for"a","c","b","x","s","d","g","f","r", and"q", but there is no branch for"u". a user in fallback mode (non-TUI terminal) who reads the prompt, types"u", and presses enter will receive:…which lists
"u"as valid, so they try it again — infinite loop with no escape other thanCtrl-Cor guessing"q".there are two ways to fix this:
option A — remove
"u"from both the prompt string and theinvalidModePromptin the fallback copy (restore-backup requires a TUI and is exposed as a dedicated sub-commandcodex auth restore-backup):option B — add a handler for
"u"in the fallback path ofpromptLoginModethat returns{ mode: "restore-backup" }so the caller can surface the "requires interactive TTY" message and continue.Prompt To Fix With AI
lib/storage.ts, line 1300-1347 (link)Sync
lstatSync/realpathSyncinsideresolvePathForNamedBackupContainmentblocks the event loopnamedBackupContainmentFs.lstatand.realpathcalllstatSync/realpathSync.nativesynchronously insideresolvePathForNamedBackupContainment. this function is called fromassertNamedBackupRestorePath, which in turn is called concurrently across up toNAMED_BACKUP_LIST_CONCURRENCY = 8chunks insidelistNamedBackups. on windows the sync realpath call can take 50–200 ms under an AV scan — doing this 8 times concurrently blocks the event loop and freezes the entire interactive restore menu until all chunks finish. the__testOnlyhook exposesnamedBackupContainmentFsso tests can spy on it, which suggests the design intended mockability; usingfs.lstat/fs.realpath(async) here and awaiting them in a serial containment check would keep the loop unblocked and be no harder to test.this also means no vitest coverage exists for the race between
lstatcompleting and an AV lock being released mid-walk — a scenario specifically called out in the codebase's own comments.lib/codex-manager.ts, line 640-651 (link)Re-assessment passes a stale
candidateCache-less call — cache eviction leaks tokens to disk twicethe re-assessment on line 642 calls
assessNamedBackupRestorewithoutcandidateCache, soloadBackupCandidatere-reads the file from disk. that is intentional (fresh snapshot). however,assessNamedBackupRestorealso callscandidateCache?.delete(backupPath)when a cache is passed to evict after use — without a cache, the candidate is loaded a second time, parsed, and then immediately discarded. on windows with token files that include refresh tokens, this means two separatefs.readFilecalls for the same path both hold the file contents in memory simultaneously for the duration of the second JSON parse. if an AV scanner triggers aEBUSYduring that second parse (not wrapped inretryTransientFilesystemOperationhere), the user sees an unexplained "Restore failed" after successfully passing the menu.consider passing an isolated one-shot cache or wrapping this second read in the same retry wrapper used by
loadBackupCandidate.lib/ui/auth-menu.ts, line 1688-1699 (link)"Recovery" heading item maps to
cancel— keyboard navigation skips it correctly but focus-key collision is silentauthMenuFocusKeyfor"cancel"returns the same key as"cancel"in the switch, and the heading item is also givenvalue: { type: "cancel" }. if the user had previously highlighted the "Cancel" item and the menu is re-rendered (e.g. after a background quota refresh),findFocusedItemwill match the firstcancel-keyed item — which may now be this heading separator — not the original cancel button. this can cause the keyboard cursor to silently jump to the heading on re-render. the heading should carry a distinct non-actionable sentinel like{ type: "heading" }or an unused type constant so focus restoration doesn't collide.lib/storage.ts, line 1093-1112 (link)eligibleForRestoreisfalsewhenimported === 0butchanged === true— metadata-only restores are silently blockedwhen a backup contains accounts that all already exist by ID but with updated tokens/metadata,
importedis0,changedistrue, andwouldExceedLimitisfalse. the current formula is:so a metadata-refresh-only backup is correctly eligible here. however the UI in
runBackupRestoreManagercheckslatestAssessment.eligibleForRestoreafter the re-assessment, andrestoreAssessedNamedBackupthrows if!eligibleForRestore— so that path is fine.but
assessNamedBackupRestoreearly-returnseligibleForRestore: falsewhen!backup.accountCount || backup.accountCount <= 0. a backup containing one account with updated tokens but noaccountCountwritten in its metadata (e.g. an older export format) would hit this branch and be marked ineligible, discarding a valid metadata-refresh restore silently. the guard should be oncandidate.normalized === null, not onbackup.accountCount.lib/storage.ts, line 2232-2238 (link)sync containment fs blocks event loop — no retry for transient EPERM/EBUSY
lstatSyncandrealpathSync.nativerun synchronously insideresolvePathForNamedBackupContainment, which is called per-entry duringlistNamedBackups(up toNAMED_BACKUP_LIST_CONCURRENCY = 8entries in flight). on windows with an active AV scanner, a singlelstatSynccall can stall for hundreds of milliseconds, blocking the entire event loop. more importantly, when the AV holds a transient lock,lstatSyncthrowsEPERM, which is caught and re-thrown asBackupPathValidationTransientError— but there is no automatic retry at this level. the asyncretryTransientFilesystemOperationwrapper exists for all async reads in this file, but the containment check bypasses it entirely. a user hitting a transient lock on any backup in the listing sees "Backup path validation failed. Try again." and must manually retry the whole operation.if the sync calls are intentional for reentrancy/simplicity, at minimum document that gap explicitly. if not, the containment check could be made async and wrapped in
retryTransientFilesystemOperationlike the other disk operations.lib/codex-manager.ts, line 671-672 (link)changed === falsebranch skips sync butautoSyncActiveAccountToCodexstill needed for metadata-refresh pathwhen
result.changed === falsethe function returnstruewithout callingautoSyncActiveAccountToCodex. this is correct for a pure no-op (all accounts already identical). however, the branch is reachable only via a race:assessNamedBackupRestoresaidchanged = true(eligible) just before confirmation, but by the timeimportAccountscommitted, another writer had already merged the same data. in that race the codex cli auth state is consistent because the second writer should have synced. if that second writer was a non-interactive path that never syncs (e.g. a rawimportAccountscall), the cli state could be stale. this is low-probability, but worth a comment noting the deliberate omission of sync here, so future reviewers don't add it without understanding thechanged === falsesemantics.lib/storage.ts, line 1227-1233 (link)Sync
lstatSync/realpathSyncblock event loop on WindowsnamedBackupContainmentFsuses synchronous FS calls inside async code paths. on Windows under AV scanner or file-indexer pressure, bothlstatSyncandrealpathSync.nativecan hang for hundreds of milliseconds before throwingEPERM/EBUSY. every call goes throughresolvePathForNamedBackupContainment, which is reached once per backup file duringlistNamedBackupsand again duringassessNamedBackupRestore. with e.g. 8 backups andNAMED_BACKUP_LIST_CONCURRENCY = 8, all 8 concurrent chunks call these sync functions simultaneously — all on the same event loop thread, potentially adding up to seconds of visible freeze in the interactive restore terminal.the existing retry mechanism (
retryTransientNamedBackupPathValidation→BackupPathValidationTransientError) correctly recovers after the fact, but the initial sync call still blocks. replacing both fields with async equivalents (fs.lstat/fs.realpathvia the existingretryTransientFilesystemOperationwrapper) would keep the TOCTOU guarantee while staying non-blocking.suggest making both fields async-compatible so the
__testOnlyswap pattern still works but callers can be properly awaited.lib/storage.ts, line 2177-2178 (link)Windows junction points silently break all backup restore
on windows,
mklink /Jjunction points are reported bylstat().isSymbolicLink()astrue— exactly the same as a symlink. this means any user who redirected their~/.codex/multi-auth/backupsdir to external storage via a junction (a very common windows pattern) getsBackupContainmentErrorfor every single restore attempt. the error message"Backup path escapes backup directory"is especially confusing here because the backup path doesn't escape anything — the issue is the root itself is a junction.the canonical path check that follows (lines 2180-2192) already handles path traversal via
realpath+relative(), so the symlink-root guard is redundant for containment security. if the intent is strictly "no redirected roots", the error message should reflect that explicitly.at minimum, the message should distinguish between a root-is-junction rejection and an actual path-traversal rejection so windows users aren't left chasing a red herring:
lib/storage.ts, line 3151-3159 (link)restoreAssessedNamedBackupskips containment re-validation before importingthe function calls
importAccounts(assessment.backup.path)directly with no check that the path is still inside the backup directory.importAccountsitself has no containment guard either, so any caller that constructs aBackupRestoreAssessmentwith an arbitrarybackup.path— or one whose canonical location has shifted since assessment (e.g. the backup root was remounted on windows between assess and import) — can read from outside the designated backup directory without triggering aBackupContainmentError.the fix is to call
assertNamedBackupRestorePath(orresolveNamedBackupRestorePath) on the path before delegating toimportAccounts:this is especially important on windows where the backup root can be on a drive that gets remapped or where AV software can briefly make a symlink appear as a regular file during the window between
assessNamedBackupRestoreand this call.lib/storage.ts, line 3143-3196 (link)silent active-account shift when backup has accounts in a different order
haveEquivalentAccountRowscompares the two arrays index-by-index.deduplicateAccounts([...existingDeduped, ...incomingDeduped])keeps the last occurrence, so if the backup lists the same accounts in a different order,mergedAccountswill be ordered by the backup's arrangement.changedbecomestrue, the restore proceeds, accounts are reordered in storage, andactiveIndex(which is preserved as-is fromexistingActiveIndex) now points to a different account silently.example:
[A(idx=0, active), B(idx=1)][B, A](same data, different order)[B, A]— order follows the backupactiveIndexstays 0 → now points to B instead of Athe impact is a silent active-account change without any user confirmation that this happened. since the confirmation message only shows the import/skipped counts and not any "account order will change" warning, the user has no signal.
consider normalising account order before comparison in
haveEquivalentAccountRows, or adding a dedicated "order-only change" detection path that either suppresses the restore or warns the user. at minimum, updateactiveIndexafter the merge so it still points to the same account by ID rather than by position.lib/storage.ts, line 3169-3196 (link)importAccountsnow skipspersist()on duplicate-only imports — silent behavioural break for existing callersthe new early return:
means a call to
importAccountswith a file containing only accounts already in storage no longer writes anything to disk. before this PR,importAccountsalways calledpersist, even for identical content.callers that relied on the write as a side-effect (e.g. to bump the file's
mtime, trigger a filesystem watcher, or force a WAL flush) will silently see no write. the public-api.md documentschangedas additive and the existing tests have been updated, but any caller outside this repo that callsimportAccountsand expects a write on every call will be broken with no error or warning.changedis typed aschanged?: boolean(optional) so external callers don't need to model it — but they also can't tell whether the no-write was intentional or a failure. consider either:persist) even for no-op imports (old behaviour, safe), ordocs/upgrade.md(the governance checklist shows that file was not updated for this PR)lib/storage.ts, line 3209-3215 (link)log.infofires on no-op import pathwhen
changed === false, the transaction callback returns early without callingpersist(), butlog.info("Imported accounts", ...)still fires withimported: 0, changed: false. log aggregation systems will count this as an import event even when nothing was written. rename or suppress the log for the no-op path:lib/codex-manager.ts, line 640-645 (link)re-assessment re-reads directory and file on every selection
the pre-confirm re-assessment at this point calls
assessNamedBackupRestorewithout acandidateCache. internally it callsfindExistingNamedBackupPath(freshreaddir) andloadBackupCandidate(freshreadFile). that means each user selection triggers at least 3 i/o passes per backup: one duringlistNamedBackups, one from the cache during batch assessment, and one fresh here.on windows with av scanning, a third uncached
readFileon the same file could hit anEBUSYand trigger the full retry loop (up to ~1.26 s). theretryTransientNamedBackupPathValidationinsideresolveNamedBackupRestorePathwraps the path check but not the file read that happens vialoadBackupCandidate. passing a short-livedcandidateCacheto the re-assessment call would avoid the extra read while still reflecting any mtime change:(note: an empty
Mapis still safe here —getCachedNamedBackupCandidatejust misses and falls through toloadBackupCandidate, so behavior is identical; the point is that future callers who pass a warm cache benefit without any code path change.)Prompt To Fix All With AI
Last reviewed commit: "fix(restore): revali..."