feat(sync): add sync center and status surface#78
feat(sync): add sync center and status surface#78ndycode wants to merge 17 commits intogit-plan/01-reset-safetyfrom
Conversation
|
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 a preview-first sync center, codex-cli sync orchestration, pending-run commit/failure signaling around storage persistence, live-account-sync snapshot publication, and per-call storage backup control. see Changes
notes:
Sequence Diagram(s)sequenceDiagram
participant user
participant settings as rgba(30,144,255,0.5)
participant preview as rgba(34,139,34,0.5)
participant codex as rgba(255,165,0,0.5)
participant storage as rgba(128,0,128,0.5)
user->>settings: open sync-center
settings->>preview: request preview (previewCodexCliSync)
preview->>codex: load codex-cli state (loadCodexCliState)
codex-->>preview: return state snapshot
preview->>storage: read current storage (getStoragePath/loadAccounts)
storage-->>preview: return storage
preview->>preview: reconcile -> summary + pendingRun
preview-->>settings: display preview
user->>settings: apply
settings->>preview: applyCodexCliSyncToStorage
preview->>storage: saveAccounts(storage, options)
alt save success
storage-->>preview: saved
preview->>codex: commitPendingCodexCliSyncRun(pendingRun)
else save error
storage-->>preview: error
preview->>codex: commitCodexCliSyncRunFailure(pendingRun, error)
end
preview-->>settings: apply complete
sequenceDiagram
participant live as rgba(70,130,180,0.5)
participant watcher as rgba(46,139,87,0.5)
participant snapshotStore as rgba(220,20,60,0.5)
live->>watcher: start watcher / poll
watcher-->>live: change detected / poll tick
live->>live: reload -> compute snapshot
live->>snapshotStore: publish snapshot (guarded by instanceId)
snapshotStore-->>live: snapshot available to consumers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 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 unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
# Conflicts: # index.ts # lib/codex-cli/sync.ts # lib/codex-manager.ts # lib/codex-manager/settings-hub.ts # test/codex-cli-sync.test.ts # test/codex-manager-cli.test.ts # test/settings-hub-utils.test.ts # test/storage.test.ts
…it-plan/04-sync-center # Conflicts: # index.ts # lib/codex-manager.ts # test/codex-manager-cli.test.ts # test/storage.test.ts
|
@greptile @cubic review |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/accounts.test.ts">
<violation number="1" location="test/accounts.test.ts:290">
P3: This test does not verify call order, so it can pass even if the sync run is committed before `saveAccounts` completes.</violation>
</file>
<file name="docs/reference/settings.md">
<violation number="1" location="docs/reference/settings.md:67">
P3: `Sync Center` is nested under `Color Theme` due to heading level; it should be a top-level peer section.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/accounts-load-from-disk.test.ts (1)
47-50:⚠️ Potential issue | 🟡 Minormocked sync responses should include
pendingRunexplicitly.
test/accounts-load-from-disk.test.ts:47-50,:72-75, and:92-95return partial sync payloads withoutpendingRun. includependingRun: nullto matchlib/codex-cli/sync.ts:741-745and avoid hiding contract regressions in load-from-disk paths.Also applies to: 72-75, 92-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/accounts-load-from-disk.test.ts` around lines 47 - 50, The mocked responses for syncAccountStorageFromCodexCli in the tests return partial sync payloads missing the pendingRun field; update each mockResolvedValue call that returns { changed: false, storage: null } to include pendingRun: null so the shape matches the real payload from syncAccountStorageFromCodexCli (as used in lib/codex-cli/sync.ts) and prevents contract regressions in the load-from-disk tests.test/codex-cli-sync.test.ts (1)
46-102: 🧹 Nitpick | 🔵 Trivialconsider adding a regression test for concurrent sync attempts.
test/codex-cli-sync.test.tscovers sequential preview/apply flows but doesn't have a test reproducing concurrentapplyCodexCliSyncToStoragecalls racing. given the pr adds pending-run tracking with revision numbers, a concurrency test would strengthen coverage.as per coding guidelines,
test/**: "demand regression cases that reproduce concurrency bugs."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-cli-sync.test.ts` around lines 46 - 102, Add a regression test that simulates concurrent calls to applyCodexCliSyncToStorage to ensure the pending-run tracking and revision handling prevent races: in test/codex-cli-sync.test.ts add a new test that uses the existing beforeEach setup (mkdtemp paths, mocked storageModule.getStoragePath) and resets state via clearCodexCliStateCache() and __resetLastCodexCliSyncRunForTests(), then invoke applyCodexCliSyncToStorage concurrently (e.g., Promise.all of multiple calls) and assert the expected single-apply outcome by checking the storage target (mocked getStoragePath target file contents or spy on the underlying apply implementation) and that revision/pending-run behavior was honored (no conflicting writes/calls).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/settings.md`:
- Around line 67-79: Update the "Sync Center" section to include a short
upgrade-note reference and list the required npm verification scripts;
specifically, add a one-line pointer to the project upgrade notes (e.g., "See
upgrade notes for sync workflow changes") and append a small "Validation" bullet
listing the commands `npm run typecheck`, `npm run build`, and `npm test` so
rollout docs explicitly require these checks; modify the Sync Center section in
docs/reference/settings.md accordingly and ensure README/SECURITY references
remain consistent with the new sync CLI behavior.
In `@lib/codex-cli/sync.ts`:
- Around line 146-156: The publishCodexCliSyncRun function currently allows a
second publish with the same revision because it uses a strict less-than check;
change the guard in publishCodexCliSyncRun to use <= so equal-revision commits
are rejected, and update the related duplicate-publish logic also referenced
around the other occurrence (lines ~182-210) to mirror this tightened check; add
a new regression test in test/codex-cli-sync.test.ts that attempts to publish
the same revision twice and asserts the second publish returns false and does
not overwrite lastCodexCliSyncRun, and ensure any tests you add or modify
mention this behavior in their descriptions.
- Around line 625-658: The applyCodexCliSyncToStorage function currently
forwards options.forceRefresh as-is to loadCodexCliState, which lets apply run
without a fresh codex-cli read when callers omit the flag; change
applyCodexCliSyncToStorage so that it defaults to forcing a refresh (i.e., pass
forceRefresh: options.forceRefresh ?? true) when calling loadCodexCliState, and
only skip the refresh if the caller explicitly sets forceRefresh to false;
update references in applyCodexCliSyncToStorage and the loadCodexCliState call
accordingly.
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2661-2678: The call to buildPreview (which calls loadAccounts and
previewCodexCliSync) is currently executed outside promptSyncCenter's try/catch
so any failure (e.g., EBUSY on Windows or transient 429) escapes instead of
presenting a recoverable error; wrap both initial and refresh buildPreview
invocations inside try/catch blocks in promptSyncCenter (and any other places
where buildPreview is called, e.g., the second call around the refresh flow) and
on catch: log the error via the same logger used in this module, surface a
user-friendly recoverable error state (so the user can back out), and implement
or reuse a retry/backoff queue that retries on EBUSY/429 (with limited attempts)
before failing. Update/add vitest regression tests
(test/codex-manager-cli.test.ts) to simulate loadAccounts/previewCodexCliSync
throwing EBUSY and 429 and assert promptSyncCenter stays in the error state and
allows retry/exit.
- Around line 2753-2756: The call to saveAccounts(synced.storage, {
backupEnabled }) must be wrapped in withQueuedRetry(...) to handle transient
Windows EBUSY/EPERM errors; currently saveAccounts only uses withStorageLock
(which won't retry) and can surface hard sync failures. Update the sync-center
apply path to call withQueuedRetry(() => saveAccounts(synced.storage, {
backupEnabled })) using the existing retry infrastructure (the withQueuedRetry
implementation and RETRYABLE_SETTINGS_WRITE_CODES used around lines ~707-731) so
the saveAccounts call benefits from the same queued retry logic as other
settings writes rather than calling saveAccounts directly.
- Around line 1337-1347: formatSyncSourceLabel currently compares
preview.sourcePath to context.accountsPath and context.authPath using raw ===
which is brittle on Windows; call the existing normalizePathForComparison (or
implement the same normalize/realpath+lowercase behavior) on preview.sourcePath,
context.accountsPath and context.authPath before comparing in
formatSyncSourceLabel so comparisons are canonical across platforms, and update
the comparisons in that function to use the normalized values; also add a
regression test in test/codex-manager-cli.test.ts that mirrors the existing
posix test but uses Windows-style backslashes and mixed casing to assert the
correct label is returned.
In `@lib/live-account-sync.ts`:
- Around line 30-40: The module-level variable lastLiveAccountSyncSnapshot is
shared across all LiveAccountSync instances causing race conditions; change the
implementation to either enforce single-instance ownership (throw or prevent
construction if an active instance exists) or namespace snapshots by instance id
(e.g., map instanceId -> snapshot) and update getLastLiveAccountSyncSnapshot and
__resetLastLiveAccountSyncSnapshotForTests to accept or use that instance key
(referencing lastLiveAccountSyncSnapshot and those function names). Update
LiveAccountSync constructors/start/stop to set and clear the per-instance
snapshot and add a vitest regression that starts two LiveAccountSync instances
in the same process and asserts correct per-instance snapshot/state when
start/stop events interleave; ensure any new queues used for snapshotting handle
EBUSY/429 retry semantics and that logs avoid leaking tokens/emails.
In `@test/accounts.test.ts`:
- Around line 243-293: The test name promises ordering but only checks both
calls occurred; update the test that calls AccountManager.loadFromDisk to also
assert call order by comparing the mocks' invocationCallOrder so saveAccounts
ran before commitPendingCodexCliSyncRun: read the mocked saveAccounts
(saveAccounts / mockSaveAccounts) and commitPendingCodexCliSyncRunMock
invocationCallOrder values after loadFromDisk and add an expectation that the
saveAccounts invocation index is less than the commitPendingCodexCliSyncRunMock
index (keep the existing assertions for commitCodexCliSyncRunFailureMock
unchanged).
In `@test/live-account-sync.test.ts`:
- Around line 32-36: afterEach currently calls fs.rm(workDir...) which can fail
on Windows with transient EBUSY; change this to use the same retry helper
pattern used in test/codex-cli-sync.test.ts (removeWithRetry) or implement
equivalent retry logic: call removeWithRetry(workDir, { recursive: true, force:
true }) instead of fs.rm, preserving the existing vi.useRealTimers() and
__resetLastLiveAccountSyncSnapshotForTests() calls and ensure the helper retries
on EBUSY/EPERM before finally failing.
---
Outside diff comments:
In `@test/accounts-load-from-disk.test.ts`:
- Around line 47-50: The mocked responses for syncAccountStorageFromCodexCli in
the tests return partial sync payloads missing the pendingRun field; update each
mockResolvedValue call that returns { changed: false, storage: null } to include
pendingRun: null so the shape matches the real payload from
syncAccountStorageFromCodexCli (as used in lib/codex-cli/sync.ts) and prevents
contract regressions in the load-from-disk tests.
In `@test/codex-cli-sync.test.ts`:
- Around line 46-102: Add a regression test that simulates concurrent calls to
applyCodexCliSyncToStorage to ensure the pending-run tracking and revision
handling prevent races: in test/codex-cli-sync.test.ts add a new test that uses
the existing beforeEach setup (mkdtemp paths, mocked
storageModule.getStoragePath) and resets state via clearCodexCliStateCache() and
__resetLastCodexCliSyncRunForTests(), then invoke applyCodexCliSyncToStorage
concurrently (e.g., Promise.all of multiple calls) and assert the expected
single-apply outcome by checking the storage target (mocked getStoragePath
target file contents or spy on the underlying apply implementation) and that
revision/pending-run behavior was honored (no conflicting writes/calls).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ca6b889-3172-4701-8bef-2d303d4f9a0a
📒 Files selected for processing (14)
docs/reference/settings.mdlib/accounts.tslib/codex-cli/sync.tslib/codex-manager/settings-hub.tslib/live-account-sync.tslib/storage.tslib/ui/copy.tstest/accounts-edge.test.tstest/accounts-load-from-disk.test.tstest/accounts.test.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/live-account-sync.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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/accounts-load-from-disk.test.tstest/codex-cli-sync.test.tstest/accounts.test.tstest/live-account-sync.test.tstest/accounts-edge.test.tstest/codex-manager-cli.test.tstest/settings-hub-utils.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/settings.md
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.tslib/live-account-sync.tslib/accounts.tslib/ui/copy.tslib/codex-manager/settings-hub.tslib/codex-cli/sync.ts
🔇 Additional comments (15)
lib/accounts.ts (1)
121-126: good persistence-to-commit sequencing.
lib/accounts.ts:121-126correctly commits the pending run only aftersaveAccountssucceeds and records failure on write errors. this keeps sync status aligned with actual disk state.lib/live-account-sync.ts (1)
68-76: good windows-safe mtime fallback behavior.
lib/live-account-sync.ts:68-76handlesENOENT/EBUSY/EACCESwithout crashing the loop, which is solid for windows filesystem edge cases.lib/storage.ts (1)
1789-2012: nice per-call backup control without global-state mutation.
lib/storage.ts:1789-2012cleanly threadsbackupEnabledthroughsaveAccountsinto the locked write path, so sync callers can opt out of backup creation safely per operation.test/accounts-edge.test.ts (1)
8-9: good mock wiring for new sync commit hooks.
test/accounts-edge.test.ts:8-9,:32-33, and:86-87correctly add and reset the new commit mocks, which keeps edge-branch tests isolated across runs.Also applies to: 32-33, 86-87
test/live-account-sync.test.ts (1)
38-61: test coverage for snapshot lifecycle looks good.the test validates
runningtransitions fromfalse→true→falseacross construct/start/stop. assertions are deterministic and use vitest correctly.test/settings-hub-utils.test.ts (1)
249-306: solid test coverage for sync-center overview rendering.the test at
test/settings-hub-utils.test.ts:249-306validates all nine overview items with fixed inputs. assertions are deterministic and cover preservation counts, rollback paths, and live watcher state.lib/ui/copy.ts (1)
69-69: ui copy additions look correct.all nine
syncCenter*entries atlib/ui/copy.ts:69,113-121are referenced inlib/codex-manager/settings-hub.ts:2680-2725,3195-3200. no tokens or pii in these strings.Also applies to: 113-121
test/codex-cli-sync.test.ts (3)
254-327: preview test validates one-way sync without mutation.
test/codex-cli-sync.test.ts:254-327correctly asserts thatpreviewCodexCliSyncproduces a ready preview with expected counts while leaving the original storage unchanged at line 326.
499-577: timestamp-based selection precedence test is well-structured.
test/codex-cli-sync.test.ts:499-577usesutimesto set explicit mtimes and mocks timestamp functions to isolate the precedence logic. this covers the "newer persisted local selection after restart" scenario.
579-632: commit/failure path coverage looks complete.tests at
test/codex-cli-sync.test.ts:579-632andtest/codex-cli-sync.test.ts:730-779validate that sync runs are only recorded after explicit commit, and failures are recorded with error messages.Also applies to: 730-779
test/codex-manager-cli.test.ts (5)
325-333: sync-center added to menu order.
test/codex-manager-cli.test.ts:327inserts"sync-center"intoSETTINGS_HUB_MENU_ORDER. this matches the actual menu structure inlib/codex-manager/settings-hub.ts.
2619-2663: disabled apply hotkey test validates guard behavior.
test/codex-manager-cli.test.ts:2619-2663asserts that pressing"a"when status is not ready returnsundefinedand does not triggerapplyCodexCliSyncToStorageMock. this covers the blocked apply hotkey requirement.
2665-2796: backup disabled path is well-tested.
test/codex-manager-cli.test.ts:2665-2796validates:
loadPluginConfigMockreturns{ storageBackupEnabled: false }saveAccountsMockis called with{ backupEnabled: false }commitPendingCodexCliSyncRunMockis called after successful save- preview is refreshed after apply (called twice)
2798-2926: save failure test covers EBUSY and commit failure recording.
test/codex-manager-cli.test.ts:2798-2926usesmakeErrnoError("busy", "EBUSY")to simulate windows filesystem contention. assertions verify:
commitPendingCodexCliSyncRunMockis not calledcommitCodexCliSyncRunFailureMockis called once- error message "Failed to save synced storage: busy" appears in menu items
this satisfies the windows filesystem behavior coverage requirement.
132-151: mock setup for sync modules is comprehensive.
test/codex-manager-cli.test.ts:132-151mocks all exported functions fromcodex-cli/sync.js,codex-cli/state.js, andlive-account-sync.js. the mocks are reset in beforeEach at lines 492-512.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/accounts-load-from-disk.test.ts (1)
72-77: 🧹 Nitpick | 🔵 Trivialthe new commit hooks are wired but never asserted.
test/accounts-load-from-disk.test.ts:72andtest/accounts-load-from-disk.test.ts:94still forcependingRun: null, so these paths do not prove thatAccountManager.loadFromDisk()callscommitPendingCodexCliSyncRunonly aftersaveAccounts()succeeds andcommitCodexCliSyncRunFailureon save failure. add one success case and one persist-failure case with a non-nullpendingRunand explicit mock assertions before this contract slips.As per coding guidelines
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.Also applies to: 94-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/accounts-load-from-disk.test.ts` around lines 72 - 77, Tests currently always set pendingRun: null so they never assert commitPendingCodexCliSyncRun/commitCodexCliSyncRunFailure behavior; update test/accounts-load-from-disk.test.ts to add two deterministic cases that set a non-null pendingRun on the mocked syncAccountStorageFromCodexCli response and then assert that AccountManager.loadFromDisk() calls commitPendingCodexCliSyncRun when saveAccounts() (the mocked persister invoked by loadFromDisk) resolves successfully and calls commitCodexCliSyncRunFailure when saveAccounts() rejects; specifically modify the existing vi.mocked(syncAccountStorageFromCodexCli) setup to return a pendingRun object in one test and a pendingRun in the other, stub loadAccounts() and saveAccounts() to simulate success and failure respectively, and add explicit vi assertions for commitPendingCodexCliSyncRun and commitCodexCliSyncRunFailure to prevent regression.
♻️ Duplicate comments (2)
lib/live-account-sync.ts (1)
198-203:⚠️ Potential issue | 🟠 Majornewest-wins snapshotting still goes stale when that instance stops first.
lib/live-account-sync.ts:198permanently suppresses publishes from any older watcher after a newer instance has published once. if instance b stops while instance a is still running,lib/live-account-sync.ts:184records b asrunning: falseand a can never reclaim the exported status surface, so settings can report an idle watcher while a is still active.test/live-account-sync.test.ts:92only covers the inverse ordering. either enforce a single activeliveaccountsyncinstance or namespace the published snapshot by instance id.As per coding guidelines
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/live-account-sync.ts` around lines 198 - 203, The publish logic permanently suppresses older instances because publishSnapshot uses global lastLiveAccountSyncSnapshotInstanceId/lastLiveAccountSyncSnapshot; fix by namespacing snapshots by instance id instead of a single global: replace lastLiveAccountSyncSnapshotInstanceId/lastLiveAccountSyncSnapshot with a map (e.g., lastLiveAccountSyncSnapshots keyed by instanceId), have publishSnapshot() store lastLiveAccountSyncSnapshots[this.instanceId] = this.getSnapshot(), and update any consumers to compute the active snapshot by selecting the appropriate instance (e.g., newest running instance or max instanceId among running entries determined from the running flag) so a newer instance stopping won’t block an older running instance reclaiming exported status; update tests in test/live-account-sync.test.ts to cover the scenario where the newer instance stops first and ensure logging changes do not leak tokens/emails and that new behavior passes concurrency/EBUSY/429-related expectations.lib/codex-cli/sync.ts (1)
556-624:⚠️ Potential issue | 🟠 Majorretryable source-preview failures never reach the retry queue.
lib/codex-cli/sync.ts:613convertsloadCodexCliState()exceptions into{ status: "error" }, solib/codex-manager/settings-hub.ts:2743cannot actually retry source-side429orebusyfailures withwithQueuedRetry(). initial open and manual refresh still fall straight into the red error state even though the retry wrapper is present. please surface retryable preview failures separately — throw them, or return structured retry metadata — and add a vitest regression intest/codex-manager-cli.test.tsfor source-side429andebusy.As per coding guidelines
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-cli/sync.ts` around lines 556 - 624, The preview failure path swallows retryable errors from loadCodexCliState() by converting them into a generic error status, preventing withQueuedRetry() in settings-hub from retrying; modify lib/codex-cli/sync.ts so loadCodexCliState() retryable exceptions (e.g., 429/EBUSY) are surfaced instead of being reduced to status: "error" — either rethrow the error from the try block when isRetryable(error) is true or return a structured result (e.g., { status: "retryable", retryMeta: { code, retryAfter } }) that reconcileCodexCliState()/caller can handle with withQueuedRetry(); update callers (notably the code path that invokes reconcileCodexCliState and settings-hub’s withQueuedRetry usage) to respect the new thrown/structured retry signal and add vitest regression tests in test/codex-manager-cli.test.ts that simulate source-side 429 and EBUSY to verify the retry queue is used and the UI remains in preview/queued state rather than immediate error.
🤖 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 141-156: The bug is that allocateCodexCliSyncRunRevision() is
called at apply-start causing publishCodexCliSyncRun(run, revision) to use
start-time ordering and block later commits; change to assign monotonic revision
at commit/save time instead: stop calling allocateCodexCliSyncRunRevision() when
an apply starts, instead call it inside the commit/save path that persists the
run and then pass that revision into publishCodexCliSyncRun; also maintain a
separate pending-runs map keyed by a temporary id so saves can settle
independently of start order (update functions that reference
nextCodexCliSyncRunRevision/lastCodexCliSyncRunRevision and the
cloneCodexCliSyncRun usage), add a vitest regression in
test/codex-cli-sync.test.ts that exercises overlapping applies where a
later-started run fails before an earlier one completes, and ensure new commit
code handles EBUSY/HTTP 429 retries/backoff and does not log tokens/emails.
In `@test/codex-manager-cli.test.ts`:
- Around line 25-30: The test duplicates the rollback-suffix list by hardcoding
values in formatRollbackPathsMock; to prevent drift when lib/codex-cli/sync.ts
changes, export the suffix list or export formatRollbackPaths from the lib and
have the test import it instead of hardcoding. Update the implementation to
expose a shared constant (e.g., ROLLBACK_SUFFIXES) or make formatRollbackPaths
exported, then replace the hardcoded array in the test (formatRollbackPathsMock)
with a reference to that imported constant/function so the test follows any
future changes in sync.ts.
---
Outside diff comments:
In `@test/accounts-load-from-disk.test.ts`:
- Around line 72-77: Tests currently always set pendingRun: null so they never
assert commitPendingCodexCliSyncRun/commitCodexCliSyncRunFailure behavior;
update test/accounts-load-from-disk.test.ts to add two deterministic cases that
set a non-null pendingRun on the mocked syncAccountStorageFromCodexCli response
and then assert that AccountManager.loadFromDisk() calls
commitPendingCodexCliSyncRun when saveAccounts() (the mocked persister invoked
by loadFromDisk) resolves successfully and calls commitCodexCliSyncRunFailure
when saveAccounts() rejects; specifically modify the existing
vi.mocked(syncAccountStorageFromCodexCli) setup to return a pendingRun object in
one test and a pendingRun in the other, stub loadAccounts() and saveAccounts()
to simulate success and failure respectively, and add explicit vi assertions for
commitPendingCodexCliSyncRun and commitCodexCliSyncRunFailure to prevent
regression.
---
Duplicate comments:
In `@lib/codex-cli/sync.ts`:
- Around line 556-624: The preview failure path swallows retryable errors from
loadCodexCliState() by converting them into a generic error status, preventing
withQueuedRetry() in settings-hub from retrying; modify lib/codex-cli/sync.ts so
loadCodexCliState() retryable exceptions (e.g., 429/EBUSY) are surfaced instead
of being reduced to status: "error" — either rethrow the error from the try
block when isRetryable(error) is true or return a structured result (e.g., {
status: "retryable", retryMeta: { code, retryAfter } }) that
reconcileCodexCliState()/caller can handle with withQueuedRetry(); update
callers (notably the code path that invokes reconcileCodexCliState and
settings-hub’s withQueuedRetry usage) to respect the new thrown/structured retry
signal and add vitest regression tests in test/codex-manager-cli.test.ts that
simulate source-side 429 and EBUSY to verify the retry queue is used and the UI
remains in preview/queued state rather than immediate error.
In `@lib/live-account-sync.ts`:
- Around line 198-203: The publish logic permanently suppresses older instances
because publishSnapshot uses global
lastLiveAccountSyncSnapshotInstanceId/lastLiveAccountSyncSnapshot; fix by
namespacing snapshots by instance id instead of a single global: replace
lastLiveAccountSyncSnapshotInstanceId/lastLiveAccountSyncSnapshot with a map
(e.g., lastLiveAccountSyncSnapshots keyed by instanceId), have publishSnapshot()
store lastLiveAccountSyncSnapshots[this.instanceId] = this.getSnapshot(), and
update any consumers to compute the active snapshot by selecting the appropriate
instance (e.g., newest running instance or max instanceId among running entries
determined from the running flag) so a newer instance stopping won’t block an
older running instance reclaiming exported status; update tests in
test/live-account-sync.test.ts to cover the scenario where the newer instance
stops first and ensure logging changes do not leak tokens/emails and that new
behavior passes concurrency/EBUSY/429-related expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69e33424-5d11-4fca-bf65-3e72ef16f0b8
📒 Files selected for processing (10)
docs/reference/settings.mdlib/codex-cli/sync.tslib/codex-manager/settings-hub.tslib/live-account-sync.tstest/accounts-load-from-disk.test.tstest/accounts.test.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/live-account-sync.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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/accounts-load-from-disk.test.tstest/accounts.test.tstest/live-account-sync.test.tstest/codex-manager-cli.test.tstest/codex-cli-sync.test.tstest/settings-hub-utils.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/settings.md
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/live-account-sync.tslib/codex-manager/settings-hub.tslib/codex-cli/sync.ts
🔇 Additional comments (8)
test/accounts.test.ts (2)
243-298: ordering assertion is now present.the invocationCallOrder check at
test/accounts.test.ts:294-296correctly locks down the save-before-commit ordering. this addresses the past review comment and provides the regression coverage needed for sync-run semantics.
300-354: save failure path is well-covered.test at
test/accounts.test.ts:300-354verifies that when saveAccounts rejects, commitPendingCodexCliSyncRun is not called and commitCodexCliSyncRunFailure is invoked with the correct error. this is a solid regression case for the failure-recording behavior described in the pr objectives.test/codex-manager-cli.test.ts (6)
2633-2677: disabled hotkey test looks good.the test at
test/codex-manager-cli.test.ts:2633-2677verifies that pressingawhen apply is disabled returns undefined and does not trigger apply. deterministic check on the disabled path.
2679-2708: EBUSY recovery on initial preview load is covered.
test/codex-manager-cli.test.ts:2679-2708rejects previewCodexCliSyncMock with EBUSY and expects 4 calls (initial + 3 retries). this locks down the windows retry behavior for sync-center entry.
2900-3010: transient save failure retry is well-tested.
test/codex-manager-cli.test.ts:2900-3010simulates one EBUSY rejection followed by success. assertions at lines 3006-3009 confirm exactly 2 saveAccounts calls, commit success called once, and failure commit not called. solid regression for windows fs contention.
3136-3264: save failure surfacing test is thorough.
test/codex-manager-cli.test.ts:3136-3264exhausts all retry attempts (4 total at line 3260) and verifies:
- commitPendingCodexCliSyncRunMock is not called (line 3261)
- commitCodexCliSyncRunFailureMock is called once (line 3262)
- ui displays "Failed to save synced storage: busy" (line 3246)
this covers the exhausted-retry path and ensures the failure is recorded, not silently dropped.
2767-2898: backupEnabled: false passthrough is verified.test at
test/codex-manager-cli.test.ts:2767-2898loadsstorageBackupEnabled: falsefrom plugin config and asserts saveAccountsMock receives{ backupEnabled: false }at line 2893. this locks down the new per-call SaveAccountsOptions behavior.
3012-3134: apply-time read retry covers transient EBUSY during loadAccounts.
test/codex-manager-cli.test.ts:3012-3134togglesfailNextApplyReadto simulate a single EBUSY on loadAccounts before apply. line 3125 asserts loadAccountsMock was called at least 3 times, confirming the retry occurred. the apply then proceeds and commits successfully.
|
@coderabbitai review Latest fixes are on |
|
✅ Actions performedFull review triggered. |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/settings-hub-utils.test.ts">
<violation number="1" location="test/settings-hub-utils.test.ts:46">
P2: The test-local `buildSyncCenterOverview` context type is stale (`state.accounts`) and no longer matches the real API (`sourceAccountCount`), which undermines type-safety for these new tests.</violation>
</file>
<file name="lib/codex-manager/settings-hub.ts">
<violation number="1" location="lib/codex-manager/settings-hub.ts:2835">
P2: Apply forces a fresh source read, so it can write different data than the preview the user just reviewed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/live-account-sync.ts (1)
121-127:⚠️ Potential issue | 🟠 Majorstale in-flight reloads can corrupt state after a path switch.
lib/live-account-sync.ts:124-167tears down the old watch and immediately rebinds the instance to a new path, but any priorreloadInFlightkeeps running. when that older reload settles,lib/live-account-sync.ts:245-247still writes the old path's mtime/counters into shared instance state andlib/live-account-sync.ts:266republishes a snapshot for the new path. that mixes generations, and it also makes the first reload request on the new path a no-op while the stale promise still occupiesreloadInFlight. please add a generation/token guard around reload completion and cover the interleaving with a vitest that switchessyncToPath()whilereload()is blocked;test/live-account-sync.test.ts:247-275only exercises overlapping reloads on one 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. check for logging that leaks tokens or emails."Also applies to: 170-185, 234-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/live-account-sync.ts` around lines 121 - 127, The instance state is corrupted by stale in-flight reloads: add a generation token that increments in syncToPath() (when switching paths in methods syncToPath/stop) and capture that token inside reload() and any promise assigned to reloadInFlight; on reload completion verify the captured token still matches the instance's current generation before writing lastKnownMtimeMs, counters, or calling publishSnapshot so stale completions are dropped; update references to reloadInFlight, syncToPath, reload, stop, lastKnownMtimeMs and publishSnapshot to use this guard and ensure the first reload on a new path can proceed (i.e., clear or replace reloadInFlight when generation changes); add a vitest in test/live-account-sync.test.ts that blocks reload() (e.g., via a controllable promise), calls syncToPath() to switch paths while the first reload is blocked, then unblocks the first reload and assert no old-path state was written and the new-path reload runs and publishes the correct snapshot.
♻️ Duplicate comments (1)
lib/codex-cli/sync.ts (1)
665-665:⚠️ Potential issue | 🟠 Majorrevision ordering is still start-time based and can publish stale outcomes under overlap.
lib/codex-cli/sync.ts:665allocates revision before persistence, while final publication happens later vialib/codex-cli/sync.ts:181-209. if two applies overlap, a later-started failure can publish first and block an earlier successful completion because oflib/codex-cli/sync.ts:149, leaving last-sync state inconsistent with disk state. add a vitest regression intest/codex-cli-sync.test.tsthat forces out-of-order completion and verifies final published outcome tracks completion, not start order.as per coding guidelines
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.Also applies to: 149-155, 181-209, 739-743
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-cli/sync.ts` at line 665, The revision is currently allocated at start (allocateCodexCliSyncRunRevision) which orders runs by start-time and can publish stale outcomes; move revision allocation to the point of final persistence/publication (the code paths that publish final outcome) so the published revision reflects completion order, and update any callers that assumed pre-allocated revisions (references: allocateCodexCliSyncRunRevision, the publish/finalization functions used around the sync publish code) to accept a generated revision at publish time; add a Vitest regression in test/codex-cli-sync.test.ts that spawns two overlapping sync apply runs, injects engineered delays to force out-of-order completion (simulate EBUSY/429/backoff scenarios), and assert the final published outcome matches the completion that finished last (not the earlier start), also ensure new/changed code handles EBUSY/429 retries and logging does not leak tokens/emails and cite the new test in the commit message.
🤖 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/settings-hub.ts`:
- Around line 2830-2836: The applyCodexCliSyncToStorage call must be executed
inside the queued-retry wrapper so transient source-read errors don't abort the
apply: move the applyCodexCliSyncToStorage(...) invocation into the
withQueuedRetry(preview.targetPath, async () => { ... }) closure (so both
loadAccounts() and applyCodexCliSyncToStorage run under the same retry queue),
ensure the retry logic treats EBUSY/EPERM and HTTP 429 as retryable, and
preserve existing save/target retry semantics; add a Vitest regression in
test/codex-cli-sync.test.ts that simulates one retryable failure (EBUSY/429)
then success on the sync-center apply path to validate the change, and audit
logs emitted around applyCodexCliSyncToStorage/loadAccounts to ensure no tokens
or emails are logged.
In `@test/codex-cli-sync.test.ts`:
- Around line 254-327: The test must assert the preview payload contains no
secret tokens: after obtaining preview from previewCodexCliSync (the preview
variable) serialize it (e.g., JSON.stringify(preview)) and add expectations that
the original token strings ("access-a-new", "access-c", "refresh-a",
"refresh-c") do not appear in that serialized string; place these assertions
immediately after the preview is computed and before final state assertions
(keep the existing current/account checks intact) so the preview surface is
proven token-free.
In `@test/codex-manager-cli.test.ts`:
- Around line 3112-3130: The test currently only asserts call counts; change it
to use the existing createDeferred helper to block the second saveAccountsMock
resolution so you can assert commitPendingCodexCliSyncRunMock is not called
until the second save resolves: identify saveAccountsMock and replace the
.mockResolvedValueOnce(undefined) for the second call with a deferred.promise;
after invoking runCodexMultiAuthCli(["auth","login"]) assert
commitPendingCodexCliSyncRunMock has not been called while the deferred is
unresolved, then resolve the deferred and finally assert
commitPendingCodexCliSyncRunMock was called once and saveAccountsMock call
counts remain as expected; use selectMock and runCodexMultiAuthCli to locate the
flow to test.
---
Outside diff comments:
In `@lib/live-account-sync.ts`:
- Around line 121-127: The instance state is corrupted by stale in-flight
reloads: add a generation token that increments in syncToPath() (when switching
paths in methods syncToPath/stop) and capture that token inside reload() and any
promise assigned to reloadInFlight; on reload completion verify the captured
token still matches the instance's current generation before writing
lastKnownMtimeMs, counters, or calling publishSnapshot so stale completions are
dropped; update references to reloadInFlight, syncToPath, reload, stop,
lastKnownMtimeMs and publishSnapshot to use this guard and ensure the first
reload on a new path can proceed (i.e., clear or replace reloadInFlight when
generation changes); add a vitest in test/live-account-sync.test.ts that blocks
reload() (e.g., via a controllable promise), calls syncToPath() to switch paths
while the first reload is blocked, then unblocks the first reload and assert no
old-path state was written and the new-path reload runs and publishes the
correct snapshot.
---
Duplicate comments:
In `@lib/codex-cli/sync.ts`:
- Line 665: The revision is currently allocated at start
(allocateCodexCliSyncRunRevision) which orders runs by start-time and can
publish stale outcomes; move revision allocation to the point of final
persistence/publication (the code paths that publish final outcome) so the
published revision reflects completion order, and update any callers that
assumed pre-allocated revisions (references: allocateCodexCliSyncRunRevision,
the publish/finalization functions used around the sync publish code) to accept
a generated revision at publish time; add a Vitest regression in
test/codex-cli-sync.test.ts that spawns two overlapping sync apply runs, injects
engineered delays to force out-of-order completion (simulate EBUSY/429/backoff
scenarios), and assert the final published outcome matches the completion that
finished last (not the earlier start), also ensure new/changed code handles
EBUSY/429 retries and logging does not leak tokens/emails and cite the new test
in the commit message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d811762-8c8c-4447-a04a-635b88a55576
📒 Files selected for processing (14)
docs/reference/settings.mdlib/accounts.tslib/codex-cli/sync.tslib/codex-manager/settings-hub.tslib/live-account-sync.tslib/storage.tslib/ui/copy.tstest/accounts-edge.test.tstest/accounts-load-from-disk.test.tstest/accounts.test.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/live-account-sync.test.tstest/settings-hub-utils.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). (3)
- GitHub Check: Greptile Review
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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/accounts-edge.test.tstest/live-account-sync.test.tstest/accounts-load-from-disk.test.tstest/codex-manager-cli.test.tstest/accounts.test.tstest/settings-hub-utils.test.tstest/codex-cli-sync.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/settings.md
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/ui/copy.tslib/live-account-sync.tslib/storage.tslib/codex-manager/settings-hub.tslib/accounts.tslib/codex-cli/sync.ts
🔇 Additional comments (9)
docs/reference/settings.md (1)
67-85: sync-center docs now align with runtime behavior and rollout checks.the section matches the preview-first flow surfaced in
lib/codex-cli/sync.ts:550and settings rendering inlib/codex-manager/settings-hub.ts:2765, and it now includes explicit validation commands.as per coding guidelines
docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.lib/ui/copy.ts (1)
69-69: sync-center copy wiring looks complete.
lib/ui/copy.ts:113-121matches the settings-hub references atlib/codex-manager/settings-hub.ts:2768-2809, so this avoids missing-label/runtime-key drift.Also applies to: 113-121
lib/storage.ts (1)
294-296: per-call backup control is threaded correctly through persistence.
lib/storage.ts:1793-1803andlib/storage.ts:2006-2012preserve default behavior while allowing scoped backup overrides, andlib/storage.ts:294-296gives callers a safe read surface for current backup state.Also applies to: 1789-1803, 2006-2012
lib/accounts.ts (1)
123-125: save/commit sequencing is correct in the load path.
lib/accounts.ts:123-125commits pending sync run only aftersaveAccountssucceeds and records failure only on save error, which is the right ordering contract.test/accounts-edge.test.ts (1)
8-9: mock isolation for the new sync commit hooks is clean.
test/accounts-edge.test.ts:8-9,test/accounts-edge.test.ts:31-35, andtest/accounts-edge.test.ts:86-87properly wire and reset the commit mocks, so cross-test state bleed is avoided.Also applies to: 31-35, 86-87
test/settings-hub-utils.test.ts (2)
19-59: the test api exposure for sync-center overview looks well-scoped.
test/settings-hub-utils.test.ts:19-59cleanly exposes the new overview builder contract for focused utility-level tests.
249-349: good windows and sync-overview regression coverage here.
test/settings-hub-utils.test.ts:249-349adds deterministic checks for rollback labels, watcher/status text, and windows-style source-path matching, which is exactly the right guardrail for this ui surface.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.test/live-account-sync.test.ts (1)
92-139: good interleaved snapshot regression.
test/live-account-sync.test.ts:92-139is the right guardrail for the instance-guarded status surface. it proves an older watcher stopping later does not clobber the newer published state.test/accounts.test.ts (1)
243-354: good post-persist bookkeeping coverage.
test/accounts.test.ts:243-354now locks both the success ordering and the save-error failure path aroundAccountManager.loadFromDisk(). that gives the new sync-run commit/failure helpers real regression coverage.
|
@coderabbitai review Latest fixes are on |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedFull review triggered. |
|
@coderabbitai review Latest fixes are now on |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/live-account-sync.ts (1)
242-290:⚠️ Potential issue | 🟠 Majoroverlapping file changes can be dropped while a reload is in flight.
the fast path at
lib/live-account-sync.ts:246-252just waits for the current reload and returns. if the file changes again afterthis.reload()has already read old contents, the second trigger is discarded, and thenlib/live-account-sync.ts:260-263advanceslastKnownMtimeMsto the newest mtime anyway. that can leave memory stale even though the watcher observed the later write. this needs a dirty flag or a queued second pass after the in-flight reload settles.test/live-account-sync.test.ts:261-289currently expects exactly one reload, so it is locking in the race instead of reproducing it.As per coding guidelines,
lib/**: "focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/live-account-sync.ts` around lines 242 - 290, runReload currently drops overlapping file-change events by returning early when reloadInFlight exists, which lets lastKnownMtimeMs be bumped to a newer mtime after an in-flight reload read older contents; add a pending/dirty indicator and queue a follow-up reload: in runReload (and the reloadInFlight handling) when a reload is in flight and its generation matches, set a boolean like reloadPending or enqueue a second pass instead of returning, then after the in-flight promise settles (in the finally block where you clear this.reloadInFlight) check that flag and run another reload (or call runReload("watch")/triggerQueuedReload) to ensure the later write is re-read before updating lastKnownMtimeMs/publishing snapshot; update tests (test/live-account-sync.test.ts) to expect the queued second pass and ensure any logging still avoids leaking secrets and that the new queue handles transient IO/EBUSY by retrying appropriately.
♻️ Duplicate comments (1)
lib/live-account-sync.ts (1)
93-115:⚠️ Potential issue | 🟠 Majorconstructor-time ordering breaks the most-recently-started watcher contract.
line 114 in
lib/live-account-sync.ts:114assigns the winning order in the constructor, and line 207 inlib/live-account-sync.ts:207uses that order to suppress later publications from "older" instances. if two watchers are constructed up front and then started in reverse order, or an older instance is restarted after a newer one has already published, the later-started watcher can never surface its snapshot. that means the sync-center can show the wrong path/running state.test/live-account-sync.test.ts:106-153only covers construction and start in the same order, so this regression stays invisible. move the monotonic ordering tosyncToPath()/start time and add a reverse-order vitest.As per coding guidelines,
lib/**: "focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails."Also applies to: 206-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/live-account-sync.ts` around lines 93 - 115, The constructor currently assigns the monotonic instance/order (this.instanceId = ++nextLiveAccountSyncInstanceId) too early causing newer-started watchers to be suppressed by older instances; move the monotonic ordering so the increment and assignment of instanceId (or a new startOrder/generation value used by the publication-suppression logic) happens in syncToPath() (or the start path) just before the watcher begins publishing, update any code that compares instanceId/generation (the suppression checks around generation/lastSyncAt/reloadInFlight) to use the new start-time order, and add a vitest that constructs two LiveAccountSync instances then starts them in reverse order to assert the most-recently-started watcher wins and the sync-center reflects the correct path/running state.
🤖 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 475-499: In shouldApplyCodexCliSelection, handle the
unreadable-local case before the zero-version fast path: check if
persistedLocalTimestamp === null immediately after computing localVersion and
return false (so local wins) before evaluating codexVersion <= 0; update the
logic in shouldApplyCodexCliSelection to move the persistedLocalTimestamp null
check ahead of the codexVersion early return and add/adjust vitest coverage in
test/codex-cli-sync.test.ts (the combined case around the existing ranges
~638-706 and ~783-876) to assert that a null persistedLocalTimestamp prevents
the codex selection from overwriting local state on restart.
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2846-2851: The apply path currently reuses sourceState from the
last preview (used in the withQueuedRetry call that invokes
applyCodexCliSyncToStorage), so add a staleness check right before calling
apply: if sourceState is older than a chosen threshold (e.g. compare a timestamp
field on sourceState or preview.updatedAt), then refresh the authoritative state
by calling loadAccounts() (or loadAccounts({forceRefresh: true})) and replace
sourceState before invoking applyCodexCliSyncToStorage; alternatively (or
additionally) update the UI hint text at the existing settings-hub UI location
referenced around settings-hub.ts:2413 to document the “preview-first” semantics
and warn users about possible staleness. Ensure you reference preview.targetPath
and withQueuedRetry exactly as the existing call so the replacement happens in
the same code path.
- Around line 1356-1367: Add a unit test covering mixed-separator Windows-style
paths for the normalizePathForComparison function: create a test that passes a
forward-slash drive-letter path like "C:/Users/neil/.codex/accounts.json" and
assert that the result is lowercased and has normalized slashes (e.g.,
"c:/Users/neil/.codex/accounts.json" normalized to
"c:/Users/neil/.codex/accounts.json" with any trailing slashes removed as
appropriate), and also assert equality between this result and the result for
the equivalent backslash variant ("C:\\Users\\neil\\.codex\\accounts.json") to
confirm case-insensitive matching is triggered even when only forward slashes
are present; add this to the existing settings-hub utils tests alongside the
existing backslash test for normalizePathForComparison.
In `@test/settings-hub-utils.test.ts`:
- Around line 247-304: Add unit tests in test/settings-hub-utils.test.ts that
cover the other sync run outcome branches by calling api.buildSyncCenterOverview
with payloads where lastSync.outcome is "error", "disabled", and "unavailable"
(in addition to the existing "changed"/ready case); assert the overview entries
reflect formatSyncRunOutcome's expected messages (referencing
formatSyncRunOutcome in lib/codex-manager/settings-hub.ts) and any associated
hint/label differences (use the same buildSyncCenterOverview helper to locate
behavior). Ensure each test constructs the minimal summary/liveSync context
needed to exercise the branch and checks the specific label/hint text produced
for that outcome.
- Around line 247-347: Add a regression test that exercises buildPreviewSafely's
queued-retry behavior by mocking loadAccounts (not previewCodexCliSync) to throw
an EBUSY/EPERM error on the first call and succeed on the second; call
buildPreviewSafely (ensuring loadCodexCliState is mocked appropriately to allow
the code path) and assert it returns a valid preview and that loadAccounts was
invoked twice, thus verifying withQueuedRetry retry behavior in the actual
sync-center load path.
---
Outside diff comments:
In `@lib/live-account-sync.ts`:
- Around line 242-290: runReload currently drops overlapping file-change events
by returning early when reloadInFlight exists, which lets lastKnownMtimeMs be
bumped to a newer mtime after an in-flight reload read older contents; add a
pending/dirty indicator and queue a follow-up reload: in runReload (and the
reloadInFlight handling) when a reload is in flight and its generation matches,
set a boolean like reloadPending or enqueue a second pass instead of returning,
then after the in-flight promise settles (in the finally block where you clear
this.reloadInFlight) check that flag and run another reload (or call
runReload("watch")/triggerQueuedReload) to ensure the later write is re-read
before updating lastKnownMtimeMs/publishing snapshot; update tests
(test/live-account-sync.test.ts) to expect the queued second pass and ensure any
logging still avoids leaking secrets and that the new queue handles transient
IO/EBUSY by retrying appropriately.
---
Duplicate comments:
In `@lib/live-account-sync.ts`:
- Around line 93-115: The constructor currently assigns the monotonic
instance/order (this.instanceId = ++nextLiveAccountSyncInstanceId) too early
causing newer-started watchers to be suppressed by older instances; move the
monotonic ordering so the increment and assignment of instanceId (or a new
startOrder/generation value used by the publication-suppression logic) happens
in syncToPath() (or the start path) just before the watcher begins publishing,
update any code that compares instanceId/generation (the suppression checks
around generation/lastSyncAt/reloadInFlight) to use the new start-time order,
and add a vitest that constructs two LiveAccountSync instances then starts them
in reverse order to assert the most-recently-started watcher wins and the
sync-center reflects the correct path/running state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3972432-bca0-4d88-a22d-9a9938a7b1cb
📒 Files selected for processing (7)
lib/codex-cli/sync.tslib/codex-manager/settings-hub.tslib/live-account-sync.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/live-account-sync.test.tstest/settings-hub-utils.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). (3)
- GitHub Check: Greptile Review
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/settings-hub-utils.test.tstest/live-account-sync.test.tstest/codex-manager-cli.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/codex-manager/settings-hub.tslib/codex-cli/sync.tslib/live-account-sync.ts
🔇 Additional comments (7)
lib/codex-manager/settings-hub.ts (5)
304-316: lgtm on the new sync center types.
SyncCenterActionandSyncCenterOverviewContextare well-scoped for the preview-first sync flow. thesourceAccountCount: number | nullnullable pattern correctly handles missing source state.
1282-1325: formatting helpers are clean.good defensive checks on timestamp validity (
Number.isFinite+> 0). no tokens or emails leak through these pure formatters.
1369-1437: buildSyncCenterOverview is well-structured.the overview array cleanly separates concerns (status, paths, live watcher, preview mode, summary, selection, preservation, backup). paths are intentionally shown to the user for transparency. no tokens or emails leak here.
2845-2878: apply flow retry handling looks comprehensive.the apply path correctly wraps both
loadAccounts+applyCodexCliSyncToStoragetogether in onewithQueuedRetryblock (lines 2846-2851), then wrapssaveAccountsin a separate retry block (lines 2856-2860).commitPendingCodexCliSyncRunandcommitCodexCliSyncRunFailureare called after persistence succeeds/fails, which matches the pr objective of "commits sync run state only after persistence succeeds." error messages don't leak tokens or emails.
3381-3384: sync-center menu integration is wired correctly.
promptSyncCenterreceivesbackendConfigsogetStorageBackupEnabled(config)works as expected for per-call backup control.test/settings-hub-utils.test.ts (2)
18-57: SettingsHubTestApi type aligns with production SyncCenterOverviewContext.the
sourceAccountCount: number | nullat line 46 matcheslib/codex-manager/settings-hub.ts:313. the greptile concern about type mismatch ("state vs sourceAccountCount") appears resolved.
306-347: windows path test covers the critical case-insensitive + separator normalization.the test at lines 306-347 validates backslash-to-forward-slash conversion and case-insensitive matching (
C:\\Users\\Neil\\.codex\\Accounts.jsonvsc:/users/neil/.codex/accounts.json). this exerciseslib/codex-manager/settings-hub.ts:1356-1367well.
There was a problem hiding this comment.
1 issue found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/live-account-sync.ts">
<violation number="1" location="lib/live-account-sync.ts:178">
P1: Don’t clear `reloadInFlight` across generations; wait for the old reload to finish before starting a new-generation reload, otherwise the new reload can reuse stale in-flight work and falsely report the new path as synced.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/codex-cli/sync.ts">
<violation number="1" location="lib/codex-cli/sync.ts:398">
P2: The new exhausted-retry `log.debug` is unreachable, so retry exhaustion is still returned as `null` with no diagnostic log.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-cli/sync.ts`:
- Around line 388-400: The exhausted-retry debug log is placed after a return so
it never runs; move the log into the branch that returns null so failures are
recorded before returning (i.e., where the code checks attempt >= 3 and returns
null). Specifically, in the function that reads the persisted local selection
timestamp (the block using attempt, RETRYABLE_SELECTION_TIMESTAMP_CODES,
lastError, and sleep), add a process-safe debug log (using log.debug)
immediately before the return null, including the error text drawn from
lastError (lastError instanceof Error ? lastError.message : String(lastError));
then add a vitest in test/codex-cli-sync.test.ts that simulates four retryable
fs.stat failures (EBUSY/EPERM) and asserts the new debug log path is taken.
Ensure logs do not leak tokens/emails and follow existing logging conventions.
In `@lib/live-account-sync.ts`:
- Around line 245-253: The loop that bails out when it sees an in-flight reload
(using this.reloadInFlight and generation) can miss a concurrent second write
because reload() is opaque; change the logic to track a boolean dirty flag on
the instance (e.g., this._pendingDirty) whenever a write/update arrives during
an in-flight reload, and after awaiting inFlightReload.promise clear/merge the
in-flight pointer and, if dirty was set, run one additional reload pass before
returning (i.e., ensure one follow-up call to reload() after the current promise
settles), then clear the dirty flag; update the while/await block around
this.reloadInFlight, the reload() invocation sites, and the generation check to
reflect this behavior, and replace the single-call assertion in
test/live-account-sync.test.ts (lines ~261-289) with a vitest that defers the
first reload, performs two writes during that defer, and asserts the second
reload pass actually ran.
In `@test/codex-cli-sync.test.ts`:
- Around line 783-876: The tests currently verify noop behavior but don't assert
the retry loop ran to its full budget; after calling previewCodexCliSync in each
of those test cases, add an assertion that the mocked nodeFs.promises.stat
(statSpy) was invoked the full retry budget for attempts against
targetStoragePath: import or reference the exported retry budget constant from
the module that implements the stat retry (e.g., writerModule.STAT_RETRY_BUDGET
or similarly named export), then assert something like
expect(statSpy.mock.calls.filter(c => c[0] ===
targetStoragePath).length).toBe(writerModule.STAT_RETRY_BUDGET) (or the
appropriate constant), and restore the spy as before; target symbols to locate
are previewCodexCliSync, the statSpy on nodeFs.promises.stat, targetStoragePath,
and the retry budget constant on writerModule.
In `@test/settings-hub-utils.test.ts`:
- Around line 498-536: Add a new vitest that mirrors the existing retry test but
targets the source-side read: spyOn the module that exports loadCodexCliState
and mock it to throw a retryable error (set error.code = "EBUSY" or simulate a
429) on the first call and return a successful state on the second call, then
call api.promptSyncCenter({}) (use the same queueSelectResults setup) and assert
loadCodexCliState was called twice and any dependent storage load spies behave
as expected; finally restore the mocks. Ensure you reference loadCodexCliState
and api.promptSyncCenter in the test so the queued-retry branch in settings-hub
(the source read paths) is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96ca7759-3546-4775-90de-b1d1d2c53e0f
📒 Files selected for processing (7)
lib/codex-cli/sync.tslib/codex-manager/settings-hub.tslib/live-account-sync.tstest/accounts-load-from-disk.test.tstest/codex-cli-sync.test.tstest/live-account-sync.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/live-account-sync.test.tstest/accounts-load-from-disk.test.tstest/codex-cli-sync.test.tstest/settings-hub-utils.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/live-account-sync.tslib/codex-cli/sync.tslib/codex-manager/settings-hub.ts
🔇 Additional comments (5)
test/accounts-load-from-disk.test.ts (1)
100-102: updatelib/accounts.ts:122to passbackupEnabledand assert it in test.
lib/accounts.ts:122callssaveAccounts(sourceOfTruthStorage)without thebackupEnabledoption, leaving backup behavior unspecified. the test attest/accounts-load-from-disk.test.ts:100asserts this single-argument call, locking in the gap instead of catching it. windows filesystem behavior during concurrent mutations depends on backup rollback, which requires verifyingbackupEnabledis explicitly controlled and passed. update the production call to includeoptions: { backupEnabled: true }(or whatever the intended behavior is), then assert both the success path at line 100 and the failure path at line 144-148 receive the correct options object.this aligns with the coding guideline requirement: "demand regression cases that reproduce... windows filesystem behavior."
test/codex-cli-sync.test.ts (4)
75-77: solid test isolation for shared sync state.resetting both mocks and
__resetLastCodexCliSyncRunForTestsin setup/teardown prevents cross-test bleed from module-level state. good guardrail for this suite. seetest/codex-cli-sync.test.ts:75-77andtest/codex-cli-sync.test.ts:81-84.Also applies to: 81-84
312-331: good regression for token-free preview payloads.the serialized preview assertions explicitly prove source tokens do not leak into the preview surface. this closes a sensitive path well. see
test/codex-cli-sync.test.ts:312-331.
961-1062: nice windows transient-ebusy regression coverage.this one correctly validates retry-then-success by asserting two target
statcalls and areadypreview outcome. seetest/codex-cli-sync.test.ts:961-1062.
1402-1461: good concurrency coverage for pending-run isolation and publish ordering.you cover concurrent apply isolation (
distinct revision), last-finisher publish semantics, and duplicate publish suppression in one flow. this is exactly the risky sync-run concurrency surface. seetest/codex-cli-sync.test.ts:1402-1461andtest/codex-cli-sync.test.ts:1514-1636.Also applies to: 1514-1636
|
@coderabbitai review Latest fixes are on |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/settings-hub-utils.test.ts (1)
538-574:⚠️ Potential issue | 🟡 Minoradd one sync-center source-read regression for http 429.
test/settings-hub-utils.test.ts:538-574validates ebusy retry, but the source-read path inapi.promptSyncCenterstill has no direct 429 regression.test/settings-hub-utils.test.ts:657-694proves generic queue behavior, not this integration path.targeted test addition
+it("retries sync-center preview loading when loadCodexCliState returns 429 once", async () => { + const api = await loadSettingsHubTestApi(); + const storageModule = await import("../lib/storage.js"); + const codexCliState = await import("../lib/codex-cli/state.js"); + vi.spyOn(storageModule, "loadAccounts").mockResolvedValue({ + version: 3, + accounts: [], + activeIndex: 0, + activeIndexByFamily: {}, + }); + let attempts = 0; + const loadStateSpy = vi + .spyOn(codexCliState, "loadCodexCliState") + .mockImplementation(async () => { + attempts += 1; + if (attempts === 1) { + const err = new Error("rate limited") as Error & { status: number; retryAfterMs: number }; + err.status = 429; + err.retryAfterMs = 1; + throw err; + } + return { path: "/tmp/source/accounts.json", accounts: [] }; + }); + queueSelectResults({ type: "back" }); + await api.promptSyncCenter({}); + expect(loadStateSpy).toHaveBeenCalledTimes(2); +});As per coding guidelines,
test/**: "tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions."Also applies to: 657-694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/settings-hub-utils.test.ts` around lines 538 - 574, The test file adds an EBUSY retry case but lacks a deterministic regression test for HTTP 429 during the sync-center "source-read" path; add a new vitest case alongside the EBUSY test that calls api.promptSyncCenter and mocks the network/source-read behavior to return a 429 on the first attempt and a successful response on retry, asserting that api.promptSyncCenter retries and that the relevant functions (mock the same symbols used: loadCodexCliState via codex-cli/state.loadCodexCliState and storage.loadAccounts) are invoked the expected number of times; ensure you restore mocks after the test so it remains deterministic.lib/live-account-sync.ts (1)
207-213:⚠️ Potential issue | 🟠 Majorsnapshot publication is still cross-instance racy when a newer watcher stops.
lib/live-account-sync.ts:207-213blocks older instances from publishing forever once a higherinstanceIdhas published. if instance-2 starts/stops while instance-1 is still running,lib/live-account-sync.ts:193publishes stopped state from instance-2 and instance-1 can no longer correct it. global status can stay wrong.proposed fix
+const liveAccountSyncSnapshots = new Map<number, LiveAccountSyncSnapshot>(); + +function refreshLastSnapshot(): void { + const running = [...liveAccountSyncSnapshots.values()].filter((s) => s.running); + const candidate = + running[running.length - 1] ?? + [...liveAccountSyncSnapshots.values()][liveAccountSyncSnapshots.size - 1] ?? + EMPTY_LIVE_ACCOUNT_SYNC_SNAPSHOT; + lastLiveAccountSyncSnapshot = { ...candidate }; +} private publishSnapshot(): void { - if (this.instanceId < lastLiveAccountSyncSnapshotInstanceId) { - return; - } - lastLiveAccountSyncSnapshotInstanceId = this.instanceId; - lastLiveAccountSyncSnapshot = this.getSnapshot(); + liveAccountSyncSnapshots.set(this.instanceId, this.getSnapshot()); + refreshLastSnapshot(); } stop(): void { this.generation += 1; this.reloadQueued = false; this.running = false; + liveAccountSyncSnapshots.delete(this.instanceId); ... this.publishSnapshot(); }please add a vitest regression that interleaves two
LiveAccountSyncinstances and assertsgetLastLiveAccountSyncSnapshot()reflects the still-running watcher after the newer watcher stops.As per coding guidelines,
lib/**: "focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails."Also applies to: 177-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/live-account-sync.ts` around lines 207 - 213, publishSnapshot currently prevents older instances from ever overwriting a snapshot once a higher lastLiveAccountSyncSnapshotInstanceId is set, causing cross-instance races where a newer watcher can publish stopped state and block an older still-running watcher from correcting it; update publishSnapshot (method publishSnapshot on LiveAccountSync) so it only defers when this.instanceId < lastLiveAccountSyncSnapshotInstanceId AND the existing global snapshot represents an active watcher (i.e., do not block if the global lastLiveAccountSyncSnapshot indicates stopped and the current this.getSnapshot() indicates running), then update lastLiveAccountSyncSnapshotInstanceId/lastLiveAccountSyncSnapshot accordingly; add a vitest regression that creates two LiveAccountSync instances, interleaves start/stop actions to simulate the race, and asserts getLastLiveAccountSyncSnapshot() ends up reflecting the still-running watcher after the newer watcher stops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-cli-sync.test.ts`:
- Around line 255-333: The test must assert that previewCodexCliSync does not
mutate the input storage object: before calling previewCodexCliSync, deep-clone
the AccountStorageV3 object referenced by current (capture activeIndex,
activeIndexByFamily and accounts including nested accessToken/refreshToken
fields), then call previewCodexCliSync, and finally assert deep equality between
the cloned snapshot and the current object (e.g., using
expect(current).toEqual(snapshot) or equivalent) to ensure no in-place mutations
occur; update the test around the previewCodexCliSync invocation to take this
snapshot and the equality assertion.
In `@test/live-account-sync.test.ts`:
- Around line 331-406: The test currently doesn't pause the second reload, so a
stale publish from the first reload can be masked; modify the test around
LiveAccountSync (the reload mock and the invoke/runReload usage) to mirror the
secondReloadFinished pattern: add a secondReloadFinished deferred, have the
reload mock await secondReloadFinished when reloadCall === 2 (so the second
reload is paused), after switching to secondStoragePath resolve
firstReloadFinished and then assert getLastLiveAccountSyncSnapshot() still shows
path: secondStoragePath (pinned) before resolving secondReloadFinished and
awaiting completion; keep references to sync, reload, runReload/invoke and the
seenPaths reloadCall logic unchanged.
---
Duplicate comments:
In `@lib/live-account-sync.ts`:
- Around line 207-213: publishSnapshot currently prevents older instances from
ever overwriting a snapshot once a higher lastLiveAccountSyncSnapshotInstanceId
is set, causing cross-instance races where a newer watcher can publish stopped
state and block an older still-running watcher from correcting it; update
publishSnapshot (method publishSnapshot on LiveAccountSync) so it only defers
when this.instanceId < lastLiveAccountSyncSnapshotInstanceId AND the existing
global snapshot represents an active watcher (i.e., do not block if the global
lastLiveAccountSyncSnapshot indicates stopped and the current this.getSnapshot()
indicates running), then update
lastLiveAccountSyncSnapshotInstanceId/lastLiveAccountSyncSnapshot accordingly;
add a vitest regression that creates two LiveAccountSync instances, interleaves
start/stop actions to simulate the race, and asserts
getLastLiveAccountSyncSnapshot() ends up reflecting the still-running watcher
after the newer watcher stops.
In `@test/settings-hub-utils.test.ts`:
- Around line 538-574: The test file adds an EBUSY retry case but lacks a
deterministic regression test for HTTP 429 during the sync-center "source-read"
path; add a new vitest case alongside the EBUSY test that calls
api.promptSyncCenter and mocks the network/source-read behavior to return a 429
on the first attempt and a successful response on retry, asserting that
api.promptSyncCenter retries and that the relevant functions (mock the same
symbols used: loadCodexCliState via codex-cli/state.loadCodexCliState and
storage.loadAccounts) are invoked the expected number of times; ensure you
restore mocks after the test so it remains deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d026c11a-d699-42c2-bde0-0c1ea2f66b9e
📒 Files selected for processing (7)
lib/codex-cli/sync.tslib/codex-manager/settings-hub.tslib/live-account-sync.tstest/accounts-load-from-disk.test.tstest/codex-cli-sync.test.tstest/live-account-sync.test.tstest/settings-hub-utils.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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
- 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/live-account-sync.test.tstest/settings-hub-utils.test.tstest/accounts-load-from-disk.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/codex-manager/settings-hub.tslib/live-account-sync.tslib/codex-cli/sync.ts
🔇 Additional comments (3)
lib/codex-cli/sync.ts (1)
214-248: pending-run commit ordering and windows retry handling look correct.
lib/codex-cli/sync.ts:214-248correctly commits/fails only once per pending revision, andlib/codex-cli/sync.ts:399-403now logs exhausted windows retry paths.lib/codex-cli/sync.ts:751-754also keeps apply refresh-safe by default.Also applies to: 377-406, 751-754
test/accounts-load-from-disk.test.ts (1)
56-151: good regression coverage for pending-run commit/failure hooks.
test/accounts-load-from-disk.test.ts:56-151now asserts both sides of the persistence boundary: success commits pending run, failure records pending-run failure. this closes the sequencing gap cleanly.lib/codex-manager/settings-hub.ts (1)
1340-1367: sync-center retry and windows-path handling are in good shape.
lib/codex-manager/settings-hub.ts:2754-2774andlib/codex-manager/settings-hub.ts:2846-2862consistently route preview/apply io through queued retry, andlib/codex-manager/settings-hub.ts:1340-1367normalizes path comparisons for windows-style inputs.Also applies to: 2754-2774, 2846-2862
|
@coderabbitai review Latest fixes are on |
@ndycode I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-cli-sync.test.ts`:
- Around line 970-1081: The test isolates a mocked logger with
vi.resetModules()/vi.doMock() but a prior test could have cached the real
../lib/logger.js; to ensure module graph is clean between tests add
vi.resetModules() to the global afterEach block (the existing afterEach handler
near the top of the test file) so that the module cache is cleared after every
test; update that afterEach to call vi.resetModules() (in addition to any
existing cleanup like vi.restoreAllMocks()) to prevent logger mock leakage
across tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 626705d7-17d8-477d-8770-ca0966517e33
📒 Files selected for processing (4)
lib/live-account-sync.tstest/codex-cli-sync.test.tstest/live-account-sync.test.tstest/settings-hub-utils.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). (3)
- GitHub Check: Greptile Review
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/settings-hub-utils.test.tstest/live-account-sync.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/live-account-sync.ts
🔇 Additional comments (21)
test/codex-cli-sync.test.ts (3)
255-335: LGTM - preview immutability now properly guarded.the snapshot comparison at
test/codex-cli-sync.test.ts:313-314andtest/codex-cli-sync.test.ts:334correctly captures and asserts thatcurrentremains unchanged afterpreviewCodexCliSync. addresses the previous review finding.
786-882: LGTM - retry budget now asserted.
test/codex-cli-sync.test.ts:833trackstargetStatCallsandtest/codex-cli-sync.test.ts:877asserts it equalsSELECTION_TIMESTAMP_READ_MAX_ATTEMPTS. this guards against regressions where the retry loop is accidentally reduced to single-attempt fallback.
1636-1703: good coverage for out-of-order completion ordering.
test/codex-cli-sync.test.ts:1636-1703verifies that whenfirstandsecondapply calls complete in reverse order, the sync run published last wins regardless of start order. this guards against the revision-based concurrency logic regressing.test/settings-hub-utils.test.ts (6)
248-307: LGTM - sync-center overview test covers preservation and rollback details.
test/settings-hub-utils.test.ts:248-307validates the ready-state overview labels including add/update/preserve counts, rollback paths, and live watcher status. addresses past review finding about missing status tests.
309-373: LGTM - parameterized tests cover error/disabled/unavailable outcomes.
test/settings-hub-utils.test.ts:309-373usesit.eachto verifyformatSyncRunOutcomebranches for all non-ready states. good regression coverage for the sync-center status surface.
375-416: good windows path normalization coverage.
test/settings-hub-utils.test.ts:375-416tests that windows-style backslash paths (e.g.,C:\\Users\\Neil\\.codex\\Accounts.json) are correctly matched against forward-slash context paths. this guards against path comparison regressions on windows.
498-536: LGTM - loadAccounts retry on EBUSY now covered.
test/settings-hub-utils.test.ts:498-536mocksloadAccountsto throw EBUSY on first call, then succeed. asserts it's called twice, proving the queued retry wrapper works for target-side reads. addresses past review finding.
538-574: LGTM - source-side loadCodexCliState retry now covered.
test/settings-hub-utils.test.ts:538-574proves the queued retry wrapper also handles EBUSY fromloadCodexCliState. addresses the past review finding about missing source-read retry coverage.
576-618: LGTM - 429 retry-after scenario covered.
test/settings-hub-utils.test.ts:576-618tests that HTTP 429 withretryAfterMsproperty triggers retry behavior. good coverage for rate-limiting scenarios in the sync-center flow.test/live-account-sync.test.ts (6)
75-79: LGTM - windows cleanup now uses retry logic.
test/live-account-sync.test.ts:78now callsremoveWithRetryinstead of rawfs.rm, preventing flaky EBUSY failures on windows. addresses past review finding.
106-153: good multi-instance snapshot publication test.
test/live-account-sync.test.ts:106-153verifies that when twoLiveAccountSyncinstances run concurrently, the newer instance's snapshot is published, and stopping the older instance doesn't regress the published snapshot. this guards against the instance-id tracking logic regressing.
155-189: good fallback-to-older-watcher test.
test/live-account-sync.test.ts:155-189confirms that when the newer watcher stops first, the published snapshot falls back to the still-running older watcher. important for correctness when watchers are created/destroyed in various orders.
297-365: LGTM - coalesced reload with follow-up now tested.
test/live-account-sync.test.ts:297-365uses deferred promises to simulate writes landing during an in-flight reload, then asserts exactly 2 reloads occur (not 3). this proves thereloadQueuedflag coalesces multiple requests into one follow-up. addresses past review finding.
367-454: LGTM - stale reload completion dropped after path switch.
test/live-account-sync.test.ts:367-454pauses both reloads with deferreds, switches path mid-flight, and asserts the first reload's completion doesn't update the snapshot with the old path.test/live-account-sync.test.ts:422-429checks snapshot state before second reload finishes, guarding against stale publishes. addresses past review finding.
456-538: good reload queuing across path switches test.
test/live-account-sync.test.ts:456-538verifies that the watcher waits for the prior path's in-flight reload to complete before starting reloads on the new path, ensuring proper sequencing.test/live-account-sync.test.ts:517-523asserts intermediate state during the handoff.lib/live-account-sync.ts (6)
30-59: LGTM - instance-aware snapshot tracking resolves multi-watcher race.
lib/live-account-sync.ts:33uses a map keyed by instance id, andlib/live-account-sync.ts:39-59picks the highest active instance id when refreshing the public snapshot. when no active watchers exist, it falls back to the last stopped snapshot. this addresses the past review finding about module-level snapshot races.
132-135: generation tracking prevents stale reload application.
lib/live-account-sync.ts:132tracks a generation counter that increments on stop.lib/live-account-sync.ts:133-135pairs the generation with each in-flight reload promise, allowing the runReload loop to detect path switches and abort stale updates.
285-299: correct handling of concurrent reloads with generation checks.
lib/live-account-sync.ts:287-290queues a follow-up if an in-flight reload exists for the same generation. if generation differs (path switched), it awaits the old reload then rechecks state atlib/live-account-sync.ts:295-298. this prevents applying stale reloads after path switches.
301-346: do-while loop with reloadQueued flag coalesces concurrent writes.
lib/live-account-sync.ts:301-346uses a do-while loop that continues whilereloadQueuedis true. multiplerunReloadcalls during an in-flight reload set the flag atlib/live-account-sync.ts:288, and the loop processes exactly one follow-up reload. generation checks atlib/live-account-sync.ts:306-308, 312-314, 320-322, 342-345prevent stale updates after each await. addresses past review finding about missing follow-up reloads.
233-250: publishSnapshot correctly updates global state based on running status.
lib/live-account-sync.ts:235-248either adds the snapshot to the active map or removes it and updates the stopped snapshot.lib/live-account-sync.ts:239-247only overwriteslastStoppedLiveAccountSyncSnapshotif this instance's id is >= the current stopped instance, preventing older stopped watchers from overwriting newer ones.
61-63: shallow copy prevents external mutation of snapshot.
lib/live-account-sync.ts:62returns{ ...lastLiveAccountSyncSnapshot }to prevent callers from mutating the internal state. note that theSyncCenterOverviewContextinlib/codex-manager/settings-hub.ts:1327-1338stores this snapshot and may access it multiple times, but since it's a copy, staleness is bounded to the context's lifetime.
| it.each(["EBUSY", "EPERM"] as const)( | ||
| "logs exhausted retries when reading the persisted target timestamp fails with %s", | ||
| async (code) => { | ||
| const debugSpy = vi.fn(); | ||
| vi.resetModules(); | ||
| vi.doMock("../lib/logger.js", async () => { | ||
| const actual = await vi.importActual<typeof import("../lib/logger.js")>( | ||
| "../lib/logger.js", | ||
| ); | ||
| return { | ||
| ...actual, | ||
| createLogger: () => ({ | ||
| debug: debugSpy, | ||
| info: vi.fn(), | ||
| warn: vi.fn(), | ||
| error: vi.fn(), | ||
| time: () => () => 0, | ||
| timeEnd: () => undefined, | ||
| }), | ||
| }; | ||
| }); | ||
|
|
||
| try { | ||
| const freshStorageModule = await import("../lib/storage.js"); | ||
| const freshStateModule = await import("../lib/codex-cli/state.js"); | ||
| const freshWriterModule = await import("../lib/codex-cli/writer.js"); | ||
| const freshSyncModule = await import("../lib/codex-cli/sync.js"); | ||
| freshStateModule.clearCodexCliStateCache(); | ||
| freshSyncModule.__resetLastCodexCliSyncRunForTests(); | ||
|
|
||
| await writeFile( | ||
| accountsPath, | ||
| JSON.stringify( | ||
| { | ||
| activeAccountId: "acc_a", | ||
| accounts: [ | ||
| { | ||
| accountId: "acc_a", | ||
| email: "a@example.com", | ||
| auth: { | ||
| tokens: { | ||
| access_token: "access-a", | ||
| refresh_token: "refresh-a", | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| "utf-8", | ||
| ); | ||
|
|
||
| vi.spyOn(freshStorageModule, "getLastAccountsSaveTimestamp").mockReturnValue(0); | ||
| vi.spyOn( | ||
| freshWriterModule, | ||
| "getLastCodexCliSelectionWriteTimestamp", | ||
| ).mockReturnValue(0); | ||
| vi.spyOn(freshStorageModule, "getStoragePath").mockReturnValue(targetStoragePath); | ||
|
|
||
| const nodeFs = await import("node:fs"); | ||
| const originalStat = nodeFs.promises.stat.bind(nodeFs.promises); | ||
| const statSpy = vi | ||
| .spyOn(nodeFs.promises, "stat") | ||
| .mockImplementation(async (...args: Parameters<typeof originalStat>) => { | ||
| if (args[0] === targetStoragePath) { | ||
| const error = new Error(`${code.toLowerCase()} target`) as NodeJS.ErrnoException; | ||
| error.code = code; | ||
| throw error; | ||
| } | ||
| return originalStat(...args); | ||
| }); | ||
|
|
||
| const current: AccountStorageV3 = { | ||
| version: 3, | ||
| accounts: [ | ||
| { | ||
| accountId: "acc_a", | ||
| accountIdSource: "token", | ||
| email: "a@example.com", | ||
| refreshToken: "refresh-a", | ||
| accessToken: "access-a", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| }; | ||
|
|
||
| try { | ||
| const preview = await freshSyncModule.previewCodexCliSync(current, { | ||
| forceRefresh: true, | ||
| }); | ||
|
|
||
| expect(preview.status).toBe("noop"); | ||
| expect(debugSpy).toHaveBeenCalledWith( | ||
| "Exhausted retries reading persisted local selection timestamp", | ||
| { | ||
| error: `${code.toLowerCase()} target`, | ||
| }, | ||
| ); | ||
| } finally { | ||
| statSpy.mockRestore(); | ||
| } | ||
| } finally { | ||
| vi.doUnmock("../lib/logger.js"); | ||
| vi.resetModules(); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
verify logger mock cleanup doesn't leak across tests.
test/codex-cli-sync.test.ts:974-990 uses vi.resetModules() and vi.doMock() to isolate the logger, with cleanup at test/codex-cli-sync.test.ts:1077-1078. the finally block calls vi.doUnmock then vi.resetModules, which is correct. however, if a test before this one cached an import from ../lib/logger.js, the module graph could be inconsistent.
the test appears correctly isolated but consider whether the global afterEach at test/codex-cli-sync.test.ts:81-103 should also call vi.resetModules() to ensure clean module state between all tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-cli-sync.test.ts` around lines 970 - 1081, The test isolates a
mocked logger with vi.resetModules()/vi.doMock() but a prior test could have
cached the real ../lib/logger.js; to ensure module graph is clean between tests
add vi.resetModules() to the global afterEach block (the existing afterEach
handler near the top of the test file) so that the module cache is cleared after
every test; update that afterEach to call vi.resetModules() (in addition to any
existing cleanup like vi.restoreAllMocks()) to prevent logger mock leakage
across tests.
1 participant
What:
Why:
Risk:
Rollback:
0d8dcfaif only the latest review-gap fixes need to be backed outValidation:
npm run buildnpm testnpm test -- test/codex-cli-sync.test.ts test/live-account-sync.test.ts test/settings-hub-utils.test.tsDocs/Governance:
docs/reference/settings.mddocuments the Sync Center flow and validation commandsnote: 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 ships the sync center UI surface and hardens the sync reconciliation lifecycle across three axes: pending-run commits are now gated on persistence success,
saveAccountsaccepts a per-callbackupEnabledoption eliminating the global-flag mutation window, and the live watcher snapshot tracking separates running and stopped state to prevent a newer-stopped watcher from masking an older running one.the three critical issues called out in previous thread comments — double
loadCodexCliStateTOCTOU inbuildPreview,setStorageBackupEnabledmutation race across async retries, and stale cached state inapplyCodexCliSyncToStorage— are all addressed:buildPreviewpasses the already-loadedsourceStateintopreviewCodexCliSyncso only one disk read occurs;saveAccountstakes{ backupEnabled }directly; andpromptSyncCenterpassessourceStatestraight through to apply.key concerns:
shouldApplyCodexCliSelectionuses a 1000ms tolerance whensyncVersionis absent (falling back tosourceUpdatedAtMs), meaning codex selection can overwrite a local selection written up to 1 second before the codex file mtime. on windows with antivirus write-lock delays, this window is large enough to cause real misfires. the existing tests use a 5-second delta; no test covers the boundary at ±1000ms.in
syncToPath,this.currentPath = pathis set beforeawait readMtimeMs(path). a concurrentstop()during the await publishes a snapshot that reflects the intermediate new path even though the watcher for that path was never successfully started. no dedicated test covers this window.pendingRun.revisionis used only as an idempotency key to prevent double-commit; the actual publish ordering is determined by a freshallocateCodexCliSyncRunRevision()call at commit time (last-commit-wins). this is correct and tested, but the field namerevisionimplies ordering, which could confuse future maintainers.test
"retries transient sync-center apply-time reconcile failures"mocksapplyCodexCliSyncToStorageas throwing, but the real implementation wraps all errors in a catch and never propagates — the test is exercising thewithQueuedRetryretry infrastructure for a scenario that cannot occur with the live code.Confidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant SyncCenter as promptSyncCenter participant Preview as previewCodexCliSync participant Apply as applyCodexCliSyncToStorage participant Storage as saveAccounts participant Commit as commitPending/Failure User->>SyncCenter: open sync center SyncCenter->>Preview: buildPreviewSafely(forceRefresh=true) Preview->>Preview: loadCodexCliState (source) Preview->>Preview: loadAccounts (target) Preview->>Preview: getPersistedLocalSelectionTimestamp (mtime, up to 4 EBUSY retries) Preview->>Preview: reconcileCodexCliState Preview-->>SyncCenter: CodexCliSyncPreview + sourceState User->>SyncCenter: click Apply SyncCenter->>Apply: withQueuedRetry → loadAccounts + applyCodexCliSyncToStorage(sourceState) Apply->>Apply: getPersistedLocalSelectionTimestamp Apply->>Apply: reconcileCodexCliState Apply-->>SyncCenter: {storage, changed, pendingRun} SyncCenter->>Storage: withQueuedRetry → saveAccounts(storage, {backupEnabled}) alt save succeeds Storage-->>SyncCenter: ok SyncCenter->>Commit: commitPendingCodexCliSyncRun(pendingRun) Note over Commit: allocates new revision<br/>→ publishCodexCliSyncRun (last-commit-wins) SyncCenter->>Preview: buildPreviewSafely(forceRefresh=true) Preview-->>SyncCenter: refreshed CodexCliSyncPreview else save fails (EBUSY/EPERM exhausted) Storage-->>SyncCenter: throw SyncCenter->>Commit: commitCodexCliSyncRunFailure(pendingRun, error) Note over SyncCenter: preview.status = error<br/>apply button disabled<br/>user must Refresh endComments Outside Diff (1)
lib/codex-cli/sync.ts, line 549-562 (link)1000ms tolerance can silently overwrite a recent local selection
shouldApplyCodexCliSelectionapplies a 1000ms tolerance whensyncVersionis absent, meaning codex wins whenevercodexVersion >= localVersion - 1000. concretely: if a user saved their local selection 800ms after the codex source was last written, codex selection still overwrites it.on windows, where antivirus write-lock retries can add hundreds of milliseconds of latency before the accounts file mtime is updated, this window is large enough to cause real misfires — a save that completed "after" the codex update (as measured by wall clock) could still be within the 1-second tolerance, silently losing the user's selection.
there's also no test that exercises the exact boundary (e.g.
codexVersion = localVersion - 999ms→ apply,codexVersion = localVersion - 1001ms→ preserve), which means the tolerance semantics are undocumented and a future refactor could break the boundary without a failing test.consider either tightening the tolerance (
< 500mson NTFS where granularity is 100ns), removing it forsourceUpdatedAtMspaths entirely and treating equal-or-later as "local wins", or adding explicit boundary tests that pin the behavior.Prompt To Fix All With AI
Last reviewed commit: 0d8dcfa
Context used: