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:
📝 WalkthroughWalkthroughhealth summary is assembled from storage and sync/doctor state and passed into the multi-account login menu as an optional read-only row. changes thread Changes
Sequence DiagramsequenceDiagram
participant client as client
participant cli as lib/cli.ts
participant codex as lib/codex-manager.ts
participant storage as storage
participant ui as lib/ui/auth-menu.ts
client->>cli: start login flow
cli->>codex: runAuthLogin()
codex->>storage: read sync / doctor / backups / rollback state
storage-->>codex: return states
note over codex: buildLoginMenuHealthSummary() aggregates states
codex->>cli: promptLoginMode(options + healthSummary)
cli->>ui: showAuthMenu(healthSummary)
ui-->>client: render health summary row + accounts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes suggested labels
notes and review 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}
There was a problem hiding this comment.
Pull request overview
Hardens the interactive login/dashboard “health summary” so sync/restore/rollback/doctor state can be surfaced in the auth menu without expanding retention/recovery logic.
Changes:
- Adds a read-only “Health Summary” row to the auth menu UI and copy.
- Builds a consolidated health-summary string during
auth loginfrom sync, restore, rollback, and read-only doctor signals. - Updates/extends CLI tests to cover the new health-summary behavior and updated restore checks.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/codex-manager-cli.test.ts | Updates expectations and adds coverage for the new login menu health summary and UI rendering. |
| lib/ui/copy.ts | Adds menu copy for the new “Health Summary” section. |
| lib/ui/auth-menu.ts | Extends auth menu options and renders the read-only health summary row (sanitized, disabled). |
| lib/codex-manager.ts | Implements health summary aggregation for login menu, including sync/restore/rollback/doctor summaries. |
| lib/cli.ts | Plumbs the new optional health summary option from CLI prompt into the auth menu. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/codex-manager.ts
Outdated
| const activeIndex = resolveActiveIndex(storage, "codex"); | ||
| const activeExists = | ||
| activeIndex >= 0 && activeIndex < storage.accounts.length; | ||
| if (!activeExists) { |
| for (const account of storage.accounts) { | ||
| const token = account.refreshToken.trim(); | ||
| if (seenRefreshTokens.has(token)) { | ||
| duplicateTokenCount += 1; | ||
| } else { | ||
| seenRefreshTokens.add(token); | ||
| } | ||
| if (hasPlaceholderEmail(account.email)) { | ||
| placeholderEmailCount += 1; | ||
| } | ||
| if (hasLikelyInvalidRefreshToken(account.refreshToken)) { | ||
| likelyInvalidRefreshTokenCount += 1; | ||
| } | ||
| } |
lib/codex-manager.ts
Outdated
| [actionableRestores, rollbackPlan] = await Promise.all([ | ||
| getActionableNamedBackupRestores({ currentStorage: storage }), | ||
| getLatestCodexCliSyncRollbackPlan(), | ||
| ]); |
lib/codex-manager.ts
Outdated
| getLatestCodexCliSyncRollbackPlan(), | ||
| ]); | ||
| } catch (error) { | ||
| console.warn("Failed to build login menu health summary", error); |
| fixIssues: "Auto-Repair Issues", | ||
| settings: "Settings", | ||
| moreChecks: "Advanced Checks", | ||
| healthSummary: "Health Summary", |
![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/codex-manager.ts`:
- Around line 4025-4031: The loop over storage.accounts that calls
account.refreshToken.trim() can throw when refreshToken is missing; replace the
direct trim usage in the duplicate-token detection with the existing safe helper
getDoctorRefreshTokenKey(account) (or otherwise null-check/typeof-check
account.refreshToken before trimming) so seenRefreshTokens and
duplicateTokenCount logic uses the normalized key returned by
getDoctorRefreshTokenKey; ensure the change is applied where the for-loop,
seenRefreshTokens, duplicateTokenCount, and the call sites
buildLoginMenuHealthSummary / summarizeReadOnlyDoctorState interact so a
malformed account cannot throw and crash the login flow.
- Around line 4093-4102: The try/catch currently only wraps Promise.all for
getActionableNamedBackupRestores and getLatestCodexCliSyncRollbackPlan, but
summarizeLatestCodexCliSyncState() and summarizeReadOnlyDoctorState(storage) are
called after the catch and can throw; expand the try block to include the calls
to summarizeLatestCodexCliSyncState and summarizeReadOnlyDoctorState (or wrap
them in the same try) so any error thrown by those helpers is caught, and in the
catch log the error with context and return/set safe default summaries (e.g.,
empty/placeholder syncSummary and doctorSummary) to avoid blocking login menu
rendering; locate this change around the getActionableNamedBackupRestores,
getLatestCodexCliSyncRollbackPlan, summarizeLatestCodexCliSyncState, and
summarizeReadOnlyDoctorState symbols.
In `@test/codex-manager-cli.test.ts`:
- Line 2323: The test currently asserts getActionableNamedBackupRestoresMock is
called twice which forces duplicate filesystem scans; change the test to expect
a single call and update the code paths so the same
getActionableNamedBackupRestores result is reused between the health-summary and
startup-recovery flows (e.g., cache or share the promise returned by
getActionableNamedBackupRestores in the service that both health-summary and
startupRecovery handlers call). Locate usages of
getActionableNamedBackupRestores/getActionableNamedBackupRestoresMock and the
startup-recovery and health-summary handlers, ensure they reference the shared
result instead of invoking a second scan, and update the test expectation to
toHaveBeenCalledTimes(1). Ensure behavior still covers error handling for
EBUSY/429 by queuing/retrying in the shared implementation.
- Around line 3233-3273: The healthSummary label/hint may contain ANSI/control
characters but showAuthMenu currently passes them through; update showAuthMenu
to sanitize healthSummary.label and healthSummary.hint before pushing the item
(the code that builds the "Health Summary" heading and the next disabled item
inside showAuthMenu), stripping ANSI escape sequences and control characters
(e.g. use a utility like stripAnsi or a regex to remove /\x1B\[[0-9;]*[A-Za-z]/g
and other non-printables) and ensure the item created for healthSummary sets the
sanitized strings as label and hint so the test can assert plain text; also
add/adjust the test to inject ANSI/control chars into healthSummary and assert
they are removed.
- Around line 3199-3231: The test's simulated failure uses new Error("EBUSY
backups") which doesn't emulate Windows errno semantics; update the mocked
rejection for getActionableNamedBackupRestoresMock to throw an errno-shaped
error (e.g., const err = Object.assign(new Error("EBUSY backups"), { code:
"EBUSY" }); getActionableNamedBackupRestoresMock.mockRejectedValue(err)) so the
code path that checks error.code === "EBUSY" is exercised; change only the mock
rejection creation in the test surrounding getActionableNamedBackupRestoresMock.
🪄 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: 75641a8a-8ebf-475c-981b-4fd7838b7ea4
📒 Files selected for processing (5)
lib/cli.tslib/codex-manager.tslib/ui/auth-menu.tslib/ui/copy.tstest/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 (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/ui/copy.tslib/cli.tslib/ui/auth-menu.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.ts
🔇 Additional comments (13)
lib/ui/copy.ts (1)
12-12: good ui copy addition.
lib/ui/copy.ts:12adds the heading key cleanly and matches the menu rendering path.lib/ui/auth-menu.ts (2)
52-60: type plumbing looks correct.
lib/ui/auth-menu.ts:52-60introduces a tight read-only shape and keepshealthSummaryoptional in menu options.
614-637: health summary rendering is safely integrated.
lib/ui/auth-menu.ts:614-637sanitizes both fields and renders the summary as a disabled row, which keeps the dashboard section non-interactive as intended.lib/cli.ts (1)
7-7: health summary passthrough is clean.
lib/cli.ts:7,lib/cli.ts:98, andlib/cli.ts:275wire the read-only summary through without altering auth control flow.Also applies to: 98-98, 275-275
test/codex-manager-cli.test.ts (4)
779-779: no comment needed here.
test/codex-manager-cli.test.ts:779is harmless setup churn.
2668-2668: same duplicate-scan concern as above.this expectation in
test/codex-manager-cli.test.ts:2668tracks the same startup-scan behavior already flagged at line 2323.
2962-2962: same duplicate-scan concern as above.this expectation in
test/codex-manager-cli.test.ts:2962is another downstream instance of the line 2323 issue.
3112-3197: good regression coverage for health summary wiring.
test/codex-manager-cli.test.ts:3112-3197validates that login options carry a read-only health summary with expected label and hint content.lib/codex-manager.ts (5)
93-96: imports look correct.
getLastCodexCliSyncRun()returnsCodexCliSyncRun | nullandgetLatestCodexCliSyncRollbackPlan()returnsPromise<CodexCliSyncRollbackPlan>perlib/codex-cli/sync.ts:289-309andlib/codex-cli/sync.ts:448-458. both are used appropriately in the new health summary functions.
3219-3228: interfaces are well-defined.
DashboardHealthSummaryandDoctorReadOnlySummaryare internal types with appropriate fields for read-only display. the optionalhintonDashboardHealthSummaryvs required onDoctorReadOnlySummarymatches their usage patterns inbuildLoginMenuHealthSummaryandsummarizeReadOnlyDoctorState.
3937-3943: simple utility, looks good.
3945-3990: sync state summarization handles null correctly.the null check at line 3950 covers the case where
getLastCodexCliSyncRun()returnsnull(perlib/codex-cli/sync.ts:309). thesummaryfield access at lines 3969-3978 is safe sinceCodexCliSyncSummaryfields are non-optional perlib/codex-cli/sync.ts:68-80.
4265-4273: health summary integration into login menu looks correct.the
healthSummaryis built before rendering and passed topromptLoginMode. per the ai summary,test/codex-manager-cli.test.tscovers health summary integration.one note: since
buildLoginMenuHealthSummaryis awaited without a surrounding try/catch here, an unhandled throw (if the fixes above aren't applied) would exit the login flow. the resilience fix suggested forlib/codex-manager.ts:4093-4102would address this.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
test/codex-manager-cli.test.ts (2)
3216-3218:⚠️ Potential issue | 🟠 Majoruse errno-shaped failure in the fallback test.
test/codex-manager-cli.test.ts:3216-3218still usesnew Error("EBUSY backups"), which does not exercise windows errno handling paths. usemakeErrnoError(..., "EBUSY")so the regression matches real fs failures.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."proposed fix
- getActionableNamedBackupRestoresMock.mockRejectedValue( - new Error("EBUSY backups"), - ); + getActionableNamedBackupRestoresMock.mockRejectedValue( + makeErrnoError("resource busy", "EBUSY"), + );🤖 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 3216 - 3218, Replace the generic Error used in the fallback test with an errno-shaped error so Windows-specific code paths are exercised: change the rejection of getActionableNamedBackupRestoresMock (the mock currently using new Error("EBUSY backups")) to use makeErrnoError(..., "EBUSY") (providing the same message/subject like "backups") so the test reproduces real fs failures and triggers errno-based handling in the code under test.
3268-3308:⚠️ Potential issue | 🟡 Minoradd ansi/control-char sanitization coverage for health summary row rendering.
test/codex-manager-cli.test.ts:3268-3308verifies disabled-row rendering, but it still does not assert stripping ansi/control chars from label/hint before display. add a regression case with escaped input and assert sanitized output.example extension
await showAuthMenu( [ { index: 0, email: "a@example.com", isCurrentAccount: true, }, ], { healthSummary: { - label: - "Pool 1 active | Sync none | Restore none | Rollback none | Doctor ok", - hint: "Accounts: 1 enabled / 0 disabled / 1 total", + label: + "\u001b[31mPool 1 active\u001b[0m | Sync none | Restore none | Rollback none | Doctor ok\u0007", + hint: "\u001b[2mAccounts: 1 enabled / 0 disabled / 1 total\u001b[0m\u0000", }, }, ); @@ expect(items[headingIndex + 1]).toEqual( expect.objectContaining({ - label: - "Pool 1 active | Sync none | Restore none | Rollback none | Doctor ok", + label: + "Pool 1 active | Sync none | Restore none | Rollback none | Doctor ok", disabled: true, hint: "Accounts: 1 enabled / 0 disabled / 1 total", }), );🤖 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 3268 - 3308, Update the test to cover ANSI/control-character sanitization for the health summary row by providing escaped/ANSI-laden strings for healthSummary.label and healthSummary.hint when calling showAuthMenu (or add a new similar test), then assert that the items produced via selectMock include the sanitized label and hint (no ANSI/control chars) and still have disabled: true and kind: "heading" placement preserved; reference the existing selectMock usage and the items extraction logic to locate where to assert sanitized output for showAuthMenu's healthSummary rendering.
🤖 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 4131-4141: The current Promise.all call that awaits
getActionableNamedBackupRestores({ currentStorage: storage }) and
getLatestCodexCliSyncRollbackPlan() exits immediately on any error; implement a
bounded retry loop around the combined attempt in the codex-manager code path
(the block using actionableRestores and rollbackPlan) to retry transient Windows
file-lock/EBUSY/EPERM and HTTP-429 style errors with exponential backoff (e.g.,
initial delay ~50–200ms, max attempts 3–5, small jitter) and abort immediately
on non-transient errors; ensure the retry predicate checks error codes/messages
for EBUSY/EPERM/ERR_FS/429, log each retry via logLoginMenuHealthSummaryWarning
without printing tokens/emails, keep final behavior unchanged on hard failures,
and add/modify vitest unit tests to cover transient EBUSY and 429 retries and
final failure cases.
- Around line 4167-4173: The menu hint currently injects raw rollbackPlan.reason
into hintParts (see the hintParts array and the use of rollbackPlan.reason);
replace that direct insertion with a sanitized/ mapped status string (e.g., via
a new helper like getRollbackHint(reason) or sanitizeRollbackReason(reason))
that returns a bounded label or status code (no filesystem paths, max length,
and mapped common reasons to human-friendly phrases). Update places referencing
rollbackPlan.reason in lib/codex-manager.ts to call the sanitizer, add/adjust
vitest unit tests to assert sanitized outputs, and ensure logs still avoid
leaking paths/tokens/emails while handling EBUSY/429 retry logic where relevant.
In `@test/codex-manager-cli.test.ts`:
- Around line 3202-3241: The test "falls back to a safe health summary when
restore or rollback state reads fail" (uses runCodexMultiAuthCli,
promptLoginModeMock, getActionableNamedBackupRestoresMock,
getLastCodexCliSyncRunMock) must add a console.warn spy assertion to ensure the
fallback warning is redacted: create a spy for console.warn before invoking
runCodexMultiAuthCli, assert it was called, and assert the captured warning
string does NOT contain raw filesystem paths, raw Error/Object dumps, or
unredacted tokens (e.g., assert the joined console.warn calls do not match
regexes for path separators like / or \\, "Error(", "Object{", or known error
text like "EBUSY"). Ensure the spy is restored after the test so other tests
remain deterministic.
- Line 779: Remove the duplicate default mock setup
getLastCodexCliSyncRunMock.mockReturnValue(null) that is repeated in the test;
keep only the first assignment (the earlier setup at the other occurrence) and
delete the later duplicate to reduce setup noise and avoid redundant state
initialization in the test file.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 3216-3218: Replace the generic Error used in the fallback test
with an errno-shaped error so Windows-specific code paths are exercised: change
the rejection of getActionableNamedBackupRestoresMock (the mock currently using
new Error("EBUSY backups")) to use makeErrnoError(..., "EBUSY") (providing the
same message/subject like "backups") so the test reproduces real fs failures and
triggers errno-based handling in the code under test.
- Around line 3268-3308: Update the test to cover ANSI/control-character
sanitization for the health summary row by providing escaped/ANSI-laden strings
for healthSummary.label and healthSummary.hint when calling showAuthMenu (or add
a new similar test), then assert that the items produced via selectMock include
the sanitized label and hint (no ANSI/control chars) and still have disabled:
true and kind: "heading" placement preserved; reference the existing selectMock
usage and the items extraction logic to locate where to assert sanitized output
for showAuthMenu's healthSummary rendering.
🪄 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: 598fcf11-8eb8-4438-83cc-b7c77ea8a2b6
📒 Files selected for processing (2)
lib/codex-manager.tstest/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 (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.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.ts
🔇 Additional comments (3)
test/codex-manager-cli.test.ts (1)
2326-2328: nice tightening on startup scan expectations.the assertions in
test/codex-manager-cli.test.ts:2326,test/codex-manager-cli.test.ts:2671, andtest/codex-manager-cli.test.ts:2965now lock in a single startup scan path while still verifying no restore prompt side effects. this keeps the login flow contract clearer.Also applies to: 2671-2671, 2965-2965
lib/codex-manager.ts (2)
4039-4048: good hardening on duplicate-token detection.
lib/codex-manager.ts:4039-4048now avoids raw token handling in this read-only summary path and safely skips unusable refresh tokens. this is a solid safety improvement, and i don’t see a new concurrency risk in this loop.
4098-4181: missing regression test coverage for two error fallback paths inlib/codex-manager.ts:4143-4156.test at
test/codex-manager-cli.test.ts:3202("falls back to a safe health summary...") only covers the promise.all failure path and doesn't mocksummarizeLatestCodexCliSyncState()orsummarizeReadOnlyDoctorState(). need explicit tests for:
- sync-summary error fallback (
lib/codex-manager.ts:4143-4148)- doctor-summary error fallback (
lib/codex-manager.ts:4151-4156)logLoginMenuHealthSummaryWarning()calls with error details- windows-style filesystem error redaction (EBUSY/resource busy handling)
no mocks exist for these two functions in the test suite, so their error paths are untested. verify windows error details don't leak in logs per coding guidelines.
| try { | ||
| [actionableRestores, rollbackPlan] = await Promise.all([ | ||
| getActionableNamedBackupRestores({ currentStorage: storage }), | ||
| getLatestCodexCliSyncRollbackPlan(), | ||
| ]); | ||
| } catch (error) { | ||
| logLoginMenuHealthSummaryWarning( | ||
| "Failed to load async login menu health summary state", | ||
| error, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
add a bounded retry for transient windows file-lock failures.
lib/codex-manager.ts:4131-4141 falls back immediately after one failure. for windows EBUSY/EPERM-style contention, a short retry would reduce false “unknown” health summaries without changing behavior on hard failures. this also lowers noise from transient concurrency between background io and menu rendering.
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 4131 - 4141, The current Promise.all call
that awaits getActionableNamedBackupRestores({ currentStorage: storage }) and
getLatestCodexCliSyncRollbackPlan() exits immediately on any error; implement a
bounded retry loop around the combined attempt in the codex-manager code path
(the block using actionableRestores and rollbackPlan) to retry transient Windows
file-lock/EBUSY/EPERM and HTTP-429 style errors with exponential backoff (e.g.,
initial delay ~50–200ms, max attempts 3–5, small jitter) and abort immediately
on non-transient errors; ensure the retry predicate checks error codes/messages
for EBUSY/EPERM/ERR_FS/429, log each retry via logLoginMenuHealthSummaryWarning
without printing tokens/emails, keep final behavior unchanged on hard failures,
and add/modify vitest unit tests to cover transient EBUSY and 429 retries and
final failure cases.
| reason: "No manual Codex CLI apply with a rollback checkpoint is available.", | ||
| snapshot: null, | ||
| }); | ||
| getLastCodexCliSyncRunMock.mockReturnValue(null); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
remove duplicate default mock setup for sync run state.
test/codex-manager-cli.test.ts:779 repeats the same default already set at test/codex-manager-cli.test.ts:737. keep one assignment to reduce setup noise.
proposed cleanup
- getLastCodexCliSyncRunMock.mockReturnValue(null);📝 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.
| getLastCodexCliSyncRunMock.mockReturnValue(null); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` at line 779, Remove the duplicate default
mock setup getLastCodexCliSyncRunMock.mockReturnValue(null) that is repeated in
the test; keep only the first assignment (the earlier setup at the other
occurrence) and delete the later duplicate to reduce setup noise and avoid
redundant state initialization in the test file.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/codex-manager.ts (1)
4151-4168: 🧹 Nitpick | 🔵 Trivialconsider bounded retry for transient windows file-lock failures.
the try/catch blocks at
lib/codex-manager.ts:4151-4168fall back immediately on any error. on windows, EBUSY/EPERM contention from concurrent file access is common. a short retry with exponential backoff (e.g., 2-3 attempts, 50-100ms initial delay) would reduce false "unknown" health summaries without changing behavior on hard failures.this was flagged in a past review and remains unaddressed. per coding guidelines: "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 4151 - 4168, The two try/catch blocks that call getActionableNamedBackupRestores and getLatestCodexCliSyncRollbackPlan should implement a bounded retry with exponential backoff (2–3 attempts, initial delay 50–100ms doubling each retry) and only retry on transient filesystem contention errors (e.g., EBUSY, EPERM); on other errors immediately call logLoginMenuHealthSummaryWarning with the error as today. Modify the calls around getActionableNamedBackupRestores and getLatestCodexCliSyncRollbackPlan to wrap their awaits in a small retry helper or inline loop that inspects error.code/name for EBUSY/EPERM, applies delays, and falls back to the existing logLoginMenuHealthSummaryWarning behavior on final failure. Add/adjust vitest unit tests to simulate EBUSY/EPERM transient failures and assert the retry behavior and that hard errors still log immediately, and ensure any new retry queue logic accounts for EBUSY/429 scenarios 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 `@lib/codex-manager.ts`:
- Line 4347: The call to buildLoginMenuHealthSummary(currentStorage) inside the
inner menu loop triggers repeated disk I/O (via getActionableNamedBackupRestores
and getLatestCodexCliSyncRollbackPlan) on every iteration; change this by
introducing a short-lived cache (e.g., memoized result with a 30–60s TTL) keyed
by currentStorage (or a last-modified token) and use that cached summary in the
menu loop, and/or only rebuild the summary after storage-modifying actions
(invalidate the cache on functions that change storage/backup state). Update the
code paths that call buildLoginMenuHealthSummary to read from the cache (or call
an async getCachedLoginMenuHealthSummary wrapper) and add Vitest unit tests for
buildLoginMenuHealthSummary to cover caching behavior and invalidation when
storage changes.
In `@test/codex-manager-cli.test.ts`:
- Around line 3144-3147: The test uses
getActionableNamedBackupRestoresMock.mockResolvedValue({ assessments: [{}],
totalBackups: 2 }) with an empty assessment object which may hide null/undefined
property access in production; update the mock to provide a minimal valid
assessment shape (e.g., include the specific properties your code reads such as
backup, name, status, or whatever health-summary logic in codex-manager.ts
expects) or alternatively adjust the code under test (functions that process
assessments in codex-manager.ts / health summary builder) to defensively check
for missing nested fields before access; target the
getActionableNamedBackupRestoresMock setup in the test and ensure the mocked
assessments mirror the real assessment interface used by the
health-summary/building logic.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4151-4168: The two try/catch blocks that call
getActionableNamedBackupRestores and getLatestCodexCliSyncRollbackPlan should
implement a bounded retry with exponential backoff (2–3 attempts, initial delay
50–100ms doubling each retry) and only retry on transient filesystem contention
errors (e.g., EBUSY, EPERM); on other errors immediately call
logLoginMenuHealthSummaryWarning with the error as today. Modify the calls
around getActionableNamedBackupRestores and getLatestCodexCliSyncRollbackPlan to
wrap their awaits in a small retry helper or inline loop that inspects
error.code/name for EBUSY/EPERM, applies delays, and falls back to the existing
logLoginMenuHealthSummaryWarning behavior on final failure. Add/adjust vitest
unit tests to simulate EBUSY/EPERM transient failures and assert the retry
behavior and that hard errors still log immediately, and ensure any new retry
queue logic accounts for EBUSY/429 scenarios 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: abed6bb3-0e20-48e4-8053-fada14584b8a
📒 Files selected for processing (2)
lib/codex-manager.tstest/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 (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.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.ts
🔇 Additional comments (16)
lib/codex-manager.ts (9)
1-1: lgtm on crypto import for fingerprinting.importing
createHashfromnode:cryptofor the sha256 fingerprint is appropriate and avoids external dependencies.
3273-3279: good: token fingerprinting reduces leakage surface.
getReadOnlyDoctorRefreshTokenFingerprintatlib/codex-manager.ts:3273-3279hashes the refresh token and truncates to 12 chars, which is a solid approach to avoid logging or exposing raw tokens while still enabling duplicate detection insummarizeReadOnlyDoctorState.
4039-4049: past review addressed: safe token handling in doctor summary.at
lib/codex-manager.ts:4039-4049, the loop now usesgetReadOnlyDoctorRefreshTokenFingerprint(account.refreshToken)which internally callsgetDoctorRefreshTokenKeyand handles undefined/non-string values safely. no more risk of.trim()throwing on malformed account entries.
4090-4096: good: redacted error logging avoids path/token leakage.
logLoginMenuHealthSummaryWarningatlib/codex-manager.ts:4090-4096usesgetRedactedFilesystemErrorLabel(error)which should strip sensitive filesystem paths and error details. this aligns with the coding guideline to avoid leaking tokens or emails in logs.
4098-4116: past review addressed: rollback hint is now sanitized.
formatLoginMenuRollbackHintatlib/codex-manager.ts:4098-4116mapsrollbackPlanto bounded, user-friendly strings instead of injecting rawrollbackPlan.reasonwhich could contain noisy filesystem paths. the logic at lines 4101-4115 returns one of four predefined labels, preventing uncontrolled text from reaching the ui.
4118-4123: good: early return on zero-account storage.
buildLoginMenuHealthSummaryatlib/codex-manager.ts:4121-4123correctly returnsnullfor fresh installs with no accounts, avoiding unnecessary i/o and ui clutter.
4194-4200: hint construction looks correct.the
hintPartsarray atlib/codex-manager.ts:4194-4200uses sanitized/mapped values for all sections:syncSummary.hint,formatLoginMenuRollbackHint(rollbackPlan), anddoctorSummary.hint. no raw filesystem paths or tokens are interpolated.
3954-3999: sync state summarizer is safe and degrades gracefully.
summarizeLatestCodexCliSyncStateatlib/codex-manager.ts:3954-3999pulls from the in-memory cache first viagetLastCodexCliSyncRun()and returns a safe default when no run exists. no exceptions can propagate here.
4001-4088: doctor state summary correctly uses fingerprint for duplicate detection.
summarizeReadOnlyDoctorStateatlib/codex-manager.ts:4001-4088iterates accounts usinggetReadOnlyDoctorRefreshTokenFingerprintinstead of raw tokens, ensuring duplicate detection doesn't expose sensitive data. the function returns safe defaults for empty or null storage.test/codex-manager-cli.test.ts (7)
779-779: duplicate default mock setup for sync run state.
test/codex-manager-cli.test.ts:779repeats the same default already set attest/codex-manager-cli.test.ts:737. keep one assignment to reduce setup noise.
3202-3241: add a redaction assertion for the fallback warning path.
test/codex-manager-cli.test.ts:3202-3241validates fallback summary text, but it does not assert that warning output is sanitized (no raw filesystem paths/error objects). add aconsole.warnspy assertion so this path has a privacy-safe regression guard.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."
3268-3308: add a sanitization regression for summary text.
test/codex-manager-cli.test.ts:3268-3308checks disabled-row rendering, but it does not assert ansi/control-char stripping fromhealthSummary.labelandhealthSummary.hintinlib/ui/auth-menu.ts:614-617.
3115-3200: lgtm — happy path health summary propagation test is solid.the test correctly verifies:
- pool summary line includes enabled/disabled account counts
- rollback hint reflects checkpoint availability
- promptLoginModeMock receives the healthSummary option
good coverage for the primary flow.
3243-3266: lgtm — zero-account early-return guard is well tested.verifies that health-summary i/o (rollback plan, sync run) is skipped when accounts array is empty, and that the single
getActionableNamedBackupRestoresMockcall comes from the startup recovery path rather than the health summary path (as the inline comment explains).
2326-2328: assertion updates align with health-summary i/o changes.the expectation changes from
not.toHaveBeenCalled()totoHaveBeenCalledTimes(1)attest/codex-manager-cli.test.ts:2671andtest/codex-manager-cli.test.ts:2965correctly reflect that health-summary i/o now fires after oauth completes. the newhealthSummary: undefinedassertion attest/codex-manager-cli.test.ts:2327confirms fresh-install behavior.Also applies to: 2671-2671, 2965-2965
3115-3200: code already uses isolated error handling, no Promise.all pattern found.the health summary function at lib/codex-manager.ts:4100-4180 implements four separate try/catch blocks with graceful fallbacks, not
Promise.all(). each call togetActionableNamedBackupRestores(),getLatestCodexCliSyncRollbackPlan(),summarizeLatestCodexCliSyncState(), andsummarizeReadOnlyDoctorState()is awaited independently with its own error boundary. there's no partial failure risk from concurrent execution since the functions aren't combined in a Promise.all. the test at test/codex-manager-cli.test.ts:3115-3200 correctly validates that the health summary passes read-only data to the login menu, and the mocked functions already demonstrate successful handling.> Likely an incorrect or invalid review comment.
| } | ||
| } | ||
| const flaggedStorage = await loadFlaggedAccounts(); | ||
| const healthSummary = await buildLoginMenuHealthSummary(currentStorage); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# search for caching or debounce patterns around buildLoginMenuHealthSummary
rg -n -C5 'buildLoginMenuHealthSummary' lib/Repository: ndycode/codex-multi-auth
Length of output: 1324
🏁 Script executed:
#!/bin/bash
# examine the loop structure around line 4347
sed -n '4300,4360p' lib/codex-manager.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 2827
🏁 Script executed:
#!/bin/bash
# read the buildLoginMenuHealthSummary function implementation
sed -n '4118,4160p' lib/codex-manager.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1485
🏁 Script executed:
#!/bin/bash
# search for caching patterns like memoize, cache, ttl around the function
rg -i 'memoiz|cache|ttl|debounce' lib/codex-manager.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 856
🏁 Script executed:
#!/bin/bash
# read more of the function to see all async calls
sed -n '4118,4180p' lib/codex-manager.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 2126
🏁 Script executed:
#!/bin/bash
# find where the loop ends (around line 4495)
sed -n '4490,4500p' lib/codex-manager.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 367
🏁 Script executed:
#!/bin/bash
# search for tests related to buildLoginMenuHealthSummary
find test -type f -name "*.ts" -o -name "*.js" | xargs rg 'buildLoginMenuHealthSummary' 2>/dev/nullRepository: ndycode/codex-multi-auth
Length of output: 52
🏁 Script executed:
#!/bin/bash
# count actual async/await patterns in the function more carefully
sed -n '4118,4175p' lib/codex-manager.ts | rg 'await|async' -nRepository: ndycode/codex-multi-auth
Length of output: 243
🏁 Script executed:
#!/bin/bash
# search for test files
find test -type f \( -name "*.ts" -o -name "*.test.ts" -o -name "*.spec.ts" \) | head -10Repository: ndycode/codex-multi-auth
Length of output: 327
🏁 Script executed:
#!/bin/bash
# search for memoization or caching at module level
rg -n 'const.*=.*new.*Map|const.*cache|memoize' lib/codex-manager.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 683
🏁 Script executed:
#!/bin/bash
# check the implementations of the two main async calls
rg -A10 'async function getActionableNamedBackupRestores' lib/Repository: ndycode/codex-multi-auth
Length of output: 636
🏁 Script executed:
#!/bin/bash
# find scanNamedBackups implementation
rg -A15 'async function scanNamedBackups' lib/Repository: ndycode/codex-multi-auth
Length of output: 923
🏁 Script executed:
#!/bin/bash
# find getLatestCodexCliSyncRollbackPlan implementation
rg -A10 'async function getLatestCodexCliSyncRollbackPlan' lib/Repository: ndycode/codex-multi-auth
Length of output: 696
🏁 Script executed:
#!/bin/bash
# search for any tests related to health summary or login menu
find test -type f -name "*.ts" | xargs rg -l 'health|menu' 2>/dev/null | head -5Repository: ndycode/codex-multi-auth
Length of output: 214
rebuild health summary on every menu iteration causes redundant disk i/o without caching.
buildLoginMenuHealthSummary(currentStorage) at lib/codex-manager.ts:4347 is called inside the inner menu loop and performs two async disk operations on every iteration: getActionableNamedBackupRestores() calls fs.readdir() on the backup directory, and getLatestCodexCliSyncRollbackPlan() performs rollback snapshot I/O. this creates latency spikes on slower disks or under contention, especially with concurrent filesystem operations.
missing regression tests for buildLoginMenuHealthSummary itself; add vitest coverage when implementing caching. consider a short ttl (30-60 seconds) or rebuild only after storage-modifying actions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` at line 4347, The call to
buildLoginMenuHealthSummary(currentStorage) inside the inner menu loop triggers
repeated disk I/O (via getActionableNamedBackupRestores and
getLatestCodexCliSyncRollbackPlan) on every iteration; change this by
introducing a short-lived cache (e.g., memoized result with a 30–60s TTL) keyed
by currentStorage (or a last-modified token) and use that cached summary in the
menu loop, and/or only rebuild the summary after storage-modifying actions
(invalidate the cache on functions that change storage/backup state). Update the
code paths that call buildLoginMenuHealthSummary to read from the cache (or call
an async getCachedLoginMenuHealthSummary wrapper) and add Vitest unit tests for
buildLoginMenuHealthSummary to cover caching behavior and invalidation when
storage changes.
| getActionableNamedBackupRestoresMock.mockResolvedValue({ | ||
| assessments: [{}], | ||
| totalBackups: 2, | ||
| }); |
There was a problem hiding this comment.
assessment mock uses empty object — verify property access won't break.
test/codex-manager-cli.test.ts:3145 passes assessments: [{}] which is a bare object with no properties. if lib/codex-manager.ts accesses assessment.backup.name or similar during health summary building, the production code could throw whereas this test silently passes due to mock isolation. consider providing a minimal valid assessment shape or explicitly documenting that the code only checks assessments.length.
suggested minimal shape
getActionableNamedBackupRestoresMock.mockResolvedValue({
- assessments: [{}],
+ assessments: [{ backup: { name: "mock-backup" }, eligibleForRestore: true }],
totalBackups: 2,
});🤖 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 3144 - 3147, The test uses
getActionableNamedBackupRestoresMock.mockResolvedValue({ assessments: [{}],
totalBackups: 2 }) with an empty assessment object which may hide null/undefined
property access in production; update the mock to provide a minimal valid
assessment shape (e.g., include the specific properties your code reads such as
backup, name, status, or whatever health-summary logic in codex-manager.ts
expects) or alternatively adjust the code under test (functions that process
assessments in codex-manager.ts / health summary builder) to defensively check
for missing nested fields before access; target the
getActionableNamedBackupRestoresMock setup in the test and ensure the mocked
assessments mirror the real assessment interface used by the
health-summary/building logic.
ndycode
force-pushed
the
fresh/16-retention-doctor
branch
from
be0837b to
60b3541
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
test/codex-manager-cli.test.ts (3)
779-779:⚠️ Potential issue | 🟡 Minorremove the duplicate sync-run default mock.
test/codex-manager-cli.test.ts:737already sets the same default. keepingtest/codex-manager-cli.test.ts:779adds setup noise and can mask per-test intent.proposed cleanup
- getLastCodexCliSyncRunMock.mockReturnValue(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` at line 779, Remove the duplicate default mock call getLastCodexCliSyncRunMock.mockReturnValue(null) that was added redundantly; it duplicates the setup already established earlier and should be deleted to reduce setup noise and avoid masking per-test intent—locate the second occurrence of getLastCodexCliSyncRunMock.mockReturnValue(null) in the test file and remove that line so only the original default mock remains.
3281-3321:⚠️ Potential issue | 🟡 Minoradd a sanitization regression assertion for the read-only row.
test/codex-manager-cli.test.ts:3281-3321validates rendering, but it still does not assert ansi/control-char stripping forhealthSummary.labelandhealthSummary.hintpassed intolib/ui/auth-menu.ts:614-617.🤖 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 3281 - 3321, Update the test that calls showAuthMenu to include a healthSummary.label and healthSummary.hint containing ANSI/control characters and add assertions that the produced items (from selectMock calls) contain the sanitized versions (control chars stripped) for both label and hint and remain disabled; locate the call to showAuthMenu and the items/headingIndex checks in the test (using selectMock, showAuthMenu, healthSummary.label/hint and the items array) and assert that items[headingIndex + 1].label equals the cleaned label string and items[headingIndex + 1].hint equals the cleaned hint string while still expecting disabled: true to ensure sanitization in lib/ui/auth-menu.ts (the code around the showAuthMenu handling at the referenced lines).
3144-3147:⚠️ Potential issue | 🟡 Minoruse a minimally valid assessment shape in this health-summary test.
test/codex-manager-cli.test.ts:3144-3147still usesassessments: [{}]. that can hide nested access regressions and weakens this regression case.proposed test-shape hardening
getActionableNamedBackupRestoresMock.mockResolvedValue({ - assessments: [{}], + assessments: [ + { + backup: { name: "mock-backup" }, + eligibleForRestore: true, + }, + ], totalBackups: 2, });🤖 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 3144 - 3147, The mock for getActionableNamedBackupRestoresMock currently returns assessments: [{}]; replace that with a minimally valid assessment object shape (in the assessments array returned by getActionableNamedBackupRestoresMock.mockResolvedValue) that includes the real fields the health-summary paths read — e.g., an id/assessmentId, a status/severity field, a target name (targetName or name), a timestamp (lastBackupAt or checkedAt) and any nested details/backups array or counts that the health-summary logic accesses — so the test exercises nested access and prevents regressions.
🤖 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-cli/sync.ts`:
- Around line 359-369: captureRollbackSnapshot currently calls
snapshotAccountStorage with failurePolicy: "warn" which can silently return null
on transient Windows EBUSY/EPERM; update captureRollbackSnapshot to retry
snapshotAccountStorage on EBUSY/EPERM with a bounded exponential backoff (e.g.,
3-5 attempts with increasing delays) and only fall back to returning null after
retries fail, using the snapshotAccountStorage call-site in
captureRollbackSnapshot to detect the specific error codes; update or add vitest
coverage to validate retry behavior and that any new queue or retry helpers
handle EBUSY and HTTP 429 semantics consistently.
- Around line 390-446: loadRollbackSnapshot currently fails immediately when
fs.readFile throws transient Windows file-lock errors (EBUSY/EPERM); add a
bounded retry loop around the fs.readFile call in loadRollbackSnapshot with 2–3
attempts and a short exponential/backoff delay (e.g., 50–200ms) only for
retryable error codes ("EBUSY", "EPERM", maybe "EACCES"), then proceed to the
existing error handling if all retries fail. Keep the normalized parsing and
existing return shapes unchanged, ensure the retry only wraps the readFile
operation (refer to loadRollbackSnapshot and the fs.readFile usage), and
update/add vitest cases to simulate EBUSY/EPRM transient failures to validate
the retry behavior.
- Around line 359-507: Add two unit tests: (1) exercise
getLatestCodexCliSyncRollbackPlan when there are no manual "changed" runs by
stubbing readSyncHistory so findLatestManualRollbackRun returns null and assert
the returned plan has status "unavailable" and reason "No manual Codex CLI
apply..."; (2) exercise loadRollbackSnapshot handling of corrupted JSON by
creating a CodexCliSyncRollbackSnapshot with a real temp file containing invalid
JSON (or stub fs.readFile to return malformed text), call loadRollbackSnapshot
and assert it returns status "unavailable" with reason "Rollback checkpoint is
invalid or empty." (use
normalizeAccountStorage/readSyncHistory/loadRollbackSnapshot/getLatestCodexCliSyncRollbackPlan
to locate behavior).
In `@test/codex-cli-sync.test.ts`:
- Around line 1578-1686: Add a deterministic regression test that mirrors the
existing TOCTOU test but exercises rollbackLatestCodexCliSync(plan): preload a
rollback plan (read snapshot + CodexCliSyncRun with rollbackSnapshot) into a
variable, then mutate storage/history concurrently before executing the rollback
by calling rollbackLatestCodexCliSync(preloadedPlan); use the same
withAccountStorageTransaction spy pattern (mocking
storageModule.withAccountStorageTransaction to run a concurrent
storageModule.saveAccounts and await persist) to force the race, then assert
rollback result.status is "restored" (or expected), txSpy was called once, and
the final storage reflects the concurrent save (e.g., account refreshToken
equals "refresh-concurrent"); reference rollbackLatestCodexCliSync(plan),
rollbackLastCodexCliSync(), storageModule.withAccountStorageTransaction,
storageModule.saveAccounts, and appendSyncHistoryEntry to locate where to add
the test.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Line 779: Remove the duplicate default mock call
getLastCodexCliSyncRunMock.mockReturnValue(null) that was added redundantly; it
duplicates the setup already established earlier and should be deleted to reduce
setup noise and avoid masking per-test intent—locate the second occurrence of
getLastCodexCliSyncRunMock.mockReturnValue(null) in the test file and remove
that line so only the original default mock remains.
- Around line 3281-3321: Update the test that calls showAuthMenu to include a
healthSummary.label and healthSummary.hint containing ANSI/control characters
and add assertions that the produced items (from selectMock calls) contain the
sanitized versions (control chars stripped) for both label and hint and remain
disabled; locate the call to showAuthMenu and the items/headingIndex checks in
the test (using selectMock, showAuthMenu, healthSummary.label/hint and the items
array) and assert that items[headingIndex + 1].label equals the cleaned label
string and items[headingIndex + 1].hint equals the cleaned hint string while
still expecting disabled: true to ensure sanitization in lib/ui/auth-menu.ts
(the code around the showAuthMenu handling at the referenced lines).
- Around line 3144-3147: The mock for getActionableNamedBackupRestoresMock
currently returns assessments: [{}]; replace that with a minimally valid
assessment object shape (in the assessments array returned by
getActionableNamedBackupRestoresMock.mockResolvedValue) that includes the real
fields the health-summary paths read — e.g., an id/assessmentId, a
status/severity field, a target name (targetName or name), a timestamp
(lastBackupAt or checkedAt) and any nested details/backups array or counts that
the health-summary logic accesses — so the test exercises nested access and
prevents regressions.
🪄 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: 1a05c78c-fe69-444c-bb2d-0c2a6a5d8e0f
📒 Files selected for processing (4)
lib/codex-cli/sync.tslib/codex-manager.tstest/codex-cli-sync.test.tstest/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 (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/codex-cli/sync.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/codex-cli-sync.test.ts
🔇 Additional comments (30)
lib/codex-manager.ts (15)
1-1: lgtm: crypto import for token fingerprinting.importing
createHashfromnode:cryptoto support the sha-256 fingerprint approach is appropriate for reducing token-leakage surface.
70-70: lgtm: new storage and sync imports for health summary features.the new imports for
listAccountSnapshots,getLastCodexCliSyncRun, andgetLatestCodexCliSyncRollbackPlanare correctly scoped for the health summary and rollback plan features.Also applies to: 94-97
3220-3229: lgtm: new interfaces for health summary types.
DashboardHealthSummaryandDoctorReadOnlySummaryare minimal and match theAuthMenuReadOnlyRowinterface fromlib/ui/auth-menu.ts:52-61for the fields actually used (label,hint).
3273-3279: lgtm: token fingerprinting reduces leakage surface.
getReadOnlyDoctorRefreshTokenFingerprintproperly uses sha-256 and slices to 12 chars. this is a safe replacement for raw token handling in read-only doctor state summaries.
3376-3419: lgtm: wal and rotating backup checks in doctor.the new doctor checks for storage-journal (wal), rotating-backups, and snapshot-backups are well-structured with appropriate severity levels.
3469-3486: lgtm: recovery chain check aggregates all backup sources.the recovery-chain check at
lib/codex-manager.ts:3469-3486correctly aggregates storage, wal, rotating, and snapshot artifacts to determine recovery readiness.
3592-3641: lgtm: rollback checkpoint and named-backup restore checks.the doctor now checks for rollback checkpoint readiness and actionable named backup restores, providing actionable guidance in the details field.
3946-3952: lgtm: formatCountNoun helper.simple and correct pluralization helper for count labels.
3954-3999: lgtm: summarizeLatestCodexCliSyncState.correctly formats sync run outcome with relative date and delta counts. no token/email leakage since it only uses outcome, counts, and timestamps.
4001-4088: lgtm: summarizeReadOnlyDoctorState uses fingerprinting.at
lib/codex-manager.ts:4040-4042, the loop usesgetReadOnlyDoctorRefreshTokenFingerprintinstead of raw tokens for duplicate detection. this aligns with the pr objective to replace raw refresh-token storage with fingerprints.
4090-4096: lgtm: logLoginMenuHealthSummaryWarning uses redacted error labels.
getRedactedFilesystemErrorLabelis used to avoid leaking filesystem paths in warnings, per the pr objective.
4098-4117: lgtm: formatLoginMenuRollbackHint avoids raw reason injection.this addresses the past review comment at
lib/codex-manager.ts:4197-4203by mapping rollback status to bounded hint strings rather than injecting rawrollbackPlan.reason.
4119-4124: lgtm: early return on empty storage.early return at line 4122-4124 avoids unnecessary i/o on fresh installs, per pr objectives.
4129-4171: each health-summary sub-call is now isolated in its own try/catch.the refactored structure with separate try/catch blocks for
getActionableNamedBackupRestores,getLatestCodexCliSyncRollbackPlan,summarizeLatestCodexCliSyncState, andsummarizeReadOnlyDoctorStateaddresses the past review comment about try/catch not covering sync helper calls.
4350-4356: buildLoginMenuHealthSummary still called per menu iteration without caching.the past review comment at
lib/codex-manager.ts:4350flagged thatbuildLoginMenuHealthSummaryis called inside the inner menu loop, causing redundant disk i/o. this concern remains unaddressed. consider a short ttl cache (30-60 seconds) or only rebuild after storage-modifying actions.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."
lib/codex-cli/sync.ts (15)
10-14: lgtm: new storage imports for rollback capability.importing
NamedBackupMetadata,normalizeAccountStorage,snapshotAccountStorage, andwithAccountStorageTransactioncorrectly supports the new rollback snapshot and restore features.
79-99: lgtm: new rollback types are well-structured.
CodexCliSyncTrigger,CodexCliSyncRollbackSnapshot,CodexCliSyncRollbackPlan, andCodexCliSyncRollbackResultprovide clear type safety for the rollback feature.
125-126: lgtm: CodexCliSyncRun extended with trigger and rollbackSnapshot.extending the run type to track trigger source and rollback snapshot enables rollback plan resolution from sync history.
166-179: lgtm: normalizeRollbackSnapshot guards against malformed snapshots.defensive validation ensures both
nameandpathare strings before returning a snapshot object.
181-195: lgtm: normalizeCodexCliSyncRun and cloneCodexCliSyncRun updated for rollback.correctly normalizes trigger to "manual" or "automatic" and applies
normalizeRollbackSnapshotfor the rollbackSnapshot field.
247-256: lgtm: buildSyncRunError sets rollbackSnapshot to null.on error, the rollback snapshot is explicitly nulled out to prevent stale snapshot references.
259-269: lgtm: createSyncRun accepts partial trigger/rollbackSnapshot with defaults.clean signature extension that defaults trigger to "automatic" and rollbackSnapshot to null.
312-329: lgtm: commitPendingCodexCliSyncRun normalizes rollbackSnapshot.ensures the snapshot is normalized before publishing to sync history.
371-373: lgtm: isManualChangedSyncRun predicate.correctly identifies manual trigger syncs that resulted in changes.
375-388: lgtm: findLatestManualRollbackRun iterates history in reverse.correctly finds the most recent manual changed sync run by iterating from newest to oldest.
448-458: lgtm: getLatestCodexCliSyncRollbackPlan exported for health summary.correctly delegates to
findLatestManualRollbackRunandloadRollbackSnapshot.
460-493: rollbackLatestCodexCliSync uses withAccountStorageTransaction for serialization.the use of
withAccountStorageTransactionatlib/codex-cli/sync.ts:463addresses the greptile-noted TOCTOU concern by ensuring plan resolution and persist happen atomically within the transaction. if a pre-resolved plan is passed with status "ready", it's used directly; otherwise, the plan is re-resolved within the transaction at line 467.however, if an external caller passes a "ready" plan that was resolved before entering the transaction, there's still a small TOCTOU window. the current implementation mitigates this since
resolvedPlan.storageis what gets persisted, not a re-read. this is acceptable for the current use case.
495-507: rollbackLastCodexCliSync throws on non-restored status.this throwing variant is suitable for test helpers but production callers should use
rollbackLatestCodexCliSyncwhich returns typed results. per context snippet 4 (lib/codex-manager/settings-hub.ts:337-345), production code correctly uses the non-throwing variant.
973-979: lgtm: applyCodexCliSyncToStorage extended with trigger parameter.the optional
triggerparameter defaults to "automatic" and is correctly propagated through the sync run creation.
1040-1048: rollback snapshot only captured for manual trigger with changes.the conditional at
lib/codex-cli/sync.ts:1040-1041correctly limits snapshot capture to manual triggers that result in changes, avoiding unnecessary i/o for automatic syncs or no-op manual syncs.
| async function captureRollbackSnapshot(): Promise<CodexCliSyncRollbackSnapshot | null> { | ||
| const snapshot: NamedBackupMetadata | null = await snapshotAccountStorage({ | ||
| reason: "codex-cli-sync", | ||
| failurePolicy: "warn", | ||
| }); | ||
| if (!snapshot) return null; | ||
| return { | ||
| name: snapshot.name, | ||
| path: snapshot.path, | ||
| }; | ||
| } | ||
|
|
||
| function isManualChangedSyncRun(run: CodexCliSyncRun | null): run is CodexCliSyncRun { | ||
| return Boolean(run && run.outcome === "changed" && run.trigger === "manual"); | ||
| } | ||
|
|
||
| async function findLatestManualRollbackRun(): Promise< | ||
| CodexCliSyncRun | null | ||
| > { | ||
| const history = await readSyncHistory({ kind: "codex-cli-sync" }); | ||
| for (let index = history.length - 1; index >= 0; index -= 1) { | ||
| const entry = history[index]; | ||
| if (!entry || entry.kind !== "codex-cli-sync") continue; | ||
| const run = normalizeCodexCliSyncRun(entry.run); | ||
| if (isManualChangedSyncRun(run)) { | ||
| return run; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| async function loadRollbackSnapshot( | ||
| snapshot: CodexCliSyncRollbackSnapshot | null, | ||
| ): Promise<CodexCliSyncRollbackPlan> { | ||
| if (!snapshot) { | ||
| return { | ||
| status: "unavailable", | ||
| reason: "No rollback checkpoint is available for the last manual apply.", | ||
| snapshot: null, | ||
| }; | ||
| } | ||
| if (!snapshot.name.trim()) { | ||
| return { | ||
| status: "unavailable", | ||
| reason: "Rollback checkpoint is missing its snapshot name.", | ||
| snapshot: null, | ||
| }; | ||
| } | ||
| if (!snapshot.path.trim()) { | ||
| return { | ||
| status: "unavailable", | ||
| reason: "Rollback checkpoint is missing its snapshot path.", | ||
| snapshot: null, | ||
| }; | ||
| } | ||
|
|
||
| try { | ||
| const raw = await fs.readFile(snapshot.path, "utf-8"); | ||
| const parsed = JSON.parse(raw) as unknown; | ||
| const normalized = normalizeAccountStorage(parsed); | ||
| if (!normalized) { | ||
| return { | ||
| status: "unavailable", | ||
| reason: "Rollback checkpoint is invalid or empty.", | ||
| snapshot, | ||
| }; | ||
| } | ||
| return { | ||
| status: "ready", | ||
| reason: `Rollback checkpoint ready (${normalized.accounts.length} account(s)).`, | ||
| snapshot, | ||
| accountCount: normalized.accounts.length, | ||
| storage: normalized, | ||
| }; | ||
| } catch (error) { | ||
| const reason = | ||
| (error as NodeJS.ErrnoException).code === "ENOENT" | ||
| ? `Rollback checkpoint is missing at ${snapshot.path}.` | ||
| : `Failed to read rollback checkpoint: ${ | ||
| error instanceof Error ? error.message : String(error) | ||
| }`; | ||
| return { | ||
| status: "unavailable", | ||
| reason, | ||
| snapshot, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| export async function getLatestCodexCliSyncRollbackPlan(): Promise<CodexCliSyncRollbackPlan> { | ||
| const lastManualRun = await findLatestManualRollbackRun(); | ||
| if (!lastManualRun) { | ||
| return { | ||
| status: "unavailable", | ||
| reason: "No manual Codex CLI apply with a rollback checkpoint is available.", | ||
| snapshot: null, | ||
| }; | ||
| } | ||
| return loadRollbackSnapshot(lastManualRun.rollbackSnapshot); | ||
| } | ||
|
|
||
| export async function rollbackLatestCodexCliSync( | ||
| plan?: CodexCliSyncRollbackPlan, | ||
| ): Promise<CodexCliSyncRollbackResult> { | ||
| return withAccountStorageTransaction(async (_current, persist) => { | ||
| const resolvedPlan = | ||
| plan && plan.status === "ready" | ||
| ? plan | ||
| : await getLatestCodexCliSyncRollbackPlan(); | ||
| if (resolvedPlan.status !== "ready" || !resolvedPlan.storage) { | ||
| return { | ||
| status: "unavailable", | ||
| reason: resolvedPlan.reason, | ||
| snapshot: resolvedPlan.snapshot, | ||
| }; | ||
| } | ||
|
|
||
| try { | ||
| await persist(resolvedPlan.storage); | ||
| return { | ||
| status: "restored", | ||
| reason: resolvedPlan.reason, | ||
| snapshot: resolvedPlan.snapshot, | ||
| accountCount: | ||
| resolvedPlan.accountCount ?? resolvedPlan.storage.accounts.length, | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| status: "error", | ||
| reason: error instanceof Error ? error.message : String(error), | ||
| snapshot: resolvedPlan.snapshot, | ||
| }; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| export async function rollbackLastCodexCliSync(): Promise< | ||
| CodexCliSyncRollbackResult & { status: "restored"; snapshot: CodexCliSyncRollbackSnapshot } | ||
| > { | ||
| const result = await rollbackLatestCodexCliSync(); | ||
| if (result.status !== "restored" || !result.snapshot) { | ||
| throw new Error(result.reason); | ||
| } | ||
| return { | ||
| ...result, | ||
| status: "restored", | ||
| snapshot: result.snapshot, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# search for test coverage of rollback functions
rg -n 'rollback|captureRollback|loadRollback' test/ --type ts -C2 | head -80Repository: ndycode/codex-multi-auth
Length of output: 4269
🏁 Script executed:
#!/bin/bash
# Find test files related to sync.ts
fd -e test.ts -e spec.ts | xargs grep -l "captureRollbackSnapshot\|loadRollbackSnapshot\|getLatestCodexCliSyncRollbackPlan\|rollbackLatestCodexCliSync\|rollbackLastCodexCliSync"Repository: ndycode/codex-multi-auth
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Also search for any test file that imports from sync.ts
rg "from.*sync.ts|import.*sync.ts" test/ --type tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check if there's a test file for codex-cli sync specifically
fd "sync" test/ --type tsRepository: ndycode/codex-multi-auth
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Read the codex-cli-sync.test.ts file to check what tests exist
wc -l test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Get the file size first, then read it
head -200 test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 6286
🏁 Script executed:
#!/bin/bash
# Search for tests that specifically test rollback functions
rg -n "getLatestCodexCliSyncRollbackPlan|rollbackLatestCodexCliSync|rollbackLastCodexCliSync|captureRollbackSnapshot|loadRollbackSnapshot" test/codex-cli-sync.test.ts -A5 | head -150Repository: ndycode/codex-multi-auth
Length of output: 2763
🏁 Script executed:
#!/bin/bash
# Search for test descriptions that describe the rollback behavior
rg -n "it\(" test/codex-cli-sync.test.ts | grep -i "rollback\|snapshot"Repository: ndycode/codex-multi-auth
Length of output: 641
🏁 Script executed:
#!/bin/bash
# Look for tests around line 1565-1845 to understand the complete test suite for rollback
sed -n '1540,1850p' test/codex-cli-sync.test.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 10712
🏁 Script executed:
#!/bin/bash
# Search for tests related to captureRollbackSnapshot and snapshot creation
rg -n "captureRollbackSnapshot" test/codex-cli-sync.test.ts -B3 -A3Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Look for the "records rollback snapshot metadata" test which creates the snapshot
sed -n '1349,1430p' test/codex-cli-sync.test.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 3036
🏁 Script executed:
#!/bin/bash
# Check which functions are exported from sync.ts
rg "^export" lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 1197
🏁 Script executed:
#!/bin/bash
# Check if captureRollbackSnapshot and loadRollbackSnapshot are defined and exported
sed -n '359,507p' lib/codex-cli/sync.ts | rg "export|async function"Repository: ndycode/codex-multi-auth
Length of output: 452
🏁 Script executed:
#!/bin/bash
# Search for tests that check invalid JSON or JSON parse errors
rg -n "JSON\|parse\|invalid" test/codex-cli-sync.test.ts | grep -i "json\|invalid" | head -20Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check the full test file around the missing file test to see all error scenarios
sed -n '1688,1850p' test/codex-cli-sync.test.ts | grep -n "it\|expect"Repository: ndycode/codex-multi-auth
Length of output: 1556
🏁 Script executed:
#!/bin/bash
# Search for any tests that test JSON parse errors or invalid JSON scenarios
rg -n "invalid\|corrupted\|malformed" test/codex-cli-sync.test.ts -B2 -A2Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check if there are tests for scenarios without manual runs
rg -n "no.*manual\|unavailable" test/codex-cli-sync.test.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Look at the actual function implementations to understand what errors are possible
sed -n '359,430p' lib/codex-cli/sync.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 2565
🏁 Script executed:
#!/bin/bash
# Check if there's a test for when no manual runs exist
sed -n '1500,1600p' test/codex-cli-sync.test.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 3546
🏁 Script executed:
#!/bin/bash
# Look at the complete test descriptions to see what scenarios are covered
rg "it\(" test/codex-cli-sync.test.ts -A1 | grep -A1 "rollback\|snapshot" | head -40Repository: ndycode/codex-multi-auth
Length of output: 1050
🏁 Script executed:
#!/bin/bash
# Search for test that checks when there are NO manual runs in history
rg -n "no.*manual\|empty.*history\|getLatestCodexCliSyncRollbackPlan\(\)" test/codex-cli-sync.test.ts -B3 -A5 | head -60Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check what happens when getLatestCodexCliSyncRollbackPlan is called with empty history
sed -n '1545,1577p' test/codex-cli-sync.test.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1259
🏁 Script executed:
#!/bin/bash
# Search if there's any test that doesn't set up any manual runs
rg -n "const plan = await getLatestCodexCliSyncRollbackPlan" test/codex-cli-sync.test.ts -B10 | grep -E "it\(|const plan"Repository: ndycode/codex-multi-auth
Length of output: 320
🏁 Script executed:
#!/bin/bash
# Look at lines around the first rollback test to see what the setup is
sed -n '1530,1580p' test/codex-cli-sync.test.ts | head -60Repository: ndycode/codex-multi-auth
Length of output: 1742
🏁 Script executed:
#!/bin/bash
# Let's check if there's a test that runs without setting up any history
sed -n '1, 200p' test/codex-cli-sync.test.ts | tail -50Repository: ndycode/codex-multi-auth
Length of output: 1048
🏁 Script executed:
#!/antml:
# Look for all tests that start fresh with no pre-existing sync history
rg "it\(" test/codex-cli-sync.test.ts | head -30Repository: ndycode/codex-multi-auth
Length of output: 2882
🏁 Script executed:
#!/bin/bash
# Find the test "restores the latest manual apply"
rg -n "restores the latest manual apply" test/codex-cli-sync.test.ts -A30Repository: ndycode/codex-multi-auth
Length of output: 1080
🏁 Script executed:
#!/antml bash
# Check if there's a test where history is completely empty or no manual runs exist
sed -n '1700,1730p' test/codex-cli-sync.test.ts | cat -nRepository: ndycode/codex-multi-auth
Length of output: 1163
🏁 Script executed:
#!/antml:
# Check if there's a test for when no manual runs exist (empty scenario)
rg -n "No manual Codex CLI" test/codex-cli-sync.test.ts -B5 -A5Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/antml/bash
# Search for tests of invalid/corrupted JSON in rollback snapshot
rg -n "JSON\|parse\|corrupted\|invalid or empty" test/codex-cli-sync.test.ts -B2 -A2Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/an/bash
# Search for any test that explicitly tests scenarios without pre-populated history
rg -n "\.test\.ts.*it\(" test/codex-cli-sync.test.ts | wc -lRepository: ndycode/codex-multi-auth
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Look at the function that returns "No manual Codex CLI apply" - check if it's tested
rg -n "No manual Codex CLI apply with a rollback checkpoint" lib/codex-cli/sync.ts -B5 -A5Repository: ndycode/codex-multi-auth
Length of output: 498
🏁 Script executed:
#!/bin/bash
# Check if there's test coverage for the normalizeAccountStorage path
rg -n "invalid or empty\|Rollback checkpoint is invalid" test/codex-cli-sync.test.ts -B3 -A3Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Get a full list of test descriptions to verify coverage
rg "it\(" test/codex-cli-sync.test.ts | grep -i "rollback\|snapshot"Repository: ndycode/codex-multi-auth
Length of output: 612
🏁 Script executed:
#!/bin/bash
# Check what all the rollback-related test descriptions say
sed -n '1349,1360p' test/codex-cli-sync.test.ts
sed -n '1428,1438p' test/codex-cli-sync.test.ts
sed -n '1471,1478p' test/codex-cli-sync.test.ts
sed -n '1545,1555p' test/codex-cli-sync.test.ts
sed -n '1578,1585p' test/codex-cli-sync.test.ts
sed -n '1688,1695p' test/codex-cli-sync.test.ts
sed -n '1726,1732p' test/codex-cli-sync.test.ts
sed -n '1773,1778p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 2441
test coverage exists for rollback functions but has gaps in edge cases.
the three exported rollback functions (getLatestCodexCliSyncRollbackPlan, rollbackLatestCodexCliSync, rollbackLastCodexCliSync) are tested in test/codex-cli-sync.test.ts with coverage for success paths and several error scenarios:
✓ test/codex-cli-sync.test.ts:1349 - snapshot capture on manual apply
✓ test/codex-cli-sync.test.ts:1471 - successful rollback restore
✓ test/codex-cli-sync.test.ts:1578 - rollback serialization against concurrent writes
✓ test/codex-cli-sync.test.ts:1688 - missing file (ENOENT) handling
✓ test/codex-cli-sync.test.ts:1726 - blank name/path validation
✓ test/codex-cli-sync.test.ts:1773 - persist failure handling
however, these edge cases are missing test coverage:
- no manual runs in sync history (the "No manual Codex CLI apply..." path at lib/codex-cli/sync.ts:453)
- corrupted/invalid json in snapshot file (would hit the normalizeAccountStorage null case at lib/codex-cli/sync.ts:61-66)
add tests for these two scenarios to close the gaps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-cli/sync.ts` around lines 359 - 507, Add two unit tests: (1)
exercise getLatestCodexCliSyncRollbackPlan when there are no manual "changed"
runs by stubbing readSyncHistory so findLatestManualRollbackRun returns null and
assert the returned plan has status "unavailable" and reason "No manual Codex
CLI apply..."; (2) exercise loadRollbackSnapshot handling of corrupted JSON by
creating a CodexCliSyncRollbackSnapshot with a real temp file containing invalid
JSON (or stub fs.readFile to return malformed text), call loadRollbackSnapshot
and assert it returns status "unavailable" with reason "Rollback checkpoint is
invalid or empty." (use
normalizeAccountStorage/readSyncHistory/loadRollbackSnapshot/getLatestCodexCliSyncRollbackPlan
to locate behavior).
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/codex-manager/settings-hub.ts (1)
2815-2963:⚠️ Potential issue | 🟠 Majoradd integration tests for sync-center rollback action execution and hotkey gating.
the rollback flow at
lib/codex-manager/settings-hub.ts:2879gates thelhotkey correctly andlib/codex-manager/settings-hub.ts:2896-2915wraps execution withwithQueuedRetryto handle retries, but test coverage is incomplete.test/codex-manager-cli.test.tstests rollback plan loading failure (line 4988) and underlying sync functions intest/codex-cli-sync.test.ts, but there are no regression tests for:
lhotkey triggering rollback action when plan ready- rollback action failure updating preview to "error" status and keeping sync-center interactive
- rollback action retries for
EBUSY/429during execution (not just preview loading)add tests that mock
rollbackLatestCodexCliSyncMockwith both success and failure outcomes, verify the UI loop behavior, and confirm retry logic applies to the rollback execution path viawithQueuedRetry(preview.targetPath, ...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager/settings-hub.ts` around lines 2815 - 2963, Add integration tests exercising the sync-center rollback action: simulate a ready rollback plan from getSyncCenterRollbackPlan and assert that pressing the "l" hotkey triggers the rollback action (the menu's onInput mapping) by executing runSyncCenterRollback via withQueuedRetry(preview.targetPath, ...). Create cases where the mocked rollback implementation (rollbackLatestCodexCliSyncMock / runSyncCenterRollback) returns success and where it throws/fails; for the failure case assert preview is updated to status "error" with statusDetail and the UI loop remains interactive (does not exit), and for retry behavior mock transient EBUSY/429 errors and assert withQueuedRetry retries the rollback execution path rather than only the preview load. Reference the interactive loop in settings-hub (the select onInput handler and the result.type === "rollback" branch) and ensure tests hook or spy on withQueuedRetry to confirm it wraps the rollback call.
♻️ Duplicate comments (5)
test/codex-manager-cli.test.ts (4)
3281-3321:⚠️ Potential issue | 🟡 Minoradd ansi/control-char sanitization assertions for health summary row.
test/codex-manager-cli.test.ts:3281-3321validates disabled-row rendering, but it still does not prove sanitization ofhealthSummary.labelandhealthSummary.hintbefore display inlib/ui/auth-menu.ts:line.proposed regression assertion update
await showAuthMenu( [ { index: 0, email: "a@example.com", isCurrentAccount: true, }, ], { healthSummary: { - label: - "Pool 1 active | Sync none | Restore none | Rollback none | Doctor ok", - hint: "Accounts: 1 enabled / 0 disabled / 1 total", + label: + "\u001b[31mPool 1 active | Sync none | Restore none | Rollback none | Doctor ok\u001b[0m", + hint: "Accounts: 1 enabled / 0 disabled / 1 total\u0007", }, }, ); @@ expect(items[headingIndex + 1]).toEqual( expect.objectContaining({ label: "Pool 1 active | Sync none | Restore none | Rollback none | Doctor ok", disabled: true, hint: "Accounts: 1 enabled / 0 disabled / 1 total", }), );🤖 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 3281 - 3321, Update the test for showAuthMenu to assert that healthSummary.label and healthSummary.hint are sanitized of ANSI/control characters before rendering: in the existing test use selectMock to capture items (the items array from selectMock.mock.calls[0][0]), locate the health summary row via headingIndex (finding item.label === "Health Summary" && item.kind === "heading"), then add assertions on items[headingIndex + 1] that verify its label and hint do not contain control/ANSI escape sequences (e.g. no \u001b or other control chars) to prove lib/ui/auth-menu.ts sanitizes healthSummary.label and healthSummary.hint.
3144-3147:⚠️ Potential issue | 🟡 Minoruse a real assessment shape in this health-summary fixture.
Line 3145 in
test/codex-manager-cli.test.tsusesassessments: [{}]. this can hide nested access regressions whenlib/codex-manager.ts:lineconsumes assessment fields.proposed test-fixture fix
getActionableNamedBackupRestoresMock.mockResolvedValue({ - assessments: [{}], + assessments: [ + { + backup: { + name: "mock-backup", + path: "/mock/backups/mock-backup.json", + createdAt: null, + updatedAt: now, + sizeBytes: 128, + version: 3, + accountCount: 1, + schemaErrors: [], + valid: true, + loadError: undefined, + }, + currentAccountCount: 2, + mergedAccountCount: 2, + imported: 0, + skipped: 0, + wouldExceedLimit: false, + eligibleForRestore: true, + error: undefined, + }, + ], + allAssessments: [], totalBackups: 2, });🤖 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 3144 - 3147, The fixture uses an empty assessment object in getActionableNamedBackupRestoresMock.mockResolvedValue({ assessments: [{}], ... }), which can hide regressions when lib/codex-manager.ts reads assessment properties; replace the empty object with a realistic assessment shape containing the exact fields the code consumes (e.g., id/backupId, status, createdAt, health/metrics, actionable flags or actions array, and any nested objects accessed) so tests exercise the same nested accesses used by functions in lib/codex-manager.ts (reference getActionableNamedBackupRestoresMock and the health-summary test fixture) and update any other assertions in test/codex-manager-cli.test.ts that expect specific values.
779-779: 🧹 Nitpick | 🔵 Trivialremove duplicate default sync-run mock setup.
Line 779 repeats the same default already set at Line 737 in
test/codex-manager-cli.test.ts, which adds setup noise without value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` at line 779, Remove the duplicated mock setup call getLastCodexCliSyncRunMock.mockReturnValue(null) at the later occurrence (the one around line 779) since the same default is already established earlier in the test file (around the earlier setup). Keep the initial setup where getLastCodexCliSyncRunMock is configured and delete the redundant call to avoid noisy duplicate test setup.
4988-5052:⚠️ Potential issue | 🟠 Majordo not lock in 4 rollback-plan reads; add explicit win32 eperm regression.
Line 5050 in
test/codex-manager-cli.test.tshard-codestoHaveBeenCalledTimes(4)forgetLatestCodexCliSyncRollbackPlanMock, which bakes duplicate rollback-plan io into behavior exercised bylib/codex-manager.ts:line. this increases fs contention risk under windows load. this new failure-path test also only exercisesEBUSY(Line 5019-5021), leavingEPERMbehavior unguarded.proposed direction
- expect(getLatestCodexCliSyncRollbackPlanMock).toHaveBeenCalledTimes(4); + // one rollback-plan load per sync-center render cycle (open + refresh) + expect(getLatestCodexCliSyncRollbackPlanMock).toHaveBeenCalledTimes(2);it("keeps sync-center recoverable when rollback plan loading fails with eperm", async () => { // mirror the existing ebusy test setup, but use: // getLatestCodexCliSyncRollbackPlanMock.mockRejectedValue( // Object.assign(new Error("access denied"), { code: "EPERM" }), // ); // assert degraded rollback text remains safe and flow continues. });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 4988 - 5052, The test currently hard-codes expect(getLatestCodexCliSyncRollbackPlanMock).toHaveBeenCalledTimes(4) which locks in the exact number of rollback-plan reads; change that assertion to a non-exact check (e.g., expect(getLatestCodexCliSyncRollbackPlanMock).toHaveBeenCalled() or .toHaveBeenCalledTimesGreaterThanOrEqual(1)) so the test does not enforce duplicate IO, and add a new sibling test named like "keeps sync-center recoverable when rollback plan loading fails with eperm" that mirrors this EBUSY setup but uses getLatestCodexCliSyncRollbackPlanMock.mockRejectedValue(Object.assign(new Error("access denied"), { code: "EPERM" })) and asserts the CLI still shows degraded rollback text and exits 0 (reusing runCodexMultiAuthCli, previewCodexCliSyncMock, selectMock flows and asserting applyCodexCliSyncToStorageMock not called).test/codex-cli-sync.test.ts (1)
1638-1746:⚠️ Potential issue | 🟠 Majorstill missing the stale-plan race for
rollbackLatestCodexCliSync(plan).
test/codex-cli-sync.test.ts:1638-1746only exercisesrollbackLastCodexCliSync()while it resolves the plan internally.rollbackLatestCodexCliSync(plan)is touched intest/codex-cli-sync.test.ts:1976-1982, but only on a forced save failure. there is still no regression that preloads a ready plan, mutates storage/history, and then executes the explicit-plan rollback, so the toctou path remains uncovered.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: 1976-1982
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-cli-sync.test.ts` around lines 1638 - 1746, Add a deterministic test that exercises the TOCTOU (stale-plan) path by preparing a ready rollback plan (call rollbackLatestCodexCliSync(plan) with an explicitly preloaded plan object that you obtain from the same plan-builder used in tests), then before invoking rollbackLatestCodexCliSync(plan) mutate storage/history concurrently (use storageModule.withAccountStorageTransaction or spy on it like the existing test does to start a concurrent storageModule.saveAccounts() and capture its Promise) so the explicit-plan rollback sees stale storage; assert the rollback result and final stored accounts reflect the concurrent mutation (await the concurrent save Promise and verify refreshToken/account list), and ensure the test uses vitest constructs (vi.spyOn) and temp fixtures to remain deterministic; reference rollbackLatestCodexCliSync(plan), rollbackLastCodexCliSync, storageModule.withAccountStorageTransaction, and storageModule.saveAccounts to locate where to add the new case.
🤖 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-cli/sync.ts`:
- Around line 493-502: Add a new vitest in test/codex-cli-sync.test.ts that
covers the path where findLatestManualRollbackRun() returns null: call
getLatestCodexCliSyncRollbackPlan() and assert the returned object has status
=== "unavailable", reason === "No manual Codex CLI apply with a rollback
checkpoint is available.", and snapshot === null; set up the test scenario so
sync history has only automatic runs or is empty (mock or stub
findLatestManualRollbackRun to return null) and reference this new test in the
lib/** change note per coding guidelines.
- Around line 42-49: Add a parametrized vitest that covers the Windows EPERM
retry path: mock process.platform to "win32" and have the stubbed
retryTransientRollbackIo (or the function that throws within the rollback loop)
throw an error with code "EPERM" so that isRetryableRollbackIoErrorCode returns
true for the retry loop and the loop completes successfully; specifically,
update the existing codex-cli sync tests to include a case where
retryTransientRollbackIo throws { code: "EPERM" } and assert the retry logic
retries and resolves (or completes) rather than failing, ensuring the test
toggles process.platform back after the case to avoid global test pollution.
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2895-2914: The rollback branch calls
withQueuedRetry(preview.targetPath, () => runSyncCenterRollback(rollbackPlan))
without a local try/catch, so any thrown error will escape the sync-center loop;
wrap that call in a try/catch around the withQueuedRetry invocation, and on
catch set preview = { ...preview, status: "error", lastSync:
getLastCodexCliSyncRun(), statusDetail: (err && err.message) || String(err) },
then refresh rollbackPlan = await getSyncCenterRollbackPlan() and continue the
loop; reference the functions/vars withQueuedRetry, runSyncCenterRollback,
preview, rollbackPlan, getLastCodexCliSyncRun, getSyncCenterRollbackPlan, and
preserve existing flow when rollbackResult is returned successfully.
In `@lib/storage.ts`:
- Around line 2279-2286: The function pruneAutoGeneratedSnapshots silently
clamps the option keepLatestPerReason to a minimum of 1 (via Math.max) which can
surprise callers who pass 0; update the AutoSnapshotPruneOptions JSDoc (and/or
the pruneAutoGeneratedSnapshots docblock) to explicitly state that
keepLatestPerReason is clamped to a minimum of 1 and describe the behavior when
callers pass 0 or negative values, referencing the pruneAutoGeneratedSnapshots
function and the keepLatestPerReason variable so readers know the enforced
minimum.
In `@test/codex-cli-sync.test.ts`:
- Around line 1428-1486: The test only exercises the EBUSY retry path; add a
sibling vitest case that mocks process.platform to "win32" and simulates a
transient NodeJS.ErrnoException with code "EPERM" on
storageModule.snapshotAccountStorage (use the same mock pattern:
mockRejectedValueOnce(transientError) then mockResolvedValueOnce(...)) to drive
the Win32-specific retry branch in applyCodexCliSyncToStorage; ensure the test
asserts snapshotAccountStorage was called twice and that
result.pendingRun?.run.rollbackSnapshot matches the resolved snapshot, and
restore process.platform after the test to keep tests deterministic.
In `@test/storage.test.ts`:
- Around line 2301-2342: The test currently only checks that a snapshot file was
created by importAccounts; update the test so it reads and parses the created
snapshot to assert it was taken before the merge (i.e., contains only the
original "existing" account). After calling importAccounts(importPath) use
getNamedBackupsDirectoryPath() to find the backup file whose name
startsWith("accounts-import-accounts-snapshot-"), read and JSON.parse that
snapshot file, and assert snapshot.accounts has length 1 and
snapshot.accounts[0].accountId === "existing" (keep the existing expect that
final loadAccounts() has length 2). This ensures importAccounts() snapshotting
occurs pre-merge and keeps the test deterministic under vitest.
---
Outside diff comments:
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2815-2963: Add integration tests exercising the sync-center
rollback action: simulate a ready rollback plan from getSyncCenterRollbackPlan
and assert that pressing the "l" hotkey triggers the rollback action (the menu's
onInput mapping) by executing runSyncCenterRollback via
withQueuedRetry(preview.targetPath, ...). Create cases where the mocked rollback
implementation (rollbackLatestCodexCliSyncMock / runSyncCenterRollback) returns
success and where it throws/fails; for the failure case assert preview is
updated to status "error" with statusDetail and the UI loop remains interactive
(does not exit), and for retry behavior mock transient EBUSY/429 errors and
assert withQueuedRetry retries the rollback execution path rather than only the
preview load. Reference the interactive loop in settings-hub (the select onInput
handler and the result.type === "rollback" branch) and ensure tests hook or spy
on withQueuedRetry to confirm it wraps the rollback call.
---
Duplicate comments:
In `@test/codex-cli-sync.test.ts`:
- Around line 1638-1746: Add a deterministic test that exercises the TOCTOU
(stale-plan) path by preparing a ready rollback plan (call
rollbackLatestCodexCliSync(plan) with an explicitly preloaded plan object that
you obtain from the same plan-builder used in tests), then before invoking
rollbackLatestCodexCliSync(plan) mutate storage/history concurrently (use
storageModule.withAccountStorageTransaction or spy on it like the existing test
does to start a concurrent storageModule.saveAccounts() and capture its Promise)
so the explicit-plan rollback sees stale storage; assert the rollback result and
final stored accounts reflect the concurrent mutation (await the concurrent save
Promise and verify refreshToken/account list), and ensure the test uses vitest
constructs (vi.spyOn) and temp fixtures to remain deterministic; reference
rollbackLatestCodexCliSync(plan), rollbackLastCodexCliSync,
storageModule.withAccountStorageTransaction, and storageModule.saveAccounts to
locate where to add the new case.
In `@test/codex-manager-cli.test.ts`:
- Around line 3281-3321: Update the test for showAuthMenu to assert that
healthSummary.label and healthSummary.hint are sanitized of ANSI/control
characters before rendering: in the existing test use selectMock to capture
items (the items array from selectMock.mock.calls[0][0]), locate the health
summary row via headingIndex (finding item.label === "Health Summary" &&
item.kind === "heading"), then add assertions on items[headingIndex + 1] that
verify its label and hint do not contain control/ANSI escape sequences (e.g. no
\u001b or other control chars) to prove lib/ui/auth-menu.ts sanitizes
healthSummary.label and healthSummary.hint.
- Around line 3144-3147: The fixture uses an empty assessment object in
getActionableNamedBackupRestoresMock.mockResolvedValue({ assessments: [{}], ...
}), which can hide regressions when lib/codex-manager.ts reads assessment
properties; replace the empty object with a realistic assessment shape
containing the exact fields the code consumes (e.g., id/backupId, status,
createdAt, health/metrics, actionable flags or actions array, and any nested
objects accessed) so tests exercise the same nested accesses used by functions
in lib/codex-manager.ts (reference getActionableNamedBackupRestoresMock and the
health-summary test fixture) and update any other assertions in
test/codex-manager-cli.test.ts that expect specific values.
- Line 779: Remove the duplicated mock setup call
getLastCodexCliSyncRunMock.mockReturnValue(null) at the later occurrence (the
one around line 779) since the same default is already established earlier in
the test file (around the earlier setup). Keep the initial setup where
getLastCodexCliSyncRunMock is configured and delete the redundant call to avoid
noisy duplicate test setup.
- Around line 4988-5052: The test currently hard-codes
expect(getLatestCodexCliSyncRollbackPlanMock).toHaveBeenCalledTimes(4) which
locks in the exact number of rollback-plan reads; change that assertion to a
non-exact check (e.g.,
expect(getLatestCodexCliSyncRollbackPlanMock).toHaveBeenCalled() or
.toHaveBeenCalledTimesGreaterThanOrEqual(1)) so the test does not enforce
duplicate IO, and add a new sibling test named like "keeps sync-center
recoverable when rollback plan loading fails with eperm" that mirrors this EBUSY
setup but uses
getLatestCodexCliSyncRollbackPlanMock.mockRejectedValue(Object.assign(new
Error("access denied"), { code: "EPERM" })) and asserts the CLI still shows
degraded rollback text and exits 0 (reusing runCodexMultiAuthCli,
previewCodexCliSyncMock, selectMock flows and asserting
applyCodexCliSyncToStorageMock not called).
🪄 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: 09336b4c-b9f9-4a7f-8506-96736d872ecf
📒 Files selected for processing (8)
lib/codex-cli/sync.tslib/codex-manager/settings-hub.tslib/storage.tslib/sync-history.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/storage.test.tstest/sync-history.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.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/sync-history.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/sync-history.tslib/storage.tslib/codex-manager/settings-hub.tslib/codex-cli/sync.ts
🔇 Additional comments (23)
lib/codex-manager/settings-hub.ts (1)
2845-2851: rollback gating is implemented correctly.at
lib/codex-manager/settings-hub.ts:2845(line 2845) andlib/codex-manager/settings-hub.ts:2879(line 2879), both menu selection and hotkey path are gated onrollbackPlan.status === "ready". this is the right safety check for interactive rollback.Also applies to: 2879-2881
test/codex-manager-cli.test.ts (1)
3204-3254: good fallback redaction regression coverage.this is a solid guardrail in
test/codex-manager-cli.test.ts:3204-3254: you assert warning emission while blocking rawebusytext and raw path leakage in the output path exercised bylib/codex-manager.ts:line.lib/storage.ts (10)
51-53: looks good.constants are well-defined. the regex pattern correctly captures the reason group for snapshot classification.
211-226: types are clean.the
AccountSnapshotFailurePolicygives callers explicit control over error handling behavior—nice for making rollback-critical paths fail loudly while allowing doctor flows to proceed.
2183-2210: timestamp and naming helpers are solid.using UTC ensures consistent snapshot ordering across timezones. the millisecond precision in timestamps prevents collisions during rapid successive operations.
2212-2232: parsing logic is clean.the fallback chain
updatedAt ?? createdAt ?? 0handles edge cases where filesystem metadata may be missing.
2234-2266: rollback snapshot name lookup is well-guarded.dynamic import of
./sync-history.jsavoids circular dependency issues. the function correctly finds the most recent manual "changed" run and extracts its snapshot name for retention.
2340-2344: retention enforcement is straightforward.delegates to
pruneAutoGeneratedSnapshotswith the module constant. clean.
2346-2370: path redaction helpers are well-designed.the regex at
lib/storage.ts:2352-2355handles both unix and windows paths, including drive letters. this prevents leaking usernames embedded in filesystem paths to logs.
2372-2426: snapshotAccountStorage implementation is solid.the
failurePolicyparameter gives callers control over error propagation. error messages useformatSnapshotErrorForLogatlib/storage.ts:2391,2410,2421to redact paths. the retention enforcement at line 2416 ensures stale snapshots are cleaned up even if the current snapshot isn't used.
1995-1998: listAccountSnapshots is clean.simple filter over
listNamedBackupsusing theisAccountSnapshotNamepredicate.
3811-3813: pre-import snapshot placement is correct.capturing the snapshot before the limit check at
lib/storage.ts:3813ensures a rollback checkpoint exists even if the import fails validation. using the default "warn" policy lets imports proceed when snapshot creation fails transiently.lib/codex-cli/sync.ts (11)
112-132: rollback types are well-structured.the
CodexCliSyncRollbackPlanincludes the loadedstoragefor ready plans, avoiding redundant reads during rollback execution.
199-228: normalization helpers are defensive.
normalizeRollbackSnapshotatlib/codex-cli/sync.ts:199-212validates both name and path are strings before returning, preventing downstream crashes from malformed history entries.
292-302: createSyncRun signature extension is clean.defaults ensure backward compatibility for existing callers not passing trigger or rollbackSnapshot.
356-361: rollback snapshot normalization on commit is correct.normalizing at publish time at
lib/codex-cli/sync.ts:356-358ensures history entries have consistent structure regardless of how the pending run was constructed.
392-412: captureRollbackSnapshot retry handling is solid.using
failurePolicy: "error"atlib/codex-cli/sync.ts:398ensures errors propagate to the retry wrapper. after retries exhaust, the function degrades gracefully by logging and returning null.
414-431: findLatestManualRollbackRun is efficient.backwards iteration at
lib/codex-cli/sync.ts:422finds the most recent matching entry without scanning the entire history.
433-491: loadRollbackSnapshot validation and retry are correct.input validation at
lib/codex-cli/sync.ts:443-456prevents unnecessary I/O. the retry wrapper at line 459 handles Windows file locks. error messages at lines 479-488 are descriptive without leaking sensitive data.
505-538: rollbackLatestCodexCliSync uses transaction for atomicity.wrapping the persist in
withAccountStorageTransactionatlib/codex-cli/sync.ts:508ensures the rollback is atomic. accepting a pre-resolved plan avoids redundant reads when the caller has already validated availability.
540-552: rollbackLastCodexCliSync provides throwing semantics.callers who need a guaranteed rollback can use this instead of checking the result object. the runtime check at
lib/codex-cli/sync.ts:544validates the type assertion.
1085-1093: trigger-conditional rollback snapshot is efficient.only capturing snapshots for manual triggers at
lib/codex-cli/sync.ts:1086avoids disk overhead during frequent automatic syncs. the snapshot is attached to the run for history tracking.
280-290: error runs correctly clear rollback snapshot.setting
rollbackSnapshot: nullatlib/codex-cli/sync.ts:288ensures failed sync runs don't reference snapshots that may be in an inconsistent state.
| function isRetryableRollbackIoErrorCode( | ||
| code: string | undefined, | ||
| ): code is "EBUSY" | "EAGAIN" | "EPERM" { | ||
| if (code && RETRYABLE_ROLLBACK_IO_CODES.has(code)) { | ||
| return true; | ||
| } | ||
| return code === "EPERM" && process.platform === "win32"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check for existing tests covering EPERM retry on windows
rg -n "EPERM|win32" test/codex-cli-sync.test.ts -B2 -A2 | head -40Repository: ndycode/codex-multi-auth
Length of output: 833
🏁 Script executed:
# search for specific tests of the rollback retry functions
rg -n "isRetryableRollbackIoErrorCode|retryTransientRollbackIo" test/ -B2 -A5Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# check if any tests mock process.platform
rg -n "process\.platform" test/codex-cli-sync.test.ts -B2 -A2Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# get the full context of the EPERM tests to understand what they're testing
rg -n "it\.each.*EPERM" test/codex-cli-sync.test.ts -A10 | head -50Repository: ndycode/codex-multi-auth
Length of output: 1266
🏁 Script executed:
# get the exact location and context of isRetryableRollbackIoErrorCode and retryTransientRollbackIo
rg -n "function isRetryableRollbackIoErrorCode|function retryTransientRollbackIo" lib/codex-cli/sync.ts -A8Repository: ndycode/codex-multi-auth
Length of output: 600
🏁 Script executed:
# also check where retryTransientRollbackIo is used in the codebase
rg -n "retryTransientRollbackIo" lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 213
🏁 Script executed:
# see the full test names to understand what they're testing
rg -n "it\.each.*EPERM" test/codex-cli-sync.test.ts -B2 | grep -E "^\s*(it|describe)" | head -5Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# check if captureRollbackSnapshot or other rollback functions are tested
rg -n "captureRollbackSnapshot|rollbackLatestCodexCliSync|rollbackLastCodexCliSync" test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 690
🏁 Script executed:
# check what specific errors the rollback tests are throwing to see if they test the EPERM branch
rg -n "rollbackLastCodexCliSync|rollbackLatestCodexCliSync" test/codex-cli-sync.test.ts -A15 | grep -E "(throw|Error|EBUSY|EPERM|code)" | head -20Repository: ndycode/codex-multi-auth
Length of output: 404
🏁 Script executed:
# also check the test around line 1638 to see what it's testing
sed -n '1638,1700p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1618
🏁 Script executed:
# search for test setup that throws EBUSY or EPERM during rollback operations
rg -n "rollbackLastCodexCliSync\|rollbackLatestCodexCliSync" test/codex-cli-sync.test.ts -B5 -A20 | grep -E "(throw|Error|EBUSY|EPERM|code:)" | head -20Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# check if there's any vitest mocking of file operations in rollback tests
rg -n "vi\.spyOn.*readFile|vi\.spyOn.*writeFile|vi\.spyOn.*unlink" test/codex-cli-sync.test.ts -A3 | head -30Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# let's see the full test suite for rollbackLastCodexCliSync to understand what's being tested
rg -n "it\(.*rollback" test/codex-cli-sync.test.ts -A2Repository: ndycode/codex-multi-auth
Length of output: 1276
🏁 Script executed:
# check if the functions captureRollbackSnapshot and related rollback functions are tested at all
rg -n "captureRollbackSnapshot|loadRollbackSnapshot|getLatestCodexCliSyncRollbackPlan" test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 423
🏁 Script executed:
# check the EBUSY retry test at line 1428 to see what error codes it covers
sed -n '1428,1480p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1392
🏁 Script executed:
# check the rollback checkpoint read retry test at line 1786
sed -n '1786,1850p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1812
🏁 Script executed:
# verify what error codes are tested in the rollback retry tests - check if EPERM is covered
sed -n '1786,1900p' test/codex-cli-sync.test.ts | grep -E "(code|EBUSY|EAGAIN|EPERM)"Repository: ndycode/codex-multi-auth
Length of output: 276
🏁 Script executed:
# check if any tests mock process.platform in the entire test file
rg -n "process\.platform|vi\.mock.*process" test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 50
add test for windows EPERM retry path in rollback i/o.
lib/codex-cli/sync.ts:48 returns true for EPERM when process.platform === "win32", but no test covers this branch. existing rollback tests at test/codex-cli-sync.test.ts:1428 and test/codex-cli-sync.test.ts:1786 throw EBUSY only. need a parametrized test that mocks process.platform to "win32" and throws EPERM from retryTransientRollbackIo to verify the retry loop completes successfully.
per coding guidelines: "lib/**: verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios." windows EPERM edge case is untested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-cli/sync.ts` around lines 42 - 49, Add a parametrized vitest that
covers the Windows EPERM retry path: mock process.platform to "win32" and have
the stubbed retryTransientRollbackIo (or the function that throws within the
rollback loop) throw an error with code "EPERM" so that
isRetryableRollbackIoErrorCode returns true for the retry loop and the loop
completes successfully; specifically, update the existing codex-cli sync tests
to include a case where retryTransientRollbackIo throws { code: "EPERM" } and
assert the retry logic retries and resolves (or completes) rather than failing,
ensuring the test toggles process.platform back after the case to avoid global
test pollution.
| const parsed = JSON.parse(raw) as unknown; | ||
| const normalized = normalizeAccountStorage(parsed); | ||
| if (!normalized) { | ||
| return { | ||
| status: "unavailable", | ||
| reason: "Rollback checkpoint is invalid or empty.", | ||
| snapshot, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check if there's a test for corrupted/invalid snapshot content
rg -n "invalid or empty\|corrupted\|malformed" test/codex-cli-sync.test.ts -B2 -A2Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# first, verify the source file and code exists
head -n 475 lib/codex-cli/sync.ts | tail -n 20Repository: ndycode/codex-multi-auth
Length of output: 586
🏁 Script executed:
# check if test file exists and get its size
wc -l test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 100
🏁 Script executed:
# search for loadRollbackSnapshot tests
rg -n "loadRollbackSnapshot\|normalizeAccountStorage" test/codex-cli-sync.test.ts -A5Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# check all test names in the test file to understand test structure
rg -n "describe\|it\(" test/codex-cli-sync.test.ts | head -30Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# search for snapshot or rollback tests
rg -n "snapshot\|rollback" test/codex-cli-sync.test.ts -iRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# get describe/it structure from test file
rg -n "describe\(|it\(" test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 3360
🏁 Script executed:
# check what normalizeAccountStorage does
rg -n "normalizeAccountStorage" lib/codex-cli/sync.ts -B2 -A10 | head -40Repository: ndycode/codex-multi-auth
Length of output: 825
🏁 Script executed:
# look at the test that marks rollback plan unavailable (line 1748)
sed -n '1748,1785p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1177
🏁 Script executed:
# check what normalizeAccountStorage does in storage.ts
rg -n "normalizeAccountStorage" lib/ -A20 | head -60Repository: ndycode/codex-multi-auth
Length of output: 2833
add regression test for corrupted snapshot content.
test covers missing checkpoint file at test/codex-cli-sync.test.ts:1748 but misses the normalizeAccountStorage null path at lib/codex-cli/sync.ts:464. create snapshot with valid json but invalid schema (missing accounts array, wrong version, etc.) and verify getLatestCodexCliSyncRollbackPlan returns status: "unavailable" with reason containing "invalid or empty". normalizeAccountStorage at lib/storage.ts:1673 validates against multiple schema rules, so the rollback flow needs coverage for that failure mode.
| export async function getLatestCodexCliSyncRollbackPlan(): Promise<CodexCliSyncRollbackPlan> { | ||
| const lastManualRun = await findLatestManualRollbackRun(); | ||
| if (!lastManualRun) { | ||
| return { | ||
| status: "unavailable", | ||
| reason: "No manual Codex CLI apply with a rollback checkpoint is available.", | ||
| snapshot: null, | ||
| }; | ||
| } | ||
| return loadRollbackSnapshot(lastManualRun.rollbackSnapshot); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check if there's a test for the "no manual runs" scenario
rg -n "No manual Codex CLI" test/codex-cli-sync.test.ts -B3 -A5Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Check if test file exists and search for related tests
fd "sync.test.ts" --type fRepository: ndycode/codex-multi-auth
Length of output: 153
🏁 Script executed:
# Search for tests of getLatestCodexCliSyncRollbackPlan
rg -n "getLatestCodexCliSyncRollbackPlan" --type tsRepository: ndycode/codex-multi-auth
Length of output: 2420
🏁 Script executed:
# Search for findLatestManualRollbackRun mocks or tests
rg -n "findLatestManualRollbackRun" test/ -A3 -B3Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Read test cases for getLatestCodexCliSyncRollbackPlan around the lines found
sed -n '1620,1635p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 680
🏁 Script executed:
sed -n '1770,1790p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 728
🏁 Script executed:
sed -n '1850,1870p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 598
🏁 Script executed:
# Also check around the function definition to understand test structure
sed -n '1,50p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1875
🏁 Script executed:
# Search for tests that might set up automatic-only runs or mock findLatestManualRollbackRun returning null
rg -n "automatic" test/codex-cli-sync.test.ts -i -B2 -A2Repository: ndycode/codex-multi-auth
Length of output: 565
🏁 Script executed:
# Look for test descriptions around the unavailable scenarios
rg -n "unavailable|no manual|automatic" test/codex-cli-sync.test.ts -i -B3Repository: ndycode/codex-multi-auth
Length of output: 1148
🏁 Script executed:
# Check if there's any test setup with only automatic runs in sync history
rg -n 'kind.*automatic|automatic.*run' test/codex-cli-sync.test.ts -i -B2 -A2Repository: ndycode/codex-multi-auth
Length of output: 368
🏁 Script executed:
# Check the full context of the test that uses automaticRun - what happens after it's added?
sed -n '1580,1640p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1981
🏁 Script executed:
# Search for any test that checks for the specific message "No manual Codex CLI apply with a rollback checkpoint"
rg -n "No manual" test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Look at the function implementation to understand the exact condition
sed -n '493,510p' lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 707
add test for the "no manual runs" unavailable path at lib/codex-cli/sync.ts:495-500.
getLatestCodexCliSyncRollbackPlan() needs explicit coverage for when sync history contains only automatic runs or is empty. current tests at test/codex-cli-sync.test.ts:1580+ cover manual run with checkpoint (ready status) and missing checkpoint file (unavailable status), but don't exercise the findLatestManualRollbackRun() null case. add a test that verifies status: "unavailable" and reason: "No manual Codex CLI apply..." when no manual runs with rollback checkpoints exist.
per coding guidelines: lib/** changes must cite affected tests (vitest).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-cli/sync.ts` around lines 493 - 502, Add a new vitest in
test/codex-cli-sync.test.ts that covers the path where
findLatestManualRollbackRun() returns null: call
getLatestCodexCliSyncRollbackPlan() and assert the returned object has status
=== "unavailable", reason === "No manual Codex CLI apply with a rollback
checkpoint is available.", and snapshot === null; set up the test scenario so
sync history has only automatic runs or is empty (mock or stub
findLatestManualRollbackRun to return null) and reference this new test in the
lib/** change note per coding guidelines.
| export async function pruneAutoGeneratedSnapshots( | ||
| options: AutoSnapshotPruneOptions = {}, | ||
| ): Promise<AutoSnapshotPruneResult> { | ||
| const backups = options.backups ?? (await listNamedBackups()); | ||
| const keepLatestPerReason = Math.max( | ||
| 1, | ||
| options.keepLatestPerReason ?? 1, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
undocumented minimum clamp on keepLatestPerReason.
the Math.max(1, ...) at lib/storage.ts:2283-2285 silently enforces a floor of 1, but AutoSnapshotPruneOptions doesn't document this. callers passing keepLatestPerReason: 0 expecting full pruning will be surprised.
📝 add jsdoc clarifying the clamping
export interface AutoSnapshotPruneOptions {
backups?: NamedBackupMetadata[];
preserveNames?: Iterable<string>;
+ /**
+ * Number of snapshots to keep per reason. Clamped to a minimum of 1.
+ * `@default` 1
+ */
keepLatestPerReason?: number;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2279 - 2286, The function
pruneAutoGeneratedSnapshots silently clamps the option keepLatestPerReason to a
minimum of 1 (via Math.max) which can surprise callers who pass 0; update the
AutoSnapshotPruneOptions JSDoc (and/or the pruneAutoGeneratedSnapshots docblock)
to explicitly state that keepLatestPerReason is clamped to a minimum of 1 and
describe the behavior when callers pass 0 or negative values, referencing the
pruneAutoGeneratedSnapshots function and the keepLatestPerReason variable so
readers know the enforced minimum.
| it("retries transient rollback snapshot capture for manual applies", async () => { | ||
| const transientError = new Error("snapshot busy") as NodeJS.ErrnoException; | ||
| transientError.code = "EBUSY"; | ||
| const snapshotSpy = vi | ||
| .spyOn(storageModule, "snapshotAccountStorage") | ||
| .mockRejectedValueOnce(transientError) | ||
| .mockResolvedValueOnce({ | ||
| name: "accounts-codex-cli-sync-snapshot-2026-03-16_00-00-00_002", | ||
| path: join(tempDir, "rollback-snapshot-retry.json"), | ||
| createdAt: 1, | ||
| updatedAt: 1, | ||
| sizeBytes: 128, | ||
| version: 3, | ||
| accountCount: 1, | ||
| schemaErrors: [], | ||
| valid: true, | ||
| }); | ||
|
|
||
| const current: AccountStorageV3 = { | ||
| version: 3, | ||
| accounts: [ | ||
| { | ||
| accountId: "acc_a", | ||
| accountIdSource: "token", | ||
| email: "a@example.com", | ||
| refreshToken: "refresh-a", | ||
| accessToken: "access-a", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| }; | ||
|
|
||
| const result = await applyCodexCliSyncToStorage(current, { | ||
| trigger: "manual", | ||
| sourceState: { | ||
| path: accountsPath, | ||
| activeAccountId: "acc_b", | ||
| accounts: [ | ||
| { | ||
| accountId: "acc_b", | ||
| email: "b@example.com", | ||
| accessToken: "access-b", | ||
| refreshToken: "refresh-b", | ||
| isActive: true, | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
|
|
||
| expect(result.changed).toBe(true); | ||
| expect(snapshotSpy).toHaveBeenCalledTimes(2); | ||
| expect(result.pendingRun?.run.rollbackSnapshot).toEqual({ | ||
| name: "accounts-codex-cli-sync-snapshot-2026-03-16_00-00-00_002", | ||
| path: join(tempDir, "rollback-snapshot-retry.json"), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
cover the win32 EPERM rollback-io path.
test/codex-cli-sync.test.ts:1428-1486 and test/codex-cli-sync.test.ts:1786-1864 only drive EBUSY. the retry hardening here still never exercises the win32 EPERM branch called out in the PR notes, so the platform-specific rollback path can regress without a deterministic vitest signal. add a case that mocks process.platform to "win32" and forces EPERM through the same 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.
Also applies to: 1786-1864
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-cli-sync.test.ts` around lines 1428 - 1486, The test only
exercises the EBUSY retry path; add a sibling vitest case that mocks
process.platform to "win32" and simulates a transient NodeJS.ErrnoException with
code "EPERM" on storageModule.snapshotAccountStorage (use the same mock pattern:
mockRejectedValueOnce(transientError) then mockResolvedValueOnce(...)) to drive
the Win32-specific retry branch in applyCodexCliSyncToStorage; ensure the test
asserts snapshotAccountStorage was called twice and that
result.pendingRun?.run.rollbackSnapshot matches the resolved snapshot, and
restore process.platform after the test to keep tests deterministic.
| it("creates a snapshot before importing into an existing account pool", async () => { | ||
| const importPath = join(testWorkDir, "import.json"); | ||
|
|
||
| await saveAccounts({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "existing", | ||
| refreshToken: "ref-existing", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); | ||
| await fs.writeFile( | ||
| importPath, | ||
| JSON.stringify({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| accountId: "imported", | ||
| refreshToken: "ref-imported", | ||
| addedAt: 2, | ||
| lastUsed: 2, | ||
| }, | ||
| ], | ||
| }), | ||
| "utf-8", | ||
| ); | ||
|
|
||
| await importAccounts(importPath); | ||
|
|
||
| const entries = await fs.readdir(getNamedBackupsDirectoryPath()); | ||
| expect( | ||
| entries.some((name) => | ||
| name.startsWith("accounts-import-accounts-snapshot-"), | ||
| ), | ||
| ).toBe(true); | ||
| expect((await loadAccounts())?.accounts).toHaveLength(2); | ||
| }); |
There was a problem hiding this comment.
prove the import snapshot is pre-merge.
test/storage.test.ts:2301-2342 only proves that importAccounts() created some snapshot file. if the implementation starts snapshotting after the merge, this still passes because the backup directory is populated either way. parse the created snapshot back out, or restore it, and assert it contains only the original "existing" account.
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 2301 - 2342, The test currently only
checks that a snapshot file was created by importAccounts; update the test so it
reads and parses the created snapshot to assert it was taken before the merge
(i.e., contains only the original "existing" account). After calling
importAccounts(importPath) use getNamedBackupsDirectoryPath() to find the backup
file whose name startsWith("accounts-import-accounts-snapshot-"), read and
JSON.parse that snapshot file, and assert snapshot.accounts has length 1 and
snapshot.accounts[0].accountId === "existing" (keep the existing expect that
final loadAccounts() has length 2). This ensures importAccounts() snapshotting
occurs pre-merge and keeps the test deterministic under vitest.
Summary
fresh/16-retention-doctor#81What Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
6decea7Additional Notes
fresh/19-first-run-wizard, which contains this branch unchanged plus later stacked commits.fresh/16-retention-doctor, notmain.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 re-opens the preserved health-summary cleanup from #81, scoped to the dashboard and its supporting rollback/snapshot infrastructure. the core additions are: a
buildLoginMenuHealthSummarythat assembles pool/sync/restore/rollback/doctor state into a single dashboard row, a rollback snapshot workflow tied to manual codex-cli syncs, automatic snapshot capture before destructive operations, and a refactored sync-history trim pipeline.previously flagged issues that are now addressed:
buildLoginMenuHealthSummarynow wraps each of the four state sources in its own independenttry/catch— async I/O failures no longer suppress the in-memorysummarizeReadOnlyDoctorStateresultnullimmediately, skipping all async health-summary I/O on fresh installsseenRefreshTokens;getReadOnlyDoctorRefreshTokenFingerprinthashes with SHA-256 and keeps only the first 12 hex chars — reduces token-leakage surface on windows heap dumpsformatLoginMenuRollbackHint's actual output formatremaining items worth tracking:
pruneAutoGeneratedSnapshotsdefaultskeepLatestPerReasonto1, whileACCOUNT_SNAPSHOT_RETENTION_PER_REASONexports3; rollback-referenced snapshots are protected bygetLatestManualCodexCliRollbackSnapshotNamesso there's no current data-loss path, but the inconsistency is a caller footgun — see comment inlib/storage.tsgetSyncCenterRollbackPlanandrunSyncCenterRollbackinsettings-hub.tsguard statically-imported functions withtypeofchecks, producing unreachable fallback branches — see comment inlib/codex-manager/settings-hub.tsrollbackLatestCodexCliSyncignores the current on-disk state passed bywithAccountStorageTransaction; the settings-hub path is serialised viawithQueuedRetry, but the clirollbackLastCodexCliSyncpath carries the pre-existing toctou race on windowsConfidence Score: 3/5
pruneAutoGeneratedSnapshots, dead code fromtypeofguards insettings-hub.ts, and the pre-existing cli toctou on windows. none block correctness of the dashboard health summary path, which is the stated scope. test coverage is thorough across all new code paths including transient io retries and concurrent write scenarios.lib/storage.ts(retention default inconsistency) andlib/codex-manager/settings-hub.ts(dead fallback branches in rollback helpers)Important Files Changed
CodexCliSyncRollbackPlan/CodexCliSyncRollbackResulttypes,getLatestCodexCliSyncRollbackPlan,rollbackLatestCodexCliSync, and retry logic for transient windows I/O;_currentis intentionally unused in the transaction handler (documented risk)buildLoginMenuHealthSummarywith four independent try/catch blocks, SHA-256 token fingerprinting for read-only doctor deduplication, new checks for WAL, rotating backups, snapshot backups, recovery chain, rollback checkpoint, and named backup restores inrunDoctorsnapshotAccountStorage,pruneAutoGeneratedSnapshots,enforceSnapshotRetention, and snapshot name utilities;pruneAutoGeneratedSnapshotsdefaultkeepLatestPerReasonis1butACCOUNT_SNAPSHOT_RETENTION_PER_REASONexports3, creating a caller-visible inconsistencytrimHistoryFileIfNeededto returnPrunedSyncHistory, addspruneSyncHistoryEntries(pure function keeping latest-per-kind even when cap is exceeded),loadHistoryEntriesFromDisk,rewriteLatestEntry,parseHistoryContent, and publicpruneSyncHistory;pendingHistoryWritestype widened toSet<Promise<unknown>>getSyncCenterRollbackPlan/runSyncCenterRollbackhelpers;typeofguards on statically imported functions produce dead code in both fallback branchesFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[manual sync trigger\napplyCodexCliSyncToStorage] --> B{trigger === manual\nAND changed?} B -- no --> C[createSyncRun\nrollbackSnapshot: null] B -- yes --> D[captureRollbackSnapshot\nretryTransientRollbackIo] D -- EBUSY/EAGAIN retry --> D D -- success --> E[NamedBackupMetadata\nname + path] D -- all retries fail --> F[log.warn\nreturn null] E --> G[createSyncRun\nrollbackSnapshot: snapshot] F --> C G --> H[commitPendingCodexCliSyncRun\nappends to sync history] C --> H H --> I[getLatestCodexCliSyncRollbackPlan\nscans history for latest manual changed run] I --> J{snapshot in\nhistory?} J -- no --> K[status: unavailable\nno checkpoint] J -- yes --> L[loadRollbackSnapshot\nretryTransientRollbackIo readFile] L -- ENOENT --> M[status: unavailable\nmissing at path] L -- invalid JSON --> N[status: unavailable\ninvalid/empty] L -- success --> O[status: ready\naccountCount + storage] O --> P[rollbackLatestCodexCliSync\nwithAccountStorageTransaction] K --> Q[return unavailable\nto caller] M --> Q P --> R{persist\nsnapshot.storage} R -- success --> S[status: restored] R -- error --> T[status: error] style D fill:#f9f,stroke:#333 style L fill:#f9f,stroke:#333 style P fill:#bbf,stroke:#333Comments Outside Diff (4)
lib/codex-manager/settings-hub.ts, line 956 (link)getSyncCenterRollbackPlancrashes sync center on EBUSYgetSyncCenterRollbackPlan()callsgetLatestCodexCliSyncRollbackPlan(), which reads the sync-history file from disk. on windows, antivirus or rotation holds can produceEBUSY/EPERMfrom that read. there is no try/catch around any of the sixgetSyncCenterRollbackPlan()call sites inpromptSyncCenter— the initial one before the loop and all five refreshes inside it. a singleEBUSYanywhere crashes the entire sync-center UI, leaving the user unable to apply or rollback.buildLoginMenuHealthSummarywraps every equivalent call in an individual try/catch. the sync-center path needs the same treatment. a minimal fix wraps each refresh in a guard that falls back to the last-known plan:the same pattern should be applied to every
rollbackPlan = await getSyncCenterRollbackPlan()inside the loop (after refresh, apply, and rollback actions). missing vitest coverage for the throwing-getSyncCenterRollbackPlanpath in the sync-center loop — worth adding a test that mocks the function to reject and asserts the loop continues rather than crashes.Prompt To Fix With AI
lib/storage.ts, line 1334-1337 (link)loadAccounts()insidesnapshotAccountStoragebypassesfailurePolicy = "warn"snapshotAccountStoragecallsloadAccounts()unconditionally before attempting the backup.failurePolicyonly guards thecreateBackupstep — ifloadAccounts()throws (e.g., windowsEBUSYfrom an antivirus hold on the storage file), the error propagates directly to the caller regardless offailurePolicy.callers in
destructive-actions.tspass nofailurePolicy, so they get the default"warn". they expect a failed snapshot to log-and-continue, not to abort the destructive action entirely. anEBUSYonloadAccounts()would do the latter.deleteAccountAtIndexalready holdsstoragein memory;deleteSavedAccounts/resetLocalStatecould accept it similarly. accepting an optional pre-loaded storage avoids the extra disk read and the unguarded throw:this also removes the redundant disk read in
deleteAccountAtIndex, which matters on windows where every extra file open is anEBUSYcandidate.Prompt To Fix With AI
lib/sync-history.ts, line 353-365 (link)pruneSyncHistoryresetslastAppendErrorunconditionallylastAppendError = nullon line 361 clears the module-level error flag that tracks append failures.pruneSyncHistoryis a standalone maintenance operation — clearing an unrelated pending append error here masks failures from any caller that checkslastAppendErrorafter a previous failedappendSyncHistoryEntry. the prune should not own that flag.lib/storage.ts, line 2254-2256 (link)breaksilently drops protection for the previous valid checkpointwhen the latest manual+changed run has
rollbackSnapshot: null(failed snapshot capture), thisbreakstops the scan entirely. any snapshot from an earlier successful manual sync is no longer added topreserveNames, so once thecodex-cli-syncper-reason bucket exceedsACCOUNT_SNAPSHOT_RETENTION_PER_REASON(3), that older snapshot gets pruned. the user ends up withgetLatestCodexCliSyncRollbackPlan()returningunavailablefor the most recent run and the prior valid checkpoint deleted from disk.continueis the safer choice — skip the no-snapshot run and keep scanning backwards for the nearest older run that does have a snapshot name to protect:Prompt To Fix All With AI
Last reviewed commit: "fix(sync): keep sync..."
Context used: