Skip to content

bake: validate app.bundlerOptions and its server/client/ssr fields are objects #30638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 2 commits into from
Closed

bake: validate app.bundlerOptions and its server/client/ssr fields are objects #30638

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03faa26
bake: validate app.bundlerOptions and its server/client/ssr fields ar…
robobun May 13, 2026
dd9f586
test: use describe.each/test.each for parameterized cases
robobun May 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/bake/bake.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,17 @@
}

if (try config.getOptional(global, "bundlerOptions", JSValue)) |js_options| {
if (!js_options.isObject()) {
return global.throwInvalidArguments("'" ++ api_name ++ ".bundlerOptions' must be an object", .{});
}
if (try js_options.getOptional(global, "server", JSValue)) |server_options| {
bundler_options.server = try BuildConfigSubset.fromJS(global, server_options);
bundler_options.server = try BuildConfigSubset.fromJS(global, "server", server_options);
}
if (try js_options.getOptional(global, "client", JSValue)) |client_options| {
bundler_options.client = try BuildConfigSubset.fromJS(global, client_options);
bundler_options.client = try BuildConfigSubset.fromJS(global, "client", client_options);
}
if (try js_options.getOptional(global, "ssr", JSValue)) |ssr_options| {
bundler_options.ssr = try BuildConfigSubset.fromJS(global, ssr_options);
bundler_options.ssr = try BuildConfigSubset.fromJS(global, "ssr", ssr_options);
}
}

Expand Down Expand Up @@ -202,9 +205,13 @@
minify_identifiers: ?bool = null,
minify_whitespace: ?bool = null,

pub fn fromJS(global: *jsc.JSGlobalObject, js_options: JSValue) bun.JSError!BuildConfigSubset {
pub fn fromJS(global: *jsc.JSGlobalObject, comptime property_name: []const u8, js_options: JSValue) bun.JSError!BuildConfigSubset {
var options = BuildConfigSubset{};

if (!js_options.isObject()) {
return global.throwInvalidArguments("'" ++ api_name ++ ".bundlerOptions." ++ property_name ++ "' must be an object", .{});
}

Check failure on line 213 in src/bake/bake.zig

View check run for this annotation

Claude / Claude Code Review

minify: false still triggers same debugAssert crash 

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 cl
Comment on lines +211 to +213

@claude claude Bot May 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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 here

getBooleanLoose (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 through
  • minify: 5 → isBoolean() = false → falls through
  • minify: "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 } },
  },
});
  1. UserOptions.fromJS reads bundlerOptions (an object — passes the new check at line 71).
  2. Reads server (an object — passes the new check at line 211 inside BuildConfigSubset.fromJS).
  3. js_options.getOptional(global, "minify", JSValue) returns the JS boolean false (not filtered, since getOptional only drops undefined/null).
  4. Line 225: minify_options.isBoolean() and minify_options.asBoolean() → true and false → false. Does not break :brk.
  5. Line 232: minify_options.getBooleanLoose(global, "whitespace") calls this.get(...) with this = false.
  6. 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.


if (try js_options.getOptional(global, "sourcemap", JSValue)) |val| brk: {
if (try bun.schema.api.SourceMapMode.fromJS(global, val)) |sourcemap| {
options.source_map = sourcemap;
Expand Down
25 changes: 25 additions & 0 deletions test/bake/app-options-validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { describe, expect, test } from "bun:test";

describe("Bun.serve app.bundlerOptions validation", () => {
test("throws when bundlerOptions is not an object", () => {
expect(() =>
Bun.serve({
// @ts-expect-error
app: { bundlerOptions: 42 },
}),
).toThrow("'app.bundlerOptions' must be an object");
});

Check warning on line 11 in test/bake/app-options-validation.test.ts

View check run for this annotation

Claude / Claude Code Review

Test will fail on non-canary release builds (bake feature flag not set in-process) 

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), `ServerCo
Comment on lines +4 to +11

@coderabbitai coderabbitai Bot May 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Comment on lines +4 to +11

@claude claude Bot May 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. .buildkite/ci.mjs:1293-1297 — when RELEASE is set or the commit message contains [release], the build sets canary: 0, which translates to --canary=off and produces a binary with env.is_canary = false and env.isDebug = false.
  2. scripts/runner.node.mjs:1182-1202 — the test runner spawns bun test with process.env plus BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING=1, but not BUN_FEATURE_FLAG_EXPERIMENTAL_BAKE.
  3. So inside the test process on a release build, bake() evaluates to false || false || false.
  4. Bun.serve({ app: { bundlerOptions: 42 } }) → ServerConfig.fromJS skips app → throws the fetch-required error.
  5. 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, app never read.
  • Config has no fetch/routes → Bun.serve throws 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.


for (const key of ["server", "client", "ssr"] as const) {
for (const value of [1073741824, "foo", true]) {
test(`throws when bundlerOptions.${key} is ${JSON.stringify(value)}`, () => {
expect(() =>
Bun.serve({
// @ts-expect-error
app: { bundlerOptions: { [key]: value } },
}),
).toThrow(`'app.bundlerOptions.${key}' must be an object`);
});
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
});
Loading