diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0f5cd1c99..125b7b7ef 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -5,3 +5,5 @@ SPDX-License-Identifier: Apache-2.0 When reviewing a pull request, follow `REVIEW.md` at the repository root. When writing code, follow `AGENTS.md`. +Keep code comments to at most two lines that state the constraint or the reason; +link an issue or PR for the backstory. CI fails longer comment blocks. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e6153a776..87bbf1997 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -198,6 +198,12 @@ jobs: skip=() if [ "$SKIP" = true ]; then skip=(--skip-label); fi python scripts/check_changelog_fragment.py --base "origin/$BASE_REF" --head HEAD "${skip[@]}" + # Fails a comment block over two lines that the PR adds or edits; AGENTS.md has the rule. + - name: Comment blocks + if: ${{ !cancelled() && github.event_name == 'pull_request' }} + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: python scripts/check_comment_blocks.py --base "origin/$BASE_REF" --head HEAD # Lists the docs pages whose `depends_on` front matter matches a file this PR # changes while the PR leaves the page alone, in the job summary and as a # warning on each page (scripts/check_docs_review_triggers.py). A prompt to diff --git a/AGENTS.md b/AGENTS.md index 9f223b277..b79d98a20 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -41,6 +41,10 @@ also holds a PR to: (`tests/test_docs_code_refs.py`): rename in code, rename in docs. - Tests run offline: a connection off loopback fails the test, so mock LLM and network calls. +- Comments of at most two lines (`scripts/check_comment_blocks.py`): say the + constraint or the reason, and end with an issue or PR URL when a reader needs + the backstory. A block of three or more `#` lines fails once the PR adds or + edits a line in it, so shorten an old block when you touch it. - User outcomes (`tests/outcomes/*.json`, checked by `pytest --outcomes`): a test marked `@pytest.mark.outcome(...)` is evidence for a promise in the docs; weaken or drop it only together with the doc and the contract. The same diff --git a/REVIEW.md b/REVIEW.md index b82910fe0..76c6d1590 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -18,8 +18,8 @@ checks. - A command gives a wrong number, verdict or exit code, or crashes where it should print a message. - A test passes when the behaviour it names is broken. -- A doc, docstring, PR title or changelog fragment says something the code - does not do. +- A doc, docstring, comment, PR title or changelog fragment says something the + code does not do. - A stored record is written or read with keys or a shape other than the schema's. - Behaviour differs on Windows or with a `None`, empty or missing input. @@ -39,6 +39,9 @@ checks. - A nit is one the author can take: no style point the configured ruff rules leave out (line length, quotes, import order), and no suggestion that fails CI, such as one that adds a line to a file at its growth baseline. +- CI fails a comment block over two lines. A shorter comment that tells the + backstory (run IDs, measurements, alternatives tried) instead of the reason + is a nit: suggest the short form with an issue or PR link. - A bug in code the PR did not change: mention it once as pre-existing. It does not block the PR unless the change makes it worse. - Do not repeat a finding already raised on the PR, unless its fix did not diff --git a/dokimi/CONTRIBUTING.md b/dokimi/CONTRIBUTING.md index 7b9717c65..775becc01 100644 --- a/dokimi/CONTRIBUTING.md +++ b/dokimi/CONTRIBUTING.md @@ -134,6 +134,11 @@ CI additionally holds a PR to: `changelog.d/` (`scriv create`) instead of editing `CHANGELOG.md`; see [changelog.d/README.md](changelog.d/README.md). A change with no user-visible effect gets the `skip-changelog` label instead; adding or removing the label re-runs the check. +- **Comment blocks of at most two lines** — `python scripts/check_comment_blocks.py + --base /main --head HEAD`. A run of three or more `#` comment lines in a + Python, shell, YAML or TOML file, a Makefile or a Dockerfile fails once the PR adds + or edits a line in it, so shorten an old block when you edit it. Keep the reason, + and link an issue or PR for the backstory. **pre-commit.** `.pre-commit-config.yaml` at the repo root runs the fast offline checks on the files a commit stages: `ruff check` (the version and diff --git a/dokimi/scripts/check_comment_blocks.py b/dokimi/scripts/check_comment_blocks.py new file mode 100755 index 000000000..001940cce --- /dev/null +++ b/dokimi/scripts/check_comment_blocks.py @@ -0,0 +1,167 @@ +#!/usr/bin/env python +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Fail a PR that adds or edits a line of a comment block longer than two lines. + +A comment states the constraint or the reason in at most two lines; the +backstory goes in the PR or an issue, linked from the comment (``AGENTS.md``). +A block is a run of consecutive full-line ``#`` comments in a Python, shell, +YAML or TOML file, a Makefile or a Dockerfile. Python comments come from the +tokenizer, so a ``#`` line inside a string is not one. A block of three or more +lines fails when the PR adds or changes any of its lines, so an existing long +block passes until a PR edits it. Not counted, and ending a block: a shebang on +line 1, the SPDX header lines, and tool pragmas (``# noqa``, ``# type:``, +``# pragma:``, ``# fmt:`` and the like). A ruler line such as ``# -----`` stays in +its block but does not count toward its length, so a banner is one line. Files +under ``EXCLUDE`` are third-party copies. + + python scripts/check_comment_blocks.py --base origin/main --head HEAD + +Exits 1 after an ``::error file=...`` line per block, 2 when git cannot diff +``--base``/``--head``. +""" + +from __future__ import annotations + +import argparse +import io +import re +import subprocess +import sys +import tokenize +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] +MAX_LINES = 2 +SUFFIXES = {".py", ".sh", ".yml", ".yaml", ".toml"} +NAMES = {"Makefile", "Dockerfile"} +# Copied unchanged from other projects, as ruff's extend-exclude says. +EXCLUDE = ("dokimi/tests/fixtures/task_formats/",) +_LICENSE = re.compile(r"#\s*(Copyright \(C\)|SPDX-License-Identifier:)") +_PRAGMA = re.compile( + r"#\s*(noqa\b|type:|pragma:|fmt:|ruff:|mypy:|pylint:|nosec\b|isort:|shellcheck\s|yamllint\s|-\*-)") +_RULER = re.compile(r"#+\s*([-=*#~_+])\1{2,}\s*$") +_HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@", re.MULTILINE) + + +def _git(*args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run(["git", *args], cwd=REPO, capture_output=True, text=True, + encoding="utf-8", errors="replace", check=False) + + +def checked(path: str) -> bool: + """Whether the check reads ``path`` (repository-relative, ``/``-separated).""" + name = path.rsplit("/", 1)[-1] + return (Path(name).suffix in SUFFIXES or name in NAMES) and not path.startswith(EXCLUDE) + + +def _python_comments(text: str) -> set[int] | None: + """Lines whose first token is a comment; None when ``text`` does not tokenize.""" + found = set() + try: + for tok in tokenize.generate_tokens(io.StringIO(text).readline): + if tok.type == tokenize.COMMENT and not tok.line[:tok.start[1]].strip(): + found.add(tok.start[0]) + except (tokenize.TokenError, SyntaxError): + return None + return found + + +def _exempt(number: int, stripped: str) -> bool: + return ((number == 1 and stripped.startswith("#!")) + or bool(_LICENSE.match(stripped) or _PRAGMA.match(stripped))) + + +def comment_lines(text: str, python: bool) -> set[int]: + """1-based numbers of the full-line comments in ``text`` that count toward a block.""" + # Not splitlines(): it also splits on form feeds, so line numbers would drift from git's. + lines = text.split("\n") + found = _python_comments(text) if python else None + if found is None: + found = {i for i, line in enumerate(lines, 1) if line.lstrip().startswith("#")} + return {i for i in found if not _exempt(i, lines[i - 1].strip())} + + +def blocks(numbers: set[int]) -> list[tuple[int, int]]: + """(first, last) line of each run of consecutive numbers.""" + runs: list[tuple[int, int]] = [] + for n in sorted(numbers): + if runs and n == runs[-1][1] + 1: + runs[-1] = (runs[-1][0], n) + else: + runs.append((n, n)) + return runs + + +def long_blocks(text: str, path: str, changed: set[int]) -> list[tuple[int, int, int]]: + """(first, last, length) of the blocks over ``MAX_LINES`` that hold a line of ``changed``.""" + lines, found = text.split("\n"), [] + for first, last in blocks(comment_lines(text, path.endswith(".py"))): + size = sum(not _RULER.match(lines[n - 1].strip()) for n in range(first, last + 1)) + if size > MAX_LINES and any(first <= n <= last for n in changed): + found.append((first, last, size)) + return found + + +def changed_paths(base: str, head: str) -> list[tuple[str, ...]]: + """(path at head, path at base if renamed) of each file ``base...head`` adds or changes.""" + result = _git("diff", "-z", "-M", "--name-status", "--diff-filter=AMR", f"{base}...{head}") + result.check_returncode() + fields, paths, i = result.stdout.split("\0")[:-1], [], 0 + while i < len(fields): + if fields[i].startswith("R"): + paths.append((fields[i + 2], fields[i + 1])) + i += 3 + else: + paths.append((fields[i + 1],)) + i += 2 + return paths + + +def added_lines(base: str, head: str, paths: tuple[str, ...]) -> set[int]: + """Line numbers at ``head`` that ``base...head`` adds or changes in ``paths[0]``.""" + result = _git("diff", "-U0", "-M", f"{base}...{head}", "--", *paths) + result.check_returncode() + lines: set[int] = set() + for m in _HUNK.finditer(result.stdout): + start = int(m.group(1)) + lines.update(range(start, start + int(m.group(2) or 1))) + return lines + + +def findings(base: str, head: str) -> list[tuple[str, int, int, int]]: + """(path, first, last, length) of every long block the PR adds to or edits.""" + found = [] + for paths in changed_paths(base, head): + if not checked(paths[0]): + continue + shown = _git("show", f"{head}:{paths[0]}") + shown.check_returncode() + changed = added_lines(base, head, paths) + found += [(paths[0], *block) for block in long_blocks(shown.stdout, paths[0], changed)] + return found + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--base", default="origin/main") + parser.add_argument("--head", default="HEAD") + args = parser.parse_args(argv) + try: + found = findings(args.base, args.head) + except subprocess.CalledProcessError as exc: + print(f"cannot diff {args.base}...{args.head}: {exc.stderr.strip()}", file=sys.stderr) + return 2 + for path, first, last, size in found: + print(f"::error file={path},line={first},endLine={last},title=Comment block::" + f"{size} comment lines; keep it to {MAX_LINES} that say the reason, " + "and link an issue or PR for the rest (AGENTS.md)") + if found: + return 1 + print(f"No comment block over {MAX_LINES} lines among the lines this PR adds or changes.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/dokimi/tests/test_check_comment_blocks.py b/dokimi/tests/test_check_comment_blocks.py new file mode 100644 index 000000000..2191bbcd9 --- /dev/null +++ b/dokimi/tests/test_check_comment_blocks.py @@ -0,0 +1,131 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for scripts/check_comment_blocks.py — the comment block length gate.""" + +from __future__ import annotations + +import importlib.util +import subprocess +from pathlib import Path + +import pytest + +_SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "check_comment_blocks.py" +_spec = importlib.util.spec_from_file_location("check_comment_blocks", _SCRIPT) +assert _spec and _spec.loader +ccb = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(ccb) + +LONG = "# one\n# two\n# three\nx = 1\n" + + +@pytest.mark.parametrize(("path", "read"), [ + ("dokimi/dokimi/cli.py", True), + ("dokimi/Makefile", True), + (".agent-eval/Dockerfile", True), + (".github/workflows/ci.yml", True), + (".pre-commit-config.yaml", True), + ("dokimi/pyproject.toml", True), + ("dokimi/docs/README.md", False), # `#` is a heading there + ("dokimi/requirements.lock.txt", False), # generated + ("dokimi/tests/fixtures/task_formats/x/solve.sh", False), # third-party copy +]) +def test_which_files_are_read(path, read): + assert ccb.checked(path) is read + + +def test_a_hash_inside_a_python_string_is_not_a_comment(): + text = 'x = """\n# a\n# b\n# c\n"""\n' + assert ccb.comment_lines(text, python=True) == set() + + +def test_a_trailing_comment_does_not_join_a_block(): + text = "# a\n# b\nx = 1 # c\n" + assert ccb.blocks(ccb.comment_lines(text, python=True)) == [(1, 2)] + + +def test_shebang_license_and_pragmas_do_not_count(): + text = ("#!/usr/bin/env python\n# Copyright (C) 2026 Intel Corporation\n" + "# SPDX-License-Identifier: Apache-2.0\n# a\n# b\n# fmt: off\n# c\n") + assert ccb.blocks(ccb.comment_lines(text, python=True)) == [(4, 5), (7, 7)] + + +def test_python_that_does_not_tokenize_falls_back_to_lines(): + text = "# a\n# b\n# c\nx = '\n" + assert ccb.comment_lines(text, python=True) == {1, 2, 3} + + +def test_yaml_comments_are_read_by_line_including_in_run_scripts(): + text = "steps:\n - run: |\n # a\n # b\n # c\n make\n" + assert ccb.blocks(ccb.comment_lines(text, python=False)) == [(3, 5)] + + +def test_a_form_feed_keeps_line_numbers_in_step_with_git(): + text = "a: 1\f\n# x\n# y\n# z\n" + assert ccb.long_blocks(text, "f.yml", {2}) == [(2, 4, 3)] + + +def test_a_banner_is_one_line_but_rulers_do_not_hide_a_long_block(): + banner = "# ------\n# Section\n# ------\nx = 1\n" + assert ccb.long_blocks(banner, "f.py", {2}) == [] + ruled = "# ======\n# a\n# b\n# c\n# ======\n" + assert ccb.long_blocks(ruled, "f.py", {3}) == [(1, 5, 3)] + + +@pytest.mark.parametrize(("changed", "flagged"), [ + ({2}, [(1, 3, 3)]), # an edited line inside the block + ({4}, []), # only the code below it changed + (set(), []), # an existing block nobody touched +]) +def test_a_long_block_fails_only_when_the_pr_touches_it(changed, flagged): + assert ccb.long_blocks(LONG, "f.py", changed) == flagged + + +def test_two_lines_pass_even_when_both_are_new(): + assert ccb.long_blocks("# one\n# two\nx = 1\n", "f.py", {1, 2}) == [] + + +def _git_repo(path: Path) -> None: + for cmd in (["init", "-q"], ["config", "user.email", "t@example.com"], + ["config", "user.name", "t"], ["config", "commit.gpgsign", "false"]): + subprocess.run(["git", *cmd], cwd=path, check=True) + + +def _commit(path: Path, msg: str, tag: str | None = None) -> None: + subprocess.run(["git", "add", "-A"], cwd=path, check=True) + subprocess.run(["git", "commit", "-q", "-m", msg], cwd=path, check=True) + if tag: + subprocess.run(["git", "tag", tag], cwd=path, check=True) + + +def test_main_flags_a_new_long_block_with_its_lines(tmp_path, monkeypatch, capsys): + _git_repo(tmp_path) + (tmp_path / "a.py").write_text("x = 1\n", encoding="utf-8") + _commit(tmp_path, "base", tag="base") + (tmp_path / "a.py").write_text("x = 1\n" + LONG, encoding="utf-8") + _commit(tmp_path, "pr") + monkeypatch.setattr(ccb, "REPO", tmp_path) + assert ccb.main(["--base", "base", "--head", "HEAD"]) == 1 + assert "::error file=a.py,line=2,endLine=4," in capsys.readouterr().out + + +def test_main_passes_an_untouched_long_block_and_a_renamed_file(tmp_path, monkeypatch): + _git_repo(tmp_path) + (tmp_path / "old.py").write_text(LONG + "y = 2\n" * 10, encoding="utf-8") + (tmp_path / "b.yml").write_text(LONG, encoding="utf-8") + _commit(tmp_path, "base", tag="base") + (tmp_path / "old.py").rename(tmp_path / "new.py") + (tmp_path / "b.yml").write_text(LONG + "z: 3\n", encoding="utf-8") + _commit(tmp_path, "move and append") + monkeypatch.setattr(ccb, "REPO", tmp_path) + assert ccb.findings("base", "HEAD") == [] + + +def test_main_exits_2_for_a_base_git_cannot_diff(tmp_path, monkeypatch, capsys): + _git_repo(tmp_path) + (tmp_path / "f.py").write_text("x = 1\n", encoding="utf-8") + _commit(tmp_path, "base") + monkeypatch.setattr(ccb, "REPO", tmp_path) + assert ccb.main(["--base", "no-such-ref", "--head", "HEAD"]) == 2 + assert "no-such-ref" in capsys.readouterr().err