fix(deps): upgrade vitest 2→4, resolve all npm audit findings#63
Open
flyingrobots wants to merge 10 commits intomainfrom
Open
fix(deps): upgrade vitest 2→4, resolve all npm audit findings#63flyingrobots wants to merge 10 commits intomainfrom
flyingrobots wants to merge 10 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).
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughBumps Vitest to v4; centralizes or adjusts test timeouts (suite-level or per-test) across many unit tests; converts several mock factories from arrow to function declarations; adds Vitest externals for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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
| "typescript": "^5.9.3", | ||
| "typescript-eslint": "^8.54.0", | ||
| "vitest": "^2.1.8" | ||
| "vitest": "^4.0.18" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd '^package-lock\.json$' -a
python - <<'PY'
import json, os
lock_path = "package-lock.json"
if not os.path.exists(lock_path):
print("RESULT: package-lock.json not found in this PR branch")
raise SystemExit(0)
with open(lock_path, "r", encoding="utf-8") as f:
lock = json.load(f)
pkgs = lock.get("packages", {})
root = pkgs.get("", {})
print("root.devDependencies.vitest =", root.get("devDependencies", {}).get("vitest"))
for dep in ["vitest", "vite", "vite-node", "@vitest/mocker", "esbuild"]:
key = f"node_modules/{dep}"
print(f"{dep} locked version =", pkgs.get(key, {}).get("version"))
PYRepository: git-stunts/git-warp
Length of output: 299
Lockfile is missing resolved version for vite-node.
The package-lock.json has been updated with most Vitest toolchain dependencies (vitest → 4.0.18, vite → 7.3.1, @vitest/mocker → 4.0.18, esbuild → 0.27.3), but vite-node is not present in the resolved lockfile. Ensure vite-node is also locked to a compatible version to complete the Vitest upgrade.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at 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.
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
CHANGELOG.md
Outdated
|
|
||
| ### Fixed | ||
|
|
||
| - **Roaring native module loading under Bun** — `initRoaring()` now catches dynamic `import('roaring')` failures and falls back to `createRequire()` for direct `.node` binary loading. Bun Dockerfile updated to install `nodejs`, `python3`, and `ca-certificates` so that `node-pre-gyp` can download or compile the roaring native binary (Bun reports a fictional Node ABI v137 with no prebuilt available). Fixes `test-bun` CI failures caused by missing native binary after Vitest 4 upgrade invalidated Docker layer cache. |
There was a problem hiding this comment.
Update release note wording to match the current Docker implementation.
Line 12 says the Bun Dockerfile installs nodejs, python3, and ca-certificates, but the current approach is a multi-stage build with a separate Node builder image. The note should reflect that to avoid operator confusion.
Proposed fix
-- **Roaring native module loading under Bun** — `initRoaring()` now catches dynamic `import('roaring')` failures and falls back to `createRequire()` for direct `.node` binary loading. Bun Dockerfile updated to install `nodejs`, `python3`, and `ca-certificates` so that `node-pre-gyp` can download or compile the roaring native binary (Bun reports a fictional Node ABI v137 with no prebuilt available). Fixes `test-bun` CI failures caused by missing native binary after Vitest 4 upgrade invalidated Docker layer cache.
+- **Roaring native module loading under Bun** — `initRoaring()` now catches dynamic `import('roaring')` failures and falls back to `createRequire()` for direct `.node` binary loading. Bun Dockerfile now uses a Node.js builder stage to run `node-pre-gyp` and then copies roaring native artifacts into the Bun runtime image (Bun reports a fictional Node ABI v137 with no prebuilt available). Fixes `test-bun` CI failures caused by missing native binary after Vitest 4 upgrade invalidated Docker layer cache.📝 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
| - **Roaring native module loading under Bun** — `initRoaring()` now catches dynamic `import('roaring')` failures and falls back to `createRequire()` for direct `.node` binary loading. Bun Dockerfile updated to install `nodejs`, `python3`, and `ca-certificates` so that `node-pre-gyp` can download or compile the roaring native binary (Bun reports a fictional Node ABI v137 with no prebuilt available). Fixes `test-bun` CI failures caused by missing native binary after Vitest 4 upgrade invalidated Docker layer cache. | |
| - **Roaring native module loading under Bun** — `initRoaring()` now catches dynamic `import('roaring')` failures and falls back to `createRequire()` for direct `.node` binary loading. Bun Dockerfile now uses a Node.js builder stage to run `node-pre-gyp` and then copies roaring native artifacts into the Bun runtime image (Bun reports a fictional Node ABI v137 with no prebuilt available). Fixes `test-bun` CI failures caused by missing native binary after Vitest 4 upgrade invalidated Docker layer cache. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at 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.
…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 |
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