Repository navigation
Conversation
The text loader feeds raw file bytes into an E::String, which the
printer serializes via the Encoding::Utf8 path of
write_pre_quoted_string_inner. That path used the WTF-8 stepper
(wtf8_byte_sequence_length_with_invalid + decode_wtf8_rune_t with
zero=0), which for ill-formed input:
* widened an invalid lead or lone continuation byte to its Latin-1
code point (0xFF -> \xFF),
* returned 0 for a bad multibyte sequence and still advanced by the
lead-byte-implied width, dropping the following byte(s), and
* let WTF-8-encoded surrogates through verbatim.
So importing a .txt file disagreed with Bun.file().text(),
fs.readFileSync(p, 'utf8'), and TextDecoder on the same bytes.
Switch the Utf8 arm to strings::convert_utf8_bytes_into_utf16, the
same WHATWG decoder Bun.file().text() already uses, which yields
U+FFFD with maximal-subpart advancement. On replacement.fail emit
\uFFFD (ascii-only output) or the raw EF BF BD bytes and continue,
so neither the raw-byte fast path nor the \xHH branch ever sees the
invalid input. Valid sequences keep the existing behaviour.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesThe UTF-8 decoder is made publicly accessible, quoted-string printing adopts WHATWG-style malformed UTF-8 replacement handling, and text-loader tests compare results with Invalid UTF-8 decoding
🚥 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun_core/string/immutable/unicode.rs (1)
413-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument preconditions now that this crosses a crate boundary.
convert_utf8_bytes_into_utf16now becomes part ofjs_printer's public contract viastrings::convert_utf8_bytes_into_utf16, but its preconditions (non-empty slice, first byte non-ASCII) are only enforced by an unconditionalunreachable!()and a debug-onlydebug_assert!. Any future caller that violates them will panic in release builds too. A short doc comment stating the precondition would make the widened surface safer to consume.📝 Suggested doc comment
+/// Decodes one UTF-8 sequence starting at `bytes[0]` into UTF-16. +/// Preconditions: `bytes` is non-empty and `bytes[0] >= 0x80` (non-ASCII lead byte). +/// Violating either precondition panics (`unreachable!()`/`debug_assert!`). pub fn convert_utf8_bytes_into_utf16(bytes: &[u8]) -> UTF16Replacement {🤖 Prompt for 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. In `@src/bun_core/string/immutable/unicode.rs` around lines 413 - 424, Add a concise doc comment to convert_utf8_bytes_into_utf16 documenting that bytes must be non-empty and begin with a non-ASCII UTF-8 byte, since these are required preconditions for callers across the crate boundary.Source: Coding guidelines
🤖 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 `@test/js/bun/util/text-loader-invalid-utf8.test.ts`:
- Around line 4-7: Update the invalid-UTF-8 test in entry.ts to invoke
Bun.file(f).text() and assert its decoded result matches the imported text,
alongside the existing fs.readFileSync comparison. Keep the header comment’s
stated parity coverage accurate.
- Around line 47-50: Update the test assertion around JSON.parse(stdout) to
check exitCode and stderr first, or guard JSON parsing so subprocess crashes
surface the captured stderr diagnostics. Preserve the existing parsed JSON
assertions for successful execution and continue requiring an empty stderr and
exit code 0.
- Around line 1-53: Move the UTF-8 decoding cases and assertions from the
standalone suite into the existing text-loader suite in text-loader.test.ts,
reusing its setup and helpers where applicable. Keep a separate file only if one
case is required as a tracked regression; otherwise remove the standalone test
file while preserving coverage for all listed invalid and valid sequences.
---
Outside diff comments:
In `@src/bun_core/string/immutable/unicode.rs`:
- Around line 413-424: Add a concise doc comment to
convert_utf8_bytes_into_utf16 documenting that bytes must be non-empty and begin
with a non-ASCII UTF-8 byte, since these are required preconditions for callers
across the crate boundary.
🪄 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: f1b1f6a8-d77c-4495-89c5-73ee4a7a2615
📒 Files selected for processing (4)
src/bun_core/string/immutable.rssrc/bun_core/string/immutable/unicode.rssrc/js_printer/lib.rstest/js/bun/util/text-loader-invalid-utf8.test.ts
…ile().text() parity and assert stderr before JSON.parse
|
Updated 2:51 PM PT - Jul 23rd, 2026
❌ @robobun, your commit ee9d06e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35306That installs a local version of the PR into your bun-35306 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…pre_quoted_string The bun_core copy (used by format_escapes, quote_for_json, snapshot keys, sourcemap JSON) is documented as 'Same algorithm as bun_js_printer::write_pre_quoted_string'; keep that true by giving its Utf8 arm the same convert_utf8_bytes_into_utf16 replacement decode.
…UTF-8 The Encoding::Utf8 path now does a strict WHATWG decode, so the WTF-8-shaped is_valid_wtf8 guard (which accepted ED A0 80..ED BF BF) documented an invariant the decoder no longer honours. Replace it with strings::is_valid_utf8 and note that raw text-loader bytes reach the printer via print_string_characters_e_string without this assert.
…r comment Add itBundled cases for target=bun (ascii-only output, \uFFFD arm) and target=browser (raw EF BF BD arm) so both replacement.fail branches and the bun build entry point for #12981 have fail-before coverage. Drop the bug-history sentence from the runtime test header and replace it with the issue URL.
|
CI on build 78861 is green for this diff. The new tests ( The one red lane is Ready for review/merge. |
There was a problem hiding this comment.
All five earlier inline comments have been addressed and no new issues were found. Deferring to a human for final sign-off since this reworks the Encoding::Utf8 arm of write_pre_quoted_string_inner (the printer hot path shared by the runtime module loader and bun build) and its bun_core::printer sibling, and tightens the print_string_literal_utf8 debug_assert from WTF-8 to strict UTF-8.
What was reviewed:
- Traced
convert_utf8_bytes_into_utf16through valid/invalid/overlong/surrogate/truncated inputs —replacement.lennever exceeds remaining bytes, soclamped_widthand the raw-byte fast path at lib.rs:824 stay in-bounds. - Confirmed the
bun_core::printer::write_pre_quoted_stringcopy now matches (used byquote_for_json, sourcemap chunk, snapshot keys,format_escapes). - Checked test coverage: 10-case parity test against
TextDecoder/readFileSync/Bun.file().text(), plusitBundledfor bothtarget: bun(ascii_only) andtarget: browser(rawEF BF BD) arms.
Extended reasoning...
Overview
Switches the Encoding::Utf8 arm of both bun_js_printer::write_pre_quoted_string_inner and bun_core::printer::write_pre_quoted_string from the WTF-8 stepper (wtf8_byte_sequence_length_with_invalid + decode_wtf8_rune_t) to strings::convert_utf8_bytes_into_utf16 (the WHATWG maximal-subpart decoder already used by to_utf16_alloc / Bun.file().text()). Ill-formed sequences now emit U+FFFD (as \uFFFD in ascii-only output or raw EF BF BD otherwise) instead of Latin-1 widening / NUL-with-over-advance / passing WTF-8 surrogates through. Also promotes convert_utf8_bytes_into_utf16 from pub(super) to pub, replaces the hand-rolled is_valid_wtf8 guard on print_string_literal_utf8 with strings::is_valid_utf8, and adds runtime + bundler tests.
Security risks
None identified. The change moves from a permissive decoder (which could emit raw invalid bytes into bundled JS, causing browser SyntaxError) to a strict WHATWG replacement decoder. No new untrusted-input parsing; the unreachable!() on empty bytes in convert_utf8_bytes_into_utf16 is guarded by the first < 0x80 check at both call sites (i < n ⇒ text[i..] non-empty).
Level of scrutiny
Medium-high. write_pre_quoted_string_inner runs for every string literal the printer emits (runtime module loading + bun build), and the bun_core sibling backs sourcemap JSON, snapshot keys, and format_escapes. The tightened debug_assert!(strings::is_valid_utf8(str)) on print_string_literal_utf8 is a new invariant that could fire on a caller not audited here — the author traced ~18 call sites and ran the transpiler/bundler_string/bundler_loader suites under debug, but this is the kind of change a maintainer should confirm.
Other factors
Every prior review round was addressed: the sibling copy was fixed (a571bfb), the debug_assert tightened (8140ce3), bug-history comments trimmed (5853f83, ee9d06e), and itBundled coverage added for both ascii_only arms (ee9d06e). Fail-before/pass-after was demonstrated under debug+ASAN. I verified replacement.len from convert_utf8_bytes_into_utf16_with_length is always ≤ the actual remaining bytes for non-fail results (padded zeros fail the continuation check first), so the downstream &text[i..i + clamped_width] slice at lib.rs:824 stays in-bounds. The separate-test-file placement was justified (existing text-loader.test.ts has an unrelated timeout under debug+ASAN).
|
#38253 fixes the same text-loader symptom at the loader instead of in the printer: the text and md loaders validate the file with simdutf and, only when it is ill-formed, re-encode it through the same decoder before building the The reason for the different layer: the 8-bit |
|
A correction to my comment above, with more data. The printer sink has more producers than the text and md loaders, so a printer-level repair is worth having next to #38253 rather than instead of it:
For those doors the printer is the one shared point, but the repair has to stay WTF-8 aware: replace invalid lead bytes, stray continuation bytes, overlong and truncated sequences and anything above U+10FFFF with one U+FFFD per maximal subpart, and keep accepting |
2 participants
The
textloader (import t from "./f.txt"/with { type: "text" }) decodes invalid UTF-8 as Latin-1, while every other reader in Bun returns U+FFFD for the same bytes.Repro
Cause
The text loader wraps the raw file bytes in
E::String, which the printer serializes via theEncoding::Utf8path ofwrite_pre_quoted_string_inner. That path used the WTF-8 stepper (wtf8_byte_sequence_length_with_invalid+decode_wtf8_rune_twithzero = 0). For ill-formed input it0xFF->\xFF),0for a bad multibyte sequence and still advanced by the lead-byte-implied width, so the following byte(s) were dropped (a\xC2 b->[97, 0, 98], the space is gone), andED A0 80->\uD800).Fix
Switch the
Encoding::Utf8arm tostrings::convert_utf8_bytes_into_utf16, the same WHATWG decoderBun.file().text()/to_utf16_allocalready use, which yields U+FFFD with the WebKit maximal-subpart advancement. Onreplacement.failthe loop emits\uFFFD(ascii-only output) or the rawEF BF BDbytes directly and continues, so neither the raw-byte fast path nor the\xHHbranch ever sees the invalid input. Valid sequences are unchanged (regression-guarded in the new test).Both the runtime loader and
bun buildshare this printer path, so a bundled artifact now agrees too.Verification
7 of the 10 cases fail on
mainand all 10 pass with this change; the 3 valid-UTF-8 cases guard against regressions.In the bundler's non-ascii-only output the old path wrote the raw invalid byte into the JS source (
"a<0xFF><0xFE>b"), which browsers reject as an invalid token; the output is now well-formed UTF-8.Fixes #12981
[review] gate passed · iteration 1 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 1
evidence per changed file