diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index f1e187de55..ea76363c50 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -643,6 +643,16 @@ jobs: continue-on-error: true run: python3 scripts/check-code-metrics-skill-prose.py + # A `path:N` citation in markdown goes stale when the CITED file shrinks, + # usually in a change that never touches the citing file, so the scan is + # whole-repository and ungated. It fails only the mechanical case, a line + # past the end of the file; whether the line still says what the citation + # claims is left to review. + - name: Check markdown line citations stay inside the cited file + id: line_citations + continue-on-error: true + run: python3 scripts/check-line-citations.py + # The review-disposition-and-resolution contract is canonical in ONE file # and restated across half a dozen others; nothing checked that the # copies agreed. #1633 shipped seven such defects across five review @@ -1072,6 +1082,7 @@ jobs: config-cascade-semantics=${{ steps.config_cascade_semantics.outcome }} code-metrics-config-reference=${{ steps.code_metrics_config_reference.outcome }} code-metrics-skill-prose=${{ steps.code_metrics_skill_prose.outcome }} + line-citations=${{ steps.line_citations.outcome }} contract-clause-coverage=${{ steps.contract_clause_coverage.outcome }} stale-base-overlap=${{ steps.stale_base.outcome }} docs-only-contract=${{ steps.docs_only_contract.outcome }} diff --git a/scripts/check-line-citations.py b/scripts/check-line-citations.py new file mode 100755 index 0000000000..2599459e21 --- /dev/null +++ b/scripts/check-line-citations.py @@ -0,0 +1,160 @@ +#!/usr/bin/env python3 +"""Fail on a markdown `path:N` or `path:N-M` citation past the end of the cited file. + +A citation such as `docs/foo.md:269-271` goes stale when the cited file +shrinks. Review bots flag the mechanical case, a line number past the end of +the file, again and again; this gate catches it before review does. + +WHAT FIRES. In a tracked markdown file, a token `PATH:SPEC` where SPEC is one or +more comma-separated `N` or `N-M` line numbers, PATH resolves to a tracked file +(relative to the repository root or to the citing file's directory), and some +cited number exceeds that file's line count. When PATH resolves against both +bases, the citation fires only when it is past the end of every candidate. + +WHAT STAYS QUIET, by construction: + + * a PATH that resolves to no tracked file: a missing file is another check's + job, and a bare basename (`ci.yml:2639`), a time (`10:30`) or a host and + port (`localhost:8080`) resolves to nothing; + * a URL (`https://host:8080/x.md:12`) and an external-repository citation + (`owner/repo:path.md:12`), because a token preceded by `:` or `/` is part + of something larger and is never matched from its middle; + * a citation inside a fenced code block: fences hold sample output and + illustrative rows (`path/to/file.cs:42`), not claims about this tree; + * a point-in-time record (an ADR, a `docs/upstream/` decision record, a + CHANGELOG or a `.changes/` fragment): it cites the tree as it was when + written, and rewriting its line numbers later would falsify the record; + * whether the cited line still says what the citation claims: semantic drift + is not mechanical, and is left to review. + +The scan covers every tracked markdown file, not only a pull request's changed +files, because the usual way a citation goes stale is a change to the CITED +file that never touches the citing one. + +Usage: + + scripts/check-line-citations.py + +Exit codes: 0 clean, 1 at least one citation past the end, 2 could not run. +""" + +from __future__ import annotations + +import re +import subprocess +import sys +from pathlib import Path + +# `from __future__ import annotations` needs 3.7; the .test.sh wrapper parses +# this tuple to pick an interpreter. +MIN_PYTHON = (3, 7) + +REPO_ROOT = Path(__file__).resolve().parent.parent + +# PATH:SPEC. The lookbehind keeps a token that continues a URL, an +# `owner/repo:` prefix or a longer word from matching from its middle; the +# lookahead stops `file.md:12.5`, `file.md:12:3` and `file.md:12abc` from +# reading as line 12. +CITATION = re.compile( + r"(?[\w.@-]+(?:/[\w.@-]+)*)" + r":(?P\d+(?:-\d+)?(?:,\d+(?:-\d+)?)*)" + r"(?![\w.:/-]*\w)" +) + +# A fence may sit inside a block quote (`> ````) or open a list item (`- ````). +FENCE = re.compile(r"^(?:\s*>)*\s*(?:(?:[-*+]|\d+[.)])\s+)?(`{3,}|~{3,})") + +# Point-in-time records, by repository-relative prefix or by basename. +RECORD_PREFIXES = ("docs/adr/", "docs/upstream/", ".changes/") +RECORD_NAMES = frozenset({"CHANGELOG.md"}) + + +def is_record(path: str) -> bool: + return path.startswith(RECORD_PREFIXES) or Path(path).name in RECORD_NAMES + + +def line_count(path: Path, cache: dict) -> int: + if path not in cache: + data = path.read_bytes() + cache[path] = data.count(b"\n") + (0 if not data or data.endswith(b"\n") else 1) + return cache[path] + + +def candidates(cited: str, citing: Path, tracked: set) -> list: + out = [] + for base in (REPO_ROOT, (REPO_ROOT / citing).parent): + target = (base / cited).resolve() + try: + rel = target.relative_to(REPO_ROOT).as_posix() + except ValueError: + continue + if rel in tracked and target not in out: + out.append(target) + return out + + +def scan(citing: Path, tracked: set, cache: dict) -> list: + findings = [] + fence = "" + text = (REPO_ROOT / citing).read_text(encoding="utf-8", errors="replace") + for lineno, line in enumerate(text.splitlines(), 1): + opener = FENCE.match(line) + if opener: + run = opener[1] + if not fence: + fence = run + elif ( + run[0] == fence[0] + and len(run) >= len(fence) + and not line[opener.end() :].strip() + ): + fence = "" + continue + if fence: + continue + for match in CITATION.finditer(line): + targets = candidates(match["path"], citing, tracked) + if not targets: + continue + highest = max(int(n) for n in re.split(r"[-,]", match["spec"])) + best = max(targets, key=lambda t: line_count(t, cache)) + longest = line_count(best, cache) + if highest > longest: + findings.append( + f"{citing.as_posix()}:{lineno}: `{match[0]}` cites line {highest}, " + f"but {best.relative_to(REPO_ROOT).as_posix()} has {longest} lines" + ) + return findings + + +def main() -> int: + try: + listed = subprocess.run( + ["git", "-C", str(REPO_ROOT), "ls-files", "-z"], + check=True, + capture_output=True, + ).stdout.decode("utf-8") + except (OSError, subprocess.CalledProcessError) as exc: + print( + f"check-line-citations: cannot list tracked files: {exc}", file=sys.stderr + ) + return 2 + tracked = {p for p in listed.split("\0") if p and (REPO_ROOT / p).is_file()} + files = sorted(Path(p) for p in tracked if p.endswith(".md") and not is_record(p)) + cache: dict = {} + findings = [f for path in files for f in scan(path, tracked, cache)] + for finding in findings: + print(finding) + if findings: + print( + f"check-line-citations: {len(findings)} citation(s) past the end of the cited file; " + "update the line numbers or drop them.", + file=sys.stderr, + ) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/check-line-citations.test.sh b/scripts/check-line-citations.test.sh new file mode 100755 index 0000000000..d9e4c07e7e --- /dev/null +++ b/scripts/check-line-citations.test.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash +# Cross-platform contract wrapper for the line-citation gate's test suite. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# The Python floor has one origin: MIN_PYTHON in the gate itself. +# shellcheck source=lib/python-probe.sh +. "$SCRIPT_DIR/lib/python-probe.sh" + +PYTHON="" +python_probe::require_to PYTHON "$SCRIPT_DIR/check-line-citations.py" + +# Execute the test file directly (its unittest.main() guard) rather than via +# `-m unittest `, which resolves the path relative to the caller's cwd. +"$PYTHON" "$SCRIPT_DIR/test_check_line_citations.py" -v diff --git a/scripts/test_check_line_citations.py b/scripts/test_check_line_citations.py new file mode 100755 index 0000000000..836b9662cc --- /dev/null +++ b/scripts/test_check_line_citations.py @@ -0,0 +1,144 @@ +#!/usr/bin/env python3 +"""Self-test for the line-citation gate. + +Each case builds a throwaway git repository holding a copy of the gate, a +ten-line cited file and one citing markdown file, so the suite proves the +detector rather than the current state of this checkout. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +# FIXTURE ISOLATION: `git init` and `git add` below must never reach the +# caller's repository through an inherited git environment. The variable list +# mirrors scripts/test-git-helpers.sh; scripts/check-fixture-git-isolation.sh +# keeps it true. +for _leaked_git_var in ( + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_COMMON_DIR", + "GIT_PREFIX", + "GIT_OBJECT_DIRECTORY", + "GIT_CONFIG", +): + os.environ.pop(_leaked_git_var, None) +del _leaked_git_var + +GATE = Path(__file__).resolve().parent / "check-line-citations.py" +TEN_LINES = "".join(f"line {n}\n" for n in range(1, 11)) + + +def run_gate(citing: str, citing_path: str = "docs/guide.md", extra=None): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / "scripts").mkdir() + shutil.copy(GATE, root / "scripts" / GATE.name) + (root / "src").mkdir() + (root / "src" / "ten.py").write_text(TEN_LINES) + for rel, body in (extra or {}).items(): + (root / rel).parent.mkdir(parents=True, exist_ok=True) + (root / rel).write_text(body) + doc = root / citing_path + doc.parent.mkdir(parents=True, exist_ok=True) + doc.write_text(citing) + subprocess.run(["git", "init", "-q"], cwd=root, check=True) + subprocess.run(["git", "add", "-A"], cwd=root, check=True) + result = subprocess.run( + [sys.executable, str(root / "scripts" / GATE.name)], + cwd=root, + capture_output=True, + text=True, + ) + return result.returncode, result.stdout + + +class Fires(unittest.TestCase): + def assert_fires(self, citing: str, citing_path: str = "docs/guide.md"): + code, out = run_gate(citing, citing_path) + self.assertEqual(code, 1, out) + self.assertIn("has 10 lines", out) + + def test_line_past_end(self): + self.assert_fires("See `src/ten.py:11` for the loop.\n") + + def test_range_end_past_end(self): + self.assert_fires("See src/ten.py:8-12 for the loop.\n") + + def test_later_item_of_a_list_past_end(self): + self.assert_fires("See `src/ten.py:2-3,9-40`.\n") + + def test_path_relative_to_citing_file(self): + self.assert_fires("See `../ten.py:99`.\n", "src/docs/guide.md") + + def test_past_end_of_every_candidate_names_the_longest(self): + code, out = run_gate( + "See `ten.py:11`.\n", extra={"ten.py": "a\nb\n", "docs/ten.py": TEN_LINES} + ) + self.assertEqual(code, 1, out) + self.assertIn("but docs/ten.py has 10 lines", out) + + def test_sentence_end_punctuation(self): + self.assert_fires("The loop is at src/ten.py:11.\n") + + +class StaysQuiet(unittest.TestCase): + def assert_quiet(self, citing: str, citing_path: str = "docs/guide.md"): + code, out = run_gate(citing, citing_path) + self.assertEqual(code, 0, out) + + def test_valid_line_and_range(self): + self.assert_quiet("See `src/ten.py:10` and src/ten.py:1-10.\n") + + def test_nonexistent_path(self): + self.assert_quiet("See `src/gone.py:500` and `ten.py:500`.\n") + + def test_url_with_port(self): + self.assert_quiet( + "Open http://localhost:8080/src/ten.py:99 or localhost:8080.\n" + ) + + def test_external_repo_citation(self): + self.assert_quiet("See melodic-software/standards:src/ten.py:99.\n") + + def test_time_of_day(self): + self.assert_quiet("The run starts at 10:30 and ends at 11:45-12:00.\n") + + def test_line_and_column(self): + self.assert_quiet("Reported at src/ten.py:99:4 by the linter.\n") + + def test_fenced_example(self): + self.assert_quiet("```text\n| 1 | src/ten.py:42 | sample row |\n```\n") + + def test_fence_in_block_quote_or_list_item(self): + self.assert_quiet("> ```text\n> src/ten.py:42\n> ```\n") + self.assert_quiet("- ```text\n src/ten.py:42\n ```\n") + + def test_point_in_time_record(self): + self.assert_quiet("Cut `src/ten.py:13-30`.\n", "docs/adr/0001-cut.md") + self.assert_quiet("- Fixed `src/ten.py:99`.\n", "plugins/x/CHANGELOG.md") + self.assert_quiet("Cut `src/ten.py:99`.\n", "docs/upstream/x.md") + self.assert_quiet("- Fixed `src/ten.py:99`.\n", ".changes/testing/x.md") + + def test_line_inside_the_longer_of_two_candidates(self): + # `ten.py` resolves at the root (2 lines) and beside the citing file (10). + code, out = run_gate( + "See `ten.py:9`.\n", extra={"ten.py": "a\nb\n", "docs/ten.py": TEN_LINES} + ) + self.assertEqual(code, 0, out) + + def test_citation_after_a_closed_fence_still_fires(self): + code, out = run_gate("```\nsrc/ten.py:99\n```\n\nSee `src/ten.py:99`.\n") + self.assertEqual(code, 1, out) + self.assertIn("docs/guide.md:5:", out) + + +if __name__ == "__main__": + unittest.main()