Commit 5f9da9f
Changed files (1)
modules
dev
agents
skills
prune-comments-and-docs
modules/dev/agents/skills/prune-comments-and-docs/SKILL.md
@@ -1,104 +1,120 @@
---
name: prune-comments-and-docs
description: >
- Review and tighten code comments, doc comments, and prose docs (README, guides,
- design notes). Use when asked to review, clean up, cut, or improve comments or
- documentation.
+ Review, tighten, and rewrite code comments, doc comments, and prose docs
+ (README, guides, design notes). Use when asked to review, clean up, cut,
+ clarify, or improve comments or documentation.
---
# Prune comments and docs
-Write for a reader who sees only the current code, has never seen a previous
-version, and doesn't know who wrote it. A comment earns its place when it tells
-that reader something they need, where they're reading, that the code doesn't
-already say. The same fact told twice to the same audience is told once too
-many.
+Write for someone who knows only the current code, not its history or author.
+Each comment should make one point the reader needs here and cannot get from the
+code. Use one point per doc-comment section and per prose paragraph. Tell each
+fact once per audience.
-Bad comments are usually written during a change and describe the change: what
-was replaced, what was split, what was just implemented and how. That belongs in
-the commit message. If you made the change, you're the worst judge of this. Read
-each comment as if opening the file for the first time.
+Comments written during a change often describe the change: what was replaced,
+split, or just implemented, and how. That belongs in the commit message. If you
+made the change, assume you're too close to judge. Read each comment as if
+opening the file for the first time.
## Who reads what
- **Doc comments** (`///`, `//!`, docstrings) serve callers. State the contract:
- what it does, what the types don't say, when it fails or panics, what the
- caller must ensure. It should stay true if the body is rewritten with the same
- behavior.
-- **Inline comments** serve maintainers. Explain why the code is this way where
- the code alone would mislead: a constraint, a workaround, a rejected obvious
- alternative. Don't narrate what the next lines do.
+ what it does, what the types don't say, when it fails or panics, and what the
+ caller must ensure. The whole comment should stay true if the body is
+ rewritten with the same behavior.
+- **Inline comments** serve maintainers. Answer their question at this line: why
+ it's this way, or what breaks if they change it. The answer is usually a
+ constraint, workaround, or consequence. Don't narrate the next lines.
- **Prose docs** serve someone arriving cold. Cover concepts, usage, and
- decisions that span files. Don't mirror the code, report progress, or restate
- contracts that doc comments own.
+ decisions that span files. Don't mirror the code, report progress, or repeat
+ contracts owned by doc comments.
-Each kind holds the facts its audience needs. The same fact may serve two
-audiences (a README and a doc comment), but not two spots for one audience.
+Each kind holds the facts its audience needs. A fact may serve two audiences,
+such as `README` readers and API callers, but should not appear twice for one
+audience.
-The best home for a fact is often not a comment: a name, a type, an error
-message, an assert, a test. Prefer those; a comment is the home of last resort.
-Suggest such moves in your summary; this pass reviews comments, not code.
+A fact often belongs in a name, type, error message, assertion, or test rather
+than a comment. Prefer those homes. Suggest such moves in your summary; this
+pass changes comments and docs, not code.
## The pass
-Comments written during a change re-explain its decisions at every site. Map the
-duplicates before pruning: cluster the comments and surviving plan-prose around
-repeated domain terms ("state", "rendered", "atomic", "converge") and give each
-recurring fact one home per audience, on the item it's about. If two spots both
-want the fact, the item it's about wins within an audience; otherwise the one
-the reader reaches first.
+Map duplicates before pruning. Group comments and leftover planning prose around
+recurring domain terms ("state", "rendered", "atomic", "converge"). Give each
+repeated fact one home per audience. Prefer the item the fact describes;
+otherwise, use the first place the reader reaches. Delete the other copies.
+Check 4 governs any pointers that remain.
-Then check each comment in order. Act on the first failure.
+Parallel reviewers each see only their own files, so map duplicates across the
+whole set before editing starts.
+
+For each comment, apply these checks in order. Act on the first failure.
1. **True?** Read the code it describes. If they disagree and you can't tell
- which is wrong, flag it. Don't make the comment match what might be a bug.
-2. **Needed?** If the name, signature, types, or next few lines already say it,
- delete it. Also delete it if the fact's home already gives this reader
- everything they need here.
-3. **Free of time?** If it only makes sense to someone who saw an earlier
+ which is wrong, flag it. Don't rewrite the comment to match a possible bug.
+2. **Needed?** Delete it if the name, signature, types, nearby strings (error
+ messages, test or check labels), or next few lines already say it. Also
+ delete it if the fact's home already gives this reader everything they need
+ here.
+3. **About the current code?** Delete history that depends on knowing an earlier
version ("now", "no longer", "instead of", "replaced", "split", "moved",
- "refactored", "originally", "ported from"), delete it. Plans are the mirror
- image ("will", "could", "eventually", "in the future", "not implemented
- yet"); delete them too. If it guards against a real regression, restate it as
- a present-tense constraint. A limitation that still bites ("teardown is not
- implemented") is plain prose and joins the map. An intended action becomes a
- TODO or an issue link, not prose.
-4. **Is this the fact's home?** If not, keep only what this reader must do about
- it: an obligation, not a re-explanation. Implementation detail in a doc
- comment moves inline if it explains a why. Otherwise it goes.
-5. **As short as it can be?** Point first. Most doc comments need one sentence.
- Cut filler, hedges, and asides.
-
-A comment that passes all five and holds its fact's home stays untouched, word
-for word.
-
-Delete is the default: a comment goes unless it holds a fact with no other home;
-shorten, move, or rewrite to rescue that fact. Add a comment only when a reader
-would otherwise get something wrong: a missing why, or a missing error, panic,
-or safety contract. If a comment exists only to explain an unclear name, suggest
-the rename in your summary instead of polishing the comment.
-
-Judge blocks, not just sentences. For a comment over two lines: if deleting it
-loses no fact, delete it. If you can predict a comment from the code's structure
-(bare numbering, section rules, enum glosses), it's narration; cut on sight.
-Re-judge each file as a whole: when comments track the code's structure one to
-one, the file is over-commented even if each comment survives alone. Cut the
-class, not the instances, unless a project convention enforces them. Only within
-one class: bare numbering is one class, labeled decisions another.
+ "refactored", "originally", "ported from"). Judge the meaning, not the
+ marker: "instead of ORing" names a live alternative, and "new routes" is not
+ history. Delete plans too ("will", "could", "eventually", "in the future",
+ "not implemented yet"). Preserve a real regression guard as a present-tense
+ constraint. Keep current limitations ("teardown is not implemented") as plain
+ prose and include them in the duplicate map. Turn intended actions into TODOs
+ or issue links, not narrative.
+4. **At the fact's home?** If not, delete it. Keep a pointer only when a caller
+ must take another step to use the API correctly, or a maintainer changing
+ this code must check or update another place. State the obligation ("new
+ routes also need approval in infra"), not the fact again. A pointer that
+ merely locates a topic is a tour. Keep links to issues, specs, and upstream
+ bugs that justify a workaround. Move implementation detail from a doc comment
+ inline only if it explains why; otherwise, delete it.
+5. **Clear at a glance?** Fail if the point is buried, an inline comment makes
+ several points, or filler, hedges, or asides pad it. Keep the point this
+ reader needs here. Move another point only if the reader needs it at another
+ line. Otherwise, drop it even if it has no other home.
+
+A comment that passes all five stays word for word, even if you would phrase it
+differently. When a comment fails, fix only that failure. Match the file's
+comment style: casing, punctuation, and terseness. In text you write, spell
+proper nouns and acronyms as usual (Nix, DNS).
+
+Delete by default: a comment goes unless it holds a fact with no other home.
+Rewriting may keep that fact, not the whole comment. Identify the reader's
+question at this line, answer it in a sentence, and add the reason if the answer
+alone seems arbitrary. Add a comment only when the reader would otherwise get
+something wrong: a missing why, or a missing error, panic, or safety contract.
+If a comment only explains an unclear name, suggest a rename in your summary
+instead of polishing it.
+
+Judge whole blocks, not just sentences. For a comment longer than two lines, ask
+whether deleting it loses anything this reader needs. If not, delete it. If the
+code's structure predicts the comment (bare numbering, section dividers, enum
+glosses), it's narration; cut it.
+
+Then review the whole file. When comments mirror the code's structure one to
+one, the file is over-commented even if each comment passes alone. Cut the
+class, not individual instances, unless a project convention requires it. Keep
+classes distinct: bare numbering and labeled decisions are different classes.
## Keep
- Safety contracts and `// SAFETY:` justifications.
-- Error, panic, and invariant contracts that callers rely on.
-- Why the obvious approach wasn't taken.
-- "N.B." and test labels that flag a deliberate decision ("N.B. we deliberately
- allow src to not exist") or name the case under construction ("// not ours",
- "// ancestor is a regular file").
+- Error, panic, and invariant contracts callers rely on.
+- Why the obvious approach wasn't taken, stated as what goes wrong with it.
+- Notes and test labels that mark a deliberate decision ("N.B. we deliberately
+ allow src to not exist") or name the case being set up ("// not ours", "//
+ ancestor is a regular file").
- Links to issues, specs, or upstream bugs behind a workaround.
- License and attribution notices, even when cutting "ported from" narrative.
-- Doctests and examples. The compiler checks them. It doesn't check prose.
-- Conventions the language or project enforces: `# Errors` / `# Panics` /
- `# Safety` sections, `missing_docs`, the existing doc style.
+- Doctests and examples.
+- Language and project conventions: `# Errors` / `# Panics` / `# Safety`
+ sections, `missing_docs`, and the existing doc style.
## Cut on sight
@@ -108,24 +124,45 @@ one class: bare numbering is one class, labeled decisions another.
support for...".
- Mappings to code, designs, or reference implementations the code no longer
follows.
-- Commented-out code. TODOs that are done.
-- Throat-clearing openers: "Note:", "Important:", "This function...", "Helper
- to...", "This ensures that...". Bare sequence markers: "// 1.", "// step 2",
- "// Case 3".
-- Assert predictions in tests: a comment that announces what the next assert
- checks. Name the case or the setup intent instead.
+- Commented-out code and completed TODOs.
+- Empty openers and labels: "Note:", "Important:", "This function...", "Helper
+ to...", "This ensures that...", "upstream quirk:", "host side,". Bare sequence
+ markers: "// 1.", "// step 2", "// Case 3".
+- Tours: where else a topic lives, or how a request passes through other
+ components (`a -> b -> c`), when nothing at this line depends on it.
+- Assert predictions in tests: comments announcing what the next assert checks.
+ Name the case or setup intent instead.
- Hype and padding: "robust", "seamless", "comprehensive", "simply", "just".
- Links on types the signature already links.
-- Doc files that are work logs (summaries, plans, migration notes, status
+- Doc files that serve as work logs (summaries, plans, migration notes, status
reports). Delete the file and name it in your summary.
-No em dashes. An em dash usually attaches an aside that should be cut.
-Restructure the sentence rather than swapping in `;`, `:`, ` - `, or
-parentheses.
+## Sentences
+
+These rules apply to rewrites and new comments. They are not a sixth check. A
+comment that passes the five keeps its wording even if it breaks these rules.
+
+Shorten by reducing points, not squeezing words. Use whole sentences, but let
+labels like "// not ours" stay labels. Keep connectives ("because", "so",
+"otherwise") that explain the relationship between facts. Without them, the
+reader must reconstruct the reason. Don't compress into note-speak or replace a
+plain word with a shorter, vaguer one.
+
+Say what the thing is or does. Don't define it through what it isn't ("not a
+managed host", "unlike `retry`"). Use a negative when it rules out a likely
+wrong assumption ("without verifying it", "not thread-safe"). Name an
+alternative only when the reader would reach for it, then say what goes wrong
+with it.
+
+Use one point per sentence. Facts that support one point can share a sentence
+("`idx < self.len` is checked above, and `buf` never shrinks"). Separate points
+cannot, however you join them. Don't use em dashes or imitate them with `;`,
+`:`, ` - `, or parentheses. If a sentence needs such a join, check whether it
+holds two points and whether both belong.
## Examples
-## Over-explained contract
+### Over-explained contract
```rust
// Before
@@ -147,14 +184,14 @@ pub fn discover(root: &Path) -> Result<Vec<AgentDef>, Report>
/// # Errors
///
/// Fails if a directory can't be read or a file doesn't parse. The error
-/// names the file.
+/// names the path.
pub fn discover(root: &Path) -> Result<Vec<AgentDef>, Report>
```
The signature already says `Vec` and `Report`. "Walks, reads, parses, collects"
-narrates the body. What's left is the contract.
+narrates the body. Keep the contract.
-## History
+### History
```rust
// Before
@@ -167,10 +204,9 @@ let mut seen = BTreeMap::new();
let mut seen = BTreeMap::new();
```
-The reason survives. The history doesn't. Nobody reading this has seen the
-HashMap.
+Keep the reason, not the history. The reader need not know about the HashMap.
-## Cross-reference told as a story
+### Cross-reference told as a story
```rust
// Before
@@ -187,10 +223,11 @@ pub async fn download(url: &Url) -> Result<Bytes>
pub async fn download(url: &Url) -> Result<Bytes>
```
-The caller needs their obligation, not the architecture's history.
-`fetch_verified` doesn't exist, so that sentence can't help anyone.
+The caller needs their obligation, not the architecture's history. "Without
+verifying it" rules out a likely wrong assumption. The pointer survives because
+the caller must act on it. The removed helper is irrelevant.
-## The same fact in several places
+### The same fact in several places
```rust
// Before
@@ -214,13 +251,30 @@ pub fn write(self) -> Result<()>
fn retry_converges() { ... }
```
-The module doc holds the fact for maintainers and stays word for word. `write()`
-keeps its local obligation and drops the re-explanation. The test comment
-restates the fact to the same audience, and the test body already shows a retry
-succeeding. A README promising convergent retries would hold it for users, not
-count as a fourth copy.
+The module doc owns the convergence fact and stays unchanged. `write()` keeps
+its local failure guarantee. The test comment adds nothing beyond the test name
+and body. A `README` may repeat the convergence guarantee if it serves a
+different audience.
-## Implementation detail in the contract
+### A pile of facts rewritten around its point
+
+```rust
+// Before
+// default 0 (no retries); bundled defaults skipped (see config/README.md).
+// server side, retries go gateway -> backend
+cfg.set("retries", 3);
+
+// After
+// Set explicitly because we skip the bundled defaults, and without them
+// the client never retries.
+cfg.set("retries", 3);
+```
+
+The reader's question is why a default-looking value is set explicitly.
+"Because" connects the facts into an answer. The pointer and server-side path
+are tours; nothing here depends on them.
+
+### Implementation detail in the contract
```rust
// Before
@@ -233,17 +287,19 @@ count as a fourth copy.
pub fn thumbnail(&mut self, id: ImageId) -> Result<&Thumbnail>
// After
-/// Returns the thumbnail for `id`, generating and caching it on first use.
+/// Returns the thumbnail for `id`, generating and caching it on a cache miss.
pub fn thumbnail(&mut self, id: ImageId) -> Result<&Thumbnail> {
// ...
- // Triangle is faster but aliases visibly on line art.
+ // Triangle aliases visibly on line art.
let small = img.resize(w, h, FilterType::Lanczos3);
```
-Cache layout and decode steps can change without the contract changing. The
-filter choice is a real why, so it moves to the line it explains.
+Cache layout and decoding steps can change without changing the contract. The
+filter choice explains why, so it moves to the relevant line. Naming Triangle
+helps because a maintainer might choose it, and the comment explains the
+consequence.
-## Reference implementation
+### Reference implementation
```rust
// Before
@@ -253,15 +309,14 @@ filter choice is a real why, so it moves to the line it explains.
//! Java's `reserveEarliestAvailable()`.
// After
-//! Token-bucket rate limiter. Tokens refill lazily inside `acquire`, so an
-//! idle limiter costs nothing.
+//! Token-bucket rate limiter. Tokens refill lazily inside `acquire`, so
+//! idle limiters do no refill work.
```
-The reader won't open the Java code, and mappings to it rot as the code
-diverges. Keep the fact a user cares about. Keep a license notice if one is
-required.
+Mappings to reference code rot as implementations diverge. Keep the fact the
+reader needs, along with any required license or attribution notice.
-## Leave these alone
+### Leave these alone
```rust
// SAFETY: `idx < self.len` is checked above, and `buf` never shrinks.
@@ -272,10 +327,16 @@ fs::copy(&tmp, &dest)?;
fs::remove_file(&tmp)?;
```
-Each says something the code can't, and stays true as long as the code below it
-does.
+```nix
+# router runs openwrt
+router = { static_ip = "10.0.1.1"; };
+```
+
+These supply facts the code doesn't. The router comment identifies its OS in the
+author's style. Replacing it with "not a NixOS machine" would emphasize a
+comparison instead. Fixing its casing would restyle a comment that passed.
-## Prose docs
+### Prose docs
```markdown
<!-- Before, in README.md -->
@@ -285,21 +346,25 @@ does.
- **Removed** legacy `config.ini` support
```
-Delete the section. A README describes the project as it is. If users need the
-config format, document the format that exists.
+Delete the section. A `README` describes the project as it is. If users need the
+config format, document the current format.
## Finish
-1. Re-scan the survivors. For each fact you assigned a home, grep the fact, not
- one keyword: every wording it might wear ("rendered", "literal", "template
- text"). One hit per audience; more means a missed duplicate. Search every
- file that could hold a home, including prose docs outside the reviewed set.
-2. Search comments in the reviewed files for `—` and time markers: history
- (`now`, `no longer`, `instead`, `previously`, `old`, `new`, `refactor`) and
- plans (`will`, `could`, `eventually`, `not implemented yet`). Recheck each
- hit.
-3. Build docs and run doctests if the project has them. Deletions break
- intra-doc links.
-4. Report briefly: scope covered (files examined), facts that had several homes
- and where they ended up, deleted files, comments flagged as possibly wrong,
- suggested renames and other code moves. No per-edit changelog.
+1. Re-scan the survivors. For each fact assigned a home, search for all its
+ phrasings, not one keyword ("rendered", "literal", "template text"). Verify
+ one statement of each fact per audience. Keep additional references only when
+ they satisfy check 4. Search every plausible home, including prose docs
+ outside the reviewed scope.
+2. Search comments in the reviewed files for `—`, mid-sentence `;` and `:`,
+ contrasts (`unlike`, `not a`, `rather than`), pointers (`see`, `lives in`,
+ `->`, file paths), history (`now`, `no longer`, `instead`, `previously`,
+ `old`, `new`, `refactor`), and plans (`will`, `could`, `eventually`,
+ `not implemented yet`). Each hit is a candidate, not a failure. Change it
+ only if the comment fails a check, or if text you wrote breaks "Sentences".
+3. Build docs and run doctests if available. Deletions can break intra-doc
+ links.
+4. Report briefly: files examined; repeated facts and their chosen homes; points
+ dropped with no other home (one line each so the author can veto); deleted
+ files; possibly incorrect comments; suggested renames and other code moves.
+ No per-edit changelog.