Conversation
Route patterns were derived by slicing the project-root-relative path by abs_root.len() - root.len(), which assumes every router root is inside the project root. A root like "../web/pages" (a sibling package in a monorepo) tripped the has_prefix debug assertion in init_empty, and in release builds produced wrong route patterns (every request 404s) or an index underflow panic when the resolved root was shorter than the project root. Derive the pattern from the path relative to the router type's own abs_root instead.
|
Warning Review limit reached
Next review available in: 3 minutes 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 (1)
WalkthroughFrameworkRouter's route scanning now computes relative paths from each Type's abs_root instead of the router's root, altering values passed to syntax error and collision error callbacks; init_empty asserts abs_root is absolute. A new test suite validates router roots located outside the project root. ChangesFrameworkRouter Path Computation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs — the simplified relative-path derivation looks correct and the tests are solid — but this reworks how bake derives route patterns and changes what path string is shown in router syntax errors (now abs_root-relative with a leading / instead of project-root-relative), so a bake maintainer should confirm that display change is intended.
Extended reasoning...
Overview
This PR touches src/runtime/bake/FrameworkRouter.rs (scan_inner and init_empty) and adds two integration tests to test/bake/framework-router.test.ts. The core change replaces the old two-step route-pattern computation (path relative to project root, then slice off abs_root.len() - root.len() - 1 bytes) with a direct relative_normalized_buf against the type's own abs_root. The has_prefix(abs_root, root) debug assertion is removed since the invariant it guarded is no longer needed. The route-collision error path now recomputes the project-root-relative display path on demand via path_buffer_pool::get(), and the syntax-error path now passes the exact parsed string (with unmodified cursor_at) to on_router_syntax_error.
Security risks
None identified. The scan still walks only under abs_root (no user-controlled traversal is introduced), the arithmetic that could underflow is removed rather than added, and no external input reaches new sinks. The change strictly narrows the surface where slice indexing can go wrong.
Level of scrutiny
Medium. The diff is small (~40 native LOC net) and the new logic is simpler and easier to prove correct than what it replaces — every scanned file is under abs_root, so relativizing against abs_root cannot produce .. and the leading-/ prefix is unconditionally correct. The two new subprocess tests exercise both the sibling-root (previously wrong patterns → 404) and ancestor-root (previously underflow panic) shapes end-to-end through Bun.serve({ app }), covering static and dynamic routes.
Other factors
There is an intentional, user-visible behavior change on the error path that isn't covered by tests: on_router_syntax_error (which DevServer forwards to TinyLog::print and JSFrameworkRouter embeds in its AggregateError) now receives the abs_root-relative path with a leading / (e.g. /foo/[bad.tsx) instead of the project-root-relative path without one (e.g. pages/foo/[bad.tsx). The PR description calls this out and it fixes an off-by-one in the old cursor shift, but it does change what users see in "is not a valid route" messages for the common in-project-root case. The collision-error path preserves the old project-root-relative labeling. Given bake is an actively-developed framework surface and this is a semantic change to route derivation plus a UX change to error output, I'd like a maintainer familiar with bake to sign off rather than shadow-approving.
|
The syntax-error display change is intentional, to expand on the note in the description: Collision errors keep the project-root-relative label: they have no cursor, and the handler prints the other colliding file project-relative on the next line, so both lines stay comparable. |
![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/framework-router.test.ts`:
- Around line 129-164: The current router tests in framework-router.test.ts only
cover successful sibling and parent directory routing, but the PR also changes
the user-facing labels for syntax and collision errors. Add out-of-root fixtures
in the same test area that exercise a malformed route and a route collision,
then assert the exact caret/label output for each error path. Use the existing
run(String(dir)) style and keep the new assertions anchored to the
framework-router behavior so the changed error labels are covered by automated
tests.
🪄 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: d80b815a-b7f1-47a8-ae84-56474d42eba1
📒 Files selected for processing (2)
src/runtime/bake/FrameworkRouter.rstest/bake/framework-router.test.ts
There was a problem hiding this comment.
I didn't find any issues, but this reworks how scan_inner derives route patterns and changes the user-visible syntax-error label (now router-root-relative instead of project-root-relative even for in-root cases), so it's worth a human sign-off on that behavior change.
Extended reasoning...
Overview
Two files touched:
src/runtime/bake/FrameworkRouter.rs:init_emptydrops thehas_prefix(abs_root, root)debug assertion in favor ofis_absolute(abs_root);scan_innernow derives each file's route pattern directly from its path relative to the router type'sabs_rootinstead of computing it relative to the project root and slicing offabs_root.len() - root.len() - 1bytes. The syntax-error path no longer offsetslog.cursor_atand reports against the exact string the parser saw; the collision-error path recomputes a project-root-relative display label on demand viapath_buffer_pool::get().test/bake/framework-router.test.ts: three new spawned-subprocess tests covering a sibling router root, a parent-directory router root, and the syntax/collision error labels for an out-of-root router root.
Security risks
None identified. This is startup-time filesystem route discovery; the paths involved come from the resolver's directory walk under abs_root, not from untrusted network input. No auth, crypto, or permission surfaces are touched.
Level of scrutiny
Moderate. The core diff is a ~30-line simplification that removes brittle length arithmetic, and I traced that the new computation produces the same rel_path as the old code for the previously-working in-root case (both yield /<path-under-abs_root>). However, this is the route-pattern derivation for Bun.serve({ app })'s bake router, and it deliberately changes the user-visible syntax-error label for all roots (e.g. "/[oops.ts" instead of "pages/[oops.ts"). The author's rationale — TinyLog.cursor_at indexes into the parsed string, and the old offset translation underflows for ancestor roots — is sound, but it's a design call a maintainer should confirm rather than a pure bugfix.
Other factors
- The bug hunter found no issues.
- CodeRabbit's one review request (negative coverage for the changed error labels) was addressed in 7856565 and the thread is resolved.
path_buffer_pool::get()on the collision path matches existing repo idiom.- Test coverage is solid: static + dynamic route resolution for both out-of-root shapes, plus exact-string assertions on both changed error labels; existing in-root tests are untouched.
- No CODEOWNERS entry matches
src/runtime/bake/.
|
CI status for 7856565 (build 67748): every red lane is a test this PR does not touch, and each of them is currently red on unrelated PRs too.
|
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-01, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What does this PR do?
Bun.serve({ app })registers no usable routes when afileSystemRouterTypes[].rootresolves outside the project root, which is the normal monorepo shape (root: "../web/pages"while serving fromapps/api).Reproduction (cwd is
/repo/apps/api, routes live in/repo/apps/web/pages):panic: assertion failed: strings::has_prefix(&ty.abs_root, root)(FrameworkRouter.rs,init_empty)404 Not Found(in the example above,pages/index.tsis registered as/pagesinstead of/).root: ".."), release builds crash instead:panic: range start index 18446744073709551611 out of range for slice of length 14Cause
FrameworkRouter::scan_innercomputed each file's path relative to the project root, then sliced offabs_root.len() - root.len() - 1bytes to get the route pattern. That arithmetic is only meaningful when the router root is inside the project root;init_empty's debug assertion documented the assumption but nothing validated user input against it. For a sibling root the slice lands mid-path (wrong patterns, so 404s); for an ancestor root the subtraction underflows (panic).Fix
Derive the route pattern directly from the file's path relative to the router type's own
abs_root(the directory the scan walks), with no dependence on the project root. The project-root-relative path is still used to label files in the route collision error, computed on that error path. Thehas_prefixassertion is gone because the invariant it asserted is no longer required by anything.Route syntax errors now report against the same string the pattern parser saw, since
TinyLog.cursor_atis an index into it; the previous code shifted the cursor byabs_root.len() - root.len(), which is meaningless for out-of-root roots.Tests
test/bake/framework-router.test.tsgains two cases (sibling root, ancestor root) that spawnBun.serve({ app })and assert both a static and a dynamic route resolve with the expected bodies.Existing coverage over in-project roots is unchanged:
test/bake/dev/esm.test.ts(17 pass) and the rest offramework-router.test.ts.