Conversation
process.report.getReport(err) previously ignored its argument entirely, always emitting the synthetic ERR_SYNTHETIC stack. It now validates that err is an object (ERR_INVALID_ARG_TYPE otherwise) and embeds the error's own .stack and own enumerable properties into javascriptStack, matching Node.js. The configuration properties on process.report (compact, directory, filename, signal, reportOnFatalError, reportOnSignal, reportOnUncaughtException, excludeEnv, excludeNetwork) are now native accessors with Node-compatible type validation (ERR_INVALID_ARG_TYPE for boolean/string mismatches, ERR_UNKNOWN_SIGNAL for an unrecognized signal name). Previously they were plain data properties that accepted any value. This also fixes two incidental bugs in the old constructor: the default for 'signal' was being written to the 'excludeEnv' key, and 'excludeNetwork' was missing entirely. writeReport() now validates its (file, err) overloads the same way Node does; the underlying write is still a no-op.
WalkthroughChangesRefactors Process report behavior
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:39 PM PT - Jul 16th, 2026
❌ @robobun, your commit 0f8584f has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34405That installs a local version of the PR into your bun-34405 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not duplicates of #34400 or #34403; the three are complementary:
The only functional overlap is the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/jsc/bindings/BunProcess.cpp`:
- Around line 2649-2650: The excludeEnv and excludeNetwork settings are exposed
but not applied when generating reports. Update POSIX report assembly in
src/jsc/bindings/BunProcess.cpp lines 2649-2650 to consult both fields and omit
the protected sections, update
src/jsc/bindings/BunProcessReportObjectWindows.cpp line 46 to pass process
through the equivalent exclusion logic on Windows, and add assertions in
test/js/node/process/process.test.js lines 1101-1112 verifying reports omit
environment and network data after each setting is enabled.
- Around line 2538-2559: Update Process_functionWriteReport in
src/jsc/bindings/BunProcess.cpp:2538-2559 to generate and write the process
report after argument normalization, then return the resulting output filename
instead of echoing file. Add a temp-directory test in
test/js/node/process/process.test.js:1052-1061 that verifies writeReport creates
the file and that it contains report data.
- Around line 2643-2647: Check for and propagate any pending exception
immediately after JSC::constructEmptyObject in the process report setup, before
mutating report with putDirectCustomAccessor or subsequent properties. Reuse the
surrounding DECLARE_TOP_EXCEPTION_SCOPE(vm) handling and preserve the existing
return behavior.
- Around line 2112-2116: Update the javascriptStack construction around
errorObject stack access to use a local catch scope, clearing and ignoring
exceptions from both the stack getter and getOwnPropertyNames() so
process.report.getReport() returns its fallback or partial report. Preserve
successfully retrieved stack data, and add coverage for throwing getters and
proxies.
🪄 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: 7be850e7-e876-4a52-866d-ac31b895ab93
📒 Files selected for processing (4)
src/jsc/bindings/BunProcess.cppsrc/jsc/bindings/BunProcess.hsrc/jsc/bindings/BunProcessReportObjectWindows.cpptest/js/node/process/process.test.js
…and properties getReport(err) now uses a TopExceptionScope around the .stack getter, property-name enumeration, and per-property reads so a throwing getter, throwing toString, or Proxy trap falls back to the "No stack." shape or skips the offending property instead of aborting the whole report. This matches Node's TryCatch-wrapped PrintJavaScriptErrorStack. Also adds the missing RETURN_IF_EXCEPTION after constructEmptyObject in constructProcessReportObject.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Custom getters/setters and getReport() receive the lexical global, which is a NodeVMGlobalObject (sibling of Zig::GlobalObject) when invoked from a node:vm context. Route through defaultGlobalObject() like the rest of this file instead of uncheckedDowncast.
…ctions for err
errorProperties enumeration now skips integer-index own keys, matching
Node and avoiding the putDirect ASSERT(!parseIndex(propertyName)) on
inputs like {stack: '...', 0: 'v'}.
getReport(err)/writeReport(err) now go through V::validateObject so
arrays and functions are rejected with ERR_INVALID_ARG_TYPE like Node.
writeReport's first-arg overload shuffle excludes callables so a
function argument fails validateString('file') rather than being
treated as err.
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 (1)
src/jsc/bindings/BunProcess.cpp (1)
2503-2503: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck for exceptions before storing the generated stack.
constructReportJavaScriptStackcan enter JS and return an empty value with a termination exception pending. Line 2503 immediately passes that result toputDirect.Proposed fix
- report->putDirect(vm, JSC::Identifier::fromString(vm, "javascriptStack"_s), constructReportJavaScriptStack(vm, globalObject, errorValue), 0); + auto javascriptStack = constructReportJavaScriptStack(vm, globalObject, errorValue); + RETURN_IF_EXCEPTION(scope, {}); + report->putDirect(vm, JSC::Identifier::fromString(vm, "javascriptStack"_s), javascriptStack, 0);As per coding guidelines, check for exceptions after every call that can throw or run user code before using its result.
🤖 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/BunProcess.cpp` at line 2503, Update the report construction flow around constructReportJavaScriptStack to check vm.exception() immediately after the call and before passing its result to report->putDirect. Return or propagate the pending exception using the surrounding function’s established error-handling path, while preserving the existing property assignment when no exception is pending.Source: Coding guidelines
🤖 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/BunProcess.cpp`:
- Line 2503: Update the report construction flow around
constructReportJavaScriptStack to check vm.exception() immediately after the
call and before passing its result to report->putDirect. Return or propagate the
pending exception using the surrounding function’s established error-handling
path, while preserving the existing property assignment when no exception is
pending.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c934786-31d3-4f41-9eed-3a750118c2e1
📒 Files selected for processing (2)
src/jsc/bindings/BunProcess.cpptest/js/node/process/process.test.js
…tDirect constructReportJavaScriptStack can return an empty JSValue when a termination exception is pending; check for that before passing the result to putDirect (which asserts on empty values).
…ll platforms isSignalName() on Windows only covers the seven libuv-registerable console signals, so the setter rejected its own "SIGUSR2" default. Validate against the full getSignalNames() table instead so names like SIGUSR1/SIGUSR2/SIGABRT are accepted on Windows, matching Node's os.constants.signals-based validateSignalName.
There was a problem hiding this comment.
No new issues found on this pass, but this is ~350 lines of JSC binding C++ (custom accessors, catch scopes over user-controlled property enumeration, new Process fields) that took several fix rounds to settle — worth a human look, especially on the declined SIGPWR/os.constants.signals parity question the author left to maintainer preference.
What was reviewed:
constructReportJavaScriptStack: exception-scope discipline around user.stackreads,getOwnPropertyNames, andtoString;parseIndexskip avoids theputDirectassert; the no-newline.stackcase correctly omits thestackarray.- Accessor macros /
setSignal: all sites go throughdefaultGlobalObject()(node:vm safe);getSignalNames()scan acceptsSIGUSR2on Windows. getReport/writeReportarg validation viaBun::V::validateObject— arrays/functions rejected;writeReportoverload shuffle excludes callables so functions route to thefileerror.
Extended reasoning...
Overview
This PR reworks process.report in src/jsc/bindings/BunProcess.cpp (+ the Windows twin and BunProcess.h): getReport(err) now embeds the passed error's stack into javascriptStack, writeReport validates its (file, err) overloads, and the nine configuration properties become native CustomAccessors backed by new Process member fields with validateBoolean/validateString/signal-name validation. The duplicated POSIX/Windows javascriptStack builders are unified into a shared constructReportJavaScriptStack. ~230 lines of tests added to process.test.js.
Security risks
Low but nonzero. constructReportJavaScriptStack enumerates and reads properties off a user-supplied object under a catch scope — the classic "user code can throw / re-enter" surface. The catch-scope handling looks correct (each get/toString failure is cleared and skipped, termination is propagated), and the earlier-found node:vm type-confusion and putDirect index-key assert have been fixed. No auth/crypto/filesystem writes are touched (writeReport remains a validation-only no-op).
Level of scrutiny
Medium-high. This is hand-written JSC binding C++ with ThrowScope/TopExceptionScope interleaving, macro-generated custom getters/setters, and new state on the Process object. The PR's own history — five fix commits addressing a node:vm type-confusion crash, a debug-build assert on integer-index keys, array/function validation gaps, and a Windows signal-table mismatch — demonstrates the surface is subtle enough to warrant human eyes even after automated review is clean.
Other factors
- All my prior blocking findings are resolved and covered by new tests (node:vm subprocess test, integer-index test,
SIGUSR2round-trip row). - One nit was intentionally declined: the
signalsetter validates against the fixed 32-entrygetSignalNames()table rather than the platform-derivedos.constants.signals, soSIGPWR/SIGSTKFLT/SIGPOLLare rejected on Linux andSIGINFO/SIGBREAKare over-accepted. The author left this to maintainer preference. m_reportOnUncaughtException(pre-existing, aliases the capture-callback flag) and the newm_reportReportOnUncaughtExceptionnow coexist onProcess— intentional per the PR description, but a maintainer may want to weigh in on the naming/duplication.- Candidate issues examined and ruled out this run: the "no newline in
err.stack→javascriptStack.stackomitted" case matches Node's shape (Node also omits thestackarray when the split yields a single line).
|
CI status: the |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-17, 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. |
1 participant
What does this PR do?
Fixes
process.reportargument validation and error embedding to match Node.js.Repro
Cause
Process_functionGetReportnever readcallFramearguments, so the passed error was discarded and the synthetic current stack was substituted. Theprocess.reportconfiguration properties were plainputDirectdata properties with no setter validation, so any value was accepted verbatim. The old constructor also wrote the"SIGUSR2"default onto the"excludeEnv"key instead of"signal", and omittedexcludeNetworkentirely.Fix
getReport(err): whenerris provided it must be a non-null object (ERR_INVALID_ARG_TYPEotherwise). Its.stackis split intojavascriptStack.message(first line) andjavascriptStack.stack(remaining trimmed lines), and its own enumerable properties (other thanstack/message) are stringified intojavascriptStack.errorProperties. If.stackis not a string, Node's"No stack." / ["Unavailable."]shape is used. Omittingerrkeeps the existing synthetic behavior.writeReport(file, err): validates the(string?, object?)/(object)overloads the same way Node does. The underlying write is still a no-op.compact,directory,filename,signal,reportOnFatalError,reportOnSignal,reportOnUncaughtException,excludeEnv,excludeNetwork) are now native accessors backed by fields onProcess. Setters runvalidateBoolean/validateString/validateSignalName, throwingERR_INVALID_ARG_TYPEorERR_UNKNOWN_SIGNAL.reportOnUncaughtExceptionuses its own storage so it no longer aliases thesetUncaughtExceptionCaptureCallback"already set" flag.javascriptStackbuilder is factored into a singleconstructReportJavaScriptStackshared by the POSIX and Windows report paths.Verification
bun bd test test/js/node/process/process.test.js -t "process.report"passes (18 tests) on linux-x64 and windows-x64; the new cases fail underUSE_SYSTEM_BUN=1.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process.test.js