bake: validate app.bundlerOptions and its server/client/ssr fields are objects#30638
bake: validate app.bundlerOptions and its server/client/ssr fields are objects#30638robobun wants to merge 2 commits into
Conversation
WalkthroughThis PR adds stricter validation for ChangesBundler Options Validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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/bake/app-options-validation.test.ts`:
- Around line 13-24: Replace the nested for-loops in the test (which iterate
over ["server","client","ssr"] and [1073741824,"foo",true]) with nested
describe.each() calls so each combination is reported as a separate case; wrap
the outer suite with describe.each([ "server", "client", "ssr" ]) and the inner
with describe.each([1073741824, "foo", true]) and keep the existing expect(() =>
Bun.serve({ /* `@ts-expect-error` */ app: { bundlerOptions: { [key]: value } }
})).toThrow(...) inside the inner test, preserving the original test title
template `throws when bundlerOptions.${key} is ${JSON.stringify(value)}` and the
error message `'app.bundlerOptions.${key}' must be an object`.
🪄 Autofix (Beta)
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: e1f7826a-305e-4a96-b02f-6d2a4f59323f
📒 Files selected for processing (2)
src/bake/bake.zigtest/bake/app-options-validation.test.ts
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/bake/app-options-validation.test.ts`:
- Around line 4-11: The test currently only asserts the error message; update
the test "throws when bundlerOptions is not an object" (and the similar test
around the other case) to assert the full error shape: capture the thrown error
(via try/catch or Jest's toThrow wrapper), then assert it is an instance of
TypeError, that it has property code === 'ERR_INVALID_ARG_TYPE', and that the
message matches "'app.bundlerOptions' must be an object"; reference the test
file/test name to locate where to replace the single toThrow message check with
these three assertions.
🪄 Autofix (Beta)
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: c2b30498-5916-496f-a5ee-95916d72fed8
📒 Files selected for processing (1)
test/bake/app-options-validation.test.ts
| test("throws when bundlerOptions is not an object", () => { | ||
| expect(() => | ||
| Bun.serve({ | ||
| // @ts-expect-error | ||
| app: { bundlerOptions: 42 }, | ||
| }), | ||
| ).toThrow("'app.bundlerOptions' must be an object"); | ||
| }); |
There was a problem hiding this comment.
Assert error shape (TypeError + ERR_INVALID_ARG_TYPE), not only message.
Right now these tests only check message text, so they won’t catch regressions where the thrown error loses the expected type/code.
Suggested test hardening
describe("Bun.serve app.bundlerOptions validation", () => {
+ function expectInvalidArgType(fn: () => void, expectedMessage: string) {
+ try {
+ fn();
+ throw new Error("Expected Bun.serve() to throw");
+ } catch (err: any) {
+ expect(err).toBeInstanceOf(TypeError);
+ expect(err?.code).toBe("ERR_INVALID_ARG_TYPE");
+ expect(err?.message).toBe(expectedMessage);
+ }
+ }
+
test("throws when bundlerOptions is not an object", () => {
- expect(() =>
+ expectInvalidArgType(() =>
Bun.serve({
// `@ts-expect-error`
app: { bundlerOptions: 42 },
}),
- ).toThrow("'app.bundlerOptions' must be an object");
+ "'app.bundlerOptions' must be an object");
});
describe.each(["server", "client", "ssr"] as const)("bundlerOptions.%s", key => {
test.each([1073741824, "foo", true])("throws when value is %p", value => {
- expect(() =>
+ expectInvalidArgType(() =>
Bun.serve({
// `@ts-expect-error`
app: { bundlerOptions: { [key]: value } },
}),
- ).toThrow(`'app.bundlerOptions.${key}' must be an object`);
+ `'app.bundlerOptions.${key}' must be an object`);
});
});
});Also applies to: 15-21
🤖 Prompt for 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.
In `@test/bake/app-options-validation.test.ts` around lines 4 - 11, The test
currently only asserts the error message; update the test "throws when
bundlerOptions is not an object" (and the similar test around the other case) to
assert the full error shape: capture the thrown error (via try/catch or Jest's
toThrow wrapper), then assert it is an instance of TypeError, that it has
property code === 'ERR_INVALID_ARG_TYPE', and that the message matches
"'app.bundlerOptions' must be an object"; reference the test file/test name to
locate where to replace the single toThrow message check with these three
assertions.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #30125, which includes the same fix plus additional |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (!js_options.isObject()) { | ||
| return global.throwInvalidArguments("'" ++ api_name ++ ".bundlerOptions." ++ property_name ++ "' must be an object", .{}); | ||
| } |
There was a problem hiding this comment.
🔴 The new isObject() guard fixes the crash for server/client/ssr, but the same debugAssert(target.isObject()) crash still exists ~15 lines below for the minify field. With minify: false (or any non-boolean non-object like minify: 5), the isBoolean() and asBoolean() guard evaluates to false and falls through to minify_options.getBooleanLoose(global, "whitespace"), which calls .get() on a non-object and trips the same assertion. Since this PR's goal is eliminating this crash class in BuildConfigSubset.fromJS, the minify path should get the same treatment — break :brk on any boolean (not just true) and add an isObject() check before the getBooleanLoose calls.
Extended reasoning...
What the bug is
BuildConfigSubset.fromJS now correctly rejects non-object values for js_options itself (the new check at bake.zig:211), but the nested minify field at lines 224–240 still passes a non-object JSValue into a property getter, triggering the exact same bun.debugAssert(target.isObject()) panic this PR is fixing.
Code path
if (try js_options.getOptional(global, "minify", JSValue)) |minify_options| brk: {
if (minify_options.isBoolean() and minify_options.asBoolean()) {
// ... set all three to true
break :brk;
}
if (try minify_options.getBooleanLoose(global, "whitespace")) |value| { // <-- crash heregetBooleanLoose (JSValue.zig:1866) immediately calls this.get(global, property_name), and get() has bun.debugAssert(target.isObject()) at JSValue.zig:1534 — the same assertion in the stack trace from the PR description.
Why the existing guard doesn't help
The guard is minify_options.isBoolean() and minify_options.asBoolean(). It only takes the early break :brk when the value is boolean AND true. Anything else falls through to the property reads:
minify: false→isBoolean() and asBoolean()=true and false=false→ falls throughminify: 5→isBoolean()=false→ falls throughminify: "yes"→isBoolean()=false→ falls through
getOptional only filters undefined/null, so all of these reach minify_options as non-object JSValues and then hit getBooleanLoose → get → debugAssert(target.isObject()) → panic.
Step-by-step proof
With this input (a debug build of Bun):
Bun.serve({
app: {
framework: "react",
bundlerOptions: { server: { minify: false } },
},
});UserOptions.fromJSreadsbundlerOptions(an object — passes the new check at line 71).- Reads
server(an object — passes the new check at line 211 insideBuildConfigSubset.fromJS). js_options.getOptional(global, "minify", JSValue)returns the JS booleanfalse(not filtered, sincegetOptionalonly dropsundefined/null).- Line 225:
minify_options.isBoolean() and minify_options.asBoolean()→true and false→false. Does notbreak :brk. - Line 232:
minify_options.getBooleanLoose(global, "whitespace")callsthis.get(...)withthis = false. - JSValue.zig:1534:
bun.debugAssert(target.isObject())fails →panic: reached unreachable code.
This is the same crash signature as the one in the PR description, just one frame deeper.
Impact
minify: false is a very natural thing for a user to write to explicitly disable minification for one graph. In debug/assert builds it panics; in release builds it calls get() on a primitive, which is undefined behavior per the function's contract. The same Fuzzilli setup that produced fingerprint 40903585822636ec will immediately rediscover this once the outer check lands.
Relationship to this PR
The lines themselves (224–240) are pre-existing, but they are inside BuildConfigSubset.fromJS — the function this PR modifies specifically to eliminate this crash class. Leaving an identical crash ~15 lines below the new guard makes the fix incomplete for its stated purpose, so it's worth addressing here rather than in a follow-up.
Suggested fix
Mirror what JSBundler.zig does for the same option (JSBundler.zig:693–713):
if (try js_options.getOptional(global, "minify", JSValue)) |minify_options| brk: {
if (minify_options.isBoolean()) {
const b = minify_options.asBoolean();
options.minify_syntax = b;
options.minify_identifiers = b;
options.minify_whitespace = b;
break :brk;
}
if (!minify_options.isObject()) {
return global.throwInvalidArguments(
"'" ++ api_name ++ ".bundlerOptions." ++ property_name ++ ".minify' must be a boolean or an object",
.{},
);
}
// ... existing getBooleanLoose calls
}And add minify: false / minify: 5 cases to test/bake/app-options-validation.test.ts.
| test("throws when bundlerOptions is not an object", () => { | ||
| expect(() => | ||
| Bun.serve({ | ||
| // @ts-expect-error | ||
| app: { bundlerOptions: 42 }, | ||
| }), | ||
| ).toThrow("'app.bundlerOptions' must be an object"); | ||
| }); |
There was a problem hiding this comment.
🟡 This test calls Bun.serve({ app: ... }) directly in the test process, but the app option is gated behind bun.FeatureFlags.bake() (is_canary || isDebug || BUN_FEATURE_FLAG_EXPERIMENTAL_BAKE). The CI test runner (scripts/runner.node.mjs) doesn't set BUN_FEATURE_FLAG_EXPERIMENTAL_BAKE on the test process — only bunEnv from test/harness.ts sets it for spawned subprocesses — so on non-canary release builds (.buildkite/ci.mjs sets canary=0 for RELEASE/[release] runs), ServerConfig.fromJS silently skips app and Bun.serve falls through to the "fetch is required" error, failing every assertion here with the wrong message. Consider spawning a subprocess with bunEnv like test/bake/deinitialization.test.ts does.
Extended reasoning...
What the bug is
The new test file calls Bun.serve({ app: { bundlerOptions: ... } }) directly inside the bun test process and asserts on the validation error. However, the app key in Bun.serve is feature-flagged: src/runtime/api/BunObject.zig:1013 passes allow_bake_config = bun.FeatureFlags.bake(), and src/bun_core/feature_flags.zig:135-138 defines bake() as:
pub fn bake() bool {
return env.is_canary or env.isDebug or
bun.getRuntimeFeatureFlag(.BUN_FEATURE_FLAG_EXPERIMENTAL_BAKE);
}When this returns false, ServerConfig.zig:837-841 does break :brk and silently skips reading app entirely — UserOptions.fromJS (where the new validation lives) is never called. Bun.serve then falls through to the missing-handler path and throws something like "Bun.serve() requires either a fetch handler...", not "'app.bundlerOptions' must be an object".
The code path that triggers it
.buildkite/ci.mjs:1293-1297— whenRELEASEis set or the commit message contains[release], the build setscanary: 0, which translates to--canary=offand produces a binary withenv.is_canary = falseandenv.isDebug = false.scripts/runner.node.mjs:1182-1202— the test runner spawnsbun testwithprocess.envplusBUN_FEATURE_FLAG_INTERNAL_FOR_TESTING=1, but notBUN_FEATURE_FLAG_EXPERIMENTAL_BAKE.- So inside the test process on a release build,
bake()evaluates tofalse || false || false. Bun.serve({ app: { bundlerOptions: 42 } })→ServerConfig.fromJSskipsapp→ throws the fetch-required error.expect(...).toThrow("'app.bundlerOptions' must be an object")fails because the actual message doesn't match.
Why existing code doesn't prevent it
test/harness.ts:64 does set BUN_FEATURE_FLAG_EXPERIMENTAL_BAKE: "1" — but only in the bunEnv object, which is meant to be passed as env when spawning child processes. It does not affect the environment of the test process itself. Other bake tests handle this correctly: test/bake/deinitialization.test.ts spawns a subprocess with Bun.spawn({ env: bunEnv, ... }), and the dev-server tests go through bake-harness.ts which also spawns. This is the only test in the repo that calls Bun.serve({ app: ... }) in-process.
Step-by-step proof
- Build: release pipeline →
is_canary=false,isDebug=false. - Runner env: no
BUN_FEATURE_FLAG_EXPERIMENTAL_BAKE→getRuntimeFeatureFlag(...) = false. bake() = false.ServerConfig.zig:837:if (!(allow_bake_config and bun.FeatureFlags.bake())) break :brk;→ breaks,appnever read.- Config has no
fetch/routes→Bun.servethrows the generic missing-handler TypeError. - Test assertion expected
'app.bundlerOptions' must be an object→ fails (×10 cases in the file).
Impact
This won't show up on this PR's own CI because PR builds default to canary (getCanaryRevision returns 1 for PRs), masking the problem. But the release-build test pipeline (and any developer running the suite against a stable release binary) will see all 10 tests in this file fail with the wrong error message.
How to fix
Match the pattern used by test/bake/deinitialization.test.ts: write the Bun.serve({ app: ... }) snippets to a fixture/temp file and spawn them via Bun.spawn({ cmd: [bunExe(), file], env: bunEnv }), asserting on the subprocess's stderr/exit. Alternatively, have scripts/runner.node.mjs set BUN_FEATURE_FLAG_EXPERIMENTAL_BAKE=1 for the test process so in-process bake tests work everywhere.
1 participant
What does this PR do?
Fixes a debug assertion crash (
reached unreachable code) inBun.serve()whenapp.bundlerOptionsor one of itsserver/client/ssrfields is a non-object value (e.g. a number, string, or boolean).BuildConfigSubset.fromJScalled.getOptional()on its argument without first checking that it was an object, tripping thebun.debugAssert(target.isObject())insideJSValue.get. The same issue applied tobundlerOptionsitself.Repro
Before:
After:
How did you verify your code works?
Added
test/bake/app-options-validation.test.tscoveringbundlerOptionsand each ofserver/client/ssrwith non-object values.Found by Fuzzilli (fingerprint
40903585822636ec).