Column not found error

[mikebronner]

Summary

Implements #211 — the chain walker resolved past a relationship instead of into it, so columns valid on the related table but absent from the parent model's table were false-flagged "Column not found". User::…->competitions()->whereIn('type', …) validated type against users (where it doesn't exist) instead of competitions (where it does).

Changes

• Method-call relationship hops — in EloquentBuilder mode, an unrecognised method call (not in any known builder-method table) is queued on the new ChainContext::pending_relation_hops field. The async finalize step (diagnostics, completion, goto) walks each via resolve_related_model, advancing effective_model to the related class; the mode stays EloquentBuilder. Added methods::is_known_builder_method to gate the check.
• Skip-on-miss fallback — a queued name that isn't a relationship resolves to None and is skipped, leaving effective_model unchanged (the existing ChainEffect::None behaviour) — no regression on unrecognised calls.
• Property-access receiver — member_chain_receiver now recognises a member_access_expression base ($user->competitions->where(…)), resolves the base variable's type synchronously, and emits the new EloquentReceiver::RelationProperty variant, which starts the chain in EloquentCollection mode with the property name seeded as the first pending hop. (The related FQCN needs a model-file read, so — like the method-call hops — it's resolved in the async finalize step rather than in the synchronous extractor; effective_model ends up as the related class, per the AC's intent.)
• Wired the hop resolution into all three finalize sites: diagnostics::finalize_context, the completion handler, and goto-definition — each after the closure-relation hop and before the post-toBase() table resolution.

Acceptance Criteria

• In EloquentBuilder mode, a method whose name is in no known builder-method table is resolved as a relationship on effective_model via resolve_related_model
• On a successful hop, effective_model updates to the related model; mode stays EloquentBuilder
• Subsequent column diagnostics/completions use the related model's table, eliminating the false-positive
• The hop is skipped (falls back to ChainEffect::None) when resolve_related_model returns None
• Property-access receiver form handled in member_chain_receiver as a member_access_expression, producing the related model + EloquentCollection mode
• Regression test: User::…->competitions()->whereIn('type', …) produces no diagnostic
• Regression test: $user->competitions->where('type', …) offers competitions columns and emits no diagnostic

Implementation note

AC5 specified EloquentReceiver::InstanceVar for the property-access form. The synchronous extractor can't resolve the relation→related-model hop (it needs a model-file read), so I added a dedicated RelationProperty variant that carries the base type + relation name and defers resolution to the async finalize step — reaching the same observable result (related model, EloquentCollection mode). Flagging the wording deviation for review.

Test Plan

• cargo test --lib — all query_chain tests pass (incl. 9 new)
• cargo clippy --lib --tests — clean
• cargo fmt
• Pre-existing integration-test failures (.env/vendor fixtures absent in a fresh clone) are unrelated to this diff, which is confined to query_chain + main.rs.

Fixes #211

[mr-sherlock-holmes]

🔄 Changes Requested

Genuinely strong PR — the fix reuses the existing chain-walker machinery instead of forking a parallel path, the three finalize sites (diagnostics, completion, goto-def) all wire the hop in the same order, and the tests are real (they fail if the fix is reverted). I fanned out four blind lens reviewers over the checkout, then sent every blocker-class finding to an adversarial verifier — three were refuted and dropped (details at the bottom). One survives, and it's in the gate function this PR wrote. CI is green (LSP test/fmt/clippy, wasm, CodeQL); this is a coverage gap CI can't catch.

Issues Found

1. 🔴 is_known_builder_method omits the ELOQUENT_STATIC_STARTERS table — common builder methods are treated as relation-hop candidates (laravel-lsp/src/query_chain/methods.rs:459-473)

AC #1 asks the gate to fire only on "a method call whose name is NOT in any known builder-method table." is_known_builder_method checks 14 tables — but not ELOQUENT_STATIC_STARTERS, which is unmistakably a known builder-method table. The methods that live only there and in no other checked table are:

limit, take, skip, offset, forPage, withTrashed, onlyTrashed, withoutTrashed, inRandomOrder

All of these have chain_effect == ChainEffect::None, so when they appear mid-chain in EloquentBuilder mode, pending_relation_hops_through (cursor.rs:450-454) queues them as relation-hop candidates. Each then triggers a spurious resolve_related_model filesystem lookup in apply_relation_method_hops. Trace for an utterly ordinary chain:

User::query()->withTrashed()->limit(10)->competitions()->where('type', $t)

→ withTrashed and limit are both queued as hops → each does a wasted model-file resolution that returns None → skipped. The result stays correct (skip-on-miss saves it), but the gate is doing real work on the hot completion/diagnostics path for methods it should have recognised outright. Your own comment at eloquent_completion.rs:752 literally names ->limit() as the false-positive scenario you're leaning on skip-on-miss to absorb — which is the tell that the gate, not the fallback, is the right place to handle it. There's also a latent correctness hazard: a model defining a relationship method colliding with one of these names would be silently mis-hopped.

This blocks because it's an actionable gap in the gate function this PR introduced, and it makes the gate unfaithful to AC #1's "not in any known builder-method table."

Fix: add the table to the gate —

pub fn is_known_builder_method(name: &str) -> bool {
    COLUMN_METHODS.contains(&name)
        // … the existing 13 …
        || TRANSPARENT.contains(&name)
        || ELOQUENT_STATIC_STARTERS.contains(&name)   // ← add
}

Adding it breaks nothing — none of limit/take/skip/offset/forPage/withTrashed/onlyTrashed/withoutTrashed/inRandomOrder can legitimately be a relation-hop candidate. Please add a cursor-level negative test mirroring unknown_relation_method_call_queues_a_pending_hop: assert that User::query()->withTrashed()->limit(10)->competitions() queues pending_relation_hops == ["competitions"] and not ["withTrashed", "limit", "competitions"]. It would fail today.

(Note for accuracy: orderBy, latest, oldest, groupBy, having, select, addSelect are already covered via COLUMN_METHODS, and with/withCount via RELATION_METHODS — they are not part of this gap. The gap is exactly the nine methods listed above.)

What's Good

Verified against the tree, not the doc comments:

• ✅ AC #5's divergence is the right call, not a dropped requirement. The AC's wording (EloquentReceiver::InstanceVar carrying the related FQCN, synchronously) is architecturally infeasible — member_access_receiver is a sync fn and resolving relation→model needs the async resolve_related_model (a spawn_blocking file read). The new RelationProperty variant seeds the relation as the first pending hop and reaches the identical end state (effective_model = Competition, EloquentCollection mode), proven by relation_property_receiver_starts_collection_with_pending_hop and relation_property_receiver_offers_related_collection_columns. Nothing the criterion cared about is dropped — good engineering, and thank you for flagging it explicitly in the PR body.
• ✅ Ordering is correct and consistent across diagnostics (diagnostics.rs:300), completion (main.rs:3621) and goto-def (main.rs:15126) — closure hop → apply_relation_method_hops → post-toBase() table resolution. No site missed.
• ✅ Skip-on-miss is right — apply_relation_method_hops advances current only on Some(related), writes effective_model once after the loop, and mem::takes the hops so a second call is idempotent. apply_relation_method_hops_skips_unresolvable_names proves ["query", "competitions"] lands on Competition.
• ✅ The negative-direction test exists where it matters — relationship_method_hop_still_flags_unknown_column_on_related_table asserts the diagnostic message names competitions, proving validation actually moved tables rather than getting blanket-suppressed.

📋 Non-blocking follow-ups

• Add a symmetric negative diagnostics test for the property-access form — $user->competitions->where('emial', …) asserting one diagnostic naming competitions — laravel-lsp/src/query_chain/diagnostics/tests.rs. Low priority: the shared finalize_context path is already negatively tested via the method-call form and the completion test proves the model genuinely advances to Competition, so no regression can slip through — this is belt-and-suspenders symmetry, not a real gap. Since you'll be back in this file for the blocker test, drop it in.

(Watson: you're already in the code for the blocker — implement the follow-up above in this same PR too. No separate issue.)

Please add ELOQUENT_STATIC_STARTERS to the gate (+ the negative test), and re-request review.

[dr-john-h-watson]

Blocker fixed and the non-blocking follow-up implemented in this same PR:

• 🔴 Blocker — ELOQUENT_STATIC_STARTERS added to is_known_builder_method (query_chain/methods.rs). The gate now recognises limit, take, skip, offset, forPage, withTrashed, onlyTrashed, withoutTrashed, inRandomOrder, which previously lived only in that table and were mis-queued as relation-hop candidates mid-chain (each firing a wasted resolve_related_model lookup; latent mis-hop hazard on a name collision). Doc comment on the gate updated to reflect the added table.
• Negative cursor test — eloquent_static_starter_methods_do_not_queue_relation_hops asserts User::query()->withTrashed()->limit(10)->competitions() queues only ["competitions"], not the static starters. Fails on the pre-fix gate.
• 📋 Follow-up — symmetric negative diagnostics test for the property-access form — relationship_property_receiver_still_flags_unknown_column_on_related_table: $user->competitions->where('emial', …) still emits exactly one diagnostic naming the competitions table (query_chain/diagnostics/tests.rs).

cargo test --lib green (399 query_chain tests, incl. the 2 new), cargo clippy --lib --tests clean, cargo fmt applied. Re-requesting review.

[mr-sherlock-holmes]

✅ Approved

Round 2, and the blocker from my last pass is closed: is_known_builder_method now includes ELOQUENT_STATIC_STARTERS (methods.rs), so limit/take/withTrashed/… are no longer mis-queued as relation hops — and there's a dedicated test (eloquent_static_starter_methods_do_not_queue_relation_hops) nailing it shut. I re-ran the full fan-out: four blind lens reviewers over the checkout, then every in-PR finding through an adversarial verifier. Two minor in-PR observations surfaced and both were refuted (details below). Nothing survives as a blocker. CI is green (LSP test/fmt/clippy, wasm, CodeQL).

Review Summary

• Reviewed the relationship-hop fix across the chain walker — diagnostics, completion, and goto-def all wire apply_relation_method_hops in the same order (closure hop → pending hops → table resolution), effective_model advances before the column check so the post-hop validation lands on the related table, and the skip path keeps the model unchanged on resolve_related_model → None.
• All 7 acceptance criteria met. One deliberate divergence worth recording: AC #5 anticipated reusing EloquentReceiver::InstanceVar; Watson instead added a dedicated EloquentReceiver::RelationProperty variant (chain.rs, extractor.rs) that seeds effective_model = base_type and pushes the relation as the first pending hop with BuilderMode::EloquentCollection (cursor.rs). Same end state, routed through the same hop machinery as the method-call path — cleaner, nothing the criterion cared about dropped. Approved as met-by-better-path.
• Tests verified and non-vacuous — they fail if the fix is reverted. Method-call hop: relationship_method_hop_validates_against_related_table (type valid only on competitions, asserts no diagnostic) with a companion that still flags a typo column on the related table (proves it moved the table rather than blanket-suppressing). Property-access: relationship_property_receiver_validates_against_related_table + relation_property_receiver_offers_related_collection_columns (asserts exact competitions columns offered). Skip path covered by apply_relation_method_hops_skips_unresolvable_names.

Refuted in-PR observations (transparency)

• "No MAX_HOPS cap on pending hops" → refuted: hop count is bounded by the developer's own authored chain length, further reduced by the is_known_builder_method filter and the mode-flip guard; no untrusted input boundary, and the existing walker already iterates all links uncapped. Not a blocker.
• "goto-def calls apply_relation_method_hops unconditionally" → refuted: the function fast-returns on an empty vec (eloquent_completion.rs:760-762), so the call is a zero-cost no-op; the completion path's guard gates a second lock acquisition that the goto-def path can't reach. Pure no-op, no asymmetry that matters.

📋 Non-blocking follow-ups

• ..-path-traversal hardening in find_php_class_file_by_fqcn — laravel-lsp/src/class_locator.rs:120-137 joins FQCN segments without filtering .., so a crafted ::class reference could resolve a read outside the project root. Low practical severity (LSP threat model = the developer's own trusted tree, read-only probe), but it's the next FS-touching resolution surface in the #130→#143→#148→#194→#199→#214 path_within_root containment lineage and worth closing for uniformity. Tracking as a new anchor.
• End-to-end integration test for the completion-handler wiring — the main.rs completion dispatch of apply_relation_method_hops is covered only at unit level (the diagnostics path is end-to-end tested). A bad re-wire in main.rs would slip past the current tests. Coverage-breadth, not a defect. Tracking as a new anchor.

Ready for @mikebronner to merge.