Repository navigation
Conversation
…he value themselves ERR_UNKNOWN_ENCODING, the fixed-template codes (ERR_INVALID_HTTP_TOKEN and others), ERR_UNKNOWN_SIGNAL from child_process, crypto.randomInt and the http2 copy of determineSpecificType printed -0 as 0. Node prints -0. Route the C++ sites through a %s-style renderer that keeps the sign and, like util.format, prints an object with a user toString through String(). Format randomInt's max from the double instead of the truncated i64.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe PR updates Node-compatible error formatting. It adds shared ChangesError formatting compatibility
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The formatting changes have no remaining supported compatibility issue and are ready to merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
A scope that ends inside the object branch simulates a throw that the tail call into JSValueToStringSafe never checks. Hold one scope for the whole function and release it on the tail call.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked: the exception handling in hasUserToString/JSValueToStringLikeFormatS (every object->get and toWTFString is followed by RETURN_IF_EXCEPTION, and the tail call goes through RELEASE_AND_RETURN); the deleted StringView overload of UNKNOWN_ENCODING has no remaining callers (all five sites in JSBuffer.cpp, JSStringDecoder.cpp and CryptoUtil.cpp pass a JSValue); and the 124-byte stack buffer in randomInt matches the &mut [u8; 124] contract of FormatDouble::dtoa_with_negative_zero in src/bun_core/fmt.rs.
Extended reasoning...
Findings were reported inline, so this body only records what else was examined and ruled out. Read the ErrorCode.cpp/.h and node_crypto_binding.rs hunks directly: the new C++ helpers carry a ThrowScope with checks after each JS-entering call, no non-throwing accessor is used on an unvalidated value, and the Proxy walk bails on revoked proxies. Grepped the tree for remaining UNKNOWN_ENCODING( callers and confirmed all pass the original JSValue. Confirmed the dtoa buffer size against the Rust FFI signature. None of these produced an additional issue; the inline findings (function rendering, bound-function/callable-Proxy detection, inspect depth) remain the substantive concerns a human should weigh.
4 verified lower-impact observations (convention, logging or cleanup points) were not posted.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/jsc/bindings/webcore/streams/ReadableStreamOperations.cpp— pre-existing: ReadableStream.from(x) with an x whose toString or Symbol.toPrimitive is user code still reports{ toString: [Function: toString] } must be iterable, where Node's ERR_ARG_NOT_ITERABLE ('%s must be iterable') printsx must be iterable. throwNotIterable at ReadableStreamOperations.cpp:812 keeps calling JSValueToStringSafe although its own comment says it renders the argument the way %s does, and this PR adds the %s renderer without switching this sibling. Fix: every Node %s code rendered in C++ goes through JSValueToStringLikeFormatS; replace the JSValueToStringSafe call here (the C++ ERR_OPERATION_FAILED case at ErrorCode.cpp:2091 is the other %s site, though its callers pass strings).Extended reasoning...
The PR introduces JSValueToStringLikeFormatS (ErrorCode.cpp:449) as the %s renderer and applies it to the simpleErrorMessages table, the JS ERR_UNKNOWN_ENCODING case and the C++ UNKNOWN_ENCODING overload.
ReadableStreamOperations.cpp:806-816 is a %s site by its own comment ("renders the argument the way%sdoes") and Node defines ERR_ARG_NOT_ITERABLE as '%s must be iterable'.
It calls Bun::JSValueToStringSafe(globalObject, builder, iterable, false), which for any object goes to Bun__inspect_singleline (ErrorCode.cpp:421) regardless of a user toString.
Trigger: ReadableStream.from({ toString() { return 'x'; } }) or ReadableStream.from(new Proxy(objWithUserToString, {})).
Node: hasBuiltInToString is false, %s uses String(obj), message 'x must be iterable'.
Bun before and after this PR: '{ toString: [Function: toString] } must be iterable'.
The base already behaves this way; the PR interacts with this line by changing JSValueToStringSafe (the -0 hunk) and by creating the helper that should cover it, so it is the remaining member of the class the PR fixes.
ERR_OPERATION_FAILED at…Verification: pre-existing — triggered whenever ReadableStream.from(x) is called with a non-iterable object whose toString / Symbol.toPrimitive is user code. Mechanism verified: src/jsc/bindings/webcore/streams/ReadableStreamOperations.cpp:812 (untouched by this PR) still calls
Bun::JSValueToStringSafe(globalObject, builder, iterable, false), and JSValueToStringSafe (ErrorCode.cpp:353-421) has no… -
🟣
src/jsc/bindings/JSBuffer.cpp— Callers of Buffer#toString or Buffer#write with an encoding object whose valueOf returns a valid name get ERR_UNKNOWN_ENCODING, where Node decodes successfully. Bun's parseEncoding coerces with arg.toString() at src/jsc/bindings/JSBuffer.cpp:336 (hint string, toString first), while Node's getEncodingOps doesencoding += ''(hint default, valueOf first). The PR rewires this exact error site to render the object like Node but leaves the coercion that decides whether the error fires at all. Fix: coerce with ToPrimitive default hint (JSValue::toPrimitive then toString) at parseEncoding for the validateUnknown=false callers, so{valueOf(){return 'utf8'}}resolves to utf8 as in Node.Extended reasoning...
User calls
Buffer.from('hi').toString({ valueOf() { return 'utf8'; } }). jsBufferPrototypeFunction_toString reaches parseEncoding(scope, global, arg1, false) at JSBuffer.cpp:2303. Line 336 calls arg.toString(), which runs ToPrimitive with hint string: Object.prototype.toString wins and returns '[object Object]'; valueOf is never consulted. parseEnumerationFromView fails, validateUnknown is false, so line 327 throws UNKNOWN_ENCODING(arg). After this PR the message is 'Unknown encoding: { valueOf: [Function: valueOf] }'. Node's Buffer.prototype.toString calls getEncodingOps(encoding) which doesencoding += '': ToPrimitive with hint default calls valueOf first, gets 'utf8', and the call returns 'hi'. Same for Buffer#write at JSBuffer.cpp:2557 and 2595. Base behaves the same way (I checked line 336 is untouched), so this is pre-existing, but the dismissing finder only noted the message text changed; the real divergence is throw versus success. The PR's test block asserts object-argument parity for Buffer.toString and would have caught this with a valueOf-only fixture. Remedy: in the JSValue…Verification: pre-existing — triggered when a caller passes Buffer#toString / Buffer#write an object encoding whose valueOf (not toString) yields a valid name, e.g.
Buffer.from('hi').toString({ valueOf() { return 'utf8' } }). Mechanism verified: the JSValue overload of parseEncoding at /home/claude/bun/src/jsc/bindings/JSBuffer.cpp:336 doesauto arg_ = arg.toString(lexicalGlobalObject);(ECMAScript…
…ions as source util.format's %s sends a function and an object without a built-in toString through String(). Only the ECMAScript globals count as built-in constructors, so Buffer, TypedArray and URL print through String() too. ReadableStream.from's ERR_ARG_NOT_ITERABLE uses the same renderer. randomInt adds numerical separators to a received max beyond 2^32, as its max - min branch already did. The http2 copy of determineSpecificType renders a bigint.
robobun
changed the title
|
Updated 12:36 PM PT - Sep 17th, 2026
✅ @robobun, your commit 4e69367bb5ff243cf4c921d734d3738887102434 passed in 🧪 To try this PR locally: bunx bun-pr 43087That installs a local version of the PR into your bun-43087 --bun |
These rows come from #43069, which carries the same JSValueToStringSafe change. They cover ERR_OUT_OF_RANGE from C++ and from JS, ERR_INVALID_ARG_VALUE, ERR_UNKNOWN_ENCODING from a JS builtin and ReadableStream.from.
|
I moved the seven test rows of #43069 into this PR (commit 4e69367, test file only) and closed #43069. That PR carried the same The rows are in a new block, "a received -0 keeps its sign at sites that use the shared value renderer". They cover I updated the PR body to match. It says that this PR replaces #43069, and it counts four new test blocks. |
### Problem
- Seven Node error codes have a broken `.message`. `http.request({
headers: { Trailer: "X-T" } }).end()` throws `ERR_HTTP_TRAILER_INVALID`
with the message `undefined`. `url.fileURLToPath(httpUrl, { windows:
true })` throws `ERR_INVALID_URL_SCHEME` with the message `file`. Notes
list all seven.
- The cause is the tail of `jsFunctionMakeErrorWithCode`
(`src/jsc/bindings/ErrorCode.cpp:2478`). A code with no message template
uses its first argument as the whole message. These call sites pass
Node's template arguments, or nothing.
- The reverse also happens. A sentence passed to a templated code gives
`Cannot call Stream is destroyed after a stream was destroyed`.
### Fix
- Four codes get Node's template: `ERR_HTTP_TRAILER_INVALID`,
`ERR_SCRIPT_EXECUTION_INTERRUPTED`, `ERR_HTTP_CONTENT_LENGTH_MISMATCH`,
`ERR_INVALID_URL_SCHEME`. The server `strictContentLength` check throws
from Rust (`NodeHTTPResponse.rs`). It now formats Node's sentence with
both byte counts.
- The call sites of three codes now pass Node's argument:
`ERR_STREAM_DESTROYED("write")`, `ERR_OPERATION_FAILED("write failed
after retries")`, `ERR_METHOD_NOT_IMPLEMENTED("FileHandle with fs")`.
Dead arguments are gone.
- Verified: `test/js/node/errors/error-code-messages.test.ts`. Bun
1.4.3-canary.1+367d939d9 fails 7 of its 8 tests. The expected messages
are node v26.3.0's.
- Self-reviewed: 12 concerns raised, 8 addressed, 4 rejected (see
Notes).
### Background
- Built-in JS writes `$ERR_FOO(a, b)`. The build turns it into a call of
`jsFunctionMakeErrorWithCode`, which builds the message in C++.
- A message template is a `case` in that function, or a row in its
`simpleErrorMessages` table: fixed text around one or two arguments.
- `strictContentLength` makes `node:http` throw when the body size
differs from `Content-Length`. The client checks in JS. The server
checks in native code, which never reaches the C++ template.
<details><summary>Notes</summary>
Before and after, per code. The "after" text is identical to node
v26.3.0.
| code | call | before | after |
| --- | --- | --- | --- |
| `ERR_HTTP_TRAILER_INVALID` | client request with a `Trailer` header
and no chunked body | `undefined` | `Trailers are invalid with this
transfer encoding` |
| `ERR_HTTP_CONTENT_LENGTH_MISMATCH` | client `req.end("abc")`,
`Content-Length: 5` | `3` | `Response body's content-length of 3 byte(s)
does not match the content-length of 5 byte(s) set in header` |
| `ERR_HTTP_CONTENT_LENGTH_MISMATCH` | server `res.end("abc")`,
`Content-Length: 5` | `Content-Length mismatch` | the same sentence |
| `ERR_INVALID_URL_SCHEME` | `fileURLToPath(httpUrl, { windows })` |
`file` | `The URL must be of scheme file` |
| `ERR_STREAM_DESTROYED` | `res.destroy(); res.write("x", cb)` | `Cannot
call Stream is destroyed after a stream was destroyed` | `Cannot call
write after a stream was destroyed` |
| `ERR_SCRIPT_EXECUTION_INTERRUPTED` | REPL, Ctrl+C during `await` |
`undefined` | ``Script execution was interrupted by `SIGINT` `` |
| `ERR_OPERATION_FAILED` | `FileHandle` writer, every write returns 0
bytes | `Operation failed: Operation failed: write failed after retries`
| `Operation failed: write failed after retries` |
| `ERR_METHOD_NOT_IMPLEMENTED` | `createReadStream(null, { fd:
fileHandle, fs })` | `The fs.FileHandle with custom fs operations method
is not implemented` | `The FileHandle with fs method is not implemented`
|
- How the list was made: a sweep over `src/js` for every `$ERR_X(` call,
split by whether `X` has a `case` or a table row in `ErrorCode.cpp`. 57
codes have no template. All of them pass a full sentence, except the
four above and `ERR_HTTP2_UNSUPPORTED_PROTOCOL`. The reverse direction
(a sentence passed into a template) gave `ERR_STREAM_DESTROYED` and
`ERR_OPERATION_FAILED`. `ERR_METHOD_NOT_IMPLEMENTED` passes a fragment,
but not the one Node passes.
- The ported `test-fs-read-stream-file-handle.js` gets its upstream
`message:` assertion back. It was commented out because of the
`ERR_METHOD_NOT_IMPLEMENTED` text.
- Deleted arguments: the sentence in two
`$ERR_HTTP_TRAILER_INVALID(...)` calls in `_http_server.ts`, and the
`...args` of the REPL wrapper for `ERR_SCRIPT_EXECUTION_INTERRUPTED`.
The constant-message `case` ignores them.
- `fileURLToPathBuffer` (`url.ts:1320`) passed the whole sentence and
was correct. With the new table row it passes `"file"`, like its sibling
and like Node. Without that edit the message would read `The URL must be
of scheme The URL must be of scheme file`.
- `src/js/builtins.d.ts` declares the argument shapes of the four codes
that got a template. Without a declaration the code generator emits
`(message: string)`.
- Not in this PR: `ERR_HTTP2_UNSUPPORTED_PROTOCOL`
(`http2.connect("ftp://...")` prints `ftp:`) has the same cause. A
separate change owns it (branch
`robobun/d80742c6/http2-unsupported-protocol`), so this PR does not add
its row. #43087 also edits `makeSimpleErrorMessage` and adds a row at
the end of the table. The new rows here are in the middle of the table
to keep the merges clean.
- Not in this PR: `fetch()` also throws
`ERR_HTTP_CONTENT_LENGTH_MISMATCH` (`FetchTasklet.rs`) with its own
sentence about the request body. That is a Bun `fetch` error, not a
`node:http` one, so its text stays.
- Not in this PR: `fs.readFileSync(new URL("http://example.com"))`
throws `ERR_INVALID_URL_SCHEME` from `src/runtime/node/types.rs` with
the text `URL must be a non-empty "file:" path`.
`ERR_INVALID_FILE_URL_PATH` and `ERR_INVALID_FILE_URL_HOST` share that
same sentence there. A separate change fixes the three together, because
the path and host texts need more than a new literal.
- Three of the five `ERR_OPERATION_FAILED` call sites doubled the
prefix, all in the `FileHandle` writer. The other two already pass
Node's argument. The test reaches the synchronous writer site: it
replaces `fs.writeSync` with a function that returns 0, and the writer
looks `writeSync` up on the public module at call time. The two async
sites bind `write` and `writev` at module load, so a test cannot make
them return 0. They get the same change. Node's own writer calls its
binding directly, so this expected string comes from Node's source
(`'Operation failed: %s'` with `'write failed after retries'`) and not
from a Node run. Every other expected string is Node's output for the
same call.
- The REPL test asserts that the output contains the message, not the
whole line. Bun prints `Uncaught Error: <message>` where Node prints
`Uncaught:` and the inspected error with its `[ERR_...]` bracket. That
difference is about the stack header, not the message.
- Node does not check the first `res.write()` against `Content-Length`
(its `_contentLength` is still null at that point). Bun's native check
does. The tests use a second write, which both runtimes reject with `6
byte(s)`.
- Found on the way and not changed here (#43520). Four templates in
`ErrorCode.cpp` differ from Node's own text: `ERR_HTTP_SOCKET_ASSIGNED`
(`Socket already assigned`, Node: `ServerResponse has an already
assigned socket`), `ERR_TLS_INVALID_PROTOCOL_VERSION` and
`ERR_TLS_PROTOCOL_VERSION_CONFLICT` (Node formats the values with `%j`,
so it prints `"TLSv9" is not a valid minimum TLS protocol version`), and
`ERR_IPC_CHANNEL_CLOSED` (`Channel closed.`, Node has no period). Those
are wrong templates, not call sites that miss a template. A script
compared the 118 constant and table messages with the literal templates
in Node's `lib/internal/errors.js`. 108 are identical, and these four
differ.
- Found on the way and not changed here (#43519). The server
`strictContentLength` check differs from Node in behavior:
`Content-Length: 0` is never checked, the first `res.write()` is checked
(Node checks from the second write on), and a string header is parsed
with `parseInt` where Node uses `+value`.
- Self-review, the four concerns I did not act on. (1) Fix the
`types.rs` arm of `ERR_INVALID_URL_SCHEME` here: a separate change fixes
the three arms together. (2) Add the `ERR_HTTP2_UNSUPPORTED_PROTOCOL`
row here: a separate change owns it. (3) Fix `ERR_HTTP_SOCKET_ASSIGNED`
and the `Content-Length` parsing here: they are a different class,
tracked in #43520 and #43519. (4) The REPL test passes a 20 s timeout,
and `test/CLAUDE.md` says not to set one: `node:repl` takes 5 to 9 s to
load on a debug build with ASAN, and `test/js/bun/repl/repl.test.ts`
uses the same value for the same reason.
- Earlier work: #35791 covered five of these codes and was closed as
stale with conflicts, not on its merits. #35777 covered two. This PR
follows the review threads of #35791: the server path, the REPL code,
and the dead arguments.
- Suites run on the debug build: `error-code-messages.test.ts`,
`test/js/node/url/url-fileurltopath*.test.*`,
`node-http-transfer-encoding.test.ts`, `node-http.test.ts`,
`test/js/node/fs/promises.test.js`, and the ported
`test-http-content-length-mismatch.js`,
`test-http-server-de-chunked-trailer.js`, `test-http-set-trailers.js`,
`test-url-fileurltopath.js`, `test-fs-whatwg-url.js`,
`test-fs-read-stream-file-handle.js`, `test-repl-sigint.js`,
`test-repl-sigint-nested-eval.js`, `test-worker-unsupported-path.js`. In
`node-http.test.ts`, `should propagate exception in sync data handler`
timed out once in the full run and passes alone in 3 s.
</details>
This branch has not been deployed
1 participant
Problem
-0as0. Node prints-0(issue Node error messages print a received -0 as 0 at several sites (Node prints -0) #43070).Buffer.alloc(1).toString(-0),new StringDecoder(-0),Readable#setEncoding(-0),crypto.randomInt(-0),http.validateHeaderName(-0),http2.getUnpackedSettings(-0)andspawnSyncwithkillSignal: -0all lose the sign.JSBuffer.cppandJSStringDecoder.cpppass aWTF::StringtoERR::UNKNOWN_ENCODING. The fixed-template table inErrorCode.cpp(makeSimpleErrorMessage) usestoWTFString, so it also prints a plain object as[object Object]where Node prints{ a: 1 }.child_process.tsbuildsERR_UNKNOWN_SIGNALfrom a template string.node_crypto_binding.rstruncatesmaxtoi64before it formats.http2.tshas a JS copy ofdetermineSpecificType.Fix
JSValueToStringLikeFormatSinErrorCode.cpprenders a value like the%sofutil.format. A function or an object without a built-intoStringgoes throughString(). Any other object goes throughutil.inspect.hasBuiltInToStringis a port of Node's, with Node's list of built-in constructor names.JSValueToStringSafekeeps the sign of-0. This PR replaces node errors: render -0 with its sign in JSValueToStringSafe #43069, which had the same hunk.ERR_UNKNOWN_ENCODING,ERR_ARG_NOT_ITERABLEand the C++UNKNOWN_ENCODINGhelper use that renderer.UNKNOWN_ENCODINGnow takes only theJSValue, and the four call sites pass the original argument.ERR_UNKNOWN_SIGNALjoins the table, sochild_process.tsthrows$ERR_UNKNOWN_SIGNAL.randomIntformatsmaxbefore truncation and adds numerical separators beyond 2^32, as itsmax - minbranch already did. Thehttp2.tscopy renders-0and a bigint.test/js/node/errors/error-code-messages.test.ts(four new blocks, stock bun fails all four). Alsobuffer.test.js,string_decoder,streams.test.js,node-http.test.ts,node-http2.test.js,crypto.test.ts,child_process/and the Node parallel tests for spawnSync signals,process.kill,Buffer#toString,StringDecoderand header validation.Background
util.inspect, or with the%sofutil.format. Both keep the sign of-0. JSString(-0)is"0".$ERR_*codes that JS builtins throw go throughjsFunctionMakeErrorWithCodeinErrorCode.cpp. Codes whose message is fixed text around one or two%sarguments come from thesimpleErrorMessagestable.%s(hasBuiltInToStringinlib/internal/util/inspect.js) walks to the prototype that ownstoStringorSymbol.toPrimitiveand checks whether its constructor is one of the ECMAScript globals.Buffer,URLandUint8Arrayare not in that list, so they print throughString().Notes
hasBuiltInToStringinstead of a host-function heuristic,ReadableStream.fromon the same renderer, and a title that names the table codes. Not done: dropping thehttp2.tsline because node:http2: throw getUnpackedSettings/respondWithFD ERR_INVALID_ARG_TYPE through the shared error machinery #38644 deletes that function (the line is correct on its own, and node:http2: throw getUnpackedSettings/respondWithFD ERR_INVALID_ARG_TYPE through the shared error machinery #38644 needs node errors: render -0 and constructor-less objects in determineSpecificType like node #38642 too before the C++ path prints-0). The review also asked to stack this PR on node errors: render -0 with its sign in JSValueToStringSafe #43069. That is moot: node errors: render -0 with its sign in JSValueToStringSafe #43069 is closed, and its seven test rows are the first new test block here.crypto.pbkdf2Sync(Rustout_of_rangeformatter, node: render the Rust validators' ERR_OUT_OF_RANGE values like JS #40737) andprocess.chdir(C++determineSpecificType, node errors: render -0 and constructor-less objects in determineSpecificType like node #38642) from the issue are covered by those open PRs and are not in this one.ERR_UNKNOWN_SIGNALfromchild_processnow has the same shape as every other$ERR_*error (codeis non-enumerable,stackstarts withTypeError [ERR_UNKNOWN_SIGNAL]).process.killalready threw it from C++.%scode in Node v26.3.0 exceptERR_TLS_INVALID_PROTOCOL_VERSION,ERR_TLS_PROTOCOL_VERSION_CONFLICT,ERR_IP_BLOCKEDandERR_INSPECTOR_COMMAND. Bun calls those four with strings only, so their output does not change.validateHeaderName:-0,{ a: 1 },Object.create(null), a usertoString(own, inherited, bound, behind a Proxy),Symbol.toPrimitive(own and inherited),Buffer,Uint8Array,URL,Date, aDatesubclass,Map,Promise, a function, an arrow, a symbol, a bigint, a revoked Proxy, atoStringgetter that throws and a constructor namedObject. The String-or-inspect decision matches Node in every case. Where inspect runs, Bun's own inspect format differs from Node's forMap,Promise,Errorand aDatesubclass. Those are pre-existing inspect differences.%suses depth 0. That fallback is shared with every inspect site inErrorCode.cpp, so it is left for a separate change.[human-review] gate passed · iteration 0 · 10 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
root cause · written by the author bot
Several Bun error sites stringified a received numeric argument before it reached the message formatter, so a
-0lost its sign throughtoString, ani64conversion, or a plain template string and printed as0where Node'sutil.inspectsemantics print-0. The fix routes those sites through a shared Node-style%svalue formatter inErrorCode.cppthat preserves negative zero, BigInt and large-integer rendering, passes the originalJSValuefor unknown encodings instead of a pre-converted string, moves the unknown-signal error inchild_process.tsonto the$ERR_UNKNOWN_SIGNAL…