From 2353d17106b086098b9962d4cfa298b255f10e49 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 12:49:58 -0400 Subject: [PATCH 1/6] Require fix pull requests to change a test and name their reproducer A pull request is a fix when it adds or modifies a changelog.d/*.fixed.md fragment or carries the bug or bug-fix label. The new Fix evidence check fails such a pull request unless it adds or modifies an executable test (a pytest module under policyengine_uk/tests/ or a YAML policy test) and its description names the reproducer on a 'Reproducer:' line: a pytest node id or YAML test that exists in a changed test file. A CI- or docs-only fix can write 'Reproducer: n/a: ' when it changes no model source. The check does not yet run the reproducer against the old code. The workflow uses pull_request with the PR text passed through the environment, and also reports on merge queue groups so a branch rule can require it. The PR template gains the Reproducer line, and docs/engineering/skills/github-prs.md describes the check. Built on a GPT-6.1 Sol lane from W2's specification; reviewed and completed in session. Co-Authored-By: Claude Opus 5.5 --- .github/PULL_REQUEST_TEMPLATE.md | 5 + .github/check_fix_pr.py | 368 ++++++++++ .github/workflows/pr_fix_evidence.yaml | 59 ++ changelog.d/fix-pr-evidence-check.added.md | 1 + docs/engineering/skills/github-prs.md | 26 + .../tests/code_health/test_check_fix_pr.py | 638 ++++++++++++++++++ 6 files changed, 1097 insertions(+) create mode 100644 .github/check_fix_pr.py create mode 100644 .github/workflows/pr_fix_evidence.yaml create mode 100644 changelog.d/fix-pr-evidence-check.added.md create mode 100644 policyengine_uk/tests/code_health/test_check_fix_pr.py diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index efaaeeb295..d6c046f90d 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -8,5 +8,10 @@ Fixes # - [ ] `make format` - [ ] Changelog fragment in `changelog.d/` +## Reproducer + + +Reproducer: + axiom: encoded-correct | encoded | queued | n/a: diff --git a/.github/check_fix_pr.py b/.github/check_fix_pr.py new file mode 100644 index 0000000000..c51bb5c855 --- /dev/null +++ b/.github/check_fix_pr.py @@ -0,0 +1,368 @@ +"""Require a changed test and a named reproducer for fix pull requests. + +This runs before the package is installed. Read test definitions rather than +importing PR code, so checking evidence needs neither dependencies nor datasets. +""" + +import argparse +import ast +import html +import json +import re +from dataclasses import dataclass +from pathlib import Path, PurePosixPath + + +TESTS_PREFIX = "policyengine_uk/tests/" +POLICY_PREFIX = TESTS_PREFIX + "policy/" +REPRODUCER_HINT = ( + 'Use "Reproducer: policyengine_uk/tests/test_example.py::test_regression", ' + '"Reproducer: policyengine_uk/tests/policy/example.yaml::Case name", ' + 'or "Reproducer: n/a: " for CI/docs-only fixes.' +) + + +@dataclass +class Report: + title: str + is_fix: bool + passed: bool + messages: list[str] + + +def _relative_path(path: str) -> bool: + return bool(path) and all(part not in ("", ".", "..") for part in path.split("/")) + + +def parse_changes(text: str) -> list[tuple[str, str]]: + """Read the two columns emitted by git diff --name-status --no-renames.""" + changes = [] + for number, line in enumerate(text.splitlines(), 1): + if not line.strip(): + continue + match = re.fullmatch(r"([A-Z])\t([^\t]+)", line) + if not match: + raise ValueError(f"Unparseable changes line {number}: {line!r}") + status, path = match.groups() + if path.startswith('"'): + # Git quotes unusual paths and encodes non-ASCII bytes as octal. + try: + path = ast.literal_eval(path) + if re.search(r"\\[0-7]{3}", match[2]): + path = path.encode("latin-1").decode("utf-8") + except (SyntaxError, ValueError, UnicodeError) as error: + raise ValueError( + f"Unparseable path on changes line {number}" + ) from error + if not isinstance(path, str) or not _relative_path(path): + raise ValueError( + f"Invalid repository path on changes line {number}: {path!r}" + ) + changes.append((status, path)) + return changes + + +def classify_fix_pr( + title: str, body: str, labels: list[str], changes: list[tuple[str, str]] +) -> bool: + """Classify from PR metadata only; title and body are reserved for later rules.""" + return bool({"bug", "bug-fix"}.intersection(labels)) or any( + status in ("A", "M") and re.fullmatch(r"changelog\.d/[^/]+\.fixed\.md", path) + for status, path in changes + ) + + +def _pytest_module(path: str) -> bool: + name = PurePosixPath(path).name + return ( + _relative_path(path) + and path.startswith(TESTS_PREFIX) + and name not in ("conftest.py", "__init__.py") + and (name.startswith("test_") or name.endswith("_test.py")) + and name.endswith(".py") + ) + + +def _policy_test(path: str) -> bool: + return ( + _relative_path(path) + and path.startswith(POLICY_PREFIX) + and PurePosixPath(path).suffix in (".yaml", ".yml") + ) + + +def changed_executable_tests(changes: list[tuple[str, str]]) -> list[str]: + return [ + path + for status, path in changes + if status in ("A", "M") and (_pytest_module(path) or _policy_test(path)) + ] + + +def _exemption(reproducer: str) -> re.Match | None: + return re.fullmatch(r"(?:n/a\s*[:\-]|none\s*:)\s*(.*)", reproducer, re.IGNORECASE) + + +def _unquote_id(value: str) -> str: + value = value.strip() + return value[1:-1] if value.startswith("`") and value.endswith("`") else value + + +def parse_reproducers(body: str) -> list[str]: + ids = [] + for line in body.splitlines(): + match = re.match(r"^\s*reproducer:\s*(.*)$", line, re.IGNORECASE) + if not match or not match[1].strip(): + continue + value = match[1].strip() + start = 0 + quoted = False + brackets = 0 + for index, character in enumerate(value): + if character == "`": + quoted = not quoted + elif not quoted: + if character == "[": + brackets += 1 + elif character == "]" and brackets: + brackets -= 1 + elif character in ",;" and brackets == 0: + ids.append(_unquote_id(value[start:index])) + start = index + 1 + ids.append(_unquote_id(value[start:])) + return ids + + +def _yaml_double_quoted(value: str) -> str: + # YAML allows additional escapes beyond JSON. Translate those while keeping + # JSON's handling of Unicode and escaped quotes, without installing PyYAML. + extra_escapes = { + "0": "\0", + "a": "\a", + "v": "\v", + "e": "\x1b", + " ": " ", + "\t": "\t", + "N": "\u0085", + "_": "\u00a0", + "L": "\u2028", + "P": "\u2029", + } + + def translate(match: re.Match) -> str: + escape = match[1] + if escape[0] in ("x", "U") and len(escape) > 1: + character = chr(int(escape[1:], 16)) + elif escape in extra_escapes: + character = extra_escapes[escape] + else: + return match[0] + return json.dumps(character)[1:-1] + + return json.loads( + re.sub(r"\\(x[0-9a-fA-F]{2}|U[0-9a-fA-F]{8}|.)", translate, value) + ) + + +def _yaml_names(text: str) -> set[str]: + """Read name scalars at test-entry level, excluding nested input names.""" + lines = [ + line + for line in text.splitlines() + if line.strip() and not line.lstrip().startswith(("#", "---", "...", "%")) + ] + if not lines: + return set() + root_indent = min(len(line) - len(line.lstrip(" ")) for line in lines) + is_list = lines[0].lstrip().startswith("-") + names = set() + for line in lines: + match = re.fullmatch(r"( *)(-\s+)?name:\s*(.*)", line) + if not match: + continue + indent, marker, value = match.groups() + expected_indent = root_indent + (2 if is_list and not marker else 0) + if len(indent) != expected_indent or (marker and not is_list): + continue + if value.startswith("'"): + scalar = re.fullmatch(r"'((?:[^']|'')*)'\s*(?:#.*)?", value) + if scalar: + names.add(scalar[1].replace("''", "'")) + elif value.startswith('"'): + scalar = re.fullmatch(r'("(?:[^"\\]|\\.)*")\s*(?:#.*)?', value) + if scalar: + try: + names.add(_yaml_double_quoted(scalar[1])) + except ValueError: + continue + elif value and not value.startswith(("#", "|", ">")): + names.add(re.split(r"\s+#", value, maxsplit=1)[0].strip()) + return names + + +def _reference_problem(reproducer: str, tests: set[str], repo_root: Path) -> str | None: + path, separator, name = reproducer.partition("::") + if path not in tests: + return "the path must be an added or modified executable test in this PR" + try: + file = (repo_root / path).resolve() + if not file.is_relative_to(repo_root.resolve()): + return "the test file must be inside the PR checkout" + if _policy_test(path): + text = file.read_text(encoding="utf-8") + if separator and name not in _yaml_names(text): + return f"the YAML file has no test entry named {name!r}" + return None + if not separator or not name: + return "a pytest reproducer must name a test with ::test_name" + # Parameters can contain ::, so remove them before splitting the node id. + if "[" in name: + if not name.endswith("]"): + return "pytest parameters must be a terminal [param] suffix" + name = name.split("[", 1)[0] + parts = name.split("::") + tree = ast.parse(file.read_bytes(), filename=path) + definitions = (ast.FunctionDef, ast.AsyncFunctionDef) + if len(parts) == 1 and any( + isinstance(node, definitions) and node.name == parts[0] + for node in tree.body + ): + return None + if len(parts) == 2 and any( + isinstance(node, ast.ClassDef) + and node.name == parts[0] + and any( + isinstance(method, definitions) and method.name == parts[1] + for method in node.body + ) + for node in tree.body + ): + return None + return "the pytest file does not define the named test" + except (OSError, RuntimeError, SyntaxError, UnicodeError, ValueError) as error: + return f"the test file cannot be read or parsed: {error}" + + +def check_fix_pr( + title: str, + body: str, + labels: list[str], + changes: list[tuple[str, str]], + repo_root: Path, +) -> Report: + if not classify_fix_pr(title, body, labels, changes): + return Report( + title, + False, + True, + [ + "Not a fix PR: no added/modified .fixed.md fragment or bug/bug-fix label." + ], + ) + tests = set(changed_executable_tests(changes)) + model_changed = any( + status in ("A", "M", "D") + and path.startswith("policyengine_uk/") + and not path.startswith(TESTS_PREFIX) + for status, path in changes + ) + reproducer_messages = [] + valid_exemption = False + reproducers = parse_reproducers(body) + for reproducer in reproducers: + exemption = _exemption(reproducer) + if exemption: + problem = ( + "n/a requires a non-empty reason" + if not exemption[1].strip() + else "n/a is allowed only when no model source outside tests changes" + if model_changed + else None + ) + valid_exemption |= problem is None + else: + problem = _reference_problem(reproducer, tests, repo_root) + if problem: + reproducer_messages.append( + f"Invalid reproducer {reproducer!r}: {problem}. {REPRODUCER_HINT}" + ) + if not reproducers: + reproducer_messages.append(f"Missing named reproducer. {REPRODUCER_HINT}") + messages = [] + if not tests and not valid_exemption: + messages.append( + "No added or modified executable test. Add a pytest module under " + "policyengine_uk/tests/ or YAML test under policyengine_uk/tests/policy/. " + + REPRODUCER_HINT + ) + messages.extend(reproducer_messages) + return Report( + title, + True, + not messages, + messages + or [ + "Fix PR has valid reproducer evidence. " + + ( + "CI/docs-only exemption accepted." + if valid_exemption + else f"Changed executable tests: {len(tests)}." + ) + ], + ) + + +def _emit(report: Report, summary_file: Path | None, unusable: bool = False) -> None: + outcome = "ERROR" if unusable else "PASS" if report.passed else "FAIL" + title = " ".join(report.title.splitlines()) + if not report.is_fix and not unusable: + print(f"Fix evidence: {outcome} ({title!r}). {report.messages[0]}") + else: + print(f"Fix evidence: {outcome} ({title!r})") + for message in report.messages: + print(f"- {message}") + if summary_file: + + def markdown(text: str) -> str: + return re.sub(r"([\\`*_[\]])", r"\\\1", html.escape(text)) + + with summary_file.open("a", encoding="utf-8") as summary: + summary.write(f"\n## Fix evidence\n\n{outcome}: {markdown(title)}\n\n") + for message in report.messages: + summary.write(f"- {markdown(message)}\n") + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + for option in ("title", "body", "labels", "changes"): + parser.add_argument(f"--{option}-file", type=Path, required=True) + parser.add_argument("--repo-root", type=Path, required=True) + parser.add_argument("--summary-file", type=Path) + args = parser.parse_args(argv) + title = "" + unusable = False + try: + title = args.title_file.read_text(encoding="utf-8").strip() + body = args.body_file.read_text(encoding="utf-8") + labels = [ + label.strip() + for label in args.labels_file.read_text(encoding="utf-8").splitlines() + if label.strip() + ] + changes = parse_changes(args.changes_file.read_text(encoding="utf-8")) + if not args.repo_root.is_dir(): + raise ValueError(f"Repository root is not a directory: {args.repo_root}") + report = check_fix_pr(title, body, labels, changes, args.repo_root) + except (OSError, UnicodeError, ValueError) as error: + report = Report(title, False, False, [f"Unusable input: {error}"]) + unusable = True + try: + _emit(report, args.summary_file, unusable) + except (OSError, UnicodeError) as error: + print(f"Fix evidence: ERROR. Cannot append summary: {error}") + return 2 + return 2 if unusable else 0 if report.passed else 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/workflows/pr_fix_evidence.yaml b/.github/workflows/pr_fix_evidence.yaml new file mode 100644 index 0000000000..0cf87d6d15 --- /dev/null +++ b/.github/workflows/pr_fix_evidence.yaml @@ -0,0 +1,59 @@ +# Fix pull requests must change an executable test and name the test that +# reproduces the bug (.github/check_fix_pr.py; docs/engineering/skills/github-prs.md). +# +# It also reports on merge queue groups (merge_group), so a branch rule can +# require it with a merge queue on: a group passes, because each pull +# request's evidence was checked on the pull request before it could join +# the queue, and a group has no pull request text to check. + +name: Fix evidence +on: + pull_request: + branches: [main] + types: [opened, edited, synchronize, reopened, labeled, unlabeled, ready_for_review] + merge_group: + +permissions: + contents: read + +concurrency: + group: fix-evidence-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + Fix-evidence: + name: Fix evidence + runs-on: ubuntu-24.04-arm + timeout-minutes: 5 + steps: + - name: Merge queue group + if: github.event_name == 'merge_group' + run: echo "Each pull request in this group passed Fix evidence before it joined the queue." + - name: Check out the pull request head + if: github.event_name == 'pull_request' + uses: actions/checkout@v6 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + - name: Collect pull request evidence + if: github.event_name == 'pull_request' + env: + PR_TITLE: ${{ github.event.pull_request.title }} + PR_BODY: ${{ github.event.pull_request.body }} + PR_LABELS_JSON: ${{ toJSON(github.event.pull_request.labels.*.name) }} + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + # PR text stays in environment variables so shell syntax in it cannot run. + printf '%s' "$PR_TITLE" > title.txt + printf '%s' "$PR_BODY" > body.md + python3 -c 'import json, os; print("\n".join(json.loads(os.environ["PR_LABELS_JSON"])), end="")' > labels.txt + merge_base=$(git merge-base "$PR_BASE_SHA" "$PR_HEAD_SHA") + git diff --name-status --no-renames "$merge_base" "$PR_HEAD_SHA" > changes.txt + - name: Check fix evidence + if: github.event_name == 'pull_request' + run: | + python3 .github/check_fix_pr.py \ + --title-file title.txt --body-file body.md --labels-file labels.txt \ + --changes-file changes.txt --repo-root . \ + --summary-file "$GITHUB_STEP_SUMMARY" diff --git a/changelog.d/fix-pr-evidence-check.added.md b/changelog.d/fix-pr-evidence-check.added.md new file mode 100644 index 0000000000..1e6e48f12d --- /dev/null +++ b/changelog.d/fix-pr-evidence-check.added.md @@ -0,0 +1 @@ +- Added a Fix evidence pull request check requiring bug fixes to change an executable test and name their reproducer, with an exemption for CI/docs-only fixes. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index a1ac512107..82849f1d6e 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -54,6 +54,32 @@ unless the user or maintainer explicitly asks. The type controls the inferred version bump: `breaking` is major, `added` and `removed` are minor, and other types are patch. +## Fix evidence + +The **Fix evidence** pull request check applies when a PR adds or modifies a +`changelog.d/*.fixed.md` fragment or has the `bug` or `bug-fix` label. A fix PR +must add or modify an executable test under `policyengine_uk/tests/` and name +its reproducer in the PR description: + +```text +Reproducer: policyengine_uk/tests/path/to/test_file.py::test_name +``` + +Accepted ids are pytest node ids (`file.py::test_name` or +`file.py::Class::test_name`) and YAML policy tests under +`policyengine_uk/tests/policy/` (`file.yaml` or `file.yaml::test name`, also +accepting `.yml`). Each named file must be added or modified in the PR, and +each named test must exist in that file. Python files must match `test_*.py` +or `*_test.py`. Separate multiple ids with commas or semicolons. Backticks +around each id are optional; use them when a name or reason contains these +separators. + +For CI/docs-only fixes, use `Reproducer: n/a: ` with a non-empty reason. +This exempts the PR from changing a test only when no file under +`policyengine_uk/` outside `policyengine_uk/tests/` is added, modified or deleted. +The check validates the named reproducer at the PR head; it does not run the +reproducer against the old code. + ## Required Axiom Line Every PR that adds, updates or fixes policy (programmes, parameters, eligibility diff --git a/policyengine_uk/tests/code_health/test_check_fix_pr.py b/policyengine_uk/tests/code_health/test_check_fix_pr.py new file mode 100644 index 0000000000..dbea16b3f8 --- /dev/null +++ b/policyengine_uk/tests/code_health/test_check_fix_pr.py @@ -0,0 +1,638 @@ +"""Fix PRs carry executable, named evidence of the bug they fix. + +The check runs without installing the model. These tests hold its rules to +the invariants reviewers rely on: + +1. Exactly an added or modified fixed fragment, or a bug/bug-fix label, + classifies a PR as a fix. +2. Deleted tests never count as changed executable tests or reproducers. +3. Formatting reproducer ids with supported separators and quoting round + trips through the body parser. +4. A pytest reproducer resolves exactly when its changed file defines the + named function or class method, ignoring parametrisation suffixes. +5. An n/a reason is accepted exactly when it is non-empty and no model + source outside the test tree was added, modified or deleted. +6. Non-fix PRs pass regardless of their bodies. +7. The CLI returns 0/1/2 for passing/failing/unusable input and appends its + Markdown report without replacing an existing job summary. +8. Real git diffs and script subprocesses obey the same rules for passing + and failing fix PRs. +9. The workflow reports the check on pull requests and on merge queue + groups, so a branch rule can require it with a merge queue. +""" + +import importlib.util +import subprocess +import sys +from pathlib import Path + +import pytest +from hypothesis import HealthCheck, example, given, settings +from hypothesis import strategies as st + +TESTS_DIR = Path(__file__).resolve().parents[1] +REPO_ROOT = TESTS_DIR.parents[1] +CHECKER = REPO_ROOT / ".github" / "check_fix_pr.py" + +if not CHECKER.exists(): + pytest.skip( + "the .github folder is not part of an installed package", + allow_module_level=True, + ) + +_spec = importlib.util.spec_from_file_location("check_fix_pr", CHECKER) +checker = importlib.util.module_from_spec(_spec) +sys.modules[_spec.name] = checker +_spec.loader.exec_module(checker) + +PROPERTY_SETTINGS = settings( + max_examples=20, + deadline=None, + derandomize=True, + # Cold imports can be charged to input generation on busy runners. + suppress_health_check=[HealthCheck.function_scoped_fixture, HealthCheck.too_slow], +) + +_identifier = st.from_regex(r"[A-Za-z_][A-Za-z0-9_]{0,10}", fullmatch=True) +_status = st.sampled_from(list("ABCDEFGHIJKLMNOPQRSTUVWXYZ")) +_label = st.one_of( + st.sampled_from( + ["bug", "bug-fix", "Bug", "bugfix", "enhancement", "documentation"] + ), + st.text(alphabet="abcdefghijklmnopqrstuvwxyz-_", max_size=20), +) +_change_path = st.one_of( + _identifier.map(lambda name: f"changelog.d/{name}.fixed.md"), + st.sampled_from( + [ + "changelog.d/regression.added.md", + "changelog.d/regression.changed.md", + "docs/regression.fixed.md", + "policyengine_uk/variables/regression.py", + "policyengine_uk/parameters/regression.yaml", + "policyengine_uk/__init__.py", + "policyengine_uk/tests/test_regression.py", + "policyengine_uk/tests/policy/regression.yaml", + "policyengine_uk/tests2/test_regression.py", + ".github/workflows/check.yaml", + "docs/regression.md", + ] + ), +) +_changes = st.lists(st.tuples(_status, _change_path), max_size=12) +_python_path = _identifier.map( + lambda name: f"policyengine_uk/tests/code_health/test_{name}.py" +) +_yaml_path = st.builds( + lambda name, extension: f"policyengine_uk/tests/policy/{name}.{extension}", + _identifier, + st.sampled_from(["yaml", "yml"]), +) +_executable_path = st.one_of(_python_path, _yaml_path) +_params = st.one_of( + st.just(""), + st.text(alphabet="abc012 ::/-,;", max_size=15).map(lambda value: f"[{value}]"), +) +_valid_id = st.one_of( + st.builds( + lambda path, name, params: f"{path}::test_{name}{params}", + _python_path, + _identifier, + _params, + ), + st.builds( + lambda path, name, params: f"{path}::TestRegression::test_{name}{params}", + _python_path, + _identifier, + _params, + ), + _yaml_path, + st.builds(lambda path, name: f"{path}::{name}", _yaml_path, _identifier), + st.builds( + lambda prefix, reason: prefix + reason, + st.sampled_from(["n/a: ", "n/a - ", "none: "]), + _identifier, + ), +) + + +def _write(repo_root, path, text): + destination = repo_root / path + destination.parent.mkdir(parents=True, exist_ok=True) + destination.write_text(text, encoding="utf-8") + return destination + + +def _report(repo_root, body, changes, labels=("bug",)): + return checker.check_fix_pr( + "Fix a regression", body, list(labels), changes, repo_root + ) + + +@PROPERTY_SETTINGS +@given(labels=st.sets(_label, max_size=10), changes=_changes) +def test_classification_matches_only_fragments_and_labels(labels, changes): + expected = bool({"bug", "bug-fix"} & labels) or any( + status in {"A", "M"} + and path.startswith("changelog.d/") + and path.endswith(".fixed.md") + for status, path in changes + ) + for title, body in [("Fix a regression", "Reproducer: missing"), ("", "")]: + assert checker.classify_fix_pr(title, body, list(labels), changes) is expected + + +@PROPERTY_SETTINGS +@given(paths=st.lists(_executable_path, min_size=1, max_size=6, unique=True)) +def test_deleted_tests_never_count_as_executable_or_reproducers(tmp_path, paths): + changes = [("D", path) for path in paths] + assert checker.changed_executable_tests(changes) == [] + + path = paths[0] + content = ( + "def test_regression():\n pass\n" + if path.endswith(".py") + else "- name: regression\n" + ) + _write(tmp_path, path, content) + # A separate changed test isolates reproducer eligibility from the + # requirement that the PR change some executable test. + companion = "policyengine_uk/tests/test_companion.py" + _write(tmp_path, companion, "def test_companion():\n pass\n") + target = path + ("::test_regression" if path.endswith(".py") else "") + assert not _report( + tmp_path, f"Reproducer: {target}", changes + [("A", companion)] + ).passed + + +@PROPERTY_SETTINGS +@given(ids=st.lists(_valid_id, max_size=8), data=st.data()) +def test_reproducer_line_round_trips_supported_formatting(ids, data): + pieces = [] + for index, nodeid in enumerate(ids): + if index: + pieces.append(data.draw(st.sampled_from([",", ";", ", ", " ; ", "\t;\t"]))) + spaces = data.draw(st.sampled_from(["", " ", "\t", " "])) + quoted = data.draw(st.booleans()) + pieces.append(spaces + (f"`{nodeid}`" if quoted else nodeid) + spaces) + key = data.draw( + st.sampled_from(["Reproducer", "reproducer", "REPRODUCER", "RePrOdUcEr"]) + ) + body = "A description.\n" + key + ": " + "".join(pieces) + "\nA test plan." + assert checker.parse_reproducers(body) == ids + + +@PROPERTY_SETTINGS +@given( + status=_status, + name=_identifier, + defines_name=st.booleans(), + in_class=st.booleans(), + asynchronous=st.booleans(), + params=_params, +) +@example( + status="A", + name="case", + defines_name=True, + in_class=True, + asynchronous=False, + params="[a::b]", +) +@example( + status="M", + name="case", + defines_name=True, + in_class=False, + asynchronous=True, + params="[2026]", +) +def test_pytest_reproducer_requires_changed_file_and_definition( + tmp_path, status, name, defines_name, in_class, asynchronous, params +): + path = "policyengine_uk/tests/test_regression.py" + wanted = f"test_{name}" + defined = wanted if defines_name else f"test_other_{name}" + prefix = "async " if asynchronous else "" + if in_class: + source = ( + f"class TestRegression:\n {prefix}def {defined}(self):\n pass\n" + ) + nodeid = f"{path}::TestRegression::{wanted}{params}" + else: + source = f"{prefix}def {defined}():\n pass\n" + nodeid = f"{path}::{wanted}{params}" + _write(tmp_path, path, source) + + report = _report(tmp_path, f"Reproducer: {nodeid}", [(status, path)]) + assert report.passed is (status in {"A", "M"} and defines_name) + + +@PROPERTY_SETTINGS +@given( + changes=_changes, + reason=st.one_of(st.sampled_from(["", " ", "\t"]), _identifier), + prefix=st.sampled_from(["n/a: ", "n/a - ", "none: "]), +) +@example( + changes=[("D", "policyengine_uk/variables/regression.py")], + reason="CI only", + prefix="n/a: ", +) +@example( + changes=[("M", "policyengine_uk/tests2/test_regression.py")], + reason="CI only", + prefix="none: ", +) +def test_na_requires_reason_and_no_model_source_changes( + tmp_path, changes, reason, prefix +): + has_model_source = any( + status in {"A", "M", "D"} + and path.startswith("policyengine_uk/") + and not path.startswith("policyengine_uk/tests/") + for status, path in changes + ) + report = _report(tmp_path, f"Reproducer: {prefix}{reason}", changes) + assert report.passed is (bool(reason.strip()) and not has_model_source) + + +@PROPERTY_SETTINGS +@given(title=st.text(max_size=80), body=st.text(max_size=300), changes=_changes) +@example( + title="Fix the model", + body="Reproducer: n/a:\nReproducer: missing::test_x", + changes=[], +) +def test_non_fix_pr_always_passes(tmp_path, title, body, changes): + non_fix_changes = [ + (status, path) + for status, path in changes + if not (status in {"A", "M"} and path.endswith(".fixed.md")) + ] + report = checker.check_fix_pr( + title, body, ["enhancement", "Bug"], non_fix_changes, tmp_path + ) + assert not report.is_fix + assert report.passed + assert report.title == title + assert len(report.messages) == 1 + + +@pytest.mark.parametrize( + "path, executable", + [ + ("policyengine_uk/tests/test_regression.py", True), + ("policyengine_uk/tests/code_health/regression_test.py", True), + ("policyengine_uk/tests/policy/regression.yaml", True), + ("policyengine_uk/tests/policy/regression.yml", True), + ("policyengine_uk/tests/conftest.py", False), + ("policyengine_uk/tests/__init__.py", False), + ("policyengine_uk/tests/helpers.py", False), + ("policyengine_uk/tests/gov/regression.yaml", False), + ("policyengine_uk/tests/policy2/regression.yaml", False), + ("tests/test_regression.py", False), + ("policyengine_uk/variables/test_regression.py", False), + ], +) +def test_only_collected_test_paths_are_executable(path, executable): + for status in ["A", "M", "D", "T", "U"]: + assert checker.changed_executable_tests([(status, path)]) == ( + [path] if executable and status in {"A", "M"} else [] + ) + + +@pytest.mark.parametrize( + "source", + [ + "- name: Regression case\n period: 2026\n", + "name: Regression case\nperiod: 2026\n", + "- name: 'Regression case'\n", + '- name: "Regression case"\n', + "- period: 2026\n name: Regression case\n", + "- name: Regression case # An inline comment\n", + ], +) +@pytest.mark.parametrize("extension", ["yaml", "yml"]) +def test_yaml_reproducer_accepts_bare_files_and_named_entries( + tmp_path, source, extension +): + path = f"policyengine_uk/tests/policy/regression.{extension}" + _write(tmp_path, path, source) + changes = [("M", path)] + assert _report(tmp_path, f"Reproducer: {path}", changes).passed + assert _report(tmp_path, f"Reproducer: {path}::Regression case", changes).passed + assert not _report(tmp_path, f"Reproducer: {path}::Different case", changes).passed + + +@pytest.mark.parametrize( + "source, name", + [ + ("- name: 'It''s a case' # Comment\n", "It's a case"), + ('- name: "A \\"quoted\\" # case" # Comment\n', 'A "quoted" # case'), + ('- name: "Case, with; punctuation"\n', "Case, with; punctuation"), + ('- name: "Regression\\x20case"\n', "Regression case"), + ('- name: "\\U00000041"\n', "A"), + ('- name: "\\\\x41"\n', "\\x41"), + ], +) +def test_quoted_yaml_names_resolve_without_treating_punctuation_as_ids( + tmp_path, source, name +): + path = "policyengine_uk/tests/policy/regression.yaml" + _write(tmp_path, path, source) + assert _report(tmp_path, f"Reproducer: `{path}::{name}`", [("A", path)]).passed + + +def test_nested_yaml_name_is_not_a_test_entry(tmp_path): + path = "policyengine_uk/tests/policy/regression.yaml" + _write(tmp_path, path, "- name: Regression case\n input:\n name: Impostor\n") + changes = [("M", path)] + assert _report(tmp_path, f"Reproducer: {path}::Regression case", changes).passed + assert not _report(tmp_path, f"Reproducer: {path}::Impostor", changes).passed + + +def test_backticked_na_reason_can_include_commas_and_semicolons(tmp_path): + assert _report(tmp_path, "Reproducer: `n/a: CI, docs; no model source`", []).passed + + +@pytest.mark.parametrize( + "source, suffix", + [ + ("def test_regression():\n pass\n", ""), + ("def test_regression():\n pass\n", "::test_regression[unterminated"), + ("def test_regression():\n pass\n", "::test_regression[p]::junk"), + ("def test_regression(:\n pass\n", "::test_regression"), + ("test_regression = True\n", "::test_regression"), + ( + "def wrapper():\n def test_regression():\n pass\n", + "::test_regression", + ), + ( + "class TestOther:\n def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "class TestRegression:\n def test_regression(self):\n pass\n", + "::TestRegression::Nested::test_regression", + ), + ], +) +def test_pytest_reproducer_rejects_bare_paths_and_nonexistent_definitions( + tmp_path, source, suffix +): + path = "policyengine_uk/tests/test_regression.py" + _write(tmp_path, path, source) + assert not _report(tmp_path, f"Reproducer: {path}{suffix}", [("M", path)]).passed + + +def test_reproducer_file_must_exist_at_pr_head(tmp_path): + path = "policyengine_uk/tests/test_regression.py" + assert not _report( + tmp_path, f"Reproducer: {path}::test_regression", [("A", path)] + ).passed + + +@pytest.mark.parametrize("outside_checkout", [True, False]) +def test_reproducer_symlinks_cannot_escape_checkout_or_crash_on_loops( + tmp_path, outside_checkout +): + repo_root = tmp_path / "checkout" + path = "policyengine_uk/tests/test_regression.py" + destination = repo_root / path + destination.parent.mkdir(parents=True) + if outside_checkout: + target = _write(tmp_path, "outside.py", "def test_regression():\n pass\n") + else: + target = destination + destination.symlink_to(target) + assert not _report( + repo_root, f"Reproducer: {path}::test_regression", [("A", path)] + ).passed + + +def test_every_reproducer_on_every_line_must_resolve(tmp_path): + path = "policyengine_uk/tests/test_regression.py" + _write(tmp_path, path, "def test_one():\n pass\n\ndef test_two():\n pass\n") + changes = [("M", path)] + body = f"Reproducer: `{path}::test_one`\nreproducer: {path}::test_two" + assert _report(tmp_path, body, changes).passed + assert not _report(tmp_path, body + f"; {path}::test_missing", changes).passed + assert not _report(tmp_path, "Reproducer: n/a: CI only; missing", []).passed + + +def test_report_explains_each_failed_requirement(tmp_path): + report = _report(tmp_path, "A description without evidence.", []) + assert not report.passed + assert len(report.messages) >= 2 + assert sum("Reproducer:" in message for message in report.messages) >= 2 + + +def _arguments( + input_dir, + repo_root, + *, + body="Reproducer: n/a: CI only", + labels="bug\n", + changes="", + summary=None, +): + values = { + "title": "Fix a regression", + "body": body, + "labels": labels, + "changes": changes, + } + args = [] + for name, value in values.items(): + path = _write(input_dir, f"{name}.txt", value) + args.extend([f"--{name}-file", str(path)]) + args.extend(["--repo-root", str(repo_root)]) + if summary is not None: + args.extend(["--summary-file", str(summary)]) + return args + + +@pytest.mark.parametrize( + "body, labels, changes, expected", + [ + ("Reproducer: n/a: CI only", "bug\n", "M\t.github/workflows/check.yaml\n", 0), + ( + "Reproducer: policyengine_uk/tests/test_regression.py::test_regression", + "bug-fix\n", + "M\tpolicyengine_uk/tests/test_regression.py\n", + 0, + ), + ("No evidence.", "bug\n", "M\tpolicyengine_uk/variables/regression.py\n", 1), + ( + "Reproducer: invalid", + "enhancement\n", + "D\tchangelog.d/regression.fixed.md\n", + 0, + ), + ( + "Reproducer: n/a: CI only", + "bug\n", + "T\t.github/workflows/check.yaml\nQ\tdocs/regression.md\n", + 0, + ), + ], +) +def test_main_returns_zero_or_one_for_usable_inputs( + tmp_path, capsys, body, labels, changes, expected +): + _write( + tmp_path, + "policyengine_uk/tests/test_regression.py", + "def test_regression():\n pass\n", + ) + args = _arguments(tmp_path, tmp_path, body=body, labels=labels, changes=changes) + assert checker.main(args) == expected + assert "Fix a regression" in capsys.readouterr().out + + +@pytest.mark.parametrize("missing", ["title", "body", "labels", "changes"]) +def test_main_returns_two_for_missing_input_files(tmp_path, capsys, missing): + args = _arguments(tmp_path, tmp_path) + (tmp_path / f"{missing}.txt").unlink() + assert checker.main(args) == 2 + assert capsys.readouterr().out.strip() + + +@pytest.mark.parametrize( + "changes", + [ + "M path/to/test.py\n", + "M\t\n", + "MM\tpath.py\n", + "R100\told.py\tnew.py\n", + "M\tfirst.py\tsecond.py\n", + ], +) +def test_main_returns_two_for_unparseable_changes(tmp_path, capsys, changes): + args = _arguments(tmp_path, tmp_path, changes=changes) + assert checker.main(args) == 2 + assert capsys.readouterr().out.strip() + + +def test_main_returns_two_for_missing_repository_root(tmp_path): + args = _arguments(tmp_path, tmp_path / "missing") + assert checker.main(args) == 2 + + +def test_main_returns_two_for_unwritable_summary(tmp_path): + # A directory is unwritable as a summary file even when tests run as root. + args = _arguments(tmp_path, tmp_path, summary=tmp_path) + assert checker.main(args) == 2 + + +def test_main_appends_markdown_to_existing_summary(tmp_path): + summary = tmp_path / "summary.md" + summary.write_text("Existing summary.\n", encoding="utf-8") + args = _arguments(tmp_path, tmp_path, summary=summary) + assert checker.main(args) == 0 + first = summary.read_text(encoding="utf-8") + assert first.startswith("Existing summary.\n") + assert "## Fix evidence" in first + + (tmp_path / "body.txt").write_text("No reproducer.", encoding="utf-8") + assert checker.main(args) == 1 + second = summary.read_text(encoding="utf-8") + assert second.startswith(first) + assert second.count("## Fix evidence") == 2 + assert "Reproducer:" in second + + (tmp_path / "changes.txt").write_text("unparseable", encoding="utf-8") + assert checker.main(args) == 2 + third = summary.read_text(encoding="utf-8") + assert third.startswith(second) + assert third.count("## Fix evidence") == 3 + assert "Unusable input" in third + + +def _git(repo_root, *args): + result = subprocess.run( + [ + "git", + "-c", + "user.name=Fix evidence tests", + "-c", + "user.email=tests@example.invalid", + "-c", + "commit.gpgsign=false", + "-c", + "core.hooksPath=/dev/null", + *args, + ], + cwd=repo_root, + capture_output=True, + text=True, + timeout=15, + ) + assert result.returncode == 0, result.stdout + result.stderr + return result.stdout.strip() + + +def test_real_git_diff_and_script_subprocess(tmp_path): + repo_root = tmp_path / "repo" + repo_root.mkdir() + _git(repo_root, "init", "--initial-branch=fixture") + python_path = "policyengine_uk/tests/test_regression.py" + yaml_path = "policyengine_uk/tests/gov/regression.yaml" + _write(repo_root, python_path, "def test_regression():\n pass\n") + _write(repo_root, yaml_path, "- name: Regression\n period: 2025\n") + _git(repo_root, "add", ".") + _git(repo_root, "commit", "-m", "Fixture before the fix") + base = _git(repo_root, "rev-parse", "HEAD") + + _write(repo_root, python_path, "def test_regression():\n assert 1 + 1 == 2\n") + _write(repo_root, yaml_path, "- name: Regression\n period: 2026\n") + _write(repo_root, "changelog.d/regression.fixed.md", "- Fixed a regression.\n") + _git(repo_root, "add", ".") + _git(repo_root, "commit", "-m", "Fixture fix and its evidence") + head = _git(repo_root, "rev-parse", "HEAD") + merge_base = _git(repo_root, "merge-base", base, head) + changes = ( + _git(repo_root, "diff", "--name-status", "--no-renames", merge_base, head) + + "\n" + ) + assert f"M\t{python_path}\n" in changes + assert f"M\t{yaml_path}\n" in changes + assert "A\tchangelog.d/regression.fixed.md\n" in changes + + for nodeid, expected in [(f"{python_path}::test_regression", 0), (yaml_path, 1)]: + args = _arguments( + tmp_path / "inputs", + repo_root, + body=f"Reproducer: {nodeid}", + labels="", + changes=changes, + ) + result = subprocess.run( + [sys.executable, str(CHECKER), *args], + cwd=repo_root, + capture_output=True, + text=True, + timeout=15, + ) + assert result.returncode == expected, result.stdout + result.stderr + assert "Fix a regression" in result.stdout + if expected: + assert "Reproducer:" in result.stdout + + +def test_the_workflow_reports_on_pull_requests_and_merge_groups(): + import yaml + + workflow = yaml.safe_load( + (REPO_ROOT / ".github" / "workflows" / "pr_fix_evidence.yaml").read_text( + encoding="utf-8" + ) + ) + # PyYAML reads the key "on" as True. + triggers = workflow.get("on", workflow.get(True)) + assert set(triggers) == {"pull_request", "merge_group"} + assert "paths" not in triggers["pull_request"] + [job] = workflow["jobs"].values() + assert job["name"] == "Fix evidence" + assert "if" not in job From 9e378d93e708df53fd6edfe580762d94749a6b16 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 20:51:52 -0400 Subject: [PATCH 2/6] Address the GPT-6.1 Sol review of the Fix evidence check - Run the checker from the base branch under pull_request_target and read the pull request's files as git objects at its head SHA, never from a checkout: a pull request can no longer replace the checker that judges it, and a symlink cannot redirect a read or overwrite a file. Metadata goes to a fresh directory under RUNNER_TEMP. The job executes, installs and imports nothing from the pull request. - Classify a fragment as a fix the way towncrier reads names: 'fixed' in any part after the first (x.fixed.1.md, x.fixed.md.md), with T (type change) counted as a change. - A pytest reproducer must be something pytest collects: a test* function, or a test* method of a Test* class without __init__, outside a module or class that sets __test__ = False. - A YAML reproducer must parse (PyYAML) to a list with a test mapping that has a name and an output; the named entry must be such a mapping. - 'n/a' is refused when pyproject.toml or uv.lock change, not only model source; HTML comments are ignored when reading Reproducer: lines; the change list is NUL-separated. Built on a GPT-6.1 Sol lane from W2's design notes; checked in session. Co-Authored-By: Claude Opus 5.5 --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- .github/check_fix_pr.py | 290 +++++---- .github/workflows/pr_fix_evidence.yaml | 46 +- changelog.d/fix-pr-evidence-check.added.md | 2 +- docs/engineering/skills/github-prs.md | 39 +- .../tests/code_health/test_check_fix_pr.py | 562 +++++++++++++++--- 6 files changed, 723 insertions(+), 218 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index d6c046f90d..ef38f44eeb 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -10,7 +10,7 @@ Fixes # ## Reproducer - + Reproducer: diff --git a/.github/check_fix_pr.py b/.github/check_fix_pr.py index c51bb5c855..2c99a77da4 100644 --- a/.github/check_fix_pr.py +++ b/.github/check_fix_pr.py @@ -1,14 +1,15 @@ """Require a changed test and a named reproducer for fix pull requests. This runs before the package is installed. Read test definitions rather than -importing PR code, so checking evidence needs neither dependencies nor datasets. +importing PR code. YAML validation uses PyYAML when available and fails closed +without it; checking evidence never needs model dependencies or datasets. """ import argparse import ast import html -import json import re +import subprocess from dataclasses import dataclass from pathlib import Path, PurePosixPath @@ -18,7 +19,7 @@ REPRODUCER_HINT = ( 'Use "Reproducer: policyengine_uk/tests/test_example.py::test_regression", ' '"Reproducer: policyengine_uk/tests/policy/example.yaml::Case name", ' - 'or "Reproducer: n/a: " for CI/docs-only fixes.' + 'or "Reproducer: n/a: " when model source and dependencies are unchanged.' ) @@ -35,29 +36,18 @@ def _relative_path(path: str) -> bool: def parse_changes(text: str) -> list[tuple[str, str]]: - """Read the two columns emitted by git diff --name-status --no-renames.""" + """Read status/path pairs from git diff --name-status --no-renames -z.""" + if not text: + return [] + fields = text.split("\0") + if fields.pop() != "" or len(fields) % 2: + raise ValueError("Changes must be NUL-terminated status/path pairs") changes = [] - for number, line in enumerate(text.splitlines(), 1): - if not line.strip(): - continue - match = re.fullmatch(r"([A-Z])\t([^\t]+)", line) - if not match: - raise ValueError(f"Unparseable changes line {number}: {line!r}") - status, path = match.groups() - if path.startswith('"'): - # Git quotes unusual paths and encodes non-ASCII bytes as octal. - try: - path = ast.literal_eval(path) - if re.search(r"\\[0-7]{3}", match[2]): - path = path.encode("latin-1").decode("utf-8") - except (SyntaxError, ValueError, UnicodeError) as error: - raise ValueError( - f"Unparseable path on changes line {number}" - ) from error - if not isinstance(path, str) or not _relative_path(path): - raise ValueError( - f"Invalid repository path on changes line {number}: {path!r}" - ) + for status, path in zip(fields[::2], fields[1::2]): + if not re.fullmatch(r"[A-Z]", status): + raise ValueError(f"Unparseable change status: {status!r}") + if not _relative_path(path): + raise ValueError(f"Invalid repository path: {path!r}") changes.append((status, path)) return changes @@ -66,9 +56,16 @@ def classify_fix_pr( title: str, body: str, labels: list[str], changes: list[tuple[str, str]] ) -> bool: """Classify from PR metadata only; title and body are reserved for later rules.""" - return bool({"bug", "bug-fix"}.intersection(labels)) or any( - status in ("A", "M") and re.fullmatch(r"changelog\.d/[^/]+\.fixed\.md", path) - for status, path in changes + return ( + bool({"bug", "bug-fix"}.intersection(labels)) + or any( + status in ("A", "M", "T") + and path.startswith("changelog.d/") + and len(path.split("/")) == 2 + # Towncrier searches every basename part after the issue for its type. + and "fixed" in PurePosixPath(path).name.split(".")[1:] + for status, path in changes + ) ) @@ -95,7 +92,7 @@ def changed_executable_tests(changes: list[tuple[str, str]]) -> list[str]: return [ path for status, path in changes - if status in ("A", "M") and (_pytest_module(path) or _policy_test(path)) + if status in ("A", "M", "T") and (_pytest_module(path) or _policy_test(path)) ] @@ -109,6 +106,8 @@ def _unquote_id(value: str) -> str: def parse_reproducers(body: str) -> list[str]: + # Template guidance is not evidence supplied by the contributor. + body = re.sub(r"", "", body, flags=re.DOTALL) ids = [] for line in body.splitlines(): match = re.match(r"^\s*reproducer:\s*(.*)$", line, re.IGNORECASE) @@ -133,84 +132,93 @@ def parse_reproducers(body: str) -> list[str]: return ids -def _yaml_double_quoted(value: str) -> str: - # YAML allows additional escapes beyond JSON. Translate those while keeping - # JSON's handling of Unicode and escaped quotes, without installing PyYAML. - extra_escapes = { - "0": "\0", - "a": "\a", - "v": "\v", - "e": "\x1b", - " ": " ", - "\t": "\t", - "N": "\u0085", - "_": "\u00a0", - "L": "\u2028", - "P": "\u2029", - } - - def translate(match: re.Match) -> str: - escape = match[1] - if escape[0] in ("x", "U") and len(escape) > 1: - character = chr(int(escape[1:], 16)) - elif escape in extra_escapes: - character = extra_escapes[escape] - else: - return match[0] - return json.dumps(character)[1:-1] +def _read_test(repo_root: Path, path: str, head_sha: str | None) -> bytes: + if head_sha is not None: + + def git(*args: str) -> bytes: + return subprocess.run( + ["git", "-C", str(repo_root), "--literal-pathspecs", *args], + check=True, + capture_output=True, + timeout=15, + ).stdout + + entries = git("ls-tree", "-z", head_sha, "--", path).split(b"\0") + if len(entries) != 2 or entries[-1] != b"": + raise ValueError("the test file is missing from the PR head") + metadata, entry_path = entries[0].split(b"\t", 1) + mode, kind, _ = metadata.split() + if entry_path.decode("utf-8", errors="surrogateescape") != path: + raise ValueError("the test file is missing from the PR head") + # A symlink's blob is its target text, not an executable test file. + if mode == b"120000": + raise ValueError("a symlink cannot be a reproducer") + if mode not in (b"100644", b"100755") or kind != b"blob": + raise ValueError("the reproducer must be a regular test file") + return git("cat-file", "blob", f"{head_sha}:{path}") + + # Filesystem mode supports unit fixtures; CI always reads committed blobs. + file = repo_root + for part in path.split("/"): + file = file / part + if file.is_symlink(): + raise ValueError("a symlink cannot be a reproducer") + if not file.resolve().is_relative_to(repo_root.resolve()): + raise ValueError("the test file must be inside the PR checkout") + return file.read_bytes() + + +def _assigns_name(node: ast.AST, name: str) -> bool: + targets = ( + node.targets + if isinstance(node, ast.Assign) + else [node.target] + if isinstance(node, ast.AnnAssign) + else [] + ) + return any(isinstance(target, ast.Name) and target.id == name for target in targets) + - return json.loads( - re.sub(r"\\(x[0-9a-fA-F]{2}|U[0-9a-fA-F]{8}|.)", translate, value) +def _suppresses_tests(body: list[ast.stmt]) -> bool: + return any( + _assigns_name(node, "__test__") + and isinstance(node.value, ast.Constant) + and node.value.value is False + for node in body ) -def _yaml_names(text: str) -> set[str]: - """Read name scalars at test-entry level, excluding nested input names.""" - lines = [ - line - for line in text.splitlines() - if line.strip() and not line.lstrip().startswith(("#", "---", "...", "%")) - ] - if not lines: - return set() - root_indent = min(len(line) - len(line.lstrip(" ")) for line in lines) - is_list = lines[0].lstrip().startswith("-") - names = set() - for line in lines: - match = re.fullmatch(r"( *)(-\s+)?name:\s*(.*)", line) - if not match: - continue - indent, marker, value = match.groups() - expected_indent = root_indent + (2 if is_list and not marker else 0) - if len(indent) != expected_indent or (marker and not is_list): - continue - if value.startswith("'"): - scalar = re.fullmatch(r"'((?:[^']|'')*)'\s*(?:#.*)?", value) - if scalar: - names.add(scalar[1].replace("''", "'")) - elif value.startswith('"'): - scalar = re.fullmatch(r'("(?:[^"\\]|\\.)*")\s*(?:#.*)?', value) - if scalar: - try: - names.add(_yaml_double_quoted(scalar[1])) - except ValueError: - continue - elif value and not value.startswith(("#", "|", ">")): - names.add(re.split(r"\s+#", value, maxsplit=1)[0].strip()) - return names - - -def _reference_problem(reproducer: str, tests: set[str], repo_root: Path) -> str | None: +def _reference_problem( + reproducer: str, tests: set[str], repo_root: Path, head_sha: str | None +) -> str | None: path, separator, name = reproducer.partition("::") if path not in tests: - return "the path must be an added or modified executable test in this PR" + return "the path must be an added, modified or type-changed executable test in this PR" try: - file = (repo_root / path).resolve() - if not file.is_relative_to(repo_root.resolve()): - return "the test file must be inside the PR checkout" + source = _read_test(repo_root, path, head_sha) if _policy_test(path): - text = file.read_text(encoding="utf-8") - if separator and name not in _yaml_names(text): + try: + import yaml + except ImportError: + return "cannot validate YAML tests without PyYAML" + try: + entries = yaml.safe_load(source.decode("utf-8")) + except yaml.YAMLError as error: + return f"the YAML file cannot be parsed: {error}" + names = ( + { + entry["name"] + for entry in entries + if isinstance(entry, dict) + and isinstance(entry.get("name"), str) + and "output" in entry + } + if isinstance(entries, list) + else set() + ) + if not names: + return "the YAML file has no test mapping with a string name and output" + if separator and name not in names: return f"the YAML file has no test entry named {name!r}" return None if not separator or not name: @@ -221,25 +229,49 @@ def _reference_problem(reproducer: str, tests: set[str], repo_root: Path) -> str return "pytest parameters must be a terminal [param] suffix" name = name.split("[", 1)[0] parts = name.split("::") - tree = ast.parse(file.read_bytes(), filename=path) + tree = ast.parse(source, filename=path) + if _suppresses_tests(tree.body): + return "pytest collection is disabled by module __test__ = False" definitions = (ast.FunctionDef, ast.AsyncFunctionDef) - if len(parts) == 1 and any( - isinstance(node, definitions) and node.name == parts[0] - for node in tree.body + if ( + len(parts) == 1 + and parts[0].startswith("test") + and any( + isinstance(node, definitions) and node.name == parts[0] + for node in tree.body + ) ): return None - if len(parts) == 2 and any( - isinstance(node, ast.ClassDef) - and node.name == parts[0] + if ( + len(parts) == 2 + and parts[0].startswith("Test") + and parts[1].startswith("test") and any( - isinstance(method, definitions) and method.name == parts[1] - for method in node.body + isinstance(node, ast.ClassDef) + and node.name == parts[0] + and not _suppresses_tests(node.body) + and not any( + (isinstance(method, definitions) and method.name == "__init__") + or _assigns_name(method, "__init__") + for method in node.body + ) + and any( + isinstance(method, definitions) and method.name == parts[1] + for method in node.body + ) + for node in tree.body ) - for node in tree.body ): return None - return "the pytest file does not define the named test" - except (OSError, RuntimeError, SyntaxError, UnicodeError, ValueError) as error: + return "the pytest file does not define a collectable named test" + except ( + OSError, + RuntimeError, + SyntaxError, + UnicodeError, + ValueError, + subprocess.SubprocessError, + ) as error: return f"the test file cannot be read or parsed: {error}" @@ -249,21 +281,28 @@ def check_fix_pr( labels: list[str], changes: list[tuple[str, str]], repo_root: Path, + head_sha: str | None = None, ) -> Report: + if head_sha is not None and not re.fullmatch( + r"(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})", head_sha + ): + raise ValueError("The PR head must be a full hexadecimal commit SHA") if not classify_fix_pr(title, body, labels, changes): return Report( title, False, True, [ - "Not a fix PR: no added/modified .fixed.md fragment or bug/bug-fix label." + "Not a fix PR: no added/modified/type-changed fixed fragment or bug/bug-fix label." ], ) tests = set(changed_executable_tests(changes)) model_changed = any( - status in ("A", "M", "D") - and path.startswith("policyengine_uk/") - and not path.startswith(TESTS_PREFIX) + status in ("A", "M", "T", "D") + and ( + (path.startswith("policyengine_uk/") and not path.startswith(TESTS_PREFIX)) + or path in ("pyproject.toml", "uv.lock") + ) for status, path in changes ) reproducer_messages = [] @@ -275,13 +314,13 @@ def check_fix_pr( problem = ( "n/a requires a non-empty reason" if not exemption[1].strip() - else "n/a is allowed only when no model source outside tests changes" + else "n/a is allowed only when model source outside tests, pyproject.toml and uv.lock are unchanged" if model_changed else None ) valid_exemption |= problem is None else: - problem = _reference_problem(reproducer, tests, repo_root) + problem = _reference_problem(reproducer, tests, repo_root, head_sha) if problem: reproducer_messages.append( f"Invalid reproducer {reproducer!r}: {problem}. {REPRODUCER_HINT}" @@ -291,7 +330,7 @@ def check_fix_pr( messages = [] if not tests and not valid_exemption: messages.append( - "No added or modified executable test. Add a pytest module under " + "No added, modified or type-changed executable test. Add a pytest module under " "policyengine_uk/tests/ or YAML test under policyengine_uk/tests/policy/. " + REPRODUCER_HINT ) @@ -304,7 +343,7 @@ def check_fix_pr( or [ "Fix PR has valid reproducer evidence. " + ( - "CI/docs-only exemption accepted." + "Exemption accepted: model source and dependencies are unchanged." if valid_exemption else f"Changed executable tests: {len(tests)}." ) @@ -337,6 +376,10 @@ def main(argv: list[str] | None = None) -> int: for option in ("title", "body", "labels", "changes"): parser.add_argument(f"--{option}-file", type=Path, required=True) parser.add_argument("--repo-root", type=Path, required=True) + parser.add_argument( + "--head-sha", + help="Read tests from this commit's git objects instead of the filesystem", + ) parser.add_argument("--summary-file", type=Path) args = parser.parse_args(argv) title = "" @@ -349,10 +392,15 @@ def main(argv: list[str] | None = None) -> int: for label in args.labels_file.read_text(encoding="utf-8").splitlines() if label.strip() ] - changes = parse_changes(args.changes_file.read_text(encoding="utf-8")) + # Preserve path bytes, including literal newlines, rather than decoding Git quotes. + changes = parse_changes( + args.changes_file.read_bytes().decode("utf-8", errors="surrogateescape") + ) if not args.repo_root.is_dir(): raise ValueError(f"Repository root is not a directory: {args.repo_root}") - report = check_fix_pr(title, body, labels, changes, args.repo_root) + report = check_fix_pr( + title, body, labels, changes, args.repo_root, args.head_sha + ) except (OSError, UnicodeError, ValueError) as error: report = Report(title, False, False, [f"Unusable input: {error}"]) unusable = True diff --git a/.github/workflows/pr_fix_evidence.yaml b/.github/workflows/pr_fix_evidence.yaml index 0cf87d6d15..fa2d709ec2 100644 --- a/.github/workflows/pr_fix_evidence.yaml +++ b/.github/workflows/pr_fix_evidence.yaml @@ -1,6 +1,10 @@ # Fix pull requests must change an executable test and name the test that # reproduces the bug (.github/check_fix_pr.py; docs/engineering/skills/github-prs.md). # +# pull_request_target is safe here because the job never executes, installs or +# imports code from the PR. The base checkout supplies the trusted checker, +# which reads the PR's files as data from git objects. +# # It also reports on merge queue groups (merge_group), so a branch rule can # require it with a merge queue on: a group passes, because each pull # request's evidence was checked on the pull request before it could join @@ -8,7 +12,7 @@ name: Fix evidence on: - pull_request: + pull_request_target: branches: [main] types: [opened, edited, synchronize, reopened, labeled, unlabeled, ready_for_review] merge_group: @@ -18,7 +22,7 @@ permissions: concurrency: group: fix-evidence-${{ github.event.pull_request.number || github.ref }} - cancel-in-progress: ${{ github.event_name == 'pull_request' }} + cancel-in-progress: ${{ github.event_name == 'pull_request_target' }} jobs: Fix-evidence: @@ -29,31 +33,45 @@ jobs: - name: Merge queue group if: github.event_name == 'merge_group' run: echo "Each pull request in this group passed Fix evidence before it joined the queue." - - name: Check out the pull request head - if: github.event_name == 'pull_request' + - name: Check out the trusted base branch + if: github.event_name == 'pull_request_target' uses: actions/checkout@v6 with: - ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 0 - name: Collect pull request evidence - if: github.event_name == 'pull_request' + id: evidence + if: github.event_name == 'pull_request_target' env: + PR_NUMBER: ${{ github.event.pull_request.number }} PR_TITLE: ${{ github.event.pull_request.title }} PR_BODY: ${{ github.event.pull_request.body }} PR_LABELS_JSON: ${{ toJSON(github.event.pull_request.labels.*.name) }} PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | + # A fresh directory keeps metadata writes away from any PR paths. + evidence_dir=$(mktemp -d "$RUNNER_TEMP/fix-evidence.XXXXXX") + printf 'directory=%s\n' "$evidence_dir" >> "$GITHUB_OUTPUT" # PR text stays in environment variables so shell syntax in it cannot run. - printf '%s' "$PR_TITLE" > title.txt - printf '%s' "$PR_BODY" > body.md - python3 -c 'import json, os; print("\n".join(json.loads(os.environ["PR_LABELS_JSON"])), end="")' > labels.txt + printf '%s' "$PR_TITLE" > "$evidence_dir/title.txt" + printf '%s' "$PR_BODY" > "$evidence_dir/body.md" + python3 -c 'import json, os; print("\n".join(json.loads(os.environ["PR_LABELS_JSON"])), end="")' > "$evidence_dir/labels.txt" + git fetch --no-tags origin "+refs/pull/${PR_NUMBER}/head:refs/remotes/pr/head" + # Fetch the event's immutable head too in case the PR changed again. + git fetch --no-tags origin "$PR_BASE_SHA" "$PR_HEAD_SHA" merge_base=$(git merge-base "$PR_BASE_SHA" "$PR_HEAD_SHA") - git diff --name-status --no-renames "$merge_base" "$PR_HEAD_SHA" > changes.txt + git diff --name-status --no-renames -z "$merge_base" "$PR_HEAD_SHA" > "$evidence_dir/changes.bin" + - name: Set up uv for the trusted checker + if: github.event_name == 'pull_request_target' + uses: astral-sh/setup-uv@v8.1.0 - name: Check fix evidence - if: github.event_name == 'pull_request' + if: github.event_name == 'pull_request_target' + env: + EVIDENCE_DIR: ${{ steps.evidence.outputs.directory }} + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | - python3 .github/check_fix_pr.py \ - --title-file title.txt --body-file body.md --labels-file labels.txt \ - --changes-file changes.txt --repo-root . \ + uv run --no-project --with pyyaml==6.0.2 python .github/check_fix_pr.py \ + --title-file "$EVIDENCE_DIR/title.txt" --body-file "$EVIDENCE_DIR/body.md" \ + --labels-file "$EVIDENCE_DIR/labels.txt" --changes-file "$EVIDENCE_DIR/changes.bin" \ + --repo-root . --head-sha "$PR_HEAD_SHA" \ --summary-file "$GITHUB_STEP_SUMMARY" diff --git a/changelog.d/fix-pr-evidence-check.added.md b/changelog.d/fix-pr-evidence-check.added.md index 1e6e48f12d..7b08b4ac57 100644 --- a/changelog.d/fix-pr-evidence-check.added.md +++ b/changelog.d/fix-pr-evidence-check.added.md @@ -1 +1 @@ -- Added a Fix evidence pull request check requiring bug fixes to change an executable test and name their reproducer, with an exemption for CI/docs-only fixes. +- Added a Fix evidence pull request check using a trusted base-branch checker to require bug fixes to name a collectable Python or executable YAML test from the PR's git objects, with an exemption only when model files and dependencies are unchanged. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 82849f1d6e..1fc5d89dae 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -56,29 +56,42 @@ types are patch. ## Fix evidence -The **Fix evidence** pull request check applies when a PR adds or modifies a -`changelog.d/*.fixed.md` fragment or has the `bug` or `bug-fix` label. A fix PR -must add or modify an executable test under `policyengine_uk/tests/` and name -its reproducer in the PR description: +The **Fix evidence** pull request check applies when a PR adds, modifies or +type-changes a fixed fragment directly in `changelog.d/`, or has the `bug` or +`bug-fix` label. A fragment counts as fixed when its basename has `fixed` in +any dot-separated part after the first, including `x.fixed.md`, `x.fixed.1.md`, +`x.fixed.md.md` and `x.fixed`. Use the canonical fragment naming convention +above when writing new fragments. A fix PR must add, modify or type-change an +executable test under `policyengine_uk/tests/` and name its reproducer in the +PR description: ```text Reproducer: policyengine_uk/tests/path/to/test_file.py::test_name ``` Accepted ids are pytest node ids (`file.py::test_name` or -`file.py::Class::test_name`) and YAML policy tests under +`file.py::TestClass::test_name`) and YAML policy tests under `policyengine_uk/tests/policy/` (`file.yaml` or `file.yaml::test name`, also -accepting `.yml`). Each named file must be added or modified in the PR, and -each named test must exist in that file. Python files must match `test_*.py` -or `*_test.py`. Separate multiple ids with commas or semicolons. Backticks -around each id are optional; use them when a name or reason contains these -separators. +accepting `.yml`). Each named file must be added, modified or type-changed in +the PR. Python files must match `test_*.py` or `*_test.py`, and names must +resolve to module-level functions starting with `test`, or methods starting +with `test` in module-level classes starting with `Test` and defining no +`__init__`. A module or class with `__test__ = False` is excluded, as are +definitions nested in functions or conditional blocks. YAML files must parse +to a non-empty list with at least one mapping containing a string `name` and +an `output` key; a named YAML reproducer must match such a mapping exactly. +Separate multiple ids with commas or semicolons. Backticks around each id +are optional; use them when a name or reason contains these separators. +Reproducer lines inside HTML comments are ignored. For CI/docs-only fixes, use `Reproducer: n/a: ` with a non-empty reason. This exempts the PR from changing a test only when no file under -`policyengine_uk/` outside `policyengine_uk/tests/` is added, modified or deleted. -The check validates the named reproducer at the PR head; it does not run the -reproducer against the old code. +`policyengine_uk/` outside `policyengine_uk/tests/` is added, modified, +type-changed or deleted, and neither `pyproject.toml` nor `uv.lock` changes. +Dependency changes can change model results, so they also require a test. +The workflow runs the trusted base-branch checker and reads test files from +the PR head's git objects, rejecting symlinked test paths. It validates the +named reproducer without running PR code or testing against the old code. ## Required Axiom Line diff --git a/policyengine_uk/tests/code_health/test_check_fix_pr.py b/policyengine_uk/tests/code_health/test_check_fix_pr.py index dbea16b3f8..de941e0cb6 100644 --- a/policyengine_uk/tests/code_health/test_check_fix_pr.py +++ b/policyengine_uk/tests/code_health/test_check_fix_pr.py @@ -3,25 +3,33 @@ The check runs without installing the model. These tests hold its rules to the invariants reviewers rely on: -1. Exactly an added or modified fixed fragment, or a bug/bug-fix label, - classifies a PR as a fix. +1. Exactly an added, modified or type-changed Towncrier fixed fragment, + or a bug/bug-fix label, classifies a PR as a fix. 2. Deleted tests never count as changed executable tests or reproducers. 3. Formatting reproducer ids with supported separators and quoting round trips through the body parser. -4. A pytest reproducer resolves exactly when its changed file defines the - named function or class method, ignoring parametrisation suffixes. +4. A pytest reproducer requires a changed file and a directly defined, + collectable test function or class method, ignoring parametrisation suffixes. 5. An n/a reason is accepted exactly when it is non-empty and no model - source outside the test tree was added, modified or deleted. + source outside the test tree or dependency file was added, modified, + type-changed or deleted. 6. Non-fix PRs pass regardless of their bodies. 7. The CLI returns 0/1/2 for passing/failing/unusable input and appends its Markdown report without replacing an existing job summary. -8. Real git diffs and script subprocesses obey the same rules for passing - and failing fix PRs. -9. The workflow reports the check on pull requests and on merge queue - groups, so a branch rule can require it with a merge queue. +8. NUL-delimited git diffs preserve literal paths, and git-object reads + validate committed content without following symlinks or executing PR code. + The standalone checker entry point obeys the same passing and failing rules. +9. The workflow runs the trusted base checker on pull_request_target events, + isolates artifacts under RUNNER_TEMP and reports on merge queue groups. +10. YAML reproducers require parsed list entries with a string name and output; + missing PyYAML fails closed. +11. Reproducer lines inside HTML comments never count as evidence. """ import importlib.util +import os +import re +import runpy import subprocess import sys from pathlib import Path @@ -62,11 +70,17 @@ st.text(alphabet="abcdefghijklmnopqrstuvwxyz-_", max_size=20), ) _change_path = st.one_of( - _identifier.map(lambda name: f"changelog.d/{name}.fixed.md"), + st.builds( + lambda name, suffix: f"changelog.d/{name}.{suffix}", + _identifier, + st.sampled_from(["fixed.md", "fixed.1.md", "fixed.md.md", "fixed"]), + ), st.sampled_from( [ "changelog.d/regression.added.md", "changelog.d/regression.changed.md", + "changelog.d/fixed.md", + "changelog.d/nested/regression.fixed.md", "docs/regression.fixed.md", "policyengine_uk/variables/regression.py", "policyengine_uk/parameters/regression.yaml", @@ -76,6 +90,8 @@ "policyengine_uk/tests2/test_regression.py", ".github/workflows/check.yaml", "docs/regression.md", + "pyproject.toml", + "uv.lock", ] ), ) @@ -123,9 +139,16 @@ def _write(repo_root, path, text): return destination -def _report(repo_root, body, changes, labels=("bug",)): +def _report(repo_root, body, changes, labels=("bug",), *, head_sha=None): return checker.check_fix_pr( - "Fix a regression", body, list(labels), changes, repo_root + "Fix a regression", body, list(labels), changes, repo_root, head_sha=head_sha + ) + + +def _fixed_fragment(path): + return ( + Path(path).parent == Path("changelog.d") + and "fixed" in Path(path).name.split(".")[1:] ) @@ -133,15 +156,26 @@ def _report(repo_root, body, changes, labels=("bug",)): @given(labels=st.sets(_label, max_size=10), changes=_changes) def test_classification_matches_only_fragments_and_labels(labels, changes): expected = bool({"bug", "bug-fix"} & labels) or any( - status in {"A", "M"} - and path.startswith("changelog.d/") - and path.endswith(".fixed.md") - for status, path in changes + status in {"A", "M", "T"} and _fixed_fragment(path) for status, path in changes ) for title, body in [("Fix a regression", "Reproducer: missing"), ("", "")]: assert checker.classify_fix_pr(title, body, list(labels), changes) is expected +@pytest.mark.parametrize("status", ["A", "M", "T"]) +@pytest.mark.parametrize( + "basename", ["x.fixed.md", "x.fixed.1.md", "x.fixed.md.md", "x.fixed"] +) +def test_towncrier_fixed_fragments_require_evidence(tmp_path, status, basename): + changes = [ + (status, f"changelog.d/{basename}"), + ("M", "policyengine_uk/variables/regression.py"), + ] + report = _report(tmp_path, "No evidence.", changes, labels=()) + assert report.is_fix + assert not report.passed + + @PROPERTY_SETTINGS @given(paths=st.lists(_executable_path, min_size=1, max_size=6, unique=True)) def test_deleted_tests_never_count_as_executable_or_reproducers(tmp_path, paths): @@ -152,7 +186,7 @@ def test_deleted_tests_never_count_as_executable_or_reproducers(tmp_path, paths) content = ( "def test_regression():\n pass\n" if path.endswith(".py") - else "- name: regression\n" + else "- name: regression\n output: {}\n" ) _write(tmp_path, path, content) # A separate changed test isolates reproducer eligibility from the @@ -225,7 +259,7 @@ def test_pytest_reproducer_requires_changed_file_and_definition( _write(tmp_path, path, source) report = _report(tmp_path, f"Reproducer: {nodeid}", [(status, path)]) - assert report.passed is (status in {"A", "M"} and defines_name) + assert report.passed is (status in {"A", "M", "T"} and defines_name) @PROPERTY_SETTINGS @@ -244,19 +278,39 @@ def test_pytest_reproducer_requires_changed_file_and_definition( reason="CI only", prefix="none: ", ) +@example( + changes=[("T", "policyengine_uk/variables/regression.py")], + reason="CI only", + prefix="n/a: ", +) +@example(changes=[("M", "pyproject.toml")], reason="CI only", prefix="n/a: ") +@example(changes=[("M", "uv.lock")], reason="CI only", prefix="n/a: ") def test_na_requires_reason_and_no_model_source_changes( tmp_path, changes, reason, prefix ): has_model_source = any( - status in {"A", "M", "D"} - and path.startswith("policyengine_uk/") - and not path.startswith("policyengine_uk/tests/") + status in {"A", "M", "T", "D"} + and ( + path in {"pyproject.toml", "uv.lock"} + or ( + path.startswith("policyengine_uk/") + and not path.startswith("policyengine_uk/tests/") + ) + ) for status, path in changes ) report = _report(tmp_path, f"Reproducer: {prefix}{reason}", changes) assert report.passed is (bool(reason.strip()) and not has_model_source) +@pytest.mark.parametrize("status", ["A", "M", "T", "D"]) +@pytest.mark.parametrize( + "path", ["policyengine_uk/variables/regression.py", "pyproject.toml", "uv.lock"] +) +def test_na_rejects_every_model_or_dependency_change(tmp_path, status, path): + assert not _report(tmp_path, "Reproducer: n/a: CI only", [(status, path)]).passed + + @PROPERTY_SETTINGS @given(title=st.text(max_size=80), body=st.text(max_size=300), changes=_changes) @example( @@ -268,7 +322,7 @@ def test_non_fix_pr_always_passes(tmp_path, title, body, changes): non_fix_changes = [ (status, path) for status, path in changes - if not (status in {"A", "M"} and path.endswith(".fixed.md")) + if not (status in {"A", "M", "T"} and _fixed_fragment(path)) ] report = checker.check_fix_pr( title, body, ["enhancement", "Bug"], non_fix_changes, tmp_path @@ -298,19 +352,18 @@ def test_non_fix_pr_always_passes(tmp_path, title, body, changes): def test_only_collected_test_paths_are_executable(path, executable): for status in ["A", "M", "D", "T", "U"]: assert checker.changed_executable_tests([(status, path)]) == ( - [path] if executable and status in {"A", "M"} else [] + [path] if executable and status in {"A", "M", "T"} else [] ) @pytest.mark.parametrize( "source", [ - "- name: Regression case\n period: 2026\n", - "name: Regression case\nperiod: 2026\n", - "- name: 'Regression case'\n", - '- name: "Regression case"\n', - "- period: 2026\n name: Regression case\n", - "- name: Regression case # An inline comment\n", + "- name: Regression case\n period: 2026\n input: {}\n output: {}\n", + "- name: 'Regression case'\n output: {}\n", + '- name: "Regression case"\n output: {}\n', + "- period: 2026\n name: Regression case\n output: {}\n", + "- name: Regression case # An inline comment\n output: {}\n", ], ) @pytest.mark.parametrize("extension", ["yaml", "yml"]) @@ -340,18 +393,72 @@ def test_quoted_yaml_names_resolve_without_treating_punctuation_as_ids( tmp_path, source, name ): path = "policyengine_uk/tests/policy/regression.yaml" - _write(tmp_path, path, source) + _write(tmp_path, path, source + " output: {}\n") assert _report(tmp_path, f"Reproducer: `{path}::{name}`", [("A", path)]).passed def test_nested_yaml_name_is_not_a_test_entry(tmp_path): path = "policyengine_uk/tests/policy/regression.yaml" - _write(tmp_path, path, "- name: Regression case\n input:\n name: Impostor\n") + _write( + tmp_path, + path, + "- name: Regression case\n input:\n name: Impostor\n output: {}\n", + ) changes = [("M", path)] assert _report(tmp_path, f"Reproducer: {path}::Regression case", changes).passed assert not _report(tmp_path, f"Reproducer: {path}::Impostor", changes).passed +@pytest.mark.parametrize( + "source", + [ + "[]\n", + "# No policy tests here.\n", + "- |\n name: Ghost\n", + "- name: Ghost\n period: 2026\n", + "name: Ghost\noutput: {}\n", + "- name: 123\n output: {}\n", + "- name: [invalid\n output: {}\n", + ], +) +@pytest.mark.parametrize("suffix", ["", "::Ghost"]) +def test_yaml_reproducer_rejects_non_executable_documents(tmp_path, source, suffix): + path = "policyengine_uk/tests/policy/fake.yaml" + _write(tmp_path, path, source) + assert not _report(tmp_path, f"Reproducer: {path}{suffix}", [("A", path)]).passed + + +def test_yaml_named_reproducer_requires_output_on_the_named_entry(tmp_path): + path = "policyengine_uk/tests/policy/regression.yaml" + _write( + tmp_path, + path, + "- name: Ghost\n period: 2026\n- name: Real\n output: {}\n", + ) + changes = [("A", path)] + assert _report(tmp_path, f"Reproducer: {path}", changes).passed + assert _report(tmp_path, f"Reproducer: {path}::Real", changes).passed + assert not _report(tmp_path, f"Reproducer: {path}::Ghost", changes).passed + + +def test_yaml_reproducer_fails_closed_without_pyyaml(tmp_path, monkeypatch): + path = "policyengine_uk/tests/policy/regression.yaml" + _write(tmp_path, path, "- name: Regression\n output: {}\n") + monkeypatch.setitem(sys.modules, "yaml", None) + report = _report(tmp_path, f"Reproducer: {path}::Regression", [("A", path)]) + assert not report.passed + assert any( + "cannot validate YAML tests without PyYAML" in message + for message in report.messages + ) + # Python evidence still works when the optional YAML parser is unavailable. + python_path = "policyengine_uk/tests/test_regression.py" + _write(tmp_path, python_path, "def test_regression():\n pass\n") + assert _report( + tmp_path, f"Reproducer: {python_path}::test_regression", [("A", python_path)] + ).passed + + def test_backticked_na_reason_can_include_commas_and_semicolons(tmp_path): assert _report(tmp_path, "Reproducer: `n/a: CI, docs; no model source`", []).passed @@ -364,10 +471,52 @@ def test_backticked_na_reason_can_include_commas_and_semicolons(tmp_path): ("def test_regression():\n pass\n", "::test_regression[p]::junk"), ("def test_regression(:\n pass\n", "::test_regression"), ("test_regression = True\n", "::test_regression"), + ("def helper():\n pass\n", "::helper"), + ( + "class Regression:\n def test_regression(self):\n pass\n", + "::Regression::test_regression", + ), + ( + "class TestRegression:\n def __init__(self):\n pass\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "__test__ = False\ndef test_regression():\n pass\n", + "::test_regression", + ), + ( + "def test_regression():\n pass\n__test__: bool = False\n", + "::test_regression", + ), + ( + "class TestRegression:\n __test__ = False\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "class TestRegression:\n __test__: bool = False\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), ( "def wrapper():\n def test_regression():\n pass\n", "::test_regression", ), + ( + "if False:\n def test_regression():\n pass\n", + "::test_regression", + ), + ( + "if True:\n class TestRegression:\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "class TestRegression:\n if True:\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), ( "class TestOther:\n def test_regression(self):\n pass\n", "::TestRegression::test_regression", @@ -386,6 +535,63 @@ def test_pytest_reproducer_rejects_bare_paths_and_nonexistent_definitions( assert not _report(tmp_path, f"Reproducer: {path}{suffix}", [("M", path)]).passed +def test_pytest_default_collection_confirms_function_and_class_names( + tmp_path, monkeypatch +): + path = "policyengine_uk/tests/test_regression.py" + _write( + tmp_path, + path, + "def testament():\n pass\n" + "def helper():\n pass\n" + "class Testimony:\n def testimony(self):\n pass\n" + "class Regression:\n def test_hidden(self):\n pass\n" + "class TestConstructor:\n def __init__(self):\n pass\n" + " def test_hidden(self):\n pass\n" + "class TestDisabled:\n __test__ = False\n" + " def test_hidden(self):\n pass\n", + ) + + class Collection: + def pytest_collection_finish(self, session): + self.nodeids = [item.nodeid for item in session.items] + + collection = Collection() + # Reuse pytest's interpreter so the oracle only pays the cost of collection. + with monkeypatch.context() as context: + context.chdir(tmp_path) + context.setenv("PYTEST_DISABLE_PLUGIN_AUTOLOAD", "1") + result = pytest.main( + [ + "-c", + os.devnull, + "--collect-only", + "--import-mode=importlib", + "--confcutdir", + str(tmp_path), + "-q", + "-p", + "no:cacheprovider", + path, + ], + plugins=[collection], + ) + assert result == pytest.ExitCode.OK + collected = {nodeid.split("::", 1)[1] for nodeid in collection.nodeids} + assert collected == {"testament", "Testimony::testimony"} + for suffix in [ + "testament", + "helper", + "Testimony::testimony", + "Regression::test_hidden", + "TestConstructor::test_hidden", + "TestDisabled::test_hidden", + ]: + nodeid = f"{path}::{suffix}" + report = _report(tmp_path, f"Reproducer: {nodeid}", [("A", path)]) + assert report.passed is (suffix in collected) + + def test_reproducer_file_must_exist_at_pr_head(tmp_path): path = "policyengine_uk/tests/test_regression.py" assert not _report( @@ -421,6 +627,18 @@ def test_every_reproducer_on_every_line_must_resolve(tmp_path): assert not _report(tmp_path, "Reproducer: n/a: CI only; missing", []).passed +def test_html_commented_reproducer_is_ignored(tmp_path): + path = "policyengine_uk/tests/test_regression.py" + _write(tmp_path, path, "def test_regression():\n pass\n") + commented = "\n" + body = commented + f"Reproducer: {path}::test_regression" + assert checker.parse_reproducers(body) == [f"{path}::test_regression"] + assert _report(tmp_path, body, [("A", path)]).passed + assert not _report( + tmp_path, f"", [("A", path)] + ).passed + + def test_report_explains_each_failed_requirement(tmp_path): report = _report(tmp_path, "A description without evidence.", []) assert not report.passed @@ -436,6 +654,7 @@ def _arguments( labels="bug\n", changes="", summary=None, + head_sha=None, ): values = { "title": "Fix a regression", @@ -448,32 +667,44 @@ def _arguments( path = _write(input_dir, f"{name}.txt", value) args.extend([f"--{name}-file", str(path)]) args.extend(["--repo-root", str(repo_root)]) + if head_sha is not None: + args.extend(["--head-sha", head_sha]) if summary is not None: args.extend(["--summary-file", str(summary)]) return args +def _standalone_checker(args, repo_root, monkeypatch): + # Execute the real trusted entry point without starting another interpreter. + with monkeypatch.context() as context: + context.chdir(repo_root) + context.setattr(sys, "argv", [str(CHECKER), *args]) + with pytest.raises(SystemExit) as exit: + runpy.run_path(str(CHECKER), run_name="__main__") + return exit.value.code + + @pytest.mark.parametrize( "body, labels, changes, expected", [ - ("Reproducer: n/a: CI only", "bug\n", "M\t.github/workflows/check.yaml\n", 0), + ("Reproducer: n/a: CI only", "bug\n", "M\0.github/workflows/check.yaml\0", 0), ( "Reproducer: policyengine_uk/tests/test_regression.py::test_regression", "bug-fix\n", - "M\tpolicyengine_uk/tests/test_regression.py\n", + "M\0policyengine_uk/tests/test_regression.py\0", 0, ), - ("No evidence.", "bug\n", "M\tpolicyengine_uk/variables/regression.py\n", 1), + ("No evidence.", "bug\n", "M\0policyengine_uk/variables/regression.py\0", 1), ( "Reproducer: invalid", "enhancement\n", - "D\tchangelog.d/regression.fixed.md\n", + "D\0changelog.d/regression.fixed.md\0", 0, ), ( "Reproducer: n/a: CI only", "bug\n", - "T\t.github/workflows/check.yaml\nQ\tdocs/regression.md\n", + "T\0.github/workflows/check.yaml\0Q\0docs/regression.md\0", 0, ), ], @@ -503,10 +734,14 @@ def test_main_returns_two_for_missing_input_files(tmp_path, capsys, missing): "changes", [ "M path/to/test.py\n", - "M\t\n", - "MM\tpath.py\n", - "R100\told.py\tnew.py\n", - "M\tfirst.py\tsecond.py\n", + "M\tpath.py\n", + "M\0\0", + "MM\0path.py\0", + "R100\0old.py\0new.py\0", + "M\0first.py\0second.py\0", + "M\0path.py", + "M\0../outside.py\0", + "M\0/absolute.py\0", ], ) def test_main_returns_two_for_unparseable_changes(tmp_path, capsys, changes): @@ -550,6 +785,14 @@ def test_main_appends_markdown_to_existing_summary(tmp_path): assert "Unusable input" in third +def _git_env(): + return { + **os.environ, + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + } + + def _git(repo_root, *args): result = subprocess.run( [ @@ -562,43 +805,82 @@ def _git(repo_root, *args): "commit.gpgsign=false", "-c", "core.hooksPath=/dev/null", + "-c", + "gc.auto=0", + "-c", + "maintenance.auto=false", *args, ], cwd=repo_root, capture_output=True, text=True, timeout=15, + env=_git_env(), ) assert result.returncode == 0, result.stdout + result.stderr - return result.stdout.strip() + # Stripping output would corrupt paths ending in whitespace in NUL diffs. + return result.stdout -def test_real_git_diff_and_script_subprocess(tmp_path): +def _commit(repo_root, message): + _git(repo_root, "add", ".") + _git(repo_root, "commit", "-m", message) + return _git(repo_root, "rev-parse", "HEAD").strip() + + +@pytest.fixture +def git_repo(tmp_path, monkeypatch): + monkeypatch.setenv("GIT_CONFIG_GLOBAL", os.devnull) + monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") repo_root = tmp_path / "repo" repo_root.mkdir() - _git(repo_root, "init", "--initial-branch=fixture") + _git(repo_root, "init", "--initial-branch=fixture", "--template=") + return repo_root + + +def test_nul_delimited_git_diff_preserves_literal_paths(git_repo): + _write(git_repo, "baseline.txt", "Before the change.\n") + base = _commit(git_repo, "Fixture baseline") + paths = ["docs/é\\303.md", "docs/tab\tnewline\ntrailing .md ", 'docs/"quoted".md'] + for path in paths: + _write(git_repo, path, "A documentation change.\n") + head = _commit(git_repo, "Fixture literal paths") + changes = _git( + git_repo, + "-c", + "core.quotePath=false", + "diff", + "--name-status", + "--no-renames", + "-z", + base, + head, + ) + assert set(checker.parse_changes(changes)) == {("A", path) for path in paths} + + +def test_real_git_diff_and_standalone_checker(tmp_path, git_repo, monkeypatch, capsys): + repo_root = git_repo python_path = "policyengine_uk/tests/test_regression.py" yaml_path = "policyengine_uk/tests/gov/regression.yaml" _write(repo_root, python_path, "def test_regression():\n pass\n") _write(repo_root, yaml_path, "- name: Regression\n period: 2025\n") - _git(repo_root, "add", ".") - _git(repo_root, "commit", "-m", "Fixture before the fix") - base = _git(repo_root, "rev-parse", "HEAD") + base = _commit(repo_root, "Fixture before the fix") _write(repo_root, python_path, "def test_regression():\n assert 1 + 1 == 2\n") _write(repo_root, yaml_path, "- name: Regression\n period: 2026\n") _write(repo_root, "changelog.d/regression.fixed.md", "- Fixed a regression.\n") - _git(repo_root, "add", ".") - _git(repo_root, "commit", "-m", "Fixture fix and its evidence") - head = _git(repo_root, "rev-parse", "HEAD") - merge_base = _git(repo_root, "merge-base", base, head) - changes = ( - _git(repo_root, "diff", "--name-status", "--no-renames", merge_base, head) - + "\n" + head = _commit(repo_root, "Fixture fix and its evidence") + merge_base = _git(repo_root, "merge-base", base, head).strip() + changes = _git( + repo_root, "diff", "--name-status", "--no-renames", "-z", merge_base, head ) - assert f"M\t{python_path}\n" in changes - assert f"M\t{yaml_path}\n" in changes - assert "A\tchangelog.d/regression.fixed.md\n" in changes + assert f"M\0{python_path}\0" in changes + assert f"M\0{yaml_path}\0" in changes + assert "A\0changelog.d/regression.fixed.md\0" in changes + + # The base checkout need not contain any of the PR's changed test files. + _git(repo_root, "checkout", "--detach", base) for nodeid, expected in [(f"{python_path}::test_regression", 0), (yaml_path, 1)]: args = _arguments( @@ -607,18 +889,97 @@ def test_real_git_diff_and_script_subprocess(tmp_path): body=f"Reproducer: {nodeid}", labels="", changes=changes, + head_sha=head, ) - result = subprocess.run( - [sys.executable, str(CHECKER), *args], - cwd=repo_root, - capture_output=True, - text=True, - timeout=15, - ) - assert result.returncode == expected, result.stdout + result.stderr - assert "Fix a regression" in result.stdout + code = _standalone_checker(args, repo_root, monkeypatch) + output = capsys.readouterr().out + assert code == expected, output + assert "Fix a regression" in output if expected: - assert "Reproducer:" in result.stdout + assert "Reproducer:" in output + + +@pytest.mark.parametrize("target_in_repository", [True, False]) +def test_git_object_mode_rejects_symlinked_test_paths( + tmp_path, git_repo, target_in_repository +): + path = "policyengine_uk/tests/test_fake.py" + target_root = git_repo if target_in_repository else tmp_path + target = _write(target_root, "test_real.py", "def test_regression():\n pass\n") + link = git_repo / path + link.parent.mkdir(parents=True) + link.symlink_to(target) + head = _commit(git_repo, "Fixture symlink reproducer") + report = _report( + git_repo, + f"Reproducer: {path}::test_regression", + [("A", path)], + head_sha=head, + ) + assert not report.passed + assert any("symlink" in message.lower() for message in report.messages) + + +def test_git_object_mode_ignores_fabricated_worktree_evidence( + tmp_path, git_repo, monkeypatch, capsys +): + path = "policyengine_uk/tests/test_fake.py" + _write(git_repo, path, "# No test exists in the committed file.\n") + _write(git_repo, ".github/check_fix_pr.py", "raise SystemExit(0)\n") + (git_repo / "body.md").symlink_to(path) + head = _commit(git_repo, "Fixture metadata symlink") + body = ( + f'"""\nReproducer: {path}::test_regression\n"""\n' + "def test_regression():\n pass\n" + ) + # This recreates the old metadata write so the blob must remain authoritative. + (git_repo / "body.md").write_text(body, encoding="utf-8") + args = _arguments( + tmp_path / "inputs", + git_repo, + body=body, + changes=f"A\0{path}\0A\0.github/check_fix_pr.py\0A\0body.md\0", + head_sha=head, + ) + code = _standalone_checker(args, git_repo, monkeypatch) + output = capsys.readouterr().out + assert code == 1, output + assert "does not define a collectable named test" in output + + +def test_git_object_mode_reads_head_without_importing_pr_code(git_repo): + path = "policyengine_uk/tests/test_regression.py" + marker = git_repo / "pr-code-executed" + _write( + git_repo, + path, + f"from pathlib import Path\nPath({str(marker)!r}).touch()\n" + "def test_regression():\n pass\n", + ) + _write(git_repo, ".github/check_fix_pr.py", "raise SystemExit(0)\n") + head = _commit(git_repo, "Fixture PR code") + (git_repo / path).unlink() + report = _report( + git_repo, + f"Reproducer: {path}::test_regression", + [("A", path)], + head_sha=head, + ) + assert report.passed + assert not marker.exists() + + +def test_git_object_mode_rejects_paths_missing_from_head(git_repo): + _write(git_repo, "baseline.txt", "A committed baseline.\n") + head = _commit(git_repo, "Fixture without test") + path = "policyengine_uk/tests/test_regression.py" + _write(git_repo, path, "def test_regression():\n pass\n") + assert not _report( + git_repo, + f"Reproducer: {path}::test_regression", + [("A", path)], + head_sha=head, + ).passed def test_the_workflow_reports_on_pull_requests_and_merge_groups(): @@ -631,8 +992,73 @@ def test_the_workflow_reports_on_pull_requests_and_merge_groups(): ) # PyYAML reads the key "on" as True. triggers = workflow.get("on", workflow.get(True)) - assert set(triggers) == {"pull_request", "merge_group"} - assert "paths" not in triggers["pull_request"] + assert set(triggers) == {"pull_request_target", "merge_group"} + assert "paths" not in triggers["pull_request_target"] + assert workflow["permissions"] == {"contents": "read"} [job] = workflow["jobs"].values() assert job["name"] == "Fix evidence" assert "if" not in job + assert job.get("permissions", workflow["permissions"]) == {"contents": "read"} + + steps = job["steps"] + [checkout] = [ + step for step in steps if step.get("uses", "").startswith("actions/checkout@") + ] + assert checkout["if"] == "github.event_name == 'pull_request_target'" + # The event's default checkout supplies the trusted base workflow and checker. + assert "ref" not in checkout.get("with", {}) + for step in steps: + if step.get("uses", "").startswith("actions/checkout@"): + reference = str(step.get("with", {}).get("ref", "")) + assert "github.event.pull_request.head.sha" not in reference + assert "github.event.pull_request.head.ref" not in reference + run = step.get("run", "") + assert not re.search(r"\b(?:pip(?:3)?\s+install|uv\s+sync|make)\b", run) + assert not re.search(r"\bgit\s+(?:checkout|switch|reset)\b", run) + + [setup_uv] = [ + step for step in steps if step.get("uses", "").startswith("astral-sh/setup-uv@") + ] + assert setup_uv["uses"] == "astral-sh/setup-uv@v8.1.0" + assert setup_uv["if"] == "github.event_name == 'pull_request_target'" + + [collect] = [step for step in steps if step.get("id") == "evidence"] + assert collect["if"] == "github.event_name == 'pull_request_target'" + assert collect["env"]["PR_NUMBER"] == "${{ github.event.pull_request.number }}" + collect_run = collect["run"] + assert 'mktemp -d "$RUNNER_TEMP/fix-evidence.XXXXXX"' in collect_run + assert "directory=%s\\n" in collect_run + assert '"$GITHUB_OUTPUT"' in collect_run + for filename in ["title.txt", "body.md", "labels.txt", "changes.bin"]: + assert f'> "$evidence_dir/{filename}"' in collect_run + assert ( + 'git fetch --no-tags origin "+refs/pull/${PR_NUMBER}/head:refs/remotes/pr/head"' + in collect_run + ) + assert 'git merge-base "$PR_BASE_SHA" "$PR_HEAD_SHA"' in collect_run + assert "git diff --name-status --no-renames -z" in collect_run + + [check] = [ + step for step in steps if ".github/check_fix_pr.py" in step.get("run", "") + ] + assert check["if"] == "github.event_name == 'pull_request_target'" + assert check.get("working-directory", ".") == "." + assert check["env"]["EVIDENCE_DIR"] == "${{ steps.evidence.outputs.directory }}" + check_run = check["run"] + assert ( + "uv run --no-project --with pyyaml==6.0.2 python .github/check_fix_pr.py" + in check_run + ) + for option, filename in [ + ("title", "title.txt"), + ("body", "body.md"), + ("labels", "labels.txt"), + ("changes", "changes.bin"), + ]: + assert f'--{option}-file "$EVIDENCE_DIR/{filename}"' in check_run + assert '--repo-root . --head-sha "$PR_HEAD_SHA"' in check_run + + [merge_group] = [ + step for step in steps if step.get("if") == "github.event_name == 'merge_group'" + ] + assert merge_group["run"].startswith("echo ") From 11059edbce9c800710ea6fe5d1530479adbc285c Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 21:35:23 -0400 Subject: [PATCH 3/6] Close the fix-evidence review's non-blocking findings From the GPT-6.1 Sol review of 4ddff4c59 (APPROVE, N1-N4): - N1: a named pytest reproducer must be a test pytest collects. A falsy __test__ of any value, an assigned name.__test__, a fixture, a class with __new__, and a class with any base but object or unittest.TestCase are rejected. The real-pytest oracle covers the new shapes. - N2: the reproducer must run and pass, so a skip or xfail mark on it, its class or its module's pytestmark is rejected. skipif stays allowed: its condition is usually a platform check that holds false in CI. Over the repository's tests, the check accepts 1,006 and rejects only the two strict xfails. - N3: the base checkout keeps no token (persist-credentials: false). - N4: a YAML reproducer entry needs a non-empty output mapping, as all 3,007 existing entries have. Co-Authored-By: Claude Opus 5.5 --- .github/check_fix_pr.py | 152 +++++++++++++--- .github/workflows/pr_fix_evidence.yaml | 2 + docs/engineering/skills/github-prs.md | 17 +- .../tests/code_health/test_check_fix_pr.py | 170 ++++++++++++++++-- 4 files changed, 294 insertions(+), 47 deletions(-) diff --git a/.github/check_fix_pr.py b/.github/check_fix_pr.py index 2c99a77da4..a4a276e23a 100644 --- a/.github/check_fix_pr.py +++ b/.github/check_fix_pr.py @@ -16,6 +16,15 @@ TESTS_PREFIX = "policyengine_uk/tests/" POLICY_PREFIX = TESTS_PREFIX + "policy/" +# pytest marks under which a named reproducer would not run, or would not need +# to pass. skipif is allowed: its condition is usually a platform check that +# holds false in CI. +NON_RUNNING_MARKS = frozenset({"skip", "xfail"}) +# pytest does not collect a test class that defines either. +CONSTRUCTORS = ("__init__", "__new__") +# Base classes a collectable test class may name; any other base could bring a +# constructor or __test__ = False with it. +TEST_CLASS_BASES = frozenset({"object", "unittest.TestCase", "TestCase"}) REPRODUCER_HINT = ( 'Use "Reproducer: policyengine_uk/tests/test_example.py::test_regression", ' '"Reproducer: policyengine_uk/tests/policy/example.yaml::Case name", ' @@ -180,14 +189,81 @@ def _assigns_name(node: ast.AST, name: str) -> bool: def _suppresses_tests(body: list[ast.stmt]) -> bool: + # pytest skips a class whose __test__ is falsy; anything but a literal True + # counts as switching collection off. return any( _assigns_name(node, "__test__") - and isinstance(node.value, ast.Constant) - and node.value.value is False + and not (isinstance(node.value, ast.Constant) and node.value.value is True) for node in body ) +def _dotted_name(node: ast.AST) -> str | None: + parts = [] + while isinstance(node, ast.Attribute): + parts.append(node.attr) + node = node.value + if not isinstance(node, ast.Name): + return None + parts.append(node.id) + return ".".join(reversed(parts)) + + +def _sets_test_attribute(body: list[ast.stmt], name: str) -> bool: + """Whether the body assigns name.__test__, which pytest reads on collection.""" + for node in body: + if isinstance(node, ast.Assign): + targets = node.targets + elif isinstance(node, (ast.AnnAssign, ast.AugAssign)): + targets = [node.target] + else: + continue + if any( + isinstance(target, ast.Attribute) + and target.attr == "__test__" + and _dotted_name(target.value) == name + for target in targets + ): + return True + return False + + +def _decorator_names(definition: ast.AST) -> list[str | None]: + return [ + _dotted_name(decorator.func if isinstance(decorator, ast.Call) else decorator) + for decorator in definition.decorator_list + ] + + +def _pytestmark_names(body: list[ast.stmt]) -> list[str | None]: + """The dotted names in a module's or class's pytestmark assignment.""" + names = [] + for node in body: + if _assigns_name(node, "pytestmark") and node.value is not None: + names.extend( + _dotted_name(part) + for part in ast.walk(node.value) + if isinstance(part, ast.Attribute) + ) + return names + + +def _not_run_problem(names: list[str | None]) -> str | None: + """Why marks or decorators would keep the named test from running, if so.""" + for name in names: + if name is None: + continue + parts = name.split(".") + if parts[-1] == "fixture": + return "the named test is a fixture, which pytest does not run as a test" + if parts[-1] in NON_RUNNING_MARKS and "mark" in parts[:-1]: + return ( + f"the named test is marked {parts[-1]}, so it would not run as a " + "reproducer" + ) + return None + + def _reference_problem( reproducer: str, tests: set[str], repo_root: Path, head_sha: str | None ) -> str | None: @@ -211,13 +287,14 @@ def _reference_problem( for entry in entries if isinstance(entry, dict) and isinstance(entry.get("name"), str) - and "output" in entry + and isinstance(entry.get("output"), dict) + and entry["output"] } if isinstance(entries, list) else set() ) if not names: - return "the YAML file has no test mapping with a string name and output" + return "the YAML file has no test mapping with a string name and a non-empty output mapping" if separator and name not in names: return f"the YAML file has no test entry named {name!r}" return None @@ -231,38 +308,57 @@ def _reference_problem( parts = name.split("::") tree = ast.parse(source, filename=path) if _suppresses_tests(tree.body): - return "pytest collection is disabled by module __test__ = False" + return "pytest collection is disabled by module __test__" + module_marks = _pytestmark_names(tree.body) definitions = (ast.FunctionDef, ast.AsyncFunctionDef) - if ( - len(parts) == 1 - and parts[0].startswith("test") - and any( - isinstance(node, definitions) and node.name == parts[0] - for node in tree.body - ) - ): - return None + # A later definition of the same name replaces an earlier one. + functions = [ + node + for node in tree.body + if isinstance(node, definitions) and node.name == parts[0] + ] + if len(parts) == 1 and parts[0].startswith("test") and functions: + if _sets_test_attribute(tree.body, parts[0]): + return "the named test's __test__ attribute is set after its definition" + return _not_run_problem(module_marks + _decorator_names(functions[-1])) + classes = [ + node + for node in tree.body + if isinstance(node, ast.ClassDef) and node.name == parts[0] + ] if ( len(parts) == 2 and parts[0].startswith("Test") and parts[1].startswith("test") - and any( - isinstance(node, ast.ClassDef) - and node.name == parts[0] - and not _suppresses_tests(node.body) + and classes + ): + cls = classes[-1] + methods = [ + node + for node in cls.body + if isinstance(node, definitions) and node.name == parts[1] + ] + # pytest does not collect a class with its own constructor or + # __test__ switched off, and a base class could bring either. + collectable = ( + not _suppresses_tests(cls.body) and not any( - (isinstance(method, definitions) and method.name == "__init__") - or _assigns_name(method, "__init__") - for method in node.body - ) - and any( - isinstance(method, definitions) and method.name == parts[1] - for method in node.body + (isinstance(node, definitions) and node.name in CONSTRUCTORS) + or any(_assigns_name(node, name) for name in CONSTRUCTORS) + for node in cls.body ) - for node in tree.body + and all(_dotted_name(base) in TEST_CLASS_BASES for base in cls.bases) + and not cls.keywords + and not _sets_test_attribute(tree.body, parts[0]) + and not _sets_test_attribute(cls.body, parts[1]) ) - ): - return None + if collectable and methods: + return _not_run_problem( + module_marks + + _decorator_names(cls) + + _pytestmark_names(cls.body) + + _decorator_names(methods[-1]) + ) return "the pytest file does not define a collectable named test" except ( OSError, diff --git a/.github/workflows/pr_fix_evidence.yaml b/.github/workflows/pr_fix_evidence.yaml index fa2d709ec2..af26b99cf2 100644 --- a/.github/workflows/pr_fix_evidence.yaml +++ b/.github/workflows/pr_fix_evidence.yaml @@ -38,6 +38,8 @@ jobs: uses: actions/checkout@v6 with: fetch-depth: 0 + # The repository is public, so the fetches below need no token. + persist-credentials: false - name: Collect pull request evidence id: evidence if: github.event_name == 'pull_request_target' diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 1fc5d89dae..db9e1a2fcf 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -75,11 +75,18 @@ Accepted ids are pytest node ids (`file.py::test_name` or accepting `.yml`). Each named file must be added, modified or type-changed in the PR. Python files must match `test_*.py` or `*_test.py`, and names must resolve to module-level functions starting with `test`, or methods starting -with `test` in module-level classes starting with `Test` and defining no -`__init__`. A module or class with `__test__ = False` is excluded, as are -definitions nested in functions or conditional blocks. YAML files must parse -to a non-empty list with at least one mapping containing a string `name` and -an `output` key; a named YAML reproducer must match such a mapping exactly. +with `test` in module-level classes starting with `Test`. Such a class may +derive only from `object` or `unittest.TestCase` and may not define +`__init__` or `__new__`. A module or class whose `__test__` is anything but +`True`, a test or class whose `__test__` attribute is set after it is defined, +a fixture, and definitions nested in functions or conditional blocks are +excluded. The reproducer must run and pass: a `skip` or `xfail` mark on it, +its class or its module (`pytestmark`) is rejected. A `skipif` condition must +hold false in CI, so the reproducer runs there. A test marked +`microsimulation` runs only where the dataset is available, which includes +merge-queue and main runs. YAML files must parse to a non-empty list with at +least one mapping containing a string `name` and a non-empty `output` mapping; +a named YAML reproducer must match such a mapping exactly. Separate multiple ids with commas or semicolons. Backticks around each id are optional; use them when a name or reason contains these separators. Reproducer lines inside HTML comments are ignored. diff --git a/policyengine_uk/tests/code_health/test_check_fix_pr.py b/policyengine_uk/tests/code_health/test_check_fix_pr.py index de941e0cb6..d1ec319130 100644 --- a/policyengine_uk/tests/code_health/test_check_fix_pr.py +++ b/policyengine_uk/tests/code_health/test_check_fix_pr.py @@ -186,7 +186,7 @@ def test_deleted_tests_never_count_as_executable_or_reproducers(tmp_path, paths) content = ( "def test_regression():\n pass\n" if path.endswith(".py") - else "- name: regression\n output: {}\n" + else "- name: regression\n output: {x: 1}\n" ) _write(tmp_path, path, content) # A separate changed test isolates reproducer eligibility from the @@ -359,11 +359,11 @@ def test_only_collected_test_paths_are_executable(path, executable): @pytest.mark.parametrize( "source", [ - "- name: Regression case\n period: 2026\n input: {}\n output: {}\n", - "- name: 'Regression case'\n output: {}\n", - '- name: "Regression case"\n output: {}\n', - "- period: 2026\n name: Regression case\n output: {}\n", - "- name: Regression case # An inline comment\n output: {}\n", + "- name: Regression case\n period: 2026\n input: {}\n output: {x: 1}\n", + "- name: 'Regression case'\n output: {x: 1}\n", + '- name: "Regression case"\n output: {x: 1}\n', + "- period: 2026\n name: Regression case\n output: {x: 1}\n", + "- name: Regression case # An inline comment\n output: {x: 1}\n", ], ) @pytest.mark.parametrize("extension", ["yaml", "yml"]) @@ -393,7 +393,7 @@ def test_quoted_yaml_names_resolve_without_treating_punctuation_as_ids( tmp_path, source, name ): path = "policyengine_uk/tests/policy/regression.yaml" - _write(tmp_path, path, source + " output: {}\n") + _write(tmp_path, path, source + " output: {x: 1}\n") assert _report(tmp_path, f"Reproducer: `{path}::{name}`", [("A", path)]).passed @@ -402,7 +402,7 @@ def test_nested_yaml_name_is_not_a_test_entry(tmp_path): _write( tmp_path, path, - "- name: Regression case\n input:\n name: Impostor\n output: {}\n", + "- name: Regression case\n input:\n name: Impostor\n output: {x: 1}\n", ) changes = [("M", path)] assert _report(tmp_path, f"Reproducer: {path}::Regression case", changes).passed @@ -416,9 +416,9 @@ def test_nested_yaml_name_is_not_a_test_entry(tmp_path): "# No policy tests here.\n", "- |\n name: Ghost\n", "- name: Ghost\n period: 2026\n", - "name: Ghost\noutput: {}\n", - "- name: 123\n output: {}\n", - "- name: [invalid\n output: {}\n", + "name: Ghost\noutput: {x: 1}\n", + "- name: 123\n output: {x: 1}\n", + "- name: [invalid\n output: {x: 1}\n", ], ) @pytest.mark.parametrize("suffix", ["", "::Ghost"]) @@ -428,12 +428,22 @@ def test_yaml_reproducer_rejects_non_executable_documents(tmp_path, source, suff assert not _report(tmp_path, f"Reproducer: {path}{suffix}", [("A", path)]).passed +@pytest.mark.parametrize("output", ["{}", "[]", "5", "null", "[x]"]) +def test_yaml_reproducer_needs_a_non_empty_output_mapping(tmp_path, output): + # An entry whose output is empty or not a mapping asserts nothing. + path = "policyengine_uk/tests/policy/regression.yaml" + _write(tmp_path, path, f"- name: Regression\n output: {output}\n") + changes = [("A", path)] + assert not _report(tmp_path, f"Reproducer: {path}", changes).passed + assert not _report(tmp_path, f"Reproducer: {path}::Regression", changes).passed + + def test_yaml_named_reproducer_requires_output_on_the_named_entry(tmp_path): path = "policyengine_uk/tests/policy/regression.yaml" _write( tmp_path, path, - "- name: Ghost\n period: 2026\n- name: Real\n output: {}\n", + "- name: Ghost\n period: 2026\n- name: Real\n output: {x: 1}\n", ) changes = [("A", path)] assert _report(tmp_path, f"Reproducer: {path}", changes).passed @@ -443,7 +453,7 @@ def test_yaml_named_reproducer_requires_output_on_the_named_entry(tmp_path): def test_yaml_reproducer_fails_closed_without_pyyaml(tmp_path, monkeypatch): path = "policyengine_uk/tests/policy/regression.yaml" - _write(tmp_path, path, "- name: Regression\n output: {}\n") + _write(tmp_path, path, "- name: Regression\n output: {x: 1}\n") monkeypatch.setitem(sys.modules, "yaml", None) report = _report(tmp_path, f"Reproducer: {path}::Regression", [("A", path)]) assert not report.passed @@ -525,6 +535,44 @@ def test_backticked_na_reason_can_include_commas_and_semicolons(tmp_path): "class TestRegression:\n def test_regression(self):\n pass\n", "::TestRegression::Nested::test_regression", ), + ( + "__test__ = 0\ndef test_regression():\n pass\n", + "::test_regression", + ), + ( + "class TestRegression:\n __test__ = None\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "def test_regression():\n pass\ntest_regression.__test__ = False\n", + "::test_regression", + ), + ( + "class TestRegression:\n def test_regression(self):\n pass\n" + "TestRegression.__test__ = False\n", + "::TestRegression::test_regression", + ), + ( + "class TestRegression:\n def __new__(cls):\n pass\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "class Base:\n def __init__(self):\n pass\n" + "class TestRegression(Base):\n def test_regression(self):\n" + " pass\n", + "::TestRegression::test_regression", + ), + ( + "import pytest\n@pytest.fixture\ndef test_regression():\n pass\n", + "::test_regression", + ), + ( + "def test_regression():\n pass\ndef test_regression(x):\n pass\n" + "test_regression.__test__ = False\n", + "::test_regression", + ), ], ) def test_pytest_reproducer_rejects_bare_paths_and_nonexistent_definitions( @@ -549,7 +597,15 @@ def test_pytest_default_collection_confirms_function_and_class_names( "class TestConstructor:\n def __init__(self):\n pass\n" " def test_hidden(self):\n pass\n" "class TestDisabled:\n __test__ = False\n" - " def test_hidden(self):\n pass\n", + " def test_hidden(self):\n pass\n" + "class TestFalsy:\n __test__ = 0\n" + " def test_hidden(self):\n pass\n" + "class TestAllocator:\n def __new__(cls):\n pass\n" + " def test_hidden(self):\n pass\n" + "class TestInherited(TestConstructor):\n" + " def test_hidden(self):\n pass\n" + "def test_switched_off():\n pass\n" + "test_switched_off.__test__ = False\n", ) class Collection: @@ -586,12 +642,96 @@ def pytest_collection_finish(self, session): "Regression::test_hidden", "TestConstructor::test_hidden", "TestDisabled::test_hidden", + "TestFalsy::test_hidden", + "TestAllocator::test_hidden", + "TestInherited::test_hidden", + "test_switched_off", ]: nodeid = f"{path}::{suffix}" report = _report(tmp_path, f"Reproducer: {nodeid}", [("A", path)]) assert report.passed is (suffix in collected) +@pytest.mark.parametrize( + "source, suffix", + [ + ( + "import pytest\n@pytest.mark.skip(reason='later')\n" + "def test_regression():\n pass\n", + "::test_regression", + ), + ( + "from pytest import mark\n@mark.xfail\ndef test_regression():\n pass\n", + "::test_regression", + ), + ( + "import pytest\npytestmark = [pytest.mark.skip]\n" + "def test_regression():\n pass\n", + "::test_regression", + ), + ( + "import pytest\n@pytest.mark.xfail(strict=True)\nclass TestRegression:\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "import pytest\nclass TestRegression:\n" + " pytestmark = pytest.mark.skip(reason='later')\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ], +) +def test_pytest_reproducer_must_not_be_skipped_or_expected_to_fail( + tmp_path, source, suffix +): + # pytest collects these, but a skipped or xfailed test reproduces nothing. + path = "policyengine_uk/tests/test_regression.py" + _write(tmp_path, path, source) + report = _report(tmp_path, f"Reproducer: {path}{suffix}", [("M", path)]) + assert not report.passed + assert any("so it would not run" in message for message in report.messages) + + +@pytest.mark.parametrize( + "source, suffix", + [ + ( + "from hypothesis import given, strategies as st\n" + "@given(st.integers())\ndef test_regression(x):\n pass\n", + "::test_regression", + ), + ( + "import pytest\n@pytest.mark.parametrize('x', [1, 2])\n" + "@pytest.mark.microsimulation\ndef test_regression(x):\n pass\n", + "::test_regression[1]", + ), + ( + "import unittest\nclass TestRegression(unittest.TestCase):\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "import sys, pytest\n" + "@pytest.mark.skipif(sys.platform == 'win32', reason='no FIFOs')\n" + "def test_regression():\n pass\n", + "::test_regression", + ), + ( + "class TestRegression(object):\n __test__ = True\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ], +) +def test_pytest_reproducer_accepts_marks_and_decorators_that_still_run( + tmp_path, source, suffix +): + path = "policyengine_uk/tests/test_regression.py" + _write(tmp_path, path, source) + assert _report(tmp_path, f"Reproducer: {path}{suffix}", [("M", path)]).passed + + def test_reproducer_file_must_exist_at_pr_head(tmp_path): path = "policyengine_uk/tests/test_regression.py" assert not _report( @@ -1007,6 +1147,8 @@ def test_the_workflow_reports_on_pull_requests_and_merge_groups(): assert checkout["if"] == "github.event_name == 'pull_request_target'" # The event's default checkout supplies the trusted base workflow and checker. assert "ref" not in checkout.get("with", {}) + # No token stays in .git/config while PR objects are fetched and read. + assert checkout["with"]["persist-credentials"] is False for step in steps: if step.get("uses", "").startswith("actions/checkout@"): reference = str(step.get("with", {}).get("ref", "")) From 2a6fc1b48a5c592bef8e309608d1119b28a1639c Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 22:06:57 -0400 Subject: [PATCH 4/6] Accept TestCase only from unittest; reject unittest's skip decorators From the GPT-6.1 Sol review of 11059edbc (APPROVE, non-blocking 5 and 6): - A bare TestCase base counts only when the module imports it from unittest; any other TestCase could bring its own constructor. - unittest's skip and expectedFailure are rejected like pytest's skip and xfail marks; skipIf, like skipif, stays allowed. - The docs say a reproducer must not skip itself at run time (pytest.skip, pytest.importorskip), which the check cannot see. - The test module's invariants 4 and 10 now state the collection and non-empty output rules. Correction to 11059edbc's message: over the repository's tests the check accepts 1,008 of 1,010 pytest tests (not 1,006), rejecting only the two strict xfails, and all 340 YAML files. That is unchanged here. Co-Authored-By: Claude Opus 5.5 --- .github/check_fix_pr.py | 36 ++++++++++++++---- docs/engineering/skills/github-prs.md | 12 +++--- .../tests/code_health/test_check_fix_pr.py | 38 +++++++++++++++++-- 3 files changed, 69 insertions(+), 17 deletions(-) diff --git a/.github/check_fix_pr.py b/.github/check_fix_pr.py index a4a276e23a..16b238127a 100644 --- a/.github/check_fix_pr.py +++ b/.github/check_fix_pr.py @@ -16,15 +16,16 @@ TESTS_PREFIX = "policyengine_uk/tests/" POLICY_PREFIX = TESTS_PREFIX + "policy/" -# pytest marks under which a named reproducer would not run, or would not need -# to pass. skipif is allowed: its condition is usually a platform check that -# holds false in CI. -NON_RUNNING_MARKS = frozenset({"skip", "xfail"}) +# pytest marks and unittest decorators under which a named reproducer would +# not run, or would not need to pass. skipif (and unittest's skipIf) is allowed: +# its condition is usually a platform check that holds false in CI. +NON_RUNNING_MARKS = frozenset({"skip", "xfail", "expectedFailure"}) # pytest does not collect a test class that defines either. CONSTRUCTORS = ("__init__", "__new__") # Base classes a collectable test class may name; any other base could bring a -# constructor or __test__ = False with it. -TEST_CLASS_BASES = frozenset({"object", "unittest.TestCase", "TestCase"}) +# constructor or __test__ = False with it. A bare TestCase counts only when the +# module imports it from unittest. +TEST_CLASS_BASES = frozenset({"object", "unittest.TestCase"}) REPRODUCER_HINT = ( 'Use "Reproducer: policyengine_uk/tests/test_example.py::test_regression", ' '"Reproducer: policyengine_uk/tests/policy/example.yaml::Case name", ' @@ -228,6 +229,18 @@ def _sets_test_attribute(body: list[ast.stmt], name: str) -> bool: return False +def _imports_unittest_testcase(body: list[ast.stmt]) -> bool: + return any( + isinstance(node, ast.ImportFrom) + and node.module == "unittest" + and any( + alias.name == "TestCase" and alias.asname in (None, "TestCase") + for alias in node.names + ) + for node in body + ) + + def _decorator_names(definition: ast.AST) -> list[str | None]: return [ _dotted_name(decorator.func if isinstance(decorator, ast.Call) else decorator) @@ -256,7 +269,7 @@ def _not_run_problem(names: list[str | None]) -> str | None: parts = name.split(".") if parts[-1] == "fixture": return "the named test is a fixture, which pytest does not run as a test" - if parts[-1] in NON_RUNNING_MARKS and "mark" in parts[:-1]: + if parts[-1] in NON_RUNNING_MARKS: return ( f"the named test is marked {parts[-1]}, so it would not run as a " "reproducer" @@ -347,7 +360,14 @@ def _reference_problem( or any(_assigns_name(node, name) for name in CONSTRUCTORS) for node in cls.body ) - and all(_dotted_name(base) in TEST_CLASS_BASES for base in cls.bases) + and all( + _dotted_name(base) in TEST_CLASS_BASES + or ( + _dotted_name(base) == "TestCase" + and _imports_unittest_testcase(tree.body) + ) + for base in cls.bases + ) and not cls.keywords and not _sets_test_attribute(tree.body, parts[0]) and not _sets_test_attribute(cls.body, parts[1]) diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index db9e1a2fcf..64bd401713 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -76,13 +76,15 @@ accepting `.yml`). Each named file must be added, modified or type-changed in the PR. Python files must match `test_*.py` or `*_test.py`, and names must resolve to module-level functions starting with `test`, or methods starting with `test` in module-level classes starting with `Test`. Such a class may -derive only from `object` or `unittest.TestCase` and may not define -`__init__` or `__new__`. A module or class whose `__test__` is anything but +derive only from `object` or `unittest.TestCase` (also as `TestCase` imported +from `unittest`) and may not define `__init__` or `__new__`. A module or class whose `__test__` is anything but `True`, a test or class whose `__test__` attribute is set after it is defined, a fixture, and definitions nested in functions or conditional blocks are -excluded. The reproducer must run and pass: a `skip` or `xfail` mark on it, -its class or its module (`pytestmark`) is rejected. A `skipif` condition must -hold false in CI, so the reproducer runs there. A test marked +excluded. The reproducer must run and pass: a `skip` or `xfail` mark, or +unittest's `skip` or `expectedFailure`, on it, its class or its module +(`pytestmark`) is rejected. A `skipif` condition must hold false in CI, so the +reproducer runs there, and the test must not skip itself at run time +(`pytest.skip`, `pytest.importorskip`), which the check cannot see. A test marked `microsimulation` runs only where the dataset is available, which includes merge-queue and main runs. YAML files must parse to a non-empty list with at least one mapping containing a string `name` and a non-empty `output` mapping; diff --git a/policyengine_uk/tests/code_health/test_check_fix_pr.py b/policyengine_uk/tests/code_health/test_check_fix_pr.py index d1ec319130..4d5db17c6b 100644 --- a/policyengine_uk/tests/code_health/test_check_fix_pr.py +++ b/policyengine_uk/tests/code_health/test_check_fix_pr.py @@ -8,8 +8,10 @@ 2. Deleted tests never count as changed executable tests or reproducers. 3. Formatting reproducer ids with supported separators and quoting round trips through the body parser. -4. A pytest reproducer requires a changed file and a directly defined, - collectable test function or class method, ignoring parametrisation suffixes. +4. A pytest reproducer requires a changed file and a directly defined test + function or class method that pytest collects, checked against real pytest + collection, ignoring parametrisation suffixes. A collected test marked to + skip or expected to fail is rejected too, since it reproduces nothing. 5. An n/a reason is accepted exactly when it is non-empty and no model source outside the test tree or dependency file was added, modified, type-changed or deleted. @@ -21,8 +23,8 @@ The standalone checker entry point obeys the same passing and failing rules. 9. The workflow runs the trusted base checker on pull_request_target events, isolates artifacts under RUNNER_TEMP and reports on merge queue groups. -10. YAML reproducers require parsed list entries with a string name and output; - missing PyYAML fails closed. +10. YAML reproducers require parsed list entries with a string name and a + non-empty output mapping; missing PyYAML fails closed. 11. Reproducer lines inside HTML comments never count as evidence. """ @@ -573,6 +575,11 @@ def test_backticked_na_reason_can_include_commas_and_semicolons(tmp_path): "test_regression.__test__ = False\n", "::test_regression", ), + ( + "from helpers import TestCase\nclass TestRegression(TestCase):\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), ], ) def test_pytest_reproducer_rejects_bare_paths_and_nonexistent_definitions( @@ -680,6 +687,18 @@ def pytest_collection_finish(self, session): " def test_regression(self):\n pass\n", "::TestRegression::test_regression", ), + ( + "import unittest\nclass TestRegression(unittest.TestCase):\n" + " @unittest.skip('later')\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "from unittest import TestCase, expectedFailure\n" + "class TestRegression(TestCase):\n @expectedFailure\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), ], ) def test_pytest_reproducer_must_not_be_skipped_or_expected_to_fail( @@ -717,6 +736,17 @@ def test_pytest_reproducer_must_not_be_skipped_or_expected_to_fail( "def test_regression():\n pass\n", "::test_regression", ), + ( + "from unittest import TestCase\nclass TestRegression(TestCase):\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "import unittest, sys\nclass TestRegression(unittest.TestCase):\n" + " @unittest.skipIf(sys.platform == 'win32', 'no FIFOs')\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), ( "class TestRegression(object):\n __test__ = True\n" " def test_regression(self):\n pass\n", From 354a1a7ed62102bc494755e41f43440ad79fa8cb Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 22:14:11 -0400 Subject: [PATCH 5/6] Read TestCase's last binding; take only marks from pytestmark From the Opus review of 2a6fc1b48 (APPROVE, non-blocking 2, 5, 7, 8): - A bare TestCase base counts as unittest's only when the module's last top-level binding of TestCase is an absolute import from unittest. A relative import or a later import or definition no longer passes. - pytestmark contributes only names with a mark part, so an argument such as config.skip in pytest.mark.skipif(config.skip, ...) is not taken for a skip mark. - The real-pytest oracle includes a unittest TestCase class. - The docs paragraph is reflowed and names unittest's skipIf/skipUnless. Each new case fails against 2a6fc1b48's checker. Over the repository the check still accepts 1,008 of 1,010 pytest tests and all 340 YAML files. Co-Authored-By: Claude Opus 5.5 --- .github/check_fix_pr.py | 36 +++++++++++++------ docs/engineering/skills/github-prs.md | 17 ++++----- .../tests/code_health/test_check_fix_pr.py | 25 +++++++++++-- 3 files changed, 57 insertions(+), 21 deletions(-) diff --git a/.github/check_fix_pr.py b/.github/check_fix_pr.py index 16b238127a..11bd7ecf6d 100644 --- a/.github/check_fix_pr.py +++ b/.github/check_fix_pr.py @@ -230,15 +230,23 @@ def _sets_test_attribute(body: list[ast.stmt], name: str) -> bool: def _imports_unittest_testcase(body: list[ast.stmt]) -> bool: - return any( - isinstance(node, ast.ImportFrom) - and node.module == "unittest" - and any( - alias.name == "TestCase" and alias.asname in (None, "TestCase") - for alias in node.names - ) - for node in body - ) + """Whether the module's last top-level binding of TestCase is unittest's.""" + from_unittest = False + for node in body: + if isinstance(node, (ast.Import, ast.ImportFrom)): + for alias in node.names: + if (alias.asname or alias.name) == "TestCase": + from_unittest = ( + isinstance(node, ast.ImportFrom) + and node.module == "unittest" + and node.level == 0 + and alias.name == "TestCase" + ) + elif isinstance(node, ast.ClassDef) and node.name == "TestCase": + from_unittest = False + elif _assigns_name(node, "TestCase"): + from_unittest = False + return from_unittest def _decorator_names(definition: ast.AST) -> list[str | None]: @@ -249,14 +257,20 @@ def _decorator_names(definition: ast.AST) -> list[str | None]: def _pytestmark_names(body: list[ast.stmt]) -> list[str | None]: - """The dotted names in a module's or class's pytestmark assignment.""" + """The pytest marks in a module's or class's pytestmark assignment. + + Only names with a mark part count, so an argument such as config.skip in + pytest.mark.skipif(config.skip, ...) is not taken for a mark. + """ names = [] for node in body: if _assigns_name(node, "pytestmark") and node.value is not None: names.extend( - _dotted_name(part) + name for part in ast.walk(node.value) if isinstance(part, ast.Attribute) + and (name := _dotted_name(part)) is not None + and "mark" in name.split(".")[:-1] ) return names diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 64bd401713..122e490a59 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -77,14 +77,15 @@ the PR. Python files must match `test_*.py` or `*_test.py`, and names must resolve to module-level functions starting with `test`, or methods starting with `test` in module-level classes starting with `Test`. Such a class may derive only from `object` or `unittest.TestCase` (also as `TestCase` imported -from `unittest`) and may not define `__init__` or `__new__`. A module or class whose `__test__` is anything but -`True`, a test or class whose `__test__` attribute is set after it is defined, -a fixture, and definitions nested in functions or conditional blocks are -excluded. The reproducer must run and pass: a `skip` or `xfail` mark, or -unittest's `skip` or `expectedFailure`, on it, its class or its module -(`pytestmark`) is rejected. A `skipif` condition must hold false in CI, so the -reproducer runs there, and the test must not skip itself at run time -(`pytest.skip`, `pytest.importorskip`), which the check cannot see. A test marked +from `unittest`) and may not define `__init__` or `__new__`. A module or class +whose `__test__` is anything but `True`, a test or class whose `__test__` +attribute is set after it is defined, a fixture, and definitions nested in +functions or conditional blocks are excluded. The reproducer must run and +pass: a `skip` or `xfail` mark, or unittest's `skip` or `expectedFailure`, on +it, its class or its module (`pytestmark`) is rejected. A `skipif` (or +unittest `skipIf`/`skipUnless`) condition must let the reproducer run in CI, +and the test must not skip itself at run time (`pytest.skip`, +`pytest.importorskip`), which the check cannot see. A test marked `microsimulation` runs only where the dataset is available, which includes merge-queue and main runs. YAML files must parse to a non-empty list with at least one mapping containing a string `name` and a non-empty `output` mapping; diff --git a/policyengine_uk/tests/code_health/test_check_fix_pr.py b/policyengine_uk/tests/code_health/test_check_fix_pr.py index 4d5db17c6b..a9ff3233b4 100644 --- a/policyengine_uk/tests/code_health/test_check_fix_pr.py +++ b/policyengine_uk/tests/code_health/test_check_fix_pr.py @@ -580,6 +580,17 @@ def test_backticked_na_reason_can_include_commas_and_semicolons(tmp_path): " def test_regression(self):\n pass\n", "::TestRegression::test_regression", ), + ( + "from .unittest import TestCase\nclass TestRegression(TestCase):\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "from unittest import TestCase\nfrom helpers import TestCase\n" + "class TestRegression(TestCase):\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), ], ) def test_pytest_reproducer_rejects_bare_paths_and_nonexistent_definitions( @@ -612,7 +623,10 @@ def test_pytest_default_collection_confirms_function_and_class_names( "class TestInherited(TestConstructor):\n" " def test_hidden(self):\n pass\n" "def test_switched_off():\n pass\n" - "test_switched_off.__test__ = False\n", + "test_switched_off.__test__ = False\n" + "from unittest import TestCase\n" + "class TestUnittest(TestCase):\n" + " def test_case(self):\n pass\n", ) class Collection: @@ -641,7 +655,7 @@ def pytest_collection_finish(self, session): ) assert result == pytest.ExitCode.OK collected = {nodeid.split("::", 1)[1] for nodeid in collection.nodeids} - assert collected == {"testament", "Testimony::testimony"} + assert collected == {"testament", "Testimony::testimony", "TestUnittest::test_case"} for suffix in [ "testament", "helper", @@ -653,6 +667,7 @@ def pytest_collection_finish(self, session): "TestAllocator::test_hidden", "TestInherited::test_hidden", "test_switched_off", + "TestUnittest::test_case", ]: nodeid = f"{path}::{suffix}" report = _report(tmp_path, f"Reproducer: {nodeid}", [("A", path)]) @@ -741,6 +756,12 @@ def test_pytest_reproducer_must_not_be_skipped_or_expected_to_fail( " def test_regression(self):\n pass\n", "::TestRegression::test_regression", ), + ( + "import pytest, config\n" + "pytestmark = pytest.mark.skipif(config.skip, reason='optional')\n" + "def test_regression():\n pass\n", + "::test_regression", + ), ( "import unittest, sys\nclass TestRegression(unittest.TestCase):\n" " @unittest.skipIf(sys.platform == 'win32', 'no FIFOs')\n" From 3de2f81a01611b829a2f87fbce5d868fa884e082 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 22:24:20 -0400 Subject: [PATCH 6/6] Catch aliased marks in pytestmark; read TestCase as bound at the class From the Opus review of 354a1a7ed (APPROVE; findings 2-5, 8), adopting its prototype: - pytestmark is walked without descending into call arguments, instead of requiring a mark part in each name. An aliased mark such as m.skip or m.xfail counts again (354a1a7ed had let it through), and config.skip in pytest.mark.skipif(config.skip, ...) still does not. - A bare TestCase base is unittest's only when its last binding before the class is an absolute import from unittest. Any star import or any other binding of TestCase, including inside a block or a tuple assignment, resets it; a rebinding after the class no longer matters. - The docs say the check reads tests statically: it catches mistakes, not deliberate evasion, and reviewers still read the test. The seven new cases fail against 354a1a7ed's checker and pass here. Over the repository the check still accepts 1,008 of 1,010 pytest tests and all 340 YAML files. Co-Authored-By: Claude Opus 5.5 --- .github/check_fix_pr.py | 72 ++++++++++++------- docs/engineering/skills/github-prs.md | 3 +- .../tests/code_health/test_check_fix_pr.py | 43 +++++++++++ 3 files changed, 93 insertions(+), 25 deletions(-) diff --git a/.github/check_fix_pr.py b/.github/check_fix_pr.py index 11bd7ecf6d..9c9452a38c 100644 --- a/.github/check_fix_pr.py +++ b/.github/check_fix_pr.py @@ -23,8 +23,8 @@ # pytest does not collect a test class that defines either. CONSTRUCTORS = ("__init__", "__new__") # Base classes a collectable test class may name; any other base could bring a -# constructor or __test__ = False with it. A bare TestCase counts only when the -# module imports it from unittest. +# constructor or __test__ = False with it. A bare TestCase counts only when its +# last binding before the class is an absolute import from unittest. TEST_CLASS_BASES = frozenset({"object", "unittest.TestCase"}) REPRODUCER_HINT = ( 'Use "Reproducer: policyengine_uk/tests/test_example.py::test_regression", ' @@ -229,22 +229,38 @@ def _sets_test_attribute(body: list[ast.stmt], name: str) -> bool: return False +def _binds_name(node: ast.AST, name: str) -> bool: + """Whether a statement could bind name, including inside compound blocks.""" + for part in ast.walk(node): + if isinstance(part, ast.Name) and isinstance(part.ctx, ast.Store): + if part.id == name: + return True + elif isinstance(part, (ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)): + if part.name == name: + return True + elif isinstance(part, (ast.Import, ast.ImportFrom)): + if any( + alias.name == "*" or (alias.asname or alias.name.split(".")[0]) == name + for alias in part.names + ): + return True + return False + + def _imports_unittest_testcase(body: list[ast.stmt]) -> bool: - """Whether the module's last top-level binding of TestCase is unittest's.""" + """Whether the last top-level binding of TestCase in body is unittest's.""" from_unittest = False for node in body: - if isinstance(node, (ast.Import, ast.ImportFrom)): + if ( + isinstance(node, ast.ImportFrom) + and node.module == "unittest" + and node.level == 0 + and all(alias.name != "*" for alias in node.names) + ): for alias in node.names: if (alias.asname or alias.name) == "TestCase": - from_unittest = ( - isinstance(node, ast.ImportFrom) - and node.module == "unittest" - and node.level == 0 - and alias.name == "TestCase" - ) - elif isinstance(node, ast.ClassDef) and node.name == "TestCase": - from_unittest = False - elif _assigns_name(node, "TestCase"): + from_unittest = alias.name == "TestCase" + elif _binds_name(node, "TestCase"): from_unittest = False return from_unittest @@ -256,22 +272,28 @@ def _decorator_names(definition: ast.AST) -> list[str | None]: ] +def _mark_attributes(node: ast.AST): + """Attribute nodes in a pytestmark value, leaving out call arguments.""" + if isinstance(node, ast.Call): + yield from _mark_attributes(node.func) + return + if isinstance(node, ast.Attribute): + yield node + for child in ast.iter_child_nodes(node): + yield from _mark_attributes(child) + + def _pytestmark_names(body: list[ast.stmt]) -> list[str | None]: - """The pytest marks in a module's or class's pytestmark assignment. + """The marks in a module's or class's pytestmark assignment. - Only names with a mark part count, so an argument such as config.skip in - pytest.mark.skipif(config.skip, ...) is not taken for a mark. + Call arguments are left out, so config.skip in + pytest.mark.skipif(config.skip, ...) is not taken for a mark, while an + aliased mark such as m.skip still counts. """ names = [] for node in body: if _assigns_name(node, "pytestmark") and node.value is not None: - names.extend( - name - for part in ast.walk(node.value) - if isinstance(part, ast.Attribute) - and (name := _dotted_name(part)) is not None - and "mark" in name.split(".")[:-1] - ) + names.extend(_dotted_name(part) for part in _mark_attributes(node.value)) return names @@ -378,7 +400,9 @@ def _reference_problem( _dotted_name(base) in TEST_CLASS_BASES or ( _dotted_name(base) == "TestCase" - and _imports_unittest_testcase(tree.body) + and _imports_unittest_testcase( + tree.body[: tree.body.index(cls)] + ) ) for base in cls.bases ) diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 122e490a59..ecc892b694 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -87,7 +87,8 @@ unittest `skipIf`/`skipUnless`) condition must let the reproducer run in CI, and the test must not skip itself at run time (`pytest.skip`, `pytest.importorskip`), which the check cannot see. A test marked `microsimulation` runs only where the dataset is available, which includes -merge-queue and main runs. YAML files must parse to a non-empty list with at +merge-queue and main runs. The check reads test files statically, so it +catches mistakes, not deliberate evasion; reviewers still read the test. YAML files must parse to a non-empty list with at least one mapping containing a string `name` and a non-empty `output` mapping; a named YAML reproducer must match such a mapping exactly. Separate multiple ids with commas or semicolons. Backticks around each id diff --git a/policyengine_uk/tests/code_health/test_check_fix_pr.py b/policyengine_uk/tests/code_health/test_check_fix_pr.py index a9ff3233b4..986767f1d1 100644 --- a/policyengine_uk/tests/code_health/test_check_fix_pr.py +++ b/policyengine_uk/tests/code_health/test_check_fix_pr.py @@ -591,6 +591,32 @@ def test_backticked_na_reason_can_include_commas_and_semicolons(tmp_path): " def test_regression(self):\n pass\n", "::TestRegression::test_regression", ), + ( + "from helpers import TestCase\nclass TestRegression(TestCase):\n" + " def test_regression(self):\n pass\n" + "from unittest import TestCase\n", + "::TestRegression::test_regression", + ), + ( + "from unittest import TestCase\nfrom helpers import *\n" + "class TestRegression(TestCase):\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "from unittest import TestCase\nif True:\n" + " from helpers import TestCase\n" + "class TestRegression(TestCase):\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), + ( + "import helpers\nfrom unittest import TestCase\n" + "TestCase, _ = helpers.TestCase, 0\n" + "class TestRegression(TestCase):\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), ], ) def test_pytest_reproducer_rejects_bare_paths_and_nonexistent_definitions( @@ -714,6 +740,17 @@ def pytest_collection_finish(self, session): " def test_regression(self):\n pass\n", "::TestRegression::test_regression", ), + ( + "from pytest import mark as m\npytestmark = m.skip\n" + "def test_regression():\n pass\n", + "::test_regression", + ), + ( + "import pytest\nm = pytest.mark\nclass TestRegression:\n" + " pytestmark = [m.xfail]\n" + " def test_regression(self):\n pass\n", + "::TestRegression::test_regression", + ), ], ) def test_pytest_reproducer_must_not_be_skipped_or_expected_to_fail( @@ -756,6 +793,12 @@ def test_pytest_reproducer_must_not_be_skipped_or_expected_to_fail( " def test_regression(self):\n pass\n", "::TestRegression::test_regression", ), + ( + "from unittest import TestCase\nclass TestRegression(TestCase):\n" + " def test_regression(self):\n pass\n" + "from helpers import TestCase\n", + "::TestRegression::test_regression", + ), ( "import pytest, config\n" "pytestmark = pytest.mark.skipif(config.skip, reason='optional')\n"