Repository navigation
Conversation
A route value that is not a function/Response/HTMLBundle/BunFile/false
throws a TypeError at registration time. The same invalid value nested
inside a method object ({GET: 42}, {GET: null}, {GET: "str"}, etc.) was
accepted silently and the method became dead: requests fell through to the
fetch fallback with no error at registration or request time.
The method loop set found=true for any own method key before checking the
value type, so an unrecognized value skipped both the callback and static
route branches and fell out of the if/else without error, then found=true
suppressed the top-level validator.
Now per-method values match top-level semantics: undefined skips the key,
false marks the object as a method map without registering a handler, any
other value that is not a callable or a valid static route throws with the
path and method name.
Walkthrough
ChangesPer-method route validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced on stock 1.4.0-canary: Follow-ups from review, all addressed:
CI status: the diff is green. Build 84881 failures are unrelated to this change: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/runtime/server/ServerConfig.rs`:
- Around line 972-977: Update the invalid route-handler error construction in
the ServerConfig route validation branch to include a non-coercing description
of the rejected value or its stable JavaScript type, while preserving the route,
method, accepted representations, and remedy guidance in the message. Use the
existing value-formatting or type-description utility if available rather than
coercing the value directly.
- Around line 924-933: Update the method-entry handling around the existing
found flag so undefined and false sentinel values are skipped before route-name
validation, while tracking recognized method entries separately from whether a
route name has been validated. Ensure a lone undefined method does not fall
through to AnyRoute::from_js, false method entries do not validate names, and
the first real handler or static route validates the name; add coverage for lone
undefined and invalid-name-with-false cases.
🪄 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: 1eb75c76-9c41-47cc-9312-67872a2a743a
📒 Files selected for processing (2)
src/runtime/server/ServerConfig.rstest/js/bun/http/bun-serve-routes.test.ts
…ved type in error
Split found (any method key present) from route_name_validated (checked
once before the first real handler) so a lone {GET: undefined} or
{GET: false} is recognized as a method map and does not fall through to
the top-level validator, preserving the pre-existing behavior for those
sentinels. The error message now names the received typeof (null special
cased).
|
Updated 4:14 AM PT - Jul 29th, 2026
❌ @robobun, your commit 2c84eec has 3 failures in
🧪 To try this PR locally: bunx bun-pr 36311That installs a local version of the PR into your bun-36311 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Per-method false means the method is explicitly unhandled and falls through to fetch. The false arm now sets has_head_route for Method::HEAD so the derived HEAD route is not pushed after the loop, matching the callable and static-route arms.
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 `@src/runtime/server/ServerConfig.rs`:
- Around line 936-939: Update the route-processing flow around
validate_route_name so the method value is first confirmed as callable or an
allowed static route before validating route parameter names. Ensure invalid
method values produce the method-specific TypeError before any route-name error,
and add coverage for a route such as "/:1" with GET set to a number.
🪄 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: 006d9e18-ebcf-4321-93b7-007f0b81c5fb
📒 Files selected for processing (2)
src/runtime/server/ServerConfig.rstest/js/bun/http/bun-serve-routes.test.ts
There was a problem hiding this comment.
LGTM. All earlier review threads are addressed (52b3a5c, 9356b6a); the remaining static-GET HEAD: false nit is not a regression and is reasonably deferred to a follow-up.
What was reviewed:
found = truenow precedes theundefined/falseskips, so a lone{ GET: undefined|false }is recognized as a method map and does not fall through to the top-level validator — covered by the new lone-sentinel tests.- The
falsearm setshas_head_routeforMethod::HEAD, matching the callable and static sibling arms; covered by the new implicit-HEAD test. - The new error path uses
js_type_string(non-throwing, SmallStrings-backed) with anullspecial-case, andthrow_invalid_argumentsreturnsErrso nothing allocated above leaks. route_name_validatedis separate fromfound, so sentinel-only method maps never triggervalidate_route_nameon paths like/:1.
Extended reasoning...
Overview
This PR closes a validation gap in ServerConfig::from_js: per-method route values ({ "/x": { GET: <value> } }) are now validated with the same semantics as top-level route values. The Rust change is ~25 lines confined to the METHODS loop in the routes-object parse path — a new route_name_validated flag, undefined/false sentinel handling placed after found = true, a has_head_route = true write in the false arm for Method::HEAD, and an else branch that throws a descriptive invalid_arguments error naming the route, method, received typeof, and accepted types. The test change adds a describe block covering rejection of null/number/string/object/array/true, acceptance of undefined/false (both mixed and lone), a HEAD: false suppression test, and a positive control that a per-method Response is still accepted.
Security risks
None. This is registration-time input validation on a user-supplied config object; no untrusted network data, no auth/crypto/permissions. js_type_string is documented as non-throwing (SmallStrings-backed) so the error path cannot enter user JS. The change tightens validation rather than loosening it.
Level of scrutiny
Medium. Bun.serve is a core API, but the change is on the cold config-parse path, not the request-handling hot path, and it only adds an else to an existing if/else if chain plus reorders two flag writes. The behavior change is that previously-silently-dead per-method values now throw at registration — a strict improvement for the typo'd-import case the PR describes. The two shapes that could plausibly appear in working code (undefined from a conditional, false as a disable) are explicitly preserved and tested, so no realistic working config is broken.
Other factors
Three rounds of review feedback (CodeRabbit ×2, my prior runs ×3) have all been addressed or consciously deferred: the lone-{GET: undefined} regression, the missing received-type in the error, and the callable-GET HEAD: false suppression are fixed with tests; the static-GET HEAD: false sibling is deferred because it requires threading a per-path signal into apply_static_route via StaticRouteEntry/setup_routes, which is out of scope for a validation change and is not a regression (identical observable behavior to pre-PR, RFC-9110-compliant). Test coverage is thorough (it.each matrices for both accept and reject paths, lone-sentinel cases, positive control), and the PR body shows USE_SYSTEM_BUN=1 fails / debug+release pass. No outstanding unresolved threads.
What
Bun.serve({ routes })validates top-level route values:{ "/x": 42 }throws aTypeErrorat registration. The same invalid value nested inside a method object was accepted silently, leaving that method dead (falls through tofetchor 404) with no error at registration or request time.The realistic face of this is a typo'd handler import, e.g.
{ GET: handlers.getUsr }, producing a route that silently never runs.Why
The per-method loop in
ServerConfig::from_jssetfound = truefor any own method key before checking the value type. A value that was neither callable nor a validAnyRoutefell out of theif/else ifwithout anelse, andfound = truethen suppressed the top-level validator. Panic/top-frame signature:Invalid value for route "/x" method GET.Fix
Per-method values now match top-level semantics:
undefinedskips the key (same as top-level"/x": undefined)falsemarks the object as a method map without registering a handler (documented disable)Invalid value for route "<path>" method <METHOD>. Expected a function, Response, HTMLBundle, BunFile, or false.Verification
[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file