feat(sync): add result history and last-result log#110
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughintroduces a new ndjson-backed sync history subsystem and integrates it with codex-cli sync and live account sync. codex-cli sync gains a history-backed async run lifecycle, public run APIs and storage reconciliation. tests expanded to cover preview/apply flows, history hydration, and history module behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Codex CLI Sync
participant Live as Live Account Sync
participant History as Sync History
participant FS as Filesystem
CLI->>History: publishCodexCliSyncRun(entry)
History->>History: withHistoryLock()
History->>FS: append ndjson line to history file (append)
History->>FS: write latest snapshot file
History->>FS: trim history file if > MAX
FS-->>History: write complete
History-->>CLI: ack
Live->>History: recordHistory(reason, outcome, snapshot)
History->>History: toHistorySnapshot()
History->>History: withHistoryLock()
History->>FS: append ndjson line and update latest
FS-->>History: done
History-->>Live: ack
CLI->>History: loadLastCodexCliSyncRun()
History->>FS: read latest snapshot or tail history
FS-->>History: latest entry or tail
History->>History: hydrate last run state
History-->>CLI: CodexCliSyncRun | null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes notes on review
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds persisted sync result history (NDJSON + latest snapshot) and integrates history logging into Codex CLI sync and live account sync, with tests validating bounded tail reads and recovery behavior.
Changes:
- Introduces
lib/sync-history.tsfor appending, trimming, and reading sync history (including a tail-reader for bounded reads). - Logs Codex CLI sync runs and live-account sync outcomes to history and hydrates “last run” from history when missing.
- Extends existing Vitest suites and adds a new sync-history-focused test file.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/sync-history.test.ts | New tests validating bounded tail reads and recovery of last Codex CLI sync run from history. |
| test/live-account-sync.test.ts | Updates tests to configure/reset history and assert live sync history entries are recorded. |
| test/codex-cli-sync.test.ts | Updates tests to configure/reset history and assert Codex CLI sync history persistence/hydration. |
| lib/sync-history.ts | New sync history module (append/read/latest snapshot, tail read, trimming, test hooks). |
| lib/live-account-sync.ts | Records live sync successes/errors into sync history with a snapshot tailored for history. |
| lib/codex-cli/sync.ts | Appends Codex CLI sync runs to history and hydrates last run from latest/tail history. |
| lib/index.ts | Re-exports sync-history from the library entrypoint. |
Comments suppressed due to low confidence (1)
lib/live-account-sync.ts:1
basenamedoes not acceptnull; ifsummarizeWatchPathis ever called withnullthis will throw at runtime (and should also fail typecheck unless the call sites guarantee non-null). Add a null guard (returning a placeholder like"<unknown>") or change the function signature topath: stringand ensure callers never pass null.
import { type FSWatcher, promises as fs, watch as fsWatch } from "node:fs";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| export function configureSyncHistoryForTests(directory: string | null): void { | ||
| historyDirOverride = directory ? directory.trim() : null; |
| export * from "./parallel-probe.js"; | ||
| export * from "./session-affinity.js"; | ||
| export * from "./live-account-sync.js"; | ||
| export * from "./sync-history.js"; |
| const reload = vi.fn(async () => { | ||
| throw new Error("reload failed"); | ||
| }); | ||
| const appendSpy = vi.spyOn(syncHistory, "appendSyncHistoryEntry"); |
| sync.stop(); | ||
| appendSpy.mockRestore(); |
| async function trimHistoryFileIfNeeded(paths: SyncHistoryPaths): Promise<void> { | ||
| const content = await fs.readFile(paths.historyPath, "utf8").catch((error) => { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code === "ENOENT") { | ||
| return ""; | ||
| } | ||
| throw error; | ||
| }); | ||
| if (!content) { | ||
| return; | ||
| } | ||
| const lines = content.split(/\r?\n/).filter(Boolean); | ||
| if (lines.length <= MAX_HISTORY_ENTRIES) { | ||
| return; | ||
| } | ||
| const trimmedContent = `${lines.slice(-MAX_HISTORY_ENTRIES).join("\n")}\n`; | ||
| await fs.writeFile(paths.historyPath, trimmedContent, { | ||
| encoding: "utf8", | ||
| mode: 0o600, | ||
| }); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 242-272: The race comes from checking
lastCodexCliSyncHistoryLoadAttempted then setting it after a small window; to
fix, make the check-and-set atomic inside loadLastCodexCliSyncRun by assigning
lastCodexCliSyncHistoryLoadAttempted = true immediately before (or as part of)
creating lastCodexCliSyncRunLoadPromise so no two callers can both pass the
check; update the function so the guard is: if
(!lastCodexCliSyncHistoryLoadAttempted) { lastCodexCliSyncHistoryLoadAttempted =
true; lastCodexCliSyncRunLoadPromise = (async () => { ... })().catch(() =>
null).finally(() => { lastCodexCliSyncRunLoadPromise = null; }); } and leave the
rest of the logic (awaiting lastCodexCliSyncRunLoadPromise and cloning via
cloneCodexCliSyncRun/hydrateLastCodexCliSyncRunFromHistory) unchanged.
In `@lib/sync-history.ts`:
- Around line 300-314: Remove the dead, blocking function
readLatestSyncHistorySync from lib/sync-history.ts: delete the entire function
definition and any export referencing readLatestSyncHistorySync, confirm there
are zero usages (search for readLatestSyncHistorySync) and run typechecks/tests
to ensure nothing else imports it; since it performs synchronous fs IO, do not
replace it—simply remove the symbol and related references to avoid blocking the
event loop.
In `@test/codex-cli-sync.test.ts`:
- Around line 1332-1342: Add a regression test that simulates transient
filesystem contention by mocking the appendFile call used by
appendSyncHistoryEntry (lib/sync-history.ts) to throw an EBUSY/EPERM/EAGAIN
error for the first N attempts and succeed thereafter, then invoke
commitPendingCodexCliSyncRun (lib/codex-cli/sync.ts) so the call path goes
through retryHistoryWrite (lib/sync-history.ts) and finally assert the history
was persisted/recoverable (e.g., use readSyncHistory/loadLastCodexCliSyncRun as
in the existing test); specifically, spy/mock fs.promises.appendFile (or the
wrapper used by appendSyncHistoryEntry) to fail twice with {code: 'EBUSY'} then
resolve, run the code that calls appendSyncHistoryEntry, and assert the retry
logic retried and the final persisted run outcome/summary matches expectations.
In `@test/sync-history.test.ts`:
- Around line 174-211: Add three regression tests: (1) for retry behavior, mock
node fs operations used by retryHistoryWrite to throw transient errors
EBUSY/EAGAIN/EPERM on the first N attempts and succeed thereafter, assert that
retryHistoryWrite was retried the expected number of times and that exponential
backoff delays (or at least increasing retry intervals) occurred; (2) for
concurrency serialization, fire multiple concurrent appendSyncHistoryEntry calls
and assert via withHistoryLock or by checking persisted history order/length
that appends were serialized (no lost/raced entries) and that withHistoryLock
was used to serialize; (3) for Windows filesystem edge cases, simulate
Windows-specific EBUSY/EAGAIN behavior during file removal in
__resetSyncHistoryForTests and during appends (mocking fs.unlink/fs.writeFile to
throw platform-specific errors) and assert cleanup and append logic handle
retries and do not leave corrupt state. Use spies/mocks for the specific
functions (retryHistoryWrite, withHistoryLock, appendSyncHistoryEntry,
__resetSyncHistoryForTests) to inject errors and verify expected
retries/serialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44978562-2ee0-49c2-a1bd-33d7a688275e
📒 Files selected for processing (7)
lib/codex-cli/sync.tslib/index.tslib/live-account-sync.tslib/sync-history.tstest/codex-cli-sync.test.tstest/live-account-sync.test.tstest/sync-history.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/index.tslib/live-account-sync.tslib/sync-history.tslib/codex-cli/sync.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-cli-sync.test.tstest/sync-history.test.tstest/live-account-sync.test.ts
🔇 Additional comments (23)
lib/index.ts (1)
20-20: lgtm, but note that test-only helpers are now public api.the wildcard re-export exposes
configureSyncHistoryForTests,__resetSyncHistoryForTests,__getLastSyncHistoryErrorForTests, and__getLastSyncHistoryPathsForTestsfromlib/sync-history.ts:322-361. the__prefix convention signals internal/test use, so this is acceptable. tests intest/codex-cli-sync.test.ts,test/live-account-sync.test.ts, andtest/sync-history.test.tsdepend on these exports.test/codex-cli-sync.test.ts (2)
77-78: proper test isolation for sync history.configuring and resetting sync history in
beforeEachensures each test starts with a clean slate. good practice.
111-112: cleanup correctly resets sync history state.teardown properly resets history and clears the directory override. this prevents test pollution.
lib/live-account-sync.ts (4)
115-122: good path redaction before persisting history.
toHistorySnapshotreduces the full path tobasename, preventing leakage of sensitive directory structures in history entries. this aligns with the coding guidelines about not logging tokens or emails.
243-264: fire-and-forget history recording is intentional but worth noting.
recordHistorycatches errors and logs at debug level, which is appropriate for non-critical telemetry. the fire-and-forget pattern ensures history failures don't break the main reload flow. concurrency is safe becauseappendSyncHistoryEntryuseswithHistoryLockinternally (lib/sync-history.ts:72-79).test coverage exists at
test/live-account-sync.test.ts:229-271.
348-348: history recorded on successful reload.the
voidprefix makes this fire-and-forget, which is correct since history recording shouldn't block the reload flow.
358-362: history recorded on reload failure with error message.captures the error message in the history entry for debugging purposes. the error message comes from the caught error, not from sensitive fields.
test/live-account-sync.test.ts (4)
67-68: proper test isolation for sync history in live-account-sync tests.configuring and resetting sync history ensures each test starts clean.
81-82: cleanup correctly resets sync history state.proper teardown prevents test pollution.
229-271: good coverage for history recording on reload failure.the test spies on
appendSyncHistoryEntry, triggers a reload failure, and verifies the history entry has the correctkind,outcome,reason,path, andsnapshot.pathfields. spy is properly restored at line 271.
388-395: verifies success outcome in history after multiple reloads.checks that the latest history entry has
outcome: "success"after the follow-up reload completes.lib/codex-cli/sync.ts (3)
178-186: fire-and-forget history append with proper error handling.the
.catch()logs errors at debug level without breaking the main flow. no sensitive fields are logged. concurrency is safe becauseappendSyncHistoryEntryuseswithHistoryLockinternally.
315-316: test reset properly clears new state variables.resetting
lastCodexCliSyncHistoryLoadAttemptedandlastCodexCliSyncRunLoadPromiseensures test isolation for the new loading logic.
691-691: preview now awaits async history load for cold-start recovery.this adds I/O to the preview path but is necessary to populate
lastSyncfrom persisted history when the in-memory state is empty. acceptable tradeoff for the recovery feature.test/sync-history.test.ts (3)
47-63: proper test setup and teardown with temp directory isolation.creates a temp directory, configures sync history to use it, and cleans up after each test. good practice for deterministic tests.
65-136: good test for bounded tail-reading optimization.verifies that
readSyncHistorywithlimit: 1usesreadHistoryTailinstead of loading the entire file viareadFile. the spy assertion at line 135 confirms the optimization works.
138-172: good coverage for cold-start recovery path.tests that
loadLastCodexCliSyncRuncan recover the last run from the ndjson history file when the latest snapshot file is deleted. this covers the fallback path inlib/codex-cli/sync.ts:259-263.lib/sync-history.ts (6)
72-79: correct single-process mutex implementation.
withHistoryLockchains promises to serialize operations within a single node process. this doesn't protect against multi-process races, but that's acceptable for history logs where occasional duplicates are tolerable.
90-105: good retry logic for windows transient locks.
retryHistoryWritehandles EBUSY/EPERM/EAGAIN with exponential backoff (25ms → 50ms → 100ms → 200ms, 5 attempts). this aligns with the coding guidelines for windows filesystem IO.per pr known issues, this path lacks explicit vitest coverage. see
test/sync-history.test.tsreview comment.
133-186: efficient bounded tail-reading for history.
readHistoryTailreads 8KB chunks from the end of the file, avoiding full-file reads for large histories. the implementation correctly handles:
- empty files (line 141-143)
- partial lines spanning chunks (lines 155-157)
- kind filtering (line 164)
- early exit when limit is reached (lines 166-168)
188-211: history trimming to bound file size.
trimHistoryFileIfNeededrewrites the history file to the last 200 entries when it exceeds the cap. the trim logic reads 201 entries to detect overflow and only rewrites if necessary.one note: if
retryHistoryWritefails on the rewrite (line 205-210), the history file could be left in an inconsistent state. consider catching and logging the error without throwing, since trim is best-effort.
213-247: correct append flow with lock, retry, trim, and latest update.the sequence is:
- acquire lock
- ensure directory exists
- append entry with retry
- trim if oversized
- write latest snapshot with retry
pendingHistoryWritestracking enableswaitForPendingHistoryWritesto wait for in-flight writes before reads.
86-88: file permissions note for cross-platform.
mode: 0o700for directories andmode: 0o600for files are good security defaults on unix. on windows, these modes are largely ignored, but node doesn't throw. no issue here, just noting the cross-platform behavior.
| export function readLatestSyncHistorySync(): SyncHistoryEntry | null { | ||
| try { | ||
| const content = readFileSync(getSyncHistoryPaths().latestPath, "utf8"); | ||
| const parsed = parseEntry(content); | ||
| return parsed ? cloneEntry(parsed) : null; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.debug("Failed to read latest sync history", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check if readLatestSyncHistorySync is used anywhere
rg -n "readLatestSyncHistorySync" --type=ts lib/ test/Repository: ndycode/codex-multi-auth
Length of output: 162
🏁 Script executed:
# Additional check: verify no indirect calls via string lookups
rg -n "\"readLatestSyncHistorySync\"|'readLatestSyncHistorySync'" --type=ts lib/ test/Repository: ndycode/codex-multi-auth
Length of output: 50
remove readLatestSyncHistorySync from lib/sync-history.ts:300-314—it's dead code with zero usages.
verified this function isn't called anywhere in lib/ or test/. it also blocks the event loop via readFileSync, which violates guidelines for lib/** around filesystem io patterns. no initialization logic depends on it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sync-history.ts` around lines 300 - 314, Remove the dead, blocking
function readLatestSyncHistorySync from lib/sync-history.ts: delete the entire
function definition and any export referencing readLatestSyncHistorySync,
confirm there are zero usages (search for readLatestSyncHistorySync) and run
typechecks/tests to ensure nothing else imports it; since it performs
synchronous fs IO, do not replace it—simply remove the symbol and related
references to avoid blocking the event loop.
| const history = await readSyncHistory({ kind: "codex-cli-sync" }); | ||
| const lastHistory = history.at(-1); | ||
| expect(lastHistory?.kind).toBe("codex-cli-sync"); | ||
| if (lastHistory?.kind === "codex-cli-sync") { | ||
| expect(lastHistory.run.outcome).toBe("changed"); | ||
| expect(lastHistory.run.summary.addedAccountCount).toBe(1); | ||
| } | ||
| __resetLastCodexCliSyncRunForTests(); | ||
| const persisted = await loadLastCodexCliSyncRun(); | ||
| expect(persisted?.outcome).toBe("changed"); | ||
| expect(persisted?.summary.addedAccountCount).toBe(1); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check if any test explicitly covers retryHistoryWrite with EBUSY/EPERM injection
rg -n "retryHistoryWrite|EBUSY.*append|EPERM.*append" --type=ts test/Repository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Find where retryHistoryWrite is defined
rg -n "retryHistoryWrite" --type=ts lib/Repository: ndycode/codex-multi-auth
Length of output: 339
🏁 Script executed:
# Find appendSyncHistoryEntry and commitPendingCodexCliSyncRun functions
rg -n "appendSyncHistoryEntry|commitPendingCodexCliSyncRun" --type=ts lib/Repository: ndycode/codex-multi-auth
Length of output: 750
🏁 Script executed:
# Check test file context around lines 1332-1342
sed -n '1320,1350p' test/codex-cli-sync.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1262
🏁 Script executed:
# Search for any EBUSY or EPERM error handling in lib
rg -n "EBUSY|EPERM" --type=ts lib/Repository: ndycode/codex-multi-auth
Length of output: 1946
🏁 Script executed:
# Get the retryHistoryWrite function implementation
sed -n '90,120p' lib/sync-history.tsRepository: ndycode/codex-multi-auth
Length of output: 858
🏁 Script executed:
# Search for any test error injection helpers or mocks for appendFile
rg -n "appendFile.*mock|mock.*appendFile|vi\.mock|afterEach.*jest" --type=ts test/ | head -20Repository: ndycode/codex-multi-auth
Length of output: 1627
🏁 Script executed:
# Check if there are any tests that inject filesystem errors
rg -n "reject|throws|EBUSY|EPERM" --type=ts test/codex-cli-sync.test.ts | head -20Repository: ndycode/codex-multi-auth
Length of output: 434
solid integration test for history persistence and recovery, but add regression test for retry logic.
verifies that appendSyncHistoryEntry (lib/sync-history.ts:213) is called during commitPendingCodexCliSyncRun (lib/codex-cli/sync.ts:274) and that loadLastCodexCliSyncRun can recover the run from persisted history after resetting in-memory state. good coverage for the cold-start recovery path.
however, retryHistoryWrite (lib/sync-history.ts:90) wraps all history writes with exponential backoff retry logic for EBUSY/EPERM/EAGAIN, but this test never exercises that retry path. add a regression test that injects transient EBUSY during the appendFile call inside appendSyncHistoryEntry to confirm the retry logic handles transient fs contention, especially on windows where EPERM can occur during concurrent file access.
🤖 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 1332 - 1342, Add a regression test
that simulates transient filesystem contention by mocking the appendFile call
used by appendSyncHistoryEntry (lib/sync-history.ts) to throw an
EBUSY/EPERM/EAGAIN error for the first N attempts and succeed thereafter, then
invoke commitPendingCodexCliSyncRun (lib/codex-cli/sync.ts) so the call path
goes through retryHistoryWrite (lib/sync-history.ts) and finally assert the
history was persisted/recoverable (e.g., use
readSyncHistory/loadLastCodexCliSyncRun as in the existing test); specifically,
spy/mock fs.promises.appendFile (or the wrapper used by appendSyncHistoryEntry)
to fail twice with {code: 'EBUSY'} then resolve, run the code that calls
appendSyncHistoryEntry, and assert the retry logic retried and the final
persisted run outcome/summary matches expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/codex-cli-sync.test.ts (1)
1335-1346: 🧹 Nitpick | 🔵 Trivialmissing regression test for appendFile EBUSY retry in history writes.
past review at test/codex-cli-sync.test.ts:1335-1345 requested a test that injects transient EBUSY during
appendFileto verifyretryHistoryWriteinappendSyncHistoryEntry. the current test at lines 1282-1346 tests the lifecycle but doesn't mock appendFile to fail.the test/sync-history.test.ts tests cover EBUSY on
openandreadFile, but not onappendFilespecifically.example test outline for appendFile retry
it("retries transient EBUSY while appending to history", async () => { const historyPath = getSyncHistoryPaths().historyPath; const originalAppendFile = nodeFs.appendFile.bind(nodeFs); let failedAppendAttempts = 0; vi.spyOn(nodeFs, "appendFile").mockImplementation( async (...args: Parameters<typeof nodeFs.appendFile>) => { const [path] = args; if (String(path) === historyPath && failedAppendAttempts < 2) { failedAppendAttempts += 1; const error = new Error("busy") as NodeJS.ErrnoException; error.code = "EBUSY"; throw error; } return originalAppendFile(...args); }, ); await appendSyncHistoryEntry({ kind: "codex-cli-sync", recordedAt: 1, run: { /* ... */ }, }); expect(failedAppendAttempts).toBe(2); const history = await readSyncHistory({ kind: "codex-cli-sync" }); expect(history).toHaveLength(1); });🤖 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 1335 - 1346, Add a regression test that simulates transient EBUSY failures on nodeFs.appendFile to exercise retryHistoryWrite used by appendSyncHistoryEntry: spy/mock nodeFs.appendFile to throw a NodeJS.ErrnoException with code "EBUSY" for the historyPath returned by getSyncHistoryPaths().historyPath for the first N attempts (e.g., 2), then delegate to the original appendFile; call appendSyncHistoryEntry({ kind: "codex-cli-sync", recordedAt: ..., run: {...} }), assert the mock failedAppendAttempts equals the number of retries and then verify persistence by calling readSyncHistory({ kind: "codex-cli-sync" }) (and/or loadLastCodexCliSyncRun) to check the entry was written; ensure you restore/unmock appendFile after the test.lib/sync-history.ts (1)
315-329:⚠️ Potential issue | 🟡 Minorremove dead code
readLatestSyncHistorySync.per past review, this function has zero usages in lib/ or test/. it blocks the event loop via
readFileSync, violating guidelines for lib/** around filesystem io patterns. the jsdoc claims "compatibility export for existing tier b callers" but no callers exist.proposed fix: delete the function
-/** - * Compatibility export for existing Tier B callers. - * Blocks the event loop; prefer readLatestSyncHistory() unless the caller is in - * startup-only code where synchronous I/O is acceptable. - */ -export function readLatestSyncHistorySync(): SyncHistoryEntry | null { - try { - const content = readFileSync(getSyncHistoryPaths().latestPath, "utf8"); - const parsed = parseEntry(content); - return parsed ? cloneEntry(parsed) : null; - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - if (code !== "ENOENT") { - log.debug("Failed to read latest sync history", { - error: error instanceof Error ? error.message : String(error), - }); - } - return null; - } -}also remove the
readFileSyncimport from line 1 if no other usages remain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sync-history.ts` around lines 315 - 329, The function readLatestSyncHistorySync is dead code and uses blocking IO; delete the entire exported function (including its body) and remove the readFileSync import if that import is no longer used elsewhere; when removing, also ensure there are no remaining exports or references to readLatestSyncHistorySync and that helper symbols referenced inside it (getSyncHistoryPaths, parseEntry, cloneEntry) remain untouched if used elsewhere.
🤖 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/sync-history.test.ts`:
- Around line 1-452: Add a new test that verifies concurrent
appendSyncHistoryEntry calls are serialized by withHistoryLock: generate N
distinct entries (unique recordedAt), fire them concurrently with Promise.all
calling appendSyncHistoryEntry for each, then call readSyncHistory({ kind:
"codex-cli-sync" }) and assert the history contains N entries with no duplicates
or missing items; verify correctness by collecting history.map(e =>
e.recordedAt).sort((a,b)=>a-b) and comparing to the expected sorted list [1..N],
and optionally ensure final ordering matches recordedAt if desired. Use the
existing helpers (__resetSyncHistoryForTests, configureSyncHistoryForTests,
getSyncHistoryPaths, etc.) and place the test alongside other it(...) blocks in
this file so it runs under the same setup/teardown.
---
Duplicate comments:
In `@lib/sync-history.ts`:
- Around line 315-329: The function readLatestSyncHistorySync is dead code and
uses blocking IO; delete the entire exported function (including its body) and
remove the readFileSync import if that import is no longer used elsewhere; when
removing, also ensure there are no remaining exports or references to
readLatestSyncHistorySync and that helper symbols referenced inside it
(getSyncHistoryPaths, parseEntry, cloneEntry) remain untouched if used
elsewhere.
In `@test/codex-cli-sync.test.ts`:
- Around line 1335-1346: Add a regression test that simulates transient EBUSY
failures on nodeFs.appendFile to exercise retryHistoryWrite used by
appendSyncHistoryEntry: spy/mock nodeFs.appendFile to throw a
NodeJS.ErrnoException with code "EBUSY" for the historyPath returned by
getSyncHistoryPaths().historyPath for the first N attempts (e.g., 2), then
delegate to the original appendFile; call appendSyncHistoryEntry({ kind:
"codex-cli-sync", recordedAt: ..., run: {...} }), assert the mock
failedAppendAttempts equals the number of retries and then verify persistence by
calling readSyncHistory({ kind: "codex-cli-sync" }) (and/or
loadLastCodexCliSyncRun) to check the entry was written; ensure you
restore/unmock appendFile after the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a38764bb-81b8-477a-8657-0111b1353571
📒 Files selected for processing (4)
lib/codex-cli/sync.tslib/sync-history.tstest/codex-cli-sync.test.tstest/sync-history.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/sync-history.tslib/codex-cli/sync.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/sync-history.test.tstest/codex-cli-sync.test.ts
🔇 Additional comments (38)
lib/sync-history.ts (10)
1-15: imports and constants look solid.the retryable codes cover the windows-specific EBUSY/EPERM contention cases. EAGAIN is included for write retries which is appropriate for unix/linux edge cases.
44-48: module-level mutable state needs careful reset in tests.
historyMutex,lastAppendError,lastAppendPaths, andpendingHistoryWritesare module-scoped. the__resetSyncHistoryForTestsfunction at lib/sync-history.ts:341 resetslastAppendErrorandlastAppendPathsbut doesn't resethistoryMutexor clearpendingHistoryWritesexplicitly after awaiting. if a test crashes mid-write, a stale promise could linger.the reset does call
waitForPendingHistoryWrites()andwithHistoryLock()which should drain in-flight work, butpendingHistoryWritesset isn't explicitly cleared. this is minor since settled promises are deleted inappendSyncHistoryEntryfinally block.
72-79: mutex implementation is correct but lacks timeout.
withHistoryLockserializes concurrent calls properly via promise chaining. however, if an operation hangs indefinitely, all subsequent calls queue forever. for a history subsystem this is acceptable, but consider adding a timeout in production if this becomes a reliability concern.
90-107: retry logic has correct exponential backoff.
retryHistoryWritecorrectly retries 5 times with delays of 25ms, 50ms, 100ms, 200ms, 400ms. the unreachablethrowat line 106 is defensive—good practice. handles EBUSY/EPERM/EAGAIN which covers windows contention per coding guidelines.
123-133: parseEntry logs error message but not entry content—good.no token or email leakage in the debug log at lib/sync-history.ts:128-130. only the error message is logged.
135-188: tail-read implementation is efficient and bounded.
readHistoryTailuses 8kb chunked reads from the end of the file, avoiding full file loads. correctly handles file handle lifecycle with try/finally. the line parsing logic correctly handles the remainder across chunk boundaries at lib/sync-history.ts:159.one edge case: if a single entry is larger than 8kb, the logic should still work since
remainderaccumulates across iterations.
190-215: trim logic has a subtle TOCTOU window.
trimHistoryFileIfNeededreads entries then writes. between the read and write, another process could append. the mutex protects in-process concurrency, but cross-process writes on windows could interleave. since this is a local user log (not shared across machines), the risk is low.
253-270: readLatestSyncHistory now has retry wrapper—good fix.the pr objectives noted
readLatestSyncHistorylacked retry. line 256-258 now wraps the read inretryHistoryWrite, addressing the transient windows lock issue. test coverage exists at test/sync-history.test.ts:295-338.
272-308: readSyncHistory handles both limited and unlimited reads with retry.limited reads use
readHistoryTailwrapped in retry at lib/sync-history.ts:279-284. unlimited reads usereadFilewrapped in retry at lib/sync-history.ts:286-288. both paths handle ENOENT gracefully.
341-368: test reset function has proper retry for windows contention.
__resetSyncHistoryForTestsat lib/sync-history.ts:341 retries file removal with exponential backoff for EBUSY/EPERM/ENOTEMPTY. correctly waits for pending writes and acquires the lock before cleanup. good windows filesystem handling per coding guidelines.lib/codex-cli/sync.ts (9)
24-29: sync-history imports are correct.importing the necessary functions for history persistence. no issues here.
119-126: module-level state management for history load deduplication.
lastCodexCliSyncHistoryLoadAttempted,lastCodexCliSyncRunLoadedFromHistory, andlastCodexCliSyncRunLoadPromisemanage the history hydration lifecycle. the test reset at lib/codex-cli/sync.ts:322-330 clears all of these.
170-189: publishCodexCliSyncRun fires and forgets history append.at lib/codex-cli/sync.ts:179-187,
appendSyncHistoryEntryis called withvoidand.catch()to swallow errors. this is intentional—history write failure shouldn't block sync completion. the error is logged at debug level without sensitive data.
245-284: race condition in loadLastCodexCliSyncRun appears fixed.past review noted a race between checking
lastCodexCliSyncHistoryLoadAttemptedat line 253 and setting it at line 259. looking at the current code:
- line 253: check
lastCodexCliSyncHistoryLoadAttempted- line 259: set
lastCodexCliSyncHistoryLoadAttempted = true- line 260: create
lastCodexCliSyncRunLoadPromisethe flag is now set synchronously before the async iife executes, which closes the window. the
.then()at lines 273-278 also caches the loaded result inlastCodexCliSyncRunLoadedFromHistoryfor subsequent calls after the promise settles.test coverage at test/codex-cli-sync.test.ts:1348-1412 (deduplicates concurrent history loads) and test/codex-cli-sync.test.ts:1414-1482 (late concurrent load alignment) verify this behavior.
322-330: test reset function clears all state correctly.
__resetLastCodexCliSyncRunForTestsclears all module-level state including the pending revision set and load promise. this prevents test pollution.
452-484: getPersistedLocalSelectionTimestamp has proper retry with backoff.retries EBUSY/EPERM up to 4 attempts with exponential backoff (10ms, 20ms, 40ms). handles ENOENT as valid (returns 0). logs exhausted retries at debug level without sensitive data.
test coverage at test/codex-cli-sync.test.ts:882-978 (preserves local selection when stat fails with EBUSY/EPERM) and test/codex-cli-sync.test.ts:1066-1177 (logs exhausted retries).
657-673: debug logs use fingerprint helper, not raw email.at lib/codex-cli/sync.ts:657-672, when logging skipped selection overwrite, no email or token is logged directly. the
makeAccountFingerprintat lib/codex-cli/sync.ts:875-878 is used elsewhere which presumably hashes/redacts the identifier.
915-957: syncAccountStorageFromCodexCli returns pendingRun: null always.this function at lib/codex-cli/sync.ts:915 normalizes indexes but never returns a pending run since it doesn't actually sync from codex cli state—it just normalizes local storage. the
pendingRun: nullat line 955 is correct.
871-879: no email leak in makeAccountFingerprint.lib/codex-cli/observability.ts:makeAccountFingerprint hashes the input (accountId or email) using SHA256 and returns only the first 12 hex characters. raw email never reaches logs—the debug output at lib/codex-cli/sync.ts:871-879 receives only the hash digest.
> Likely an incorrect or invalid review comment.test/sync-history.test.ts (9)
19-46: removeWithRetry helper handles windows cleanup robustly.includes EBUSY, EPERM, ENOTEMPTY, EACCES, ETIMEDOUT with exponential backoff. treats ENOENT as success. good test utility for cross-platform cleanup.
48-64: test setup/teardown is thorough.creates isolated tmpdir, configures history for tests, resets both sync-history and codex-cli-sync state, restores mocks. prevents test pollution.
66-137: test verifies tail-read doesn't load full file.at test/sync-history.test.ts:123-136, spies on
readFileand asserts it's not called when reading withlimit: 1. this confirmsreadHistoryTailis used instead of full file read.
139-185: EBUSY retry test for single-entry read is solid.mocks
nodeFs.opento fail twice with EBUSY then succeed. verifiesfailedOpenAttempts === 2and history is still returned. good regression test for windows contention.
187-257: EBUSY retry test for full history read is solid.mocks
nodeFs.readFileto fail twice with EBUSY then succeed. verifies retry behavior on the unlimited read path.
259-293: recovery test when latest snapshot is missing is valuable.removes the latest file, resets in-memory state, then calls
loadLastCodexCliSyncRun. verifies fallback to ndjson history file works.
295-338: EBUSY retry test for latest snapshot read covers the fix.this test at test/sync-history.test.ts:295 addresses the pr objective noting
readLatestSyncHistorylacked retry. verifies 2 failed attempts then success.
340-377: trim test verifies 200-entry cap without full file read.appends 205 entries, asserts
readFilewasn't called during append (trimming uses tail-read), then verifies only 200 entries remain with correct bounds (recordedAt 6-205).
379-451: EBUSY retry during trim is tested.populates 200 entries, mocks open to fail twice with EBUSY, appends one more to trigger trim. verifies retry succeeded and history has 200 entries with correct bounds.
test/codex-cli-sync.test.ts (10)
1-31: imports are comprehensive for the test suite.imports fs utilities, vitest, storage/state/writer modules, sync-history test helpers. the
* as storageModuleimport enables spying ongetStoragePath.
68-90: test setup properly configures isolated environment.creates tmpdir, configures sync-history for tests, sets env vars, mocks
getStoragePath, resets all module state. thetargetStoragePathvariable at line 61 is now mocked consistently.
92-117: teardown is thorough.restores mocks, resets modules, clears state, resets env vars, removes tmpdir with retry. prevents test pollution.
269-349: preview test validates no mutation and no token leakage.at test/codex-cli-sync.test.ts:343-347, serializes the preview and asserts it doesn't contain access tokens or refresh tokens. this verifies no sensitive data leaks into the preview response.
882-978: EBUSY/EPERM retry tests for selection timestamp are comprehensive.parameterized tests verify that when
fs.statfails with EBUSY or EPERM on all attempts, the local selection is preserved. verifiestargetStatCalls === SELECTION_TIMESTAMP_READ_MAX_ATTEMPTSto confirm retry exhaustion.
1282-1346: pending run lifecycle test is well-structured.verifies that:
applyCodexCliSyncToStoragereturns a pending rungetLastCodexCliSyncRun()is null before commitcommitPendingCodexCliSyncRunpublishes the run- history entry is persisted
loadLastCodexCliSyncRuncan recover after resetthis covers the deferred-commit pattern documented in the pr objectives.
1348-1412: concurrent load deduplication test verifies single read.blocks
readFileon the latest path, fires two concurrentloadLastCodexCliSyncRuncalls, releases the block, verifieslatestReadCount === 1. confirms promise deduplication works.
1484-1587: stale history hydration test prevents overwrite.blocks history read, commits a fresh sync run while blocked, releases block, verifies the in-memory state reflects the fresh run not the stale history. this tests the revision-based ordering.
1872-1931: concurrent apply returns isolated pending runs.fires two concurrent
applyCodexCliSyncToStoragecalls, verifies both return pending runs with different revisions. confirmsgetLastCodexCliSyncRun()is null until commit.
2053-2106: duplicate commit test verifies idempotency.commits the same pending run twice (once success, once failure), verifies the second commit is ignored and the original outcome persists. tests
markPendingCodexCliSyncRunCompletedreturning false on duplicate.
| import { promises as nodeFs } from "node:fs"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { | ||
| __resetLastCodexCliSyncRunForTests, | ||
| getLastCodexCliSyncRun, | ||
| loadLastCodexCliSyncRun, | ||
| } from "../lib/codex-cli/sync.js"; | ||
| import { | ||
| __resetSyncHistoryForTests, | ||
| appendSyncHistoryEntry, | ||
| configureSyncHistoryForTests, | ||
| getSyncHistoryPaths, | ||
| readLatestSyncHistory, | ||
| readSyncHistory, | ||
| } from "../lib/sync-history.js"; | ||
|
|
||
| const RETRYABLE_REMOVE_CODES = new Set([ | ||
| "EBUSY", | ||
| "EPERM", | ||
| "ENOTEMPTY", | ||
| "EACCES", | ||
| "ETIMEDOUT", | ||
| ]); | ||
|
|
||
| async function removeWithRetry( | ||
| targetPath: string, | ||
| options: { recursive?: boolean; force?: boolean }, | ||
| ): Promise<void> { | ||
| for (let attempt = 0; attempt < 6; attempt += 1) { | ||
| try { | ||
| await nodeFs.rm(targetPath, options); | ||
| return; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code === "ENOENT") { | ||
| return; | ||
| } | ||
| if (!code || !RETRYABLE_REMOVE_CODES.has(code) || attempt === 5) { | ||
| throw error; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| describe("sync history", () => { | ||
| let workDir = ""; | ||
|
|
||
| beforeEach(async () => { | ||
| workDir = await nodeFs.mkdtemp(join(tmpdir(), "codex-sync-history-")); | ||
| configureSyncHistoryForTests(join(workDir, "logs")); | ||
| await __resetSyncHistoryForTests(); | ||
| __resetLastCodexCliSyncRunForTests(); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| __resetLastCodexCliSyncRunForTests(); | ||
| await __resetSyncHistoryForTests(); | ||
| configureSyncHistoryForTests(null); | ||
| await removeWithRetry(workDir, { recursive: true, force: true }); | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it("reads the last matching history entry without loading the whole file", async () => { | ||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 1, | ||
| run: { | ||
| outcome: "noop", | ||
| runAt: 1, | ||
| sourcePath: "source-1.json", | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: 0, | ||
| targetAccountCountBefore: 0, | ||
| targetAccountCountAfter: 0, | ||
| addedAccountCount: 0, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 0, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: false, | ||
| }, | ||
| }, | ||
| }); | ||
| await appendSyncHistoryEntry({ | ||
| kind: "live-account-sync", | ||
| recordedAt: 2, | ||
| reason: "watch", | ||
| outcome: "success", | ||
| path: "openai-codex-accounts.json", | ||
| snapshot: { | ||
| path: "openai-codex-accounts.json", | ||
| running: true, | ||
| lastKnownMtimeMs: 2, | ||
| lastSyncAt: 2, | ||
| reloadCount: 1, | ||
| errorCount: 0, | ||
| }, | ||
| }); | ||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 3, | ||
| run: { | ||
| outcome: "changed", | ||
| runAt: 3, | ||
| sourcePath: "source-2.json", | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: 1, | ||
| targetAccountCountBefore: 0, | ||
| targetAccountCountAfter: 1, | ||
| addedAccountCount: 1, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 0, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: true, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const readFileSpy = vi.spyOn(nodeFs, "readFile"); | ||
|
|
||
| const history = await readSyncHistory({ kind: "codex-cli-sync", limit: 1 }); | ||
|
|
||
| expect(history).toHaveLength(1); | ||
| expect(history[0]).toMatchObject({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 3, | ||
| run: expect.objectContaining({ | ||
| outcome: "changed", | ||
| sourcePath: "source-2.json", | ||
| }), | ||
| }); | ||
| expect(readFileSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("retries transient EBUSY while reading the last matching history entry", async () => { | ||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 1, | ||
| run: { | ||
| outcome: "changed", | ||
| runAt: 1, | ||
| sourcePath: "source-1.json", | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: 1, | ||
| targetAccountCountBefore: 0, | ||
| targetAccountCountAfter: 1, | ||
| addedAccountCount: 1, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 0, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: false, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const historyPath = getSyncHistoryPaths().historyPath; | ||
| const originalOpen = nodeFs.open.bind(nodeFs); | ||
| let failedOpenAttempts = 0; | ||
| vi.spyOn(nodeFs, "open").mockImplementation( | ||
| async (...args: Parameters<typeof nodeFs.open>) => { | ||
| const [path] = args; | ||
| if (String(path) === historyPath && failedOpenAttempts < 2) { | ||
| failedOpenAttempts += 1; | ||
| const error = new Error("busy") as NodeJS.ErrnoException; | ||
| error.code = "EBUSY"; | ||
| throw error; | ||
| } | ||
| return originalOpen(...args); | ||
| }, | ||
| ); | ||
|
|
||
| const history = await readSyncHistory({ kind: "codex-cli-sync", limit: 1 }); | ||
|
|
||
| expect(failedOpenAttempts).toBe(2); | ||
| expect(history).toHaveLength(1); | ||
| expect(history[0]).toMatchObject({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 1, | ||
| }); | ||
| }); | ||
|
|
||
| it("retries transient EBUSY while reading the full history file", async () => { | ||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 1, | ||
| run: { | ||
| outcome: "changed", | ||
| runAt: 1, | ||
| sourcePath: "source-1.json", | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: 1, | ||
| targetAccountCountBefore: 0, | ||
| targetAccountCountAfter: 1, | ||
| addedAccountCount: 1, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 0, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: false, | ||
| }, | ||
| }, | ||
| }); | ||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 2, | ||
| run: { | ||
| outcome: "noop", | ||
| runAt: 2, | ||
| sourcePath: "source-2.json", | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: 1, | ||
| targetAccountCountBefore: 1, | ||
| targetAccountCountAfter: 1, | ||
| addedAccountCount: 0, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 1, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: false, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const historyPath = getSyncHistoryPaths().historyPath; | ||
| const originalReadFile = nodeFs.readFile.bind(nodeFs); | ||
| let failedReadAttempts = 0; | ||
| vi.spyOn(nodeFs, "readFile").mockImplementation( | ||
| async (...args: Parameters<typeof nodeFs.readFile>) => { | ||
| const [path] = args; | ||
| if (String(path) === historyPath && failedReadAttempts < 2) { | ||
| failedReadAttempts += 1; | ||
| const error = new Error("busy") as NodeJS.ErrnoException; | ||
| error.code = "EBUSY"; | ||
| throw error; | ||
| } | ||
| return originalReadFile(...args); | ||
| }, | ||
| ); | ||
|
|
||
| const history = await readSyncHistory({ kind: "codex-cli-sync" }); | ||
|
|
||
| expect(failedReadAttempts).toBe(2); | ||
| expect(history).toHaveLength(2); | ||
| expect(history[0]).toMatchObject({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 1, | ||
| }); | ||
| expect(history[1]).toMatchObject({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 2, | ||
| }); | ||
| }); | ||
|
|
||
| it("recovers the last codex-cli sync run from history when the latest snapshot is missing", async () => { | ||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 11, | ||
| run: { | ||
| outcome: "changed", | ||
| runAt: 11, | ||
| sourcePath: "source.json", | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: 1, | ||
| targetAccountCountBefore: 0, | ||
| targetAccountCountAfter: 1, | ||
| addedAccountCount: 1, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 0, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: true, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await nodeFs.rm(getSyncHistoryPaths().latestPath, { force: true }); | ||
| __resetLastCodexCliSyncRunForTests(); | ||
|
|
||
| expect(getLastCodexCliSyncRun()).toBeNull(); | ||
| await expect(loadLastCodexCliSyncRun()).resolves.toMatchObject({ | ||
| outcome: "changed", | ||
| sourcePath: "source.json", | ||
| targetPath: "target.json", | ||
| summary: expect.objectContaining({ | ||
| addedAccountCount: 1, | ||
| }), | ||
| }); | ||
| }); | ||
|
|
||
| it("retries transient EBUSY while reading the latest sync history snapshot", async () => { | ||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 7, | ||
| run: { | ||
| outcome: "changed", | ||
| runAt: 7, | ||
| sourcePath: "source-7.json", | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: 1, | ||
| targetAccountCountBefore: 0, | ||
| targetAccountCountAfter: 1, | ||
| addedAccountCount: 1, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 0, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: false, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const latestPath = getSyncHistoryPaths().latestPath; | ||
| const originalReadFile = nodeFs.readFile.bind(nodeFs); | ||
| let failedReadAttempts = 0; | ||
| vi.spyOn(nodeFs, "readFile").mockImplementation( | ||
| async (...args: Parameters<typeof nodeFs.readFile>) => { | ||
| const [path] = args; | ||
| if (String(path) === latestPath && failedReadAttempts < 2) { | ||
| failedReadAttempts += 1; | ||
| const error = new Error("busy") as NodeJS.ErrnoException; | ||
| error.code = "EBUSY"; | ||
| throw error; | ||
| } | ||
| return originalReadFile(...args); | ||
| }, | ||
| ); | ||
|
|
||
| await expect(readLatestSyncHistory()).resolves.toMatchObject({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 7, | ||
| }); | ||
| expect(failedReadAttempts).toBe(2); | ||
| }); | ||
|
|
||
| it("trims oversized history without re-reading the full history file", async () => { | ||
| const readFileSpy = vi.spyOn(nodeFs, "readFile"); | ||
|
|
||
| for (let index = 1; index <= 205; index += 1) { | ||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: index, | ||
| run: { | ||
| outcome: "changed", | ||
| runAt: index, | ||
| sourcePath: `source-${index}.json`, | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: index, | ||
| targetAccountCountBefore: index - 1, | ||
| targetAccountCountAfter: index, | ||
| addedAccountCount: 1, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 0, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: false, | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| expect(readFileSpy).not.toHaveBeenCalled(); | ||
| const history = await readSyncHistory({ kind: "codex-cli-sync" }); | ||
| expect(history).toHaveLength(200); | ||
| expect(history[0]).toMatchObject({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 6, | ||
| }); | ||
| expect(history.at(-1)).toMatchObject({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 205, | ||
| }); | ||
| }); | ||
|
|
||
| it("retries transient EBUSY while opening history during trim", async () => { | ||
| for (let index = 1; index <= 200; index += 1) { | ||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: index, | ||
| run: { | ||
| outcome: "changed", | ||
| runAt: index, | ||
| sourcePath: `source-${index}.json`, | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: index, | ||
| targetAccountCountBefore: index - 1, | ||
| targetAccountCountAfter: index, | ||
| addedAccountCount: 1, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 0, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: false, | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const historyPath = getSyncHistoryPaths().historyPath; | ||
| const originalOpen = nodeFs.open.bind(nodeFs); | ||
| let failedOpenAttempts = 0; | ||
| vi.spyOn(nodeFs, "open").mockImplementation( | ||
| async (...args: Parameters<typeof nodeFs.open>) => { | ||
| const [path] = args; | ||
| if (String(path) === historyPath && failedOpenAttempts < 2) { | ||
| failedOpenAttempts += 1; | ||
| const error = new Error("busy") as NodeJS.ErrnoException; | ||
| error.code = "EBUSY"; | ||
| throw error; | ||
| } | ||
| return originalOpen(...args); | ||
| }, | ||
| ); | ||
|
|
||
| await appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 201, | ||
| run: { | ||
| outcome: "changed", | ||
| runAt: 201, | ||
| sourcePath: "source-201.json", | ||
| targetPath: "target.json", | ||
| summary: { | ||
| sourceAccountCount: 201, | ||
| targetAccountCountBefore: 200, | ||
| targetAccountCountAfter: 201, | ||
| addedAccountCount: 1, | ||
| updatedAccountCount: 0, | ||
| unchangedAccountCount: 0, | ||
| destinationOnlyPreservedCount: 0, | ||
| selectionChanged: false, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const history = await readSyncHistory({ kind: "codex-cli-sync" }); | ||
| expect(failedOpenAttempts).toBe(2); | ||
| expect(history).toHaveLength(200); | ||
| expect(history[0]).toMatchObject({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 2, | ||
| }); | ||
| expect(history.at(-1)).toMatchObject({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: 201, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
missing test: concurrent appendSyncHistoryEntry serialization.
past review requested a test verifying that concurrent appendSyncHistoryEntry calls serialize correctly via withHistoryLock. current tests verify retry behavior but don't verify that two concurrent appends don't interleave or lose entries.
consider adding a test that fires multiple concurrent appends and verifies:
- all entries are persisted
- entries appear in the correct order
- no entries are lost or duplicated
example test outline
it("serializes concurrent append calls without losing entries", async () => {
const entries = Array.from({ length: 10 }, (_, i) => ({
kind: "codex-cli-sync" as const,
recordedAt: i + 1,
run: {
outcome: "changed" as const,
runAt: i + 1,
sourcePath: `source-${i + 1}.json`,
targetPath: "target.json",
summary: { /* ... */ },
},
}));
// fire all appends concurrently
await Promise.all(entries.map((entry) => appendSyncHistoryEntry(entry)));
const history = await readSyncHistory({ kind: "codex-cli-sync" });
expect(history).toHaveLength(10);
// verify all entries present (order may vary due to scheduling)
const recordedAts = history.map((e) => e.recordedAt).sort((a, b) => a - b);
expect(recordedAts).toEqual([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sync-history.test.ts` around lines 1 - 452, Add a new test that verifies
concurrent appendSyncHistoryEntry calls are serialized by withHistoryLock:
generate N distinct entries (unique recordedAt), fire them concurrently with
Promise.all calling appendSyncHistoryEntry for each, then call readSyncHistory({
kind: "codex-cli-sync" }) and assert the history contains N entries with no
duplicates or missing items; verify correctness by collecting history.map(e =>
e.recordedAt).sort((a,b)=>a-b) and comparing to the expected sorted list [1..N],
and optionally ensure final ordering matches recordedAt if desired. Use the
existing helpers (__resetSyncHistoryForTests, configureSyncHistoryForTests,
getSyncHistoryPaths, etc.) and place the test alongside other it(...) blocks in
this file so it runs under the same setup/teardown.
|
Closing this preserved |
Summary
#84fresh/06-docs-frontdoor-cleanupWhat Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
44bb909andd57449fAdditional Notes
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr adds sync result history (
sync-history.ndjson+sync-history-latest.json), the last-result log for the codex cli sync center, a pending-run commit pattern for tracking sync outcomes across async save boundaries, and several new dashboard entry points (reset local state, restore-backup, startup recovery prompt).key changes:
lib/sync-history.ts(new): bounded NDJSON history with backward tail-read, retry onEBUSY/EPERM, and async-history-latest.jsonfast-path;retryHistoryWriteis correctly applied to reads and writes;trimHistoryFileIfNeedednow usesreadHistoryTailinstead of a fullreadFilelib/codex-cli/sync.ts: addsapplyCodexCliSyncToStorage,previewCodexCliSync,loadLastCodexCliSyncRun, and the pending-run commit/failure helpers;hydrateLastCodexCliSyncRunFromHistorynow guards against clobbering a newer in-memory run;loadLastCodexCliSyncRunuses correct.then/.finallyordering so no stale-null window existslib/codex-manager.ts: adds reset mode, restore-backup entry point, and startup recovery prompt withdestructiveActionInFlightguardlib/destructive-actions.ts: centralizes delete-account, delete-saved-accounts, and reset-local-state operationsissues found:
readLatestSyncHistorySyncis exported (re-exported fromlib/index.ts) with zero callers — dead blocking export usingreadFileSync; latent event-loop and windows filesystem risktrimHistoryFileIfNeededruns a chunked tail-read on every append regardless of current file size; an in-memory entry counter would eliminate the disk open/read for the common sub-cap casecommitPendingCodexCliSyncRunoverwritesrunAtwithDate.now()at commit time, stamping the save-completion time rather than the reconcile-computation time into the history recordConfidence Score: 3/5
hydrateLastCodexCliSyncRunFromHistoryguard and.then/.finallyordering are correct; remaining issues are a dead blocking export in the public api, a per-append disk read that should be guarded by an in-memory counter, and arunAttimestamp that reflects save latency rather than reconcile timelib/sync-history.ts(dead blocking export + per-append trim read),lib/codex-cli/sync.ts(runAt timestamp at commit rather than compute time)Important Files Changed
retryHistoryWriteandretryHistoryReadare wired correctly;trimHistoryFileIfNeededstill does a chunked tail-read on every append (no in-memory counter guard);readLatestSyncHistorySyncis a dead blocking export with no callershydrateLastCodexCliSyncRunFromHistorynow guards against clobbering newer in-memory runs;loadLastCodexCliSyncRunuses async history reads with correct.then/.finallyordering;commitPendingCodexCliSyncRunoverwritesrunAtwithDate.now()at commit time rather than preserving the computation timestamppromptSyncCenterUI andbuildSyncCenterOverviewhelper;normalizePathForComparisonhandles windows paths correctly; no new concurrency or token-safety concerns introducedresetmode andrestore-backupentry point;destructiveActionInFlightguard prevents double-execution; startup recovery prompt with scan failure handling looks correct;runBackupRestoreManagerre-assesses before committing restore to mitigate TOCTOUdeleteAccountAtIndex,deleteSavedAccounts, andresetLocalState; straightforward delegation with matching testsloadLastCodexCliSyncRuncold-start paths;LiveAccountSyncSnapshotshapes match the interface; missing a test that verifies the trim-on-every-append frequency is acceptableappendSyncHistoryEntrycalls looks correctSequence Diagram
sequenceDiagram participant Caller participant sync as codex-cli/sync.ts participant history as sync-history.ts participant disk as Disk (NDJSON + latest.json) Caller->>sync: applyCodexCliSyncToStorage(current) sync->>sync: reconcileCodexCliState() → pendingRun {revision: N, runAt: T1} sync-->>Caller: {storage, changed:true, pendingRun} Caller->>Caller: saveAccounts(storage) [T1..T2] Caller->>sync: commitPendingCodexCliSyncRun(pendingRun) sync->>sync: markPendingCodexCliSyncRunCompleted(N) sync->>sync: publishCodexCliSyncRun({...run, runAt:Date.now()}, allocateRevision()) Note over sync: runAt stamped at T2 (commit), not T1 (compute) sync->>history: void appendSyncHistoryEntry({kind:"codex-cli-sync", run}) history->>history: withHistoryLock → retryHistoryWrite history->>disk: fs.appendFile(sync-history.ndjson) history->>history: trimHistoryFileIfNeeded (readHistoryTail on every append) history->>disk: fs.writeFile(sync-history-latest.json) Caller->>sync: loadLastCodexCliSyncRun() [cold start] sync->>history: readLatestSyncHistory() history->>disk: retryHistoryWrite(readFile(latest.json)) disk-->>history: latest entry history-->>sync: SyncHistoryEntry | null alt latest.kind == "codex-cli-sync" sync->>sync: hydrateLastCodexCliSyncRunFromHistory(run) [guards if !lastCodexCliSyncRun] else latest missing or wrong kind sync->>history: readSyncHistory({kind:"codex-cli-sync", limit:1}) history->>disk: retryHistoryWrite(readHistoryTail ← bounded backward read) disk-->>history: entries history-->>sync: SyncHistoryEntry[] sync->>sync: hydrateLastCodexCliSyncRunFromHistory(lastEntry.run) end sync-->>Caller: CodexCliSyncRun | nullComments Outside Diff (2)
lib/codex-cli/sync.ts, line 322-330 (link).then()callback overwrites state after test reset__resetLastCodexCliSyncRunForTestssetslastCodexCliSyncRunLoadedFromHistory = nullandlastCodexCliSyncRunLoadPromise = nullsynchronously, but ifloadLastCodexCliSyncRunkicked off a history read that is still awaiting i/o at the moment of reset, the queued.then()callback (line 273-277 in the load chain) will fire after the reset and write the stale loaded run back intolastCodexCliSyncRunLoadedFromHistory. this leaves module-level state from the previous test in place, which can cause subsequent tests that checkloadLastCodexCliSyncRun()to return a non-null value when they expect null — a classic flaky-test vector.the standard fix is a generation counter that the in-flight closure captures and checks before writing:
Prompt To Fix With AI
lib/codex-cli/sync.ts, line 1284-1298 (link)runAttimestamp withDate.now()at commit time, not at sync timeapplyCodexCliSyncToStoragecreatessyncRunwithcreateSyncRun(...)→runAt: Date.now()at computation time T1. it stores this inpendingRun.run. thencommitPendingCodexCliSyncRundoes:if the
saveAccountscall takes a few hundred milliseconds (e.g., windows antivirus holding a lock), the publishedrunAtis the save-completion time, not when the sync was actually computed. this makes the history timestamp misleading — a user looking at the log sees a time that reflects how slow the disk write was rather than when the reconcile happened.since
syncRun.runAtis already set to the computation time inapplyCodexCliSyncToStorage, preserving it in the commit is more accurate:same fix applies to
commitCodexCliSyncRunFailure.Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "fix(sync): retry lat..."