Merge branch 'php8.4' of https://github.com/LongTermSupport/php-qa-ci into php8.4

Author: LTSCommerce

.claude/agents/php-qa-ci_phpstan-rule-creator.md

@@ -12,6 +12,7 @@ specific bug patterns at static analysis level, preventing entire classes of bug
 ## Your Role
 
 You create custom PHPStan rules as part of the "Defence Before Fix" strategy:
+
 1. Analyse the pattern to detect
 2. Create a PHPStan rule class
 3. Register it in phpstan.neon
@@ -20,6 +21,7 @@ You create custom PHPStan rules as part of the "Defence Before Fix" strategy:
 ## Prerequisites - Read First
 
 Before creating any rule, read these files:
+
 1. `qaConfig/PHPStan/CLAUDE.md` -- Documentation on custom rules for this project
 2. Existing rules in `qaConfig/PHPStan/Rules/` -- For style and pattern reference
 3. The example rules in `.claude/skills/defence-before-fix/examples/` -- For common patterns
@@ -36,6 +38,7 @@ Before creating any rule, read these files:
 ### Step 1: Understand the Pattern
 
 From the prompt, identify:
+
 - What AST node type to inspect (Attribute, ClassConst, MethodCall, etc.)
 - What condition makes it a violation
 - What the correct pattern looks like
@@ -45,17 +48,17 @@ From the prompt, identify:
 
 Common PhpParser node types for rules:
 
-| Pattern to Detect | Node Type | Class |
-|---|---|---|
-| Attribute arguments | `Node\Attribute` | Route paths, names, etc. |
-| Class constants | `Node\Stmt\ClassConst` | Magic string constants |
-| Method calls | `Node\Expr\MethodCall` | Dangerous method patterns |
-| Function calls | `Node\Expr\FuncCall` | Banned functions |
-| Catch blocks | `Node\Stmt\Catch_` | Empty catches |
-| Binary operations | `Node\Expr\BinaryOp\Coalesce` | Silent defaults (`??`) |
-| Property access | `Node\Expr\PropertyFetch` | Unsafe property access |
-| String literals | `Node\Scalar\String_` | Magic strings |
-| Return statements | `Node\Stmt\Return_` | Missing return checks |
+| Pattern to Detect   | Node Type                     | Class                     |
+| ------------------- | ----------------------------- | ------------------------- |
+| Attribute arguments | `Node\Attribute`              | Route paths, names, etc.  |
+| Class constants     | `Node\Stmt\ClassConst`        | Magic string constants    |
+| Method calls        | `Node\Expr\MethodCall`        | Dangerous method patterns |
+| Function calls      | `Node\Expr\FuncCall`          | Banned functions          |
+| Catch blocks        | `Node\Stmt\Catch_`            | Empty catches             |
+| Binary operations   | `Node\Expr\BinaryOp\Coalesce` | Silent defaults (`??`)    |
+| Property access     | `Node\Expr\PropertyFetch`     | Unsafe property access    |
+| String literals     | `Node\Scalar\String_`         | Magic strings             |
+| Return statements   | `Node\Stmt\Return_`           | Missing return checks     |
 
 ### Step 3: Create the Rule Class
 
@@ -122,6 +125,7 @@ Read the current `qaConfig/phpstan.neon` and add the new rule under `rules:`.
 ### Step 5: Return Summary
 
 Report:
+
 - Rule class created at: [path]
 - Registered in: qaConfig/phpstan.neon
 - Pattern detected: [description]
@@ -131,12 +135,14 @@ Report:
 ## Rule Quality Standards
 
 ### Error Messages MUST:
+
 - Explain WHAT is wrong
 - Explain HOW to fix it
 - Include the actual problematic value when possible
 - Use sprintf for dynamic content
 
 ### Rules MUST:
+
 - Use `->identifier('ruleName.violationType')` on every error
 - Have comprehensive docblock explaining the problem and solution
 - Include WRONG/RIGHT examples in the docblock
@@ -145,11 +151,21 @@ Report:
 - Be `final class`
 
 ### Rules MUST NOT:
+
 - Use `@phpstan-ignore` suppressions
 - Modify any code (analysis only)
 - Have side effects
 - Depend on runtime state
 
+### Make "delete to silence" visibly wrong
+
+Where the pattern is a dead/half-wired contract, design the rule to flag the **absent
+producer** (the contract a consumer reads but nothing supplies), NOT merely the presence
+of the member. That way the cheapest way to satisfy the rule is to WIRE the contract to a
+real producer — not to delete the member and bake in the broken state. Wire-don't-delete
+is the intended fix; the rule should make deletion the obviously-wrong shortcut. See
+`CLAUDE/DefenceBeforeFix.md`.
+
 ## Using Scope for Type Information
 
 The `Scope` parameter provides type information about the current context:
@@ -185,6 +201,7 @@ if ($type instanceof ObjectType && $type->getClassName() === SomeClass::class) {
 ## Remember
 
 You are a CREATOR, not a RUNNER or FIXER. Your job is to:
+
 - Understand the bug pattern
 - Create a PHPStan rule that detects it
 - Register the rule

.claude/skills/defence-before-fix/SKILL.md

@@ -32,15 +32,32 @@ Reference: https://ltscommerce.dev/articles/defence-before-fix-static-analysis
 One PHPStan rule prevents an entire class of bugs across all future commits and
 untested code paths. Tests only catch specific manifestations. Rules catch the pattern.
 
+## Core Framing: Net and Filter
+
+> Static analysis is the NET. TDD is the FILTER. Together they are belt and braces.
+
+- **NET (the static rule):** catches the whole *class* structurally — cheap, broad,
+  permanent. Once it exists the class cannot silently recur on any future commit.
+- **FILTER (TDD):** reproduces the *specific instance* on the real production path and
+  proves the fix. A failing test that goes green is honest evidence this instance is
+  resolved.
+- Neither alone suffices — a rule with no test can be gamed structurally; a test with no
+  rule lets the class recur elsewhere.
+
+Full philosophy (wire-don't-delete, coverage theatre, nullable both-paths, issue-type
+applicability): see `CLAUDE/DefenceBeforeFix.md` in php-qa-ci (read in vendor).
+
 ## Prerequisites
 
 Before starting, verify PHPStan rule infrastructure exists:
+
 - `qaConfig/PHPStan/Rules/` directory exists
 - `qaConfig/PHPStan/CLAUDE.md` documentation exists
 - `QaConfig\` PSR-4 entry in `composer.json` autoload-dev
 - `qaConfig/phpstan.neon` has a `rules:` section
 
 If any are missing, run deployment:
+
 ```bash
 vendor/lts/php-qa-ci/scripts/deploy-skills.bash vendor/lts/php-qa-ci .
 ```
@@ -49,14 +66,14 @@ vendor/lts/php-qa-ci/scripts/deploy-skills.bash vendor/lts/php-qa-ci .
 
 Parse the user's request to determine which phase to start at:
 
-| User Says | Start Phase | Notes |
-|---|---|---|
-| "defence before fix [bug description]" | Phase 1 | Full workflow |
-| "create a PHPStan rule for [pattern]" | Phase 2 | Pattern already understood |
-| "detect [pattern] with static analysis" | Phase 2 | Pattern already understood |
-| "ratchet this bug class" | Phase 2 | Pattern already analysed |
-| "reproduce this bug with tests" | Phase 3 | Rule already created |
-| "fix [bug] and verify" | Phase 4 | Tests already written |
+| User Says                               | Start Phase | Notes                      |
+| --------------------------------------- | ----------- | -------------------------- |
+| "defence before fix [bug description]"  | Phase 1     | Full workflow              |
+| "create a PHPStan rule for [pattern]"   | Phase 2     | Pattern already understood |
+| "detect [pattern] with static analysis" | Phase 2     | Pattern already understood |
+| "ratchet this bug class"                | Phase 2     | Pattern already analysed   |
+| "reproduce this bug with tests"         | Phase 3     | Rule already created       |
+| "fix [bug] and verify"                  | Phase 4     | Tests already written      |
 
 ## Phase 1: ANALYSE
 
@@ -69,17 +86,20 @@ This phase is primarily manual/guided. Help the user by:
    Magic string? Type coercion? Empty catch?"
 
 2. **Find all instances** — Search the codebase for the same pattern:
+
    ```
    Use Grep tool to search for the pattern across the codebase
    ```
 
 3. **Document the pattern** — Describe:
+
    - What the buggy code looks like (AST-level: what node types are involved?)
    - What the correct code looks like
    - Why the pattern is dangerous
    - How many instances exist
 
 4. **Assess feasibility** — Can PHPStan detect this at the AST level?
+
    - Simple patterns (magic strings, empty catches, silent defaults): YES
    - Type-level patterns (wrong return types, missing checks): MAYBE (needs Scope)
    - Domain logic patterns (wrong business rules): NO — use tests instead
@@ -144,11 +164,13 @@ manually edit the rule in `qaConfig/PHPStan/Rules/`.
 This phase uses standard TDD workflow:
 
 1. **Write failing tests** — Each test should:
+
    - Target a specific bug instance (not the general pattern — the rule handles that)
    - Assert the CORRECT behaviour (what should happen after the fix)
    - Currently FAIL (proving the bug exists)
 
 2. **Run tests to confirm they fail:**
+
    ```
    Use Skill tool:
      skill: "phpunit-runner"
@@ -160,27 +182,49 @@ This phase uses standard TDD workflow:
 The tests (Phase 3) verify the specific fix works correctly. Both are needed — they
 serve different purposes.
 
+**Coverage-theatre warning:** the failing test MUST exercise the real production path. A
+fixture that supplies a value production never sets yields false-green line/branch
+coverage and proves nothing — it is exactly how a "dead contract" (a consumer with no
+producer) ships green. Drive the real producer, not a hand-fed fixture value.
+
+**Nullable ⇒ both paths:** a nullable member is TWO code paths (value-present and null).
+Prove BOTH — a with-value test AND a null test. A single populated-path test leaves the
+other branch unexercised. Prefer non-nullable types where null is not a genuinely valid
+domain state; nullable should be reserved for legitimately-absent values.
+
 ## Phase 4: FIX (Implementation)
 
 **Goal:** Fix the code so tests pass and PHPStan rules pass.
 
+**Wire, don't delete.** GREEN must come from making the code do its job — wiring the
+flagged contract to a real producer, proven by a production-path test. Deleting the
+flagged element to silence the rule **bakes in the broken / half-built state** and is
+forbidden unless it is a deliberate, agreed scope decision (the element is genuinely
+unwanted dead code with no intended producer). Clearing red by deletion is never the
+default move.
+
 1. **Implement fixes** — Make the failing tests pass.
 
 2. **Verify PHPStan rules pass:**
+
    ```
    Use Skill tool:
      skill: "phpstan-runner"
    ```
+
    The rule violations from Phase 2 should now be resolved.
 
 3. **Run full QA:**
+
    ```
    Use Skill tool:
      skill: "qa"
    ```
+
    Run allCS then allStatic to verify everything is clean.
 
 4. **Done** — The bug class is now permanently prevented:
+
    - PHPStan rule catches the pattern in all future code
    - Tests verify the specific fix works
    - The ratchet has turned — quality only goes one way
@@ -200,16 +244,33 @@ The plan provides structure for tracking progress across phases.
 
 ## Common Patterns and Their Rules
 
-| Bug Pattern | PHPStan Node Type | Example Rule |
-|---|---|---|
-| Magic strings in attributes | `Node\Attribute` | RoutePathMustUseConstantsRule |
-| Silent defaults (`?? ''`) | `Node\Expr\BinaryOp\Coalesce` | ExampleSilentDefaultRule |
-| Empty catch blocks | `Node\Stmt\Catch_` | ExampleEmptyCatchRule |
-| Missing return value check | `Node\Expr\MethodCall` | Custom per-method |
-| Constant not composed | `Node\Stmt\ClassConst` | RouteNameConstantMustComposeRule |
+| Bug Pattern                 | PHPStan Node Type             | Example Rule                     |
+| --------------------------- | ----------------------------- | -------------------------------- |
+| Magic strings in attributes | `Node\Attribute`              | RoutePathMustUseConstantsRule    |
+| Silent defaults (`?? ''`)   | `Node\Expr\BinaryOp\Coalesce` | ExampleSilentDefaultRule         |
+| Empty catch blocks          | `Node\Stmt\Catch_`            | ExampleEmptyCatchRule            |
+| Missing return value check  | `Node\Expr\MethodCall`        | Custom per-method                |
+| Constant not composed       | `Node\Stmt\ClassConst`        | RouteNameConstantMustComposeRule |
 
 Example rules are in `.claude/skills/defence-before-fix/examples/`.
 
+## Does TDD Apply? By Issue Type
+
+TDD applies where the issue type supports it — the dividing line is *is there behaviour
+to assert?*
+
+| Issue type                                 | Static rule (NET)         | TDD (FILTER)              |
+| ------------------------------------------ | ------------------------- | ------------------------- |
+| Pure coding-standards / style / formatting | yes — the rule IS the fix | no — nothing to assert    |
+| Behaviour / procedure / contract defect    | yes — catches the class   | yes — reproduce & prove   |
+| Dead-contract / coverage-theatre           | yes — catches the class   | yes — via PRODUCTION path |
+| Nullable member                            | yes — catches the class   | yes — BOTH paths asserted |
+
+Rule of thumb: if you can write an assertion about behaviour that fails before the fix
+and passes after, TDD applies. If the rule is purely structural with no behaviour to
+assert, the static rule alone is the complete defence. Full detail in
+`CLAUDE/DefenceBeforeFix.md`.
+
 ## When NOT to Use This Skill
 
 - **One-off bugs** with no general pattern — just fix and test

CLAUDE/DefenceBeforeFix.md

@@ -0,0 +1,109 @@
+# Defence Before Fix — Net and Filter
+
+This document is the single source of truth for the **Defence Before Fix** philosophy.
+The `defence-before-fix` skill orchestrates the workflow; this doc states *why* it is
+shaped the way it is, and the rules that keep a "green" honest.
+
+Reference: https://ltscommerce.dev/articles/defence-before-fix-static-analysis
+
+## Canonical Framing
+
+> Static analysis is the NET. TDD is the FILTER. Together they are belt and braces.
+
+- **The NET (a PHPStan / CS rule).** Catches the whole *class* of problem at the
+  structural level. Cheap, broad, and permanent: once the rule exists, the bug class
+  can never silently recur on any future commit or any untested code path. This is the
+  ratchet — quality only turns one way.
+- **The FILTER (TDD).** Zeroes in on the *specific instance*. A failing test reproduces
+  the actual defect on the real production path; making it pass proves *this* instance is
+  genuinely fixed, not merely silenced.
+- **Belt and braces.** The net guarantees the class is caught forever; the filter
+  guarantees each instance is truly resolved. Neither alone suffices: a rule with no test
+  can be satisfied by gaming the structure; a test with no rule lets the class recur
+  elsewhere.
+
+## Fix by making it work — never by deleting
+
+When a rule goes RED, the correct GREEN comes from **making the code do its job**, not
+from deleting the flagged element to silence the rule.
+
+A classic trap is a **dead contract**: a consumer reads an optional member that no real
+producer ever supplies. It can ship green as **coverage theatre** — a fixture sets the
+value that production never sets, so line/branch coverage looks green on a path
+production never actually takes.
+
+```php
+final class Foo
+{
+    // RED: $bar is consumed by a renderer, but no production caller ever passes it.
+    public function __construct(public readonly ?string $bar = null) {}
+}
+```
+
+- **WRONG fix:** delete `$bar` to clear the rule. This bakes in the broken / half-built
+  state — it removes the symptom and the half-finished feature in one stroke, and the
+  rule goes quiet for the wrong reason.
+- **RIGHT fix:** WIRE `$bar` to a genuine producer and prove it with a test that
+  exercises the **production** path (not a fixture that hand-feeds `$bar`). Only then is
+  the contract live and the GREEN honest.
+- **Deleting is correct ONLY** when the member is genuinely unwanted dead code with no
+  intended producer — a deliberate scope decision, not a reflex to clear a red rule.
+
+## Nullable members: test BOTH paths
+
+A nullable member introduces **two** code paths — value-present and null. **Both must be
+proven**: a with-value test AND a null test. A single populated-path test is exactly the
+coverage-theatre trap above — it proves one branch and leaves the other unexercised,
+where a dead contract can hide.
+
+```php
+// Foo above has a nullable ?string $bar — BOTH are required:
+//   test 1: new Foo('x')   asserts the value-present behaviour
+//   test 2: new Foo(null)  asserts the value-absent behaviour
+```
+
+**Avoid nullable unless null is a genuinely valid domain state.** If a value is always
+known, type it non-nullable — fewer paths, and no false "optional" that can rot into a
+dead contract. Reserve nullable for states that are legitimately absent.
+
+## Does TDD apply? By issue type
+
+TDD applies **where the issue type supports it**. The dividing line is simple: *is there
+behaviour to assert?*
+
+| Issue type                                 | Static rule (NET)         | TDD (FILTER)              | Why                                                                    |
+| ------------------------------------------ | ------------------------- | ------------------------- | ---------------------------------------------------------------------- |
+| Pure coding-standards / style / formatting | yes — the rule IS the fix | no — nothing to assert    | No runtime behaviour to assert; the rule both defines and enforces it. |
+| Behaviour / procedure / contract defect    | yes — catches the class   | yes — reproduce & prove   | Real behaviour exists; write a failing test, then fix to green.        |
+| Dead-contract / coverage-theatre           | yes — catches the class   | yes — via PRODUCTION path | Test must drive the real producer, not a fixture-fed value.            |
+| Nullable member                            | yes — catches the class   | yes — BOTH paths asserted | Two code paths exist (value-present and null); prove each.             |
+
+**Rule of thumb:** if you can write an assertion about behaviour that would fail before
+the fix and pass after, TDD applies — use it. If the rule is purely structural/stylistic
+with no behaviour to assert, the static rule alone is the complete defence.
+
+## The Ratchet in Practice
+
+A worked illustration of the philosophy (shape, not specifics): a tightened static-analysis
+ratchet — e.g. bumping the bundled analyser to a stricter version — surfaces a batch of
+pre-existing latent errors that the looser net never caught. The discipline is to **fix
+every surfaced instance at root cause and never suppress**:
+
+- No `@phpstan-ignore`, no baseline entries, no `@var` forcing, no cast-to-silence.
+- Each error is a real defect the stricter net just made visible; resolve the underlying
+  type/logic issue so the code is genuinely correct.
+- The result is a permanent gain: the net is now stricter for all future commits, and the
+  backlog it exposed is gone rather than papered over.
+
+This is the same net-and-filter principle applied at the tooling level: tightening the net
+is only worthwhile if every instance it catches is honestly fixed.
+
+## Cross-Reference
+
+- Workflow skill: `.claude/skills/defence-before-fix/SKILL.md` (model-invoked; the
+  4-phase ANALYSE → DETECT → TDD → FIX ratchet).
+- Rule authoring: `qaConfig/PHPStan/CLAUDE.md` (deployed into each project) and the
+  `php-qa-ci_phpstan-rule-creator` agent.
+- Project root signpost: the auto-generated `<phpqaci>...</phpqaci>` block in the project
+  root `CLAUDE.md` carries a terse pointer back here (written on every
+  `composer install`/`update`).