add onboarding restore for latest saved backup#137
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughadds an onboarding “recover saved accounts” path to auth. storage now discovers named backups and can restore them. login flow offers restore when the account pool is empty. ui copy, docs, types, and tests updated to cover discovery, restore, persistence, and failure cases. Changes
Sequence Diagram(s)sequenceDiagram
participant user as user
participant cli as codex manager
participant ui as ui/prompts
participant storage as storage
user->>cli: run `codex auth login` (no accounts)
cli->>storage: getNamedBackups()
storage-->>cli: list of NamedBackupSummary or []
alt backup found
cli->>ui: promptOAuthSignInMode(latestBackup)
ui-->>cli: "restore-backup"
cli->>ui: promptBackupRestoreMode(latestBackup)
ui-->>cli: "latest" or "manual"
alt manual selected
cli->>ui: promptManualBackupSelection(backups)
ui-->>cli: chosen backup path
end
cli->>ui: confirm(restoreBackupConfirm)
ui-->>cli: confirmed
cli->>storage: restoreAccountsFromBackup(path)
storage-->>cli: restored AccountStorage
cli->>cli: persistAndSyncSelectedAccount(preserveActiveIndexByFamily=true)
cli-->>user: show restored accounts / finish login
else no backup or user chooses oauth
cli->>ui: promptOAuthSignInMode(null)
ui-->>cli: "browser" or "manual"
cli->>cli: runOAuthFlow(forceNewLogin, signInMode)
cli-->>user: complete oauth flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes flags:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 822-859: In getLatestNamedBackup, the silent catch that surrounds
loadAccountsFromPath/stat hides useful diagnostics; update the catch for the
per-file processing (around candidatePath handling in getLatestNamedBackup) to
log the caught error at debug or trace level (including candidatePath and
error.message/stack) before continuing so permission/parse errors are visible to
operators; ensure you reference candidatePath, entry.name and the functions
loadAccountsFromPath/fs.stat in the log message and continue skipping only after
logging.
In `@test/codex-manager-cli.test.ts`:
- Around line 3026-3070: The test configures loadAccountsMock with two identical
mockResolvedValueOnce calls which is fragile; replace the second
mockResolvedValueOnce with a persistent mockResolvedValue (or keep only one
mockResolvedValueOnce if only one call is expected) so extra storage reads won't
change behavior, and add an explicit assertion that the UI wasn't offered a
backup restore by checking promptLoginModeMock was called with choices that do
not include a "restore" option (while still asserting getLatestNamedBackupMock
was not called) when invoking runCodexMultiAuthCli(["auth","login"]). Use the
symbols loadAccountsMock, promptLoginModeMock, getLatestNamedBackupMock, and
runCodexMultiAuthCli to locate the edits.
- Around line 2966-3024: Add three regression tests: (1) simulate
restoreAccountsFromBackupMock rejecting with an EBUSY error and assert
runCodexMultiAuthCli returns non-zero or logs the platform-specific error
message (verify console output contains the EBUSY text), (2) simulate
setCodexCliActiveSelectionMock resolving/returning false and assert that the
restoreBackupSyncWarning (or the CLI warning path) is logged via console.warn
and that no successful-active-selection log is emitted, and (3) simulate
confirmMock returning false and assert that restoreAccountsFromBackupMock is not
called, getLatestNamedBackupMock is not acted upon for restore, and that the CLI
logs/skips the restore flow accordingly; locate these behaviors via the mocks
restoreAccountsFromBackupMock, setCodexCliActiveSelectionMock, confirmMock,
getLatestNamedBackupMock and exercise runCodexMultiAuthCli to verify outputs.
- Line 2996: Change the test's mocking of loadAccounts so it returns null only
for the first call and then returns the restored storage on the subsequent
read-back: replace the blanket loadAccountsMock.mockResolvedValue(null) with
loadAccountsMock.mockResolvedValueOnce(null) followed by a mockResolvedValue(or
mockResolvedValueOnce) that returns the expected persisted storage; this ensures
the behavior around persistAndSyncSelectedAccount() and the restart of the
loginFlow (the later loadAccounts() call) is exercised and verified.
In `@test/storage-last-backup.test.ts`:
- Around line 50-87: Add three deterministic unit tests for getLatestNamedBackup
to cover the untested edge cases: (1) when the backups directory is missing
ensure getLatestNamedBackup returns null; (2) when all named backup files exist
but each has zero accounts ensure it returns null; and (3) simulate a backup
file being deleted between reading its contents and statting it (e.g., create a
file, have loadAccountsFromPath succeed then remove the file before fs.stat) and
assert getLatestNamedBackup skips that file and still returns the correct result
or null as appropriate. Use the same patterns in this test file
(buildNamedBackupPath, fs.writeFile, fs.utimes) and reference
getLatestNamedBackup and loadAccountsFromPath to locate where to add the tests.
🪄 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: b8f87b1a-2024-4032-9b2c-cdd7eeae83c3
📒 Files selected for processing (7)
docs/features.mddocs/getting-started.mdlib/codex-manager.tslib/storage.tslib/ui/copy.tstest/codex-manager-cli.test.tstest/storage-last-backup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/getting-started.mddocs/features.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-last-backup.test.tstest/codex-manager-cli.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/ui/copy.tslib/storage.tslib/codex-manager.ts
🔇 Additional comments (16)
docs/features.md (1)
12-12: lgtm.the new capability row accurately describes the onboarding backup restore feature and correctly references
codex auth loginas the primary entry point, consistent with the implementation inlib/codex-manager.ts.docs/getting-started.md (1)
48-54: lgtm.the updated flow documentation correctly describes the three sign-in options and their outcomes. the backup restore path is appropriately scoped to empty pool scenarios, matching the implementation in
lib/codex-manager.ts.lib/ui/copy.ts (1)
36-47: lgtm.the new ui copy strings follow the existing pattern, correctly pluralize account counts, and don't leak any sensitive data. the
restoreBackupSyncWarningmessage appropriately informs users when codex auth sync fails but local restore succeeds.test/codex-manager-cli.test.ts (2)
9-10: mock setup looks good.the new storage mocks
getLatestNamedBackupMockandrestoreAccountsFromBackupMockare properly declared, registered in the module mock attest/codex-manager-cli.test.ts:119-120, and reset inbeforeEachattest/codex-manager-cli.test.ts:440-441. the default return ofnullforgetLatestNamedBackupMockat line 531 ensures existing tests aren't affected.Also applies to: 119-120, 440-441
175-178: confirm mock added correctly.the
../lib/ui/confirm.jsmock attest/codex-manager-cli.test.ts:175-178with reset at line 456-457 follows the existing pattern for ui mocks.lib/codex-manager.ts (6)
56-60: lgtm - imports align with new storage exports.
getLatestNamedBackupandrestoreAccountsFromBackupare both exported fromlib/storage.ts. usage in the onboarding flow is correct.
1140-1144: interface subset is fine for ui consumption.
mtimeMsis omitted since it's only used internally for sorting inlib/storage.ts:857. no issue here.
1157-1167: conditional backup menu entry looks correct.the spread pattern for conditionally adding the restore option is clean.
hinttext properly references filename and count without exposing tokens.
1453-1456: signature refactor simplifies flow control.moving mode selection out of
runOAuthFlowis correct - callers now handle cancel/restore before invoking the oauth exchange.
4273-4312: restore flow is well-guarded but has a minor type narrowing opportunity.at
lib/codex-manager.ts:4273, the conditionsignInMode === "restore-backup" && latestNamedBackupis safe, but typescript already knowslatestNamedBackupis non-null whensignInMode === "restore-backup"because you only offer that mode when backup exists (line 4257-4263). the runtime check is still good defensive code.the
persistAndSyncSelectedAccountcall at line 4294-4299 correctly preserves the restoredactiveIndexfrom the backup viaresolveActiveIndex(restoredStorage).
4121-4121: call site update correct.
runOAuthFlow(true, "browser")for refresh flow matches the expected behavior.lib/storage.ts (2)
861-868: restore function correctly preserves active selection.
loadAccountsFromPath→parseAndNormalizeStorage→normalizeAccountStoragepreservesactiveIndexandactiveIndexByFamilywith proper clamping. the test attest/storage-last-backup.test.ts:116-119confirms this.
saveAccountsinternally useswithStorageLock, so concurrent writes are safe.
118-123: interface is well-defined.
mtimeMsenables sorting by recency,accountCountis shown in ui hint. all fields populated atlib/storage.ts:846-851.test/storage-last-backup.test.ts (3)
14-30: good retry helper for windows filesystem cleanup.the
EBUSY/EPERM/ENOTEMPTYretry set matches common windows errors. exponential backoff at25 * 2^attemptis reasonable.
89-120: restore test correctly verifies replacement semantics.
expect(loaded?.accounts).toHaveLength(2)at line 117 confirms the original account was replaced, not merged.activeIndexpreservation is verified at line 118.one minor note: consider asserting
loaded?.accounts[0]?.refreshToken === "refresh-1"as well to confirm both accounts are from the backup.
45-48: cleanup properly resets storage path.
setStoragePathDirect(null)beforeremoveWithRetryensures no lingering state between tests. the order matters here since cleanup happens before directory removal.
|
@greptile @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
Addressed the actionable onboarding-restore review items on the current PR head. What changed since the earlier bot summaries:
Validation now passes:
The remaining large CodeRabbit/Greptile summaries appear stale against the current branch head and still reference older symbols/behavior that are no longer present. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/getting-started.md (1)
44-56:⚠️ Potential issue | 🟠 Majoradd an upgrade-note reference for this onboarding behavior change.
this section documents a changed login workflow, but there is no upgrade-note pointer in the updated path. add a short upgrade note reference near this flow so rollout expectations are explicit.
As per coding guidelines,
docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started.md` around lines 44 - 56, Add a short upgrade-note reference adjacent to the "Expected flow" block in docs/getting-started.md summarizing the changed login workflow and linking to the upgrade notes (or file/section where upgrade notes live), and explicitly call out any new npm scripts and changed CLI flags related to the onboarding flow; update the paragraph near the steps (the "Expected flow" heading) to include one-line pointer like "See upgrade note: [summary/link] for rollout and CLI flag/npm script changes."
♻️ Duplicate comments (1)
test/codex-manager-cli.test.ts (1)
3141-3214:⚠️ Potential issue | 🟡 Minorassert the warning branch when codex sync returns false.
line 3141 persists local restore correctly, but it does not assert the warning path when
setCodexCliActiveSelectionMockreturnsfalseintest/codex-manager-cli.test.ts:3191. that leaves the sync-failure regression partially unverified.proposed test hardening
it("keeps the restored pool when Codex sync returns false", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const now = Date.now(); setInteractiveTTY(true); let storageState: { @@ expect(setCodexCliActiveSelectionMock).toHaveBeenCalledWith( expect.objectContaining({ accountId: "acc_warn", email: "warn@example.com", }), ); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("Codex auth sync did not complete"), + ); expect(promptLoginModeMock).toHaveBeenCalledTimes(1); expect(selectMock).toHaveBeenCalledTimes(2); });As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 3141 - 3214, The test currently stubs setCodexCliActiveSelectionMock to resolve false but never asserts the warning path; update the test to verify the warning branch was taken by asserting the logger warning was called when setCodexCliActiveSelectionMock returns false—specifically, add an expectation that the processLogger.warn (or the module's logger.warn used by runCodexMultiAuthCli) mock was called (e.g. toHaveBeenCalledTimes(1) and toHaveBeenCalledWith(expect.stringMatching(/sync|warning|failed/i))) after invoking runCodexMultiAuthCli, while keeping the existing assertions for setCodexCliActiveSelectionMock, saveAccountsMock, promptLoginModeMock and selectMock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started.md`:
- Around line 48-52: Update the docs/getting-started text so the sign-in option
label matches the CLI and tests: replace the phrase "Restore Saved Backup" (and
any dependent wording in the following step 4) with the exact CLI label "Recover
saved accounts" (as referenced in the test `Recover saved accounts` in
test/codex-manager-cli.test.ts:3048) to avoid user confusion; also ensure
README, SECURITY, and other docs are updated for consistency, add an upgrade
note describing the label/behavior change, and mention any new npm scripts
introduced by this change.
In `@lib/codex-manager.ts`:
- Around line 4406-4416: The restore flow is overwriting the backup's per-family
selection map because persistAndSyncSelectedAccount always resets
activeIndexByFamily to targetIndex; update the restore path so the original
restoredStorage.activeIndexByFamily is preserved: either add an option/flag to
persistAndSyncSelectedAccount (called from the restore code that calls
restoreAccountsFromBackup and resolveActiveIndex) to skip resetting
activeIndexByFamily, or change persistAndSyncSelectedAccount to only initialize
activeIndexByFamily when it is missing, leaving an existing map intact; adjust
the call-site that passes storage/targetIndex/parsed/switchReason so it sets
this new preserve flag (or ensures restoredStorage.activeIndexByFamily is
carried through), and add a vitest that creates a backup whose
activeIndexByFamily differs from activeIndex and verifies that after restore the
per-family map remains unchanged while other rotation/auth behaviors still run.
- Around line 4429-4432: The catch block in the restore flow is printing a
write-oriented hint by calling formatStorageErrorHint for all errors; change it
so the extra hint is only emitted for storage write errors (i.e., when error
instanceof StorageError from lib/storage.ts) or replace it with a read-specific
hint for restore errors from restoreAccountsFromBackup, and still log the
original backup restore failure message (the message variable) unchanged so the
vitest assertion still matches; specifically update the catch in
codex-manager.ts to gate the call to formatStorageErrorHint on error instanceof
StorageError (or call a new restore-read hint helper) and leave the
console.error(`Backup restore failed: ${message}`) as-is.
In `@lib/storage.ts`:
- Around line 874-885: Ensure restoreAccountsFromBackup only accepts files
inside the managed named-backup root by resolving and validating real paths:
call fs.realpath on the provided path and on the named-backup root (using the
same root resolution used by collectNamedBackups / getNamedBackupRoot), then
verify the resolved file path is contained within the resolved root before
calling loadAccountsFromPath and saveAccounts; if containment fails, throw an
error. Update restoreAccountsFromBackup to reference loadAccountsFromPath and
saveAccounts as before but perform the realpath containment check first, and add
a vitest in test/storage-last-backup.test.ts that creates a symlinked/escaped
backup path demonstrating the check rejects paths outside the named-backup root.
In `@test/codex-manager-cli.test.ts`:
- Around line 3430-3493: Add a regression test that covers the user backing out
of the manual backup picker by having selectMock resolve the same initial
choices but return null for the manual-pick selection; call
runCodexMultiAuthCli(["auth","login"]) and assert exitCode is 0, that
restoreAccountsFromBackupMock was NOT called with
"/mock/backups/manual-choice.json", that confirmMock was NOT called for "Load
manual-choice.json (1 account)?", and that saveAccountsMock call count reflects
no restore (e.g., toHaveBeenCalledTimes(0 or existing count)); use the same
mocks as the existing test (selectMock, restoreAccountsFromBackupMock,
saveAccountsMock, confirmMock, promptLoginModeMock) and mirror the setup for
getNamedBackupsMock and loadAccountsMock but replace mockResolvedValueOnce for
the third selectMock call with null to simulate backing out.
In `@test/storage-last-backup.test.ts`:
- Around line 145-164: The fs.stat spy created as statSpy in the test must be
restored even if getNamedBackups() throws; wrap the call to getNamedBackups()
and the subsequent assertions in a try/finally (or ensure finally-like cleanup)
so that statSpy.mockRestore() is executed in the finally block; locate the
mockImplementation on fs.stat and the call to getNamedBackups() in
test/storage-last-backup.test.ts and move the mockRestore() into the finally to
prevent the spy leaking into other tests.
---
Outside diff comments:
In `@docs/getting-started.md`:
- Around line 44-56: Add a short upgrade-note reference adjacent to the
"Expected flow" block in docs/getting-started.md summarizing the changed login
workflow and linking to the upgrade notes (or file/section where upgrade notes
live), and explicitly call out any new npm scripts and changed CLI flags related
to the onboarding flow; update the paragraph near the steps (the "Expected flow"
heading) to include one-line pointer like "See upgrade note: [summary/link] for
rollout and CLI flag/npm script changes."
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 3141-3214: The test currently stubs setCodexCliActiveSelectionMock
to resolve false but never asserts the warning path; update the test to verify
the warning branch was taken by asserting the logger warning was called when
setCodexCliActiveSelectionMock returns false—specifically, add an expectation
that the processLogger.warn (or the module's logger.warn used by
runCodexMultiAuthCli) mock was called (e.g. toHaveBeenCalledTimes(1) and
toHaveBeenCalledWith(expect.stringMatching(/sync|warning|failed/i))) after
invoking runCodexMultiAuthCli, while keeping the existing assertions for
setCodexCliActiveSelectionMock, saveAccountsMock, promptLoginModeMock and
selectMock.
🪄 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: 6159b437-5334-4ec0-b320-bd54749a54dd
📒 Files selected for processing (7)
docs/features.mddocs/getting-started.mdlib/codex-manager.tslib/storage.tslib/ui/copy.tstest/codex-manager-cli.test.tstest/storage-last-backup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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/features.mddocs/getting-started.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-cli.test.tstest/storage-last-backup.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.tslib/ui/copy.tslib/codex-manager.ts
🔇 Additional comments (2)
lib/codex-manager.ts (1)
4389-4397: nice fix on the destructive restore gate.
lib/codex-manager.ts:4389-4397now requiresconfirm()for both the latest and manual backup paths, which closes the earlier one-enter overwrite path.docs/features.md (1)
12-12: looks good.the new capability row is consistent with the onboarding restore behavior exercised in
test/codex-manager-cli.test.ts:3047andtest/codex-manager-cli.test.ts:3487.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
lib/codex-manager.ts (1)
4433-4438:⚠️ Potential issue | 🟡 Minorprint the storage error's own hint here.
lib/codex-manager.ts:4435-4437is now correctly gated toStorageError, but it still formats the hint withselectedBackup.path. whensaveAccounts()fails,lib/storage.ts:176-193already carries the destination storage path and hint, so this currently points users at the source backup file instead of the file that actually failed to write. a small onboarding vitest around the persistence-failure branch would keep this from regressing.proposed fix
- if (error instanceof StorageError) { - console.error(formatStorageErrorHint(error, selectedBackup.path)); - } + if (error instanceof StorageError) { + console.error(error.hint); + }also drop the now-unused
formatStorageErrorHintimport atlib/codex-manager.ts:56.
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 4433 - 4438, The catch block for StorageError is printing a hint for the wrong path (using selectedBackup.path) — change the call to use the StorageError's own hint/path (e.g., use the StorageError instance's properties or call formatStorageErrorHint with the error's destination/hint rather than selectedBackup.path) so the message points at the actual failed write (the saveAccounts destination); update the catch to cast error to StorageError and pass the error's path/hint into formatStorageErrorHint (or print error.hint directly), then remove the now-unused formatStorageErrorHint import from the top of the file; add a small vitest that simulates persistence failure in saveAccounts to assert the code logs the storage error hint for the failing destination.lib/storage.ts (1)
878-893:⚠️ Potential issue | 🟠 Majorclose the remaining restore check/use race.
lib/storage.ts:878-893validates the backup path withrealpath(), then reopens the same pathname later throughloadAccountsFromPath(). a file or symlink swap between those two steps can still redirect the read outside the managed backup root, so the containment check is not atomic yet. please collapse validation + read into one guarded open/read path, and add a vitest besidetest/storage-last-backup.test.ts:258-276that swaps a validated backup before the read.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 878 - 893, The TOCTOU comes from calling fs.realpath() to validate the backup path then later calling loadAccountsFromPath(path); fix by collapsing validation+read: open the target with fs.promises.open using flags fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW (or open the parent dir and open the entry by name without following symlinks), read the file contents from the returned FileHandle (don't re-resolve the pathname), and pass the buffer/string into a new loadAccountsFromBuffer/loadAccountsFromFd variant (or extend loadAccountsFromPath to accept a Buffer/FileHandle) so the read happens from the guarded open; keep the existing getNamedBackupRoot/resolvedBackupRoot check by realpath-ing the resolvedBackupRoot once and ensure you only accept files opened via the non-following FileHandle, then throw if opening fails or the handle indicates the entry is outside the root; add a vitest alongside test/storage-last-backup.test.ts (covering lines ~258-276) that swaps the backup symlink between validation and read and assert the operation is rejected; ensure logging changes avoid leaking tokens/emails and that new queue/error paths handle EBUSY/429 per guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started.md`:
- Around line 46-57: Update the onboarding steps in docs/getting-started.md so
they reflect the true empty-pool CLI flow implemented in lib/codex-manager.ts
(the branch starting at the sign-in menu around the logic near lines 4355-4369)
— remove or rework the “dashboard → Add New Account” path that only appears when
accounts exist (the branch near 4246-4253), and instead document that a
first-time login goes straight to the sign-in menu with the three options (Open
Browser (Easy), Manual / Incognito, Recover saved accounts); when mentioning the
backup directory use the repo’s default (~/.codex/multi-auth/backups) and also
document the CODEX_MULTI_AUTH_DIR override in the same paragraph; keep steps
concise and ensure the described menu flow (automatic newest valid backup vs.
manual newest-first selection, completing OAuth, restoring and syncing active
account, and confirming saved account) matches the behavior in the
sign-in/restore logic.
In `@lib/codex-manager.ts`:
- Around line 1144-1151: The function formatBackupSavedAt currently forces an
"en-US" locale; change its toLocaleString call to pass undefined (i.e., use new
Date(mtimeMs).toLocaleString(undefined, {...})) so it respects the runtime
locale, and update callers that render the backup restore hints (where
formatBackupSavedAt is used) if needed to accept the localized string unchanged;
additionally add a Vitest unit test that renders the backup hint output (the
code path that calls formatBackupSavedAt) and asserts the expected formatted
substring appears to provide coverage and prevent regressions.
In `@test/codex-manager-cli.test.ts`:
- Around line 3078-3110: Add a deterministic regression test that covers the
restore-mode returning "back" before the manual/latest flow by invoking
runCodexMultiAuthCli(["auth","login"]) and configuring selectMock to resolve the
sequence that includes "restore-backup" followed by "back" (instead of null),
asserting exitCode === 0 and that restoreAccountsFromBackupMock, confirmMock,
saveAccountsMock, and promptLoginModeMock were not called; place this alongside
the existing test that uses
selectMock.mockResolvedValueOnce("restore-backup")/("manual")/null so you cover
the alternate branch where restore mode itself yields "back" early.
- Around line 3112-3156: This test is non-deterministic because it doesn't force
TTY detection; ensure the interactive onboarding branch is exercised by stubbing
the process TTY flags before invoking runCodexMultiAuthCli: set
process.stdin.isTTY = true (and process.stdout.isTTY = true if your CLI checks
it) prior to importing or calling runCodexMultiAuthCli, and restore the original
values after the test; update the test that references runCodexMultiAuthCli,
promptLoginModeMock, loadAccountsMock and getNamedBackupsMock so the TTY stub is
in place for the duration of the test (and cleaned up) to deterministically
trigger the onboarding interactive path.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4433-4438: The catch block for StorageError is printing a hint for
the wrong path (using selectedBackup.path) — change the call to use the
StorageError's own hint/path (e.g., use the StorageError instance's properties
or call formatStorageErrorHint with the error's destination/hint rather than
selectedBackup.path) so the message points at the actual failed write (the
saveAccounts destination); update the catch to cast error to StorageError and
pass the error's path/hint into formatStorageErrorHint (or print error.hint
directly), then remove the now-unused formatStorageErrorHint import from the top
of the file; add a small vitest that simulates persistence failure in
saveAccounts to assert the code logs the storage error hint for the failing
destination.
In `@lib/storage.ts`:
- Around line 878-893: The TOCTOU comes from calling fs.realpath() to validate
the backup path then later calling loadAccountsFromPath(path); fix by collapsing
validation+read: open the target with fs.promises.open using flags
fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW (or open the parent dir and open
the entry by name without following symlinks), read the file contents from the
returned FileHandle (don't re-resolve the pathname), and pass the buffer/string
into a new loadAccountsFromBuffer/loadAccountsFromFd variant (or extend
loadAccountsFromPath to accept a Buffer/FileHandle) so the read happens from the
guarded open; keep the existing getNamedBackupRoot/resolvedBackupRoot check by
realpath-ing the resolvedBackupRoot once and ensure you only accept files opened
via the non-following FileHandle, then throw if opening fails or the handle
indicates the entry is outside the root; add a vitest alongside
test/storage-last-backup.test.ts (covering lines ~258-276) that swaps the backup
symlink between validation and read and assert the operation is rejected; ensure
logging changes avoid leaking tokens/emails and that new queue/error paths
handle EBUSY/429 per guidelines.
🪄 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: 3be4dcb6-c23d-44ec-b8ff-04475a340d07
📒 Files selected for processing (5)
docs/getting-started.mdlib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage-last-backup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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
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-last-backup.test.tstest/codex-manager-cli.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.tslib/codex-manager.ts
🔇 Additional comments (2)
test/storage-last-backup.test.ts (1)
118-170: good regression coverage on disappearing backups and non-persist restore.
test/storage-last-backup.test.ts:118-236covers thefs.statrace fallback and thepersist: falserestore path with deterministic filesystem setup. that is the right shape of vitest coverage for this onboarding change.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.Also applies to: 173-236
test/codex-manager-cli.test.ts (1)
3257-3341: good coverage for windows ebusy and hardened restore-failure behavior.the cases at
test/codex-manager-cli.test.ts:3257-3341are solid and directly exercise windows-styleebusyplus persistence failure without killing the onboarding session.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
test/storage-last-backup.test.ts (1)
297-315:⚠️ Potential issue | 🟡 Minorthis still doesn't pin the realpath escape check.
test/storage-last-backup.test.ts:297passes a path that already lives outside the managed root, so the assertion would still pass iflib/storage.ts:878-905stopped resolving realpaths. the security-sensitive regression here is a symlink, or a win32 junction, inside the managed backup tree that resolves outside it. add that case so the containment guard is actually covered.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.test/codex-manager-cli.test.ts (1)
3229-3302:⚠️ Potential issue | 🟡 Minorassert the onboarding sync warning when codex sync returns false.
test/codex-manager-cli.test.ts:3229-3302proves the restored pool is persisted, but it never checks the warn path behindsetCodexCliActiveSelectionMock.mockResolvedValueOnce(false)inlib/codex-manager.ts:4401-4450. if that warning regresses, this test still passes.suggested fix
+ const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); const exitCode = await runCodexMultiAuthCli(["auth", "login"]); expect(exitCode).toBe(0); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("Codex auth sync did not complete"), + ); expect(restoreAccountsFromBackupMock).toHaveBeenCalledWith( "/mock/backups/warn.json", { persist: false },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 3229 - 3302, The test currently verifies persistence but doesn't assert the onboarding-sync warning path triggered when setCodexCliActiveSelectionMock.mockResolvedValueOnce(false) is returned; update the test for runCodexMultiAuthCli to spy/mock the logger or the specific warning helper used in the code (e.g., processLogger.warn or showOnboardingSyncWarning) before invoking runCodexMultiAuthCli, then add an assertion that the warning helper was called with the expected sync/onboarding message after the call (use expect(...).toHaveBeenCalledWith/CalledTimes matching the warning text or expect.stringContaining('sync' / 'onboard')). Ensure you reference the same mocks used in the test (setCodexCliActiveSelectionMock, promptLoginModeMock, selectMock) and place the new assertion alongside the existing expects that check restoreAccountsFromBackupMock and saveAccountsMock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started.md`:
- Around line 49-52: Update the docs/getting-started.md option text to read
"valid named backup" and add a short clarifier that the "Recover saved accounts"
menu entry only appears for backups that successfully parse and contain at least
one account (i.e., the filtering logic in lib/storage.ts that discards
unreadable or empty backup files); keep the existing mention of the storage root
override but ensure the phrasing matches this behavior so users don't expect
entries for empty or invalid backup files.
In `@lib/codex-manager.ts`:
- Around line 1144-1152: Add a Vitest unit test for the function
formatBackupSavedAt so its user-facing output is asserted and guarded against
regression: create a test (e.g., codex-manager.spec.ts) that imports
formatBackupSavedAt, stubs Date.prototype.toLocaleString (or uses vi.spyOn) to
return a deterministic string for the provided options, calls
formatBackupSavedAt with a fixed mtimeMs value, and asserts the returned string
matches the expected formatted output; ensure the stub is restored after the
test so other tests are unaffected.
In `@lib/storage.ts`:
- Around line 169-170: The sort comparator in candidates.sort currently orders
only by mtimeMs and is non-deterministic when mtimes tie; update the comparator
to add a stable secondary key (e.g., compare file name such as entry.name or
entry.filename lexicographically) so ties are deterministically ordered (keep
primary mtimeMs descending, then secondary name ascending/descending as desired)
in the candidates.sort call, and add/update the vitest at
test/storage-last-backup.test.ts (lines ~74-116) to assert deterministic order
for entries with identical mtimes; also ensure any related queue logic touched
by this change continues to handle EBUSY/429 retry semantics per lib/**
guidelines.
In `@test/codex-manager-cli.test.ts`:
- Around line 3554-3625: The test relies on a fixed getNamedBackupsMock payload
so a regression that re-fetches backups in the manual-restore branch could slip
by; update the test to pin single-discovery behavior by either asserting
getNamedBackupsMock was called exactly once after
runCodexMultiAuthCli(["auth","login"]) or make getNamedBackupsMock return a
different list on a second call (use mockResolvedValueOnce / mockResolvedValue)
so any additional fetch would fail the expectation; reference
getNamedBackupsMock and runCodexMultiAuthCli (and the manual restore branch in
lib/codex-manager.ts) when making the change.
- Around line 3304-3334: The test currently only asserts exitCode and the error
log; change it to also assert that the interactive menu continued after the
EBUSY failure by verifying the queued "cancel" selection was actually consumed —
e.g., after calling runCodexMultiAuthCli(["auth","login"]) assert on selectMock
usage (reference selectMock and runCodexMultiAuthCli) such as
expect(selectMock).toHaveBeenCalledTimes(3) or
expect(selectMock).toHaveBeenLastCalledWith("cancel") so the test fails if the
CLI returned early instead of processing the final menu choice.
In `@test/storage-last-backup.test.ts`:
- Around line 173-204: The tests for restoreAccountsFromBackup and the reload
path are only asserting activeIndex but not activeIndexByFamily, so add
assertions to both the returned "restored" object and the reloaded "loaded"
result to verify activeIndexByFamily was preserved; update the test cases that
call restoreAccountsFromBackup, loadAccounts and saveAccounts to expect the same
activeIndexByFamily map (e.g., { codex: 1 }) on both
restored.activeIndexByFamily and loaded.activeIndexByFamily to prevent
regressions where the per-family selection is dropped.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 3229-3302: The test currently verifies persistence but doesn't
assert the onboarding-sync warning path triggered when
setCodexCliActiveSelectionMock.mockResolvedValueOnce(false) is returned; update
the test for runCodexMultiAuthCli to spy/mock the logger or the specific warning
helper used in the code (e.g., processLogger.warn or showOnboardingSyncWarning)
before invoking runCodexMultiAuthCli, then add an assertion that the warning
helper was called with the expected sync/onboarding message after the call (use
expect(...).toHaveBeenCalledWith/CalledTimes matching the warning text or
expect.stringContaining('sync' / 'onboard')). Ensure you reference the same
mocks used in the test (setCodexCliActiveSelectionMock, promptLoginModeMock,
selectMock) and place the new assertion alongside the existing expects that
check restoreAccountsFromBackupMock and saveAccountsMock.
🪄 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: dffa8e39-9b6f-4639-8980-93d47d1e0842
📒 Files selected for processing (5)
docs/getting-started.mdlib/codex-manager.tslib/storage.tstest/codex-manager-cli.test.tstest/storage-last-backup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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
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-last-backup.test.tstest/codex-manager-cli.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.tslib/codex-manager.ts
🔇 Additional comments (12)
test/codex-manager-cli.test.ts (3)
9-10: good restore test scaffolding.
test/codex-manager-cli.test.ts:9-10,23,106-123,175-177,440-457,531-535wires the new backup and confirm mocks into the suite cleanly. the resets plusrestoreTTYDescriptors()keep the onboarding cases deterministic and isolated.Also applies to: 23-23, 106-123, 175-177, 440-457, 531-535
2966-3094: good coverage on the new onboarding restore branches.
test/codex-manager-cli.test.ts:2966-3227pins the single-save latest-restore path, localized timestamp hint, restore-mode back, manual-picker back, existing-pool skip, and discovery fallback againstlib/codex-manager.ts:4273-4330.Also applies to: 3096-3227
3336-3427: good failure-path and opt-out coverage around restore.
test/codex-manager-cli.test.ts:3336-3552locks downStorageErrorhinting, pre-save failure handling, confirm rejection, and keeping manual refresh reachable. that giveslib/codex-manager.ts:4273-4450much better regression protection.Also applies to: 3429-3552
lib/codex-manager.ts (9)
4358-4366: backup discovery is correctly hoisted — looks good.capturing
namedBackupsonce before the inner loop and reusing the same snapshot for both"latest"and"manual"branches addresses the staleness asymmetry flagged earlier. the try/catch fallback to empty array is defensive and appropriate.
4553-4559:preserveActiveIndexByFamilylogic is correct.the condition
!preserveActiveIndexByFamily || !storage.activeIndexByFamilyensures the family map is only overwritten when the flag is false or the map is missing. when restoring a backup withpreserveActiveIndexByFamily: true, the original per-family selections remain intact.
4218-4227: refresh flow now prompts for sign-in mode — looks good.decoupling mode selection from
runOAuthFlowkeeps the restore-backup path out of the refresh action. the early return forsignInMode !== "browser" && signInMode !== "manual"is safe since"restore-backup"is only offered whenbackupOptionis non-null (passed asnullhere).
56-63: new imports align with backup restore feature — no concerns.
getNamedBackups,restoreAccountsFromBackup,StorageError,NamedBackupSummary, andformatStorageErrorHintare all used in the new onboarding restore flow. no unused imports detected.
1209-1253:promptBackupRestoreModehandles non-TTY gracefully — looks good.falling back to
"latest"when not interactive is a sensible default for CI/headless scenarios. the hotkey mapping (1→ latest,2→ manual,q→ back) is consistent with other prompts.
1255-1289:promptManualBackupSelectionhandles empty list edge.if
backupsis empty,backups[0] ?? nullreturnsnullin non-TTY mode, and the caller atlib/codex-manager.ts:4386-4388continues the loop. this is safe.
1550-1553:runOAuthFlowsignature change decouples mode prompting.moving
signInModeto a parameter simplifies the flow and enables the caller to decide the mode (including restore-backup branching). no regression risk since all callers now pass the mode explicitly.
4407-4418: regression test for per-family selection preservation during restore exists — comment accurate.the restore flow test at
test/codex-manager-cli.test.ts:2969explicitly exercises the scenario: restoredStorage hasactiveIndex: 0andactiveIndexByFamily: { codex: 1 }(lines 2987-2988), then verifies post-restore that both values are correctly persisted (lines 3075-3079). thepersistAndSyncSelectedAccountcall withpreserveActiveIndexByFamily: true(lib/codex-manager.ts:4417) correctly skips the reset logic at lines 4554-4559, keeping the backup's per-family map intact.
4431-4443: error handling correctly gates hint on StorageError.the catch block checks
error instanceof StorageErrorbefore callingformatStorageErrorHint, preventing misleading write-error hints for read-side failures (ENOENT, parse errors, etc.). the fallback console.error path handles other errors cleanly.the toctou edge case where a backup file disappears between selection and
restoreAccountsFromBackupis already covered. test/storage-last-backup.test.ts:118 tests the "disappears before stat" scenario and line 258 tests "disappears before restore realpath" — both explicitly set error.code = "ENOENT" and verify the fallback path. windows-specific file locking (EBUSY) is tested in test/codex-manager-cli.test.ts:3318 and 3355 with rejection handling and retry logic.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/upgrade.md`:
- Line 73: The docs contain inconsistent escaping for the Windows backup path:
replace the double-backslash form `%CODEX_MULTI_AUTH_DIR%\\backups` found in
docs/upgrade.md with the single-backslash form `%CODEX_MULTI_AUTH_DIR%\backups`
to match docs/getting-started.md (standardize this single-backslash
representation across all documentation files mentioning the backup path).
In `@lib/codex-manager.ts`:
- Around line 4362-4368: The catch for getNamedBackups() in lib/codex-manager.ts
currently swallows all errors and sets namedBackups = [], which hides real
discovery failures (e.g. EPERM); update the catch to inspect error.code and only
treat ENOENT as “no backups” (set namedBackups = []), while for other codes
(EPERM, EACCES, etc.) rethrow or surface the error (or log and propagate) so the
restore option isn’t silently dropped; then add a vitest in
test/codex-manager-cli.test.ts that stubs/mocks getNamedBackups() to throw an
Error with code 'EPERM' and asserts the restore discovery fails (or that the
error is propagated) rather than returning an empty list.
- Around line 4358-4373: The loop captures existingCount and latestNamedBackup
once causing a stale "empty onboarding" state after an account is added; update
the loop so that after a successful account add you clear the empty-onboarding
snapshot or refresh refreshedStorage/namedBackups (re-run getNamedBackups and
recompute existingCount/latestNamedBackup or explicitly unset the
empty-onboarding marker) before the next promptOAuthSignInMode call (symbols:
refreshedStorage, existingCount, getNamedBackups, latestNamedBackup,
promptOAuthSignInMode and the account-add path around lines ~4474-4485). Also
add a vitest in test/codex-manager-cli.test.ts that starts with empty storage,
performs an add one account + "add another" flow, and asserts that restore is no
longer offered and that cancel returns to the main menu to verify the fix.
In `@lib/storage.ts`:
- Around line 878-932: Add a Windows-specific regression test in
test/storage-last-backup.test.ts that validates restoreAccountsFromBackup's
realpath-based containment logic for UNC and cross-drive scenarios: create a
backup root (getNamedBackupRoot) and then craft both an allowed UNC path inside
that root and a disallowed UNC or cross-drive path outside it, call
restoreAccountsFromBackup(path, { persist: true|false }) and assert the inside
path succeeds (loads normalized accounts and optionally persists via
saveAccounts) while the outside path throws the containment Error; make the test
skip on non-Windows/unsupported environments and reuse
loadAccountsFromPath/saveAccounts mocks to avoid touching the real FS.
Optionally add a separate test that simulates concurrent restore EBUSY/429 to
drive adding retry logic to restoreAccountsFromBackup if you plan to implement
retries.
In `@test/storage-last-backup.test.ts`:
- Around line 377-391: The fs.readFile spy only checks String(path) ===
backupPath so it misses the canonicalized path used after fs.realpath; before
installing the spy call the real fs.realpath on backupPath to get
resolvedBackupPath and update the spy to throw ENOENT when path equals either
backupPath or resolvedBackupPath; adjust the mockImplementation in the test
(around readFileSpy creation) so it compares against both values and still
delegates to originalReadFile otherwise, ensuring loadAccountsFromPath and its
fs.readFile call hit the ENOENT branch across platforms.
🪄 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: 9bdd954b-74c2-43b9-b25f-4f05ed57533b
📒 Files selected for processing (9)
docs/getting-started.mddocs/upgrade.mdlib/accounts.tslib/codex-manager.tslib/schemas.tslib/storage.tslib/storage/migrations.tstest/codex-manager-cli.test.tstest/storage-last-backup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/upgrade.mddocs/getting-started.md
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/schemas.tslib/accounts.tslib/storage/migrations.tslib/storage.tslib/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/codex-manager-cli.test.tstest/storage-last-backup.test.ts
🔇 Additional comments (25)
lib/schemas.ts (1)
85-91: lgtm —"restore"added to the schema enum.this aligns with the storage layer and
ManagedAccounttype expansion. the narrower signature onmarkSwitchedatlib/accounts.ts:573is intentional since restore reasons are assigned viapersistAndSyncSelectedAccount, not the rotation path.lib/accounts.ts (1)
95-100: type expansion is consistent with schema and storage layer.
markSwitchedatlib/accounts.ts:573intentionally keeps a narrower union since restore reasons are set outside the rotation path. no action needed.lib/storage/migrations.ts (2)
28-33: type expansion for v1 metadata is consistent.aligns with
SwitchReasonSchemaandManagedAccountchanges. migration logic atlib/storage/migrations.ts:102passeslastSwitchReasonthrough unchanged, which remains correct.
58-63: type expansion for v3 metadata is consistent.matches the schema and accounts layer. no runtime changes needed in
migrateV1ToV3.docs/getting-started.md (1)
46-60: onboarding restore flow documented correctly.steps match the cli behavior in
lib/codex-manager.ts— empty pool routes to sign-in menu,Recover saved accountsappears only when valid named backups exist, and the newest-first picker is documented. the filtering behavior at line 57 matcheslib/storage.ts:141-147.lib/storage.ts (2)
118-175: named backup discovery looks solid.
collectNamedBackupshandles the key edge cases:
lib/storage.ts:135-136returns[]for missing backup root (ENOENT), re-throws EPERM/EACCES.lib/storage.ts:169-173sorts bymtimeMsdescending withfileNameas tiebreaker for determinism on windows/coarse-timestamp filesystems.lib/storage.ts:155-164logs failures at debug level without leaking sensitive account data.the
.jsoncheck at line 142 usestoLowerCase()which handles mixed-case extensions on case-insensitive filesystems.
2188-2189: type guard updated to match schema.
isSwitchReasonnow validates all five values including"restore", consistent withlib/schemas.ts:85-91.test/codex-manager-cli.test.ts (18)
9-10: lgtm on mock setup.mock additions for
getNamedBackupsMock,restoreAccountsFromBackupMock,confirmMock, and logger methods follow the existing test patterns. the defaultconfirmMockreturningtruealigns with the common happy-path scenario.Also applies to: 23-23, 33-43
179-181: lgtm.
444-445: proper test isolation.resetting
getNamedBackupsMock,restoreAccountsFromBackupMock, andconfirmMockinbeforeEachwith sensible defaults (empty backups, confirm=true) ensures each test starts from a clean state.Also applies to: 460-461, 535-535
546-564: nice deterministic locale test.spying on
Date.prototype.toLocaleStringand verifying the exact options passed ensures the formatter uses the intended locale settings without depending on the test runner's locale.
2990-3118: solid restore happy-path coverage.the test verifies the full restore flow: menu presentation, hint formatting, confirm dialog,
restoreAccountsFromBackupwithpersist: false, storage persistence viawithAccountStorageTransaction, and codex cli sync. the locale spy in try/finally ensures cleanup.one minor note:
restoreAccountsFromBackupMock.mockResolvedValue(...)attest/codex-manager-cli.test.ts:3055could bemockResolvedValueOncesince restore is expected to be called exactly once, but since the test assertsrestoreAccountsFromBackupMockcall count implicitly through the flow, this is acceptable.
3120-3145: good regression for back navigation.addresses the prior review request for testing the restore-mode "back" path. confirms no side effects occur when user backs out of the restore menu.
3147-3179: manual picker back-out covered.verifies that returning
nullfrom the backup picker correctly aborts the restore flow without callingrestoreAccountsFromBackuporsaveAccounts.
3181-3227: correctly skips restore when accounts exist.the test ensures
getNamedBackupsMockis never called when the account pool is non-empty, which matches the pr objective that restore appears only during onboarding with an empty pool.
3229-3255: good graceful degradation test.verifies that when
getNamedBackupsthrows (e.g., EACCES on windows), the cli falls back to normal sign-in without crashing and logs the error at debug level. this addresses the greptile comment about non-ENOENT errors being silently swallowed—the test confirmsloggerDebugMockis called with the error context attest/codex-manager-cli.test.ts:3251-3254.
3257-3334: sync failure warning path verified.addresses the prior review request for testing when
setCodexCliActiveSelectionMockreturnsfalse. the test confirms the restored pool is still persisted locally and the warning is logged, matching therestoreBackupSyncWarningcopy behavior.
3336-3369: ebusy menu survival regression is solid.uses a windows-style path
C:/mock/backups/locked.jsonand verifiesselectMockis called 3 times, confirming the cli session survives theEBUSYfailure and processes the queued"cancel"selection. this addresses the prior review concern attest/codex-manager-cli.test.ts:3367.
3371-3408: storage error hint deduplication verified.verifies that
StorageErrorwith a hint is printed exactly once viaformatStorageErrorHint, preventing duplicate or confusing error output.
3410-3462: persistence failure keeps session alive.verifies that when
saveAccountsfails after a successful restore, the cli logs the error and returns to the onboarding menu rather than crashing or proceeding with stale state.setCodexCliActiveSelectionMocknot being called confirms the flow didn't complete.
3464-3490: confirmation rejection path covered.addresses the prior review request for testing when
confirmMockreturnsfalse. verifies the restore is cleanly skipped without callingrestoreAccountsFromBackuporsaveAccounts.
3492-3523: manual picker confirmation rejection covered.verifies the manual backup selection path also respects user declining the confirmation dialog.
3525-3587: existing refresh path preserved.good sanity check that the manual refresh flow from the existing-account menu still works alongside the new restore functionality.
3589-3661: single-discovery pinned in manual picker test.
expect(getNamedBackupsMock).toHaveBeenCalledTimes(1)attest/codex-manager-cli.test.ts:3647ensures the backup discovery is hoisted and not re-fetched when entering the manual picker branch, addressing the prior review concern.
3041-3054: the tied-mtime regression test already exists attest/storage-last-backup.test.ts:118. it sets identical mtimes and verifies deterministic filename-based ordering with localeCompare. the cli test correctly uses distinct mtimes (nowvsnow - 60_000) for its specific case; the storage layer handles ties atlib/storage.ts:170-171.
|
@greptile @coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
docs/getting-started.md (1)
53-54:⚠️ Potential issue | 🟡 Minorpoint the docs at the actual menu action.
docs/getting-started.md:53-54says users chooseRecover saved accounts, buttest/codex-manager-cli.test.ts:3072-3085shows that string is the heading. the selectable row isRestore Saved Backup. right now the steps send users to a non-actionable label.proposed fix
- - `Recover saved accounts` when the current pool is empty and at least one valid named backup exists under `~/.codex/multi-auth/backups` by default, or under `%CODEX_MULTI_AUTH_DIR%\backups` if you override the storage root with `CODEX_MULTI_AUTH_DIR` -3. If you choose `Recover saved accounts`, the next menu lets you either: + - `Restore Saved Backup` under the `Recover saved accounts` heading when the current pool is empty and at least one valid named backup exists under `~/.codex/multi-auth/backups` by default, or under `%CODEX_MULTI_AUTH_DIR%\backups` if you override the storage root with `CODEX_MULTI_AUTH_DIR` +3. If you choose `Restore Saved Backup`, the next menu lets you either:As per coding guidelines,
docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started.md` around lines 53 - 54, Update the documentation to point at the actual actionable CLI menu label: change the user-facing phrase "Recover saved accounts" to the selectable row text "Restore Saved Backup" in the getting-started docs so the steps map to the real menu item; ensure any nearby descriptive text and the sequence that follows still read correctly (the test reference in test/codex-manager-cli.test.ts shows "Recover saved accounts" is a heading, not the selectable action, so align the doc string with "Restore Saved Backup").lib/codex-manager.ts (1)
1156-1208:⚠️ Potential issue | 🟠 Majorkeep backup discovery failures visible across the next prompt render.
lib/codex-manager.ts:4368-4377only emits a transientconsole.warnwhengetNamedBackups()fails, but the next onboarding prompt atlib/codex-manager.ts:4383goes throughpromptOAuthSignInMode()withclearScreen: trueatlib/codex-manager.ts:1194. in the interactive path that still degradesEPERM/EACCESinto the same user experience as "no backups found", because the warning is wiped before the user can act on it. keep this state in the prompt subtitle/help or surface a durable error banner instead of clearing it away. please add a vitest intest/codex-manager-cli.test.tsthat forcesgetNamedBackups()to throwEPERMand asserts the failure stays visible after the next prompt render. 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: 4360-4379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 1156 - 1208, The transient backup-discovery warning from getNamedBackups() is being cleared by the next interactive select (promptOAuthSignInMode -> select with clearScreen:true); update the flow so the error is persisted into the prompt UI instead of a console.warn (e.g., return or attach an error string to the prompt's subtitle/help or pass an error/banner item into the MenuItem list used by promptOAuthSignInMode) and ensure promptOAuthSignInMode consumes that persisted error (check the select call options: clearScreen: true, subtitle/help, and onInput). Add a Vitest in test/codex-manager-cli.test.ts that stubs getNamedBackups() to throw an EPERM and asserts the resulting prompt still displays the error message after render (use the promptOAuthSignInMode/select hooks or captured subtitle text), cite/adjust any affected tests, and avoid logging sensitive data (tokens/emails) while adding durable error handling for EPERM/EACCES/EBUSY/429 scenarios.test/storage-last-backup.test.ts (1)
470-521:⚠️ Potential issue | 🟠 Majorgate the unc regression to win32 or it stops testing the intended branch.
test/storage-last-backup.test.ts:470-521drives windows UNC strings throughlib/storage.ts:912-916, which uses platform-nativenode:path. on non-windows runners,relative()andisAbsolute()do not treat\\\\server\\share\\...as a windows UNC path, so this test can fail or give a false signal while never exercising the real windows containment behavior. skip it onprocess.platform !== "win32"or move the assertion behind a win32-scoped helper. 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.#!/bin/bash node - <<'NODE' const path = require('node:path'); const from = '\\\\server\\share\\codex\\backups'; const inside = '\\\\server\\share\\codex\\backups\\backup-unc-inside.json'; console.log({ platform: process.platform }); console.log({ relative: path.relative(from, inside), isAbsoluteFrom: path.isAbsolute(from), isAbsoluteInside: path.isAbsolute(inside), }); NODE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage-last-backup.test.ts` around lines 470 - 521, The UNC-path test "accepts UNC-resolved backups inside the managed root and rejects cross-share escapes" exercises Windows-specific path behavior but runs on non-Windows runners; guard it so it only executes on win32: at the start of that test (the one that calls buildNamedBackupPath and restoreAccountsFromBackup and mocks fs.realpath/fs.readFile) add a platform gate using Vitest (e.g., if (process.platform !== "win32") { test.skip(); } or an early return/skip) so the UNC strings and node:path semantics are only exercised on Windows, leaving the rest of the test suite deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 3098-3104: The test only asserts active indices but must also
assert the persisted lastSwitchReason equals "restore"; update the expectation
on the saveAccountsMock call (the argument inspected at
saveAccountsMock.mock.calls[0]?.[0]) to include an assertion for
lastSwitchReason: "restore" (e.g., extend the objectContaining to include
lastSwitchReason) so changes in lib/schemas.ts, lib/accounts.ts, and
lib/storage/migrations.ts that add the "restore" literal are validated by the
test.
- Line 3746: The manual-picker single-attempt restore test is still asserting
two getNamedBackups() calls, allowing a second scan; change the assertion in the
single-attempt manual restore flow to expect one call to getNamedBackupsMock (so
the manual picker is pinned to a single discovery), leaving the separate
refresh-on-reopen test unchanged; locate the assertion using the symbol
getNamedBackupsMock in the single-attempt manual restore test and update the
toHaveBeenCalledTimes(2) → toHaveBeenCalledTimes(1).
In `@test/storage-last-backup.test.ts`:
- Around line 524-542: The test never ensures the named-backup root exists, so
the call to restoreAccountsFromBackup hits the "missing-root" branch instead of
the containment guard; before calling
restoreAccountsFromBackup(escapedBackupPath) create the parent directory for
buildNamedBackupPath("backup-inside-root") (i.e.,
mkdir(dirname(buildNamedBackupPath(...)), { recursive: true })) so the
named-backup root is present and the containment check in lib/storage.ts
(restoreAccountsFromBackup) is exercised deterministically under vitest.
---
Duplicate comments:
In `@docs/getting-started.md`:
- Around line 53-54: Update the documentation to point at the actual actionable
CLI menu label: change the user-facing phrase "Recover saved accounts" to the
selectable row text "Restore Saved Backup" in the getting-started docs so the
steps map to the real menu item; ensure any nearby descriptive text and the
sequence that follows still read correctly (the test reference in
test/codex-manager-cli.test.ts shows "Recover saved accounts" is a heading, not
the selectable action, so align the doc string with "Restore Saved Backup").
In `@lib/codex-manager.ts`:
- Around line 1156-1208: The transient backup-discovery warning from
getNamedBackups() is being cleared by the next interactive select
(promptOAuthSignInMode -> select with clearScreen:true); update the flow so the
error is persisted into the prompt UI instead of a console.warn (e.g., return or
attach an error string to the prompt's subtitle/help or pass an error/banner
item into the MenuItem list used by promptOAuthSignInMode) and ensure
promptOAuthSignInMode consumes that persisted error (check the select call
options: clearScreen: true, subtitle/help, and onInput). Add a Vitest in
test/codex-manager-cli.test.ts that stubs getNamedBackups() to throw an EPERM
and asserts the resulting prompt still displays the error message after render
(use the promptOAuthSignInMode/select hooks or captured subtitle text),
cite/adjust any affected tests, and avoid logging sensitive data (tokens/emails)
while adding durable error handling for EPERM/EACCES/EBUSY/429 scenarios.
In `@test/storage-last-backup.test.ts`:
- Around line 470-521: The UNC-path test "accepts UNC-resolved backups inside
the managed root and rejects cross-share escapes" exercises Windows-specific
path behavior but runs on non-Windows runners; guard it so it only executes on
win32: at the start of that test (the one that calls buildNamedBackupPath and
restoreAccountsFromBackup and mocks fs.realpath/fs.readFile) add a platform gate
using Vitest (e.g., if (process.platform !== "win32") { test.skip(); } or an
early return/skip) so the UNC strings and node:path semantics are only exercised
on Windows, leaving the rest of the test suite deterministic.
🪄 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: 3d2e115b-169d-4198-810c-b498a6f091ac
📒 Files selected for processing (9)
docs/getting-started.mddocs/upgrade.mdlib/accounts.tslib/codex-manager.tslib/schemas.tslib/storage.tslib/storage/migrations.tstest/codex-manager-cli.test.tstest/storage-last-backup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/upgrade.mddocs/getting-started.md
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/accounts.tslib/storage/migrations.tslib/schemas.tslib/storage.tslib/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-last-backup.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (1)
lib/storage.ts (1)
125-184: good hardening on discovery and restore paths.
lib/storage.ts:125-184now keeps named-backup discovery deterministic and race-tolerant, andlib/storage.ts:887-941gates restore through realpath containment while letting the caller own the single write withpersist: false.test/storage-last-backup.test.ts:68-468,544-609covers the empty-root, tie-break, disappearing-file, persist-split, and path-escape cases.Also applies to: 887-941
1 participant
Summary
Validation
npm test -- --run test/codex-manager-cli.test.ts test/storage-last-backup.test.ts test/documentation.test.tsnpm run typechecknpm run lintnpm run buildnote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr adds an onboarding backup restore path to
codex auth login: when the active account pool is empty and at least one valid named backup exists, the sign-in menu gains a "restore saved backup" option that lets the user load the latest backup automatically or pick from a sorted list. the feature hooks intorunAuthLogin's existing inner loop, addsgetNamedBackups/restoreAccountsFromBackuptolib/storage.tswith realpath-based containment checks and enoent guards, and extendspersistAndSyncSelectedAccountwith apreserveActiveIndexByFamilyflag to keep per-family routing from the restored data when internally consistent.lib/storage.ts—collectNamedBackupsuses stat-before-load + stat-after-load race detection;restoreAccountsFromBackupguardsrealpath(backupRoot)andrealpath(path)separately and wrapsloadAccountsFromPathin an enoent handler, preventing raw node errors from reaching the user on windows when antivirus locks or deletes a file between scan and restore.lib/codex-manager.ts—promptOAuthSignInModenow accepts aNamedBackupSummary | nulland a discovery warning; the restore-backup branch in the inner loop always callsconfirm()before writing, runsrestoreAccountsFromBackupwithpersist: falseand commits a singlesaveAccountswrite viapersistAndSyncSelectedAccount, and wrapsrunActionPanelin a try/catch that keeps the session alive on windows ebusy/eperm failures.onboardingBackupDiscoveryWarningis not reset alongsidenamedBackups = []after a successful oauth login, leaving a stale "backup discovery failed" subtitle visible in the add-more-accounts flow for users who hit a prior eperm on the backup dir. fix is a one-liner:onboardingBackupDiscoveryWarning = nullat the same site.lastSwitchReasonacrosslib/schemas.ts,lib/accounts.ts,lib/storage/migrations.ts, and thenormalizeFlaggedStorageguard.Confidence Score: 4/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User participant CM as codex-manager (runAuthLogin) participant ST as storage (getNamedBackups) participant FS as filesystem participant RB as restoreAccountsFromBackup CM->>ST: loadNamedBackupsForOnboarding() ST->>FS: readdir(backupRoot) FS-->>ST: entries[] loop each .json entry ST->>FS: stat(candidatePath) [statsBefore] ST->>FS: loadAccountsFromPath(candidatePath) ST->>FS: stat(candidatePath) [statsAfter — race check] end ST-->>CM: NamedBackupSummary[] sorted by mtime desc CM->>U: promptOAuthSignInMode(latestBackup, warning?) U-->>CM: "restore-backup" CM->>U: promptBackupRestoreMode(latestBackup) U-->>CM: "latest" | "manual" alt manual CM->>U: promptManualBackupSelection(backups[]) U-->>CM: selectedBackup end CM->>U: confirm("Load fileName (N accounts)?") U-->>CM: true CM->>RB: restoreAccountsFromBackup(path, {persist:false}) RB->>FS: realpath(backupRoot) — ENOENT guard RB->>FS: realpath(path) — ENOENT guard RB->>RB: containment check (relative path) RB->>FS: loadAccountsFromPath(resolvedPath) — ENOENT guard RB-->>CM: AccountStorageV3 CM->>CM: resolveActiveIndex(storage) CM->>CM: persistAndSyncSelectedAccount({switchReason:"restore"}) CM->>FS: saveAccounts(storage) [single write] CM->>FS: setCodexCliActiveSelection(account) CM-->>U: "Loaded fileName (N accounts)." CM->>CM: continue loginFlow → dashboardComments Outside Diff (2)
lib/codex-manager.ts, line 496-509 (link)activeIndexByFamilyentries not bounds-checked when preservedshouldPreserveActiveIndexByFamilyis computed beforestorage.activeIndex = targetIndex, so it correctly compares the original backupactiveIndexagainsttargetIndex. when they match — meaningresolveActiveIndexleft the active index unchanged because the backup's index was already valid — the entireactiveIndexByFamilymap from the backup is written to disk without checking whether any of its values are withinstorage.accounts.length.concretely: a backup with
activeIndex: 1(valid, 2 accounts) butactiveIndexByFamily: { codex: 5 }(out of bounds) passes thetargetIndex === storage.activeIndexguard, skips the family-routing update block, and silently persistscodex: 5. downstream routing code that readsactiveIndexByFamily.codexwould then land on a non-existent account — windows token-safety risk if the wrong slot gets refreshed.no vitest case in
test/storage-last-backup.test.tscovers a restore whereactiveIndexis valid butactiveIndexByFamilyhas an out-of-bounds entry (the existing "realigns restored family routing" test only covers the stale-activeIndexpath wheretargetIndex !== storage.activeIndex).suggest adding a bounds clamp before the preserve block:
lib/codex-manager.ts, line 4509 (link)stale
onboardingBackupDiscoveryWarningshown after successful oauth loginnamedBackups = []is cleared here after a successful oauth login, butonboardingBackupDiscoveryWarningis not. if backup discovery previously failed withEPERMand the user then signs in via browser/manual and chooses to add another account, the inner loop re-enters,latestNamedBackupresolves tonull(correct), butpromptOAuthSignInMode(null, onboardingBackupDiscoveryWarning)still injects the stale"Named backup discovery failed…"string into the subtitle — even though the user already has an account.the warning is only cleared via
loadNamedBackupsForOnboarding(), which is not called on this path:no vitest case in
test/codex-manager-cli.test.tscovers discovery-fails → oauth-success → add-another, so this path has zero regression coverage. worth adding a test that asserts the subtitle of the second sign-in prompt has no warning text.Prompt To Fix All With AI
Last reviewed commit: "fix(restore): clamp ..."