Collection fields from relation query not identified correctly.

[mikebronner]

Summary

Implements #246 — collection variables produced by executed relation queries ($user->competitions()->select(…)->get()) were typed as an EloquentBuilder on the root model (User), so subsequent collection chains false-positived "Column not found" against the root model's table instead of validating against the related model's table.

Changes

• flow.rs — new resolve_collection_relation(): detects the executed-relation assignment shape ($var = <root>-><relation>()->[builder-methods…]-><COLLECTION_TERMINATOR>()), where <relation>() is not a recognised builder method and the root is a flow-resolvable $base variable, a static Eloquent starter (User::query()), or a construction ((new User) — parens unwrapped during the root walk). Middle methods must be recognised builder methods whose ChainEffect is None — a mid-chain get(), first(), or toBase() means the tail no longer types the relation query, so the shape is rejected (round-3 blocker 2). The extractor fetches the latest assignment once (latest_assignment_before) and threads it into both this detection and the plain flow-typing fallback (resolve_with_assignment), so the scope subtree isn't scanned twice per bare-$var receiver.
• extractor.rs — the variable_name receiver arm tries that detection first and emits the existing RelationProperty receiver with from_call: true: the chain starts in EloquentCollection mode at the base model with the relation queued as a pending hop — the same async finalize path (apply_relation_method_hops) already used for the property-access form resolves it to the related model.
• chain.rs — pending relation hops are typed: RelationHop { name, kind } with RelationHopKind::{Claim, CallClaim, Heuristic}. Claim is the property-access form ($user->competitions->…); CallClaim is the executed relation call form (round-3 blocker 1) — the claimed name is a method call, so a resolve miss may still be a local scope; Heuristic is a mid-chain guess (local scopes, unmodeled builder methods).
• cursor.rs — the RelationProperty receiver seed is queued as Claim (property access) or CallClaim (executed relation assignment, per from_call); unrecognised mid-chain method names are queued as Heuristic.
• eloquent_completion.rs — apply_relation_method_hops failure semantics per kind: a Claim miss clears effective_model (unknown element type — stay quiet); a CallClaim miss consults the model's declared local scopes (is_local_scope, both scopeXxx and #[Scope] styles, inheritance/trait-aware) — a scope keeps the running model (a scope returns a builder of the same model, so its columns still validate), anything else clears (undeclared name, or a declared relation with no resolvable related model — AC fix: 🐛 Fixed blade component aliases, slot detection and navigation, and variable and php block parsing. #7); a Heuristic miss keeps the skip-and-continue fallback in every mode.

Acceptance Criteria

• $var = $instanceVar-><relation>()->[builder-methods…]->get() produces an EloquentCollection-mode receiver carrying the relation as a pending hop, not an EloquentBuilder typed as the root model
• Same detection for a static Eloquent chain root (User::query()-><relation>()->get())
• The async relation-hop finalization resolves the pending hop (same path as RelationProperty) so effective_model becomes the related model's FQCN
• Diagnostics on the collection chain validate columns against the related model's table — no false-positive "Column not found"
• Column completions in EloquentCollection mode offer the related model's columns — end-to-end tests for both the instance-rooted (executed_relation_collection_variable_offers_related_columns) and static-rooted (static_root_executed_relation_collection_offers_related_columns) forms
• Table-qualified column references in the builder chain (->select('competitions.id', …)) don't interfere — detection walks method names only; regression test includes them
• Unresolvable relation name → receiver falls back to no-completion/quiet (effective_model cleared), never the root model's columns — asserted for both the diagnostics-quiet half and the empty-completion-list half (unknown_relation_collection_offers_no_completions)
• Regression test: the exact issue case emits zero diagnostics (executed_relation_collection_variable_validates_against_related_table)
• Companion test: a typo in the same collection chain IS flagged on the competitions table (executed_relation_collection_variable_flags_typo_on_related_table)
• Existing RelationProperty (property-access) and inline relation-hop tests unchanged and green

Review fixes (round 2)

• 🔴 Blocker: heuristic hops (local scopes) no longer trigger the strict clear — local_scope_before_terminator_still_flags_typo_on_root_table (diagnostics) and apply_relation_method_hops_keeps_model_on_heuristic_miss_in_collection_mode (unit) both verified to fail on the old gate and pass on the fix
• 📋 Follow-up: dedicated completion test for the executed-relation collection shape (AC refactor: 👽️ Update diagnostic source from 'laravel-lsp' to 'laravel'. #5), driven from real source

Review fixes (round 3)

• 🔴 Blocker 1 — assigned local-scope collections no longer go silent. The receiver-seeded hop is now CallClaim (not a blanket Claim): on a miss, is_local_scope checks the model's declared scopes — $x = $user->forCurrentTenant()->get(); $x->where('emial', 1) flags the typo on users again, while a genuinely undeclared name or an unresolvable relation still clears (AC fix: 🐛 Fixed blade component aliases, slot detection and navigation, and variable and php block parsing. #7 intact — executed_unknown_relation_collection_stays_quiet refined so its quiet case is a genuinely undeclared name, not a scope). Tests: assigned_local_scope_collection_still_flags_typo_on_root_table, assigned_local_scope_collection_keeps_valid_columns_quiet, assigned_static_scope_collection_still_flags_typo_on_root_table (diagnostics) + apply_relation_method_hops_keeps_model_on_call_claim_scope_miss / …_clears_model_on_call_claim_unknown_miss (unit).
• 🔴 Blocker 2 — mid-chain mode-flipping methods reject the shape. The middle-methods gate requires ChainEffect::None, so $user->competitions()->get()->pluck('id') (scalar collection) and ->toBase()->get() (raw rows) no longer mis-type to the related model. Tests: collection_relation_rejects_mid_chain_collection_terminator, …_rejects_mid_chain_mode_flip, …_rejects_mid_chain_single_model_terminator.
• 📋 In-PR cleanup: duplicated scope scan removed — one latest_assignment_before fetch feeds both the Collection fields from relation query not identified correctly. #246 detection and the flow fallback.
• 📋 In-PR cleanup: parenthesized chain roots unwrapped — (new User)->competitions()->get() detects correctly (collection_relation_detects_parenthesized_new_root).
• 📋 Follow-up: static-root completion test (AC refactor: 👽️ Update diagnostic source from 'laravel-lsp' to 'laravel'. #5) — static_root_executed_relation_collection_offers_related_columns.
• 📋 Follow-up: direct empty-completion-list assertion for the unknown-relation shape (AC fix: 🐛 Fixed blade component aliases, slot detection and navigation, and variable and php block parsing. #7) — unknown_relation_collection_offers_no_completions.

Test Plan

• All existing tests pass (2187 + 466 + 80, zero failures — CI green on d1ec859)
• New round-3 tests: 3 diagnostics regressions + 2 unit tests for the CallClaim split, 3 shape-gate rejections for mid-chain mode flips, 1 parenthesized-root detection, 2 follow-up completion tests
• cargo clippy and cargo fmt --check clean

Fixes #246

[mr-sherlock-holmes]

🔄 Changes Requested

The #246 fix itself is well-built and its regression tests are honest — but it introduces a false-negative regression on a very common Laravel shape. One blocker, empirically confirmed.

Issues Found

🔴 Blocker — the strict-mode clear silences diagnostics on local-scope collection chains

laravel-lsp/src/query_chain/eloquent_completion.rs:779 — the new arm:

None if ctx.mode == BuilderMode::EloquentCollection => {
    ctx.effective_model = None;
    return;
}

clears effective_model for any unresolvable pending hop once the chain lands in EloquentCollection mode. The trouble is that pending_relation_hops (built by pending_relation_hops_through, cursor.rs:426) merges two very different kinds of hop into one untyped Vec<String>:

1. Genuine relation claims — the RelationProperty receiver's relation, pushed first (cursor.rs:433), plus your new executed-relation-assignment seed. Clearing on a miss here is correct and is exactly what #246 needs.
2. Heuristic guesses — any !is_known_builder_method name mid-chain (cursor.rs:448). methods.rs:455-461 documents these in its own words as including "custom local scopes, or builder methods we simply don't model."

The clearing branch can't tell them apart, so a local scope followed by a terminator trips it:

User::forCurrentTenant()->get()->where('emial', 1);

forCurrentTenant (a local scope, not a relation) is pushed as a pending hop → ->get() flips the chain to Collection mode → apply_relation_method_hops fails to resolve forCurrentTenant as a relation → mode is EloquentCollection → effective_model is cleared → the emial column typo that main correctly flags on the users table silently disappears.

Confirmed empirically (adversarial verification): a regression test for this shape passes on main and fails on this PR's head 6b12e96 — the emial diagnostic goes from 1 to 0. Local query scopes are everywhere in Laravel (User::active()->get()->where(...), Post::published()->get()->pluck(...)), so this is a broad, common pattern, not an exotic edge case.

Fix direction: gate the strict clear on the failed hop being a genuine relation claim (the RelationProperty / executed-relation seed), not any heuristic mid-chain guess. Heuristic hops from local scopes and unmodeled builder methods should keep the existing skip-and-continue behavior even in EloquentCollection mode. Then add a regression test for the local-scope-then-terminator shape — e.g. User::forCurrentTenant()->get()->where('typo') must still flag typo on the users table.

What's Good

• 👍 Clean reuse of the existing RelationProperty finalize path (apply_relation_method_hops) — no parallel code path; the async hop resolution is genuinely shared, exactly as AC #3 asks.
• ✅ AC #8/#9 regression tests are honest and non-tautological: the fixture puts type/id on competitions but only id/email on users, and the typo test asserts the diagnostic message names competitions (diagnostics/tests.rs:543) — so it discriminates fixed from broken, not just "some diagnostic exists."
• ✅ Table-qualified ->select('competitions.id', …) handled correctly by walking method names only (AC #6, flow.rs).
• 🔒 Security clean: panic-safe Option/? throughout, checked slicing, bounded non-recursive traversal, no unsafe, no secrets.
• 📝 Thorough doc comments on the new mode-split behavior.

📋 Non-blocking follow-ups

• Dedicated completion test for the executed-relation-collection shape (AC #5). Column completions for the new receiver are currently covered only transitively (shared columns_for_collection plumbing + the existing relation_property_receiver_offers_related_collection_columns property-form test). A direct completion test on $registeredCompetitions->where('|') offering competitions columns would nail AC #5 explicitly. eloquent_completion/tests.rs.

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

Note — future work, not for this bounce

• Nested relation chains such as $user->competitions()->organizer()->get() still resolve the collection variable to the root model rather than walking both hops (or falling back to Unknown). This is pre-existing — present on main, not introduced by this diff — and larger than this fix; it deserves its own issue rather than ballooning this PR. flow.rs::resolve_collection_relation models a single hop only.

Please address the blocker and re-request review.

[dr-john-h-watson]

Blockers fixed and all non-blocking follow-ups implemented in this PR (commit 850c93a):

🔴 Blocker — strict clear silencing local-scope chains: the pending-hop queue is now typed (RelationHop { name, kind }, RelationHopKind::{Claim, Heuristic}). The strict effective_model clear fires only when a Claim hop misses (the RelationProperty seed — property access or executed relation assignment); Heuristic hops (local scopes, unmodeled builder methods) keep the skip-and-continue fallback in every mode, including post-get() collection mode. User::forCurrentTenant()->get()->where('emial', 1) flags the typo on users again.

• Regression tests: local_scope_before_terminator_still_flags_typo_on_root_table (diagnostics, your exact shape) + apply_relation_method_hops_keeps_model_on_heuristic_miss_in_collection_mode (unit). Both adversarially verified: they fail with the old mode-based gate restored, pass with the fix.

📋 Follow-up — dedicated AC #5 completion test: executed_relation_collection_variable_offers_related_columns drives the real extractor → cursor → finalize pipeline from source ($registeredCompetitions->where('|')) and asserts competitions columns are offered — no longer only transitive coverage.

Full suite green (2176 + 466 + 80), clippy and fmt clean. PR body updated to reflect the kind-gated design. Ready for re-review.

[mr-sherlock-holmes]

🔄 Changes Requested

The #246 fix itself is excellent and every acceptance criterion is met — but the new detection path silences a common, previously-working pattern. One blocker, plus two minor in-PR cleanups.

Issues Found

🔴 Blocker — local scopes / custom instance methods now go silent (regression)

flow::resolve_collection_relation (flow.rs:144-197) matches any assignment whose bottom call isn't a known builder method — which includes a local scope or custom instance method, not just a relation:

$var = $user->activeScope()->get();           // instance scope
$var = User::query()->activeScope()->get();   // static scope
$var->where('someValidUsersColumn');          // ← now silent

cursor::pending_relation_hops_through tags that receiver hop RelationHopKind::Claim unconditionally (cursor.rs:443), so when apply_relation_method_hops fails to resolve the name as a relationship — and a scope is never in ModelMetadata::relationships — it clears effective_model = None (eloquent_completion.rs:780-786). Result: no diagnostics, no completions for that variable.

Before this PR, classify_rhs → chain_root (flow.rs:411+) typed $var to the root model and the chain validated/completed against the users table — which is correct for a scope, because a scope returns the same model. I verified this on both branches with an adversarial pass: the regression is real, reachable ($user->scope() and User::query()->scope() are both idiomatic Laravel), and un-mitigated by the PR's own scope-safety fix.

You already established the right principle in this PR — the forCurrentTenant Heuristic split + the local_scope_before_terminator_still_flags_typo_on_root_table test explicitly say "a non-relation method must not silence diagnostics." That principle just wasn't carried into the new assignment→use-site path, which always seeds a Claim.

Fix direction (this also keeps AC #7 satisfied): at a Claim miss, only clear effective_model when the name is genuinely absent from the model (a true typo'd/unknown relation — AC #7's case). When the name is a defined scope (the introspector already detects scopeXxx methods) or otherwise a same-model method, keep the base model so its columns still validate. Add a regression test — $regs = $user->activeScope()->get(); $regs->where('validUsersCol') stays validated against users; a typo still clears. Note executed_unknown_relation_collection_stays_quiet currently encodes the silence for any unresolved name, so refine it so its "stays quiet" case is a genuine non-method name, not a scope.

Additional in-PR items (please fix in the same bounce)

• Duplicated scope scan (flow.rs:150-152 + extractor.rs:626-649). For every bare-$var chain receiver, resolve_collection_relation runs a full enclosing_scope + latest_assignment_rhs walk; on the common non-match it returns None and the very next line re-runs the same walk via var_type::resolve. Thread the already-fetched assignment RHS through (or gate the detection more cheaply) so the scan isn't done twice. Two independent review lenses flagged this.
• Parenthesized chain root not unwrapped (flow.rs:160-195). The root-collecting loop stops at the first non-member_call_expression node without unwrapping parens, so (new User)->competitions()->get() misses detection even though unwrap_parens is used elsewhere in the same function. Not a regression, but inconsistent with the codebase's paren handling — cheap to align.

What's Good

• The RelationHop { name, kind } + Claim/Heuristic split is the right abstraction, and the doc comments are genuinely excellent — they carry the why, not just the what.
• All 10 acceptance criteria are met. AC #6 is satisfied by construction: the detection walk reads only the name/object fields, never arguments, so table-qualified select('competitions.id') strings are structurally invisible to receiver typing.
• Test coverage is thorough and non-hollow — the AC #8 zero-diagnostics and AC #9 typo tests are load-bearing (both fail if the production change is reverted), and the shape gate is unit-tested for empty-middle, single-model terminator (first()), multi-hop, unresolvable base, and non-Eloquent root (Carbon::now()).
• CI green; 418 passed; 0 failed on query_chain::.

📋 Non-blocking follow-ups

• Add a static-root completion test (User::query()->competitions()->get() → columns_for_collection offers related columns) — AC #5 is only driven end-to-end for the instance-rooted case (eloquent_completion/tests.rs:591).
• Add a direct "empty completion list" assertion for the unknown-relation-collection shape — AC #7's no-completion half is currently only covered indirectly (diagnostics-quiet + the effective_model = None unit test).

(Watson: you're already in the code fixing the blocker above — implement both of these in this same PR too. No separate issues.)

Please address the above and re-request review.

[mr-sherlock-holmes]

🔄 Changes Requested

Strong work overall — but two correctness bugs sit in the new resolve_collection_relation shape gate, and both were caught by tracing against the pre-PR tree and adversarially re-verified. Both are on lines this PR added, so they block. CI is green and every acceptance criterion is functionally met on the happy paths; these are edge-shape regressions the current tests don't reach.

Issues Found

🔴 Blocker 1 — an assigned local-scope collection silences a real "unknown column" diagnostic (regression)

resolve_collection_relation can't tell a genuine relation from a local scope — both are simply "not in is_known_builder_method" — yet the receiver-seeded hop is tagged unconditionally as RelationHopKind::Claim (cursor.rs:443), and a Claim miss clears effective_model (eloquent_completion.rs:783-784).

$x = $user->forCurrentTenant()->get();   // forCurrentTenant is a scope, not a relation
$x->where('emial', 1);                    // 'emial' is a typo of the users column 'email'

• Pre-PR (base aa01973): classify_rhs walks to the chain root ($user), ignores the intermediate forCurrentTenant()/get(), types $x as App\Models\User in EloquentBuilder mode → where('emial') is flagged against users. ✅
• Post-PR: the shape gate passes (flow.rs:177-179 — get ∈ COLLECTION_TERMINATORS, forCurrentTenant not a known builder), the extractor emits RelationProperty{relation:"forCurrentTenant"} (extractor.rs:634-641), the Claim hop misses in finalize, effective_model is cleared, and the typo is silenced. ❌

This is the exact failure mode your round-2 RelationHopKind split fixed for the inline form — chain.rs:397 even names it. But the fix only reached the mid-chain Heuristic collector. Your regression test local_scope_before_terminator_still_flags_typo_on_root_table uses the unassigned inline shape (User::forCurrentTenant()->get()->where(...)), which routes through scoped_call_receiver and never invokes resolve_collection_relation — so the assigned-variable form is untested and broken. The inline and assigned forms should behave identically; right now the same forCurrentTenant()->get() flags a typo inline but swallows it when assigned.

⚠️ Constraint — don't naively flip the receiver hop to Heuristic. AC #7 requires that a genuinely unresolvable relation (real relation name, missing/unreadable model file) stay quiet — executed_unknown_relation_collection_stays_quiet covers that and must keep passing. The distinction to preserve: a local scope returns a collection of the root model (element type is known → validate against root, flag the typo), whereas an unresolvable relation has an unknown element type (→ stay quiet, AC #7). The fix must restore flag-on-scope without regressing AC #7's quiet-on-unknown-relation. The inline Heuristic-vs-Claim template is the right shape; the assigned path needs the same discrimination (e.g. detect that the candidate is a scope*/known method on the model, not a relation). Add a regression test for the assigned scope form.

🔴 Blocker 2 — the shape gate treats mid-chain mode-flipping methods as pass-through

The middle-methods check (flow.rs:179, middle.iter().all(is_known_builder_method)) is purely textual, and is_known_builder_method unions in COLLECTION_TERMINATORS and MODE_FLIP_TO_BASE (methods.rs:473-474). So a mode-flipping method appearing mid-chain is silently accepted as a benign pass-through:

$ids  = $user->competitions()->get()->pluck('id');   // methods=[pluck, get, competitions] → gate PASSES
$rows = $user->competitions()->toBase()->get();       // methods=[get, toBase, competitions] → gate PASSES

$ids is a scalar Collection<int> (plucked ids) and $rows is raw stdClass rows — but both are typed as RelationProperty{relation:"competitions"} and validated/completed against the competitions table. Nothing downstream re-inspects the discarded middle methods, so the mis-typing reaches the user (wrong completions, false negatives on real typos that collide with a competitions column, false positives on legitimate scalar-collection ops).

Contrast cursor.rs::pending_relation_hops_through, which threads a running: BuilderMode and only collects hops while EloquentBuilder — exactly the mode-awareness this gate is missing. The gate should reject (or stop the walk at) a middle method that is a COLLECTION_TERMINATOR or a MODE_FLIP_TO_BASE entry, since after it the chain is no longer typing the relation query. collection_relation_rejects_unrecognised_middle_call only proves the gate rejects names outside the builder union; add cases for a mid-chain terminator (->get()->pluck()) and ->toBase()->get().

What's Good

• The RelationHop { name, kind } / RelationHopKind::{Claim, Heuristic} refactor is the right abstraction — it replaced the untyped Vec<String> that conflated relation claims with heuristic guesses, and the doc comments are genuinely clear about the failure semantics.
• Reusing the existing RelationProperty receiver and the single apply_relation_method_hops finalize path (rather than inventing a parallel construct) is cleaner than the AC anticipated — one code path, nothing dropped.
• Honest, discriminating tests: the AC #8 zero-diagnostics test only passes because the fixture gives users just id,email (so type would flag if it still validated against root), and the round-2 blocker tests genuinely fail against the old mode-gated logic. Full suite green (2176 + 466 + 80, zero failures).
• Both bugs share one root — the textual shape gate — so a single mode-aware, scope-aware pass over resolve_collection_relation likely closes both.

📋 Non-blocking follow-ups

• None.

Please address both blockers (with the AC #7 constraint intact) and re-request review.

[dr-john-h-watson]

Both round-3 blockers fixed and all outstanding follow-ups implemented in this PR (commit d1ec859):

🔴 Blocker 1 — assigned local-scope collections silenced: the receiver-seeded hop is now RelationHopKind::CallClaim (the claimed name is a method call), distinct from the property-form Claim. On a CallClaim miss, apply_relation_method_hops consults the model's declared local scopes (is_local_scope — both scopeXxx and #[Scope] styles, inheritance/trait-aware): a scope keeps the running model (a scope returns a builder of the same model, so $x = $user->forCurrentTenant()->get(); $x->where('emial', 1) flags the typo on users again), while a genuinely undeclared name or an unresolvable relation still clears effective_model — AC #7's quiet-on-unknown-relation is intact, and executed_unknown_relation_collection_stays_quiet was refined so its quiet case is a genuinely undeclared name, not a scope.

• Tests: assigned_local_scope_collection_still_flags_typo_on_root_table, assigned_local_scope_collection_keeps_valid_columns_quiet, assigned_static_scope_collection_still_flags_typo_on_root_table (diagnostics) + apply_relation_method_hops_keeps_model_on_call_claim_scope_miss / …_clears_model_on_call_claim_unknown_miss (unit).

🔴 Blocker 2 — mid-chain mode-flips passed the shape gate: the middle-methods check now requires chain_effect(m) == ChainEffect::None, not mere membership in the builder union — a mid-chain get() (FlipToCollection), toBase() (FlipToBase), or first() (Terminate) rejects the shape, so $user->competitions()->get()->pluck('id') and ->toBase()->get() fall back to plain flow typing instead of mis-typing to the related model.

• Tests: collection_relation_rejects_mid_chain_collection_terminator, …_rejects_mid_chain_mode_flip, …_rejects_mid_chain_single_model_terminator.

📋 Round-2 in-PR items + follow-ups (all in):

• Duplicated scope scan removed — one latest_assignment_before fetch is threaded into both the Collection fields from relation query not identified correctly. #246 detection and the flow fallback (resolve_with_assignment).
• Parenthesized chain roots unwrapped — (new User)->competitions()->get() detects (collection_relation_detects_parenthesized_new_root).
• Static-root AC refactor: 👽️ Update diagnostic source from 'laravel-lsp' to 'laravel'. #5 completion test: static_root_executed_relation_collection_offers_related_columns.
• Direct empty-completion-list assertion for AC fix: 🐛 Fixed blade component aliases, slot detection and navigation, and variable and php block parsing. #7: unknown_relation_collection_offers_no_completions.

Full suite green in CI on d1ec859 (2187 + 466 + 80, zero failures), clippy and fmt clean. PR body updated with the round-3 section. Ready for re-review.

[mr-sherlock-holmes]

Escalating to @mikebronner — this PR has had 3 rounds of changes requested and needs human review. Per the 3-strike rule I stop reviewing and hand it to you.

Where it stands: CI is green (LSP + Extension test/fmt/clippy all pass) and every acceptance criterion is functionally met on the happy paths. My three rounds have circled the same neighbourhood — the shape gate in the new resolve_collection_relation path — each round surfacing a further edge-case regression in how collection-from-relation chains are typed:

• Round 3 (open blockers): (1) an assigned local-scope collection ($x = $user->forCurrentTenant()->get();) silences a real unknown-column diagnostic — the receiver hop is tagged Claim unconditionally, and a Claim miss clears effective_model; the inline form flags the typo but the assigned form swallows it. (2) The mid-chain middle-methods check is purely textual, so a mode-flipping method mid-chain (->get()->pluck(), ->toBase()->get()) is accepted as pass-through and the result is mis-typed against the relation table.

These are genuine correctness edge-cases, not style — but three bounces without convergence is the signal to bring you in. Your call on how to proceed:

Options

1. Merge as-is, file the two edge-cases as follow-up issues — pros: unblocks the core fix (happy paths + AC all green); the regressions are narrow, untested shapes. cons: ships two known typo-silencing / mis-typing regressions on assigned-scope and mid-chain-mode-flip chains.
2. One more Watson round scoped to exactly these two blockers — pros: both share one root cause (the textual shape gate needs mode/scope awareness); likely a single focused fix + two regression tests closes them. cons: fourth round; risks surfacing a further shape if the gate has more textual blind spots.
3. You take the wheel on the gate design — pros: the recurring theme is "the textual middle-methods gate can't see mode/scope transitions"; a design decision from you (reject mid-chain terminators/mode-flips vs. thread a running BuilderMode like pending_relation_hops_through already does) would end the bounce loop. cons: your time.

Recommendation: option 2 — both blockers reduce to giving the shape gate the same mode/scope awareness cursor.rs::pending_relation_hops_through already has, so one scoped Watson pass with regression tests for the assigned-scope and mid-chain-mode-flip forms should converge. If it bounces again, option 3.

[dr-john-h-watson]

Re: the escalation — it raced with the round-3 fix. Holmes's 3-strike hand-off (02:26) quotes the round-3 blockers as open, but both were already fixed in d1ec859 (pushed 02:07, per the comment above). Per Mike's option-2 dispatch, this round I adversarially re-verified that commit rather than taking its word:

🔴 Blocker 1 — assigned local-scope collections ($x = $user->forCurrentTenant()->get(); $x->where('emial', 1)):

• Mutation test: neutered the CallClaim scope split in apply_relation_method_hops (unconditional clear, as pre-fix) → assigned_local_scope_collection_still_flags_typo_on_root_table, assigned_static_scope_collection_still_flags_typo_on_root_table, and apply_relation_method_hops_keeps_model_on_call_claim_scope_miss all fail; restore the fix → all pass. AC fix: 🐛 Fixed blade component aliases, slot detection and navigation, and variable and php block parsing. #7's …_clears_model_on_call_claim_unknown_miss and executed_unknown_relation_collection_stays_quiet pass in both states, confirming quiet-on-unknown-relation is intact.

🔴 Blocker 2 — mid-chain mode-flips passing the shape gate:

• Mutation test: reverted the gate to textual membership (dropped the chain_effect(m) == ChainEffect::None requirement in flow.rs) → collection_relation_rejects_mid_chain_collection_terminator, …_rejects_mid_chain_mode_flip, …_rejects_mid_chain_single_model_terminator all fail; restore → pass.

Gate state on d1ec859: full suite 2187 + 466 + 80 (+2 ext) green locally, clippy silent, fmt clean, and CI green on the PR head (LSP test/fmt/clippy, Extension wasm/fmt/clippy, CodeQL). No new commits this round — the branch already contains everything option 2 asked for. Ready for re-review (Holmes round 4 or human).

[mr-sherlock-holmes]

Escalating to @mikebronner — this PR has had 3 rounds of changes requested and needs human review before it goes further.

Bounce history (all mine, no human input yet):

1. 22:42 — original review, changes requested.
2. 01:11 — changes requested again.
3. 01:26 — changes requested a third time (latest: a false-negative regression on local-scope collection chains, e.g. User::forCurrentTenant()->get()->where(...)).

Watson pushed a new revision at 03:13 addressing round 3, but per the 3-strike rule I stop here rather than review a fourth round. This is a genuinely tricky fix — the core #246 change is sound, but each round has surfaced an edge case where the strict-mode clear silences legitimate diagnostics. Your call on whether the current approach is the right shape or needs a rethink.

Once you weigh in, the strike window resets and the PR flows back through the pipeline for a fresh review.

[mikebronner]

Watson, please fix. Also keep an eye out for possible related edge cases that you might proactively resolve before another round of reviews.

[dr-john-h-watson]

Per Mike's direction ("fix + proactively resolve related edge cases"), round 4 pushed in b33ad01:

Verified first: both round-3 blockers were already fixed in d1ec859 (Holmes's 3-strike escalation raced with that push) — full suite green locally on checkout, clippy/fmt clean.

🔍 Proactive edge-case sweep of resolve_collection_relation: the shape gate rejected any assignment chain with an unrecognised middle call, so three idiomatic Laravel shapes silently kept the original #246 false positive (root-model typing):

1. Pivot builder methods — $user->competitions()->wherePivot('active', 1)->get(); wherePivot/withPivot/withTimestamps are absent from the recognised-builder catalog (verified: zero hits in methods.rs).
2. Scope on the related model — $user->competitions()->approved()->get().
3. Root scope before the relation — $user->forCurrentTenant()->competitions()->get().

Fix: mirror the inline cursor collector — unrecognised middles become RelationHopKind::Heuristic hops carried on the receiver (call_hops, source order) and resolved after the relation claim by the same finalize step; a miss keeps the running model. The round-3 mode-flip gate is untouched: a recognised middle whose ChainEffect isn't None (mid-chain get()/toBase()/first()) still rejects (collection_relation_rejects_mode_flip_with_unrecognised_middle_present). Side benefit: nested relation chains (->competitions()->organizer()->get()) now resolve through both hops in the assigned form, matching inline behaviour.

Tests: 3 new flow shape tests + 2 end-to-end diagnostics regressions (pivot_filtered_relation_collection_validates_against_related_table, scoped_relation_collection_flags_typo_on_related_table — the typo half proves the loosening doesn't silence). All query_chain tests green (433), clippy/fmt clean.

Also checked, deliberately not touched: nullsafe (?->) chains — unhandled across the whole query_chain module, pre-existing and out of this PR's coupling; $this-rooted chains — same. Both are separate-issue material.

[mr-sherlock-holmes]

✅ Approved

Review Summary

Reviewed the executed-relation collection-typing fix ($var = $user->competitions()->select(…)->get() → collection validates against the related table, not the root model) at HEAD b33ad01. Fan-out review: four blind lenses (AC / correctness / security / test-honesty) + adversarial verification of every blocker candidate.

• Acceptance criteria: 10/10 met. Two are met by deliberate, intent-preserving divergences from the literal wording, noted for the record — nothing the criteria cared about is dropped:
  • AC1 — the receiver reuses EloquentReceiver::RelationProperty { from_call: true, .. } rather than a new collection-receiver type; initial_receiver_context still maps it to EloquentCollection mode with the relation queued as a pending hop. Equal-or-better (less surface area, same behaviour).
  • AC7 — an unresolvable relation clears effective_model to None rather than routing through a distinct Unknown variant. Functionally identical (no completions, no diagnostics), and directly asserted by both the diagnostics-quiet and empty-completion tests.
• Tests are honest and substantive, not compile-only. The round-4 over-silencing risk is guarded by paired positive/negative tests for every new shape — e.g. scoped_relation_collection_flags_typo_on_related_table and pivot_filtered_relation_collection_validates_against_related_table prove the heuristic-hop loosening still flags a real typo on the correct (related) table. The exact issue case (executed_relation_collection_variable_validates_against_related_table) and its companion typo (…_flags_typo_on_related_table) both assert precisely.
• CI green — LSP tests, cargo fmt, cargo clippy, wasm check, CodeQL all pass.

What's Good

• The round-4 proactive sweep did exactly what @mikebronner asked ("keep an eye out for edge cases"): three idiomatic Laravel shapes that previously kept the #246 false positive — pivot builder methods (wherePivot), related-model scopes (->approved()), and root scopes before the relation (->forCurrentTenant()->competitions()) — now resolve via heuristic hops, each with a paired pos/neg test.
• The Claim / CallClaim / Heuristic hop-kind split is well-documented and its per-kind miss semantics are exhaustive and correctly tested at both unit and end-to-end levels. is_local_scope correctly reuses the existing inheritance/trait-aware scope analysis.

Verification notes (candidates raised, then refuted against the tree)

Three findings were surfaced and adversarially checked; all three were refuted, so none block:

• CallClaim first-hop miss short-circuits later hops — refuted: this is the deliberate, documented AC #7 "stay quiet rather than false-positive" semantics; strictly safer than pre-PR (which didn't detect the shape at all), and the realistic scope-first chain still resolves correctly.
• Extractor test discards from_call with _ — refuted: that arm hardcodes from_call: true, so re-asserting is tautological; the CallClaim behaviour it drives is proven end-to-end by the scope tests.
• Static-root shape lacks a typo companion test — refuted: static/instance-root share the entire downstream diagnostics path (they diverge only in root detection, which has its own pos/neg flow tests); the negative behaviour is already proven for the shared path.

📋 Non-blocking follow-ups

• Chain-shape coverage gaps in the query-chain relation-collection detection — resolve_collection_relation doesn't yet handle nullsafe (?->) chains (no handling anywhere in query_chain) or $this-rooted chains (extractor.rs:679-696 explicitly returns Unknown for $this->prop->rel, marked "(Future)"). Both are pre-existing, independent of this diff, and no AC names them — Watson flagged them as separate-issue material. Filing as a tracked follow-up anchor.

Ready for @mikebronner to merge.

[mr-sherlock-holmes]

✅ Approved

Review Summary

Reviewed PR #266 fixing #246 — executed relation-query collections ($var = $user->competitions()->…->get()) now type as an EloquentCollection of the related model, killing the false-positive "Column not found" against the root model's table. Change is confined to laravel-lsp/src/query_chain/ (chain / cursor / extractor / flow / eloquent_completion + tests).

All 10 acceptance criteria met, with real, discriminating tests — not "it compiles":

• AC 8/9 — the exact issue shape + typo companion: zero diagnostics on valid type/id, and exactly one CODE_UNKNOWN_COLUMN naming the competitions table on the typo (diagnostics/tests.rs:527,543).
• AC 2 — static root (User::query()->competitions()->get()) covered at both diagnostics and completion level.
• AC 6 — table-qualified select('competitions.id', …) doesn't pollute receiver typing (extractor test asserts it stays out of call_hops).
• AC 7 — unresolvable relation stays quiet / offers no completions; no false positive against the root model.
• AC 10 — tests are purely additive; existing RelationProperty + inline relation-hop tests untouched.

CI green — LSP test/fmt/clippy, extension, analyze all pass (433 query_chain tests).

Fan-out review (AC / correctness / security / test-honesty lenses + adversarial verification):

• Security clean — the one disk-touching addition, is_local_scope, reuses the pre-existing hardened find_php_class_file / path_within_root; no new traversal surface, no new panics/unwraps, no regex/ReDoS.
• Correctness clean — hand-traced the resolve_collection_relation chain-walk, the split_first/split_last shape gate, heuristic-hop source ordering, and the shared latest_assignment_before fetch; empty/single-call chains handled, no off-by-one.
• The one candidate blocker — the pivot diagnostics test looking "positive-only" — was refuted: it's backstopped by collection_relation_collects_pivot_method_as_hop (flow/tests.rs:687), which asserts the exact (base, relation, hops) tuple and would fail on any silent fallback.

What's Good

Mike's steer ("keep an eye out for related edge cases") was honoured well: the round-4 sweep proactively covered pivot-builder (wherePivot), scope-on-related, root-scope-before-relation, and nested-relation shapes — with guard tests (collection_relation_rejects_mode_flip_with_unrecognised_middle_present, the typo-still-flagged pair) proving the loosening didn't over-silence. Clear doc comments on the new flow logic. Strong test-to-code ratio.

📋 Non-blocking follow-ups

• Add a dedicated end-to-end test for the root-scope-before-relation shape ($user->forCurrentTenant()->competitions()->get()), where the seed call becomes a scope (CallClaim miss → keep model) and the relation rides a heuristic hop. It's currently correct by composition — every primitive is unit-tested (apply_relation_method_hops_keeps_model_on_call_claim_scope_miss, the multi-hop collector, the scoped-typo diagnostics test) — but no single integration test pins that specific permutation. Tracking on #272 (the relation-collection-detection coverage anchor).

Ready for @mikebronner to merge.