feat(sync): add sync center and status surface#78
feat(sync): add sync center and status surface#78ndycode wants to merge 98 commits intogit-plan/01-reset-safetyfrom
Conversation
…d-primitives # Conflicts: # test/dashboard-settings.test.ts # test/unified-settings.test.ts
# Conflicts: # test/helpers/remove-with-retry.ts
… into release/all-open-v0.1.8
…nto release/all-open-v0.1.8
…' into release/all-open-v0.1.8 # Conflicts: # README.md # lib/named-backup-export.ts # lib/oc-chatgpt-import-adapter.ts # lib/oc-chatgpt-target-detection.ts # lib/storage.ts # test/codex-manager-cli.test.ts # test/named-backup-export.test.ts # test/oc-chatgpt-import-adapter.test.ts # test/oc-chatgpt-target-detection.test.ts
…lease/all-open-v0.1.8 # Conflicts: # test/documentation.test.ts
# Conflicts: # lib/storage.ts # test/documentation.test.ts # test/storage.test.ts
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (42)
✨ 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}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| }); | ||
|
|
||
| const items: MenuItem<BackendSettingsHubAction>[] = [ | ||
| { label: UI_COPY.settings.previewHeading, value: { type: "cancel" }, kind: "heading" }, | ||
| { | ||
| label: UI_COPY.settings.previewHeading, | ||
| value: { type: "cancel" }, | ||
| kind: "heading", | ||
| }, | ||
| { | ||
| label: preview.label, |
There was a problem hiding this comment.
double loadCodexCliState in buildPreview — TOCTOU on Windows
buildPreview calls loadCodexCliState({ forceRefresh }) on its own and then calls previewCodexCliSync, which internally calls loadCodexCliState({ forceRefresh: options.forceRefresh }) again. both pass forceRefresh: true, so the cache is bypassed on both calls and two separate disk reads occur.
on Windows, antivirus holds (EBUSY) or a concurrent Codex CLI write between those two reads can produce different on-disk state for each call. the result: context.state (from read 1) reflects a different snapshot than preview.sourcePath / preview.status / preview.summary (from read 2 inside previewCodexCliSync). the sync-center overview row "Codex CLI source visibility" and the status / source-path rows would then be internally inconsistent — one reflecting the pre-write state and the other the post-write state.
fix: expose state as a return value from previewCodexCliSync or thread the already-loaded state into it so a single read covers both the preview computation and the context resolution.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager/settings-hub.ts
Line: 2509-2518
Comment:
**double `loadCodexCliState` in `buildPreview` — TOCTOU on Windows**
`buildPreview` calls `loadCodexCliState({ forceRefresh })` on its own and then calls `previewCodexCliSync`, which internally calls `loadCodexCliState({ forceRefresh: options.forceRefresh })` again. both pass `forceRefresh: true`, so the cache is bypassed on both calls and two separate disk reads occur.
on Windows, antivirus holds (EBUSY) or a concurrent Codex CLI write between those two reads can produce different on-disk state for each call. the result: `context.state` (from read 1) reflects a different snapshot than `preview.sourcePath` / `preview.status` / `preview.summary` (from read 2 inside `previewCodexCliSync`). the sync-center overview row "Codex CLI source visibility" and the status / source-path rows would then be internally inconsistent — one reflecting the pre-write state and the other the post-write state.
fix: expose state as a return value from `previewCodexCliSync` or thread the already-loaded state into it so a single read covers both the preview computation and the context resolution.
How can I resolve this? If you propose a fix, please make it concise.
| @@ -2040,6 +2612,139 @@ async function promptBackendSettings( | |||
| } | |||
There was a problem hiding this comment.
setStorageBackupEnabled global mutation unguarded across async retries
the apply flow saves, mutates, and restores the module-level storageBackupEnabled flag around saveAccounts:
const previousStorageBackupEnabled = isStorageBackupEnabled();
setStorageBackupEnabled(storageBackupEnabled);
try {
await saveAccounts(synced.storage);
} finally {
setStorageBackupEnabled(previousStorageBackupEnabled);
}saveAccounts is async and, on Windows, can retry several times on EBUSY / EPERM via withQueuedRetry. during any of those retry delays the module-level flag is in the mutated state. any concurrent operation that also reads isStorageBackupEnabled() — for example a live-sync reload or a background storage write — will observe the wrong backup flag for the duration of the entire retry window.
the mutation should be scoped to the specific call rather than mutating shared module state. pass storageBackupEnabled as a parameter to saveAccounts directly, or guard the window with the existing write queue.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager/settings-hub.ts
Line: 2593-2612
Comment:
**`setStorageBackupEnabled` global mutation unguarded across async retries**
the apply flow saves, mutates, and restores the module-level `storageBackupEnabled` flag around `saveAccounts`:
```ts
const previousStorageBackupEnabled = isStorageBackupEnabled();
setStorageBackupEnabled(storageBackupEnabled);
try {
await saveAccounts(synced.storage);
} finally {
setStorageBackupEnabled(previousStorageBackupEnabled);
}
```
`saveAccounts` is async and, on Windows, can retry several times on EBUSY / EPERM via `withQueuedRetry`. during any of those retry delays the module-level flag is in the mutated state. any concurrent operation that also reads `isStorageBackupEnabled()` — for example a live-sync reload or a background storage write — will observe the wrong backup flag for the duration of the entire retry window.
the mutation should be scoped to the specific call rather than mutating shared module state. pass `storageBackupEnabled` as a parameter to `saveAccounts` directly, or guard the window with the existing write queue.
How can I resolve this? If you propose a fix, please make it concise.# 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
| const current = await loadAccounts(); | ||
| const synced = await applyCodexCliSyncToStorage(current); |
There was a problem hiding this comment.
apply uses stale cached state — result diverges from the shown preview
applyCodexCliSyncToStorage calls loadCodexCliState() with no forceRefresh, so it reads from whatever is in the in-process cache. the preview was built with forceRefresh: true (both in the initial buildPreview(true) call and via previewCodexCliSync). on windows, if codex cli writes a new version of its accounts file between when the user sees the preview and when they click apply, the cached state is stale and the applied result can silently differ from what the user approved.
concrete windows risk: antivirus-triggered deferred writes can hold the codex cli accounts file open and force a retry after preview was displayed. the apply then uses the pre-retry cached snapshot.
fix: either add a forceRefresh option to applyCodexCliSyncToStorage and pass it from the apply path, or re-run previewCodexCliSync with forceRefresh: true immediately before the apply and abort/re-prompt the user if the new preview diverges.
// option A: pass forceRefresh into the apply
const synced = await applyCodexCliSyncToStorage(current, { forceRefresh: true });no vitest regression exists for the stale-cache divergence scenario.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager/settings-hub.ts
Line: 2750-2751
Comment:
**apply uses stale cached state — result diverges from the shown preview**
`applyCodexCliSyncToStorage` calls `loadCodexCliState()` with no `forceRefresh`, so it reads from whatever is in the in-process cache. the preview was built with `forceRefresh: true` (both in the initial `buildPreview(true)` call and via `previewCodexCliSync`). on windows, if codex cli writes a new version of its accounts file between when the user sees the preview and when they click apply, the cached state is stale and the applied result can silently differ from what the user approved.
concrete windows risk: antivirus-triggered deferred writes can hold the codex cli accounts file open and force a retry after preview was displayed. the apply then uses the pre-retry cached snapshot.
fix: either add a `forceRefresh` option to `applyCodexCliSyncToStorage` and pass it from the apply path, or re-run `previewCodexCliSync` with `forceRefresh: true` immediately before the apply and abort/re-prompt the user if the new preview diverges.
```ts
// option A: pass forceRefresh into the apply
const synced = await applyCodexCliSyncToStorage(current, { forceRefresh: true });
```
no vitest regression exists for the stale-cache divergence scenario.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Summary by cubic
Adds a preview‑first Sync Center in Settings to inspect and apply one‑way (mirror‑only) Codex CLI account sync with a clear status surface and last‑run history. Also adds experimental oc‑chatgpt sync and named backup export options behind Settings.
New Features
getLastLiveAccountSyncSnapshot.Bug Fixes
saveAccountssucceeds; on failure, records a save‑specific error and preserves state.Written for commit e12966a. Summary will update on new commits.
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
adds a preview-first sync center panel to settings, surfaces codex cli source visibility, live watcher status, destination-only preservation counts, and backup/rollback context. also hardens the apply flow to only commit "last run" after a successful
saveAccounts, and correctly hoists theisCodexCliSyncEnabled()guard before any disk i/o in bothpreviewCodexCliSyncandapplyCodexCliSyncToStorage.key changes:
lib/codex-cli/sync.ts: oldsyncAccountStorageFromCodexClirenamed toapplyCodexCliSyncToStorage; a new mirror-only stub replaces the old name and only normalizes local indexes — no codex cli state import at startup anymore;previewCodexCliSyncadded for the read-only preview step; revision-guardedlastCodexCliSyncRunmodule state added withcommitPendingCodexCliSyncRun/commitCodexCliSyncRunFailurelib/codex-manager/settings-hub.ts:promptSyncCenterwires the preview/apply ui loop;buildSyncCenterOverviewrenders the 8-row status panel;loadExperimentalSyncTargetadds the oc-chatgpt target detection path; two pre-existing unfixed issues remain: (a) doubleloadCodexCliStateTOCTOU insidebuildPreview, and (b)setStorageBackupEnabledglobal mutation window during asyncsaveAccountsretrieslib/live-account-sync.ts: module-levellastLiveAccountSyncSnapshotpublished on every state transition so the sync center can surface watcher status without holding a direct reference to the instancelib/accounts.ts: callers of the now-stubsyncAccountStorageFromCodexClino longer import codex cli state on startup; intentional under the mirror-only design butpendingRunis alwaysnullnow so startup index normalizations are not recorded in sync run historyConfidence Score: 2/5
applyCodexCliSyncToStoragehas noforceRefreshpath and the apply inpromptSyncCenterdoes not force a cache refresh before writing, creating a TOCTOU between what was previewed and what gets persisted; two previously flagged issues (doubleloadCodexCliStateinbuildPreview,setStorageBackupEnabledglobal mutation during async retries) remain unaddressed; no vitest regression for the stale-cache apply divergence scenario; concurrency reasoning for windows filesystem interactions is partially present but incomplete for the apply pathlib/codex-manager/settings-hub.ts(apply-path stale cache, global mutation);lib/codex-cli/sync.ts(no forceRefresh option onapplyCodexCliSyncToStorage)Important Files Changed
applyCodexCliSyncToStorage, addspreviewCodexCliSync, revision-guardedlastCodexCliSyncRunmodule state, and a new mirror-onlysyncAccountStorageFromCodexClistub; disabled-env guard correctly hoisted before disk I/O in both apply and preview pathspromptSyncCenter,buildSyncCenterOverview, andloadExperimentalSyncTarget; the apply flow callsapplyCodexCliSyncToStoragewithoutforceRefresh, so the applied snapshot may diverge from the previewed state on windows;setStorageBackupEnabledglobal mutation during async retries (pre-existing flagged issue) remains; doubleloadCodexCliStateTOCTOU inbuildPreview(pre-existing flagged issue) remainslastLiveAccountSyncSnapshotupdated viapublishSnapshot()in constructor,syncToPath,stop, error, and reload callbacks; exposed viagetLastLiveAccountSyncSnapshot()for the sync center overview; no concurrency issues in the single-threaded js modelsyncAccountStorageFromCodexClinow get the mirror-only stub (index normalization only, no Codex CLI state import);pendingRunis alwaysnullfrom the stub socommitPendingCodexCliSyncRunis always a no-op at startup — intentional under the new mirror-only designSequence Diagram
sequenceDiagram participant U as User participant SC as promptSyncCenter participant PCS as previewCodexCliSync participant ALCS as applyCodexCliSyncToStorage participant LCCS as loadCodexCliState(cache) participant LA as loadAccounts participant SA as saveAccounts U->>SC: open sync center SC->>LA: loadAccounts() SC->>LCCS: loadCodexCliState(forceRefresh=true) [READ 1] SC->>PCS: previewCodexCliSync(current, {forceRefresh:true}) PCS->>LCCS: loadCodexCliState(forceRefresh=true) [READ 2 — TOCTOU] LCCS-->>PCS: state snapshot B PCS-->>SC: preview (built from snapshot B) SC-->>U: show preview (based on B) Note over U,LCCS: Codex CLI may write new state here on Windows U->>SC: click Apply SC->>LA: loadAccounts() SC->>ALCS: applyCodexCliSyncToStorage(current) ALCS->>LCCS: loadCodexCliState() [NO forceRefresh — may return stale B or cached C] LCCS-->>ALCS: possibly stale snapshot ALCS-->>SC: synced (may differ from previewed result) SC->>SA: saveAccounts(synced.storage) Note over SC,SA: setStorageBackupEnabled mutates global flag during async save SA-->>SC: ok SC->>LCCS: loadCodexCliState(forceRefresh=true) [READ 3] SC->>PCS: previewCodexCliSync(nextStorage, {forceRefresh:true}) PCS->>LCCS: loadCodexCliState(forceRefresh=true) [READ 4]Prompt To Fix All With AI
Last reviewed commit: e12966a
Context used: