node:http2: populate internal/http2/util for --expose-internals tests [1tpjlb] - #29825
node:http2: populate internal/http2/util for --expose-internals tests [1tpjlb]#29825robobun wants to merge 1 commit into
Conversation
WalkthroughThe PR refactors HTTP/2 Refactor and Test Alignment
x64 Baseline Static Verification
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:11 AM PT - Jul 2nd, 2026
❌ @robobun, your commit 64f1420 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 29825That installs a local version of the PR into your bun-29825 --bun |
Status: closed, superseded on mainEverything this PR set out to do for node-compat group Nothing needs to be carried forward from this branch. Previous status (historical)Before being superseded, this PR reached 10/14 passing for the group with a 7-file diff (http2 |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re duplicate detection: #29823 (also mine, group
The http2 util module, the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/ErrorCode.cpp`:
- Around line 590-600: The code does not guard for expected_types.size() == 0
which causes unsigned underflow when the later code uses length - 1; add an
early check for length == 0 after computing unsigned length and handle it by
calling ERR_INVALID_ARG_TYPE with a safe empty/placeholder expected-type (e.g.,
an empty string or "[]") so no index math or toString() is performed; modify the
branch that currently handles length == 1 / length - 1 to first return when
length == 0 using the same ERR_INVALID_ARG_TYPE path so expected_types and any
subsequent indexing are never accessed when empty.
In `@src/js/internal/http2/util.ts`:
- Around line 559-562: The function assertWithinRange allows NaN because typeof
NaN === "number" and comparisons with NaN are false; update assertWithinRange to
explicitly reject NaN by adding a check (e.g., Number.isNaN(value)) to the
existing conditional so that any NaN value triggers the throw of
$ERR_HTTP2_INVALID_SETTING_VALUE_RangeError with the same message; locate the
function assertWithinRange and augment its if condition to include the NaN test.
In `@src/js/internal/test/binding.ts`:
- Around line 17-20: internalBinding currently returns an empty object when a
binding is missing which masks real missing/native failures; change the fallback
so that when bindings[name] is undefined the function calls and returns
process.binding(name) (preserving the existing behavior of omitting argument
validation), i.e., locate the internalBinding function and replace the final
"return {}" with "return process.binding(name)" so unknown bindings fall through
to Node's native process.binding.
In `@test/js/node/child_process/child_process-node.test.js`:
- Around line 674-678: The test currently asserts the full error.message string
using arg?.toString(), but Node formats the received value with its own
determineSpecificType rendering so the exact text can differ; update the
assertion for fork(...) to instead check error.code === "ERR_INVALID_ARG_TYPE"
and error.name === "TypeError" and assert the message matches a looser pattern
(e.g. use a RegExp like /^The "args" argument must be an instance of Array\.
Received .+/) rather than comparing to arg?.toString() so the test no longer
depends on determineSpecificType formatting.
In `@test/js/node/test/parallel/test-http2-misc-util.js`:
- Around line 19-21: The test's forEach loop is calling sessionName(2) instead
of using the loop variable i, so only the value 2 is actually tested; update the
assertion to call sessionName(i) and keep the rest of the test unchanged (the
loop over [2, '', 'test', {}, [], true] should assert
strictEqual(sessionName(i), '<invalid>') to validate all invalid inputs);
reference: sessionName and the forEach callback variable i in
test-http2-misc-util.js.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ee828b5-00c6-42d7-bad8-370f930c25ff
📒 Files selected for processing (25)
src/bun.js/HardcodedModule.zigsrc/bun.js/ModuleLoader.zigsrc/bun.js/bindings/ErrorCode.cppsrc/bun.js/bindings/ErrorCode.tssrc/js/internal/http2/core.tssrc/js/internal/http2/util.tssrc/js/internal/test/binding.tssrc/js/node/http2.tstest/js/node/child_process/child_process-node.test.jstest/js/node/http2/node-http2.test.jstest/js/node/test/parallel/test-assert.jstest/js/node/test/parallel/test-buffer-concat.jstest/js/node/test/parallel/test-child-process-constructor.jstest/js/node/test/parallel/test-dns-setservers-type-check.jstest/js/node/test/parallel/test-http2-createserver-options.jstest/js/node/test/parallel/test-http2-misc-util.jstest/js/node/test/parallel/test-http2-server-socket-destroy.jstest/js/node/test/parallel/test-http2-util-assert-valid-pseudoheader.jstest/js/node/test/parallel/test-http2-util-asserts.jstest/js/node/test/parallel/test-http2-util-headers-list.jstest/js/node/test/parallel/test-http2-util-nghttp2error.jstest/js/node/test/parallel/test-http2-util-update-options-buffer.jstest/js/node/test/parallel/test-process-hrtime.jstest/js/node/test/parallel/test-process-setgroups.jstest/js/web/workers/message-event.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
7e457a8 to
3c643a5
Compare
3c643a5 to
1969eee
Compare
robobun
changed the title
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/bindings/ErrorCode.ts:102— InsertingERR_HTTP2_INVALID_CONNECTION_HEADERSmid-array (alphabetical position) violates the trailing comment in this file — discriminants are index-aligned with the checked-insrc/jsc/ErrorCode.rs, which this PR doesn't touch. Every RustErrorCodeconstant ≥86 is now off-by-one against the regenerated C++errors[]table andErrorCode::COUNT(324) no longer matches; this is what's failingbuild-ruston all 8 platforms. Either append the entry afterERR_PROXY_TUNNELat the end of the array, or regeneratesrc/jsc/ErrorCode.rsin this PR.Extended reasoning...
What the bug is
ERR_HTTP2_INVALID_CONNECTION_HEADERSis inserted at line 102 ofsrc/jsc/bindings/ErrorCode.ts, betweenERR_HTTP2_INFO_STATUS_NOT_ALLOWEDandERR_HTTP2_INVALID_HEADER_VALUE(the alphabetical position). However, the trailing comment in this same file (lines 334-335) explicitly states:// Appended (not alphabetical): discriminants are index-aligned with the checked-in Rust mirror (src/jsc/ErrorCode.rs) — only ever append here.The mid-array insertion shifts the index of every subsequent entry by +1, breaking the index alignment with
src/jsc/ErrorCode.rs, which is not regenerated in this PR.The specific code path that triggers it
There are two consumers of this array, with different update mechanisms:
- C++ side (
ErrorCode+List.h/ theerrors[]table): regenerated at build time fromErrorCode.tsbysrc/codegen/generate-node-errors.ts. After this PR it will have 325 entries withERR_HTTP2_INVALID_CONNECTION_HEADERSat index 86 and emitNODE_ERROR_COUNT = 325. - Rust side (
src/jsc/ErrorCode.rs): checked-in, not build-generated. Its header (lines 3-8) says "Discriminants MUST stay index-aligned with the C++ errors[] table so Bun__createErrorWithCode picks the correct ctor / name / code triple. Regenerate by re-running the inline extractor against ErrorCode.ts; do not hand-edit individual entries." At HEAD it still has:HTTP2_INFO_STATUS_NOT_ALLOWED = ErrorCode(85)immediately followed byHTTP2_INVALID_HEADER_VALUE = ErrorCode(86)— no entry for the new codepub const COUNT: u16 = 324CODE_STR: [&str; ErrorCode::COUNT as usize]going straight from"ERR_HTTP2_INFO_STATUS_NOT_ALLOWED"to"ERR_HTTP2_INVALID_HEADER_VALUE"
The PR's changed-files list does not include
src/jsc/ErrorCode.rs, andgit log -- src/jsc/ErrorCode.rsshows it was last touched before this PR.Why existing code doesn't prevent it
The C++ generator runs every build, so it picks up the new entry automatically. The Rust mirror does not — it must be regenerated manually via the inline extractor mentioned in its header. The only guard is the in-source comment in
ErrorCode.tstelling contributors to append, which was overlooked here. The robobun CI comment showsbuild-rustfailing on all 8 platforms (linux x64/aarch64/musl/asan/baseline/android, macOS x64/aarch64) for commit 1969eee, consistent with a count/alignment static assertion firing during the Rust build.Step-by-step proof
- Before this PR,
ErrorCode.tsindex 85 =ERR_HTTP2_INFO_STATUS_NOT_ALLOWED, index 86 =ERR_HTTP2_INVALID_HEADER_VALUE.ErrorCode.rsmirrors this:ErrorCode(85)/ErrorCode(86). Both sides have 324 entries. - This PR inserts
["ERR_HTTP2_INVALID_CONNECTION_HEADERS", TypeError]at TS index 86. Now TS index 86 =ERR_HTTP2_INVALID_CONNECTION_HEADERS, index 87 =ERR_HTTP2_INVALID_HEADER_VALUE, …, and the array has 325 entries. generate-node-errors.tsregenerates the C++errors[]table to match: C++ index 86 →ERR_HTTP2_INVALID_CONNECTION_HEADERS,NODE_ERROR_COUNT = 325.ErrorCode.rsis unchanged: Rust still saysHTTP2_INVALID_HEADER_VALUE = ErrorCode(86)andCOUNT = 324.- Any Rust caller passing
ErrorCode::HTTP2_INVALID_HEADER_VALUE(value 86) intoBun__createErrorWithCodewould now get anERR_HTTP2_INVALID_CONNECTION_HEADERSinstance — and likewise every one of the ~238 codes from index 86 onward is off by one. TheCOUNTmismatch (324 vs 325) trips the build-time check, which is whybuild-rustis red across the board.
Impact
The PR cannot build on any platform (all 8
build-rustjobs fail). If the count check were somehow bypassed, every Rust-originated error from index 86 onward would surface with the wrong.code/ constructor / name at runtime.Fix
Either of:
- Append instead of insert: move the new entry to the end of the array, after
["ERR_PROXY_TUNNEL", Error](in the section already marked "Appended (not alphabetical)"). This requires no Rust change. - Regenerate the Rust mirror: re-run the inline extractor against
ErrorCode.tsand include the updatedsrc/jsc/ErrorCode.rs(with the new entry at 86, all subsequent discriminants +1,COUNT = 325, and the newCODE_STRrow) in this PR.
The first option is the minimal change and keeps the file's stated invariant.
- C++ side (
|
Already fixed in 5353e3e (the review landed against the older diff). |
5353e3e to
a4d4ba5
Compare
There was a problem hiding this comment.
No issues found and CI is green; deferring to a human since the ERR_INVALID_ARG_TYPE wording change in ErrorCode.cpp is a cross-cutting behaviour change worth a second pair of eyes.
Extended reasoning...
Overview
This PR populates the internal/http2/util and internal/test/binding shims that Node's --expose-internals tests need, moving 10 http2 tests off the expectations FAIL list. It touches: src/js/node/http2.ts (~290 added lines of test-only helpers — buildNgHeaderString, NghttpError, nghttp2ErrorString, updateOptionsBuffer/optionsBuffer, etc., exposed via the Symbol.for('::bunhttp2internals::') slot); src/jsc/bindings/ErrorCode.cpp (reworks single-type ERR_INVALID_ARG_TYPE formatting to emit "an instance of X" for CamelCase class names and lowercased "of type object/function" for Object/Function, plus a length==0 underflow guard); ErrorCode.ts/ErrorCode.rs (append ERR_HTTP2_INVALID_CONNECTION_HEADERS and bump COUNT); the test-common Bun.plugin shim; and ~8 test files updating expected error wording.
Security risks
None identified. The new helpers are only reachable via the --expose-internals test shim (a Bun.plugin in test/js/node/test/common/index.js), not via the public node:http2 surface. The C++ change is purely error-message formatting; the new length==0 guard removes a latent unsigned-underflow OOB read in the ArgList overload.
Level of scrutiny
Moderate. The http2 additions are low-risk test infrastructure, but the ErrorCode.cpp change is a cross-cutting behaviour change to a globally-used error formatter — every caller that passes a single CamelCase identifier ("Array", "RegExp", "MessagePort", "Date", …) now produces different message text. The isSingleClassName heuristic looks correct and matches Node's /^(?:[A-Z][a-z0-9]*)+$/ classifier, and CI (build 63274) is fully green with the updated test expectations, but the blast radius is wide enough that a maintainer should sign off on the wording policy. The index-aligned ErrorCode.ts/ErrorCode.rs append also deserves a quick eyeball since misalignment there silently shifts every subsequent error code.
Other factors
All prior review threads (CodeRabbit's underflow guard, process.binding fallthrough, my dead-binding nits, and the nghttp2_strerror table mismatch) are resolved. The most recent commit (2161ddc) re-transcribed the nghttp2 error table directly from nghttp2_helper.c per my last comment. The bug-hunting system found nothing on this revision. The PR description is thorough and the remaining 4 still-failing tests are clearly scoped out as runtime-behaviour follow-ups.
de86af6 to
7462c34
Compare
There was a problem hiding this comment.
No issues found in this pass; deferring because the ERR_INVALID_ARG_TYPE single-type formatter rewrite in ErrorCode.cpp is a production-path change (affects error wording for every caller passing a bare class name), so it's worth a human sanity-check alongside the http2 internals additions.
Extended reasoning...
Overview
This PR populates the Symbol.for("::bunhttp2internals::").util / .binding slots in src/js/node/http2.ts (~300 new lines of helpers ported from Node's internal/http2/util: buildNgHeaderString, NghttpError, nghttp2ErrorString, updateOptionsBuffer, pseudo-header validators, etc.), adds the ERR_HTTP2_INVALID_CONNECTION_HEADERS error code (ErrorCode.ts/.rs), rewrites the single-type branch of ERR_INVALID_ARG_TYPE in ErrorCode.cpp to match Node's wording ("an instance of X" for CamelCase identifiers, "of type object/function" for Object/Function), wires an internal/test/binding shim into the test-common Bun.plugin, and removes 10 http2 tests from test/expectations.txt. Test expectation strings in test-assert.js and message-event.test.ts are updated for the new wording.
Security risks
None identified. The new http2 helpers are reachable only via the --expose-internals symbol slot and the test-harness plugin shim; they don't alter request handling, auth, or socket paths. The ErrorCode.cpp change is pure message formatting. The internalBinding shim falls through to process.binding(name), which is already a privileged surface gated the same way.
Level of scrutiny
Medium. Most of the diff is test-only infrastructure, but two pieces run in production: (1) the ERR_INVALID_ARG_TYPE formatter in ErrorCode.cpp — every caller that passes a single CamelCase type name (e.g. "Array", "RegExp", "MessagePort", "Date") now gets "an instance of X" instead of "of type X", which is a user-visible wording change across the runtime; and (2) the two new http2.constants entries. The C++ change is small and well-scoped (the isSingleClassName predicate plus a 4-way branch), but it's the kind of cross-cutting wording change a maintainer should sign off on rather than a bot.
Other factors
All prior review threads (CodeRabbit and my three earlier nits — unused destructures, dead kIsNodeError getter, mis-keyed kNghttp2ErrorMessages table) are resolved. The author reports 10/14 target tests passing with the rest blocked on unrelated runtime gaps, and CI is green modulo known flakes. The bug-hunting pass on the current revision found nothing. I'm deferring rather than approving solely because of the production-path ErrorCode.cpp change and overall diff size, not because of any open concern.
|
Adding scope detail on the Why it is needed. Two of the ten target tests assert on it directly:
Before this PR, Bun's single- Blast radius. Call sites in
Every one of these is a case where Node already says In Bun's own test suite, exactly two files asserted the old wording and both are updated in this PR: |
7462c34 to
17d5b44
Compare
There was a problem hiding this comment.
No issues found in the current revision. Deferring to a human reviewer because the ERR_INVALID_ARG_TYPE single-type formatter change in ErrorCode.cpp is a cross-cutting production-path change (the scope analysis is thorough and it moves toward Node, but error-message wording shifts across ~40 call sites are worth a human sign-off), and the ~290 new lines in http2.ts add non-trivial header/options logic alongside the test-only exports.
Extended reasoning...
Overview
This PR populates the previously-empty Symbol.for("::bunhttp2internals::")].util and adds a .binding slot in src/js/node/http2.ts so that ten Node --expose-internals http2 tests can resolve internal/http2/util and internalBinding('http2'). It adds ~290 lines of helpers to http2.ts (buildNgHeaderString, updateOptionsBuffer/optionsBuffer, NghttpError, the kNghttp2ErrorMessages table, sessionName, assertWithinRange, pseudo-header/connection-header validators, isPayloadMeaningless), wires internal/test/binding into the test-common Bun.plugin shim, adds two NGHTTP2_NV_FLAG_* constants, registers ERR_HTTP2_INVALID_CONNECTION_HEADERS in ErrorCode.{ts,rs}, and removes ten tests from test/expectations.txt.
The one production-path change is to the single-StringView overload of ERR_INVALID_ARG_TYPE in src/jsc/bindings/ErrorCode.cpp: "Object"/"Function" now lowercase to of type object/function, and a bare CamelCase identifier renders as an instance of X. Two test files (test-assert.js, message-event.test.ts) are updated for the new wording.
Security risks
None identified. The new http2 helpers are exposed only via the Symbol.for("::bunhttp2internals::") slot and the test-only Bun.plugin shim gated behind --expose-internals; none of them are wired into the runtime request path. buildNgHeaderString performs header validation but is not called by the http2 engine itself in this PR. The ErrorCode.cpp change is pure string formatting with no parsing of untrusted input.
Level of scrutiny
Medium-high. Most of the diff is test-infrastructure (the .util/.binding exports, the Bun.plugin shim entry, expectations removals), which is low-risk and well-covered by the ten newly-passing Node tests. However, the ERR_INVALID_ARG_TYPE formatter rewording is a user-observable behaviour change that fans out to ~40 call sites across JS builtins and C++ bindings (ReadableStream, AbortSignal, Array, KeyObject, etc.). The author's blast-radius comment is thorough — every affected site moves toward Node's wording, and a grep confirms no remaining test depends on the old rendering — but a wording shift this broad on a hot error path deserves a human sign-off rather than a bot approval.
Other factors
The PR has been through several review rounds with all threads resolved: an unsigned-underflow guard added to the ArgList overload, internalBinding falling through to process.binding, unused destructure cleanup, and a full re-transcription of the kNghttp2ErrorMessages table after I flagged off-by-one and mis-keyed entries. The bug-hunting system found nothing in the current revision. CI on the latest commit (17d5b44) is building; the previous red run was attributed to an unrelated main-branch Windows sourcemap issue since fixed by #32797. Given the breadth of new logic in http2.ts plus the cross-cutting error-formatter change, I'm deferring rather than approving.
|
Two points to close out the remaining review surface, both new relative to the earlier scope comment. The To make that concrete rather than inferred: Bun's vendored message: 'The "regexp" argument must be an instance of RegExp. ' +
"Received type string ('string')"so this edit makes Bun's copy match upstream byte for byte on that line (1549 is identical). None of the ~290 new lines in |
55d2a00 to
b9bee93
Compare
|
For anyone orienting from the CodeRabbit walkthrough: its summary describes a revision that is several force-pushes stale (its incremental-review footer reported the range
The actual file list at |
There was a problem hiding this comment.
No issues found. The bulk of this is test-only --expose-internals infrastructure filling the ::bunhttp2internals:: slots, but the ERR_INVALID_ARG_TYPE single-type formatter change in ErrorCode.cpp is a production-path C++ change that rewords ~40 call sites — the blast-radius analysis in the thread looks solid, but that's the piece worth a human sanity check.
Extended reasoning...
Overview
This PR populates the previously-empty Symbol.for("::bunhttp2internals::")].util and adds a .binding slot in src/js/node/http2.ts (~290 new lines: buildNgHeaderString, updateOptionsBuffer/optionsBuffer, NghttpError, nghttp2ErrorString, sessionName, pseudo-header validators, etc.), wires internalBinding('http2') in src/js/internal/test/binding.ts, appends ERR_HTTP2_INVALID_CONNECTION_HEADERS to ErrorCode.ts/ErrorCode.rs, and — the one production-path piece — refactors the single-StringView overload of ERR_INVALID_ARG_TYPE in src/jsc/bindings/ErrorCode.cpp so bare class names render as "an instance of X" and Object/Function lowercase to "of type object/function" (matching Node and the existing ArgList overload). Ten node-parallel http2 tests move off test/expectations.txt; two Bun-authored tests and one vendored-Node test are updated to the corrected wording.
Security risks
None identified. The new http2 helpers are only reachable via the ::bunhttp2internals:: symbol under --expose-internals (test-only). buildNgHeaderString validates header tokens and rejects connection-specific headers but is not wired into any request path yet. The ErrorCode.cpp change is pure string-formatting of an already-thrown error message; no new user-input parsing, no allocation-size arithmetic, no auth/crypto surface.
Level of scrutiny
Medium. Most of the diff is test-only infrastructure ported from Node's lib/internal/http2/util.js and is exercised by the 10 newly-enabled node-parallel tests. The part warranting human attention is the ERR_INVALID_ARG_TYPE formatter: it's shared C++ that changes user-visible error text at ~40 call sites (ReadableStream, Array, AbortSignal, KeyObject, etc.). The author's blast-radius comment enumerates every affected caller and confirms all move toward Node's wording, and a grep of test/ for the old rendering came back empty after the two in-PR test updates — but a cross-cutting error-message change in native code is exactly the sort of thing a human should skim rather than auto-approve.
Other factors
All prior review threads (CodeRabbit's empty-array underflow guard, process.binding fallthrough, my nits on unused destructures / kIsNodeError / the mis-keyed kNghttp2ErrorMessages table) are resolved with commits. CI is reported green (283/283 test lanes; remaining failures were a broken darwin runner). The ErrorCode.rs/ErrorCode.ts append is index-aligned at 331 with COUNT=332. The test-assert.js edit was verified against upstream Node v26.3.0 as a byte-for-byte revert to upstream wording. This is a solid, well-documented Node-compat PR — just larger and more cross-cutting than the auto-approve bar.
b9bee93 to
1788fb9
Compare
|
Closing out the one thing claude[bot] deferred on across its last three passes: the Node's The diff is now 7 files: the http2 helpers filling the Teaching the ~35 existing single-string callers Node's wording is a real compat improvement but a separate cleanup, not something this PR should carry. |
There was a problem hiding this comment.
No blockers found, but this adds ~320 lines to src/js/node/http2.ts (including a production-path change to assertIsObject/assertIsArray error routing and two new public http2.constants), so it's worth a human sign-off from someone familiar with the http2 module.
Extended reasoning...
Overview
Populates the previously-empty Symbol.for("::bunhttp2internals::")].util and adds a .binding slot on node:http2 so that 10 upstream Node --expose-internals http2 tests can resolve internal/http2/util and internalBinding('http2'). Seven files touched: src/js/node/http2.ts (~320 new lines: buildNgHeaderString, updateOptionsBuffer/optionsBuffer, NghttpError, nghttp2ErrorString, sessionName, assertWithinRange, isIllegalConnectionSpecificHeader, pseudo-header validators, isPayloadMeaningless, plus the exported util/binding objects), src/js/internal/test/binding.ts (adds the http2 case), src/jsc/ErrorCode.{rs,ts} (appends ERR_HTTP2_INVALID_CONNECTION_HEADERS), test/expectations.txt (removes 10 FAIL entries), test/js/node/http2/node-http2.test.js (asserts two new constants), and test/js/node/test/common/index.js (drops the superseded NghttpError stub).
Two changes reach production paths rather than test-only surface: (1) assertIsObject/assertIsArray now wrap their types argument in a one-element array so the throw routes through the array overload of $ERR_INVALID_ARG_TYPE (which applies Node's type-vs-instance classification), altering error-message wording for existing callers; and (2) NGHTTP2_NV_FLAG_NONE/NGHTTP2_NV_FLAG_NO_INDEX are added to the public http2.constants object.
Security risks
None identified. All new code is pure JS with no I/O, crypto, auth, or filesystem access. buildNgHeaderString validates header names via the existing checkIsHttpToken and rejects connection-specific headers, but it is only reachable via the internal symbol and is not wired into Bun's own request path. optionsBuffer is a module-level Uint32Array that mirrors Node's layout for test parity only; nothing native reads it.
Level of scrutiny
Medium. The bulk of the addition is Node-compat test scaffolding gated behind a private symbol, which is low-risk. However, the file is a production node:* module, the assertIsObject/assertIsArray reroute changes user-observable error text on real API paths, and buildNgHeaderString is ~70 lines of non-trivial header-processing logic ported from Node — enough surface that a maintainer familiar with the http2 subsystem should confirm it matches upstream semantics and doesn't regress any Bun-specific behaviour.
Other factors
The bug-hunting system found nothing on the current head. All prior review threads (CodeRabbit's empty-array/underflow, NaN-in-range, fallthrough; my three earlier nits on unused destructures, the unreachable kIsNodeError getter, and the mis-keyed kNghttp2ErrorMessages table) are resolved — though several of those were against files/revisions that no longer exist in the current 7-file diff. CI on the previous head was 283/283 green with only unrelated darwin infra timeouts. I verified that every identifier referenced by the new buildNgHeaderString (ArrayIsArray, ObjectKeys, SafeSet, checkIsHttpToken, kSingleValueHeaders, sensitiveHeaders) is already defined in the file. Note the PR description and the most recent status comment still reference ErrorCode.cpp, test-assert.js, and message-event.test.ts, which are not in the current diff — the assertIsObject/assertIsArray array-wrap approach appears to have superseded the single-string formatter change, making those edits unnecessary.
|
Making the one thing this review hands to a human quick to verify. The phrase "production-path change to
So the entire production-visible effect of the "error routing" change is one message, on one option, moved to Node's exact text. Everything else renders byte-for-byte the same. One small correction to the review's "Other factors" note: the PR description and status comment do mention |
… [1tpjlb]
Fills the Symbol.for('::bunhttp2internals::').util and .binding slots (landed
empty by #31584) with the helpers Node's internal/http2/util exports, so the
ported node tests that require('internal/http2/util') exercise Bun's real
implementation: assertValidPseudoHeader{,Response,Trailer}, assertWithinRange,
buildNgHeaderString, isIllegalConnectionSpecificHeader, NghttpError +
nghttp2ErrorString, sessionName, updateOptionsBuffer + optionsBuffer,
isPayloadMeaningless.
internalBinding('http2') is served by the real internal/test/binding built-in
(added by #31824), not by a test-common plugin shim; the shim's superseded
virtual module is deleted.
assertIsObject/assertIsArray now pass their expected type as a one-element
list. Node's ERR_INVALID_ARG_TYPE normalizes a string to a list internally;
Bun's single-string overload renders its input literally (for pre-formatted
phrases), so the list form is what selects Node's type classification. One
call site already used it. No C++ changes.
Also: ERR_HTTP2_INVALID_CONNECTION_HEADERS (appended, so the checked-in
ErrorCode.rs mirror stays index-aligned), and NGHTTP2_NV_FLAG_NONE /
NGHTTP2_NV_FLAG_NO_INDEX in http2.constants.
Enables 10 node-compat parallel tests previously marked [ FAIL ].
1788fb9 to
64f1420
Compare
|
Closing: this PR has been superseded on main. While resolving the latest merge conflicts (main is ~1074 commits ahead of this branch), I found that everything this PR set out to do for node-compat group Verified against a bare build of main
Per file:
Nothing here needs to be carried forward. Thanks to the reviewers who pushed back on the cross-cutting |
1 participant
Fixes 10 of the 14 tests in node-compat group
1tpjlb: the Node--expose-internalstests thatrequire('internal/http2/util').Problem
These tests import Node's private
internal/http2/utilmodule for its helpers (buildNgHeaderString,NghttpError,updateOptionsBuffer, the pseudo-header validators, ...). #31584 added aSymbol.for("::bunhttp2internals::")slot on thenode:http2default export and a test-commonBun.pluginshim that servesinternal/http2/utilfrom it, but landed theutilsub-object empty, so every test fails on the import.Fix
src/js/node/http2.ts: add theinternal/http2/utilhelpers, ported from Node'slib/internal/http2/util.js, and populate theSymbol.for("::bunhttp2internals::")].util/.bindingslots from them. The error table innghttp2ErrorStringis transcribed directly fromnghttp2_strerror()(nghttp2_helper.c) keyed by the codes innghttp2.h.src/js/internal/test/binding.ts: add acase "http2"sointernalBinding('http2')returns the.bindingslot (constants,nghttp2ErrorString,optionsBuffer). This is the real built-in trace_events: implement Node's trace-events subsystem and port the v26.3.0 trace tests — JSON trace writer, --trace-event-* / --trace-env / --trace-exit flags, per-subsystem events, worker tids, NodeTracing inspector domain (+34 tests, 100% passing) #31824 added; it wins over aBun.pluginvirtual module, so the shim version this PR originally carried was dead and is deleted from the test-common plugin.assertIsObject/assertIsArraynow pass their expected type as a one-element list. Node'sERR_INVALID_ARG_TYPEnormalizes a stringexpectedto a list internally and classifies each element (kTypes lowercase,classRegExpto "an instance of X"). Bun exposes that logic as theArgListoverload of$ERR_INVALID_ARG_TYPE, while the single-string overload is a phrase-passthrough for pre-formatted messages. One call site already passed an array; the list form is simply how a caller selects Node's classification. No C++ changes.ERR_HTTP2_INVALID_CONNECTION_HEADERSis added toErrorCode.ts(appended, so the discriminants in the checked-inErrorCode.rsmirror stay index-aligned: 331,COUNT332).buildNgHeaderStringthrows it andtest-http2-util-headers-listasserts it.NGHTTP2_NV_FLAG_NONE/NGHTTP2_NV_FLAG_NO_INDEXadded tohttp2.constants(asserted bytest-http2-util-headers-listand the existingnode-http2.test.jsconstants test).Ten tests move off
test/expectations.txt:The remaining 4 (
test-http2-client-destroy,test-http2-client-http1-server,test-http2-misbehaving-multiplex,test-http2-server-http1-client) resolveinternal/http2/utilcorrectly now but still fail on runtime behaviour (abort-signal listener count,HPE_PAUSED_H2_UPGRADEparser code, reused-stream-id error class) and stay on the expectations list.Not in this PR
No changes to
src/jsc/bindings/ErrorCode.cppand nothing undertest/js/node/test/parallel/beyondtest/expectations.txt. An earlier revision taught the single-stringERR_INVALID_ARG_TYPEoverload Node's type classification, which reworded ~35 production call sites and required updating two test expectations. The review thread (correctly) asked for a human to sign off on that cross-cutting change; it turned out to be unnecessary once the http2 helpers use the list form, so it was dropped. Teaching the single-string callers Node's wording is a separate compat cleanup.Verification
All 10 newly-enabled tests pass, the full
node-http2.test.jsruntime suite passes (287/287, 0 fail),test-http2-createserver-options/test-http2-createsecureserver-options/test-http2-update-settings(which assert on the productionassertIsObjectmessage) pass unchanged,test-assert.jsandmessage-event.test.tspass unmodified,internalBinding('trace_events')/('constants')consumers (#31824's own tests) are unaffected, andBuffer.alloc(3).swap32()/buildNgHeaderStringreport their own error codes, proving theErrorCode.rs/ErrorCode.tsdiscriminants stay aligned across the append.