Repository navigation
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 9 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 8:04 AM PT - Sep 6th, 2026
❌ @robobun, your commit 4f7a22f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41546That installs a local version of the PR into your bun-41546 --bun |
There was a problem hiding this comment.
LGTM — straightforward Node compat: 13 hidden constants added to http2.constants with the correct descriptors.
What was reviewed:
- Values cross-checked against nghttp2's
nghttp2.henums (HCAT 0-3, NV_FLAG 0/1, ERR -501/-508/-509/-510/-901) and Node'snode_http2.hStreamOptions (1/2) — all match. { value }-only descriptors give writable/enumerable/configurable all false, matchingNODE_DEFINE_HIDDEN_CONSTANT;Object.keys(constants)is unchanged.Object.definePropertiesruns at module-eval time on a fresh literal, so no primordial-tampering exposure.- Test appended to the existing
node-http2.test.js, asserts values + full descriptors + non-enumeration, and fails on current Bun where these keys are undefined.
Extended reasoning...
Overview
This PR adds 13 constants to require("node:http2").constants that Node.js defines via NODE_DEFINE_HIDDEN_CONSTANT in src/node_http2.cc: four NGHTTP2_HCAT_* header-category enums, two NGHTTP2_NV_FLAG_* flags, five NGHTTP2_ERR_* codes, and two STREAM_OPTION_* flags. They are attached with Object.defineProperties using { value }-only descriptors so the properties are non-writable, non-enumerable, and non-configurable — exactly what Node's macro produces. A test in the existing test/js/node/http2/node-http2.test.js asserts each value, the full property descriptor, and that hidden entries stay out of Object.keys(constants).
Security risks
None. This is a pure additive constants table on a built-in module, evaluated once at module load before any user code runs. No input parsing, no I/O, no auth or crypto surface. The Object.defineProperties call operates on a locally-constructed object literal at load time, so prototype-pollution / global-tampering concerns from src/js/CLAUDE.md do not apply here.
Level of scrutiny
Low. The change is mechanical Node-compat data entry. The only thing to get wrong is a numeric value or a descriptor attribute, and both were checked: the NGHTTP2_* values line up with nghttp2's public header (NGHTTP2_ERR_INVALID_ARGUMENT=-501, DEFERRED=-508, STREAM_ID_NOT_AVAILABLE=-509, STREAM_CLOSED=-510, NOMEM=-901; HCAT_* 0-3; NV_FLAG_NONE=0, NO_INDEX=1), and STREAM_OPTION_EMPTY_PAYLOAD=1 / GET_TRAILERS=2 match Node's enum StreamOptions in node_http2.h. A bare { value } descriptor defaults writable/enumerable/configurable to false per ECMA-262 §6.2.6.1, matching NODE_DEFINE_HIDDEN_CONSTANT's ReadOnly | DontDelete | DontEnum.
Other factors
The test follows repo conventions: appended to the existing module test file (not a new file), uses a composite .toEqual on the value map rather than 13 .toBe calls, and asserts the strongest invariant (full descriptor equality plus non-enumeration). It fails on current Bun (all keys read undefined) and passes with the change, satisfying the USE_SYSTEM_BUN=1 requirement. No CODEOWNERS entry covers src/js/node/http2.ts or the test file. The bug hunt exited on dry_streak with no findings.
|
CI status: the diff is green. The two red items are not from this change.
This PR changes src/js/node/http2.ts only, which cannot affect either. |
Node defines 13 entries of http2.constants with NODE_DEFINE_HIDDEN_CONSTANT. They are read-only and not enumerable, but present. Bun did not define them, so a comparison against NGHTTP2_ERR_STREAM_CLOSED and friends read undefined.
b4b4bec to
4f7a22f
Compare
There was a problem hiding this comment.
LGTM — small, self-contained Node compat addition.
What was reviewed:
- Verified all 13 constant values against nghttp2's
nghttp2.henums (HCAT, NV_FLAG, ERR codes) and Node'snode_http2.h(STREAM_OPTION_* = 0x1/0x2). - Confirmed
{ value }-only descriptors matchNODE_DEFINE_HIDDEN_CONSTANTsemantics (non-writable/enumerable/configurable), and thatObject.definePropertiesat module-load time on a fresh literal is the established pattern here — no tamper-resistance concern. - Test asserts exact values, exact descriptors, and non-enumerability; added to the existing http2 test file.
Extended reasoning...
Overview
This PR adds 13 constants to require("node:http2").constants that Node.js defines via NODE_DEFINE_HIDDEN_CONSTANT in src/node_http2.cc — the NGHTTP2_HCAT_*, NGHTTP2_NV_FLAG_*, five NGHTTP2_ERR_* codes, and two STREAM_OPTION_* flags. In Node these are present but non-enumerable/non-writable/non-configurable, so a { value }-only descriptor via Object.defineProperties on the existing constants literal reproduces that exactly. A test in the existing node-http2.test.js file asserts each value, each full property descriptor, and that the keys stay out of Object.keys(constants).
Security risks
None. This is a pure additive constants table on a Node compat module. No user input is parsed, no I/O, no auth/crypto surface. The Object.defineProperties call runs once at module-load time on a literal created three lines above, before any user code can monkey-patch globals, so the "hostile userland" concern for src/js/ builtins does not apply here.
Level of scrutiny
Low. The change is 16 lines of source plus a 34-line test. The only correctness risk is a wrong magic number, so I cross-checked each value: the NGHTTP2_HCAT_* (0-3) and NGHTTP2_NV_FLAG_* (0, 1) values match the nghttp2_headers_category and nghttp2_nv_flag enums in nghttp2; the error codes -501/-508/-509/-510/-901 match NGHTTP2_ERR_INVALID_ARGUMENT/DEFERRED/STREAM_ID_NOT_AVAILABLE/STREAM_CLOSED/NOMEM in nghttp2.h; and STREAM_OPTION_EMPTY_PAYLOAD=1/GET_TRAILERS=2 match kStreamOptionEmptyPayload = 0x1 / kStreamOptionGetTrailers = 0x2 in Node's src/node_http2.h. NODE_DEFINE_HIDDEN_CONSTANT in Node's node_internals.h sets ReadOnly | DontDelete | DontEnum, which is exactly what a bare { value } descriptor produces.
Other factors
No CODEOWNERS entry covers these files. The test follows repo conventions (added to the existing module test file, .toEqual on a whole object for values, exact descriptor assertions). The PR evidence shows the new test fails on main and passes with the fix in both debug+ASAN and release. The destructuring block immediately after the literal (const { NGHTTP2_SESSION_SERVER, ... } = constants) does not reference any of the new keys, so no interaction there. Exit reason was dry_streak. This is exactly the kind of mechanical, verifiable compat fill-in that does not need a human reviewer.
|
CI status for 4f7a22f (build 111227): the one red test is not caused by this diff.
I reported the stress test's cold-start sensitivity separately. The other red item, |
Problem
require("http2").constantslacks 13 entries node exports:NGHTTP2_HCAT_REQUEST/RESPONSE/PUSH_RESPONSE/HEADERS,NGHTTP2_NV_FLAG_NONE/NO_INDEX,NGHTTP2_ERR_DEFERRED/STREAM_ID_NOT_AVAILABLE/INVALID_ARGUMENT/STREAM_CLOSED/NOMEM,STREAM_OPTION_EMPTY_PAYLOAD/GET_TRAILERS. A comparison such ascode === constants.NGHTTP2_ERR_STREAM_CLOSEDreadsundefinedand never matches.node_http2.ccwithNODE_DEFINE_HIDDEN_CONSTANT. They are read-only and not enumerable, so a key-by-key diff ofObject.keysdoes not show them. Bun'sconstantsliteral insrc/js/node/http2.ts:1452never had them.Fix
constantsobject withObject.definePropertiesafter the literal. Each descriptor is{ value }only, so the property is not writable, not enumerable, and not configurable, the same as node.Object.keys(constants)stays at 240 entries, as in node v26.3.0.test/js/node/http2/node-http2.test.js(new test, fails on bun 1.4.3, passes with this change).Background
http2.constantsmirrors nghttp2's enums and node's own stream option flags. Node fills it from C++ and marks a few entries hidden so they do not show up in enumeration but still resolve by name.node:http2is a pure JS module with a hand-writtenconstantsliteral. The literal can only express enumerable keys, so the hidden set needsdefineProperties.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file