Conversation
print_error_instance_body only followed Error.cause when the cause was
itself a JSC ErrorInstance; a plain object, string, number, or other
value set via new Error(msg, { cause }) was silently dropped from both
console.error and the uncaught-throw report. util.inspect already
handled this correctly, so the context was only lost at the point it
mattered most.
When the non-enumerable cause is not an ErrorInstance, format it inline
using the same style as enumerable own properties. Also mark cause as
seen regardless of its type so an enumerable non-Error cause is not
printed twice.
|
Warning Review limit reached
Next review available in: 1 minute 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 (2)
Comment |
|
Updated 5:10 PM PT - Jul 22nd, 2026
❌ @robobun, your commit 8dc6995 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35172That installs a local version of the PR into your bun-35172 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
The formatter collapsed a blank line, shifting the inline-snapshot line references in the minified-file tests. Split the normalizeError comment over two lines to keep the original offsets.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Both the enumerable-property loop and the non-Error cause fallback now expand the same macro body, so the formatter settings and exception policy can't drift between them.
There was a problem hiding this comment.
LGTM — the two inline notes are nits (a niche depth-≥2 enumerable-Error-cause edge case where pre-PR output was already wrong, and a test-file require placement); neither blocks. The earlier dedup feedback was addressed in fc83a03 with the write_error_field! macro.
Extended reasoning...
Overview
Adds a fallback render path in VirtualMachine::print_error_instance_body so a non-ErrorInstance cause (object, string, number, null, undefined) is printed inline in the property block instead of being silently dropped. The enumerable-property render body was extracted into a local write_error_field! macro (addressing my earlier dedup comment), saw_cause is now set for any enumerable cause to avoid double-printing, and the trailing-newline check moved after the cause fallback so it fires when the cause is the only rendered field. Test coverage in inspect-error.test.js spans Bun.inspect, console.error, and the uncaught-throw path via spawned subprocesses.
Security risks
None. This is diagnostic output formatting; no new user-controlled paths into allocation, syscalls, or exception handling beyond what the existing property loop already exercised. The macro reuses the same clear_exception / early-return policy the pre-PR code had.
Level of scrutiny
Moderate — VirtualMachine.rs is core, but the touched region is the error printer, whose blast radius is stderr formatting rather than runtime semantics. The refactor is largely mechanical (hoist struct + macro-ize existing block + add one else branch). I traced the RestoreFmt guard scoping through the macro expansion and confirmed is_first_property / formatter borrow lifetimes stay correct after the reshuffle.
Other factors
- Fails-on-main / passes-with-fix evidence provided for both ASAN-debug and release builds.
- The two remaining findings are explicitly nits: the
saw_causehoist changes a depth-≥2 enumerable-Error-cause path from "double-printed with full stack" to "single truncated inline render" — the old output was itself a bug, so this is a lateral move on a niche path. Therequire("harness")placement is a style guideline deviation that appears deliberate to preserve line-number-sensitive snapshots above it. - Prior review thread (dedup) is resolved and marked as such.
…import - An enumerable Error-typed cause now always pushes to errors_to_append for a full recursive render, independent of prev_had_errors, so a depth>=2 assigned cause keeps its stack trace instead of falling through to the truncated inline path. - Moved bunEnv/bunExe to a module-scope import and regenerated the line-number-sensitive inline snapshots.
There was a problem hiding this comment.
LGTM — all three prior review notes (macro dedup, depth≥2 enumerable Error cause, top-level harness import) are addressed cleanly.
What was reviewed:
write_error_field!macro: verified theRestoreFmtguard, formatter-state capture/restore, and exception-handling policy match the pre-PR property-loop body it replaces.- Enumerable
causehandling: an Error-typed enumerable cause now unconditionally pushes toerrors_to_append(independent ofprev_had_errors), so depth-1 behavior is unchanged and depth-2 gets a single full render instead of the old truncated+double-print. !is_first_propertynewline reorder: moved after the cause fallback so an inline-rendered non-Error cause still gets its trailing blank line; thecodeblock ordering is preserved.normalizeErrorregex: anchored to\(:?\d+:\d+\)$so it only strips sourceless debug frames — path-bearing frames like([dir]/file.js:95:7)don't match.
Extended reasoning...
Overview
Output-only fix in VirtualMachine::print_error_instance_body (src/jsc/VirtualMachine.rs): when a non-enumerable Error.cause is not itself an ErrorInstance, render it inline in the property block instead of dropping it. The change extracts the pre-existing enumerable-property render body into a local write_error_field! macro (per my first-round feedback), reuses it for the cause fallback, and fixes the depth≥2 enumerable Error-typed cause case (per my second-round feedback) by pushing it to errors_to_append unconditionally rather than only when !prev_had_errors. Test file gains a non-Error cause describe block plus a widened normalizeError helper for debug-only frames.
Security risks
None. This is the error-instance pretty-printer — output formatting only. No new user-input parsing, no allocation sizing on external data, no FFI surface change.
Level of scrutiny
Medium-low. VirtualMachine.rs is a core file, but the touched region is the property-dump section of the error printer, and the change is behavior-additive (previously-dropped values now render). The macro is a mechanical extraction of the pre-existing block; I diffed it line-by-line against the original and the only substantive changes are the parameterized label/label-len/value and the RestoreFmt field-initializer reorder (state is now captured inline in the struct literal rather than via four prev_* locals — semantically identical). The is_first_property newline moving after the cause fallback is intentional and correct: a non-Error cause rendered inline needs the same trailing blank line the enumerable-property block produces.
Other factors
This is my third pass. Both prior rounds of nits were addressed in follow-up commits (fc83a03, 8dc6995) with the exact fixes I suggested, and the author added the depth-2 test I described. The evidence block shows the new tests fail on the unfixed debug build (7 failures) and pass on both debug+ASAN and release with the fix. Subprocess tests drain pipes concurrently, use test.concurrent, and assert content before exit code. The runtime-constructed marker strings are a reasonable workaround for the source-preview lines Bun.inspect prepends. The regenerated inline snapshots are a mechanical consequence of adding the line-2 import.
|
CI on build #78100 is green for this diff. The two remaining red tests are pre-existing GC-timing Node compat failures also present on main:
Neither touches the error printer or console output. Ready for review. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-22, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
The native error printer silently drops
Error.causewhen it is not itself anErrorinstance.util.inspectalready shows[cause]: {...}for these, so the context is only lost on the code paths that matter most (console.errorand the uncaught-throw report).Repro
An assigned enumerable
e.cause = "x"did print (it flows through the own-property loop), which is why this hid.Cause
VirtualMachine::print_error_instance_bodylooks up the non-enumerablecauseviaget_ownbut only recurses whencause.js_type() == JSType::ErrorInstance. Every other value (plain object, string, number,DOMException, aResponse, etc.) falls through with no fallback render.Fix
src/jsc/VirtualMachine.rs:causeis not anErrorInstance, render it inline using the same formatter settings as the enumerable own-property dump, so it appears ascause: <value>,in the property block.saw_causefor any enumerable property namedcause(not justErrorInstance-typed ones) so an assigned non-Error cause is not rendered twice.RestoreFmtdrop guard one scope out so both the property loop and the cause fallback can share it.Output after
Tests
New
non-Error causedescribe block intest/js/bun/util/inspect-error.test.jscovering object cause, string cause at depth 2,null/number/undefinedcause, no double-print for an enumerable cause, plus spawned-process checks forconsole.errorand the uncaught-throw path. All fail on the unfixed build and pass with the fix.Also widened the existing
normalizeErrorhelper to strip debug-onlyat require (N:N)frames as well as the olderat require (:N:N)form so the two pre-existing minified-file snapshot tests stay green underbun bd.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The native error printer only followed an error's
causewhen the value was a JSCErrorInstance, so plain objects, strings, numbers, and other non-Error causes were silently omitted from bothconsole.erroroutput and the uncaught error report. The fix removes that type gate and adds a fallback that renders non-Error cause values inline using the inspector, while still recursing into Error-typed causes so chained errors print fully at any depth. This brings the printer's behavior in line withutil.inspectand Node's[cause]: ...output.