node:string_decoder: make end(buf) accept what write(buf) accepts

[robobun]

Problem

• new StringDecoder().end(undefined), .end("abc") and .end(123) all throw TypeError: Expected Uint8Array (no .code). Node returns "", "abc", and throws TypeError [ERR_INVALID_ARG_TYPE]: The "buf" argument must be ... Received type number (123) respectively.
• With a partial character buffered, Node's end("abc") returns "abc\ufffd"; Bun throws, and the partial stays buffered.
• Cause: jsStringDecoderPrototypeFunction_endBody (src/jsc/bindings/JSStringDecoder.cpp:430) has its own argument check that only accepts an ArrayBufferView when an argument is present, while writeBody right above it already implements Node's string pass-through and ERR_INVALID_ARG_TYPE. In Node, end(buf) is (buf === undefined ? "" : this.write(buf)) plus the flush (https://github.com/nodejs/node/blob/v26.3.0/lib/string_decoder.js#L96-L101), so it accepts exactly what write() accepts. @types/node declares end(buffer?: string | ArrayBufferView) accordingly, so this compiles and then throws at runtime on Bun.

Fix

• write() and end() resolve buf through one helper (bufArgument), which throws the ERR_INVALID_ARG_TYPE write() already threw; end() no longer has its own error.
• end() now mirrors Node's three outcomes: undefined (explicit or omitted) only flushes; a string is returned followed by whatever the flush produces; any other non-view throws before the decoder state is touched, so the buffered partial is still returned by the next end() (also Node's behavior, since write() throws before the flush runs).
• write() reads its argument with callFrame->argument(0), so write() with no argument now throws Node's ERR_INVALID_ARG_TYPE (Received undefined) instead of ERR_MISSING_ARGS.
• Why this is correct: matches Node. Every expected value in the new tests was checked against Node v26.3.0 with the same scenarios written in node:assert (output below); the only remaining difference is the must be of type vs must be an instance of wording of the shared message, which test/js/node/test/parallel/test-string-decoder.js currently pins to Bun's wording and which this PR deliberately leaves alone so write() and end() keep throwing the identical message.
• Intentionally unchanged: detached views are still rejected by both methods (Node decodes them as empty), and the legacy text(buf, offset) keeps its own check, because Node's text() is write(buf.slice(offset)) and fails with a plain TypeError from .slice, not with write()'s error.
• Tests: test/js/node/string_decoder/string-decoder.test.js, new describe("end(buf) handles its argument like write(buf)"): 30 of the 31 new cases fail on the released Bun (USE_SYSTEM_BUN=1), all 126 in the file pass with this build. The one case that passes both ways (end(view) decodes and flushes) guards the path the refactor kept.
• Also run on this build: test/js/node/test/parallel/test-string-decoder.js, test-string-decoder-end.js, test-string-decoder-fuzz.js (exit 0), and the new cases under BUN_JSC_validateExceptionChecks=1.

Background

• StringDecoder decodes a byte stream that may split multi-byte characters across chunks: write(chunk) returns the complete characters and keeps the trailing incomplete bytes; end() flushes those (as U+FFFD for utf8, a lone surrogate for utf16le, padding for base64) and resets the decoder. Bun implements it natively in JSStringDecoder.cpp; JSStringDecoder::end(ptr, len) decodes len bytes and then flushes, so end(nullptr, 0) is a pure flush.
• ERR_INVALID_ARG_TYPE is Node's error for a wrongly typed argument; Bun::ERR::INVALID_ARG_TYPE (src/jsc/bindings/ErrorCode.cpp) builds the same The "name" argument must be ... Received ... message with Node's rendering of the received value and sets .code. Callers pass the ThrowScope in and check RETURN_IF_EXCEPTION afterwards, which is the pattern the new helper follows.

Node v26.3.0 vs Bun 1.4.0 for the reported calls

$ node repro.js
end()                -> ""
end(undefined)       -> ""
end('abc')           -> "abc"
partial + end('abc') -> "abc\ufffd"
end(123)             -> TypeError ERR_INVALID_ARG_TYPE: The "buf" argument must be an instance of Buffer, TypedArray, or DataView. Received type number (123)
write()              -> TypeError ERR_INVALID_ARG_TYPE: The "buf" argument must be an instance of Buffer, TypedArray, or DataView. Received undefined

$ USE_SYSTEM_BUN=1 bun repro.js
end()                -> ""
end(undefined)       -> TypeError: Expected Uint8Array   (code undefined)
end('abc')           -> TypeError: Expected Uint8Array   (code undefined)
partial + end('abc') -> TypeError: Expected Uint8Array   (code undefined)
end(123)             -> TypeError: Expected Uint8Array   (code undefined)
write()              -> TypeError ERR_MISSING_ARGS: Not enough arguments

$ bun bd repro.js   (this branch)
end()                -> ""
end(undefined)       -> ""
end('abc')           -> "abc"
partial + end('abc') -> "abc\ufffd"
end(123)             -> TypeError ERR_INVALID_ARG_TYPE: The "buf" argument must be of type Buffer, TypedArray, or DataView. Received type number (123)
write()              -> TypeError ERR_INVALID_ARG_TYPE: The "buf" argument must be of type Buffer, TypedArray, or DataView. Received undefined

The new test's scenarios rewritten with node:assert pass under Node v26.3.0 and under this build (all scenarios agree); the released Bun fails at the first end(undefined).

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 31 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ac5be594-e37a-477f-b159-c48adc93069d

📥 Commits

Reviewing files that changed from the base of the PR and between 9cff2a1 and dc360a9.

📒 Files selected for processing (2)

• src/jsc/bindings/JSStringDecoder.cpp
• test/js/node/string_decoder/string-decoder.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: fix pushed, waiting on CI.

Reproduced on Bun 1.4.0 and main with new StringDecoder().end(undefined) / .end("abc") / .end(123) (all throw TypeError: Expected Uint8Array); Node v26.3.0 returns "" / "abc" / throws ERR_INVALID_ARG_TYPE.

Tests: the new end(buf) handles its argument like write(buf) block in test/js/node/string_decoder/string-decoder.test.js (30 of 31 cases fail on the released Bun, all pass with this branch). The same scenarios written with node:assert pass under Node v26.3.0.

[claude]

LGTM — focused Node-compat fix with thorough coverage.

What was reviewed:

• endBody/writeBody control flow: undefined → flush-only, string → flush then str + flushed via jsString rope (matches Node's write(buf) + flush order); non-view throws before ResetScope fires so buffered state is preserved.
• Exception discipline: RETURN_IF_EXCEPTION after bufArgument and after the flush; RELEASE_AND_RETURN on the throwing jsString tail; flushed->length() is non-throwing so the early return is clean under validateExceptionChecks.
• write() no-arg now routes through argument(0) → ERR_INVALID_ARG_TYPE (was ERR_MISSING_ARGS); grepped — no existing test asserts the old error, and test/parallel/test-string-decoder.js already pins the shared message wording.
• Tests cover all encodings, 10 rejected value types, state-after-throw, and a DataView guard for the unchanged path.

Extended reasoning...

Overview

The PR aligns StringDecoder.prototype.end(buf) argument handling with Node.js: end() now accepts exactly what write() accepts (undefined → flush only, string → passed through then flush appended, ArrayBufferView → decode+flush, anything else → ERR_INVALID_ARG_TYPE). It touches two functions in src/jsc/bindings/JSStringDecoder.cpp (writeBody, endBody) and extracts their shared view-or-throw check into bufArgument. writeBody also switches from argumentCount()<1 + uncheckedArgument to argument(0), so write() with no argument now throws Node's ERR_INVALID_ARG_TYPE instead of ERR_MISSING_ARGS. 31 new test cases in the existing string-decoder.test.js file cover the matrix.

Security risks

None. This is argument-type validation and string concatenation for a text decoder. No untrusted lengths drive allocation; the string path calls end(nullptr, 0) (pure flush of ≤4 buffered bytes) then rope-concatenates two existing JSStrings. No new pointer arithmetic on user data.

Level of scrutiny

Moderate — native C++ in the JSC bindings layer, so exception-scope discipline and null-safety matter, but the change is small (~35 net lines), localized to two prototype bodies, and reuses idioms already present in the same file (jsString(globalObject, a, b) for concatenation, RETURN_IF_EXCEPTION after end(), RELEASE_AND_RETURN on throwing tails). I traced each path:

• bufArgument throws via the caller's ThrowScope, callers check RETURN_IF_EXCEPTION immediately — matches the ErrorCode pattern used elsewhere.
• String path in endBody: end(nullptr, 0) returns non-null on success (all branches in JSStringDecoder::end return jsEmptyString or an encodingToString result); flushed->length() is a plain field read (non-throwing), so the early return encode(buffer) is exception-check-clean; asString(buffer) is safe because buffer.isString() was checked.
• Error-before-state-mutation: for a rejected value, bufArgument throws before castedThis->end runs, so ResetScope never fires and the buffered partial survives — matches Node (where write() throws before the flush) and is directly tested.
• The utf16le single-byte partial (flushed as "") exercises the length()==0 fast path returning the string unchanged.

Other factors

• The PR description documents cross-verification against Node v26.3.0 and a run under BUN_JSC_validateExceptionChecks=1, and confirms 30/31 new cases fail on released Bun.
• I grepped for existing assertions on the old write() no-arg error (ERR_MISSING_ARGS / "Not enough arguments") in string_decoder tests — none, so no test regression from that change. test/js/node/test/parallel/test-string-decoder.js:199 already pins the must be of type wording this PR keeps for both methods.
• Tests are placed in the existing test file per repo convention, use it.each for the matrix, assert error class/code/message, and include a positive guard (end(view)) for the path the refactor left in place.