Add concise-comments skill; wire into /spec & /process-reviews - #9
Add concise-comments skill; wire into /spec & /process-reviews#9ZmeiGorynych wants to merge 1 commit into
Conversation
New skill with the rules for terse comments/docstrings (limits + delete-on-sight anti-patterns) and a `/concise-comments` trimming pass, plus scripts/count_comments.py to measure reduction. Referenced from /spec (write step) and /process-reviews (validate + fix) so verbose comments are caught early. spec/SKILL.md also carries the local pros/cons-in-choices refinement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change adds a ChangesConcise Comments
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
skills/concise-comments/scripts/count_comments.py (1)
26-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNarrow the exception handlers.
The two
except Exceptionblocks turn unexpected failures into valid-looking counts. Catch only the expected tokenization and AST parse errors. Let unexpected failures surface.Suggested direction
- except Exception: + except (tokenize.TokenError, IndentationError): ... - except Exception: + except (SyntaxError, ValueError):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/concise-comments/scripts/count_comments.py` around lines 26 - 31, Replace the broad exception handlers in the comment-counting logic with only the expected tokenization error for the tokenization path and AST parse error for the ast.parse path. Preserve the fallback counts for those expected failures, while allowing all other exceptions to propagate.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/concise-comments/scripts/count_comments.py`:
- Around line 65-71: Update the reporting logic around counts(), the per-file
print, and the final total print so --range output includes comment and
docstring before→after counts and the required ratio, matching the report
contract in SKILL.md. Define and consistently apply behavior for a zero baseline
ratio; alternatively, update the documented requirement to explicitly permit
delta-only output.
- Around line 59-62: Update main so the --range branch validates that argv
contains both a reference and at least one path before accessing argv[1] or
assigning paths. Return the existing usage-error result for missing arguments,
while preserving the current range processing when both are supplied.
- Line 18: Add comprehensive tests for the utility centered on counts, covering
valid and malformed Python, module/class/function/async docstrings, missing
files, invalid Git references, deleted paths, and incomplete --range CLI
arguments. Verify expected counts and error behavior for each case, including
both valid and malformed inputs.
- Around line 52-56: Update read so missing files raise an error in normal mode
instead of returning None, while allowing None for intentionally missing
working-tree paths when --range deletion analysis is active. Thread the mode or
range context through the callers of read and preserve the existing
deletion-count behavior.
- Around line 47-49: Update git_show to validate the Git ref separately before
reading ref:path, preserving distinct handling for invalid refs and missing
paths. Return None only when the reference is valid but the requested path is
unavailable, and ensure invalid refs do not become empty content that is
interpreted as additions.
---
Nitpick comments:
In `@skills/concise-comments/scripts/count_comments.py`:
- Around line 26-31: Replace the broad exception handlers in the
comment-counting logic with only the expected tokenization error for the
tokenization path and AST parse error for the ast.parse path. Preserve the
fallback counts for those expected failures, while allowing all other exceptions
to propagate.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df945488-faa0-4427-bda0-4bf850cf8d65
📒 Files selected for processing (4)
skills/concise-comments/SKILL.mdskills/concise-comments/scripts/count_comments.pyskills/process-reviews/SKILL.mdskills/spec/SKILL.md
| import tokenize | ||
|
|
||
|
|
||
| def counts(src: str | None) -> tuple[int, int]: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add solid tests for the shipped utility.
This change adds parsing, Git-reference, file, and CLI behavior without adding tests. Cover valid and malformed Python, module/class/function/async docstrings, missing files, invalid refs, deleted paths, and incomplete --range arguments.
As per coding guidelines: “Whenever you do any changes to production code ... ALWAYS add solid test coverage for it.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/concise-comments/scripts/count_comments.py` at line 18, Add
comprehensive tests for the utility centered on counts, covering valid and
malformed Python, module/class/function/async docstrings, missing files, invalid
Git references, deleted paths, and incomplete --range CLI arguments. Verify
expected counts and error behavior for each case, including both valid and
malformed inputs.
Source: Coding guidelines
| 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 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- file map ---'
ast-grep outline skills/concise-comments/scripts/count_comments.py
printf '%s\n' '--- relevant source ---'
cat -n skills/concise-comments/scripts/count_comments.py | sed -n '1,140p'
printf '%s\n' '--- tracked related files ---'
git ls-files | rg '(^|/)(count_comments|test|tests)' | head -100
printf '%s\n' '--- structural references to git_show ---'
ast-grep run --lang python --pattern 'git_show($_, $_)' skills/concise-comments/scripts/count_comments.pyRepository: MotleyAI/claude-configs
Length of output: 3891
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import subprocess
def git_show(ref: str, path: str):
r = subprocess.run(
["git", "show", f"{ref}:{path}"],
capture_output=True,
text=True,
)
return r.stdout if r.returncode == 0 else None, r.returncode, r.stderr.strip()
head = subprocess.run(
["git", "rev-parse", "--verify", "HEAD"],
capture_output=True,
text=True,
check=True,
).stdout.strip()
cases = [
("invalid-ref", "HEAD_DOES_NOT_EXIST", "skills/concise-comments/scripts/count_comments.py"),
("missing-path", head, "skills/concise-comments/scripts/path_does_not_exist.py"),
("valid-path", head, "skills/concise-comments/scripts/count_comments.py"),
]
for name, ref, path in cases:
value, code, error = git_show(ref, path)
print(f"{name}: returncode={code}, result_is_none={value is None}, stdout_len={len(value or '')}, stderr={error!r}")
PYRepository: MotleyAI/claude-configs
Length of output: 563
Validate the Git reference before reading the file. git_show maps both invalid references and missing paths to None, so an invalid ref produces false additions. Validate ref separately, then treat only a missing ref:path as empty content.
🧰 Tools
🪛 ast-grep (0.45.1)
[error] 47-47: Command coming from incoming request
Context: subprocess.run(["git", "show", f"{ref}:{path}"], capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.1)
[error] 48-48: subprocess call: check for execution of untrusted input
(S603)
[error] 48-48: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/concise-comments/scripts/count_comments.py` around lines 47 - 49,
Update git_show to validate the Git ref separately before reading ref:path,
preserving distinct handling for invalid refs and missing paths. Return None
only when the reference is valid but the requested path is unavailable, and
ensure invalid refs do not become empty content that is interpreted as
additions.
| def read(path: str) -> str | None: | ||
| try: | ||
| return open(path, encoding="utf-8").read() | ||
| except FileNotFoundError: | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make missing-file handling mode-aware.
read maps FileNotFoundError to None, so normal mode reports a misspelled path as a successful zero count. Fail for missing paths in normal mode. Allow missing working-tree paths only when --range intentionally measures deletions.
Also applies to: 77-79
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 53-53: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path, encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/concise-comments/scripts/count_comments.py` around lines 52 - 56,
Update read so missing files raise an error in normal mode instead of returning
None, while allowing None for intentionally missing working-tree paths when
--range deletion analysis is active. Thread the mode or range context through
the callers of read and preserve the existing deletion-count behavior.
| def main(argv: list[str]) -> int: | ||
| if argv and argv[0] == "--range": | ||
| ref = argv[1] | ||
| paths = argv[2:] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate --range arguments before indexing argv.
--range without a reference raises IndexError. --range REF also lacks a required path. Return a usage error unless both a reference and at least one path are present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/concise-comments/scripts/count_comments.py` around lines 59 - 62,
Update main so the --range branch validates that argv contains both a reference
and at least one path before accessing argv[1] or assigning paths. Return the
existing usage-error result for missing arguments, while preserving the current
range processing when both are supplied.
| 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})") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align --range output with the skill’s report contract.
This branch prints only deltas. skills/concise-comments/SKILL.md Line 39 requires per-file and total before→after counts plus a ratio. Print those values and define the zero-baseline ratio, or change the skill requirement to match the delta-only output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/concise-comments/scripts/count_comments.py` around lines 65 - 71,
Update the reporting logic around counts(), the per-file print, and the final
total print so --range output includes comment and docstring before→after counts
and the required ratio, matching the report contract in SKILL.md. Define and
consistently apply behavior for a zero baseline ratio; alternatively, update the
documented requirement to explicitly permit delta-only output.
1 participant
What
Adds a
concise-commentsskill that captures the rules for keeping code comments/docstrings terse, and provides a/concise-commentspass to trim over-verbose ones from existing code.skills/concise-comments/SKILL.md— the test ("could a reader of this codebase infer it?"), what comments are for vs. what belongs in the PR/DECISIONS/issue/tests, hard line limits, a delete-on-sight anti-pattern list, and the trimming-pass workflow (scope → edit comments-only → verify tests/lint + comment-only diff → measure).skills/concise-comments/scripts/count_comments.py— counts comment + docstring lines (tokenize for#, AST for docstrings); supports--range <ref>for net-added over a git range, to measure reduction.Prevention wiring
/spec— implement/test steps now say to followconcise-commentsso bloat isn't written in the first place./process-reviews— over-verbose comments are a valid maintainability finding to flag; fix-time comments must stay concise.Origin
Distilled from a real cleanup (SLayer DEV-1756) that cut added comment/docstring lines ~3.1× with the code AST provably unchanged.
Note:
skills/spec/SKILL.mdalso carries a pre-existing local refinement (pros/cons/recommendation on every choice), which rides along in the same file.Summary by CodeRabbit
New Features
Documentation