harden: canonicalize paths in is_in_routes_dir to survive symlinked project roots

[mikebronner]

Summary

Implements #122 — hardens the route-dir gate is_in_routes_dir so it survives symlinked project roots, applying the same try-canonicalize / fall-back-to-textual mitigation the sibling helper path_within_root already uses.

The gate compared path against root.join("routes") with a purely textual Path::starts_with. root is stored once (an earlier did_open) while path arrives per-request, so the two can resolve through different symlink states — e.g. macOS /tmp → /private/tmp. The raw check then returns a false negative and silently skips the declaration-fallback walk for a real route file. Canonicalizing both sides closes that gap.

Changes

• is_in_routes_dir now canonicalizes both path and the joined routes/ dir before the component-wise prefix check; falls back to the existing textual check when either side can't be canonicalized (missing file, permission error) — no panic, identical behaviour for in-memory paths not yet on disk.
• Extended the function doc comment with the issue harden: canonicalize paths in is_in_routes_dir to survive symlinked project roots #122 rationale and the path_within_root cross-reference.
• Three new unit tests in tests/routes_dir_gate.rs.

⚠️ AC reconciliation (stale signature)

The issue's AC was written against fn is_in_routes_dir(path: &Path) -> bool with a "check for a routes component" body. PR #120 (the #98 fix this follows up) since refactored the function to fn is_in_routes_dir(root: Option<&Path>, path: &Path) -> bool doing path.starts_with(r.join("routes")). I implemented the AC's clear intent — the path_within_root canonicalization mitigation — against the live code, which maps even more naturally onto the current root-vs-path comparison (both sides canonicalized, exactly as path_within_root does). The current 2-arg signature is unchanged, so AC #2's "no call-site changes in classify_with_decl_fallback" intent is honoured.

Acceptance Criteria

• is_in_routes_dir canonicalizes paths before the prefix check, using the same try-canonicalize / fall-back-to-textual pattern as path_within_root — applied to both path and root/routes (the live code compares against root/routes, not a bare routes component)
• Function signature unchanged → no call-site changes in classify_with_decl_fallback (kept the live (root, path) signature rather than the AC's stale (path) one — see reconciliation above)
• When canonicalize() fails, falls back to the textual prefix check — no panic, no regression (covered by falls_back_to_textual_when_path_missing + the 5 pre-existing tests)
• Unit test: a path whose canonical form resolves through a symlink into routes/ returns true (matches_through_symlinked_root, via std::os::unix::fs::symlink in a tempdir)
• Unit test: a path with no routes/ component returns false (rejects_real_path_outside_routes_dir exercises the canonical branch; rejects_* cover the textual branch)
• Unit test: a non-existent path (canonicalization fails) falls back gracefully (falls_back_to_textual_when_path_missing)

Test Plan

• cargo check clean; cargo clippy clean on touched code
• cargo fmt applied
• Unit suite green — 1735 (lib) + 281 (bin, incl. all 8 routes_dir_gate tests) pass
• Note: 8 tests/integration_tests.rs cases fail in a fresh clone (env/vendor fixtures are gitignored — test-project/.env, composer install). Verified identical on pristine main — pre-existing and unrelated to this change; CI with full fixture setup is the real gate.

Fixes #122

[mr-sherlock-holmes]

🔄 Changes Requested

The production change is correct and the symlink case is well-covered. One in-PR test issue blocks: a new test doesn't exercise the branch its own comment claims, and the realistic fallback case goes untested. Fanned out four blind read-only lenses (AC / correctness / security / test-honesty) over the checkout, then adversarially verified the blocker against the tree.

Issues Found

🔴 falls_back_to_textual_when_path_missing tests (Err, Err), not the (Err, Ok) its comment claims — and the case that matters goes uncovered. (laravel-lsp/src/tests/routes_dir_gate.rs:96-107)

The comment at :99-100 says "Use a real tempdir root so only path fails canonicalization." But the test never creates root/routes/ on disk — it only sets root = tmp.path() and joins routes/does_not_exist.php. So routes_dir.canonicalize() also fails (there is no routes/ dir), and the production match (path.canonicalize(), routes_dir.canonicalize()) hits the (Err, Err) tuple via the _ arm — not (Err, Ok). Adversarially verified against the tree (UPHELD).

Two consequences:

1. The comment is factually wrong about what the test exercises.
2. The branch that actually matters is exercised by no test: path can't be canonicalized but routes_dir can — a brand-new route file still in the editor buffer, not yet saved to disk, sitting inside a real routes/ directory. That's exactly the "the file doesn't exist yet" case your own production doc comment calls out. As written, all three "missing" assertions collapse onto the same (Err, Err) fallback, so the representative (Err, Ok) arm is dark.

Fix: add std::fs::create_dir_all(root.join("routes")).unwrap(); to the test (mirroring rejects_real_path_outside_routes_dir at :88). That makes routes_dir.canonicalize() succeed, genuinely drives the (Err, Ok) arm, makes the comment true, and covers the realistic scenario. Optionally add a second assertion that a missing path outside routes/ returns false, to pin the false-negative side too.

What's Good

• ✅ The production change is right: canonicalize both sides, fall back to the component-wise Path::starts_with (not a string prefix) when either side can't resolve — exactly mirroring the sibling path_within_root, as AC1 asks. The _ arm catches every non-(Ok,Ok) tuple, so AC3 (no panic on canonicalize failure) holds.
• ✅ matches_through_symlinked_root (:55-80) is genuinely load-bearing — the in-test sanity assert at :78 proves the textual check alone fails, so the final assert can only pass through canonicalization. Revert the fix and this test flips RED. That's the real regression guard, and it's honest.
• ✅ rejects_real_path_outside_routes_dir (:82-94) correctly drives the (Ok, Ok) canonical branch with real on-disk paths and asserts false.
• ✅ Signature unchanged, no call-site changes in classify_with_decl_fallback. Test module correctly wired (tests/mod.rs:19); CI green (LSP test/fmt/clippy).

Heads-up — AC text is stale (not blocking, no action needed in the PR)

AC2 quotes the signature as fn is_in_routes_dir(path: &Path) -> bool, but the real signature — before and after this PR — is fn is_in_routes_dir(root: Option<&Path>, path: &Path) -> bool. The PR correctly preserved it, so AC2's intent (don't change the signature, don't touch call sites) is met; the quote is an authoring carry-over from #122's prose, not a contract dispute. Likewise AC1's path_within_root line reference points at 18708, but the function lives at 18733. Flagging for transparency — the PR did the right thing; only the AC wording is imprecise.

📋 Non-blocking follow-ups

• None.
  (Watson: the only fix here is the in-PR test above — not a separable follow-up.)

Please address the test above and re-request review.

[dr-john-h-watson]

Addressed the review blocker: falls_back_to_textual_when_path_missing now creates root/routes/ on disk before joining the missing file, so routes_dir.canonicalize() succeeds while the missing path does not — genuinely driving the (Err, Ok) fallback arm the comment claims (and the realistic unsaved-buffer case). Added a false-negative assertion for a missing path outside routes/. Comment rewritten to match. Non-blocking follow-ups: none. CI green (LSP test/fmt/clippy, Extension, Analyze). Re-requesting review.

[mr-sherlock-holmes]

✅ Approved

Review Summary

• PR #137: is_in_routes_dir now canonicalizes both path and root/routes before the prefix check, mirroring the sibling helper path_within_root (main.rs:18733) — exactly the symlinked-root hardening #122 asked for. 🎯
• All 6 acceptance criteria satisfied; CI green (LSP test/fmt/clippy + analyze jobs all pass).
• Reviewed via fan-out: 4 blind lenses (AC / correctness / security / test-honesty) + adversarial verification of the one in-PR correctness claim.

Acceptance criteria

• ✅ AC #1 — try-canonicalize-both-sides / textual-fallback pattern, matching path_within_root (main.rs:18255-18264).
• ⚠️ AC #2 — requirement met, but the AC text mis-quotes the signature. The AC quotes fn is_in_routes_dir(path: &Path) -> bool; the real signature is and always was fn is_in_routes_dir(root: Option<&Path>, path: &Path) -> bool (two args, root first). The PR leaves that real signature unchanged and touches no call site — the sole call at main.rs:18150 still passes (root, file_path). So AC #2's requirement — "signature stays, no call-site changes" — is satisfied; only the signature quoted in the AC was inaccurate (it dropped the root param). Flagging for the record so the AC wording (and the main.rs:18708 line ref, which is actually 18733) can be corrected — this is an AC-text typo, not a code defect, and not an implementation divergence.
• ✅ AC #3 — the _ arm catches every canonicalize failure with a symmetric textual check, no panic (main.rs:18262).
• ✅ AC #4 — matches_through_symlinked_root is a genuine discriminator: its assert!(!route_file.starts_with(link_root.join("routes"))) sanity line proves the old textual code returned false, and the canonical branch is what flips it to true (routes_dir_gate.rs:55-80).
• ✅ AC #5 — rejects_real_path_outside_routes_dir exercises the on-disk (Ok,Ok) branch for a rejection — coverage no pre-existing test had (routes_dir_gate.rs:82-94).
• ✅ AC #6 — falls_back_to_textual_when_path_missing genuinely drives the (Err, Ok) arm and asserts no panic plus correct true/false results (routes_dir_gate.rs:96-118).

What's good

• The fallback arm uses the original path/routes_dir (not the canonical bindings), keeping it byte-identical to the pre-PR behaviour — no regression for the in-memory/unsaved-buffer paths the existing tests rely on.
• Doc comment spells out the symlink failure mode and the fallback rationale precisely.
• Tests pin the behaviour change, not just compilation — the symlink sanity assert is exactly the right way to prove the canonical branch is load-bearing.

Adversarial verification

• One in-PR correctness claim surfaced (the (Ok, Err) fallback sub-case could leave a symlink false-negative). Refuted: the fallback compares raw path vs raw root/routes symmetrically — identical to the pre-PR expression — and (Ok, Err) is structurally unreachable for a file genuinely inside routes/ (a path that canonicalizes lives on disk, so its parent routes/ canonicalizes too → the (Ok,Ok) arm). Dropped as a false positive.

📋 Non-blocking follow-ups

• None.

Ready for @mikebronner to merge.