util.inspect: freeze builtInObjects to Node's 47-name bootstrap set - #36138
util.inspect: freeze builtInObjects to Node's 47-name bootstrap set#36138robobun wants to merge 6 commits into
Conversation
Node computes the builtInObjects set during bootstrap before Buffer, URL, and other Node/Web globals are installed on globalThis, so only ECMAScript language built-ins end up in the set. Bun has those globals present when inspect.js loads, so the set was too broad and hasBuiltInToString treated Buffer.prototype.toString as a generic built-in toString, sending Buffers down the inspect path for %s. Hardcode the ECMAScript language built-in names so that %s with a Buffer, URL, URLSearchParams, or any class that defines its own toString on a non-language prototype goes through String() like Node does.
|
Reproduced with Gate passed (test fails without the fix and passes with it on ASAN and release). All review feedback addressed through 38e2f55. CI build 83594 is red only on This is the focused |
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 2:47 PM PT - Jul 27th, 2026
❌ @robobun, your commit 38e2f55 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36138That installs a local version of the PR into your bun-36138 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #33462. That PR updates the prototype walk inside This PR changes the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — correct root-cause fix for util.format('%s', Buffer) Node compat.
What was reviewed:
- Traced all three consumers of
builtInObjects(hasBuiltInToString,getConstructorName,addPrototypeProperties) — each moves closer to Node's behavior since Node's set never contained Buffer/URL/etc. - Verified the hardcoded list covers Node's bootstrap-time set (plus newer language intrinsics like
Float16Array,DisposableStack) and excludes Node/Web globals as intended. - Confirmed the removed primordials (
ArrayPrototypeFilter,RegExpPrototypeExec,ObjectGetOwnPropertyNames) are still used elsewhere — no dead code introduced. - Tests cover positive cases (Buffer, URL, URLSearchParams, Buffer subclass) and negative controls (Uint8Array via %TypedArray%.prototype.toString →
"65", Array still inspects).
Extended reasoning...
Overview
This PR fixes a Node.js compatibility bug where util.format('%s', buffer) returned <Buffer 61 62> instead of ab. The root cause is that builtInObjects in src/js/internal/util/inspect.js was populated via Object.getOwnPropertyNames(globalThis) at module load time. In Node this runs at bootstrap before Node/Web globals are installed, so the set contains only ~50 ECMAScript intrinsics. In Bun, all globals are already present, so the set contained ~115 names including Buffer, URL, Request, etc. — causing hasBuiltInToString to misclassify Buffer.prototype.toString as a language built-in and route %s through inspect() instead of String().
The fix replaces the dynamic scan with a hardcoded list of ECMAScript language built-in names, matching what Node observes at bootstrap (plus newer language intrinsics like Float16Array and DisposableStack that Node itself misses only due to load ordering).
Security risks
None. This is a formatting/inspection code path with no auth, crypto, or filesystem implications. The change replaces dynamic global enumeration with a static string list.
Level of scrutiny
Low-to-medium. This is a small, well-scoped Node compat fix in the util.inspect layer. The root-cause analysis is precise and the fix is at the correct layer (the set itself, not the consumers). I traced all three uses of builtInObjects:
hasBuiltInToString(the reported bug) — now correctly returnsfalsefor Buffer/URL, routing%sthroughString().getConstructorNamewithprotoProps— now callsaddPrototypePropertiesfor Buffer/URL undershowHidden: true, matching Node.addPrototypePropertiesstop condition — now walks past Buffer.prototype to Uint8Array.prototype, matching Node.
All three become more Node-compatible; none regress. The showHidden: true output for Web/Node globals will now include prototype properties it previously omitted, but that is the Node behavior and the author confirmed the inspect/console test suites remain green.
Other factors
- Tests are gate-verified (fail on system Bun, pass on debug build) per the PR description.
- Test coverage is thorough: Buffer, multi-byte UTF-8 Buffer, URL, URLSearchParams, Buffer subclass, plus negative controls (Uint8Array still stringifies via %TypedArray%.prototype →
"65", Array still takes the inspect path →[ 1, 2 ]). - No dead primordials:
ArrayPrototypeFilter,RegExpPrototypeExec, andObjectGetOwnPropertyNamesare all still used elsewhere in the file. - The hardcoded list includes non-constructor names like
Infinity/NaN/Math/JSON— harmless (they can never be a prototype'sconstructor.name) and present because Node's regex-filtered set includes them too. - The comment explaining why the list is hardcoded is durable and non-obvious — exactly the kind of comment the review guide asks for.
|
Superseded by #36141, which includes the same |
… set (47 names) Drop the five language globals Node itself does not see at bootstrap (SharedArrayBuffer, Float16Array, DisposableStack, AsyncDisposableStack, SuppressedError). They are observably absent from Node's set: showHidden on a Float16Array walks into Float16Array.prototype in Node but not in the other typed arrays, and that asymmetry must match. Also drop Float16Array from the showHidden typed-array loops in util-inspect.test.js (upstream Node's test never had it), and give that test body a debug-build timeout: the surrogate-pair loop runs ~10k inspects and already exceeds the 5s default on bun bd with or without this change.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js:244-247— DroppingFloat16Arrayfrom the showHidden loops removes the only assertion coveringutil.inspect(new Float16Array(...), { showHidden: true }), whose output this PR intentionally changes (it now walks prototype getters, matching Node). Consider adding a standalone assertion on the new output right after the loop so a future edit that re-adds Float16Array tobuiltInObjects— or otherwise changes its prototype walk — trips a test.Extended reasoning...
What changed
builtInObjectsgatesaddPrototypePropertiesat inspect.js:1240/:1295: when a constructor name is in the set, showHidden stops at the typed-array's own hidden fields; when it is absent, showHidden also walks up the prototype chain and prints inherited getters.Before this PR, the live
globalThisscrape included"Float16Array", soutil.inspect(new Float16Array(2), { showHidden: true })produced the same shape asFloat32Arrayand passed the shared loop assertion at util-inspect.test.js:264. After this PR,"Float16Array"is deliberately excluded from the frozen 47-name set, so the same call now additionally emitsFloat16Array.prototype's getters — the PR description explicitly calls this out as an observable, Node-matching asymmetry.Why this is a coverage gap
The old assertion would now fail, so it was removed from both loops with an inline comment. That satisfies the letter of REVIEW.md's "every deletion needs a stated reason or replacement", but not the stronger rule that "every behavioral change ships an automated test in the same PR": the new Float16Array showHidden output — the very asymmetry the PR is implementing — has no positive assertion locking it in.
The remaining Float16Array coverage in this file doesn't reach this path:
- line 2058 is a
__proto__: nullcase (no prototype to walk) - line 2652 is the sorted-keys array test (doesn't call
inspecton a Float16Array instance withshowHidden)
Concrete regression scenario
- A future contributor, seeing that
Float16Arrayis a language global just likeFloat32Array, adds it to thebuiltInObjectsarray in inspect.js. util.inspect(new Float16Array(2), { showHidden: true })reverts to the pre-PR shape (no prototype getters), diverging from Node again.bun bd test util-inspect.test.jsis green — nothing in the suite asserts on Float16Array's showHidden output anymore, and the loop at line 247 still doesn't include it.- The Node-compat regression ships undetected.
Suggested fix
Add one assertion right after the first loop (or anywhere in this test body) that pins the actual Node-matching output, e.g.:
// Float16Array is NOT in builtInObjects, so showHidden walks its prototype. assert.match( util.inspect(new Float16Array(2), { showHidden: true }), /\[Symbol\(Symbol\.toStringTag\)\]: 'Float16Array'/, );
(or an exact
strictEqualon whatever Node v26.3.0 prints — whichever is stable across JSC prototype-getter ordering). This is a one-liner and turns the inline comment's claim into something CI enforces.Severity
Nit — this is missing test coverage for a secondary consequence of the fix, not a runtime defect. The primary Buffer/URL/
%sbehavior is well-covered by the new util-format.test.js assertions. - line 2058 is a
- %s subclass fixture: use MyURL (honors new.target) and setPrototypeOf for the Buffer subclass so the extra prototype hop is actually walked - Float16Array: add a positive showHidden assertion that locks in the prototype-getter walk, instead of only dropping it from the loop - Extract the ~10k-inspect surrogate-pair loop into its own test with a 30s timeout, dropping the file-wide prettier-ignore and keeping the large ported test body formatted
|
Addressed the three review findings in d9ce557:
|
… raising the timeout Release builds still sweep every code point; debug+ASAN samples every 17th (coprime with the high/low-surrogate boundary), which hits both branches and brings the test from ~6s to ~0.4s.
There was a problem hiding this comment.
LGTM — all three earlier review points are addressed in the current revision.
What was reviewed:
builtInObjectsis consumed at three sites (hasBuiltInToString,getConstructorName,addPrototypeProperties); the Float16ArrayshowHiddenassertion locks in the intended behavior at the latter two.- Subclass fixtures now genuinely exercise the extra prototype hop (
MyURLhonorsnew.target;MyBuffervia explicitsetPrototypeOf). - Extracted surrogate-pair test strides by 17 under debug with no explicit timeout; both high/low-surrogate branches are still reached (0xd800 and 0xdc0d).
Extended reasoning...
Overview
Replaces the live Object.getOwnPropertyNames(globalThis) scrape in src/js/internal/util/inspect.js with a hardcoded 47-name SafeSet matching what Node v26.3.0 observes at bootstrap. Adds %s coverage for Buffer/URL/URLSearchParams (plus subclass and negative-control cases) to util-format.test.js, drops Float16Array from the two typed-array showHidden loops in util-inspect.test.js and adds a positive assertion locking in its divergent output, and extracts the ~10k-call surrogate-pair loop into its own test that strides by 17 under isDebug.
Security risks
None. Pure output-formatting compat change in a JS builtin; no untrusted-input parsing, no allocation sizing, no auth/crypto surface.
Level of scrutiny
Moderate — Node-compat behavior change in a widely-used module (util.format/util.inspect), but the change is a data-only substitution (a smaller constant set) whose three consumers I traced. The set only shrinks, so the effect is monotone: fewer classes get treated as "built-in", meaning more user-defined-looking toStrings are honored by %s and more prototypes are walked under showHidden — both of which move Bun toward Node's observed behavior. The deliberate exclusions (SharedArrayBuffer, Float16Array, WebAssembly, DisposableStack, etc.) are explained in the PR body and one of them is pinned by a test.
Other factors
This PR has been through two rounds of my own review; all three findings (redundant MyBuffer fixture, 1580-line prettier-ignore scope, raise-timeout-instead-of-shrink-workload) are resolved in 38e2f55/d9ce5574 and visible in the final diff. The comment-cop bot's "paragraph-long comment" warning was addressed by trimming to three lines, and the remaining comment is load-bearing (prevents a future Node-sync from restoring the scrape). Test coverage includes both directions: values that must now take String() (Buffer, URL, URLSearchParams, subclasses) and values that must still take the inspect path ([1, 2], Uint8Array). The author noted #36141 supersedes this with a broader native-console fix, but also stated either can land; this one is the minimal, self-contained version.
2 participants
Reproduction
util.format('%s', Buffer.from('ab'))ab<Buffer 61 62>abutil.format('%s', new URLSearchParams('a=1'))a=1URLSearchParams {}a=1Any logger built on
util.format(winston, pino-pretty style%sinterpolation) printed<Buffer ...>noise where Node prints the decoded content, andURLSearchParams {}where Node prints the query string.Cause
hasBuiltInToStringinsrc/js/internal/util/inspect.jsclassifies an inheritedtoStringas "built-in" when the prototype that owns it has a constructor whose name is inbuiltInObjects. Node populates that set at bootstrap fromObject.getOwnPropertyNames(globalThis), at which point only the ECMAScript language intrinsics are present.Buffer,URL,URLSearchParams,Request, and every other Node/Web global are installed later, so they never appear in Node's set.Bun already has those globals on
globalThiswhen this module loads, so the live scrape collected 115 names.hasBuiltInToString(Buffer.from('ab'))walked toBuffer.prototype, saw constructor name"Buffer", found it in the set, returnedtrue, and%stook the inspect branch instead of callingBuffer.prototype.toString.Fix
Replace the dynamic scrape with the 47 names Node v26.3.0 observes at bootstrap:
This deliberately excludes
SharedArrayBuffer,WebAssembly,Float16Array,DisposableStack,AsyncDisposableStackandSuppressedError. Those are language globals too, but V8 installs them after Node computes the set, so they are absent from Node'sbuiltInObjectsand the difference is observable:util.inspect(new Float16Array(1), { showHidden: true })walks intoFloat16Array.prototypein Node, whileFloat32Arraydoes not. Matching Node means matching that asymmetry.Buffer,URL,URLSearchParamsand any user class with atoStringnow take theString()path through%s. ECMAScript built-ins (Array,Error,Map,Date, boxed primitives, ...) keep the inspect path as before.The
util-inspect.test.jstyped-arrayshowHiddenloops dropFloat16Array(upstream'stest-util-inspect.jsnever had it), and that 1500-line test body gets a debug-build timeout: its surrogate-pair loop runs ~10kutil.inspectcalls and already overshoots the 5s default underbun bdon main.Verification
Also green:
test/js/bun/util/inspect.test.js,test/js/node/util/bun-inspect.test.ts,test/js/node/util/util.test.js,test/js/node/util/custom-inspect.test.js,test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js,test/js/node/util/node-inspect-tests/parallel/util-inspect-proxy.test.js,test/js/node/console/,test/js/bun/console/,test/js/web/console/.The global
console.log('%s', ...)throw onSymbol/null-prototype/revoked-proxy is a separate native-side path (PercentTag::SinConsoleObject.rs); #34603 addresses that.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js