feat(auth): prompt for recovery on startup#77
feat(auth): prompt for recovery on startup#77ndycode wants to merge 9 commits intogit-plan/02-backup-restore-managerfrom
Conversation
…artup-recovery-prompt-devtools # Conflicts: # test/codex-manager-cli.test.ts
📝 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 (1 warning, 1 inconclusive)
✅ 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.
| continue; | ||
| } | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); |
There was a problem hiding this comment.
startup scan results discarded before manager opens
getActionableNamedBackupRestores already read and assessed every backup file to populate recoveryState. when restoreNow is true, runBackupRestoreManager then calls listNamedBackups() + assessNamedBackupRestore() in a second full pass. on windows this doubles the open-file-handle window — two separate readdir+readFile sequences back-to-back — exactly the scenario where antivirus is most likely to inject an EBUSY between the two passes.
it also means the count quoted in the outer confirm ("Found N recoverable backups") can silently diverge from what the manager actually shows if a file disappears or becomes unreadable in the gap.
since recoveryState.assessments already contains the assessed list, pass the pre-scanned backups directly to the manager to avoid the second read:
if (restoreNow) {
const restoreResult = await runBackupRestoreManager(
displaySettings,
recoveryState.assessments.map((a) => a.backup),
);
// ...
}then let runBackupRestoreManager accept an optional backups override and skip listNamedBackups() when it is provided.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4441-4444
Comment:
**startup scan results discarded before manager opens**
`getActionableNamedBackupRestores` already read and assessed every backup file to populate `recoveryState`. when `restoreNow` is `true`, `runBackupRestoreManager` then calls `listNamedBackups()` + `assessNamedBackupRestore()` in a second full pass. on windows this doubles the open-file-handle window — two separate readdir+readFile sequences back-to-back — exactly the scenario where antivirus is most likely to inject an EBUSY between the two passes.
it also means the count quoted in the outer confirm ("Found N recoverable backups") can silently diverge from what the manager actually shows if a file disappears or becomes unreadable in the gap.
since `recoveryState.assessments` already contains the assessed list, pass the pre-scanned backups directly to the manager to avoid the second read:
```typescript
if (restoreNow) {
const restoreResult = await runBackupRestoreManager(
displaySettings,
recoveryState.assessments.map((a) => a.backup),
);
// ...
}
```
then let `runBackupRestoreManager` accept an optional `backups` override and skip `listNamedBackups()` when it is provided.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
What Changed
runAuthLoginto degrade cleanly when startup backup discovery or prompting fails and to re-offer recovery after dismissed restore-manager flowsValidation
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
git-plan/03-startup-recovery-promptback tobd8060bto restore the previous OAuth-only startup path.Additional Notes
npm testis currently blocked in this environment becausetest/oauth-server.integration.test.tscannot bind127.0.0.1:1455while an externalnode ...\scripts\codex.js auth loginprocess is already listening on that port.npm test -- test/codex-manager-cli.test.ts test/recovery.test.ts.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 adds a startup backup recovery prompt to
runAuthLogin: when the session starts with zero accounts and at least one restorable named backup exists, the user is offered a restore before OAuth opens. the prompt is suppressed after same-sessionfresh/resetactions and in non-interactive/fallback flows.key improvements included:
getActionableNamedBackupRestoresfast-path reuses pre-loaded scan entries so backup files are read only once in the default flowrunBackupRestoreManagernow returns a typedBackupRestoreManagerResultand has per-backup error guards so one locked backup doesn't abort the full listrecoveryPromptAttempted/suppressRecoveryPromptflags correctly bound re-prompt behaviour and intentional-wipe suppressionremoveWithRetryexponential back-offtwo issues worth addressing:
runBackupRestoreManagercallslistNamedBackups()+assessNamedBackupRestore()independently aftergetActionableNamedBackupRestoreshas already scanned. this creates two separate readdir+readFile passes, doubling the EBUSY exposure window on windows desktops. passing the pre-scanned backups fromrecoveryStateinto the manager would close this gap.console.warn:error.messagefrom node.js EBUSY/EPERM errors contains the full absolute path of the locked file (e.g.,EBUSY: resource busy or locked, open 'C:\Users\...\backups\work.json'). three newconsole.warncall sites write this directly to stderr; surfacing onlyerror.codeinstead would avoid leaking backup file names to terminal history or CI logs.Confidence Score: 3/5
console.warncalls that surface full file paths from OS errors — both are actionable improvements rather than blockers. the storage layer changes are solid and the test suite covers the core happy-path and failure modes.Important Files Changed
recoveryPromptAttempted/suppressRecoveryPromptflags are well-placed, but the manager re-scans the backup directory instead of reusingrecoveryState, doubling windows EBUSY exposure;console.warnsurfaces raw OS error messages including full file pathsgetActionableNamedBackupRestoreswith a clean fast-path (reuses pre-loaded scan entries) and slow-path (injected assessor, no pre-read); per-entry error guards added throughout;buildNamedBackupMetadataalready absorbs stat errors internally so the outer try/catch inlistNamedBackupsWithoutLoadingis good defence-in-depthisInteractiveLoginMenuAvailablehelper and replaces the ad-hoc!isTTY()check — clean, minimal change with no riskwarnSpyinstances are manually restored; one test shadows the module-levelcreateAuthorizationFlowMockwith a same-named local constant that may confuse future editsremoveWithRetrywith exponential back-off correctly handles Windows cleanup races; top-levelafterEach(() => vi.restoreAllMocks())is presentSequence Diagram
sequenceDiagram participant U as User participant RL as runAuthLogin participant GAR as getActionableNamedBackupRestores participant RBR as runBackupRestoreManager participant FS as Filesystem RL->>RL: loadAccounts() → existingCount=0 RL->>RL: canPromptForRecovery? (check flags) RL->>GAR: getActionableNamedBackupRestores({currentStorage}) GAR->>FS: scanNamedBackups() [fast path: readdir + readFile per backup] FS-->>GAR: NamedBackupScanEntry[] GAR->>GAR: assessNamedBackupRestoreCandidate() per entry (no extra I/O) GAR-->>RL: {assessments[], totalBackups} alt assessments.length > 0 RL->>U: confirm("Found N backups … Restore now?") U-->>RL: true RL->>RBR: runBackupRestoreManager(displaySettings) RBR->>FS: listNamedBackups() [second readdir + readFile scan] FS-->>RBR: NamedBackupMetadata[] RBR->>FS: assessNamedBackupRestore() per backup [additional readFile] FS-->>RBR: BackupRestoreAssessment[] RBR->>U: select backup U-->>RBR: selection RBR->>U: confirm restore U-->>RBR: confirmed RBR->>FS: restoreNamedBackup() FS-->>RBR: result alt restored RBR-->>RL: "restored" RL->>RL: continue (recoveryPromptAttempted stays true) else dismissed RBR-->>RL: "dismissed" RL->>RL: recoveryPromptAttempted=false, continue (re-prompt next iteration) end else no assessments / user declines RL->>RL: fall through to OAuth end RL->>RL: suppressRecoveryPrompt=true after fresh/resetPrompt To Fix All With AI
Last reviewed commit: dcd1bac
Context used: