Skip to content

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

Merged
pekral merged 1 commit into
masterfrom
feat/733-self-documenting-code-cr-lens
Jul 15, 2026
Merged

feat(code-review): add self-documenting comment & doc hygiene lens#735
pekral merged 1 commit into
masterfrom
feat/733-self-documenting-code-cr-lens

Conversation

@pekral

@pekral pekral commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Summary

  • Adds a new normative Core Analysis lens Self-Documenting Code — Comment & Doc Hygiene to rules/code-review/general.mdc: comments / PHPDoc / inline docs added or modified by the diff that only restate what the code already says must be deleted.
  • Two preserved exceptions that must never be flagged for deletion: (1) rationale / considered alternatives / domain language, which belongs in ADRs & glossaries; (2) navigation pointers (@rules/…, @skills/…, links to collaborating classes/methods).
  • Thin reference added to skills/code-review/SKILL.md Core Analysis walk so every CR wrapper (code-review-github, code-review-jira, code-review-bugsnag) inherits the lens through the always-run engine — no wrapper edits needed.
  • Content-pin test added to tests/Installer/CodeReviewContentTest.php pinning the lens prose and both exceptions verbatim.
  • CHANGELOG.md entry.

Closes #733

Test plan

  • composer build green locally (skill-check 100/100, phpcs, pint, rector, phpstan, 326 tests / 100% coverage)
  • argos / athena code review + security review loop (runs after this draft PR)

Adds a new normative Core Analysis lens to the CR skills: comments / PHPDoc /
inline docs added or modified by the diff that only restate what the code
already says must be deleted, with two preserved exceptions - rationale /
considered alternatives / domain language (belongs in ADRs & glossaries) and
navigation pointers (@rules/..., @skills/..., links to collaborating classes).

- rules/code-review/general.mdc: new "Self-Documenting Code - Comment & Doc
  Hygiene Analysis" section plus a Core Analysis Walk-through bullet.
- skills/code-review/SKILL.md: thin reference in the Core Analysis walk so
  every CR wrapper inherits it via the always-run engine.
- tests/Installer/CodeReviewContentTest.php: content-pin test for the lens
  prose and both exceptions.
- CHANGELOG.md: feat(code-review) entry.

Closes #733
@pekral

pekral commented Jul 15, 2026

Copy link
Copy Markdown
Owner Author

Status: CR done — converged (0 Critical · 0 Moderate)
Counts: Critical 0 · Moderate 0 · Minor 1 · Refactoring 0
Last updated: 2026-07-15
Issue tracker summary: posted summary to issue #733

Minor

[Minor] Security rationale / invariants are not named in Exception 1 — residual classification ambiguity. rules/code-review/general.mdc, section Self-Documenting Code — Comment & Doc Hygiene Analysis, Exception 1 (consolidated from the parallel security review). The "never delete" examples list performance / external-bug workaround / ordering / domain language, but security-rationale and security-invariant comments are not called out by name. The generic "answers why" clause already protects them, but a comment such as // constant-time compare — do not replace with === (timing attack) or // re-check ownership here (IDOR) could be misread as what-narration over a comparison and wrongly flagged for deletion. Suggested Fix: add one explicit example to Exception 1 — e.g. "the reason a security control exists (constant-time compare, an authorization re-check, a do-not-log / PII note) belongs to the why exception and is never deleted." Optional hardening; the whole dimension is Minor and never blocks merge.

Summary

The diff is pure normative prose plus a content-pin test across 4 files (rules/code-review/general.mdc, skills/code-review/SKILL.md, tests/Installer/CodeReviewContentTest.php, CHANGELOG.md). Quality / architecture / optimisation review: 0 findings. The new Self-Documenting Code — Comment & Doc Hygiene lens follows the class-refactoring lens pattern exactly (normative prose in the tracked rule + thin reference in the code-review engine's Core Analysis walk inherited by every wrapper + content-pin test shipped in the same PR); every pinned substring (Self-Documenting Code — Comment & Doc Hygiene, restates what the code already says, considered alternatives, ADR, glossar, navigation pointer) is backed verbatim by the prose. Its gating clause ("one finding per violation, never two") makes severity deterministic against the suppression / test-organization / naming walks, so there is no severity collision. It aligns verbatim with @rules/php/core-standards.mdc Documentation. Wrappers (github/jira/bugsnag) are deliberately not edited (Simplicity First — Core Analysis is inherited via the always-run engine). The lens dogfoods clean — the comments in its own test diff fall under the why / navigation exceptions and produce no finding. Assignment #733 conformant: both required exceptions (rationale / considered alternatives / domain language → ADRs & glossaries; navigation pointers) are named and marked "never delete". Consolidated with athena's parallel security CR (0 Critical / 0 Moderate / 1 Minor, listed above). reviewer comments: none. Coverage: CI "Quality Checks (PHP 8.5)" green on head 19e1a7d; diff carries no executable lines.

@pekral
pekral marked this pull request as ready for review July 15, 2026 11:14
@pekral
pekral merged commit bbb3c4e into master Jul 15, 2026
1 check passed
@pekral
pekral deleted the feat/733-self-documenting-code-cr-lens branch July 15, 2026 11:19
@pekral pekral mentioned this pull request Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FEAT | AI code review clean code

1 participant