fix(deps): upgrade vitest 2→4, resolve all npm audit findings#63
Merged
flyingrobots merged 21 commits intomainfrom Mar 4, 2026
Merged
fix(deps): upgrade vitest 2→4, resolve all npm audit findings#63flyingrobots merged 21 commits intomainfrom
flyingrobots merged 21 commits intomainfrom
Conversation
Vitest 4 removed the deprecated test(name, fn, { options }) signature.
Migrate all per-test timeouts to the new test(name, { timeout }, fn)
form, or lift uniform timeouts to the describe level.
Also fix vi.fn().mockImplementation(() => ...) constructor mocks to
use function expressions per Vitest 4 requirements (SyncController,
HealthCheckService mocks).
Resolves all 5 remaining moderate-severity npm audit advisories
(esbuild, vite, @vitest/mocker, vite-node, vitest).
📝 WalkthroughWalkthroughAdd a three-tier Roaring loader (native ESM → CJS require → wasm fallback), bulk-add DAG-focused traversal APIs and tests (levels, transitiveReduction, transitiveClosure, rootAncestors), bump Vitest and adjust test timeouts, mark roaring as external for SSR/Vitest, add Changes
Sequence Diagram(s)sequenceDiagram
participant App as Init / initRoaring()
participant ESM as Native ESM Module
participant CJS as Native CJS Module
participant WASM as roaring-wasm (WASM)
App->>ESM: try dynamic import('roaring')
alt ESM import succeeds
ESM-->>App: module (maybe default-wrapped)
else ESM import fails
App->>CJS: try require('roaring') via createRequire
alt CJS require succeeds
CJS-->>App: module (CJS export)
else CJS require fails
App->>WASM: load `roaring-wasm` adapter
WASM-->>App: wasm-backed API (shims applied)
end
end
App->>App: unwrap default / adapt API, cache availability, return roaring instance
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Release Preflight
If you tag this commit as |
1 similar comment
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 125: package.json's dev dependency upgrade to "vitest": "^4.0.18"
requires locking its toolchain dependency vite-node in the lockfile; add or
update the package-lock entry for vite-node to a compatible version (matching
the vitest/vite toolchain, e.g., the version used by vitest 4.x) and run npm
install/npm audit-fix to regenerate package-lock.json so vite-node appears with
a resolved version; ensure the dependency is present and resolved alongside
vitest, vite, `@vitest/mocker`, and esbuild to complete the upgrade.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fee3feeb-2ff9-4fce-a73d-bd9e7498dc94
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsontest/unit/cli/doctor.test.jstest/unit/domain/WarpGraph.cascadeDelete.test.jstest/unit/domain/WarpGraph.deleteGuardEnforce.test.jstest/unit/domain/WarpGraph.noCoordination.test.jstest/unit/domain/WarpGraph.patchMany.test.jstest/unit/domain/WarpGraph.syncMaterialize.test.jstest/unit/domain/services/BisectService.test.jstest/unit/domain/services/IndexRebuildService.deep.test.jstest/unit/domain/services/SyncController.test.js
Vite 7 (pulled in by vitest 4) attempts to transform all imports through its pipeline, which breaks native C++ addons like roaring. Add server.deps.external to skip transformation of the roaring package, restoring Bun integration test compatibility.
Release Preflight
If you tag this commit as |
The external config was placed at the top-level server key instead of under test.server.deps.external where Vitest 4 reads it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vitest.config.js`:
- Around line 4-9: The deps.external setting for the native addon 'roaring' is
placed under server.deps (Vite dev server) but needs to be applied to Vitest;
move the external: ['roaring'] entry from server.deps to test.server.deps so
Vitest's runner sees it (i.e., remove it from server.deps and add it under the
test object as test.server.deps.external). Ensure you keep the same array value
and configuration structure so Vitest will not transform the native C++ addon.
Release Preflight
If you tag this commit as |
Vitest 4 (Vite 7) intercepts dynamic import('roaring') through its
SSR transform pipeline, preventing the native .node binary from
loading under Bun. Add both ssr.external and test.server.deps.external
(regex pattern) to ensure the roaring package bypasses Vite's
transform and resolution phases entirely.
Release Preflight
If you tag this commit as |
… Bun/Vite Vite 7's module runner intercepts dynamic import() calls and fails to transform native C++ addons (.node binaries). initRoaring() now catches the import failure and falls back to createRequire() which loads native modules directly, bypassing the module runner.
Release Preflight
If you tag this commit as |
bun install blocks lifecycle scripts by default, so roaring's node-pre-gyp install never runs, leaving the .node binary missing. Bun also reports a fictional Node ABI (v137) that has no prebuilt binaries available. Fix: install nodejs, python3, and ca-certificates in the Bun Dockerfile, then run node-pre-gyp under real Node.js after bun install. This downloads the correct prebuilt binary (or compiles from source as fallback). Bun loads the binary via roaring's MODULE_NOT_FOUND fallback path (build/Release/roaring.node). The root cause was Docker layer cache invalidation from the Vitest 4 upgrade — previous CI runs reused a cached layer that happened to have the binary from an earlier build.
Release Preflight
If you tag this commit as |
The single-stage approach installed nodejs in the Bun container, which caused Vitest 4 to use node instead of bun for running tests, breaking globalThis.crypto (and all 64 integration tests). Switch to a multi-stage build: - Stage 1 (node:18-slim): npm install + node-pre-gyp to download or compile the roaring native binary with the correct Node ABI - Stage 2 (oven/bun:1.2-slim): bun install + COPY the compiled binary from stage 1, keeping the runtime image Node-free
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/domain/utils/roaring.js (1)
117-124: Preserve both load failures for actionable diagnostics.At Line 117, the original
import('roaring')error is dropped. If Line 123 also fails, you lose the first root cause and debugging gets harder.Proposed fix
- } catch { + } catch (importErr) { // Dynamic import() can fail when a module runner (e.g. Vite 7) // intercepts the call and cannot transform native C++ addons. // Fall back to CJS require() which loads .node binaries directly. - const { createRequire } = await import('node:module'); - const req = createRequire(import.meta.url); - roaringModule = /** `@type` {RoaringModule} */ (req('roaring')); + try { + const { createRequire } = await import('node:module'); + const req = createRequire(import.meta.url); + roaringModule = /** `@type` {RoaringModule} */ (req('roaring')); + } catch (requireErr) { + throw new AggregateError( + [importErr, requireErr], + 'Failed to load roaring via both import() and require()' + ); + } }🤖 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 117 - 124, The dynamic import fallback currently swallows the original import('roaring') failure; change the catch to capture the first error (e.g., errImport) then attempt the CJS require fallback (using createRequire/import.meta.url and req('roaring')), and if the require also fails, throw or log a new Error that includes both the original import error and the require error (or attach them as properties) so both failure causes are preserved; target the try/catch around the import('roaring') and the fallback require where roaringModule is assigned.docker/Dockerfile.bun (1)
15-15: Use lockfile-strict install in the builder stage.At Line 15, switching to
npm ci --ignore-scriptswill make roaring native build inputs deterministic across CI runs.Proposed fix
-RUN npm install --ignore-scripts +RUN npm ci --ignore-scripts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile.bun` at line 15, Replace the non-deterministic install in the builder stage: find the Dockerfile instruction "RUN npm install --ignore-scripts" and change it to use the lockfile-strict command "npm ci --ignore-scripts" so native build inputs are deterministic across CI runs; ensure the builder stage still has package-lock.json present in the image context so npm ci can run successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: Update the release note sentence that currently states the Bun
Dockerfile installs `nodejs`, `python3`, and `ca-certificates` so it accurately
describes the multi-stage Docker build and the use of a separate Node builder
image; e.g., change the wording around the sentence mentioning those packages to
note that native binary build/dependencies are provided in a Node builder stage
(or separate builder image) which supplies `node`, `python3`, and certs for
node-pre-gyp rather than being installed in the final Bun runtime image.
---
Nitpick comments:
In `@docker/Dockerfile.bun`:
- Line 15: Replace the non-deterministic install in the builder stage: find the
Dockerfile instruction "RUN npm install --ignore-scripts" and change it to use
the lockfile-strict command "npm ci --ignore-scripts" so native build inputs are
deterministic across CI runs; ensure the builder stage still has
package-lock.json present in the image context so npm ci can run successfully.
In `@src/domain/utils/roaring.js`:
- Around line 117-124: The dynamic import fallback currently swallows the
original import('roaring') failure; change the catch to capture the first error
(e.g., errImport) then attempt the CJS require fallback (using
createRequire/import.meta.url and req('roaring')), and if the require also
fails, throw or log a new Error that includes both the original import error and
the require error (or attach them as properties) so both failure causes are
preserved; target the try/catch around the import('roaring') and the fallback
require where roaringModule is assigned.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40bb41c0-4645-4199-8a78-91011f98aa7b
📒 Files selected for processing (4)
CHANGELOG.mddocker/Dockerfile.bunsrc/domain/utils/roaring.jsvitest.config.js
…ity) The roaring npm package uses V8 C++ API (#include <node.h>), not Node-API/NAPI. Bun uses JavaScriptCore, not V8, so the native .node binary can never load — 'napi_register_module_v1' symbol not found. Bitmap index tests (materializedView, checkpointIndex.notStale) are excluded from the Bun test suite. This is not a regression — these tests only passed historically due to Docker layer cache luck. The bitmap index system already handles this gracefully via _buildView()'s catch block, degrading to linear scan when roaring is unavailable. Reverts the multi-stage Docker build (unnecessary since the binary can't work regardless of how it's compiled). Simplifies Dockerfile back to git + bun install.
Release Preflight
If you tag this commit as |
When both import('roaring') and createRequire('roaring') fail, throw
an AggregateError containing both root causes instead of silently
dropping the original import error. Also update vitest.config.js
comment to be accurate (roaring uses V8 C++ API, not NAPI).
Release Preflight
If you tag this commit as |
initRoaring() now has a three-tier fallback chain:
Tier 1: import('roaring') — ESM native V8 bindings
Tier 2: createRequire('roaring') — CJS native (Vite workaround)
Tier 3: import('roaring-wasm') — WASM portable fallback
The WASM tier activates automatically when native V8 C++ bindings are
unavailable (Bun's JavaScriptCore, Deno). Serialization formats are
wire-compatible — portable bitmaps from native and WASM are byte-identical.
Bitmap index tests (materializedView, checkpointIndex.notStale) are no
longer excluded from the Bun test suite.
Refactored initRoaring() into per-tier helper functions (tryNativeImport,
tryCjsRequire, tryWasmFallback, unwrapDefault) to satisfy ESLint
complexity/depth/line-count limits.
- README.md: add roaring-wasm to dependency table, add WASM fallback bullet to What's New in v13.0.1 - CLAUDE.md: add roaring-wasm to dependency table
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/utils/roaring.js (1)
190-197:⚠️ Potential issue | 🟡 MinorReset native availability cache when reinitializing the module.
initRoaring(mod)can swap module implementations, but cachednativeAvailabilityis left untouched. SubsequentgetNativeRoaringAvailable()calls can return stale data from a previous module load.🔧 Proposed fix
export async function initRoaring(mod) { if (mod) { - roaringModule = mod; + roaringModule = unwrapDefault(mod); + nativeAvailability = NOT_CHECKED; initError = null; return; } if (roaringModule) { return; } - roaringModule = + const loaded = (await tryNativeImport()) ?? (await tryCjsRequire()) ?? (await tryWasmFallback()); - if (!roaringModule) { + if (!loaded) { throw new Error( 'Failed to load roaring via import(), require(), and roaring-wasm', ); } - roaringModule = unwrapDefault(roaringModule); + roaringModule = unwrapDefault(loaded); + nativeAvailability = NOT_CHECKED; + initError = null; }Also applies to: 207-207
🤖 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 190 - 197, When reinitializing the roaring module in initRoaring(mod), the cached nativeAvailability must be cleared so later calls to getNativeRoaringAvailable() don't return stale values; update initRoaring to reset nativeAvailability (and keep initError handling) whenever roaringModule is replaced or a new mod is assigned (e.g., where roaringModule = mod and on other code paths that swap implementations around the initError/roaringModule logic) so the availability cache is recomputed for the new module.
🧹 Nitpick comments (1)
src/domain/utils/roaring.js (1)
121-127: Preserve per-tier load failures for actionable diagnostics.Each tier currently swallows errors, so the final throw loses root causes. Keeping the caught exceptions and throwing an
AggregateErrormakes production/runtime failures much easier to debug.🧭 Proposed refactor
-async function tryNativeImport() { +async function tryNativeImport(errors) { try { return /** `@type` {RoaringModule} */ (await import('roaring')); - } catch { + } catch (err) { + errors.push(err); return null; } } -async function tryCjsRequire() { +async function tryCjsRequire(errors) { try { const { createRequire } = await import('node:module'); const req = createRequire(import.meta.url); return /** `@type` {RoaringModule} */ (req('roaring')); - } catch { + } catch (err) { + errors.push(err); return null; } } -async function tryWasmFallback() { +async function tryWasmFallback(errors) { try { const wasmMod = await import('roaring-wasm'); if (typeof wasmMod.roaringLibraryInitialize === 'function') { await wasmMod.roaringLibraryInitialize(); } return adaptWasmApi(/** `@type` {RoaringModule} */ (wasmMod)); - } catch { + } catch (err) { + errors.push(err); return null; } } export async function initRoaring(mod) { ... + const loadErrors = []; const loaded = - (await tryNativeImport()) ?? - (await tryCjsRequire()) ?? - (await tryWasmFallback()); + (await tryNativeImport(loadErrors)) ?? + (await tryCjsRequire(loadErrors)) ?? + (await tryWasmFallback(loadErrors)); if (!loaded) { - throw new Error( - 'Failed to load roaring via import(), require(), and roaring-wasm', - ); + throw new AggregateError( + loadErrors, + 'Failed to load roaring via import(), require(), and roaring-wasm', + ); } ... }Also applies to: 135-142, 150-159, 203-205
🤖 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 121 - 127, The import helpers currently swallow exceptions, losing root causes; update each tier function (e.g., tryNativeImport and the other try*Import functions around the other ranges) to capture the caught Error into a shared errors array instead of simply returning null, and in the final loader (the function that attempts each tier in sequence) throw a single AggregateError(errors, "Failed to load Roaring module") when all tiers fail; keep the existing return types (module or null) for each try*Import but ensure every catch pushes the error into errors so diagnostics are preserved and the final failure surfaces all collected causes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 13-14: The release label in the README ("What's New in v13.0.1")
is inconsistent with the unreleased entry in CHANGELOG.md; update either the
README heading "What's New in v13.0.1" to "Unreleased" (or the correct release
tag) or move the described items from README into the release section in
CHANGELOG.md so both documents reference the same release state; ensure the two
items ("roaring-wasm WASM fallback for Bun/Deno bitmap indexes" and "Dev
dependency security updates") are listed under the identical release heading in
README and CHANGELOG.md.
---
Outside diff comments:
In `@src/domain/utils/roaring.js`:
- Around line 190-197: When reinitializing the roaring module in
initRoaring(mod), the cached nativeAvailability must be cleared so later calls
to getNativeRoaringAvailable() don't return stale values; update initRoaring to
reset nativeAvailability (and keep initError handling) whenever roaringModule is
replaced or a new mod is assigned (e.g., where roaringModule = mod and on other
code paths that swap implementations around the initError/roaringModule logic)
so the availability cache is recomputed for the new module.
---
Nitpick comments:
In `@src/domain/utils/roaring.js`:
- Around line 121-127: The import helpers currently swallow exceptions, losing
root causes; update each tier function (e.g., tryNativeImport and the other
try*Import functions around the other ranges) to capture the caught Error into a
shared errors array instead of simply returning null, and in the final loader
(the function that attempts each tier in sequence) throw a single
AggregateError(errors, "Failed to load Roaring module") when all tiers fail;
keep the existing return types (module or null) for each try*Import but ensure
every catch pushes the error into errors so diagnostics are preserved and the
final failure surfaces all collected causes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f54b7e5a-2de4-444e-a1d1-925125eb1802
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mddocker-compose.test.ymldocker/Dockerfile.bunpackage.jsonsrc/domain/utils/roaring.jsvitest.config.js
🚧 Files skipped from review as they are similar to previous changes (3)
- docker/Dockerfile.bun
- vitest.config.js
- package.json
…rs to GraphTraversal Five new graph algorithms in GraphTraversal, all backed by NeighborProviderPort for memory-efficient traversal: - levels(): longest-path level assignment for DAGs (O(V+E)) - transitiveReduction(): minimal edge set preserving reachability (DAGs) - transitiveClosure(): all implied reachability edges with maxEdges safety - rootAncestors(): find all in-degree-0 ancestors via backward BFS - BFS reverse reachability verification (direction: 'in') Includes LogicalTraversal facade methods, index.d.ts types, cross-provider equivalence tests, 4 new fixtures (F15-F18), and dedicated test files (47 new test cases).
Release Preflight
If you tag this commit as |
Three items for making new graph algorithms memory-efficient on large graphs: - B149: levels() two-pass streaming (O(V+E) → O(V) memory) - B150: transitiveReduction() on-demand neighbor fetch - B151: transitiveClosure() async iterator output
…ff items - B152: async generator traversal API (generalized streaming) - B153: topologicalSort lightweight mode (O(V) memory) - B154: transitiveReduction redundant adjList copy fix - B155: levels() as lightweight --view layout (skip ELK) - B156: structural diff via transitive reduction comparison
…execution order - Restructure Standalone Lane from flat Near-Term table + separate pack sections into 8 priority-grouped sub-tables (P0 Quick Wins through P6 Documentation, plus Uncategorized/Platform) - Replace Standalone Priority Sequence with 6 execution waves mapping concrete implementation order - Add Dependency Chains section with ASCII graph showing B97→B85→B57, B153→B149/B150, B154→B150, B151→B152, B36→test velocity chains - Add effort estimates (XS/S/M/L) and dependency annotations to all items - Mark all milestones complete (M10–M14); update Final Command section - Fix inventory count: 43→45 standalone, 131→133 total tracked - Archive B44, B124, B125, B146, B148 to COMPLETED.md
Release Preflight
If you tag this commit as |
…tier load errors - Reset `nativeAvailability = NOT_CHECKED` in `initRoaring()` when called with a new module or after fresh load, preventing stale cached values from `getNativeRoaringAvailable()` after module swap - Apply `unwrapDefault()` to injected modules in `initRoaring(mod)` path so default-export wrapping is handled consistently - Collect per-tier errors in `tryNativeImport()`, `tryCjsRequire()`, and `tryWasmFallback()` and throw `AggregateError` on total failure instead of a plain Error, preserving root cause diagnostics - Add 3 unit tests: cache reset on reinit, sequential reinit with different module types, and default-export unwrapping
The heading said "v13.0.1" but listed features (new graph algorithms, roaring-wasm fallback) that are in [Unreleased] in CHANGELOG.md. v13.0.1 was only a dev dependency security patch.
Member
Author
CodeRabbit Review Feedback — Round 5 ResponseAddressed (3 fixes)1.
2.
3. README version heading mismatch (Minor,
Dismissed (3 false positives / stale)
|
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/domain/services/LogicalTraversal.js (1)
433-441: Extract repeated multi-start validation into a single helper.The same async loop appears in three methods. Centralizing it will reduce drift risk for
NODE_NOT_FOUNDbehavior.♻️ Proposed refactor
+ async _assertStartsExist(start) { + const starts = Array.isArray(start) ? start : [start]; + for (const s of starts) { + if (!(await this._graph.hasNode(s))) { + throw new TraversalError(`Start node not found: ${s}`, { + code: 'NODE_NOT_FOUND', + context: { start: s }, + }); + } + } + } async levels(start, options = {}) { const { engine, direction, options: opts } = await this._prepareEngine(options); - - const starts = Array.isArray(start) ? start : [start]; - for (const s of starts) { - if (!(await this._graph.hasNode(s))) { - throw new TraversalError(`Start node not found: ${s}`, { - code: 'NODE_NOT_FOUND', - context: { start: s }, - }); - } - } + await this._assertStartsExist(start);Also applies to: 465-473, 497-505
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/LogicalTraversal.js` around lines 433 - 441, Extract the repeated async start-node validation into a single private helper on the LogicalTraversal class (e.g., async _ensureStartNodesExist(start)): normalize start to an array, loop the same await this._graph.hasNode(s) check, and throw the exact same TraversalError(`Start node not found: ${s}`, { code: 'NODE_NOT_FOUND', context: { start: s } }) on missing nodes; then replace the three inline loops in the methods that currently contain that code with await this._ensureStartNodesExist(start). Ensure the helper uses the same this._graph and preserves identical error message/code/context so existing behavior stays unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Line 492: The document text has a mismatch: it states "6 priority tiers" but
lists "P0–P6" (seven tiers); update the sentence so the numeric count matches
the range—either change "6 priority tiers" to "7 priority tiers" or change the
range to "P1–P6" depending on intended semantics; locate and edit the sentence
containing "45 standalone items" and "P0–P6" to ensure the count and range are
consistent.
In `@src/domain/services/GraphTraversal.js`:
- Around line 1268-1273: transitiveClosure can exceed maxNodes during the
second-phase expansion because neighbors are added to nextFrontier without
checking the node cap; modify the expansion loop in transitiveClosure (the block
using visited, nextFrontier, edgeCount) to check the current node count (e.g.,
visited.size or nodesVisited) against maxNodes before adding a neighbor, only
add and mark visited when under the cap, increment the node counter at the point
of adding, and break out of neighbor/loop(s) once maxNodes is reached; apply the
same guard to the similar expansion at the other occurrence (around the block
referenced at 1296) so nodesVisited remains accurate and the algorithm never
expands past maxNodes.
- Around line 1093-1113: In transitiveReduction, the newly created rs (from
this._newRunStats()) is instantiated after calling this.topologicalSort, so the
topologicalSort run statistics are never included in the returned stats; fix by
ensuring stats from topologicalSort are merged into the transitiveReduction
rs—either create rs before calling topologicalSort and pass it into
topologicalSort, or after the call merge the returned topologicalSort stats into
rs (merge keys/counters from the topologicalSort result into rs) so that the
traversal metrics from topologicalSort (and any properties on its returned stats
object) are included before returning rs; reference: transitiveReduction,
topologicalSort, _newRunStats, rs, _neighborEdgeMap.
In `@src/domain/utils/roaring.js`:
- Around line 155-162: The tryWasmFallback function passes the raw import
namespace (wasmMod) from 'roaring-wasm' into adaptWasmApi, which can fail if the
module is wrapped as { default: ... }; update tryWasmFallback to call
unwrapDefault(wasmMod) before passing to adaptWasmApi so adaptWasmApi receives
the actual module (use the existing unwrapDefault helper already used
elsewhere), i.e., locate tryWasmFallback, get the imported wasmMod, run
unwrapDefault(wasmMod) and pass that result into adaptWasmApi instead of
wasmMod.
---
Nitpick comments:
In `@src/domain/services/LogicalTraversal.js`:
- Around line 433-441: Extract the repeated async start-node validation into a
single private helper on the LogicalTraversal class (e.g., async
_ensureStartNodesExist(start)): normalize start to an array, loop the same await
this._graph.hasNode(s) check, and throw the exact same TraversalError(`Start
node not found: ${s}`, { code: 'NODE_NOT_FOUND', context: { start: s } }) on
missing nodes; then replace the three inline loops in the methods that currently
contain that code with await this._ensureStartNodesExist(start). Ensure the
helper uses the same this._graph and preserves identical error
message/code/context so existing behavior stays unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67e34c0b-353e-4126-a741-7ce069556e28
📒 Files selected for processing (16)
CHANGELOG.mdREADME.mdROADMAP.mddocs/ROADMAP/COMPLETED.mdindex.d.tssrc/domain/services/GraphTraversal.jssrc/domain/services/LogicalTraversal.jssrc/domain/utils/roaring.jstest/helpers/fixtureDsl.jstest/unit/domain/services/GraphTraversal.bfs.test.jstest/unit/domain/services/GraphTraversal.crossProvider.test.jstest/unit/domain/services/GraphTraversal.levels.test.jstest/unit/domain/services/GraphTraversal.rootAncestors.test.jstest/unit/domain/services/GraphTraversal.transitiveClosure.test.jstest/unit/domain/services/GraphTraversal.transitiveReduction.test.jstest/unit/domain/utils/roaring.test.js
✅ Files skipped from review due to trivial changes (1)
- test/unit/domain/utils/roaring.test.js
| All milestones are complete: M10 → M12 → M13 (internal) → M11 → M14. M13 wire-format cutover remains deferred by ADR 3 readiness gates. | ||
|
|
||
| M14 HYGIENE is the current priority — test hardening, DRY extraction, and SOLID quick-wins from the HEX_AUDIT. M11 is complete and archived in COMPLETED.md. | ||
| The active backlog is **45 standalone items** sorted into **6 priority tiers** (P0–P6) with **6 execution waves**. Wave 1 (foundation) targets quick wins and test infrastructure. See [Execution Order](#execution-order) for the full sequence. |
There was a problem hiding this comment.
Priority-tier count mismatch.
Line 492 says 6 tiers, but P0–P6 is 7 tiers.
✏️ Suggested fix
-The active backlog is **45 standalone items** sorted into **6 priority tiers** (P0–P6) with **6 execution waves**.
+The active backlog is **45 standalone items** sorted into **7 priority tiers** (P0–P6) with **6 execution waves**.📝 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
| The active backlog is **45 standalone items** sorted into **6 priority tiers** (P0–P6) with **6 execution waves**. Wave 1 (foundation) targets quick wins and test infrastructure. See [Execution Order](#execution-order) for the full sequence. | |
| The active backlog is **45 standalone items** sorted into **7 priority tiers** (P0–P6) with **6 execution waves**. Wave 1 (foundation) targets quick wins and test infrastructure. See [Execution Order](`#execution-order`) for the full sequence. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ROADMAP.md` at line 492, The document text has a mismatch: it states "6
priority tiers" but lists "P0–P6" (seven tiers); update the sentence so the
numeric count matches the range—either change "6 priority tiers" to "7 priority
tiers" or change the range to "P1–P6" depending on intended semantics; locate
and edit the sentence containing "45 standalone items" and "P0–P6" to ensure the
count and range are consistent.
Comment on lines
+1093
to
+1113
| const { sorted, _neighborEdgeMap } = await this.topologicalSort({ | ||
| start, | ||
| direction, | ||
| options, | ||
| maxNodes, | ||
| throwOnCycle: true, | ||
| signal, | ||
| _returnAdjList: true, | ||
| }); | ||
|
|
||
| const rs = this._newRunStats(); | ||
| /** @type {Map<string, string[]>} */ | ||
| const adjList = new Map(); | ||
|
|
||
| // Build adjacency list from topo sort data | ||
| for (const nodeId of sorted) { | ||
| const neighbors = _neighborEdgeMap | ||
| ? (_neighborEdgeMap.get(nodeId) || []) | ||
| : await this._getNeighbors(nodeId, direction, rs, options); | ||
| adjList.set(nodeId, neighbors.map((n) => n.neighborId)); | ||
| } |
There was a problem hiding this comment.
transitiveReduction stats underreport traversal work.
A fresh rs is created after topologicalSort, but topological traversal metrics are not merged, so returned stats can be materially lower than actual work.
📊 Proposed fix
- const { sorted, _neighborEdgeMap } = await this.topologicalSort({
+ const { sorted, _neighborEdgeMap, stats: topoStats } = await this.topologicalSort({
start,
direction,
options,
maxNodes,
throwOnCycle: true,
signal,
_returnAdjList: true,
});
- const rs = this._newRunStats();
+ const rs = {
+ cacheHits: topoStats.cacheHits,
+ cacheMisses: topoStats.cacheMisses,
+ edgesTraversed: topoStats.edgesTraversed,
+ };Also applies to: 1194-1194
🤖 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 1093 - 1113, In
transitiveReduction, the newly created rs (from this._newRunStats()) is
instantiated after calling this.topologicalSort, so the topologicalSort run
statistics are never included in the returned stats; fix by ensuring stats from
topologicalSort are merged into the transitiveReduction rs—either create rs
before calling topologicalSort and pass it into topologicalSort, or after the
call merge the returned topologicalSort stats into rs (merge keys/counters from
the topologicalSort result into rs) so that the traversal metrics from
topologicalSort (and any properties on its returned stats object) are included
before returning rs; reference: transitiveReduction, topologicalSort,
_newRunStats, rs, _neighborEdgeMap.
Comment on lines
+1268
to
+1273
| for (const { neighborId } of neighbors) { | ||
| if (!visited.has(neighborId)) { | ||
| visited.add(neighborId); | ||
| nextFrontier.push(neighborId); | ||
| edgeCount++; | ||
| if (edgeCount > maxEdges) { |
There was a problem hiding this comment.
transitiveClosure can exceed maxNodes during phase-2 expansion.
Line 1269 expands to neighbors even if they were excluded by phase-1 node capping, so work can grow beyond the requested bound and nodesVisited becomes misleading.
🐛 Proposed fix
- for (const { neighborId } of neighbors) {
- if (!visited.has(neighborId)) {
+ for (const { neighborId } of neighbors) {
+ // Enforce phase-1 node boundary (`maxNodes`) for closure expansion.
+ if (!allVisited.has(neighborId)) { continue; }
+ if (!visited.has(neighborId)) {
visited.add(neighborId);
nextFrontier.push(neighborId);
edgeCount++;Also applies to: 1296-1296
🤖 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 1268 - 1273,
transitiveClosure can exceed maxNodes during the second-phase expansion because
neighbors are added to nextFrontier without checking the node cap; modify the
expansion loop in transitiveClosure (the block using visited, nextFrontier,
edgeCount) to check the current node count (e.g., visited.size or nodesVisited)
against maxNodes before adding a neighbor, only add and mark visited when under
the cap, increment the node counter at the point of adding, and break out of
neighbor/loop(s) once maxNodes is reached; apply the same guard to the similar
expansion at the other occurrence (around the block referenced at 1296) so
nodesVisited remains accurate and the algorithm never expands past maxNodes.
Comment on lines
+155
to
+162
| async function tryWasmFallback(errors) { | ||
| try { | ||
| const wasmMod = await import('roaring-wasm'); | ||
| if (typeof wasmMod.roaringLibraryInitialize === 'function') { | ||
| await wasmMod.roaringLibraryInitialize(); | ||
| } | ||
| return adaptWasmApi(/** @type {RoaringModule} */ (wasmMod)); | ||
| } catch (err) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "roaring.js" -type f 2>/dev/null | head -5Repository: git-stunts/git-warp
Length of output: 45
🏁 Script executed:
git ls-files | grep -E "roaring\.js|roaring" | head -20Repository: git-stunts/git-warp
Length of output: 129
🏁 Script executed:
cat -n src/domain/utils/roaring.js | head -180Repository: git-stunts/git-warp
Length of output: 7431
🏁 Script executed:
rg "unwrapDefault" -A 2 -B 2Repository: git-stunts/git-warp
Length of output: 910
Use unwrapDefault() before passing the roaring-wasm module to adaptWasmApi().
Line 161 passes the raw import namespace to adaptWasmApi(). If roaring-wasm arrives wrapped as { default: { RoaringBitmap32 } }, accessing wasmMod.RoaringBitmap32 fails and Tier 3 breaks even though the package loaded successfully. The codebase already has unwrapDefault() (line 175) for this exact scenario and uses it elsewhere; apply it here as well.
🛠️ Proposed fix
async function tryWasmFallback(errors) {
try {
- const wasmMod = await import('roaring-wasm');
+ const wasmNs = /** `@type` {RoaringModule} */ (await import('roaring-wasm'));
+ const wasmMod = unwrapDefault(wasmNs);
if (typeof wasmMod.roaringLibraryInitialize === 'function') {
await wasmMod.roaringLibraryInitialize();
}
- return adaptWasmApi(/** `@type` {RoaringModule} */ (wasmMod));
+ return adaptWasmApi(wasmMod);
} catch (err) {
errors.push(err instanceof Error ? err : new Error(String(err)));
return 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 155 - 162, The tryWasmFallback
function passes the raw import namespace (wasmMod) from 'roaring-wasm' into
adaptWasmApi, which can fail if the module is wrapped as { default: ... };
update tryWasmFallback to call unwrapDefault(wasmMod) before passing to
adaptWasmApi so adaptWasmApi receives the actual module (use the existing
unwrapDefault helper already used elsewhere), i.e., locate tryWasmFallback, get
the imported wasmMod, run unwrapDefault(wasmMod) and pass that result into
adaptWasmApi instead of wasmMod.
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
test(name, fn, { timeout })signatures to Vitest 4'stest(name, { timeout }, fn)form across 7 test filesvi.fn().mockImplementation(() => ...)constructor mocks to usefunctionexpressions per Vitest 4 requirementsCombined with v13.0.1 (which fixed the 4 high-severity findings), the repo now has 0 npm audit vulnerabilities.
Test plan
npm auditreturns 0 vulnerabilitiesSummary by CodeRabbit
New Features
Tests
Chores
Documentation