feat(code-review): walk the full class-refactoring lens on every CR

[pekral]

Summary

The ask: every CR must run class-refactoring in read-only mode and write the things that skill defines into the report.

What I found: the lens was already wired in. @skills/class-refactoring/SKILL.md with MODE=cr sits in the Always run set of all four CR skills (code-review, code-review-github, code-review-jira, code-review-bugsnag). So the invocation was never the gap.

The real gap: the lens was described narrowly at every call site — "surface DRY duplication, single-responsibility breaches, oversized methods" — and the diff-scoped checklist in @rules/code-review/general.mdc was a closed six-bullet list. A reviewer reading either one literally walked a fraction of what the skill actually defines, and everything outside that fraction never reached the published report. The skill defines far more: speculative interfaces, pass-through Actions, >4 parameters → DTO, Simplicity First, business-logic-layer placement, Repository scope, query performance non-regression, intention-revealing extraction, naming, nesting depth.

The fix — three changes:

1. Open the closed lists. Both the rule's checklist and each CR skill's invocation line now say explicitly that the enumeration is the high-frequency subset, not the boundary of the walk, and that the lens walks the skill's complete guideline set (Refactoring Guidelines + Laravel Context + the behavior-preservation clauses of Execution). A caller's one-line summary can no longer be read as permission to narrow it.

2. Routing & no-drop contract — new step 5 in @rules/code-review/general.mdc Refactoring & Tech Debt (DRY) Analysis — diff-scoped detail. Every item the lens returns reaches the report:

   • underlying rule declares a severity (speculative interfaces, Simplicity First, >4 params, pass-through Action, Repository scope, read/write separation, layer placement, SQL perf non-regression) → the item goes to that Critical / Moderate / Minor bucket with the four reproducer fields; a SQL performance item goes to ## Database Analysis;
   • no severity (DRY, data-shaping duplication, oversized methods, nesting, Livewire/Blade splitting, ->when() composition) → Refactoring (DRY / tech debt) in-scope, Refactoring proposals out-of-scope;
   • de-duplication: a violation already raised by Core Analysis / Strict rule compliance stays there and the lens's duplicate is dropped — the lens is a second pass over the same rules, not a second finding. Each violation is reported exactly once.

3. Completeness contract in the skill. class-refactoring now states in Modes that MODE=cr walks its full guideline set and that the caller must render every returned item; each item carries the matched guideline so the CR can route and de-duplicate it. The four CR output templates carry that guideline in the Why: field.

Docs / prompt-contract change only — no PHP source touched.

Testing

• composer skill-check → 0 errors, 0 warnings, 62 skills
• vendor/bin/pest --no-coverage tests → 324 passed (1856 assertions)
• composer build → skill-check / composer-normalize / phpcs / pint / rector / phpstan / composer audit all clean; test:coverage could not run locally (no PCOV driver in this environment — pre-existing, unrelated to this diff, which touches zero PHP source). CI runs it.

How to verify the behavior change: run any CR (/code-review, code-review-github, …) on a PR that contains, say, a single-consumer project interface or a 5-parameter constructor. Before this change the refactoring lens had no instruction to look for either; now it walks them, and the routing contract sends them to the Moderate / Critical buckets rather than letting them fall out of the report.

Risk areas

• Double-reporting. The lens re-walks rules that Core Analysis and Strict rule compliance already cover. The de-duplication clause in step 5 is what keeps a violation from appearing twice — worth watching on the first few CR runs that the same finding does not show up in both a severity bucket and the Refactoring section.
• Report length. A broader walk means more items on large diffs. The existing "omit empty sections" rules still apply, but expect richer Refactoring sections.

TODO

• The code-review-bugsnag wrapper keeps its Refactoring analysis as a prose paragraph rather than the numbered checklist the GitHub / JIRA wrappers use. It now points at the canonical routing contract, but converging the three wrappers on one shared checklist would remove the remaining duplication. Deferred — it is a structural docs refactor beyond this assignment.

[pekral]

CR status — resolved items

Review loop converged after 2 iterations: 0 Critical · 0 Moderate.

• New normative contract shipped with no test pinning it (Critical)

  • Why: the Completeness contract and Routing & no-drop contract landed with no content-pin test.
  • Reason: in this repo the content-pin test in tests/Installer/CodeReviewContentTest.php is the only mechanism that stops normative rule text from drifting — the exact failure mode this PR exists to fix, left unguarded.
  • Solution: added every CR walks the full class-refactoring guideline set and drops no returned item, pinning the contract across the rule, the lens skill, and all four CR skills.

• Template Why: field mandated a value that does not always exist (Moderate)

  • Why: the field turned an or into a plus, demanding a class-refactoring guideline on every Refactoring item.
  • Reason: the section also holds items the lens never produced (->when() composition, Livewire/Blade splitting), and @rules/laravel/architecture.mdc does not apply on non-Laravel projects.
  • Solution: restored the alternative form across all four CR output templates — the guideline is required only on lens-produced items.

• The no-drop contract contained an escape hatch (Moderate)

  • Why: the clause "a guideline that matched but produced no reportable item is simply not rendered" sanctioned dropping a matched item while recording nothing.
  • Reason: it reopened the gap the contract closes and contradicted Output Rules Default severity for rule violations, which requires an exemption to be cited rather than suppressed.
  • Solution: matched and dropped are now mutually exclusive — a no-match renders nothing, an exempt match is rendered with its exemption cited.

• Normative guideline enumeration duplicated across five files (Refactoring / DRY)

  • Why: the walked-guideline list was restated near-verbatim in the rule plus four CR skills.
  • Reason: the next guideline added to class-refactoring would have to be hand-propagated to five places or they drift — the repo convention is canonical-home + reference.
  • Solution: the list now lives only in @rules/code-review/general.mdc; the four CR skills reference that section. The reuse-first gate routing (issue FIX | CR skills #722) is kept explicit in the GitHub / JIRA wrappers, since a pin test depends on it.

Reviewer threads: 0 (no line-anchored threads on this PR).
Quality gates: skill-check, composer-normalize, phpcs, pint, rector, phpstan, composer audit — all clean. Tests: 325 passed (1870 assertions). test:coverage could not run locally (no PCOV driver in this environment; the diff touches no PHP source); CI runs it.

[pekral]

CR status — final diff (head 075012b)

Re-run after docs(memory): record content-pil-tests… landed on top of the converged state, so the review covers the exact commits being merged.

Status: 0 Critical · 0 Moderate · 0 Minor

Delta since the converged review (ed98090): one commit, docs(memory), appending a single PROJECT_MEMORY.md entry. Walked it against the repo rules — no code paths, no secrets or PII, entry shape matches the existing records (Trigger / Rule / Example / Source / Role). No findings.

The three CR findings and the DRY item from the previous run remain resolved (see the earlier CR status — resolved items comment). Quality gates green; 325 tests passing.