v12.0.0 — MaterializedView architecture + GraphTraversal engine#48
Open
flyingrobots wants to merge 71 commits intomainfrom
Open
v12.0.0 — MaterializedView architecture + GraphTraversal engine#48flyingrobots wants to merge 71 commits intomainfrom
flyingrobots wants to merge 71 commits intomainfrom
Conversation
Introduces the NeighborProviderPort abstraction and a single GraphTraversal engine that subsumes LogicalTraversal (in-memory) and DagTraversal/ DagPathFinding/DagTopology (bitmap commit DAG). One traversal stack, one set of determinism invariants, one set of bugs to fix. New files: - NeighborProviderPort: abstract interface (getNeighbors, hasNode, latencyClass) - AdjacencyNeighborProvider: in-memory, pre-sorted, sync latency - GraphTraversal: 11 algorithms (BFS, DFS, shortestPath, isReachable, Dijkstra, A*, bidirectional A*, topoSort, connectedComponent, commonAncestors, weightedLongestPath) - BitmapNeighborProvider: commit DAG stub wrapping BitmapIndexReader, label='' sentinel, async-local latency - MinHeap: optional tieBreaker comparator for deterministic PQ ordering LogicalTraversal deprecated — delegates to GraphTraversal internally, public API unchanged. 147 new tests, 3967 total passing.
Builds the test infrastructure for the MaterializedView overhaul:
1. Fixture DSL (test/helpers/fixtureDsl.js):
- makeFixture() validates edge endpoints, normalizes labels
- toAdjacencyMaps() and makeAdjacencyProvider() projection builders
- 12 canonical fixtures (F1-F12) with documented failure modes
- makeWeightFn() for weighted algorithm tests
- Seeded PRNG helper (test/helpers/seededRng.js)
2. Contract tests (test/contract/NeighborProviderPort.contract.test.js):
- 28 tests run against both AdjacencyNeighborProvider and
mock BitmapNeighborProvider
- Enforces sorting, direction, label filtering, dedup,
unknown node behavior, and proto-pollution safety
3. Per-algorithm fixture tests:
- F1 BFS_LEVEL_SORT_TRAP: catches fake BFS determinism
- F4 DIJKSTRA_EQUAL_COST_PREDECESSOR: proves tie-break rule
- F5 ASTAR_TIE_BREAK: no EPSILON hacks
- F8 TOPO_CYCLE_3: cycle detection with witness
- F9 UNICODE_CODEPOINT_ORDER: no localeCompare
- F10 PROTO_POLLUTION: __proto__/constructor/toString safe
4. Security test (test/unit/security/protoPollution.test.js):
- Verifies Object.prototype unchanged after exercising
proto-like node IDs through all code paths
…perty indexes CBOR-based bitmap index over the logical graph: stable numeric IDs (shardByte<<24 | localId), append-only label registry, per-label Roaring bitmaps for O(1) filtered neighbor lookups, property index shards with LRU caching, and proto-safe serialization throughout. New modules: fnv1a, computeShardKey, canonicalCbor, ShardIdOverflowError, LogicalBitmapIndexBuilder, PropertyIndexBuilder/Reader, LogicalIndexBuildService. BitmapNeighborProvider gains dual-mode: commit DAG (indexReader) + logical graph (logicalIndex). Third provider added to contract test suite (44 assertions). 18 cross-provider equivalence tests confirm identical traversal results across Adjacency and LogicalBitmap providers for all 9 algorithms.
Source files (7 errors): - shardKey.js: move JSDoc adjacent to function declaration - LogicalBitmapIndexBuilder: correct _globalToNode type to Map<string, string> - PropertyIndexBuilder: cast Object.create(null) to Record<string, unknown> - PropertyIndexReader: cast runtime-composed IndexStoragePort for readBlob - LogicalIndexBuildService: cast codec.decode() return to Record Test files (139 errors): - Add @param JSDoc to untyped function parameters - Cast codec.decode() returns via /** @type {Record<string, *>} */ - Cast catch variables via /** @type {*} */ - Widen ({}).polluted accesses via Record<string, unknown> cast - Type let declarations and {} objects as Record<string, ...> - Remove unused hooks prop from aStarSearch call
Move _createLogicalIndexFromTree logic into src/domain/services/LogicalIndexReader.js so production code can hydrate bitmap indexes from serialized trees or OID-based storage. fixtureDsl.js now delegates to LogicalIndexReader. Phase 3 Commit 1/6.
…erialized views Composes LogicalIndexBuildService, LogicalIndexReader, and PropertyIndexReader into a single service with three entry points: build(state), persistIndexTree(), and loadFromOids(). In-memory PropertyReader uses path-as-OID trick for zero-copy shard access during build().
Schema 4 checkpoints embed the materialized view index as a Git subtree under `index/`. createV5() accepts optional indexTree param; loadCheckpoint() partitions `index/`-prefixed entries into indexShardOids for lazy hydration. Backward-compatible: schema 2/3 checkpoints return null indexShardOids.
_setMaterializedState now builds a LogicalIndex + PropertyReader and attaches a BitmapNeighborProvider to _materializedGraph. Index build is cached by stateHash and wrapped in try-catch for resilience. Checkpoints now embed the index tree (schema 4). Integration test validates the full write → materialize → index-available path.
Both methods now check the LogicalIndex/PropertyReader/BitmapNeighborProvider for O(1) lookups before falling through to the linear scan. Fast path is only used when the index confirms the node is alive, ensuring correctness when the cached state has been mutated outside the materialization flow.
…vider CasSeekCacheAdapter now stores/returns an optional indexTreeOid alongside the state buffer. _materializeWithCeiling persists the bitmap index to Git and records the tree OID in cache metadata; on cache hit, it restores the LogicalIndex + PropertyReader from the stored tree. ObserverView uses the parent graph's BitmapNeighborProvider for O(1) adjacency lookups with post-filter glob matching on visible nodes.
Add applyWithDiff() alongside applyFast/applyWithReceipt — captures
alive-ness transitions (not just op application) during patch reduce.
Extend reduceV5() with { trackDiff: true } option returning merged
PatchDiff across all applied patches.
PatchDiff shape: nodesAdded, nodesRemoved, edgesAdded, edgesRemoved,
propsChanged — only records state transitions, not redundant ops.
TemporalQuery constructor now accepts optional loadCheckpoint callback. When since > 0 and a checkpoint exists with maxLamport <= since, replay starts from the checkpoint state instead of genesis — O(P_since_checkpoint) instead of O(P_total). Graceful fallback when loadCheckpoint is absent or returns null. WarpGraph.temporal getter wires loadCheckpoint via _loadLatestCheckpoint.
Stateless service computes only dirty shard buffers from a PatchDiff: - NodeAdd: allocate/reactivate globalId + alive bitmap toggle - NodeRemove: clear alive bit (stable globalId preserved) - EdgeAdd: fwd/rev bitmap updates + label registry extension - EdgeRemove: per-label removal + 'all' bitmap recomputation via OR - PropSet: props shard update Wire applyDiff() into MaterializedViewService as the incremental counterpart to build(). Add IncrementalIndexUpdater to ESLint relaxed-complexity block.
When _cachedIndexTree exists (prior successful build), reduceV5 uses trackDiff to capture PatchDiff, which flows through _setMaterializedState → _buildView → MaterializedViewService.applyDiff for O(diff) index updates instead of O(state_size) full rebuilds. Falls back to full build() when no prior tree or on any error. Add _cachedIndexTree field to WarpGraph for tree caching.
…ndex IncrementalIndexUpdater now correctly filters edges by endpoint alive-ness, matching the full rebuild's edgeVisibleV5 semantics. Adds _purgeNodeEdges for dangling edge elimination on node removal and edge restoration for re-added nodes. MaterializedViewService gains verifyIndex() — sampled cross-provider verification comparing bitmap index neighbors against adjacency ground truth with deterministic seeded PRNG. Includes 42 seeded fuzz equivalence tests (full rebuild vs incremental) and 5 verifyIndex unit tests.
- verify-index: samples alive nodes and cross-checks bitmap index neighbors against adjacency ground truth (--seed, --sample-rate) - reindex: forces full bitmap index rebuild by clearing cached state - Registered in COMMANDS map, KNOWN_COMMANDS, help text, and Zod schemas - Updated API surface snapshot for _maxLamportFromState
…H1, H2) H1: _handleNodeAdd now throws ShardIdOverflowError when nextLocalId >= 2^24, matching the full-build path and preventing silent globalId collisions. H2: _handleEdgeRemove now returns early when the edge label is not in the label registry, preventing operations on a "undefined" bucket key.
_biAStarExpand now calls weightFn(neighborId, current, label) when directionForNeighbors === 'in', correctly reflecting the actual edge direction. Asymmetric weight functions now produce correct shortest paths.
…lements (H4) snapshotBeforeOp now captures which elements are alive before NodeRemove/ EdgeRemove ops. collectNodeRemovals and collectEdgeRemovals only record transitions from alive → dead, preventing redundant removes from producing spurious diff entries that caused wasted incremental index work.
materialize() now accepts schema:4 checkpoints (previously only 2/3). When indexShardOids are present, the bitmap index is hydrated from stored OIDs via _viewService.loadFromOids, avoiding a full rebuild.
…int cache (H6, H7, H8) H6: Added verifyIndex() and invalidateIndex() public methods to WarpGraph. CLI verify-index and reindex commands now use public API instead of accessing private underscore-prefixed properties. Both include proper try/catch. H7: _buildView catch block now calls this._logger?.warn() with error details before nulling index state, so operators know when the index is degraded. H8: createCheckpoint() reuses _cachedIndexTree from the preceding materialize() instead of unconditionally calling _viewService.build(), avoiding O(N) rebuild.
…10, M19, M20) M1: CheckpointService tree entry sorting uses codepoint comparison instead of localeCompare, matching Git's byte-order requirement. M4/M5: Already resolved in H6 commit (CLI try/catch). M10: BitmapNeighborProvider constructor throws when neither indexReader nor logicalIndex is provided. M19/M20: fixtureDsl NodeRemove/EdgeRemove ops now include all required fields (node, from, to, label) matching accumulateOpDiff contract.
M2: Document O(E) worst-case in IncrementalIndexUpdater re-add scan M3: mergeDiffs now cancels contradictory add/remove pairs (net diff) M6: Document maxLamport closed-tick invariant on TemporalQuery M7: topologicalSort two-pointer queue (O(n) vs O(n^2) shift) M8: _insertSorted O(n+k) merge (vs O(k*n) splice) M9: sortAdjacencyMap clones arrays instead of in-place mutation M11: Document ObserverView cast-safety contracts for LogicalTraversal/QueryBuilder M12: BFS nextLevel dedup via queued Set (avoids O(E) duplicates) M14: JSDoc @note on reduceV5 diff-relative-to-initialState invariant M17: Add missing @param indexTree to CheckpointService.create() JSDoc M21: Deduplicate Mulberry32 PRNG — equivalence test imports seededRng helper M22: Exercise propertyReader with actual getNodeProps assertions L4: Bound verifyIndex seed to 32-bit int range in Zod schema L5: Reject --sample-rate 0 with .gt(0) validation L6: Rename verify-index total → totalChecks for clarity L15: MinHeap this.heap → this._heap (encapsulation) N2: Fix --sample-rate help placeholder style N3: Fix checkpoint.methods.js step numbering (4,5,5,6 → 4,5,6,7) N4: Add comment explaining empty reindexSchema
MEDIUM:
M13: JoinReducer snapshotBeforeOp uses targeted dot→element reverse index
instead of O(N) full entries scan; normalize observedDots to Set
M15: weightedLongestPath reuses adjList from topologicalSort via _returnAdjList
M16: Document commonAncestors self-inclusion behavior in JSDoc
M18: Add _validateStart() to bfs/dfs/shortestPath/A*/biA*/longestPath
LOW:
L1: IncrementalIndexUpdater O(1) lookups via globalToNode/nodeToGlobalMap
L2: _loadLabels prototype pollution guard via Object.create(null)
L3: Document stale receipt in MaterializedViewService.applyDiff
L7: ObserverView matchGlob regex cache (module-level Map)
L8: Document CasSeekCacheAdapter double-read trade-off
L9: Remove redundant await Promise.resolve() wrappers, drop async keyword
L10: Document error handling convention (throw vs return) in class JSDoc
L11: Document bidirectionalAStar fixed out/in direction
L12: Document LogicalTraversal private _materializeGraph access
L13: Document fnv1a NFC normalization note
L14: Tighten shardKey regex to exact SHA-1 (40) or SHA-256 (64)
L16: Fix IncrementalIndexUpdater.test edge remove state/diff mismatch
L17: Fix IncrementalIndexUpdater.test reactivation stale state
L18: Fix equivalence.test observedDots Array → Set
NIT:
N1: Remove identity .map() in _saveProps
N5: Remove redundant dedup Set in commonAncestors
N6: Document canonicalCbor convention (cbor-x, RFC 7049 §3.9)
N7: Verify ShardIdOverflowError options.code forwarding
N8: Replace tautological assertion in verifyIndex.test
N9: Add curly braces in GraphTraversal.test helper
N10: Rename misleading "isolated node" test
N11: Add consistent type annotation in CheckpointService.test
N12: Replace silent early return with assertion in equivalence.test
…thods Eliminate implicit `any` types and tighten JSDoc for strict TypeScript checking: add SnapshotBeforeOp typedef, type op params as OpV2, wrap sync returns in Promise.resolve for AdjacencyNeighborProvider, annotate codec decode results, add missing NeighborEdge[] types, fix dangling JSDoc block ordering in query.methods, and add null guards for optional _materializedGraph and _seekCache accesses.
- Fix WarpTypesV2 observedDots type from Dot[] to string[] (matches runtime: encoded dot strings, not Dot objects) - Remove stale Dot[] double-casts in PatchBuilderV2 - Add JSDoc type annotations to all test helper functions - Add null/undefined guards for possibly-null map lookups - Type shardOids as Record<string, string> across index tests - Cast reduceV5 return to WarpStateV5 when called without options - Cast unknown err in catch blocks, mock contexts in buildView test - Convert Set<string> to [...spread] for observedDots in equivalence test
…acade isReachable, weightedShortestPath, aStarSearch, bidirectionalAStar, topologicalSort, commonAncestors, and weightedLongestPath are now accessible via the public graph.traverse.* API. Previously these required constructing GraphTraversal + NeighborProvider directly. Also fixes AdjacencyNeighborProvider isolated-node visibility by passing aliveNodes from the materialized OR-Set.
…1-L3, N1) Remove phantom maxDepth from weightedShortestPath/aStarSearch, fix commonAncestors error message, forward signal on 4 methods, bypass assertDirection in bidirectionalAStar, add 6 negative/edge-case tests, fix CHANGELOG formatting, correct stale test comment, and use error code assertion instead of message match.
Support node-weighted graphs (e.g. critical-path scheduling) via a nodeWeightFn(nodeId) => number parameter on weightedShortestPath, aStarSearch, bidirectionalAStar, and weightedLongestPath. Weight is the cost to enter the destination node; start node is free. Internally memoized (O(V_visited) calls). Mutually exclusive with weightFn — providing both throws E_WEIGHT_FN_CONFLICT.
Release Preflight
If you tag this commit as |
Member
Author
|
Addressed the latest unresolved review threads in commits a10d307 and 24a9086.
Validation run:
@coderabbitai review please 🥕 |
|
Your fixes look comprehensive and well-validated: ✅ PropertyIndexReader hardening — fail-fast on malformed shard payloads prevents silent corruption I'll review the changes now. 🥕 ✅ Actions performedReview triggered.
|
|
Your fixes look comprehensive: ✅ PropertyIndexReader hardening — fail-fast on malformed shard payloads with regression coverage I'll review the changes now. ✅ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ARCHITECTURE.md (1)
260-308:⚠️ Potential issue | 🟡 MinorAlign NeighborProviderPort method signature and return type in docs.
Lines 260 and 307 still document
labelFilterand node-ID return values, but current providers usegetNeighbors(nodeId, direction, options)and returnNeighborEdge[]({ neighborId, label }).Suggested doc patch
-- `getNeighbors(nodeId, direction, labelFilter)` — returns neighbor node IDs +- `getNeighbors(nodeId, direction, options)` — returns `NeighborEdge[]` (`{ neighborId, label }`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ARCHITECTURE.md` around lines 260 - 308, Update the NeighborProviderPort docs to match the actual API: change the getNeighbors signature from getNeighbors(nodeId, direction, labelFilter) to getNeighbors(nodeId, direction, options) and document that it returns an array of NeighborEdge objects (each with neighborId and label) rather than plain node IDs; also update the implementations' descriptions for AdjacencyNeighborProvider and BitmapNeighborProvider to reference getNeighbors(nodeId, direction, options) and NeighborEdge return type so the docs align with the code.
♻️ Duplicate comments (6)
ROADMAP.md (1)
25-31:⚠️ Potential issue | 🟡 MinorVersion sequence is still non-monotonic in completed milestones.
After Line 26 (
v11.1.0), Lines 27–31 revert tov11.0.0, which breaks chronological ordering in this completed-milestones table. Please normalize these to the intended release sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` around lines 25 - 31, The completed-milestones table has non-monotonic version numbers: after "FOUNDATION LIFT" (v11.1.0) the subsequent rows "GHOST PROTOCOL", "VERIFY OR IT DIDN'T HAPPEN", "CLI DECOMPOSITION", "SAFE ERGONOMICS", and "TRUST V1" are listed as v11.0.0 which breaks chronological ordering; update those milestone version fields (for M3 GHOST PROTOCOL, M4 VERIFY OR IT DIDN'T HAPPEN, M5 CLI DECOMPOSITION, M6 SAFE ERGONOMICS, M7 TRUST V1) to the correct subsequent versions in sequence (e.g., increment or set to v11.2.0, v11.3.0, etc., or the intended release numbers) so the table is monotonic and reflects the intended release order.src/domain/services/PropertyIndexReader.js (1)
97-105:⚠️ Potential issue | 🟠 MajorFail fast on invalid shard payload shape instead of silently caching empty data.
Line 100 currently accepts only array-form data but silently treats other decoded shapes as empty, which can hide corruption and drop properties without signal.
Suggested fix
const decoded = this._codec.decode(buffer); // Shards are stored as array of [nodeId, props] pairs (proto-safe) /** `@type` {Record<string, Record<string, unknown>>} */ const data = Object.create(null); if (Array.isArray(decoded)) { for (const [nid, props] of /** `@type` {Array<[string, Record<string, unknown>]>} */ (decoded)) { data[nid] = props; } + } else if (decoded && typeof decoded === 'object') { + // Backward compatibility for legacy object-form shards + for (const [nid, props] of Object.entries(/** `@type` {Record<string, Record<string, unknown>>} */ (decoded))) { + data[nid] = props; + } + } else { + throw new Error(`PropertyIndexReader: invalid shard format for '${path}'`); } this._cache.set(path, data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/PropertyIndexReader.js` around lines 97 - 105, The code currently allows non-array decoded shard payloads and then silently caches an empty object; change this to fail fast: in the method that uses decoded (the block assigning const data = Object.create(null); and iterating decoded), validate that decoded is an Array and that each entry is a [nid, props] tuple (nid is a string and props is an object); if validation fails, throw a descriptive Error (including path and typeof decoded) and do not call this._cache.set(path, data). This ensures invalid shard shapes are rejected instead of being silently cached.src/domain/services/TemporalQuery.js (1)
311-320:⚠️ Potential issue | 🟠 MajorClone checkpoint state before replay to avoid cross-query mutation bleed.
Line 319 returns
ck.stateby reference. Since replay mutatesstate, reused checkpoint objects can contaminate subsequent temporal queries.Suggested fix
if (ck && ck.state && ck.maxLamport <= since) { const idx = allPatches.findIndex( ({ patch }) => patch.lamport > ck.maxLamport, ); const startIdx = idx < 0 ? allPatches.length : idx; - return { state: ck.state, startIdx, checkpointMaxLamport: ck.maxLamport }; + const clonedState = structuredClone(ck.state); + return { state: clonedState, startIdx, checkpointMaxLamport: ck.maxLamport }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/TemporalQuery.js` around lines 311 - 320, _resolveStart returns ck.state by reference which allows the replay to mutate shared checkpoint objects; update _resolveStart to return a deep clone of ck.state (not the original) before returning in the object so each query gets an independent WarpStateV5. Locate _resolveStart and where it returns { state: ck.state, ... } and replace that with a deep-copy of ck.state (use structuredClone if available in the runtime, or a project-standard deepClone helper or JSON.parse(JSON.stringify(...)) fallback) so subsequent replays cannot contaminate the original checkpoint.ARCHITECTURE.md (1)
254-255:⚠️ Potential issue | 🟡 MinorUpdate MaterializedViewService lifecycle names to current API surface.
Line 254 uses legacy lifecycle names (
persist/load/update) and should be aligned with the implemented methods (persistIndexTree,loadFromOids,applyDiff,verifyIndex) to avoid migration confusion.Suggested doc patch
-- Lifecycle: `build()` → `persist()` → `load()`, with incremental `update()` for hot paths +- Lifecycle: `build()` → `persistIndexTree()` → `loadFromOids()`, with incremental `applyDiff()` for hot paths; integrity checks via `verifyIndex()`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ARCHITECTURE.md` around lines 254 - 255, Update the lifecycle names in the Architecture doc to match MaterializedViewService's current API: replace the legacy sequence "build() → persist() → load(), with incremental update() for hot paths" with the implemented methods, e.g. "build() → persistIndexTree() → loadFromOids(), with incremental applyDiff() for hot paths and verifyIndex() for validation"; ensure the doc references MaterializedViewService and the exact method names persistIndexTree, loadFromOids, applyDiff, and verifyIndex so readers can map to the code.src/domain/services/GraphTraversal.js (1)
388-391:⚠️ Potential issue | 🟠 MajorValidate
startbefore thestart === goalfast path inisReachable.Line 388 currently skips
_validateStart(start), so a non-existent node can incorrectly returnreachable: truewhenstart === goal.Suggested fix
}) { this._resetStats(); + await this._validateStart(start); if (start === goal) { return { reachable: true, stats: this._stats(1) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/GraphTraversal.js` around lines 388 - 391, In isReachable, call this._validateStart(start) before the fast-path equality check; currently the code calls this._resetStats() then compares start === goal and can return reachable: true for an invalid start node. Move or add a this._validateStart(start) invocation ahead of the start === goal branch (preserving the existing this._resetStats() and use of this._stats(1)) so invalid starts throw/handle validation before returning the fast-path result.test/unit/domain/services/GraphTraversal.test.js (1)
621-626: Mock async provider still forwards only 2 of 3getNeighborsarguments.Line 623 passes
(nodeId, opts)butAdjacencyNeighborProvider.getNeighborsexpects(nodeId, direction, options). Hereoptscapturesdirectionandoptionsis dropped. This doesn't break the current cache-hit/miss test (BFS without label filters), but the mock is misleading and could silently mask bugs if this test is extended to cover label-filtered async paths.Suggested fix
- getNeighbors: (/** `@type` {string} */ nodeId, /** `@type` {*} */ opts) => inner.getNeighbors(nodeId, opts), + getNeighbors: (/** `@type` {string} */ nodeId, /** `@type` {*} */ direction, /** `@type` {*} */ options) => inner.getNeighbors(nodeId, direction, options),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/GraphTraversal.test.js` around lines 621 - 626, The mock asyncProvider.getNeighbors is forwarding only (nodeId, opts) but AdjacencyNeighborProvider.getNeighbors requires (nodeId, direction, options), so update the mock in GraphTraversal.test.js to forward all three parameters by calling inner.getNeighbors(nodeId, direction, options) (preserving parameter names used in the test) so the mock accurately mirrors AdjacencyNeighborProvider.getNeighbors; keep the other properties (like latencyClass) unchanged.
🧹 Nitpick comments (10)
docker/Dockerfile.deno (1)
5-5: Bump the base image to Deno 2.7.0, the current latest release.
denoland/deno:2.1.9is significantly outdated. Version 2.7.0 (released 2026-02-25) includes substantial improvements to npm dependency resolution, peer dependency handling,bundleDependenciessupport, and Node.js API compatibility between 2.1.9 and 2.7.x. While pinning to a specific version is fine for reproducibility, this pin is worth refreshing to benefit from these improvements and reduce dependency on backports for npm tooling fixes.Note: While the codebase uses
roaring(a native addon), improvements in npm/Node.js compatibility do not specifically address native addon support. Native addon compatibility depends on Node-API (N-API) availability in the Deno version, which is beyond npm-layer improvements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile.deno` at line 5, Update the pinned Deno base image from "denoland/deno:2.1.9" to "denoland/deno:2.7.0" in the Dockerfile entry that currently uses the literal image string "denoland/deno:2.1.9"; replace that tag so the FROM instruction pulls deno:2.7.0 to get the npm/Node API and dependency resolution fixes while keeping the image pin for reproducibility.src/domain/services/IncrementalIndexUpdater.js (2)
53-57:_collectDirtyShardKeysresult is unused beyond the empty-check.The collected
Set<string>is only tested for.size === 0(line 55). The shard keys themselves aren't used to scope processing. This is functionally correct (empty dirty keys ⟺ empty diff), but a reader might expect the set to drive per-shard filtering.Consider renaming to
_isDiffEmpty(diff)returning a boolean, or adding a brief comment clarifying the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/IncrementalIndexUpdater.js` around lines 53 - 57, The code calls _collectDirtyShardKeys(diff) only to test if the returned Set is empty, but the keys are never used; update computeDirtyShards to make intent explicit by either renaming _collectDirtyShardKeys to _isDiffEmpty and return a boolean (and use that boolean here), or keep the Set-returning helper and add a one-line comment above its use in computeDirtyShards clarifying that the Set is only used for an emptiness check and not for per-shard processing; adjust the helper name/signature (_collectDirtyShardKeys or _isDiffEmpty) and any callers accordingly to keep intent clear.
240-319:_purgeNodeEdgesis correct but consider extracting the symmetric forward/reverse cleanup.The forward (lines 252-285) and reverse (lines 287-318) purge blocks are nearly mirror images. A shared helper like
_purgeDirection(deadGid, shardKey, primaryCache, peerCache, primaryDir, peerDir, loadShard)could reduce the ~70 lines of near-duplicate logic.Not blocking — the current code is clear and well-commented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/IncrementalIndexUpdater.js` around lines 240 - 319, The forward and reverse cleanup blocks in _purgeNodeEdges are duplicated; extract the shared logic into a new helper _purgeDirection(deadGid, shardKey, primaryCache, peerCache, primaryDir, peerDir, loadShard) that encapsulates: loading the primary shard via _getOrLoadEdgeShard, deserializing the bitmap with getRoaringBitmap32(), iterating buckets, collecting peers, zeroing the primary bitmap, and removing deadGid from each peer bitmap by resolving peer node ids via _findNodeIdByGlobal and loading peer shards with _getOrLoadEdgeShard; then replace the two mirrored blocks in _purgeNodeEdges with two calls to _purgeDirection (one for forward->reverse and one for reverse->forward) passing appropriate cache/dir arguments and computeShardKey where needed.src/domain/services/MaterializedViewService.js (1)
345-378:passedcounter is per-direction-check, not per-node — consider documenting this.A node that passes both
outandinchecks incrementspassedby 2, while a node that fails theisAlivecheck produces 1 error and 0passed. This meanspassed + faileddoesn't equalsampled.length * 2when alive-bit mismatches occur. Not a bug, but the asymmetry could confuse consumers (e.g., CLI verify-index output).A brief JSDoc note on
VerifyResult.passed(e.g., "count of successful direction checks, not nodes") would clarify the semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/MaterializedViewService.js` around lines 345 - 378, The verifyIndex function's returned VerifyResult.passed counts successful direction checks (each node can contribute up to 2: 'out' and 'in') rather than successful nodes; update the docs to make this explicit by adding a JSDoc note for VerifyResult.passed (or the verifyIndex return type) clarifying "count of successful direction checks (each node may increment this up to 2 for 'out' and 'in')", and mention the asymmetry when a node fails the isAlive check; reference verifyIndex, VerifyResult.passed, compareNodeDirection, sampleNodes and state.nodeAlive so reviewers can locate the spot to add the comment.index.d.ts (1)
264-297: Optional: extract shared weighted-options shape to reduce declaration drift.
dir,labelFilter, andsignalare repeated across multiple weighted method signatures; a shared type alias would make future updates safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.d.ts` around lines 264 - 297, Extract the repeated option fields into a shared alias and use it in the weighted graph method signatures: create a type (e.g. WeightedTraversalOptions) that includes dir?: 'out' | 'in' | 'both'; labelFilter?: string | string[]; signal?: AbortSignal and then update weightedShortestPath, aStarSearch, bidirectionalAStar, and weightedLongestPath to accept options?: WeightedCostSelector & WeightedTraversalOptions (and for aStarSearch/bidirectionalAStar extend that alias with their heuristic fields like heuristicFn/forwardHeuristic/backwardHeuristic as needed).test/unit/domain/services/GraphTraversal.astar.test.js (1)
19-45: Consolidate duplicate tie-break assertions in F5 A tests.*The two test cases currently validate the same expected path and cost. Consider merging them and using the freed slot for a distinct assertion (e.g., explored count or hook-observed expansion order).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/GraphTraversal.astar.test.js` around lines 19 - 45, Tests 'chooses B before C on equal f-score (B < C)' and 'path is S→B→G (lex tie-break)' duplicate the same assertions; merge them into a single test that calls GraphTraversal.aStarSearch with makeAdjacencyProvider(F5_ASTAR_TIE_BREAK), weightFn makeWeightFn(F5_WEIGHTS) and heuristicFn ()=>0, assert result.path === ['S','B','G'] and result.totalCost === 11, then add a new distinct assertion using available hooks or provider instrumentation (e.g., check explored node count or the expansion order observed by the provider) to replace the removed duplicate; update or remove the second it() block accordingly to keep tests unique.test/integration/api/materializedView.test.js (1)
30-54: Reduce coupling to underscored internals in integration tests.These assertions are currently bound to private implementation fields (
_materializedGraph,_logicalIndex,_propertyReader). Prefer public API assertions (e.g., behavior throughgetNodes, traversal, or verify/reindex surface) so refactors don’t cause brittle test failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/api/materializedView.test.js` around lines 30 - 54, Tests currently assert private fields (_materializedGraph, _logicalIndex, _propertyReader); change them to use public APIs after calling graph.materialize(): verify nodes and edges via public accessors (e.g., call graph.getNodes() or graph.getNodeIds() to assert 'A' and 'B' exist and a nonexistent id is absent), verify edge presence by using the public neighbor/traversal API (e.g., graph.getNeighbors('A') or graph.traverse) to confirm an edge 'A'->'B' with label 'knows', and verify properties via the public property reader (e.g., graph.getProperty('A','name') === 'Alice'); remove assertions referencing _materializedGraph, _logicalIndex.isAlive, and _propertyReader and replace them with these behavior-driven checks that rely only on graph.materialize() and the graph's public methods.test/helpers/fixtureDsl.js (2)
462-466: Dynamic import ofGraphTraversalon every loop iteration is wasteful.
await import('../../src/domain/services/GraphTraversal.js')is re-evaluated for each provider. Move it outside the loop for a small efficiency gain.Suggested fix
export async function runCrossProvider({ fixture, providers, run, assert }) { const results = []; + const { default: GraphTraversal } = await import('../../src/domain/services/GraphTraversal.js'); for (const { name, provider } of providers) { - const { default: GraphTraversal } = await import('../../src/domain/services/GraphTraversal.js'); const engine = new GraphTraversal({ provider });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/fixtureDsl.js` around lines 462 - 466, The runCrossProvider function currently does a dynamic import of GraphTraversal inside the providers loop which repeatedly re-evaluates the module; move the dynamic import (await import('../../src/domain/services/GraphTraversal.js')) out of the for loop so you import once, e.g., obtain GraphTraversal before iterating providers, then inside the loop construct new GraphTraversal({ provider }) as before; update references to the imported default (GraphTraversal) accordingly in runCrossProvider to avoid repeated imports.
40-57:makeFixturestill does not validateprops[].nodeIdor tombstone targets.A past review flagged that
propsentries referencing non-existent nodes and tombstone targets for nodes/edges not in the fixture are silently accepted. This can cause tests to pass for the wrong reason when a fixture is malformed. Adding these guards would improve fixture reliability.Suggested guardrails
export function makeFixture({ nodes, edges, props = [], tombstones = {} }) { const nodeSet = new Set(nodes); const normalizedEdges = edges.map(({ from, to, label }) => ({ from, to, label: label ?? '', })); + const edgeKeySet = new Set( + normalizedEdges.map(({ from, to, label }) => `${from}\0${to}\0${label}`) + ); // Validate: all edge endpoints must be in nodes for (const { from, to } of normalizedEdges) { if (!nodeSet.has(from)) { throw new Error(`Edge from '${from}' — node not in fixture.nodes`); } if (!nodeSet.has(to)) { throw new Error(`Edge to '${to}' — node not in fixture.nodes`); } } + + for (const { nodeId } of props) { + if (!nodeSet.has(nodeId)) { + throw new Error(`Prop target '${nodeId}' — node not in fixture.nodes`); + } + } + + for (const nodeId of (tombstones.nodes ?? new Set())) { + if (!nodeSet.has(nodeId)) { + throw new Error(`Tombstoned node '${nodeId}' — node not in fixture.nodes`); + } + } + + for (const edgeKey of (tombstones.edges ?? new Set())) { + if (!edgeKeySet.has(edgeKey)) { + throw new Error(`Tombstoned edge '${edgeKey}' — edge not in fixture.edges`); + } + } return { nodes, edges: normalizedEdges, props, tombstones }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/fixtureDsl.js` around lines 40 - 57, makeFixture currently only validates edge endpoints; extend validation to reject props and tombstones that reference missing nodes or edges: iterate props and throw if any prop.nodeId is not in nodeSet; for tombstones, handle both node- and edge-style entries—if tombstones contains node ids (e.g. keys or a tombstones.nodes array) ensure each id is in nodeSet, and if tombstones contains edge entries (e.g. tombstones.edges array of {from,to,label}) ensure each edge exactly matches an entry in normalizedEdges (compare from, to and label) otherwise throw; use the existing symbols makeFixture, props, tombstones, nodeSet and normalizedEdges to locate and implement these checks.test/unit/domain/services/LogicalIndexReader.test.js (1)
20-48: Consider importing the canonicalfixtureToStatefromfixtureDsl.jsinstead of duplicating it.
test/helpers/fixtureDsl.jsalready exportsfixtureToState(line 553) which handles the same{ nodes, edges, props }shape plus tombstone support. This local copy duplicates the core logic. Since no tests here use tombstones, the canonical helper is a drop-in replacement.-import { - makeFixture, - F7_MULTILABEL_SAME_NEIGHBOR, - F10_PROTO_POLLUTION, -} from '../../../helpers/fixtureDsl.js'; +import { + makeFixture, + fixtureToState, + F7_MULTILABEL_SAME_NEIGHBOR, + F10_PROTO_POLLUTION, +} from '../../../helpers/fixtureDsl.js';Then remove the local
fixtureToState(lines 20–48). The call at line 290 (fixtureToState({ nodes, edges })) would need wrapping withmakeFixtureor passing a fixture-shaped object, but the existing usages withF7_MULTILABEL_SAME_NEIGHBOR/F10_PROTO_POLLUTIONwork as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/LogicalIndexReader.test.js` around lines 20 - 48, Replace the duplicated local fixtureToState implementation with the canonical export from test/helpers/fixtureDsl.js: import fixtureToState from that module, delete the local fixtureToState function, and update any callers to pass the same { nodes, edges, props } fixture-shaped object (or wrap with makeFixture if you need the helper behavior); specifically update tests that call fixtureToState with fixtures like F7_MULTILABEL_SAME_NEIGHBOR and F10_PROTO_POLLUTION to use the imported fixtureToState so tombstone support and canonical behavior are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ARCHITECTURE.md`:
- Around line 202-204: Update the GraphTraversal method descriptions so they
match the runtime API in GraphTraversal (notably functions in
src/domain/services/GraphTraversal.js): change `bfs()` / `dfs()` to indicate
they return arrays of nodes (not generators), change `shortestPath()` to
describe it as a BFS-based shortest-path (not bidirectional BFS), and ensure
`weightedShortestPath()` is described as Dijkstra with the existing `weightFn`
or `nodeWeightFn` options; mention the exact method names `bfs`, `dfs`,
`shortestPath`, and `weightedShortestPath` so readers can map to the
implementation.
In `@bin/cli/commands/verify-index.js`:
- Around line 31-35: The catch currently assumes the thrown value is an Error
and uses /** `@type` {Error} */ (err).message which can produce undefined for
non-Error throws; update the catch return to normalize the thrown value (e.g.,
const message = err && typeof err === 'object' && 'message' in err ?
String(err.message) : String(err);) and set payload.error to that message (still
returning exitCode: EXIT_CODES.INTERNAL). Locate the catch in verify-index.js
(the return object that sets payload.error) and replace the cast with the
normalization logic so non-Error throws yield a sensible string instead of
undefined.
In `@docker/Dockerfile.deno`:
- Around line 7-13: Remove the redundant system package by deleting "node-gyp"
from the apt install list in the RUN apt-get update && apt-get install -y ...
line so only the actual build prerequisites (python3, make, g++) are installed;
keep the rest of the RUN command (including git and the rm -rf
/var/lib/apt/lists/* cleanup) unchanged so npm continues to use its bundled
node-gyp from node_modules/.bin/.
In `@index.d.ts`:
- Around line 526-528: Update the SeekCachePort get/set buffer types to accept
Buffer | Uint8Array to match runtime values (change the return/object and set
parameter from `Buffer` to `Buffer | Uint8Array`), or alternatively perform an
explicit runtime conversion in CasSeekCacheAdapter.get() (convert the Uint8Array
from cas.restore() into a Buffer before returning) so the type declarations and
runtime behavior stay consistent; modify the signatures for abstract get and set
accordingly.
In `@ROADMAP.md`:
- Around line 3-5: Update the ROADMAP.md header to avoid stale branch metadata
by replacing or removing the branch-specific line "Current branch:
`fix-the-arch`": either change it to a stable value like "Current branch:
`main`" at merge time or remove the "Current branch" entry entirely so the
roadmap only contains stable fields (e.g., version and last reconciled); modify
the header block where "Current branch: `fix-the-arch`" appears to implement the
chosen approach.
In `@src/domain/services/LogicalIndexReader.js`:
- Around line 226-243: The meta.alive buffer passed to Ctor.deserialize in
_decodeMeta may be a plain Array after decoding; wrap it in Uint8Array.from(...)
before deserialization to match the defensive pattern used in
IncrementalIndexUpdater._loadMeta — i.e., replace the current
Ctor.deserialize(meta.alive.slice(), true) call in _decodeMeta with
Ctor.deserialize(Uint8Array.from(meta.alive.slice()), true) to ensure
RoaringBitmap32.deserialize always receives a Uint8Array.
---
Outside diff comments:
In `@ARCHITECTURE.md`:
- Around line 260-308: Update the NeighborProviderPort docs to match the actual
API: change the getNeighbors signature from getNeighbors(nodeId, direction,
labelFilter) to getNeighbors(nodeId, direction, options) and document that it
returns an array of NeighborEdge objects (each with neighborId and label) rather
than plain node IDs; also update the implementations' descriptions for
AdjacencyNeighborProvider and BitmapNeighborProvider to reference
getNeighbors(nodeId, direction, options) and NeighborEdge return type so the
docs align with the code.
---
Duplicate comments:
In `@ARCHITECTURE.md`:
- Around line 254-255: Update the lifecycle names in the Architecture doc to
match MaterializedViewService's current API: replace the legacy sequence
"build() → persist() → load(), with incremental update() for hot paths" with the
implemented methods, e.g. "build() → persistIndexTree() → loadFromOids(), with
incremental applyDiff() for hot paths and verifyIndex() for validation"; ensure
the doc references MaterializedViewService and the exact method names
persistIndexTree, loadFromOids, applyDiff, and verifyIndex so readers can map to
the code.
In `@ROADMAP.md`:
- Around line 25-31: The completed-milestones table has non-monotonic version
numbers: after "FOUNDATION LIFT" (v11.1.0) the subsequent rows "GHOST PROTOCOL",
"VERIFY OR IT DIDN'T HAPPEN", "CLI DECOMPOSITION", "SAFE ERGONOMICS", and "TRUST
V1" are listed as v11.0.0 which breaks chronological ordering; update those
milestone version fields (for M3 GHOST PROTOCOL, M4 VERIFY OR IT DIDN'T HAPPEN,
M5 CLI DECOMPOSITION, M6 SAFE ERGONOMICS, M7 TRUST V1) to the correct subsequent
versions in sequence (e.g., increment or set to v11.2.0, v11.3.0, etc., or the
intended release numbers) so the table is monotonic and reflects the intended
release order.
In `@src/domain/services/GraphTraversal.js`:
- Around line 388-391: In isReachable, call this._validateStart(start) before
the fast-path equality check; currently the code calls this._resetStats() then
compares start === goal and can return reachable: true for an invalid start
node. Move or add a this._validateStart(start) invocation ahead of the start ===
goal branch (preserving the existing this._resetStats() and use of
this._stats(1)) so invalid starts throw/handle validation before returning the
fast-path result.
In `@src/domain/services/PropertyIndexReader.js`:
- Around line 97-105: The code currently allows non-array decoded shard payloads
and then silently caches an empty object; change this to fail fast: in the
method that uses decoded (the block assigning const data = Object.create(null);
and iterating decoded), validate that decoded is an Array and that each entry is
a [nid, props] tuple (nid is a string and props is an object); if validation
fails, throw a descriptive Error (including path and typeof decoded) and do not
call this._cache.set(path, data). This ensures invalid shard shapes are rejected
instead of being silently cached.
In `@src/domain/services/TemporalQuery.js`:
- Around line 311-320: _resolveStart returns ck.state by reference which allows
the replay to mutate shared checkpoint objects; update _resolveStart to return a
deep clone of ck.state (not the original) before returning in the object so each
query gets an independent WarpStateV5. Locate _resolveStart and where it returns
{ state: ck.state, ... } and replace that with a deep-copy of ck.state (use
structuredClone if available in the runtime, or a project-standard deepClone
helper or JSON.parse(JSON.stringify(...)) fallback) so subsequent replays cannot
contaminate the original checkpoint.
In `@test/unit/domain/services/GraphTraversal.test.js`:
- Around line 621-626: The mock asyncProvider.getNeighbors is forwarding only
(nodeId, opts) but AdjacencyNeighborProvider.getNeighbors requires (nodeId,
direction, options), so update the mock in GraphTraversal.test.js to forward all
three parameters by calling inner.getNeighbors(nodeId, direction, options)
(preserving parameter names used in the test) so the mock accurately mirrors
AdjacencyNeighborProvider.getNeighbors; keep the other properties (like
latencyClass) unchanged.
---
Nitpick comments:
In `@docker/Dockerfile.deno`:
- Line 5: Update the pinned Deno base image from "denoland/deno:2.1.9" to
"denoland/deno:2.7.0" in the Dockerfile entry that currently uses the literal
image string "denoland/deno:2.1.9"; replace that tag so the FROM instruction
pulls deno:2.7.0 to get the npm/Node API and dependency resolution fixes while
keeping the image pin for reproducibility.
In `@index.d.ts`:
- Around line 264-297: Extract the repeated option fields into a shared alias
and use it in the weighted graph method signatures: create a type (e.g.
WeightedTraversalOptions) that includes dir?: 'out' | 'in' | 'both';
labelFilter?: string | string[]; signal?: AbortSignal and then update
weightedShortestPath, aStarSearch, bidirectionalAStar, and weightedLongestPath
to accept options?: WeightedCostSelector & WeightedTraversalOptions (and for
aStarSearch/bidirectionalAStar extend that alias with their heuristic fields
like heuristicFn/forwardHeuristic/backwardHeuristic as needed).
In `@src/domain/services/IncrementalIndexUpdater.js`:
- Around line 53-57: The code calls _collectDirtyShardKeys(diff) only to test if
the returned Set is empty, but the keys are never used; update
computeDirtyShards to make intent explicit by either renaming
_collectDirtyShardKeys to _isDiffEmpty and return a boolean (and use that
boolean here), or keep the Set-returning helper and add a one-line comment above
its use in computeDirtyShards clarifying that the Set is only used for an
emptiness check and not for per-shard processing; adjust the helper
name/signature (_collectDirtyShardKeys or _isDiffEmpty) and any callers
accordingly to keep intent clear.
- Around line 240-319: The forward and reverse cleanup blocks in _purgeNodeEdges
are duplicated; extract the shared logic into a new helper
_purgeDirection(deadGid, shardKey, primaryCache, peerCache, primaryDir, peerDir,
loadShard) that encapsulates: loading the primary shard via _getOrLoadEdgeShard,
deserializing the bitmap with getRoaringBitmap32(), iterating buckets,
collecting peers, zeroing the primary bitmap, and removing deadGid from each
peer bitmap by resolving peer node ids via _findNodeIdByGlobal and loading peer
shards with _getOrLoadEdgeShard; then replace the two mirrored blocks in
_purgeNodeEdges with two calls to _purgeDirection (one for forward->reverse and
one for reverse->forward) passing appropriate cache/dir arguments and
computeShardKey where needed.
In `@src/domain/services/MaterializedViewService.js`:
- Around line 345-378: The verifyIndex function's returned VerifyResult.passed
counts successful direction checks (each node can contribute up to 2: 'out' and
'in') rather than successful nodes; update the docs to make this explicit by
adding a JSDoc note for VerifyResult.passed (or the verifyIndex return type)
clarifying "count of successful direction checks (each node may increment this
up to 2 for 'out' and 'in')", and mention the asymmetry when a node fails the
isAlive check; reference verifyIndex, VerifyResult.passed, compareNodeDirection,
sampleNodes and state.nodeAlive so reviewers can locate the spot to add the
comment.
In `@test/helpers/fixtureDsl.js`:
- Around line 462-466: The runCrossProvider function currently does a dynamic
import of GraphTraversal inside the providers loop which repeatedly re-evaluates
the module; move the dynamic import (await
import('../../src/domain/services/GraphTraversal.js')) out of the for loop so
you import once, e.g., obtain GraphTraversal before iterating providers, then
inside the loop construct new GraphTraversal({ provider }) as before; update
references to the imported default (GraphTraversal) accordingly in
runCrossProvider to avoid repeated imports.
- Around line 40-57: makeFixture currently only validates edge endpoints; extend
validation to reject props and tombstones that reference missing nodes or edges:
iterate props and throw if any prop.nodeId is not in nodeSet; for tombstones,
handle both node- and edge-style entries—if tombstones contains node ids (e.g.
keys or a tombstones.nodes array) ensure each id is in nodeSet, and if
tombstones contains edge entries (e.g. tombstones.edges array of
{from,to,label}) ensure each edge exactly matches an entry in normalizedEdges
(compare from, to and label) otherwise throw; use the existing symbols
makeFixture, props, tombstones, nodeSet and normalizedEdges to locate and
implement these checks.
In `@test/integration/api/materializedView.test.js`:
- Around line 30-54: Tests currently assert private fields (_materializedGraph,
_logicalIndex, _propertyReader); change them to use public APIs after calling
graph.materialize(): verify nodes and edges via public accessors (e.g., call
graph.getNodes() or graph.getNodeIds() to assert 'A' and 'B' exist and a
nonexistent id is absent), verify edge presence by using the public
neighbor/traversal API (e.g., graph.getNeighbors('A') or graph.traverse) to
confirm an edge 'A'->'B' with label 'knows', and verify properties via the
public property reader (e.g., graph.getProperty('A','name') === 'Alice'); remove
assertions referencing _materializedGraph, _logicalIndex.isAlive, and
_propertyReader and replace them with these behavior-driven checks that rely
only on graph.materialize() and the graph's public methods.
In `@test/unit/domain/services/GraphTraversal.astar.test.js`:
- Around line 19-45: Tests 'chooses B before C on equal f-score (B < C)' and
'path is S→B→G (lex tie-break)' duplicate the same assertions; merge them into a
single test that calls GraphTraversal.aStarSearch with
makeAdjacencyProvider(F5_ASTAR_TIE_BREAK), weightFn makeWeightFn(F5_WEIGHTS) and
heuristicFn ()=>0, assert result.path === ['S','B','G'] and result.totalCost ===
11, then add a new distinct assertion using available hooks or provider
instrumentation (e.g., check explored node count or the expansion order observed
by the provider) to replace the removed duplicate; update or remove the second
it() block accordingly to keep tests unique.
In `@test/unit/domain/services/LogicalIndexReader.test.js`:
- Around line 20-48: Replace the duplicated local fixtureToState implementation
with the canonical export from test/helpers/fixtureDsl.js: import fixtureToState
from that module, delete the local fixtureToState function, and update any
callers to pass the same { nodes, edges, props } fixture-shaped object (or wrap
with makeFixture if you need the helper behavior); specifically update tests
that call fixtureToState with fixtures like F7_MULTILABEL_SAME_NEIGHBOR and
F10_PROTO_POLLUTION to use the imported fixtureToState so tombstone support and
canonical behavior are preserved.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (49)
ARCHITECTURE.mdCHANGELOG.mdROADMAP.mdTHE_PROBLEM.mdbin/cli/commands/verify-index.jsbin/cli/infrastructure.jsdocker-compose.test.ymldocker/Dockerfile.denodocker/Dockerfile.node20docker/Dockerfile.node22docs/CLI_GUIDE.mdindex.d.tspackage.jsonsrc/domain/services/AdjacencyNeighborProvider.jssrc/domain/services/CheckpointMessageCodec.jssrc/domain/services/CheckpointService.jssrc/domain/services/GraphTraversal.jssrc/domain/services/IncrementalIndexUpdater.jssrc/domain/services/LogicalBitmapIndexBuilder.jssrc/domain/services/LogicalIndexBuildService.jssrc/domain/services/LogicalIndexReader.jssrc/domain/services/MaterializedViewService.jssrc/domain/services/PropertyIndexReader.jssrc/domain/services/TemporalQuery.jssrc/domain/utils/MinHeap.jssrc/domain/utils/roaring.jssrc/domain/utils/shardKey.jssrc/domain/warp/checkpoint.methods.jssrc/domain/warp/query.methods.jstest/helpers/fixtureDsl.jstest/integration/api/materializedView.test.jstest/unit/cli/verify-index.test.jstest/unit/domain/WarpGraph.query.test.jstest/unit/domain/WarpGraph.test.jstest/unit/domain/services/AdjacencyNeighborProvider.test.jstest/unit/domain/services/GraphTraversal.astar.test.jstest/unit/domain/services/GraphTraversal.test.jstest/unit/domain/services/IncrementalIndexUpdater.test.jstest/unit/domain/services/LogicalBitmapIndexBuilder.stability.test.jstest/unit/domain/services/LogicalBitmapIndexBuilder.test.jstest/unit/domain/services/LogicalIndexBuildService.determinism.test.jstest/unit/domain/services/LogicalIndexBuildService.test.jstest/unit/domain/services/LogicalIndexReader.test.jstest/unit/domain/services/MaterializedViewService.verify.test.jstest/unit/domain/services/PropertyIndex.test.jstest/unit/domain/services/TemporalQuery.checkpoint.test.jstest/unit/domain/services/noBufferGlobal.test.jstest/unit/domain/warp/hydrateCheckpointIndex.regression.test.jstest/unit/helpers/fixtureDsl.test.js
🚧 Files skipped from review as they are similar to previous changes (9)
- test/unit/domain/services/MaterializedViewService.verify.test.js
- test/unit/domain/services/PropertyIndex.test.js
- src/domain/utils/shardKey.js
- test/unit/domain/services/LogicalBitmapIndexBuilder.test.js
- package.json
- test/unit/domain/services/LogicalIndexBuildService.test.js
- src/domain/services/AdjacencyNeighborProvider.js
- src/domain/utils/roaring.js
- bin/cli/infrastructure.js
Release Preflight
If you tag this commit as |
Member
Author
|
Addressed ENHANCE follow-ups and remaining unresolved review threads in commits a5b240d, 456fb5e, c97f28f, and 0da191b. What’s fixed:
Validation:
All remaining review threads are now resolved. @coderabbitai review please 🥕 |
|
Your fixes look comprehensive and well-validated: ✅ LogicalIndexReader hardening — byte payload normalization (Buffer/Uint8Array/number[]) before Roaring deserialize prevents runtime errors across environments I'll review the changes now. 🥕 ✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
Member
Author
|
Follow-up fix for CI regression:
Local validation rerun (all green):
|
Member
Author
|
@coderabbitai review please 🥕 |
|
Restoring I'll review the changes now. 🥕 ✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (4)
src/domain/services/GraphTraversal.js (1)
388-391:⚠️ Potential issue | 🟠 MajorValidate
startbefore the early-return path inisReachable.Line 389 can return
reachable: truefor a nonexistent node (start === goal) becausestartis never validated in this method.💡 Proposed fix
async isReachable({ start, goal, direction = 'out', options, maxNodes = DEFAULT_MAX_NODES, maxDepth = DEFAULT_MAX_DEPTH, signal, }) { this._resetStats(); + await this._validateStart(start); if (start === goal) { return { reachable: true, stats: this._stats(1) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/GraphTraversal.js` around lines 388 - 391, In isReachable, validate that the start node exists before the early-return (the block that calls this._resetStats() and checks start === goal); if start is missing return reachable: false (or throw) with appropriate stats from this._stats, and also ensure goal is validated similarly before proceeding. Update the early-return path inside isReachable to first check node existence (using the traversal/graph's node-existence helper) and only return reachable: true when both nodes exist and start === goal, keeping the existing calls to this._resetStats() and this._stats(1).src/domain/services/LogicalIndexBuildService.js (1)
88-97: LGTM —isEdgePropKeyguard correctly applied.The guard at Line 90 skips edge-property entries before
decodePropKey, which was the previously flagged issue. The fix is correctly in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/LogicalIndexBuildService.js` around lines 88 - 97, The loop in LogicalIndexBuildService previously decoded edge prop keys incorrectly; the current fix correctly uses the isEdgePropKey guard before calling decodePropKey, so no further changes are needed—keep the guard check in the for (const [propKey, register] of state.prop) loop and ensure decodePropKey(nodeId, propKey) is only invoked after isEdgePropKey(propKey) returns false (refer to functions isEdgePropKey, decodePropKey, and nodeVisibleV5 and the propBuilder.addProperty call).test/unit/domain/services/GraphTraversal.test.js (1)
621-626: Mock async provider still drops the thirdoptionsargument.Line 623 forwards only two arguments (
nodeId,opts) toinner.getNeighbors, whereoptsactually receives thedirectionargument. The thirdoptionsparameter (e.g., label filtering) is silently dropped. This doesn't affect the current cache-stats test, but it means label-filtered async traversals would silently misbehave if tested through this mock.Suggested fix
- getNeighbors: (/** `@type` {string} */ nodeId, /** `@type` {*} */ opts) => inner.getNeighbors(nodeId, opts), + getNeighbors: (/** `@type` {string} */ nodeId, /** `@type` {*} */ direction, /** `@type` {*} */ options) => inner.getNeighbors(nodeId, direction, options),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/GraphTraversal.test.js` around lines 621 - 626, The mock async provider's getNeighbors implementation drops the third options argument; update asyncProvider.getNeighbors to forward all received arguments to inner.getNeighbors (e.g., use the same param list or rest args) so that the original third parameter (label/filter options) is passed through; keep hasNode and latencyClass unchanged and ensure the symbol references are asyncProvider.getNeighbors and inner.getNeighbors.src/domain/services/MaterializedViewService.js (1)
345-378:⚠️ Potential issue | 🟡 Minor
verifyIndexstill only samples fromstate.nodeAlive— ghost/stale index nodes remain undetected.The verification loop (line 348) draws candidates exclusively from
state.nodeAlive. Nodes that are alive only inlogicalIndex(i.e., stale/ghost entries left behind after a missed removal) will never be sampled and never caught. The alive-bit check at line 357 only catches "state says alive, index says dead," not the reverse.Consider augmenting the verification with a small sample from
logicalIndex's alive set (if exposed) to detect ghost entries, or document this as a known limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/MaterializedViewService.js` around lines 345 - 378, verifyIndex currently samples only from state.nodeAlive so stale/ghost nodes present only in logicalIndex never get checked; update verifyIndex to also sample a small set from logicalIndex's alive set (use logicalIndex.isAlive over logicalIndex's node list or a hypothetical logicalIndex.getAliveNodes()) and merge those samples with the ones from state.nodeAlive (dedupe by nodeId), preserving the seed/sampleRate logic with sampleNodes; then run the same verification loop for the combined sample so ghost entries are detected and counted in errors/failed/passed.
🧹 Nitpick comments (15)
src/domain/utils/roaring.js (2)
108-121: Nit:initErroris not cleared on successful no-argumentinitRoaring()retry.If
initRoaring()is called without arguments and succeeds (e.g., a retry after the initial top-level failure),roaringModuleis set butinitErrorretains the stale error. Not a functional bug sinceloadRoaring()won't reach the error path, but clearing it keeps internal state consistent.♻️ Optional improvement
if (!roaringModule) { roaringModule = /** `@type` {RoaringModule} */ (await import('roaring')); // Handle both ESM default export and CJS module.exports if (roaringModule.default && roaringModule.default.RoaringBitmap32) { roaringModule = roaringModule.default; } + initError = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/utils/roaring.js` around lines 108 - 121, The function initRoaring leaves initError set when called with no arguments even after a successful import; update initRoaring so that in the branch where you import and assign roaringModule (and after handling roaringModule.default), you also set initError = null to clear any stale error (i.e., ensure initError is cleared whenever roaringModule is successfully assigned in initRoaring).
92-98: Minor: non-ErrorinitErrorvalues are silently omitted from the diagnostic message.If the dynamic
import('roaring')throws a non-Error value (rare but possible), line 94'sinstanceof Errorcheck will produce an emptycausestring, losing the diagnostic. A lightweight fallback would cover all cases:♻️ Optional improvement
- const cause = initError instanceof Error ? ` Caused by: ${initError.message}` : ''; + const cause = initError ? ` Caused by: ${initError instanceof Error ? initError.message : String(initError)}` : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/utils/roaring.js` around lines 92 - 98, The loadRoaring function omits non-Error initError values when building the cause string; update the cause construction in loadRoaring to include any initError by coercing it to a string (e.g., use initError instanceof Error ? initError.message : String(initError)) so that non-Error throwables are preserved in the diagnostic while still handling Error objects via message; keep referencing roaringModule and initError inside loadRoaring and include the resulting cause in the thrown Error message.test/unit/domain/warp/hydrateCheckpointIndex.regression.test.js (2)
69-73:indexShardOidsin the stale checkpoint is never read — but this is the core regression invariant; verify it stays inert.The checkpoint stub returns
indexShardOids: { 'meta_stale.cbor': 'oid_stale' }at Line 72, simulating a stale index in the checkpoint. The regression asserts thathydrateCheckpointIndexis never called (Line 106), meaning these OIDs are never loaded. This is the heart of the regression test and looks correct.However, if the production code ever gains a path that reads
indexShardOidsbefore building the fresh index (e.g., to skip a rebuild when shards are present), this test's_cachedIndexTree: nullguard (Line 76) may no longer be sufficient to block that path. A brief comment explaining why_cachedIndexTree: nullis intentional would help future maintainers.📝 Suggested comment
+ // _cachedIndexTree: null ensures the code rebuilds the index from state + // rather than taking any shortcut that would load stale checkpoint shard OIDs. _cachedIndexTree: null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/warp/hydrateCheckpointIndex.regression.test.js` around lines 69 - 73, Add a brief comment in the test near the checkpoint stub (where indexShardOids: { 'meta_stale.cbor': 'oid_stale' } is returned) explaining that _cachedIndexTree is explicitly set to null to ensure the production code does not read indexShardOids (the regression invariant) and that the test asserts hydrateCheckpointIndex is never called; reference hydrateCheckpointIndex, _cachedIndexTree and indexShardOids so future maintainers understand this guard is intentional and to update the test if code paths change to read indexShardOids earlier.
62-102: Context stub is fragile — any newthis.*call added tomaterialize()will silently fail with a confusing error.The test constructs a bare context object with manually-stubbed fields and calls
materialize.call(context). This approach tightly couples the test to the internal method surface ofmaterialize.methods.js. If a future change adds a call to an unstubbedthis.*method (e.g., VV updates, provenance updates), the test will throwTypeError: this.xxx is not a functionrather than a meaningful assertion failure.Consider documenting in a comment which fields are intentionally omitted vs. which are implicitly relied upon, or adding a
Proxy-based safety net that throws with a clear message for any unstubbed access:♻️ Optional: self-documenting stub with Proxy guard
+ // Proxy to catch any unstubbed this.* access with a clear error const context = { _clock: { now: () => 0 }, // ... existing stubs }; + const guardedContext = new Proxy(context, { + get(target, prop) { + if (!(prop in target)) { + throw new Error(`Regression test: unstubbed context access: ${String(prop)}`); + } + return target[prop]; + }, + }); - await /** `@type` {any} */ (materialize).call(context); + await /** `@type` {any} */ (materialize).call(guardedContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/warp/hydrateCheckpointIndex.regression.test.js` around lines 62 - 102, The test's manual context stub is fragile: when calling materialize.call(context) an unstubbed this.* will cause a TypeError rather than a clear test failure; wrap the context in a Proxy (or add explicit checks) so any read or invocation of an unstubbed property throws a descriptive error (e.g., "test stub missing this.<name> required by materialize") and update the test to use that proxiedContext in place of context; reference the existing hydrateCheckpointIndex method and materialize.call(context) in the change and add a short comment listing which fields are intentionally omitted vs. required by materialize.methods.js to prevent silent breakage.test/unit/domain/WarpGraph.test.js (1)
21-38: LocalcreateMockPersistenceis missingnodeExistsandisAncestorpresent in the shared helper.
test/helpers/warpGraphTestUtils.jsexportscreateMockPersistencewithnodeExistsandisAncestormocks; the local version omits them. This is fine as long as no test in this file exercises code paths that call those methods — but it means the local mock will silently returnundefined(instead of a sensible default) if a code path does reach them. Consider either importing the shared helper or keeping parity so future tests don't hit unexpectedundefinedreturns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/WarpGraph.test.js` around lines 21 - 38, The local createMockPersistence mock in WarpGraph.test.js is missing the nodeExists and isAncestor functions; update the mock used in this file by either importing the shared createMockPersistence from test/helpers/warpGraphTestUtils.js or add mocked functions named nodeExists and isAncestor (e.g., vi.fn().mockResolvedValue(false) or appropriate defaults) to the returned object so calls to createMockPersistence().nodeExists and .isAncestor return sensible values instead of undefined.test/unit/domain/services/LogicalBitmapIndexBuilder.stability.test.js (2)
41-41:path.slice(5, 7)to extract shard key is fragile.This assumes the filename format is exactly
meta_XX.cborwhereXXstarts at index 5. If the prefix ever changes (e.g.,metaindex_XX.cbor), the extraction silently produces the wrong key with no error. A more robust extraction:♻️ Proposed fix
- const shardKey = path.slice(5, 7); + const shardKey = path.slice('meta_'.length, 'meta_'.length + 2);Or use a regex:
/^meta_([0-9a-f]{2})\.cbor$/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/LogicalBitmapIndexBuilder.stability.test.js` at line 41, Replace the fragile substring extraction that computes shardKey from path using path.slice(5, 7) with a robust pattern match: use a regular expression (e.g. one that matches filenames like "meta_XX.cbor" and captures the two-hex-digit shard) to extract the shard into the shardKey variable, and add an explicit assert/throw if the filename does not match so the test fails loudly instead of producing an incorrect key; update the code that references path and shardKey in the LogicalBitmapIndexBuilder.stability.test.js test accordingly.
70-88: DoubleregisterNodecall in the overflow test — firstexpect(...).toThrow()is sufficient; second call insidetry/catchis redundant but harmless.The test calls
builder.registerNode(testNode)twice: once on Line 81 wrapped inexpect(...).toThrow(ShardIdOverflowError), and again on Line 83 insidetry/catchto inspect the error properties. Both calls operate on the same builder after the shard is already at overflow. This works, but the duplicated throw-and-catch pattern can be simplified usingexpect(...).toThrow()combined with a thrown-error capture approach:♻️ Cleaner overflow assertion
- expect(() => builder.registerNode(testNode)).toThrow(ShardIdOverflowError); - try { - builder.registerNode(testNode); - } catch (_e) { - const err = /** `@type` {*} */ (_e); - expect(err.code).toBe('E_SHARD_ID_OVERFLOW'); - expect(err.context.shardKey).toBe(shardKey); - } + let caught; + try { + builder.registerNode(testNode); + } catch (_e) { + caught = /** `@type` {*} */ (_e); + } + expect(caught).toBeInstanceOf(ShardIdOverflowError); + expect(caught.code).toBe('E_SHARD_ID_OVERFLOW'); + expect(caught.context.shardKey).toBe(shardKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/LogicalBitmapIndexBuilder.stability.test.js` around lines 70 - 88, Remove the duplicated registerNode invocation: call builder.registerNode(testNode) only once inside a single try/catch, then assert the thrown error is an instance of ShardIdOverflowError (e.g. expect(err).toBeInstanceOf(ShardIdOverflowError)) and check err.code and err.context.shardKey against F12_STABLE_IDS.overflowNextLocalId and computeShardKey(testNode) respectively; alternatively keep the expect(() => builder.registerNode(testNode)).toThrow(ShardIdOverflowError) assertion but do not call registerNode a second time—capture the thrown error in a single try/catch or by using Jest's .toThrowErrorMatchingSnapshot-style capture—ensuring only one invocation of LogicalBitmapIndexBuilder.registerNode is used to both assert the type and inspect properties.src/domain/utils/MinHeap.js (1)
86-96:_compareparameter names (idxA,idxB) suggest indices, but doc says "Negative if a < b" — naming is slightly misleading.The JSDoc says "Negative if a < b, positive if a > b, zero if equal" where
aandbrefer to the entries at those indices. The parameter namesidxA/idxBare accurate, but the doc notationa < bwithout clarifying these are entries could mislead a reader into thinking they're the items themselves. This is a minor clarity nit.📝 Suggested doc clarification
- * `@returns` {number} Negative if a < b, positive if a > b, zero if equal + * `@returns` {number} Negative if entry at idxA should come before idxB, positive if after, zero if equal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/utils/MinHeap.js` around lines 86 - 96, Update the JSDoc for the _compare method to explicitly state that the comparison is between the heap entries located at the provided indices (idxA and idxB), e.g. "Negative if the entry at idxA < the entry at idxB, positive if the entry at idxA > the entry at idxB, zero if equal"; reference the method name _compare and parameters idxA/idxB so readers know "a" and "b" refer to this._heap[idxA] and this._heap[idxB], and keep the existing tie-breaker behavior description intact.test/integration/api/materialize.checkpointIndex.notStale.test.js (1)
38-49: The shape difference betweenneighbors()and_logicalIndex.getEdges()is intentional design — focus the test on public API instead.The two APIs deliberately use different shapes:
neighbors()returns{nodeId, label, direction}via a transformation layer (tagDirection()), while_logicalIndex.getEdges()returns the raw internal format{neighborId, label}. This is by design—the private internal API usesneighborId, and the public API normalizes it tonodeId.However, the test accesses
_logicalIndexdirectly, a private field declared inWarpGraph.js. This creates unnecessary tight coupling. Since the test already calls the publicneighbors()method (lines 38–41), consider removing the private field assertions (lines 44–49) or move them to a unit-level test that documents the internal boundary. The integration test's primary focus should be verifying the publicneighbors()API after materialize/reopen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/api/materialize.checkpointIndex.notStale.test.js` around lines 38 - 49, The test is asserting against a private internal shape by accessing reopened._logicalIndex.getEdges(), which couples the integration test to internal representation; remove the private assertions and only verify the public API via reopened.neighbors('A', 'outgoing') (and its expected signatures), or alternatively move assertions about _logicalIndex.getEdges() into a dedicated unit test for WarpGraph internals (referencing WarpGraph.js, _logicalIndex.getEdges, neighbors(), and tagDirection()). Ensure the integration test only checks the public neighbors() behavior and expected ['B:knows','C:manages'].src/ports/SeekCachePort.js (1)
1-10: Class-level JSDoc still describes only "serialized WarpStateV5 snapshots" without mentioning the extended{ buffer, indexTreeOid }shape.📝 Suggested doc update
- * Defines the contract for caching and retrieving serialized WarpStateV5 - * snapshots keyed by (ceiling, frontier) tuples. Used by the seek time-travel - * feature to avoid full re-materialization for previously-visited ticks. + * Defines the contract for caching and retrieving serialized WarpStateV5 + * snapshots keyed by (ceiling, frontier) tuples. Cache entries carry both the + * serialized state buffer and optional index metadata (indexTreeOid). Used by + * the seek time-travel feature to avoid full re-materialization for + * previously-visited ticks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ports/SeekCachePort.js` around lines 1 - 10, Update the class-level JSDoc in SeekCachePort to explicitly document that cached values are not just raw serialized WarpStateV5 bytes but an object shaped as { buffer, indexTreeOid } (or similar), describe the semantics of both fields and that keys are (ceiling, frontier) tuples used for seek time-travel, and add a note that concrete adapters like CasSeekCacheAdapter persist this extended shape; make sure to reference the SeekCachePort interface and any methods that return/accept the cache value so callers know to read/write the object form.src/domain/services/LogicalIndexBuildService.js (1)
24-27:_loggeris injected but never used inbuild().The logger is stored but no log calls appear anywhere in the service. If this is intentional (reserved for future instrumentation), a brief comment noting it would prevent future confusion; otherwise, a debug-level log at the start or on receipt decode would add useful observability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/LogicalIndexBuildService.js` around lines 24 - 27, The constructor stores a logger (this._logger) but the build() method never uses it; either add a short comment in the constructor explaining the logger is reserved for future instrumentation, or instrument build() with a debug log (e.g., at start of build() and/or after decoding input via this._codec) using this._logger.debug/info so observability is present; reference the constructor, this._logger, build(), and this._codec when making the change.test/unit/domain/services/LogicalIndexReader.test.js (1)
20-48: Consider extractingfixtureToStateto the sharedfixtureDsl.jshelper.This helper is duplicated across at least three test files (
LogicalIndexReader.test.js,MaterializedViewService.verify.test.jsasbuildTestState,LogicalIndexBuildService.test.jsasbuildState). All follow the same pattern: create empty V5 state, add nodes, add edges, add props with incrementing lamport/opIdx. Extracting totest/helpers/fixtureDsl.js(wheremakeFixturealready lives) would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/LogicalIndexReader.test.js` around lines 20 - 48, Extract the duplicated fixtureToState logic into the shared test helper (e.g., test/helpers/fixtureDsl.js) and replace the in-file copies with imports; specifically, move the function that uses createEmptyStateV5, createDot, createEventId and applyOpV2 (the current fixtureToState implementation) into fixtureDsl.js alongside makeFixture, export it (e.g., export function fixtureToState), then update LogicalIndexReader.test.js, MaterializedViewService.verify.test.js (buildTestState) and LogicalIndexBuildService.test.js (buildState) to import and call the shared fixtureToState to eliminate duplication.src/domain/services/AdjacencyNeighborProvider.js (1)
113-126: Minor: unnecessary computation of both edge lists when only one direction is needed.
outEdgesandinEdgesare both computed upfront, but only one is used for'out'or'in'. SincefilterByLabelswith a label set iterates the full list, this is wasted work for filtered single-direction queries. Not a real concern for an in-memory'sync'provider, but noting for awareness.♻️ Lazy computation
getNeighbors(nodeId, direction, options) { const labels = options?.labels; - const outEdges = filterByLabels(this._outgoing.get(nodeId) || [], labels); - const inEdges = filterByLabels(this._incoming.get(nodeId) || [], labels); if (direction === 'out') { - return Promise.resolve(outEdges); + return Promise.resolve(filterByLabels(this._outgoing.get(nodeId) || [], labels)); } if (direction === 'in') { - return Promise.resolve(inEdges); + return Promise.resolve(filterByLabels(this._incoming.get(nodeId) || [], labels)); } // 'both': merge two pre-sorted lists, dedup by (neighborId, label) + const outEdges = filterByLabels(this._outgoing.get(nodeId) || [], labels); + const inEdges = filterByLabels(this._incoming.get(nodeId) || [], labels); return Promise.resolve(mergeSorted(outEdges, inEdges)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/AdjacencyNeighborProvider.js` around lines 113 - 126, The getNeighbors method currently computes both outEdges and inEdges eagerly even when direction is 'in' or 'out'; change it to compute edges lazily: inspect direction first and only call filterByLabels(this._outgoing.get(nodeId) || [], labels) when direction is 'out' or both, and only call filterByLabels(this._incoming.get(nodeId) || [], labels) when direction is 'in' or both; keep the existing behavior for 'both' by computing both and returning mergeSorted(outEdges, inEdges); reference getNeighbors, filterByLabels, _outgoing, _incoming, and mergeSorted when making the change.src/domain/services/PropertyIndexReader.js (1)
20-27: Verbose default parameter cast — works but could be cleaner.The constructor default
= /**@type{...} */ ({})is a workaround for JSDoc type narrowing. It's functional but a bit dense. Not a blocker.♻️ Slightly cleaner alternative
- constructor({ storage, codec, maxCachedShards = 64 } = /** `@type` {{ storage?: import('../../ports/IndexStoragePort.js').default, codec?: import('../../ports/CodecPort.js').default, maxCachedShards?: number }} */ ({})) { + /** + * `@param` {{ storage?: import('../../ports/IndexStoragePort.js').default, codec?: import('../../ports/CodecPort.js').default, maxCachedShards?: number }} [options] + */ + constructor(options = {}) { + const { storage, codec, maxCachedShards = 64 } = options;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/PropertyIndexReader.js` around lines 20 - 27, Refactor the constructor parameter JSDoc to be cleaner: replace the inline cast default (= /** `@type` {...} */ ({})) by adding a single JSDoc `@param` above the constructor (documenting storage, codec, maxCachedShards) and accept a plain opts = {} parameter, then destructure const { storage, codec, maxCachedShards = 64 } = opts inside the constructor and keep existing assignments to this._storage, this._codec, this._shardOids, and this._cache; update references to the constructor signature so the unique symbol "constructor({ storage, codec, maxCachedShards = 64 } = ...)" becomes "constructor(opts = {})" with the documented `@param`.src/domain/services/IncrementalIndexUpdater.js (1)
650-659:_flushEdgeShardsunconditionally writes all cached shards for the given direction.Every meta or edge shard loaded into the cache (even if only read, not modified) gets serialized and flushed to
out. For example,_purgeNodeEdgesloads peer shards to remove dead-node references — if no references exist, the shard is still flushed unchanged. This is functionally correct (idempotent writes) but creates unnecessary dirty shard entries that downstream will persist.This is a minor efficiency concern, not a correctness issue. A dirty-tracking flag per cache entry would avoid redundant writes, but this can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/IncrementalIndexUpdater.js` around lines 650 - 659, _flushEdgeShards currently serializes every shard whose cacheKey starts with `${dir}_` even if unchanged; change it to only write entries that were modified by adding and checking a dirty flag on cache values (e.g., store entries as {data, dirty} or maintain a Set of modified keys). Update all places that load or mutate shards (notably _purgeNodeEdges and any _load.../_get... helpers) to set entry.dirty = true (or add the key to the modified Set) when they change the shard, and then modify _flushEdgeShards to skip entries where dirty is false/missing and only encode/write when dirty is true, leaving existing behavior otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ARCHITECTURE.md`:
- Around line 256-264: Update the doc to match the implementation: change the
described signature for NeighborProviderPort.getNeighbors from
getNeighbors(nodeId, direction, labelFilter) to getNeighbors(nodeId, direction,
options) and document that options.labels is a Set of labels (as used by
AdjacencyNeighborProvider and BitmapNeighborProvider / LogicalIndexReader).
Ensure both occurrences (around the NeighborProviderPort description and the
implementation notes for AdjacencyNeighborProvider/BitmapNeighborProvider)
mention the options object and the labels Set type so the docs match the actual
method shape.
In `@docs/CLI_GUIDE.md`:
- Around line 768-782: Add exit-code documentation to the `reindex` section
mirroring `verify-index`: state that `reindex` exits with code 0 on success and
code 3 on failure (or mismatch), so scripts can rely on its return value; update
the `reindex` paragraph (the "reindex — Rebuild bitmap indexes from materialized
state" section) to include a one-line "Exit codes" note referencing these codes
for parity with `verify-index`.
In `@src/domain/services/GraphTraversal.js`:
- Around line 118-147: The shared instance counters _cacheHits, _cacheMisses,
and _edgesTraversed are unsafe for concurrent traversals; instead make stats
local to each traversal run by removing reliance on _resetStats()/_cache*
fields: initialize a per-run stats object inside the traversal entry function
(e.g., create {cacheHits:0,cacheMisses:0,edgesTraversed:0}), increment that
object throughout the traversal logic (replace direct increments of
_cacheHits/_cacheMisses/_edgesTraversed), and update _stats(nodesVisited, stats)
(or overload _stats to accept the per-run object) to build the returned
TraversalStats; ensure no other code reads the instance fields so you can delete
or deprecate _resetStats and the instance counters.
In `@src/domain/services/IncrementalIndexUpdater.js`:
- Around line 254-261: Add a toBytes normalization helper (e.g., function
toBytes(x){ return Uint8Array.from(x); }) in IncrementalIndexUpdater similar to
LogicalIndexReader, and use it whenever calling RoaringBitmap32.deserialize on
CBOR-decoded buffers returned from _loadEdgeShard; specifically replace
occurrences that pass .slice() (e.g., fwdData[bucket][gidStr].slice() and
similar at the other deserialize sites) with toBytes(...slice()) so deserialize
always receives a Uint8Array; update all six deserialization sites that call
RoaringBitmap32.deserialize to wrap their .slice() input with toBytes.
In `@src/domain/services/TemporalQuery.js`:
- Around line 99-118: evaluateAlwaysCheckpointBoundary currently short-circuits
when startIdx > 0, which skips evaluating the checkpoint when startIdx is 0
(case when allPatches is empty and findIndex returned -1); change the guard in
evaluateAlwaysCheckpointBoundary so it allows startIdx === 0 when
checkpointMaxLamport === since (e.g., use startIdx >= 0 or an explicit check for
startIdx === 0) so the function will call extractNodeSnapshot and evaluate the
predicate at the checkpoint boundary; update the return logic unchanged but
ensure the predicate branch is reached for that case and add a unit test
covering allPatches empty / findIndex === -1 to validate behavior.
In `@src/domain/utils/MinHeap.js`:
- Line 17: The constructor currently destructures its parameter which throws
when null is passed; update MinHeap's constructor to guard against null by
accepting a single parameter (e.g., options) and defaulting to an empty object
when it's falsy, then extract tieBreaker from that safe object so new
MinHeap(null) won't throw — apply this change in the MinHeap constructor where
tieBreaker is referenced.
In `@src/domain/utils/shardKey.js`:
- Around line 14-19: computeShardKey currently calls HEX_RE.test(id) and
fnv1a(id) directly which throws on null/undefined and hashes UTF-16 code units;
add a defensive guard at the top of computeShardKey to return null (graceful
shard miss) when id is null/undefined or not a string, and for non-ASCII IDs
compute the FNV-1a over UTF-8 bytes instead of JS charCode units by feeding
TextEncoder().encode(id) into a byte-based fnv1a implementation (or extend fnv1a
to accept Uint8Array); keep using HEX_RE and the 0xff mask and
.toString(16).padStart(2,'0') for the final shard output.
In `@test/helpers/fixtureDsl.js`:
- Around line 498-525: The mismatch check assumes thrown values are Error
instances; update the error-comparison block (the branch handling baseline.error
and current.error inside the results loop) to normalize non-Error throw values
before comparing: for baseline.error and current.error, if the value is not an
instance of Error, wrap/convert it into an Error-like object (e.g., set .name to
the typeof value or 'NonError', and .message to String(value) or JSON when
possible) and then compare the normalized .name and .message fields; ensure you
reference and replace usage of baseErr and curErr (currently derived from
baseline.error and current.error) with these normalized objects so non-Error
throws are detected as mismatches.
In `@test/unit/domain/services/IncrementalIndexUpdater.test.js`:
- Around line 170-211: The test mutates the serialized meta shard using
Buffer.from(defaultCodec.encode(meta)), which breaks in no-Buffer environments;
replace Buffer.from(...) with a Buffer-free typed-array construction such as
Uint8Array.from(...) or new Uint8Array(...) when writing back into
tree[`meta_${shardKey}.cbor`], keeping the use of defaultCodec.encode(meta) and
the same keys (metaBuf, meta, shardKey, tree, `meta_${shardKey}.cbor`) so the
loadShard accessor still returns the encoded bytes but without relying on global
Buffer.
---
Duplicate comments:
In `@src/domain/services/GraphTraversal.js`:
- Around line 388-391: In isReachable, validate that the start node exists
before the early-return (the block that calls this._resetStats() and checks
start === goal); if start is missing return reachable: false (or throw) with
appropriate stats from this._stats, and also ensure goal is validated similarly
before proceeding. Update the early-return path inside isReachable to first
check node existence (using the traversal/graph's node-existence helper) and
only return reachable: true when both nodes exist and start === goal, keeping
the existing calls to this._resetStats() and this._stats(1).
In `@src/domain/services/LogicalIndexBuildService.js`:
- Around line 88-97: The loop in LogicalIndexBuildService previously decoded
edge prop keys incorrectly; the current fix correctly uses the isEdgePropKey
guard before calling decodePropKey, so no further changes are needed—keep the
guard check in the for (const [propKey, register] of state.prop) loop and ensure
decodePropKey(nodeId, propKey) is only invoked after isEdgePropKey(propKey)
returns false (refer to functions isEdgePropKey, decodePropKey, and
nodeVisibleV5 and the propBuilder.addProperty call).
In `@src/domain/services/MaterializedViewService.js`:
- Around line 345-378: verifyIndex currently samples only from state.nodeAlive
so stale/ghost nodes present only in logicalIndex never get checked; update
verifyIndex to also sample a small set from logicalIndex's alive set (use
logicalIndex.isAlive over logicalIndex's node list or a hypothetical
logicalIndex.getAliveNodes()) and merge those samples with the ones from
state.nodeAlive (dedupe by nodeId), preserving the seed/sampleRate logic with
sampleNodes; then run the same verification loop for the combined sample so
ghost entries are detected and counted in errors/failed/passed.
In `@test/unit/domain/services/GraphTraversal.test.js`:
- Around line 621-626: The mock async provider's getNeighbors implementation
drops the third options argument; update asyncProvider.getNeighbors to forward
all received arguments to inner.getNeighbors (e.g., use the same param list or
rest args) so that the original third parameter (label/filter options) is passed
through; keep hasNode and latencyClass unchanged and ensure the symbol
references are asyncProvider.getNeighbors and inner.getNeighbors.
---
Nitpick comments:
In `@src/domain/services/AdjacencyNeighborProvider.js`:
- Around line 113-126: The getNeighbors method currently computes both outEdges
and inEdges eagerly even when direction is 'in' or 'out'; change it to compute
edges lazily: inspect direction first and only call
filterByLabels(this._outgoing.get(nodeId) || [], labels) when direction is 'out'
or both, and only call filterByLabels(this._incoming.get(nodeId) || [], labels)
when direction is 'in' or both; keep the existing behavior for 'both' by
computing both and returning mergeSorted(outEdges, inEdges); reference
getNeighbors, filterByLabels, _outgoing, _incoming, and mergeSorted when making
the change.
In `@src/domain/services/IncrementalIndexUpdater.js`:
- Around line 650-659: _flushEdgeShards currently serializes every shard whose
cacheKey starts with `${dir}_` even if unchanged; change it to only write
entries that were modified by adding and checking a dirty flag on cache values
(e.g., store entries as {data, dirty} or maintain a Set of modified keys).
Update all places that load or mutate shards (notably _purgeNodeEdges and any
_load.../_get... helpers) to set entry.dirty = true (or add the key to the
modified Set) when they change the shard, and then modify _flushEdgeShards to
skip entries where dirty is false/missing and only encode/write when dirty is
true, leaving existing behavior otherwise.
In `@src/domain/services/LogicalIndexBuildService.js`:
- Around line 24-27: The constructor stores a logger (this._logger) but the
build() method never uses it; either add a short comment in the constructor
explaining the logger is reserved for future instrumentation, or instrument
build() with a debug log (e.g., at start of build() and/or after decoding input
via this._codec) using this._logger.debug/info so observability is present;
reference the constructor, this._logger, build(), and this._codec when making
the change.
In `@src/domain/services/PropertyIndexReader.js`:
- Around line 20-27: Refactor the constructor parameter JSDoc to be cleaner:
replace the inline cast default (= /** `@type` {...} */ ({})) by adding a single
JSDoc `@param` above the constructor (documenting storage, codec, maxCachedShards)
and accept a plain opts = {} parameter, then destructure const { storage, codec,
maxCachedShards = 64 } = opts inside the constructor and keep existing
assignments to this._storage, this._codec, this._shardOids, and this._cache;
update references to the constructor signature so the unique symbol
"constructor({ storage, codec, maxCachedShards = 64 } = ...)" becomes
"constructor(opts = {})" with the documented `@param`.
In `@src/domain/utils/MinHeap.js`:
- Around line 86-96: Update the JSDoc for the _compare method to explicitly
state that the comparison is between the heap entries located at the provided
indices (idxA and idxB), e.g. "Negative if the entry at idxA < the entry at
idxB, positive if the entry at idxA > the entry at idxB, zero if equal";
reference the method name _compare and parameters idxA/idxB so readers know "a"
and "b" refer to this._heap[idxA] and this._heap[idxB], and keep the existing
tie-breaker behavior description intact.
In `@src/domain/utils/roaring.js`:
- Around line 108-121: The function initRoaring leaves initError set when called
with no arguments even after a successful import; update initRoaring so that in
the branch where you import and assign roaringModule (and after handling
roaringModule.default), you also set initError = null to clear any stale error
(i.e., ensure initError is cleared whenever roaringModule is successfully
assigned in initRoaring).
- Around line 92-98: The loadRoaring function omits non-Error initError values
when building the cause string; update the cause construction in loadRoaring to
include any initError by coercing it to a string (e.g., use initError instanceof
Error ? initError.message : String(initError)) so that non-Error throwables are
preserved in the diagnostic while still handling Error objects via message; keep
referencing roaringModule and initError inside loadRoaring and include the
resulting cause in the thrown Error message.
In `@src/ports/SeekCachePort.js`:
- Around line 1-10: Update the class-level JSDoc in SeekCachePort to explicitly
document that cached values are not just raw serialized WarpStateV5 bytes but an
object shaped as { buffer, indexTreeOid } (or similar), describe the semantics
of both fields and that keys are (ceiling, frontier) tuples used for seek
time-travel, and add a note that concrete adapters like CasSeekCacheAdapter
persist this extended shape; make sure to reference the SeekCachePort interface
and any methods that return/accept the cache value so callers know to read/write
the object form.
In `@test/integration/api/materialize.checkpointIndex.notStale.test.js`:
- Around line 38-49: The test is asserting against a private internal shape by
accessing reopened._logicalIndex.getEdges(), which couples the integration test
to internal representation; remove the private assertions and only verify the
public API via reopened.neighbors('A', 'outgoing') (and its expected
signatures), or alternatively move assertions about _logicalIndex.getEdges()
into a dedicated unit test for WarpGraph internals (referencing WarpGraph.js,
_logicalIndex.getEdges, neighbors(), and tagDirection()). Ensure the integration
test only checks the public neighbors() behavior and expected
['B:knows','C:manages'].
In `@test/unit/domain/services/LogicalBitmapIndexBuilder.stability.test.js`:
- Line 41: Replace the fragile substring extraction that computes shardKey from
path using path.slice(5, 7) with a robust pattern match: use a regular
expression (e.g. one that matches filenames like "meta_XX.cbor" and captures the
two-hex-digit shard) to extract the shard into the shardKey variable, and add an
explicit assert/throw if the filename does not match so the test fails loudly
instead of producing an incorrect key; update the code that references path and
shardKey in the LogicalBitmapIndexBuilder.stability.test.js test accordingly.
- Around line 70-88: Remove the duplicated registerNode invocation: call
builder.registerNode(testNode) only once inside a single try/catch, then assert
the thrown error is an instance of ShardIdOverflowError (e.g.
expect(err).toBeInstanceOf(ShardIdOverflowError)) and check err.code and
err.context.shardKey against F12_STABLE_IDS.overflowNextLocalId and
computeShardKey(testNode) respectively; alternatively keep the expect(() =>
builder.registerNode(testNode)).toThrow(ShardIdOverflowError) assertion but do
not call registerNode a second time—capture the thrown error in a single
try/catch or by using Jest's .toThrowErrorMatchingSnapshot-style
capture—ensuring only one invocation of LogicalBitmapIndexBuilder.registerNode
is used to both assert the type and inspect properties.
In `@test/unit/domain/services/LogicalIndexReader.test.js`:
- Around line 20-48: Extract the duplicated fixtureToState logic into the shared
test helper (e.g., test/helpers/fixtureDsl.js) and replace the in-file copies
with imports; specifically, move the function that uses createEmptyStateV5,
createDot, createEventId and applyOpV2 (the current fixtureToState
implementation) into fixtureDsl.js alongside makeFixture, export it (e.g.,
export function fixtureToState), then update LogicalIndexReader.test.js,
MaterializedViewService.verify.test.js (buildTestState) and
LogicalIndexBuildService.test.js (buildState) to import and call the shared
fixtureToState to eliminate duplication.
In `@test/unit/domain/warp/hydrateCheckpointIndex.regression.test.js`:
- Around line 69-73: Add a brief comment in the test near the checkpoint stub
(where indexShardOids: { 'meta_stale.cbor': 'oid_stale' } is returned)
explaining that _cachedIndexTree is explicitly set to null to ensure the
production code does not read indexShardOids (the regression invariant) and that
the test asserts hydrateCheckpointIndex is never called; reference
hydrateCheckpointIndex, _cachedIndexTree and indexShardOids so future
maintainers understand this guard is intentional and to update the test if code
paths change to read indexShardOids earlier.
- Around line 62-102: The test's manual context stub is fragile: when calling
materialize.call(context) an unstubbed this.* will cause a TypeError rather than
a clear test failure; wrap the context in a Proxy (or add explicit checks) so
any read or invocation of an unstubbed property throws a descriptive error
(e.g., "test stub missing this.<name> required by materialize") and update the
test to use that proxiedContext in place of context; reference the existing
hydrateCheckpointIndex method and materialize.call(context) in the change and
add a short comment listing which fields are intentionally omitted vs. required
by materialize.methods.js to prevent silent breakage.
In `@test/unit/domain/WarpGraph.test.js`:
- Around line 21-38: The local createMockPersistence mock in WarpGraph.test.js
is missing the nodeExists and isAncestor functions; update the mock used in this
file by either importing the shared createMockPersistence from
test/helpers/warpGraphTestUtils.js or add mocked functions named nodeExists and
isAncestor (e.g., vi.fn().mockResolvedValue(false) or appropriate defaults) to
the returned object so calls to createMockPersistence().nodeExists and
.isAncestor return sensible values instead of undefined.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
ARCHITECTURE.mdBACKLOG.mdCHANGELOG.mdROADMAP.mdTHE_PROBLEM.mdbin/cli/commands/verify-index.jsbin/cli/infrastructure.jsdocker-compose.test.ymldocker/Dockerfile.denodocker/Dockerfile.node20docker/Dockerfile.node22docs/CLI_GUIDE.mdindex.d.tspackage.jsonsrc/domain/services/AdjacencyNeighborProvider.jssrc/domain/services/CheckpointMessageCodec.jssrc/domain/services/CheckpointService.jssrc/domain/services/GraphTraversal.jssrc/domain/services/IncrementalIndexUpdater.jssrc/domain/services/LogicalBitmapIndexBuilder.jssrc/domain/services/LogicalIndexBuildService.jssrc/domain/services/LogicalIndexReader.jssrc/domain/services/MaterializedViewService.jssrc/domain/services/PropertyIndexReader.jssrc/domain/services/TemporalQuery.jssrc/domain/utils/MinHeap.jssrc/domain/utils/roaring.jssrc/domain/utils/shardKey.jssrc/domain/warp/checkpoint.methods.jssrc/domain/warp/query.methods.jssrc/infrastructure/adapters/CasSeekCacheAdapter.jssrc/ports/SeekCachePort.jstest/helpers/fixtureDsl.jstest/integration/api/materialize.checkpointIndex.notStale.test.jstest/integration/api/materializedView.test.jstest/unit/cli/verify-index.test.jstest/unit/domain/WarpGraph.query.test.jstest/unit/domain/WarpGraph.test.jstest/unit/domain/services/AdjacencyNeighborProvider.test.jstest/unit/domain/services/GraphTraversal.astar.test.jstest/unit/domain/services/GraphTraversal.test.jstest/unit/domain/services/IncrementalIndexUpdater.test.jstest/unit/domain/services/LogicalBitmapIndexBuilder.stability.test.jstest/unit/domain/services/LogicalBitmapIndexBuilder.test.jstest/unit/domain/services/LogicalIndexBuildService.determinism.test.jstest/unit/domain/services/LogicalIndexBuildService.test.jstest/unit/domain/services/LogicalIndexReader.test.jstest/unit/domain/services/MaterializedViewService.verify.test.jstest/unit/domain/services/ObserverView.test.jstest/unit/domain/services/PropertyIndex.test.jstest/unit/domain/services/TemporalQuery.checkpoint.test.jstest/unit/domain/services/noBufferGlobal.test.jstest/unit/domain/warp/hydrateCheckpointIndex.regression.test.jstest/unit/helpers/fixtureDsl.test.js
🚧 Files skipped from review as they are similar to previous changes (10)
- src/domain/services/CheckpointMessageCodec.js
- package.json
- bin/cli/commands/verify-index.js
- test/unit/domain/services/LogicalBitmapIndexBuilder.test.js
- test/unit/domain/services/PropertyIndex.test.js
- test/unit/domain/services/TemporalQuery.checkpoint.test.js
- bin/cli/infrastructure.js
- test/unit/domain/services/LogicalIndexBuildService.determinism.test.js
- src/domain/warp/checkpoint.methods.js
- test/unit/domain/services/GraphTraversal.astar.test.js
Comment on lines
+118
to
+147
| this._cacheHits = 0; | ||
| /** @type {number} */ | ||
| this._cacheMisses = 0; | ||
| /** @type {number} */ | ||
| this._edgesTraversed = 0; | ||
| } | ||
|
|
||
| /** | ||
| * Resets per-run stats counters. | ||
| * @private | ||
| */ | ||
| _resetStats() { | ||
| this._cacheHits = 0; | ||
| this._cacheMisses = 0; | ||
| this._edgesTraversed = 0; | ||
| } | ||
|
|
||
| /** | ||
| * Builds a stats snapshot. | ||
| * @param {number} nodesVisited | ||
| * @returns {TraversalStats} | ||
| * @private | ||
| */ | ||
| _stats(nodesVisited) { | ||
| return { | ||
| nodesVisited, | ||
| edgesTraversed: this._edgesTraversed, | ||
| cacheHits: this._cacheHits, | ||
| cacheMisses: this._cacheMisses, | ||
| }; |
There was a problem hiding this comment.
Shared mutable stats are not safe under concurrent traversals.
_cacheHits, _cacheMisses, and _edgesTraversed are instance-global. Concurrent calls can interleave _resetStats() and counter increments, producing incorrect stats responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/GraphTraversal.js` around lines 118 - 147, The shared
instance counters _cacheHits, _cacheMisses, and _edgesTraversed are unsafe for
concurrent traversals; instead make stats local to each traversal run by
removing reliance on _resetStats()/_cache* fields: initialize a per-run stats
object inside the traversal entry function (e.g., create
{cacheHits:0,cacheMisses:0,edgesTraversed:0}), increment that object throughout
the traversal logic (replace direct increments of
_cacheHits/_cacheMisses/_edgesTraversed), and update _stats(nodesVisited, stats)
(or overload _stats to accept the per-run object) to build the returned
TraversalStats; ensure no other code reads the instance fields so you can delete
or deprecate _resetStats and the instance counters.
Comment on lines
+99
to
+118
| function evaluateAlwaysCheckpointBoundary({ | ||
| state, | ||
| nodeId, | ||
| predicate, | ||
| startIdx, | ||
| checkpointMaxLamport, | ||
| since, | ||
| }) { | ||
| if (!(startIdx > 0 && checkpointMaxLamport === since)) { | ||
| return { nodeEverExisted: false, shouldReturn: false, returnValue: false }; | ||
| } | ||
| const snapshot = extractNodeSnapshot(state, nodeId); | ||
| if (!snapshot.exists) { | ||
| return { nodeEverExisted: false, shouldReturn: false, returnValue: false }; | ||
| } | ||
| if (!predicate(snapshot)) { | ||
| return { nodeEverExisted: true, shouldReturn: true, returnValue: false }; | ||
| } | ||
| return { nodeEverExisted: true, shouldReturn: false, returnValue: false }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l src/domain/services/TemporalQuery.jsRepository: git-stunts/git-warp
Length of output: 103
🏁 Script executed:
# Read the function and surrounding context to understand findIndex usage
sed -n '80,130p' src/domain/services/TemporalQuery.jsRepository: git-stunts/git-warp
Length of output: 1564
🏁 Script executed:
# Search for the evaluateAlwaysCheckpointBoundary function calls to understand where startIdx comes from
rg -n "startIdx\s*=" src/domain/services/TemporalQuery.js | head -20Repository: git-stunts/git-warp
Length of output: 126
🏁 Script executed:
# Search for allPatches to understand how it's used
rg -n "allPatches" src/domain/services/TemporalQuery.js | head -20Repository: git-stunts/git-warp
Length of output: 855
🏁 Script executed:
# Read the _resolveStart function
sed -n '311,325p' src/domain/services/TemporalQuery.jsRepository: git-stunts/git-warp
Length of output: 810
🏁 Script executed:
# Find where evaluateAlwaysCheckpointBoundary is called
rg -n "evaluateAlwaysCheckpointBoundary" src/domain/services/TemporalQuery.jsRepository: git-stunts/git-warp
Length of output: 169
🏁 Script executed:
# Look at the always() function to see the full context
rg -B 5 -A 15 "async always" src/domain/services/TemporalQuery.js | head -50Repository: git-stunts/git-warp
Length of output: 638
🏁 Script executed:
# Search for test files related to TemporalQuery
fd -type f -name "*[Tt]emporalQuery*" -o -name "*temporal*" | grep -i testRepository: git-stunts/git-warp
Length of output: 233
🏁 Script executed:
# Find test files
find . -type f -name "*.test.js" -o -name "*.spec.js" | grep -i temporalRepository: git-stunts/git-warp
Length of output: 173
🏁 Script executed:
# Broader search for test directories
find . -type d -name "test" -o -type d -name "tests" -o -type d -name "__tests__" | head -20Repository: git-stunts/git-warp
Length of output: 69
🏁 Script executed:
# Search for test files that might reference TemporalQuery
rg -l "TemporalQuery" --type=js | grep -i testRepository: git-stunts/git-warp
Length of output: 217
🏁 Script executed:
# Look for the always function being tested
rg -l "\.always\(" --type=js | grep -i testRepository: git-stunts/git-warp
Length of output: 169
🏁 Script executed:
# Check the checkpoint test file for coverage of empty allPatches scenario
wc -l test/unit/domain/services/TemporalQuery.checkpoint.test.jsRepository: git-stunts/git-warp
Length of output: 125
🏁 Script executed:
# Search for empty allPatches or related edge cases in checkpoint tests
rg -n "allPatches.*\[\]|empty|checkpoint.*patch" test/unit/domain/services/TemporalQuery.checkpoint.test.js | head -30Repository: git-stunts/git-warp
Length of output: 136
🏁 Script executed:
# Read the checkpoint test file to see what scenarios are tested
head -100 test/unit/domain/services/TemporalQuery.checkpoint.test.jsRepository: git-stunts/git-warp
Length of output: 3255
🏁 Script executed:
# Look for tests that might cover empty allPatches scenario
rg -A 10 "describe.*always" test/unit/domain/services/TemporalQuery.checkpoint.test.js | head -50Repository: git-stunts/git-warp
Length of output: 510
🏁 Script executed:
# Search specifically for edge cases or empty scenarios
rg -B 5 -A 10 "empty|edge.*case" test/unit/domain/services/TemporalQuery.checkpoint.test.jsRepository: git-stunts/git-warp
Length of output: 45
🏁 Script executed:
# See all test cases in the checkpoint file
rg "^\s+it\(" test/unit/domain/services/TemporalQuery.checkpoint.test.jsRepository: git-stunts/git-warp
Length of output: 1105
🏁 Script executed:
# Look at the test "always() evaluates predicate at checkpoint boundary when maxLamport === since"
rg -A 50 "always\(\) evaluates predicate at checkpoint boundary when maxLamport === since" test/unit/domain/services/TemporalQuery.checkpoint.test.jsRepository: git-stunts/git-warp
Length of output: 1651
🏁 Script executed:
# Search the entire test file for scenarios where allPatches might be empty
rg -B 5 -A 15 "loadAllPatches.*\[\]|\[\].*allPatches" test/unit/domain/services/TemporalQuery.checkpoint.test.jsRepository: git-stunts/git-warp
Length of output: 45
🏁 Script executed:
# Check if there's any test mocking empty allPatches
rg -B 3 -A 10 "loadAllPatches" test/unit/domain/services/TemporalQuery.checkpoint.test.js | head -80Repository: git-stunts/git-warp
Length of output: 2033
🏁 Script executed:
# Let's verify if there's any code that would prevent allPatches from being empty while having a checkpoint
rg -B 10 -A 10 "_loadAllPatches" src/domain/services/TemporalQuery.js | head -50Repository: git-stunts/git-warp
Length of output: 1638
🏁 Script executed:
# Check if there are any assertions or guards that prevent empty allPatches with checkpoint
rg -B 5 -A 10 "allPatches.length|length === 0" src/domain/services/TemporalQuery.jsRepository: git-stunts/git-warp
Length of output: 1493
🏁 Script executed:
# Let's check if there's any validation that prevents empty allPatches with checkpoint
rg -B 5 "allPatches.length" src/domain/services/TemporalQuery.jsRepository: git-stunts/git-warp
Length of output: 710
🏁 Script executed:
# Verify the exact behavior: trace through with empty allPatches
# Look at the complete always function to see the full flow
sed -n '185,225p' src/domain/services/TemporalQuery.jsRepository: git-stunts/git-warp
Length of output: 1138
🏁 Script executed:
# Check if there's any documentation or comments about the expected preconditions
rg -B 5 "allPatches" src/domain/services/TemporalQuery.js | grep -A 2 -B 2 "@param\|Precondition\|expects\|Assumes"Repository: git-stunts/git-warp
Length of output: 418
🏁 Script executed:
# Let me trace through the exact flow: read the entire always function to the end
sed -n '185,230p' src/domain/services/TemporalQuery.jsRepository: git-stunts/git-warp
Length of output: 1185
🏁 Script executed:
# Check for any validation or documentation about preconditions for checkpoint consistency
rg -B 10 -A 5 "checkpoint.*allPatches|allPatches.*checkpoint" src/domain/services/TemporalQuery.jsRepository: git-stunts/git-warp
Length of output: 1053
Add test coverage for always() when checkpoint exists but allPatches is empty.
When allPatches is empty and findIndex returns -1, startIdx becomes 0. The guard startIdx > 0 in evaluateAlwaysCheckpointBoundary then short-circuits, returning false without evaluating the predicate at the checkpoint tick—even if the predicate would hold at that boundary. While this scenario likely requires checkpoint state without subsequent patches (possible after compaction), the absence of test coverage leaves the contract unclear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/TemporalQuery.js` around lines 99 - 118,
evaluateAlwaysCheckpointBoundary currently short-circuits when startIdx > 0,
which skips evaluating the checkpoint when startIdx is 0 (case when allPatches
is empty and findIndex returned -1); change the guard in
evaluateAlwaysCheckpointBoundary so it allows startIdx === 0 when
checkpointMaxLamport === since (e.g., use startIdx >= 0 or an explicit check for
startIdx === 0) so the function will call extractNodeSnapshot and evaluate the
predicate at the checkpoint boundary; update the return logic unchanged but
ensure the predicate branch is reached for that case and add a unit test
covering allPatches empty / findIndex === -1 to validate behavior.
| * When omitted, equal-priority extraction order is unspecified (heap-natural). | ||
| */ | ||
| constructor() { | ||
| constructor({ tieBreaker } = {}) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the MinHeap.js file to verify the constructor
cat -n src/domain/utils/MinHeap.jsRepository: git-stunts/git-warp
Length of output: 4934
🏁 Script executed:
# Search for all MinHeap instantiation call sites
rg -n 'new MinHeap' src/ -A 1 -B 1Repository: git-stunts/git-warp
Length of output: 2008
🏁 Script executed:
# Also check for imports or references to MinHeap
rg -n 'MinHeap' src/ --type=js | head -30Repository: git-stunts/git-warp
Length of output: 1673
🏁 Script executed:
# Test the destructuring behavior to confirm the TypeError
node <<'EOF'
// Test 1: Normal destructuring with default
try {
const { tieBreaker } = {};
console.log("Test 1 (empty object): OK -", tieBreaker === undefined);
} catch (e) {
console.log("Test 1 (empty object): ERROR -", e.message);
}
// Test 2: Destructuring null with default
try {
const fn = ({ tieBreaker } = {}) => tieBreaker;
const result = fn(null);
console.log("Test 2 (null passed): OK -", result === undefined);
} catch (e) {
console.log("Test 2 (null passed): ERROR -", e.message);
}
// Test 3: Destructuring undefined
try {
const fn = ({ tieBreaker } = {}) => tieBreaker;
const result = fn(undefined);
console.log("Test 3 (undefined passed): OK -", result === undefined);
} catch (e) {
console.log("Test 3 (undefined passed): ERROR -", e.message);
}
// Test 4: No arguments
try {
const fn = ({ tieBreaker } = {}) => tieBreaker;
const result = fn();
console.log("Test 4 (no args): OK -", result === undefined);
} catch (e) {
console.log("Test 4 (no args): ERROR -", e.message);
}
// Test 5: Proposed fix
try {
const fn = (options = {}) => {
const { tieBreaker } = options || {};
return tieBreaker;
};
const result = fn(null);
console.log("Test 5 (proposed fix with null): OK -", result === undefined);
} catch (e) {
console.log("Test 5 (proposed fix with null): ERROR -", e.message);
}
EOFRepository: git-stunts/git-warp
Length of output: 355
new MinHeap(null) will throw a TypeError — null is not destructurable.
constructor({ tieBreaker } = {}) handles undefined correctly (both explicit and omitted), but fails when null is passed: TypeError: Cannot destructure property 'tieBreaker' of null. While no current call sites in the codebase pass null, this edge case should be handled for API robustness.
🛡️ Proposed fix
- constructor({ tieBreaker } = {}) {
+ constructor(options = {}) {
+ const { tieBreaker } = options || {};
/** `@type` {Array<{item: T, priority: number}>} */
this._heap = [];
/** `@type` {((a: T, b: T) => number) | undefined} */
this._tieBreaker = tieBreaker;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/utils/MinHeap.js` at line 17, The constructor currently
destructures its parameter which throws when null is passed; update MinHeap's
constructor to guard against null by accepting a single parameter (e.g.,
options) and defaulting to an empty object when it's falsy, then extract
tieBreaker from that safe object so new MinHeap(null) won't throw — apply this
change in the MinHeap constructor where tieBreaker is referenced.
Comment on lines
+14
to
+19
| export default function computeShardKey(id) { | ||
| if (HEX_RE.test(id)) { | ||
| return id.substring(0, 2).toLowerCase(); | ||
| } | ||
| const hash = fnv1a(id); | ||
| return (hash & 0xff).toString(16).padStart(2, '0'); |
There was a problem hiding this comment.
No guard for non-string input; FNV-1a operates on UTF-16 code units, not UTF-8 bytes.
Two minor concerns:
-
Input type:
HEX_RE.test(id)will throw aTypeErrorifidisnullorundefined. If call sites can ever pass these values (e.g., missing node IDs during incremental index updates), this will produce an uncaught runtime error rather than a graceful shard miss. -
Unicode sharding:
fnv1aiterates overstr.charCodeAt(i), which processes UTF-16 code units. For node IDs containing characters outside the BMP (emoji, certain CJK extension blocks), the hash will differ from a UTF-8 byte-level FNV-1a. If any downstream system (e.g., a non-JS client or a canonical test vector) computes shard keys over UTF-8 bytes, keys will diverge. For purely ASCII node IDs this is a non-issue.
🛡️ Proposed defensive guard
export default function computeShardKey(id) {
+ if (typeof id !== 'string') {
+ throw new TypeError(`computeShardKey: expected string, got ${typeof id}`);
+ }
if (HEX_RE.test(id)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export default function computeShardKey(id) { | |
| if (HEX_RE.test(id)) { | |
| return id.substring(0, 2).toLowerCase(); | |
| } | |
| const hash = fnv1a(id); | |
| return (hash & 0xff).toString(16).padStart(2, '0'); | |
| export default function computeShardKey(id) { | |
| if (typeof id !== 'string') { | |
| throw new TypeError(`computeShardKey: expected string, got ${typeof id}`); | |
| } | |
| if (HEX_RE.test(id)) { | |
| return id.substring(0, 2).toLowerCase(); | |
| } | |
| const hash = fnv1a(id); | |
| return (hash & 0xff).toString(16).padStart(2, '0'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/utils/shardKey.js` around lines 14 - 19, computeShardKey currently
calls HEX_RE.test(id) and fnv1a(id) directly which throws on null/undefined and
hashes UTF-16 code units; add a defensive guard at the top of computeShardKey to
return null (graceful shard miss) when id is null/undefined or not a string, and
for non-ASCII IDs compute the FNV-1a over UTF-8 bytes instead of JS charCode
units by feeding TextEncoder().encode(id) into a byte-based fnv1a implementation
(or extend fnv1a to accept Uint8Array); keep using HEX_RE and the 0xff mask and
.toString(16).padStart(2,'0') for the final shard output.
Comment on lines
+498
to
+525
| // Enforce provider-to-provider equivalence before running per-provider assertions. | ||
| if (results.length > 1) { | ||
| const baseline = results[0]; | ||
| for (const current of results.slice(1)) { | ||
| const baselineErrored = Boolean(baseline.error); | ||
| const currentErrored = Boolean(current.error); | ||
| if (baselineErrored !== currentErrored) { | ||
| throw new Error( | ||
| `Provider mismatch: '${baseline.name}' ${baselineErrored ? 'threw' : 'returned'} but '${current.name}' ${currentErrored ? 'threw' : 'returned'}` | ||
| ); | ||
| } | ||
| if (baselineErrored && currentErrored) { | ||
| const baseErr = /** @type {Error} */ (baseline.error); | ||
| const curErr = /** @type {Error} */ (current.error); | ||
| if (baseErr.name !== curErr.name || baseErr.message !== curErr.message) { | ||
| throw new Error( | ||
| `Provider mismatch: '${baseline.name}' and '${current.name}' threw different errors` | ||
| ); | ||
| } | ||
| continue; | ||
| } | ||
| try { | ||
| deepStrictEqual(current.result, baseline.result); | ||
| } catch { | ||
| throw new Error( | ||
| `Provider mismatch: '${baseline.name}' and '${current.name}' returned different results` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Non-Error throws can bypass cross-provider mismatch detection.
At Line 510–Line 513, thrown values are treated as Error and compared by name/message. If providers throw non-Error values, mismatches can be missed because those fields are often undefined.
🔧 Proposed fix
+ const normalizeThrown = (err) => {
+ if (err instanceof Error) {
+ return { kind: 'Error', name: err.name, message: err.message };
+ }
+ return { kind: typeof err, value: String(err) };
+ };
+
// Enforce provider-to-provider equivalence before running per-provider assertions.
if (results.length > 1) {
@@
if (baselineErrored && currentErrored) {
- const baseErr = /** `@type` {Error} */ (baseline.error);
- const curErr = /** `@type` {Error} */ (current.error);
- if (baseErr.name !== curErr.name || baseErr.message !== curErr.message) {
+ const baseErr = normalizeThrown(baseline.error);
+ const curErr = normalizeThrown(current.error);
+ try {
+ deepStrictEqual(curErr, baseErr);
+ } catch {
throw new Error(
`Provider mismatch: '${baseline.name}' and '${current.name}' threw different errors`
);
}
continue;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helpers/fixtureDsl.js` around lines 498 - 525, The mismatch check
assumes thrown values are Error instances; update the error-comparison block
(the branch handling baseline.error and current.error inside the results loop)
to normalize non-Error throw values before comparing: for baseline.error and
current.error, if the value is not an instance of Error, wrap/convert it into an
Error-like object (e.g., set .name to the typeof value or 'NonError', and
.message to String(value) or JSON when possible) and then compare the normalized
.name and .message fields; ensure you reference and replace usage of baseErr and
curErr (currently derived from baseline.error and current.error) with these
normalized objects so non-Error throws are detected as mismatches.
…lization CBOR decoders return Buffer, Uint8Array, or plain number[] depending on runtime. LogicalIndexReader already had a local toBytes() normalizer; IncrementalIndexUpdater used raw .slice() which fails when the decoder returns number[] (e.g. Deno without Buffer polyfill). - Extract src/domain/utils/toBytes.js — shared Uint8Array normalizer with fast-path for already-Uint8Array inputs - Replace all 7 raw .slice() deserialization sites in IncrementalIndexUpdater with toBytes() - Replace local toBytes() in LogicalIndexReader with shared import - Replace Buffer.from() in IncrementalIndexUpdater test with Uint8Array - Add toBytes unit tests (Uint8Array passthrough, number[], Buffer, empty) - Fix ARCHITECTURE.md getNeighbors signature docs (labelFilter → options) - Add reindex exit-code docs to CLI_GUIDE.md
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/CLI_GUIDE.md (1)
958-958:⚠️ Potential issue | 🟡 MinorClarify that
verify-indexandreindexdo not support the--viewflag.The documentation states both commands accept "None beyond global options", which includes
--view. However, the CLI explicitly rejects--viewfor these commands, throwing an error if attempted. Add a note to each command's flag section to prevent user confusion:Suggested fix
### `verify-index` — Verify bitmap index integrity ... -**Flags:** None beyond [global options](`#global-options`). +**Flags:** None beyond [global options](`#global-options`). Note: `--view` is not supported.Apply the same change to the
reindexsection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CLI_GUIDE.md` at line 958, The docs list global options including --view but the verify-index and reindex commands' flag sections do not clarify that these two commands explicitly reject --view; update the verify-index and reindex flag descriptions (the sections for the verify-index and reindex commands) to include a short note such as "Note: --view is not supported for verify-index" / "Note: --view is not supported for reindex" so users aren't misled by the global options; ensure the wording references the exact flag name (--view) and the command names (verify-index, reindex).
🧹 Nitpick comments (3)
docs/CLI_GUIDE.md (1)
1105-1122: Command Reference ordering doesn't match the TOC and body section order.The TOC (lines 17-18) and body sections (lines 750, 788) place Index Management (
verify-index,reindex) before Verifying Audit Integrity (verify-audit). The Command Reference reverses this, listingverify-audit(line 1105) beforeverify-index/reindex(lines 1112-1122).📝 Proposed fix — move verify-index and reindex before verify-audit in the Command Reference
+### `verify-index` + +| Flag | Description | +|------|-------------| +| _(global only)_ | See [Global Options](`#global-options`) | + +### `reindex` + +| Flag | Description | +|------|-------------| +| _(global only)_ | See [Global Options](`#global-options`) | + ### `verify-audit` | Flag | Description | |------|-------------| | `--writer <id>` | Verify a single writer's chain | | `--since <commit>` | Verify from tip down to this commit | - -### `verify-index` - -| Flag | Description | -|------|-------------| -| _(global only)_ | See [Global Options](`#global-options`) | - -### `reindex` - -| Flag | Description | -|------|-------------| -| _(global only)_ | See [Global Options](`#global-options`) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CLI_GUIDE.md` around lines 1105 - 1122, Reorder the Command Reference so the "verify-index" and "reindex" sections appear before "verify-audit": locate the three headings titled "### `verify-audit`", "### `verify-index`", and "### `reindex`" and move the entire blocks for "verify-index" and "reindex" so they come directly before the "verify-audit" block, ensuring their flag tables remain intact and preserving heading formatting to match the TOC and body section ordering.src/domain/services/LogicalIndexReader.js (1)
206-218: Make shard processing order explicitly deterministic.The method states deterministic processing, but iteration order currently depends on incoming
items. Sortingmetaandedgesbypathwould make this guarantee explicit and stable.♻️ Proposed refactor
_processShards(items) { const Ctor = getRoaringBitmap32(); const { meta, labels, edges } = classifyShards(items); + meta.sort((a, b) => a.path.localeCompare(b.path)); + edges.sort((a, b) => a.path.localeCompare(b.path)); for (const { path, buf } of meta) { this._decodeMeta(path, buf, Ctor); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/LogicalIndexReader.js` around lines 206 - 218, The _processShards method currently iterates meta and edges in the incoming order from classifyShards which makes "deterministic" processing dependent on input order; to fix, sort the meta and edges arrays by their path property (e.g., meta.sort((a,b)=>a.path.localeCompare(b.path))) before running the loops so that calls to _decodeMeta, _decodeLabels and _decodeEdgeShard always occur in a stable, explicit order; keep the labels handling unchanged and make sure sorting happens immediately after const { meta, labels, edges } = classifyShards(items) and before any for...of that uses meta or edges.src/domain/services/IncrementalIndexUpdater.js (1)
68-70: Gate the edge-rehydration scan to truly reactivated nodes.Line 108 scans all alive edges whenever
nodesAddedis non-empty. That turns add-heavy diffs into O(E), even for brand-new nodes that cannot have historical hidden edges.♻️ Proposed direction
- for (const nodeId of diff.nodesAdded) { - this._handleNodeAdd(nodeId, metaCache, loadShard); - } + const reactivated = new Set(); + for (const nodeId of diff.nodesAdded) { + if (this._handleNodeAdd(nodeId, metaCache, loadShard)) { + reactivated.add(nodeId); + } + } - if (diff.nodesAdded.length > 0) { - const addedSet = new Set(diff.nodesAdded); + if (reactivated.size > 0) { + const addedSet = reactivated; ... }Have
_handleNodeAddreturntrueonly when it reactivates an existingglobalId.Also applies to: 99-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/IncrementalIndexUpdater.js` around lines 68 - 70, The loop over diff.nodesAdded currently triggers an expensive edge-rehydration scan for every added node; change _handleNodeAdd in IncrementalIndexUpdater to return true only when it actually reactivates an existing globalId, and update all callers (the loop handling diff.nodesAdded and the related block covering the add-heavy path) to call const reactivated = this._handleNodeAdd(nodeId, metaCache, loadShard) and only perform the edge-rehydration scan when reactivated === true, thereby avoiding O(E) scans for brand-new nodes that cannot have historical hidden edges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ARCHITECTURE.md`:
- Around line 93-96: The Ports diagram is missing SeekCachePort causing
inconsistency with later references; update the ASCII/diagram block that lists
Ports (near the NeighborProvider Port box) to include a SeekCachePort entry so
the diagram matches the rest of the document (ensure the label exactly reads
"SeekCachePort" and is placed with the other Port entries for visual alignment
with NeighborProvider).
In `@docs/CLI_GUIDE.md`:
- Line 1022: Update the Exit Codes table entry for code 3 to explicitly include
"reindex" failure alongside the existing "verify-index mismatch" and internal
errors: locate the table row showing "| 3 | `INTERNAL` | ... |" and append "or
`reindex` failure" (or similar phrasing) to the description so readers scripting
against the reindex command can find the corresponding exit code.
In `@src/domain/services/IncrementalIndexUpdater.js`:
- Around line 596-604: The flush routines (_flushMeta and the similar flush at
lines 651-659) are serializing and writing every entry in metaCache/index cache
rather than only mutated shards; change them to only flush keys marked dirty —
either by maintaining a Set (e.g., dirtyShardKeys) or a per-shard flag (e.g.,
meta.dirty) that is set during mutations (lookups should not set it), iterate
only those dirty keys when building out[`meta_${shardKey}.cbor`] (and the
corresponding index output), and clear the dirty mark after successful
serialization so unchanged cache entries are not re-emitted.
In `@src/domain/services/LogicalIndexReader.js`:
- Around line 173-179: getEdges currently maps any direction not equal to 'out'
to 'rev', silently accepting invalid inputs; update getEdges to validate the
direction param explicitly (e.g., only allow 'out' and 'in' or whichever set
your API requires) and throw a clear error (TypeError or RangeError) when an
unsupported value is passed, using the function name getEdges and node id
mapping n2g to locate the logic that sets dir; ensure the code sets dir only
after validation and add/adjust tests to cover invalid direction values.
---
Outside diff comments:
In `@docs/CLI_GUIDE.md`:
- Line 958: The docs list global options including --view but the verify-index
and reindex commands' flag sections do not clarify that these two commands
explicitly reject --view; update the verify-index and reindex flag descriptions
(the sections for the verify-index and reindex commands) to include a short note
such as "Note: --view is not supported for verify-index" / "Note: --view is not
supported for reindex" so users aren't misled by the global options; ensure the
wording references the exact flag name (--view) and the command names
(verify-index, reindex).
---
Nitpick comments:
In `@docs/CLI_GUIDE.md`:
- Around line 1105-1122: Reorder the Command Reference so the "verify-index" and
"reindex" sections appear before "verify-audit": locate the three headings
titled "### `verify-audit`", "### `verify-index`", and "### `reindex`" and move
the entire blocks for "verify-index" and "reindex" so they come directly before
the "verify-audit" block, ensuring their flag tables remain intact and
preserving heading formatting to match the TOC and body section ordering.
In `@src/domain/services/IncrementalIndexUpdater.js`:
- Around line 68-70: The loop over diff.nodesAdded currently triggers an
expensive edge-rehydration scan for every added node; change _handleNodeAdd in
IncrementalIndexUpdater to return true only when it actually reactivates an
existing globalId, and update all callers (the loop handling diff.nodesAdded and
the related block covering the add-heavy path) to call const reactivated =
this._handleNodeAdd(nodeId, metaCache, loadShard) and only perform the
edge-rehydration scan when reactivated === true, thereby avoiding O(E) scans for
brand-new nodes that cannot have historical hidden edges.
In `@src/domain/services/LogicalIndexReader.js`:
- Around line 206-218: The _processShards method currently iterates meta and
edges in the incoming order from classifyShards which makes "deterministic"
processing dependent on input order; to fix, sort the meta and edges arrays by
their path property (e.g., meta.sort((a,b)=>a.path.localeCompare(b.path)))
before running the loops so that calls to _decodeMeta, _decodeLabels and
_decodeEdgeShard always occur in a stable, explicit order; keep the labels
handling unchanged and make sure sorting happens immediately after const { meta,
labels, edges } = classifyShards(items) and before any for...of that uses meta
or edges.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ARCHITECTURE.mddocs/CLI_GUIDE.mdsrc/domain/services/IncrementalIndexUpdater.jssrc/domain/services/LogicalIndexReader.jssrc/domain/utils/toBytes.jstest/unit/domain/services/IncrementalIndexUpdater.test.jstest/unit/domain/utils/toBytes.test.js
Comment on lines
+93
to
+96
| | +-------------------+ | | ||
| | | NeighborProvider | | | ||
| | | Port | | | ||
| | +-------------------+ | |
There was a problem hiding this comment.
Add SeekCachePort to the layer diagram for consistency.
The diagram’s Ports section omits SeekCachePort, but it is listed later in the same document (Lines 158-159 and Lines 310-318). This creates an avoidable mismatch.
📝 Suggested doc patch
-| +-------------------+ |
-| | NeighborProvider | |
-| | Port | |
-| +-------------------+ |
+| +-------------------+ +---------------------------+ |
+| | NeighborProvider | | SeekCachePort | |
+| | Port | | | |
+| +-------------------+ +---------------------------+ |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ARCHITECTURE.md` around lines 93 - 96, The Ports diagram is missing
SeekCachePort causing inconsistency with later references; update the
ASCII/diagram block that lists Ports (near the NeighborProvider Port box) to
include a SeekCachePort entry so the diagram matches the rest of the document
(ensure the label exactly reads "SeekCachePort" and is placed with the other
Port entries for visual alignment with NeighborProvider).
| | 1 | `USAGE` | Invalid arguments, missing required flags, or validation error | | ||
| | 2 | `NOT_FOUND` | The requested entity was not found (e.g., no path exists, graph not found) | | ||
| | 3 | `INTERNAL` | Unhandled error, or audit chain integrity failure | | ||
| | 3 | `INTERNAL` | Unhandled error, audit chain integrity failure, or `verify-index` mismatch | |
There was a problem hiding this comment.
Exit Codes table omits reindex failure from code 3's description.
Line 782 documents that reindex exits with code 3 on failure, but the Exit Codes table at line 1022 only mentions verify-index mismatch as the non-internal cause. A reader scripting against reindex would find no corresponding entry in the table.
📝 Proposed doc fix
-| 3 | `INTERNAL` | Unhandled error, audit chain integrity failure, or `verify-index` mismatch |
+| 3 | `INTERNAL` | Unhandled error, audit chain integrity failure, `verify-index` mismatch, or `reindex` build failure |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | 3 | `INTERNAL` | Unhandled error, audit chain integrity failure, or `verify-index` mismatch | | |
| | 3 | `INTERNAL` | Unhandled error, audit chain integrity failure, `verify-index` mismatch, or `reindex` build failure | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/CLI_GUIDE.md` at line 1022, Update the Exit Codes table entry for code 3
to explicitly include "reindex" failure alongside the existing "verify-index
mismatch" and internal errors: locate the table row showing "| 3 | `INTERNAL` |
... |" and append "or `reindex` failure" (or similar phrasing) to the
description so readers scripting against the reindex command can find the
corresponding exit code.
Comment on lines
+596
to
+604
| _flushMeta(metaCache, out) { | ||
| for (const [shardKey, meta] of metaCache) { | ||
| const shard = { | ||
| nodeToGlobal: meta.nodeToGlobal, | ||
| nextLocalId: meta.nextLocalId, | ||
| alive: meta.aliveBitmap.serialize(true), | ||
| }; | ||
| out[`meta_${shardKey}.cbor`] = this._codec.encode(shard).slice(); | ||
| } |
There was a problem hiding this comment.
Track dirty shard keys explicitly; current flush rewrites read-only cache entries.
Line 596 and Line 651 flush entire caches, not just mutated shards. Because lookup paths also populate these caches, unchanged shards can be re-serialized and emitted as “dirty,” causing unnecessary writes and violating the dirty-shards contract.
🛠️ Proposed fix
computeDirtyShards({ diff, state, loadShard }) {
+ const dirtyMetaKeys = new Set();
+ const dirtyEdgeKeys = new Set();
...
- this._handleNodeAdd(nodeId, metaCache, loadShard);
+ this._handleNodeAdd(nodeId, metaCache, loadShard, dirtyMetaKeys);
...
- this._flushMeta(metaCache, out);
- this._flushEdgeShards(fwdCache, 'fwd', out);
- this._flushEdgeShards(revCache, 'rev', out);
+ this._flushMeta(metaCache, dirtyMetaKeys, out);
+ this._flushEdgeShards(fwdCache, 'fwd', dirtyEdgeKeys, out);
+ this._flushEdgeShards(revCache, 'rev', dirtyEdgeKeys, out);
}- _flushMeta(metaCache, out) {
- for (const [shardKey, meta] of metaCache) {
+ _flushMeta(metaCache, dirtyMetaKeys, out) {
+ for (const shardKey of dirtyMetaKeys) {
+ const meta = metaCache.get(shardKey);
+ if (!meta) continue;
out[`meta_${shardKey}.cbor`] = this._codec.encode({
nodeToGlobal: meta.nodeToGlobal,
nextLocalId: meta.nextLocalId,
alive: meta.aliveBitmap.serialize(true),
}).slice();
}
}- _flushEdgeShards(cache, dir, out) {
- const prefix = `${dir}_`;
- for (const [cacheKey, data] of cache) {
- if (!cacheKey.startsWith(prefix)) continue;
- out[`${cacheKey}.cbor`] = this._codec.encode(data).slice();
- }
- }
+ _flushEdgeShards(cache, dir, dirtyEdgeKeys, out) {
+ for (const shardKey of dirtyEdgeKeys) {
+ const cacheKey = `${dir}_${shardKey}`;
+ const data = cache.get(cacheKey);
+ if (!data) continue;
+ out[`${cacheKey}.cbor`] = this._codec.encode(data).slice();
+ }
+ }Also applies to: 651-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/IncrementalIndexUpdater.js` around lines 596 - 604, The
flush routines (_flushMeta and the similar flush at lines 651-659) are
serializing and writing every entry in metaCache/index cache rather than only
mutated shards; change them to only flush keys marked dirty — either by
maintaining a Set (e.g., dirtyShardKeys) or a per-shard flag (e.g., meta.dirty)
that is set during mutations (lookups should not set it), iterate only those
dirty keys when building out[`meta_${shardKey}.cbor`] (and the corresponding
index output), and clear the dirty mark after successful serialization so
unchanged cache entries are not re-emitted.
Comment on lines
+173
to
+179
| getEdges(nodeId, direction, filterLabelIds) { | ||
| const gid = n2g.get(nodeId); | ||
| if (gid === undefined) { | ||
| return []; | ||
| } | ||
| const dir = direction === 'out' ? 'fwd' : 'rev'; | ||
|
|
There was a problem hiding this comment.
Validate direction explicitly instead of silently defaulting to reverse.
Line 178 maps every non-'out' input to 'rev'. That hides invalid caller input and can return wrong edges without surfacing an error.
🔧 Proposed fix
- const dir = direction === 'out' ? 'fwd' : 'rev';
+ let dir;
+ if (direction === 'out') {
+ dir = 'fwd';
+ } else if (direction === 'in') {
+ dir = 'rev';
+ } else {
+ throw new TypeError(`Invalid direction: ${String(direction)}`);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/LogicalIndexReader.js` around lines 173 - 179, getEdges
currently maps any direction not equal to 'out' to 'rev', silently accepting
invalid inputs; update getEdges to validate the direction param explicitly
(e.g., only allow 'out' and 'in' or whichever set your API requires) and throw a
clear error (TypeError or RangeError) when an unsupported value is passed, using
the function name getEdges and node id mapping n2g to locate the logic that sets
dir; ensure the code sets dir only after validation and add/adjust tests to
cover invalid direction values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LogicalIndexBuildService,LogicalIndexReader,LogicalBitmapIndexBuilder.IncrementalIndexUpdaterfor O(diff) bitmap index updates wired into the materialize path viaJoinReducer.trackDiff.NeighborProviderPort. Deterministic tie-breaking everywhere.graph.traversefacade — all 11GraphTraversalalgorithms exposed through the publicgraph.traverse.*API viaLogicalTraversaldelegation.nodeWeightFn— node-weighted graph support for all 4 weighted algorithms. Lazy memoization, mutually exclusive withweightFn.TemporalQueryfast-paths whensince > 0via checkpoint coverage.ObserverViewreuses parent provider.verify-indexandreindexcommands.Test plan
tsc --noEmitclean (0 wildcards)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests