[psr-0001] fix(rules): ForbidEloquentMutationInControllersRule misses method-local receivers

[dmooibroek]

Problem

ForbidEloquentMutationInControllersRule registered on Class_ and walked method bodies manually, resolving receiver types via $scope->getType() against the class-entry scope. That scope has no flow knowledge of method-local variables, so the most common controller-mutation idioms never fired:

$m = new Model; $m->save();                        // never flagged
$m = Model::query()->where(...)->firstOrFail(); $m->delete();  // never flagged
$q = Model::query()->where(...); $q->update([...]);            // never flagged

Only signature-typed receivers (typed method parameters) matched. Empirically proven on tc-api PR #133: spliced baseline entries for exactly these shapes came back unmatched — the rule never emitted them.

Fix

Register on CallLike per-node (mirrors LogRule / EnforceCurrentUserAttributeRule) so PHPStan supplies method-level flow scope. checkInstanceCall/checkStaticCall type gates, 24-method blocklist, identifier, message format, and controllerNamespacePrefixes NEON wiring unchanged. Controller FQCN for the message now from $scope->getClassReflection() at the call site. Dead walkNodes/collectViolations/resolveClassFqcn deleted (no sibling docblock referenced this rule's copy).

Tests

All pre-existing fixtures unchanged, same expected lines. New: 3 violation fixtures covering the three local-receiver shapes + 1 compliant fixture pinning that a non-Model local receiver stays clean.

Versioning

Candidate MAJOR per ADR-0021 — surfaces new errors in previously-clean consumers (known: tc-api EducationController::store/destroy). Pre-1.0 caret means tagging auto-adopts nobody; per-territory pre-cascade audit at pin-bump time. CHANGELOG [Unreleased] entry included.

Refs: psr-0001

[Goosterhof]

✅ Approve-worthy (substance) — event COMMENT while CI is pending on a draft

0 blockers · 0 majors · 1 minor · 1 nit · 1 inline

Independent first look. The diagnosis is verified, not taken on faith: the class-entry Scope genuinely carries no flow knowledge of method-local receivers, and this package's own sibling rules (LogRule, EnforceCurrentUserAttributeRule) already model the correct per-node registration — this PR brings the mutation rule onto that established pattern rather than inventing a new one. The empirical seed is honest too: the tc-api#133 baseline-splice test is exactly how this blind spot surfaced in our own review of that PR, and the three violation fixtures pin each missed shape (new+save, firstOrFail+delete, local Builder+update) with a negative fixture proving the type gate still discriminates under flow scope.

Contract preservation checked: identifier, message format, blocklist, controllerNamespacePrefixes wiring all byte-identical; the static-call path never depended on flow scope and is untouched. The deleted resolveClassFqcn rationale ("reflection can be null in fixture mode") is correctly obsoleted — at a call-site scope inside a method the AST-derived class reflection resolves without autoload, and the defensive null-guard degrades to silence in exactly the situations the old rule also never fired (top-level calls).

The MAJOR versioning call is correct per ADR-0021 and the pre-1.0 caret means tagging auto-adopts nobody — agreed. Known remediation surface at pin-bump: tc-api EducationController::store/destroy (matches our own #133 finding).

Findings (detail inline)

• 🟡 src/Rules/ForbidEloquentMutationInControllersRule.php:180 — nullsafe gap
• ⚪ Trait-file surface: per-node registration now reaches code in trait files under a controller namespace (the old Class_ walk structurally never ran there — trait files carry no Class_ node). Almost certainly an improvement, but it can fire inside a controllers-namespace trait used by a non-controller class, naming the using class in the message. Worth one docblock line declaring the trait behavior intended, so the next reader doesn't classify it as an accident. Unconfirmed — needs a trait fixture to pin either way.

Converts to APPROVE on green check (8.4)/check (8.5) — substance is already verified.

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.

[Goosterhof]

🛑 Blockers

1 blocker · 0 majors · 0 minors · 0 nits

Round 2 — since 333f4b0: both round-1 findings addressed at the source level (nullsafe branch + removeNull(), trait docblock + ViolationInTraitFile fixture — the TypeCombinator::removeNull() insight is the deeper half of the gap I flagged, and catching the plain-->delete()-on-?Post shape alongside it is better than what my finding asked for). But check (8.4)/check (8.5) are RED at eb9e4bd, own-code: testViolationNullsafeDelete fails with the error emitted twice at line 18, byte-identical.

The mechanism (this is the fix, not just the symptom): PHPStan's NodeScopeResolver analyses $post?->delete() by short-circuiting — it also materializes a virtual plain MethodCall for the call with the receiver scope already narrowed non-null, and hands that virtual node to rules too. So a rule matching both NullsafeMethodCall and MethodCall under CallLike sees the same source call as two nodes → two identical errors. The CI diff (one expected occurrence, two actual, same line/message) is exactly this signature.

Fix: drop the explicit NullsafeMethodCall branch (revert the union in processNode and checkInstanceCall's signature) and let PHPStan's virtual MethodCall carry the nullsafe path — it preserves the original node position, so the fixture still pins line 18, now exactly once. Keep TypeCombinator::removeNull() — it is load-bearing independently of the branch, for the plain ->delete() on a ?Post receiver (and note the virtual nullsafe node arrives already-narrowed, so it never needed the branch in the first place). Keep both fixtures unchanged; ViolationNullsafeDelete then pins single-emission via the virtual-node path. Docblock: replace the "sibling of MethodCall, so a MethodCall-only branch would miss it" rationale with the actual contract — "PHPStan re-visits nullsafe calls as virtual plain MethodCalls with a non-null-narrowed scope; matching NullsafeMethodCall as well double-reports."

Everything else from round 1 stands: contract byte-preserved, MAJOR call correct per ADR-0021, empirical seed honest. This converts to APPROVE on the branch-drop + green.

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.

[Goosterhof]

✅ Approve-worthy

0 blockers · 0 majors · 0 minors · 0 nits

Round 3 — since eb9e4bd: the double-emission blocker is fixed exactly along the prescribed line, and better documented than prescribed. The NullsafeMethodCall branch is gone; removeNull() stays with its rationale correctly narrowed to the plain-call-on-?Model shape; the docblock now records the real mechanism (PHPStan's synthetic MethodCall twin, virtualNullsafeMethodCall) so the next maintainer won't re-add the branch in good faith; and the new ViolationPlainNullableDelete fixture pins the distinct nullable-plain shape separately from ViolationNullsafeDelete — the two shapes now each have their own regression pin. check (8.4) / check (8.5) / ci-passed all green at b01cca7; the remaining red town-crier/gate is my own round-2 blocker row, dispositioned with this turn.

All round-1/round-2 threads settled. The MAJOR versioning call stands per ADR-0021 — pre-cascade audits ride each consumer's pin-bump (tc-api EducationController::store/destroy + the nullsafe/trait/nullable surfaces as new-error candidates).

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.

[dmooibroek]

Re-review: all 3 prior finding(s) resolved at this HEAD.

Resolved since last review: 3.