util: node v26.3.0 compat — styleText hex/validateStream, getCallSites, regexp highlighting, vm namespace, tty.WriteStream + deepEquals fixes (test-util 70% → 83%) - #34434
Conversation
Brings node:util closer to node v26.3.0 and fixes several deepEquals bugs
found by the newly ported tests.
util.styleText was still on the pre-v26 API. Port node v26.3.0's version:
hex/RGB colors (#RGB and #RRGGBB), the "none" format, nested close-code
handling, and the { validateStream, stream } option, which suppresses color
when the target stream is not a TTY.
Add util.getCallSites (capturing through a private prepareStackTrace so a
user-installed Error.prepareStackTrace is never invoked and
Error.stackTraceLimit does not bound the result) and
util.convertProcessSignalToExitCode. CallSite gains getScriptId, backed by
the source ID the stack frame already carries.
Port node v26.3.0's highlightRegExp so util.inspect syntax-highlights
regular expressions by group depth, replacing the flat "red" style.
Bun.deepEquals fixes, all strict-mode only so loose comparison
(expect().toEqual) is unchanged:
- boxed Strings ignored extra own properties, so Object('test') with an
added property compared equal to a plain Object('test')
- boxed Symbols and BigInts are plain ObjectType in JSC, so two different
boxed Symbols compared equal on properties alone
- the fast path fetched properties from the right-hand side without
checking enumerability, so a non-enumerable property satisfied an
enumerable one
- typed arrays never reached the constructor check, so Buffer and
Uint8Array with equal bytes compared equal
- typed arrays skipped own non-index properties (e.g. symbols)
The typed-array property walk is gated on the structure actually carrying
named properties: elements live in the vector, so an unconditional
fall-through would enumerate every index and make each comparison
O(elements).
util.isDeepStrictEqual takes node v26's third skipPrototype argument,
threaded through Bun__deepEquals as a template parameter.
Tests: add test-util-{convert-signal-to-exit-code,getcallsites,
getcallsites-preparestacktrace,inspect-regexp,isDeepStrictEqual,
styletext-hex} from v26.3.0 verbatim, and update test-util-styletext.js,
which was a stale pre-v26 copy asserting that styleText colors
unconditionally — v26 defaults validateStream to true.
|
Updated 9:05 PM PT - Jul 23rd, 2026
❌ @cirospaciari, your commit 7b1d0c7 has some failures in 🧪 To try this PR locally: bunx bun-pr 34434That installs a local version of the PR into your bun-34434 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
…tes columns
The strict boxed-String fall-through sent plain boxed strings into the
index-enumerating slow path: comparing two new String('a'.repeat(100000))
took 2.0s (1000 chars took 21ms, scaling linearly) where it had been a
single string compare. StringObject overrides getOwnPropertySlot, so
canPerformFastPropertyEnumeration() is false and the property walk
enumerates every character. Guard the fall-through on the structure
actually carrying named properties, the same way the typed-array case
does, and share the check as hasNamedOwnProperties(). Back to 7.6us.
getStylizeWithColor, the stylize handed to [util.inspect.custom]
implementations, still only resolved color names, so a custom inspector
asking for the 'regexp' style got plain text while the default formatter
highlighted it. Check for a function style there too, and check it before
the inspect.colors lookup in both stylizers: the lookup stringifies
highlightRegExp's source into a property key otherwise.
getCallSites reported columnNumber one less than node (bun's
CallSite#getColumnNumber() is 0-based, V8's is 1-based) and omitted node's
`column` alias. Verified against the node v26.3.0 binary.
Restore two primordials the port dropped: node guards the hex color regexp
with RegExpPrototypeExec and backs the hex style cache with a SafeMap, so
userland cannot break hex validation by patching RegExp.prototype.exec, or
misdirect the LRU eviction by patching Map.prototype.keys/size.
Also honor skipPrototype for boxed String subclasses (a String subclass is
DerivedStringObjectType, which the case rejected before comparing values;
node returns true there), add the missing RETURN_IF_EXCEPTION after the
swapped specialObjectsDequal call now that it can throw, pair
outOfLineSize() with inlineSize() as the existing idiom in this file does,
and move the typed-array constructor check after the memcmp so unequal
arrays don't pay two calculatedClassName allocations.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe PR expands Node utility APIs, adds prototype-skipping deep equality, introduces token-level regexp inspection and shared metadata formatting, updates VM namespace behavior, and adds WriteStream fallback handling with corresponding tests. Node utility APIs
Deep equality behavior
Regular expression and object inspection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/util/inspect.js`:
- Around line 959-963: Cache regexpString.length in a local variable at the
start of highlightRegExp, then replace all repeated regexpString.length accesses
within the parser, including the checks near the referenced locations, with that
cached value.
- Around line 969-976: Update the palette construction and write() flow around
paletteNames and palette so palettes containing only unknown color names cannot
cause an undefined dereference. When no entries resolve through inspect.colors,
fall back to the valid default palette or return the uncolored string, while
preserving custom valid colors and existing colored output behavior.
- Line 880: Update the highlighter around highlightRegExpColors to use the
captured slice intrinsic instead of the palette’s .slice method, replace the
for...of palette construction with indexed traversal, and guard
inspect.colors[style] access by first checking typeof style === "function".
- Around line 873-878: Update the color stylizer around the inspect style
handling and its corresponding UtilInspect typing/imports to support
function-valued inspect.styles entries. Invoke a style function with the
inspected value, while preserving existing inspect.colors lookup for
string-valued styles, so inspect.styles.regexp uses highlightRegExp instead of
plain text.
In `@src/js/node/util.ts`:
- Around line 423-429: In the frameCount options handling and the corresponding
second branch, read options.sourceMap once into a local variable before
validation, then use that variable for the undefined check and validateBoolean
call. Preserve the existing validation behavior while avoiding repeated property
access and getter invocation.
- Around line 420-458: Update getCallSites to apply options.sourceMap when
preparing and returning call sites, rather than only validating it. Ensure true
and false explicitly override the global source-map setting, while an omitted
option preserves the existing global behavior; keep frameCount validation and
stack-trace restoration unchanged.
- Around line 266-276: The color conversion path uses mutable prototype methods
instead of primordial-safe helpers. Update hexToRgb and the related color
parsing logic to use captured $ string/regular-expression helpers for slicing,
index lookup, and RegExp execution, and replace cache.keys().next().value with
cache.$keys(). Preserve the existing color parsing and error behavior.
- Around line 405-418: Update the call-site capture flow around prepareCallSites
to use Bun’s private/native capture mechanism instead of modifying
Error.prepareStackTrace or Error.stackTraceLimit. Ensure capture occurs without
changing public Error state, including when stackTraceLimit access throws, while
preserving prepareCallSites’ conversion of captured frames into the existing
result shape.
In `@src/jsc/bindings/bindings.cpp`:
- Around line 734-738: After the second swapped-argument specialObjectsDequal
call in the equality flow, add the same RETURN_IF_EXCEPTION(scope, false) check
used after the first call before evaluating isSpecialEqual.has_value().
🪄 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: 252b0ae6-ab92-4787-9fe4-f4f19f5a486c
📒 Files selected for processing (15)
src/js/internal/util/inspect.jssrc/js/node/util.tssrc/jsc/bindings/BunObject.cppsrc/jsc/bindings/CallSite.cppsrc/jsc/bindings/CallSite.hsrc/jsc/bindings/CallSitePrototype.cppsrc/jsc/bindings/bindings.cppsrc/jsc/bindings/headers-handwritten.htest/js/node/test/parallel/test-util-convert-signal-to-exit-code.mjstest/js/node/test/parallel/test-util-getcallsites-preparestacktrace.jstest/js/node/test/parallel/test-util-getcallsites.jstest/js/node/test/parallel/test-util-inspect-regexp.jstest/js/node/test/parallel/test-util-isDeepStrictEqual.jstest/js/node/test/parallel/test-util-styletext-hex.jstest/js/node/test/parallel/test-util-styletext.js
…TY env
oxlint's no-duplicate-conditional-property-access flagged five reads in the
new code: hoist regexpString.length once in highlightRegExp, and read
options.sourceMap through a small validateSourceMapOption helper.
Three suites asserted the pre-v26 styleText behaviour, where colour was
emitted unconditionally. styleText now defaults validateStream to true, so
util.test.js opts out with { validateStream: false } the way node's own
test does. The vendored util-inspect test drove testColorStyle('regexp'),
which assumes every style is a colour name; node v26.3.0 dropped that call
when regexp became a function, so drop it here too, with node's comment.
test-util-styletext{,-hex}.js assert styleText's own colour decisions
against a TTY, which the runner's forced FORCE_COLOR=0/NO_COLOR=1 makes
unreachable — node would fail them too. Let those two files see the real
environment, the way the runner already special-cases other files.
Revert the Buffer/Uint8Array distinction and restore its pin in the
deep-equality matrix. It is correct (node rejects them, and the matrix
marks bun's answer as a known bug), but it surfaces a separate defect:
bun's advanced child_process serialization is backed by
SerializedScriptValue, which downgrades a Buffer to a Uint8Array, so
test-child-process-advanced-serialization.js only passes today because two
bugs cancel out. Preserving Buffer through structured clone belongs in its
own change. test-util-isDeepStrictEqual.js depends on that distinction, so
it goes with it.
Cover the fixes that remain in the deep-equality matrix instead: distinct
boxed symbols, unequal boxed bigints, a boxed string with an extra own
property, a non-enumerable symbol key against an enumerable one, typed
arrays differing only in a symbol property, and a skipPrototype block.
Every expectation was taken from the node v26.3.0 binary, and the typed
array with an extra own property is no longer a known strict-mode bug.
cirospaciari
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/bindings/bindings.cpp:1630-1648— The Symbol/BigInt wrapper check here is not gated byif constexpr (isStrict), so looseBun.deepEquals/expect().toEqualnow returnsfalseforObject(1n)vsObject(2n)andObject(Symbol())vs{}where it previously returnedtrue. The same is true of the enumerability filter at lines 905-913. Both changes are arguably improvements (they match the existing Number/Boolean wrapper handling and fix a pre-existing asymmetry), but they contradict the PR description's claim that loose comparison is "byte-for-byte unchanged" — worth either gating onisStrictor updating the description and adding loose-mode coverage.Extended reasoning...
What changed in loose mode
The PR description states under
Bun.deepEqualscorrectness: "all strict-mode only, so loose comparison (expect().toEqual) is byte-for-byte unchanged". Two of the new checks are not actually gated onisStrict, so they run forBun.deepEquals(a, b)(loose) and therefore forexpect().toEqual():- Symbol/BigInt wrapper check (
bindings.cpp:1630-1648) — the new post-switch block that unboxesSymbolObject/BigIntObjectand compares internal values viasameValue. The three other new checks in this function (StringObject fallthrough, typed-array class-name, typed-array own-property fallthrough) are all wrapped inif constexpr (isStrict); this one is not. - Enumerability filter in the fast path (
bindings.cpp:905-913) — the replacement ofo2->getDirect(vm, PropertyName(entry.key()))with a structure lookup that skipsDontEnumproperties. Also un-gated.
Step-by-step: Symbol/BigInt wrapper (loose mode)
Take
Bun.deepEquals(Object(1n), Object(2n))(no third arg →isStrict = false).Before this PR: both operands are plain
ObjectTypein JSC, so the switch inspecialObjectsDequalhitsdefault: break, returnsstd::nullopt, and control falls through to the generic property walk. Both wrappers have zero enumerable own properties, so they compare equal →true.After this PR: the switch still hits
default: break, but the new block at 1630 runs:obj1->inherits<BigIntObject>()is true,obj2->inherits<BigIntObject>()is true,sameValue(1n, 2n)is false →false.Similarly,
Bun.deepEquals(Object(Symbol()), {})previously fell through to the property walk (both empty →true), but now hits theelse if (obj2->inherits<SymbolObject>() || obj2->inherits<BigIntObject>())branch on the secondspecialObjectsDequalcall (with args swapped) →false.Step-by-step: enumerability filter (loose mode)
Take
o1 = {x: 1}ando2 = Object.defineProperty({}, 'x', {value: 1, enumerable: false}), both plain objects taking the fast path with different structures (theelsebranch at line 896).Before:
forEachProperty(o1)visitsx,left = 1,right = o2->getDirect(vm, 'x') = 1(getDirect ignores enumerability), values match,count = 1. Reverse loop overo2skipsx(DontEnum). Result →true.After:
o2Structure->get(vm, 'x', o2Attributes)returns a valid offset withDontEnumset, sorightstays empty.left(1) is not undefined, so the!isStrictearly-continue at 915-919 doesn't fire.!rightat 921 →false.Note also that the slow path at lines ~987-1016 still uses
o2->getIfPropertyExists(), which finds non-enumerable properties, so for objects that miss the fast path (e.g. accessors present) loose mode would still returntruefor this same input — the fast and slow paths now disagree.Why this is a nit rather than a blocker
The new loose-mode results are more correct, not less:
- The pre-existing
NumberObjectType/BooleanObjectTypecase at lines 1611-1622 already comparesinternalValue()without anisStrictgate, so treating Symbol/BigInt wrappers the same way is consistent, and matches Jest (which unboxes viaObject.is). - The old loose-mode enumerability behavior was asymmetric: with the same
o1/o2above,deepEquals(o1, o2)returnedtruebutdeepEquals(o2, o1)returnedfalse(forward loop skipped o2's DontEnumxsocount = 0; reverse loop found o1's enumerablexwithremain == 0→ false). The new code makes both directions agree onfalse, which is what Jest'stoEqualdoes (non-enumerable properties are invisible).
So the code is defensible; only the PR description's "byte-for-byte unchanged" claim is inaccurate.
Suggested action
Either wrap both blocks in
if constexpr (isStrict) { ... }to make the description true, or (probably better) drop the "byte-for-byte unchanged" claim from the description, note that loose mode also picks up the wrapper/enumerability fixes, and add a couple of loose-mode assertions (expect(Object(1n)).not.toEqual(Object(2n)), and the enumerable/non-enumerable pair) so the delta is covered. If keeping the loose-mode change, the slow path'sgetIfPropertyExistsat ~1000 should also be aligned so fast and slow paths agree. - Symbol/BigInt wrapper check (
A Module Namespace Exotic Object is specified to have a null [[Prototype]].
Bun points the shared namespace structure at an object carrying an
__esModule accessor for CJS interop, which is fine for real imports but
leaves vm module namespaces inspecting as
"Module <[Object: null prototype] {}> [Module] { ... }" instead of node's
"[Module: null prototype] { ... }". Null the prototype in
NodeVMModule::namespaceObject so only vm modules are affected; real ESM
namespaces keep the accessor. Adds test-util-inspect-namespace.js.
hasBuiltInToString rejected any Symbol.toPrimitive, but
Date.prototype[Symbol.toPrimitive] is built-in, so util.format('%s', date)
printed the toString form where node prints inspect's ISO form. Port node
v26.3.0's version, which distinguishes an own Symbol.toPrimitive from an
inherited one.
Port node v26.3.0's extraKeys mechanism, replacing unshift(keys, ...) for
typed arrays, ArrayBuffer/SharedArrayBuffer and DataView. These entries are
getters rather than own properties, so node brackets them: ArrayBuffer now
inspects as "{ [Uint8Contents]: <..>, [byteLength]: 4 }", and showHidden on
a typed array reports [BYTES_PER_ELEMENT], [length], [byteLength],
[byteOffset] and [buffer].
The styletext tests assert styleText's own color decisions, so the runner
must not force color for them. Deleting the forced FORCE_COLOR=0/NO_COLOR=1
was not enough: spawnBun sets FORCE_COLOR=1 in its own base env, which then
reached the child and colored output the test expects plain. Set the three
color variables to undefined instead, which child_process drops. With no
color variables set, bun matches node v26.3.0 on all eight TTY cases.
buffer.test.js redefines `length` as an own enumerable property and then
compares the buffer to a plain one. Node reports those as deep-unequal for
exactly that reason (verified against the v26.3.0 binary), which bun now
does too, so compare the bytes instead — the test is about fill() not
aborting.
… requires
The enumerability filter in the fast path was not gated on strict, so loose
comparison started rejecting an enumerable property matched against a
non-enumerable one. Node accepts that pair in loose mode (verified against
the v26.3.0 binary), so gate the check and let loose keep matching either.
The three other loose-visible changes — distinct boxed symbols, unequal
boxed bigints, a boxed symbol against {} — all now agree with node, so they
stay; the matrix covers loose for each.
internal/util/colors runs refresh() at load, touching process.stderr and
reading FORCE_COLOR, and internal/streams/utils is only needed for stream
validation. Requiring both at the top of node:util gave `require("util")` an
observable side effect it did not have before. Load them inside styleText's
validateStream branch instead.
Drop test-util-getcallsites.js. It opens with a `node_use_amaro` guard that
is undefined in bun, so common.skip() exited before a single assertion ran —
the file passed while testing nothing. Making it run shows it cannot pass:
it asserts getCallSites() sees more than one frame at module scope, which
holds in node only because node wraps modules in JS functions
(8 frames vs bun's 1, whose loader is native). getCallSites keeps its real
coverage in test-util-getcallsites-preparestacktrace.js.
|
Thanks — the loose-mode finding was right, and sharper than stated. I checked all four loose-visible cases against the real node v26.3.0 binary:
So three of the loose changes moved bun onto node's behaviour and are worth keeping — but the enumerability filter was a real regression: node accepts that pair in loose mode and bun had started rejecting it. Gated that one on The PR description was simply wrong to say loose was unchanged — I've corrected it. Also actioned from this review:
Two findings I deliberately did not action (the empty regexp palette, and the style-cache TypeError): both reproduce identically on the real node v26.3.0 binary, so they're upstream bugs this port faithfully reproduces. Diverging would defeat the point of the port, but happy to fix if preferred. |
|
Flagging the overlap the duplicate-detector found, so maintainers can pick rather than discover it in review. This PR overlaps four open PRs:
I didn't check for existing PRs before building this, which is on me. Rather than unpick it, I've left this one as the superset — it additionally carries regexp syntax highlighting, Happy to go either way: I can drop the overlapping areas and rebase on whichever of those lands first, or close this in favour of them. Just say which — no attachment to this particular diff. |
cirospaciari
changed the title
…ters Removing formatTypedArray's showHidden block left its last two parameters unused, which the src/js lint rejects. They are positional — the formatter is called through a bind — so prefix them rather than drop them, matching node, which keeps the same signature for every formatter. The vendored util-inspect test still carried the pre-v26 expectations for ArrayBuffer, DataView and typed arrays: byteLength/byteOffset/buffer unbracketed. Node v26 surfaces those getters through extraKeys, which brackets them, so update the expectations to match — including the three cases where the longer bracketed keys push the line past the default break length and node's own test now passes breakLength: 82.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/util/inspect.js`:
- Around line 1691-1695: Update the extra-property handling around
formatExtraProperties so the fallback catches only failures while reading the
metadata value, not exceptions thrown by recursive formatting. Restore
ctx.indentationLvl to its pre-call value before executing the fallback, then
retry formatting with the intended buffer-derived value while preserving normal
recursive errors.
🪄 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: 105041ce-6666-474a-ae20-6a48b3d7a356
📒 Files selected for processing (11)
scripts/runner.node.mjssrc/js/builtins/UtilInspect.tssrc/js/internal/util/inspect.jssrc/js/node/util.tssrc/jsc/bindings/NodeVMModule.cppsrc/jsc/bindings/bindings.cpptest/js/node/assert/deep-equal.test.tstest/js/node/buffer.test.jstest/js/node/test/parallel/test-util-inspect-namespace.jstest/js/node/util/node-inspect-tests/parallel/util-inspect.test.jstest/js/node/util/util.test.js
The x64-asan shard sets CI but not BUILDKITE, and getColorDepth's CI branch returns "no color" when it can't identify the vendor — so styleText correctly emitted nothing where the test wanted color. Node's own getColorDepth does the same, so this was the environment, not the runtime. Reproduced it locally over a pty with CI=true and no vendor variable: case "isTTY=true with no env vars" fails exactly as on CI, and passes with CI dropped and TERM pinned. Do that for these two files so every agent agrees; all eight TTY cases then match node v26.3.0. Also bracket the SharedArrayBuffer byteLength in the vendored util-format test, the last pre-v26 expectation left over from the extraKeys port.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
test-util-styletext-hex.js can't pass here, and the runner can't fix it: the file uses node:test, so it runs under `bun test`, and `bun test` puts NO_COLOR=1 into process.env for its own output. styleText then correctly reports no color, and the file's TTY block expects color. Reproduced over a pty with every color variable unset and CI dropped -- the child still sees NO_COLOR=1 and that one case fails, 30 pass 1 fail. That is bun test's environment, not styleText. The useful assertions in that file are the hex ones, which do not depend on the environment at all, so carry them in bun's own util test rather than lose the coverage: 6-digit, case-insensitivity, 3-digit expansion, combining with named formats, nesting, `none`, and seven invalid spellings. Output diffed against the node v26.3.0 binary -- identical on all 14 cases. Also gate the Symbol/BigInt wrapper check on ObjectType, per review: it ran for every pair reaching the end of the switch, costing four inherits<>() walks on the recursive path for plain objects and arrays, which have their own JSType. The else-if went with it -- the swapped second call already covers the c2-is-wrapper direction.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/bindings/bindings.cpp (2)
1092-1150: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake Set/Map fallback matching one-to-one.
The fallback search can reuse the same entry for multiple distinct keys because it never records matched candidates. With
skipPrototype, this can report unequal collections as equal—for example, two left-side keys matching the first right-side key while the second right-side key is different. Track consumed candidates; for Maps, match key/value pairs before marking an entry consumed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/bindings.cpp` around lines 1092 - 1150, The Set/Map fallback matching in Bun__deepEquals must enforce one-to-one candidate usage. Track consumed entries during fallback searches so each right-side Set value or Map entry can match only once; for Maps, compare both key and value before marking a candidate consumed, while preserving exception handling and normal fast-path behavior.
1442-1454: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not bypass strict own-property checks on typed-array fast paths.
The new
hasNamedOwnPropertiesfallback is unreachable when Lines 1387-1401 return early. Strict comparisons of empty or aliased typed arrays with differing named properties can therefore incorrectly returntrue. Move this guard before both fast returns and fall through to the property walk when named properties exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/bindings.cpp` around lines 1442 - 1454, Update the typed-array comparison logic before the early returns around the fast-path checks so that, when isStrict and either structure has named own properties, execution falls through to the existing property walk. Ensure this guard applies to empty and aliased arrays as well as the memcmp path, while preserving fast returns when no named properties exist and loose-mode behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/jsc/bindings/bindings.cpp`:
- Around line 1092-1150: The Set/Map fallback matching in Bun__deepEquals must
enforce one-to-one candidate usage. Track consumed entries during fallback
searches so each right-side Set value or Map entry can match only once; for
Maps, compare both key and value before marking a candidate consumed, while
preserving exception handling and normal fast-path behavior.
- Around line 1442-1454: Update the typed-array comparison logic before the
early returns around the fast-path checks so that, when isStrict and either
structure has named own properties, execution falls through to the existing
property walk. Ensure this guard applies to empty and aliased arrays as well as
the memcmp path, while preserving fast returns when no named properties exist
and loose-mode behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8420cea9-4ada-4900-89a3-1a4f04741b47
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/node/util/util.test.js
node's tty.WriteStream accepts a read-only descriptor -- common/index.js's
getTTYfd() hands it one from fs.openSync('/dev/tty') whenever stdout and
stderr are piped, which is how the macOS CI agents run. bun's fs.WriteStream
eagerly opens a fast writer for any fd it is given, and that fails on such a
descriptor with EINVAL from kqueue, so the constructor threw where node
returns a usable stream. Fall back to the general write path when the fast
writer can't be created; the failure then surfaces at write time, as it does
in node. Verified over a pty: `new tty.WriteStream(fs.openSync('/dev/tty'))`
now succeeds in bun as it does in node, and test-util-styletext.js passes.
Also extend the boxed-primitive guard to indexed storage, per review. A
boxed string carrying an out-of-range index (`new String('ab')[5] = 'x'`) has
an own property that the contents comparison misses, and the structure's
named-slot counts don't see it. node reports those as unequal; bun now does
too. Typed arrays don't carry that indexing type, so the fast path stays
flat -- Uint8Array(65536) still compares in ~3us.
Checked the surrounding fs suite for fallout: 572 pass / 5 fail both with
and without the WriteStream change, the same five pre-existing failures
(three of them 5s-90s timeouts).
…o the slow path The hoisted guard in be5cfc8 skipped byteLength/memcmp entirely when a typed array had any extra own property, which (a) lost node's byte-level semantics for the one case where sameValue and memcmp diverge the other way (distinct NaN payloads now compared equal via the property walk) and (b) made a byte-mismatch comparison O(elements) instead of O(bytes). Compute the extras flag up front but only divert the equal-bytes exits to the property walk; mismatches still return false from the byte checks. Matrix coverage added for the NaN-payload case.
There was a problem hiding this comment.
No new issues found in 2a1ad47 — the byte-checks-first reordering addresses the NaN-payload/perf concern from the last round. Deferring to a human for the scope call: this threads a new skipPrototype template parameter through Bun__deepEquals (backs every toStrictEqual/assert.deepStrictEqual), changes loose-mode answers for boxed Symbol/BigInt, adds three new util APIs, and the description flags overlap with four open PRs.
What was reviewed:
bindings.cpptyped-array path — byteLength/memcmp now run before the extra-own-properties divert; empty, shared-vector, and NaN-payload exits all covered in the matrix.getCallSites—prepareStackTrace/stackTraceLimitrestoration under a non-writable limit is inside try/finally and tested.styleTextskipColorize loop — validation still runs on every key when colorize is skipped;noneand cached-hex paths don't bypassvalidateOneOf.WriteStreamfast-path fallback —fd != nullhandles fd 0; failed writer open now clearsfastPathsokWriteStreamFastPathisn't left asundefinedwhen fd is set.
Extended reasoning...
Overview
22 files spanning src/js/node/util.ts (styleText v26 rewrite, getCallSites, convertProcessSignalToExitCode, isDeepStrictEqual skipPrototype), src/js/internal/util/inspect.js (~250-line highlightRegExp port, extraKeys/formatExtraProperties, hasBuiltInToString rewrite), src/jsc/bindings/bindings.cpp (adds skipPrototype template param to Bun__deepEquals/specialObjectsDequal, boxed Symbol/BigInt handling, enumerability check on the fast property path, boxed-String and typed-array extra-own-property fall-through), CallSite.{h,cpp}/CallSitePrototype.cpp (getScriptId), NodeVMModule.cpp (null the vm namespace prototype), src/js/internal/fs/streams.ts (WriteStream fast-path try/catch), scripts/runner.node.mjs (env override for two styletext tests), plus ~1000 lines of ported/updated tests.
Security risks
None identified. No auth/crypto/permissions surface. styleText and highlightRegExp operate on caller-supplied strings and only emit ANSI sequences; the runner env change is scoped by filename to two util tests.
Level of scrutiny
High. Bun__deepEquals is the correctness primitive behind expect().toStrictEqual, assert.deepStrictEqual, and util.isDeepStrictEqual; the PR both adds a template dimension and changes strict- and loose-mode answers for several inputs (boxed Symbol/BigInt, enumerability, boxed String extras, typed-array extras). It also adds public API (getCallSites, convertProcessSignalToExitCode, styleText hex/validateStream) and overlaps four open PRs per the author — all things a maintainer should sign off on.
Other factors
The PR has been through several automated review rounds; every prior finding (loose-mode enumerability regression, non-writable stackTraceLimit leak, fd=0 truthiness, empty/shared-buffer typed-array early returns, NaN-payload semantics) has a corresponding fix commit and matrix case. CI on the previous build was 284/286 green with the two reds noted as unrelated. No outstanding unresolved review threads.
There was a problem hiding this comment.
No new issues found on 2a1ad47. This is a large change to Bun__deepEquals (backs expect().toStrictEqual and assert.deepStrictEqual) plus new public node:util API surface, so leaving final sign-off to a human.
What was reviewed on this revision:
- Verified the 2a1ad47 reorder in the typed-array case: byte mismatch now returns
falsebefore the property walk, andcompareOwnPropertiesonly diverts the three equal-bytes exits — the NaN-payload and O(bytes) concerns from the earlier thread are closed, with a matrix case pinning it. - Checked that the new loose-mode-visible paths (boxed Symbol/BigInt
ObjectTypeblock, strict-only enumerability check on the fast-property path) are gated so pre-existing loose behavior for plain objects is unchanged. - Checked
getCallSitesrestoresprepareStackTrace/stackTraceLimiton all exits including a non-writablestackTraceLimit, and thatstyleText'sskipColorizebranch still validates hex format beforecontinue.
Extended reasoning...
Overview
Node v26.3.0 node:util compat: rewrites styleText (hex/RGB, validateStream, nested close-code handling), adds getCallSites/convertProcessSignalToExitCode/isDeepStrictEqual(…, skipPrototype), ports highlightRegExp and the extraKeys/hasBuiltInToString rewrites into inspect.js, nulls the vm module namespace prototype, and adds a tty.WriteStream read-only-fd fallback in streams.ts. On the native side it threads a third skipPrototype template parameter through Bun__deepEquals, adds hasExtraOwnProperties gating for boxed strings and typed arrays, adds a boxed-Symbol/BigInt ObjectType block, adds an enumerability check to the strict fast-property path, and adds CallSite::getScriptId. 22 files, ~1000 lines changed.
Security risks
None identified. No auth/crypto/permissions surface. styleText and highlightRegExp operate on caller-supplied strings and produce ANSI output; the hex cache is bounded at 256 entries. getCallSites mutates and restores Error.prepareStackTrace/stackTraceLimit inside a try/finally with a nested best-effort try around the non-writable case.
Level of scrutiny
High. Bun__deepEquals is the shared implementation behind expect().toEqual/toStrictEqual, Bun.deepEquals, assert.deep[Strict]Equal, and util.isDeepStrictEqual, so a subtle regression fans out across the entire test runner. The PR intentionally moves three loose-mode cases onto Node's answer (boxed BigInt/Symbol identity), which is a user-visible behavior change for Bun.deepEquals(a, b) and expect().toEqual — the description documents this table but it's a call a maintainer should confirm. The inspect.js changes alter default output formatting ([byteLength] bracketing, regexp highlighting as a function-valued style), which is technically a Bun-native output change driven by Node compat.
Other factors
This revision has been through several review rounds: a human maintainer left detailed feedback and later adopted the PR, and earlier automated findings (fd=0 truthiness, empty/shared-buffer typed-array early returns, the NaN-payload/perf regression from the initial hoist) were all fixed with test coverage added. All inline threads are resolved. CI on the adopted revision was reported 284/286 green with the two reds pre-existing on main. The remaining known gaps (sourceMap not applied, [Foo: null prototype], Buffer≠Uint8Array in strict) are documented in the description as deliberate follow-ups. Given the breadth — a template-parameter change to a core comparison function, new public API, and default inspect-output changes — this warrants a maintainer's final look rather than bot approval.
|
@robobun fix conflicts |
…compat # Conflicts: # src/js/node/util.ts
…s, regexp highlighting, vm namespace, tty.WriteStream + deepEquals fixes (test-util 70% → 83%) (#34434) ## What Brings `node:util` closer to **node v26.3.0** and fixes several `Bun.deepEquals` bugs the newly ported tests exposed. Fixes #33074 Fixes #25736 Fixes #20129 ### `test-util-*` compatibility vs node v26.3.0 | | before | after | |---|---|---| | passing | 21 / 30 (70%) | **25 / 30 (83%)** | **+3 tests added** (verbatim from v26.3.0), **+1 updated**, plus new coverage in bun's own deep-equality matrix and util tests. Beyond `node:util`, this also fixes two bugs the porting turned up: `tty.WriteStream` throwing on a read-only fd, and vm module namespaces having a non-null prototype. > Overlaps four open PRs (#30985, #33080, #32872, #29037) — see [this comment](oven-sh/bun#34434 (comment)). Happy to drop the overlapping areas or close in favour of them. ## Fixes **`util.styleText` was still on the pre-v26 API.** Ported node v26.3.0's version: hex/RGB colors (`#RGB`/`#RRGGBB`), the `none` format, nested close-code handling, and `{ validateStream, stream }` — colour is suppressed when the target stream isn't a TTY. **New APIs** - `util.getCallSites(frameCount, { sourceMap })` — captures through a private `prepareStackTrace`, so a user-installed `Error.prepareStackTrace` is never invoked and `Error.stackTraceLimit` doesn't bound the result. `CallSite` gains `getScriptId`. Column numbers and the `column` alias verified against the node v26.3.0 binary. - `util.convertProcessSignalToExitCode(signal)` - `util.isDeepStrictEqual(a, b, skipPrototype)` — node v26's third argument, threaded through `Bun__deepEquals` as a template parameter. **`util.inspect`** - **Regexes are syntax-highlighted** by group depth (ported v26's `highlightRegExp`), replacing the flat `red` style — removing the `TODO(BridgeAR): Highlight regular expressions properly` bun inherited from node. - **`hasBuiltInToString`** rejected *any* `Symbol.toPrimitive`, but `Date.prototype[Symbol.toPrimitive]` is built-in, so `util.format('%s', date)` printed the `toString` form where node prints inspect's ISO form. Ported v26's version, which distinguishes own from inherited. - **`extraKeys`** — ported v26's mechanism replacing `unshift(keys, …)`. Those entries are getters, so node brackets them: `ArrayBuffer { [Uint8Contents]: <..>, [byteLength]: 4 }`, and `showHidden` on a typed array reports `[BYTES_PER_ELEMENT]`, `[length]`, `[byteLength]`, `[byteOffset]`, `[buffer]`. - **vm module namespaces get a null prototype.** A Module Namespace Exotic Object is specified to have `[[Prototype]] = null`; bun points the shared structure at an object carrying an `__esModule` accessor for CJS interop. Nulled in `NodeVMModule::namespaceObject` only, so real ESM namespaces keep the accessor. **`Bun.deepEquals` correctness (strict mode)** - boxed **Strings** ignored extra own properties - boxed **Symbols/BigInts** are plain `ObjectType` in JSC and never reached the type switch, so two *different* boxed Symbols compared equal - the fast path fetched the right-hand property without checking **enumerability** - typed arrays skipped own **non-index properties** (e.g. symbols) **Loose mode** is *not* untouched (an earlier revision of this description wrongly said so). Three loose-visible cases moved onto node's behaviour, and one regression was caught in review and gated back: | loose case | node | bun before | bun now | |---|---|---|---| | `Object(1n)` vs `Object(2n)` | false | true | **false** | | `Object(Symbol())` vs `{}` | false | true | **false** | | `Object(Symbol('a'))` vs `Object(Symbol('a'))` | false | true | **false** | | enumerable sym vs non-enumerable | true | true | **true** | ## Perf Both strict fall-throughs are gated on the structure actually carrying named properties. Elements and characters are synthesized by `getOwnPropertySlot` rather than stored in the structure, so an unguarded fall-through lands in the index-enumerating slow path. Measured before the guard: `new String('a'.repeat(100000))` took **2,004,127µs/op** and a 1KB `Uint8Array` **4264µs/op**, both linear. With the guard both are flat (~5µs). ## Verification - A/B against a clean build of the same base commit, using the runner's own dispatch (`bun test` for files containing `node:test`, `bun run` otherwise). - Every expectation checked against a **real node v26.3.0 binary**, not read off the source. Two review findings (a `TypeError` from a primed style cache, and an empty regexp palette) **reproduce identically on node** and are deliberately left as faithful ports. - Regression sweep: `test-assert-*` / `test-buffer-*` / `test-console-*`, `buffer.test.js` (617), the 271-case deep-equality matrix, bun's `expect` suites, and 97/97 node vm tests. `oxlint` clean. ## Notes for reviewers **`test-util-styletext{,-hex}.js` need the real environment.** They assert styleText's own colour decisions against a TTY. Deleting the runner's forced `FORCE_COLOR=0`/`NO_COLOR=1` wasn't enough — `spawnBun` sets `FORCE_COLOR=1` in its own base env, which reached the child. The three colour variables are now set to `undefined` (which `child_process` drops) for those two files. With no colour variables set, bun matches node on all eight TTY cases. ## Known gaps (deliberate) - **`test-util-inspect.js`** — `--expose-internals`; needs `internalBinding('js_stream').JSStream`, whose `_externalStream` must be a real napi external. It gets ~145 assertions in before that line. - **`test-util-format.js`** — one assertion: `[Foo: null prototype]`. V8 keeps the constructor on the object's map; JSC has no equivalent. I tried recovering it from JSC's structure transition chain — `previousID()` yields nothing usable after a prototype change (verified across `setPrototypeOf`, `__proto__ =`, and `Reflect.setPrototypeOf`), so this needs a WebKit-side change. Everything else in the file passes. - **`test-util-isDeepStrictEqual.js`** — needs `Buffer` ≠ `Uint8Array` in strict mode. That's correct (node rejects them, and the matrix pins bun's answer as a known bug), but enabling it turns `test-child-process-advanced-serialization.js` red: bun's advanced IPC is backed by `SerializedScriptValue`, which downgrades `Buffer` to `Uint8Array`, so that test currently passes only because two bugs cancel out: ``` bun: received.buffer → Uint8Array, isBuffer: false node: received.buffer → Buffer, isBuffer: true ``` Preserving `Buffer` through structured clone belongs in its own change. - **`test-util-getcallsites.js`** — asserts `getCallSites().length > 1` at module scope, which holds in node only because node wraps modules in JS functions (8 frames vs bun's 1; bun's loader is native). Not portable. `getCallSites` keeps real coverage in `test-util-getcallsites-preparestacktrace.js`. - `getCallSites`'s `sourceMap` option is validated but not applied. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 11 · 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 <!-- 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>
3 participants
What
Brings
node:utilcloser to node v26.3.0 and fixes severalBun.deepEqualsbugs the newly ported tests exposed.Fixes #33074
Fixes #25736
Fixes #20129
test-util-*compatibility vs node v26.3.0+3 tests added (verbatim from v26.3.0), +1 updated, plus new coverage in bun's own deep-equality matrix and util tests.
Beyond
node:util, this also fixes two bugs the porting turned up:tty.WriteStreamthrowing on a read-only fd, and vm module namespaces having a non-null prototype.Fixes
util.styleTextwas still on the pre-v26 API. Ported node v26.3.0's version: hex/RGB colors (#RGB/#RRGGBB), thenoneformat, nested close-code handling, and{ validateStream, stream }— colour is suppressed when the target stream isn't a TTY.New APIs
util.getCallSites(frameCount, { sourceMap })— captures through a privateprepareStackTrace, so a user-installedError.prepareStackTraceis never invoked andError.stackTraceLimitdoesn't bound the result.CallSitegainsgetScriptId. Column numbers and thecolumnalias verified against the node v26.3.0 binary.util.convertProcessSignalToExitCode(signal)util.isDeepStrictEqual(a, b, skipPrototype)— node v26's third argument, threaded throughBun__deepEqualsas a template parameter.util.inspecthighlightRegExp), replacing the flatredstyle — removing theTODO(BridgeAR): Highlight regular expressions properlybun inherited from node.hasBuiltInToStringrejected anySymbol.toPrimitive, butDate.prototype[Symbol.toPrimitive]is built-in, soutil.format('%s', date)printed thetoStringform where node prints inspect's ISO form. Ported v26's version, which distinguishes own from inherited.extraKeys— ported v26's mechanism replacingunshift(keys, …). Those entries are getters, so node brackets them:ArrayBuffer { [Uint8Contents]: <..>, [byteLength]: 4 }, andshowHiddenon a typed array reports[BYTES_PER_ELEMENT],[length],[byteLength],[byteOffset],[buffer].[[Prototype]] = null; bun points the shared structure at an object carrying an__esModuleaccessor for CJS interop. Nulled inNodeVMModule::namespaceObjectonly, so real ESM namespaces keep the accessor.Bun.deepEqualscorrectness (strict mode)ObjectTypein JSC and never reached the type switch, so two different boxed Symbols compared equalLoose mode is not untouched (an earlier revision of this description wrongly said so). Three loose-visible cases moved onto node's behaviour, and one regression was caught in review and gated back:
Object(1n)vsObject(2n)Object(Symbol())vs{}Object(Symbol('a'))vsObject(Symbol('a'))Perf
Both strict fall-throughs are gated on the structure actually carrying named properties. Elements and characters are synthesized by
getOwnPropertySlotrather than stored in the structure, so an unguarded fall-through lands in the index-enumerating slow path. Measured before the guard:new String('a'.repeat(100000))took 2,004,127µs/op and a 1KBUint8Array4264µs/op, both linear. With the guard both are flat (~5µs).Verification
bun testfor files containingnode:test,bun runotherwise).TypeErrorfrom a primed style cache, and an empty regexp palette) reproduce identically on node and are deliberately left as faithful ports.test-assert-*/test-buffer-*/test-console-*,buffer.test.js(617), the 271-case deep-equality matrix, bun'sexpectsuites, and 97/97 node vm tests.oxlintclean.Notes for reviewers
test-util-styletext{,-hex}.jsneed the real environment. They assert styleText's own colour decisions against a TTY. Deleting the runner's forcedFORCE_COLOR=0/NO_COLOR=1wasn't enough —spawnBunsetsFORCE_COLOR=1in its own base env, which reached the child. The three colour variables are now set toundefined(whichchild_processdrops) for those two files. With no colour variables set, bun matches node on all eight TTY cases.Known gaps (deliberate)
test-util-inspect.js—--expose-internals; needsinternalBinding('js_stream').JSStream, whose_externalStreammust be a real napi external. It gets ~145 assertions in before that line.test-util-format.js— one assertion:[Foo: null prototype]. V8 keeps the constructor on the object's map; JSC has no equivalent. I tried recovering it from JSC's structure transition chain —previousID()yields nothing usable after a prototype change (verified acrosssetPrototypeOf,__proto__ =, andReflect.setPrototypeOf), so this needs a WebKit-side change. Everything else in the file passes.test-util-isDeepStrictEqual.js— needsBuffer≠Uint8Arrayin strict mode. That's correct (node rejects them, and the matrix pins bun's answer as a known bug), but enabling it turnstest-child-process-advanced-serialization.jsred: bun's advanced IPC is backed bySerializedScriptValue, which downgradesBuffertoUint8Array, so that test currently passes only because two bugs cancel out:Bufferthrough structured clone belongs in its own change.test-util-getcallsites.js— assertsgetCallSites().length > 1at module scope, which holds in node only because node wraps modules in JS functions (8 frames vs bun's 1; bun's loader is native). Not portable.getCallSiteskeeps real coverage intest-util-getcallsites-preparestacktrace.js.getCallSites'ssourceMapoption is validated but not applied.no test proof · iteration 11 · 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