Folio find-references: is_in_routes_dir over-broad (mount under routes/ yields no references) - #140
Conversation
… the decl walk `is_in_routes_dir` used a `starts_with` prefix check, so a Folio mount under `routes/pages/about.php` matched the gate and was routed into the route-declaration fallback walk — bypassing the Folio branch in `classify_with_decl_fallback` and silently returning no references. Tighten the gate to an immediate-parent equality check: a file qualifies only when its parent directory IS `<root>/routes`. Direct children (`routes/web.php`, `routes/api.php`) still resolve; anything nested deeper falls through to its own resolver. The symlink-resilient canonicalization (issue #122) is preserved, now applied to the parent rather than the path. Root-`None` still returns false — the broad `components().any("routes")` fallback removed by #98 is deliberately NOT reinstated. Add unit tests: api.php (true), Folio page under routes/pages/ (false), a routes/ dir outside the project root (false), and a both-missing textual-fallback case. Docs updated to match the new semantics. Fixes #105
There was a problem hiding this comment.
✅ Approved
This is the clean reconciliation that the closed PR #124 was asking for. PR #124 was built before main advanced and would have regressed #137; this branch is built on top of #137 (1af808b) and resolves the exact tension I flagged there: it keeps #137's canonicalization + on-disk textual fallback, but swaps the comparison from starts_with to path.parent() == so the Folio-under-routes/ page (#105) is rejected without re-breaking the symlinked-root case.
Review Summary
Reviewed the diff against #137 base (git diff 1af808b..f6ae5bf, 2 files +86/−35): the is_in_routes_dir rewrite, its single call site, and the routes_dir_gate test module. (Lens fan-out was unavailable in this runtime — reviewed inline against the same criteria.)
Every acceptance criterion is met:
- #1 —
is_in_routes_dir(root: Option<&Path>, path: &Path);Some(root)returns true only whenpath.parent() == <root>/routes(main.rs:18229). (The AC phrases the signature as(path, root); the codebase uses(root, path)from the prior merged fix — substantive requirement met, parameter order is the existing repo convention.) - #2 —
None => return false; the broadcomponents().any("routes")fallback is deliberately not reinstated, preserving #98 (main.rs:18230-18232). - #3 — the single call site passes the already-in-scope
rootdown (main.rs:18141). - #4 —
routes/pages/about.phpnow returnsfalse(parent/project/routes/pages≠/project/routes), so the Folio branch inclassify_with_decl_fallbackis reachable for these pages. - #5 —
routes/web.php/routes/api.php(direct children) still returntrue→ decl-walk preserved. - #6 — tests cover web.php/api.php true,
routes/pages/about.phpfalse, routes-dir-outside-root false, root-Nonefalse.
What's good
- Correctness:
parent ==is stricter than the oldstarts_with, so it tightens #105 while still rejecting every #98 case (vendor routes, nestedroutescomponent,routesXfalse-prefix sibling). The canonical arm preserves #137's symlink survival; the(Ok,Err)/(Err,Ok)arms fall to the uncanonicalizedparent ==comparison — the same on-disk fallback semantics #137 shipped, not a new defect. - Tests are honest, not tautological:
rejects_folio_page_nested_under_routesis a real regression guard — it would pass under the newparent ==logic but fail under the oldstarts_with. All of main's #98/#122/#137 gate tests are carried over (package-vendor, false-prefix sibling, symlinked-root via real tempdir+symlink, real-path-outside), and the textual-fallback arm is exercised with genuine on-disk tempdir setups including the both-missing case. - No merge conflict (
MERGEABLE), and CI'sLSP — test, fmt, clippyjob is green.
Non-blocking follow-ups
- None.
Ready for @mikebronner to merge.
Summary
Implements #105.
is_in_routes_dirused astarts_withprefix check, so a Folio page mounted underroutes/pages/about.phpmatched the gate and was routed into the route-declaration fallback walk — bypassing the Folio branch inclassify_with_decl_fallbackand silently returning no references. This tightens the gate to an immediate-parent equality check.Changes
is_in_routes_dirnow qualifies a file only when its immediate parent directory equals<root>/routes(was:starts_with(<root>/routes)). Direct children (routes/web.php,routes/api.php) still resolve; anything nested deeper (a Folio mount underroutes/pages/) falls through to its own resolver.Nonestill returnsfalse; the broadcomponents().any("routes")fallback removed by sibling fix fix: is_in_routes_dir matches any 'routes' path component, not just project-root routes/ #98 is deliberately not reinstated.is_in_routes_dirdoc comment and the call-site comment inclassify_with_decl_fallbackto match the new semantics.laravel-lsp/src/tests/routes_dir_gate.rs.Acceptance Criteria
is_in_routes_diracceptsroot: Option<&Path>; whenrootisSome, it returnstrueonly when the file's immediate parent equals<root>/routes/—routes/web.php→ true,routes/pages/about.php→ falserootisNone,is_in_routes_dirreturnsfalse; the broadpath.components().any("routes")fallback is not retained (would regress fix: is_in_routes_dir matches any 'routes' path component, not just project-root routes/ #98)classify_with_decl_fallbackpasses the already-availablerootparameter down tois_in_routes_dir(confirmed; unchanged from fix: is_in_routes_dir matches any 'routes' path component, not just project-root routes/ #120)name('...')call in a Folio page mounted underroutes/pages/resolves to allroute('...')usages — the Folio branch is no longer bypassed for these filesroutes/web.phpandroutes/api.php(direct children ofroutes/) continue to resolve correctlyis_in_routes_dir: root-known →truefor<root>/routes/web.phpand<root>/routes/api.php,falsefor<root>/routes/pages/about.php(Folio page) and aroutes/dir outside the project root; root-None→falseTest Plan
cargo test --all-features: 2110 tests green (1743 + 287 unit, 80 integration), 0 failuresrejects_folio_page_nested_under_routes,rejects_routes_dir_outside_project_root, api.php case added tomatches_project_root_routes_dir, both-missing textual-fallback casecargo fmt --checkcleancargo clippy --all-targets -- -D warningscleanFixes #105