Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Status: reproduced on the released 1.4.0 with the ledger snippet ( CI (build 93820, finished): 179 jobs passed, 0 test failures against this diff (the handful of flagged tests passed on retry and are unrelated to routing). The build is marked failed only because the two |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes user-facing route-grammar semantics (newly rejecting /:id:verb-style keys and newly accepting /v1:2/:id, /a:x/:x) and touches the public ExtractRouteParams type, a maintainer sign-off on the API decision would be worthwhile.
What was reviewed:
- Confirmed the new segment scan in
ServerRouteList.cppandvalidate_route_namematchesHttpRouter::add()/executeHandlers()(segment is a param iff it starts with:; whole segment captured). - Traced
ExtractRouteParamson/:id/:test,/orgs/:orgId/repos/:repoId/settings,/files:batchGet/:id,/v1:beta/:name/x,/static/*— the/${Rest}re-prefix keeps recursion correct and the removed${string}*branch was dead (both arms were{}). - Checked
strings::split(&path[1..], b"/")edge cases (bare/, trailing/, anonymous:segment) against the C++ loop — both agree on identifier count.
Extended reasoning...
Overview
The PR aligns three previously divergent parsers of Bun.serve route keys onto uWS's actual grammar (split on /; a segment is a parameter only when it starts with :, and the parameter captures the whole segment):
src/jsc/bindings/ServerRouteList.cpp— the params-name builder now scans by segment instead of by every:, so identifier count matchesreq->getParameter(i)count.src/runtime/server/ServerConfig.rs—validate_route_nameuses the same split, so the digit/duplicate checks only see real parameter names, and adds a new throw for":name:verb"-style segments that the router can never match as written.packages/bun-types/serve.d.ts—ExtractRouteParamsnow anchors on/:and re-prefixes/on recursion.- Docs paragraph, type fixture, and runtime tests updated to match.
I read packages/bun-uws/src/HttpRouter.h and confirmed add() (line 291) strips a segment to : only when segment[0] == ':', and executeHandlers() (line 190) pushes a param only when the stored node name starts with : — so a mid-segment : is indeed literal and :id:undelete captures the whole segment. The new C++ loop, the Rust split, and the TS template-literal recursion all produce the same identifier list for the tested keys and the edge cases I traced (leading /, trailing /, bare : segment, wildcard).
Security risks
None. Pure string-scan refactors over already-ASCII-validated route keys; no allocation ownership changes, no user-input parsing beyond what already existed.
Level of scrutiny
Medium-high. The mechanics are a focused ~40-line rewrite with good tests, but the change reaches user-facing API surface in three ways: (1) a new TypeError from Bun.serve() for keys like "/n/:id:undelete" and "/range/:from-:to" that previously registered (albeit with broken req.params); (2) previously-rejected keys "/v1:2beta/:id" and "/a:x/:x" now accepted; (3) the exported Serve.ExtractRouteParams type shape changes for literal-colon keys. Each is well-argued in the PR body, but "reject vs. name-the-whole-segment" for ":a:b" and "align to uWS vs. teach uWS Express-style mid-segment params" are API design calls a maintainer should ratify.
Other factors
Test coverage is thorough (both handler and per-method route objects, both positive matches and fallthrough, both new-throw and newly-accepted keys, plus the type fixture). No prior review comments to address. No memory-safety or exception-scope concerns in the C++ change — it's a pure loop over a WTF::String with the same m_pathIdentifiers.append as before.
|
Agreed that the one decision here is what to do with a parameter segment that contains a second
The rest of the PR (params object and validator using the router's segment split, the |
|
What does express do? Fastify? Etc... Use that to make your decision. |
|
Ran the three shapes through Express 4.22 (path-to-regexp 0.1), Express 5.2 and @koa/router 15 (path-to-regexp 8), Fastify 5.11 / find-my-way 9.7, and Hono 4.13 (full output in the PR description).
So for the For a |
…ith ':'
ServerRouteList built the req.params identifier list from every ':' in
the route key, while uWS's HttpRouter only captures a segment whose
first byte is ':'. A literal like "/files:batchGet/:id" therefore got
identifiers [batchGet, id] for a single captured value, so params.id was
empty and the value landed under params.batchGet.
Build the identifiers per segment with the router's grammar, validate
route keys with the same split, and reject a parameter segment that
contains a second ':' (":id:undelete", ":from-:to"), since the router
can only capture the whole segment. ExtractRouteParams follows the same
rule so the declared params match the runtime object.
a628e7c to
0b359bf
Compare
| // Same grammar as uWS::HttpRouter::add(): the path splits on '/', and a | ||
| // segment is a parameter only when it starts with ':'. paramsObjectForRoute | ||
| // pairs these identifiers positionally with req->getParameter(i), so a ':' | ||
| // inside a literal segment must not produce one. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Same grammar as uWS::HttpRouter::add() and ServerRouteList.cpp: the path | ||
| // splits on '/', and a segment is a parameter only when it starts with ':'. | ||
| // A ':' anywhere else in a segment is literal text. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // For now, we don't support params that start with a number. | ||
| // Mostly because it makes the params object more complicated to implement and it's easier to cut scope this way for now. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The router captures the whole segment as the parameter's value, so | ||
| // ":id:verb" or ":from-:to" can neither be matched nor named. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I reviewed this and didn't find bugs — the segment-split in ServerRouteList.cpp, validate_route_name, and ExtractRouteParams all agree with uWS's HttpRouter::add() grammar, and the tests cover the shifted-params, false-rejection, and new-throw cases. Deferring to a maintainer because the :id:undelete / :from-:to throw is a user-facing API decision that alii engaged on but hasn't signed off on yet, and there are open comment-cop notes on the multi-line comments (they look like false positives to me — those comments name the cross-component invariant, not a workaround — but they're unaddressed).
What was checked:
- The C++ segment scan handles the leading
/, empty-name (/:), and trailing-segment cases; identifier count now matches the router's capture count. strings::split/contains_charexist andpath[1..]is safe given the caller'spath[0] == '/'assert.ExtractRouteParamstraced for/:a/:b,/x:y/:z,/:a/b:c/:d,/static/*— matches runtime.
Extended reasoning...
Overview
Aligns three independent parsers of the same route-key grammar with the actual matcher (uWS HttpRouter): the params-object identifier list in ServerRouteList.cpp, the Rust-side validate_route_name, and the TS-level ExtractRouteParams. Adds a new registration-time throw for parameter segments containing a second :. Docs paragraph, type-fixture cases, and runtime tests included.
Security risks
None identified. Route keys are developer-supplied at Bun.serve() time, not request-derived; the change tightens validation and does not touch request parsing, TLS, or auth paths.
Level of scrutiny
Medium-high. Bun.serve routing is core user-facing API surface, and this changes what Bun.serve() accepts/rejects: /:id:verb and /:from-:to now throw where they previously registered (silently misbehaving), and /v1:2/:id / /a:x/:x now register where they previously threw. The author surveyed Express 4/5, Fastify, Hono, and koa-router at alii's request, and the chosen behavior matches Express 5 / path-to-regexp 8 for the throw case — but alii has not explicitly confirmed the choice.
Other factors
- The four
comment-copinline notes fire on comments that state a cross-file invariant ("same grammar asHttpRouter::add()"), one of which is a pre-existing comment that merely moved. I read them as false positives, but they are technically outstanding. - No bugs from the multi-agent hunt; I traced the new C++ loop against the leading-
/, empty-param-name (/:), and no-trailing-slash cases and it produces one identifier per router capture. The old code dropped the empty identifier for/:/, which caused the/anon/:/:idshift the PR description shows; the new code keeps it, matching the router. - CI green per the status comment (only unrelated darwin agent cancellations).
- Given the API-design decision and the open maintainer thread, a human sign-off is appropriate here rather than an automated approval.
|
Updated 4:14 AM PT - Aug 16th, 2026
❌ @robobun, your commit 0b359bf has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37983That installs a local version of the PR into your bun-37983 --bun |
Problem
Bun.serve({ routes })buildsreq.paramswith shifted values when a route key has a:that is not the first byte of a segment:"/files:batchGet/:id"served/files:batchGet/123gives{ batchGet: "123", id: "" }instead of{ id: "123" }"/v1:beta/:name/x"gives{ beta: "bob", name: "" }"/n/:id:undelete"gives{ undelete: "42:undelete" }, and also matches/n/42packages/bun-uws/src/HttpRouter.h,add()/executeHandlers()) splits on/and captures a segment only when its first byte is:.files:batchGetis a literal;:id:undeleteis one whole-segment parameter.src/jsc/bindings/ServerRouteList.cpp,finishCreation) took every:in the key as the start of a name, so it produced more names than the router captures, andparamsObjectForRoutefills names positionally fromreq->getParameter(i)(out of range reads as"").validate_route_name(src/runtime/server/ServerConfig.rs) used a third variant of the same scan, so it rejected"/v1:2beta/:id"(literal segment "starting with a digit") and"/a:x/:x"(as a "duplicate") while accepting the keys above.Fix
ServerRouteList.cpp: build the identifier list per segment with the router's rule (segment starts with:), so there is exactly one name per captured value. This is the line that fixes the reported output.ServerConfig.rs:validate_route_nameuses the same split. The leading-digit and duplicate checks now only look at real parameter names, and a parameter segment containing a second:(":id:undelete",":from-:to") throwsInvalid route "...". A route parameter must span the whole path segment, ..., since the router can only capture the whole segment and there is no correct params object for such a key. This is the one behavior change for previously accepted keys; those keys never worked as written. The check stays invalidate_route_name(handler routes), which is where the existing parameter-name rules live; staticResponse/HTML routes have no params object and are unchanged./:id:undeleteat registration; Fastify and Hono accept it as one parameter literally named"id:undelete"; Express 4 splits it intoid: "4",undelete: "2". Express and Fastify support/:from-:toas two parameters, which Bun's router cannot match, so throwing now keeps that spelling available for a later addition instead of giving it a different meaning. A:inside a literal segment is a parameter in Express/Fastify (they spell a literal colon\:/::) but a literal in Bun's router, Hono's TrieRouter and PatternRouter; this PR keeps it literal because static routes already match such paths today.packages/bun-types/serve.d.ts:ExtractRouteParamsonly extracts names preceded by/, matching the runtime (BunRequest<"/files:batchGet/:id">["params"]is{ id: string }, previously{ batchGet: string; id: string }). Checked with tsc and tsgo; existing fixtures are unaffected.docs/runtime/http/routing.mdx: one paragraph stating the rule.test/js/bun/http/bun-serve-routes.test.ts: new':' that does not start a segment is literal textandroute parameter validation uses the segment grammarblocks. 9 of the new tests fail on the released 1.4.0 (shifted params, missing throw, and the digit / duplicate false rejections); the whole file (68 tests) passes with this change.test/integration/bun-types/fixture/serve.ts: addedexpectTypecases for the literal-colon keys;bun test test/integration/bun-types/bun-types.test.tstype checks pass.cargo clippy -p bun_runtime,cargo fmt --check, clang-format check, and the byte-search source lint are clean.Background
"/users/:id"is registered verbatim with uWS'sHttpRouter, which stores it as a tree of path segments. While matching a request, each segment of the request path is compared against the tree; a:node matches any non-empty segment and pushes that segment onto a parameter stack. The handler only sees that stack, indexed by position, throughreq->getParameter(i).ServerRouteListis the JS-side companion: for each route key it precomputes the list of parameter names and a JSCStructurefor the params object, and on each request it fills that object by position from the router's stack. The two sides agree only if both derive the same number of parameters from the key, which is what this change makes true.Repro before / after
/files:batchGet/123{"batchGet":"123","id":""}{"id":"123"}/v1:beta/bob/x{"beta":"bob","name":""}{"name":"bob"}/anon/a/b{"id":"a"}{"":"a","id":"b"}"/n/:id:undelete"and"/range/:from-:to"previously registered silently; they now throw aTypeErrorfromBun.serve()."/v1:2beta/:id"and"/a:x/:x"previously threw (digit / duplicate) and now register with params{ id }/{ x }.Survey of other routers (Express 4.22, Express 5.2, @koa/router 15.7, Fastify 5.11 / find-my-way 9.7, Hono 4.13); each route registered on its own, then probed
Hono per router (its default picks RegExpRouter here):