Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughadds startup recovery decision API, named/rotating backup and snapshot management, backup browser restore flows, extensive codex-cli sync/rollback orchestration, and an in-file sync-history system with atomic append/prune. see Changes
Sequence Diagram(s)sequenceDiagram
participant user as user
participant cli as doctor-cli
participant manager as codex-manager
participant storage as storage (`lib/storage.ts`)
participant codexcli as codex-cli (`lib/codex-cli/sync.ts`)
participant fs as backups/fs
user->>cli: run doctor
cli->>manager: request diagnostics
manager->>storage: listAccountSnapshots(), listNamedBackups(), assessNamedBackupRestore()
manager->>codexcli: getLatestCodexCliSyncRollbackPlan()
storage->>fs: enumerate backups, rotating slots, read snapshot files
codexcli->>manager: rollback plan result
storage-->>manager: backups + assessments
manager->>cli: aggregated doctor JSON (recovery-chain, rollback-checkpoint, actionable restores)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes review notes
🚥 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
Adds retention and recovery-oriented diagnostics by introducing sync-history pruning, snapshot retention rules, and expanded auth doctor coverage for recovery artifacts and rollback/named-backup restore readiness.
Changes:
- Add sync history pruning utilities (
pruneSyncHistoryEntries,pruneSyncHistory) and update tests to cover aggressive trimming and on-disk pruning. - Add account snapshot discovery + retention (
listAccountSnapshots,pruneAutoGeneratedSnapshots, retention enforcement after snapshot creation). - Expand
auth doctorchecks to report recovery chain artifacts, rollback checkpoint readiness, and actionable named-backup restores (with CLI tests).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/sync-history.test.ts | Adds coverage for new sync-history pruning behaviors and refactors test helpers. |
| test/storage-recovery-paths.test.ts | Adds tests for snapshot pruning and rollback-referenced snapshot retention. |
| test/codex-manager-cli.test.ts | Extends mocked storage surface and asserts new doctor JSON checks. |
| lib/sync-history.ts | Implements pruning APIs and refactors history loading/parsing and latest-pointer rewriting. |
| lib/storage.ts | Implements snapshot listing + pruning/retention (including rollback snapshot preservation). |
| lib/codex-manager.ts | Adds doctor checks for WAL/journal, backups/snapshots, recovery-chain, rollback checkpoint, and named-backup restores. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return { | ||
| backup, | ||
| name: backup.name, | ||
| reason, | ||
| sortTimestamp: backup.updatedAt ?? backup.createdAt ?? 0, | ||
| }; |
| for (const snapshots of snapshotsByReason.values()) { | ||
| snapshots.sort((left, right) => right.sortTimestamp - left.sortTimestamp); |
test/storage-recovery-paths.test.ts
Outdated
| await new Promise((resolve) => setTimeout(resolve, 20)); | ||
| await fs.writeFile(newerPath, JSON.stringify(payload), "utf-8"); |
| async function trimHistoryFileIfNeeded(paths: SyncHistoryPaths): Promise<PrunedSyncHistory> { | ||
| const entries = await loadHistoryEntriesFromDisk(paths); | ||
| const result = pruneSyncHistoryEntries(entries, MAX_HISTORY_ENTRIES); | ||
| if (result.removed === 0) { | ||
| return result; | ||
| } |
| const prunedHistory = await trimHistoryFileIfNeeded(paths); | ||
| await rewriteLatestEntry(prunedHistory.latest ?? entry, paths); |
lib/codex-manager.ts
Outdated
| details: walPath, | ||
| }); | ||
|
|
||
| const rotatingBackups = await listRotatingBackups(); |
lib/codex-manager.ts
Outdated
| : dirname(storagePath), | ||
| }); | ||
|
|
||
| const snapshotBackups = await listAccountSnapshots(); |
lib/codex-manager.ts
Outdated
| const actionableNamedBackupRestores = await getActionableNamedBackupRestores({ | ||
| currentStorage: storage, | ||
| }); |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/sync-history.ts (1)
311-338:⚠️ Potential issue | 🟠 Major
pruneSyncHistory()is missing the same writer tracking as append.
lib/sync-history.ts:311-338only waits onpendingHistoryWrites, butlib/sync-history.ts:356-385rewrites both history files without registering its promise there. a concurrentreadSyncHistory()can now race a prune and read a truncated file whilefs.writeFile()is in flight. please track prune operations inpendingHistoryWrites, or make reads wait on the history lock too. add a vitest that reads during prune.possible fix
export async function pruneSyncHistory( options: { maxEntries?: number } = {}, ): Promise<{ removed: number; kept: number; latest: SyncHistoryEntry | null }> { const maxEntries = options.maxEntries ?? MAX_HISTORY_ENTRIES; await waitForPendingHistoryWrites(); - return withHistoryLock(async () => { + const prunePromise = withHistoryLock(async () => { const paths = getSyncHistoryPaths(); await ensureHistoryDir(paths.directory); const entries = await loadHistoryEntriesFromDisk(paths); const result = pruneSyncHistoryEntries(entries, maxEntries); if (result.entries.length === 0) { @@ return { removed: result.removed, kept: result.entries.length, latest: result.latest, }; }); + pendingHistoryWrites.add(prunePromise); + try { + return await prunePromise; + } finally { + pendingHistoryWrites.delete(prunePromise); + } }As per coding guidelines,
lib/**: focus on windows filesystem io, and concurrency; verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 356-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sync-history.ts` around lines 311 - 338, The pruneSyncHistory implementation does not register its file-write promise with the existing pendingHistoryWrites tracking used by appendSyncHistory, allowing readSyncHistory to race with in-flight rewrites; update pruneSyncHistory to push its write/rewrite Promise into the same pendingHistoryWrites queue (and remove it when settled) or reuse the same helper the append path uses so readSyncHistory's await waitForPendingHistoryWrites() will block until pruning completes; update tests by adding a vitest that concurrently calls readSyncHistory() while pruneSyncHistory() is running (simulate slow fs.writeFile and Windows EBUSY/429 scenarios) to assert no truncated reads and ensure the new queue handling is resilient to EBUSY/429 retries per lib/** 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`:
- Around line 3446-3463: The recovery-chain check currently treats storageExists
as a recovery artifact which makes addCheck(... key: "recovery-chain") report
success even when walExists, validRotatingBackups, and validSnapshots are all
empty; change the success condition and message to base "ok" only on walExists
|| validRotatingBackups.length > 0 || validSnapshots.length > 0 (remove
storageExists from the predicate), update the message/details accordingly, and
add a vitest that asserts when storageExists is true but wal/rotating/snapshots
are absent the check is "warn" (test name: "storage exists, backup artifacts
missing"); run/update any queues modified to handle EBUSY/429 retry semantics
per lib/** guidelines.
In `@lib/storage.ts`:
- Around line 2248-2280: The loop in
getLatestManualCodexCliRollbackSnapshotNames stops early when a manual "changed"
run has no rollbackSnapshot because it uses "if (!snapshotName) { break; }" —
change that to skip that entry and continue searching (e.g., remove the break
and continue to the next older entry) so the function returns the most recent
manual changed run that actually has a snapshot name; update the logic around
run.rollbackSnapshot?.name trimming and null-checking accordingly, and add a
vitest titled "latest manual changed run has no snapshot; previous one does"
that feeds readSyncHistory results with a newer manual changed run missing
rollbackSnapshot and an older manual changed run with a snapshot to assert the
returned Set contains the older snapshot name.
- Around line 2293-2300: The default for keepLatestPerReason in
pruneAutoGeneratedSnapshots currently uses 1 which conflicts with exported
ACCOUNT_SNAPSHOT_RETENTION_PER_REASON (3); update pruneAutoGeneratedSnapshots to
default keepLatestPerReason to ACCOUNT_SNAPSHOT_RETENTION_PER_REASON (use
options.keepLatestPerReason ?? ACCOUNT_SNAPSHOT_RETENTION_PER_REASON) so callers
without overrides follow the exported policy, and add a vitest that calls
pruneAutoGeneratedSnapshots() with no overrides (mocking listNamedBackups as
needed) to assert it keeps ACCOUNT_SNAPSHOT_RETENTION_PER_REASON per reason.
In `@lib/sync-history.ts`:
- Around line 247-279: The deletes in rewriteLatestEntry and
trimHistoryFileIfNeeded currently call fs.rm(..., {force:true}) directly and
must be replaced with the same retry-on-transient-fs-error loop used elsewhere
in this module (the existing remove-with-retries logic used in the later delete
block around the history-trimming code) so that EBUSY/EPERM/EACCES transient
Windows lock errors are retried before failing; update rewriteLatestEntry and
the branch in trimHistoryFileIfNeeded that calls fs.rm(paths.historyPath) to
invoke that retry loop (or extract the existing loop into a helper used by both
places), keep the same force and mode semantics, and ensure the functions still
return the same PrunedSyncHistory behavior; add a vitest that simulates
transient fs.rm failures (throwing EBUSY/EPERM for the first N attempts then
succeeding) to verify the retry behavior and include assertions covering the
failure-to-remove and eventual-success cases.
In `@test/codex-manager-cli.test.ts`:
- Around line 13-14: The tests currently never exercise listAccountSnapshotsMock
(and listNamedBackupsMock) and only assert
rollback-checkpoint/actionable-restore; add deterministic vitest cases that call
the CLI/doctor diagnostics entry (the test that triggers the
snapshot-backup/wal/rotating-backup/recovery-chain branches) and assert exact
JSON output for each branch, explicitly invoking listAccountSnapshotsMock and
listNamedBackupsMock with controlled return values to simulate Windows
backup-discovery and rotation scenarios; use vitest mocks/promises (vi.fn()
returning resolved/rejected Promises) and deterministic timers
(vi.useFakeTimers/advanceTimersToNextTimer where needed) to reproduce
token-refresh/concurrency races and ensure snapshot-backup emits the expected
JSON shape so these paths are covered and non-flaky.
In `@test/storage-recovery-paths.test.ts`:
- Around line 797-822: The test currently only checks one pruned and one kept
snapshot which can pass under a weaker retention behavior; update the test that
calls pruneAutoGeneratedSnapshots() (in the "prunes older auto snapshots while
keeping the latest per reason and manual backups" case and the similar rollback
case) to either pass an explicit keepLatestPerReason override to
pruneAutoGeneratedSnapshots(keepLatestPerReason: number) so the intended
multi-slot retention is enforced, or expand assertions to assert the full
retained set (i.e., that result.kept contains all expected snapshot names
including the newest per reason and any rollback-referenced snapshot(s) and that
result.pruned contains all older snapshots), referencing
pruneAutoGeneratedSnapshots, keepLatestPerReason, result.kept and result.pruned
to locate where to change the test.
In `@test/sync-history.test.ts`:
- Around line 173-208: Add a deterministic concurrency regression test in
test/sync-history.test.ts that simulates an interleaving between
appendSyncHistoryEntry and pruneSyncHistory (or pruneSyncHistoryEntries) using
deferred promises so Vitest can control the scheduling; specifically locate the
existing prune test around pruneSyncHistoryEntries and create a new case that
pauses the prune mid-write, then runs appendSyncHistoryEntry to add a newer
entry, resumes the prune, and asserts that readLatestSyncHistorySync (and any
latest pointer) reflects the newest recordedAt, that the newest entry is not
removed, and that removed counts are correct — ensure the test uses Vitest's
deterministic async helpers (deferred promise) rather than timing-based waits.
---
Outside diff comments:
In `@lib/sync-history.ts`:
- Around line 311-338: The pruneSyncHistory implementation does not register its
file-write promise with the existing pendingHistoryWrites tracking used by
appendSyncHistory, allowing readSyncHistory to race with in-flight rewrites;
update pruneSyncHistory to push its write/rewrite Promise into the same
pendingHistoryWrites queue (and remove it when settled) or reuse the same helper
the append path uses so readSyncHistory's await waitForPendingHistoryWrites()
will block until pruning completes; update tests by adding a vitest that
concurrently calls readSyncHistory() while pruneSyncHistory() is running
(simulate slow fs.writeFile and Windows EBUSY/429 scenarios) to assert no
truncated reads and ensure the new queue handling is resilient to EBUSY/429
retries per lib/** 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: a79a5fa5-9100-4a62-8be0-e0af40b1c8b1
📒 Files selected for processing (6)
lib/codex-manager.tslib/storage.tslib/sync-history.tstest/codex-manager-cli.test.tstest/storage-recovery-paths.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/codex-manager-cli.test.tstest/storage-recovery-paths.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/codex-manager.tslib/storage.ts
✅ Actions performedComments resolved and changes approved. |
ndycode
force-pushed
the
fresh/15-sync-rollback
branch
from
e522b04 to
1b45f34
Compare
ndycode
force-pushed
the
fresh/16-retention-doctor
branch
from
be0837b to
60b3541
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
test/storage-recovery-paths.test.ts (1)
832-977:⚠️ Potential issue | 🟠 Majorassert the full retained set, not just one pin and one prune.
test/storage-recovery-paths.test.ts:832-977 only proves that the rollback-pinned snapshot survives and one older snapshot disappears. that still passes if retention regresses below the default contract already asserted at test/storage-recovery-paths.test.ts:979-1006. after the final snapshot/prune, assert the exact kept/pruned snapshot names on disk so rollback pinning cannot silently collapse the other retained slots.
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-recovery-paths.test.ts` around lines 832 - 977, The test currently only checks that the rollback-pinned snapshot survives and that one older snapshot is removed; update the assertions after the call to pruneAutoGeneratedSnapshots() (in the same test block using snapshotAccountStorage(), appendSyncHistoryEntry(), getNamedBackupsDirectoryPath()) to assert the full exact set of retained and pruned snapshot names: compare result.kept.map(e => e.name) strictly against the expected array of snapshot names, and then loop over the expected-kept list to assert existsSync(path) === true and over the expected-pruned list to assert existsSync(path) === false so the test fails if rollback pinning collapses other retention slots.test/codex-manager-cli.test.ts (1)
3555-3685:⚠️ Potential issue | 🟠 Majorcover the remaining doctor branches.
test/codex-manager-cli.test.ts:3555-3685 still leaves the wal, rotating-backup, and snapshot-backup checks unexercised.
listAccountSnapshotsMockis added at test/codex-manager-cli.test.ts:17, wired at test/codex-manager-cli.test.ts:144, and reset at test/codex-manager-cli.test.ts:639-653, but none of the new cases drive it. add deterministic--jsonassertions that seedlistAccountSnapshotsMockandlistRotatingBackupsMock, otherwise those doctor diagnostics can regress without a failing vitest.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 3555 - 3685, The tests are missing deterministic branches for the wal, rotating-backup, and snapshot-backup doctor checks; seed the appropriate mocks (listAccountSnapshotsMock and listRotatingBackupsMock, and where applicable getLatestCodexCliSyncRollbackPlanMock/getStoragePathMock) before importing and calling runCodexMultiAuthCli(["auth","doctor","--json"]) so the doctor JSON includes those checks, then assert their severity and details (e.g., wal=true/false, rotating-backup entries, snapshot-backup paths). Locate and update the test cases around runCodexMultiAuthCli in codex-manager-cli.test.ts to add explicit mockResolvedValueOnce/ mockReturnValueOnce calls for listAccountSnapshotsMock and listRotatingBackupsMock and deterministic expectations on payload.checks entries like "recovery-chain", "rotating-backup" and "snapshot-backup".lib/codex-manager.ts (1)
3423-3444:⚠️ Potential issue | 🟠 Majorkeep backup-directory probes from aborting
doctor.both
await listAccountSnapshots()inlib/codex-manager.ts:3423-3444andawait getActionableNamedBackupRestores()inlib/codex-manager.ts:3597-3616can still throw out ofrunDoctor().lib/storage.ts:1860-1926andlib/storage.ts:2063-2156bubble non-enoentscan failures, so a locked or unreadable backups directory on windows turnscodex auth doctorinto a hard failure instead of anerrorcheck. catch each probe, downgrade it to diagnostics, and add a vitest regression intest/codex-manager-cli.test.tsfor the unreadable-backup-dir path.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 3597-3616
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 3423 - 3444, The snapshot probe calls listAccountSnapshots() and getActionableNamedBackupRestores() can throw and should not escape runDoctor(); wrap each call in a try/catch, and on error create an addCheck entry for the same keys ("snapshot-backups" and the restore-related check used around getActionableNamedBackupRestores) with severity "diagnostic" (instead of letting the exception bubble), a concise message that includes the caught error.message, and set details to the backup directory path (getNamedBackupsDirectoryPath()) or the error stack as appropriate; ensure the try/catch blocks sit immediately around the await calls in the code paths referencing listAccountSnapshots and getActionableNamedBackupRestores so runDoctor continues, and add a Vitest regression in test/codex-manager-cli.test.ts that simulates an unreadable/locked backup directory (e.g., stub the storage scan to throw EBUSY or an EPERM/ENOTDIR) and asserts the CLI returns an "error" or "diagnostic" check instead of throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage.ts`:
- Around line 52-55: The AUTO_SNAPSHOT_NAME_PATTERN and
AUTO_SNAPSHOT_TIMESTAMP_PATTERN are too permissive and will treat any
user-created name matching accounts-<reason>-snapshot-... as auto-generated;
update matching to only accept the canonical reasons (use the
AccountSnapshotReason enum/union) or persist and check an explicit marker when
creating snapshots in createNamedBackup(), then change listAccountSnapshots()
and pruneAutoGeneratedSnapshots() to use the restricted matcher or marker check
instead of the current regex; add a vitest that creates a manual backup named
like the old pattern and asserts it is NOT pruned, and update/annotate affected
tests (vitest) for lib/** to cover EBUSY/429 retry behavior where applicable.
- Around line 2272-2304: getLatestManualCodexCliRollbackSnapshotNames currently
swallows all errors and returns an empty Set, which causes retention to prune
rollback pins on transient read failures; update
getLatestManualCodexCliRollbackSnapshotNames to treat ENOENT as "no history"
(return new Set()), but for any other error (EPERM/EBUSY/other) propagate/return
a sentinel (e.g. throw or return null/undefined) so callers can abort retention;
then update pruneAutoGeneratedSnapshots to check the result from
getLatestManualCodexCliRollbackSnapshotNames and abort the pruning run if it
receives the sentinel (do not proceed with an empty preserve set), and add a
vitest that mocks sync-history.readSyncHistory to throw and asserts
snapshotAccountStorage/pruneAutoGeneratedSnapshots leaves the pinned snapshot
intact; reference the functions getLatestManualCodexCliRollbackSnapshotNames,
pruneAutoGeneratedSnapshots, snapshotAccountStorage, and
sync-history.readSyncHistory in your changes and tests.
In `@lib/sync-history.ts`:
- Around line 365-375: The append to history inside
trackPendingHistoryWrite/withHistoryLock currently calls fs.appendFile directly
(see getSyncHistoryPaths, ensureHistoryDir, trimHistoryFileIfNeeded,
rewriteLatestEntry and lastAppendError) and must be wrapped with the same
retry+backoff logic used for rename/remove so transient Windows EBUSY/EPERM
doesn't drop entries; implement a retry loop that catches EBUSY and EPERM (and
related transient IO codes), backs off exponentially with a max attempts/time,
logs non-sensitive context on each retry, sets lastAppendError only after final
failure, and preserves the existing flow to call trimHistoryFileIfNeeded and
rewriteLatestEntry on success; add a vitest that stubs fs.appendFile to fail
once with EBUSY/EPERM then succeed to validate the recovery path referenced by
test/storage-recovery-paths.test.ts, and ensure no sensitive tokens/emails are
emitted in logs.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 3423-3444: The snapshot probe calls listAccountSnapshots() and
getActionableNamedBackupRestores() can throw and should not escape runDoctor();
wrap each call in a try/catch, and on error create an addCheck entry for the
same keys ("snapshot-backups" and the restore-related check used around
getActionableNamedBackupRestores) with severity "diagnostic" (instead of letting
the exception bubble), a concise message that includes the caught error.message,
and set details to the backup directory path (getNamedBackupsDirectoryPath()) or
the error stack as appropriate; ensure the try/catch blocks sit immediately
around the await calls in the code paths referencing listAccountSnapshots and
getActionableNamedBackupRestores so runDoctor continues, and add a Vitest
regression in test/codex-manager-cli.test.ts that simulates an unreadable/locked
backup directory (e.g., stub the storage scan to throw EBUSY or an
EPERM/ENOTDIR) and asserts the CLI returns an "error" or "diagnostic" check
instead of throwing.
In `@test/codex-manager-cli.test.ts`:
- Around line 3555-3685: The tests are missing deterministic branches for the
wal, rotating-backup, and snapshot-backup doctor checks; seed the appropriate
mocks (listAccountSnapshotsMock and listRotatingBackupsMock, and where
applicable getLatestCodexCliSyncRollbackPlanMock/getStoragePathMock) before
importing and calling runCodexMultiAuthCli(["auth","doctor","--json"]) so the
doctor JSON includes those checks, then assert their severity and details (e.g.,
wal=true/false, rotating-backup entries, snapshot-backup paths). Locate and
update the test cases around runCodexMultiAuthCli in codex-manager-cli.test.ts
to add explicit mockResolvedValueOnce/ mockReturnValueOnce calls for
listAccountSnapshotsMock and listRotatingBackupsMock and deterministic
expectations on payload.checks entries like "recovery-chain", "rotating-backup"
and "snapshot-backup".
In `@test/storage-recovery-paths.test.ts`:
- Around line 832-977: The test currently only checks that the rollback-pinned
snapshot survives and that one older snapshot is removed; update the assertions
after the call to pruneAutoGeneratedSnapshots() (in the same test block using
snapshotAccountStorage(), appendSyncHistoryEntry(),
getNamedBackupsDirectoryPath()) to assert the full exact set of retained and
pruned snapshot names: compare result.kept.map(e => e.name) strictly against the
expected array of snapshot names, and then loop over the expected-kept list to
assert existsSync(path) === true and over the expected-pruned list to assert
existsSync(path) === false so the test fails if rollback pinning collapses other
retention slots.
🪄 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: 060da95e-7667-441c-ba59-49f248e5e8e1
📒 Files selected for processing (6)
lib/codex-manager.tslib/storage.tslib/sync-history.tstest/codex-manager-cli.test.tstest/storage-recovery-paths.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)
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/codex-manager.tslib/storage.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-recovery-paths.test.tstest/codex-manager-cli.test.tstest/sync-history.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/storage.ts (1)
955-956:⚠️ Potential issue | 🟠 Majorretry wal reads the same way you retry backup reads.
lib/storage.ts:955-956andlib/storage.ts:3117-3143still use rawfs.readFile()even thoughretryTransientFilesystemOperation()already exists atlib/storage.ts:2052-2070. on windows a transientEPERM/EBUSYfrom av or indexing will make the doctor path mark a good journal invalid and the recovery path skip a recoverable wal. please route both reads through the retry helper and add regressions intest/storage-recovery-paths.test.tsandtest/codex-manager-cli.test.ts.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 3117-3143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 955 - 956, The code uses raw fs.readFile() in the WAL/doctor and recovery code (the reads at the call sites shown as the variables raw/parsed and the block around 3117-3143) instead of the existing retry helper retryTransientFilesystemOperation; replace those direct fs.readFile(...) calls with calls to retryTransientFilesystemOperation(() => fs.readFile(...)) so transient EPERM/EBUSY on Windows are retried, and add/adjust vitest regressions in test/storage-recovery-paths.test.ts and test/codex-manager-cli.test.ts to simulate EPERM/EBUSY and ensure the recovery/doctor paths succeed; also ensure any new queue logic you add handles EBUSY and 429 backoff semantics per existing patterns in retryTransientFilesystemOperation and update tests to assert correct backoff/retry behavior.test/storage-recovery-paths.test.ts (1)
56-62:⚠️ Potential issue | 🟡 Minorrestore vitest spies in shared cleanup.
test/storage-recovery-paths.test.ts:57 resets sync-history state, but it never calls
vi.restoreAllMocks(). test/storage-recovery-paths.test.ts:1089 relies on manualmockRestore(), so an early assertion failure will leak the spy into later cases and make the suite order-dependent. add global mock cleanup here.as per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage-recovery-paths.test.ts` around lines 56 - 62, The afterEach cleanup in test/storage-recovery-paths.test.ts currently resets sync history and storage state but does not restore Vitest spies; add a call to vi.restoreAllMocks() inside the same afterEach block (alongside __resetSyncHistoryForTests(), configureSyncHistoryForTests(null), setStoragePathDirect(null), setStorageBackupEnabled(true), and await removeWithRetry(workDir, ...)) so any spies/mocks are restored between tests and do not leak into later cases like the manual mockRestore() at line ~1089.
♻️ Duplicate comments (3)
lib/storage.ts (1)
2342-2365:⚠️ Potential issue | 🟠 Majorthis still doesn't abort retention when sync-history read fails.
lib/storage.ts:2342-2344callsreadSyncHistory(), butlib/sync-history.ts:432-439already swallows non-ENOENTfailures and returns[]. that means thenullsentinel fromlib/storage.ts:2360-2365never fires on the failures that matter, andlib/storage.ts:2406-2413still drops rollback pins after a transient or corrupted history read. please use a strict history read here, and cover it in the rollback-pin suite attest/storage-recovery-paths.test.ts:856-977.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 2342 - 2365, Replace the lenient history read in storage.ts (the call to syncHistoryModule.readSyncHistory) with a strict read that propagates errors so failures don't get treated as an empty history; call the strict variant (e.g., syncHistoryModule.readSyncHistoryStrict or syncHistoryModule.readSyncHistory({ strict: true }) depending on available API) inside the same block so that thrown errors reach the catch and allow returning the null sentinel, ensuring rollback pins are not dropped; update the rollback-pin test suite (test/storage-recovery-paths.test.ts:856-977) to cover the strict-read behavior and reference this change in the test, and ensure any new code follows project IO/concurrency guidance (handle EBUSY/429 scenarios) when adding the strict read.lib/codex-manager.ts (1)
3398-3401:⚠️ Potential issue | 🟠 Majorguard backup probes so
doctordegrades to diagnostics instead of crashing.lib/codex-manager.ts:3398 (
listRotatingBackups/listAccountSnapshots) and lib/codex-manager.ts:3601 (getActionableNamedBackupRestores) still await backup discovery without a guard. a locked or unreadable backup directory on windows will throw out ofcodex auth doctorbefore it can emit the new recovery checks. catch each probe separately, surface anerrorcheck instead, and add a vitest that forcesEPERM/EBUSYon the backup dir.as per coding guidelines,
lib/**: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 3601-3603
test/storage-recovery-paths.test.ts (1)
876-938:⚠️ Potential issue | 🟠 Majorassert the full retained set in the rollback-pin cases.
test/storage-recovery-paths.test.ts:876 and test/storage-recovery-paths.test.ts:940 still only prove that the pinned snapshot survives and that one middle snapshot is pruned. these cases still pass if rollback-pin retention regresses from “pinned + latest 3” to “pinned + latest 1”. assert the exact
kept/prunednames, or drivepruneAutoGeneratedSnapshots()with the expected default explicitly.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.Also applies to: 940-1021
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage-recovery-paths.test.ts` around lines 876 - 938, The test currently only checks that the pinned snapshot exists and one middle snapshot was removed; change it to deterministically assert the entire set of retained vs pruned snapshot names: after creating snapshots with snapshotAccountStorage + appendSyncHistoryEntry, call pruneAutoGeneratedSnapshots() (so the test drives pruning deterministically) or explicitly list files in getNamedBackupsDirectoryPath() and assert that the resulting filenames equal the expected kept set (e.g., firstSnapshot.name plus the latest N snapshot filenames) and that all other generated snapshot filenames were pruned; reference snapshotAccountStorage, appendSyncHistoryEntry, getNamedBackupsDirectoryPath, and pruneAutoGeneratedSnapshots when locating where to add the deterministic assertions.
🤖 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 4810-4825: The CLI currently maps operational errors in the
backup/restore flows to "dismissed" (silently returning 0) instead of signaling
failure; update the handlers that call select<BackupDetailAction> (the "Preview
Restore" flow) and the reassess/list/restore handlers to return the "failed"
BackupDetailAction on real operational errors (especially filesystem errors like
EPERM/EBUSY and transient concurrency/429 cases) instead of "dismissed", and
ensure the top-level exit logic that inspects the return from these flows (the
codex auth restore-backup path) treats "failed" as a non-zero exit; add a vitest
that calls runCodexMultiAuthCli(["auth","restore-backup"]) to assert non-zero
exit for simulated EPERM/EBUSY and other operational failures and update any
affected tests to expect the new behavior.
In `@lib/storage.ts`:
- Around line 2459-2462: enforceSnapshotRetention() currently calls
pruneAutoGeneratedSnapshots() without the active storage root, causing pruning
to use getStoragePath() and miss snapshots created under an overridden
options.storagePath; update enforceSnapshotRetention() and
pruneAutoGeneratedSnapshots() to accept and propagate a storagePath parameter
(or options object) and ensure all callers (including where snapshots are
created e.g. the snapshot creation function that uses options.storagePath) pass
the same storagePath so pruning targets the correct directory; add a vitest in
test/storage-recovery-paths.test.ts that creates a snapshot under a custom root
and verifies prune removes it, and when adding any new queue or retry logic for
pruning/snapshot operations ensure they handle EBUSY and 429 by retry/backoff
per project concurrency/IO guidelines.
- Around line 3483-3487: The reset-marker unlink currently swallows errors
(best-effort) after a successful save; replace the try/catch
unlink(resetMarkerPath) at both occurrences with the existing
unlinkWithRetry(resetMarkerPath) call and if unlinkWithRetry still fails throw
an error to fail the save so the intentional-reset marker cannot hide newly
written data; update the save/flush paths that reference resetMarkerPath (the
save routine and the companion cleanup block) to propagate the unlinkWithRetry
rejection instead of ignoring it, and add a vitest in
test/storage-recovery-paths.test.ts that stubs fs.unlink (or the platform
wrapper) to make the first unlink attempt fail with a Windows-style error code
(e.g., EPERM or EBUSY) to assert the save fails when the marker cannot be
removed.
In `@lib/sync-history.ts`:
- Around line 181-188: The read-tail loop currently decodes each raw chunk to
UTF-8 before reassembling lines, which corrupts multibyte characters split
across chunks; change the logic in the loop that uses position, chunkSize,
buffer, remainder and combined so you treat remainder and the newly read data as
Buffers, find the last newline byte boundary in the concatenated Buffer, keep
the trailing partial bytes in remainder (as a Buffer), decode only the complete
portion to string and then split into lines to push into matchesNewestFirst with
respect to limit; add a unicode-boundary regression test in
test/sync-history.test.ts that reproduces a multibyte character split across
chunks, and update related tests to reference this change and ensure any new
retry/queue code handles EBUSY and 429 scenarios per the lib/** guidelines.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 955-956: The code uses raw fs.readFile() in the WAL/doctor and
recovery code (the reads at the call sites shown as the variables raw/parsed and
the block around 3117-3143) instead of the existing retry helper
retryTransientFilesystemOperation; replace those direct fs.readFile(...) calls
with calls to retryTransientFilesystemOperation(() => fs.readFile(...)) so
transient EPERM/EBUSY on Windows are retried, and add/adjust vitest regressions
in test/storage-recovery-paths.test.ts and test/codex-manager-cli.test.ts to
simulate EPERM/EBUSY and ensure the recovery/doctor paths succeed; also ensure
any new queue logic you add handles EBUSY and 429 backoff semantics per existing
patterns in retryTransientFilesystemOperation and update tests to assert correct
backoff/retry behavior.
In `@test/storage-recovery-paths.test.ts`:
- Around line 56-62: The afterEach cleanup in
test/storage-recovery-paths.test.ts currently resets sync history and storage
state but does not restore Vitest spies; add a call to vi.restoreAllMocks()
inside the same afterEach block (alongside __resetSyncHistoryForTests(),
configureSyncHistoryForTests(null), setStoragePathDirect(null),
setStorageBackupEnabled(true), and await removeWithRetry(workDir, ...)) so any
spies/mocks are restored between tests and do not leak into later cases like the
manual mockRestore() at line ~1089.
---
Duplicate comments:
In `@lib/storage.ts`:
- Around line 2342-2365: Replace the lenient history read in storage.ts (the
call to syncHistoryModule.readSyncHistory) with a strict read that propagates
errors so failures don't get treated as an empty history; call the strict
variant (e.g., syncHistoryModule.readSyncHistoryStrict or
syncHistoryModule.readSyncHistory({ strict: true }) depending on available API)
inside the same block so that thrown errors reach the catch and allow returning
the null sentinel, ensuring rollback pins are not dropped; update the
rollback-pin test suite (test/storage-recovery-paths.test.ts:856-977) to cover
the strict-read behavior and reference this change in the test, and ensure any
new code follows project IO/concurrency guidance (handle EBUSY/429 scenarios)
when adding the strict read.
In `@test/storage-recovery-paths.test.ts`:
- Around line 876-938: The test currently only checks that the pinned snapshot
exists and one middle snapshot was removed; change it to deterministically
assert the entire set of retained vs pruned snapshot names: after creating
snapshots with snapshotAccountStorage + appendSyncHistoryEntry, call
pruneAutoGeneratedSnapshots() (so the test drives pruning deterministically) or
explicitly list files in getNamedBackupsDirectoryPath() and assert that the
resulting filenames equal the expected kept set (e.g., firstSnapshot.name plus
the latest N snapshot filenames) and that all other generated snapshot filenames
were pruned; reference snapshotAccountStorage, appendSyncHistoryEntry,
getNamedBackupsDirectoryPath, and pruneAutoGeneratedSnapshots when locating
where to add the deterministic assertions.
🪄 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: c7a435f3-5899-4ce5-9e9a-59a00681cf85
📒 Files selected for processing (6)
lib/codex-manager.tslib/storage.tslib/sync-history.tstest/codex-manager-cli.test.tstest/storage-recovery-paths.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-recovery-paths.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/sync-history.tslib/storage.ts
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
lib/storage.ts (1)
2304-2336:⚠️ Potential issue | 🟠 Majordon't fall back to name-only pruning when callers pass
backups.
lib/storage.ts:2420-2427loses theLoadedBackupCandidatemetadata, soparseAutoSnapshot()inlib/storage.ts:2304-2336falls back to the filename regex again. that reopens the manual-lookalike prune bug for callers that passoptions.backups(for exampleawait listNamedBackups()): a user-created backup named likeaccounts-...-snapshot-...can still be deleted. carrybackupKind/snapshotReasononNamedBackupMetadata, or reload candidates before pruning, and add a vitest for thebackups:code path.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 2405-2427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 2304 - 2336, parseAutoSnapshot currently re-parses the filename when callers pass plain NamedBackupMetadata (causing loss of LoadedBackupCandidate.backupKind/snapshotReason) which allows user-created files named like auto snapshots to be pruned; to fix, ensure callers that pass options.backups (e.g., listNamedBackups) supply the original LoadedBackupCandidate or augment NamedBackupMetadata to carry backupKind and snapshotReason so parseAutoSnapshot's candidate parameter retains those fields; update the types (NamedBackupMetadata) and the code paths that build backup lists (where backups are constructed around lines referenced) to preserve/attach backupKind and snapshotReason instead of dropping them, or alternatively reload candidates before pruning, and add a vitest covering the backups: code path to assert user-created names are not treated as auto snapshots.
🤖 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 222-239: publishCodexCliSyncRun currently updates in-memory state
(lastCodexCliSyncRun/lastCodexCliSyncRunRevision) then fire-and-forgets
appendSyncHistoryEntry, creating a race where getLatestCodexCliSyncRollbackPlan
(and other rollback builders) only consult persisted ndjson history and may not
see the just-published run; fix by making the commit durable before returning:
either await appendSyncHistoryEntry inside publishCodexCliSyncRun
(propagating/logging errors) or modify rollback planners (functions that read
history: getLatestCodexCliSyncRollbackPlan and the code paths around lines
315-350, 400-422, 514-524) to first consult lastCodexCliSyncRun in-memory
checkpoint when history is not yet updated, and update tests in
test/codex-cli-sync.test.ts:1283-1428 to assert rollback planning immediately
after commit; ensure handling of EBUSY/429 and follow lib/** concurrency
guidelines when adding awaits or queues.
- Around line 424-478: The loadRollbackSnapshot function currently does a single
fs.readFile and treats transient Windows EBUSY/EPERM as a permanent failure;
update loadRollbackSnapshot to reuse the storage-side retry/backoff helper used
for snapshot saves (e.g., the same retryReadFile or withStorageRetry utility
used elsewhere) so that fs.readFile(snapshot.path, "utf-8") is retried on
transient errors (EBUSY, EPERM and transient 429-like conditions if your retry
helper treats them) before returning "unavailable"; preserve the existing
error-labeling via getRedactedFilesystemErrorLabel and keep the same return
shape but only fail after the retries are exhausted. Also add a vitest in
test/codex-cli-sync.test.ts that simulates a transient EBUSY/EPERM on the first
N reads and succeeds thereafter to assert loadRollbackSnapshot eventually
returns status "ready" and accountCount, and include a negative test where
retries exhaust to assert "unavailable."
In `@lib/codex-manager.ts`:
- Line 112: The module-level boolean destructiveActionInFlight only prevents
reentry inside the current process; update the code to either (A) document this
limitation next to the destructiveActionInFlight declaration so callers know it
does not protect against concurrent CLI invocations, or (B) implement
cross-process coordination by acquiring a filesystem lock (e.g. lock file with
atomic create/truncate) or by leveraging the existing write-ahead-log mechanism
before starting restore/delete/reset flows (ensure acquisition, release, and
error handling are added around the critical sections that reference
destructiveActionInFlight).
In `@lib/storage.ts`:
- Around line 2197-2233: writeNamedBackupFromStorage currently writes the
snapshot directly to backupPath; change it to write to a temp staging file
(e.g., backupPath + ".tmp-{random}") in the same directory, fs.writeFile that
temp file, then atomically commit it by calling renameFileWithRetry(tempPath,
backupPath) and remove the temp on failure; update any other snapshot writer(s)
in the 2520-2556 area to use the same staging+rename pattern and ensure
buildNamedBackupMetadata(name, backupPath) is called only after successful
rename; add a vitest regression that simulates a Windows-style EBUSY/EPERM on
the first rename/write and verifies renameFileWithRetry succeeds (and that the
final snapshot is complete), and reference the modified function name
writeNamedBackupFromStorage and renameFileWithRetry in the test description.
In `@lib/sync-history.ts`:
- Line 196: The call to fs.open(historyPath, "r") inside readHistoryTail can
fail immediately on transient Windows locks (EBUSY/EPERM); wrap the open call in
the same retry/backoff loop used elsewhere (or extract the existing retry
helper) so readHistoryTail retries on transient errors before giving up,
checking for the same error codes and backoff timing, then proceed with the
existing read loop once the handle is acquired.
- Around line 463-467: The code at the return path uses
loadHistoryEntriesFromDisk (no retry) which causes transient
EBUSY/EPERM/EACCES/EAGAIN read failures on Windows; change the call in the
branch that currently does parsed = await loadHistoryEntriesFromDisk(paths) to
use loadHistoryEntriesFromDiskWithRetry(paths) so unlimited sync-history reads
benefit from the 5-attempt retry logic (preserve the subsequent filtering and
cloneEntry mapping). Also add a unit test that mocks fs.readFile to throw a
single EBUSY (or EPERM) error on first attempt then succeed on retry—mirroring
the existing append retry test pattern—to cover the retry behavior in
test/sync-history.test.ts.
- Around line 94-108: The shutdown system isn't aware of inflight history writes
so append operations can be lost; import registerCleanup (from the shutdown
module) into the file containing waitForPendingHistoryWrites and call
registerCleanup(waitForPendingHistoryWrites) after that function is defined (or
before exports) so the shutdown runner drains pendingHistoryWrites; ensure you
add the import for registerCleanup and pass the waitForPendingHistoryWrites
function reference (no wrapping promise) to registerCleanup so
pendingHistoryWrites are awaited on exit.
---
Duplicate comments:
In `@lib/storage.ts`:
- Around line 2304-2336: parseAutoSnapshot currently re-parses the filename when
callers pass plain NamedBackupMetadata (causing loss of
LoadedBackupCandidate.backupKind/snapshotReason) which allows user-created files
named like auto snapshots to be pruned; to fix, ensure callers that pass
options.backups (e.g., listNamedBackups) supply the original
LoadedBackupCandidate or augment NamedBackupMetadata to carry backupKind and
snapshotReason so parseAutoSnapshot's candidate parameter retains those fields;
update the types (NamedBackupMetadata) and the code paths that build backup
lists (where backups are constructed around lines referenced) to preserve/attach
backupKind and snapshotReason instead of dropping them, or alternatively reload
candidates before pruning, and add a vitest covering the backups: code path to
assert user-created names are not treated as auto snapshots.
🪄 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: d31a4b6d-2221-4b3e-9b5a-bd310743403c
📒 Files selected for processing (8)
lib/codex-cli/sync.tslib/codex-manager.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)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/storage.tslib/codex-cli/sync.tslib/sync-history.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-cli-sync.test.ts
🔇 Additional comments (6)
lib/sync-history.ts (1)
1-12: lgtm on the overall structure and retry patterns.the atomic write path with temp+rename at
lib/sync-history.ts:388-405, retry helpers atlib/sync-history.ts:305-372, and mutex coordination atlib/sync-history.ts:85-92are solid. the logging doesn't leak tokens or emails. the addressed past review comments (append retry, remove retry, utf-8 boundary handling) look correctly implemented.lib/codex-manager.ts (5)
3482-3493: recovery-chain check logic now correctly excludesstorageExists.
lib/codex-manager.ts:3484-3491now bases the "ok" severity onvalidRotatingBackups.length > 0 || validSnapshots.length > 0without includingstorageExists. this addresses the past review comment about the check being green when there's nothing to actually recover from.
5210-5214: backup browser exit code mapping looks correct.
lib/codex-manager.ts:5211-5213properly returns exit code 1 whenrunBackupBrowserManagerreturns"failed", and 0 otherwise. this addresses the past review comment about propagating restore failures to the CLI exit code.
4284-4287: logging uses redacted error labels, no pii leaks detected.
lib/codex-manager.ts:4284-4287and similar recovery scan paths usegetRedactedFilesystemErrorLabel(error)before logging, which suggests intentional redaction of potentially sensitive path info. the changed logging statements don't leak tokens or emails.
4032-4042: no action needed:resolveStartupRecoveryActionhas direct test coverage.test/recovery.test.ts:783 contains a dedicated describe block for
resolveStartupRecoveryActionwith test cases validating all three branches (show-recovery-prompt, continue-with-oauth, open-empty-storage-menu). the function's startup recovery logic is directly tested and meets coding guidelines.> Likely an incorrect or invalid review comment.
3598-3629: targetPath filtering is present—the underlying concern does not apply.
lib/codex-cli/sync.ts:413shows thatfindLatestManualRollbackRunexplicitly filters by targetPath usingmatchesRollbackTargetPath(run.targetPath, storagePath)before returning a rollback run. the function correctly skips runs that don't match the current storage path, so the rollback checkpoint check atlib/codex-manager.ts:3600will only report "ready" for snapshots matching the active storage path. no issue here.
| function publishCodexCliSyncRun( | ||
| run: CodexCliSyncRun, | ||
| revision: number, | ||
| ): boolean { | ||
| if (revision <= lastCodexCliSyncRunRevision) { | ||
| return false; | ||
| } | ||
| lastCodexCliSyncRunRevision = revision; | ||
| lastCodexCliSyncRun = cloneCodexCliSyncRun(run); | ||
| void appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: run.runAt, | ||
| run: cloneCodexCliSyncRun(run), | ||
| }).catch((error) => { | ||
| log.debug("Failed to record codex-cli sync history", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
make sync-run commit durable before rollback consumes it.
lib/codex-cli/sync.ts:231-239 publishes the run in memory and then fire-and-forgets appendSyncHistoryEntry(). lib/codex-cli/sync.ts:400-422 and lib/codex-cli/sync.ts:514-524 build rollback plans from persisted history only. that leaves a race where a manual apply is committed, getLastCodexCliSyncRun() shows the checkpoint, but getLatestCodexCliSyncRollbackPlan() still says unavailable because the ndjson append has not finished yet. either await the append in the commit path or consult the in-memory run first. the current coverage in test/codex-cli-sync.test.ts:1283-1428 checks the happy path after commit, but it does not assert rollback planning in the same tick.
As per coding guidelines, lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.
Also applies to: 315-350, 400-422, 514-524
🤖 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 222 - 239, publishCodexCliSyncRun
currently updates in-memory state
(lastCodexCliSyncRun/lastCodexCliSyncRunRevision) then fire-and-forgets
appendSyncHistoryEntry, creating a race where getLatestCodexCliSyncRollbackPlan
(and other rollback builders) only consult persisted ndjson history and may not
see the just-published run; fix by making the commit durable before returning:
either await appendSyncHistoryEntry inside publishCodexCliSyncRun
(propagating/logging errors) or modify rollback planners (functions that read
history: getLatestCodexCliSyncRollbackPlan and the code paths around lines
315-350, 400-422, 514-524) to first consult lastCodexCliSyncRun in-memory
checkpoint when history is not yet updated, and update tests in
test/codex-cli-sync.test.ts:1283-1428 to assert rollback planning immediately
after commit; ensure handling of EBUSY/429 and follow lib/** concurrency
guidelines when adding awaits or queues.
| type PromptTone = "accent" | "success" | "warning" | "danger" | "muted"; | ||
| const log = createLogger("codex-manager"); | ||
|
|
||
| let destructiveActionInFlight = false; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
destructiveActionInFlight only guards within a single process.
lib/codex-manager.ts:112 uses a module-level boolean to prevent reentry during restore/delete/reset flows. this works for the interactive menu loop but won't protect against concurrent CLI invocations from separate terminals. consider documenting this limitation or, if cross-process safety is needed, using a lock file or the existing WAL mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` at line 112, The module-level boolean
destructiveActionInFlight only prevents reentry inside the current process;
update the code to either (A) document this limitation next to the
destructiveActionInFlight declaration so callers know it does not protect
against concurrent CLI invocations, or (B) implement cross-process coordination
by acquiring a filesystem lock (e.g. lock file with atomic create/truncate) or
by leveraging the existing write-ahead-log mechanism before starting
restore/delete/reset flows (ensure acquisition, release, and error handling are
added around the critical sections that reference destructiveActionInFlight).
| async function waitForPendingHistoryWrites(): Promise<void> { | ||
| if (pendingHistoryWrites.size === 0) return; | ||
| await Promise.allSettled(Array.from(pendingHistoryWrites)); | ||
| } | ||
|
|
||
| function trackPendingHistoryWrite<T>(promise: Promise<T>): Promise<T> { | ||
| const trackedPromise = promise.then( | ||
| () => undefined, | ||
| () => undefined, | ||
| ); | ||
| pendingHistoryWrites.add(trackedPromise); | ||
| return promise.finally(() => { | ||
| pendingHistoryWrites.delete(trackedPromise); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "waitForPendingHistoryWrites" --type tsRepository: ndycode/codex-multi-auth
Length of output: 338
🏁 Script executed:
sed -n '445,465p' lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 603
🏁 Script executed:
sed -n '490,510p' lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 870
🏁 Script executed:
sed -n '525,545p' lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 684
🏁 Script executed:
sed -n '225,240p' lib/codex-cli/sync.tsRepository: ndycode/codex-multi-auth
Length of output: 525
🏁 Script executed:
rg -n "process\.on|exitHandler|graceful|cleanup|SIGTERM|SIGINT" --type ts lib/Repository: ndycode/codex-multi-auth
Length of output: 3951
🏁 Script executed:
fd -e ts lib/ -x grep -l "export.*main\|export.*run" {}Repository: ndycode/codex-multi-auth
Length of output: 389
🏁 Script executed:
rg -n "registerCleanupFunction\|import.*shutdown" lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
cat -n lib/shutdown.tsRepository: ndycode/codex-multi-auth
Length of output: 1472
🏁 Script executed:
rg -n "registerCleanupFunction" --type ts lib/Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
rg -n "registerCleanup\|import.*shutdown" lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
rg "registerCleanup" --type ts lib/ -A 2 -B 2Repository: ndycode/codex-multi-auth
Length of output: 495
🏁 Script executed:
rg "appendSyncHistoryEntry" --type ts lib/ -B 3 -A 3Repository: ndycode/codex-multi-auth
Length of output: 1936
🏁 Script executed:
fd . lib/codex-cli --type f -name "*.ts" | xargs grep -l "main\|runCleanup\|export"Repository: ndycode/codex-multi-auth
Length of output: 300
🏁 Script executed:
rg "runCleanup" --type ts lib/Repository: ndycode/codex-multi-auth
Length of output: 224
🏁 Script executed:
find lib/codex-cli -name "*.ts" -type fRepository: ndycode/codex-multi-auth
Length of output: 167
🏁 Script executed:
rg "appendSyncHistoryEntry" lib/codex-cli/sync.ts -B 10 -A 10Repository: ndycode/codex-multi-auth
Length of output: 1281
🏁 Script executed:
rg "waitForPendingHistoryWrites\|publishCodexCliSyncRun" test/ --type tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
rg "appendSyncHistoryEntry" test/ --type ts -A 3Repository: ndycode/codex-multi-auth
Length of output: 9497
🏁 Script executed:
rg "publishCodexCliSyncRun" test/ --type tsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
wc -l lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 91
🏁 Script executed:
sed -n '1,100p' lib/sync-history.ts | head -60Repository: ndycode/codex-multi-auth
Length of output: 1838
🏁 Script executed:
rg "void appendSyncHistoryEntry\|publishCodexCliSyncRun" test/ --type ts -B 2 -A 2Repository: ndycode/codex-multi-auth
Length of output: 50
fire-and-forget appends may be lost on quick shutdown.
lib/codex-cli/sync.ts:231 calls void appendSyncHistoryEntry(...) without awaiting. the pendingHistoryWrites tracker at lib/sync-history.ts:55 and waitForPendingHistoryWrites() at lib/sync-history.ts:94-97 exist to drain inflight writes, but sync-history never registers with the shutdown system in lib/shutdown.ts. the calls to waitForPendingHistoryWrites at lib/sync-history.ts:454, 499, and 534 are in normal operations (read, prune, test reset), not in shutdown paths. if the process exits shortly after publishCodexCliSyncRun() fires—e.g., via error or signal—the history entry is silently dropped.
register waitForPendingHistoryWrites as a cleanup function via registerCleanup() from lib/shutdown.ts:6 to ensure pending writes complete before exit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sync-history.ts` around lines 94 - 108, The shutdown system isn't aware
of inflight history writes so append operations can be lost; import
registerCleanup (from the shutdown module) into the file containing
waitForPendingHistoryWrites and call
registerCleanup(waitForPendingHistoryWrites) after that function is defined (or
before exports) so the shutdown runner drains pendingHistoryWrites; ensure you
add the import for registerCleanup and pass the waitForPendingHistoryWrites
function reference (no wrapping promise) to registerCleanup so
pendingHistoryWrites are awaited on exit.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
lib/storage.ts (3)
2304-2315:⚠️ Potential issue | 🟠 Major
options.backupsreopens the manual-lookalike prune path.
lib/storage.ts:2308-2314falls back to filename-only detection whencandidateis absent, andlib/storage.ts:2413-2417takes that path whenever callers passoptions.backups.pruneAutoGeneratedSnapshots({ backups: await listNamedBackups() })can still delete a manual backup named likeaccounts-...-snapshot-.... please require on-disk provenance before pruning, or skip ambiguous entries, and add a vitest neartest/storage-recovery-paths.test.ts:835-874that passes preloaded backup metadata and asserts the manual lookalike survives.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 2409-2431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 2304 - 2315, The prune path currently uses filename-only detection in parseAutoSnapshot(candidate undefined) which allows pruneAutoGeneratedSnapshots when callers pass options.backups to accidentally delete manual files; change parseAutoSnapshot and pruneAutoGeneratedSnapshots to require on-disk provenance (i.e., LoadedBackupCandidate.present/metadata or a flag from listNamedBackups that proves the backup was generated) before treating an entry as auto-generated—if provenance is missing, treat it as ambiguous and skip pruning; update callers that pass options.backups to supply the preloaded LoadedBackupCandidate metadata instead of relying on filename heuristics, and add a vitest near test/storage-recovery-paths.test.ts (around 835–874) that calls pruneAutoGeneratedSnapshots({ backups: await listNamedBackups() }) with a preloaded manual-lookalike candidate and asserts it is not deleted.
2197-2233:⚠️ Potential issue | 🟠 Majormake snapshot writes atomic and retryable.
lib/storage.ts:2212-2232writes the snapshot straight to its final path. a transient windowseperm/ebusyor a short write leaves rollback checkpoints partially written exactly wheresnapshotAccountStorage()expects them to be recoverable. stage to a temp file and commit it withrenameFileWithRetry(), then add a vitest intest/storage-recovery-paths.test.tsthat forces the first commit attempt to fail.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 2197 - 2233, The current writeNamedBackupFromStorage writes directly to the final backupPath which can leave partial files on Windows; change writeNamedBackupFromStorage to write the JSON to a temporary file in the same directory (e.g., backupPath + ".tmp-<random>"), fs.writeFile the temp file with mode 0o600, then atomically move it to backupPath using renameFileWithRetry() (referencing resolveNamedBackupPath and buildNamedBackupMetadata) so commits are atomic and retriable; also add a vitest in test/storage-recovery-paths.test.ts that simulates a failing first rename (force renameFileWithRetry to throw once) and verifies the function retries and final backup exists and metadata from buildNamedBackupMetadata is correct.
2527-2538:⚠️ Potential issue | 🟠 Majoruse the resolved storage path when enforcing retention.
lib/storage.ts:2536-2538computesresolvedStoragePath, butlib/storage.ts:2566-2568still passes the raw option back into retention. insidewithAccountStorageTransaction(), a latersetStoragePathDirect()change will write the snapshot understate.storagePathand prune undergetStoragePath(), which can target the wrong backup root. please passresolvedStoragePathhere and add a vitest intest/storage-recovery-paths.test.tsthat flips the global storage path mid-transaction.suggested fix
- await enforceSnapshotRetention(storagePath); + await enforceSnapshotRetention(resolvedStoragePath);As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.Also applies to: 2566-2568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 2527 - 2538, The code computes resolvedStoragePath but still passes the original options.storagePath into the retention/prune call inside withAccountStorageTransaction; update the retention enforcement call to pass resolvedStoragePath instead of options.storagePath so pruning uses the actual path that will be used when setStoragePathDirect writes the snapshot (references: variable resolvedStoragePath, function withAccountStorageTransaction, method setStoragePathDirect, helper getStoragePath). Add a vitest in test/storage-recovery-paths.test.ts that starts a transaction, flips the global storage path mid-transaction (to exercise the resolved vs global path behavior), and asserts that backups are written and pruned under resolvedStoragePath.lib/codex-cli/sync.ts (1)
229-247:⚠️ Potential issue | 🟠 Majormake the sync run durable before exposing it.
lib/codex-cli/sync.ts:229-247publisheslastCodexCliSyncRunand then fire-and-forgetsappendSyncHistoryEntry().lib/codex-cli/sync.ts:404-425andlib/codex-cli/sync.ts:543-552build rollback plans from persisted history only, so an immediategetLatestCodexCliSyncRollbackPlan()can still report"unavailable"for a just-committed manual apply. please await the append on the commit path, or consult the in-memory run as a fallback, and add a same-tick regression intest/codex-cli-sync.test.ts.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 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 229 - 247, publishCodexCliSyncRun currently sets lastCodexCliSyncRun and returns before the appendSyncHistoryEntry completes, which allows getLatestCodexCliSyncRollbackPlan to see history as "unavailable"; change publishCodexCliSyncRun to await appendSyncHistoryEntry (or on failure, persist the run in-memory as a durable fallback) before updating lastCodexCliSyncRunRevision/lastCodexCliSyncRun and returning, and update getLatestCodexCliSyncRollbackPlan to consult the in-memory lastCodexCliSyncRun if history is not yet persisted; also add a same-tick regression test in test/codex-cli-sync.test.ts that commits a manual apply and immediately calls getLatestCodexCliSyncRollbackPlan to assert it returns the expected rollback plan.lib/sync-history.ts (1)
94-97:⚠️ Potential issue | 🟠 Majordrain pending history writes during shutdown.
lib/sync-history.ts:94-97only waits when a caller explicitly invokes the helper. becauselib/codex-cli/sync.ts:238-246still records history in the background, a fast exit can drop the last entry after the in-memory run is already visible. please register this drain with the shutdown cleanup path and add a vitest intest/sync-history.test.tsthat exercisesvoid appendSyncHistoryEntry(...)followed by cleanup.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sync-history.ts` around lines 94 - 97, The shutdown path doesn't drain pendingHistoryWrites, so background history writes started via appendSyncHistoryEntry can be lost on fast exits; modify the shutdown/cleanup registration to call waitForPendingHistoryWrites() (ensuring pendingHistoryWrites and waitForPendingHistoryWrites are imported/accessible) and await its completion during process shutdown/cleanup; add a vitest in test/sync-history.test.ts that calls void appendSyncHistoryEntry(...), triggers the cleanup/shutdown routine you registered, and asserts the history entry was flushed; ensure the new cleanup handles transient filesystem/concurrency failures per project guidelines (retry/backoff for EBUSY/429) and reference waitForPendingHistoryWrites, pendingHistoryWrites, appendSyncHistoryEntry in the test and shutdown registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage.ts`:
- Around line 3721-3733: Modify snapshotAndClearAccounts so it fails closed:
call snapshotAccountStorage inside a try/catch and if it throws (or returns a
failure) do NOT call clearAccountsUnlocked and propagate/return an error/false
so the on-disk storage is preserved; update the implementation that currently
calls snapshotAccountStorage then immediately clearAccountsUnlocked (functions:
snapshotAndClearAccounts, snapshotAccountStorage, clearAccountsUnlocked,
loadAccountsInternal, withStorageLock) to abort on snapshot errors. Add a vitest
(storage-recovery-paths.test.ts) that stubs/mocks snapshotAccountStorage to
throw (or simulate EBUSY/EPERM), asserts clearAccountsUnlocked is not invoked
and that the original storage remains on disk, and ensure the test covers
concurrent/lock behavior via withStorageLock where applicable.
In `@test/codex-cli-sync.test.ts`:
- Around line 2493-2544: The test reads cached sync state via
getLastCodexCliSyncRun() without clearing the in-memory cache, so it can miss
persistence bugs; update the failing test(s) (referencing
commitCodexCliSyncRunFailure and getLastCodexCliSyncRun) to either call
__resetLastCodexCliSyncRunForTests() and re-check getLastCodexCliSyncRun() or
directly call readSyncHistory() to validate the persisted record (runAt,
outcome, message, summary) after committing the failure; ensure you add the same
post-reset/assert pattern for the other affected range that uses these helpers
so assertions verify persisted state rather than the in-memory cache.
- Around line 576-654: The test relies on a 500ms mtime delta which is flaky on
coarse filesystems; update the test to avoid sub-second deltas by either mocking
the timestamp read path (spy on storageModule.getLastAccountsSaveTimestamp and
writerModule.getLastCodexCliSelectionWriteTimestamp to return deterministic
epoch values) or increase the file mtime gap to >1000ms (e.g., use targetTime =
sourceTime + 1500ms) so the comparison in previewCodexCliSync is stable; locate
the calls around previewCodexCliSync, and adjust the utimes/writeFile setup or
the vi.spyOn mocks accordingly to ensure deterministic behavior.
- Around line 808-881: The test currently sets storageModule.getStoragePath to
an invalid string ("\0busy-target") which triggers path validation instead of
simulating a transient Windows read error; instead restore a valid path and mock
the filesystem stat call to throw a transient error (EBUSY/EPERM) so
previewCodexCliSync exercises the retry logic. Concretely, keep the vi.spyOn
calls to storageModule.getLastAccountsSaveTimestamp and
writerModule.getLastCodexCliSelectionWriteTimestamp, do not change
storageModule.getStoragePath to an invalid value, and replace that line with a
mock of fs.stat or fs.promises.stat (using vi.spyOn) that throws an error object
with code "EBUSY" or "EPERM" once (or for the desired number of retries) before
returning a successful stat, so previewCodexCliSync sees a transient unreadable
target rather than an invalid-path validation failure.
- Around line 2027-2147: The test currently asserts only that warnSpy was called
with an object containing attempt/maxAttempts/error, but doesn't ensure
filesystem paths were redacted; update the assertion after rollback to also
inspect the full logged payload (from warnSpy.mock.calls[0][1]) and assert it
does NOT contain the raw temp path or filename (e.g., join(tempDir,
"openai-codex-accounts.json") or tempDir) to guarantee redaction; reference
warnSpy, transientError, saveSpy and the existing
expect(warnSpy).toHaveBeenCalledWith(...) and add a negative assertion on the
serialized payload (or payload fields) to ensure no leaked path appears.
---
Duplicate comments:
In `@lib/codex-cli/sync.ts`:
- Around line 229-247: publishCodexCliSyncRun currently sets lastCodexCliSyncRun
and returns before the appendSyncHistoryEntry completes, which allows
getLatestCodexCliSyncRollbackPlan to see history as "unavailable"; change
publishCodexCliSyncRun to await appendSyncHistoryEntry (or on failure, persist
the run in-memory as a durable fallback) before updating
lastCodexCliSyncRunRevision/lastCodexCliSyncRun and returning, and update
getLatestCodexCliSyncRollbackPlan to consult the in-memory lastCodexCliSyncRun
if history is not yet persisted; also add a same-tick regression test in
test/codex-cli-sync.test.ts that commits a manual apply and immediately calls
getLatestCodexCliSyncRollbackPlan to assert it returns the expected rollback
plan.
In `@lib/storage.ts`:
- Around line 2304-2315: The prune path currently uses filename-only detection
in parseAutoSnapshot(candidate undefined) which allows
pruneAutoGeneratedSnapshots when callers pass options.backups to accidentally
delete manual files; change parseAutoSnapshot and pruneAutoGeneratedSnapshots to
require on-disk provenance (i.e., LoadedBackupCandidate.present/metadata or a
flag from listNamedBackups that proves the backup was generated) before treating
an entry as auto-generated—if provenance is missing, treat it as ambiguous and
skip pruning; update callers that pass options.backups to supply the preloaded
LoadedBackupCandidate metadata instead of relying on filename heuristics, and
add a vitest near test/storage-recovery-paths.test.ts (around 835–874) that
calls pruneAutoGeneratedSnapshots({ backups: await listNamedBackups() }) with a
preloaded manual-lookalike candidate and asserts it is not deleted.
- Around line 2197-2233: The current writeNamedBackupFromStorage writes directly
to the final backupPath which can leave partial files on Windows; change
writeNamedBackupFromStorage to write the JSON to a temporary file in the same
directory (e.g., backupPath + ".tmp-<random>"), fs.writeFile the temp file with
mode 0o600, then atomically move it to backupPath using renameFileWithRetry()
(referencing resolveNamedBackupPath and buildNamedBackupMetadata) so commits are
atomic and retriable; also add a vitest in test/storage-recovery-paths.test.ts
that simulates a failing first rename (force renameFileWithRetry to throw once)
and verifies the function retries and final backup exists and metadata from
buildNamedBackupMetadata is correct.
- Around line 2527-2538: The code computes resolvedStoragePath but still passes
the original options.storagePath into the retention/prune call inside
withAccountStorageTransaction; update the retention enforcement call to pass
resolvedStoragePath instead of options.storagePath so pruning uses the actual
path that will be used when setStoragePathDirect writes the snapshot
(references: variable resolvedStoragePath, function
withAccountStorageTransaction, method setStoragePathDirect, helper
getStoragePath). Add a vitest in test/storage-recovery-paths.test.ts that starts
a transaction, flips the global storage path mid-transaction (to exercise the
resolved vs global path behavior), and asserts that backups are written and
pruned under resolvedStoragePath.
In `@lib/sync-history.ts`:
- Around line 94-97: The shutdown path doesn't drain pendingHistoryWrites, so
background history writes started via appendSyncHistoryEntry can be lost on fast
exits; modify the shutdown/cleanup registration to call
waitForPendingHistoryWrites() (ensuring pendingHistoryWrites and
waitForPendingHistoryWrites are imported/accessible) and await its completion
during process shutdown/cleanup; add a vitest in test/sync-history.test.ts that
calls void appendSyncHistoryEntry(...), triggers the cleanup/shutdown routine
you registered, and asserts the history entry was flushed; ensure the new
cleanup handles transient filesystem/concurrency failures per project guidelines
(retry/backoff for EBUSY/429) and reference waitForPendingHistoryWrites,
pendingHistoryWrites, appendSyncHistoryEntry in the test and shutdown
registration.
🪄 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: 2f2f5ac3-789e-4f7f-ba9e-c85131047306
📒 Files selected for processing (7)
lib/codex-cli/sync.tslib/storage.tslib/sync-history.tstest/codex-cli-sync.test.tstest/storage-recovery-paths.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-recovery-paths.test.tstest/codex-cli-sync.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/codex-cli/sync.tslib/storage.ts
Summary
Add snapshot and sync-history retention rules plus the expanded recovery doctor surface.
Current head
devb73f1afLatest follow-up on this head
restore-backuppath withdestructiveActionInFlightValidation
npm run lintnpm run typechecknpm run buildnpm test -- test/storage.test.ts test/storage-recovery-paths.test.ts test/codex-cli-sync.test.ts test/sync-history.test.tsnpm testNotes
Current live correctness blockers on this head are addressed. Remaining comments should be re-evaluated against
b73f1af; performance-only or stale comments are intentionally out of scope for this remediation pass.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 lands snapshot/sync-history retention rules and an expanded recovery doctor surface. the new
lib/sync-history.tsis solid: atomic writes via tmp+rename, windows EPERM/EBUSY retry wrappers on every i/o path, a mutex protecting all writes, and a chunked tail-reader that avoids full-file loads. the core correctness fixes from previous review rounds are in — WAL severity is correctly inverted,commitPendingCodexCliSyncRunpreserves originalrunAt,pruneSyncHistoryno longer clearslastAppendError,findLatestManualRollbackRunnow filters bymatchesRollbackTargetPath, and snapshot pruning uses name-embedded timestamps rather than mtime.restore-backupin the login menu is guarded withdestructiveActionInFlight.three items to address before merging:
deleteAccountAtIndex(lib/destructive-actions.ts:140): the snapshot call explicitly passesstoragePath: getStoragePath(), which bypasses the path thatwithAccountAndFlaggedStorageTransactioncaptured at transaction start. this directly contradicts the PR's stated "pin transaction-scoped snapshots and writes to the storage path captured at transaction start" fix. drop the explicitstoragePathargument so the transaction context provides it.lib/codex-cli/sync.ts:306): the fire-and-forgetreadSyncHistorycall at startup uses.catch(() => undefined), swallowing windows EPERM/EBUSY errors with no log output.getLastCodexCliSyncRun()returnsnullwith no signal and never retries sincelastCodexCliSyncHistoryLoadAttemptedis alreadytrue.destructiveActionInFlightguard (lib/codex-manager.ts:74): two terminal windows against the same profile is the exact windows scenario this guard targets; no test exercises it.Confidence Score: 3/5
Important Files Changed
sortTimestampviaAUTO_SNAPSHOT_TIMESTAMP_PATTERN) rather than mtime for ordering — addressing the previous sort-order concern.unlinkWithRetryandretryTransientFilesystemOperationwrappers added throughout.transactionSnapshotContextextended withstoragePathto pin writes to the path captured at transaction start.writeHistoryFileAtomicallyvia tmp+rename), retry wrappers for all windows EPERM/EBUSY codes, a chunked tail-reader to avoid full-file loads on limit queries, and a mutex (withHistoryLock) serialising all writes.pruneSyncHistorydoes not clearlastAppendError(previous concern resolved). solid implementation.commitPendingCodexCliSyncRunpreserves originalrunAt(previous timestamp-skew concern resolved).findLatestManualRollbackRunnow filters bymatchesRollbackTargetPath(previous targetPath concern resolved). background history read on startup silently drops errors — missing alog.debug.runDoctorsurface (WAL check, rotating/snapshot backup checks, rollback checkpoint, named backup restore count),restore-backupcommand entry point, anddestructiveActionInFlightguard on all three destructive login-menu paths. WAL severity is now correctly inverted (previous concern resolved). no vitest coverage for the concurrent guard scenario.deleteAccountAtIndex,deleteSavedAccounts, andresetLocalStatefrom codex-manager.deleteAccountAtIndexexplicitly passesstoragePath: getStoragePath()tosnapshotAccountStorage, bypassing the transaction context's captured path — inconsistent with the PR's stated goal of pinning snapshots to the path at transaction start.restore-backupmenu entry and wires it through the login menu result type. no issues identified.Sequence Diagram
sequenceDiagram participant Menu as login-menu participant CM as codex-manager participant DA as destructive-actions participant ST as storage.ts participant SH as sync-history.ts participant FS as filesystem Note over Menu,FS: manual codex-cli sync with rollback checkpoint CM->>ST: snapshotAccountStorage(reason="codex-cli-sync") ST->>FS: createNamedBackup (write .json) ST->>ST: enforceSnapshotRetention() ST->>SH: readSyncHistory(kind="codex-cli-sync") SH-->>ST: history entries ST->>ST: getLatestManualCodexCliRollbackSnapshotNames() ST->>ST: pruneAutoGeneratedSnapshots() [sortTimestamp from filename] ST-->>CM: NamedBackupMetadata (snapshot) CM->>SH: appendSyncHistoryEntry(run + rollbackSnapshot) SH->>FS: appendHistoryFileWithRetry (ndjson) SH->>SH: trimHistoryFileIfNeeded → writeHistoryFileAtomically SH->>FS: writeFile(.tmp) → rename (atomic) SH->>FS: rewriteLatestEntry (.tmp → rename) Note over Menu,FS: rollback latest manual sync Menu->>CM: restore-backup (destructiveActionInFlight guard) CM->>SH: findLatestManualRollbackRun() [filtered by targetPath] SH-->>CM: CodexCliSyncRun with rollbackSnapshot CM->>FS: readFile(snapshot.path) [RETRYABLE_ROLLBACK_READ_CODES] FS-->>CM: raw JSON CM->>ST: saveRollbackStorageWithRetry(storage) ST-->>FS: saveAccounts (WAL → write → rename → delete WAL) Note over Menu,FS: deleteAccountAtIndex Menu->>DA: deleteAccountAtIndex(index) DA->>ST: withAccountAndFlaggedStorageTransaction [captures storagePath] DA->>ST: snapshotAccountStorage(reason="delete-account", storagePath=getStoragePath()) Note right of DA: ⚠ bypasses transaction-context storagePath ST->>FS: createNamedBackup DA->>ST: persist(nextStorage, updatedFlagged)Comments Outside Diff (4)
lib/storage.ts, line 984-1011 (link)getLatestManualCodexCliRollbackSnapshotNamesreads from the sync history file only, not the filesystem. it returns the first matchingsnapshotNamefrom history regardless of whether that snapshot file still exists on disk.when that file has been manually deleted (or lost due to a prior prune failure),
preserveNamesinpruneAutoGeneratedSnapshotsgets populated with a ghost name. becausescanNamedBackupsonly lists files that are actually on disk, the ghost name never matches anyautoSnapshot. the entirepreserveNamesguard is silently wasted and the next-oldest rollback checkpoint can fall off thekeepLatestPerReasonwindow and get deleted — leaving the user with no rollback point.to defend against this, skip snapshot names whose files no longer exist before returning:
no vitest test exercises the scenario where the most-recent rollback snapshot file is missing but a previous entry with a live file exists.
Prompt To Fix With AI
lib/storage.ts, line 1 (link)writeNamedBackupFromStoragehas no retry for EPERM/EBUSYwriteNamedBackupFromStoragecallsfs.mkdirandfs.writeFilewith no retry. on windows, antivirus and search indexers routinely hold a write lock on the backup directory or a just-created.jsonfile for 50–200 ms. if either call fails with EPERM/EBUSY,snapshotAccountStoragecatches the error (viafailurePolicy = "warn") and returnsnull. that silently leavescaptureRollbackSnapshot()returningnull, recordingrollbackSnapshot: nullin the sync-history entry for the manual run. the user's only rollback point for that sync is gone with no user-visible feedback.the write pattern used everywhere else in this file (
unlinkWithRetry, the atomic temp→rename path, etc.) should be applied here too. at minimum, wrapfs.writeFilein a retry loop forEPERM/EBUSY:no vitest test covers a transient
EPERMduring snapshot file creation.lib/storage.ts, line 1191 (link)enforceSnapshotRetentionusesoptions.storagePathinstead ofresolvedStoragePathcreateBackupis called withstoragePath: resolvedStoragePath, which correctly resolves to the transaction-pinned path when insidetransactionSnapshotContext. butenforceSnapshotRetention(storagePath)passes the rawoptions.storagePath— which isundefinedwhen no explicit path was given. the default fallback insideenforceSnapshotRetentionisgetStoragePath(), nottransactionState.storagePath.in production this almost never matters because
transactionState.storagePathis captured fromgetStoragePath()at transaction start and the global path doesn't change. but the PR's stated goal is to pin all transaction-scoped operations to the captured path, and this call diverges from that contract. it also means test helpers that spin up transactions with a custom path can silently prune snapshots from the wrong bucket.lib/destructive-actions.ts, line 140-145 (link)snapshot bypasses transaction-context storage path
deleteAccountAtIndexruns insidewithAccountAndFlaggedStorageTransaction, which capturesstoragePath = getStoragePath()at transaction start and pins all writes to it. the snapshot here callsgetStoragePath()again at snapshot time instead of inheriting the transaction context's captured path.snapshotAccountStorageresolves the path as:because an explicit
storagePathoption is provided, the transaction context is bypassed entirely. ifsetStoragePathis called from another async context between transaction start and this snapshot (the mutex serialises storage reads/writes but does not blocksetStoragePath), the snapshot and the persist write can target different paths — silently. the PR's stated goal is specifically "pin transaction-scoped snapshots and writes to the storage path captured at transaction start".drop the explicit
storagePathso the transaction context provides it:Prompt To Fix All With AI
Last reviewed commit: "test(sync): tighten ..."