Repository navigation
Conversation
…INVALID message text
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 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 (3)
Comment |
|
Status: Fix verified locally. Reproduced with: Both assertions fail before (messages |
|
Updated 1:15 PM PT - Jul 25th, 2026
❌ @autofix-ci[bot], your commit 35de9ec has some failures in 🧪 To try this PR locally: bunx bun-pr 35777That installs a local version of the PR into your bun-35777 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| case Bun::ErrorCode::ERR_HTTP_CONTENT_LENGTH_MISMATCH: { | ||
| auto arg0 = callFrame->argument(1); | ||
| auto str0 = arg0.toWTFString(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| auto arg1 = callFrame->argument(2); | ||
| auto str1 = arg1.toWTFString(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| auto message = makeString("Response body's content-length of "_s, str0, " byte(s) does not match the content-length of "_s, str1, " byte(s) set in header"_s); | ||
| return JSC::JSValue::encode(createError(globalObject, ErrorCode::ERR_HTTP_CONTENT_LENGTH_MISMATCH, message)); | ||
| } |
There was a problem hiding this comment.
🟡 Heads-up: the server-side ServerResponse path still throws ERR_HTTP_CONTENT_LENGTH_MISMATCH with the old "Content-Length mismatch" message — _http_server.ts calls handle.write/end(..., strictContentLength(this)), which lands in src/runtime/server/NodeHTTPResponse.rs:1992-2003 and throws via err_throw(global, code, "Content-Length mismatch"), bypassing jsFunctionMakeErrorWithCode entirely. Same-class sibling of this fix per REVIEW.md; worth either updating the Rust literal to the templated Node text (it has both byte counts in scope) or noting the exclusion in the PR description. Not blocking — nothing regresses.
Extended reasoning...
What's left unfixed
This PR correctly adds an ERR_HTTP_CONTENT_LENGTH_MISMATCH case to jsFunctionMakeErrorWithCode, so any JS-side $ERR_HTTP_CONTENT_LENGTH_MISMATCH(actual, expected) call now produces Node's templated message. That covers the client (_http_outgoing.ts) path.
However, the server ServerResponse write/end path does not go through $ERR_HTTP_CONTENT_LENGTH_MISMATCH(...). In src/js/node/_http_server.ts (lines 3186/3217/3363/3366/3544/3547), ServerResponse#write / #end call handle.write(chunk, encoding, cb, strictContentLength(this)) / handle.end(...) — passing the strict-content-length flag into native code. The native handle is NodeHTTPResponse (src/runtime/server/NodeHTTPResponse.rs), whose write_or_end throws the mismatch error directly:
// NodeHTTPResponse.rs:1991-2003
if bytes_written as u64 != content_length {
return err_throw(
global_object,
ErrorCode::ERR_HTTP_CONTENT_LENGTH_MISMATCH,
"Content-Length mismatch",
);
}And err_throw → err_throw_cold (lines 281-282) does global.err(code, format_args!("{}", msg)).throw() — it constructs the JS error from the literal string right there. It never routes through jsFunctionMakeErrorWithCode, so the new template case in ErrorCode.cpp is never reached for server responses.
Step-by-step repro
http.createServer((req, res) => { res.strictContentLength = true; res.setHeader('Content-Length', 5); res.writeHead(200); try { res.end('abc'); } catch (e) { console.log(e.code, JSON.stringify(e.message)); } }).listen(0)- Send any request.
res.end('abc')→_http_server.ts:3217→handle.end('abc', ..., true /* strictContentLength */). - In
NodeHTTPResponse.rs::write_or_end,is_end == true,bytes_written = 3,content_length = 5, so thebytes_written != content_lengthbranch at line 1991 fires. err_throw(global, ERR_HTTP_CONTENT_LENGTH_MISMATCH, "Content-Length mismatch")is returned; the JS error's.messageis the literal"Content-Length mismatch".- Output:
ERR_HTTP_CONTENT_LENGTH_MISMATCH "Content-Length mismatch"— Node.js prints"Response body's content-length of 3 byte(s) does not match the content-length of 5 byte(s) set in header".
The existing test test/js/bun/test/parallel/test-http-strictContentLength-should-work-on-server.ts only asserts .code, not .message, so it doesn't catch this.
Why this is in scope
REVIEW.md, Correctness: the bug class, not the bug: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern... If a site is intentionally excluded, say so in the PR." This is the same error code with the same wrong-message-text problem, just on the Rust-native server path instead of the JS client path. The PR title is "fix ERR_HTTP_CONTENT_LENGTH_MISMATCH ... message text", so the server-side sibling is directly in the stated scope.
Impact & fix
Message-text-only Node compat gap; .code is already correct and nothing regresses if merged as-is — hence nit. To fix, replace the literal in NodeHTTPResponse.rs:1992-2003 with the templated form (both bytes_written and content_length are in scope), e.g. via global.err(code, format_args!("Response body's content-length of {} byte(s) does not match the content-length of {} byte(s) set in header", bytes_written, content_length)).throw(), or route the throw back through JS via $ERR_HTTP_CONTENT_LENGTH_MISMATCH(actual, expected). If intentionally deferred, a note in the PR description would satisfy REVIEW.md.
There was a problem hiding this comment.
LGTM — straightforward error-message template fix following the existing jsFunctionMakeErrorWithCode case pattern. The prior inline note about the Rust-native NodeHTTPResponse server path is a pre-existing gap and non-blocking.
Checked: all JS-side $ERR_HTTP_CONTENT_LENGTH_MISMATCH/$ERR_HTTP_TRAILER_INVALID call sites match the new arg shapes; RETURN_IF_EXCEPTION after each toWTFString; message text matches Node's lib/internal/errors.js and the existing test-http-server-de-chunked-trailer.js assertion; no other tests assert the old broken text.
Extended reasoning...
Overview
Adds two switch cases to jsFunctionMakeErrorWithCode in src/jsc/bindings/ErrorCode.cpp: a two-arg templated case for ERR_HTTP_CONTENT_LENGTH_MISMATCH and a constant-message case for ERR_HTTP_TRAILER_INVALID. Drops the now-redundant inline string args from two $ERR_HTTP_TRAILER_INVALID(...) calls in src/js/node/_http_server.ts. Adds test/js/node/http/node-http-outgoing-errors.test.ts with two tests exercising the client-side throw paths and asserting exact .code and .message.
Security risks
None. Pure error-message text change; no input parsing, no auth, no allocation sizing based on untrusted data.
Level of scrutiny
Low. The C++ change is byte-for-byte the same shape as the neighboring ERR_HTTP_INVALID_HEADER_VALUE case (two toWTFString + RETURN_IF_EXCEPTION + makeString), and the constant-message case matches the neighboring ERR_HTTP_BODY_NOT_ALLOWED/ERR_HTTP_SOCKET_ASSIGNED block. All five $ERR_* call sites in src/js/ (_http_outgoing.ts:563,934,1079 and _http_server.ts:2117,2147) were checked against the new signatures — the two-arg mismatch calls pass (actual, expected) in the order the template expects, and all trailer-invalid calls are now zero-arg. Grepped test/ for assertions on the old text — none exist; the one existing message assertion (test-http-server-de-chunked-trailer.js:14) already expects the Node-correct string, so this fix makes it pass rather than break.
Other factors
The test uses once(srv, 'listening'), port: 0, no network egress, and cleans up in finally. robobun confirmed USE_SYSTEM_BUN=1 fails / bun bd test passes. A prior review pass left a non-blocking nit about the Rust-native NodeHTTPResponse.rs server path still using a "Content-Length mismatch" literal — that path pre-dates this PR, is not touched here, and does not go through jsFunctionMakeErrorWithCode, so nothing in this diff regresses it. The author can address it in a follow-up or note the exclusion; it does not block this strictly-improving change.
|
Closing in favor of #35791, which covers these two codes plus |
### 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>
2 participants
What
Two
node:httpclient error codes had broken message templates. The codes and throw timing were already node-identical; only the.messagetext was wrong.ERR_HTTP_CONTENT_LENGTH_MISMATCH"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_TRAILER_INVALID"undefined""Trailers are invalid with this transfer encoding"Repro
Cause
jsFunctionMakeErrorWithCodeinsrc/jsc/bindings/ErrorCode.cpphad no case for either code, so they fell through to the default which stringifiescallFrame->argument(1).$ERR_HTTP_CONTENT_LENGTH_MISMATCH(3, 5)became"3";$ERR_HTTP_TRAILER_INVALID()becameString(undefined).Fix
Added a two-arg template case for
ERR_HTTP_CONTENT_LENGTH_MISMATCHand a constant-message case forERR_HTTP_TRAILER_INVALID, matching Node'slib/internal/errors.js. Also dropped the now-redundant inline string from the two server-side$ERR_HTTP_TRAILER_INVALID(...)calls in_http_server.ts.Verification
Also passes
node-http-transfer-encoding.test.ts(23 tests) and the Node parallel teststest-http-content-length-mismatch.js/test-http-server-de-chunked-trailer.js.