node: per-stream console inspectOptions, connect error Local suffix, v8 flag validation (+3 tests) - #34523
Conversation
…v8 flag validation
Three independent Node v26.3.0 compatibility gaps, each with the upstream test
that covers it, copied verbatim.
net: ExceptionWithHostPort dropped Node's fifth `additional` argument, which
appends ` - Local (address:port)` to the message. Both connect-failure paths in
net.ts already computed that string and threw it away, so a failed connect with
a bound local address reported `connect ECONNREFUSED 127.0.0.1:12399` where Node
reports `connect ECONNREFUSED 127.0.0.1:12399 - Local (127.0.0.1:12400)`.
console: `new Console({ inspectOptions })` did not accept a Map keyed by stream,
so per-stream options were read as a plain options object with no `colors` and
silently ignored. Look the options up per stream instead, keeping the
plain-object form applying to both streams. Uses $get and avoids constructing a
Map so a tampered global Map cannot influence the result.
v8: setFlagsFromString threw the not-implemented error before validating its
argument. Validate first, so a non-string is rejected with ERR_INVALID_ARG_TYPE
as in Node; a real flag string still reports the gap rather than pretending the
flag was applied.
Adds test-net-connect-local-error, test-console-tty-colors-per-stream and
test-v8-flag-type-check. Verified against the Node v26.3.0 binary: console
inspect output, the connect error message, and the v8 validation errors are
identical across ten checks including per-stream colors, a Map missing an entry
for the stream, and the colorMode conflict in both forms.
|
Updated 11:04 PM PT - Jul 18th, 2026
@cirospaciari, your commit 94d7439 is building: |
|
@robobun adopt |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThe PR adds per-stream console color inspection, local address and port details to connection errors, and string validation for ChangesConsole stream inspection
Connection error details
V8 flag validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)test/js/node/test/sequential/test-net-connect-local-error.jsFile contains syntax errors that prevent linting: Line 42: Illegal return statement outside of a function Comment |
|
Adopted. #75682 finished: the three new tests pass on every lane. The only hard reds are pre-existing on main ( Ready for merge. The Biome warning on |
There was a problem hiding this comment.
LGTM — three small, self-contained Node compat fixes each covered by a vendored upstream test.
What was reviewed:
ExceptionWithHostPort's newadditionalparam matches Node'slib/internal/errors.js; bothnet.tscallers already computeddetailsand now pass it through.- Console per-stream
inspectOptions:validateObjectaccepts aMap(checkedNodeValidator.cpp); lookup uses$isMap/.$getintrinsics so it's tamper-resistant; the plain-object path is unchanged. v8.setFlagsFromStringnow validates beforenotimpl; a valid string still surfacesERR_NOT_IMPLEMENTED.
Extended reasoning...
Overview
Three independent Node v26.3.0 compat gaps, each with a vendored upstream test:
src/js/internal/shared.ts—ExceptionWithHostPortgains an optionaladditionalparam that appends- Local (…)to the message, matching Node'slib/internal/errors.js.src/js/node/net.ts— the two async connect-failure paths (afterConnect,createConnectionError) now pass thedetailsstring they were already building intoExceptionWithHostPort.src/js/builtins/ConsoleObject.ts—inspectOptionsmay now be aMapkeyed by stream; the constructor'scolorModeconflict check andkGetInspectOptionsboth branch on$isMapand use.$get(stream).src/js/node/v8.ts—setFlagsFromStringcallsvalidateString(flags, "flags")before throwingERR_NOT_IMPLEMENTED.
Security risks
None. No untrusted input parsing, no auth/crypto, no filesystem or network surface added. The console change reads options from a user-supplied Map via the tamper-proof $get intrinsic rather than public Map.prototype.get.
Level of scrutiny
Low. Each change is a few lines in the Node compat JS layer, mirrors upstream Node behavior line-for-line, and ships with the exact upstream test that exercises it. The PR description documents byte-identical verification against Node v26.3.0 and a regression sweep across 249 related vendored tests with only pre-existing failures.
Other factors
- Verified
jsFunction_validateObjectinNodeValidator.cppaccepts aMap(rejects only null/array/callable/non-object), so passing aMapasinspectOptionsdoesn't break existing validation. - The
for (const stream of [stdout, stderr])loop in the constructor uses public array iteration, but this matches existing patterns in the same file (for (const method of Reflect.ownKeys(...)),for (const row of rows)) and runs once atConsoleconstruction, not on a hot path. - The two synchronous connect-failure sites (
net.ts:2892,:3022) that also dropdetailsin Node are noted in the PR as pre-existing and left for follow-up; that scoping is reasonable since Bun doesn't computedetailsthere yet. - No prior review comments to address; CI is building.
There was a problem hiding this comment.
LGTM — three small, well-scoped Node v26 compat fixes, each backed by a vendored upstream test.
What was reviewed:
ExceptionWithHostPortfifth-arg plumbing and bothnet.tscall sites —detailswas already computed and discarded; now threaded through. Other callers (dgram, bind paths) unaffected since the param is optional.ConsoleObject.tsper-stream Map lookup —$isMap/.$getare existing intrinsics;validateObjectaccepts a Map (checked NodeValidator.cpp:720); plain-object path unchanged.v8.setFlagsFromString—validateStringruns before the not-implemented throw; string args still hitERR_NOT_IMPLEMENTEDas intended.
Extended reasoning...
Overview
Three independent Node v26.3.0 compatibility gaps, ~30 lines of source changes across four files plus three vendored upstream tests:
src/js/internal/shared.ts—ExceptionWithHostPortgains the optionaladditionalparam that appends- Local (…)to the message, matching Node'slib/internal/errors.js.src/js/node/net.ts— the two async connect-failure sites (afterConnect,createConnectionError) now pass the already-computeddetailsstring through. The two synchronous sites at :2892/:3022 are explicitly noted as out of scope.src/js/builtins/ConsoleObject.ts—inspectOptionsmay now be aMapkeyed by stream; the constructor validatescolorModeconflict per stream, andkGetInspectOptionslooks up the entry for the target stream. Uses$isMap+.$getintrinsics, consistent with existing usage in this file.src/js/node/v8.ts—setFlagsFromStringvalidates its argument type before throwing not-implemented.
Security risks
None. No new external input surfaces, no auth/crypto/permissions. The console change reads a user-provided Map via the tamper-safe $get intrinsic; the for…of over a two-element array literal in the constructor is no less tamper-safe than the existing Object.keys(...).forEach in the same scope, and only runs on explicit new Console(...).
Level of scrutiny
Low-to-moderate. All four source changes are direct ports of Node behavior with cited upstream line references, each covered by a verbatim upstream test, and the PR description reports byte-identical output against a real Node v26.3.0 binary plus a 249/251 regression sweep of the vendored net/dgram/v8/console suites. The only non-trivial hunk (Map-keyed inspectOptions) is ~12 lines and falls back to the previous plain-object behavior when the input isn't a Map.
Other factors
- Verified
validateObject(NodeValidator.cpp:720-738) accepts a Map — it only rejects null/array/callable/non-object. - Grepped all
ExceptionWithHostPortcall sites: dgram and the net bind/listen paths don't pass a fifth arg, so they're unaffected by the optional param. $isMapandmap.$getare established intrinsics already used in this file (ConsoleObject.ts:682) and elsewhere (trace_events.ts,CommonJS.ts).- No prior review comments to address; CI build in progress at time of review.
|
@robobun adopt it |
There was a problem hiding this comment.
LGTM — three small, independent Node v26 compat fixes, each backed by a vendored upstream test.
What was reviewed:
ExceptionWithHostPortgains the optionaladditionalparam and bothnet.tsconnect-failure sites now pass the already-computeddetails; other callers (dgram, bind paths) remain unchanged since the param is optional.- Per-stream
inspectOptions: confirmedvalidateObject(NodeValidator.cpp:720) accepts aMap, and$isMap/.$getare established intrinsics; plain-object path is unchanged (loop just checks the same object twice, harmless). v8.setFlagsFromString:validateStringruns before the not-implemented throw; valid string args still surfaceERR_NOT_IMPLEMENTED.
Extended reasoning...
Overview
Three unrelated Node v26.3.0 compatibility gaps, each fixed in a small, localized diff and covered by a verbatim upstream test:
src/js/internal/shared.ts+src/js/node/net.ts:ExceptionWithHostPortgets a fifth optionaladditionalparameter that appends" - Local (…)"to the message, matching Nodelib/internal/errors.js. The two async connect-failure paths (afterConnect,createConnectionError) already computeddetailsand setex.localAddress/ex.localPortfrom it — they just never passed it into the constructor. Now they do.src/js/builtins/ConsoleObject.ts:new Console({ inspectOptions })may now be aMapkeyed by stream. The constructor checks$isMapand, per stream, looks up options with the.$getintrinsic to run the existingcolorModeconflict check;kGetInspectOptionsdoes the same lookup at format time. WheninspectOptionsis a plain object, the loop evaluates the same object twice — redundant but behavior-preserving.src/js/node/v8.ts:setFlagsFromStringnowvalidateString(flags, "flags")before throwingERR_NOT_IMPLEMENTED, so a non-string argument surfacesERR_INVALID_ARG_TYPEas Node does.
Security risks
None. No parsing of untrusted structured input, no auth/crypto/permissions, no filesystem/process boundaries. The console change reads a user-supplied Map via the tamper-proof $get intrinsic and brand-checks with $isMap, so a poisoned global Map.prototype.get cannot influence it.
Level of scrutiny
Low-to-moderate. These are additive Node-compat shims in JS-side runtime modules: an optional constructor parameter, an argument validator moved before a throw, and a Map branch in the console options lookup. No native code, no memory-safety surface, no hot-path perf claims. Each change is exercised by a copied upstream Node test file, and the PR description reports byte-identical output against Node v26.3.0 plus a clean run of the surrounding vendored test buckets.
Other factors
- Checked that
validateObject(NodeValidator.cpp:720) accepts aMapinstance (rejects only null/array/callable/non-object), so the pre-existingvalidateObject(inspectOptions, ...)call does not reject the new Map form. - Checked that
$isMapand.$getare established intrinsics already used elsewhere insrc/js/(e.g.inspect.js,trace_events.ts), so the tamper-resistance approach follows house style. - Grepped all
ExceptionWithHostPortcall sites — the new fifth param is optional and only the two intendednet.tssites pass it; dgram and bind callers are untouched. The PR description explicitly notes the two synchronous connect paths (net.ts:2892/:3022) as pre-existing gaps left for follow-up. - The added top-level
require("internal/validators")inv8.tsmatches the file's existing eager-require style. - No prior human review comments to address; CI is building.
…+4 tests) (#34550) Implements `v8.GCProfiler` on a JSC `HeapObserver` and adds `v8.isStringOneByteRepresentation`, with 4 upstream Node v26.3.0 tests copied verbatim. Stacked on #34523 (which carries the `setFlagsFromString` validation these share). ## GCProfiler Was a not-implemented stub. Now backed by a JSC `HeapObserver` — the same mechanism JSC's own inspector heap agent uses. `willGarbageCollect` / `didGarbageCollect` bracket each collection; the record carries measured before/after live bytes and the collection cost. **Concurrency.** JSC collects concurrently with the mutator, so a collection can already be in flight when a profiler starts. That epilogue arrives with no matching prologue, and is dropped rather than reported against a start time that was never taken. The stored timestamp is additionally cleared whenever the observer attaches or detaches, so a collection observed before a detach cannot pair with a *later* session and report all the wall time in between as its cost — that second case was found in review after the first was fixed, and measured a cost 64,294 µs larger than the session's entire lifetime before the fix. Post-fix: 0 violations of `cost <= session wall-clock`. **Honest gaps, not invented numbers.** Counters JSC does not track (`heapSizeLimit`, `mallocedMemory`, global handles) report `0`, and the single JSC heap reports as one space rather than V8's thirteen named spaces. A consumer looking for `old_space` finds nothing instead of a fabricated number. Tests: `test-v8-collect-gc-profile`, `-using`, `-exit-before-stop` ## `isStringOneByteRepresentation` Native, answering the representation question directly. Verified byte-identical to node v26.3.0 across empty strings, latin1 and mixed ropes, a rope slice, astral characters, `new String(...)` and no-args. Test: `test-v8-string-is-one-byte-representation` ## Deliberately excluded - **`test-v8-collect-gc-profile-in-worker`** — a fake pass. Its `testGCProfiler()` is `async` and unawaited, so every assertion lands in a rejected promise inside a worker, and Bun swallows worker unhandled rejections where node exits 1. The file exits 0 in Bun even with deliberately broken assertions. The swallowed-rejection divergence is a real Bun bug and deserves its own issue; a harness change to polyfill `gc` in workers was reverted along with the test, since nothing else needs it. - **`test-v8-stats`** — needs `getHeapSpaceStatistics()` to return V8's 13 heap-space names with numbers JSC cannot supply. - **`test-v8-flags`** — asserts V8 natives syntax (`%IsSmi(42)`) evaluates; no JSC equivalent. - **`test-v8-version-tag`** — needs `cachedDataVersionTag()` to change after `setFlagsFromString`, which would require pretending flags were applied. - **The five `v8.promiseHooks` tests** — Bun fires no promise hooks; wiring only the argument validation moves each past its first assertion and then fails on hook-firing counts, for a net gain of zero. ## Verification - the 4 new tests plus `test-v8-flag-type-check`: 3/3 green each under the runner's exact invocation - the profiler records real collections (`statistics.length` 2-3, `gcType: "Scavenge"`, non-zero costs) — it is not an empty-profile stub. This matters because the vendored `common/v8.js` helper guards its per-entry checks behind `if (data.statistics.length)`, so an empty array would pass all these tests while recording nothing - validated on a debug+ASAN build, since these callbacks run inside the collector; no JSC assertion fired - no regressions: 110 vendored `test-v8-*` / `test-worker-*` tests pass, 0 fail ## Two Bun bugs found and characterized, not fixed here 1. A `worker_threads` worker does not honor `--expose-gc` even though it reports the flag in `execArgv`; node gives the worker a real `gc`. 2. Bun swallows unhandled promise rejections inside a worker (node exits 1). This is what makes the worker GC-profile test unfalsifiable. Fixes #21362 <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 9 · 10 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/v8/v8-module.test.ts" ninja: Entering directory `/workspace/bun/build/debug' [1/184] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [2/184] gen cpp.rs (cppbind) [3/184] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 238 extern-C blocks audited [4/184] gen JS modules (bundle-modules) Preprocess modules (11573ms) Bundle modules (81ms) Postprocesss modules (283ms) Bundle Functions (991ms) Generate Code (27ms) [12.99s] Bundled "src/js" for development 2749 kb 193 internal modules 13 native modules 90 internal functions across 19 files [4/184] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) [6/184] fetch WebKit (prebuilt) [WebKit] fetching https://github.com/oven-sh/WebKit/releases/download/autobuild-e6e37cda216c0292ae68c30c84a9dc8601d0fba5/bun-webkit-linux-amd64-debug-asan.tar.gz [WebKit] extracted to ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (6f0b33b) test/js/node/v8/v8-module.test.ts: (pass) v8.isStringOneByteRepresentation > rejects non-string arguments [1.03ms] (pass) v8.isStringOneByteRepresentation > reports storage width [0.05ms] (pass) v8.GCProfiler > class name [0.02ms] (pass) v8.GCProfiler > start/stop records a forced collection [2.40ms] (pass) v8.GCProfiler > Symbol.dispose stops without returning a report [0.07ms] (pass) v8.GCProfiler > restart after stop [1.28ms] (pass) v8.GCProfiler > full collection does not report external memory growing [1.38ms] (pass) v8.GCProfiler > worker exiting with an open session does not crash [76.63ms] 8 pass 0 fail 57 expect() calls Ran 8 tests across 1 file. [356.00ms] __F:0:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/v8/v8-module.test.ts" bun test v1.4.0 (e5afe67) test/js/node/v8/v8-module.test.ts: (pass) v8.isStringOneByteRepresentation > rejects non-string arguments [13.99ms] (pass) v8.isStringOneByteRepresentation > reports storage width [2.50ms] (pass) v8.GCProfiler > class name [1.39ms] (pass) v8.GCProfiler > start/stop records a forced collection [67.43ms] (pass) v8.GCProfiler > Symbol.dispose stops without returning a report [4.15ms] (pass) v8.GCProfiler > restart after stop [30.43ms] (pass) v8.GCProfiler > full collection does not report external memory growing [35.54ms] (pass) v8.GCProfiler > worker exiting with an open session does not crash [3565.73ms] 8 pass 0 fail 57 expect() calls Ran 8 tests across 1 file. [6.42s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision e5afe67 features baseline 22 deps, 108 codegen, 1171 objects in 869ms ninja: Entering directory `/workspace/bun/build/release' [1/141] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [2/141] gen cpp.rs (cppbind) [3/141] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 238 extern-C blocks audited [4/141] gen JS modules (bundle-modules) Preprocess modules (11552ms) Bundle modules (50ms) Postprocesss modules (199ms) Bundle Functions (1020ms) Generate Code (51ms) [12.90s] Bundled "src/js" for production 2560 kb 193 internal modules 13 native modules 90 internal functions across 19 files [4/141] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_alloc v0.0.0 (/w ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/node/v8.ts | 94 ++++++++++++- src/jsc/bindings/NodeV8.cpp | 108 +++++++++++++++ src/jsc/bindings/NodeV8.h | 151 +++++++++++++++++++++ src/jsc/bindings/ZigGlobalObject.cpp | 1 + src/jsc/bindings/ZigGlobalObject.h | 6 + .../test-v8-collect-gc-profile-exit-before-stop.js | 17 +++ .../parallel/test-v8-collect-gc-profile-using.js | 26 ++++ .../test/parallel/test-v8-collect-gc-profile.js | 12 ++ .../test-v8-string-is-one-byte-representation.js | 36 +++++ test/js/node/v8/v8-module.test.ts | 133 ++++++++++++++++++ 10 files changed, 580 insertions(+), 4 deletions(-) ``` </details> **gate history** · 8 passed · 1 rejected · iteration 9 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/node/v8.ts 8 7 0 src/jsc/bindings/NodeV8.cpp 2 3 0 src/jsc/bindings/NodeV8.h 2 2 0 src/jsc/bindings/ZigGlobalObject.cpp 1 1 0 src/jsc/bindings/ZigGlobalObject.h 1 2 0 …parallel/test-v8-collect-gc-profile-exit-before-stop.js 0 0 0 …/node/test/parallel/test-v8-collect-gc-profile-using.js 0 0 0 test/js/node/test/parallel/test-v8-collect-gc-profile.js 0 0 0 …t/parallel/test-v8-string-is-one-byte-representation.js 0 0 0 test/js/node/v8/v8-module.test.ts 2 5 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com> Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Three independent Node v26.3.0 compatibility gaps, each shipped with the upstream test that covers it, copied verbatim. None of them touch
perf_hooks.Selected the same way as #34517: diffed Node v26.3.0's
test/parallel+test/sequentialagainst main and against the vendored tests of all 32 open PRs, then ran the 956 missing tests through the runner's exact invocation.net— connect errors lost the- Local (…)suffixExceptionWithHostPortdropped Node's fifthadditionalargument (lib/internal/errors.js:765-786), which appends- Local (address:port). Both connect-failure paths innet.tsalready computed that string and threw it away.Test:
sequential/test-net-connect-local-errorconsole— per-streaminspectOptionswas ignoredNode v26 lets
new Console({ inspectOptions })take aMapkeyed by stream so stdout and stderr can be formatted differently (lib/internal/console/constructor.js:144-157, 334-335). Bun stored the value raw and looked it up unkeyed, so a Map was treated as an options object with nocolorsand per-stream colors silently did nothing.Looks the options up per stream instead; the plain-object form still applies to both. Implemented with
$getand without constructing aMap, so unlike a direct port of Node's code a tampered globalMapcannot influence the result — verified against two of the three tamper repros review raised (the third,_times/kCountsatConsoleObject.ts:414,416, is pre-existing and untouched).Test:
test-console-tty-colors-per-streamv8.setFlagsFromString— validation ran after the not-implemented throwNode rejects a non-string argument with
ERR_INVALID_ARG_TYPE; Bun threwERR_NOT_IMPLEMENTEDfirst. Now it validates first.Deliberately not turned into a no-op that records flags, which is what would be needed to also pass
test-v8-version-tag— that would tell callers a flag applied when it didn't.v8.setFlagsFromString('--allow_natives_syntax')still reportsERR_NOT_IMPLEMENTED.Test:
test-v8-flag-type-checkVerification
Driven against the real Node v26.3.0 binary: console inspect output, the connect error message and the v8 validation errors are byte-identical across ten checks, including per-stream colors, a Map with no entry for the stream being written to, and the
colorModeconflict in both the Map and plain-object forms.test-net-*/test-dgram-*/test-v8-*/test-console-*tests pass; the 2 failures (test-net-connect-keepalive,test-net-server-keepalive) were confirmed pre-existing by running them on a build without this changetest/js/node/net+test/js/node/dgramis 208 tests / 0 fail,test/js/node/console9 / 0 failNoted, not fixed
net.ts:2892and:3022(the synchronous connect-failure paths) also take adetailsargument in Node, computed fromself._getsockname(). Bun never computes it there, so those paths still lose the suffix. Pre-existing and not reachable from this test; left for a follow-up.Independent of #34517 (14 vendored tests) and #34518 (perf_hooks).