Skip to content

fix(bake): validate app.bundlerOptions values are objects #30653

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 1 commit into from
Closed

fix(bake): validate app.bundlerOptions values are objects #30653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 23 additions & 14 deletions src/bake/bake.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ pub const UserOptions = struct {
}

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);
}
Expand Down Expand Up @@ -205,6 +208,10 @@ const BuildConfigSubset = struct {
pub fn fromJS(global: *jsc.JSGlobalObject, js_options: JSValue) bun.JSError!BuildConfigSubset {
var options = BuildConfigSubset{};

if (!js_options.isObject()) {
return global.throwInvalidArguments("Expected bundler options to be an object", .{});
}

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 All @@ -214,22 +221,24 @@ const BuildConfigSubset = struct {
return bun.jsc.Node.validators.throwErrInvalidArgType(global, "sourcemap", .{}, "\"inline\" | \"external\" | \"linked\"", val);
}

if (try js_options.getOptional(global, "minify", JSValue)) |minify_options| brk: {
if (minify_options.isBoolean() and minify_options.asBoolean()) {
options.minify_syntax = minify_options.asBoolean();
options.minify_identifiers = minify_options.asBoolean();
options.minify_whitespace = minify_options.asBoolean();
break :brk;
}

if (try minify_options.getBooleanLoose(global, "whitespace")) |value| {
options.minify_whitespace = value;
}
if (try minify_options.getBooleanLoose(global, "syntax")) |value| {
if (try js_options.getOptional(global, "minify", JSValue)) |minify_options| {
if (minify_options.isBoolean()) {
const value = minify_options.asBoolean();
options.minify_syntax = value;
}
if (try minify_options.getBooleanLoose(global, "identifiers")) |value| {
options.minify_identifiers = value;
options.minify_whitespace = value;
} else if (minify_options.isObject()) {
if (try minify_options.getBooleanLoose(global, "whitespace")) |value| {
options.minify_whitespace = value;
}
if (try minify_options.getBooleanLoose(global, "syntax")) |value| {
options.minify_syntax = value;
}
if (try minify_options.getBooleanLoose(global, "identifiers")) |value| {
options.minify_identifiers = value;
}
} else {
return global.throwInvalidArguments("Expected minify to be a boolean or an object", .{});
}
}

Expand Down
36 changes: 36 additions & 0 deletions test/js/bun/http/bun-serve-args.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,3 +670,39 @@
}
});
});

describe("app.bundlerOptions validation", () => {
test("minify as non-boolean, non-object throws", () => {
expect(() =>
// @ts-expect-error
serve({ app: { bundlerOptions: { ssr: { minify: 1225 } } } }),
).toThrow("Expected minify to be a boolean or an object");
expect(() =>
// @ts-expect-error
serve({ app: { bundlerOptions: { client: { minify: "yes" } } } }),
).toThrow("Expected minify to be a boolean or an object");
});

test("minify as false does not crash", () => {
expect(() =>
// @ts-expect-error
serve({ app: { bundlerOptions: { server: { minify: false } } } }),
).not.toThrow("reached unreachable code");
});

Check warning on line 691 in test/js/bun/http/bun-serve-args.test.ts

View check run for this annotation

Claude / Claude Code Review

Ineffective .not.toThrow('reached unreachable code') assertion 

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'")
Comment on lines +686 to +691

@claude claude Bot May 14, 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 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:

  1. .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".

  2. A debug-build panic isn't a catchable JS error. The original failure was bun.debugAssert inside JSValue.get(), which aborts the process. An in-process expect().not.toThrow() cannot observe a process abort — the test runner just dies. And in release builds, debugAssert is 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() hits bun.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.debugAssert is a no-op. get("whitespace") etc. on the JS value false either returns nothing useful or proceeds; eventually execution reaches the framework check 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.


test("server/client/ssr as non-object throws", () => {
for (const key of ["server", "client", "ssr"]) {
expect(() =>
// @ts-expect-error
serve({ app: { bundlerOptions: { [key]: 123 } } }),
).toThrow("Expected bundler options to be an object");
}
});

test("bundlerOptions as non-object throws", () => {
expect(() =>
// @ts-expect-error
serve({ app: { bundlerOptions: 123 } }),
).toThrow("'app.bundlerOptions' must be an object");
});
});
Loading