Repository navigation
Conversation
…ender like node fs streams (options.fd), zlib's sync convenience methods, diagnostics_channel channel()/tracingChannel() and fs._toUnixTimestamp passed a pre-joined string such as "number or FileHandle" as the expected type, so the message read "must be of type number or FileHandle". Node passes an array and groups the entries into "of type ..." / "one of type ..." / "an instance of ...". The array overload of the error builder already does that grouping, so pass the same arrays node passes. Restores test-zlib-not-string-or-buffer.js to its upstream wording.
|
Warning Review limit reached
Next review available in: 31 minutes 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 (8)
Comment |
|
Status: reproduced and fixed, waiting on CI.
|
There was a problem hiding this comment.
LGTM — mechanical Node-compat fix that switches six $ERR_INVALID_ARG_TYPE sites from pre-joined strings to arrays so the existing message builder's grouping renders node's exact text.
What was reviewed:
- Traced each new array through the
ErrorCode.cpparray overload (types/instances/other grouping, theobject→Objectmove) and confirmed it produces the exact strings the tests assert. - The odd-looking
["Date", "Time in seconds"]and["string", "object", "TracingChannel"]lists match node's own arrays and yield node's verbatim (if awkward) wording. - Existing diagnostics_channel regex assertions were tightened to exact matches, not weakened; the vendored zlib test is restored to upstream.
Extended reasoning...
Overview
Six one-line source changes across src/js/internal/fs/streams.ts (2 sites), src/js/node/diagnostics_channel.ts (2 sites), src/js/node/fs.ts (_toUnixTimestamp), and src/js/node/zlib.ts (zlibBufferSync), each replacing a pre-joined "X or Y" string with the array node passes. Four test files add exact-message assertions and test-zlib-not-string-or-buffer.js is restored to the upstream node file.
I read the ERR_INVALID_ARG_TYPE array overload at src/jsc/bindings/ErrorCode.cpp:690 and traced each new array through it: ["number", "FileHandle"] → one primitive + one class → "of type number or an instance of FileHandle"; ["string", "symbol"] → two primitives → "one of type string or symbol"; ["string", "object", "TracingChannel"] → the special-case at line 718 moves object into instances as Object → "of type string or an instance of TracingChannel or Object"; ["Date", "Time in seconds"] → one class + one free-form phrase → "an instance of Date or an Time in seconds"; ["string", "Buffer", "TypedArray", "DataView", "ArrayBuffer"] → one primitive + four classes. Each matches the test assertions and the node output quoted in the description.
Security risks
None. This only changes the second argument to an error-message helper on already-throwing validation paths. No new inputs are accepted, no parsing, no allocation shape changes.
Level of scrutiny
Low. The change is error-message text only, in the Node compat layer where node's observed output is the contract. The array overload is already the shape used by ~70 other call sites in src/js; nothing in the error builder is being modified.
Other factors
- Tests are strengthened, not weakened: the two pre-existing diagnostics_channel regexes are replaced with exact
code+messagematches. The vendored zlib test that had been edited to bun's old wording is restored byte-for-byte to upstream. - New tests cover multiple entry points per site (createReadStream/createWriteStream/new fs.ReadStream/new fs.WriteStream, five zlib sync methods, channel/subscribe/unsubscribe/tracingChannel,
_toUnixTimestampwith default and customname), and multiple received-value shapes (primitive, bigint, object, null, undefined) so the "Received …" tail is exercised too. - The two source comments citing node source lines are appropriate given the surprising wording ("an Time in seconds") and the
TracingChannelentry that isn't obviously right without the reference. - The PR notes overlap with #35429 on the zlib line; both are the same one-line change and rebase cleanly.
Problem
ERR_INVALID_ARG_TYPEmessages thrown from the TypeScript builtins do not match node's text.fs.createReadStream(null, { fd: "x" })saysThe "options.fd" property must be of type number or FileHandle. Received type string ('x'); node v26.3.0 says... must be of type number or an instance of FileHandle. ....zlib.*Sync(notABuffer): bunof type string, Buffer, TypedArray, DataView, or ArrayBuffer, nodeof type string or an instance of Buffer, TypedArray, DataView, or ArrayBufferdiagnostics_channel.channel(123)(alsosubscribe/unsubscribe): bunof type string or symbol, nodeone of type string or symboldiagnostics_channel.tracingChannel(123): bunmust be an string, object, or Channel, nodemust be of type string or an instance of TracingChannel or Objectfs._toUnixTimestamp("x"): bunof type number or Date, nodean instance of Date or an Time in seconds"number or FileHandle", or["string, object, or Channel"]), so they hit the single-string overload of the message builder (src/jsc/bindings/ErrorCode.cpp:631), which printsof type+ the string verbatim. Node passes an array (['number', 'FileHandle']) and groups the entries. Sites:src/js/internal/fs/streams.ts:173and:418,src/js/node/zlib.ts:117,src/js/node/diagnostics_channel.ts:222and:280,src/js/node/fs.ts:1025.Fix
ErrorCode.cpp:690) is already a port of node's grouping and is what the other ~70 array call sites insrc/jsuse, so the output is node's text byte for byte, including node'san Time in seconds(lib/internal/fs/utils.js#L795) and theTracingChannelentry (lib/diagnostics_channel.js#L315-L317), which are cited at the two sites where the list looks odd.node:*modules node's observed output is the contract; every new assertion below is the string real node v26.3.0 prints for the same call.src/js(scanned all 232$ERR_INVALID_ARG_TYPEcalls). The two other joined strings (_http_client.ts:72,tls.ts:1778) already render node's text and are untouched. The"path"site next to the fixed ones instreams.tsis left alone because its real problem is that Buffer paths are rejected, which fs: accept Buffer paths in createReadStream/createWriteStream #35981 fixes.zlib.tsline and the vendored test restore below are also contained in node: Node-accurate ERR_INVALID_ARG_TYPE / ERR_INVALID_THIS / ERR_MISSING_ARGS; resync test/common and 117 vendored tests with v26.3.0 #35429; they are one line each and rebase cleanly whichever lands first.test/js/node/fs/fs.test.ts: newoptions.fdcases forcreateReadStream,createWriteStream,new fs.ReadStream,new fs.WriteStream(primitive, bigint and object values) andfs._toUnixTimestampcases (default and explicitname). 21 new tests fail on the unfixed build, pass with the fix; the whole file passes (532 pass, 0 fail).test/js/node/zlib/zlib.test.js: new cases across deflate/gunzip/brotli/zstd/inflateRaw sync methods; fail before, pass after; file passes (392 pass).test/js/node/diagnostics_channel/diagnostics_channel.test.ts: the two existing regexes for the old wording are replaced with node's exact text, pluschannel/unsubscribe/tracingChannelcases; 4 fail before, all pass after.test/js/node/test/parallel/test-zlib-not-string-or-buffer.js: restored to the upstream v26.3.0 file byte for byte (it had been edited to bun's wording); exits 1 on the unfixed build, 0 with the fix, 0 under real node.test-diagnostics-channel-*.js,test-fs-timestamp-parsing-error.js,test-fs-utimes.jsand the fd/FileHandle stream tests still exit 0.ERR::INVALID_ARG_TYPE(string_decoder, process, crypto); that is a separate change, not touched here.Background
ERR_INVALID_ARG_TYPEmessage shape: node'slib/internal/errors.jstakes the expected types as an array and splits it into primitive type names (of type string, orone of type string or symbolwhen there are several), class names (an instance of A, B, or C) and free-form phrases, joined withor. A primitive and a class together therefore readof type number or an instance of FileHandle.$ERR_INVALID_ARG_TYPE(name, expected, value)insrc/jscompiles to a call intosrc/jsc/bindings/ErrorCode.cpp. Ifexpectedis a JS array it goes to the grouping overload described above; if it is a string, the string is printed as-is afterof type(or afteranwhen it is a single capitalized phrase). So a list joined in JS skips the grouping, which is the whole bug.bun 1.4.0 vs node v26.3.0 for the affected calls
With this change every bun line above is identical to the node line.