Repository navigation
Conversation
Node.js defines the code property on ERR_INVALID_ARG_TYPE, ERR_OUT_OF_RANGE
and every other ERR_* error as an own, enumerable, writable, configurable
data property on the error instance. Bun was placing it on a per-code
prototype object instead, so Object.keys(err), JSON.stringify(err),
{...err} and err.hasOwnProperty('code') all missed it. Structured error
loggers (pino, Sentry integrations, Object.assign-based serializers) rely
on code being own.
createErrorPrototype no longer writes code to the prototype; every
ErrorInstance::create site in ErrorCode.cpp now putDirect()s code on the
instance with attributes 0 (matching Node). The name and toString
properties that remain on the per-code prototype are now DontEnum so they
no longer leak into for...in.
|
Warning Review limit reached
Next review available in: 17 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 10:31 AM PT - Jul 26th, 2026
❌ @robobun, your commit 1a59c33 has 2 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35926That installs a local version of the PR into your bun-35926 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit: the third site (INVALID_ARG_VALUE_RangeError, line 1015) uses the ErrorInstance::create(vm, structure, WTF::String, ...) overload — message is already a WTF::String, so no user JS runs and no exception guard is needed there. Also confirmed createErrorStructure no longer reads data.code, so dropping the parameter is dead-code cleanup only.
Extended reasoning...
One nit was filed inline (missing exception guard in the non-Zig-global fallback). I checked whether the same guard is needed at the other two putDirect sites: ErrorCodeCache::createError already has the scope+guard immediately above the new line, and INVALID_ARG_VALUE_RangeError calls the (vm, structure, WTF::String, ...) overload which cannot enter user JS, so neither needs a change. Deferring to human review given this is a broad Node-compat behavior change (affects Object.keys/JSON.stringify/util.inspect output for every ERR_* error) and CI hasn't reported yet.
There was a problem hiding this comment.
Thanks for applying the exception guard in a0790f8 — that resolves my earlier note. I didn't find further issues, but since this changes the observable shape of every ERR_* error (own code, DontEnum prototype name/toString, and by extension util.inspect output), it's worth a maintainer's eyes before merge.
What was reviewed:
- All three
createErrorStructurecall sites nowputDirectcodeon the instance; no other callers exist. NodeError_proto_toStringreadscodeviathisVal.get(...), so it still resolves from the own property.INVALID_ARG_VALUE_RangeErroruses theWTF::StringErrorInstance::createoverload (non-throwing), so no extra guard is needed there.- Grepped test snapshots for existing
[ERR_*]/inspect assertions that might flip — none found.
Extended reasoning...
Overview
The PR moves the .code property on Node-style ERR_* errors from an enumerable data property on the per-code prototype to an own writable/enumerable/configurable data property on each error instance, and marks the remaining prototype name and toString as DontEnum. This matches Node's makeNodeErrorWithCode, so Object.keys, spread, JSON.stringify, hasOwnProperty and for...in now behave as they do in Node. Production changes are ~20 lines across three creation sites in src/jsc/bindings/ErrorCode.cpp, plus a new 74-line test file.
Security risks
None. This is property-placement/enumerability metadata on error objects; no user input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Medium-high. ErrorCode.cpp is the single funnel for all C++/Rust/JS ERR_* throws (fs, buffer, url, net, events, crypto, child_process, …). The change is small and dictated by Node's observable behavior — there is no real design freedom here — but it alters the enumerable own-property set and util.inspect output of every such error, which is a broad user-visible surface. That breadth is why I'm deferring rather than auto-approving.
Other factors
- My earlier inline nit (missing exception guard on the non-Zig-global fallback before dereferencing
created_error) was addressed in a0790f8; the guard now mirrorsErrorCodeCache::createErrorexactly. - I confirmed
createErrorStructure/createErrorPrototypehave no other callers, so dropping thecodeparameter is complete. NodeError_proto_toStringstill works because it uses.get()(prototype-chain lookup), and the test assertsString(err)still contains[ERR_...].- The
INVALID_ARG_VALUE_RangeErrorpath uses the(VM&, Structure*, WTF::String, ...)ErrorInstance::createoverload, which does not run usertoString, so no guard is needed before its newputDirect. - Structure caching is preserved: instances of a given
ErrorCodestill start from the same cached structure and take the samecodetransition, so they continue to share a final structure. - Test coverage is solid (matrix across fs/buffer/URL/events, descriptor shape, keys/spread/JSON/for-in, prototype absence, toString), with fail-on-main / pass-on-PR evidence. The author also ran adjacent suites (buffer, events, capture-stack-trace), and I found no
.snapfiles asserting on the old inspect shape. CI (#82419) will exercise the full suite.
SystemError__toErrorInstance (errno family: ENOENT etc.) used DontDelete
on code/errno/syscall/path/dest/fd/hostname, giving configurable:false
where Node has configurable:true.
SystemError__toErrorInstanceWithInfoObject (ERR_SYSTEM_ERROR from node:os)
used DontEnum on code, so Object.keys/spread/JSON.stringify dropped it.
Both now use attributes 0 to match Node's {writable:true, enumerable:true,
configurable:true}.
There was a problem hiding this comment.
Beyond the inline finding, I confirmed the earlier exception-guard concern on the non-Zig-global fallback is addressed in a0790f8, and that NodeError_proto_toString still resolves code via thisVal.get() (prototype-chain lookup) so moving it to the instance does not break toString() — the new test covers this. The duplicate info->putDirect(code) line removed from SystemError__toErrorInstanceWithInfoObject was a pre-existing redundant write, not a behavior change.
Extended reasoning...
Previous review concern (missing exception scope in the non-Zig-global fallback) was applied in a0790f8 and is visible in the current diff. This run's remaining finding is the Windows-specific os.setPriority(0x7ffffffe, 0) trigger in the new test, which is covered by the inline comment. I also traced NodeError_proto_toString — it reads code via thisVal.get() so relocating the property from prototype to instance keeps [ERR_*] in the string, and checked that the removed duplicate info code write in SystemError__toErrorInstanceWithInfoObject was redundant. Deferring rather than approving because this changes observable error shape (Object.keys, for...in, util.inspect) for every ERR_* error Bun throws, which is worth a human glance at CI results across the wider suite.
… too set_process_priority_impl short-circuits pid < 0 to ESRCH on every platform before any syscall runs. On Windows, a bogus positive pid maps to EINVAL which set_priority1 swallows, so the call returned undefined.
There was a problem hiding this comment.
LGTM — both earlier findings addressed; the remaining diff is a focused property-attribute change to match Node's makeNodeErrorWithCode.
What was reviewed:
- Verified all three
createErrorStructurecall sites nowputDirectcodeon the instance, andNodeError_proto_toStringreads via.get()so it still finds the moved property. - Confirmed the a0790f8 exception guard mirrors the sibling in
ErrorCodeCache::createError, and theINVALID_ARG_VALUE_RangeErrorsite uses the non-throwing WTFStringcreateoverload so no guard is needed there. - Checked that removing the duplicate
info->putDirect(code)inSystemError__toErrorInstanceWithInfoObjectonly reordersinfokeys;os.test.js'stoEqualonerr.infois order-insensitive. os.setPriority(-1, 0)hits thepid < 0 → ESRCHshort-circuit on all platforms, resolving the Windows CI failure.
Extended reasoning...
Overview
Moves the Node-style .code property on ERR_* errors from the per-code prototype onto each error instance (writable/enumerable/configurable), marks the remaining prototype name/toString as DontEnum, and drops DontDelete/DontEnum from the SystemError property descriptors so they match Node's uvException/SystemError. Touches src/jsc/bindings/ErrorCode.cpp (three creation sites + prototype builder), src/jsc/bindings/bindings.cpp (two SystemError__toErrorInstance* functions), and adds test/js/node/errors/error-code-own-property.test.ts.
Security risks
None. This adjusts property attribute flags on error objects Bun itself constructs; no user input flows into new code paths, and no new JS re-entry points were added beyond what ErrorInstance::create already had (now guarded on both branches).
Level of scrutiny
Medium. The change funnels through the central error-creation path so it affects every ERR_* throw site, but the transformation is mechanical (move one putDirect from prototype to instance; flip attribute bits). I traced each createErrorStructure caller, confirmed NodeError_proto_toString still resolves code via prototype-chain get(), and checked that the info dedup in SystemError__toErrorInstanceWithInfoObject only changes key order (not observed by any toEqual assertion).
Other factors
Two prior review rounds were addressed: (1) the non-Zig-global fallback now guards ErrorInstance::create with a TopExceptionScope before putDirect, matching the sibling path; (2) the ERR_SYSTEM_ERROR test now uses setPriority(-1, 0), which set_process_priority_impl short-circuits to ESRCH on every platform (confirmed against the Windows CI failure the author cited). PR verification shows the new test fails 9/10 on system Bun and passes 10/10 on the debug+ASAN and release builds, plus green runs of buffer.test.js, os.test.js, and capture-stack-trace.test.js.
|
Build 82444 finished: all test lanes green (194/196 jobs pass, including Windows x64 and aarch64 for the new test). The two red jobs are infra:
Ready for review. |
|
Closing in favor of #41737, which rebuilds this change on current main. It also covers the |
Repro
Node v26.3.0:
Bun before:
for (k in err)on the same errors yielded["name","code","toString"](leaked from the prototype) in Bun and["code"]in Node.Cause
createErrorPrototypeinsrc/jsc/bindings/ErrorCode.cppwrotecode(along withnameandtoString) as enumerable data properties on a per-ErrorCodeprototype object. The error instance itself never received an owncode, soObject.keys,JSON.stringify, spread andhasOwnPropertyall missed it, andfor...inpicked up the three enumerable prototype properties instead. In Node.js,makeNodeErrorWithCodeassignserror.code = keydirectly on the instance.Fix
codefrom the per-code prototype andputDirectit on the instance at the threeErrorInstance::createsites (ErrorCodeCache::createError, the non-Zig-global fallback, andINVALID_ARG_VALUE_RangeError) with attributes0(writable, enumerable, configurable).nameandtoStringasDontEnumso they no longer appear infor...in.SystemError__toErrorInstance(errno errors:ENOENT,EACCES, ...), dropDontDeletefromcode/errno/syscall/path/dest/fd/hostnameso they areconfigurable:truelike Node'suvException.SystemError__toErrorInstanceWithInfoObject(ERR_SYSTEM_ERRORfromnode:os), dropDontEnumfromcodesoObject.keys/spread/JSON.stringifysee it, and dropDontDeletefrom the remaining properties.All ERR_* creation paths (C++
Bun::throwError/Bun::ERR::*, RustErrorCode::throw, and JS-side$ERR_*macros viajsFunctionMakeErrorWithCode) funnel through these sites, so one change covers fs, buffer, url, net, events, child_process, etc.As a side effect,
util.inspect(err)now shows the trailing{ code: 'ERR_...' }block like Node, sincecodeis now own and enumerable.Verification
New
test/js/node/errors/error-code-own-property.test.tsasserts, for errors fromnode:fs,node:buffer,URL,node:eventsandnode:os, thatcodehas the Node descriptor{value, writable:true, enumerable:true, configurable:true}, shows up inObject.keys/spread/JSON.stringify, thatfor...indoes not yieldname/toString, thattoString()still includes the bracketed code, and that errno-family error properties areconfigurable:true.[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file