feat: EnforceAuditTransactionScopeRule (ADR-0029 enforcement, queue #86)

[Goosterhof]

Summary

Adds EnforceAuditTransactionScopeRule enforcing the success-side of ADR-0029 (Audit Row Durability Contract): non-transactional state mutations (StatefulGuard / Session / Cache / Bus / Queue / Mail / Notification / Broadcast / Storage facades + contracts, mutation methods only) MUST NOT happen inside transaction(...) closures in App\Actions\* classes.

Identifier: enforceAuditTransactionScope.nonTransactionalMutationInClosure.

Doctrine source: ADR-0029 (Proposed 2026-05-28). Seed: ISMS-0003 PR #7 commit f1d357b — three Auth Actions (AuthenticateWorkerAction, VerifyTwoFactorChallengeAction, LogoutWorkerAction) mutated StatefulGuard + Session state inside the transaction closure before the audit row write; an audit-write failure would have rolled back the audit row while leaving the session/guard mutation intact (ISO 27001 A.8.15 / A.5.33 violation). The fix moved guard/session calls post-commit; this rule prevents reintroduction across every consuming territory.

Failure-side enforcement (sentinel-return discipline; throws inside closure detection) is enforcement queue #85, separate per-territory Pest arch tests — not bundled. Combining would couple two distinct AST-walks and inflate false-positive surface.

War-room enforcement queue: #86. Order: orders/phpstan-warroom-rules/queue-86-enforce-audit-transaction-scope-armorer-deployment.md.

Scope

• New rule src/Rules/EnforceAuditTransactionScopeRule.php (~330 LoC; mirrors EnforceActionTransactionsRule namespace-gate + AST-walk shape and EnforceAuditSnapshotOnRetryRule closure-walking).
• 16 new tests in tests/Rules/EnforceAuditTransactionScopeRuleTest.php with one fixture per case under tests/Fixtures/AuditTransactionScope/.
• Rule registration in extension.neon (positioned after EnforceAuditSnapshotOnRetryRule).
• Doc updates — CLAUDE.md Rules-shipped table, README.md Rules table, CHANGELOG.md [Unreleased] Added section.

Algorithm

1. Namespace gate — return early unless $scope->getNamespace() starts with App\Actions.
2. Locate execute() method; skip if absent or empty.
3. Find every top-level transaction() call in the method body (MethodCall or StaticCall). Top-level discovery deliberately skips nested-transaction calls — those are walked into transitively by the closure-walker, so counting them at discovery time would double-report.
4. For each top-level call, extract the first argument; if it's a literal Closure or ArrowFunction, walk its body recursively (including nested closures + nested transactions).
5. Match each MethodCall|StaticCall inside the closure against the blocklist:
   • Instance calls ($this->prop->method(...)) — resolve property name to constructor parameter's declared FQCN; match against blocklist keys.
   • Static-facade calls (Auth::method(...)) — resolve via Scope::resolveName(); match against blocklist facade FQCNs.

Blocklist

Mutation methods only on a canonical set of facades + contracts:

Type	Methods
Illuminate\Contracts\Auth\StatefulGuard + Auth facade	login, logout, attempt, loginUsingId, logoutCurrentDevice, viaRemember, once, onceUsingId, attemptWhen, logoutOtherDevices
Illuminate\Contracts\Session\Session + Session facade	regenerate, invalidate, put, forget, remove, flush, migrate, regenerateToken, flash, reflash, keep, now, increment, decrement, push, replace, pull
Illuminate\Contracts\Cache\Repository + Cache facade	put, forget, flush, delete, add, pull, increment, decrement, forever, remember, rememberForever, sear, restoreLock (contract only)
Illuminate\Contracts\Bus\Dispatcher + Bus facade	dispatch, dispatchNow, dispatchSync, dispatchAfterResponse, dispatchToQueue (contract only)
Illuminate\Contracts\Queue\Queue + Queue facade	push, pushOn, later, laterOn, bulk
Illuminate\Contracts\Mail\Mailer + Mail facade	send, sendNow, raw, plain, html, queue, later, to, cc, bcc (last three facade-only)
Illuminate\Contracts\Notifications\Dispatcher + Notification facade	send, sendNow, route (facade-only)
Illuminate\Contracts\Broadcasting\Broadcaster + Broadcast facade	broadcast (contract), event, channel (facade)
Illuminate\Contracts\Filesystem\Filesystem + Storage facade	put, putFile, putFileAs, delete, move, copy, prepend, append, setVisibility, makeDirectory, deleteDirectory

Reads (Auth::user(), Session::get(), Cache::get(), etc.) are deliberately permitted — only mutations carry the rollback-vs-side-effect asymmetry the rule guards against.

What's intentionally NOT in this PR

• Version bump — [Unreleased] block grows; the release-PR pattern (per CLAUDE.md) cuts the version separately. The rule is a candidate Major bump per ADR-0021 §Versioning (new errors in code that previously passed).
• Adding the rule to consuming territories' phpstan.neon — per-territory follow-up dispatches once the rule ships.
• Failure-side detection — enforcement queue #85, separate.
• Manual transaction management (DB::beginTransaction() / commit()) — out of scope; only transaction(Closure) with a literal closure first-arg fires.

Test plan

• composer test — 16 new tests pass; 50 → 66 total
• composer test:coverage — 91.11% line coverage on new rule (≥83% threshold)
• composer coverage:check — PASS (87.48% overall, ≥83 threshold)
• composer mutation:ci — 81.96% MSI (6.96pp above 75% gate)
• composer phpstan self-analysis clean (level max)
• composer format:check Pint clean
• Manual smoke against ISMS — overlay the new rule into ISMS vendor, run composer stan: zero enforceAuditTransactionScope.* violations on current ISMS code, confirming compliance per PR feat(LogRule): cover Model::destroy / Model::forceDestroy static calls #7 commit f1d357b.

🤖 Generated with Claude Code

[Goosterhof]

Stacked follow-up: #28 (ForbidEloquentMutationInControllersRule, queue #87) branches off queue-86-enforce-audit-transaction-scope. Load-bearing inheritance is confined to the extension.neon services-block ordering — no source-symbol dependency. Reviewing both in one pass is feasible: read PR #27 first, then PR #28's diff narrows to the new rule + fixtures + docs once #27 merges.

Merge order: #27 first; #28 rebases onto fresh main post-merge.

[jasperboerhof]

PR Reviewer · claimed

• Target: phpstan-warroom-rules
• PR: feat: EnforceAuditTransactionScopeRule (ADR-0029 enforcement, queue #86) #27
• Branch: queue-86-enforce-audit-transaction-scope
• AC anchor: none (no Kendo issue link, no on-branch plan dir, no AC heading in PR body; ADR-0029 is the doctrine anchor for reviewers)
• Worktree: ~/Code/agent-worktrees/phpstan-warroom-rules/review-27

[jasperboerhof]

PR Reviewer · 9/10 · PASS

• PR: feat: EnforceAuditTransactionScopeRule (ADR-0029 enforcement, queue #86) #27 · phpstan-warroom-rules
• AC anchor: none
• Reviewers run: Surface, Efficiency, Silent failures skipped — no triggers
• Surface: PASS · 9/10
• Efficiency: PASS · 9/10

Findings

• MINOR · src/Rules/EnforceAuditTransactionScopeRule.php:302 — Row 6 / contract-ripple: the transaction-call match (isTransactionCall) is purely syntactic — it fires on ANY MethodCall/StaticCall named transaction with a literal closure first arg, without confirming the receiver is a DB connection. A consumer Action with an unrelated transaction abstraction (e.g. a payment-gateway client) wrapping a blocklisted call would false-positive, newly failing previously-green CI in kendo/codebook/emmie. Bounded risk: this exactly mirrors the already-shipped EnforceActionTransactionsRule::hasTransactionCall() syntactic match, so it is consistent with package precedent rather than a new risk class — and the CHANGELOG's mandated pre-cascade audit across all six consumer territories is the correct mitigation. Note, do not block.
• MINOR · src/Rules/EnforceAuditTransactionScopeRule.php:158 — Row 6 / blocklist precision: the Broadcast facade blocklist includes channel/event and the Mail facade includes to/cc/bcc (BLOCKLIST lines 158, 171-173) — these are fluent-builder/routing entrypoints, not terminal mutations. A benign registration chain like Broadcast::channel(...) inside a closure could false-positive. No fixture proves these specific facade-method entries fire only on genuine mutations vs benign chains (fixtures cover Cache::put, Bus::dispatch, Mail send via Mailer contract, Storage::delete, Session::put, StatefulGuard::login, but not Broadcast::channel / Mail::to). For a contract package, add a fixture per fluent-builder entry or trim them. Bounded; pre-cascade audit catches real consumer impact.

Action

merge-ready

[jasperboerhof]

Auto-approved by /review-open-prs — review verdict is PASS. See the verdict comment for the per-reviewer breakdown.

[Goosterhof]

Four findings: zero blockers, three concerns, one nit. Plus praise.

The rule is sound on its core mechanic. The namespace gate, execute() location, top-level transaction discovery with deliberate non-recursion into transaction children, and the closure walk all line up with EnforceActionTransactionsRule and EnforceAuditSnapshotOnRetryRule precedent. Instance-call FQCN resolution works because PHPStan's RuleTestCase runs the full analyser including the name-resolving visitor, so $param->type->toString() and $scope->resolveName() both yield FQCNs that match the BLOCKLIST keys — the same load-bearing assumption the sibling rule relies on. The 16 fixtures cover the seed failure mode (StatefulGuard::login, Session::put) directly, plus arrow-function bodies, nested transactions, reads-allowed, and the three skip gates. This faithfully enforces ADR-0029 §Decision rule 3.

Concerns

The closure walk visits every nested closure, not just transaction closures — false-positive surface on deferred callbacks. findBlocklistViolations at src/Rules/EnforceAuditTransactionScopeRule.php:404-433 walks the entire closure body via walkNodes, which recurses unconditionally into all sub-nodes including nested closures. The contract in ADR-0029 §The shared invariant is that effects are observable independently of the audit row's durability. But a mutation registered for after-commit execution is not such an effect — DB::transaction(fn() => $this->db->afterCommit(fn() => Cache::put(...))) or a Bus job dispatched with ->afterCommit() runs only if the transaction commits, which is exactly the post-commit timing the ADR prescribes. The rule would flag the Cache::put inside the deferred closure as a violation. No fixture exercises this, and afterCommit/dispatchAfterResponse registration inside the closure is a legitimate shape. At minimum this deserves a documented known-miss in the docblock §Out of scope; better, the walk should not descend into closures passed to deferral methods. Not a blocker because the shape is uncommon in audit-writing Actions and the safe direction is over-reporting (suppressible), but consumer territories will hit it.

Cache::remember / rememberForever are read-through, not pure mutations — flagging them inside the closure contradicts the ADR's read-permit rationale. BLOCKLIST at :188-191 lists remember and rememberForever for the Repository contract. The ADR permits reads inside the closure precisely because they carry no rollback-vs-side-effect asymmetry (§The shared invariant, and the rule's own docblock at :130-132). remember($key, fn) returns a cached value and only writes on a miss; its purpose at a call site is usually the read. The cache write it performs on a miss does outlive a DB rollback, so there's a defensible argument for inclusion — but then the rationale stops being "mutations carry the asymmetry, reads don't" and becomes "any cache write, even incidental." That's a real choice, not an oversight, but the PR body and docblock both lean on the reads-permitted framing without acknowledging that remember straddles it. Either drop remember/rememberForever (treat as read-shaped) or note in the docblock why the incidental-write case keeps them in. No fixture pins this either way.

The instance-call path never proves the "same method name, wrong type" negative. Every violation fixture that uses the instance-call path injects a blocklisted contract (StatefulGuard, Session, Mailer). There is no fixture where an App\Actions\* constructor injects, say, a repository or a domain service whose method happens to share a blocklisted name (push, send, delete, put) and asserts zero violations. checkInstanceCall at :444-477 gates on $constructorPropertyTypes[$propertyName] being a BLOCKLIST key, so a $this->widgetRepo->delete() won't match — the logic is correct. But the suite proves the positive matches and the namespace/read negatives; it does not pin the "same method name, wrong type" negative. Add one fixture: an Action injecting a non-blocklisted service exposing delete()/push()/send(), called inside the closure, asserting []. That's the assertion that guards against a future blocklist-by-method-name regression.

Nits

The Cache facade entry at :192-195 omits restoreLock while the Repository contract entry at :188-191 includes it (annotated "contract only" in the PR body table). That asymmetry is intentional per the body, but nothing in the code comments records it, so the next editor will likely "fix" the drift by adding it. One inline comment on the divergence would save that round-trip.

Praise

The top-level-transaction discovery in collectTopLevelTransactions at :331-356 — stopping recursion at the first transaction() call and delegating nested-transaction handling to the closure walker — is the right call and correctly reasoned in the docblock at :304-311: counting nested transactions at discovery time would double-report, since the outer closure walk already reaches them. The NestedTransactionMutationDetected fixture proves a nested mutation reports exactly once. That's a non-obvious dedup boundary handled deliberately rather than stumbled into.

The PR also correctly scopes itself to ADR-0029 rule 3 (success-side, post-commit mutation) and leaves rule 2 (failure-side sentinel-return / throw-inside-closure) to queue #85, with the reasoning that bundling would couple two distinct AST walks and inflate false-positive surface. That's the correct split — a throw-detection walk has a different shape and a different false-positive profile, and merging them would make both harder to reason about.

Verdict: COMMENT. No blockers — the rule is correct on its core path and faithfully enforces the ADR's success-side contract. The three concerns (deferred-closure false positives, remember straddling the read/write line, and the missing same-name-wrong-type negative test) are worth resolving before the cascade audit across consumer territories, since each one shapes either false-positive volume or regression coverage at the moment the rule starts firing on real code. They don't gate the merge of the rule itself.

[Goosterhof]

Restacked for v0.4.0 — re-review requested.

v0.3.0 is now tagged: #25 merged at e3572b9 and the tag is pushed (Packagist auto-publishes). The old shared [Unreleased] block is frozen as [0.3.0]; this rule's CHANGELOG entry was repositioned into a fresh [Unreleased] = v0.4.0 material.

The rule source, tests, and extension.neon registration are byte-identical to your prior approval — only the CHANGELOG entry moved. The dismissed approval is purely the rebase-dismisses-stale-review mechanic, not a content change.

This is the bottom of the stack. Merge order: #27 → #28 → #26 (each contains the prior, so bottom-up merges are conflict-free).