Conversation
When JSC's bytecode generator rejects a bundled chunk, bun build --bytecode would log 'error: Failed to generate bytecode for ...' but then proceed to write the executable and exit 0, so CI could not detect the failure. The --outdir (non --compile) path did not even log the error. Return Err(BuildFailed) from both chunk-generation paths after logging. This also avoids a debug-build assertion in OutputFileList::take() that fires when the pre-sized bytecode slot is left unfilled. Fixes #15528
|
Warning Review limit reached
Next review available in: 10 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 (3)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 6:27 PM PT - Aug 1st, 2026
❌ @robobun, your commit b06839f has 2 failures in
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.825c6c06-57ae-4540-8073-90c85e701aa5🧪 To try this PR locally: bunx bun-pr 36708That installs a local version of the PR into your bun-36708 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
A CSS chunk created from a JS entry has a JS-like loader, so the --outdir bytecode path was attempting (and, after the previous commit, failing) bytecode generation on CSS text. Match the sibling guard used by the --compile path and the OutputFileList pre-sizing.
There was a problem hiding this comment.
LGTM — the CSS-chunk guard I flagged earlier is in place with a regression test, and both bytecode paths now fail the build consistently.
What was reviewed:
- Both chunk-emit paths (
generateChunksInParallel.rsin-memory/compile,writeOutputFilesToDisk.rs--outdir) now log +return Err(BuildFailed)onNonefromgenerate_cached_bytecode; verifiedcrate::Error::BuildFailedis the established bundler error variant andadd_error_fmtreturns()so dropping thelet _ =is correct. - The new
matches!(chunk.content, Content::Javascript(_))guard on the--outdirpath now matches the sibling in-memory path and theOutputFileListBuilderpre-sizing predicate, so CSS/HTML chunks are skipped and thetake()insertion-count invariant holds. - Checked early-return cleanup:
OwnedStringRAII derefssource_provider_url, andintermediate_outputis already restored to the chunk before the bytecode block in both files.
Extended reasoning...
Overview
Two-line native change in each of the two chunk-emit paths: when JSC bytecode generation returns None, log the existing "Failed to generate bytecode for …" message and return Err(crate::Error::BuildFailed) instead of falling through to a successful build. The --outdir path additionally gains a matches!(chunk.content, Content::Javascript(_)) guard (my prior review finding) so CSS chunks derived from a JS entry are not fed to the bytecode generator. Three tests cover --compile, --outdir, and the CSS-chunk-with-JS-entry case.
Security risks
None. This is bundler control-flow on an internal error path; no untrusted input parsing, no auth/crypto, no memory-safety changes (no new allocations, pointers, or unsafe). The early return happens after intermediate_output has been restored and while source_provider_url is held by an OwnedString whose Drop derefs it, so no new leak path.
Level of scrutiny
Low-to-moderate. The change is a focused bugfix for two user-filed issues (#15528, #15351): a build that prints error: and then exits 0 is unambiguously wrong. Err(BuildFailed) is the established idiom throughout bundle_v2.rs / computeChunks.rs for "logged an error, abort the build". The one non-trivial risk — turning a previously-silent None into a hard error for chunks that legitimately should not be bytecode-compiled — was the CSS-chunk case I raised on the last revision, and it is now guarded and regression-tested.
Other factors
- All prior inline comments (comment-cop noise, my CSS-chunk finding) are resolved on the current commit.
add_error_fmtreturns()(src/ast/lib.rs:1781), so removing thelet _ =is a correct cleanup, not a discardedResult.- Partial
--outdiroutput when bytecode fails mid-loop (earlier chunks already written) matches every other pre-existing error path inwrite_output_files_to_diskand is not a regression. - This is a user-visible behavior change (exit 0 → exit 1), but it is exactly what the linked issues request and the previous behavior was producing silently-broken binaries; not a design decision needing human sign-off.
There was a problem hiding this comment.
LGTM — the CSS-chunk guard I flagged earlier is in place and covered by bytecode/OutdirBytecodeSkipsCssChunk.
What was reviewed:
- Both
generate_cached_bytecodecall sites now log +return Err(BuildFailed)onNone; guards match the pre-sizing predicate inOutputFileListBuilder.rs:119, so thetake()insertion-count assertion stays sound. - Confirmed
add_error_fmtreturns(), so dropping thelet _ =is a no-op;log_disjoint()writes to the sameTranspiler.logaslog_mut(). - Verified these are the only two
generate_cached_bytecodecallers in the bundler; no other silent-swallow siblings. - Early-return on the
--outdirpath leaves partial output (sourcemap already written) — consistent with every other error exit in that function.
Extended reasoning...
Overview
Two-line behavioral change replicated across the two chunk-generation paths: when JSC returns None from generate_cached_bytecode, log the existing "Failed to generate bytecode" error and return Err(crate::Error::BuildFailed) instead of falling through to break 'brk None. The --outdir path also gains the matches!(chunk.content, Content::Javascript(_)) guard (added after my previous review) so a CSS chunk born from a JS entry point does not attempt bytecode generation. Three tests cover the --compile failure path, the --outdir failure path, and the CSS-chunk-skips-bytecode regression.
Security risks
None. This is bundler error-handling flow control; no untrusted-input parsing, allocation sizing, FFI, or memory-lifetime changes. add_error_fmt returns () so removing let _ = is cosmetic.
Level of scrutiny
Moderate — it is a user-visible behavior change (builds that previously exited 0 with a printed error now exit 1), but that is the explicit fix for #15528/#15351 and matches REVIEW.md's "operations the user explicitly requested fail the whole command on any failure — never warn-and-exit-zero." --bytecode is an explicit request; shipping a binary without the requested bytecode is a silent failure. The change is small, mechanical, and mirrors the established add_error_fmt + return Err(BuildFailed) pattern used throughout bundle_v2.rs and LinkerContext.rs.
Other factors
- My prior review flagged the missing
Content::Javascriptguard on the--outdirpath; the author fixed it in 2d79296 and added a dedicated regression test. All three bytecode sites (generateChunksInParallel.rs,writeOutputFilesToDisk.rs,OutputFileListBuilder.rs:119) now share the same predicate, so the pre-sizedtotal_insertions == output_files.len()invariant holds. - Grep confirms only two callers of
generate_cached_bytecodein the bundler; both are patched. - Tests follow harness conventions:
tempDir,bunEnv/bunExe, concurrent pipe drains, stderr asserted before exit code, issue URL comment. The banner-based repro is a clean way to make JSC (not bun's parser) reject the bundle. - The comment-cop bot's flag on the redundant comment was addressed in 35056f2; all inline threads are resolved.
|
CI status: the diff itself is green. The red on that build is unrelated to this change:
Ready for review/merge. |
|
Verified again on top of current main ( On main, that input panics: the debug build asserts The three tests in this PR and the other 25 |
|
Another trigger for the same failure, from a fuzz matrix. This one is an ESM-goal JSC rejection, so the printf 'var o = { x: "with-x" };\nwith (o) { exports.v = x; }\n' > s.cjs
printf 'import { v } from "./s.cjs";\nconsole.log("v =", v);\n' > entry.ts
bun run entry.ts # v = with-x
bun build --compile --bytecode --format=esm ./entry.ts --outfile app; echo rc=$?; ./appVerified on This branch merged onto main ( An extra case for this PR, if you want ESM coverage next to the two // A CommonJS dependency with sloppy-mode-only syntax is bundled into an ESM
// chunk, which JSC's module-goal parser rejects at bytecode generation time.
// The build must fail instead of writing an executable without bytecode.
test("compile/BytecodeFailureIsAnErrorESM", async () => {
using dir = tempDir("bytecode-failure-esm", {
"entry.ts": `import { v } from "./s.cjs";\nconsole.log("v =", v);\n`,
"s.cjs": `var o = { x: "with-x" };\nwith (o) { exports.v = x; }\n`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "build", "./entry.ts", "--compile", "--bytecode", "--format=esm", "--outfile", "./out"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain("error: Failed to generate bytecode");
expect(stdout).not.toContain("compile");
expect(existsSync(join(String(dir), process.platform === "win32" ? "out.exe" : "out"))).toBe(false);
expect(exitCode).toBe(1);
});The executable dying without |
|
Another report of the same failure. This trigger is a published package, not a synthetic input.
cd test/node_modules
bun build @remix-run/react/dist/esm/index.js --target=bun --bytecode --outdir /tmp/out
The This branch now conflicts with main. The conflict is one hunk in |
|
Update, and a correction to my two earlier notes here. #43822 merged ( What #43822 settled
What is left of this PR State on main today, for whoever decides
The two options
The ESM test case I posted above asserts option 2. Ignore it if the answer is option 1. I am not changing anything here until a maintainer picks one. |
This branch has not been deployed
2 participants
Fixes #15528.
Problem
bun build --compile --bytecodeprintserror: Failed to generate bytecode for ...when JSC rejects the bundled output, but then proceeds to write the executable and exit 0. CI can't detect that the binary shipped without bytecode.The
--outdir(non--compile) path was worse: it swallowed the bytecode failure entirely (no message, no.jscfile, exit 0).Repro
The banner is prepended verbatim after bun's own parser runs, so JSC is the first thing to see the bad syntax; any real-world input that trips JSC's bytecode generator (the yargs example in the issue, sloppy-mode constructs in ESM strict mode, etc.) hits the same path.
Fix
Both chunk-generation paths (
generateChunksInParallel.rsfor in-memory/compile,writeOutputFilesToDisk.rsfor--outdir) nowreturn Err(BuildFailed)after logging the bytecode error, sobun buildexits 1 andBun.build()reportssuccess: false.This also makes a latent debug assertion in
OutputFileList::take()unreachable: the output list is pre-sized on the assumption that every JS chunk produces a bytecode entry, and leaving that slot unfilled trippedtotal_insertions != output_files.len()in debug builds.Verification
All 19 existing
Bytecodetests inbundler_compile.test.tsand all ofbundler_banner.test.tsstill pass.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_compile.test.ts