node:http2: throw getUnpackedSettings/respondWithFD ERR_INVALID_ARG_TYPE through the shared error machinery

[robobun]

Problem

• require("node:http2").getUnpackedSettings(1n) throws a plain TypeError: JSON.stringify cannot serialize BigInt. with no code; node throws ERR_INVALID_ARG_TYPE (... Received type bigint (1n)). Http2Stream#respondWithFD(1n) does the same.

• The other Received ... renderings at those two sites also diverge from node v26.3.0:

  input	bun 1.4.0	node v26.3.0
  "a".repeat(40)	all 40 characters	type string ('aaaaaaaaaaaaaaaaaaaaaaaaa...')
  "it's"	type string ('it's')	type string ("it's")
  Object.create(null)	an instance of Object	[Object: null prototype] {}
  undefined (respondWithFD)	type undefined (undefined)	undefined
  undefined (getUnpackedSettings)	returns the default settings	throws ERR_INVALID_ARG_TYPE

• Cause: src/js/node/http2.ts builds these two messages by hand (new TypeError(...) + err.code = ...) through a private receivedValueLabel() helper (http2.ts:4039 on main) that approximates node's determineSpecificType. The helper was added in node:http2: rewritten inbound engine, batched write path, server push, +290 node v26.3.0 tests (79% passing) #31584 because at the time the shared $ERR_INVALID_ARG_TYPE rendered an expected-type array as of type Buffer or TypedArray; buffer: sync vendored tests to Node v26.3.0 and fix compat gaps (64/69 · 92.8%) #32626 taught the native array overload node's grouping a week later, so the helper has only been a source of divergence since.

• respondWithFD() also accepted any object with a numeric fd property in place of a FileHandle (node checks instanceof FileHandle), and the fd instanceof FileHandle branch further down the method was unreachable because the descriptor is unwrapped at the top.

Fix

• getUnpackedSettings() throws $ERR_INVALID_ARG_TYPE("buf", ["Buffer", "TypedArray"], buf) and respondWithFD() throws $ERR_INVALID_ARG_TYPE("fd", ["number", "FileHandle"], fd): the same argument lists node's lib/internal/http2/core.js passes to its ERR_INVALID_ARG_TYPE. receivedValueLabel() is deleted.
• Correct because the message now comes from the same renderer every other node-style validator in bun uses: the array overload in ErrorCode.cpp produces node's must be an instance of Buffer or TypedArray / must be of type number or an instance of FileHandle wording, and determineSpecificType produces node's Received ... suffix (bigint, truncation, quote switching, null-prototype inspect fallback, undefined). With this change the output of both sites matches node v26.3.0 for every value I tried except -0 and an object whose constructor has no name, which are rows inside the native renderer itself and are being fixed there, not in http2.
• getUnpackedSettings(undefined) now throws like node; getDefaultSettings() (the only caller that relied on the shortcut) returns { ...kDefaultSettings } directly, so its result is unchanged (the existing getDefaultSettings test pins it).
• respondWithFD() unwraps fd only when it is a real FileHandle (instanceof, as node does); other objects now get the ERR_INVALID_ARG_TYPE above. The now-dead second instanceof branch is removed; fd is always a number by that point. The FileHandle class was already imported for that branch.
• Verified with the two tests added to test/js/node/http2/node-http2.test.js (getUnpackedSettings() reports a bad buf argument like node, respondWithFD() reports a bad fd argument like node and accepts a FileHandle): both fail on the released binary (USE_SYSTEM_BUN=1) and pass with bun bd test. Expected strings are node v26.3.0's output; the second test also serves a file through a fs.promises FileHandle to cover the accept path.
• Also ran with the debug build: the vendored test-http2-getpackedsettings.js, test-http2-respond-file-fd*.js (5 files), test-http2-respond-file-filehandle.js, test-http2-util-asserts.js, and the 15 vendored http2 tests that read getDefaultSettings/localSettings/remoteSettings, all passing; the rest of node-http2.test.js passes apart from the pre-existing DATA payload survives ... block, whose 9 concurrent subprocess cases sit at the 5s default timeout on a loaded debug+ASAN machine and fail identically with this change stashed.

Background

• $ERR_INVALID_ARG_TYPE(name, expected, value) is the builtin-JS entry point to bun's shared node-error machinery (src/jsc/bindings/ErrorCode.cpp). It creates the TypeError with code set, and when expected is an array it ports node's list rendering: entries that are primitive type names become of type a or b, capitalized names become an instance of X or Y, and the two groups are joined with or.
• determineSpecificType is bun's native port of the node helper of the same name that produces the Received ... part of these messages (type bigint (1n), an instance of Array, a 25-character string prefix plus ..., double quotes when the string contains a single quote, a util.inspect fallback for objects without a usable constructor). Every validator in bun that goes through the shared machinery gets it for free; the deleted helper was a second, partial implementation of it.
• FileHandle is the object fs.promises.open() resolves to; node:fs/promises exposes the class to other builtins through its private $data export, which is how http2.ts already imported it.

Probe output (bun 1.4.0 vs node v26.3.0, both sites)

Values: 1n, "a".repeat(40), "it's", Object.create(null), { constructor: {} }, undefined, null, true, "", [], {}, Symbol("s"), named and anonymous functions, a DataView, a Map, { fd: "3" }, -0, 1.

Before, rows differing from node (same at both sites): 1n (undefined: JSON.stringify cannot serialize BigInt.), the 40-character string (not truncated), "it's" (single-quoted), Object.create(null) and { constructor: {} } (an instance of Object), undefined (type undefined (undefined) for respondWithFD, no throw for getUnpackedSettings), -0 (type number (0)).

After: only { constructor: {} } (an instance of undefined) and -0 (type number (0)) still differ; both come from the native renderer and are outside this change.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 28 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9c853443-97f5-467f-aaef-9cc1884f9d19

📥 Commits

Reviewing files that changed from the base of the PR and between 97a4363 and 86dcf72.

📒 Files selected for processing (2)

• src/js/node/http2.ts
• test/js/node/http2/node-http2.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: fix pushed, waiting on CI.

Reproduced on bun 1.4.0 against node v26.3.0 by calling http2.getUnpackedSettings(v) and stream.respondWithFD(v) over the values listed in the PR body and diffing ${e.code}: ${e.message} between the two runtimes (1n threw a codeless JSON.stringify cannot serialize BigInt. TypeError; long strings, quoted strings, null-prototype objects and undefined rendered differently from node).

With this branch the two new tests in test/js/node/http2/node-http2.test.js fail on the released binary and pass with a debug build; the probe output matches node at both sites except for the two rows that live in the native renderer (-0, constructor without a name).

[claude]

LGTM — routes two hand-rolled ERR_INVALID_ARG_TYPE sites through the shared error machinery, deletes the now-dead receivedValueLabel helper and unreachable instanceof branch, and tightens respondWithFD to Node's FileHandle-only check. The inline nit is test diagnosability only.

Extended reasoning...

Overview

Two call sites in src/js/node/http2.ts (getUnpackedSettings and ServerHttp2Stream#respondWithFD) were building ERR_INVALID_ARG_TYPE by hand via a private receivedValueLabel() helper that approximated Node's determineSpecificType. This PR replaces both with $ERR_INVALID_ARG_TYPE(name, [types], value), deletes the helper, removes an unreachable second fd instanceof FileHandle branch, and inlines getDefaultSettings() to { ...kDefaultSettings } now that getUnpackedSettings(undefined) throws (matching Node). Two new tests in node-http2.test.js pin the exact Node v26.3.0 message for nine input shapes at both sites, plus a positive-path check that a real FileHandle still serves.

Security risks

None. This is error-message rendering and input-type validation in a Node compat layer; no new I/O, no auth/crypto/permissions surface.

Level of scrutiny

Low-to-medium. The change is exactly what REVIEW.md prescribes ("Route user-facing JS errors through the centralized ErrorCode machinery … never inline new Error with a hand-assigned .code"), and it deletes a second, partial reimplementation of determineSpecificType. I verified: FileHandle is already imported (http2.ts:37); isTypedArray(Buffer) is true so dropping the redundant Buffer.isBuffer check is safe; kDefaultSettings is a plain literal so { ...kDefaultSettings } reproduces the old getDefaultSettings() output; and after the top-of-method unwrap fd is always a number, so the removed second instanceof branch was genuinely dead.

Other factors

Two intentional user-visible tightenings — getUnpackedSettings() with no argument now throws, and respondWithFD({ fd: n }) (a plain object, not a real FileHandle) now throws — are both explicitly the Node behavior being matched, are called out in the description, and are covered by the new tests. The one inline finding is a test-only nit (the second await on req "end" doesn't re-wire error → reject); it only affects how a future regression would surface (timeout vs. the real error) and does not affect the happy path or the change under test.

[claude]

LGTM — the test now wires reject to session/stream/listen errors via a single Promise.withResolvers(), which addresses the earlier note.

What was reviewed:

• getUnpackedSettings/respondWithFD now route through $ERR_INVALID_ARG_TYPE; isTypedArray(buf) still accepts Buffer (Uint8Array subclass), and FileHandle was already imported at the top of the file.
• getDefaultSettings() returning { ...kDefaultSettings } is equivalent to the removed getUnpackedSettings(undefined) shortcut; no other internal caller relied on the undefined-returns-defaults behavior.
• The removed second fd instanceof FileHandle branch was dead — fd is always a number by that point after the top-of-function unwrap.

Extended reasoning...

Overview

Two hand-rolled new TypeError(...) + err.code = "ERR_INVALID_ARG_TYPE" sites in src/js/node/http2.ts (getUnpackedSettings, ServerHttp2Stream#respondWithFD) are replaced with the shared $ERR_INVALID_ARG_TYPE builtin, and the private receivedValueLabel() helper that approximated node's determineSpecificType is deleted. getDefaultSettings() is inlined to return { ...kDefaultSettings } since getUnpackedSettings(undefined) now throws (matching node). respondWithFD now checks instanceof FileHandle rather than duck-typing on .fd, and the unreachable second instanceof branch is removed. Two tests in test/js/node/http2/node-http2.test.js pin the error name/code/message to node v26.3.0's output across nine input shapes and exercise the FileHandle accept path end-to-end.

Security risks

None. This is error-message rendering and argument validation in a Node compat layer; no auth, crypto, or untrusted-input parsing is touched.

Level of scrutiny

Low-to-medium. The change follows REVIEW.md's explicit guidance to route user-facing JS errors through the centralized ErrorCode machinery rather than inline new Error with a hand-assigned .code, and to delete dead code in the same PR that makes it dead. The two behavior tightenings (getUnpackedSettings(undefined) now throws; respondWithFD({fd: n}) now throws) are deliberate node-compat corrections, both covered by the new tests and cross-checked against node v26.3.0 in the PR body.

Other factors

A prior review pass flagged that the respondWithFD test's second await didn't wire a reject path; commit 86dcf72 addressed it by collapsing the request lifecycle into one Promise.withResolvers() with client.on("error", reject) and req.on("error", reject), and also added server.once("error", reject) to the listen promise. Confirmed isTypedArray (from node:util/types) accepts Buffer, so dropping the redundant Buffer.isBuffer check is behavior-preserving. FileHandle is imported at http2.ts:37 from the fs/promises $data export. The PR body reports the vendored test-http2-getpackedsettings.js, test-http2-respond-file-fd*, and test-http2-respond-file-filehandle.js still pass.