feat: Folio page routing support (goto / completion / find-references) - #91
Conversation
Surface Laravel Folio pages — filesystem-derived routes that never call
Route:: — across the LSP route features. A new folio_discovery module finds
the project's Folio mounts (default resources/views/pages plus any
Folio::path('...') registered in a service provider, with chained ->uri()
and ->name() prefixes), derives each page's URI from its filename (handling
static, dynamic [id], catch-all [...slug], and trailing-index segments),
and reads each page's explicit name('...') route name.
Named pages are injected into the shared RouteIndex as ordinary
RouteDefinitions in build_route_index, so goto-definition, route-name
completion, route-not-found diagnostics, and named-route find-references
all resolve Folio routes with no per-feature changes. Discovery is gated on
folio_in_use (composer.json or a Folio:: facade reference) so non-Folio
projects pay nothing, and a conventional route of the same name still wins.
Fixes #57
There was a problem hiding this comment.
🔄 Changes Requested
Solid module overall — clean separation, genuinely good doc comments, and the URI derivation is correct and well-tested. Three gaps block it, two against the acceptance criteria and one security boundary. CI is green; these are correctness/coverage issues CI can't catch. (AC numbering follows the refined ## Acceptance Criteria set.)
Issues Found
1. 🛑 Security — Folio::path('...') value is walked without a containment check (folio_discovery.rs:146 → :168, walked at :236, read at :252)
rel is captured raw from the provider source and passed straight to root.join(rel) with no is_absolute() check, no normalize_path, and no "stays under root" verification. Rust's Path::join replaces the base on an absolute argument, so a crafted project containing Folio::path('/etc') — or Folio::path('../../something') — makes the LSP WalkDir an arbitrary directory and fs::read_to_string every .blade.php it finds outside the project the user opened. Those contents then flow into the route index. This breaks the containment convention the repo already follows in route_discovery.rs::resolve_path_argument (~:1608), which guards is_absolute() and applies normalize_path. Apply the same guard here: reject absolute/escaping paths (or normalize and verify the result is under root) before storing FolioMount::directory.
2. AC #1 not met — Folio::path(resource_path('...')) is silently missed (FOLIO_PATH_RE at :60, fallback at :129-136)
FOLIO_PATH_RE requires a quote immediately after Folio::path(, so the helper-call form never matches. That form is the Laravel-scaffolded default — php artisan folio:install writes Folio::path(resource_path('views/pages')) into FolioServiceProvider. Consequences:
- Stock install: works only by luck — discovery finds nothing and the hardcoded fallback happens to equal
resources/views/pages. - Custom dir (
resource_path('views/admin')): silently resolves to the wrong directory. - Mixed provider (one string-literal mount + one
resource_pathmount): theresource_pathmount is dropped with no fallback (mountsis non-empty so:129never fires). Your own testparse_folio_mounts_custom_path(tests.rs:94-102) encodes this drop as expected behavior, anddiscover_folio_routes_honours_non_default_mountdeliberately uses a string literal to dodge it.
AC #1 is "discovered from service providers" — the canonical provider syntax has to be handled. At minimum parseresource_path('...')/base_path('...')helper wrappers; add a test that the resolved directory is correct for the helper form.
3. AC #4 not met — find-references only half-covers Folio routes (main.rs collect_declaration_locations ~:18031-18088, is_in_routes_dir gate ~:17866)
Verified leg by leg:
- ✅ Call-site usages of
route('folio.name')are found — the symbol index ingestsroute_refsfrom blade-embedded PHP, so this leg works for free as the doc comment claims. - ❌ Declaration is never returned. With
includeDeclaration=true,collect_declaration_locationswalks onlyroutes/*.phpand recognizes only->name(...)chains — never the Folio.blade.phppage nor the barename('...')helper. Conventional routes return declaration+usages; Folio routes return usages-only. - ❌ Can't invoke from the page.
classify_with_decl_fallbackgates onis_in_routes_dir(file_path); a page underresources/views/pages/fails it, so find-references triggered from inside a Folio page'sname('...')returns nothing.
AC #4 says "locates all usages of a Folio page or its named route." Return the page as a declaration and let the cursor trigger from inside the page'sname()call. There's also no test for find-references on a Folio route — add one with the fix (thesource_filesinsertion at:275is the relevant mechanism and is currently unasserted).
What's Good
derive_uri/rewrite_segmentcorrectly handle static, dynamic[id], catch-all[...slug], and trailing-indexcollapse — with exact-string unit tests for each. 👍- The named-only injection is the right call for goto/completion: unnamed pages aren't reachable via
route('...'), so omitting them from the name-keyed index is correct, not a gap (verified). - The equal-priority tiebreak genuinely makes conventional routes win —
RouteIndex::insertkeeps-first (route_discovery.rs:125-131) and injection runs after the conventional pass;inject_folio_routes_does_not_clobber_conventional_routeproves it. PAGE_NAME_REcorrectly excludes->name(,::name(, and theuse function ...\name;import.
📋 Non-blocking follow-ups
PAGE_NAME_REboundary class excludes>:\but not(or[, so a contrivedusort($a, name('cmp'))would false-positive —folio_discovery.rs:78. Low real-world risk; tighten if cheap.discover_folio_routesWalkDirusesmax_depth(12)and doesn't setfollow_links(false)explicitly — a symlink into a large real dir gets walked —folio_discovery.rs:236. Harden alongside fix #1.folio_in_use's facade-reference branch (Folio::in a provider) is untested — only thecomposer.jsonbranch is —folio_discovery.rs:92-94.
(Watson: address each in this PR, or open a tracked issue for the ones you won't — none get dropped.)
Please address the three blockers and re-request review.
Addresses Holmes's review of #57 — three blockers plus the three non-blocking follow-ups. Blockers: - Security: contain `Folio::path(...)` mounts to the project root. Raw paths were `root.join`'d and walked, so `Folio::path('/etc')` or `Folio::path('../../x')` turned the LSP into an arbitrary-file reader. `resolve_folio_path` now normalizes and verifies the result stays under root, rejecting absolute/escaping mounts (falls back to the default mount). Mirrors `route_discovery::resolve_path_argument`. - AC #1: parse the `resource_path('...')` / `base_path('...')` helper forms that `folio:install` scaffolds — previously only a bare quoted literal matched, silently dropping (or misresolving) the canonical provider syntax. The arg is now paren-balanced, so chained `->uri()`/`->name()` links survive the helper's inner parens. - AC #4: find-references now fully covers Folio routes. The backing `.blade.php` page is surfaced as the route declaration (via the injected index entry, blade-file-gated so conventional routes aren't double-counted), and `folio_name_at` lets the request trigger from a cursor on the page's own `name('...')` call. Follow-ups: - Tighten `PAGE_NAME_RE` to also exclude `(`/`[` boundaries. - `follow_links(false)` on both Folio WalkDir scans. - Test the `folio_in_use` facade-reference branch. Fixes #57
`cargo fmt` over the `laravel-lsp` crate — wrap a long `assert_eq!` and collapse a `.filter(...)` closure / `parse_folio_mounts(...)` call to the canonical layout. No behaviour change; satisfies the `cargo fmt --check` CI gate.
|
Addressed the three blockers from the review and all three non-blocking follow-ups — everything handled in this PR, nothing deferred. CI is green. Blockers
Non-blocking follow-ups (all fixed in-PR)
Folio module test count went 14 → 34; full |
There was a problem hiding this comment.
🔄 Changes Requested
All three round-1 blockers are genuinely fixed — verified leg by leg (details under What's Good). One new blocker surfaced this round: it pre-dated round 1 and I missed it, but it's a real runtime defect I can't approve around. CI is green; this is a concurrency/behaviour issue CI can't catch. (AC numbering follows the refined ## Acceptance Criteria set.)
Issues Found
1. 🛑 Correctness — folio_name_at does a full, uncached, synchronous filesystem walk on a Tokio worker thread, deviating from the repo's own async convention (main.rs:17866-17882 call site → folio_discovery.rs:342-351 body; chain: folio_in_use :89-117, discover_folio_mounts :125-142, discover_folio_routes :370-409)
classify_with_decl_fallback is an async fn invoked bare — no spawn_blocking — from the references, prepare_rename, and rename handlers (main.rs:19965 et al.). Inside it, the Folio branch (:17873) calls folio_name_at, which is entirely synchronous std::fs::read_to_string + walkdir::WalkDir:
discover_folio_routesre-runsfolio_in_use(readscomposer.json, walksapp/Providers/), thendiscover_folio_mounts(re-reads every provider file), then a fullWalkDirof the blade mount with aread_to_stringper page — and finally one moreread_to_stringfor the cursor file (:349).- It is not memoized — no Salsa query, no cache field. Every find-references / rename / prepare-rename request where the cursor is outside
routes/re-walks the entire project from scratch.
This blocks the Tokio worker thread for the whole walk, stalling all other concurrent async tasks (completion, hover, diagnostics) on that thread — tens to hundreds of file reads on a real project. It directly contradicts the convention this repo already established for exactly this work: rebuild_route_index wraps its walk in tokio::task::spawn_blocking (main.rs:14187) and cached_route_decls uses tokio::fs + an awaited cache (main.rs:4774). The new path does neither. The repo's convention wins — bring this path in line with it (wrap the call in spawn_blocking, and/or cache discover_folio_routes so the walk happens at most once; both patterns already live in this file). Per the dev decision protocol, lay the options out for Mike and let him pick the approach.
What's Good
The round-1 fixes are solid — each independently verified, not taken on faith:
- ✅ Security (round-1 blocker #1) fixed.
resolve_folio_pathnow appliesnormalize_paththenstarts_with(root)before storing a mount (folio_discovery.rs:278-281); absolute (/etc),../traversal, andbase_path('../outside')are all rejected, with tests attests.rs:138-154. BothWalkDircalls now set.follow_links(false)(:110,:384). The symlink and traversal vectors are closed. 👍 - ✅ AC #1 (round-1 blocker #2) fixed.
resolve_folio_pathhandles bare literals andresource_path(...)/base_path(...)helper wrappers via paren-balancing, so thephp artisan folio:installdefault (Folio::path(resource_path('views/pages'))) is now discovered. The previously-wrongparse_folio_mounts_custom_pathtest that encoded the drop as expected is corrected (now asserts both mounts resolve), andparse_folio_mounts_resolves_path_helpers(tests.rs:107-124) asserts the concrete resolved paths. - ✅ AC #4 (round-1 blocker #3) fixed. Find-references now returns the page declaration (
collect_declaration_locationsviafolio_decl,main.rs:18105-18136) and can be triggered from inside a Folio page'sname('...')call (folio_name_at, gated pastis_in_routes_dir), withfolio_name_at_resolves_cursor_on_the_name_call(tests.rs:395) covering the trigger. - ✅ AC #2, #3, #5 all met —
derive_uri/rewrite_segmenthandle static /[id]/[...slug]/ trailing-indexwith exact unit tests; Folio routes flow into the sharedRouteIndexso completion picks them up with no special-casing; the non-default-mount path is exercised end-to-end.
📋 Non-blocking follow-ups
- Find-references usages leg has no integration-level test — only the declaration leg (
folio_name_at) and the anchor are asserted. The wiring is correct and architecturally covered by the conventional-route reference tests, but an end-to-end assertion thatroute('folio.name')call-sites are returned would close AC #4's test gap —tests.rs:376-442. is_in_routes_dir(main.rs:17955) matches any path component namedroutes, so a Folio mount likeFolio::path('routes/pages')would never reachfolio_name_atand silently return no references. Low real-world risk (no Laravel convention puts Folio pages underroutes/); tighten to match only the project-rootroutes/if cheap.folio_name_atreads the LSP-supplied cursor path (folio_discovery.rs:349) without an explicitstarts_with(root)guard — containment is currently enforced transitively (the path must already be in the index, which only holds contained mounts). Correct today, but a direct guard would be defense-in-depth.parse_folio_mounts_rejects_traversal_pathscovers bare-string andbase_path('../…')traversal but notresource_path('../…'); thenormalize_path+starts_withcheck catches it, but it's inferred, not asserted — add one line —tests.rs:138-154.
(Watson: address each in this PR, or open a tracked issue for the ones you won't — none get dropped.)
Fix the blocker and re-request review.
The find-references / rename fallback resolved a Folio page's route name by
calling discover_folio_routes on every request — a full synchronous walkdir of
the blade mount with a read per page, run uncached on a Tokio worker thread.
That blocked the worker for the whole walk, stalling concurrent async tasks,
and contradicted the repo's own convention (rebuild_route_index walks inside
spawn_blocking; cached_route_decls uses an awaited cache).
Read the page→name mapping from the already-built in-memory route index
instead — the same source add_declaration_locations' Folio branch reads, built
once inside spawn_blocking — and read the cursor file via the editor buffer or
tokio::fs. The walk no longer runs on the request path. Containment is now
structural: the page text is read only after the index match succeeds, and the
index holds only mounts that stayed under the project root.
Split the resolution into pure, unit-tested helpers (folio_name_for_file,
cursor_on_page_name) and add a resource_path('../…') traversal-rejection case.
Refs: #57
|
Addressed round-2's blocker and its four non-blocking follow-ups — nothing dropped. Blocker — Non-blocking follow-ups
Library (1637) + binary (232) suites green; |
There was a problem hiding this comment.
✅ Approved
Review Summary
- Round-3 review of Folio page-routing support (goto / completion / find-references). All 5 acceptance criteria met; CI green (LSP test/fmt/clippy, wasm check, CodeQL all pass). (AC numbering follows the refined
## Acceptance Criteriaset.) - The round-2 blocker is genuinely fixed — verified leg by leg.
folio_route_name_for_cursor(main.rs:4798) now resolves the route name from the already-built in-memoryroute_indexviafolio_name_for_file, and reads only the single cursor file through asynctokio::fs— no synchronouswalkdir/std::fswalk on a Tokio worker thread. The full-project walk now happens at most once, insidespawn_blocking, when the index is built. The concurrency defect that blocked round 2 is gone. - AC conformance:
- AC #1 ✅ mount discovery from service providers, incl.
resource_path(...)/base_path(...)wrappers via paren-balancing, with safe-default fallback (folio_discovery.rs:125-142,:262-282). - AC #2 ✅ goto resolves static /
[id]/[...slug]viaderive_uri/rewrite_segment+ index injection (folio_discovery.rs:310-321,:438-456). - AC #3 ✅ completion offers Folio routes through the shared
get_all_route_namesover the injected index (main.rs:12364). - AC #4 ✅ find-references returns the page declaration (
collect_declaration_locationsFolio branch,main.rs:18135) and triggers from inside the page's ownname('...')(folio_route_name_for_cursor, gated pastis_in_routes_dir); call-site usages flow via the sharedroute_refsindex. - AC #5 ✅ tests cover static / dynamic / catch-all segments, named routes, and a non-default mount path (
tests.rs:11/17/23/52/297), plus traversal-rejection tests.
- AC #1 ✅ mount discovery from service providers, incl.
- Security guards from prior rounds confirmed still present and complete:
resolve_folio_pathappliesnormalize_paththenstarts_with(root)(folio_discovery.rs:262-282); bothWalkDirs set.follow_links(false). - The branch is behind
mainbut has no merge conflicts (mergeable: MERGEABLE); theBLOCKEDmerge state is just this review gate. Safe to merge once approved.
What's Good
- The round-2 fix is the right architecture — resolve from the in-memory index, never a re-walk. The doc comment at
main.rs:4784-4797documents the containment + non-blocking reasoning precisely, and it holds up under inspection. 👍 derive_uri/rewrite_segmenthandle static,[id],[...slug], and trailing-indexcollapse — each with exact-string unit tests.- Mount discovery now handles the
php artisan folio:installdefault (Folio::path(resource_path('views/pages'))) with concrete resolved-path assertions. - Path-traversal and symlink vectors are closed and tested (
tests.rs:138-154).
📋 Non-blocking follow-ups
- Folio rename is wired into the classifier but doesn't actually work, and carries a latent corruption path —
classify_with_decl_fallbackreturnsRoutefor a Folioname('...')cursor (main.rs:17906; its doc comment even claims rename reaches it), butprepare_rename→decl_range_at→cached_route_declsonly matches->name()chains, so it returnsOk(None)and Zed silently offers no rename. Andcollect_route_declaration_targets(main.rs:18293) got no Folio branch (unlikecollect_declaration_locationsat:18135), so a non-conformant client callingtextDocument/renamedirectly (bypassing theprepareRenamegate) would rewriteroute('...')call-sites but leave the bladename('...')declaration stale. Unreachable in Zed today and rename is out of this issue's scope — but either implement the Folio decl-rewrite or haveprepare_renamereturn an explicit unsupported-rename error instead of silentOk(None). - No completion-surface test —
inject_folio_routes_adds_named_routes_to_index(tests.rs:328) asserts the index entry, but nothing invokesget_all_route_names/the completion handler to assert a Folio route is actually offered. AC #5's enumerated coverage is satisfied; this would close the layer above it. - No find-references usages-leg test for Folio (carried from round 2) — declaration leg + cursor trigger are tested (
tests.rs:384/402);route('folio.name')call-site retrieval is covered for conventional routes (salsa_impl/tests.rs:684) but never bridged to a Folio page (tests.rs:383). matching_parenmis-scans PHP escaped quotes —Folio::path('foo\'bar')closes the quote early, the literal fails thefirst==lastcheck, and the mount is silently dropped (folio_discovery.rs:200-224). No security impact (containment still holds); a valid mount just disappears.- Cursor-file read lacks an explicit containment guard —
folio_route_name_for_cursor→document_or_disk_contentreads the file (main.rs:4808) relying only on the structural index-membership guard. Add an explicitstarts_with(root)for defense-in-depth. is_in_routes_diris over-broad — it matches any path component namedroutes(main.rs:17985), so a Folio mount underroutes/pageswould skip the Folio branch and silently return no references. Very low real-world risk (no Laravel convention puts Folio pages there).
Ready for @mikebronner to merge.
Summary
Implements #57 — surfaces Laravel Folio pages (filesystem-derived routes that never call
Route::) across the LSP's route features, reusing the existing route infrastructure.Approach
RouteIndexis the single hub every route feature reads (goto, completion, route-not-found diagnostics, named-route find-references). Rather than add Folio-aware branches to each handler, a newfolio_discoverymodule injects named Folio pages into that index insidebuild_route_index— so all route features resolve Folio routes with no per-feature changes.Changes
laravel-lsp/src/folio_discovery.rs:discover_folio_mounts— defaultresources/views/pages, plus anyFolio::path('...')registered in a service provider, with chained->uri(...)/->name(...)prefixes.derive_uri— derives a page's URI from its filename: static, dynamic[id]→{id}, catch-all[...slug]→{slug}, and trailingindex.blade.phpcollapse.extract_page_name— reads a page's explicitname('...')Folio helper (ignores->name(route chains and::name(static calls).inject_folio_routes— adds named pages to the sharedRouteIndexas ordinaryRouteDefinitions.folio_in_use(composer.jsonlaravel/folioor aFolio::facade reference) so non-Folio projects pay nothing.route_discovery.rs—build_route_indexcallsinject_folio_routesafter the conventional pass; a conventional route of the same name still wins (equal priority keeps the first-inserted entry).lib.rs— registers the new module.Acceptance Criteria
resources/views/pagesif not configured)..blade.phpfiles, handling static, dynamic ([id]), and catch-all ([...slug]) segments. (viaRouteIndexinjection —create_route_location_from_salsareads the index.)get_all_route_namesreading the index.)Test Plan
folio_discovery/tests.rs(URI derivation across segment kinds, name extraction, default + non-default mount discovery, prefix isolation, end-to-endRouteIndexinjection, conventional-route precedence).cargo fmt --checkclean.cargo clippy --all-targets -- -D warningsclean.cargo test— library (1626) and binary (232) suites green. (The 8 integration-test failures seen on a bare clone are pre-existing and environmental — they needtest-project/.env+ Composervendor/, which CI bootstraps; verified identical on the pristine base.)Notes / limitations
Folio::path('...')mounts are resolved;Folio::path(resource_path('...'))helper-wrapped paths are not (documented by test).name('...')are discovered with a URI but not added to the route index (they aren't reachable viaroute('...')).Fixes #57