fix(bake): validate app.bundlerOptions values are objects#30653
Conversation
Bun.serve would hit a debug assertion when app.bundlerOptions, its server/client/ssr sub-options, or minify were not objects (e.g. minify: 1225 or minify: false). Validate the types and throw a proper TypeError instead, matching Bun.build's handling of the minify option.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #30125. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| test("minify as false does not crash", () => { | ||
| expect(() => | ||
| // @ts-expect-error | ||
| serve({ app: { bundlerOptions: { server: { minify: false } } } }), | ||
| ).not.toThrow("reached unreachable code"); | ||
| }); |
There was a problem hiding this comment.
🟡 This assertion is ineffective: .not.toThrow("reached unreachable code") passes when a different error is thrown, and this config (missing framework) will throw 'app' is missing 'framework' right after bundlerOptions parsing — so the test passes for the wrong reason and would still pass if the minify: false fix were reverted. CLAUDE.md also explicitly forbids tests that check for absence of panic messages. Assert the expected error instead, e.g. .toThrow("'app' is missing 'framework'"), which proves execution got past minify parsing.
Extended reasoning...
What the bug is
The new test 'minify as false does not crash' asserts:
expect(() =>
serve({ app: { bundlerOptions: { server: { minify: false } } } }),
).not.toThrow("reached unreachable code");This violates the explicit rule in the repo's CLAUDE.md (line ~126): "NEVER write tests that check for no 'panic' or 'uncaught exception' or similar in the test output. These tests will never fail in CI." The string "reached unreachable code" is the panic message produced by bun.debugAssert — i.e. exactly the kind of negative-panic check the guidelines forbid.
Why the assertion is ineffective
There are two independent reasons this test cannot catch a regression of the minify: false fix:
-
.not.toThrow(<msg>)passes on a different error. Per Jest/Bun semantics,.not.toThrow("X")passes if the callback throws nothing or throws an error whose message does not contain"X". It only fails if the callback throws an error containing"X". -
A debug-build panic isn't a catchable JS error. The original failure was
bun.debugAssertinsideJSValue.get(), which aborts the process. An in-processexpect().not.toThrow()cannot observe a process abort — the test runner just dies. And in release builds,debugAssertis compiled out entirely.
The specific code path that hides the regression
Looking at UserOptions.fromJS in src/bake/bake.zig, after bundlerOptions is parsed the very next step is:
const framework = try Framework.fromJS(
try config.get(global, "framework") orelse {
return global.throwInvalidArguments("'" ++ api_name ++ "' is missing 'framework'", .{});
},
...
);The test config { app: { bundlerOptions: { server: { minify: false } } } } has no framework key, so serve() will throw 'app' is missing 'framework'. That error message does not contain "reached unreachable code", so .not.toThrow("reached unreachable code") passes.
Step-by-step proof that the test passes even if the fix is reverted
Suppose the minify handling is reverted to the buggy form (isBoolean() and asBoolean() falling through to property reads on false):
- Debug build:
JSValue.get()hitsbun.debugAssert(target.isObject())and the process aborts. No JS error is thrown; the test file simply crashes (which CLAUDE.md notes does not reliably fail CI). The.not.toThrow(...)assertion never gets to evaluate a thrown error. - Release build:
bun.debugAssertis a no-op.get("whitespace")etc. on the JS valuefalseeither returns nothing useful or proceeds; eventually execution reaches theframeworkcheck and throws'app' is missing 'framework'..not.toThrow("reached unreachable code")→ passes.
In neither build does the assertion fail. The test therefore provides zero regression coverage for the bug it was written to guard against.
Impact
This is a test-quality issue rather than a production bug, but it ships a permanently-green test that gives false confidence. If the minify: false handling regresses, this test will not catch it.
How to fix
Change the assertion to positively verify that bundlerOptions parsing succeeded and execution reached the next validation step:
test("minify as false does not crash", () => {
expect(() =>
// @ts-expect-error
serve({ app: { bundlerOptions: { server: { minify: false } } } }),
).toThrow("'app' is missing 'framework'");
});This proves minify: false was accepted (no panic, no "Expected minify to be a boolean or an object" error) and parsing continued to the framework check. Alternatively, supply a valid framework and assert it does not throw at all — but asserting the specific downstream error is the simplest change that turns this into an effective regression test.
1 participant
Fuzzer fingerprint:
f6f8b134d3813972What
Bun.serve({ app: { bundlerOptions: ... } })hit a debug assertion inJSValue.get()when:bundlerOptionswas not an objectbundlerOptions.{server,client,ssr}was not an objectminifywas neither a boolean nor an object (e.g. a number, orfalsewhich fell through theisBoolean() and asBoolean()check)Fix
Validate these values are objects before reading properties from them and throw a
TypeErrorinstead, matching the existing handling inJSBundler.zig.