Clear exceptions thrown by property lookups during inspect's property walk - #38463
Closed
robobun wants to merge 2 commits into
Closed
Clear exceptions thrown by property lookups during inspect's property walk#38463robobun wants to merge 2 commits into
robobun wants to merge 2 commits into
Claude / Claude Code Review
completed
Aug 14, 2026 in 17m 47s
Code review found 2 potential issues
Found 4 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 1 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/bun/util/inspect.test.js:971 |
Subprocess tests pipe stderr but never drain it |
| 🟣 Pre-existing | src/jsc/bindings/bindings.cpp:5684-5689 |
Same getPrototype().getObject() null-deref remains at napi.cpp:2080 |
Annotations
Check warning on line 971 in test/js/bun/util/inspect.test.js
claude / Claude Code Review
Subprocess tests pipe stderr but never drain it
Both new subprocess tests set `stderr: "pipe"` but never drain it — `Promise.all` only awaits `proc.stdout.text()` and `proc.exited`. REVIEW.md's "Subprocess tests: drain pipes concurrently" rule requires reading every piped stream, and the neighboring subprocess tests in this file all do so. Add `proc.stderr.text()` to the `Promise.all` (here and at line 1005), or drop `stderr: "pipe"` if stderr is intentionally ignored.
Check notice on line 5689 in src/jsc/bindings/bindings.cpp
claude / Claude Code Review
Same getPrototype().getObject() null-deref remains at napi.cpp:2080
Pre-existing, not blocking: the same `getPrototype(globalObject).getObject()` null-deref this hunk fixes has one other site in the tree — `src/jsc/bindings/napi.cpp:2080`, in `napi_get_all_property_names`'s descriptor-filter loop. A throwing Proxy `getOwnPropertyDescriptor`/`getPrototypeOf` trap there produces the same empty-JSValue → `.getObject()` segfault; worth the same split-and-guard treatment as a follow-up.
Loading