fix(auth): prompt for recovery on startup by ndycode · Pull Request #106 · ndycode/codex-multi-auth

ndycode · 2026-03-16T08:51:10Z

Summary

Reopens the preserved startup-recovery prompt slice from archived PRs #77 and #92
Stacked PR on fresh/02-backup-restore-manager

What Changed

Prompts for actionable recovery before OAuth when interactive login starts empty, and suppresses the prompt after same-session deliberate fresh or reset actions.
Keeps this branch focused to the startup-recovery prompt and related recovery/storage tests.

Validation

Docs and Governance Checklist

README updated (if user-visible behavior changed)
docs/getting-started.md updated (if onboarding flow changed)
docs/features.md updated (if capability surface changed)
relevant docs/reference/* pages updated (if commands/settings/paths changed)
docs/upgrade.md updated (if migration behavior changed)
SECURITY.md and CONTRIBUTING.md reviewed for alignment

Risk and Rollback

Risk level: medium
Rollback plan: revert 12074e2 and 390f801

Additional Notes

This is a sibling branch to fresh/04-sync-center; later branches carry an equivalent cleanup commit but are not direct descendants of 390f801.
Preserved branch validation from the fresh-restack run passed lint, typecheck, build, and full tests.

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 in the startup-recovery prompt that was preserved from archived PRs #77 and #92: when codex auth login opens with zero saved accounts and recoverable named backups exist, the user is offered a restore before oauth. the suppressRecoveryPrompt and recoveryPromptAttempted flags gate the prompt correctly — suppressed after same-session fresh/reset, and not re-shown on a deliberate dismiss. several previously flagged issues are resolved: formatRelativeDateShort now correctly guards timestamp <= 0 and non-finite values, restore-backup resets storage path before entering the manager, resolveStartupRecoveryAction returns "continue-with-oauth" when backups exist but none are actionable (and the test expectation matches), NAMED_BACKUP_ASSESS_CONCURRENCY is properly kept at 4, and EBUSY is platform-gated to win32.

key remaining concerns:

the "failed" restore result path resets recoveryPromptAttempted = false with no retry cap — a persistent file lock on windows will trap the user in an infinite prompt/fail loop with no escape to oauth short of ctrl-c.
restoreNamedBackup performs a third eligibility assessment without passing currentStorage, introducing a narrow race window vs the confirmation the user just saw.
no vitest coverage for the unbounded failed-restore loop or for the stale-assessments-on-retry scenario.

Confidence Score: 2/5

not safe to merge until the unbounded failed-restore retry loop in runAuthLogin is capped — a persistent windows file lock will trap users indefinitely on startup.
the "dismissed" loop from the previous round is fixed (sets suppressRecoveryPrompt = true), but the "failed" path resets recoveryPromptAttempted = false without any cap, creating an equivalent infinite loop on persistent file errors. the windows token/filesystem risk is real and reproducible with a locked backup file. the rest of the changes (concurrency cap at 4, platform-gated EBUSY, formatRelativeDateShort guards, storage path reset) look correct, and the test regression for resolveStartupRecoveryAction is resolved. but the loop bug and the missing vitest coverage lower confidence.
lib/codex-manager.ts lines 4127–4135 (failed-restore retry path), lib/storage.ts lines 2045–2056 (restoreNamedBackup third assessment without currentStorage)

Important Files Changed

Filename	Overview
lib/codex-manager.ts	introduces startup recovery prompt and `resolveStartupRecoveryAction`; `"failed"` restore path resets `recoveryPromptAttempted` with no retry cap, creating an unbounded loop on windows with persistent file locks; stale `allAssessments` passed to retry menu.
lib/storage.ts	retry budget correctly at 5 attempts; `EBUSY` platform-gated to win32; `assessNamedBackupRestore` properly guards `changed`/`eligibleForRestore`; `restoreNamedBackup` performs a redundant third assessment without passing `currentStorage`, introducing a narrow race window.
lib/destructive-actions.ts	`DESTRUCTIVE_ACTION_COPY` has proper separate keys for `deleteSavedAccounts` and `resetLocalState`; `suppressRecoveryPrompt = true` set correctly after both `fresh` and `reset` actions.
test/recovery.test.ts	test expectation at line 789 now correctly asserts `"continue-with-oauth"` matching the implementation; retry budget path (5 attempts) validated for locked backup; no coverage for unbounded failed-restore loop.
test/storage-recovery-paths.test.ts	new tests for recovery scan fast-path; `getActionableNamedBackupRestores` integration covered but no test for the import retry-after-failure scenario.
test/codex-manager-cli.test.ts	tests cover startup prompt suppression after `fresh`/`reset`; no coverage for unbounded `"failed"` loop scenario.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[runAuthLogin start] --> B[loadAccounts]
    B --> C{accounts > 0 OR\ncanOpenEmptyStorageMenu?}
    C -- yes --> D[show auth menu loop]
    D -- fresh/reset --> E[suppressRecoveryPrompt = true\ncontinue loginFlow]
    D -- add --> F[exit inner loop]
    F --> G[canPromptForRecovery check]
    C -- no --> G
    G -- suppressed OR\nalready attempted OR\nnot interactive --> L[runOAuthFlow]
    G -- eligible --> H[getActionableNamedBackupRestores]
    H -- no backups --> I[allowEmptyStorageMenu = true\ncontinue loginFlow]
    H -- backups exist,\nnone actionable --> L
    H -- actionable backups --> J[confirm prompt]
    J -- no --> L
    J -- yes --> K[runBackupRestoreManager]
    K -- restored --> L
    K -- dismissed --> M[suppressRecoveryPrompt = true\ncontinue loginFlow]
    K -- failed --> N[pendingRecoveryState cached\nrecoveryPromptAttempted = false\ncontinue loginFlow]
    N --> G
    style N fill:#ff9999,color:#000
    N -- no cap! --> G

Comments Outside Diff (8)

lib/codex-manager.ts, line 4596-4602 (link)

autoSyncActiveAccountToCodex dropped — codex CLI auth state not updated after standalone restore

the old runBackupRestoreManager called autoSyncActiveAccountToCodex() after every successful write. the new restoreNamedBackup path skips this entirely. for the standalone codex auth restore-backup command the codex CLI active-account file is never rewritten, so the CLI stays pointing at whichever account was active before the restore (or at a stale entry).

when used inside runAuthLogin this is less severe because the user will eventually complete an OAuth step and syncSelectionToCodex fires. but for the direct command path the sync is the only opportunity and it's now gone.

the removed call:

// old — ran after every successful restore
const synced = await autoSyncActiveAccountToCodex();
if (!synced) console.warn("Backup restored, but Codex CLI auth state could not be synced.");

suggest calling autoSyncActiveAccountToCodex() (with a non-fatal catch) after restoreNamedBackup succeeds, at minimum for the standalone command path.

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4596-4602

Comment:
**`autoSyncActiveAccountToCodex` dropped — codex CLI auth state not updated after standalone restore**

the old `runBackupRestoreManager` called `autoSyncActiveAccountToCodex()` after every successful write. the new `restoreNamedBackup` path skips this entirely. for the standalone `codex auth restore-backup` command the codex CLI active-account file is never rewritten, so the CLI stays pointing at whichever account was active before the restore (or at a stale entry). 

when used inside `runAuthLogin` this is less severe because the user will eventually complete an OAuth step and `syncSelectionToCodex` fires. but for the direct command path the sync is the only opportunity and it's now gone.

the removed call:
```ts
// old — ran after every successful restore
const synced = await autoSyncActiveAccountToCodex();
if (!synced) console.warn("Backup restored, but Codex CLI auth state could not be synced.");
```

suggest calling `autoSyncActiveAccountToCodex()` (with a non-fatal catch) after `restoreNamedBackup` succeeds, at minimum for the standalone command path.

How can I resolve this? If you propose a fix, please make it concise.

lib/storage.ts, line 1082-1104 (link)

Retry jitter is now constant 0–9 ms regardless of attempt

the old code scaled jitter proportionally to the base delay:

const baseDelayMs = TRANSIENT_FILESYSTEM_BASE_DELAY_MS * 2 ** attempt; // 10, 20, 40, 80 ms
const jitterMs = Math.floor(Math.random() * baseDelayMs); // up to 79 ms at attempt 3
await setTimeout(resolve, baseDelayMs + jitterMs);

the new code always jitters 0–9 ms regardless of attempt:

setTimeout(resolve, 10 * 2 ** attempt + Math.floor(Math.random() * 10));
// attempt 0: 10-19 ms, attempt 1: 20-29 ms, ..., attempt 3: 80-89 ms

on windows, antivirus scanners hold locks for bursts that often exceed 100 ms. concentrated retry windows (e.g., all four retries landing within 90 ms) reduce the chance of clearing the lock between attempts. the old proportional jitter was more effective for distributing retry pressure. this regression is compounded by the already-reduced 5-attempt budget (down from 7). no vitest coverage for the jitter distribution.

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/storage.ts
Line: 1082-1104

Comment:
**Retry jitter is now constant 0–9 ms regardless of attempt**

the old code scaled jitter proportionally to the base delay:
```ts
const baseDelayMs = TRANSIENT_FILESYSTEM_BASE_DELAY_MS * 2 ** attempt; // 10, 20, 40, 80 ms
const jitterMs = Math.floor(Math.random() * baseDelayMs); // up to 79 ms at attempt 3
await setTimeout(resolve, baseDelayMs + jitterMs);
```

the new code always jitters 0–9 ms regardless of attempt:
```ts
setTimeout(resolve, 10 * 2 ** attempt + Math.floor(Math.random() * 10));
// attempt 0: 10-19 ms, attempt 1: 20-29 ms, ..., attempt 3: 80-89 ms
```

on windows, antivirus scanners hold locks for bursts that often exceed 100 ms. concentrated retry windows (e.g., all four retries landing within 90 ms) reduce the chance of clearing the lock between attempts. the old proportional jitter was more effective for distributing retry pressure. this regression is compounded by the already-reduced 5-attempt budget (down from 7). no vitest coverage for the jitter distribution.

How can I resolve this? If you propose a fix, please make it concise.

lib/storage.ts, line 1316-1318 (link)

restoreNamedBackup no longer enforces eligibleForRestore or the account limit

the old restoreAssessedNamedBackup validated eligibility before writing:

if (!assessment.eligibleForRestore) {
    throw new Error(assessment.error ?? "Backup is not eligible for restore.");
}

runBackupRestoreManager in codex-manager.ts does call assessNamedBackupRestore and check latestAssessment.eligibleForRestore before invoking restoreNamedBackup, so the current UI path is guarded. however, restoreNamedBackup is an exported function — any caller that reaches it directly (e.g., future automation, programmatic use, or a test that mocks around the UI layer) will bypass both the wouldExceedLimit guard and the eligibleForRestore check, landing straight in importAccounts.

the importAccounts function does enforce the limit via a throw, so catastrophic writes are blocked, but an ineligible restore due to an unparseable backup would still proceed and likely corrupt the import silently. suggest reinstating a minimal eligibility gate:

export async function restoreNamedBackup(name: string): Promise<ImportAccountsResult> {
    const assessment = await assessNamedBackupRestore(name);
    if (!assessment.eligibleForRestore) {
        throw new Error(assessment.error ?? "Backup is not eligible for restore.");
    }
    const backupPath = await resolveNamedBackupRestorePath(name);
    return importAccounts(backupPath);
}

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/storage.ts
Line: 1316-1318

Comment:
**`restoreNamedBackup` no longer enforces `eligibleForRestore` or the account limit**

the old `restoreAssessedNamedBackup` validated eligibility before writing:
```ts
if (!assessment.eligibleForRestore) {
    throw new Error(assessment.error ?? "Backup is not eligible for restore.");
}
```

`runBackupRestoreManager` in `codex-manager.ts` does call `assessNamedBackupRestore` and check `latestAssessment.eligibleForRestore` before invoking `restoreNamedBackup`, so the current UI path is guarded. however, `restoreNamedBackup` is an exported function — any caller that reaches it directly (e.g., future automation, programmatic use, or a test that mocks around the UI layer) will bypass both the `wouldExceedLimit` guard and the `eligibleForRestore` check, landing straight in `importAccounts`.

the `importAccounts` function does enforce the limit via a throw, so catastrophic writes are blocked, but an ineligible restore due to an unparseable backup would still proceed and likely corrupt the import silently. suggest reinstating a minimal eligibility gate:
```ts
export async function restoreNamedBackup(name: string): Promise<ImportAccountsResult> {
    const assessment = await assessNamedBackupRestore(name);
    if (!assessment.eligibleForRestore) {
        throw new Error(assessment.error ?? "Backup is not eligible for restore.");
    }
    const backupPath = await resolveNamedBackupRestorePath(name);
    return importAccounts(backupPath);
}
```

How can I resolve this? If you propose a fix, please make it concise.

lib/storage.ts, line 1579-1594 (link)

Path containment check removed — NTFS junction traversal on windows no longer blocked

the old code called assertNamedBackupRestorePath (with realpathSync.native) for every resolved path before use:

if (existingPath) {
    return assertNamedBackupRestorePath(existingPath, backupRoot);
}
// ...
return assertNamedBackupRestorePath(builtPath, backupRoot);

the new code drops this entirely:

if (existingPath) {
    return existingPath;   // no containment check
}
// ...
return buildNamedBackupPath(name);   // no containment check

scanNamedBackups and listNamedBackupsWithoutLoading do filter entry.isSymbolicLink(), which handles symlinks at listing time. however, on windows, ntfs directory junctions (mklink /j) are not symlinks — entry.isSymbolicLink() returns false for them while realpathSync.native resolves through them. a backups/ directory itself configured as a junction to a directory outside the data root would not be caught by the new filter.

more concretely: resolveNamedBackupRestorePath is also reached via assessNamedBackupRestore → resolveNamedBackupRestorePath, which is called directly from restoreNamedBackup. if the backup root is a junction, the canonical path will silently escape the expected directory boundary. no vitest coverage for this scenario.

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/storage.ts
Line: 1579-1594

Comment:
**Path containment check removed — NTFS junction traversal on windows no longer blocked**

the old code called `assertNamedBackupRestorePath` (with `realpathSync.native`) for every resolved path before use:
```ts
if (existingPath) {
    return assertNamedBackupRestorePath(existingPath, backupRoot);
}
// ...
return assertNamedBackupRestorePath(builtPath, backupRoot);
```

the new code drops this entirely:
```ts
if (existingPath) {
    return existingPath;   // no containment check
}
// ...
return buildNamedBackupPath(name);   // no containment check
```

`scanNamedBackups` and `listNamedBackupsWithoutLoading` do filter `entry.isSymbolicLink()`, which handles symlinks at listing time. however, on windows, ntfs directory junctions (`mklink /j`) are not symlinks — `entry.isSymbolicLink()` returns `false` for them while `realpathSync.native` resolves through them. a `backups/` directory itself configured as a junction to a directory outside the data root would not be caught by the new filter.

more concretely: `resolveNamedBackupRestorePath` is also reached via `assessNamedBackupRestore` → `resolveNamedBackupRestorePath`, which is called directly from `restoreNamedBackup`. if the backup root is a junction, the canonical path will silently escape the expected directory boundary. no vitest coverage for this scenario.

How can I resolve this? If you propose a fix, please make it concise.

test/recovery.test.ts, line 790-812 (link)

missing required allAssessments field in test fixtures

ActionableNamedBackupRecoveries (added in this PR at lib/storage.ts) declares allAssessments: BackupRestoreAssessment[] as a required field, but all three call sites here pass an object literal without it. typescript should flag this as Property 'allAssessments' is missing at npm run typecheck. the function only reads assessments and totalBackups at runtime so tests pass, but the type contract is broken.

same fix needed for the totalBackups: 0 fixture on line ~809.
lib/codex-manager.ts, line 4116-4138 (link)

promptWasShown flag set before confirm() call — silently eats TTY failures

promptWasShown = true is written one line before await confirm(...). if confirm() throws before painting any output (TTY closed, readline init failure, etc.), the catch block sees promptWasShown === true, skips the recoveryPromptAttempted = false reset, and the user's only chance at startup recovery is silently dropped. the flag's stated purpose is to distinguish "error happened before the user saw anything" from "error happened after", but the current placement defeats that on confirm() failures.

move the flag set to after confirm() resolves (or after the prompt is guaranteed to have been painted):
```
const restoreNow = await confirm(
    `Found ${recoveryState.assessments.length} recoverable backup${
        recoveryState.assessments.length === 1 ? "" : "s"
    } out of ${recoveryState.totalBackups} total (${backupLabel}) in ${backupDir}. Restore now?`,
);
promptWasShown = true;
```
no vitest coverage for the confirm() throw-before-display path.
lib/codex-manager.ts, line 4127-4135 (link)

"failed" restore path retries indefinitely

when restoreResult === "failed", recoveryPromptAttempted is reset to false with no cap. on the next loginFlow iteration, existingCount === 0 and !suppressRecoveryPrompt still hold, so canPromptForRecovery fires again — another confirm, another restore attempt, another failure, ad infinitum. the only escapes are ctrl-c or a successful restore.

this is particularly dangerous on windows where a persistent EPERM/EBUSY will exhaust the 5-attempt retry budget and return "failed" on every iteration. the user is stuck in the recovery loop and can never reach oauth.

suggest adding a retry limit, or using a dedicated counter, or simply falling through to oauth after a single failure:
```
if (restoreResult !== "restored") {
    if (restoreResult === "dismissed") {
        suppressRecoveryPrompt = true;
    } else {
        // "failed" — re-offer once, then give up
        if (!pendingRecoveryState) {
            pendingRecoveryState = recoveryState;
            recoveryPromptAttempted = false;
        } else {
            suppressRecoveryPrompt = true; // second failure: stop looping
        }
    }
}
```
missing vitest coverage for the failed-restore loop — no test verifies that repeated "failed" results eventually suppress the prompt.
lib/storage.ts, line 2045-2056 (link)

restoreNamedBackup calls assessNamedBackupRestore without currentStorage

assessNamedBackupRestore(name) at line 2049 loads accounts fresh from disk with no passed currentStorage. but runBackupRestoreManager already called assessNamedBackupRestore with a current-storage snapshot at line 4493 just before confirming. restoreNamedBackup then re-assesses a third time, using whatever is on disk at that instant — if another process wrote an account between the confirmation and the actual import, the eligibility guard here will operate on different state than what the user confirmed.

this is a narrow race but worth noting on windows where concurrent writes are more likely. passing currentStorage through or removing the redundant re-assessment inside restoreNamedBackup would close the window.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4127-4135

Comment:
**`"failed"` restore path retries indefinitely**

when `restoreResult === "failed"`, `recoveryPromptAttempted` is reset to `false` with no cap. on the next `loginFlow` iteration, `existingCount === 0` and `!suppressRecoveryPrompt` still hold, so `canPromptForRecovery` fires again — another confirm, another restore attempt, another failure, ad infinitum. the only escapes are ctrl-c or a successful restore.

this is particularly dangerous on windows where a persistent `EPERM`/`EBUSY` will exhaust the 5-attempt retry budget and return `"failed"` on every iteration. the user is stuck in the recovery loop and can never reach oauth.

suggest adding a retry limit, or using a dedicated counter, or simply falling through to oauth after a single failure:

```ts
if (restoreResult !== "restored") {
    if (restoreResult === "dismissed") {
        suppressRecoveryPrompt = true;
    } else {
        // "failed" — re-offer once, then give up
        if (!pendingRecoveryState) {
            pendingRecoveryState = recoveryState;
            recoveryPromptAttempted = false;
        } else {
            suppressRecoveryPrompt = true; // second failure: stop looping
        }
    }
}
```

missing vitest coverage for the failed-restore loop — no test verifies that repeated `"failed"` results eventually suppress the prompt.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4123-4126

Comment:
**Stale assessments passed on failed-restore retry**

`recoveryState.allAssessments` is cached from the initial scan and re-passed to `runBackupRestoreManager` on a retry via `pendingRecoveryState`. if any backup was deleted, renamed, or already restored by another process between the first and second prompt, the menu will display outdated entries.

the re-assessment at line ~4493 does refresh the individual selected backup before confirming, so the actual restore won't operate on stale data — but the selection menu may show ghost entries. consider clearing `pendingRecoveryState` and forcing a fresh scan on retry instead of reusing the original `allAssessments`.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/storage.ts
Line: 2045-2056

Comment:
**`restoreNamedBackup` calls `assessNamedBackupRestore` without `currentStorage`**

`assessNamedBackupRestore(name)` at line 2049 loads accounts fresh from disk with no passed `currentStorage`. but `runBackupRestoreManager` already called `assessNamedBackupRestore` with a current-storage snapshot at line 4493 just before confirming. `restoreNamedBackup` then re-assesses a third time, using whatever is on disk at that instant — if another process wrote an account between the confirmation and the actual import, the eligibility guard here will operate on different state than what the user confirmed.

this is a narrow race but worth noting on windows where concurrent writes are more likely. passing `currentStorage` through or removing the redundant re-assessment inside `restoreNamedBackup` would close the window.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "fix(auth): retry sta..."}

chatgpt-codex-connector · 2026-03-16T08:51:18Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

coderabbitai · 2026-03-16T08:52:41Z

📝 Walkthrough

walkthrough

introduces a startup recovery prompt during interactive login when no saved accounts exist but recoverable named backups are present. implements a named-backup subsystem (scan, assess, restore), integrates a backup-restore manager into the login flow, adds public storage APIs, and expands tests accordingly.

changes

Cohort / File(s)	Summary
documentation `docs/getting-started.md`, `docs/troubleshooting.md`, `docs/upgrade.md`	added text describing the startup recovery prompt: only shown in interactive tty sessions, skipped in non-interactive/fallback paths, and suppressed after same-session fresh/reset actions.
cli infra `lib/cli.ts`	added `isInteractiveLoginMenuAvailable()` and updated `promptLoginMode` to require tty + non-interactive-mode check.
login & recovery state `lib/codex-manager.ts`	added `StartupRecoveryAction`, `resolveStartupRecoveryAction`, recovery-state flags, and integrated `runBackupRestoreManager` / restore-backup handling into the login loop; added destructive-action guards and messaging.
named backup subsystem `lib/storage.ts`	large addition: named-backup scanning (`scanNamedBackups`, `listNamedBackups`), assessment (`assessNamedBackupRestore`), restoration (`restoreNamedBackup`), retryable filesystem ops, `StorageError` hints, import deduplication, and new public exports (assess/list/restore, concurrency constants).
ui copy cleanup `lib/ui/copy.ts`	removed three `UI_COPY.mainMenu` entries: `restoreBackupConfirm`, `restoreBackupSuccess`, `restoreBackupRefreshSuccess`.
tests — recovery `test/recovery.test.ts`	reorganized imports, added `removeWithRetry`, restored mocks after each test, and expanded coverage for actionable restores, recovery hooks, resolveStartupRecoveryAction, and many edge cases.
tests — storage `test/storage-recovery-paths.test.ts`, `test/storage.test.ts`	added backup mtime manipulation for discovery precedence, exported/used named-backup APIs in tests, expanded tests for retry/backoff, quota-cache clearing, and import result `changed` flags.

sequence diagram

sequenceDiagram
    participant client as "interactive cli"
    participant codex as "codex manager"
    participant storage as "storage layer"
    participant restore as "backup restore manager"
    participant auth as "oauth/auth"

    client->>codex: promptLoginMode() (interactive tty)
    codex->>codex: isInteractiveLoginMenuAvailable()
    codex->>storage: getActionableNamedBackupRestores()
    storage->>storage: scanNamedBackups() + assessNamedBackupRestore()
    storage-->>codex: actionable recoveries
    codex->>codex: resolveStartupRecoveryAction()
    alt show recovery prompt
        codex->>restore: runBackupRestoreManager()
        restore->>client: show restore menu
        client->>restore: select backup
        restore->>storage: restoreNamedBackup(name)
        storage-->>restore: ImportAccountsResult
        restore->>auth: post-restore auth sync
        restore-->>codex: restored | dismissed | failed
    else continue oauth
        codex->>auth: continue standard login
    end
    auth-->>client: auth complete

estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

flagged issues

concurrency risk. review batched assessment concurrency and state handling in lib/storage.ts:1 and lib/codex-manager.ts:1. confirm no races when multiple assessments or restores run concurrently and that pendingRecoveryState prevents duplicate flows.
missing regression tests. add explicit tests for suppression after same-session fresh/reset flows. see lib/codex-manager.ts:1 for suppression flags and update test/recovery.test.ts:1 to cover this.
windows edge cases. verify path-casing normalization and retry behavior in lib/storage.ts:1 (copy/rename/unlink helpers). tests mention windows nuances but add explicit windows-specific assertions in test/storage.test.ts:1.
transient retry/backoff impact. validate exponential backoff and retry counts in test/recovery.test.ts:1 (removeWithRetry) and lib/storage.ts:1 so scans do not block interactive login indefinitely.
state leak risk. ensure recovery flags (suppressRecoveryPrompt, recoveryPromptAttempted) in lib/codex-manager.ts:1 are cleared on session exit and error paths.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.56% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	PR description follows the template structure with Summary, What Changed, Validation checklist, Docs and Governance Checklist, and Risk/Rollback sections. All required checkboxes are provided; most validation steps marked complete.	Address the 8 greptile-flagged issues before merge: unbounded failed-restore retry loop (lib/codex-manager.ts:4127–4135), stale assessments on retry (4123–4126), missing allAssessments in test fixtures (test/recovery.test.ts:790–812), prompt-flag placement race (4116–4138), restoreNamedBackup missing eligibility check (lib/storage.ts:1316–1318), path containment check removed (1579–1594), retry jitter regression (1082–1104), and missing autoSyncActiveAccountToCodex after standalone restore (4596–4602).

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title follows conventional commits format with type (fix), scope (auth), and lowercase imperative summary under 72 chars.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fresh/03-startup-recovery-prompt

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch fresh/03-startup-recovery-prompt

📝 Coding Plan

Generate coding plan for human review comments

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

Reintroduces and wires up a startup recovery prompt to offer named-backup restore options before OAuth when interactive login starts with an empty account store.

Changes:

Adds getActionableNamedBackupRestores() + startup decision logic (resolveStartupRecoveryAction) to drive the recovery prompt flow.
Refactors named-backup listing/assessment/restore plumbing and related tests.
Updates onboarding/upgrade/troubleshooting docs to describe the new startup recovery prompt behavior.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
lib/storage.ts	Adds actionable-backup scanning API; refactors backup listing/assessment/restore + import/export behavior.
lib/codex-manager.ts	Implements startup recovery prompt flow and restore-backup command refactor.
lib/cli.ts	Centralizes “interactive menu available” detection for login/menu flows.
test/storage.test.ts	Updates storage/backup tests to match new restore/import behaviors and isolation patterns.
test/storage-recovery-paths.test.ts	Adjusts backup metadata ordering expectations by manipulating checkpoint mtimes.
test/recovery.test.ts	Adds coverage for new storage-backed recovery scanning + refactors existing recovery tests.
lib/ui/copy.ts	Removes restore-backup UI copy helpers in favor of inline messaging.
docs/getting-started.md	Documents the startup recovery prompt in the login flow.
docs/upgrade.md	Adds an upgrade note explaining the new startup recovery prompt behavior.
docs/troubleshooting.md	Notes when/why startup recovery prompting occurs.
docs/reference/public-api.md	Removes note about `importAccounts().changed` return shape.

Comments suppressed due to low confidence (1)

lib/storage.ts:1

The import result math can produce invalid values: imported can be negative if existingAccounts contains duplicates (since it’s not deduped), and skipped can become negative or exceed total as a result. Align the counting with the same deduplication strategy used to build deduplicatedAccounts (dedupe the existing snapshot for baseline), and clamp skipped to >= 0 after using consistent units (deduped incoming vs. raw incoming).

import { AsyncLocalStorage } from "node:async_hooks";

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lib/storage.ts

 	const mergedAccounts = deduplicateAccounts([
-		...currentDeduplicatedAccounts,
-		...incomingDeduplicatedAccounts,
+		...currentAccounts,
+		...candidate.normalized.accounts,
 	]);
 	const wouldExceedLimit = mergedAccounts.length > ACCOUNT_LIMITS.MAX_ACCOUNTS;
 	const imported = wouldExceedLimit
 		? null
-		: mergedAccounts.length - currentDeduplicatedAccounts.length;
+		: mergedAccounts.length - currentAccounts.length;
 	const skipped = wouldExceedLimit
 		? null
-		: Math.max(0, incomingDeduplicatedAccounts.length - (imported ?? 0));
-	const changed = !haveEquivalentAccountRows(
-		mergedAccounts,
-		currentDeduplicatedAccounts,
-	);
+		: Math.max(0, candidate.normalized.accounts.length - (imported ?? 0));


lib/storage.ts

 	const transactionState = transactionSnapshotContext.getStore();
-	const currentStoragePath = normalizeStoragePathForComparison(getStoragePath());
+	const currentStoragePath = getStoragePath();
 	const storage = transactionState?.active
-		? normalizeStoragePathForComparison(transactionState.storagePath) ===
-			currentStoragePath
+		? transactionState.storagePath === currentStoragePath
 			? transactionState.snapshot


lib/storage.ts

 export async function importAccounts(
 	filePath: string,
-): Promise<ImportAccountsResult> {
+): Promise<{ imported: number; total: number; skipped: number }> {


lib/storage.ts

-		}
-		throw error;
-	}
+	const content = await fs.readFile(resolvedPath, "utf-8");


lib/codex-manager.ts

-	if (timestamp === null || timestamp === undefined || timestamp === 0)
-		return null;
-	if (!Number.isFinite(timestamp)) return null;
+	if (timestamp === null || timestamp === undefined) return null;


test/recovery.test.ts

+async function removeWithRetry(targetPath: string): Promise<void> {
+	for (let attempt = 0; attempt < 5; attempt += 1) {
+		try {
+			await fs.rm(targetPath, { recursive: true, force: true });
+			return;
+		} catch (error) {
+			const code = (error as NodeJS.ErrnoException).code;
+			if (
+				(code !== "EBUSY" && code !== "EPERM" && code !== "ENOTEMPTY") ||
+				attempt === 4
+			) {
+				throw error;
+			}
+			await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt));
+		}
+	}
+}


lib/storage.ts

lib/codex-manager.ts

lib/storage.ts

lib/codex-manager.ts

coderabbitai

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/storage.ts (1)
1942-1985: ⚠️ Potential issue | 🟡 Minor

compute imported against the deduped current state here.

lib/storage.ts:1942-1985 still subtracts currentAccounts.length, while the write path in lib/storage.ts:3008-3042 was already fixed to measure against deduped existing storage. if a caller passes duplicate-laden currentStorage, getActionableNamedBackupRestores() can drop a genuinely useful restore because the assessment.imported > 0 filter in lib/storage.ts:1849-1857 sees 0 or a negative count. there is no matching assessment regression beside test/storage.test.ts:587-630.
suggested fix
 function assessNamedBackupRestoreCandidate(
 	backup: NamedBackupMetadata,
 	candidate: LoadedBackupCandidate,
 	currentStorage: AccountStorageV3 | null,
 ): BackupRestoreAssessment {
 	const currentAccounts = currentStorage?.accounts ?? [];
+	const effectiveCurrentAccounts = deduplicateAccounts([...currentAccounts]);

 	if (!candidate.normalized || !backup.accountCount || backup.accountCount <= 0) {
 		return {
 			backup,
@@
 	}

 	const mergedAccounts = deduplicateAccounts([
-		...currentAccounts,
+		...effectiveCurrentAccounts,
 		...candidate.normalized.accounts,
 	]);
 	const wouldExceedLimit = mergedAccounts.length > ACCOUNT_LIMITS.MAX_ACCOUNTS;
 	const imported = wouldExceedLimit
 		? null
-		: mergedAccounts.length - currentAccounts.length;
+		: mergedAccounts.length - effectiveCurrentAccounts.length;
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 1942 - 1985, In
assessNamedBackupRestoreCandidate, imported and skipped are currently computed
against currentAccounts.length (which may include duplicates); change the
calculations to use the deduplicated current state (mergedAccounts minus
deduplicated current count) — i.e., compute dedupedCurrentCount =
deduplicateAccounts(currentAccounts).length (or use mergedAccounts.length and
dedupedCurrentCount) and set imported = wouldExceedLimit ? null :
mergedAccounts.length - dedupedCurrentCount, and recalc skipped =
wouldExceedLimit ? null : Math.max(0, candidate.normalized.accounts.length -
(imported ?? 0)); keep wouldExceedLimit and error logic intact and ensure
eligibleForRestore uses !wouldExceedLimit.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/cli.ts`:
- Around line 266-268: The check calling isInteractiveLoginMenuAvailable()
before returning promptLoginModeFallback(existingAccounts) is redundant because
the same non-interactive early return gate is already handled earlier; remove
the conditional block that wraps promptLoginModeFallback(existingAccounts) (the
isInteractiveLoginMenuAvailable() check and its return) so
promptLoginModeFallback is only reached via the existing early-return logic,
leaving other code paths and function names (isInteractiveLoginMenuAvailable,
promptLoginModeFallback) intact.

In `@lib/codex-manager.ts`:
- Around line 4391-4411: Wrap each assessNamedBackupRestore(backup.name, {
currentStorage }) call with a transient-retry wrapper before invoking
Promise.allSettled: on error check error.code (handle at least
"EBUSY","EPERM","EACCES","EAGAIN" and HTTP 429-like transient indications) and
retry with small exponential backoff (e.g., 3 total attempts with increasing
delays) logging each retry attempt but avoid printing sensitive data
(tokens/emails); only if all attempts fail, let the rejection propagate so the
existing settledAssessments handling logs the final skip message using
collapseWhitespace and backup.name as before; update or add vitest cases
exercising transient EBUSY/EPERM and 429 behavior to assert retries occur and
that final logs do not leak secrets.
- Around line 4062-4142: Add vitest regression tests exercising the multi-branch
startup recovery flow in codex-manager: simulate
getActionableNamedBackupRestores throwing a filesystem error (use
getRedactedFilesystemErrorLabel expectations) to assert the scan failure path
logs and continues OAuth; simulate it returning empty assessments to assert
resolveStartupRecoveryAction triggers the "open-empty-storage-menu" branch
(allowEmptyStorageMenu behavior); simulate recoverable assessments where
runBackupRestoreManager returns a non-"restored" value to assert
pendingRecoveryState is set and recoveryPromptAttempted is reset; and simulate
confirm/restore interactions where confirm throws to exercise the prompt failure
branch ensuring recoveryPromptAttempted is reset only when promptWasShown is
false; stub/mocks to manipulate applyUiThemeFromDashboardSettings,
loadDashboardDisplaySettings, getNamedBackupsDirectoryPath, and confirm to
isolate behavior and assert state transitions on recoveryPromptAttempted and
pendingRecoveryState.

In `@lib/storage.ts`:
- Around line 1988-1993: The change removed the exported boolean "changed" from
result objects, causing a silent API break; update the functions that return the
import result (e.g., restoreNamedBackup and the functions around the other diff
region that currently return { imported, total, skipped }) to include a boolean
changed property derived as (imported > 0) before returning, and ensure
importAccounts (and any other exported helpers that produce the result type)
also returns the same shape so callers still see result.changed.
- Around line 1720-1770: The current catch in listNamedBackupsWithoutLoading
collapses any readdir failure into an empty listing; change the error handling
so that only ENOENT returns {backups: [], totalBackups: 0} and any other error
is rethrown (i.e., in the catch for the top-level
retryTransientFilesystemOperation call inspect (error as
NodeJS.ErrnoException).code and if !== "ENOENT" throw the error), leaving the
per-entry ENOENT swallowing in place; update callers/tests that rely on the
distinction (the recovery tests referenced around the existing test ranges) to
expect a thrown error for locked/EBUSY directory cases instead of an empty
result.
- Around line 2987-2994: Remove the pre-check that uses existsSync and instead
let fs.readFile (inside retryTransientFilesystemOperation) surface errors; catch
errors from the read of resolvedPath and translate an ENOENT into the friendly
throw new Error(`Import file not found: ${resolvedPath}`) while rethrowing other
errors unchanged. Update the block that calls
retryTransientFilesystemOperation(() => fs.readFile(resolvedPath, "utf-8")) to
wrap that call in a try/catch, check err.code === 'ENOENT' and throw the
user-friendly message; keep retryTransientFilesystemOperation, resolvedPath, and
fs.readFile references so the change is localized. Also adjust/extend
test/storage.test.ts:1356-1410 to add a delete-between-check-and-read scenario
(simulate removal during the operation) to ensure the ENOENT branch is covered
and no raw fs errors leak.

---

Outside diff comments:
In `@lib/storage.ts`:
- Around line 1942-1985: In assessNamedBackupRestoreCandidate, imported and
skipped are currently computed against currentAccounts.length (which may include
duplicates); change the calculations to use the deduplicated current state
(mergedAccounts minus deduplicated current count) — i.e., compute
dedupedCurrentCount = deduplicateAccounts(currentAccounts).length (or use
mergedAccounts.length and dedupedCurrentCount) and set imported =
wouldExceedLimit ? null : mergedAccounts.length - dedupedCurrentCount, and
recalc skipped = wouldExceedLimit ? null : Math.max(0,
candidate.normalized.accounts.length - (imported ?? 0)); keep wouldExceedLimit
and error logic intact and ensure eligibleForRestore uses !wouldExceedLimit.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 36ff8d22-40b5-4db9-b368-e1d6b7e7891b

📥 Commits

Reviewing files that changed from the base of the PR and between 8212831 and a7d87cd.

📒 Files selected for processing (12)

docs/getting-started.md
docs/reference/public-api.md
docs/troubleshooting.md
docs/upgrade.md
lib/cli.ts
lib/codex-manager.ts
lib/storage.ts
lib/ui/copy.ts
test/codex-manager-cli.test.ts
test/recovery.test.ts
test/storage-recovery-paths.test.ts
test/storage.test.ts

💤 Files with no reviewable changes (2)

docs/reference/public-api.md
lib/ui/copy.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)

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.md
docs/troubleshooting.md
docs/upgrade.md

lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

lib/cli.ts
lib/storage.ts
lib/codex-manager.ts

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

test/storage.test.ts
test/storage-recovery-paths.test.ts
test/recovery.test.ts

lib/cli.ts

coderabbitai · 2026-03-16T11:31:35Z

lib/codex-manager.ts

+		const canPromptForRecovery =
+			!suppressRecoveryPrompt &&
+			!recoveryPromptAttempted &&
+			existingCount === 0 &&
+			isInteractiveLoginMenuAvailable();
+		if (canPromptForRecovery) {
+			recoveryPromptAttempted = true;
+			let recoveryState: Awaited<
+				ReturnType<typeof getActionableNamedBackupRestores>
+			> | null = pendingRecoveryState;
+			pendingRecoveryState = null;
+			if (recoveryState === null) {
+				let recoveryScanFailed = false;
+				let scannedRecoveryState: Awaited<
+					ReturnType<typeof getActionableNamedBackupRestores>
+				>;
+				try {
+					scannedRecoveryState = await getActionableNamedBackupRestores({
+						currentStorage: refreshedStorage,
+					});
+				} catch (error) {
+					recoveryScanFailed = true;
+					const errorLabel = getRedactedFilesystemErrorLabel(error);
+					console.warn(
+						`Startup recovery scan failed (${errorLabel}). Continuing with OAuth.`,
+					);
+					scannedRecoveryState = {
+						assessments: [],
+						allAssessments: [],
+						totalBackups: 0,
+					};
+				}
+				recoveryState = scannedRecoveryState;
+				if (
+					resolveStartupRecoveryAction(scannedRecoveryState, recoveryScanFailed) ===
+					"open-empty-storage-menu"
+				) {
+					allowEmptyStorageMenu = true;
+					continue loginFlow;
+				}
+			}
+			if (recoveryState.assessments.length > 0) {
+				let promptWasShown = false;
+				try {
+					const displaySettings = await loadDashboardDisplaySettings();
+					applyUiThemeFromDashboardSettings(displaySettings);
+					const backupDir = getNamedBackupsDirectoryPath();
+					const backupLabel =
+						recoveryState.assessments.length === 1
+							? recoveryState.assessments
+									.map((assessment) => assessment.backup.name)
+									.join("")
+							: `${recoveryState.assessments.length} backups`;
+					promptWasShown = true;
+					const restoreNow = await confirm(
+						`Found ${recoveryState.assessments.length} recoverable backup${
+							recoveryState.assessments.length === 1 ? "" : "s"
+						} out of ${recoveryState.totalBackups} total (${backupLabel}) in ${backupDir}. Restore now?`,
+					);
+					if (restoreNow) {
+						const restoreResult = await runBackupRestoreManager(
+							displaySettings,
+							recoveryState.allAssessments,
+						);
+						if (restoreResult !== "restored") {
+							pendingRecoveryState = recoveryState;
+							recoveryPromptAttempted = false;
+						}
+						continue;
+					}
+				} catch (error) {
+					if (!promptWasShown) {
+						recoveryPromptAttempted = false;
+					}
+					const errorLabel = getRedactedFilesystemErrorLabel(error);
+					console.warn(
+						`Startup recovery prompt failed (${errorLabel}). Continuing with OAuth.`,
+					);
+				}
+			}
+		}


⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash set -euo pipefail echo "== candidate recovery/auth tests ==" fd -t f '.*(recovery|auth|login).*test\.(ts|tsx)$' test echo echo "== references to new startup-recovery symbols in tests ==" rg -n -C2 'resolveStartupRecoveryAction|show-recovery-prompt|open-empty-storage-menu|continue-with-oauth|suppressRecoveryPrompt|recoveryPromptAttempted|allowEmptyStorageMenu|pendingRecoveryState' test echo echo "== references to fresh/reset suppression in tests ==" rg -n -C2 'fresh|reset|startup recovery|recovery prompt' test

Repository: ndycode/codex-multi-auth

Length of output: 50381

🏁 Script executed:

wc -l test/recovery.test.ts

Repository: ndycode/codex-multi-auth

Length of output: 94

🏁 Script executed:

sed -n '783,900p' test/recovery.test.ts

Repository: ndycode/codex-multi-auth

Length of output: 3861

🏁 Script executed:

rg -n 'suppressRecoveryPrompt|recoveryPromptAttempted|pendingRecoveryState|allowEmptyStorageMenu' test/recovery.test.ts

Repository: ndycode/codex-multi-auth

Length of output: 50

🏁 Script executed:

rg -n -A5 'restore.*manager|runBackupRestoreManager' test/recovery.test.ts

Repository: ndycode/codex-multi-auth

Length of output: 50

🏁 Script executed:

sed -n '4062,4142p' lib/codex-manager.ts

Repository: ndycode/codex-multi-auth

Length of output: 2719

🏁 Script executed:

rg -n 'canPromptForRecovery|suppressRecoveryPrompt|recoveryPromptAttempted|pendingRecoveryState' lib/codex-manager.ts | head -20

Repository: ndycode/codex-multi-auth

Length of output: 709

🏁 Script executed:

fd -t f '.*(integration|e2e|startup).*test\.(ts|tsx)$' test

Repository: ndycode/codex-multi-auth

Length of output: 139

🏁 Script executed:

rg -n 'loginFlow\|runBackupRestoreManager' test/ --type ts -A3 | head -30

Repository: ndycode/codex-multi-auth

Length of output: 50

🏁 Script executed:

rg -n 'loginFlow|startup.*recovery|recovery.*prompt' test/oauth-server.integration.test.ts test/rotation-integration.test.ts

Repository: ndycode/codex-multi-auth

Length of output: 50

🏁 Script executed:

rg -n 'EBUSY|429|rate.*limit' lib/codex-manager.ts | head -20

Repository: ndycode/codex-multi-auth

Length of output: 711

🏁 Script executed:

rg -n 'getRedactedFilesystemErrorLabel' lib/codex-manager.ts -B2 -A5 | head -40

Repository: ndycode/codex-multi-auth

Length of output: 1321

🏁 Script executed:

rg -n -C3 'console.warn.*recovery' lib/codex-manager.ts

Repository: ndycode/codex-multi-auth

Length of output: 50

add vitest regression coverage for startup recovery state transitions.

lib/codex-manager.ts:4062-4142 adds a multi-branch state machine (suppressRecoveryPrompt, recoveryPromptAttempted, pendingRecoveryState, allowEmptyStorageMenu) with only unit test coverage of the resolveStartupRecoveryAction() helper. missing regression tests for:

scan failure recovery path (continues oauth, logs redacted error)

empty scan path (re-enters empty storage menu)

same-session suppression reset when restore manager returns non-"restored" value (lib/codex-manager.ts:4127-4128)

prompt failure path with promptWasShown guard resetting recoveryPromptAttempted (lib/codex-manager.ts:4134)

also add explicit coverage for windows filesystem io errors (ebusy scenarios) in the recovery scan path (lib/codex-manager.ts:4082-4087), since the current implementation catches but does not retry on transient errors.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4062 - 4142, Add vitest regression tests exercising the multi-branch startup recovery flow in codex-manager: simulate getActionableNamedBackupRestores throwing a filesystem error (use getRedactedFilesystemErrorLabel expectations) to assert the scan failure path logs and continues OAuth; simulate it returning empty assessments to assert resolveStartupRecoveryAction triggers the "open-empty-storage-menu" branch (allowEmptyStorageMenu behavior); simulate recoverable assessments where runBackupRestoreManager returns a non-"restored" value to assert pendingRecoveryState is set and recoveryPromptAttempted is reset; and simulate confirm/restore interactions where confirm throws to exercise the prompt failure branch ensuring recoveryPromptAttempted is reset only when promptWasShown is false; stub/mocks to manipulate applyUiThemeFromDashboardSettings, loadDashboardDisplaySettings, getNamedBackupsDirectoryPath, and confirm to isolate behavior and assert state transitions on recoveryPromptAttempted and pendingRecoveryState.

coderabbitai · 2026-03-16T11:31:35Z

lib/codex-manager.ts

 		const settledAssessments = await Promise.allSettled(
 			chunk.map((backup) =>
-				assessNamedBackupRestore(backup.name, {
-					currentStorage,
-					candidateCache,
-				}),
+				assessNamedBackupRestore(backup.name, { currentStorage }),
 			),
 		);
 		for (const [resultIndex, result] of settledAssessments.entries()) {
 			if (result.status === "fulfilled") {
 				assessments.push(result.value);
 				continue;
 			}
-			if (isNamedBackupContainmentError(result.reason)) {
-				throw result.reason;
-			}
 			const backupName = chunk[resultIndex]?.name ?? "unknown";
 			const reason =
 				result.reason instanceof Error
 					? result.reason.message
 					: String(result.reason);
-			const normalizedReason =
-				collapseWhitespace(reason) || "unknown error";
-			assessmentFailures.push(`${backupName}: ${normalizedReason}`);
 			console.warn(
-				`Skipped backup assessment for "${backupName}": ${normalizedReason}`,
+				`Skipped backup assessment for "${backupName}": ${
+					collapseWhitespace(reason) || "unknown error"
+				}`,
 			);
 		}


⚠️ Potential issue | 🟠 Major

add transient fs retry before skipping backup assessments.

in lib/codex-manager.ts:4391-4411, assessment failures are skipped immediately. on windows, transient ebusy/eperm can hide recoverable backups and suppress the startup recovery prompt for this run.

proposed hardening

+async function assessNamedBackupWithRetry( + backupName: string, + currentStorage: Awaited<ReturnType<typeof loadAccounts>>, + maxAttempts = 3, +): Promise<BackupRestoreAssessment> { + let lastError: unknown; + for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { + try { + return await assessNamedBackupRestore(backupName, { currentStorage }); + } catch (error) { + lastError = error; + const code = (error as { code?: string })?.code; + const retryable = code === "EBUSY" || code === "EPERM" || code === "EMFILE"; + if (!retryable || attempt === maxAttempts) throw error; + } + } + throw lastError; +} ... - const settledAssessments = await Promise.allSettled( - chunk.map((backup) => - assessNamedBackupRestore(backup.name, { currentStorage }), - ), - ); + const settledAssessments = await Promise.allSettled( + chunk.map((backup) => + assessNamedBackupWithRetry(backup.name, currentStorage), + ), + );

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 4391 - 4411, Wrap each assessNamedBackupRestore(backup.name, { currentStorage }) call with a transient-retry wrapper before invoking Promise.allSettled: on error check error.code (handle at least "EBUSY","EPERM","EACCES","EAGAIN" and HTTP 429-like transient indications) and retry with small exponential backoff (e.g., 3 total attempts with increasing delays) logging each retry attempt but avoid printing sensitive data (tokens/emails); only if all attempts fail, let the rejection propagate so the existing settledAssessments handling logs the final skip message using collapseWhitespace and backup.name as before; update or add vitest cases exercising transient EBUSY/EPERM and 429 behavior to assert retries occur and that final logs do not leak secrets.

lib/storage.ts

test/recovery.test.ts

lib/storage.ts

…recovery-prompt

coderabbitai

Actionable comments posted: 5

♻️ Duplicate comments (2)

lib/codex-manager.ts (2)
3840-3851: ⚠️ Potential issue | 🟠 Major

add vitest coverage for the new startup recovery state machine before merge.

line [4064] through line [4144] introduces multi-flag control flow that can regress silently without branch tests. i do not see evidence in this patch context that all new transitions are asserted (scan failure, open-empty-menu routing, prompt failure before/after prompt shown, non-restored manager result resetting state). refs: lib/codex-manager.ts:3840, lib/codex-manager.ts:4064, test/recovery.test.ts:783.
#!/bin/bash
set -euo pipefail

echo "== startup recovery symbols in tests =="
rg -n -C3 --type=ts 'resolveStartupRecoveryAction|open-empty-storage-menu|recoveryPromptAttempted|pendingRecoveryState|suppressRecoveryPrompt|Startup recovery scan failed|Startup recovery prompt failed' test

echo
echo "== backup manager result branches in tests =="
rg -n -C3 --type=ts 'runBackupRestoreManager|restored|dismissed|failed' test/recovery.test.ts test/codex-manager-cli.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."

Also applies to: 4064-4144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/codex-manager.ts` around lines 3840 - 3851, Add vitest unit tests to
cover every branch of the new startup recovery state machine: exercise
resolveStartupRecoveryAction with recoveryState.assessments > 0,
recoveryScanFailed true, totalBackups > 0, and the default
open-empty-storage-menu case; add tests that simulate prompt flow toggles
(pendingRecoveryState, recoveryPromptAttempted, suppressRecoveryPrompt) both
before and after the prompt is shown and when the prompt fails; add tests that
stub/mock runBackupRestoreManager to return restored, dismissed, and failed
outcomes and assert the manager result correctly resets/updates state (e.g.,
pendingRecoveryState cleared, suppressRecoveryPrompt set/unset); put these tests
in test/recovery.test.ts (or adjacent vitest files), using the same symbols
referenced in the diff (resolveStartupRecoveryAction, pendingRecoveryState,
recoveryPromptAttempted, suppressRecoveryPrompt, runBackupRestoreManager,
restored, dismissed, failed) and ensure you assert transitions for the
scan-failure and open-empty-menu routing paths.
4387-4413: ⚠️ Potential issue | 🟠 Major

cover transient filesystem behavior for the backup assessment queue with vitest.

line [4393] batches restore assessments concurrently. this path should have explicit tests for transient EBUSY/EPERM and terminal failure behavior so queue semantics stay stable across windows io edge cases. refs: lib/codex-manager.ts:4387, lib/codex-manager.ts:4393, test/recovery.test.ts:1.
#!/bin/bash
set -euo pipefail

echo "== queue + fs transient handling in source =="
rg -n -C3 --type=ts 'NAMED_BACKUP_LIST_CONCURRENCY|Promise.allSettled|assessNamedBackupRestore|EBUSY|EPERM|EAGAIN' lib/codex-manager.ts lib/storage.ts

echo
echo "== corresponding vitest coverage =="
rg -n -C3 --type=ts 'EBUSY|EPERM|EAGAIN|retryTransientFilesystemOperation|assessNamedBackupRestore|loadBackupRestoreManagerAssessments' test
as per coding guidelines, "lib/**: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/codex-manager.ts` around lines 4387 - 4413, Add vitest coverage that
simulates transient Windows FS errors and terminal failures for the concurrent
batch path that uses NAMED_BACKUP_LIST_CONCURRENCY and Promise.allSettled in
loadBackupRestoreManagerAssessments/assessNamedBackupRestore: write tests in
test/recovery.test.ts that stub/mock the underlying filesystem calls (or stub
assessNamedBackupRestore) to first throw transient errors (EBUSY/EPERM/EAGAIN)
and then succeed, asserting the batch retry semantics allow the assessment to
eventually be pushed into assessments, and a separate test where the stub throws
a terminal error ensuring the code logs/skips the item (matching the
console.warn branch); use vitest vi.fn/mocks and, if code uses
retryTransientFilesystemOperation, assert it is invoked, otherwise assert
behavior by controlling timing and promises so the Promise.allSettled loop
handles transient vs terminal cases correctly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/storage.ts`:
- Around line 3030-3039: The code currently calls deduplicateAccounts(merged)
twice in the import path (once inside the limit check and again when assigning
deduplicatedAccounts); compute dedupe once and reuse it: call const deduplicated
= deduplicateAccounts(merged) before the ACCOUNT_LIMITS.MAX_ACCOUNTS check, use
deduplicated.length in the guard and then assign deduplicatedAccounts =
deduplicated (or rename consistently) so you avoid redundant work on large
imports and keep references to merged, deduplicateAccounts, and
ACCOUNT_LIMITS.MAX_ACCOUNTS intact.
- Around line 1779-1806: Add Windows-specific tests that mock process.platform =
"win32" and assert that retryTransientFilesystemOperation actually retries on
EPERM and EBUSY: for each code, create a fake operation that throws a
NodeJS.ErrnoException with .code set to "EPERM" or "EBUSY" for the first N
attempts then resolves, call retryTransientFilesystemOperation and assert it
eventually returns and that the fake was called multiple times; restore
process.platform after each test. Use the same pattern to keep the Linux tests
(process.platform !== "win32") asserting EPERM/EBUSY rethrow immediately, and
reference the functions isRetryableFilesystemErrorCode and
retryTransientFilesystemOperation when locating the code under test.

In `@test/recovery.test.ts`:
- Around line 380-453: The test "keeps actionable backups when fast-path scan
hits EBUSY" relies on platform-specific EBUSY handling; wrap the part that calls
getActionableNamedBackupRestores with an explicit process.platform override
(save original, use Object.defineProperty to set process.platform to 'win32' for
the Windows behavior the test expects, then restore original afterward) so the
expectations remain deterministic; apply the same pattern to the other related
tests mentioned (the ones calling getActionableNamedBackupRestores and asserting
EBUSY behavior) to ensure consistent platform-specific behavior.
- Around line 783-815: Add regression tests for resolveStartupRecoveryAction to
cover the missing branches: call resolveStartupRecoveryAction with an input
object where assessments contains at least one actionable assessment (e.g.,
assessments: [{ actionable: true }]) and totalBackups > 0 and scanFailed = false
and assert it returns the recovery prompt route (the code path currently implied
by "open-empty-storage-menu"/recovery prompt); then add a test where assessments
contains an actionable item but scanning is suppressed (pass the
suppression/scanFailed flag that triggers same-session suppression) and assert
it returns "continue-with-oauth". Keep the existing tests for zero backups/no
scan error as-is. Reference resolveStartupRecoveryAction and the input shape
(assessments, totalBackups, scanFailed) when adding the two new cases.

In `@test/storage.test.ts`:
- Around line 1605-1617: The two tests are ambiguous about platform: ensure the
non-Windows test for EBUSY retry uses a mocked process.platform returning
"linux" and the Windows-retry test uses process.platform returning "win32" so
behavior is deterministic; in the non-Windows case (test around
listNamedBackups) keep vi.spyOn(process, "platform",
"get").mockReturnValue("linux") and assert readdir was called once, and in the
separate Windows-retry test (lines ~1638-1678) mock process.platform to "win32"
and set readdir to initially reject with an EBUSY error then succeed on retry to
assert retry behavior; also restore/clear spies/mocks after each test to avoid
cross-test leakage.

---

Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 3840-3851: Add vitest unit tests to cover every branch of the new
startup recovery state machine: exercise resolveStartupRecoveryAction with
recoveryState.assessments > 0, recoveryScanFailed true, totalBackups > 0, and
the default open-empty-storage-menu case; add tests that simulate prompt flow
toggles (pendingRecoveryState, recoveryPromptAttempted, suppressRecoveryPrompt)
both before and after the prompt is shown and when the prompt fails; add tests
that stub/mock runBackupRestoreManager to return restored, dismissed, and failed
outcomes and assert the manager result correctly resets/updates state (e.g.,
pendingRecoveryState cleared, suppressRecoveryPrompt set/unset); put these tests
in test/recovery.test.ts (or adjacent vitest files), using the same symbols
referenced in the diff (resolveStartupRecoveryAction, pendingRecoveryState,
recoveryPromptAttempted, suppressRecoveryPrompt, runBackupRestoreManager,
restored, dismissed, failed) and ensure you assert transitions for the
scan-failure and open-empty-menu routing paths.
- Around line 4387-4413: Add vitest coverage that simulates transient Windows FS
errors and terminal failures for the concurrent batch path that uses
NAMED_BACKUP_LIST_CONCURRENCY and Promise.allSettled in
loadBackupRestoreManagerAssessments/assessNamedBackupRestore: write tests in
test/recovery.test.ts that stub/mock the underlying filesystem calls (or stub
assessNamedBackupRestore) to first throw transient errors (EBUSY/EPERM/EAGAIN)
and then succeed, asserting the batch retry semantics allow the assessment to
eventually be pushed into assessments, and a separate test where the stub throws
a terminal error ensuring the code logs/skips the item (matching the
console.warn branch); use vitest vi.fn/mocks and, if code uses
retryTransientFilesystemOperation, assert it is invoked, otherwise assert
behavior by controlling timing and promises so the Promise.allSettled loop
handles transient vs terminal cases correctly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: fad95d8e-47a4-43f3-a7eb-b8866b1fb048

📥 Commits

Reviewing files that changed from the base of the PR and between a7d87cd and 800d49f.

📒 Files selected for processing (7)

lib/cli.ts
lib/codex-manager.ts
lib/storage.ts
lib/ui/copy.ts
test/codex-manager-cli.test.ts
test/recovery.test.ts
test/storage.test.ts

💤 Files with no reviewable changes (1)

lib/ui/copy.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (2)

lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

lib/cli.ts
lib/codex-manager.ts
lib/storage.ts

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

test/recovery.test.ts
test/storage.test.ts

🔇 Additional comments (3)

lib/cli.ts (1)

29-31: good consolidation of interactive gating.

this keeps tty/non-interactive checks centralized and makes menu fallback behavior predictable. refs: lib/cli.ts:29, lib/cli.ts:258.

Also applies to: 258-260

lib/codex-manager.ts (1)

4510-4529: good fix: restore path now syncs codex active account.

post-restore sync closes the direct-restore drift between multi-auth state and codex cli state. refs: lib/codex-manager.ts:4510, lib/codex-manager.ts:4515.

lib/storage.ts (1)

2995-3005: nice hardening on import file read/enoent handling.

this correctly translates read-time ENOENT into a stable user-facing error and avoids pre-check race issues. refs: lib/storage.ts:2995, lib/storage.ts:3001.

coderabbitai · 2026-03-16T16:14:09Z

lib/storage.ts

 function isRetryableFilesystemErrorCode(
 	code: string | undefined,
 ): code is "EPERM" | "EBUSY" | "EAGAIN" {
 	if (code === "EAGAIN") {
 		return true;
 	}
-	if (process.platform !== "win32") {
-		return false;
-	}
-	return code === "EPERM" || code === "EBUSY";
+	return (code === "EPERM" || code === "EBUSY") && process.platform === "win32";
 }

 async function retryTransientFilesystemOperation<T>(
 	operation: () => Promise<T>,
 ): Promise<T> {
-	let attempt = 0;
-	while (true) {
+	for (let attempt = 0; attempt < 5; attempt += 1) {
 		try {
 			return await operation();
 		} catch (error) {
 			const code = (error as NodeJS.ErrnoException).code;
-			if (
-				!isRetryableFilesystemErrorCode(code) ||
-				attempt >= TRANSIENT_FILESYSTEM_MAX_ATTEMPTS - 1
-			) {
+			if (!isRetryableFilesystemErrorCode(code) || attempt === 4) {
 				throw error;
 			}
-			const baseDelayMs = TRANSIENT_FILESYSTEM_BASE_DELAY_MS * 2 ** attempt;
-			const jitterMs = Math.floor(Math.random() * baseDelayMs);
 			await new Promise((resolve) =>
-				setTimeout(resolve, baseDelayMs + jitterMs),
+				setTimeout(resolve, 10 * 2 ** attempt + Math.floor(Math.random() * 10)),
 			);
 		}
-		attempt += 1;
 	}
+
+	throw new Error("Retry loop exhausted unexpectedly");
 }


⚠️ Potential issue | 🟠 Major

❓ Verification inconclusive

Script executed:

#!/bin/bash set -euo pipefail echo "== retry policy implementation ==" rg -n -C3 --type=ts 'isRetryableFilesystemErrorCode|retryTransientFilesystemOperation|EPERM|EBUSY|EAGAIN|process.platform' lib/storage.ts echo echo "== retry policy tests ==" rg -n -C3 --type=ts 'retryTransientFilesystemOperation|EPERM|EBUSY|EAGAIN|win32|process.platform' test

Repository: ndycode/codex-multi-auth

Repository: ndycode/codex-multi-auth
Exit code: 0

stdout:

== retry policy implementation == 239-export function formatStorageErrorHint(error: unknown, path: string): string { 240- const err = error as NodeJS.ErrnoException; 241- const code = err?.code || "UNKNOWN"; 242: const isWindows = process.platform === "win32"; 243- 244- switch (code) { 245- case "EACCES": 246: case "EPERM": 247- return isWindows 248- ? `Permission denied writing to ${path}. Check antivirus exclusions for this folder. Ensure you have write permissions.` 249- : `Permission denied writing to ${path}. Check folder permissions. Try: chmod 755 ~/.codex`; 250: case "EBUSY": 251- return `File is locked at ${path}. The file may be open in another program. Close any editors or processes accessing it.`; 252- case "ENOSPC": 253- return `Disk is full. Free up space and try again. Path: ${path}`; -- 287- if (code === "ENOENT") { 288- return; 289- } 290: if ((code === "EPERM" || code === "EBUSY" || code === "EAGAIN") && attempt < 4) { 291- await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt)); 292- continue; 293- } -- 466- return; 467- } 468- const canRetry = 469: (code === "EPERM" || code === "EBUSY") && 470- attempt + 1 < BACKUP_COPY_MAX_ATTEMPTS; 471- if (canRetry) { 472- await new Promise((resolve) => -- 490- } catch (error) { 491- const code = (error as NodeJS.ErrnoException).code; 492- const canRetry = 493: (code === "EPERM" || code === "EBUSY" || code === "EAGAIN") && 494- attempt + 1 < BACKUP_COPY_MAX_ATTEMPTS; 495- if (!canRetry) { 496- throw error; -- 1651-async function scanNamedBackups(): Promise<NamedBackupScanResult> { 1652- const backupRoot = getNamedBackupRoot(getStoragePath()); 1653- try { 1654: const entries = await retryTransientFilesystemOperation(() => 1655- fs.readdir(backupRoot, { withFileTypes: true }), 1656- ); 1657- const backupEntries = entries -- 1720-async function listNamedBackupsWithoutLoading(): Promise<NamedBackupMetadataListingResult> { 1721- const backupRoot = getNamedBackupRoot(getStoragePath()); 1722- try { 1723: const entries = await retryTransientFilesystemOperation(() => 1724- fs.readdir(backupRoot, { withFileTypes: true }), 1725- ); 1726- const backups: NamedBackupMetadata[] = []; -- 1776- return scanResult.backups.map((entry) => entry.backup); 1777-} 1778- 1779:function isRetryableFilesystemErrorCode( 1780- code: string | undefined, 1781:): code is "EPERM" | "EBUSY" | "EAGAIN" { 1782: if (code === "EAGAIN") { 1783- return true; 1784- } 1785: return (code === "EPERM" || code === "EBUSY") && process.platform === "win32"; 1786-} 1787- 1788:async function retryTransientFilesystemOperation<T>( 1789- operation: () => Promise<T>, 1790-): Promise<T> { 1791- for (let attempt = 0; attempt < 5; attempt += 1) { -- 1793- return await operation(); 1794- } catch (error) { 1795- const code = (error as NodeJS.ErrnoException).code; 1796: if (!isRetryableFilesystemErrorCode(code) || attempt === 4) { 1797- throw error; 1798- } 1799- await new Promise((resolve) => -- 2022- 2023-async function loadBackupCandidate(path: string): Promise<LoadedBackupCandidate> { 2024- try { 2025: return await retryTransientFilesystemOperation(() => 2026- loadAccountsFromPath(path), 2027- ); 2028- } catch (error) { -- 2036-} 2037- 2038-function equalsNamedBackupEntry(left: string, right: string): boolean { 2039: return process.platform === "win32" 2040- ? left.toLowerCase() === right.toLowerCase() 2041- : left === right; 2042-} -- 2062- const requestedBaseName = stripNamedBackupJsonExtension(requestedWithExtension); 2063- 2064- try { 2065: const entries = await retryTransientFilesystemOperation(() => 2066- fs.readdir(backupRoot, { withFileTypes: true }), 2067- ); 2068- for (const entry of entries) { -- 2348- ctimeMs?: number; 2349- } | null = null; 2350- try { 2351: stats = await retryTransientFilesystemOperation(() => fs.stat(path)); 2352- } catch (error) { 2353- const code = (error as NodeJS.ErrnoException).code; 2354- if (code !== "ENOENT") { -- 2943- const transactionState = transactionSnapshotContext.getStore(); 2944- const currentStoragePath = getStoragePath(); 2945- const storage = transactionState?.active 2946: ? (process.platform === "win32" 2947- ? transactionState.storagePath?.toLowerCase() === 2948- currentStoragePath?.toLowerCase() 2949- : transactionState.storagePath === currentStoragePath) -- 2994- 2995- let content: string; 2996- try { 2997: content = await retryTransientFilesystemOperation(() => 2998- fs.readFile(resolvedPath, "utf-8"), 2999- ); 3000- } catch (error) { == retry policy tests == test/test-model-matrix-script.test.ts-188- it("filters non-path where output on Windows", async () => { test/test-model-matrix-script.test.ts-189- const platformSpy = vi test/test-model-matrix-script.test.ts-190- .spyOn(process, "platform", "get") test/test-model-matrix-script.test.ts:191: .mockReturnValue("win32"); test/test-model-matrix-script.test.ts-192- try { test/test-model-matrix-script.test.ts-193- spawnSync.mockReturnValue({ test/test-model-matrix-script.test.ts-194- stdout: -- test/test-model-matrix-script.test.ts-210- it("returns fallback command when where has no executable candidates", async () => { test/test-model-matrix-script.test.ts-211- const platformSpy = vi test/test-model-matrix-script.test.ts-212- .spyOn(process, "platform", "get") test/test-model-matrix-script.test.ts:213: .mockReturnValue("win32"); test/test-model-matrix-script.test.ts-214- try { test/test-model-matrix-script.test.ts-215- spawnSync.mockReturnValue({ test/test-model-matrix-script.test.ts-216- stdout: "", -- test/test-model-matrix-script.test.ts-231- it("serializes stopCodexServers calls and kills only tracked Windows PIDs", async () => { test/test-model-matrix-script.test.ts-232- const platformSpy = vi test/test-model-matrix-script.test.ts-233- .spyOn(process, "platform", "get") test/test-model-matrix-script.test.ts:234: .mockReturnValue("win32"); test/test-model-matrix-script.test.ts-235- try { test/test-model-matrix-script.test.ts-236- spawnSync.mockReturnValue({ status: 0, stdout: "", stderr: "" }); test/test-model-matrix-script.test.ts-237- -- test/unified-settings.test.ts-123- const renameSpy = vi.spyOn(fs, "rename"); test/unified-settings.test.ts-124- renameSpy.mockImplementationOnce(async () => { test/unified-settings.test.ts-125- const error = new Error("busy") as NodeJS.ErrnoException; test/unified-settings.test.ts:126: error.code = "EBUSY"; test/unified-settings.test.ts-127- throw error; test/unified-settings.test.ts-128- }); test/unified-settings.test.ts-129- try { -- test/unified-settings.test.ts-167- ).toBe(""); test/unified-settings.test.ts-168- }); test/unified-settings.test.ts-169- test/unified-settings.test.ts:170: it("retries async rename on windows-style EPERM lock", async () => { test/unified-settings.test.ts-171- const { saveUnifiedPluginConfig, loadUnifiedPluginConfigSync } = test/unified-settings.test.ts-172- await import("../lib/unified-settings.js"); test/unified-settings.test.ts-173- const renameSpy = vi.spyOn(fs, "rename"); test/unified-settings.test.ts-174- renameSpy.mockImplementationOnce(async () => { test/unified-settings.test.ts-175- const error = new Error("perm") as NodeJS.ErrnoException; test/unified-settings.test.ts:176: error.code = "EPERM"; test/unified-settings.test.ts-177- throw error; test/unified-settings.test.ts-178- }); test/unified-settings.test.ts-179- try { -- test/unified-settings.test.ts-188- } test/unified-settings.test.ts-189- }); test/unified-settings.test.ts-190- test/unified-settings.test.ts:191: it("cleans async temp file when rename repeatedly fails with EPERM", async () => { test/unified-settings.test.ts-192- const { saveUnifiedPluginConfig, getUnifiedSettingsPath } = await import( test/unified-settings.test.ts-193- "../lib/unified-settings.js" test/unified-settings.test.ts-194- ); test/unified-settings.test.ts-195- const renameSpy = vi.spyOn(fs, "rename"); test/unified-settings.test.ts-196- renameSpy.mockImplementation(async () => { test/unified-settings.test.ts-197- const error = new Error("perm locked") as NodeJS.ErrnoException; test/unified-settings.test.ts:198: error.code = "EPERM"; test/unified-settings.test.ts-199- throw error; test/unified-settings.test.ts-200- }); test/unified-settings.test.ts-201- try { -- test/unified-settings.test.ts-395- const readSpy = vi.spyOn(fs, "readFile"); test/unified-settings.test.ts-396- readSpy.mockImplementationOnce(async () => { test/unified-settings.test.ts-397- const error = new Error("file locked") as NodeJS.ErrnoException; test/unified-settings.test.ts:398: error.code = "EBUSY"; test/unified-settings.test.ts-399- throw error; test/unified-settings.test.ts-400- }); test/unified-settings.test.ts-401- -- test/storage-flagged.test.ts-12- setStoragePathDirect, test/storage-flagged.test.ts-13-} from "../lib/storage.js"; test/storage-flagged.test.ts-14- test/storage-flagged.test.ts:15:const RETRYABLE_REMOVE_CODES = new Set(["EBUSY", "EPERM", "ENOTEMPTY"]); test/storage-flagged.test.ts-16- test/storage-flagged.test.ts-17-async function removeWithRetry( test/storage-flagged.test.ts-18- targetPath: string, -- test/storage-flagged.test.ts-232- expect(flagged.accounts).toHaveLength(0); test/storage-flagged.test.ts-233- }); test/storage-flagged.test.ts-234- test/storage-flagged.test.ts:235: it.each(["EPERM", "EBUSY"] as const)( test/storage-flagged.test.ts-236- "retries transient %s when clearing flagged storage", test/storage-flagged.test.ts-237- async (code) => { test/storage-flagged.test.ts-238- await saveFlaggedAccounts({ -- test/storage-flagged.test.ts-276- }, test/storage-flagged.test.ts-277- ); test/storage-flagged.test.ts-278- test/storage-flagged.test.ts:279: it.each(["EPERM", "EBUSY"] as const)( test/storage-flagged.test.ts-280- "returns false when clearing flagged storage exhausts retryable %s failures", test/storage-flagged.test.ts-281- async (code) => { test/storage-flagged.test.ts-282- await saveFlaggedAccounts({ -- test/storage-flagged.test.ts-354- const [targetPath] = args; test/storage-flagged.test.ts-355- if (targetPath === flaggedPath) { test/storage-flagged.test.ts-356- const error = new Error( test/storage-flagged.test.ts:357: "EPERM flagged read", test/storage-flagged.test.ts-358- ) as NodeJS.ErrnoException; test/storage-flagged.test.ts:359: error.code = "EPERM"; test/storage-flagged.test.ts-360- throw error; test/storage-flagged.test.ts-361- } test/storage-flagged.test.ts-362- return originalReadFile(...args); -- test/storage.test.ts-711- flaggedRenameAttempts += 1; test/storage.test.ts-712- const error = Object.assign( test/storage.test.ts-713- new Error("flagged storage busy"), test/storage.test.ts:714: { code: "EBUSY" }, test/storage.test.ts-715- ); test/storage.test.ts-716- throw error; test/storage.test.ts-717- } -- test/storage.test.ts-822- flaggedRenameAttempts += 1; test/storage.test.ts-823- const error = Object.assign( test/storage.test.ts-824- new Error("flagged storage busy"), test/storage.test.ts:825: { code: "EBUSY" }, test/storage.test.ts-826- ); test/storage.test.ts-827- throw error; test/storage.test.ts-828- } -- test/storage.test.ts-831- if (accountRenameAttempts > 1) { test/storage.test.ts-832- const error = Object.assign( test/storage.test.ts-833- new Error("rollback account storage busy"), test/storage.test.ts:834: { code: "EBUSY" }, test/storage.test.ts-835- ); test/storage.test.ts-836- throw error; test/storage.test.ts-837- } -- test/storage.test.ts-939- if (flaggedRenameAttempts <= 2) { test/storage.test.ts-940- const error = Object.assign( test/storage.test.ts-941- new Error("flagged storage busy"), test/storage.test.ts:942: { code: "EBUSY" }, test/storage.test.ts-943- ); test/storage.test.ts-944- throw error; test/storage.test.ts-945- } -- test/storage.test.ts-1063- if (String(path) === exportPath && busyFailures === 0) { test/storage.test.ts-1064- busyFailures += 1; test/storage.test.ts-1065- const error = new Error("import file busy") as NodeJS.ErrnoException; test/storage.test.ts:1066: error.code = "EAGAIN"; test/storage.test.ts-1067- throw error; test/storage.test.ts-1068- } test/storage.test.ts-1069- return originalReadFile(...(args as Parameters<typeof fs.readFile>)); -- test/storage.test.ts-1574- it("rethrows unreadable backup directory errors while listing backups", async () => { test/storage.test.ts-1575- const readdirSpy = vi.spyOn(fs, "readdir"); test/storage.test.ts-1576- const error = new Error("backup directory locked") as NodeJS.ErrnoException; test/storage.test.ts:1577: error.code = "EPERM"; test/storage.test.ts-1578- readdirSpy.mockRejectedValue(error); test/storage.test.ts-1579- test/storage.test.ts-1580- try { test/storage.test.ts:1581: await expect(listNamedBackups()).rejects.toMatchObject({ code: "EPERM" }); test/storage.test.ts-1582- } finally { test/storage.test.ts-1583- readdirSpy.mockRestore(); test/storage.test.ts-1584- } -- test/storage.test.ts-1590- .mockReturnValue("linux"); test/storage.test.ts-1591- const readdirSpy = vi.spyOn(fs, "readdir"); test/storage.test.ts-1592- const error = new Error("backup directory locked") as NodeJS.ErrnoException; test/storage.test.ts:1593: error.code = "EPERM"; test/storage.test.ts-1594- readdirSpy.mockRejectedValue(error); test/storage.test.ts-1595- test/storage.test.ts-1596- try { test/storage.test.ts:1597: await expect(listNamedBackups()).rejects.toMatchObject({ code: "EPERM" }); test/storage.test.ts-1598- expect(readdirSpy).toHaveBeenCalledTimes(1); test/storage.test.ts-1599- } finally { test/storage.test.ts-1600- readdirSpy.mockRestore(); -- test/storage.test.ts-1602- } test/storage.test.ts-1603- }); test/storage.test.ts-1604- test/storage.test.ts:1605: it("rethrows EBUSY backup directory errors after one attempt on non-Windows platforms", async () => { test/storage.test.ts-1606- const platformSpy = vi test/storage.test.ts-1607- .spyOn(process, "platform", "get") test/storage.test.ts-1608- .mockReturnValue("linux"); test/storage.test.ts-1609- const readdirSpy = vi.spyOn(fs, "readdir"); test/storage.test.ts-1610- const error = new Error("backup directory busy") as NodeJS.ErrnoException; test/storage.test.ts:1611: error.code = "EBUSY"; test/storage.test.ts-1612- readdirSpy.mockRejectedValue(error); test/storage.test.ts-1613- test/storage.test.ts-1614- try { test/storage.test.ts:1615: await expect(listNamedBackups()).rejects.toMatchObject({ code: "EBUSY" }); test/storage.test.ts-1616- expect(readdirSpy).toHaveBeenCalledTimes(1); test/storage.test.ts-1617- } finally { test/storage.test.ts-1618- readdirSpy.mockRestore(); -- test/storage.test.ts-1623- it("rethrows unreadable backup directory errors while restoring backups", async () => { test/storage.test.ts-1624- const readdirSpy = vi.spyOn(fs, "readdir"); test/storage.test.ts-1625- const error = new Error("backup directory locked") as NodeJS.ErrnoException; test/storage.test.ts:1626: error.code = "EPERM"; test/storage.test.ts-1627- readdirSpy.mockRejectedValue(error); test/storage.test.ts-1628- test/storage.test.ts-1629- try { test/storage.test.ts-1630- await expect(restoreNamedBackup("Manual Backup")).rejects.toMatchObject({ test/storage.test.ts:1631: code: "EPERM", test/storage.test.ts-1632- }); test/storage.test.ts-1633- } finally { test/storage.test.ts-1634- readdirSpy.mockRestore(); -- test/storage.test.ts-1661- const error = new Error( test/storage.test.ts-1662- "backup directory busy", test/storage.test.ts-1663- ) as NodeJS.ErrnoException; test/storage.test.ts:1664: error.code = "EBUSY"; test/storage.test.ts-1665- throw error; test/storage.test.ts-1666- } test/storage.test.ts-1667- return originalReaddir(...(args as Parameters<typeof fs.readdir>)); -- test/storage.test.ts-1707- const error = new Error( test/storage.test.ts-1708- "backup directory busy", test/storage.test.ts-1709- ) as NodeJS.ErrnoException; test/storage.test.ts:1710: error.code = "EAGAIN"; test/storage.test.ts-1711- throw error; test/storage.test.ts-1712- } test/storage.test.ts-1713- return originalReaddir(...(args as Parameters<typeof fs.readdir>)); -- test/storage.test.ts-1780- if (String(path) === backup.path && busyFailures === 0) { test/storage.test.ts-1781- busyFailures += 1; test/storage.test.ts-1782- const error = new Error("backup file busy") as NodeJS.ErrnoException; test/storage.test.ts:1783: error.code = "EBUSY"; test/storage.test.ts-1784- throw error; test/storage.test.ts-1785- } test/storage.test.ts-1786- return originalReadFile(...(args as Parameters<typeof fs.readFile>)); -- test/storage.test.ts-1820- if (String(path) === backup.path && busyFailures === 0) { test/storage.test.ts-1821- busyFailures += 1; test/storage.test.ts-1822- const error = new Error("backup stat busy") as NodeJS.ErrnoException; test/storage.test.ts:1823: error.code = "EAGAIN"; test/storage.test.ts-1824- throw error; test/storage.test.ts-1825- } test/storage.test.ts-1826- return originalStat(...(args as Parameters<typeof fs.stat>)); -- test/storage.test.ts-2057- const testPath = "/home/user/.codex/accounts.json"; test/storage.test.ts-2058- test/storage.test.ts-2059- it("should return permission hint for EACCES on Windows", () => { test/storage.test.ts:2060: const originalPlatform = process.platform; test/storage.test.ts:2061: Object.defineProperty(process, "platform", { value: "win32" }); test/storage.test.ts-2062- test/storage.test.ts-2063- const err = { code: "EACCES" } as NodeJS.ErrnoException; test/storage.test.ts-2064- const hint = formatStorageErrorHint(err, testPath); -- test/storage.test.ts-2070- }); test/storage.test.ts-2071- test/storage.test.ts-2072- it("should return chmod hint for EACCES on Unix", () => { test/storage.test.ts:2073: const originalPlatform = process.platform; test/storage.test.ts-2074- Object.defineProperty(process, "platform", { value: "darwin" }); test/storage.test.ts-2075- test/storage.test.ts-2076- const err = { code: "EACCES" } as NodeJS.ErrnoException; -- test/storage.test.ts-2082- Object.defineProperty(process, "platform", { value: originalPlatform }); test/storage.test.ts-2083- }); test/storage.test.ts-2084- test/storage.test.ts:2085: it("should return permission hint for EPERM", () => { test/storage.test.ts:2086: const err = { code: "EPERM" } as NodeJS.ErrnoException; test/storage.test.ts-2087- const hint = formatStorageErrorHint(err, testPath); test/storage.test.ts-2088- test/storage.test.ts-2089- expect(hint).toContain("Permission denied"); test/storage.test.ts-2090- expect(hint).toContain(testPath); test/storage.test.ts-2091- }); test/storage.test.ts-2092- test/storage.test.ts:2093: it("should return file locked hint for EBUSY", () => { test/storage.test.ts:2094: const err = { code: "EBUSY" } as NodeJS.ErrnoException; test/storage.test.ts-2095- const hint = formatStorageErrorHint(err, testPath); test/storage.test.ts-2096- test/storage.test.ts-2097- expect(hint).toContain("locked"); -- test/storage.test.ts-2725- await expect(clearAccounts()).resolves.not.toThrow(); test/storage.test.ts-2726- }); test/storage.test.ts-2727- test/storage.test.ts:2728: it.each(["EPERM", "EBUSY", "EAGAIN"] as const)( test/storage.test.ts-2729- "retries transient %s when clearing saved account artifacts", test/storage.test.ts-2730- async (code) => { test/storage.test.ts-2731- await fs.writeFile(testStoragePath, "{}"); -- test/storage.test.ts-2780- await fs.rm(testWorkDir, { recursive: true, force: true }); test/storage.test.ts-2781- }); test/storage.test.ts-2782- test/storage.test.ts:2783: it.each(["EPERM", "EBUSY"] as const)( test/storage.test.ts-2784- "retries transient %s when clearing flagged account storage", test/storage.test.ts-2785- async (code) => { test/storage.test.ts-2786- const flaggedPath = getFlaggedAccountsPath(); -- test/storage.test.ts-2815- }, test/storage.test.ts-2816- ); test/storage.test.ts-2817- test/storage.test.ts:2818: it.each(["EPERM", "EBUSY"] as const)( test/storage.test.ts-2819- "returns false when clearing flagged account storage exhausts retryable %s failures", test/storage.test.ts-2820- async (code) => { test/storage.test.ts-2821- const flaggedPath = getFlaggedAccountsPath(); -- test/storage.test.ts-3010- await expect(clearAccounts()).resolves.not.toThrow(); test/storage.test.ts-3011- }); test/storage.test.ts-3012- test/storage.test.ts:3013: it.each(["EPERM", "EBUSY"] as const)( test/storage.test.ts-3014- "returns false when clearing saved accounts exhausts retryable %s failures", test/storage.test.ts-3015- async (code) => { test/storage.test.ts-3016- await fs.writeFile(testStoragePath, "{}"); -- test/storage.test.ts-3691- }); test/storage.test.ts-3692- }); test/storage.test.ts-3693- test/storage.test.ts:3694: describe("saveAccounts EPERM/EBUSY retry logic", () => { test/storage.test.ts-3695- const testWorkDir = join( test/storage.test.ts-3696- tmpdir(), test/storage.test.ts-3697- "codex-retry-" + Math.random().toString(36).slice(2), -- test/storage.test.ts-3711- await fs.rm(testWorkDir, { recursive: true, force: true }); test/storage.test.ts-3712- }); test/storage.test.ts-3713- test/storage.test.ts:3714: it("retries on EPERM and succeeds on second attempt", async () => { test/storage.test.ts-3715- const now = Date.now(); test/storage.test.ts-3716- const storage = { test/storage.test.ts-3717- version: 3 as const, -- test/storage.test.ts-3726- .mockImplementation(async (oldPath, newPath) => { test/storage.test.ts-3727- attemptCount++; test/storage.test.ts-3728- if (attemptCount === 1) { test/storage.test.ts:3729: const err = new Error("EPERM error") as NodeJS.ErrnoException; test/storage.test.ts:3730: err.code = "EPERM"; test/storage.test.ts-3731- throw err; test/storage.test.ts-3732- } test/storage.test.ts-3733- return originalRename(oldPath as string, newPath as string); -- test/storage.test.ts-3740- renameSpy.mockRestore(); test/storage.test.ts-3741- }); test/storage.test.ts-3742- test/storage.test.ts:3743: it("retries on EBUSY and succeeds on third attempt", async () => { test/storage.test.ts-3744- const now = Date.now(); test/storage.test.ts-3745- const storage = { test/storage.test.ts-3746- version: 3 as const, -- test/storage.test.ts-3755- .mockImplementation(async (oldPath, newPath) => { test/storage.test.ts-3756- attemptCount++; test/storage.test.ts-3757- if (attemptCount <= 2) { test/storage.test.ts:3758: const err = new Error("EBUSY error") as NodeJS.ErrnoException; test/storage.test.ts:3759: err.code = "EBUSY"; test/storage.test.ts-3760- throw err; test/storage.test.ts-3761- } test/storage.test.ts-3762- return originalRename(oldPath as string, newPath as string); -- test/storage.test.ts-3769- renameSpy.mockRestore(); test/storage.test.ts-3770- }); test/storage.test.ts-3771- test/storage.test.ts:3772: it("retries on EAGAIN and cleans up the WAL after rename succeeds", async () => { test/storage.test.ts-3773- const now = Date.now(); test/storage.test.ts-3774- const storage = { test/storage.test.ts-3775- version: 3 as const, -- test/storage.test.ts-3785- .mockImplementation(async (oldPath, newPath) => { test/storage.test.ts-3786- attemptCount++; test/storage.test.ts-3787- if (attemptCount === 1) { test/storage.test.ts:3788: const err = new Error("EAGAIN error") as NodeJS.ErrnoException; test/storage.test.ts:3789: err.code = "EAGAIN"; test/storage.test.ts-3790- throw err; test/storage.test.ts-3791- } test/storage.test.ts-3792- return originalRename(oldPath as string, newPath as string); -- test/storage.test.ts-3800- renameSpy.mockRestore(); test/storage.test.ts-3801- }); test/storage.test.ts-3802- test/storage.test.ts:3803: it("throws after 5 failed EPERM retries", async () => { test/storage.test.ts-3804- const now = Date.now(); test/storage.test.ts-3805- const storage = { test/storage.test.ts-3806- version: 3 as const, -- test/storage.test.ts-3811- let attemptCount = 0; test/storage.test.ts-3812- const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async () => { test/storage.test.ts-3813- attemptCount++; test/storage.test.ts:3814: const err = new Error("EPERM error") as NodeJS.ErrnoException; test/storage.test.ts:3815: err.code = "EPERM"; test/storage.test.ts-3816- throw err; test/storage.test.ts-3817- }); test/storage.test.ts-3818- -- test/storage.test.ts-3824- renameSpy.mockRestore(); test/storage.test.ts-3825- }); test/storage.test.ts-3826- test/storage.test.ts:3827: it("throws immediately on non-EPERM/EBUSY errors", async () => { test/storage.test.ts-3828- const now = Date.now(); test/storage.test.ts-3829- const storage = { test/storage.test.ts-3830- version: 3 as const, -- test/storage.test.ts-3870- statSpy.mockRestore(); test/storage.test.ts-3871- }); test/storage.test.ts-3872- test/storage.test.ts:3873: it("retries backup copyFile on transient EBUSY and succeeds", async () => { test/storage.test.ts-3874- const now = Date.now(); test/storage.test.ts-3875- const storage = { test/storage.test.ts-3876- version: 3 as const, -- test/storage.test.ts-3888- .mockImplementation(async (src, dest) => { test/storage.test.ts-3889- copyAttempts += 1; test/storage.test.ts-3890- if (copyAttempts === 1) { test/storage.test.ts:3891: const err = new Error("EBUSY copy") as NodeJS.ErrnoException; test/storage.test.ts:3892: err.code = "EBUSY"; test/storage.test.ts-3893- throw err; test/storage.test.ts-3894- } test/storage.test.ts-3895- return originalCopy(src as string, dest as string); -- test/storage.test.ts-3908- } test/storage.test.ts-3909- }); test/storage.test.ts-3910- test/storage.test.ts:3911: it("retries backup copyFile on transient EPERM and succeeds", async () => { test/storage.test.ts-3912- const now = Date.now(); test/storage.test.ts-3913- const storage = { test/storage.test.ts-3914- version: 3 as const, -- test/storage.test.ts-3926- .mockImplementation(async (src, dest) => { test/storage.test.ts-3927- copyAttempts += 1; test/storage.test.ts-3928- if (copyAttempts === 1) { test/storage.test.ts:3929: const err = new Error("EPERM copy") as NodeJS.ErrnoException; test/storage.test.ts:3930: err.code = "EPERM"; test/storage.test.ts-3931- throw err; test/storage.test.ts-3932- } test/storage.test.ts-3933- return originalCopy(src as string, dest as string); -- test/storage.test.ts-3946- } test/storage.test.ts-3947- }); test/storage.test.ts-3948- test/storage.test.ts:3949: it("retries staged backup rename on transient EBUSY and succeeds", async () => { test/storage.test.ts-3950- const now = Date.now(); test/storage.test.ts-3951- const storagePath = getStoragePath(); test/storage.test.ts-3952- const storage = { -- test/storage.test.ts-3968- stagedRenameAttempts += 1; test/storage.test.ts-3969- if (stagedRenameAttempts === 1) { test/storage.test.ts-3970- const err = new Error( test/storage.test.ts:3971: "EBUSY staged rename", test/storage.test.ts-3972- ) as NodeJS.ErrnoException; test/storage.test.ts:3973: err.code = "EBUSY"; test/storage.test.ts-3974- throw err; test/storage.test.ts-3975- } test/storage.test.ts-3976- } -- test/storage.test.ts-4299- await expect(clearQuotaCache()).resolves.toBe(true); test/storage.test.ts-4300- }); test/storage.test.ts-4301- test/storage.test.ts:4302: it.each(["EPERM", "EBUSY"] as const)( test/storage.test.ts-4303- "retries transient %s when clearing the quota cache", test/storage.test.ts-4304- async (code) => { test/storage.test.ts-4305- const quotaPath = getQuotaCachePath(); -- test/storage.test.ts-4328- }, test/storage.test.ts-4329- ); test/storage.test.ts-4330- test/storage.test.ts:4331: it.each(["EPERM", "EBUSY"] as const)( test/storage.test.ts-4332- "returns false when quota-cache clear exhausts retryable %s failures", test/storage.test.ts-4333- async (code) => { test/storage.test.ts-4334- const quotaPath = getQuotaCachePath(); -- test/settings-hub-utils.test.ts-235- attempts += 1; test/settings-hub-utils.test.ts-236- if (attempts < 3) { test/settings-hub-utils.test.ts-237- const error = new Error("busy") as NodeJS.ErrnoException; test/settings-hub-utils.test.ts:238: error.code = attempts === 1 ? "EBUSY" : "EPERM"; test/settings-hub-utils.test.ts-239- throw error; test/settings-hub-utils.test.ts-240- } test/settings-hub-utils.test.ts-241- return "ok"; -- test/settings-hub-utils.test.ts-244- expect(attempts).toBe(3); test/settings-hub-utils.test.ts-245- }); test/settings-hub-utils.test.ts-246- test/settings-hub-utils.test.ts:247: it("retries queued writes for EAGAIN filesystem errors", async () => { test/settings-hub-utils.test.ts-248- const api = await loadSettingsHubTestApi(); test/settings-hub-utils.test.ts-249- let attempts = 0; test/settings-hub-utils.test.ts-250- const result = await api.withQueuedRetry( -- test/settings-hub-utils.test.ts-253- attempts += 1; test/settings-hub-utils.test.ts-254- if (attempts < 3) { test/settings-hub-utils.test.ts-255- const error = new Error("busy") as NodeJS.ErrnoException; test/settings-hub-utils.test.ts:256: error.code = "EAGAIN"; test/settings-hub-utils.test.ts-257- throw error; test/settings-hub-utils.test.ts-258- } test/settings-hub-utils.test.ts-259- return "ok"; -- test/settings-hub-utils.test.ts-662- })); test/settings-hub-utils.test.ts-663- const nodeFs = await import("node:fs"); test/settings-hub-utils.test.ts-664- const busyError = new Error("busy") as NodeJS.ErrnoException; test/settings-hub-utils.test.ts:665: busyError.code = "EBUSY"; test/settings-hub-utils.test.ts-666- const readSpy = vi test/settings-hub-utils.test.ts-667- .spyOn(nodeFs.promises, "readFile") test/settings-hub-utils.test.ts-668- .mockRejectedValueOnce(busyError) -- test/rotation-integration.test.ts-15-let testStoragePath: string; test/rotation-integration.test.ts-16- test/rotation-integration.test.ts-17-async function removeWithRetry(path: string, options: { recursive?: boolean; force?: boolean }): Promise<void> { test/rotation-integration.test.ts:18: const retryableCodes = new Set(["ENOTEMPTY", "EPERM", "EBUSY"]); test/rotation-integration.test.ts-19- const maxAttempts = 6; test/rotation-integration.test.ts-20- test/rotation-integration.test.ts-21- for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { -- test/release-main-prs-regression.test.ts-192- const originalUnlink = fs.unlink.bind(fs); test/release-main-prs-regression.test.ts-193- const unlinkSpy = vi.spyOn(fs, "unlink").mockImplementation(async (targetPath) => { test/release-main-prs-regression.test.ts-194- if (targetPath === flaggedPath) { test/release-main-prs-regression.test.ts:195: const error = new Error("EPERM primary delete") as NodeJS.ErrnoException; test/release-main-prs-regression.test.ts:196: error.code = "EPERM"; test/release-main-prs-regression.test.ts-197- throw error; test/release-main-prs-regression.test.ts-198- } test/release-main-prs-regression.test.ts-199- return originalUnlink(targetPath); -- test/repo-hygiene.test.ts-7-import { execFileSync, spawn, spawnSync } from "node:child_process"; test/repo-hygiene.test.ts-8- test/repo-hygiene.test.ts-9-async function removeWithRetry(targetPath: string, options: { recursive?: boolean; force?: boolean }): Promise<void> { test/repo-hygiene.test.ts:10: const retryableCodes = new Set(["ENOTEMPTY", "EPERM", "EBUSY"]); test/repo-hygiene.test.ts-11- const maxAttempts = 6; test/repo-hygiene.test.ts-12- test/repo-hygiene.test.ts-13- for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { -- test/refresh-queue.test.ts-887- vi.mocked(authModule.refreshAccessToken).mockResolvedValue(mockResult); test/refresh-queue.test.ts-888- test/refresh-queue.test.ts-889- const leaseCoordinator = { test/refresh-queue.test.ts:890: acquire: vi.fn().mockRejectedValue(new Error("EBUSY lease dir")), test/refresh-queue.test.ts-891- } as unknown as RefreshLeaseCoordinator; test/refresh-queue.test.ts-892- test/refresh-queue.test.ts-893- const queue = new RefreshQueue(30_000, leaseCoordinator); -- test/runtime-paths.test.ts-117- }); test/runtime-paths.test.ts-118- test/runtime-paths.test.ts-119- it("deduplicates Windows-style fallback paths case-insensitively", async () => { test/runtime-paths.test.ts:120: const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); test/runtime-paths.test.ts-121- try { test/runtime-paths.test.ts-122- homedir.mockReturnValue("C:\\Users\\Neil"); test/runtime-paths.test.ts-123- process.env.CODEX_HOME = "C:\\USERS\\NEIL\\.codex"; -- test/runtime-paths.test.ts-136- }); test/runtime-paths.test.ts-137- test/runtime-paths.test.ts-138- it("treats default Windows CODEX_HOME with a trailing separator as the default root", async () => { test/runtime-paths.test.ts:139: const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); test/runtime-paths.test.ts-140- try { test/runtime-paths.test.ts-141- homedir.mockReturnValue("C:\\Users\\Neil"); test/runtime-paths.test.ts-142- process.env.CODEX_HOME = "C:\\Users\\Neil\\.codex\\"; -- test/runtime-paths.test.ts-169- }); test/runtime-paths.test.ts-170- test/runtime-paths.test.ts-171- it("prefers USERPROFILE over os.homedir on Windows when CODEX_HOME is unset", async () => { test/runtime-paths.test.ts:172: const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); test/runtime-paths.test.ts-173- try { test/runtime-paths.test.ts-174- homedir.mockReturnValue("C:\\Windows\\System32\\config\\systemprofile"); test/runtime-paths.test.ts-175- process.env.USERPROFILE = "C:\\Users\\Alice"; -- test/runtime-paths.test.ts-182- }); test/runtime-paths.test.ts-183- test/runtime-paths.test.ts-184- it("falls back to HOME when USERPROFILE is missing on Windows", async () => { test/runtime-paths.test.ts:185: const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); test/runtime-paths.test.ts-186- try { test/runtime-paths.test.ts-187- homedir.mockReturnValue("C:\\Windows\\System32\\config\\systemprofile"); test/runtime-paths.test.ts-188- process.env.HOME = "D:\\Users\\Bob"; -- test/runtime-paths.test.ts-194- }); test/runtime-paths.test.ts-195- test/runtime-paths.test.ts-196- it("falls back to HOMEDRIVE and HOMEPATH when USERPROFILE and HOME are missing on Windows", async () => { test/runtime-paths.test.ts:197: const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); test/runtime-paths.test.ts-198- try { test/runtime-paths.test.ts-199- homedir.mockReturnValue("C:\\Windows\\System32\\config\\systemprofile"); test/runtime-paths.test.ts-200- process.env.HOMEDRIVE = "E:"; -- test/runtime-paths.test.ts-207- }); test/runtime-paths.test.ts-208- test/runtime-paths.test.ts-209- it("normalizes HOMEPATH without a leading slash on Windows", async () => { test/runtime-paths.test.ts:210: const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); test/runtime-paths.test.ts-211- try { test/runtime-paths.test.ts-212- homedir.mockReturnValue("C:\\Windows\\System32\\config\\systemprofile"); test/runtime-paths.test.ts-213- process.env.HOMEDRIVE = "E:"; -- test/refresh-lease.test.ts-121- if (String(path).endsWith(".lock") && busyCount < 2) { test/refresh-lease.test.ts-122- busyCount += 1; test/refresh-lease.test.ts-123- const error = new Error("busy") as NodeJS.ErrnoException; test/refresh-lease.test.ts:124: error.code = "EBUSY"; test/refresh-lease.test.ts-125- throw error; test/refresh-lease.test.ts-126- } test/refresh-lease.test.ts-127- return originalUnlink(path); -- test/refresh-lease.test.ts-173- unlink: vi.fn(async (path: Parameters<typeof fsPromises.unlink>[0]) => { test/refresh-lease.test.ts-174- if (String(path).endsWith(".lock")) { test/refresh-lease.test.ts-175- const error = new Error("busy") as NodeJS.ErrnoException; test/refresh-lease.test.ts:176: error.code = "EBUSY"; test/refresh-lease.test.ts-177- throw error; test/refresh-lease.test.ts-178- } test/refresh-lease.test.ts-179- return originalUnlink(path); -- test/quota-cache.test.ts-105- } test/quota-cache.test.ts-106- }); test/quota-cache.test.ts-107- test/quota-cache.test.ts:108: it.each(["EBUSY", "EPERM"] as const)( test/quota-cache.test.ts-109- "retries transient %s while clearing cache", test/quota-cache.test.ts-110- async (code) => { test/quota-cache.test.ts-111- const { clearQuotaCache, getQuotaCachePath, saveQuotaCache } = -- test/quota-cache.test.ts-139- }, test/quota-cache.test.ts-140- ); test/quota-cache.test.ts-141- test/quota-cache.test.ts:142: it.each(["EBUSY", "EPERM"] as const)( test/quota-cache.test.ts-143- "returns false when clearQuotaCache exhausts %s retries", test/quota-cache.test.ts-144- async (code) => { test/quota-cache.test.ts-145- vi.resetModules(); -- test/quota-cache.test.ts-183- }, test/quota-cache.test.ts-184- ); test/quota-cache.test.ts-185- test/quota-cache.test.ts:186: it("retries transient EBUSY while loading cache", async () => { test/quota-cache.test.ts-187- const { loadQuotaCache, getQuotaCachePath } = test/quota-cache.test.ts-188- await import("../lib/quota-cache.js"); test/quota-cache.test.ts-189- await fs.writeFile( -- test/quota-cache.test.ts-212- attempts += 1; test/quota-cache.test.ts-213- if (attempts === 1) { test/quota-cache.test.ts-214- const error = new Error("busy") as NodeJS.ErrnoException; test/quota-cache.test.ts:215: error.code = "EBUSY"; test/quota-cache.test.ts-216- throw error; test/quota-cache.test.ts-217- } test/quota-cache.test.ts-218- } -- test/quota-cache.test.ts-228- } test/quota-cache.test.ts-229- }); test/quota-cache.test.ts-230- test/quota-cache.test.ts:231: it.each(["EBUSY", "EPERM"] as const)( test/quota-cache.test.ts-232- "retries atomic rename on transient %s errors", test/quota-cache.test.ts-233- async (code) => { test/quota-cache.test.ts-234- const { saveQuotaCache, loadQuotaCache } = -- test/quota-cache.test.ts-276- const unlinkSpy = vi.spyOn(fs, "unlink"); test/quota-cache.test.ts-277- renameSpy.mockImplementation(async () => { test/quota-cache.test.ts-278- const error = new Error("locked") as NodeJS.ErrnoException; test/quota-cache.test.ts:279: error.code = "EPERM"; test/quota-cache.test.ts-280- throw error; test/quota-cache.test.ts-281- }); test/quota-cache.test.ts-282- -- test/recovery-constants.test.ts-3-import { homedir } from "node:os"; test/recovery-constants.test.ts-4- test/recovery-constants.test.ts-5-describe("recovery/constants.ts", () => { test/recovery-constants.test.ts:6: const originalPlatform = process.platform; test/recovery-constants.test.ts-7- const originalAppData = process.env.APPDATA; test/recovery-constants.test.ts-8- const originalXdgDataHome = process.env.XDG_DATA_HOME; test/recovery-constants.test.ts-9- -- test/recovery-constants.test.ts-42- test/recovery-constants.test.ts-43- describe("getXdgData on Windows", () => { test/recovery-constants.test.ts-44- it("should use APPDATA when set", async () => { test/recovery-constants.test.ts:45: Object.defineProperty(process, "platform", { value: "win32" }); test/recovery-constants.test.ts-46- process.env.APPDATA = "C:\\Users\\Test\\AppData\\Roaming"; test/recovery-constants.test.ts-47- test/recovery-constants.test.ts-48- const { CODEX_STORAGE } = await import("../lib/recovery/constants.js"); -- test/recovery-constants.test.ts-51- }); test/recovery-constants.test.ts-52- test/recovery-constants.test.ts-53- it("should fallback to AppData/Roaming when APPDATA is not set", async () => { test/recovery-constants.test.ts:54: Object.defineProperty(process, "platform", { value: "win32" }); test/recovery-constants.test.ts-55- delete process.env.APPDATA; test/recovery-constants.test.ts-56- test/recovery-constants.test.ts-57- const { CODEX_STORAGE } = await import("../lib/recovery/constants.js"); -- test/recovery.test.ts-54- } catch (error) { test/recovery.test.ts-55- const code = (error as NodeJS.ErrnoException).code; test/recovery.test.ts-56- if ( test/recovery.test.ts:57: (code !== "EBUSY" && code !== "EPERM" && code !== "ENOTEMPTY") || test/recovery.test.ts-58- attempt === 4 test/recovery.test.ts-59- ) { test/recovery.test.ts-60- throw error; -- test/recovery.test.ts-377- expect(result.assessments[0]?.imported).toBe(1); test/recovery.test.ts-378- }); test/recovery.test.ts-379- test/recovery.test.ts:380: it("keeps actionable backups when fast-path scan hits EBUSY", async () => { test/recovery.test.ts-381- const storage = await import("../lib/storage.js"); test/recovery.test.ts-382- const emptyStorage = { test/recovery.test.ts-383- version: 3, -- test/recovery.test.ts-427- const [path] = args; test/recovery.test.ts-428- if (path === lockedBackup?.path) { test/recovery.test.ts-429- const error = new Error("resource busy") as NodeJS.ErrnoException; test/recovery.test.ts:430: error.code = "EBUSY"; test/recovery.test.ts-431- throw error; test/recovery.test.ts-432- } test/recovery.test.ts-433- return originalReadFile(...args); -- test/recovery.test.ts-452- } test/recovery.test.ts-453- }); test/recovery.test.ts-454- test/recovery.test.ts:455: it("keeps actionable backups when fast-path metadata stat hits EBUSY", async () => { test/recovery.test.ts-456- const storage = await import("../lib/storage.js"); test/recovery.test.ts-457- const emptyStorage = { test/recovery.test.ts-458- version: 3, -- test/recovery.test.ts-504- normalizedPath.endsWith("/locked-backup.json") test/recovery.test.ts-505- ) { test/recovery.test.ts-506- const error = new Error("resource busy") as NodeJS.ErrnoException; test/recovery.test.ts:507: error.code = "EBUSY"; test/recovery.test.ts-508- throw error; test/recovery.test.ts-509- } test/recovery.test.ts-510- return originalStat(...args); -- test/recovery.test.ts-599- } test/recovery.test.ts-600- }); test/recovery.test.ts-601- test/recovery.test.ts:602: it("keeps injected-assessor backups when metadata stat hits EBUSY", async () => { test/recovery.test.ts-603- const storage = await import("../lib/storage.js"); test/recovery.test.ts-604- const emptyStorage = { test/recovery.test.ts-605- version: 3, -- test/recovery.test.ts-655- normalizedPath.endsWith("/locked-backup.json") test/recovery.test.ts-656- ) { test/recovery.test.ts-657- const error = new Error("resource busy") as NodeJS.ErrnoException; test/recovery.test.ts:658: error.code = "EBUSY"; test/recovery.test.ts-659- throw error; test/recovery.test.ts-660- } test/recovery.test.ts-661- return originalStat(...args); -- test/recovery.test.ts-694- } test/recovery.test.ts-695- }); test/recovery.test.ts-696- test/recovery.test.ts:697: it("keeps actionable backups when default assessment hits EBUSY", async () => { test/recovery.test.ts-698- const storage = await import("../lib/storage.js"); test/recovery.test.ts-699- const emptyStorage = { test/recovery.test.ts-700- version: 3, -- test/recovery.test.ts-743- const [path] = args; test/recovery.test.ts-744- if (path === lockedBackup?.path) { test/recovery.test.ts-745- const error = new Error("resource busy") as NodeJS.ErrnoException; test/recovery.test.ts:746: error.code = "EBUSY"; test/recovery.test.ts-747- throw error; test/recovery.test.ts-748- } test/recovery.test.ts-749- return originalReadFile(...args); -- test/paths.test.ts-117- process.env.HOME = "C:\\Users\\test"; test/paths.test.ts-118- process.env.USERPROFILE = "C:\\Users\\test"; test/paths.test.ts-119- process.env.CODEX_HOME = "C:\\Users\\test\\.codex"; test/paths.test.ts:120: const primary = path.win32.join("C:\\Users\\test\\.codex", "multi-auth"); test/paths.test.ts:121: const fallback = path.win32.join("C:\\Users\\test", "DevTools", "config", "codex", "multi-auth"); test/paths.test.ts:122: const normalizePath = (input: string) => path.win32.normalize(input.replace(/\//g, "\\")); test/paths.test.ts-123- test/paths.test.ts-124- mockedExistsSync.mockImplementation((candidate) => { test/paths.test.ts-125- if (typeof candidate !== "string") return false; test/paths.test.ts-126- const normalizedCandidate = normalizePath(candidate); test/paths.test.ts:127: if (normalizedCandidate === normalizePath(path.win32.join(primary, "settings.json"))) return true; test/paths.test.ts:128: if (normalizedCandidate === normalizePath(path.win32.join(primary, "openai-codex-accounts.json"))) return false; test/paths.test.ts:129: if (normalizedCandidate === normalizePath(path.win32.join(primary, "codex-accounts.json"))) return false; test/paths.test.ts:130: if (normalizedCandidate === normalizePath(path.win32.join(primary, "config.json"))) return false; test/paths.test.ts:131: if (normalizedCandidate === normalizePath(path.win32.join(primary, "dashboard-settings.json"))) return false; test/paths.test.ts:132: if (normalizedCandidate === normalizePath(path.win32.join(primary, "projects"))) return false; test/paths.test.ts:133: return normalizedCandidate === normalizePath(path.win32.join(fallback, "openai-codex-accounts.json")); test/paths.test.ts-134- }); test/paths.test.ts-135- test/paths.test.ts-136- try { -- test/paths.test.ts-207- const sharedRepoRoot = path.resolve("repo"); test/paths.test.ts-208- const sharedGitDir = path.join(sharedRepoRoot, ".git"); test/paths.test.ts-209- const normalize = (value: string) => test/paths.test.ts:210: process.platform === "win32" ? value.toLowerCase() : value; test/paths.test.ts-211- test/paths.test.ts-212- mockedExistsSync.mockImplementation((candidate) => { test/paths.test.ts-213- if (typeof candidate !== "string") return false; -- test/paths.test.ts-253- const sharedRepoRoot = path.resolve("repo"); test/paths.test.ts-254- const sharedGitDir = path.join(sharedRepoRoot, ".git"); test/paths.test.ts-255- const normalize = (value: string) => test/paths.test.ts:256: process.platform === "win32" ? value.toLowerCase() : value; test/paths.test.ts-257- test/paths.test.ts-258- mockedExistsSync.mockImplementation((candidate) => { test/paths.test.ts-259- if (typeof candidate !== "string") return false; -- test/paths.test.ts-291- }); test/paths.test.ts-292- test/paths.test.ts-293- it("supports Windows-style backslash gitdir pointers", () => { test/paths.test.ts:294: const projectRoot = path.win32.join("C:\\repo", "worktrees", "pr-8"); test/paths.test.ts:295: const gitEntry = path.win32.join(projectRoot, ".git"); test/paths.test.ts:296: const worktreeGitDir = path.win32.join("C:\\repo", ".git", "worktrees", "pr-8"); test/paths.test.ts:297: const commondirFile = path.win32.join(worktreeGitDir, "commondir"); test/paths.test.ts:298: const gitdirBackRefFile = path.win32.join(worktreeGitDir, "gitdir"); test/paths.test.ts-299- const sharedRepoRoot = "C:\\repo"; test/paths.test.ts:300: const sharedGitDir = path.win32.join(sharedRepoRoot, ".git"); test/paths.test.ts:301: const normalize = (value: string) => path.win32.normalize(value).toLowerCase(); test/paths.test.ts-302- test/paths.test.ts-303- mockedExistsSync.mockImplementation((candidate) => { test/paths.test.ts-304- if (typeof candidate !== "string") return false; -- test/paths.test.ts-325- return "..\\..\\\n"; test/paths.test.ts-326- } test/paths.test.ts-327- if (normalizedCandidate === normalize(gitdirBackRefFile)) { test/paths.test.ts:328: return `${path.win32.join(projectRoot, ".git")}\n`; test/paths.test.ts-329- } test/paths.test.ts-330- throw new Error(`Unexpected read path: ${String(candidate)}`); test/paths.test.ts-331- }); -- test/paths.test.ts-337- test/paths.test.ts-338- it("supports windows UNC gitdir pointers", () => { test/paths.test.ts-339- const sharedRepoRoot = "\\\\server\\share\\repo"; test/paths.test.ts:340: const projectRoot = path.win32.join(sharedRepoRoot, "worktrees", "pr-unc"); test/paths.test.ts:341: const gitEntry = path.win32.join(projectRoot, ".git"); test/paths.test.ts:342: const worktreeGitDir = path.win32.join(sharedRepoRoot, ".git", "worktrees", "pr-unc"); test/paths.test.ts:343: const commondirFile = path.win32.join(worktreeGitDir, "commondir"); test/paths.test.ts:344: const gitdirBackRefFile = path.win32.join(worktreeGitDir, "gitdir"); test/paths.test.ts:345: const sharedGitDir = path.win32.join(sharedRepoRoot, ".git"); test/paths.test.ts:346: const normalize = (value: string) => path.win32.normalize(value).toLowerCase(); test/paths.test.ts-347- test/paths.test.ts-348- mockedExistsSync.mockImplementation((candidate) => { test/paths.test.ts-349- if (typeof candidate !== "string") return false; -- test/paths.test.ts-371- return "..\\..\\\n"; test/paths.test.ts-372- } test/paths.test.ts-373- if (normalizedCandidate === normalize(gitdirBackRefFile)) { test/paths.test.ts:374: return `${path.win32.join(projectRoot, ".git")}\n`; test/paths.test ... [stdout truncated: output exceeded 50000 characters (92678 total). Please modify your command to reduce output size, for example by: - Using grep/rg to filter results - Using head/tail to limit lines - Using --max-count flag with grep/rg - Processing output with awk/sed to extract only needed information]

add windows-platform test coverage for EPERM/EBUSY retries in retryTransientFilesystemOperation.

platform-specific tests exist for Linux behavior at test/storage.test.ts:1590–1618 (EPERM/EBUSY rethrow without retry), but windows-specific coverage is missing. lib/storage.ts:1785 narrows EPERM/EBUSY retries to win32 only—this is a meaningful constraint shift from earlier retry logic at lib/storage.ts:290, lib/storage.ts:469, and lib/storage.ts:493 which retried all three codes universally. add explicit tests that mock process.platform = "win32" and verify EPERM/EBUSY are actually retried by the new function, alongside the existing Linux rethrow assertions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1779 - 1806, Add Windows-specific tests that mock process.platform = "win32" and assert that retryTransientFilesystemOperation actually retries on EPERM and EBUSY: for each code, create a fake operation that throws a NodeJS.ErrnoException with .code set to "EPERM" or "EBUSY" for the first N attempts then resolves, call retryTransientFilesystemOperation and assert it eventually returns and that the fake was called multiple times; restore process.platform after each test. Use the same pattern to keep the Linux tests (process.platform !== "win32") asserting EPERM/EBUSY rethrow immediately, and reference the functions isRetryableFilesystemErrorCode and retryTransientFilesystemOperation when locating the code under test.

lib/storage.ts

coderabbitai · 2026-03-16T16:14:10Z

test/recovery.test.ts

+	it("keeps actionable backups when fast-path scan hits EBUSY", async () => {
+		const storage = await import("../lib/storage.js");
+		const emptyStorage = {
+			version: 3,
+			activeIndex: 0,
+			activeIndexByFamily: { codex: 0 },
+			accounts: [],
+		};
+		await storage.saveAccounts({
+			version: 3,
+			activeIndex: 0,
+			activeIndexByFamily: { codex: 0 },
+			accounts: [
+				{
+					email: "locked@example.com",
+					refreshToken: "locked-token",
+					addedAt: 1,
+					lastUsed: 1,
+				},
+			],
+		});
+		await storage.createNamedBackup("locked-backup");
+		await storage.saveAccounts({
+			version: 3,
+			activeIndex: 0,
+			activeIndexByFamily: { codex: 0 },
+			accounts: [
+				{
+					email: "valid@example.com",
+					refreshToken: "valid-token",
+					addedAt: 2,
+					lastUsed: 2,
+				},
+			],
+		});
+		await storage.createNamedBackup("valid-backup");
+		await storage.saveAccounts(emptyStorage);
+
+		const backups = await storage.listNamedBackups();
+		const lockedBackup = backups.find((backup) => backup.name === "locked-backup");
+		const validBackup = backups.find((backup) => backup.name === "valid-backup");
+		expect(lockedBackup).toBeDefined();
+		expect(validBackup).toBeDefined();
+
+		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,
+		);
+
+		try {
+			const result = await storage.getActionableNamedBackupRestores({
+				currentStorage: emptyStorage,
+			});
+
+			expect(result.totalBackups).toBe(2);
+			expect(result.assessments.map((item) => item.backup.name)).toEqual([
+				"valid-backup",
+			]);
+			expect(result.assessments[0]?.imported).toBe(1);
+			expect(readFileSpy.mock.calls.map(([path]) => path)).toEqual(
+				expect.arrayContaining([lockedBackup?.path, validBackup?.path]),
+			);
+		} finally {
+			readFileSpy.mockRestore();
+		}
+	});


⚠️ Potential issue | 🟠 Major

gate ebusy fast-path recovery tests to explicit platform behavior

these tests assert EBUSY handling outcomes but do not pin process.platform, so expectations can drift by runtime environment. make the platform explicit for each intended behavior to keep test/recovery.test.ts:380-453, test/recovery.test.ts:455-531, and test/recovery.test.ts:697-779 deterministic.

As per coding guidelines, test/**: tests must stay deterministic and use vitest, and demand regression cases that reproduce windows filesystem behavior.

Also applies to: 455-531, 697-779

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@test/recovery.test.ts` around lines 380 - 453, The test "keeps actionable backups when fast-path scan hits EBUSY" relies on platform-specific EBUSY handling; wrap the part that calls getActionableNamedBackupRestores with an explicit process.platform override (save original, use Object.defineProperty to set process.platform to 'win32' for the Windows behavior the test expects, then restore original afterward) so the expectations remain deterministic; apply the same pattern to the other related tests mentioned (the ones calling getActionableNamedBackupRestores and asserting EBUSY behavior) to ensure consistent platform-specific behavior.

coderabbitai · 2026-03-16T16:14:10Z

test/recovery.test.ts

+describe("resolveStartupRecoveryAction", () => {
+	it("falls through to OAuth when backups exist but none are actionable", async () => {
+		const { resolveStartupRecoveryAction } = await import(
+			"../lib/codex-manager.js"
+		);
+
+		expect(
+			resolveStartupRecoveryAction(
+				{ assessments: [], totalBackups: 2 },
+				false,
+			),
+		).toBe("continue-with-oauth");
+		expect(
+			resolveStartupRecoveryAction(
+				{ assessments: [], totalBackups: 2 },
+				false,
+			),
+		).not.toBe("open-empty-storage-menu");
+	});
+
+	it("falls through to OAuth when the startup recovery scan itself failed", async () => {
+		const { resolveStartupRecoveryAction } = await import(
+			"../lib/codex-manager.js"
+		);
+
+		expect(
+			resolveStartupRecoveryAction(
+				{ assessments: [], totalBackups: 0 },
+				true,
+			),
+		).toBe("continue-with-oauth");
+	});
 });


⚠️ Potential issue | 🟠 Major

add missing startup recovery routing regressions

test/recovery.test.ts:783-815 only covers oauth fallthrough. it does not cover the positive actionable-backup route or same-session suppression states, which are the highest-risk branches for startup behavior regressions.

please add cases for:

actionable backups + no suppression => recovery prompt path

actionable backups + suppression => continue oauth

zero backups + no scan error => continue oauth (already covered)

As per coding guidelines, test/**: demand regression cases that reproduce critical flow regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@test/recovery.test.ts` around lines 783 - 815, Add regression tests for resolveStartupRecoveryAction to cover the missing branches: call resolveStartupRecoveryAction with an input object where assessments contains at least one actionable assessment (e.g., assessments: [{ actionable: true }]) and totalBackups > 0 and scanFailed = false and assert it returns the recovery prompt route (the code path currently implied by "open-empty-storage-menu"/recovery prompt); then add a test where assessments contains an actionable item but scanning is suppressed (pass the suppression/scanFailed flag that triggers same-session suppression) and assert it returns "continue-with-oauth". Keep the existing tests for zero backups/no scan error as-is. Reference resolveStartupRecoveryAction and the input shape (assessments, totalBackups, scanFailed) when adding the two new cases.

coderabbitai · 2026-03-16T16:14:10Z

test/storage.test.ts

+		it("rethrows EBUSY backup directory errors after one attempt on non-Windows platforms", async () => {
 			const platformSpy = vi
 				.spyOn(process, "platform", "get")
-				.mockReturnValue("win32");
+				.mockReturnValue("linux");
+			const readdirSpy = vi.spyOn(fs, "readdir");
+			const error = new Error("backup directory busy") as NodeJS.ErrnoException;
+			error.code = "EBUSY";
+			readdirSpy.mockRejectedValue(error);
+
+			try {
+				await expect(listNamedBackups()).rejects.toMatchObject({ code: "EBUSY" });
+				expect(readdirSpy).toHaveBeenCalledTimes(1);
+			} finally {


⚠️ Potential issue | 🟠 Major

fix platform ambiguity in ebusy retry coverage

test/storage.test.ts:1605-1617 explicitly asserts no EBUSY retries on non-windows, but test/storage.test.ts:1638-1678 expects EBUSY retry success without pinning the platform. this is contradictory and can mask regressions.

proposed test fix

it("retries transient backup directory errors while listing backups", async () => { + const platformSpy = vi + .spyOn(process, "platform", "get") + .mockReturnValue("win32"); await saveAccounts({ version: 3, activeIndex: 0, @@ try { const backups = await listNamedBackups(); @@ } finally { readdirSpy.mockRestore(); + platformSpy.mockRestore(); } });

As per coding guidelines, test/**: tests must stay deterministic and use vitest, and demand regression cases that reproduce windows filesystem behavior.

Also applies to: 1638-1678

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 1605 - 1617, The two tests are ambiguous about platform: ensure the non-Windows test for EBUSY retry uses a mocked process.platform returning "linux" and the Windows-retry test uses process.platform returning "win32" so behavior is deterministic; in the non-Windows case (test around listNamedBackups) keep vi.spyOn(process, "platform", "get").mockReturnValue("linux") and assert readdir was called once, and in the separate Windows-retry test (lines ~1638-1678) mock process.platform to "win32" and set readdir to initially reject with an EBUSY error then succeed on retry to assert retry behavior; also restore/clear spies/mocks after each test to avoid cross-test leakage.

lib/codex-manager.ts

coderabbitai

Actionable comments posted: 4

♻️ Duplicate comments (2)

test/storage.test.ts (1)

1812-1852: ⚠️ Potential issue | 🟠 Major

pin this restore-retry test to win32 for deterministic behavior.

test/storage.test.ts: line 1812 asserts transient directory retry on restore but does not mock process.platform. this can be host-dependent and flaky if retry logic is windows-specific.
proposed fix
 		it("retries transient backup directory errors while restoring backups", async () => {
+			const platformSpy = vi
+				.spyOn(process, "platform", "get")
+				.mockReturnValue("win32");
 			await saveAccounts({
 				version: 3,
 				activeIndex: 0,
 				accounts: [
@@
 			try {
 				const result = await restoreNamedBackup("retry-restore-dir");
 				expect(result.total).toBe(1);
 				expect(busyFailures).toBe(1);
 			} finally {
 				readdirSpy.mockRestore();
+				platformSpy.mockRestore();
 			}
 		});
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/storage.test.ts` around lines 1812 - 1852, The test for retrying
transient directory errors is flaky because it depends on the OS-specific retry
behavior; update the test around restoreNamedBackup to pin process.platform to
"win32" during the test (use Vitest's vi.stubProperty or equivalent to mock
process.platform before calling restoreNamedBackup) so the retry logic is
exercised deterministically, then restore the original platform stub in the
finally block; ensure you reference restoreNamedBackup and the readdir spy
(readdirSpy) so you mock platform only for this test and clean up both the
platform stub and readdirSpy in the finally section.

lib/codex-manager.ts (1)

4391-4418: ⚠️ Potential issue | 🟠 Major

retry transient assessment failures before skipping a backup.

lib/codex-manager.ts: lines 4397-4416 skip assessment after a single rejection. transient filesystem failures can hide recoverable backups and suppress restore opportunities in this run.

proposed hardening

+async function assessNamedBackupRestoreWithRetry(
+	backupName: string,
+	currentStorage: Awaited<ReturnType<typeof loadAccounts>>,
+): Promise<BackupRestoreAssessment> {
+	let attempt = 0;
+	while (true) {
+		try {
+			return await assessNamedBackupRestore(backupName, { currentStorage });
+		} catch (error) {
+			const code = (error as NodeJS.ErrnoException | undefined)?.code;
+			const retryableCode =
+				code === "EBUSY" || code === "EPERM" || code === "EAGAIN" || code === "EACCES";
+			const retryable429 =
+				typeof (error as { status?: number } | undefined)?.status === "number" &&
+				(error as { status?: number }).status === 429;
+			if ((!retryableCode && !retryable429) || attempt >= 2) {
+				throw error;
+			}
+			await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt));
+			attempt += 1;
+		}
+	}
+}
+
 async function loadBackupRestoreManagerAssessments(): Promise<
 	BackupRestoreAssessment[]
 > {
@@
 		const chunk = backups.slice(index, index + NAMED_BACKUP_ASSESS_CONCURRENCY);
 		const settledAssessments = await Promise.allSettled(
 			chunk.map((backup) =>
-				assessNamedBackupRestore(backup.name, { currentStorage }),
+				assessNamedBackupRestoreWithRetry(backup.name, currentStorage),
 			),
 		);

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 4391 - 4418, The loop that processes
backups currently skips a failed assessNamedBackupRestore result after a single
rejection; change this to retry transient failures (e.g., filesystem EBUSY, HTTP
429/5xx) a few times with backoff before logging and skipping. Implement retry
logic around assessNamedBackupRestore (used in the chunk.map call) with a
maxRetries (e.g., 3), exponential or jittered delay, and a transient-error
predicate that checks error.code (like "EBUSY") or response/status codes for
429/5xx; keep NAMED_BACKUP_ASSESS_CONCURRENCY unchanged and preserve the
Promise.allSettled pattern but call the retried wrapper instead so
settledAssessments contains final results; when ultimately logging the skipped
backup (using backup.name from chunk[resultIndex]) include the final error
message but avoid leaking secrets. Ensure tests (vitest) are updated to cover
retry behavior and EBUSY/429 simulated failures.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/storage.ts`:
- Around line 143-149: The exported type ImportAccountsResult currently marks
changed as optional but importAccounts (and its callers) always produce that
field; make changed required to keep the public contract consistent: update the
type declaration ImportAccountsResult (in lib/storage.ts) to declare changed:
boolean (remove the ?), and ensure any implementations/returns in importAccounts
still supply a boolean for changed; this avoids downstream surprises when
callers later expect changed to exist (see importAccounts function and the
destructuring sites in index.ts for where it matters).
- Around line 2045-2057: The fallback call to importAccounts in
restoreNamedBackup when assessment.backup.loadError is set is likely to repeat
the same failure; change the behavior in restoreNamedBackup to not
unconditionally call importAccounts on loadError—either rethrow or wrap and
throw the original assessment.backup.loadError (or a clear Error including
assessment.error and assessment.backup.loadError details) instead of calling
importAccounts(backupPath); locate restoreNamedBackup and the
assessNamedBackupRestore/assessment.backup.loadError check and replace the
importAccounts fallback with throwing or wrapping the loadError so the
persistent load failure is surfaced cleanly.
- Around line 3003-3015: The nested ternary in computing storage (involving
currentStoragePath, transactionState, and transactionState.snapshot) hides the
validation IIFE that throws on a transaction storage-path mismatch; hoist the
validation: before assigning storage, if transactionState?.active is true verify
(process.platform === "win32" ? transactionState.storagePath?.toLowerCase() ===
currentStoragePath?.toLowerCase() : transactionState.storagePath ===
currentStoragePath) and if it fails throw a clear Error("exportAccounts called
inside an active transaction for a different storage path"); otherwise proceed
to set storage to transactionState.snapshot, and keep the fallback to await
withAccountStorageTransaction((current) => Promise.resolve(current)).
- Around line 287-305: The retry logic in unlinkWithRetry is inconsistent with
retryTransientFilesystemOperation: unlinkWithRetry currently retries
EPERM/EBUSY/EAGAIN on all platforms while isRetryableFilesystemErrorCode (used
by retryTransientFilesystemOperation) restricts EPERM/EBUSY to win32; change
unlinkWithRetry to use the same platform-aware helper (or extract the platform
check into a shared function) so both use isRetryableFilesystemErrorCode (or its
new shared helper) to decide retries for EPERM/EBUSY/EAGAIN, keeping the same
backoff and attempt limits in unlinkWithRetry and preserving ENOENT early-return
behavior.

---

Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4391-4418: The loop that processes backups currently skips a
failed assessNamedBackupRestore result after a single rejection; change this to
retry transient failures (e.g., filesystem EBUSY, HTTP 429/5xx) a few times with
backoff before logging and skipping. Implement retry logic around
assessNamedBackupRestore (used in the chunk.map call) with a maxRetries (e.g.,
3), exponential or jittered delay, and a transient-error predicate that checks
error.code (like "EBUSY") or response/status codes for 429/5xx; keep
NAMED_BACKUP_ASSESS_CONCURRENCY unchanged and preserve the Promise.allSettled
pattern but call the retried wrapper instead so settledAssessments contains
final results; when ultimately logging the skipped backup (using backup.name
from chunk[resultIndex]) include the final error message but avoid leaking
secrets. Ensure tests (vitest) are updated to cover retry behavior and EBUSY/429
simulated failures.

In `@test/storage.test.ts`:
- Around line 1812-1852: The test for retrying transient directory errors is
flaky because it depends on the OS-specific retry behavior; update the test
around restoreNamedBackup to pin process.platform to "win32" during the test
(use Vitest's vi.stubProperty or equivalent to mock process.platform before
calling restoreNamedBackup) so the retry logic is exercised deterministically,
then restore the original platform stub in the finally block; ensure you
reference restoreNamedBackup and the readdir spy (readdirSpy) so you mock
platform only for this test and clean up both the platform stub and readdirSpy
in the finally section.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a761cc4e-b662-48c2-a769-9a2bd6cdb527

📥 Commits

Reviewing files that changed from the base of the PR and between 800d49f and dc93cfe.

📒 Files selected for processing (4)

lib/codex-manager.ts
lib/storage.ts
test/codex-manager-cli.test.ts
test/storage.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (2)

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

test/storage.test.ts

lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

lib/storage.ts
lib/codex-manager.ts

🔇 Additional comments (5)

lib/storage.ts (5)
3091-3100: deduplicateAccounts(merged) still called twice.

at lib/storage.ts:3092 and lib/storage.ts:3100, deduplicateAccounts(merged) is invoked in sequence - once inside the limit guard, once after. past review suggested computing once and reusing.
suggested fix (same as prior review)
+	const deduplicatedAccounts = deduplicateAccounts(merged);
 	if (merged.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
-		const deduped = deduplicateAccounts(merged);
-		if (deduped.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
+		if (deduplicatedAccounts.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
 			throw new Error(
-				`Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduped.length})`,
+				`Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduplicatedAccounts.length})`,
 			);
 		}
 	}
-
-	const deduplicatedAccounts = deduplicateAccounts(merged);
1825-1852: windows-platform test coverage for retryTransientFilesystemOperation still missing.

per prior review, test/storage.test.ts:1590-1618 covers linux EPERM/EBUSY rethrow (no retry), but there's no explicit test that mocks process.platform = "win32" and asserts EPERM/EBUSY are actually retried by retryTransientFilesystemOperation. the constraint shift at lib/storage.ts:1831 narrowing retries to win32-only is meaningful and warrants explicit coverage.
#!/bin/bash
# check if any test mocks win32 platform and exercises retryTransientFilesystemOperation with EPERM/EBUSY
rg -n -C5 --type=ts 'retryTransientFilesystemOperation.*EPERM|retryTransientFilesystemOperation.*EBUSY|win32.*retryTransient' test/
1294-1298: LGTM on explicit type annotation.

the explicit generic type const newest: T = selectNewestAccount(...) at lib/storage.ts:1294 clarifies intent and matches the surrounding pattern.

2664-2708: signature changed from Promise<void> to Promise<boolean> - callers properly handle the return value.

lib/storage.ts:2664 return type change is appropriately handled: lib/destructive-actions.ts:174 and 186 capture the boolean and return it as part of DestructiveActionResult. the actual callers in lib/codex-manager.ts:3999 and 4029-4031 inspect the result and log distinct messages on partial failure, so users get feedback when artifact cleanup doesn't fully succeed. tests in test/destructive-actions.test.ts verify the boolean values are correct.
			> Likely an incorrect or invalid review comment.
2932-2983: clearFlaggedAccounts now returns success status - callers already handle this.

signature changed from Promise<void> to Promise<boolean> at lib/storage.ts:2932. return value is captured at lib/destructive-actions.ts:188 and properly propagated through DestructiveActionResult. tests verify both success and failure cases at test/destructive-actions.test.ts:61-72 and test/storage-flagged.test.ts.

coderabbitai · 2026-03-18T17:13:16Z

lib/storage.ts

+export type ImportAccountsResult = {
+	imported: number;
+	total: number;
+	skipped: number;
+	// Keep the legacy field in the public type so existing callers do not break.
+	changed?: boolean;
+};


🧹 Nitpick | 🔵 Trivial

ImportAccountsResult.changed is optional but callers may expect it.

the type at lib/storage.ts:148 marks changed as optional (changed?: boolean), but importAccounts at lib/storage.ts:3147 always returns it. downstream callers in index.ts:4119-4150 currently destructure only imported, skipped, total - so this is safe today. if the intent is to keep changed in the public contract, consider making it required to avoid silent undefined checks later.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 143 - 149, The exported type ImportAccountsResult currently marks changed as optional but importAccounts (and its callers) always produce that field; make changed required to keep the public contract consistent: update the type declaration ImportAccountsResult (in lib/storage.ts) to declare changed: boolean (remove the ?), and ensure any implementations/returns in importAccounts still supply a boolean for changed; this avoids downstream surprises when callers later expect changed to exist (see importAccounts function and the destructuring sites in index.ts for where it matters).

coderabbitai · 2026-03-18T17:13:16Z

lib/storage.ts

+async function unlinkWithRetry(path: string): Promise<void> {
+	for (let attempt = 0; attempt < 5; attempt += 1) {
+		try {
+			await fs.unlink(path);
+			return;
+		} catch (error) {
+			const unlinkError = error as NodeJS.ErrnoException;
+			const code = unlinkError.code;
+			if (code === "ENOENT") {
+				return;
+			}
+			if ((code === "EPERM" || code === "EBUSY" || code === "EAGAIN") && attempt < 4) {
+				await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt));
+				continue;
+			}
+			throw unlinkError;
+		}
+	}
+}


⚠️ Potential issue | 🟡 Minor

inconsistent retry policy between unlinkWithRetry and retryTransientFilesystemOperation.

unlinkWithRetry at lib/storage.ts:298 retries EPERM/EBUSY/EAGAIN unconditionally across all platforms, but isRetryableFilesystemErrorCode at lib/storage.ts:1831 restricts EPERM/EBUSY retries to win32 only. this creates divergent retry behavior depending on which helper is used.

recommend aligning the retry policies - either extract the platform check into a shared helper or document why unlink needs different behavior.

suggested alignment

async function unlinkWithRetry(path: string): Promise<void> { for (let attempt = 0; attempt < 5; attempt += 1) { try { await fs.unlink(path); return; } catch (error) { const unlinkError = error as NodeJS.ErrnoException; const code = unlinkError.code; if (code === "ENOENT") { return; } - if ((code === "EPERM" || code === "EBUSY" || code === "EAGAIN") && attempt < 4) { + if (isRetryableFilesystemErrorCode(code) && attempt < 4) { await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt)); continue; } throw unlinkError; } } }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

async function unlinkWithRetry(path: string): Promise<void> {

for (let attempt = 0; attempt < 5; attempt += 1) {

try {

await fs.unlink(path);

return;

} catch (error) {

const unlinkError = error as NodeJS.ErrnoException;

const code = unlinkError.code;

if (code === "ENOENT") {

return;

}

if ((code === "EPERM" || code === "EBUSY" || code === "EAGAIN") && attempt < 4) {

await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt));

continue;

}

throw unlinkError;

}

}

}

async function unlinkWithRetry(path: string): Promise<void> {

for (let attempt = 0; attempt < 5; attempt += 1) {

try {

await fs.unlink(path);

return;

} catch (error) {

const unlinkError = error as NodeJS.ErrnoException;

const code = unlinkError.code;

if (code === "ENOENT") {

return;

}

if (isRetryableFilesystemErrorCode(code) && attempt < 4) {

await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt));

continue;

}

throw unlinkError;

}

}

}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 287 - 305, The retry logic in unlinkWithRetry is inconsistent with retryTransientFilesystemOperation: unlinkWithRetry currently retries EPERM/EBUSY/EAGAIN on all platforms while isRetryableFilesystemErrorCode (used by retryTransientFilesystemOperation) restricts EPERM/EBUSY to win32; change unlinkWithRetry to use the same platform-aware helper (or extract the platform check into a shared function) so both use isRetryableFilesystemErrorCode (or its new shared helper) to decide retries for EPERM/EBUSY/EAGAIN, keeping the same backoff and attempt limits in unlinkWithRetry and preserving ENOENT early-return behavior.

coderabbitai · 2026-03-18T17:13:17Z

lib/storage.ts

+export async function restoreNamedBackup(
+	name: string,
+): Promise<ImportAccountsResult> {
+	const backupPath = await resolveNamedBackupRestorePath(name);
+	const assessment = await assessNamedBackupRestore(name);
+	if (!assessment.eligibleForRestore) {
+		if (assessment.backup.loadError) {
+			return importAccounts(backupPath);
+		}
+		throw new Error(assessment.error ?? "Backup is not eligible for restore.");
+	}
+	return importAccounts(backupPath);
+}


⚠️ Potential issue | 🟡 Minor

restore fallback on loadError will likely just fail again.

when !assessment.eligibleForRestore and assessment.backup.loadError is truthy, this code still calls importAccounts(backupPath) at lib/storage.ts:2052. since the assessment already retried loading via loadBackupCandidate, a persistent error will just surface again during import.

if the intent is to surface a cleaner error from importAccounts, consider adding a comment clarifying that. otherwise this looks like dead-code path that never succeeds.

if (!assessment.eligibleForRestore) { if (assessment.backup.loadError) { return importAccounts(backupPath); // will likely throw the same error } throw new Error(assessment.error ?? "Backup is not eligible for restore."); }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 2045 - 2057, The fallback call to importAccounts in restoreNamedBackup when assessment.backup.loadError is set is likely to repeat the same failure; change the behavior in restoreNamedBackup to not unconditionally call importAccounts on loadError—either rethrow or wrap and throw the original assessment.backup.loadError (or a clear Error including assessment.error and assessment.backup.loadError details) instead of calling importAccounts(backupPath); locate restoreNamedBackup and the assessNamedBackupRestore/assessment.backup.loadError check and replace the importAccounts fallback with throwing or wrapping the loadError so the persistent load failure is surfaced cleanly.

coderabbitai · 2026-03-18T17:13:17Z

lib/storage.ts

+	const currentStoragePath = getStoragePath();
 	const storage = transactionState?.active
-		? transactionState.snapshot
-		: await withAccountStorageTransaction((current) =>
-				Promise.resolve(current),
-			);
+		? (process.platform === "win32"
+				? transactionState.storagePath?.toLowerCase() ===
+					currentStoragePath?.toLowerCase()
+				: transactionState.storagePath === currentStoragePath)
+			? transactionState.snapshot
+			: (() => {
+					throw new Error(
+						"exportAccounts called inside an active transaction for a different storage path",
+					);
+				})()
+		: await withAccountStorageTransaction((current) => Promise.resolve(current));


🧹 Nitpick | 🔵 Trivial

transaction storage-path mismatch throws inside nested ternary - hard to debug.

the IIFE at lib/storage.ts:3010-3014 throws inside a deeply nested ternary, making stack traces less helpful. consider hoisting this validation before the ternary for clearer error attribution.

suggested refactor

const transactionState = transactionSnapshotContext.getStore(); const currentStoragePath = getStoragePath(); + +if (transactionState?.active) { + const pathsMatch = process.platform === "win32" + ? transactionState.storagePath?.toLowerCase() === currentStoragePath?.toLowerCase() + : transactionState.storagePath === currentStoragePath; + if (!pathsMatch) { + throw new Error( + "exportAccounts called inside an active transaction for a different storage path", + ); + } +} + const storage = transactionState?.active - ? (process.platform === "win32" - ? transactionState.storagePath?.toLowerCase() === - currentStoragePath?.toLowerCase() - : transactionState.storagePath === currentStoragePath) - ? transactionState.snapshot - : (() => { - throw new Error( - "exportAccounts called inside an active transaction for a different storage path", - ); - })() + ? transactionState.snapshot : await withAccountStorageTransaction((current) => Promise.resolve(current));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 3003 - 3015, The nested ternary in computing storage (involving currentStoragePath, transactionState, and transactionState.snapshot) hides the validation IIFE that throws on a transaction storage-path mismatch; hoist the validation: before assigning storage, if transactionState?.active is true verify (process.platform === "win32" ? transactionState.storagePath?.toLowerCase() === currentStoragePath?.toLowerCase() : transactionState.storagePath === currentStoragePath) and if it fails throw a clear Error("exportAccounts called inside an active transaction for a different storage path"); otherwise proceed to set storage to transactionState.snapshot, and keep the fallback to await withAccountStorageTransaction((current) => Promise.resolve(current)).

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (1)

lib/codex-manager.ts (1)
4105-4144: ⚠️ Potential issue | 🟠 Major

set promptWasShown before awaiting confirm().

lib/codex-manager.ts:4116-4121 only flips the guard after confirm() resolves. if confirm() renders the startup recovery prompt and then throws while reading input, the catch at lib/codex-manager.ts:4138-4139 resets recoveryPromptAttempted, and the next loginFlow pass immediately prompts again. please add the thrown-confirm vitest regression for this branch.
proposed fix
-					const restoreNow = await confirm(
+					promptWasShown = true;
+					const restoreNow = await confirm(
 						`Found ${recoveryState.assessments.length} recoverable backup${
 							recoveryState.assessments.length === 1 ? "" : "s"
 						} out of ${recoveryState.totalBackups} total (${backupLabel}) in ${backupDir}. Restore now?`,
 					);
-					promptWasShown = true;
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 4105 - 4144, The guard flag promptWasShown
is set only after awaiting confirm(), so if confirm() throws while the prompt
was displayed the catch resets recoveryPromptAttempted and immediately
retriggers the prompt; move the promptWasShown = true to before awaiting
confirm() (reference promptWasShown and the call to confirm() in the try block)
so the catch knows the prompt had been shown, and add a vitest regression that
simulates confirm() throwing after render (the "thrown-confirm" test) to assert
recoveryPromptAttempted is not reset when promptWasShown was true and ensure
logs do 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 `@lib/codex-manager.ts`:
- Around line 4370-4420: loadBackupRestoreManagerAssessments currently treats
directory-read failures and per-backup rejected assessments as "no backups" by
returning an empty list or skipping failed entries; change it so failures are
preserved as failed BackupRestoreAssessment entries instead of being swallowed:
when listNamedBackups throws, propagate or return a special failed assessment
result rather than [] (so runBackupRestoreManager and codex auth restore-backup
see an error), and inside the Promise.allSettled loop, for any result.status ===
"rejected" push a BackupRestoreAssessment object for that backup with a
failed/errored state and the collapseWhitespace(reason) message (use
assessNamedBackupRestore signature and NAMED_BACKUP_ASSESS_CONCURRENCY to locate
logic). Add vitest regression tests for unreadable-dir and rejected-assessment
cases that assert failures are returned (not an empty array) and that exit paths
in runBackupRestoreManager/codex auth restore-backup reflect failure.

---

Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4105-4144: The guard flag promptWasShown is set only after
awaiting confirm(), so if confirm() throws while the prompt was displayed the
catch resets recoveryPromptAttempted and immediately retriggers the prompt; move
the promptWasShown = true to before awaiting confirm() (reference promptWasShown
and the call to confirm() in the try block) so the catch knows the prompt had
been shown, and add a vitest regression that simulates confirm() throwing after
render (the "thrown-confirm" test) to assert recoveryPromptAttempted is not
reset when promptWasShown was true and ensure logs do not leak tokens/emails.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4c511502-f795-4175-aadb-b45a2c2a530d

📥 Commits

Reviewing files that changed from the base of the PR and between dc93cfe and c2a894a.

📒 Files selected for processing (2)

lib/codex-manager.ts
test/codex-manager-cli.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 (1)

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.ts

coderabbitai · 2026-03-19T01:05:50Z

lib/codex-manager.ts

+async function loadBackupRestoreManagerAssessments(): Promise<
+	BackupRestoreAssessment[]
+> {
+	let backups: Awaited<ReturnType<typeof listNamedBackups>>;
+	try {
+		backups = await listNamedBackups();
+	} catch (error) {
+		const message = error instanceof Error ? error.message : String(error);
+		console.error(
+			`Could not read backup directory: ${
+				collapseWhitespace(message) || "unknown error"
+			}`,
+		);
+		return [];
+	}
+	if (backups.length === 0) {
+		return [];
+	}
+
+	const currentStorage = await loadAccounts();
+	const assessments: BackupRestoreAssessment[] = [];
+	for (
+		let index = 0;
+		index < backups.length;
+		index += NAMED_BACKUP_ASSESS_CONCURRENCY
+	) {
+		const chunk = backups.slice(index, index + NAMED_BACKUP_ASSESS_CONCURRENCY);
+		const settledAssessments = await Promise.allSettled(
+			chunk.map((backup) =>
+				assessNamedBackupRestore(backup.name, { currentStorage }),
+			),
+		);
+		for (const [resultIndex, result] of settledAssessments.entries()) {
+			if (result.status === "fulfilled") {
+				assessments.push(result.value);
+				continue;
+			}
+			const backupName = chunk[resultIndex]?.name ?? "unknown";
+			const reason =
+				result.reason instanceof Error
+					? result.reason.message
+					: String(result.reason);
+			console.warn(
+				`Skipped backup assessment for "${backupName}": ${
+					collapseWhitespace(reason) || "unknown error"
+				}`,
+			);
+		}
+	}
+
+	return assessments;


⚠️ Potential issue | 🟠 Major

don't collapse backup assessment failures into the "no backups found" path.

lib/codex-manager.ts:4374-4416 turns both directory-read failures and rejected per-backup assessments into an empty list, so runBackupRestoreManager() falls through to no named backups found at lib/codex-manager.ts:4437-4439. that also makes codex auth restore-backup exit 0 at lib/codex-manager.ts:4571-4575 even when the backup dir is unreadable or the chunked assessor hit a transient windows fs error. lib/storage.ts:1858-1956 already preserves failed entries in allAssessments; reusing that path here would keep corrupt/unreadable backups visible and let real load failures return "failed" instead of looking like success. please add a vitest regression for unreadable-dir and rejected-assessment cases.

proposed fix

async function loadBackupRestoreManagerAssessments(): Promise< BackupRestoreAssessment[] > { - let backups: Awaited<ReturnType<typeof listNamedBackups>>; - try { - backups = await listNamedBackups(); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - console.error( - `Could not read backup directory: ${ - collapseWhitespace(message) || "unknown error" - }`, - ); - return []; - } + const backups = await listNamedBackups(); if (backups.length === 0) { return []; } - const currentStorage = await loadAccounts(); - const assessments: BackupRestoreAssessment[] = []; - for ( - let index = 0; - index < backups.length; - index += NAMED_BACKUP_ASSESS_CONCURRENCY - ) { - const chunk = backups.slice(index, index + NAMED_BACKUP_ASSESS_CONCURRENCY); - const settledAssessments = await Promise.allSettled( - chunk.map((backup) => - assessNamedBackupRestore(backup.name, { currentStorage }), - ), - ); - for (const [resultIndex, result] of settledAssessments.entries()) { - if (result.status === "fulfilled") { - assessments.push(result.value); - continue; - } - const backupName = chunk[resultIndex]?.name ?? "unknown"; - const reason = - result.reason instanceof Error - ? result.reason.message - : String(result.reason); - console.warn( - `Skipped backup assessment for "${backupName}": ${ - collapseWhitespace(reason) || "unknown error" - }`, - ); - } - } - - return assessments; + const { allAssessments } = await getActionableNamedBackupRestores({ + backups, + currentStorage: await loadAccounts(), + }); + return allAssessments; } ... - const assessments = - assessmentsOverride ?? (await loadBackupRestoreManagerAssessments()); + let assessments: BackupRestoreAssessment[]; + try { + assessments = + assessmentsOverride ?? (await loadBackupRestoreManagerAssessments()); + } catch (error) { + const errorLabel = getRedactedFilesystemErrorLabel(error); + console.error(`Could not load backup assessments (${errorLabel}).`); + return "failed"; + }

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.
Also applies to: 4435-4439, 4571-4575

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4370 - 4420, loadBackupRestoreManagerAssessments currently treats directory-read failures and per-backup rejected assessments as "no backups" by returning an empty list or skipping failed entries; change it so failures are preserved as failed BackupRestoreAssessment entries instead of being swallowed: when listNamedBackups throws, propagate or return a special failed assessment result rather than [] (so runBackupRestoreManager and codex auth restore-backup see an error), and inside the Promise.allSettled loop, for any result.status === "rejected" push a BackupRestoreAssessment object for that backup with a failed/errored state and the collapseWhitespace(reason) message (use assessNamedBackupRestore signature and NAMED_BACKUP_ASSESS_CONCURRENCY to locate logic). Add vitest regression tests for unreadable-dir and rejected-assessment cases that assert failures are returned (not an empty array) and that exit paths in runBackupRestoreManager/codex auth restore-backup reflect failure.

ndycode · 2026-03-19T15:12:11Z

Closing this preserved fresh/* PR as superseded. The startup-recovery / first-run behavior it was carrying is already covered on the current clean review path and in current dev, while this branch remains a large dirty review surface. The active review path for this area is the clean stack headed by #129.

ndycode added 5 commits March 15, 2026 23:19

fix(auth): define reset and delete safety flows

af7f137

feat(auth): add backup restore manager

f48ee06

feat(auth): prompt for recovery on startup

12074e2

test(storage): isolate empty export path

8212831

fix(auth): clean startup recovery prompt branch

390f801

Copilot AI review requested due to automatic review settings March 16, 2026 08:51

Copilot AI reviewed Mar 16, 2026

View reviewed changes

greptile-apps bot reviewed Mar 16, 2026

View reviewed changes

lib/storage.ts Outdated Show resolved Hide resolved

lib/storage.ts Outdated Show resolved Hide resolved

lib/codex-manager.ts Show resolved Hide resolved

lib/codex-manager.ts Show resolved Hide resolved

fix(storage): restore safe import and restore-backup handling

a7d87cd

greptile-apps bot reviewed Mar 16, 2026

View reviewed changes

lib/storage.ts Show resolved Hide resolved

lib/storage.ts Show resolved Hide resolved

lib/storage.ts Show resolved Hide resolved

lib/codex-manager.ts Show resolved Hide resolved

coderabbitai bot requested changes Mar 16, 2026

View reviewed changes

ndycode added 2 commits March 16, 2026 22:34

Block non-interactive backup restore mode

1eeec0b

Fix startup recovery backup accounting

474fbc4

greptile-apps bot reviewed Mar 16, 2026

View reviewed changes

test/recovery.test.ts Outdated Show resolved Hide resolved

lib/storage.ts Show resolved Hide resolved

ndycode added 3 commits March 16, 2026 23:30

fix(auth): harden restore backup fallback and file reads

326d5f1

fix(auth): sync direct restore and narrow retries

de6e256

Merge branch 'fresh/02-backup-restore-manager' into fresh/03-startup-…

800d49f

…recovery-prompt

coderabbitai bot requested changes Mar 16, 2026

View reviewed changes

greptile-apps bot reviewed Mar 16, 2026

View reviewed changes

lib/codex-manager.ts Show resolved Hide resolved

lib/codex-manager.ts Show resolved Hide resolved

fix(startup): restore backup recovery safeguards

d92f36e

ndycode changed the base branch from fresh/02-backup-restore-manager to dev March 18, 2026 06:47

fix(auth): guard named restore backup retries

dc93cfe

coderabbitai bot requested changes Mar 18, 2026

View reviewed changes

fix(auth): retry startup recovery after prompt failure

c2a894a

coderabbitai bot requested changes Mar 19, 2026

View reviewed changes

ndycode closed this Mar 19, 2026

Conversation

ndycode commented Mar 16, 2026 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What Changed

Validation

Docs and Governance Checklist

Risk and Rollback

Additional Notes

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

Confidence Score: 2/5

Important Files Changed

Flowchart

Comments Outside Diff (8)

Uh oh!

chatgpt-codex-connector bot commented Mar 16, 2026

Uh oh!

coderabbitai bot commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

walkthrough

changes

sequence diagram

estimated code review effort

flagged issues

❌ Failed checks (2 warnings)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 18, 2026

Choose a reason for hiding this comment

ndycode commented Mar 16, 2026 •

edited by greptile-apps bot

Loading

note: greptile review for oc-chatgpt-multi-auth. cite files like `lib/foo.ts:123`. confirm regression tests + windows concurrency/token redaction coverage.

coderabbitai bot commented Mar 16, 2026 •

edited

Loading