Decode module sources with the // @bun pragma as UTF-8 - #37162
Conversation
Files carrying the // @Bun pragma skip the transpiler and hand their raw bytes to JSC as the module source. Both the sync and async load paths built that string with clone_latin1, which treats each byte as one code point, so every non-ASCII UTF-8 sequence turned into mojibake at runtime. Use clone_utf8 instead, which keeps the Latin-1 fast path for all-ASCII sources. A bytecode sidecar generated from ASCII output is unaffected; if the source has non-ASCII bytes the SourceCodeKey hash no longer matches and JSC silently falls back to parsing.
WalkthroughThe runtime transpiler now preserves UTF-8 source contents for already-bundled modules. Main VM entry points are marked as loaded on bundled and transpiler-cache-hit paths. Tests cover Unicode loading and unknown-extension imports. ChangesRuntime UTF-8 decoding and cache loading
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
The automated review was rate limited and can run once the limit resets. The diff is ready for review in the meantime. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted encoding fix with matching regression tests.
Checked that String::clone_utf8 keeps the Latin-1 fast path for all-ASCII input (BunString__fromBytes auto-detects), so no perf change for typical bundler output. Both changed sites are the parallel sync/async already_bundled branches, and neighboring raw-source paths in jsc_hooks.rs already use clone_utf8. The ensure_hash() call after the async-path change still runs; the PR description's bytecode-sidecar analysis (ASCII-only under --target=bun, silent reparse otherwise) checks out. Tests cover import, require, and entry-point, and mix Latin-1-range and BMP code points.
Extended reasoning...
Overview
Two one-token changes swapping String::clone_latin1 for String::clone_utf8 in the // @bun pragma fast path — once in the async worker-thread transpiler store (src/jsc/RuntimeTranspilerStore.rs) and once in the sync module-loader path (src/runtime/jsc_hooks.rs). Three regression tests added to the existing // @bun describe block in test/bundler/transpiler/runtime-transpiler.test.ts.
Security risks
None. This is a source-decoding fix; no user-controlled sizing, no new allocations, no FFI surface change. clone_utf8 and clone_latin1 have identical ownership semantics (fresh WTF::StringImpl, refcount 1, owned by OwnedResolvedSource), so no lifetime or refcount balance changes.
Level of scrutiny
Low-to-medium. The diff is two identifiers plus tests. I verified clone_utf8 in src/bun_core/string/mod.rs — it calls BunString__fromBytes, which auto-detects all-ASCII and takes the Latin-1 fast path, so the common case (bundler output with escaped non-ASCII) is byte-identical to before, and ensure_hash() on the async path produces the same hash. The neighboring disable_transpilying, JSON, and bun:main branches in jsc_hooks.rs already use clone_utf8 for raw file bytes, so this brings the pragma path in line with established convention.
Other factors
Tests follow repo conventions: tempDir with using, bunEnv/bunExe, concurrent pipe draining, stderr/stdout asserted before exit code, placed in the existing test file next to related coverage, issue link in a comment. The test string deliberately mixes Latin-1-range code points (U+00B7, U+00E4) with BMP-only ones (U+2014, U+2713), which distinguishes the two decoders. The PR description explicitly covers the bytecode-sidecar hash-mismatch edge case and both sibling code paths were fixed together.
The transpiler-cache-hit path in transpile_source_code_inner returns early and skipped the is_main -> has_loaded = true write that the full parse path performs. With has_loaded still false, a later import of a file with an unknown extension fell into the potentially-the-main-module fallback and was parsed as tsx instead of using the file loader. So running an entry point larger than the 4 KiB cache minimum that imports an asset like ./template.hbs succeeded on the first run and failed with a JSX parse error on every run after. Debug builds discard restored cache entries, which is why this only reproduced in release builds (or with BUN_DEBUG_ENABLE_RESTORE_FROM_TRANSPILER_CACHE=1, as the test uses).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cli/run/transpiler-cache.test.ts`:
- Around line 92-93: Extend the second-run assertion in the transpiler cache
test after the bunRun call to verify the cache state using the existing
cache-state assertion used by the test harness. Keep the successful "string"
output check, and assert that the cache still contains the expected single entry
so the test proves restoration rather than only execution success.
🪄 Autofix
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: 48303540-47c8-47ba-b313-e138014e0dd3
📒 Files selected for processing (2)
src/runtime/jsc_hooks.rstest/cli/run/transpiler-cache.test.ts
The already-bundled early return has the same problem as the transpiler-cache-hit return one branch below: a // @Bun entry point returned without setting has_loaded, so importing a file with an unknown extension from it was parsed as tsx via the potentially-the-main-module fallback instead of using the file loader.
There was a problem hiding this comment.
LGTM — the follow-up commit addressed the sibling already_bundled early return I flagged, and the bug hunt found nothing further.
What was reviewed:
clone_latin1→clone_utf8at both already-bundled sites;BunString__fromByteskeeps the ASCII → Latin-1 fast path, so no regression for pure-ASCII sources.- Both new
has_loaded = trueguards match the existing full-print-path pattern at jsc_hooks.rs:3114; checked the remaining early returns (empty .cjs/.cts, ExportsObject) — neither can carry unknown-extension imports. - Bytecode sidecar hash concern:
--bytecodeforces--target=bun(ASCII-escaping printer), and a mismatch falls back to parsing rather than crashing. - New tests follow harness conventions (tempDir, concurrent pipe drain, stderr/stdout before exitCode) and the cache test asserts
newCacheCount() == 0on the hit run.
Extended reasoning...
Overview
Two one-token encoding fixes (String::clone_latin1 → String::clone_utf8) on the // @bun already-bundled fast path in src/jsc/RuntimeTranspilerStore.rs (async worker path) and src/runtime/jsc_hooks.rs (sync path), plus two copies of the existing if is_main { has_loaded = true } guard placed before the already-bundled and transpiler-cache-hit early returns in transpile_source_code_inner. Five new tests across test/bundler/transpiler/runtime-transpiler.test.ts and test/cli/run/transpiler-cache.test.ts.
Security risks
None. This is a source-decoding correctness change; clone_utf8 calls BunString__fromBytes, which auto-detects all-ASCII → Latin-1 and otherwise does a real UTF-8 decode via WTF, so behavior for ASCII sources is byte-identical and non-ASCII sources now decode correctly instead of being mojibake'd. No user-controlled input reaches a new sink.
Level of scrutiny
Medium — module loader hot path, but the changes are mechanical: a helper swap to the one already used everywhere else for source bytes (the cache-hit branch at line 2884 already used clone_utf8), and a guard that copies the existing pattern at line 3114 verbatim. I audited the other early returns in transpile_source_code_inner between the already-bundled branch and the full-print path: the empty-.cjs/.cts return (line 2813) has no imports, and the ExportsObject return (line 2760) is for non-JS loaders — neither can trigger the Loader::Tsx fallback, so the bug class is now covered.
Other factors
My previous review flagged that the already_bundled return itself was missing the same has_loaded guard added to the cache-hit branch; commit 89b7b73 addressed that with a test. CodeRabbit's request to assert newCacheCount() == 0 after the cache-hit run was addressed in 1cf6eba. All inline threads are resolved. The PR body's evidence gate shows all five new tests fail on main and pass on the PR under both ASAN-debug and release. Tests follow repo harness conventions (tempDir, bunEnv, concurrent stdout/stderr/exited drain, output asserted before exit code, Buffer.alloc(n, fill) for padding).
There was a problem hiding this comment.
LGTM — the follow-up commit applied the has_loaded guard to the already_bundled return with a matching test, so both early returns now mirror the full-print path.
What was reviewed:
clone_latin1→clone_utf8at both pragma sites;BunString__fromByteskeeps the Latin-1 fast path for all-ASCII input, so no regression there.- The two new
is_main → has_loaded = truewrites match the existing guard at jsc_hooks.rs:3114 and the loader-fallback read at :4464. - Bytecode-sidecar hash concern:
--target=bunescapes to ASCII, so the SourceCodeKey is unchanged for real bundler output (per description; falls back to parsing otherwise). - Tests cover import/require/entry-point for UTF-8 and both
has_loadedearly returns; CodeRabbit's cache-count assertion was added.
Extended reasoning...
Overview
Four files changed. The core fix is two String::clone_latin1 → String::clone_utf8 swaps on the // @bun pragma fast path — one in the sync loader (src/runtime/jsc_hooks.rs:2798) and one in the async transpiler store (src/jsc/RuntimeTranspilerStore.rs:997). clone_utf8 calls BunString__fromBytes, which auto-detects all-ASCII and stays Latin-1, so the fast path is preserved and only non-ASCII sources take the UTF-8 decode. Two additional if is_main { (*jsc_vm).has_loaded = true } writes were added before the already-bundled and transpiler-cache-hit early returns, copying the identical pattern from the full-print path at line 3114. Five tests were added across two existing test files.
Security risks
None. No user-controlled paths, no auth/crypto/permissions. The encoding change decodes source bytes JSC will parse anyway; a malformed UTF-8 sequence would produce replacement characters, not a memory-safety issue (BunString__fromBytes is the same helper the non-pragma path already uses on the same bytes).
Level of scrutiny
Moderate — module loading is a hot, correctness-critical path, but the changes are mechanical: two encoding-helper swaps to an existing helper with the right semantics, and two field writes copying an existing in-scope pattern byte-for-byte. is_main is computed at line 2171 and jsc_vm is already dereferenced under the same SAFETY contract throughout the function. I checked the two other early returns between 2760 and 3114 (ExportsObject at 2760, empty .cjs/.cts at 2810): neither can carry imports from the entry point, so they don't need the guard.
Other factors
My prior review found the missing guard on the already-bundled return; commit 89b7b73 addressed it with a test in runtime-transpiler.test.ts. CodeRabbit's request for a post-second-run newCacheCount() assertion was applied in 1cf6eba. All review threads are resolved. The PR body's mechgate output shows the new tests failing on both debug+ASAN and release builds without the fix (5 fail / 1 fail respectively) and passing with it. Tests follow harness conventions (tempDir, bunEnv, concurrent pipe drain, exit code asserted last, Buffer.alloc(n, fill) for padding).
|
CI status: the diff is green. Across builds 90322 and 90360, every lane that runs the new tests passed (194-195 of 196 jobs). The remaining failures are unrelated to this change:
One retrigger was already spent. Ready for review and merge. |
2 participants
Fixes #37161
A file with the
// @bunpragma had its non-ASCII string literals decoded as latin-1 at runtime, one code point per UTF-8 byte, so"\u2014"(U+2014) became U+00E2 U+0080 U+0094.Bun.buildwrites the pragma into its output, so bundles that contain raw UTF-8 bytes were corrupted without anyone opting in.Repro:
Cause: the pragma makes the loader skip the transpiler and hand the raw source bytes to JSC directly, and both the sync path (
src/runtime/jsc_hooks.rs) and the async transpiler-store path (src/jsc/RuntimeTranspilerStore.rs) built the source string withString::clone_latin1, which maps each byte to one code point. That is only correct for pure ASCII.Fix: use
String::clone_utf8at both sites. It keeps the Latin-1 fast path for all-ASCII sources and does a real UTF-8 decode otherwise, matching how the non-pragma path decodes the same bytes.Bytecode sidecars (
// @bun @bytecode) are safe:bun build --bytecodeforces--target=bun, whose printer escapes to ASCII, so the SourceCodeKey hash is unchanged for bundler output. If a source with a sidecar does contain raw non-ASCII bytes, the key no longer matches and JSC silently falls back to parsing (verified manually with an edited bundle, no crash, correct output).Tests: added import, require, and entry-point cases with raw UTF-8 to
test/bundler/transpiler/runtime-transpiler.test.ts. All three fail on bun 1.3.6 (e2 80 94instead of2014) and pass with this change, along with the rest of the file.Second commit: fixing this surfaced a related pre-existing bug in the same loader path. The transpiler-cache-hit branch for the entry point returns early and skips the
is_main -> has_loaded = truewrite the full parse path does. Withhas_loadedstill false, an import of a file with an unknown extension (for example./template.hbs) falls into the "potentially the main module" fallback and is parsed as tsx, failing withExpected JSX element name but found "!". So an entry point above the 4 KiB cache minimum that imports such an asset works on the first run and fails on every cache-hit run after. Debug builds discard restored cache entries, which is why this only reproduces in release builds (the existing// @bun > async transpilerandrequire()tests inruntime-transpiler.test.tsfail on any release build with a warm cache, including current main). Fixed by settinghas_loadedon the cache-hit path too, with a test intest/cli/run/transpiler-cache.test.tsthat enables cache restore in debug builds viaBUN_DEBUG_ENABLE_RESTORE_FROM_TRANSPILER_CACHE.Third commit: review found the same
has_loadedgap in the already-bundled early return itself, so a// @bunentry point importing an unknown-extension asset hit the same tsx fallback in every build type. Same guard applied there, with a test inruntime-transpiler.test.ts.[review] gate passed · iteration 3 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 3
evidence per changed file
root cause · written by the author bot
When a file carried the // @Bun pragma, the already-bundled fast path in the module loader converted the raw source bytes with a Latin-1 decode instead of a UTF-8 decode, so every non-ASCII character was expanded into one code point per UTF-8 byte, producing mojibake like U+2014 becoming U+00E2 U+0080 U+0094. The fix switches both affected sites, the synchronous loader path and the async transpiler worker path, to the UTF-8 decoding helper already used elsewhere, which still takes the Latin-1 fast path for all-ASCII sources so pure-ASCII behavior is unchanged. It also sets the VM's has_load…