diff --git a/skills/concise-comments/SKILL.md b/skills/concise-comments/SKILL.md new file mode 100644 index 0000000..fda23a6 --- /dev/null +++ b/skills/concise-comments/SKILL.md @@ -0,0 +1,41 @@ +--- +name: concise-comments +description: Rules for concise code comments/docstrings, plus a pass to trim verbose ones. Use when writing or reviewing code, or when asked to reduce comment verbosity / trim over-commented code / cut docstring bloat. Referenced by /spec and /process-reviews. +--- + +# Concise comments & docstrings + +Prevention: follow these when writing. Cleanup: run the pass below when invoked as `/concise-comments`. + +## The test +Keep a comment only if a competent reader of THIS codebase couldn't infer it from the code and names. Otherwise delete it, or cut it to the one missing fact in one line. + +## For / not for +For: the non-obvious *why* (constraint, invariant, footgun, edge-case gotcha) and a one-line *what* on public APIs. +Not for: design narrative, alternatives-considered, defending the approach, repro steps, byte-count walkthroughs, ticket history, or restating the code. Those go in the PR description, DECISIONS.md, the issue, or a test. + +## Limits (defaults; exceed only with a stated reason) +- Inline comment: 1 line. Needing 3+ lines to justify a line of code → rename things or move the rationale out. +- Function/method docstring: 1-line summary; +≤3 lines only for genuinely subtle behaviour. +- Module docstring: ≤8 lines. Test docstring: 1 line, or none if the name says it. +- Section banner: ≤1 label line, no `---` paragraphs. + +Say it once at the definition; don't re-explain at every call site. + +## Delete on sight +- **Restates the code** (`# strip prefix` above `x.split(...)`). +- **Ticket-ID noise** (`DEV-1234:` on every line/attribute; the value is the fact). +- **Design essay / justification debate** (paragraphs on why this is right and alternatives wrong). +- **Worked examples & repros** (byte counts, sample output) → move to the test. +- **Docstring echoing the name** (`def test_x: """Test x."""`). +- **Emphasis theatre** (ALL-CAPS sentences, "astronomically", "provably", "THE … requirement"). + +Keep, as ≤1 line: a real cross-dialect/edge gotcha, a non-obvious invariant, a "looks wrong but isn't", an ordering/side-effect warning. When unsure a *why* is inferable, keep it but shorten it. + +## The pass (`/concise-comments`) +1. Scope: default to the branch diff (`git diff $(git merge-base HEAD) -- '*.py'`); or the path/file the user names. Only added/modified comments unless told to sweep. +2. Edit comments/docstrings ONLY — never code, string literals, test data, asserts, imports, markers. +3. Verify: run affected tests + linter (green); confirm `git diff` changed no code line; measure before/after with `scripts/count_comments.py --range `. +4. Report per-file and total lines before → after + ratio, and that tests/lint pass. + +Scale to the ask: "trim a bit" = kill the essays; "3x" / "massively" = apply every limit hard. diff --git a/skills/concise-comments/scripts/count_comments.py b/skills/concise-comments/scripts/count_comments.py new file mode 100755 index 0000000..2f0982e --- /dev/null +++ b/skills/concise-comments/scripts/count_comments.py @@ -0,0 +1,87 @@ +#!/usr/bin/env python3 +"""Count comment + docstring lines in Python files. + +Comments via tokenize, docstrings via AST (module/class/function first stmt). + + count_comments.py ... # per-file + total counts + count_comments.py --range ... # net added vs a git ref (working tree) +""" +from __future__ import annotations + +import ast +import io +import subprocess +import sys +import tokenize + + +def counts(src: str | None) -> tuple[int, int]: + if src is None: + return (0, 0) + try: + comment = sum( + 1 for t in tokenize.generate_tokens(io.StringIO(src).readline) + if t.type == tokenize.COMMENT + ) + except Exception: + comment = 0 + doc = 0 + try: + tree = ast.parse(src) + except Exception: + return (comment, 0) + kinds = (ast.Module, ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef) + for node in ast.walk(tree): + if isinstance(node, kinds): + body = getattr(node, "body", []) + first = body[0] if body else None + if ( + isinstance(first, ast.Expr) + and isinstance(getattr(first, "value", None), ast.Constant) + and isinstance(first.value.value, str) + ): + doc += first.end_lineno - first.lineno + 1 + return (comment, doc) + + +def git_show(ref: str, path: str) -> str | None: + r = subprocess.run(["git", "show", f"{ref}:{path}"], capture_output=True, text=True) + return r.stdout if r.returncode == 0 else None + + +def read(path: str) -> str | None: + try: + return open(path, encoding="utf-8").read() + except FileNotFoundError: + return None + + +def main(argv: list[str]) -> int: + if argv and argv[0] == "--range": + ref = argv[1] + paths = argv[2:] + tc = td = 0 + for p in paths: + b = counts(git_show(ref, p)) + h = counts(read(p)) + dc, dd = h[0] - b[0], h[1] - b[1] + print(f"{dc + dd:+5d} comment={dc:+4d} doc={dd:+4d} {p}") + tc += dc + td += dd + print(f"{'=' * 50}\nNET ADDED total={tc + td} (comment {tc}, docstring {td})") + return 0 + if not argv: + print(__doc__) + return 1 + tc = td = 0 + for p in argv: + c, d = counts(read(p)) + print(f"{c + d:5d} comment={c:4d} doc={d:4d} {p}") + tc += c + td += d + print(f"{'=' * 50}\nTOTAL={tc + td} (comment {tc}, docstring {td})") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv[1:])) diff --git a/skills/process-reviews/SKILL.md b/skills/process-reviews/SKILL.md index 31cf09b..399479d 100644 --- a/skills/process-reviews/SKILL.md +++ b/skills/process-reviews/SKILL.md @@ -119,7 +119,7 @@ Don't dedupe across sources blindly — if multiple sources flag the same area, For each entry, classify as VALID or INVALID using a source-appropriate signal: -- **CodeRabbit / Sonar / Codex** — READ the cited file at the cited line(s). Apply the user's global rules from `~/.claude/CLAUDE.md` (trust internal code, validate only at boundaries; imports at the top; etc.). Lean toward VALID when uncertain — false positives are easier to defend later than missed bugs now. +- **CodeRabbit / Sonar / Codex** — READ the cited file at the cited line(s). Apply the user's global rules from `~/.claude/CLAUDE.md` (trust internal code, validate only at boundaries; imports at the top; etc.). Lean toward VALID when uncertain — false positives are easier to defend later than missed bugs now. Over-verbose comments/docstrings in the diff (design essays, ticket-ID-on-every-line, code-restating comments, name-echo docstrings) are a valid maintainability finding per the `concise-comments` skill — flag them even if no bot did. - **CI failure** — READ the failed-step log excerpt in the JSON. Classify as INVALID only when the failure is clearly unrelated to this PR's changes (network blip / runner died / well-known flaky test that the user has previously confirmed is flaky / out-of-date Action that times out before doing anything). Otherwise VALID. **A test failure that points at code this PR touched is always VALID** — don't argue your way out of it. Codex-specific validation hazards (apply on top of the shared CodeRabbit/Sonar rules): @@ -237,6 +237,7 @@ Do **not** start writing code until the user picks. - NEVER call any "resolve" mutation on CodeRabbit threads (no `resolveReviewThread`, no UI-equivalent). Only `/replies`. This is a hard global rule from the user. - NOSONAR suppressions MUST include the rule key and a reason. Bare `// NOSONAR` is forbidden. The rule key inside the parentheses must be alphanumeric only — never include the `python:` / `javascript:` / etc. language prefix (it makes Sonar treat the suppression as malformed). - This skill stops at the plan. Code fixes happen only after the user picks a group. +- When you later write fixes, keep any comments/docstrings concise per the `concise-comments` skill — don't add design essays or code-restating comments while resolving findings. - If validation makes you LESS than 80% confident an issue is invalid, classify it as VALID and put it in the plan. Better to over-fix than to silently dismiss a real bug. ## When NOT to use this skill diff --git a/skills/spec/SKILL.md b/skills/spec/SKILL.md index a3cdc41..ff8dfff 100644 --- a/skills/spec/SKILL.md +++ b/skills/spec/SKILL.md @@ -41,6 +41,14 @@ Interview me in detail. Cover implementation approach, edge cases, gotchas, design choices, tradeoffs, and constraints. Skip obvious questions. Ask one at a time and build on my answers. + +**IMPORTANT — WHENEVER you ask me to make a choice, you MUST give, for the +decision, explicit PROS and CONS of each option AND a clear RECOMMENDATION of +which one you'd pick and why. This is non-negotiable. Never present options as +a bare menu. If you use a structured/multiple-choice prompt, spell out the +pros, the cons, and the recommendation inside it — do not rely on a one-line +description to carry them.** + When you think you have enough information, return a detailed, complete spec. In the spec you write, NEVER take shortcuts or make simplifications or extensions to the original requirements without asking me about each one first. @@ -80,6 +88,11 @@ Implement the plan. Run the full non-integration test suite (per `feedback_unit_tests_only.md`) after changes and fix any failures. Do not declare done until every test from Step 4/5 passes. +When writing code AND tests (Steps 4 and 6), follow the `concise-comments` +skill: no design essays, ticket-ID-on-every-line, or code-restating comments; +docstrings to a line. Rationale belongs in the spec / PR description / this +issue — not in code — so verbose comments never get written in the first place. + ## Step 7 — Ask me to commit, push, and PR Once all tests pass, stop and ask me whether to commit, push, and open a PR.