feat(code-review): add self-documenting comment & doc hygiene lens

CHANGELOG.md

@@ -4,6 +4,8 @@ All notable changes to `cursor-rules` will be documented in this file.
 
 ## [Unreleased]
 
+- ✨ **Added** `feat(code-review)`: every code review now walks a new **Self-Documenting Code — Comment & Doc Hygiene** lens (#733) — for every comment / PHPDoc / inline doc the diff adds or modifies, flag redundant *what*-narration that restates what the code already says and require its deletion, while never flagging the two preserved exceptions: rationale / considered alternatives / domain language (belongs in ADRs & glossaries) and navigation pointers (`@rules/…`, `@skills/…`, links to collaborating classes/methods). Documented in `@rules/code-review/general.mdc` *Self-Documenting Code — Comment & Doc Hygiene Analysis*, referenced from `skills/code-review/SKILL.md`'s Core Analysis walk (inherited by every CR wrapper), pinned by `tests/Installer/CodeReviewContentTest.php`.
+
 - 📝 **Changed**: every code review now runs **`class-refactoring` with `MODE=cr` across its complete guideline set**, and **every item the lens returns reaches the published report**. The read-only lens was already in the always-run set of all four CR skills (`code-review`, `code-review-github`, `code-review-jira`, `code-review-bugsnag`), but each invocation described it narrowly ("DRY duplication, single-responsibility breaches, oversized methods") and the diff-scoped checklist in `@rules/code-review/general.mdc` was a **closed six-bullet list** — so a reviewer reading either literally walked a fraction of what the skill defines and the rest never made it into the report. The closed lists are now explicitly the **high-frequency subset, not the boundary of the walk**: speculative interfaces, pass-through Actions, `>4` parameters → DTO, Simplicity First, business-logic-layer placement, Repository scope, query performance non-regression, intention-revealing extraction, naming, and nesting depth are walked too. A new **Routing & no-drop contract** (`@rules/code-review/general.mdc` *Refactoring & Tech Debt (DRY) Analysis — diff-scoped detail*, step 5) says where each returned item lands: an item whose underlying rule declares a severity goes to its **Critical / Moderate / Minor** bucket with the four reproducer fields (a SQL performance item to `## Database Analysis`), everything else to **Refactoring (DRY / tech debt)** / **Refactoring proposals** — and a violation already raised by the Core Analysis / Strict rule compliance walk is kept there rather than duplicated, so each violation is reported exactly once. `@skills/class-refactoring/SKILL.md` gains the matching **Completeness contract** in *Modes* and now returns the **matched guideline** with every item so the CR can route and de-duplicate it; the four CR output templates ask for that guideline in the `Why:` field on lens-produced items. The walked-guideline list lives **only** in the rule — the four CR skills reference it instead of restating it, so the next guideline added to `class-refactoring` cannot drift across five files. Pinned by `tests/Installer/CodeReviewContentTest.php` (*every CR walks the full class-refactoring guideline set and drops no returned item*). Per user request — *při jakémkoliv CR chci, aby se spustil i skill class-refactoring v readonly mode a do reportu připsal i věci, které jsou definované v tomto skillu*.
 
 - 📝 **Changed**: the read-only **code-review agents (`argos`, `athena`) may now opt into an isolated git worktree** to run their parallel CR pass when isolation is genuinely needed, and **`daidalos` cleans every CR worktree up** after the run / merge so the repository and codebase stay clean. This refines the earlier *git worktrees are forbidden across the whole workflow* policy: the **writing path (`talos`) still never uses worktrees** and concurrent writers still serialise on the single shared working tree via the write-lock — only the read-only CR pass gains the explicit-request opt-in of `@rules/git/general.mdc` *Worktrees / Workspaces*, which carries no write-lock and never contends with the writing path. Each CR agent creates the worktree with `git worktree add` (a throwaway read-only review checkout — never edits / commits / pushes there), records its path in the handoff, and either hands it to `daidalos` for removal in cleanup (step 7: `git worktree remove` + `git worktree prune`, no `--force`) or removes it itself when run standalone. Documented in `agents/daidalos.md` *Concurrency & the working-tree write-lock* (now *Worktrees — writing path stays on the shared tree, read-only CR may isolate*) + step 6/7, `agents/argos.md` / `agents/athena.md` *Parallel review worktree*, and mirrored in `docs/agents.md`. Per user request — *CR agenti smí používat git worktrees pro paralelní code review; Daidalos po dokončení a merge vše uklidí, aby v repository nebyl bordel*.

rules/code-review/general.mdc

@@ -112,6 +112,15 @@ The detailed walk-throughs below are applied by `@skills/code-review/SKILL.md` (
 - **Request → DTO transformation belongs in the FormRequest, not the controller** — flag every controller method added or modified by the diff that calls a DTO factory (`SomeData::from($request)` / `SomeData::fromRequest($request)` or any DTO constructor taking the request) directly in the controller body instead of delegating to a `toDto()` method on the endpoint's FormRequest. The FormRequest's `toDto()` may internally call the DTO named constructor — the named constructor stays valid, it just runs inside the FormRequest so the transformation lives next to the validation rules. **Suggested Fix:** add a public `toDto(): SomeData` method to the FormRequest (delegating to `SomeData::fromRequest($this)` as needed) and replace the controller call with `$request->toDto()`. Applies where a FormRequest already exists for the endpoint; one-off mappings with no reuse and endpoints without a FormRequest are exempt (see `@rules/laravel/architecture.mdc` Controllers and Other Entry Points). Severity: **Moderate**.
 - Data Modification (DRY) — enumerate every place in the changes that modifies data before it is saved or passed downstream (DTO mapping, payload shaping, key renaming, default fallbacks, format normalization, business-driven derivation). Cross-check against existing entry points; if the same shaping appears in more than one Action / Service / controller / job / listener / Livewire component / command, flag it and require consolidation into the canonical layer (Data Builder, DTO named constructor, Data Validator, ModelManager, Repository — see `@rules/laravel/architecture.mdc` Data Modification (DRY) section). Output the list of modification places explicitly in the review so the duplication picture is visible.
 - **Entry-point error handling for known failures (Laravel)** — for every **user-facing** entry point added or modified by the diff (controller resource action, single-action `__invoke()`, Livewire component — i.e. anything that returns a synchronous response to a user), check how it handles **known, expected** post-validation failure modes versus **unexpected** errors, per `@rules/laravel/architecture.mdc` *Error Handling at the Entry-Point Boundary* and `@rules/api/general.mdc` *Handling Known Failures After Validation*. Raise a finding when an endpoint that can fail on an expected domain condition (business-rule violation, wrong resource state, unavailable external dependency, not-found / conflict / quota / payment-declined) carries **no handling at all**, so the exception bubbles uncaught into the global handler and the error reporter (Bugsnag) and the user gets a blank generic `500` with no actionable message — the fix is a typed domain exception rendered to a precise status + safe, localized (`__()`) message (centrally via the exception handler / a `render()` method, or via the controller catching that **specific** type). Also raise a finding for the opposite failure: a blanket `catch (\Throwable)` / `catch (\Exception)` / empty `catch {}` that swallows **unexpected** errors and hides them from the reporter, or converts an arbitrary exception into a fake-success — only the named known types may be caught; everything else must reach Bugsnag. **Async entry points (jobs, queued listeners, console commands) have no synchronous user**, so the precise-status / user-message half does not apply to them; for those, this bullet reduces to the catch-all half only — flag a blanket `catch (\Throwable)` / empty `catch {}` that hides a failure from Bugsnag, nothing about HTTP status. User messages must obey the safe-error contract (`@rules/security/backend.md` *Safe Validation & Error Messages* — no enumeration, no internal detail, no verbatim attacker input). Do **not** duplicate schema-validation findings (FormRequest / Data Validator → `ValidationException` → `422`) — this bullet covers runtime / domain failure modes that occur *after* validation passes. **Raise the finding once per endpoint** — this Core Analysis bullet is the home for it; do not also emit a separate Strict-rule-compliance entry for the same line citing `@rules/laravel/architecture.mdc` *Error Handling at the Entry-Point Boundary* or `@rules/api/general.mdc` *Handling Known Failures After Validation*. Severity: **Moderate** by default; escalate to **Critical** on a payment / auth / data-loss path.
+- **Self-documenting code — comment & doc hygiene (issue #733)** — for every comment / PHPDoc / inline doc the diff adds or modifies, flag redundant *what*-narration and require its deletion, per `@rules/code-review/general.mdc` *Self-Documenting Code — Comment & Doc Hygiene Analysis*. Never flag the two preserved exceptions (rationale / considered alternatives / domain language → ADRs & glossaries; navigation pointers). Severity: **Minor**.
+
+## Self-Documenting Code — Comment & Doc Hygiene Analysis
+
+- **Scope:** only comments / PHPDoc / inline documentation on lines the diff **adds or modifies**; never pre-existing comments the diff does not touch.
+- **Finding (require deletion):** a comment or docblock that **restates what the code already says** — a paraphrase of the statement that follows it (`// increment counter` above `$counter++`), a PHPDoc repeating the method name / steps with no domain meaning (`@param int $id The id`, "This method gets the user"), a redundant type docblock duplicating the native type (`@return void`, `@param string $name` on an already-typed parameter), commented-out code, changelog / history narration, dividing "section" comments, or a description of a trivial getter. **Suggested Fix:** delete the comment; if it was compensating for a poorly named symbol, rename the symbol instead, per `@rules/php/core-standards.mdc` *Documentation* ("Prefer self-documenting code over explanatory comments", "PHPDoc should describe intent and domain meaning, not restate the method name or implementation steps"). Severity: **Minor** (readability / noise, no behavior change; this dimension alone never blocks merge).
+- **Exception 1 — knowledge the code cannot express (never delete):** comments / docs that capture the *why*, not the *what* — **considered alternatives** and trade-offs (why this approach was chosen over another, the rationale for a workaround around an external bug, the justification for an ordering / performance choice) belong in an **ADR** / commit body / rationale comment; and **domain language** — glossary / ubiquitous-language definitions, business-rule explanations, non-trivial domain invariants or units, a reference to a spec or regulation that gives the term its meaning — belongs in a **glossary** / domain doc / an "intent & domain meaning" docblock. Never require deletion of a comment that answers *why* or defines a domain term.
+- **Exception 2 — navigation pointers (never delete):** thin cross-reference strings / "paths" that a reader (human or AI) follows through the source — a link to the canonical home of a rule or contract (`@rules/…`, `@skills/…`), a pointer to a collaborating class / method / file that owns the other half of a flow ("see `EmailRepository::getPendingReminders()`", "entry point: `App\Http\Controllers\…`"), issue / ADR back-links, "counterpart lives in `<file>`". A **navigation pointer** adds discoverability the code itself cannot express — it is not *what*-narration and must never be deleted.
+- **Gating (one finding per violation, never two):** when the same line is already raised by another walk (an `@`-suppression comment → *New static-analysis / linter suppression*; a stale test description → *Test organization*), keep that finding and skip this one. This dimension owns only comment *hygiene* (redundant *what*-narration), not the suppression or naming rules owned elsewhere.
 
 ## Refactoring & Tech Debt (DRY) Analysis — diff-scoped detail
 

skills/code-review/SKILL.md

@@ -84,7 +84,7 @@ Run this section only when the diff integrates with, modifies, or depends on a t
 - Business logic correctness
 - Missing or incorrect behavior
 - Type safety and error handling
-- **Full Core Analysis walk-through (canonical detail in `@rules/code-review/general.mdc` *Core Analysis Walk-through*).** Apply every bullet there to the diff and raise one finding per violation at the severity it declares: Reuse of existing logic, Action scope, Speculative interfaces, **Simplicity First**, method-parameter-count (>4 → DTO), public-method raw-array-vs-DTO, new static-analysis / linter suppression, **Strict rule compliance (mandatory walk-through)**, **Architecture conformance (Laravel)** (issue #530), **Test organization (issue #528)**, per-row DB operations in loops, variable ordering / lazy evaluation, object caching, new storage reuse analysis, SQL index reuse / performance non-regression, refactoring quality + test-coverage contract, data-validation encapsulation, pass-through Action, repository scope, inline Eloquent / read-write layer separation, Action-returns-HTTP-response, inline data mapping → Data Builder, inline validation guards / `throw_if` / `throw_unless` / enum-mode `match()` → Data Validator, only-Laravel-and-arch-layers class inventory, **Request → DTO transformation belongs in the FormRequest, not the controller**, Data Modification (DRY), and **Entry-point error handling for known failures (Laravel)**.
+- **Full Core Analysis walk-through (canonical detail in `@rules/code-review/general.mdc` *Core Analysis Walk-through*).** Apply every bullet there to the diff and raise one finding per violation at the severity it declares: Reuse of existing logic, Action scope, Speculative interfaces, **Simplicity First**, method-parameter-count (>4 → DTO), public-method raw-array-vs-DTO, new static-analysis / linter suppression, **Strict rule compliance (mandatory walk-through)**, **Architecture conformance (Laravel)** (issue #530), **Test organization (issue #528)**, per-row DB operations in loops, variable ordering / lazy evaluation, object caching, new storage reuse analysis, SQL index reuse / performance non-regression, refactoring quality + test-coverage contract, data-validation encapsulation, pass-through Action, repository scope, inline Eloquent / read-write layer separation, Action-returns-HTTP-response, inline data mapping → Data Builder, inline validation guards / `throw_if` / `throw_unless` / enum-mode `match()` → Data Validator, only-Laravel-and-arch-layers class inventory, **Request → DTO transformation belongs in the FormRequest, not the controller**, Data Modification (DRY), **Entry-point error handling for known failures (Laravel)**, and **Self-Documenting Code — Comment & Doc Hygiene**.
 ### Highest-Priority Fast Track
 
 Apply this subsection only when the source issue is flagged as **highest priority**, so the bug fix can deploy as fast as possible without sacrificing the Critical / Moderate gate.

tests/Installer/CodeReviewContentTest.php

@@ -887,3 +887,23 @@ function (): void {
         expect($content)->toContain('Refactoring & Tech Debt (DRY) Analysis — diff-scoped detail');
     }
 });
+
+test('every CR walks the self-documenting comment-hygiene lens and preserves its two exceptions (issue #733)', function (): void {
+    $packageDir = dirname(__DIR__, 2);
+
+    // Canonical home: the rule owns the lens prose and both exceptions.
+    $rule = (string) file_get_contents($packageDir . '/rules/code-review/general.mdc');
+    expect($rule)->toContain('Self-Documenting Code — Comment & Doc Hygiene');
+    expect($rule)->toContain('restates what the code already says');
+    // Exception 1 — rationale / considered alternatives / domain language (ADRs & glossaries) is never deleted.
+    expect($rule)->toContain('considered alternatives');
+    expect($rule)->toContain('ADR');
+    // glossary / glossaries
+    expect($rule)->toContain('glossar');
+    // Exception 2 — navigation pointers are never deleted.
+    expect($rule)->toContain('navigation pointer');
+
+    // The code-review engine names the lens in its Core Analysis walk; wrappers inherit it.
+    $codeReview = (string) file_get_contents($packageDir . '/skills/code-review/SKILL.md');
+    expect($codeReview)->toContain('Self-Documenting Code — Comment & Doc Hygiene');
+});