feat(partition): M9 — SyncController extraction, JoinReducer split, bitmap OID validation#44
feat(partition): M9 — SyncController extraction, JoinReducer split, bitmap OID validation#44flyingrobots merged 14 commits intomainfrom
Conversation
- Convert broken {..|..} record syntax to HTML TABLE elements (4 files, 6 nodes)
- Remove sed post-processing that stripped intentional fill="white" from nodes
- Rename fig-empty-tree → fig-data-storage; remove misleading empty-tree node
- Fix architecture arrows: port → adapter with "implements" labels
- Reorient multi-writer and materialize-pipeline diagrams to vertical (TB)
- Add missing index_port/cache_port edges in architecture diagram
- Add elision nodes for dangling leaf refs in ref-layout diagram
- Show dot warnings in build script instead of suppressing stderr
- Truncate SHA to 7 chars, fix beta fibre label, consistent cluster fontsize
- Embed diagrams in README.md (3) and docs/GUIDE.md (7)
…ath, add bitmap OID validation B33 — SyncController extraction: all 9 sync methods + 2 private helpers moved from sync.methods.js free functions into a SyncController class in src/domain/services/. sync.methods.js reduced to thin one-liner delegation stubs. WarpGraph instantiates SyncController in constructor. 16 new unit tests with mock host object. B32 — JoinReducer dual-path refactor: extracted updateFrontierFromPatch() shared helper, created applyFast() and applyWithReceipt() as named exported functions, simplified join() to 3-line dispatcher. reduceV5() calls named functions directly. 4 new tests. B31 — Bitmap OID validation: added isValidOid() domain-local validator in src/domain/utils/validateShardOid.js. BitmapIndexReader.setup() now validates each OID — strict mode throws ShardCorruptionError, non-strict skips with warning. 13 new tests. Updated mock OIDs across 4 test files to use valid hex strings. M9 gate: WarpGraph.js = 422 LOC (< 500); 3864/3864 tests pass; no-coordination regression suite green.
CHANGELOG updated with SyncController extraction, JoinReducer dual-path refactor, and bitmap OID validation. package.json and jsr.json versions synced to 11.5.1.
The previous default (strict: false) combined with the nullLogger default meant corrupt shards were silently swallowed. Flipping to strict: true ensures corruption is surfaced immediately. The only production caller (IndexRebuildService.load()) already passed strict: true, so this changes nothing in practice — it only protects direct callers from the silent-loss footgun.
B58: dead sync constants in _internal.js B59: wireWarpMethods 3-hop indirection for sync B60: test helper for mock OIDs B61: doubled @type {any} annotations in tests B62: SyncHost typedef for SyncController coupling
B58 — Delete orphaned sync constants and SyncError/OperationAbortedError re-exports from _internal.js (no remaining importers after SyncController extraction). B59 — Delete sync.methods.js entirely. Wire 9 sync methods directly onto WarpGraph.prototype via defineProperty loop delegating to _syncController. Eliminates the 3-hop indirection (prototype → stub → controller). B60 — Add test/helpers/mockOid.js: deterministic hex OID generator that prevents test breakage when OID validation is tightened. 6 tests. B61 — Remove 66 doubled /** @type */ annotations across 13 test files. B62 — Add @typedef SyncHost to SyncController.js documenting the exact 12-field + 3-method host interface the controller depends on. Constructor now typed against SyncHost instead of WarpGraph import.
…Controller tests - SyncController.applySyncResponse() was double-casting observedFrontier (Lamport counters Map<string, number>) to SHA frontier; now uses _lastFrontier with createFrontier() fallback (fixes B56) - syncWith() calls this.createSyncRequest/applySyncResponse instead of this._host.* (avoids infinite delegation loop) - BitmapIndexReader JSDoc: fix variable name in non-strict example - Delete unused mockOid helper (B60 reverted) - Update sync tests to mock on _syncController after extraction - Add SyncController unit tests: applySyncResponse, syncNeeded, processSyncRequest, syncWith direct-peer - Add validateShardOid edge-case tests
… ratchet
- SyncHost.materialize: (options?: *) → (options?: Record<string, unknown>)
- serve() graph cast: {*} → {processSyncRequest: Function} via unknown
|
@coderabbitai review please 🥕 |
|
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. 📝 WalkthroughWalkthroughThis pull request refactors the sync architecture by extracting all synchronization logic from Changes
Sequence Diagram(s)sequenceDiagram
participant WG as WarpGraph
participant SC as SyncController
participant P as Persistence
participant R as Remote (HTTP/Peer)
participant HS as HttpSyncServer
WG->>SC: invoke syncWith(remote, options)
SC->>P: getFrontier() / read refs
alt direct peer
SC->>R: call processSyncRequest(request) on peer
R-->>SC: syncResponse
else http path
SC->>R: HTTP POST /sync with auth headers
R-->>SC: HTTP response (JSON or error)
end
SC->>SC: validate/parse response
SC->>P: applySyncResponse -> update cached state / lastFrontier
SC-->>WG: return sync result (applied count / state)
WG->>HS: (serve) HttpSyncServer constructed with host/graph and auth
HS->>SC: delegate incoming requests to SC.processSyncRequest
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/domain/utils/validateShardOid.js (1)
7-9: Consider documenting the rationale for the 4-char minimum.The 4-character lower bound is permissive relative to Git's default minimum abbreviation length of 7 and its full SHA-1 length of 40. A comment explaining the intent (e.g., "allows abbreviated test fixtures and short internal IDs") would make the constraint self-explanatory for future maintainers.
📝 Suggested doc addition
/** * Validates a shard Object ID (hex string, 4-64 chars). * + * The 4-character minimum accommodates abbreviated OIDs (e.g. test fixtures, + * short internal IDs). Git full SHA-1 OIDs are 40 chars; SHA-256 OIDs are 64 chars. + * * `@param` {string} oid - The OID to validate * `@returns` {boolean} True if oid is a valid hex string of 4-64 characters */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/utils/validateShardOid.js` around lines 7 - 9, Add a short explanatory comment above the isValidOid function (which uses the /^[0-9a-fA-F]{4,64}$/ check) describing why the minimum is 4 characters—e.g., to permit abbreviated test fixtures and short internal IDs while still preventing overly short/noisy values and allowing full SHA-length IDs—so future maintainers understand the rationale and can adjust the bound intentionally.test/unit/domain/WarpGraph.syncWith.test.js (1)
28-29: Usevi.spyOn()for mocking_syncControllermethods instead of direct property assignment.Direct property assignment creates own-properties that shadow prototype methods. While the current test structure with fresh instances per
beforeEachprevents mock leakage,vi.spyOn()is the idiomatic Vitest pattern and aligns with existing patterns in this file (e.g., line 72). It provides cleaner automatic restoration and is more declarative.♻️ Suggested alternative
- /** `@type` {any} */ (graph)._syncController.applySyncResponse = vi.fn().mockReturnValue({ applied: 0 }); - /** `@type` {any} */ (graph)._syncController.createSyncRequest = vi.fn().mockResolvedValue({ type: 'sync-request', frontier: {} }); + vi.spyOn(/** `@type` {any} */ (graph)._syncController, 'applySyncResponse').mockReturnValue({ applied: 0 }); + vi.spyOn(/** `@type` {any} */ (graph)._syncController, 'createSyncRequest').mockResolvedValue({ type: 'sync-request', frontier: {} });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/WarpGraph.syncWith.test.js` around lines 28 - 29, The test currently assigns mocks directly to graph._syncController.applySyncResponse and graph._syncController.createSyncRequest which creates own-properties that shadow prototype methods; replace those direct assignments with vi.spyOn(graph._syncController, 'applySyncResponse').mockReturnValue({ applied: 0 }) and vi.spyOn(graph._syncController, 'createSyncRequest').mockResolvedValue({ type: 'sync-request', frontier: {} }) so the spies follow the idiomatic Vitest pattern, get automatic restoration, and match the style used elsewhere in the file (referencing graph._syncController.applySyncResponse and graph._syncController.createSyncRequest).test/unit/domain/services/SyncController.test.js (1)
397-413:createSyncRequesttest verifies shape but not frontier content.The
discoverWritersmock resolves to['alice']andreadRefresolves to'sha-alice', so the resulting request frontier is deterministic. Assertingfrontier.alice === 'sha-alice'(or equivalent) would give stronger signal thatgetFrontieris wired correctly end-to-end throughcreateSyncRequest.♻️ Suggested addition
expect(request).toHaveProperty('type', 'sync-request'); expect(request).toHaveProperty('frontier'); +expect(request.frontier).toMatchObject({ alice: 'sha-alice' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/SyncController.test.js` around lines 397 - 413, The test for SyncController.createSyncRequest currently only checks the request shape; update the test to assert the actual frontier entries by verifying that the produced request.frontier contains the expected mapping for the mocked writer (e.g., assert request.frontier.alice === 'sha-alice' or deep-equal the whole frontier object). Use the existing mocks (discoverWriters resolving to ['alice'] and _persistence.readRef resolving to 'sha-alice') and add an expectation after creating the request to check the frontier value returned by createSyncRequest (via SyncController.createSyncRequest / getFrontier flow).test/unit/domain/WarpGraph.syncAuth.test.js (1)
16-31: Consider a brief comment explaining the two mocking strategies.
mockClientGraphmust mock via_syncControllerbecausecreateSyncRequestandapplySyncResponseare invoked asthis.xxx()insideSyncController.syncWith— mocking on the WarpGraph instance would not intercept them.mockServerGraphcan mock directly on the instance becauseprocessSyncRequestis called externally asremote.processSyncRequest(request)and instance properties shadow the prototype delegation. A one-line JSDoc explaining this prevents future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/WarpGraph.syncAuth.test.js` around lines 16 - 31, Add a one-line JSDoc above the mockClientGraph and mockServerGraph functions explaining why they mock differently: note that mockClientGraph must stub methods on the internal _syncController because SyncController.syncWith calls createSyncRequest and applySyncResponse as this.method() (so mocking on the WarpGraph instance won't intercept them), whereas mockServerGraph can stub processSyncRequest directly on the WarpGraph instance because remote.processSyncRequest(...) is invoked externally and instance properties shadow the prototype. Include the function names (_syncController, createSyncRequest, applySyncResponse, processSyncRequest, mockClientGraph, mockServerGraph) in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 12-13: The "Publication-quality SVG diagrams" and
"`scripts/build-diagrams.sh`" bullets currently live under the Unreleased
section in CHANGELOG.md but appear to belong to release 11.5.1; move those two
bullets out of the "Unreleased" section and add them under the "11.5.1" ->
"Added" subsection (or remove/clear the Unreleased section if it should be
empty). Locate the exact lines by searching for the bullet text
"Publication-quality SVG diagrams" and "`scripts/build-diagrams.sh`" and update
the headings accordingly.
In `@src/domain/services/BitmapIndexReader.js`:
- Line 94: Constructor default for BitmapIndexReader flipped strict from false
to true causing a breaking change; restore the previous lenient default by
changing the constructor parameter back to strict = false in the
BitmapIndexReader constructor (the function named "constructor" inside class
BitmapIndexReader) and ensure the published type declaration (index.d.ts) and
any JSDoc reflect strict's default is false; also add a CHANGELOG/migration note
calling out the behavior and telling consumers to pass strict: false explicitly
if they relied on lenient validation.
In `@src/domain/services/JoinReducer.js`:
- Around line 352-358: The code in updateFrontierFromPatch calls
vvDeserialize(patch.context) without guarding against patch.context being
null/undefined, which can cause Object.entries(null) inside vvDeserialize to
throw; fix by adding a null guard so that if patch.context is null or undefined
you treat it as an empty context (e.g., pass an empty object or new Map) before
calling vvDeserialize; updateFrontierFromPatch should check patch.context (and
still accept Map) and only call vvDeserialize when safe, then proceed to vvMerge
and foldPatchDot as before.
---
Duplicate comments:
In `@test/unit/domain/WarpGraph.timing.test.js`:
- Around line 252-253: Replace direct assignments to graph._syncController
methods with spies using vi.spyOn to avoid overwriting internal controller
objects; specifically spy on (graph._syncController, 'applySyncResponse') and
(graph._syncController, 'createSyncRequest') and mock their return values
(mockReturnValue / mockResolvedValue) instead of assigning functions directly,
and for any tests that assert property descriptors verify with
Object.getOwnPropertyDescriptor on graph._syncController to ensure you did not
redefine the property (apply the same change at the other occurrences referenced
around the other blocks).
---
Nitpick comments:
In `@src/domain/utils/validateShardOid.js`:
- Around line 7-9: Add a short explanatory comment above the isValidOid function
(which uses the /^[0-9a-fA-F]{4,64}$/ check) describing why the minimum is 4
characters—e.g., to permit abbreviated test fixtures and short internal IDs
while still preventing overly short/noisy values and allowing full SHA-length
IDs—so future maintainers understand the rationale and can adjust the bound
intentionally.
In `@test/unit/domain/services/SyncController.test.js`:
- Around line 397-413: The test for SyncController.createSyncRequest currently
only checks the request shape; update the test to assert the actual frontier
entries by verifying that the produced request.frontier contains the expected
mapping for the mocked writer (e.g., assert request.frontier.alice ===
'sha-alice' or deep-equal the whole frontier object). Use the existing mocks
(discoverWriters resolving to ['alice'] and _persistence.readRef resolving to
'sha-alice') and add an expectation after creating the request to check the
frontier value returned by createSyncRequest (via
SyncController.createSyncRequest / getFrontier flow).
In `@test/unit/domain/WarpGraph.syncAuth.test.js`:
- Around line 16-31: Add a one-line JSDoc above the mockClientGraph and
mockServerGraph functions explaining why they mock differently: note that
mockClientGraph must stub methods on the internal _syncController because
SyncController.syncWith calls createSyncRequest and applySyncResponse as
this.method() (so mocking on the WarpGraph instance won't intercept them),
whereas mockServerGraph can stub processSyncRequest directly on the WarpGraph
instance because remote.processSyncRequest(...) is invoked externally and
instance properties shadow the prototype. Include the function names
(_syncController, createSyncRequest, applySyncResponse, processSyncRequest,
mockClientGraph, mockServerGraph) in the comment for clarity.
In `@test/unit/domain/WarpGraph.syncWith.test.js`:
- Around line 28-29: The test currently assigns mocks directly to
graph._syncController.applySyncResponse and
graph._syncController.createSyncRequest which creates own-properties that shadow
prototype methods; replace those direct assignments with
vi.spyOn(graph._syncController, 'applySyncResponse').mockReturnValue({ applied:
0 }) and vi.spyOn(graph._syncController,
'createSyncRequest').mockResolvedValue({ type: 'sync-request', frontier: {} })
so the spies follow the idiomatic Vitest pattern, get automatic restoration, and
match the style used elsewhere in the file (referencing
graph._syncController.applySyncResponse and
graph._syncController.createSyncRequest).
| * @param {import('../../ports/CryptoPort.js').default} [options.crypto] - CryptoPort instance for checksum verification. | ||
| */ | ||
| constructor({ storage, strict = false, logger = nullLogger, maxCachedShards = DEFAULT_MAX_CACHED_SHARDS, crypto } = /** @type {{ storage: IndexStoragePort, strict?: boolean, logger?: LoggerPort, maxCachedShards?: number, crypto?: CryptoPort }} */ ({})) { | ||
| constructor({ storage, strict = true, logger = nullLogger, maxCachedShards = DEFAULT_MAX_CACHED_SHARDS, crypto } = /** @type {{ storage: IndexStoragePort, strict?: boolean, logger?: LoggerPort, maxCachedShards?: number, crypto?: CryptoPort }} */ ({})) { |
There was a problem hiding this comment.
Breaking default change: strict flipped from false to true.
Existing callers that rely on new BitmapIndexReader({ storage }) (no strict argument) previously got lenient behaviour — shard validation failures were swallowed with warnings, allowing graceful degradation. After this change they will throw ShardCorruptionError on any shard OID or validation failure. Code paths that previously worked in degraded mode will now hard-fail.
Since BitmapIndexReader is in the published type declaration (index.d.ts) this is a semver-major API change; the version was bumped to 11.5.1 (minor). Callers that need the previous lenient behaviour must now pass strict: false explicitly. Ensure this is called out in the CHANGELOG and migration notes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/BitmapIndexReader.js` at line 94, Constructor default for
BitmapIndexReader flipped strict from false to true causing a breaking change;
restore the previous lenient default by changing the constructor parameter back
to strict = false in the BitmapIndexReader constructor (the function named
"constructor" inside class BitmapIndexReader) and ensure the published type
declaration (index.d.ts) and any JSDoc reflect strict's default is false; also
add a CHANGELOG/migration note calling out the behavior and telling consumers to
pass strict: false explicitly if they relied on lenient validation.
There was a problem hiding this comment.
Intentional — the previous lenient default silently skipped corrupt shards, masking data integrity issues. The fail-safe flip aligns with the OID validation addition in this PR. Existing callers in this codebase all pass strict: true explicitly (or inherit the default through BitmapIndexBuilder), so no consumer breakage. Documented in CHANGELOG under 'Added'.
There was a problem hiding this comment.
@flyingrobots, understood — thanks for the context! If all existing callers already pass strict: true explicitly or inherit it, and you've verified no consumer code will break, then the default flip is safe. Appreciate the proactive documentation in the CHANGELOG.
- CHANGELOG: move diagram bullets from Unreleased to 11.5.1 section - JoinReducer.updateFrontierFromPatch: null-guard patch.context before vvDeserialize (defensive against malformed patches) - SyncController.test: assert frontier content in createSyncRequest test
Release Preflight
If you tag this commit as |
Address 8 warnings and 1 info item from self-review of feat/m9-partition: - W8: mockServerGraph now mocks on _syncController (matching client pattern) - W7: standardize all BitmapIndexReader OIDs to 40-char hex - I5: fix stale comment (non-strict reader) - W1: add frontier source verification test - W5: add null/undefined context guard tests for applyFast - W3: add auto-materialize path tests for syncWith - W2: add 9 HTTP sync path tests (status codes, errors, auth, retries) - W4: add 3 serve() deeper tests (constructor, auth config, graph host)
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
eslint.config.js (1)
206-206: Remove stale reference to removed file from eslint configuration.The file
src/domain/warp/sync.methods.jsno longer exists (replaced bysrc/domain/services/SyncController.js), but the reference on line 206 remains in the files array. Remove this stale entry to clean up the lint config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` at line 206, Remove the stale filename entry "src/domain/warp/sync.methods.js" from the eslint config files array in eslint.config.js; locate the files array that currently contains that string and delete that element (no replacement needed since the file was replaced by src/domain/services/SyncController.js).src/domain/services/SyncController.js (1)
55-57:E_NO_STATE_MSGis duplicated from_internal.js.This constant also exists in
src/domain/warp/_internal.js(line 15). Consider importing it from the canonical location to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/SyncController.js` around lines 55 - 57, Remove the duplicated E_NO_STATE_MSG constant and import the canonical symbol from the existing internal module instead: replace the local const E_NO_STATE_MSG in SyncController.js with an import of E_NO_STATE_MSG from the warp internal module that already exports it (ensure you use the correct named/default import to match the original export in _internal.js), and run tests to confirm no other references rely on the local definition.CHANGELOG.md (1)
185-185: Stale orphaned## [Unreleased]at line 185 — remove it.There's a pre-existing empty
## [Unreleased]heading between[11.3.2]and[11.3.0]that predates this PR. With a real[Unreleased]section now at the top of the file, changelog parsers and readers may misinterpret the structure (e.g., treating everything between line 185 and line 187 as an unreleased block, or flagging a duplicate heading).🧹 Proposed removal
-## [Unreleased] - ## [11.3.0] — 2026-02-17 — DX-HAMMER: Read-Path CLI Improvements🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 185, Remove the stale duplicate "## [Unreleased]" heading found between the [11.3.2] and [11.3.0] sections in CHANGELOG.md (the orphaned "## [Unreleased]" that predates this PR) so only the real top-of-file "## [Unreleased]" remains; delete that empty heading and any immediately associated blank lines to avoid duplicate/uninterpretable unreleased blocks for changelog parsers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-22: The Unreleased "Fixed" section in CHANGELOG.md is missing
entries for three behavioral fixes; add concise Fixed entries describing (1) the
applySyncResponse frontier correction referencing applySyncResponse and the
_lastFrontier / createFrontier() fallback so readers know the implementation
change, (2) the BitmapIndexReader behavior change referencing BitmapIndexReader
and the strict default now true (shard OID errors now throw by default), and (3)
the infinite delegation guard in syncWith referencing the syncWith method and
its new guard against infinite delegation; place these as bullet items under the
Unreleased Fixed heading mirroring style of other entries.
---
Duplicate comments:
In `@src/domain/services/BitmapIndexReader.js`:
- Line 94: Update the JSDoc in index.d.ts so the documented default for the
BitmapIndexReader constructor's strict parameter matches the implementation
(true); locate the JSDoc comment that reads "If true, throw on validation
failures; if false, log and return empty (default: false)" and change the
parenthetical default to "(default: true)" for the strict parameter described
for the BitmapIndexReader constructor.
In `@src/domain/services/JoinReducer.js`:
- Around line 344-358: The updateFrontierFromPatch function must guard against
null/undefined patch.context when calling vvDeserialize: ensure the call uses
vvDeserialize(patch.context || {}) (as currently present) so vvDeserialize
receives an empty object instead of null and avoids Object.entries(null) errors;
leave the use of vvMerge(state.observedFrontier, contextVV) and
foldPatchDot(state.observedFrontier, patch.writer, patch.lamport) unchanged and
verify existing tests (the ones around lines 781–811) cover both undefined and
null contexts.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 185: Remove the stale duplicate "## [Unreleased]" heading found between
the [11.3.2] and [11.3.0] sections in CHANGELOG.md (the orphaned "##
[Unreleased]" that predates this PR) so only the real top-of-file "##
[Unreleased]" remains; delete that empty heading and any immediately associated
blank lines to avoid duplicate/uninterpretable unreleased blocks for changelog
parsers.
In `@eslint.config.js`:
- Line 206: Remove the stale filename entry "src/domain/warp/sync.methods.js"
from the eslint config files array in eslint.config.js; locate the files array
that currently contains that string and delete that element (no replacement
needed since the file was replaced by src/domain/services/SyncController.js).
In `@src/domain/services/SyncController.js`:
- Around line 55-57: Remove the duplicated E_NO_STATE_MSG constant and import
the canonical symbol from the existing internal module instead: replace the
local const E_NO_STATE_MSG in SyncController.js with an import of E_NO_STATE_MSG
from the warp internal module that already exports it (ensure you use the
correct named/default import to match the original export in _internal.js), and
run tests to confirm no other references rely on the local definition.
- CHANGELOG: add missing Fixed entries for frontier correction, strict default flip, and syncWith delegation guard; remove orphaned ## [Unreleased] heading between 11.3.2 and 11.3.0 - eslint.config.js: replace stale sync.methods.js with SyncController.js in relaxed-complexity file list - SyncController.js: import E_NO_STATE_MSG from _internal.js instead of duplicating the constant - validateShardOid.js: document rationale for 4-char minimum OID length - syncWith.test.js: use vi.spyOn() instead of direct property assignment - syncAuth.test.js: add JSDoc explaining mockClientGraph/mockServerGraph mocking strategies
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/domain/services/SyncController.js (3)
455-467: Response validation runs after auto-materialize.Lines 455-458 trigger
materialize()before the response shape is validated at lines 460-467. If the server returns garbage, the graph materializes unnecessarily. Consider swapping the order so validation precedes the materialize call.Proposed reorder
- if (!this._host._cachedState) { - await this._host.materialize(); - emit('materialized'); - } - if (!response || typeof response !== 'object' || response.type !== 'sync-response' || !response.frontier || typeof response.frontier !== 'object' || Array.isArray(response.frontier) || !Array.isArray(response.patches)) { throw new SyncError('Invalid sync response', { code: 'E_SYNC_PROTOCOL', }); } + if (!this._host._cachedState) { + await this._host.materialize(); + emit('materialized'); + } + const result = this.applySyncResponse(response);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/SyncController.js` around lines 455 - 467, The response shape validation currently happens after auto-materialize causing unnecessary materialization on invalid responses; move the validation block that checks response, response.type === 'sync-response', response.frontier and Array.isArray(response.patches') to run before calling this._host.materialize() and emitting 'materialized', and only call await this._host.materialize() and emit('materialized') when the response passes validation; throw the existing SyncError with code 'E_SYNC_PROTOCOL' when validation fails.
269-292:applySyncResponsereturn type JSDoc omitsfrontier.The cast at line 277 shows the actual result includes
{state, frontier, applied}, but the@returnsat line 266 documents only{state, applied}. Sinceresult.frontieris used internally (line 283) and also leaks through the return at line 291, the documented return type should reflect this—especially if callers rely on it.Proposed fix
- * `@returns` {{state: import('./JoinReducer.js').WarpStateV5, applied: number}} Result with updated state + * `@returns` {{state: import('./JoinReducer.js').WarpStateV5, frontier: Map<string, string>, applied: number}} Result with updated state and updated frontier🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/SyncController.js` around lines 269 - 292, The JSDoc for applySyncResponse does not include the frontier field in its documented return type even though applySyncResponseImpl (cast at applySyncResponse) returns {state, frontier, applied} and frontier is used and returned; update the JSDoc `@returns` for the applySyncResponse function to include frontier: Map<string,string> (or the appropriate type alias) alongside state and applied so the documentation/types match the actual returned object and callers can rely on result.frontier.
186-200: Minor:status()staleness check using spread +everyis correct but allocation-heavy.Line 196 allocates a temporary array via
[...frontier]on every call. For a method documented as O(writers), this is fine at small scale. Ifstatus()becomes hot-path, consider iterating the Map directly:Alternative without allocation
- ![...frontier].every(([w, sha]) => /** `@type` {Map<string, string>} */ (this._host._lastFrontier).get(w) === sha)) { + !(() => { for (const [w, sha] of frontier) { if (/** `@type` {Map<string, string>} */ (this._host._lastFrontier).get(w) !== sha) return false; } return true; })()) {Or extract a small helper like
mapsEqual(a, b).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/SyncController.js` around lines 186 - 200, The staleness check in SyncController.status (the block using getFrontier and computing cachedState) currently uses [...frontier].every which allocates an array; replace that allocation by iterating the Map directly (for..of frontier.entries() or a small helper mapsEqual(frontier, this._host._lastFrontier)) and compare each entry via this._host._lastFrontier.get(writer) === sha; keep the existing size and null checks (this._host._lastFrontier, size comparison) and ensure the new iteration returns false on the first mismatch to preserve O(writers) behavior without extra allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 10-25: Move the three behavioral fixes and their tests out of the
[Unreleased] changelog section into the 11.5.1 release notes: relocate the
entries referencing applySyncResponse (frontier arg), syncWith (delegation guard
/ createSyncRequest/applySyncResponse usage), and BitmapIndexReader (strict
default change) plus the related tests (Frontier fix verification,
Auto-materialize/HTTP/serve tests listed under SyncController.test.js and
Null-context guard tests in JoinReducer.test.js) into the 11.5.1 section so the
changelog reflects that these changes ship with 11.5.1 rather than remaining
under Unreleased.
---
Nitpick comments:
In `@src/domain/services/SyncController.js`:
- Around line 455-467: The response shape validation currently happens after
auto-materialize causing unnecessary materialization on invalid responses; move
the validation block that checks response, response.type === 'sync-response',
response.frontier and Array.isArray(response.patches') to run before calling
this._host.materialize() and emitting 'materialized', and only call await
this._host.materialize() and emit('materialized') when the response passes
validation; throw the existing SyncError with code 'E_SYNC_PROTOCOL' when
validation fails.
- Around line 269-292: The JSDoc for applySyncResponse does not include the
frontier field in its documented return type even though applySyncResponseImpl
(cast at applySyncResponse) returns {state, frontier, applied} and frontier is
used and returned; update the JSDoc `@returns` for the applySyncResponse function
to include frontier: Map<string,string> (or the appropriate type alias)
alongside state and applied so the documentation/types match the actual returned
object and callers can rely on result.frontier.
- Around line 186-200: The staleness check in SyncController.status (the block
using getFrontier and computing cachedState) currently uses [...frontier].every
which allocates an array; replace that allocation by iterating the Map directly
(for..of frontier.entries() or a small helper mapsEqual(frontier,
this._host._lastFrontier)) and compare each entry via
this._host._lastFrontier.get(writer) === sha; keep the existing size and null
checks (this._host._lastFrontier, size comparison) and ensure the new iteration
returns false on the first mismatch to preserve O(writers) behavior without
extra allocation.
- SyncController.syncWith: validate response shape before calling materialize() to avoid unnecessary materialization on garbage responses - SyncController.applySyncResponse: fix @returns JSDoc to include frontier field that was already in the actual return value - SyncController.status: replace spread+every with direct Map iteration via mapsEqual() helper to avoid array allocation
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eslint.config.js (1)
206-219:⚠️ Potential issue | 🟡 Minor
SyncController.jsis listed twice in the samefilesarray.Lines 206 and 219 both add
"src/domain/services/SyncController.js"to the single relaxed-complexity config block (lines 199–257). This is a stale duplicate — ESLint silently deduplicates matched files so no rule is applied twice, but the redundant entry is misleading and should be removed.🧹 Proposed fix
"src/domain/warp/fork.methods.js", - "src/domain/services/SyncController.js", "src/domain/warp/checkpoint.methods.js", ... "src/domain/services/MessageSchemaDetector.js", "src/domain/services/SyncController.js",Keep only one of the two occurrences (the one at line 219 sits closer to the other
services/entries and reads more naturally).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 206 - 219, The files array in the relaxed-complexity ESLint config block contains a duplicate entry "src/domain/services/SyncController.js"; remove the redundant entry so the file only appears once in that files array (keep the occurrence grouped with the other services entries), ensuring the relaxed-complexity config block (the files array) no longer lists SyncController.js twice.
🧹 Nitpick comments (1)
src/domain/utils/validateShardOid.js (1)
11-12: Consider aligning the file name with the exported identifier.
validateShardOid.jsexportsisValidOid, creating a minor friction point forgrep/import discovery. Either rename the file toisValidOid.jsor rename the export tovalidateShardOid(predicate form:isValidShardOid) to make the two consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/utils/validateShardOid.js` around lines 11 - 12, The exported identifier isValidOid in validateShardOid.js is inconsistent with the file name; rename the export to match the file purpose (either change the function name to validateShardOid or to isValidShardOid) and update all imports accordingly. Locate the named export function isValidOid and change its identifier to the chosen name, update any import sites that reference isValidOid to the new identifier, and run tests/linters to confirm no unresolved references remain. Ensure the exported behavior and regex remain unchanged while only renaming the symbol for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@eslint.config.js`:
- Around line 206-219: The files array in the relaxed-complexity ESLint config
block contains a duplicate entry "src/domain/services/SyncController.js"; remove
the redundant entry so the file only appears once in that files array (keep the
occurrence grouped with the other services entries), ensuring the
relaxed-complexity config block (the files array) no longer lists
SyncController.js twice.
---
Nitpick comments:
In `@src/domain/utils/validateShardOid.js`:
- Around line 11-12: The exported identifier isValidOid in validateShardOid.js
is inconsistent with the file name; rename the export to match the file purpose
(either change the function name to validateShardOid or to isValidShardOid) and
update all imports accordingly. Locate the named export function isValidOid and
change its identifier to the chosen name, update any import sites that reference
isValidOid to the new identifier, and run tests/linters to confirm no unresolved
references remain. Ensure the exported behavior and regex remain unchanged while
only renaming the symbol for consistency.
- Remove duplicate SyncController.js entry in eslint relaxed-complexity config - Rename isValidOid → isValidShardOid to align export name with file name
|
✅ Both round-4 findings addressed in cfb4d2f:
All 3889 tests pass, lint clean, IRONCLAD gates green. @coderabbitai review please 🥕 |
Release Preflight
If you tag this commit as |
|
Great to hear all 3889 tests are passing and the IRONCLAD gates are green! I'll trigger a fresh review now. 🥕 ✅ Actions performedReview triggered.
|
1 participant
Summary
SyncControllerfrom WarpGraph (555 LOC → standalone service), wire 9 sync methods viadefinePropertydelegationJoinReducer.join()intoapplyFast/applyWithReceiptdual-path with sharedupdateFrontierFromPatch()helpervalidateShardOid()utility + flipBitmapIndexReaderstrict default totrue(fail-safe)applySyncResponse()was double-castingobservedFrontier(Lamport counters) as SHA frontier; now uses_lastFrontierwithcreateFrontier()fallbackTest plan
npm run test:local)applySyncResponse,syncNeeded,processSyncRequest,syncWithSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores