From 14e9f9f1231950c2d0ad43e07955d90b95ed3c94 Mon Sep 17 00:00:00 2001 From: "Petrov, Nikolay A" Date: Fri, 13 Nov 2026 13:41:40 -0800 Subject: [PATCH 01/11] docs(review): new error paths and tests trace to the contract REVIEW.md and AGENTS.md ask that a new raise/exit names the doc, contract or issue behind it, and a test of new behaviour cites the doc line it checks; an unclear requirement goes in the PR template's new Open questions section. scripts/list_new_error_paths.py lists the added sites for the reviewer. Signed-off-by: Petrov, Nikolay A --- .github/pull_request_template.md | 5 ++ AGENTS.md | 6 ++ REVIEW.md | 5 ++ ...20261113_120000_napetrov_contract-trace.md | 7 ++ dokimi/scripts/list_new_error_paths.py | 71 +++++++++++++++++++ dokimi/tests/test_list_new_error_paths.py | 58 +++++++++++++++ 6 files changed, 152 insertions(+) create mode 100644 dokimi/changelog.d/20261113_120000_napetrov_contract-trace.md create mode 100644 dokimi/scripts/list_new_error_paths.py create mode 100644 dokimi/tests/test_list_new_error_paths.py 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 b79d98a20..e86aab606 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -146,6 +146,12 @@ 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` lists them. +- 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 76c6d1590..03aa0980b 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -23,6 +23,11 @@ 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. ## How to raise a finding 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..1af6c7daf --- /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 for the reviewer; 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..067036b25 --- /dev/null +++ b/dokimi/scripts/list_new_error_paths.py @@ -0,0 +1,71 @@ +#!/usr/bin/env python +# 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\(|\bSystemExit\(|\bexit_code\s*=\s*[1-9]")), + ("test", re.compile(r"^\s*(?:async\s+)?def\s+test_\w*\s*\(")), +) + + +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 = "", 0 + for row in diff.splitlines(): + if row.startswith("+++ "): + path = row[6:] if row.startswith("+++ b/") else "" + elif m := _HUNK.match(row): + line = int(m.group(1)) + elif row.startswith("+") and path.endswith(".py"): + text = row[1:] + kind = next((k for k, rx in _KINDS if rx.search(text)), None) + if kind: + sites.append((path, line, kind, text.strip())) + line += 1 + return sites + + +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) + result = subprocess.run( + ["git", "diff", "-U0", "--no-color", 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..cdb7eb2e5 --- /dev/null +++ b/dokimi/tests/test_list_new_error_paths.py @@ -0,0 +1,58 @@ +# 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 +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(capsys): + assert lnep.main(["--base", "HEAD", "--head", "HEAD"]) == 0 + assert capsys.readouterr().out.startswith("0 new error path(s)") + + +def test_main_returns_2_when_git_cannot_diff(capsys): + assert lnep.main(["--base", "no-such-ref-xyz", "--head", "HEAD"]) == 2 From db802330ffa1c75072745aa35c54ccabf0c40584 Mon Sep 17 00:00:00 2001 From: "Petrov, Nikolay A" Date: Fri, 13 Nov 2026 13:52:55 -0800 Subject: [PATCH 02/11] fix(scripts): list_new_error_paths reads '++' content, one-line raise, quoted paths Signed-off-by: Petrov, Nikolay A --- AGENTS.md | 2 +- dokimi/scripts/list_new_error_paths.py | 13 ++++++++----- dokimi/tests/test_list_new_error_paths.py | 5 +++++ 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e86aab606..b64e54478 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -149,7 +149,7 @@ finds little: - 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` lists them. + `python scripts/list_new_error_paths.py --base origin/main` (from `dokimi/`) lists them. - 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 diff --git a/dokimi/scripts/list_new_error_paths.py b/dokimi/scripts/list_new_error_paths.py index 067036b25..e0c963a28 100644 --- a/dokimi/scripts/list_new_error_paths.py +++ b/dokimi/scripts/list_new_error_paths.py @@ -25,7 +25,7 @@ REPO = Path(__file__).resolve().parents[2] _HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@") _KINDS = ( - ("raise", re.compile(r"^\s*raise\b")), + ("raise", re.compile(r"(?:^|:)\s*raise\b")), ("exit", re.compile(r"\bsys\.exit\(|\bSystemExit\(|\bexit_code\s*=\s*[1-9]")), ("test", re.compile(r"^\s*(?:async\s+)?def\s+test_\w*\s*\(")), ) @@ -34,9 +34,10 @@ 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 = "", 0 + path, line, prev = "", 0, "" for row in diff.splitlines(): - if row.startswith("+++ "): + # An added line `++ x` is the row `+++ x`: a header only follows a `--- ` row. + if row.startswith("+++ ") and prev.startswith("--- "): path = row[6:] if row.startswith("+++ b/") else "" elif m := _HUNK.match(row): line = int(m.group(1)) @@ -46,16 +47,18 @@ def added_sites(diff: str) -> list[tuple[str, int, str, str]]: if kind: sites.append((path, line, kind, text.strip())) line += 1 + prev = row return sites def main(argv: list[str] | None = None) -> int: - parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + 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", "diff", "-U0", "--no-color", f"{args.base}...{args.head}", "--", "*.py"], + ["git", "-c", "core.quotePath=false", "diff", "-U0", "--no-color", + 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) diff --git a/dokimi/tests/test_list_new_error_paths.py b/dokimi/tests/test_list_new_error_paths.py index cdb7eb2e5..d02284773 100644 --- a/dokimi/tests/test_list_new_error_paths.py +++ b/dokimi/tests/test_list_new_error_paths.py @@ -56,3 +56,8 @@ def test_main_lists_the_sites_of_a_real_diff(capsys): 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")] From 96e265cc5db59f8a68ce5e907407a70851cba925 Mon Sep 17 00:00:00 2001 From: vchamarthi <123653452+vchamarthi@users.noreply.github.com> Date: Fri, 13 Nov 2026 19:04:00 -0600 Subject: [PATCH 03/11] fix(scripts): list_new_error_paths reads headers only between hunks Header-like content rows inside a hunk no longer clear the path; tab-terminated headers for paths with spaces are kept; git is called with explicit a/ b/ prefixes and --inter-hunk-context=0; context rows count toward line numbers; 'sys.exit (2)' and 'SystemExit (2)' are listed; the shebang on the non-executable script is dropped; main is tested on a non-empty diff. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com> --- dokimi/scripts/list_new_error_paths.py | 33 +++++++++++------ dokimi/tests/test_list_new_error_paths.py | 43 +++++++++++++++++++++-- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/dokimi/scripts/list_new_error_paths.py b/dokimi/scripts/list_new_error_paths.py index e0c963a28..850be95ac 100644 --- a/dokimi/scripts/list_new_error_paths.py +++ b/dokimi/scripts/list_new_error_paths.py @@ -1,4 +1,3 @@ -#!/usr/bin/env python # Copyright (C) 2026 Intel Corporation # SPDX-License-Identifier: Apache-2.0 @@ -23,10 +22,10 @@ from pathlib import Path REPO = Path(__file__).resolve().parents[2] -_HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@") +_HUNK = re.compile(r"^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") _KINDS = ( ("raise", re.compile(r"(?:^|:)\s*raise\b")), - ("exit", re.compile(r"\bsys\.exit\(|\bSystemExit\(|\bexit_code\s*=\s*[1-9]")), + ("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*\(")), ) @@ -34,19 +33,30 @@ 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, prev = "", 0, "" + path, line, old_left, new_left, prev = "", 0, 0, 0, "" for row in diff.splitlines(): - # An added line `++ x` is the row `+++ x`: a header only follows a `--- ` row. - if row.startswith("+++ ") and prev.startswith("--- "): - path = row[6:] if row.startswith("+++ b/") else "" - elif m := _HUNK.match(row): - line = int(m.group(1)) - elif row.startswith("+") and path.endswith(".py"): + 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") + 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: + 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 @@ -58,6 +68,7 @@ def main(argv: list[str] | None = None) -> int: 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", f"{args.base}...{args.head}", "--", "*.py"], cwd=REPO, capture_output=True, text=True, encoding="utf-8", errors="replace", check=False) if result.returncode: diff --git a/dokimi/tests/test_list_new_error_paths.py b/dokimi/tests/test_list_new_error_paths.py index d02284773..1a97ef989 100644 --- a/dokimi/tests/test_list_new_error_paths.py +++ b/dokimi/tests/test_list_new_error_paths.py @@ -49,9 +49,14 @@ def test_a_deleted_file_lists_nothing(): assert lnep.added_sites(diff) == [] -def test_main_lists_the_sites_of_a_real_diff(capsys): - assert lnep.main(["--base", "HEAD", "--head", "HEAD"]) == 0 - assert capsys.readouterr().out.startswith("0 new error path(s)") +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): @@ -61,3 +66,35 @@ def test_main_returns_2_when_git_cannot_diff(capsys): 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_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]) From a68aeb4f4891c01a0a0936e7547df7b6c41c2366 Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Fri, 13 Nov 2026 22:46:59 -0800 Subject: [PATCH 04/11] fix(scripts): list error paths whatever the user's diff driver An external diff (diff.external, GIT_EXTERNAL_DIFF) or a textconv driver for *.py made git print no unified diff, so the listing was empty with exit 0. Pass --no-ext-diff and --no-textconv, with a real-git test for each. Signed-off-by: Alexander Rybkin --- dokimi/scripts/list_new_error_paths.py | 1 + dokimi/tests/test_list_new_error_paths.py | 30 +++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/dokimi/scripts/list_new_error_paths.py b/dokimi/scripts/list_new_error_paths.py index 850be95ac..361e49ca4 100644 --- a/dokimi/scripts/list_new_error_paths.py +++ b/dokimi/scripts/list_new_error_paths.py @@ -69,6 +69,7 @@ def main(argv: list[str] | None = None) -> int: result = subprocess.run( ["git", "-c", "core.quotePath=false", "diff", "-U0", "--no-color", "--src-prefix=a/", "--dst-prefix=b/", "--inter-hunk-context=0", + "--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: diff --git a/dokimi/tests/test_list_new_error_paths.py b/dokimi/tests/test_list_new_error_paths.py index 1a97ef989..fc478a697 100644 --- a/dokimi/tests/test_list_new_error_paths.py +++ b/dokimi/tests/test_list_new_error_paths.py @@ -6,6 +6,9 @@ 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" @@ -98,3 +101,30 @@ def fake_run(cmd, **kwargs): 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 _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 From e9a13d971ea06f196af889a7ec769c368de5f7ab Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 14 Nov 2026 11:21:33 -0800 Subject: [PATCH 05/11] fix(scripts): list a raise that follows a semicolon Address review: a raise after `;` on the same line was not listed. Thread: PRRT_kwDOVBg7g863XCBK Thread: PRRT_kwDOVBg7g863fjv9 Signed-off-by: Nikolay Petrov --- dokimi/scripts/list_new_error_paths.py | 2 +- dokimi/tests/test_list_new_error_paths.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/dokimi/scripts/list_new_error_paths.py b/dokimi/scripts/list_new_error_paths.py index 361e49ca4..942a4b4a8 100644 --- a/dokimi/scripts/list_new_error_paths.py +++ b/dokimi/scripts/list_new_error_paths.py @@ -24,7 +24,7 @@ REPO = Path(__file__).resolve().parents[2] _HUNK = re.compile(r"^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") _KINDS = ( - ("raise", re.compile(r"(?:^|:)\s*raise\b")), + ("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*\(")), ) diff --git a/dokimi/tests/test_list_new_error_paths.py b/dokimi/tests/test_list_new_error_paths.py index fc478a697..9fea7f63e 100644 --- a/dokimi/tests/test_list_new_error_paths.py +++ b/dokimi/tests/test_list_new_error_paths.py @@ -81,6 +81,11 @@ def test_a_path_with_spaces_keeps_its_sites(): 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_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")] From ede60683d56c76621908a55b359524f9a1da7e23 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 14 Nov 2026 11:21:38 -0800 Subject: [PATCH 06/11] fix(scripts): decode git-quoted paths before the b/ check Address review: a path with a tab, a quote or a backslash arrives C-quoted and its sites were dropped. Thread: PRRT_kwDOVBg7g863b_rQ Thread: PRRT_kwDOVBg7g863fjwB Signed-off-by: Nikolay Petrov --- dokimi/scripts/list_new_error_paths.py | 21 +++++++++++++++++++++ dokimi/tests/test_list_new_error_paths.py | 10 ++++++++++ 2 files changed, 31 insertions(+) diff --git a/dokimi/scripts/list_new_error_paths.py b/dokimi/scripts/list_new_error_paths.py index 942a4b4a8..846ee323d 100644 --- a/dokimi/scripts/list_new_error_paths.py +++ b/dokimi/scripts/list_new_error_paths.py @@ -30,6 +30,25 @@ ) +_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]] = [] @@ -39,6 +58,8 @@ def added_sites(diff: str) -> list[tuple[str, int, str, str]]: # 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)) diff --git a/dokimi/tests/test_list_new_error_paths.py b/dokimi/tests/test_list_new_error_paths.py index 9fea7f63e..4c03e52bc 100644 --- a/dokimi/tests/test_list_new_error_paths.py +++ b/dokimi/tests/test_list_new_error_paths.py @@ -86,6 +86,16 @@ def test_a_raise_after_a_semicolon_on_the_same_line_is_listed(): 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")] + + 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")] From d844b4066d83629e4441753c642141a0b4b7da53 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 14 Nov 2026 11:22:46 -0800 Subject: [PATCH 07/11] Address review: record threads already fixed on head Thread: PRRT_kwDOVBg7g863TV2C: already fixed in 96e265cc5 Thread: PRRT_kwDOVBg7g863TV2n: already fixed in 96e265cc5 Thread: PRRT_kwDOVBg7g863TV3P: already fixed in 96e265cc5 Thread: PRRT_kwDOVBg7g863TkqO: already fixed in 96e265cc5 Thread: PRRT_kwDOVBg7g863Tkqb: already fixed in 96e265cc5 Thread: PRRT_kwDOVBg7g863Tkqh: already fixed in 96e265cc5 Thread: PRRT_kwDOVBg7g863Tzbh: already fixed in 96e265cc5 Thread: PRRT_kwDOVBg7g863XCBV: already fixed in a68aeb4f4 Thread: IC_kwDOVBg7g88AAAABmXSS7g: already fixed in 4ac783803 Thread: PRR_kwDOVBg7g88AAAABX_t37w: already fixed in e9a13d971 Thread: IC_kwDOVBg7g88AAAABmckoFQ: already fixed in e9a13d971 Signed-off-by: Nikolay Petrov From c47a5870e3077322b892de9ff355869f6653432d Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 14 Nov 2026 11:24:20 -0800 Subject: [PATCH 08/11] test(scripts): pin the decoding of a git-quoted non-ASCII path Signed-off-by: Nikolay Petrov --- dokimi/tests/test_list_new_error_paths.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dokimi/tests/test_list_new_error_paths.py b/dokimi/tests/test_list_new_error_paths.py index 4c03e52bc..865771687 100644 --- a/dokimi/tests/test_list_new_error_paths.py +++ b/dokimi/tests/test_list_new_error_paths.py @@ -96,6 +96,11 @@ def test_a_git_quoted_path_with_a_quote_and_a_backslash_is_decoded(): assert lnep.added_sites(diff) == [('q"u\\x.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")] From abed276668c90e877541486fadca391ac436f6e0 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 14 Nov 2026 11:54:14 -0800 Subject: [PATCH 09/11] fix(scripts): pin the diff output indicators on the git call Address review: the parser reads `+` and `-` rows, so an output indicator set by the user's config would hide the added sites. The flags are now explicit; git 2.43 does not read diff.outputIndicatorNew from config, so the test pins the command line rather than a reproduction. Thread: PRRT_kwDOVBg7g863iMGT Signed-off-by: Nikolay Petrov --- dokimi/scripts/list_new_error_paths.py | 1 + dokimi/tests/test_list_new_error_paths.py | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/dokimi/scripts/list_new_error_paths.py b/dokimi/scripts/list_new_error_paths.py index 846ee323d..fa823cceb 100644 --- a/dokimi/scripts/list_new_error_paths.py +++ b/dokimi/scripts/list_new_error_paths.py @@ -90,6 +90,7 @@ def main(argv: list[str] | None = None) -> int: 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) diff --git a/dokimi/tests/test_list_new_error_paths.py b/dokimi/tests/test_list_new_error_paths.py index 865771687..09e15f252 100644 --- a/dokimi/tests/test_list_new_error_paths.py +++ b/dokimi/tests/test_list_new_error_paths.py @@ -123,6 +123,19 @@ def fake_run(cmd, **kwargs): 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") From 48092cde2973a6a43863a8c03b3251092599349d Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Sat, 14 Nov 2026 22:02:37 +0000 Subject: [PATCH 10/11] docs(review): say which refusals the error-path listing misses Signed-off-by: Anton Volkov --- AGENTS.md | 4 +++- REVIEW.md | 4 +++- dokimi/changelog.d/20261113_120000_napetrov_contract-trace.md | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6cfc4b979..194f25f18 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -152,7 +152,9 @@ finds little: - 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 them. + `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 diff --git a/REVIEW.md b/REVIEW.md index f02012433..484ff3c37 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -27,7 +27,9 @@ checks. 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. + 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 index 1af6c7daf..aa8324ac8 100644 --- a/dokimi/changelog.d/20261113_120000_napetrov_contract-trace.md +++ b/dokimi/changelog.d/20261113_120000_napetrov_contract-trace.md @@ -3,5 +3,5 @@ - **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 for the reviewer; it always + `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). From 00f71c0bc341832ea22fdb54c21624df9fad9069 Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Sat, 14 Nov 2026 22:03:01 +0000 Subject: [PATCH 11/11] test(scripts): pin the control-character escapes of a quoted path Signed-off-by: Anton Volkov --- dokimi/tests/test_list_new_error_paths.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/dokimi/tests/test_list_new_error_paths.py b/dokimi/tests/test_list_new_error_paths.py index 09e15f252..7b8d83460 100644 --- a/dokimi/tests/test_list_new_error_paths.py +++ b/dokimi/tests/test_list_new_error_paths.py @@ -96,6 +96,15 @@ def test_a_git_quoted_path_with_a_quote_and_a_backslash_is_decoded(): 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")]