diff --git a/skills/kvb-code-clean/SKILL.md b/skills/kvb-code-clean/SKILL.md index 20fde13..d2b31e7 100644 --- a/skills/kvb-code-clean/SKILL.md +++ b/skills/kvb-code-clean/SKILL.md @@ -28,15 +28,16 @@ Delete every comment the change adds except three kinds: Restated code, step narration, banners, "Note that", rationale prose and whatever the code, a name or the commit message already carries go. When in doubt, delete. Docstrings and script headers count: prose outside the -parameter doc goes too, and a `#` or `//` grep misses it. +parameter doc goes too. -Block-comment languages are not line-oriented: in Jinja (`{# … #}`), C-like -(`/* … */`), HTML (``) and kin, a closer alone on its line looks -like a comment line; deleted with the rest, the opener swallows every live -statement down to the next closer. Delete such a block by hand, count the -delimiters (`grep -c '{#'` against `grep -c '#}'`), re-parse the file and -diff it against `git show HEAD:`: only comment lines may leave. A green -lint proves nothing here. +Run `${CLAUDE_SKILL_DIR}/scripts/comments.py --repo ` (`--staged`, +`--file` as in Step 2): it lists every comment and docstring the change adds, +Jinja `{# … #}` in `.j2` templates included; exit 1 means hits. Judge each +hit not marked `directive` by the three kinds above, and run it again until +only kept hits remain. A repository's own comment lint runs too, because a +`--no-verify` commit skips the hook that would run it. Before deleting a +block comment (`{# … #}`, `/* … */`, ``), read `comments.md` next +to this file. ## Step 2: defaults diff --git a/skills/kvb-code-clean/comments.md b/skills/kvb-code-clean/comments.md new file mode 100644 index 0000000..ddcf8bf --- /dev/null +++ b/skills/kvb-code-clean/comments.md @@ -0,0 +1,9 @@ +# Deleting block comments + +Block-comment languages are not line-oriented: in Jinja (`{# … #}`), C-like +(`/* … */`), HTML (``) and kin, a closer alone on its line looks +like a comment line; deleted with the rest, the opener swallows every live +statement down to the next closer. Delete such a block by hand, count the +delimiters (`grep -c '{#'` against `grep -c '#}'`), re-parse the file and +diff it against `git show HEAD:`: only comment lines may leave. A green +lint proves nothing here. diff --git a/skills/kvb-code-clean/scripts/comments.py b/skills/kvb-code-clean/scripts/comments.py new file mode 100755 index 0000000..cd0b322 --- /dev/null +++ b/skills/kvb-code-clean/scripts/comments.py @@ -0,0 +1,199 @@ +#!/usr/bin/env python3 +"""Report every comment and docstring a change adds, for the agent to delete or keep. + +Args: + --repo: a directory inside the git work tree to scan. + --staged: scan only the staged changes (git diff --cached) against the + index version of each file. Without it: every change against HEAD, + staged and unstaged, against the working tree file. + --file: scan this whole file, every line counted as added; repeatable, + relative to --repo or absolute. Untracked files are in no diff. + --json: print the hits as a JSON list instead of tab-separated lines. + +Output: + One line per hit: file:line, language, kind (comment or docstring), the + text, and `directive` for a tool directive (noqa, nocheck, shellcheck, ...). + +Exit codes: + 0 no hits but directives, 1 hits, 2 usage error, git failure, or a file + that cannot be read or, for Python, parsed. +""" + +from __future__ import annotations + +import argparse +import ast +import bisect +import io +import json +import re +import sys +import tokenize +from dataclasses import asdict, dataclass +from pathlib import Path, PurePosixPath + +import defaults + +EXTRA = { + ".py": ("python", defaults.DQ + "|" + defaults.SQ, defaults.HASH), + ".rb": ("ruby", defaults.DQ + "|" + defaults.SQ, defaults.HASH), + ".php": ("php", defaults.DQ + "|" + defaults.SQ, defaults.HASH + "|" + defaults.SLASH), + ".css": ("css", defaults.DQ + "|" + defaults.SQ, r"/\*.*?\*/"), + ".scss": ("css", defaults.DQ + "|" + defaults.SQ, defaults.SLASH), + ".html": ("html", "", r""), + ".xml": ("html", "", r""), + ".lua": ("lua", defaults.DQ + "|" + defaults.SQ, r"--\[\[.*?\]\]|--[^\n]*"), +} +DIRECTIVE = re.compile( + r"\b(?:noqa|nocheck|nosec|type:\s*ignore|pragma|pylint|mypy|ruff|pyright|isort|fmt:\s*(?:on|off)" + r"|coding[:=]|shellcheck|yamllint|hadolint|ansible-lint|markdownlint|eslint|stylelint|prettier" + r"|rubocop|frozen_string_literal|phpcs|phpstan|psalm|jscpd|checkov|tflint)\b", + re.IGNORECASE, +) + + +@dataclass(frozen=True) +class Hit: + file: str + line: int + language: str + kind: str + text: str + directive: bool + + +def patterns(path: str) -> list[tuple[str, str, str]]: + found = [(lang, *defaults.LANGS[lang][:2]) for lang in defaults.languages(path) if lang != "python"] + stem = defaults.JINJA_SUFFIX.sub("", PurePosixPath(path).name) + extra = EXTRA.get(PurePosixPath(stem).suffix) + return found + ([extra] if extra else []) + + +def python_lang(path: str) -> bool: + return defaults.languages(path) == ["python"] + + +def known(path: str) -> bool: + return python_lang(path) or bool(patterns(path)) + + +def touched(lines: set[int], wanted: set[int] | None) -> bool: + return wanted is None or bool(lines & wanted) + + +def python_comments(path: str, tokens: list[tokenize.TokenInfo], wanted: set[int] | None) -> list[Hit]: + return [ + Hit(path, token.start[0], "python", "comment", defaults.short(token.string), bool(DIRECTIVE.search(token.string))) + for token in tokens + if token.type == tokenize.COMMENT + and not (token.start[0] == 1 and token.string.startswith("#!")) + and touched({token.start[0]}, wanted) + ] + + +def python_docstrings(path: str, tree: ast.AST, wanted: set[int] | None) -> list[Hit]: + nodes = [ + node + for node in ast.walk(tree) + if isinstance(node, ast.Expr) and isinstance(node.value, ast.Constant) and isinstance(node.value.value, str) + ] + return [ + Hit(path, node.lineno, "python", "docstring", defaults.short(node.value.value), False) + for node in nodes + if touched(set(range(node.lineno, (node.end_lineno or node.lineno) + 1)), wanted) + ] + + +def scan_python(path: str, text: str, wanted: set[int] | None) -> list[Hit]: + try: + tree = ast.parse(text, filename=path) + tokens = list(tokenize.generate_tokens(io.StringIO(text).readline)) + except (SyntaxError, ValueError, tokenize.TokenError) as error: + raise defaults.ScanError(f"{path}: unparseable Python: {error}") from error + return python_comments(path, tokens, wanted) + python_docstrings(path, tree, wanted) + + +def masked_pattern(langs: list[tuple[str, str, str]]) -> re.Pattern[str]: + strings = "|".join(pattern for _, pattern, _ in langs if pattern) + source = "(?P" + "|".join(pattern for _, _, pattern in langs) + ")" + return re.compile(source + (f"|(?P{strings})" if strings else ""), re.MULTILINE | re.DOTALL) + + +def scan_masked(path: str, text: str, langs: list[tuple[str, str, str]], wanted: set[int] | None) -> list[Hit]: + _, spans = defaults.mask(text, masked_pattern(langs)) + starts = [0] + [newline.end() for newline in re.finditer("\n", text)] + language = "+".join(lang for lang, _, _ in langs) + hits = [] + for begin, end in spans: + body = text[begin:end] + if begin == 0 and body.startswith("#!"): + continue + first = bisect.bisect_right(starts, begin) + lines = set(range(first, bisect.bisect_right(starts, max(begin, end - 1)) + 1)) + if touched(lines, wanted): + hits.append(Hit(path, first, language, "comment", defaults.short(body), bool(DIRECTIVE.search(body)))) + return hits + + +def scan(path: str, text: str, wanted: set[int] | None) -> list[Hit]: + if python_lang(path): + return scan_python(path, text, wanted) + langs = patterns(path) + return scan_masked(path, text, langs, wanted) if langs else [] + + +def changed_targets(root: Path, staged: bool) -> dict[str, tuple[str, set[int] | None]]: + targets: dict[str, tuple[str, set[int] | None]] = {} + for path, lines in defaults.added_lines(root, staged).items(): + if known(path): + text = defaults.git(root, "cat-file", "blob", f":{path}") if staged else defaults.read(root / path) + targets[path] = (text, lines) + return targets + + +def file_targets(repo: Path, root: Path, files: list[str]) -> dict[str, tuple[str, set[int] | None]]: + targets: dict[str, tuple[str, set[int] | None]] = {} + for name in files: + target = (repo / name).resolve() + if not target.is_file(): + raise defaults.ScanError(f"{name}: no such file") + shown = target.relative_to(root).as_posix() if target.is_relative_to(root) else str(target) + if known(shown): + targets[shown] = (defaults.read(target), None) + else: + print(f"comments: {shown}: no comment syntax known for this file type", file=sys.stderr) + return targets + + +def collect(repo: Path, staged: bool, files: list[str]) -> list[Hit]: + if not repo.is_dir(): + raise defaults.ScanError(f"{repo} is not a directory") + root = Path(defaults.git(repo, "rev-parse", "--show-toplevel").strip()).resolve() + targets = changed_targets(root, staged) | file_targets(repo, root, files) + hits = [hit for path, (text, wanted) in targets.items() for hit in scan(path, text, wanted)] + return sorted(hits, key=lambda hit: (hit.file, hit.line, hit.kind)) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--repo", required=True) + parser.add_argument("--staged", action="store_true") + parser.add_argument("--file", action="append", default=[]) + parser.add_argument("--json", action="store_true") + args = parser.parse_args() + try: + hits = collect(Path(args.repo), args.staged, args.file) + except defaults.ScanError as error: + print(f"comments: {error}", file=sys.stderr) + return 2 + if args.json: + print(json.dumps([asdict(hit) for hit in hits], indent=1)) + else: + for hit in hits: + row = [f"{hit.file}:{hit.line}", hit.language, hit.kind, hit.text] + (["directive"] if hit.directive else []) + print("\t".join(row)) + return 1 if any(not hit.directive for hit in hits) else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_comments.py b/tests/test_comments.py new file mode 100644 index 0000000..175b99c --- /dev/null +++ b/tests/test_comments.py @@ -0,0 +1,128 @@ +from __future__ import annotations + +import json +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] +SCRIPT = REPO_ROOT / "skills" / "kvb-code-clean" / "scripts" / "comments.py" +BASE = { + "casc.yaml.j2": "# Header kept from the base.\njenkins:\n url: x\n", + "app.py": "def old():\n return 1\n", + "run.sh": "#!/bin/bash\necho old\n", +} +UNSTAGED = { + "casc.yaml.j2": BASE["casc.yaml.j2"] + + "{# Without an override the plugin requests every scope. #}\n" + + " scopes: \"openid\"\n" + + " # a yaml narration line\n" + + ' note: "{{ value }}"\n', + "app.py": BASE["app.py"] + + "\n\ndef new(a):\n" + + ' """Return a."""\n' + + " # explain the next line\n" + + " return a # noqa: E501\n", + "run.sh": BASE["run.sh"] + + "# shellcheck disable=SC2086\n" + + 'echo "#not a comment" $X\n', +} +STAGED = { + "style.css": "a { color: red; }\n/* palette narration */\n", + "page.html.j2": "

{{ title }}

\n\n{# jinja note #}\n", + "Dockerfile": "FROM debian\n# install the tools\nRUN true\n", +} +UNSTAGED_HITS = [ + ("app.py:6", "python", "docstring", False), + ("app.py:7", "python", "comment", False), + ("app.py:8", "python", "comment", True), + ("casc.yaml.j2:4", "jinja+yaml", "comment", False), + ("casc.yaml.j2:6", "jinja+yaml", "comment", False), + ("run.sh:3", "shell", "comment", True), +] +STAGED_HITS = [ + ("Dockerfile:2", "dockerfile", "comment", False), + ("page.html.j2:2", "jinja+html", "comment", False), + ("page.html.j2:3", "jinja+html", "comment", False), + ("style.css:2", "css", "comment", False), +] + + +class TestComments(unittest.TestCase): + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.repo = Path(tmp.name) / "repo" + self.repo.mkdir() + self.env = { + "PATH": "/usr/bin:/bin", + "HOME": tmp.name, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_CEILING_DIRECTORIES": tmp.name, + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@example.org", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@example.org", + } + self._git("init", "-q", "-b", "main") + self._write(BASE) + self._git("add", ".") + self._git("commit", "-q", "-m", "base") + + def _git(self, *args: str) -> str: + return subprocess.run( + ["git", *args], cwd=self.repo, env=self.env, capture_output=True, text=True, check=True + ).stdout + + def _write(self, files: dict[str, str]) -> None: + for name, text in files.items(): + (self.repo / name).write_text(text, encoding="utf-8") + + def _change(self) -> None: + self._write(STAGED) + self._git("add", *STAGED) + self._write(UNSTAGED) + + def _hits(self, *args: str) -> tuple[int, list[tuple]]: + result = subprocess.run( + [sys.executable, str(SCRIPT), "--repo", str(self.repo), "--json", *args], + env=self.env, + capture_output=True, + text=True, + check=False, + ) + self.assertIn(result.returncode, (0, 1), result.stderr) + rows = json.loads(result.stdout) + return result.returncode, sorted( + (f"{r['file']}:{r['line']}", r["language"], r["kind"], r["directive"]) for r in rows + ) + + def test_worktree_reports_every_added_comment_and_nothing_from_the_base(self): + self._change() + code, hits = self._hits() + self.assertEqual(code, 1) + self.assertEqual(hits, sorted(UNSTAGED_HITS + STAGED_HITS)) + + def test_staged_reads_the_index_version(self): + self._change() + code, hits = self._hits("--staged") + self.assertEqual(code, 1) + self.assertEqual(hits, sorted(STAGED_HITS)) + + def test_directives_alone_exit_zero(self): + self._write({"run.sh": BASE["run.sh"] + "# shellcheck disable=SC2086\necho $X\n"}) + code, hits = self._hits() + self.assertEqual(code, 0) + self.assertEqual(hits, [("run.sh:3", "shell", "comment", True)]) + + def test_file_scans_an_untracked_template_whole(self): + (self.repo / "new.yml.j2").write_text("a: 1\n{# narration #}\n", encoding="utf-8") + code, hits = self._hits("--file", "new.yml.j2") + self.assertEqual(code, 1) + self.assertEqual(hits, [("new.yml.j2:2", "jinja+yaml", "comment", False)]) + + +if __name__ == "__main__": + unittest.main()