Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .github/workflows/pr-require-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 }}
Expand Down
160 changes: 160 additions & 0 deletions scripts/check-line-citations.py
Original file line number Diff line number Diff line change
@@ -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./@:~-])"
r"(?P<path>[\w.@-]+(?:/[\w.@-]+)*)"
r":(?P<spec>\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())
16 changes: 16 additions & 0 deletions scripts/check-line-citations.test.sh
Original file line number Diff line number Diff line change
@@ -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 <abs path>`, which resolves the path relative to the caller's cwd.
"$PYTHON" "$SCRIPT_DIR/test_check_line_citations.py" -v
144 changes: 144 additions & 0 deletions scripts/test_check_line_citations.py
Original file line number Diff line number Diff line change
@@ -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()
Loading