robobunwants to merge 3 commits into
Conversation #
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.
WalkthroughThis PR changes source content cloning from Latin-1 to UTF-8 in Rust transpiler cache-hit paths (RuntimeTranspilerStore and jsc_hooks), adds a shared UTF-8 decoding helper (stringFromPrinterBytes) for native module/CJS cache generation in ZigSourceProvider.cpp, and adds bundler tests covering non-ASCII banner and footer content. ## Changes #
🚥 Pre-merge checks | ✅ 4## ✅ Passed checks (4 passed) #
|
Updated 2:34 PM PT - Jul 9th, 2026❌
Build #71145 |
reviewed
claudeBotJul 9, 2026
** **claude Bot left a comment
left a comment
claudeBot There was a problem hiding this comment.
LGTM — targeted encoding fix with the ASCII fast-path preserved; the bytecode-generation sibling noted inline is a strict follow-up, not a regression.
Extended reasoning... #
Overview
Two one-line changes swapping `String::clone_latin1`
→ `String::clone_utf8`
on the already_bundled
paths in src/runtime/jsc_hooks.rs
and src/jsc/RuntimeTranspilerStore.rs
, plus four new itBundled
tests in bundler_banner.test.ts
/ bundler_footer.test.ts
covering non-ASCII banner/footer under `--target=bun`
for both the sync entry and async-import code paths.
Security risks
None. This is an encoding-correctness fix on bytes that Bun itself wrote to disk. No untrusted-input parsing surface is introduced; clone_utf8
→ BunString__fromBytes
already handles invalid UTF-8 (falls through to WTF::String::fromUTF8
which produces null on malformed input, same as any other file load).
Level of scrutiny
Low-to-moderate. The module is a hot path, but the change is a narrow encoding correction at two well-understood sites. I verified BunString__fromBytes
(BunString.cpp:479) does simdutf::validate_ascii
first and falls through to the identical BunString__fromLatin1
when the input is all-ASCII — which is essentially always true for // @bun
output — so the only added cost is one SIMD ASCII scan. No behavior change for the common case.
Other factors
The tests are well-structured: they assert the // @bun
pragma is present (proving the fast path is exercised), assert exact stdout including .length
(proving code-unit count, not just visual match), and cover both sites via a direct-entry run and an import "./out.js"
runtime file. The one nit found — the paired build-time bytecode path in ZigSourceProvider.cpp
still constructing via Latin-1, causing a silent .jsc
cache miss when combined with --bytecode
- non-ASCII banner — is a strict follow-up: runtime output is now correct where it was mojibake before, so this PR is a strict improvement even on that path.
reviewed
claudeBotJul 9, 2026
** **claude Bot left a comment
left a comment
claudeBot There was a problem hiding this comment.
LGTM — the follow-up commit addresses the bytecode SourceCodeKey sibling site I flagged, and the new NonAsciiBannerTargetBunBytecode
test asserts the cache actually hits.
Extended reasoning... #
Overview
Three one-line swaps from Latin-1 → UTF-8 decoding on the // @bun
already-bundled paths (jsc_hooks.rs
sync entry, RuntimeTranspilerStore.rs
async import) plus a matching 5-line stringFromPrinterBytes
helper in ZigSourceProvider.cpp
for the two build-time bytecode-generation entry points. Five new itBundled
tests cover the sync path, async-import path, non-ASCII identifier, footer, and bytecode cache-hit under BUN_JSC_verboseDiskCache=1
.
Security risks
None. This decodes the user's own bundler output from disk; no untrusted network input, no auth/crypto/permissions surface. clone_utf8
→ BunString__fromBytes
and stringFromPrinterBytes
both take the simdutf validate_ascii
fast-path to the same Latin-1 memcpy for the overwhelmingly-common all-ASCII case, and fall back to fromUTF8ReplacingInvalidSequences
on invalid UTF-8 (never null / never unbounded).
Level of scrutiny
Low-to-medium. The native change is a helper-for-helper swap with no ownership, refcount, or lifetime changes; the ASCII hot path is byte-identical. I verified BunString__fromUTF8
(the non-ASCII arm of clone_utf8
) and stringFromPrinterBytes
produce equivalent UTF-16 for both valid and invalid UTF-8, so build-time and load-time SourceCodeKey
s agree.
Other factors
My previous review's only finding (bytecode-path sibling site) was addressed in 68c1594 with a test that would fail on key mismatch. PR description shows fail-on-canary / pass-on-PR evidence for both ASAN debug and release. No CODEOWNERS on the touched paths, no outstanding human review comments.
reviewed
claudeBotJul 9, 2026
** **claude Bot left a comment
left a comment
claudeBot There was a problem hiding this comment.
LGTM — the sibling bytecode-generation path is now aligned and covered by the cache-hit assertion.
Extended reasoning... #
Overview
Two one-line changes in the already_bundled
arms (jsc_hooks.rs
, RuntimeTranspilerStore.rs
) switching `String::clone_latin1`
→ `String::clone_utf8`
, plus a 6-line stringFromPrinterBytes
helper in ZigSourceProvider.cpp
so the build-time bytecode SourceCodeKey
is derived from the same decoding as the load-time key. Five new itBundled
tests cover sync entry, async import, footer, non-ASCII identifier, and the CJS+bytecode cache-hit path.
Security risks
None. This is a source-decoding change on the // @bun
fast path; no auth/crypto/permissions surface.
Level of scrutiny
Module is a hot path, but the change is narrow: clone_utf8
calls BunString__fromBytes
, which fast-paths to the identical Latin-1 memcpy when simdutf::validate_ascii
passes (the overwhelming case for printer output), and only diverges on the exact inputs that were previously mojibake. I verified stringFromPrinterBytes
and BunString__fromUTF8
agree on valid-UTF-8 (both produce the canonical UTF-16) and on invalid-UTF-8 (both route through `fromUTF8ReplacingInvalidSequences`
), so build-time and load-time keys match. The transpiled (non-already-bundled) print path still correctly uses `clone_latin1`
on `Format::EsmAscii`
output, which is unchanged.
Other factors
My earlier comment about the paired generateCached*ByteCodeFromSourceCode
sites was addressed in 68c1594 with the shared helper and a
banner/NonAsciiBannerTargetBunBytecode
test that asserts [Disk Cache] Cache hit for sourceCode
under BUN_JSC_verboseDiskCache=1
— a key mismatch would fail it. Evidence block shows debug+ASAN and release both green, and the tests fail on canary for the right reasons.| CI is failing on unrelated lanes across three runs (71111, 71126, 71145): None of these touch the module , bundler, or bytecode generation paths this PR changes. |
| This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
[github-actions](/apps/github-actions)Bot changed the title
Jul 9, 2026
Learn more about bidirectional Unicode characters