feat(auth): prompt for recovery on startup#77
feat(auth): prompt for recovery on startup#77ndycode wants to merge 28 commits intogit-plan/02-backup-restore-managerfrom
Conversation
…artup-recovery-prompt-devtools # Conflicts: # test/codex-manager-cli.test.ts
|
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:
📝 WalkthroughWalkthroughadds an interactive startup recovery prompt before oauth when no accounts exist and recoverable named backups are present; centralizes interactivity checks via Changes
Sequence Diagram(s)sequenceDiagram
participant user as user
participant login as login flow\n(lib/codex-manager.ts)
participant storage as backup storage\n(lib/storage.ts)
participant ui as restore manager\n(ui)
participant oauth as oauth server
user->>login: start login (no accounts)
login->>login: check isInteractiveLoginMenuAvailable()\n(lib/cli.ts)
alt interactive
login->>storage: getActionableNamedBackupRestores()
storage->>storage: scan & assess named backups\n(lib/storage.ts:line)
storage-->>login: actionable backups found
login->>user: prompt "restore before oauth?"
alt user confirms
user-->>login: confirm
login->>ui: launch restore manager
ui->>ui: perform restore
ui-->>login: restored
else user skips
user-->>login: skip
end
end
login->>oauth: proceed to oauth flow
oauth-->>user: oauth complete
sequenceDiagram
participant user as user
participant login as login flow\n(lib/codex-manager.ts)
user->>login: trigger fresh/deleteAll or reset
login->>login: set suppressRecoveryPrompt = true\n(lib/codex-manager.ts:line)
login->>login: perform destructive action
login->>login: skip recovery prompt due to suppressRecoveryPrompt
login-->>user: continue to fresh login
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes review notes
🚥 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 unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/codex-manager.ts (1)
4308-4334:⚠️ Potential issue | 🟠 Majorset
suppressRecoveryPromptonly after the destructive action succeeds.
lib/codex-manager.ts:4308-4334flips the flag beforedeleteSavedAccounts()/resetLocalState()run. if either path fails — for example a windowsebusy/epermduring storage cleanup — the same session will still skip the restore prompt even though the reset never completed.proposed fix
if (menuResult.mode === "fresh" && menuResult.deleteAll) { - suppressRecoveryPrompt = true; await runActionPanel( DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.label, DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts.stage, @@ }, displaySettings, ); + suppressRecoveryPrompt = true; continue; } if (menuResult.mode === "reset") { - suppressRecoveryPrompt = true; await runActionPanel( DESTRUCTIVE_ACTION_COPY.resetLocalState.label, DESTRUCTIVE_ACTION_COPY.resetLocalState.stage, @@ }, displaySettings, ); + suppressRecoveryPrompt = true; continue; }as per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4308 - 4334, The code sets suppressRecoveryPrompt before running destructive actions; change the flow so suppressRecoveryPrompt is set only after the destructive action completes successfully: in the branches using runActionPanel for DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts and DESTRUCTIVE_ACTION_COPY.resetLocalState, remove the pre-action assignment to suppressRecoveryPrompt, run the async action (await deleteSavedAccounts()/resetLocalState() within runActionPanel), and only on successful completion set suppressRecoveryPrompt = true (and then continue); ensure exceptions from deleteSavedAccounts/resetLocalState are propagated or logged (do not set the flag on error), update or add vitest coverage for these code paths to assert the flag is only toggled on success and that EBUSY/EPERM-like errors leave the flag false, and ensure any logging inside these flows does not leak tokens/emails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started.md`:
- Line 52: Update the docs sentence to reflect the actual gate: state that the
restore prompt appears only when there are zero saved accounts (not “no active
accounts”) and that it is interactive-only and suppressed after in-session
destructive resets (i.e., align wording with the startup prompt logic in the CLI
entry and the CodexManager restore prompt logic). Also add an upgrade-note entry
describing the new startup prompt behavior and mention the new npm scripts, and
ensure README and SECURITY are updated to remain consistent with the current CLI
flags and workflows.
In `@lib/storage.ts`:
- Around line 1327-1339: The current code reopens every backup by calling
assessNamedBackupRestore for each item in Promise.all, doubling I/O and risking
EMFILE/EBUSY; change this to reuse the results from listNamedBackups() (have
listNamedBackups return validated backup objects/metadata and pass those into
assessNamedBackupRestore) or, if reuse is not possible, replace the unbounded
Promise.all over backups.map(...) with a bounded concurrency queue (e.g.,
p-limit or an internal semaphore) when invoking assess(backup.name, {
currentStorage }), and ensure assessNamedBackupRestore and assess accept the
cached backup object to avoid revalidation; update tests in
test/recovery.test.ts to exercise the reused-path and add cases simulating
EBUSY/429 and verify no token/email leaks in logs.
In `@test/codex-manager-cli.test.ts`:
- Around line 495-514: Add coverage for the true non-interactive startup path by
creating (or extending) a test that sets interactive TTY off: call
setInteractiveTTY(false) before importing and running
runCodexMultiAuthCli(["auth","login"]) (instead of setInteractiveTTY(true)),
keep isInteractiveLoginMenuAvailableMock.mockReturnValue(false) and the other
mocks, and then assert exitCode is 0 and that
getActionableNamedBackupRestoresMock and confirmMock were not called and
createAuthorizationFlowMock was invoked once and restoreNamedBackupMock was not
called; reference the existing test flow around runCodexMultiAuthCli,
isInteractiveLoginMenuAvailableMock, getActionableNamedBackupRestoresMock,
confirmMock, createAuthorizationFlowMock, and restoreNamedBackupMock to locate
where to add the tty-off case.
- Around line 427-493: Add a regression test for the startup-recovery decline
path by duplicating the existing "offers startup restore before OAuth when
interactive login starts empty" case but have confirmMock.resolve to false for
the recovery prompt (and selectMock still returning the restore option), then
assert that restoreNamedBackupMock is NOT called and createAuthorizationFlowMock
(and the OAuth flow) IS invoked; locate the test around runCodexMultiAuthCli in
test/codex-manager-cli.test.ts and use the same mocks (loadAccountsMock,
getActionableNamedBackupRestoresMock, listNamedBackupsMock,
assessNamedBackupRestoreMock, selectMock, confirmMock) to simulate decline and
verify fallback to createAuthorizationFlowMock rather than early exit.
- Around line 448-454: The test currently flips loadAccountsMock after three
calls by counting reads, which makes the assertion depend on call order;
instead, make the storage transition driven by restoreNamedBackupMock: add a
flag (e.g., restored = false) that restoreNamedBackupMock sets to true when
invoked, and change loadAccountsMock.mockImplementation to return
structuredClone(restoredStorage) when that flag is true (otherwise
structuredClone(emptyStorage)); reference loadAccountsMock,
restoreNamedBackupMock, emptyStorage and restoredStorage when making this change
so the test validates actual restore behavior rather than call count.
In `@test/recovery.test.ts`:
- Around line 1290-1357: The test block using describe.skip around the
getActionableNamedBackupRestores coverage must be enabled or removed: change
describe.skip to describe so the test runs (or delete this duplicate block if
it's intentionally duplicate), ensure the test calls
storage.getActionableNamedBackupRestores and retains the vi.fn assess mock and
assertions (expect(result.totalBackups)...,
expect(result.assessments.map(...))..., expect(assess).toHaveBeenCalledTimes(2))
so it becomes a real deterministic Vitest test exercising
getActionableNamedBackupRestores with the mockBackups and assess
implementations.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 4308-4334: The code sets suppressRecoveryPrompt before running
destructive actions; change the flow so suppressRecoveryPrompt is set only after
the destructive action completes successfully: in the branches using
runActionPanel for DESTRUCTIVE_ACTION_COPY.deleteSavedAccounts and
DESTRUCTIVE_ACTION_COPY.resetLocalState, remove the pre-action assignment to
suppressRecoveryPrompt, run the async action (await
deleteSavedAccounts()/resetLocalState() within runActionPanel), and only on
successful completion set suppressRecoveryPrompt = true (and then continue);
ensure exceptions from deleteSavedAccounts/resetLocalState are propagated or
logged (do not set the flag on error), update or add vitest coverage for these
code paths to assert the flag is only toggled on success and that
EBUSY/EPERM-like errors leave the flag false, and ensure any logging inside
these flows does not leak tokens/emails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f95895b7-9f08-4e7b-ba36-8baa8d962b68
📒 Files selected for processing (7)
docs/getting-started.mddocs/troubleshooting.mdlib/cli.tslib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/recovery.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/codex-manager.tslib/storage.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/getting-started.mddocs/troubleshooting.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/recovery.test.tstest/codex-manager-cli.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/recovery.test.ts">
<violation number="1" location="test/recovery.test.ts:1290">
P3: Avoid committing this suite as `describe.skip`; it disables coverage for backup-restore behavior and can hide regressions.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:1337">
P2: Avoid failing the entire backup scan when one backup assessment throws. A single unreadable backup should be skipped so other recoverable backups can still be offered.</violation>
<violation number="2" location="lib/storage.ts:1338">
P2: Passing `currentStorage: null` into `assessNamedBackupRestore` causes it to reload accounts for every backup. Normalize `null` before calling `assess` so assessments share one in-memory storage value.</violation>
</file>
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4376">
P1: Wrap startup recovery scan in a try/catch so backup-read failures degrade to the normal login path instead of aborting `auth login`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 4387-4389: The startup recovery scan catch block currently
swallows all errors and quietly sets recoveryState, which hides Windows
EBUSY/EPERM failures; modify the catch to capture the error (e.g., catch (err))
and emit a warning or error log including error.message/stack via the existing
logger (mentioning the startup recovery scan and recoveryState fallback), then
keep the fallback assignment; update/add vitest cases to simulate Windows
EBUSY/EPERM and HTTP 429 to ensure the new logging and queue retry behavior
handles those scenarios and update any affected queue logic to retry/backoff on
EBUSY/429 as required.
- Around line 4111-4115: The loop that awaits assessNamedBackupRestore for each
backup can be aborted by a single thrown error; change the logic in the restore
manager where backups are iterated (the for (const backup of backups) block
calling assessNamedBackupRestore) to wrap each await in a per-item try/catch so
a failing backup (e.g., EBUSY/EPERM or malformed data) is caught, logged, and
the loop continues; collect failed backup names into a failures array or map and
surface them after the loop, and add/adjust vitest cases to assert that a single
assessNamedBackupRestore rejection does not stop processing of remaining backups
and that the new flow retries or queues transient errors (EBUSY/429)
appropriately (update any queues used for restore attempts to handle EBUSY/429
backoff/retry).
In `@test/codex-manager-cli.test.ts`:
- Around line 744-771: Add a regression test in test/codex-manager-cli.test.ts
that mirrors the existing "falls back to OAuth when startup recovery scan
throws" case but makes getActionableNamedBackupRestoresMock.reject with a
windows-style errno error (use makeErrnoError("resource busy", "EBUSY") and also
include a variant for "EPERM" if desired), keep setInteractiveTTY(true),
promptLoginModeMock.mockResolvedValueOnce({ mode: "cancel" }) and await
configureSuccessfulOAuthFlow(now), then import and call
runCodexMultiAuthCli(["auth","login"]) and assert exitCode === 0,
getActionableNamedBackupRestoresMock called once, confirmMock and
restoreNamedBackupMock not called, and createAuthorizationFlowMock called once;
this ensures assessNamedBackupRestore concurrency errors fall back to OAuth
without crashing.
In `@test/recovery.test.ts`:
- Around line 139-302: Add a deterministic vitest that exercises
getActionableNamedBackupRestores without an injected assess (to hit the default
scan/assess path) and simulates a Windows-style EBUSY failure for one backup
while leaving another valid; specifically, import the storage module, stub the
underlying file-read/load used by the default assessment (e.g., spyOn
fs.promises.readFile or the internal loader function used by
getActionableNamedBackupRestores) to throw an Error with code "EBUSY" for the
failing backup path and return valid data for the other backup, call
getActionableNamedBackupRestores({backups: [...]}) and assert totalBackups
equals the mock length and that the returned assessments include the valid
backup only, verify the loader was called for both backups, and restore mocks at
test end to keep the test deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4eaf19de-2526-4a5b-81e1-1d98d882fb59
📒 Files selected for processing (9)
docs/getting-started.mddocs/upgrade.mdlib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/fetch-helpers.test.tstest/public-api-contract.test.tstest/recovery.test.tstest/request-transformer.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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/upgrade.mddocs/getting-started.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/request-transformer.test.tstest/fetch-helpers.test.tstest/codex-manager-cli.test.tstest/public-api-contract.test.tstest/recovery.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.ts
🔇 Additional comments (14)
docs/getting-started.md (1)
52-53: docs wording now matches runtime behavior.this is aligned with the startup gates in
lib/codex-manager.ts:4373-4378andlib/cli.ts:26-29.docs/upgrade.md (1)
65-70: good upgrade-note coverage for the new login flow.this section is consistent with the recovery gating implemented in
lib/codex-manager.ts:4373-4414.lib/storage.ts (2)
1295-1313: nice change: backup scan now carries loaded candidates through.
lib/storage.ts:1295-1313reduces redundant reopen/parse work and keeps the scan path safer for windows file-lock pressure.
1365-1383: good resilience in actionable filtering path.
lib/storage.ts:1365-1383keeps processing when a single backup assessment fails, which is the right behavior for startup recovery.test/request-transformer.test.ts (1)
1-5: lgtm on the host-codex-prompt mock.the mock is hoisted correctly before imports and returns a deterministic cached prompt. this matches the pattern in
test/fetch-helpers.test.ts:2-5andtest/public-api-contract.test.ts:2-5.test/fetch-helpers.test.ts (1)
2-5: lgtm on the host-codex-prompt mock.deterministic mock setup, consistent with other test files.
test/public-api-contract.test.ts (1)
2-5: lgtm on the host-codex-prompt mock.consistent with the other test files.
test/codex-manager-cli.test.ts (7)
530-580: good addition of the decline-recovery regression.this test at
test/codex-manager-cli.test.ts:530correctly verifies that when the user declines startup recovery (confirmMock.mockResolvedValue(false)), the flow continues into oauth (createAuthorizationFlowMockis called) and restore is skipped. this addresses the previous review feedback about missing decline path coverage.
718-742: good coverage for the true non-interactive path.this test at
test/codex-manager-cli.test.ts:718setssetInteractiveTTY(false)andisInteractiveLoginMenuAvailableMock.mockReturnValue(false), then asserts thatgetActionableNamedBackupRestoresMockandconfirmMockare never called while oauth proceeds. this addresses the previous review feedback about needing a tty-off case.
484-518: restore side effect now drives storage state correctly.the test at
test/codex-manager-cli.test.ts:515-518wiresrestoreNamedBackupMock.mockImplementationto flipstorageStatetorestoredStorage, so the test validates actual restore behavior rather than call ordering. this addresses the previous review feedback about making the test a real regression check.
305-308: default mock for getActionableNamedBackupRestores looks correct.setting
assessments: []andtotalBackups: 0as the default ensures tests that don't care about recovery prompts won't be affected.
236-270: configureSuccessfulOAuthFlow helper is clean.centralizes the oauth mock setup to avoid duplication across tests. the mock values are deterministic and the helper correctly awaits the dynamic imports.
773-820: parameterized suppression test uses loadCount counter - acceptable but slightly fragile.the test at
test/codex-manager-cli.test.ts:801-807usesloadCount <= 2to flip from populated to empty storage. this is a call-ordering assertion rather than action-driven state, but it's acceptable here since the test is verifying thatgetActionableNamedBackupRestoresMockis never called (i.e., the suppression guard fires before the scan). the storage flip simulates the post-wipe state correctly.
463-528: startup restore happy path test is solid.the test at
test/codex-manager-cli.test.ts:463validates the full flow: actionable backups exist → user confirms → select restore → restore executes → storage updated → oauth skipped. good coverage for the main pr objective.
…artup-recovery-prompt-devtools
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
test/recovery.test.ts (1)
375-444:⚠️ Potential issue | 🟠 Majorthis ebusy case still bypasses the startup fast path.
test/recovery.test.ts, Lines 431-434 passbackups, so this test driveslib/storage.ts, Lines 1393-1405 instead of the no-args fast path atlib/storage.ts, Lines 1373-1390 thatlib/codex-manager.ts, Lines 4401-4403 uses on startup. we still need a storage-backedebusyregression that faults during the scan path itself and proves one locked backup does not hide another actionable backup.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/recovery.test.ts` around lines 375 - 444, The test currently passes the backups array into getActionableNamedBackupRestores which exercises the non-startup code path; change the test to exercise the storage-backed scan path by calling storage.getActionableNamedBackupRestores() with no args (so it calls storage.listNamedBackups internally) and keep the fs.readFile spy that throws NodeJS.ErrnoException with code "EBUSY" for the locked backup path; this ensures the EBUSY fault occurs during the scan path in storage.getActionableNamedBackupRestores and verifies one locked backup does not hide the other actionable backup (refer to getActionableNamedBackupRestores and listNamedBackups in lib/storage.ts).
🤖 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 4390-4429: The pre-OAuth recovery prompt can throw UI errors and
must not abort login; wrap the confirm(...) and subsequent
runBackupRestoreManager(...) handling in a local try/catch so any thrown errors
(from confirm, runBackupRestoreManager, or UI libs) are caught, logged/warned,
and the code continues down the OAuth path instead of failing; locate the block
guarded by isInteractiveLoginMenuAvailable() and
getActionableNamedBackupRestores(...) and modify the branch that inspects
recoveryState.assessments to catch UI/CLI errors and treat them as a benign
fallback to OAuth, and update test/recovery.test.ts to add/adjust a case
exercising the forced-interactive/non-tty CLI error path.
- Around line 4193-4197: The call to restoreNamedBackup(assessment.backup.name)
must not be allowed to throw and abort the backup browser/startup flow; wrap the
await in a try/catch inside the surrounding function (the block that currently
logs and returns "restored"), catch filesystem/concurrency errors (EBUSY/EPERM)
and transient rate errors (429), implement a small retry/backoff or queue retry
hook, log the error with assessment.backup.name and error details via
console/processLogger, and return a non-fatal result (e.g., "restored_failed" or
"skipped") so the caller does not teardown the browser or login flow;
update/extend test/recovery.test.ts to cover the restore failure path and
EBUSY/EPERM/429 retry behavior.
In `@test/codex-manager-cli.test.ts`:
- Around line 1702-1707: The test currently only verifies oauth was skipped
(checks like getActionableNamedBackupRestoresMock and confirmMock) but doesn't
assert the actual restore side-effect; update the test to assert the restore
executed by adding an expectation that the restore handler was invoked (e.g.,
expect(restoreBackupMock or performRestoreMock).toHaveBeenCalled() or expect(the
specific restore function used by the cli command) and/or assert the observable
effect (e.g., mocked filesystem/write calls or returned restore status changed)
after invoking the command; locate the assertions near the existing checks
referencing getActionableNamedBackupRestoresMock, confirmMock, selectMock, and
createAuthorizationFlowMock and add the new expectation(s) there to guarantee
the restore ran.
In `@test/recovery.test.ts`:
- Around line 1-4: Add a top-level cleanup to restore vitest spies after each
test so any vi.spyOn(...) created in recovery.test.ts (the spies referenced
around the failing cases) cannot leak into other tests: add or update an
afterEach hook (near the test file's top imports) to call vi.restoreAllMocks()
(or restore specific spies if they are named) and optionally
vi.resetAllMocks()/vi.resetModules() as needed to fully reset mocked fs/storage
behavior between tests; ensure the new afterEach runs for every test path, not
only the happy path.
---
Duplicate comments:
In `@test/recovery.test.ts`:
- Around line 375-444: The test currently passes the backups array into
getActionableNamedBackupRestores which exercises the non-startup code path;
change the test to exercise the storage-backed scan path by calling
storage.getActionableNamedBackupRestores() with no args (so it calls
storage.listNamedBackups internally) and keep the fs.readFile spy that throws
NodeJS.ErrnoException with code "EBUSY" for the locked backup path; this ensures
the EBUSY fault occurs during the scan path in
storage.getActionableNamedBackupRestores and verifies one locked backup does not
hide the other actionable backup (refer to getActionableNamedBackupRestores and
listNamedBackups in lib/storage.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bf27991-cefa-4bef-bf1a-400915419c3f
📒 Files selected for processing (4)
lib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/recovery.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (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/codex-manager.tslib/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/recovery.test.tstest/codex-manager-cli.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/recovery.test.ts (1)
445-514:⚠️ Potential issue | 🟡 Minorrestore the
fs.readfilespy in this test.
test/recovery.test.ts:489-514createsreadFileSpyand never restores it. this can leak mocked fs behavior into later cases and make file order matter.proposed fix
const originalReadFile = fs.readFile.bind(fs); const readFileSpy = vi.spyOn(fs, "readFile").mockImplementation( (async (...args: Parameters<typeof fs.readFile>) => { const [path] = args; if (path === lockedBackup?.path) { const error = new Error("resource busy") as NodeJS.ErrnoException; error.code = "EBUSY"; throw error; } return originalReadFile(...args); }) as typeof fs.readFile, ); - - const result = await storage.getActionableNamedBackupRestores({ - backups, - currentStorage: emptyStorage, - }); - - expect(result.totalBackups).toBe(2); - expect(result.assessments.map((item) => item.backup.name)).toEqual([ - "valid-backup", - ]); - expect(readFileSpy).toHaveBeenCalledTimes(2); - expect(readFileSpy.mock.calls.map(([path]) => path)).toEqual( - expect.arrayContaining([lockedBackup?.path, validBackup?.path]), - ); + try { + const result = await storage.getActionableNamedBackupRestores({ + backups, + currentStorage: emptyStorage, + }); + + expect(result.totalBackups).toBe(2); + expect(result.assessments.map((item) => item.backup.name)).toEqual([ + "valid-backup", + ]); + expect(readFileSpy).toHaveBeenCalledTimes(2); + expect(readFileSpy.mock.calls.map(([path]) => path)).toEqual( + expect.arrayContaining([lockedBackup?.path, validBackup?.path]), + ); + } finally { + readFileSpy.mockRestore(); + } });As per coding guidelines,
test/**: tests must stay deterministic and use vitest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/recovery.test.ts` around lines 445 - 514, The test sets a spy readFileSpy via vi.spyOn(fs, "readFile") but never restores it, leaking the mock into other tests; update the "keeps actionable backups when default assessment hits EBUSY" test (or the surrounding setup) to restore the spy after use by calling readFileSpy.mockRestore() (or using a try/finally around the async test body to ensure restoration) or move restoration to an afterEach that calls readFileSpy.mockRestore()/vi.restoreAllMocks(); reference readFileSpy and fs.readFile in the change so the spy is always cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/recovery.test.ts`:
- Around line 445-514: The test sets a spy readFileSpy via vi.spyOn(fs,
"readFile") but never restores it, leaking the mock into other tests; update the
"keeps actionable backups when default assessment hits EBUSY" test (or the
surrounding setup) to restore the spy after use by calling
readFileSpy.mockRestore() (or using a try/finally around the async test body to
ensure restoration) or move restoration to an afterEach that calls
readFileSpy.mockRestore()/vi.restoreAllMocks(); reference readFileSpy and
fs.readFile in the change so the spy is always cleaned up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2c34c27-c6c1-41a7-a52d-10244dda9115
📒 Files selected for processing (2)
lib/storage.tstest/recovery.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (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/recovery.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 (3)
test/recovery.test.ts (2)
45-61: good windows cleanup helper.
test/recovery.test.ts:45-61uses bounded retry withebusy/eperm/enotemptyhandling, which is the right shape for deterministic windows teardown behavior.
1320-1362: solid concurrency regression coverage here.
test/recovery.test.ts:1320-1362explicitly reproduces in-flight duplicate recovery calls and asserts first/second outcomes, which is a good deterministic race guard.lib/storage.ts (1)
1380-1450: fast/slow path split and per-backup isolation look correct.
lib/storage.ts:1387-1394avoids prereading backup contents when a custom assessor is injected, andlib/storage.ts:1435-1447prevents one failing backup from aborting the entire assessment pass. coverage aligns withtest/recovery.test.ts:376-443andtest/recovery.test.ts:445-514.
…artup-recovery-prompt-devtools
…overy-prompt-devtools
…er' into git-plan/03-startup-recovery-prompt-devtools # Conflicts: # lib/codex-manager.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/storage.ts (1)
1935-1946:⚠️ Potential issue | 🟠 Majorredact filesystem error details before persisting
loadError.in
lib/storage.ts:1935-1946,error: String(error)can include local absolute paths. that value is later surfaced via backup metadata hints inlib/codex-manager.ts:4380-4389, so path data can leak in ui output.proposed fix
+function getRedactedFilesystemErrorLabel(error: unknown): string { + const code = (error as NodeJS.ErrnoException).code; + if (typeof code === "string" && code.trim().length > 0) return code; + if (error instanceof Error && error.name && error.name !== "Error") { + return error.name; + } + return "UNKNOWN"; +} + async function loadBackupCandidate(path: string): Promise<LoadedBackupCandidate> { try { return await retryTransientFilesystemOperation(() => loadAccountsFromPath(path), ); } catch (error) { return { normalized: null, storedVersion: undefined, schemaErrors: [], - error: String(error), + error: getRedactedFilesystemErrorLabel(error), }; } }please also add a regression that asserts backup hint/error text does not expose absolute paths during failed backup reads (e.g.,
ENOENT/EBUSY) intest/recovery.test.ts.As per coding guidelines,
lib/**: "focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1935 - 1946, The loadBackupCandidate function currently returns error: String(error) which may include absolute filesystem paths; update loadBackupCandidate to sanitize/redact filesystem-specific details before persisting loadError by mapping the thrown error from loadAccountsFromPath into a safe message (e.g., normalize known errno codes like ENOENT/EBUSY and return a generic, non-path-containing string) and preserve the original error only in non-persisted debug logs; additionally add a vitest regression in test/recovery.test.ts that simulates failed backup reads (ENOENT/EBUSY) and asserts that the backup hint/error text produced by the codex manager (the place that surfaces loadBackupCandidate errors) does not contain absolute path fragments or drive letters.lib/codex-manager.ts (1)
4421-4424:⚠️ Potential issue | 🟠 Majorguard reassessment with local redacted handling before confirmation.
lib/codex-manager.ts:4421-4424can still throw on windows races (ebusy/eperm/enoent) after item selection. that escapes this function and is rendered with raw message inlib/codex-manager.ts:3935-3939, which can leak filesystem paths and skip the manager-local redaction path.proposed fix
- const latestAssessment = await assessNamedBackupRestore( - selection.assessment.backup.name, - { currentStorage: await loadAccounts() }, - ); + let latestAssessment: BackupRestoreAssessment; + try { + latestAssessment = await assessNamedBackupRestore( + selection.assessment.backup.name, + { currentStorage: await loadAccounts() }, + ); + } catch (error) { + const errorLabel = getRedactedFilesystemErrorLabel(error); + console.warn( + `failed to reassess backup "${selection.assessment.backup.name}" (${errorLabel}).`, + ); + return "dismissed"; + }please add a regression in
test/recovery.test.tsfor “backup removed/locked after menu selection” to ensure the manager returns"dismissed"and emits redacted labels only.As per coding guidelines,
lib/**: "focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4421 - 4424, Wrap the reassessment call to assessNamedBackupRestore (currently invoked with selection.assessment.backup.name and { currentStorage: await loadAccounts() }) in a try/catch that specifically handles filesystem concurrency errors (EBUSY/EPERM/ENOENT) and, on those errors, returns the manager-level result "dismissed" and emits only redacted labels via the existing local redaction path instead of rethrowing; ensure you call the same redaction helper used for the user-facing branch rather than letting the raw error bubble to the confirmation codepath (the code that logs at the confirmation/render site). Also add a vitest regression in test/recovery.test.ts named "backup removed/locked after menu selection" that simulates the backup being removed/locked after selection and asserts the manager returns "dismissed" and only redacted labels are emitted.
♻️ Duplicate comments (2)
test/codex-manager-cli.test.ts (1)
2305-2311: 🧹 Nitpick | 🔵 Trivialavoid call-count-driven storage transitions in this regression.
in
test/codex-manager-cli.test.ts:2305-2311, storage flips from populated to empty based onloadCount. this makes correctness depend on internal read ordering rather than the destructive action side effect.deterministic refactor
- let loadCount = 0; - loadAccountsMock.mockImplementation(async () => { - loadCount += 1; - return loadCount <= 2 - ? structuredClone(populatedStorage) - : structuredClone(emptyStorage); - }); + let destructiveApplied = false; + action.mockImplementationOnce(async (...args) => { + destructiveApplied = true; + return (await Promise.resolve(action.getMockImplementation()?.(...args))) ?? { + accountsCleared: true, + flaggedCleared: mode === "reset", + quotaCacheCleared: mode === "reset", + }; + }); + loadAccountsMock.mockImplementation(async () => + structuredClone(destructiveApplied ? emptyStorage : populatedStorage), + );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 2305 - 2311, The test currently flips storage based on a call counter (loadCount) inside loadAccountsMock which makes behavior order-dependent; replace this with deterministic control: either stub loadAccountsMock using a fixed sequence of returns via loadAccountsMock.mockResolvedValueOnce(populatedStorage) / mockResolvedValueOnce(...) for each expected call, or introduce an explicit storageState flag (e.g., let storageState = 'populated') that loadAccountsMock returns structuredClone(storageState === 'populated' ? populatedStorage : emptyStorage) and then toggle storageState in the specific destructive/mock of the mutating method (e.g., in the mock for the delete/overwrite function) so transitions occur only when that action is invoked rather than by call count. Ensure you reference loadAccountsMock, populatedStorage, emptyStorage, structuredClone and the mutating mock you already have so the test becomes deterministic.test/recovery.test.ts (1)
647-656:⚠️ Potential issue | 🟠 Majorwindows ebusy stat matcher targets the wrong backup filename.
in
test/recovery.test.ts:647-656, the normalized-path branch checks"/locked-backup.json", but this case createsfirst-backupandsecond-backup. if stat path equality does not hit, the windows-style fallback matcher will never trigger.proposed fix
- if ( - path === lockedBackup?.path || - normalizedPath.endsWith("/locked-backup.json") - ) { + if ( + path === lockedBackup?.path || + normalizedPath.endsWith("/first-backup.json") + ) {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/recovery.test.ts` around lines 647 - 656, The windows-style fallback in the fs.stat mock is checking for "/locked-backup.json" which doesn't match the test-created backups; update the normalizedPath.endsWith(...) check inside the stat mock (the block around originalStat, statSpy, fs.stat and lockedBackup?.path) to match the actual backup filenames created by the test (e.g., include endsWith("/first-backup") and endsWith("/second-backup") or the exact file names like "/first-backup.json" and "/second-backup.json") so the Windows-path branch triggers when those backup files are stat-ed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/troubleshooting.md`:
- Line 21: Update the troubleshooting line to reflect the real recovery gate:
say the restore prompt on `codex auth login` appears only when a recoverable
backup is detected (not merely “named backups”) as implemented in the
recoverable-backup logic in lib/codex-manager.ts (around the recoverable backups
flow), and that the prompt is suppressed for same-session fresh/reset actions
handled by the reset-suppression logic in lib/codex-manager.ts; also update
docs/** to add an upgrade note and mention any new npm scripts per the docs
guideline.
In `@lib/codex-manager.ts`:
- Around line 4045-4048: Add a focused vitest in test/recovery.test.ts that
simulates a recovery loop where recoveryState.assessments is an empty array and
recoveryScanFailed is false so the code path in codex-manager's loginFlow sets
allowEmptyStorageMenu = true and continues; assert that the next loop iteration
enters the empty-storage menu (e.g., by spying/mocking the function or branch
that renders the storage menu) and assert explicitly that no OAuth flow (oauth
entry point) is invoked on that iteration. When adding the test, reuse the same
setup helpers used by existing recovery tests, ensure mocks/queues used simulate
Windows IO/EBUSY/429 resilience per repo guidelines, and verify logs do not leak
tokens/emails while also referencing recoveryState, allowEmptyStorageMenu,
loginFlow, recoveryScanFailed, and assessments in the test assertions.
In `@test/codex-manager-cli.test.ts`:
- Around line 1735-2327: Add a deterministic test case that covers the branch
where getActionableNamedBackupRestores returns totalBackups > 0 but an empty
assessments array: mock getActionableNamedBackupRestoresMock.mockResolvedValue({
assessments: [], totalBackups: 2 }), setInteractiveTTY(true), have
loadAccountsMock return an empty storage state, promptLoginModeMock.resolve to {
mode: "cancel" }, and then import and run
runCodexMultiAuthCli(["auth","login"]); assert the CLI returns 0, assert
getActionableNamedBackupRestoresMock was called, and assert that no
backup-selection flows ran (confirmMock, selectMock, restoreNamedBackupMock not
called) while createAuthorizationFlowMock was called once to continue into
OAuth; add this test in the same suite (referencing
getActionableNamedBackupRestoresMock, confirmMock, selectMock,
restoreNamedBackupMock, createAuthorizationFlowMock, runCodexMultiAuthCli) to
prevent the regression.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 4421-4424: Wrap the reassessment call to assessNamedBackupRestore
(currently invoked with selection.assessment.backup.name and { currentStorage:
await loadAccounts() }) in a try/catch that specifically handles filesystem
concurrency errors (EBUSY/EPERM/ENOENT) and, on those errors, returns the
manager-level result "dismissed" and emits only redacted labels via the existing
local redaction path instead of rethrowing; ensure you call the same redaction
helper used for the user-facing branch rather than letting the raw error bubble
to the confirmation codepath (the code that logs at the confirmation/render
site). Also add a vitest regression in test/recovery.test.ts named "backup
removed/locked after menu selection" that simulates the backup being
removed/locked after selection and asserts the manager returns "dismissed" and
only redacted labels are emitted.
In `@lib/storage.ts`:
- Around line 1935-1946: The loadBackupCandidate function currently returns
error: String(error) which may include absolute filesystem paths; update
loadBackupCandidate to sanitize/redact filesystem-specific details before
persisting loadError by mapping the thrown error from loadAccountsFromPath into
a safe message (e.g., normalize known errno codes like ENOENT/EBUSY and return a
generic, non-path-containing string) and preserve the original error only in
non-persisted debug logs; additionally add a vitest regression in
test/recovery.test.ts that simulates failed backup reads (ENOENT/EBUSY) and
asserts that the backup hint/error text produced by the codex manager (the place
that surfaces loadBackupCandidate errors) does not contain absolute path
fragments or drive letters.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 2305-2311: The test currently flips storage based on a call
counter (loadCount) inside loadAccountsMock which makes behavior
order-dependent; replace this with deterministic control: either stub
loadAccountsMock using a fixed sequence of returns via
loadAccountsMock.mockResolvedValueOnce(populatedStorage) /
mockResolvedValueOnce(...) for each expected call, or introduce an explicit
storageState flag (e.g., let storageState = 'populated') that loadAccountsMock
returns structuredClone(storageState === 'populated' ? populatedStorage :
emptyStorage) and then toggle storageState in the specific destructive/mock of
the mutating method (e.g., in the mock for the delete/overwrite function) so
transitions occur only when that action is invoked rather than by call count.
Ensure you reference loadAccountsMock, populatedStorage, emptyStorage,
structuredClone and the mutating mock you already have so the test becomes
deterministic.
In `@test/recovery.test.ts`:
- Around line 647-656: The windows-style fallback in the fs.stat mock is
checking for "/locked-backup.json" which doesn't match the test-created backups;
update the normalizedPath.endsWith(...) check inside the stat mock (the block
around originalStat, statSpy, fs.stat and lockedBackup?.path) to match the
actual backup filenames created by the test (e.g., include
endsWith("/first-backup") and endsWith("/second-backup") or the exact file names
like "/first-backup.json" and "/second-backup.json") so the Windows-path branch
triggers when those backup files are stat-ed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e65b778e-a079-4ad4-8ae6-b61cdc5cacbc
📒 Files selected for processing (6)
docs/troubleshooting.mdlib/cli.tslib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/recovery.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/storage.tslib/codex-manager.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/troubleshooting.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/recovery.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (1)
lib/cli.ts (1)
29-31: interactive gating refactor looks good.
lib/cli.ts:29-31andlib/cli.ts:266-268cleanly centralize login-menu interactivity checks while preserving fallback behavior.Also applies to: 266-268
…er' into git-plan/03-startup-recovery-prompt-devtools # Conflicts: # lib/storage.ts
…er' into git-plan/03-startup-recovery-prompt-devtools # Conflicts: # lib/codex-manager.ts
|
Addressed the two remaining startup-recovery issues in
Validation on this branch:
|
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4079">
P3: This null-check is unreachable and introduces dead control flow in the startup recovery path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…er' into git-plan/03-startup-recovery-prompt-devtools
…n/03-startup-recovery-prompt-devtools
1 participant
Summary
codex auth loginstarts with zero saved accounts and recoverable backups existfreshandresetactionsValidation
npm run typechecknpm run lintnpm run buildnpx vitest run test/storage.test.tscmd /c "set NODE_OPTIONS=--max-old-space-size=16384&& npx vitest run test/codex-manager-cli.test.ts -t startup"npx vitest run test/codex-manager-cli.test.ts -t "returns to the login menu when backup reassessment fails before confirmation"Notes
git-plan/02-backup-restore-managerto clear the merge conflict that GitHub was still reporting.test/codex-manager-cli.test.tssuite still has the known Vitest heap ceiling in this worktree, so validation stays focused on the startup-recovery and restore-manager slices touched here.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 wires a named-backup recovery prompt into
runAuthLogin, passes the pre-scannedallAssessmentsset into the restore manager to avoid a double-read, addsgetActionableNamedBackupRestoresto storage with per-entry EBUSY guards and correcttotalBackupstracking, and hardenslistNamedBackupsWithoutLoadingwithretryTransientFilesystemOperation. the core architecture and windows concurrency mitigations are sound.key concerns:
runBackupRestoreManagerreturns"dismissed", the code resetsrecoveryPromptAttempted = falserather than settingsuppressRecoveryPrompt = true. a user who accepts the outer confirm, backs out of the restore browser, and accepts again will loop indefinitely — no test covers two consecutive back-outsloadDashboardDisplaySettingsMockis chainedmockResolvedValueOnce(success).mockImplementationOnce(throw ENOENT); with an empty account store the outer menu is skipped, so the onlyloadDashboardDisplaySettingscall in the path is inside the recovery prompt — it hits the success mock,confirmis reached, and the throw is never triggered; the test may be silently non-functionalgetBackupRestoreAssessmentErrorLabel(storage.ts) andgetRedactedFilesystemErrorLabel(codex-manager.ts) are identical — should live in a shared utility to keep the redaction strategy in syncConfidence Score: 2/5
Important Files Changed
resolveStartupRecoveryAction,loadBackupRestoreManagerAssessments, andgetRedactedFilesystemErrorLabel; correctly passesallAssessmentsto the manager and handles scan/prompt failures — but the re-prompt-after-dismiss path still resetsrecoveryPromptAttempted = false, creating an unbounded loop when the user repeatedly accepts the outer confirm and backs out of the inner managergetActionableNamedBackupRestores,scanNamedBackups,listNamedBackupsWithoutLoading, andassessNamedBackupRestoreCandidate; wraps per-entry scan steps in try/catch, trackstotalBackupsfrom rawreaddircounts before per-entry failures, and wrapslistNamedBackupsWithoutLoadingdirectory reads inretryTransientFilesystemOperation— the duplicate error-extraction helper is a minor style concern but all critical paths appear safeisInteractiveLoginMenuAvailable()combining!isNonInteractiveMode()andisTTY(), and uses it inpromptLoginMode; straightforward and consistent with existing TTY guardsafterEach(() => vi.restoreAllMocks())is present (addressed from prior review), but the "display settings load fails before confirm" test has a suspected mock-ordering bug where a leadingmockResolvedValueOnce(success)may consume the only recovery-promptloadDashboardDisplaySettingscall, silently preventing the intended ENOENT throw from being reachedgetActionableNamedBackupRestoresoverride tests and aremoveWithRetryhelper for Windows-safe temp dir cleanup; reformats existing tests to tabs, addsafterEach(() => vi.restoreAllMocks())suite-level guard — changes look solidSequence Diagram
sequenceDiagram participant U as User participant C as runAuthLogin participant S as storage (getActionableNamedBackupRestores) participant M as runBackupRestoreManager participant O as OAuth flow C->>C: loadAccounts() → existingCount=0 C->>C: canPromptForRecovery? check suppressRecoveryPrompt + recoveryPromptAttempted + TTY alt pendingRecoveryState is null C->>S: getActionableNamedBackupRestores() S-->>C: {assessments, allAssessments, totalBackups} alt assessments empty + scan not failed C->>C: allowEmptyStorageMenu=true → continue loginFlow (open empty menu) else scan failed C-->>O: fall through to OAuth end else reuse pendingRecoveryState C->>C: recoveryState = pendingRecoveryState (no re-scan) end alt assessments.length > 0 C->>C: loadDashboardDisplaySettings() C->>U: confirm("Found N recoverable backups … Restore now?") alt user confirms C->>M: runBackupRestoreManager(displaySettings, allAssessments) M->>U: show backup list, user selects alt user restores successfully M-->>C: "restored" C->>C: accounts populated → outer menu opens else user backs out or restore fails M-->>C: "dismissed" C->>C: pendingRecoveryState=recoveryState, recoveryPromptAttempted=false Note over C: ⚠️ unbounded loop: confirm fires again on next iteration C->>C: continue loginFlow (re-prompt) end else user declines C-->>O: fall through to OAuth end else no actionable backups C-->>O: fall through to OAuth end O->>U: OAuth login flowComments Outside Diff (2)
lib/codex-manager.ts, line 4091-4098 (link)Unbounded re-prompt loop still present
when
runBackupRestoreManagerreturns"dismissed",recoveryPromptAttemptedis reset tofalseandpendingRecoveryStateis preserved. on the next iteration ofloginFlow,existingCountis still 0,suppressRecoveryPromptis stillfalse, andrecoveryPromptAttemptedis nowfalse— socanPromptForRecoveryistrueagain and the outer confirm fires once more. a user who repeatedly accepts the outer prompt but backs out of the inner manager will loop indefinitely, with no escape short of answering "no" to the confirm.the tests only exercise one back-out (second confirm returns false). a second consecutive back-out reproduces the loop. consider flipping
suppressRecoveryPrompt = trueinstead ofrecoveryPromptAttempted = falseafter a dismissal to permanently suppress the prompt within the session:this also removes the need for
pendingRecoveryStateentirely (the assessment set would no longer be replayed on a suppressed re-prompt).test/codex-manager-cli.test.ts, line 1840-1858 (link)Extra mock consumes the intended throw — test may not cover the stated scenario
loadDashboardDisplaySettingsMockis chained with.mockResolvedValueOnce(settings).mockImplementationOnce(throw ENOENT). with an empty account store, the outer menu loop is never entered, so the only call toloadDashboardDisplaySettingsbeforeconfirmis the one inside the recovery prompt. that call consumes the first mock (success),getNamedBackupsDirectoryPath()is called,promptWasShown = true, andconfirm()is reached — contradicting the assertionsexpect(getNamedBackupsDirectoryPathMock).not.toHaveBeenCalled()andexpect(confirmMock).not.toHaveBeenCalled().if there is no preliminary call to
loadDashboardDisplaySettingsbefore the recovery prompt in this path, the throw mock is never triggered and the test silently passes without exercising the intended failure. the fix is to drop the leadingmockResolvedValueOnceso the first (and only) relevant call immediately throws:if there is a legitimate preliminary call from an outer init path, add a comment explaining it so future readers understand the two-call sequence.
Prompt To Fix All With AI
Last reviewed commit: 192e4e3
Context used: