Conversation
…ersion as-is compile/HelloWorldWithProcessVersionsBun compared the value --compile inlines for process.versions.bun (Global::package_json_version, which is "x.y.z-debug" on debug builds) against Bun.version with "-debug" stripped, so the compiled program always exited 1 on a debug build. Bun.version is the same constant from the same binary, so compare the two unmodified, in both the CLI and the Bun.build variants of the test.
|
Warning Review limit reached
Next review available in: 26 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 (1)
Comment |
|
Status: fix pushed (aef13f1), waiting on CI. Reproduced on a debug build at 81cfca9 with |
There was a problem hiding this comment.
LGTM — test-only fix that removes an asymmetric -debug strip so the inlined process.versions.bun is compared against Bun.version as-is.
What was reviewed:
- Confirmed
Bun.version(BunObject.cpp:289 →Bun__version + 1),process.versions.bun(BunProcess.cpp:228), and the--compiledefine (compile_target.rs:434) all resolve toGlobal::package_json_version, which carries-debugon debug builds — so the exact-equality comparison holds on debug, release, and canary. - Both test variants (CLI and API) now use the identical comparison; the old CLI variant stripped
-debugfrom only the RHS, which is why it failed on debug builds. - No runtime code touched; assertion is strictly stronger (exact match, no normalization).
Extended reasoning...
Overview
Two-line change plus a comment in test/bundler/bundler_compile.test.ts. Removes .replaceAll("-debug", "") from the expected side of compile/HelloWorldWithProcessVersionsBun and from both sides of compile/HelloWorldWithProcessVersionsBunAPI, so both tests compare the compile-inlined process.versions.bun against Bun.version verbatim.
Correctness
I traced all three values to the same constant:
src/options_types/compile_target.rs:434— the--compiledefine forprocess.versions.bunisGlobal::package_json_version.src/runtime/node/node_process.rs:155-159—Bun__versionis"v" + Global::package_json_version;src/jsc/bindings/BunObject.cpp:289returns it with the leadingvstripped, soBun.version === package_json_version.src/jsc/bindings/BunProcess.cpp:228— runtimeprocess.versions.bunis the same string.src/bun_core/Global.rs:488-493—package_json_versionis"X.Y.Z-debug"whenIS_DEBUG, plain"X.Y.Z"otherwise, and never carries the canary tag.
Since bunExe() is the same binary as the test runner, the inlined value and Bun.version are equal by construction on every build variant. The prior CLI test stripped -debug from Bun.version but not from the inlined value, so it compared "1.4.0-debug" === "1.4.0" on debug builds. The new comparison is both simpler and stronger.
Security risks
None. Test-only change to a template-literal string comparison.
Level of scrutiny
Low. No production code touched, no test weakened (the assertion is now exact rather than normalized), and the two sibling tests are made consistent per the repo's "fix the whole class" guidance. The added comment explains why no stripping is needed here versus CLI-banner tests, which is the exact confusion that produced the original bug.
Other factors
The PR description documents fail-before on bun bd and pass-after on both debug and release/canary. No outstanding reviewer comments.
|
Updated 11:02 PM PT - Aug 10th, 2026
✅ @robobun, your commit aef13f1a15229471fa34febcafa78f0a0a41e2bc passed in 🧪 To try this PR locally: bunx bun-pr 37373That installs a local version of the PR into your bun-37373 --bun |
…gram The runtime embedded in the output is the binary that ran the bundler, so reading Bun.version there compares the inlined value against the version it has to match, and keeps working when expectBundled's BUN_EXE override makes the bundler a different build than the test runner. On a mismatch the program now reports both strings instead of a bare exit 1.
There was a problem hiding this comment.
LGTM — test-only fix that corrects a debug-build-only failure by comparing the inlined process.versions.bun against the embedded runtime's Bun.version instead of a test-runner-interpolated literal with -debug stripped from one side.
What was reviewed:
- Confirmed
Bun.versionis not in the--compiledefine list (onlyprocess.platform/process.arch/process.versions.bunare), so the comparison is inlined-constant vs. runtime-evaluated — the assertion is not vacuous. - Checked that the property the original test protected (the inlined version matches the actual runtime version) is preserved and now asserted exactly rather than with normalization.
- Both CLI and API variants now use the same comparison; failure now throws with a diagnostic instead of silently exiting 1.
Extended reasoning...
Overview
Test-only change to test/bundler/bundler_compile.test.ts fixing two itBundled cases (compile/HelloWorldWithProcessVersionsBun and its ...BunAPI variant). The old fixture interpolated ${Bun.version.replaceAll("-debug", "")} from the test runner into the compiled program's source as a string literal, then compared it against the value the bundler inlined for process.versions.bun. On debug builds the inlined value is "1.4.0-debug" while the stripped literal is "1.4.0", so the CLI variant exited 1. The new fixture compares the inlined value against Bun.version evaluated at runtime inside the compiled program — both are package_json_version, so they match on debug, release, and canary builds.
Security risks
None. No runtime/native code touched; only test fixture strings.
Level of scrutiny
Low. Small, self-contained test correction with a clearly explained root cause. I verified in src/options_types/compile_target.rs that only process.platform, process.arch, and process.versions.bun are compile-time defines — Bun.version is not, so the new comparison is genuinely inlined-value vs. runtime-value and not two constants folded to the same literal. The assertion is at least as strong as before (exact equality with a diagnostic throw on mismatch, vs. a silent exitCode = 1 gated on a normalized comparison).
Other factors
The PR description documents fail-before on debug (bun bd test), pass-before on release (USE_SYSTEM_BUN=1), and pass-after on both plus a canary build. The API variant previously stripped -debug from both sides as a workaround; both tests now use the identical comparison, which is a small consistency improvement. The added comment explains why no normalization is applied. No existing assertion was weakened or deleted.
What does this PR do?
Test-only change.
compile/HelloWorldWithProcessVersionsBunintest/bundler/bundler_compile.test.tsfails on every debug build; it passes in CI only because release builds have no-debugversion suffix.Cause: the compiled program compares
require("./<platform>-<arch>.js"), which evaluates to theprocess.versions.bunstring that--compileinlined at bundle time, against"${Bun.version.replaceAll("-debug", "")}"interpolated by the test runner. The inlined value isGlobal::package_json_version(define_valuesinsrc/options_types/compile_target.rs), which is"1.4.0-debug"on a debug build, so the comparison is"1.4.0-debug" === "1.4.0"and the program exits 1. Compiling a small program with the debug binary shows the strings involved:The
-debugstripping is the right idiom for tests that match CLI banners, which printpackage_json_version_with_sha("1.4.0 (abc1234)"on debug builds);process.versions.bunandBun.versionarepackage_json_versionand do carry the suffix.History, for the record: the one-sided strip dates from #14940, but there the fixture was the first key of
files, and expectBundled uses the first key as the entry point, so the compiled program was the one-line fixture and the comparison never ran. #21915 moved/entry.tsfirst, which is when the CLI variant started failing on debug builds, and added theBunAPIvariant, which strips-debugfrom both sides and so passes.Fix: the program compares the inlined value against
Bun.versionread inside the compiled executable, and on a mismatch throws with both strings, so a future failure saysinlined X, runtime is Yinstead of a bareRuntime failed. Both variants use the same check.Why this comparison is the right one:
Bun.versionand the--compiledefine are the samepackage_json_versionconstant of one binary, and the executable produced bybun build --compileembeds the binary that ran the bundler, so inside the output the inlined value andBun.versioncome from the same binary on debug, release and canary builds (package_json_versionnever carries the canary tag). That is exactly the property inlining has to preserve, and nothing on either side needs normalizing. ReadingBun.versioninside the executable rather than interpolating the test runner's value also keeps the CLI variant correct under expectBundled's documentedBUN_EXEoverride, where the bundler (and therefore the embedded runtime) is a different build than the runner.How did you verify your code works?
bun bd test ... -t compile/HelloWorldWithProcessVersionsBun: CLI variant fails as above, API variant passes.USE_SYSTEM_BUN=1(release) passes, so the failure is debug-only.bun bd test ...: both pass.USE_SYSTEM_BUN=1 bun test ...(a1.4.0-canary.1build): both pass.BUN_EXE=build/release/bun bun bd test ...(debug runner, release bundler): both pass.--define 'process.versions.bun="9.9.9"'exits 1 witherror: inlined 9.9.9, runtime is 1.4.0-debug.The fix is in the test itself, so the fail-before is the unmodified test on a debug build (first bullet); there is no
src/change.Separately, while reading
define_valuesI noticed that--target=bun-<os>-<arch>-vX.Y.Zinlines the host's version rather than the target's. That is runtime behavior and is not changed here.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.