Conversation
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
process.report.writeReport() was an echo stub: it returned its first argument verbatim (even an Error object) and never wrote a file. Callers checking the return value against the filename they passed would see "success" while nothing was written. writeReport() now serializes the getReport() object to JSON and writes it to disk, matching Node.js: - writeReport() generates report.YYYYMMDD.HHMMSS.<pid>.0.<seq>.json in the report directory (or cwd) and returns the filename - writeReport(filename) writes to that filename and returns it - writeReport(err) / writeReport(filename, err) accept an Error in either position; the return value is always the filename string - filename "stdout"/"stderr" writes the report to that stream - process.report.compact / directory / filename are honored - on open failure, writes a diagnostic to stderr and returns "" - validates the file argument as a string and err as an object with ERR_INVALID_ARG_TYPE Also fixes process.report.signal, which was being written under the "excludeEnv" key by mistake (so report.signal was undefined and report.excludeEnv was "SIGUSR2"). Wiring reportOnUncaughtException / reportOnSignal and the --report-* CLI flags to actually trigger reports is not included here.
38cc556 to
accc60b
Compare
|
Not a duplicate of #34400. That PR fixes the Rebased on #34400 and retargeted to its branch so they stack cleanly; |
WalkthroughChangesProcess report output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 2526-2549: Update constructReportObjectComplete and its callers to
accept and propagate the process.report.excludeEnv flag, then conditionally omit
the environmentVariables construction when the flag is enabled. Ensure the
report options parsing exposes the actual excludeEnv value rather than always
setting it to false.
- Line 2585: Update the Windows report construction around
constructReportObjectComplete so it receives and preserves the selected
filename, including values passed to writeReport such as "out.json", instead of
hardcoding header.filename to null.
- Around line 2520-2524: Update the report-generation flow around writeReport
and constructReportObjectComplete to pass the validated errArg through instead
of dropping it, using the existing object validation that rejects arrays. Ensure
constructJavaScriptStack derives javascriptStack from the supplied error, then
update the relevant test to parse the emitted JSON and assert javascriptStack
reflects that error rather than only the filename.
🪄 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: 2cbc6948-1a06-44e1-a1f2-4197eb8989cf
📒 Files selected for processing (2)
src/jsc/bindings/BunProcess.cpptest/js/node/process/process.test.js
Extract a shared constructReportJavaScriptStack used by both POSIX and Windows report builders. When an Error is passed to getReport(err) or writeReport(filename, err), its .stack is split into javascriptStack.message (first line) and javascriptStack.stack (remaining frames), matching Node. When no err is passed, the synthetic ERR_SYNTHETIC callstack is used as before. Also threads the resolved filename into the Windows report builder so header.filename is populated there too, and makes getReport(err) validate err as an Object.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/bindings/BunProcess.cpp:2504-2510— The overload dispatch usesarg0.isObject(), which in JSC is true for functions and arrays, sowriteReport(() => {})andwriteReport([])silently write a report file to cwd instead of throwingERR_INVALID_ARG_TYPElike Node does. Gating witharg0.isObject() && !arg0.isCallable()and rejecting arrays in theerrvalidation would match Node exactly.Extended reasoning...
What diverges
Node's
writeReport([filename][, err])overload dispatch inlib/internal/process/report.jsis:if (typeof file === 'object' && file !== null) { err = file; file = undefined; } else if (file !== undefined) { validateString(file, 'file'); } if (err !== undefined) validateObject(err, 'err');
The two relevant JS-semantics facts:
typeof (() => {})is'function'(not'object'), andvalidateObjectrejects arrays by default.This PR's dispatch at BunProcess.cpp:2504 uses
arg0.isObject(). In JSC,JSValue::isObject()checks whether the cell'sJSType >= ObjectType, andJSFunctionis an object subtype — so it returns true for functions. The same is true for arrays. The subsequenterrvalidation at line 2521 also useserrArg.isObject(), which again accepts both functions and arrays.Step-by-step:
process.report.writeReport(() => {})- Node:
typeof file === 'object'→'function' === 'object'→ false. Falls through tovalidateString(file, 'file')with a function → throwsERR_INVALID_ARG_TYPE("The "file" argument must be of type string"). - Bun (this PR):
arg0.isObject()→ true (functions are objects in JSC).errArg = arg0,fileArg = jsUndefined(). ThefileArg.isUndefined()check skips string validation. TheerrArg.isObject()check at line 2521 passes.fileis empty, so a defaultreport.YYYYMMDD.…jsonfilename is generated and a report file is written to cwd. Returns the filename string.
Step-by-step:
process.report.writeReport([])- Node:
typeof [] === 'object'→ true, soerr = []. ThenvalidateObject(err, 'err')rejects arrays → throwsERR_INVALID_ARG_TYPE("The "err" argument must be of type object"). - Bun (this PR):
arg0.isObject()→ true,errArg = [].errArg.isObject()at line 2521 → true, no throw. Writes a report file to cwd.
Why nothing else catches it
There is no downstream use of
errArgthat would reject a function or array — after validation,errArgis currently unused (the report doesn't yet incorporate the error's stack), so any object-typed value sails through and the side effect (file write) happens.Impact
Minor Node-compat divergence: passing a function or array — clearly a programmer error — writes a diagnostic file to disk instead of throwing. Nothing crashes and the API is rarely used, but it does perform an observable filesystem side effect on invalid input where Node fails fast.
Fix
Mirror Node's checks exactly:
if (arg0.isObject() && !arg0.isCallable()) { // typeof === 'object' && !== null errArg = arg0; fileArg = jsUndefined(); } else { … }
and for the
errvalidation, reject arrays and callables (matchingvalidateObject's defaults):if (errArg.isNull() || !errArg.isObject() || errArg.isCallable() || JSC::isArray(globalObject, errArg)) { return Bun::ERR::INVALID_ARG_TYPE(scope, globalObject, "err"_s, "Object"_s, errArg); }
(or use the existing
Bun::V::validateObjecthelper if one exists — the codebase already has theisObject() && !isCallable()pattern in bindings.cpp). - Node:
-
🟡
src/jsc/bindings/BunProcess.cpp:2520-2524— Theerrargument is parsed and type-validated here but never passed toconstructReportObjectComplete(which has no error parameter), sowriteReport(new Error('x'))produces a report identical towriteReport()— in Node the error's stack populates the report'sjavascriptStacksection. This is the same pre-existing gap asgetReport(err)at line 2489, so wiring it through means extendingconstructReportObjectCompleteand is arguably out of scope; just flagging the parsed-but-unused local.Extended reasoning...
What the bug is
Process_functionWriteReportparses the optionalerrargument at lines 2503–2510 (treating an object first-arg aserrper Node's overload rules) and validates it at lines 2520–2524 (rejecting non-object values withERR_INVALID_ARG_TYPE). After that,errArgis never referenced again. The report is built at line 2585 viaconstructReportObjectComplete(vm, zigGlobal, file), whose signature is(VM&, Zig::GlobalObject*, String fileName)— there is no error parameter to thread it through even if the caller wanted to.Code path
- User calls
process.report.writeReport(new Error('marker')). arg0.isObject()is true, soerrArg = arg0andfileArg = jsUndefined().errArgpasses the!errArg.isUndefined()/errArg.isObject()validation.- The default filename is generated, then
constructReportObjectComplete(vm, zigGlobal, file)is called with only the filename. - The report JSON is written to disk with no reference to the error's message or stack.
The result is byte-identical to what
writeReport()with no arguments would produce (modulo timestamp/seq).Why nothing else prevents it
The underlying report builder has never supported an error parameter —
getReport(err)at line 2489 has the identical gap (it doesn't even look atcallFrame->argument(0)). So this PR inherits an infrastructure limitation rather than introducing a new one; the newly-added validation just makes the unused argument more visible.Impact
In Node.js,
process.report.writeReport(err)populates the report'sjavascriptStacksection witherr.stackso the diagnostic report captures the error that prompted it. Here that context is silently dropped. Nothing crashes and the report file is still valid — this is an incomplete-compat gap, not a correctness bug. The PR remains a strict improvement over the previous stub, which returned the Error object verbatim without writing anything.Step-by-step proof
- Line 2504:
if (arg0.isObject()) { errArg = arg0; ... }—errArgbound. - Lines 2520–2524:
if (!errArg.isUndefined()) { if (errArg.isNull() || !errArg.isObject()) return ERR::INVALID_ARG_TYPE(...); }— validated, no other use. grep -n errArgin the function body after line 2524 → no hits.- Line 2585:
constructReportObjectComplete(vm, zigGlobal, file)— three args only. constructReportObjectCompletedeclaration (~line 2098):(VM&, Zig::GlobalObject*, const String& fileName)— no error slot.
The new test only asserts
typeof f2 === 'string'andf2 !== err; it never checks that'marker'appears in the written JSON, so this gap is untested.How to fix
Extend
constructReportObjectCompleteto accept an optionalJSValue errand, when present, populate thejavascriptStacksection fromerr.stack/err.message(matching Node'sreport.cc). Then passerrArgfrom bothwriteReportandgetReport. That's a larger change than this PR's scope (converting a stub into a working file writer), so it's reasonable to defer — but the validated-but-unused local is worth a comment or a follow-up. - User calls
The overload dispatch now matches Node's typeof check: a callable first
argument routes to validateString (so writeReport(() => {}) throws on
"file"), and err validation goes through Bun::V::validateObject which
rejects arrays and callables (so writeReport([]) and getReport([]) throw
on "err"). Adds test cases for each.
On Windows the report file is now opened with _wfopen over the UTF-16
path instead of narrow fopen on UTF-8 bytes, so non-ASCII directories
and filenames resolve correctly.
|
Addressed the review findings in 4d289cd:
|
…ties
writeReport now reads compact/directory/filename/excludeEnv/excludeNetwork
from process.report (via processObject) rather than thisValue, so a
detached const {writeReport} = process.report; writeReport() still honors
the config, matching Node and matching getReport in this file.
header.trigger is threaded through constructReportObjectComplete and set
to "API" for writeReport() and "GetReport" for getReport(), matching
Node.
javascriptStack.errorProperties for a user-supplied error now contains
its own-enumerable string-keyed properties (excluding name/message/stack,
values stringified), matching Node's PrintJavaScriptErrorProperties.
Tests updated to cover trigger, errorProperties, and detached calls.
|
CI on b07f3f9: all
The writeReport implementation is ready for review; verified locally on linux-x64 and windows-x64 with all 13 report tests passing and all 9 writeReport tests fail-before/pass-after. |
constructReportJavaScriptStack now uses a TopExceptionScope and swallows exceptions from reading err.stack and from per-property get/toString on errorProperties iteration, matching Node's per-property TryCatch. Index keys and Symbol-valued properties are skipped (Node skips them too), and termination exceptions are still propagated via clearExceptionExceptTermination. Also guard reportObjValue with an empty-value check before .isObject() in writeReport, matching the sibling getReport.
There was a problem hiding this comment.
I didn't find any bugs, but this implements a new user-facing Node.js compat API (process.report.writeReport) with ~200 lines of new C++ — file I/O via raw fopen/_wfopen, argument-overload disambiguation, per-property exception swallowing via clearExceptionExceptTermination, and cross-platform path handling — so it's worth a human look.
What was reviewed:
constructReportJavaScriptStackhostile-err hardening: throwing getters, Symbol values, and index keys are handled and covered by tests; termination exceptions are correctly propagated rather than cleared.writeReportoverload routing (arg0.isObject() && !arg0.isCallable()) against the test matrix — functions route tofileand arrays toerr, both rejected byvalidateString/validateObjectas tested.- Windows path shares the new
constructReportJavaScriptStackand threadstrigger/fileName/errValuethrough consistently.
Extended reasoning...
Overview
This PR implements process.report.writeReport() (previously a no-op stub returning its first argument) and extends process.report.getReport() to accept an err argument, matching Node.js. It touches src/jsc/bindings/BunProcess.cpp (+~250 lines), src/jsc/bindings/BunProcessReportObjectWindows.cpp (refactored to share the new constructReportJavaScriptStack helper and thread trigger/fileName/errValue through), and adds ~260 lines of tests in test/js/node/process/process.test.js.
The new code includes: a shared constructReportJavaScriptStack that either synthesizes a callstack or extracts .stack and own enumerable properties from a user-supplied error (swallowing per-property exceptions via clearExceptionExceptTermination to match Node's best-effort semantics); full writeReport argument disambiguation ([filename][, err]); reading of compact/excludeEnv/excludeNetwork/directory/filename from the live process.report object (receiver-agnostic — detached calls still honor config); auto-generated timestamped filenames with an atomic sequence counter; special-casing "stdout"/"stderr"; and file writing via fopen/_wfopen with stderr diagnostics on failure.
Security risks
Low but non-trivial. writeReport writes to a filesystem path constructed from user-controlled process.report.directory + filename, but this is by design (Node does the same) and requires the caller to already control JS execution. The hostile-err handling deliberately swallows exceptions from user getters/toString on the error object; termination exceptions are preserved via clearExceptionExceptTermination() rather than clearException(), which is the correct pattern. No injection or auth surface.
Level of scrutiny
High. This is a new user-facing Node-compat API with substantial native implementation surface: raw stdio file I/O (rather than bun.sys wrappers — though this is C++ bindings, not Rust), manual path-separator concatenation, a DECLARE_TOP_EXCEPTION_SCOPE with several clearExceptionExceptTermination sites (the repo's guidance flags clearException generally), getIfPropertyExists empty-value handling, and Windows-specific _wfopen/localtime_s/_getpid branches. API-shape decisions (return empty string on open failure, stderr banner text, header.trigger values "GetReport" vs "API", stringifying all errorProperties) should be verified against Node's actual behavior by a human familiar with the compat surface.
Other factors
Test coverage is thorough — auto-generated filename shape, arg validation matrix (numbers/functions/arrays/null for both file and err), directory/compact/filename config, open-failure path, stdout sink, err threading into javascriptStack, hostile err objects (index keys, Symbol values, throwing getters on both properties and .stack), and detached-call receiver-agnostic config. All tests spawn subprocesses with bunEnv and drain pipes concurrently. Given the size and the number of design/compat choices embedded here, this warrants a maintainer review rather than auto-approval.
This branch has not been deployed
1 participant
Stacked on #34400.
What
process.report.writeReport()was a// TODO:stub that returnedcallFrame->argument(0)verbatim, so:This reads as success to anything inspecting the return value while nothing is actually written to disk.
Fix
Process_functionWriteReportinsrc/jsc/bindings/BunProcess.cppnow:writeReport([filename][, err])with Node's overload rules (object first-arg iserr).fileas a string anderras an object (ERR_INVALID_ARG_TYPE).compact/directory/filename/excludeEnv/excludeNetworkfrom theprocess.reportreceiver.report.YYYYMMDD.HHMMSS.<pid>.0.<seq>.jsonwhen none is provided.constructReportObjectComplete, serializes withJSC::JSONStringify(2-space indent, or single line whencompact), and writes it with a trailing newline."stdout"/"stderr", writes the JSON to that stream and returns the name (no stderr banner).directory/filename(or cwd), printsWriting Node.js report to file: .../Node.js report completedto stderr, and returns the filename. If the file can't be opened, printsFailed to open Node.js report file: ... (errno: N)to stderr and returns"".The
errargument is now threaded through to the report body.constructReportJavaScriptStackis extracted as a shared helper (used by both POSIX and Windows builders) that derivesjavascriptStack.messageandjavascriptStack.stackfrom the supplied error's.stackwhen present, falling back to the syntheticERR_SYNTHETICcallstack otherwise.getReport(err)is wired the same way and now validateserr. The Windows report builder also receives the resolvedfileNamesoheader.filenameis populated there too.Verification
Added a
process.report.writeReportdescribe block totest/js/node/process/process.test.jscovering: default/explicit filename, Error-arg overload (return value is a string, not the Error), file contents are parseable JSON withheader.filenameset, argument validation,directory+compact+filenameconfig, open-failure returns"","stdout"special target, andjavascriptStack.messagereflecting a supplied Error for bothwriteReportandgetReport.All writeReport tests fail on current main and pass with this change. Behavior checked against Node v26.3.0.
Not in this PR
reportOnUncaughtException,reportOnSignal,reportOnFatalError, and the--report-*CLI flags still do not trigger reports; those need separate wiring into the uncaught-exception and signal paths.no test proof · iteration 1 · 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