diff --git a/plugins/praxis/.claude-plugin/plugin.json b/plugins/praxis/.claude-plugin/plugin.json index 79f7da6..fe37331 100644 --- a/plugins/praxis/.claude-plugin/plugin.json +++ b/plugins/praxis/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "praxis", - "version": "1.4.0", + "version": "1.5.0", "description": "Development workflow -- issue planning, implementation, PR creation, code review with specialized reviewers, and project conventions", "author": { "name": "Jartan LLC", diff --git a/plugins/praxis/.codex-plugin/plugin.json b/plugins/praxis/.codex-plugin/plugin.json index b640740..0e3e253 100644 --- a/plugins/praxis/.codex-plugin/plugin.json +++ b/plugins/praxis/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "praxis", - "version": "1.4.0", + "version": "1.5.0", "description": "Development workflow -- issue planning, implementation, PR creation, code review with specialized reviewers, and project conventions", "interface": { "displayName": "Praxis", diff --git a/plugins/praxis/README.md b/plugins/praxis/README.md index c24d9c6..1cf8175 100644 --- a/plugins/praxis/README.md +++ b/plugins/praxis/README.md @@ -52,8 +52,9 @@ codex plugin add gitwise@grimoire ### Skills - **api-error-patterns** -- error response format, status codes -- **code-hygiene** -- zombie code (dead/reinvented/orphaned), tombstone comments +- **code-hygiene** -- zombie code (dead/reinvented/orphaned), truthful names, real implementations - **code-structure** -- structural craft: decompose on responsibility not size, deep modules, cohesion/coupling/interface/error-contract/data shape +- **comment-hygiene** -- comment truthfulness: KEEP/DELETE/EXEMPT/CONDITIONAL, tombstone and retold-fact detection, comment density - **docs-patterns** -- writing style, structure, brevity - **frontend-patterns** -- design tokens, mobile-first, component isolation - **logging-patterns** -- log levels, formatting, structured output diff --git a/plugins/praxis/agents/general-reviewer.md b/plugins/praxis/agents/general-reviewer.md index 485b118..1208552 100644 --- a/plugins/praxis/agents/general-reviewer.md +++ b/plugins/praxis/agents/general-reviewer.md @@ -8,6 +8,7 @@ permissionMode: plan skills: - gitwise:github-conventions - code-hygiene + - comment-hygiene - review-severity --- @@ -17,8 +18,8 @@ You are a senior code reviewer focusing on general quality and adherence to proj 1. **Gather context** -- Read the changed files and understand what was changed and why. 2. **Check project conventions** -- Read `CLAUDE.md` for project constraints. -3. **Index repeated comments** -- run `find-duplicate-comments.js` from the `code-hygiene` skill; a retelling is invisible from the file it sits in. -4. **Weigh the diff as a body** -- a per-comment pass at every site still misses an over-commented diff; run the density check in `code-hygiene`. +3. **Index repeated comments** -- run `find-duplicate-comments.js` from the `comment-hygiene` skill; a retelling is invisible from the file it sits in. +4. **Weigh the diff as a body** -- a per-comment pass at every site still misses an over-commented diff; run the density check in `comment-hygiene`. 5. **Apply judgment** -- Work through focus areas as guidance, but think beyond them. ## Confidence Filtering @@ -40,16 +41,17 @@ Guidance, not an exhaustive checklist -- tier each finding with the `review-seve ### Important -- Reinvention and orphaned abstractions (speculative generality), and comments that don't earn their place -- see `code-hygiene` +- Reinvention and orphaned abstractions (speculative generality) -- see `code-hygiene` +- Comments that don't earn their place -- see `comment-hygiene` - Blanket linter/type/test suppressions -- see `code-hygiene` - Duplication a maintainer must untangle -- see `code-hygiene` -- Retold facts -- a keep-category comment told again at another site -- see `code-hygiene` +- Retold facts -- a keep-category comment told again at another site -- see `comment-hygiene` ### Minor - Non-conventional naming (casing, prefixes, project style) -- Dead / commented-out code, and debug/scaffolding output left behind -- see `code-hygiene` -- Unanchored TODOs -- see `code-hygiene` +- Dead code, commented-out code, and debug/scaffolding output left behind -- see `code-hygiene` +- Unanchored TODOs -- see `comment-hygiene` ## Deferred diff --git a/plugins/praxis/codex/agents/general-reviewer.toml b/plugins/praxis/codex/agents/general-reviewer.toml index 527f029..8ef146a 100644 --- a/plugins/praxis/codex/agents/general-reviewer.toml +++ b/plugins/praxis/codex/agents/general-reviewer.toml @@ -8,8 +8,8 @@ You are a senior code reviewer focusing on general quality and adherence to proj 1. **Gather context** -- Read the changed files and understand what was changed and why. 2. **Check project conventions** -- Read `CLAUDE.md` for project constraints. -3. **Index repeated comments** -- run `find-duplicate-comments.js` from the `code-hygiene` skill; a retelling is invisible from the file it sits in. -4. **Weigh the diff as a body** -- a per-comment pass at every site still misses an over-commented diff; run the density check in `code-hygiene`. +3. **Index repeated comments** -- run `find-duplicate-comments.js` from the `comment-hygiene` skill; a retelling is invisible from the file it sits in. +4. **Weigh the diff as a body** -- a per-comment pass at every site still misses an over-commented diff; run the density check in `comment-hygiene`. 5. **Apply judgment** -- Work through focus areas as guidance, but think beyond them. ## Confidence Filtering @@ -31,16 +31,17 @@ Guidance, not an exhaustive checklist -- tier each finding with the `review-seve ### Important -- Reinvention and orphaned abstractions (speculative generality), and comments that don't earn their place -- see `code-hygiene` +- Reinvention and orphaned abstractions (speculative generality) -- see `code-hygiene` +- Comments that don't earn their place -- see `comment-hygiene` - Blanket linter/type/test suppressions -- see `code-hygiene` - Duplication a maintainer must untangle -- see `code-hygiene` -- Retold facts -- a keep-category comment told again at another site -- see `code-hygiene` +- Retold facts -- a keep-category comment told again at another site -- see `comment-hygiene` ### Minor - Non-conventional naming (casing, prefixes, project style) -- Dead / commented-out code, and debug/scaffolding output left behind -- see `code-hygiene` -- Unanchored TODOs -- see `code-hygiene` +- Dead code, commented-out code, and debug/scaffolding output left behind -- see `code-hygiene` +- Unanchored TODOs -- see `comment-hygiene` ## Deferred @@ -74,5 +75,5 @@ ran must not read as one that found nothing. ## Skills -Load these skills before starting: `gitwise:github-conventions`, `praxis:code-hygiene`, `praxis:review-severity`. +Load these skills before starting: `gitwise:github-conventions`, `praxis:code-hygiene`, `praxis:comment-hygiene`, `praxis:review-severity`. ''' diff --git a/plugins/praxis/skills/code-hygiene/SKILL.md b/plugins/praxis/skills/code-hygiene/SKILL.md index 21309b8..bb556a2 100644 --- a/plugins/praxis/skills/code-hygiene/SKILL.md +++ b/plugins/praxis/skills/code-hygiene/SKILL.md @@ -1,109 +1,13 @@ --- name: code-hygiene -description: Language-agnostic code hygiene -- honest comments, no dead/reinvented/duplicated code, truthful names, real implementations, and scoped (never blanket) diagnostic suppressions. -when_to_use: Writing or reviewing code -- auditing comments, dead or duplicated code, naming, stubbed/placeholder implementations, or linter/type-checker/test suppressions. +description: Language-agnostic code hygiene -- no dead/reinvented/duplicated code, truthful names, real implementations, and scoped (never blanket) diagnostic suppressions. +when_to_use: Writing or reviewing code -- auditing dead or duplicated code, naming, stubbed/placeholder implementations, or linter/type-checker/test suppressions. user-invocable: false --- # Code Hygiene -Don't leave -- or write -- code that lies, hides, or that git (or an existing solution) already owns. Every line is a permanent liability someone must read, test, and carry forward, so each must earn its place. This skill is language-agnostic and guides both authors and reviewers. This skill only deletes; its constructive siblings shape what survives -- `code-structure` (the units and their contracts) and `readable-code` (how a body reads). (Python-specific pitfalls live in `pythonica:python-anti-patterns`; documentation prose in `docs-patterns`.) - -## Comments - -A comment earns its place only by telling the reader something the **code cannot**. Sharp test: *could a competent reader recover this fact from the code (and the repo)? Yes -> delete; No -> maybe keep.* One fact survives that test despite being recoverable -- the sole exception, so bake it into the test: a **legal/provenance header** is recoverable from `LICENSE` yet kept inline by mandate (see EXEMPT). The **external-anchor WHY** looks like a second exception but isn't one: naming a constant recovers its *value*, never the spec/constraint that dictated it, so that rationale stays genuinely unrecoverable and keeps on the mainline (see KEEP). - -This truthfulness standard applies wherever the annotation lives -- inline comment, block comment, or docstring/doc-comment prose. A doc-comment that restates the signature, lies about behavior, or narrates a change is the same anti-pattern as an inline one. Docstring *format/completeness* defers to the language plugin (`pythonica`); *prose quality* to `docs-patterns`. - -### KEEP -- the code can't say it - -Phrase each against the **invariant** that motivates it, not the current mechanics -- **evergreen**, so a refactor leaves it true. - -- **WHY / rationale** -- the constraint that forced a non-obvious choice, the tradeoff, or the obvious alternative rejected and why (a performance hack that replaces the clean form belongs here -- say so and cite the evidence: hot path, measured Nx). - `# gh resolves the repo from GH_REPO, so no checkout; the guard keeps re-runs idempotent` -- **GOTCHA / footgun** -- name the surprising fact, *then* why it must be so: a side effect a caller can't infer (a read that writes, a getter that primes a cache, hidden I/O or global mutation), an ordering dependency (say what silently breaks if reordered), a looks-wrong-but-right oddity (a correct off-by-one, a deliberate bare except), a cost/thread hazard, or a decode of genuinely inscrutable syntax. A warning without its why is just a WHAT-comment with an alarm on it. Delete it the moment the hazard is removed, or it becomes a phantom warning. - `# <= not <: the upper bound is inclusive per the wire spec` -- **EXTERNAL ANCHOR** -- a citation to the spec/RFC/ticket/paper/formula/legal requirement that dictates the code's shape. The most rot-resistant comment there is: its source of truth is versioned and changes deliberately elsewhere. Distinguish from Nonlocal (DELETE): citing a *versioned* spec/RFC/ticket anchors (keep); mirroring a value a *live, mutable* config or another service owns only drifts (delete). - `# 0x5F3759DF: fast inverse-sqrt magic constant, see Quake III src` - -### EXEMPT -- functional, not prose - -Judged by *"is it required?"*, not *"does it convey rationale?"* -- the prose taxonomy has no slot for annotations that use comment syntax but are machine-read. A reviewer applying "banner/noise -> delete" literally will break tooling. - -- **Legal / provenance header** -- license, copyright, SPDX mandated by law or policy. Keep minimal; point to `LICENSE` rather than inlining the full text. `# SPDX-License-Identifier: Apache-2.0`. Restrict "provenance" to legal/origin -- **not** author credit (`# Author: Jane` is a byline -> delete). Mandated *per-line* boilerplate -> delete; mandated *per-file legal* header -> keep. -- **Tooling directive** -- load-bearing to a machine, not a reader: shebang, encoding cookie, `# %%` cell marker, `# region`/`#endregion` fold, `# fmt: off`, codegen sentinel (`# BEGIN GENERATED`), type-in-comment hint, and scoped suppression pragmas (`# noqa`, `# type: ignore` -- scoping governed in *Faking done*). Not a decorative banner (below). - -### DELETE -- recoverable, or git owns it - -Cover the comment, read only the code; if a competent reader learns nothing new, delete it. - -- **What / how restatement** -- paraphrases the mechanics the line states (`i += 1 # increment i`). Highest-coupling, fastest-rotting comment: a lie the instant the mechanics change and no one updates it. -- **Derived-value echo** -- restates a value, count, threshold, or range the code already declares. No compiler catches the drift -> delete *even when currently correct*, on rot risk alone. -- **Stale / misleading** -- the current code contradicts it. Worse than none: correct it into a keep-category fact, or delete. -- **Nonlocal** -- asserts a fact another file/service/config owns; it drifts silently when *that* source changes. State it at the source. (Cross-ownership drift: told here, never at its owner -- move it there. If the repo already states it at the owner, that copy is *Retold fact* under CONDITIONAL -- point, don't restate.) -- **Noise / banner / attribution** -- section dividers (`# ==== HELPERS ====`), closing-brace labels (`} // end for`), author/date bylines. Structure and `git blame` carry these; if a file needs dividers to navigate, split it. (A `# region` fold or codegen sentinel is a tooling directive, not a banner.) -- **Commented-out code** -- delete unconditionally; git owns it (`git log -G` recovers it). "Disabled with a why" is no loophole -- the code still goes, only the codeless knowledge survives as a tracked note. See *Dead code*. Carve-out: an illustrative snippet inside a docstring/example config is documentation-by-example, not disabled program code -> keep. -- **Mandated boilerplate** -- a comment satisfying only a "comment everything" rule, or a header restating the signature's name/params/return. If a header is required, make it say what the signature cannot. - -#### Tombstone -- narrates the change, not the state - -Narrates the **change** that produced the code (what was removed, moved, renamed, or done "per review") rather than its current state. A state description is evergreen; a change narration is dated the moment it is written. Git owns change history; such comments duplicate the diff, drift, and mean nothing to a reader who never saw the prior version. Two tests, in order: - -1. **Cold-reader** -- worth writing to a first-time reader who never saw the prior version or the PR? No -> delete. -2. **Git-ownership** -- amounts to "what changed/moved/was removed", or points to a thing not in this file now (a removed block, a moved responsibility, a PR)? Version control's job -> delete. - -**First-pass token filter (flag, don't auto-verdict):** `moved`, `now handled`, `no longer`, `used to`, `previously`, `was`, `replaced`, `instead of`, `per review`, `as requested`, `see ` (explaining an *absence* here). A hit only triggers the cold-reader test -- `now`/`instead` also appear in legitimate rationale, and a **Dated advisory** legitimately uses `no longer`/`was` about the world outside this repo. - -### CONDITIONAL - -- **TODO / FIXME / HACK** -- KEEP only if actionable *and* anchored to a tracker: `TODO(#123): drop once upstream ships stubs` states what the code can't (known-incomplete, and what's owed) -> treat as an external anchor. Flag the bare orphan (`TODO: fix later`) -- no owner, rots. Never delete a *live, real-gap* marker for cleanliness; that hides debt, which is worse. -- **Magic value** -- SPLIT. The value's **meaning** -> self-document with a named constant (meta-rule) and drop the comment. Its **provenance** (spec section, RFC, empirically-tuned figure, bit-hack origin) -> KEEP as external-anchor WHY. A bare number with neither name nor anchor is a naming defect, not a comment to keep. -- **Dated advisory** -- the one deliberate exception to evergreen. A fact about the world - *outside this repo* that you cannot fix from here: an unpatched CVE, an upstream bug you - compensate for. Undated it turns false the day the world moves, so it needs an **anchor** - (advisory or tracker id, never a person), an **as-of date**, and the **impact here**. - Phrase it to name its own end -- "no fixed release as of ``" stops being true once one - ships, which is when the comment goes. Unlike a TODO, the fix is not ours to make. - `# CVE-2025-1234 in libfoo <= 2.3, no fixed release as of 2026-08-05; affected parser unreachable here` -- **Retold fact** -- SPLIT. A keep-category fact told again at a second site: every copy is - true and locally unrecoverable, so per-file review keeps them all and one edit later the - rest read as lies. The site whose own change would falsify it KEEPs the telling; the others - name the topic and point (`# config ownership: entrypoint.sh`), though a warning at its own - point of danger stays. Grep the phrase before ruling, or run - [find-duplicate-comments.js](find-duplicate-comments.js), which indexes every tracked - comment and reports the ones a diff adds that are already told elsewhere. - -### Keep vs. delete - -| Comment | Verdict | Why | -|---|---|---| -| `# gh resolves the repo from GH_REPO; the guard keeps re-runs idempotent` | keep | WHY -- present-state rationale for a non-obvious choice. | -| `# SPDX-License-Identifier: Apache-2.0` | keep | Legal header -- mandated; `git blame` can't carry it. | -| `# TODO(#123): remove once upstream ships stubs` | keep | Tracked gap -- an anchor to versioned debt. | -| `i += 1 # increment i` | delete | What restatement -- restates the line verbatim. | -| `# retries 3 times` beside a loop of 5 | delete | Stale/derived -- updating the count just re-creates the echo; elevate to a WHY instead. | -| `# timeout is 30s`, value lives in another service's config | delete | Nonlocal -- drifts when that config changes; state it at the source. | -| "...Release is created by `release.yml`, so this only handles upload" | delete | Tombstone -- narrates the split; references a removed job. | -| `# Author: Jane` \* `# ==== HELPERS ====` \* `} // end for` | delete | Byline / banner -- `git blame` and structure own these. | -| `x = f() # TODO: fix later` | flag | Orphan TODO -- no owner, no anchor; anchor it or resolve it. | -| `# CVE-2025-1234 in libfoo <= 2.3, no fixed release as of 2026-08-05; affected parser unreachable here` | keep | Dated advisory -- anchored, dated, scoped; a fix retires it. | -| The same "Geyser owns the config after first boot" in six files | split | Retold fact -- keep it where a change would falsify it; the rest point. | - -### Density -- judge the diff, not just the comment - -Every rule above weighs one comment against the code beside it, so a diff can pass at every single site and still ship bloat: *"does this fact earn a comment?"* is a different question from *"does this file need this many?"*. Ask the second one once, over the whole diff. - -**Trigger** -- a signal to re-read, never a budget to hit: added comment lines approaching or exceeding added code lines, or a rationale on nearly every changed block. Then re-read each one with the presumption reversed, because the first pass was made in the mood that wrote them. - -**The usual cause is PR narration** -- the author explains the change to the reviewer, then writes that explanation into the file. Sort by reader: a sentence aimed at whoever reads the **diff** belongs in the PR body, where it is read once by the reader it was written for and never rots; only a sentence aimed at whoever reads the **file** years later stays. The migration story, the measurement that justified the switch, and the comparison to what used to be here are all the first kind (and the last is a Tombstone besides). - -**A rejected alternative splits on that same test.** One a future editor would otherwise re-attempt is a WHY and stays inline; one that only explains why *this change* looks the way it does goes in the PR body. - -What survives is one invariant per non-obvious decision, at the site whose own change would falsify it -- the sites that share it name the topic and point (*Retold fact*). - -### Meta-rule - -Prefer self-documenting code over a comment. A better name or an extracted function is **load-bearing** -- a refactor carries it along, so it can't drift out of sync the way a bystander comment silently does. If a rename or extraction would carry the fact, do that instead. Reach for a comment only for the residue the code genuinely can't hold: a WHY, a gotcha, an external anchor. +Don't leave -- or write -- code that lies, hides, or that git (or an existing solution) already owns. Every line is a permanent liability someone must read, test, and carry forward, so each must earn its place. This skill is language-agnostic and guides both authors and reviewers. This skill only deletes; its constructive siblings shape what survives -- `code-structure` (the units and their contracts) and `readable-code` (how a body reads). Comment truthfulness is `comment-hygiene`'s. (Python-specific pitfalls live in `pythonica:python-anti-patterns`; documentation prose in `docs-patterns`.) ## Dead code & speculative generality diff --git a/plugins/praxis/skills/code-structure/SKILL.md b/plugins/praxis/skills/code-structure/SKILL.md index 496e23c..e01d308 100644 --- a/plugins/praxis/skills/code-structure/SKILL.md +++ b/plugins/praxis/skills/code-structure/SKILL.md @@ -1,6 +1,6 @@ --- name: code-structure -description: Language-agnostic structural craft -- decompose on responsibility not size, prefer deep modules over shallow piles, and shape cohesion, coupling, interfaces, error contracts, and data invariants. Sibling to code-hygiene and readable-code. +description: Language-agnostic structural craft -- decompose on responsibility not size, prefer deep modules over shallow piles, and shape cohesion, coupling, interfaces, error contracts, and data invariants. Sibling to code-hygiene, comment-hygiene and readable-code. when_to_use: Writing or reviewing code -- shaping function and module boundaries, interfaces, error contracts, and data types once code-hygiene has removed the outright liabilities. Local reading (control flow, naming, reading order) is readable-code. user-invocable: false --- @@ -9,12 +9,12 @@ user-invocable: false `code-hygiene` deletes the liability; this skill shapes the units that remain. Four owners, one test: -- **hygiene = DELETE** what git or a library already owns -- a restating comment, dead code, a name that lies or says nothing. A rule that says *delete X* is hygiene's. +- **hygiene = DELETE** what git or a library already owns -- dead code, a name that lies or says nothing (`code-hygiene`), a restating comment (`comment-hygiene`). A rule that says *delete X* is one of the two hygienes'. - **code-structure = SHAPE THE UNITS** -- how the code is decomposed, how units couple, how an interface reads, how a failure reaches a caller, how data carries its own invariants. Everything here assumes the code should exist; the question is what shape its units and boundaries take. - **readable-code = READ** -- how a single body reads line by line: control-flow shape, naming across a set, reading order and working set. Anything about how one body *reads* rather than how the units are *carved* is readable-code's. - **pythonica = Python mechanics** -- a rule that names a Python construct (`dataclass`, `NewType`, a context manager, `match`) is pythonica's; keep this skill language-agnostic. -Read `code-hygiene` first: shaping code that should have been deleted is wasted work. When a fix here would be *self-documenting code* (a revealing name, an extracted step, an explaining variable), that is exactly what hygiene's "prefer self-documenting code over a comment" meta-rule points **to** -- hygiene removes the comment, this skill supplies the structure. Where a smell reads as "there is redundant knowledge here", check the boundary: identical *text* in two places is hygiene's duplication rule; the same *decision* in two unlike forms is this skill's (DRY-as-knowledge, below). +Read `code-hygiene` and `comment-hygiene` first: shaping code that should have been deleted is wasted work. When a fix here would be *self-documenting code* (a revealing name, an extracted step, an explaining variable), that is exactly what `comment-hygiene`'s "prefer self-documenting code over a comment" meta-rule points **to** -- that skill removes the comment, this skill supplies the structure. Where a smell reads as "there is redundant knowledge here", check the boundary: identical *text* in two places is `code-hygiene`'s duplication rule; the same *decision* in two unlike forms is this skill's (DRY-as-knowledge, below). Every rule below is a recognizable **smell** (something a reader or author can actually spot) -> the **principle** it violates -> a **fix** direction. No line, parameter, or nesting-depth count gates any rule here: that false precision is the size-fallacy this skill exists to replace. (Illustrative counts like "a two-line loop index" still appear -- they describe an example, they do not set a threshold.) @@ -98,7 +98,7 @@ Keep the code cheap to reshape, and keep reshaping honest. What this skill deliberately leaves to others, one reason each: -- **Comments -- OUT.** Wholly owned by `code-hygiene` (KEEP/DELETE/EXEMPT and the self-documenting-code meta-rule). The explaining variables and phase seams that meta-rule points to live in `readable-code`; cross-link, never re-adjudicate a comment. +- **Comments -- OUT.** Wholly owned by `comment-hygiene` (KEEP/DELETE/EXEMPT and the self-documenting-code meta-rule). The explaining variables and phase seams that meta-rule points to live in `readable-code`; cross-link, never re-adjudicate a comment. - **Control flow, naming, reading order -- OUT (to `readable-code`).** How a single body reads line by line -- guard clauses, whole-set naming, locality and working set -- is the local-clarity sibling. This skill carves the units; `readable-code` reads them. - **Formatting, whitespace, line length, alignment -- OUT.** Auto-formatter territory; legislating layout would smuggle in the numeric thresholds this skill's register forbids. The one shaping-relevant piece, phase seams, is a `readable-code` rule. - **Error handling -- IN, but only as interface semantics** (define errors out of existence, un-ignorability, detect-low/handle-high, do-not-extract-a-shallow-happy-path). Which exception type, `Result` versus `Option`, and syntax defer to `pythonica:python-error-handling`. diff --git a/plugins/praxis/skills/comment-hygiene/SKILL.md b/plugins/praxis/skills/comment-hygiene/SKILL.md new file mode 100644 index 0000000..b218850 --- /dev/null +++ b/plugins/praxis/skills/comment-hygiene/SKILL.md @@ -0,0 +1,104 @@ +--- +name: comment-hygiene +description: Comment truthfulness -- what a comment must tell the reader that the code can't, the KEEP/DELETE/EXEMPT/CONDITIONAL taxonomy, tombstone and retold-fact detection, and comment density across a diff. Sibling to code-hygiene, code-structure and readable-code. +when_to_use: Writing or reviewing code -- auditing whether a comment, docstring, or doc-comment earns its place, spotting stale/tombstone/retold comments, or judging comment density across a diff. +user-invocable: false +--- + +# Comment Hygiene + +Split from `code-hygiene` -- this is comment truthfulness only; dead code, reinvention/duplication, naming, and faking done stay in `code-hygiene`, which this skill sits alongside as a sibling of `code-structure` (the units and their contracts) and `readable-code` (how a body reads). + +A comment earns its place only by telling the reader something the **code cannot**. Sharp test: *could a competent reader recover this fact from the code (and the repo)? Yes -> delete; No -> maybe keep.* One fact survives that test despite being recoverable -- the sole exception, so bake it into the test: a **legal/provenance header** is recoverable from `LICENSE` yet kept inline by mandate (see EXEMPT). The **external-anchor WHY** looks like a second exception but isn't one: naming a constant recovers its *value*, never the spec/constraint that dictated it, so that rationale stays genuinely unrecoverable and keeps on the mainline (see KEEP). + +This truthfulness standard applies wherever the annotation lives -- inline comment, block comment, or docstring/doc-comment prose. A doc-comment that restates the signature, lies about behavior, or narrates a change is the same anti-pattern as an inline one. Docstring *format/completeness* defers to the language plugin (`pythonica`); *prose quality* to `docs-patterns`. + +## KEEP -- the code can't say it + +Phrase each against the **invariant** that motivates it, not the current mechanics -- **evergreen**, so a refactor leaves it true. + +- **WHY / rationale** -- the constraint that forced a non-obvious choice, the tradeoff, or the obvious alternative rejected and why (a performance hack that replaces the clean form belongs here -- say so and cite the evidence: hot path, measured Nx). + `# gh resolves the repo from GH_REPO, so no checkout; the guard keeps re-runs idempotent` +- **GOTCHA / footgun** -- name the surprising fact, *then* why it must be so: a side effect a caller can't infer (a read that writes, a getter that primes a cache, hidden I/O or global mutation), an ordering dependency (say what silently breaks if reordered), a looks-wrong-but-right oddity (a correct off-by-one, a deliberate bare except), a cost/thread hazard, or a decode of genuinely inscrutable syntax. A warning without its why is just a WHAT-comment with an alarm on it. Delete it the moment the hazard is removed, or it becomes a phantom warning. + `# <= not <: the upper bound is inclusive per the wire spec` +- **EXTERNAL ANCHOR** -- a citation to the spec/RFC/ticket/paper/formula/legal requirement that dictates the code's shape. The most rot-resistant comment there is: its source of truth is versioned and changes deliberately elsewhere. Distinguish from Nonlocal (DELETE): citing a *versioned* spec/RFC/ticket anchors (keep); mirroring a value a *live, mutable* config or another service owns only drifts (delete). + `# 0x5F3759DF: fast inverse-sqrt magic constant, see Quake III src` + +## EXEMPT -- functional, not prose + +Judged by *"is it required?"*, not *"does it convey rationale?"* -- the prose taxonomy has no slot for annotations that use comment syntax but are machine-read. A reviewer applying "banner/noise -> delete" literally will break tooling. + +- **Legal / provenance header** -- license, copyright, SPDX mandated by law or policy. Keep minimal; point to `LICENSE` rather than inlining the full text. `# SPDX-License-Identifier: Apache-2.0`. Restrict "provenance" to legal/origin -- **not** author credit (`# Author: Jane` is a byline -> delete). Mandated *per-line* boilerplate -> delete; mandated *per-file legal* header -> keep. +- **Tooling directive** -- load-bearing to a machine, not a reader: shebang, encoding cookie, `# %%` cell marker, `# region`/`#endregion` fold, `# fmt: off`, codegen sentinel (`# BEGIN GENERATED`), type-in-comment hint, and scoped suppression pragmas (`# noqa`, `# type: ignore` -- scoping governed in `code-hygiene`'s *Faking done*). Not a decorative banner (below). + +## DELETE -- recoverable, or git owns it + +Cover the comment, read only the code; if a competent reader learns nothing new, delete it. + +- **What / how restatement** -- paraphrases the mechanics the line states (`i += 1 # increment i`). Highest-coupling, fastest-rotting comment: a lie the instant the mechanics change and no one updates it. +- **Derived-value echo** -- restates a value, count, threshold, or range the code already declares. No compiler catches the drift -> delete *even when currently correct*, on rot risk alone. +- **Stale / misleading** -- the current code contradicts it. Worse than none: correct it into a keep-category fact, or delete. +- **Nonlocal** -- asserts a fact another file/service/config owns; it drifts silently when *that* source changes. State it at the source. (Cross-ownership drift: told here, never at its owner -- move it there. If the repo already states it at the owner, that copy is *Retold fact* under CONDITIONAL -- point, don't restate.) +- **Noise / banner / attribution** -- section dividers (`# ==== HELPERS ====`), closing-brace labels (`} // end for`), author/date bylines. Structure and `git blame` carry these; if a file needs dividers to navigate, split it. (A `# region` fold or codegen sentinel is a tooling directive, not a banner.) +- **Commented-out code** -- delete unconditionally; git owns it (`git log -G` recovers it). "Disabled with a why" is no loophole -- the code still goes, only the codeless knowledge survives as a tracked note. See `code-hygiene`'s *Dead code*. Carve-out: an illustrative snippet inside a docstring/example config is documentation-by-example, not disabled program code -> keep. +- **Mandated boilerplate** -- a comment satisfying only a "comment everything" rule, or a header restating the signature's name/params/return. If a header is required, make it say what the signature cannot. + +### Tombstone -- narrates the change, not the state + +Narrates the **change** that produced the code (what was removed, moved, renamed, or done "per review") rather than its current state. A state description is evergreen; a change narration is dated the moment it is written. Git owns change history; such comments duplicate the diff, drift, and mean nothing to a reader who never saw the prior version. Two tests, in order: + +1. **Cold-reader** -- worth writing to a first-time reader who never saw the prior version or the PR? No -> delete. +2. **Git-ownership** -- amounts to "what changed/moved/was removed", or points to a thing not in this file now (a removed block, a moved responsibility, a PR)? Version control's job -> delete. + +**First-pass token filter (flag, don't auto-verdict):** `moved`, `now handled`, `no longer`, `used to`, `previously`, `was`, `replaced`, `instead of`, `per review`, `as requested`, `see ` (explaining an *absence* here). A hit only triggers the cold-reader test -- `now`/`instead` also appear in legitimate rationale, and a **Dated advisory** legitimately uses `no longer`/`was` about the world outside this repo. + +## CONDITIONAL + +- **TODO / FIXME / HACK** -- KEEP only if actionable *and* anchored to a tracker: `TODO(#123): drop once upstream ships stubs` states what the code can't (known-incomplete, and what's owed) -> treat as an external anchor. Flag the bare orphan (`TODO: fix later`) -- no owner, rots. Never delete a *live, real-gap* marker for cleanliness; that hides debt, which is worse. +- **Magic value** -- SPLIT. The value's **meaning** -> self-document with a named constant (meta-rule) and drop the comment. Its **provenance** (spec section, RFC, empirically-tuned figure, bit-hack origin) -> KEEP as external-anchor WHY. A bare number with neither name nor anchor is a naming defect, not a comment to keep. +- **Dated advisory** -- the one deliberate exception to evergreen. A fact about the world + *outside this repo* that you cannot fix from here: an unpatched CVE, an upstream bug you + compensate for. Undated it turns false the day the world moves, so it needs an **anchor** + (advisory or tracker id, never a person), an **as-of date**, and the **impact here**. + Phrase it to name its own end -- "no fixed release as of ``" stops being true once one + ships, which is when the comment goes. Unlike a TODO, the fix is not ours to make. + `# CVE-2025-1234 in libfoo <= 2.3, no fixed release as of 2026-08-05; affected parser unreachable here` +- **Retold fact** -- SPLIT. A keep-category fact told again at a second site: every copy is + true and locally unrecoverable, so per-file review keeps them all and one edit later the + rest read as lies. The site whose own change would falsify it KEEPs the telling; the others + name the topic and point (`# config ownership: entrypoint.sh`), though a warning at its own + point of danger stays. Grep the phrase before ruling, or run + [find-duplicate-comments.js](find-duplicate-comments.js), which indexes every tracked + comment and reports the ones a diff adds that are already told elsewhere. + +## Keep vs. delete + +| Comment | Verdict | Why | +|---|---|---| +| `# gh resolves the repo from GH_REPO; the guard keeps re-runs idempotent` | keep | WHY -- present-state rationale for a non-obvious choice. | +| `# SPDX-License-Identifier: Apache-2.0` | keep | Legal header -- mandated; `git blame` can't carry it. | +| `# TODO(#123): remove once upstream ships stubs` | keep | Tracked gap -- an anchor to versioned debt. | +| `i += 1 # increment i` | delete | What restatement -- restates the line verbatim. | +| `# retries 3 times` beside a loop of 5 | delete | Stale/derived -- updating the count just re-creates the echo; elevate to a WHY instead. | +| `# timeout is 30s`, value lives in another service's config | delete | Nonlocal -- drifts when that config changes; state it at the source. | +| "...Release is created by `release.yml`, so this only handles upload" | delete | Tombstone -- narrates the split; references a removed job. | +| `# Author: Jane` \* `# ==== HELPERS ====` \* `} // end for` | delete | Byline / banner -- `git blame` and structure own these. | +| `x = f() # TODO: fix later` | flag | Orphan TODO -- no owner, no anchor; anchor it or resolve it. | +| `# CVE-2025-1234 in libfoo <= 2.3, no fixed release as of 2026-08-05; affected parser unreachable here` | keep | Dated advisory -- anchored, dated, scoped; a fix retires it. | +| The same "Geyser owns the config after first boot" in six files | split | Retold fact -- keep it where a change would falsify it; the rest point. | + +## Density -- judge the diff, not just the comment + +Every rule above weighs one comment against the code beside it, so a diff can pass at every single site and still ship bloat: *"does this fact earn a comment?"* is a different question from *"does this file need this many?"*. Ask the second one once, over the whole diff. + +**Trigger** -- a signal to re-read, never a budget to hit: added comment lines approaching or exceeding added code lines, or a rationale on nearly every changed block. Then re-read each one with the presumption reversed, because the first pass was made in the mood that wrote them. + +**The usual cause is PR narration** -- the author explains the change to the reviewer, then writes that explanation into the file. Sort by reader: a sentence aimed at whoever reads the **diff** belongs in the PR body, where it is read once by the reader it was written for and never rots; only a sentence aimed at whoever reads the **file** years later stays. The migration story, the measurement that justified the switch, and the comparison to what used to be here are all the first kind (and the last is a Tombstone besides). + +**A rejected alternative splits on that same test.** One a future editor would otherwise re-attempt is a WHY and stays inline; one that only explains why *this change* looks the way it does goes in the PR body. + +What survives is one invariant per non-obvious decision, at the site whose own change would falsify it -- the sites that share it name the topic and point (*Retold fact*). + +## Meta-rule + +Prefer self-documenting code over a comment. A better name or an extracted function is **load-bearing** -- a refactor carries it along, so it can't drift out of sync the way a bystander comment silently does. If a rename or extraction would carry the fact, do that instead. Reach for a comment only for the residue the code genuinely can't hold: a WHY, a gotcha, an external anchor. diff --git a/plugins/praxis/skills/code-hygiene/find-duplicate-comments.js b/plugins/praxis/skills/comment-hygiene/find-duplicate-comments.js similarity index 100% rename from plugins/praxis/skills/code-hygiene/find-duplicate-comments.js rename to plugins/praxis/skills/comment-hygiene/find-duplicate-comments.js diff --git a/plugins/praxis/skills/docs-patterns/SKILL.md b/plugins/praxis/skills/docs-patterns/SKILL.md index 59a5a69..902e435 100644 --- a/plugins/praxis/skills/docs-patterns/SKILL.md +++ b/plugins/praxis/skills/docs-patterns/SKILL.md @@ -84,7 +84,7 @@ Prose that narrates the change which produced the page, or inventories where els `Or from source. This project installs with uv rather than pip -- in CI, in the devcontainer and in the Docker image:` -- the reader is installing locally. Which installer the project stopped using, and the three other places the new one is also used, change nothing they type. `Or from source, with uv:` is the whole of it. -This is the prose form of the habit `code-hygiene` names as PR narration in its *Density* section, and one migration commonly produces both at once -- so a diff carrying it in comments is worth reading for it in the docs too. Rationale that genuinely needs a home goes in the pull request body, which is read once, by the reader it was written for. +This is the prose form of the habit `comment-hygiene` names as PR narration in its *Density* section, and one migration commonly produces both at once -- so a diff carrying it in comments is worth reading for it in the docs too. Rationale that genuinely needs a home goes in the pull request body, which is read once, by the reader it was written for. ## What to Avoid diff --git a/plugins/praxis/skills/readable-code/SKILL.md b/plugins/praxis/skills/readable-code/SKILL.md index d22162f..3d9ed04 100644 --- a/plugins/praxis/skills/readable-code/SKILL.md +++ b/plugins/praxis/skills/readable-code/SKILL.md @@ -1,6 +1,6 @@ --- name: readable-code -description: Language-agnostic local-clarity craft -- shape control flow, the naming of a whole set, and reading order so a body reads straight down with the smallest working set. Sibling to code-hygiene and code-structure. +description: Language-agnostic local-clarity craft -- shape control flow, the naming of a whole set, and reading order so a body reads straight down with the smallest working set. Sibling to code-hygiene, comment-hygiene and code-structure. when_to_use: Writing or reviewing how a function or file READS line by line -- guard-clause control flow, naming for the reader across a set, and the order and locality that shrink what a reader must hold in mind. user-invocable: false --- @@ -49,7 +49,7 @@ Shrink the working set a reader must carry to understand one line. What this skill leaves to others, one reason each: -- **Deleting anything -- OUT.** A restating comment, a dead branch, a name that lies or says nothing is `code-hygiene`'s; this skill only reshapes code that stays. +- **Deleting anything -- OUT.** A restating comment is `comment-hygiene`'s; a dead branch or a name that lies or says nothing is `code-hygiene`'s; this skill only reshapes code that stays. - **Where a seam falls, how big a unit is -- OUT.** Decomposition, module depth, and the size-vs-depth resolution are `code-structure`'s. A guard clause here flattens nesting; it does not decide what to extract. - **Naming a single liability name -- OUT.** A name that lies (`get_*` that writes) or says nothing (`tmp`, `data`) is hygiene's rename-or-delete; this skill judges the consistency and scope-fit of the whole *set*. - **Formatting, whitespace, line length, alignment -- OUT.** Auto-formatter territory; legislating layout would smuggle in numeric thresholds. A genuine phase seam (a structural break, not a blank-line count) is the rule above.