diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 245c00e84..a4f130e5a 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -15,6 +15,10 @@ +## Open questions + + + ## Revert @@ -25,5 +29,6 @@ - [ ] `ruff`, `mypy`, and schema checks pass - [ ] Docs updated (or N/A) — see the docs-drift checklist in CONTRIBUTING.md - [ ] Self-reviewed per "Before opening a PR" in `AGENTS.md` +- [ ] Each new `raise`/exit and test names the doc, contract or issue it traces to - [ ] No ad-hoc run artifacts committed - [ ] New files carry the SPDX license header (or are exempt in `.github/.licenserc.yaml`) diff --git a/AGENTS.md b/AGENTS.md index 93991e3a3..194f25f18 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -149,6 +149,14 @@ finds little: - Where it applies, run the changed command on a bad input: missing file, empty file, wrong shape, `None` where a value is expected, a Windows path. Say in the description what you could not run. +- Trace new error paths and tests to the contract: a new `raise`, `sys.exit` + or refusal names, in its message, docstring or the PR, the doc, contract or + issue that asks for it; a test of new behaviour cites the doc line it checks. + `python scripts/list_new_error_paths.py --base origin/main` (from `dokimi/`) lists the sites + of a `raise`, a `sys.exit` or `SystemExit` call, or a literal `exit_code=N`; + refusals through `parser.error` or `return N` are not listed. +- An unclear requirement goes in the PR description's "Open questions", not + into code that guesses: no error path or test the spec cannot be quoted for. - Check that the PR title, description, docstrings and changelog fragment say what the code does (exit codes especially). - Re-review your own fix commits during review with the same care. diff --git a/REVIEW.md b/REVIEW.md index 8eb5aed62..484ff3c37 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -23,6 +23,13 @@ checks. - 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. +- A new `raise`, `sys.exit` or refusal that no doc, contract or issue asks + for, or a test of new behaviour that cites no doc line: ask which line it + traces to. Code built on a guess at an unclear requirement is a defect too; + the question belongs in the PR's "Open questions". `python + dokimi/scripts/list_new_error_paths.py --base origin/main` lists the sites of + a `raise`, a `sys.exit` or `SystemExit` call, or a literal `exit_code=N`; + refusals through `parser.error` or `return N` are not listed. ## Patterns that hide a defect diff --git a/dokimi/changelog.d/20261113_120000_napetrov_contract-trace.md b/dokimi/changelog.d/20261113_120000_napetrov_contract-trace.md new file mode 100644 index 000000000..aa8324ac8 --- /dev/null +++ b/dokimi/changelog.d/20261113_120000_napetrov_contract-trace.md @@ -0,0 +1,7 @@ +### Added + +- **New error paths and tests trace to the contract** — `REVIEW.md` and `AGENTS.md` ask that a new `raise`, + `sys.exit` or refusal names the doc, contract or issue behind it, that a test of new behaviour cites the doc + line it checks, and that an unclear requirement goes in the PR template's new "Open questions" section. + `python scripts/list_new_error_paths.py --base origin/main` lists the added sites of a `raise`, a `sys.exit` or `SystemExit` call, or a literal `exit_code=N` for the reviewer; refusals through `parser.error` or `return N` are not listed. It always + exits 0 (2 when git cannot diff). diff --git a/dokimi/scripts/list_new_error_paths.py b/dokimi/scripts/list_new_error_paths.py new file mode 100644 index 000000000..fa823cceb --- /dev/null +++ b/dokimi/scripts/list_new_error_paths.py @@ -0,0 +1,108 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""List the error paths and tests a PR adds, for the reviewer to trace to the contract. + +``REVIEW.md`` asks that each new ``raise``, ``sys.exit``/``SystemExit`` or ``exit_code=`` +names the doc, contract or issue that asks for it, and that each new test cites the doc +line it checks. This script finds those added lines in the ``--base...--head`` diff of +``*.py`` files and prints one ``path:line: kind: text`` row each; it judges nothing. + + python scripts/list_new_error_paths.py --base origin/main --head HEAD + +Exits 0 after the listing (also when it is empty), 2 when git cannot diff. +""" + +from __future__ import annotations + +import argparse +import re +import subprocess +import sys +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] +_HUNK = re.compile(r"^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") +_KINDS = ( + ("raise", re.compile(r"(?:^|[:;])\s*raise\b")), + ("exit", re.compile(r"\bsys\.exit\s*\(|\bSystemExit\s*\(|\bexit_code\s*=\s*[1-9]")), + ("test", re.compile(r"^\s*(?:async\s+)?def\s+test_\w*\s*\(")), +) + + +_ESCAPES = {"a": "\a", "b": "\b", "t": "\t", "n": "\n", "v": "\v", "f": "\f", "r": "\r", '"': '"', "\\": "\\"} + + +def _unquote(quoted: str) -> str: + """Decode a path git C-quotes, ``"b/a\\tb.py"``: escapes and ``\\ooo`` octal bytes.""" + raw, out, i = quoted[1:-1], bytearray(), 0 + while i < len(raw): + if raw[i] != "\\" or i + 1 >= len(raw): + out += raw[i].encode("utf-8") + i += 1 + elif len(raw[i + 1 : i + 4]) == 3 and set(raw[i + 1 : i + 4]) <= set("01234567"): + out.append(int(raw[i + 1 : i + 4], 8)) + i += 4 + else: + out += _ESCAPES.get(raw[i + 1], raw[i + 1]).encode("utf-8") + i += 2 + return out.decode("utf-8", errors="replace") + + +def added_sites(diff: str) -> list[tuple[str, int, str, str]]: + """``(path, line, kind, text)`` for each added line of a ``-U0`` diff that matches a kind.""" + sites: list[tuple[str, int, str, str]] = [] + path, line, old_left, new_left, prev = "", 0, 0, 0, "" + for row in diff.splitlines(): + in_hunk = old_left > 0 or new_left > 0 + # Inside a hunk every row is content, even `+++ x` or `--- x`; headers come between hunks. + if not in_hunk and row.startswith("+++ ") and prev.startswith("--- "): + target = row[4:].removesuffix("\t") + if len(target) >= 2 and target[0] == target[-1] == '"': + target = _unquote(target) + path = target[2:] if target.startswith("b/") else "" + elif not in_hunk and (m := _HUNK.match(row)): + old_left = 1 if m.group(1) is None else int(m.group(1)) + line = int(m.group(2)) + new_left = 1 if m.group(3) is None else int(m.group(3)) + elif in_hunk and row.startswith("+"): + new_left -= 1 + text = row[1:] + kind = next((k for k, rx in _KINDS if rx.search(text)), None) + if kind and path.endswith(".py"): + sites.append((path, line, kind, text.strip())) + line += 1 + elif in_hunk and row.startswith("-"): + old_left -= 1 + elif in_hunk and row.startswith(" "): + old_left -= 1 + new_left -= 1 + line += 1 + prev = row + return sites + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=(__doc__ or "").splitlines()[0]) + parser.add_argument("--base", default="origin/main") + parser.add_argument("--head", default="HEAD") + args = parser.parse_args(argv) + result = subprocess.run( + ["git", "-c", "core.quotePath=false", "diff", "-U0", "--no-color", + "--src-prefix=a/", "--dst-prefix=b/", "--inter-hunk-context=0", + "--output-indicator-new=+", "--output-indicator-old=-", "--output-indicator-context= ", + "--no-ext-diff", "--no-textconv", + f"{args.base}...{args.head}", "--", "*.py"], + cwd=REPO, capture_output=True, text=True, encoding="utf-8", errors="replace", check=False) + if result.returncode: + print(result.stderr.strip(), file=sys.stderr) + return 2 + sites = added_sites(result.stdout) + for path, line, kind, text in sites: + print(f"{path}:{line}: {kind}: {text}") + print(f"{len(sites)} new error path(s) and test(s); each should name its doc, contract or issue.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/dokimi/tests/test_list_new_error_paths.py b/dokimi/tests/test_list_new_error_paths.py new file mode 100644 index 000000000..7b8d83460 --- /dev/null +++ b/dokimi/tests/test_list_new_error_paths.py @@ -0,0 +1,172 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for scripts/list_new_error_paths.py — the reviewer's list of new error paths.""" + +from __future__ import annotations + +import importlib.util +import subprocess + +import pytest +from pathlib import Path + +_SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "list_new_error_paths.py" +_spec = importlib.util.spec_from_file_location("list_new_error_paths", _SCRIPT) +assert _spec and _spec.loader +lnep = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(lnep) + +DIFF = """\ +diff --git a/dokimi/dokimi/x.py b/dokimi/dokimi/x.py +--- a/dokimi/dokimi/x.py ++++ b/dokimi/dokimi/x.py +@@ -10,0 +11,4 @@ def f(): ++ if bad: ++ raise ValueError("bad") ++ sys.exit(2) ++ return ExitStatus(exit_code=1) +@@ -40 +44,2 @@ +- raise OldError ++ message = "raise is a word here" ++def test_bad_input_fails(): +diff --git a/dokimi/docs/x.md b/dokimi/docs/x.md +--- a/dokimi/docs/x.md ++++ b/dokimi/docs/x.md +@@ -1,0 +2 @@ ++raise ValueError in prose +""" + + +def test_lists_added_raise_exit_and_test_lines_with_their_new_line_numbers(): + assert lnep.added_sites(DIFF) == [ + ("dokimi/dokimi/x.py", 12, "raise", 'raise ValueError("bad")'), + ("dokimi/dokimi/x.py", 13, "exit", "sys.exit(2)"), + ("dokimi/dokimi/x.py", 14, "exit", "return ExitStatus(exit_code=1)"), + ("dokimi/dokimi/x.py", 45, "test", "def test_bad_input_fails():"), + ] + + +def test_a_deleted_file_lists_nothing(): + diff = "--- a/dokimi/dokimi/x.py\n+++ /dev/null\n@@ -1 +0,0 @@\n-raise X\n" + assert lnep.added_sites(diff) == [] + + +def test_main_lists_the_sites_of_a_real_diff(monkeypatch, capsys): + result = lnep.subprocess.CompletedProcess(args=[], returncode=0, stdout=DIFF, stderr="") + monkeypatch.setattr(lnep.subprocess, "run", lambda *args, **kwargs: result) + assert lnep.main(["--base", "HEAD~1", "--head", "HEAD"]) == 0 + out = capsys.readouterr().out + assert "4 new error path(s)" in out + assert 'dokimi/dokimi/x.py:12: raise: raise ValueError("bad")' in out + assert "dokimi/dokimi/x.py:45: test: def test_bad_input_fails():" in out + + +def test_main_returns_2_when_git_cannot_diff(capsys): + assert lnep.main(["--base", "no-such-ref-xyz", "--head", "HEAD"]) == 2 + + +def test_an_added_line_starting_with_two_pluses_is_not_a_file_header(): + diff = "--- a/x.py\n+++ b/x.py\n@@ -0,0 +1,2 @@\n+++ y\n+if bad: raise Z\n" + assert lnep.added_sites(diff) == [("x.py", 2, "raise", "if bad: raise Z")] + + +def test_a_removed_and_added_line_that_look_like_file_headers_do_not_hide_later_sites(): + diff = "--- a/x.py\n+++ b/x.py\n@@ -3 +3 @@\n--- old\n+++ new\n@@ -9,0 +10 @@\n+ raise Z\n" + assert lnep.added_sites(diff) == [("x.py", 10, "raise", "raise Z")] + + +def test_a_path_with_spaces_keeps_its_sites(): + diff = "--- a/my x.py\t\n+++ b/my x.py\t\n@@ -0,0 +1 @@\n+raise Z\n" + assert lnep.added_sites(diff) == [("my x.py", 1, "raise", "raise Z")] + + +def test_a_raise_after_a_semicolon_on_the_same_line_is_listed(): + diff = "--- a/x.py\n+++ b/x.py\n@@ -0,0 +1 @@\n+cleanup(); raise ValueError('bad')\n" + assert lnep.added_sites(diff) == [("x.py", 1, "raise", "cleanup(); raise ValueError('bad')")] + + +def test_a_git_quoted_path_keeps_its_sites(): + diff = '--- "a/a\\tb.py"\n+++ "b/a\\tb.py"\n@@ -0,0 +1 @@\n+raise Z\n' + assert lnep.added_sites(diff) == [("a\tb.py", 1, "raise", "raise Z")] + + +def test_a_git_quoted_path_with_a_quote_and_a_backslash_is_decoded(): + diff = '--- "a/q\\"u\\\\x.py"\n+++ "b/q\\"u\\\\x.py"\n@@ -0,0 +1 @@\n+raise Z\n' + assert lnep.added_sites(diff) == [('q"u\\x.py', 1, "raise", "raise Z")] + + +@pytest.mark.parametrize( + ("esc", "char"), + [("\\a", "\a"), ("\\b", "\b"), ("\\n", "\n"), ("\\v", "\v"), ("\\f", "\f"), ("\\r", "\r")], +) +def test_a_git_quoted_control_character_escape_is_decoded(esc, char): + diff = f'--- "a/x{esc}y.py"\n+++ "b/x{esc}y.py"\n@@ -0,0 +1 @@\n+raise Z\n' + assert lnep.added_sites(diff) == [("x" + char + "y.py", 1, "raise", "raise Z")] + + +def test_a_git_quoted_non_ascii_path_is_decoded(): + diff = '--- "a/caf\\303\\251.py"\n+++ "b/caf\\303\\251.py"\n@@ -0,0 +1 @@\n+raise Z\n' + assert lnep.added_sites(diff) == [("caf\u00e9.py", 1, "raise", "raise Z")] + + +def test_a_context_row_inside_a_fused_hunk_is_counted(): + diff = "--- a/x.py\n+++ b/x.py\n@@ -1,3 +1,3 @@\n-raise OldA\n+raise NewA\n unchanged()\n-raise OldB\n+raise NewB\n" + assert lnep.added_sites(diff) == [("x.py", 1, "raise", "raise NewA"), ("x.py", 3, "raise", "raise NewB")] + + +def test_an_exit_call_with_a_space_before_the_paren_is_listed(): + diff = "--- a/x.py\n+++ b/x.py\n@@ -0,0 +1,2 @@\n+sys.exit (2)\n+SystemExit (2)\n" + assert [s[2] for s in lnep.added_sites(diff)] == ["exit", "exit"] + + +def test_main_passes_explicit_prefixes_and_no_fused_hunks(monkeypatch): + seen = [] + + def fake_run(cmd, **kwargs): + seen.append(cmd) + return lnep.subprocess.CompletedProcess(args=cmd, returncode=0, stdout="", stderr="") + + monkeypatch.setattr(lnep.subprocess, "run", fake_run) + assert lnep.main([]) == 0 + assert {"--src-prefix=a/", "--dst-prefix=b/", "--inter-hunk-context=0"} <= set(seen[0]) + + +def test_main_pins_the_diff_output_indicators(monkeypatch): + seen = [] + + def fake_run(cmd, **kwargs): + seen.append(cmd) + return lnep.subprocess.CompletedProcess(args=cmd, returncode=0, stdout="", stderr="") + + monkeypatch.setattr(lnep.subprocess, "run", fake_run) + assert lnep.main([]) == 0 + flags = {"--output-indicator-new=+", "--output-indicator-old=-", "--output-indicator-context= "} + assert flags <= set(seen[0]) + + +def _external_diff(repo, monkeypatch): + monkeypatch.setenv("GIT_EXTERNAL_DIFF", "true") + + +def _textconv(repo, monkeypatch): + (repo / ".git" / "info").mkdir(exist_ok=True) + (repo / ".git" / "info" / "attributes").write_text("*.py diff=up\n", encoding="utf-8") + subprocess.run(["git", "config", "diff.up.textconv", "tr a-z A-Z <"], cwd=repo, check=True) + + +@pytest.mark.parametrize("configure", [_external_diff, _textconv]) +def test_main_lists_sites_whatever_the_user_diff_config(tmp_path, monkeypatch, capsys, configure): + def git(*args): + subprocess.run(["git", "-c", "user.name=t", "-c", "user.email=t@t", *args], + cwd=tmp_path, check=True, capture_output=True) + + git("init", "-q") + git("commit", "-q", "--allow-empty", "-m", "base") + (tmp_path / "x.py").write_text("raise ValueError(1)\n", encoding="utf-8") + git("add", "x.py") + git("commit", "-q", "-m", "add") + monkeypatch.setattr(lnep, "REPO", tmp_path) + configure(tmp_path, monkeypatch) + assert lnep.main(["--base", "HEAD~1", "--head", "HEAD"]) == 0 + assert "x.py:1: raise: raise ValueError(1)" in capsys.readouterr().out