Skip to content

Upgrade reported Node.js version to 26.3.0 - #31818

Closed
Jarred-Sumner wants to merge 29 commits into
mainfrom
claude/upgrade-nodejs-26
Closed

Upgrade reported Node.js version to 26.3.0#31818
Jarred-Sumner wants to merge 29 commits into
mainfrom
claude/upgrade-nodejs-26

Address review feedback: escape-slot lifetime, set-cookie consistency…

7f3e8d3
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jun 5, 2026 in 26m 55s

Code review found 3 important issues

Found 5 candidates, confirmed 4. See review comments for details.

Details

Severity Count
🔴 Important 3
🟡 Nit 0
🟣 Pre-existing 0
Severity File:Line Issue
🔴 Important scripts/build/deps/nodejs-headers.ts:20 duckdb-basic-usage.test.ts not gated for ABI 147
🔴 Important test/napi/node-napi-tests/harness.ts:17 dlopen-*.test.ts CI failures: V8 addons built without C++20 / CC-cleanup env
🔴 Important src/jsc/bindings/v8/V8HandleScope.cpp:65-70 Bun-pushed ~HandleScope leaves HandleScopeData stale, causing deleteGrantsBack to over-sweep parent grants

Annotations

Check failure on line 20 in scripts/build/deps/nodejs-headers.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

duckdb-basic-usage.test.ts not gated for ABI 147

The `NODE_MODULE_VERSION` 137→147 bump breaks `test/js/third_party/duckdb/duckdb-basic-usage.test.ts` on every non-skipped platform (9 variants in build #60774, not listed under "Known-unrelated failures"). The pinned `duckdb@1.3.1` package uses `@mapbox/node-pre-gyp`, which keys prebuilt downloads on `process.versions.modules`; there's no `node-v147` prebuilt for that release, so install falls through to a source build that fails in CI. Add an ABI-147 skip alongside the existing musl / win32-ar

Check failure on line 17 in test/napi/node-napi-tests/harness.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

dlopen-*.test.ts CI failures: V8 addons built without C++20 / CC-cleanup env

The two `test/js/node/process/dlopen-*.test.ts` files build inline V8-API addons via node-gyp with plain `bunEnv`, and were not updated for the Node 26 header bump — they're failing in Build #60774 on macOS (aarch64/x64) and Windows (x64/x64-baseline/aarch64), and aren't in the known-unrelated list. They need the same env treatment this PR already applies in `test/v8/v8.test.ts` and here in `node-napi-tests/harness.ts`: delete `CC`/`CXX` from the env, add `-std=gnu++20` / `-std=c++20` to `CXXFLA

Check failure on line 70 in src/jsc/bindings/v8/V8HandleScope.cpp

See this annotation in the file changed.

@claude claude / Claude Code Review

Bun-pushed ~HandleScope leaves HandleScopeData stale, causing deleteGrantsBack to over-sweep parent grants

The Bun-pushed branch of `~HandleScope` (lines 65-70) pops the Bun scope stack and clears its buffer but never restores the isolate's `HandleScopeData`. If `Extend` ran while this scope was current (e.g. an addon's inline `Local::New` inside the `Array::Iterate` callback), `data->limit` is left pointing into the now-cleared buffer; the next addon-side inline `v8::HandleScope` snapshots that stale limit as `prev_limit_`, and on close `DeleteExtensions` → `deleteGrantsBack(stale_limit)` runs again