Conversation
…art with a BOM The bundler reads every input through cache::Fs::read_file_with_allocator, whose reader strips a UTF-8 BOM and transcodes UTF-16LE input to UTF-8 before the loader is consulted. For loaders that emit the file itself (file, wasm, napi, sqlite, and the pending base64/dataurl) this rewrote the asset: an asset starting with EF BB BF lost its first three bytes and a UTF-16LE file was re-encoded as UTF-8. Add a BomHandling parameter to the read path. Text reads keep converting; Loader::is_binary() loaders read the bytes exactly as they are on disk.
|
Warning Review limit reached
Next review available in: 11 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: fix pushed, waiting on CI. Reproduced on bun 1.4.0 with |
There was a problem hiding this comment.
LGTM — mechanical parameter threading with an exhaustive loader classification, and the tests pin both sides.
What was reviewed:
- Verified all five
read_file_with_allocatorcallers are updated; text-only sites (package.json,tsconfig, CSS entry) passConvert, so behavior is unchanged there. - Confirmed
read_file_with_handle_impl/read_file_contents*have no other direct callers outside the updatedcache::Fswrapper. - Checked the
Loader::is_binary()classification againstshould_copy_for_bundling— the two agree on the copying loaders, plusbase64/dataurlwhich encode raw bytes. - Tests cover all four copying loaders × both BOM prefixes × api/cli, and separately pin that
.txt/.jsinputs still get stripped/transcoded.
Extended reasoning...
Overview
Threads a BomHandling { Convert, Keep } parameter through the resolver's shared file reader (read_file_with_allocator and the read_file_contents*/read_file_with_handle_impl helpers under it) so callers can opt out of BOM stripping/UTF-16LE transcoding. Adds Loader::is_binary() (an exhaustive match) as the classification, and passes BomHandling::for_loader(loader) at the two loader-aware call sites (ParseTask and the runtime Transpiler::parse). The other three callers (package.json, tsconfig.json, CSS entry) pass Convert explicitly, preserving current behavior. Adds a bundler test covering all four copy-to-outdir loaders × two BOM prefixes × api/cli backends, plus a companion test pinning that text loaders still convert.
Security risks
None. This is a data-fidelity fix to the bundler's asset copy path; no untrusted-input parsing, auth, or crypto is touched.
Level of scrutiny
Moderate. The shared reader in src/resolver/fs.rs is used by the resolver, runtime transpiler, and bundler, so a mistake here has broad reach. However, the change is a strictly additive parameter with the four in-body BOM::detect sites replaced by bom_handling.detect, and every pre-existing call site either passes Convert (identical to the old unconditional path) or derives it from a loader that was already in scope. I grepped for all callers of read_file_with_allocator, read_file_with_handle_impl, read_file_contents, and read_file_contents_in_arena — every one is updated in this diff and there are no other direct callers.
Other factors
is_binary()is an exhaustivematch, so a futureLoadervariant will fail to compile until classified — meets the review guideline about atomically updating consumers.- The classification agrees with
should_copy_for_bundling()on the copying loaders and additionally listsBase64/Dataurl(which encode raw bytes and, per the PR description, don't consume the reader's contents in the bundler today, so listing them changes nothing yet). - The runtime-transpiler call site at
transpiler.rs:1444now usesfor_loader(loader); the PR description notes only text loaders pluswasm(whose\\0asmmagic cannot collide with a BOM) reach it, so this is behavior-preserving there. - Tests are strong: byte-exact hex comparison of eight emitted assets (fails on
USE_SYSTEM_BUN=1per the description), and a positive test that.txt/.jsstill get BOM-stripped/transcoded so the unchanged side is pinned. - esbuild parity is documented in the description.
|
Updated 6:57 AM PT - Aug 13th, 2026
❌ @robobun, your commit 8d99e6b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38103That installs a local version of the PR into your bun-38103 --bun |
Problem
bun build/Bun.buildrewrite assets whose first bytes look like a byte-order mark: an asset starting withEF BB BFis emitted without those three bytes, and one starting withFF FE(a UTF-16LE text file, e.g. a.regor an Excel "Unicode text" export imported through thefileloader) is emitted re-encoded as UTF-8. The content hash in the asset name is computed from the rewritten bytes too.file,wasm,napi,sqlite(embed: "true"). Repro on bun 1.4.0:printf '\xef\xbb\xbfhello' > a.binbundled with--loader .bin:fileproduces a 5-bytea-*.bin(68 65 6c 6c 6f);printf '\xff\xfeh\0i\0' > b.binproduces a 2-byteb-*.bin(68 69).ParseTask::get_code_for_parse_task_without_plugins(src/bundler/ParseTask.rs:1454) reads every input throughcache::Fs::read_file_with_allocator, and the readers behind it (finish_arena_contentsand the three arms ofread_file_with_handle_implinsrc/resolver/fs.rs) unconditionally runBOM::detect+ strip/transcode. That is right for text that is about to be parsed and wrong for bytes that are copied into the output:process_files_to_copy(src/bundler/bundle_v2.rs:4285) writessource.contentsas the asset, so the rewritten buffer is what lands on disk.Fix
read_file_with_allocator(and theread_file_contents*helpers under it) take a newbun_resolver::fs::BomHandling { Convert, Keep }; the four BOM sites infs.rsgo throughBomHandling::detect, which returnsNoneforKeep. The fixing line is theFs::BomHandling::for_loader(loader)argument inParseTask.rs; the previously unused_loaderparameter already carried the loader there.Loader::is_binary()(src/ast/loader.rs) is the classification:file,wasm,napi,sqlite,sqlite_embedded,base64,dataurlread verbatim; everything else keeps the current stripping/transcoding. It is an exhaustivematchso a new loader has to be classified.base64/dataurlare listed because they encode the raw bytes once implemented (bundler: implement dataurl and base64 loaders #36327, runtime + --no-bundle: implement base64/dataurl loaders #36334; the latter currently re-reads the file itself to get around this reader); in the bundler today they ignore the contents, so listing them changes nothing yet.fileloader emits all three repro inputs unchanged (8, 6 and 6 bytes with esbuild 0.18.6; output in the details block). Text reads are unchanged:package.json,tsconfig.json,bun buildCSS entry points, and the runtimeTranspiler::parsepath all passConvert(the runtime path derives it from its loader like the bundler does; only text loaders, pluswasm, whose magic\0asmcannot collide with a BOM, reach it).bun build --no-bundlecopies assets with a file copy (build_copied_file_output, see bun build --no-bundle: write output files when --outdir is set #35644) and the runtimefileloader only exports the path, so neither went through this reader. Plugin-provided contents (onLoad, in-memoryfiles) never did either.test/bundler/bundler_loader.test.ts("BOM-prefixed inputs"):loader-copy-keeps-bom-bytes-{api,cli}bundle all four copying loaders with a UTF-8-BOM and a UTF-16LE-BOM payload and compare the emitted bytes with the input; all 8 assets come out rewritten onUSE_SYSTEM_BUN=1(1.4.0) and byte-exact with this build.loader-text-converts-bompins the unchanged side:.txtand.jsinputs with either BOM still come through stripped/transcoded.bundler_loader.test.ts(53 pass),bundler_bun.test.ts(sqlite embed),bundler_files.test.ts,native-plugin.test.ts(thefetchSourceCodepath goes through the same function; 19 pass, 1 pre-existing skip), and the repro above (a-*.binis 8 bytes,b-*.binis 6).Background
EF BB BF(UTF-8) orFF FE(UTF-16LE) at the start of a text file that declare its encoding. Bun's file reader (bun_core::strings::BOM) detects these two and either drops the UTF-8 marker or transcodes the UTF-16LE body to UTF-8 so the JS/JSON/CSS/... parsers only ever see UTF-8. Nothing distinguishes a BOM from arbitrary binary data that happens to start with the same bytes; only the loader knows whether the file is text.cache::Fs::read_file_with_allocator(src/resolver/lib.rs): the one file reader shared by the resolver (package.json,tsconfig.json), the runtime transpiler, and the bundler's parse tasks. It handles fd reuse and the per-worker arena, which is why the fix is a parameter on it rather than a separate raw read in the bundler.ParseTask(src/bundler/ParseTask.rs): the per-input-file unit of work in the bundler. For copying loaders its "AST" is just a placeholder string for the asset URL; the file bytes are kept assource.contentsand later written out byprocess_files_to_copyunder a content-hashed name.Repro before/after
A third input starting with
FE FF(UTF-16BE) was already copied unchanged by bun becauseBOM::detectdoes not recognize that marker.