Fix segfault when printing an AggregateError without an errors property - #39633
Conversation
The error printer iterates the own `errors` property of an AggregateError. `getDirect` returns the empty value when the property is missing, and forEachInIterable read that value as a cell at address 0 (segfault at address 0x5). JSC's JSModuleLoader::duplicateError hands out such an instance for every later load of a module with two or more build errors, so `bun test` crashed when a second file imported a module that failed to build (#36963). The same crash was reachable from console.error, Bun.inspect, and an uncaught error after `delete err.errors`. JSC__JSValue__getErrorsProperty now returns undefined unless `errors` holds an object, and get_errors_property returns an Option. The printer falls through to the normal error output when it printed no member: `errors` is missing, an accessor, a primitive, empty, or not iterable. An iteration that throws no longer leaves its exception pending on the VM, which made console.error throw for a non-iterable `errors`.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 32 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (7)
Comment |
|
Reproduced on bun 1.4.0 canary (4c68990), both forms: and the #36963 form: With this branch both print |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted crash fix with thorough shape coverage. The one inline nit (missing negative assertion on the members-only test) is worth adding but not blocking.
Extended reasoning...
Overview
The PR fixes a segfault in the native error printer (print_errorlike_object in src/jsc/VirtualMachine.rs) when an AggregateError has no walkable errors property. Root cause: JSC__JSValue__getErrorsProperty returned getDirect() verbatim — the empty value for a missing slot, or a GetterSetter cell for an accessor — and that was fed straight to forEachInIterable, which dereferenced it as a cell. The fix guards the C++ binding to return undefined for anything that isn't an object, wraps the Rust side in an Option, and makes the printer fall through to the normal error path when no member was printed. Exceptions from the walk are now cleared (except termination). Five files touched: the binding, its Rust wrapper, the printer, the swallow-lint inventory, and 15 new tests in inspect-error.test.js.
Security risks
None. This is the diagnostic/error-printing path; the change adds guards (null/object checks, exception clearing) rather than removing any. getDirect is still nothrow and does not invoke user getters — the accessor test confirms the getter is not called. No new user-controlled data reaches an allocation or syscall.
Level of scrutiny
Moderate. It's native code on a hot user-visible path (uncaught errors, console.error, Bun.inspect, bun test failure output), but the change is small and mechanical: a defensive isObject() gate in C++, an Option on the Rust side, and a printed_member flag to decide whether to fall through. I verified clear_exception_except_termination returns true when cleared / false on termination, so .is_err() && !clear...() correctly returns early only for termination. The reborrows (&mut *formatter, as_deref_mut(), moving mut exception_list to the parameter) are the borrow-checker consequence of the branch no longer always returning — mechanically correct. get_errors_property has exactly one caller, so the signature change is fully covered.
Other factors
Test coverage is strong: five errors shapes (deleted, [], {}, primitive, accessor) × two entry points (console.error, uncaught), plus Bun.inspect, plus the two real-world module-loader repros from #36963. robobun confirmed 14/15 fail on 1.4.0 and all pass on this branch. The swallow-lint inventory decrement matches the removed let _ =. The one nit — the "members are still printed" test doesn't assert the header is absent — means the if ctx.printed_member { return; } clause isn't pinned by a test, but that's a test-strength gap, not a correctness issue in the fix itself.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Review follow-ups, all pushed:
No code change beyond the comments. |
There was a problem hiding this comment.
LGTM — the earlier nit (asserting the AggregateError header is absent when members print) is addressed in 73aaf90, and the comment-cop items were resolved in the two follow-up commits.
What was reviewed:
getDirectempty/GetterSetter handling inbindings.cpp—!errorsguards theisObject()call, and GetterSetter's cell type is belowObjectTypeso accessors are correctly rejected without invoking the getter.clear_exception_except_termination()polarity — returnsfalseon termination, so!...correctly early-returns only for that case.- The reborrow of
formatter/writer/exception_list(&mut */.as_deref_mut()) so the fall-through toprint_error_from_maybe_private_datacan reuse them afterfor_each— the raw pointers inctxare dead oncefor_eachreturns. get_errors_propertyhas exactly one caller, so theJSValue → Option<JSValue>signature change is fully covered.
Extended reasoning...
Overview
Fixes a segfault in the native error printer when an AggregateError is printed without a walkable own errors property. Touches five files: the C++ binding JSC__JSValue__getErrorsProperty in bindings.cpp (now returns undefined unless errors holds an object), the Rust wrapper get_errors_property in JSValue.rs (now Option<JSValue>), the AggregateError branch of print_errorlike_object in VirtualMachine.rs (now falls through to normal error printing when no member was printed, and clears any non-termination exception the iteration threw), the source-lint inventory (one fewer discarded-JsResult site), and 15 new subprocess-spawned tests in inspect-error.test.js.
Security risks
None. The change is purely defensive — it adds null/type guards on a getDirect result and a fallback print path. No new user input is parsed, no new allocations sized from external data, no auth/crypto/permissions surface.
Level of scrutiny
Medium. The error printer runs for every uncaught error, unhandled rejection, console.error and Bun.inspect, so a regression here would be visible, but the change only adds guards and a fallback where the prior code segfaulted or printed nothing. The normal members-only output path is preserved (printed_member gate) and now pinned by expect(stderr).not.toContain(header). The evidence block confirms 14/15 new tests fail on the unfixed binary and all pass on both ASAN-debug and release builds. I verified the sole caller of get_errors_property, the return-value contract of clear_exception_except_termination (true = cleared, false = termination), and that GetterSetter cells fail isObject() so the accessor case is handled without invoking user code.
Other factors
My earlier review's only finding (missing negative assertion on the members-only test) was addressed in 73aaf90 and the thread is resolved. The comment-cop bot's four long-comment flags were addressed by 10624bb and a02dafa; the remaining comments in the diff are one-liners. Four overlapping open PRs are noted in the description; this one is deliberately the narrow crash-only fix that leaves output unchanged for the normal case, so it composes with rather than preempts them. No CODEOWNERS match for the touched paths.
… unconditionally get_errors_property has one caller, behind is_aggregate_error, so the value is always an object. The binding uses asObject, and the Rust wrapper asserts the precondition like fast_get does. The failed walk of `errors` clears the exception the same way print_error_from_maybe_private_data does, with no special case for termination.
|
Both review comments are addressed in 6e6bd4a:
|
There was a problem hiding this comment.
LGTM — a focused segfault fix in the error printer, and both of dylan-conway's review points (drop the !obj null check, use plain clear_exception() instead of termination-aware clearing) are addressed in 6e6bd4a.
What was reviewed:
getErrorsPropertynow guards the empty-value / GetterSetter cases fromgetDirect;asObject()is safe because the sole caller is behindis_aggregate_error, which requires anErrorInstance.- The
printed_memberfall-through and the&mut *formatter/exception_list.as_deref_mut()reborrows soprint_error_from_maybe_private_datastill sees them on the new fall-through path. - Test coverage: 5
errorsshapes × console.error/uncaught, Bun.inspect, getter-not-called, the members-only negative assertion (my earlier note), and both #36963 repros.
Extended reasoning...
Overview
Fixes a segfault in print_errorlike_object when an AggregateError has no walkable own errors property. Touches src/jsc/bindings/bindings.cpp (the getErrorsProperty binding now returns undefined unless the slot holds an object), src/jsc/JSValue.rs (wrapper returns Option<JSValue> with a debug_assert!(is_object()) on self), src/jsc/VirtualMachine.rs (the AggregateError branch tracks whether any member was printed and falls through to print the error itself otherwise; a walk exception is cleared), a source-lint inventory decrement, and 15 new spawned-subprocess tests in inspect-error.test.js.
Security risks
None. This is the error-printing path; the only user-influenced input is the shape of an error object, and the fix narrows what the binding will hand to for_each (object-only) rather than widening anything. getDirect remains nothrow and does not invoke getters, which the accessor test confirms.
Level of scrutiny
Medium. It touches native JSC bindings and a hot user-visible printer, but the change is small and defensive: an empty/non-object getDirect result becomes None, and the printer degrades to the ordinary error output instead of crashing or printing nothing. I confirmed get_errors_property has exactly one caller (VirtualMachine.rs), and that caller is gated by is_aggregate_error, whose C++ implementation requires value.isObject() and a dynamicDowncast<ErrorInstance> — so asObject() in the binding and the new debug_assert! are sound. The reborrow changes (std::ptr::from_mut(&mut *formatter), exception_list.as_deref_mut(), and moving mut onto the parameter) exist so the fall-through can still use those borrows after the for_each block; the removed let mut exception_list = exception_list; line is subsumed by the parameter-level mut.
Other factors
All prior review feedback is resolved: my note about the missing not.toContain(header) negative assertion (73aaf90), the comment-cop long-comment flags (10624bb, a02dafa), and dylan-conway's two points (6e6bd4a). The tests spawn one process per shape (necessary because the unfixed binary dies), drain stdout/stderr/exited concurrently, use tempDir/bunEnv/bunExe, and run under describe.concurrent. The evidence block shows 14/15 new tests fail on the unfixed ASAN build and all pass with the fix. The PR notes overlap with #36602/#35816/#39204/#39292 but keeps this change to making the printer safe without altering the normal members-only output, which the regression-guard test now pins.
Removes JSC__JSValue__getErrorsProperty and get_errors_property. The printer reads `errors` through fast_get with a new BuiltinName entry, so a missing property is None and a throwing getter is cleared like any other error the printer hits. fast_get is an ordinary [[Get]], so an accessor is now invoked and its members are printed; the tests cover that, a throwing getter, and a null value.
|
609fd91 replaces the binding with
Checked with the debug build: |
…ins (#35288) ### Problem - `console.log` of a nested `Map`, `Set`, `Array`, `MessageEvent`, Error `cause` chain, or `AggregateError` prints every level. A 1000-deep `Map` prints 2 MB. Deeper chains throw `RangeError: Maximum call stack size exceeded` out of `console.log`. A nested `AggregateError` chain overflows the native stack. - Cause: only `print_object` (`src/jsc/ConsoleObject.rs`) compares `depth` to `max_depth`. The other container printers never compare it. The `cause` loop and `agg_iter` (`src/jsc/VirtualMachine.rs`) do not track depth. ### Fix - Each container printer returns `[Array ...]`, `[Map ...]`, `[Set ...]`, `[MapIterator ...]`, or `[MessageEvent ...]` past the cap, like `[Object ...]`, through one `print_depth_exceeded_marker`. An empty container still prints `[]`, `Map {}`, or `Set {}`. - The `cause` loop and `agg_iter` bump `depth` and print `[Error ...]` past the cap. `agg_iter` also checks the native stack guard (`stack_check`). The error property dump narrows `max_depth`, so it keeps the caller's cap in `outer_max_depth` for these walks. - `Bun.inspect.table` passes its `depth` option as the cell start depth, against a fixed `max_depth` of 5. `TablePrinter::set_start_depth` now clamps it, so a `depth` above 5 prints cells like the default. - Verified: `inspect.test.js` (13 new cases, 12 fail on the released bun), `bun-inspect-table.test.ts` (2 new), and the related console suites. ### Background - `Formatter` (`ConsoleObject.rs`) is the native walker behind `console.log` and `Bun.inspect`. `max_depth` comes from `--console-depth`, `Bun.inspect(x, {depth})`, or the default of 2. - `print_error_instance_body` (`VirtualMachine.rs`) prints an Error, not `Formatter`. It dumps the error's properties one level deep and walks `cause` and `AggregateError.errors` itself. - `{depth: Infinity}` sets `max_depth` to `u16::MAX`, and `depth` saturates there. A depth comparison alone cannot stop that walk. <details><summary>Notes</summary> Sizes before and after, on this branch: | input | before | after | node | |-|-|-|-| | 100-deep `cause` chain | ~20 KB | 711 B | 575 B | | 1000-deep `Map` | ~2 MB | 167 B | 60 B | | 1000-deep `Set` | ~2 MB | 152 B | 40 B | | 100-deep `Array` | ~20 KB | 129 B | 20 B | | 1000-deep `MessageEvent` | ~3 MB | 261 B | n/a | | 3000 causes, 12000 Maps, 20000 AggregateErrors | `RangeError` or SIGSEGV | truncates | truncates | Tables. A container nested inside a cell now prints as a marker, like a nested plain object already did: `{ x: [Object ...] }`, `[ [Array ...] ]`, `Map(1) { 1: [Map ...] }`. Node's `console.table` prints the same shape (`[ [Array] ]`, `Map(1) { 1 => [Map] }`). For a `depth` option above 5, the released bun starts every cell past the cap: a plain object cell prints ` [Object ...]` and the new gates would have done the same to Array, Map, and Set cells. The clamp makes such a depth print cells like the default, object cells included. `{ depth: 0 }` keeps its meaning (`console-table.test.ts` uses it to mirror `console.table`). The option is not redefined as a `max_depth`: #34241 did that and was closed without a merge. AggregateError at the cap. When the members are one level past the cap, the AggregateError prints like any other error first (name, message, stack), then one `[Error ...]` per member. Without that, `Bun.inspect(agg, { depth: 0 })` printed only the markers. Node prints the same shape: the header, then `[errors]: [Array]`. The normal output is unchanged: members only, as #39633 left it. Errors inside Error properties. With `err.details = { inner }`, `err.list = [inner]`, or an `AggregateError` inside a property, the cause and the members of the nested error follow the caller's depth: `depth: Infinity` prints them all, `depth: 2` prints `[Error ...]`. Plain object properties keep the one-level cap. Cause depth per entry point, measured with a 50-deep chain. `console.log`: Node prints root + 2 causes, this branch prints root + 2 causes. Uncaught `throw`: Node prints root + 5 causes, this branch prints root + 8 (the error handler's `Formatter::new` depth), released bun prints all 50. #39637 changes the error handler's formatter to the console depth. Combined with this PR as written, an uncaught error would print root + 2 causes, and #39637's three-cause test would fail. The cause loop reads the depth cap on purpose so `{depth}` and `--console-depth` control it. Whichever PR lands second has to pick: the console depth for causes, or #39637 applying its depth only on the non-Error branch. `test/js/web/console/console-log.expected.txt` changes for `[[[[Array(1000).fill(4)]]]]`, which now stops at the fourth level like Node. The removed text (a long array wrapped at indent 10, then `... 900 more items`) is asserted again by `long arrays get cutoff at a nested indent` in `console-log.test.ts`, through `Bun.inspect(x, { depth: 5 })`. `stack_check` is the formatter's native stack guard. Container printers reach it in `print_as_prelude`. The error walks do not, so `agg_iter` calls it directly. Suites run on the merged branch: `inspect.test.js`, `inspect-error.test.js`, `reportError.test.ts`, `console-log.test.ts`, `console-depth.test.ts`, `bun-inspect.test.ts`, `bun-inspect-table.test.ts`, `console-table.test.ts`, `build-error.test.ts` (244 pass, 1 skip that is also skipped on main). JSX and Proxy have the same unbounded recursion. #29709 is open with that fix, so it is left out here. Review history: the `AggregateError` gate, its `stack_check`, the `print_event` gate, the relative `max_depth`, the state restore before `?` in the cause loop, the empty-container order, `outer_max_depth`, the table clamp, and the AggregateError header at the cap each came from review and each has a test. The branch merges main as of 2026-09-16. The only textual conflict was in `inspect.test.js`, where both sides appended a describe block. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Bun 1.3.14's test runner null-derefs (SIGSEGV at 0x5, surfaced as SIGILL via its own panic trap) when a second test file imports a cached module carrying two or more build errors — so a syntax error in a shared lib crashes `bun test` instead of failing it. Hit locally on 2026-09-18 while editing plugins/soleur/lib/harness-parity.ts; the panic banner was swallowed by a `| grep` in the agent's pipeline. Upstream: oven-sh/bun#36963 (same version, same shape) and #40780 (root cause: print_errorlike_object reads AggregateError.errors via getDirect(), forEachInIterable then walks the empty JSValue). Fixed by oven-sh/bun#39633, shipped in 1.4.1. Verified the reproduction fails cleanly on 1.4.2 (`AggregateError: 3 errors building lib.ts`, exit 1). scripts/test-all.sh reads this file at runtime, so no other change is needed; CI picks it up via `bun-version-file`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bun 1.3.14's test runner null-derefs (SIGSEGV at 0x5, surfaced as SIGILL via its own panic trap) when a second test file imports a cached module carrying two or more build errors — so a syntax error in a shared lib crashes `bun test` instead of failing it. Hit locally on 2026-09-18 while editing plugins/soleur/lib/harness-parity.ts; the panic banner was swallowed by a `| grep` in the agent's pipeline. Upstream: oven-sh/bun#36963 (same version, same shape) and #40780 (root cause: print_errorlike_object reads AggregateError.errors via getDirect(), forEachInIterable then walks the empty JSValue). Fixed by oven-sh/bun#39633, shipped in 1.4.1. Verified the reproduction fails cleanly on 1.4.2 (`AggregateError: 3 errors building lib.ts`, exit 1). scripts/test-all.sh reads this file at runtime, so no other change is needed; CI picks it up via `bun-version-file`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bun 1.3.14's test runner null-derefs (SIGSEGV at 0x5, surfaced as SIGILL via its own panic trap) when a second test file imports a cached module carrying two or more build errors — so a syntax error in a shared lib crashes `bun test` instead of failing it. Hit locally on 2026-09-18 while editing plugins/soleur/lib/harness-parity.ts; the panic banner was swallowed by a `| grep` in the agent's pipeline. Upstream: oven-sh/bun#36963 (same version, same shape) and #40780 (root cause: print_errorlike_object reads AggregateError.errors via getDirect(), forEachInIterable then walks the empty JSValue). Fixed by oven-sh/bun#39633, shipped in 1.4.1. Verified the reproduction fails cleanly on 1.4.2 (`AggregateError: 3 errors building lib.ts`, exit 1). scripts/test-all.sh reads this file at runtime, so no other change is needed; CI picks it up via `bun-version-file`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bun 1.3.14's test runner null-derefs (SIGSEGV at 0x5, surfaced as SIGILL via its own panic trap) when a second test file imports a cached module carrying two or more build errors — so a syntax error in a shared lib crashes `bun test` instead of failing it. Hit locally on 2026-09-18 while editing plugins/soleur/lib/harness-parity.ts; the panic banner was swallowed by a `| grep` in the agent's pipeline. Upstream: oven-sh/bun#36963 (same version, same shape) and #40780 (root cause: print_errorlike_object reads AggregateError.errors via getDirect(), forEachInIterable then walks the empty JSValue). Fixed by oven-sh/bun#39633, shipped in 1.4.1. Verified the reproduction fails cleanly on 1.4.2 (`AggregateError: 3 errors building lib.ts`, exit 1). scripts/test-all.sh reads this file at runtime, so no other change is needed; CI picks it up via `bun-version-file`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bun 1.3.14's test runner null-derefs (SIGSEGV at 0x5, surfaced as SIGILL via its own panic trap) when a second test file imports a cached module carrying two or more build errors — so a syntax error in a shared lib crashes `bun test` instead of failing it. Hit locally on 2026-09-18 while editing plugins/soleur/lib/harness-parity.ts; the panic banner was swallowed by a `| grep` in the agent's pipeline. Upstream: oven-sh/bun#36963 (same version, same shape) and #40780 (root cause: print_errorlike_object reads AggregateError.errors via getDirect(), forEachInIterable then walks the empty JSValue). Fixed by oven-sh/bun#39633, shipped in 1.4.1. Verified the reproduction fails cleanly on 1.4.2 (`AggregateError: 3 errors building lib.ts`, exit 1). scripts/test-all.sh reads this file at runtime, so no other change is needed; CI picks it up via `bun-version-file`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(ci): bump Bun pin 1.3.14 → 1.4.2 Bun 1.3.14's test runner null-derefs (SIGSEGV at 0x5, surfaced as SIGILL via its own panic trap) when a second test file imports a cached module carrying two or more build errors — so a syntax error in a shared lib crashes `bun test` instead of failing it. Hit locally on 2026-09-18 while editing plugins/soleur/lib/harness-parity.ts; the panic banner was swallowed by a `| grep` in the agent's pipeline. Upstream: oven-sh/bun#36963 (same version, same shape) and #40780 (root cause: print_errorlike_object reads AggregateError.errors via getDirect(), forEachInIterable then walks the empty JSValue). Fixed by oven-sh/bun#39633, shipped in 1.4.1. Verified the reproduction fails cleanly on 1.4.2 (`AggregateError: 3 errors building lib.ts`, exit 1). scripts/test-all.sh reads this file at runtime, so no other change is needed; CI picks it up via `bun-version-file`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review: validate-vector-config reads .bun-version instead of `latest` (P3) The one workflow whose setup-bun step floated `bun-version: latest` while every other workflow reads `bun-version-file: ".bun-version"`. Pre-existing; surfaced by the git-history seat while confirming every Bun consumer follows the pin this PR bumps. Pinning it preserves the intent the pin was introduced for (2026-03: no surprise breakage from a floating version). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review: every Bun consumer derives from .bun-version (P3, structural roll-up) Four seats surfaced five declaration sites of the Bun version that did not follow the pin this PR bumps — one gap, not five findings: - web-platform-release.yml: setup-bun with no `with:` (floated latest) - skill-security-scan-{corpus,pr-trailer}.yml: setup-bun pinned to the 2025-04 `v2` SHA while ci.yml:870 asks for lockstep on v2.1.2 - plugin-root-propagation-verify-in-image.sh: `npm i -g bun@1.3.11`, two minors behind the pin it never read - sandbox-canary-verify-in-image.sh: `curl bun.sh/install | bash` with no version, so the canary ran on whatever bun.sh served that day The two in-image scripts now read the pin host-side (they run from the repo root; /src is apps/web-platform) and pass it in with `-e BUN_VERSION` — EXPORTED, because `docker run -e NAME` reads the client environment, not shell variables (shellcheck SC2034 caught the unexported first draft; proven with a positive and a negative control). validate-vector-config.yml gains `.bun-version` in both `paths:` lists so a pin bump exercises the one workflow the three green CI runs never reached. preflight-check10: the "VERIFIED ALSO ON 1.3.14" comment this PR made false now records the 1.4.2 measurement — same suite on both binaries under FORCE_COLOR=1, `cat -v` byte-identical apart from the elapsed-time suffix, so the load-bearing terminator is unchanged. Verified: shellcheck clean; lint-workflow-install-sites OK (41 sites) and its 29-assertion test; preflight-check10 29/29; scripts-shard-runtime- coverage ALL PASSED. actionlint's SC2221/SC2222 on pr-trailer.yml:155 are pre-existing on main (4 there, 4 here) in a run: block this does not touch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review: 8 finding(s) triaged Records that soleur:review ran on this branch (see issue 6724). Empty by design: a review that finds nothing still needs to prove it ran. This is a boolean, not an attestation that the merged tree is the reviewed tree — see ADR-127. Reviewed-Coverage records HOW MUCH review ran (a separate axis from ADR-127's tree-binding decision); 'unknown' means the caller did not measure, never that coverage was full. Reviewed-By-Soleur: soleur:review Reviewed-Commit: b52a02e Reviewed-Coverage: full 4/4 agents * review: xtrace credential refusal in the two in-image verifiers (#7797) `lint-bot-statuses` reddened on the previous commit: both scripts bind a live ANTHROPIC_API_KEY (`docker run -e ANTHROPIC_API_KEY`) and carried no xtrace refusal, so an `sh -x` of either would print the credential into whatever captures stderr. Pre-existing — the lint is `--changed --base origin/main` scoped, so the gap only became visible when the previous commit brought these two files into the diff. Fixed rather than deferred: the guard is right, and the block it prescribes is what the compliant siblings already carry (seed-live-verify-user.sh, seed-dev-users.sh). Verified in both directions, not just the refusal arm: ANTHROPIC_API_KEY=… bash -x <script> → prints the refusal, exits 78 env -u ANTHROPIC_API_KEY bash -x … → no refusal (0 matches) lint now OK (3 scanned, 0 violations); shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Problem
AggregateErrorwith no ownerrorsproperty kills the process:panic(main thread): Segmentation fault at address 0x5inJSC::forEachInIterable(Sentry BUN-40MQ, 92 events on the 1.4.0 canary, 62 inbun test, and BUN-47FF, the same stack as grouped on Windows, 9 events inbun test, 1 of them on the public 1.4.0).JSC__JSValue__getErrorsProperty(src/jsc/bindings/bindings.cpp:4860) returnsgetDirect()as is, which is the empty value for a missing property.print_errorlike_object(src/jsc/VirtualMachine.rs:5221) passes it tofor_each, which reads it as a cell at address 0.bun testcrashed when a second file imported a module with two or more build errors (bun testrunner crash #36963).Fix
get_errors_propertyare deleted. The printer readserrorswithfast_getand a newBuiltinName::errors.errorsmissing, empty, or not iterable) it falls through and prints theAggregateErrorlike any other error. The normal output, members only, is unchanged.console.errorthrew for a non-iterableerrors.test/js/bun/util/inspect-error.test.js, 17 new tests, 16 fail on bun 1.4.0. Also the build error, inspect, plugin, ipc, cluster and source lint tests.Background
print_errorlike_objectis the native error printer behind uncaught errors, unhandled rejections,console.errorandBun.inspect. For anAggregateErrorit prints the members and returns.getDirectreads an own property slot with no getters. A missing slot gives the emptyJSValue, which passes JSC'sisCell(). An accessor slot gives aGetterSettercell.fast_getis an ordinary[[Get]]keyed by a preallocated identifier (BuiltinNamesMapinbindings.cpp, same order asBuiltinNameinsrc/jsc/lib.rs). Missing givesNone, a throwing getter givesErr.JSModuleLoader::duplicateError(JSModuleLoader.cpp:116) settles every later load of a module that failed to load with a copy of the stored error, rebuilt from its type and message only.Notes
Repro on bun 1.4.0, user code:
Repro without user code (#36963):
lib.tswith a syntax error that produces two or more messages, two test files that import it,bun test. The second file crashed. It now reportsAggregateError: 4 errors building ".../lib.ts". A single build error produces aBuildMessage, not anAggregateError, and is not affected. Loading the broken module twice withimport()in one file and printing the second rejection crashes the same way. That is one of the new tests, next to thebun testcase.Shapes covered by the tests, each through
console.errorand as an uncaught error:errorsdeleted,[],{},null, a number, a getter that throws. Before the fix, the deleted shape crashed (exit 139),[]printed nothing (same forPromise.any([])),{},nulland the number madeconsole.errorthrow aTypeError, and an accessor (itsGetterSettercell was passed to the walk) threw aTypeErrorin release builds and failedASSERT(isSymbol())inJSValue::synthesizePrototypein debug builds. All of these now printAggregateError: <message>with the source preview and stack. A separate test defineserrorsas an accessor that returns an array: since the read is an ordinary[[Get]]now, the getter runs and its members are printed, with no header.fast_getandfor_eachreturnErrwith the exception still pending. Both sites callclear_exception(), the same callprint_error_from_maybe_private_datamakes when printing an error throws. The removedlet _ = errors.for_each(...)lowers the count forVirtualMachine.rsintest/internal/source-lints/jsresult-swallow.inventory.jsonby one.BuiltinName::errorsis inserted before the two private names in both enums, sointernalandsharedFdmove by one on both sides. The tests that go through them still pass:test/js/bun/udp/dgram.test.ts,test/js/bun/spawn/spawn.ipc.test.ts,bun-ipc-inherit.test.ts, and the nodetest-cluster-basic,test-cluster-dgram-1,-2and-reusetests.Review history: the first revision kept the binding and returned
undefinedunless the slot held an object, the second dropped its null check and a termination special case, and the third (current) replaced it withfast_getas asked in review.Overlapping open PRs, each broader than this one: #36602 reworks the whole AggregateError output and guards this walk. #35816 guards the console formatters and adds an
is_objectcheck here, but still prints nothing for anerrors-less AggregateError. #39204 (blocked on a WebKit bump) makes the loader replay the original error witherrorsintact, and #39292 prints replayed build errors again. This PR only makes the printer safe for whatever object it gets, and keeps the normal output unchanged, so it does not change what those PRs do.Also run:
test/js/bun/resolve/build-error.test.ts,test/js/bun/util/reportError.test.ts,test/js/bun/util/inspect.test.js,test/js/bun/plugin/plugins.test.ts,test/js/bun/transpiler/transpiler-error-gc-uaf.test.ts,test/regression/issue/hashbang-still-works.test.ts(130 pass),rustfmt --check,cargo clippy -p bun_jsc,clang-formatonbindings.cpp, and the repros underBUN_JSC_validateExceptionChecks=1. The new tests spawn one process per case because the unfixed binary dies. The file takes about 4 s under the debug build.Fixes #36963
[review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file