From 7ddeda812ec233a85d5783223360d6fb869b769a Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 14:42:25 +0400 Subject: [PATCH 1/8] Test the PR description check and the bundle PR's Fixes line (failing) Port the simulation API's tests for check_pr_description.py and pr-description.yml (rule api.process.pr-tracking-issue), with four event payload fixtures: bundle-update PR #4169 without a Fixes line and fixed, PR #3776 with its Fixes line not first, and a description naming an issue in another repository. Test that the bundle-update script opens or reuses a tracking issue and puts Fixes #N first in the PR it opens. The check script and the workflow don't exist yet, and the bundle-update script writes no Fixes line, so all 59 cases fail. Co-Authored-By: Claude Opus 5.5 --- .../bad_event.json | 13 + .../bad_fixes_not_first_event.json | 13 + .../bad_other_repository_event.json | 13 + .../good_event.json | 13 + tests/unit/test_bundle_update_pins.py | 76 ++- tests/unit/test_pr_description_check.py | 589 ++++++++++++++++++ 6 files changed, 716 insertions(+), 1 deletion(-) create mode 100644 rules/fixtures/api.process.pr-tracking-issue/bad_event.json create mode 100644 rules/fixtures/api.process.pr-tracking-issue/bad_fixes_not_first_event.json create mode 100644 rules/fixtures/api.process.pr-tracking-issue/bad_other_repository_event.json create mode 100644 rules/fixtures/api.process.pr-tracking-issue/good_event.json create mode 100644 tests/unit/test_pr_description_check.py diff --git a/rules/fixtures/api.process.pr-tracking-issue/bad_event.json b/rules/fixtures/api.process.pr-tracking-issue/bad_event.json new file mode 100644 index 000000000..741d83476 --- /dev/null +++ b/rules/fixtures/api.process.pr-tracking-issue/bad_event.json @@ -0,0 +1,13 @@ +{ + "action": "opened", + "number": 4169, + "pull_request": { + "number": 4169, + "state": "open", + "title": "Update PolicyEngine bundle to 6.13.0", + "body": "## Summary\n\nUpdate PolicyEngine .py bundle from 6.12.0 to 6.13.0.\n\n## Bundled versions\n\n- policyengine: 6.13.0\n- policyengine-core: 3.33.3\n- policyengine-us: 2.2.1\n- policyengine-uk: 2.151.0\n- spm-calculator: 1.0.0\n\n## Certified data releases\n\n- US: populace-us-2024-spm-20260915\n- UK: microcosm-uk-2024-25-national-20261025T091414Z-a6299504\n\n---\nGenerated automatically by GitHub Actions" + }, + "repository": { + "full_name": "PolicyEngine/policyengine-api" + } +} diff --git a/rules/fixtures/api.process.pr-tracking-issue/bad_fixes_not_first_event.json b/rules/fixtures/api.process.pr-tracking-issue/bad_fixes_not_first_event.json new file mode 100644 index 000000000..9c39c9fe1 --- /dev/null +++ b/rules/fixtures/api.process.pr-tracking-issue/bad_fixes_not_first_event.json @@ -0,0 +1,13 @@ +{ + "action": "opened", + "number": 3776, + "pull_request": { + "number": 3776, + "state": "open", + "title": "Route API v1 simulations through the Stage 5 entrypoint", + "body": "## Summary\n\n- introduce a generic `SimulationAPIClient` and retain compatibility aliases for the former Modal-specific client;\n- keep the old Modal gateway URL and the new Cloud Run Simulation Entrypoint URL independently configured.\n\n## Validation\n\n- Unit tests for the client and the deployment configuration.\n\nFixes #3775\n" + }, + "repository": { + "full_name": "PolicyEngine/policyengine-api" + } +} diff --git a/rules/fixtures/api.process.pr-tracking-issue/bad_other_repository_event.json b/rules/fixtures/api.process.pr-tracking-issue/bad_other_repository_event.json new file mode 100644 index 000000000..1f27b0dac --- /dev/null +++ b/rules/fixtures/api.process.pr-tracking-issue/bad_other_repository_event.json @@ -0,0 +1,13 @@ +{ + "action": "opened", + "number": 4174, + "pull_request": { + "number": 4174, + "state": "open", + "title": "Check that every PR description starts with Fixes #N for an open issue", + "body": "Fixes PolicyEngine/policyengine-sim-api#862\n\n## Summary\n\n- Port the simulation API's PR description check.\n" + }, + "repository": { + "full_name": "PolicyEngine/policyengine-api" + } +} diff --git a/rules/fixtures/api.process.pr-tracking-issue/good_event.json b/rules/fixtures/api.process.pr-tracking-issue/good_event.json new file mode 100644 index 000000000..593f62ed8 --- /dev/null +++ b/rules/fixtures/api.process.pr-tracking-issue/good_event.json @@ -0,0 +1,13 @@ +{ + "action": "opened", + "number": 4169, + "pull_request": { + "number": 4169, + "state": "open", + "title": "Update PolicyEngine bundle to 6.13.0", + "body": "Fixes #4170\n\n## Summary\n\nUpdate PolicyEngine .py bundle from 6.12.0 to 6.13.0.\n\n## Bundled versions\n\n- policyengine: 6.13.0\n- policyengine-core: 3.33.3\n- policyengine-us: 2.2.1\n- policyengine-uk: 2.151.0\n- spm-calculator: 1.0.0\n\n## Certified data releases\n\n- US: populace-us-2024-spm-20260915\n- UK: microcosm-uk-2024-25-national-20261025T091414Z-a6299504\n\n---\nGenerated automatically by GitHub Actions" + }, + "repository": { + "full_name": "PolicyEngine/policyengine-api" + } +} diff --git a/tests/unit/test_bundle_update_pins.py b/tests/unit/test_bundle_update_pins.py index 0f1cfd990..e07979403 100644 --- a/tests/unit/test_bundle_update_pins.py +++ b/tests/unit/test_bundle_update_pins.py @@ -38,12 +38,18 @@ def update_checkout(tmp_path): with open(os.environ["BUNDLE_TEST_CALLS"], "a") as stream: stream.write(json.dumps([name, *args]) + "\\n") if name == "git" and args[:1] == ["ls-remote"]: - sys.exit(2) + sys.exit(0 if os.environ.get("BUNDLE_TEST_REMOTE_BRANCH") else 2) if name == "gh" and args[:2] == ["pr", "view"]: if os.environ.get("BUNDLE_TEST_OPEN_PR"): print(os.environ["BUNDLE_TEST_OPEN_PR"]) sys.exit(0) sys.exit(1) +if name == "gh" and args[:2] == ["issue", "list"]: + if os.environ.get("BUNDLE_TEST_ISSUE_LIST_FAIL"): + sys.exit(1) + print(os.environ.get("BUNDLE_TEST_OPEN_ISSUE", "")) +if name == "gh" and args[:2] == ["issue", "create"]: + print("https://github.com/PolicyEngine/policyengine-api/issues/4321") if name == "uv" and args[:2] == ["lock", "--upgrade-package"]: if os.environ.get("BUNDLE_TEST_LOCK_FAIL"): sys.exit(1) @@ -197,3 +203,71 @@ def test_existing_pull_request_is_found_by_exact_branch(update_checkout): 'select(.state == "OPEN") | .number', ] in calls assert not any(call[:1] == ["uv"] for call in calls) + + +# --- The tracking issue (rule api.process.pr-tracking-issue) --------------- + +ISSUE_TITLE = f"Update PolicyEngine bundle to {TARGET_VERSION}" + + +def _calls_to(calls, *command): + return [call for call in calls if call[:3] == ["gh", *command]] + + +def _pull_request_description(calls): + """Return the description of the one pull request the script opened.""" + (pull_request,) = _calls_to(calls, "pr", "create") + return pull_request[pull_request.index("--body") + 1] + + +def test_a_new_pull_request_names_a_new_tracking_issue_first(update_checkout): + result, calls = run_update(update_checkout) + + assert result.returncode == 0, result.stderr + (issue_lookup,) = _calls_to(calls, "issue", "list") + assert issue_lookup[issue_lookup.index("--repo") + 1] == ( + "PolicyEngine/policyengine-api" + ) + (issue,) = _calls_to(calls, "issue", "create") + assert issue[issue.index("--repo") + 1] == "PolicyEngine/policyengine-api" + assert issue[issue.index("--title") + 1] == ISSUE_TITLE + description = _pull_request_description(calls) + assert description.split("\n")[:3] == ["Fixes #4321", "", "## Summary"] + assert calls.index(issue) < calls.index(_calls_to(calls, "pr", "create")[0]) + + +def test_a_new_pull_request_reuses_the_open_tracking_issue(update_checkout): + result, calls = run_update(update_checkout, BUNDLE_TEST_OPEN_ISSUE="4300") + + assert result.returncode == 0, result.stderr + assert _calls_to(calls, "issue", "create") == [] + assert _pull_request_description(calls).startswith("Fixes #4300\n\n## Summary") + + +def test_the_issue_lookup_matches_this_versions_title_only(update_checkout): + result, calls = run_update(update_checkout) + + assert result.returncode == 0, result.stderr + (issue_lookup,) = _calls_to(calls, "issue", "list") + assert issue_lookup[issue_lookup.index("--state") + 1] == "open" + query = issue_lookup[issue_lookup.index("--jq") + 1] + assert f'.title == "{ISSUE_TITLE}"' in query + + +def test_a_pull_request_for_an_existing_branch_names_its_tracking_issue_first( + update_checkout, +): + result, calls = run_update(update_checkout, BUNDLE_TEST_REMOTE_BRANCH="1") + + assert result.returncode == 0, result.stderr + assert "already exists without an open PR" in result.stdout + assert not any(call[:2] == ["git", "commit"] for call in calls) + assert _pull_request_description(calls).startswith("Fixes #4321\n\n## Summary") + + +def test_a_failed_issue_lookup_opens_no_issue_and_no_pull_request(update_checkout): + result, calls = run_update(update_checkout, BUNDLE_TEST_ISSUE_LIST_FAIL="1") + + assert result.returncode != 0 + assert _calls_to(calls, "issue", "create") == [] + assert _calls_to(calls, "pr", "create") == [] diff --git a/tests/unit/test_pr_description_check.py b/tests/unit/test_pr_description_check.py new file mode 100644 index 000000000..47446356f --- /dev/null +++ b/tests/unit/test_pr_description_check.py @@ -0,0 +1,589 @@ +"""Tests for .github/scripts/check_pr_description.py and pr-description.yml. + +Rule api.process.pr-tracking-issue: the first line of every pull request's +description is ``Fixes #ISSUE_NUMBER``, naming an open issue in this +repository. Tests of how the script reads a description load it in this +process; tests of the issue lookup run it as the workflow does, with a fake +``gh`` first on PATH, so no test reaches GitHub. +""" + +from __future__ import annotations + +import importlib.util +import json +import os +import subprocess +import sys +from pathlib import Path +from typing import TYPE_CHECKING + +import pytest +import yaml + +if TYPE_CHECKING: + from types import ModuleType + +REPO = Path(__file__).resolve().parents[2] +CHECK_SCRIPT = REPO / ".github" / "scripts" / "check_pr_description.py" +CHECK_WORKFLOW = REPO / ".github" / "workflows" / "pr-description.yml" +CHECK_JOB = "pr-description" +PROOF_FIXTURES = REPO / "rules" / "fixtures" / "api.process.pr-tracking-issue" +REPOSITORY = "PolicyEngine/policyengine-api" +VALID_BODY = "Fixes #4173\n\n## Summary\n\n- Adds a check.\n" + +# Records its arguments, prints FAKE_GH_STDOUT, and FAKE_GH_STDERR on stderr, +# then exits with FAKE_GH_EXIT, as gh api does with GitHub's answer. +FAKE_GH = f"""#!{sys.executable} +import json +import os +import sys + +with open(os.environ["FAKE_GH_CALLS"], "a", encoding="utf-8") as calls: + calls.write(json.dumps(["gh", *sys.argv[1:]]) + "\\n") +sys.stdout.write(os.environ.get("FAKE_GH_STDOUT", "")) +if os.environ.get("FAKE_GH_STDERR"): + sys.stderr.write(os.environ["FAKE_GH_STDERR"] + "\\n") +sys.exit(int(os.environ.get("FAKE_GH_EXIT", "0"))) +""" +# What gh api prints for a number GitHub has no issue or pull request for. +NOT_FOUND_STDOUT = ( + '{"message":"Not Found","documentation_url":' + '"https://docs.github.com/rest/issues/issues#get-an-issue","status":"404"}' +) +NOT_FOUND_STDERR = "gh: Not Found (HTTP 404)" + + +class FakeGh: + """A fake ``gh`` in its own directory, and the calls made to it.""" + + def __init__(self, directory: Path) -> None: + self.binaries = directory / "bin" + self.binaries.mkdir() + fake = self.binaries / "gh" + fake.write_text(FAKE_GH, encoding="utf-8") + fake.chmod(0o755) + self.calls_log = directory / "gh-calls.jsonl" + + def environment(self, **variables: str) -> dict[str, str]: + """Return an environment with the fake first on PATH and ``variables``. + + It leaves out the runner's variables, so each test sets the ones it + needs. + """ + environment = { + name: value + for name, value in os.environ.items() + if not name.startswith(("GITHUB_", "FAKE_GH_", "GH_")) + } + environment["PATH"] = f"{self.binaries}{os.pathsep}{os.environ['PATH']}" + environment["FAKE_GH_CALLS"] = str(self.calls_log) + environment.update(variables) + return environment + + def calls(self) -> list[list[str]]: + if not self.calls_log.exists(): + return [] + return [ + json.loads(call) + for call in self.calls_log.read_text(encoding="utf-8").splitlines() + ] + + +@pytest.fixture(scope="module") +def check_module() -> ModuleType: + """Load the check script as a module in this process.""" + spec = importlib.util.spec_from_file_location("check_pr_description", CHECK_SCRIPT) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def _rejection(check_module: ModuleType, body: str | None) -> str: + """Return the message the check gives for a description it rejects.""" + with pytest.raises(check_module.DescriptionError) as rejected: + check_module.issue_number_from_description(body) + return str(rejected.value) + + +# --- The first line ------------------------------------------------------- + + +@pytest.mark.parametrize( + "body", + [ + "Fixes #4173", + "Fixes #4173\n\n## Summary\n\n- Adds a check.\n", + ], +) +def test_a_first_line_of_fixes_and_a_number_names_the_issue(check_module, body): + assert check_module.issue_number_from_description(body) == 4173 + + +@pytest.mark.parametrize( + "body", + [ + # Descriptions edited in the GitHub web interface use CRLF. + "Fixes #4173\r\n\r\n## Summary\r\n", + # A carriage return on its own also ends a line in Markdown. + "Fixes #4173\r## Summary", + ], +) +def test_carriage_return_line_endings_end_the_first_line(check_module, body): + assert check_module.issue_number_from_description(body) == 4173 + + +def test_trailing_whitespace_after_the_number_is_ignored(check_module): + body = "Fixes #4173 \t\r\n\r\n## Summary\r\n" + + assert check_module.issue_number_from_description(body) == 4173 + + +def test_a_description_without_a_fixes_line_is_rejected(check_module): + message = _rejection(check_module, "## Summary\n\n- Adds a check.\n") + + assert 'The first line is "## Summary"' in message + assert "first line is exactly Fixes #ISSUE_NUMBER" in message + assert "api.process.pr-tracking-issue" in message + + +def test_a_fixes_line_that_is_not_first_is_rejected(check_module): + message = _rejection(check_module, "## Summary\n\nFixes #4173\n") + + assert "Fixes #4173 is on line 3" in message + assert "must be the first line" in message + + +def test_a_blank_line_before_the_fixes_line_is_rejected(check_module): + message = _rejection(check_module, "\r\nFixes #4173\r\n") + + assert "Fixes #4173 is on line 2" in message + + +def test_leading_whitespace_before_fixes_is_rejected(check_module): + # Four leading spaces would make the line a Markdown code block, where + # GitHub does not read closing keywords. + message = _rejection(check_module, " Fixes #4173\n") + + assert "starts with whitespace" in message + + +@pytest.mark.parametrize("body", [None, "", " \r\n\t\n"]) +def test_an_empty_or_missing_description_is_rejected(check_module, body): + message = _rejection(check_module, body) + + assert "The description is empty." in message + assert "first line is exactly Fixes #ISSUE_NUMBER" in message + + +@pytest.mark.parametrize( + "first_line", + [ + # From PR #3726. + "Fixes #3728.", + # From PR #3801. + "Closes #3800.", + # From PR #3798. + "Part of #3797", + # From PR #4166. + "Tracking issue: none yet.", + # The pull request template's first line, left unfilled. + "Fixes #", + "fixes #4173", + "Fixes 4173", + "Fixes #0", + "Fixes #04173", + "Fixes #4173 and #4174", + # An issue in another repository. + "Fixes PolicyEngine/policyengine-sim-api#862", + "Fixes https://github.com/PolicyEngine/policyengine-api/issues/4173", + # Digits from another script are not issue numbers. + "Fixes #٤١٧٣", + ], +) +def test_other_first_lines_are_rejected(check_module, first_line): + message = _rejection(check_module, f"{first_line}\n\n## Summary\n") + + assert f'The first line is "{first_line}"' in message + + +def test_a_long_first_line_is_shortened_in_the_message(check_module): + first_line = "Draft, stacked on #4162. " + "Gate releases on a check. " * 10 + + message = _rejection(check_module, first_line) + + assert 'The first line is "Draft, stacked on #4162.' in message + assert first_line not in message + + +# --- The issue lookup ----------------------------------------------------- + + +@pytest.fixture +def fake_gh(tmp_path: Path) -> FakeGh: + return FakeGh(tmp_path) + + +def _issue( + number: int = 4173, + *, + state: str = "open", + repository: str = REPOSITORY, + **fields: object, +) -> dict: + """Return the fields gh api prints for an issue, with ``fields`` added.""" + return { + "number": number, + "state": state, + "repository_url": f"https://api.github.com/repos/{repository}", + "html_url": f"https://github.com/{repository}/issues/{number}", + **fields, + } + + +def _write_event(tmp_path: Path, event: dict) -> Path: + event_path = tmp_path / "event.json" + event_path.write_text(json.dumps(event), encoding="utf-8") + return event_path + + +def _pull_request_event(body: str | None) -> dict: + return {"action": "edited", "number": 4174, "pull_request": {"body": body}} + + +def _run_check( + fake_gh: FakeGh, tmp_path: Path, body: str | None = VALID_BODY, **env: str +) -> subprocess.CompletedProcess[str]: + """Run the check on a pull request event whose description is ``body``.""" + event_path = _write_event(tmp_path, _pull_request_event(body)) + return _run_script( + fake_gh.environment( + GITHUB_EVENT_PATH=str(event_path), GITHUB_REPOSITORY=REPOSITORY, **env + ) + ) + + +def _run_script(env: dict[str, str]) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, str(CHECK_SCRIPT)], + capture_output=True, + text=True, + check=False, + env=env, + ) + + +def _gh_answers(issue: dict) -> dict[str, str]: + """Environment for the fake gh to print ``issue`` and succeed.""" + return {"FAKE_GH_STDOUT": json.dumps(issue)} + + +def _error_annotation(result: subprocess.CompletedProcess[str]) -> str: + """Return the one workflow error annotation the check printed.""" + annotations = [ + output_line + for output_line in result.stdout.splitlines() + if output_line.startswith("::error") + ] + assert len(annotations) == 1, result.stdout + return annotations[0] + + +def test_a_description_naming_an_open_issue_in_this_repository_passes( + fake_gh, tmp_path +): + result = _run_check(fake_gh, tmp_path, **_gh_answers(_issue())) + + assert result.returncode == 0, result.stdout + result.stderr + assert fake_gh.calls() == [ + ["gh", "api", "repos/PolicyEngine/policyengine-api/issues/4173"] + ] + assert result.stdout == ( + "The first line is Fixes #4173, and #4173 is an open issue in " + "PolicyEngine/policyengine-api.\n" + ) + + +def test_a_rejected_description_is_reported_without_asking_github(fake_gh, tmp_path): + result = _run_check(fake_gh, tmp_path, "## Summary\n\nFixes #4173\n") + + assert result.returncode == 1 + assert "Fixes #4173 is on line 3" in _error_annotation(result) + assert fake_gh.calls() == [] + + +def test_a_null_description_in_the_event_fails(fake_gh, tmp_path): + result = _run_check(fake_gh, tmp_path, None) + + assert result.returncode == 1 + assert "The description is empty." in _error_annotation(result) + assert fake_gh.calls() == [] + + +def test_a_crlf_description_from_the_event_passes(fake_gh, tmp_path): + result = _run_check( + fake_gh, tmp_path, "Fixes #4173\r\n\r\n## Summary\r\n", **_gh_answers(_issue()) + ) + + assert result.returncode == 0, result.stdout + result.stderr + + +def test_a_closed_issue_fails(fake_gh, tmp_path): + result = _run_check(fake_gh, tmp_path, **_gh_answers(_issue(state="closed"))) + + assert result.returncode == 1 + annotation = _error_annotation(result) + assert "Issue #4173 is closed." in annotation + assert "reopen it and re-run this job" in annotation + + +def test_a_pull_request_number_fails(fake_gh, tmp_path): + pull_request = _issue( + 4169, + pull_request={"url": f"https://api.github.com/repos/{REPOSITORY}/pulls/4169"}, + ) + + result = _run_check(fake_gh, tmp_path, "Fixes #4169\n", **_gh_answers(pull_request)) + + assert result.returncode == 1 + assert "#4169 is a pull request, not an issue." in _error_annotation(result) + + +@pytest.mark.parametrize( + "gh_error", + [NOT_FOUND_STDERR, "gh: This issue was deleted (HTTP 410)"], +) +def test_a_missing_issue_fails(fake_gh, tmp_path, gh_error): + result = _run_check( + fake_gh, + tmp_path, + "Fixes #999999\n", + FAKE_GH_STDOUT=NOT_FOUND_STDOUT, + FAKE_GH_STDERR=gh_error, + FAKE_GH_EXIT="1", + ) + + assert result.returncode == 1 + assert ( + "There is no issue #999999 in PolicyEngine/policyengine-api." + in _error_annotation(result) + ) + + +def test_an_issue_moved_to_another_repository_fails(fake_gh, tmp_path): + # gh api follows GitHub's redirect for an issue transferred to another + # repository and prints the moved issue, here #115 in policyengine.py. + moved_issue = _issue(115, state="open", repository="PolicyEngine/policyengine.py") + + result = _run_check(fake_gh, tmp_path, "Fixes #154\n", **_gh_answers(moved_issue)) + + assert result.returncode == 1 + assert ( + "#154 is not an issue in PolicyEngine/policyengine-api: it moved to " + "PolicyEngine/policyengine.py#115." + ) in _error_annotation(result) + + +def test_an_issue_in_a_repository_whose_name_differs_only_in_case_passes( + fake_gh, tmp_path +): + issue = _issue(repository="policyengine/PolicyEngine-API") + + result = _run_check(fake_gh, tmp_path, **_gh_answers(issue)) + + assert result.returncode == 0, result.stdout + result.stderr + + +@pytest.mark.parametrize( + "repository_url", + [None, "", "https://api.github.com/orgs/PolicyEngine", "not a URL"], +) +def test_an_answer_that_names_no_repository_fails(fake_gh, tmp_path, repository_url): + issue = _issue() + issue["repository_url"] = repository_url + + result = _run_check(fake_gh, tmp_path, **_gh_answers(issue)) + + assert result.returncode == 1 + assert "GitHub's answer for #4173 names no repository" in _error_annotation(result) + + +def test_a_failed_lookup_fails_with_what_gh_reported(fake_gh, tmp_path): + result = _run_check( + fake_gh, + tmp_path, + FAKE_GH_STDERR="gh: Bad credentials (HTTP 401)", + FAKE_GH_EXIT="1", + ) + + assert result.returncode == 1 + annotation = _error_annotation(result) + assert "Could not look up #4173 in PolicyEngine/policyengine-api" in annotation + assert "gh: Bad credentials (HTTP 401)" in annotation + assert "GH_TOKEN" in annotation + + +@pytest.mark.parametrize("gh_stdout", ["not JSON", "[]"]) +def test_an_answer_that_is_not_an_issue_object_fails(fake_gh, tmp_path, gh_stdout): + result = _run_check(fake_gh, tmp_path, FAKE_GH_STDOUT=gh_stdout) + + assert result.returncode == 1 + assert "gh api did not print an issue for #4173" in _error_annotation(result) + + +def test_messages_stay_on_one_annotation_line(fake_gh, tmp_path): + result = _run_check( + fake_gh, + tmp_path, + FAKE_GH_STDERR="first problem\nsecond problem 100%", + FAKE_GH_EXIT="1", + ) + + assert result.returncode == 1 + assert result.stdout.count("\n") == 1 + assert "first problem%0Asecond problem 100%25" in _error_annotation(result) + + +@pytest.mark.parametrize("variable", ["GITHUB_EVENT_PATH", "GITHUB_REPOSITORY"]) +def test_the_check_requires_its_environment(fake_gh, tmp_path, variable): + event_path = _write_event(tmp_path, _pull_request_event(VALID_BODY)) + env = fake_gh.environment( + GITHUB_EVENT_PATH=str(event_path), GITHUB_REPOSITORY=REPOSITORY + ) + del env[variable] + + result = _run_script(env) + + assert result.returncode == 1 + assert f"{variable} is required" in result.stderr + assert fake_gh.calls() == [] + + +def test_the_check_requires_a_pull_request_event(fake_gh, tmp_path): + event_path = _write_event(tmp_path, {"ref": "refs/heads/master"}) + + result = _run_script( + fake_gh.environment( + GITHUB_EVENT_PATH=str(event_path), GITHUB_REPOSITORY=REPOSITORY + ) + ) + + assert result.returncode == 1 + assert "has no pull_request" in result.stderr + assert fake_gh.calls() == [] + + +# --- The workflow job ----------------------------------------------------- + + +@pytest.fixture(scope="module") +def check_workflow() -> dict: + return yaml.safe_load(CHECK_WORKFLOW.read_text(encoding="utf-8")) + + +def test_the_check_runs_on_every_event_that_can_change_the_description( + check_workflow, +): + # PyYAML reads the key "on" as True. + triggers = check_workflow.get("on", check_workflow.get(True)) + + # No branches or paths filter: every pull request is checked, whatever it + # targets and whatever it changes. + assert triggers == { + "pull_request": {"types": ["opened", "edited", "reopened", "synchronize"]} + } + + +def test_the_check_runs_on_every_open_pull_request_including_stacked_ones( + check_workflow, +): + """Unlike pr.yml's jobs, this one has no stack condition and needs no job. + + It skips closed pull requests: editing a merged pull request's description + would otherwise fail the check, because merging closed its issue. + """ + assert list(check_workflow["jobs"]) == [CHECK_JOB] + job = check_workflow["jobs"][CHECK_JOB] + + assert job["name"] == "PR description" + assert job["if"] == "${{ github.event.pull_request.state == 'open' }}" + assert "needs" not in job + + +def test_the_check_can_only_read(check_workflow): + assert check_workflow["permissions"] == { + "contents": "read", + "issues": "read", + "pull-requests": "read", + } + assert "permissions" not in check_workflow["jobs"][CHECK_JOB] + + +def test_the_check_runs_the_tested_script_with_the_job_token(check_workflow): + steps = check_workflow["jobs"][CHECK_JOB]["steps"] + checkout = next( + step for step in steps if step.get("uses", "").startswith("actions/checkout@") + ) + check_steps = [step for step in steps if CHECK_SCRIPT.name in step.get("run", "")] + + assert checkout["with"]["sparse-checkout"] == ( + f".github/scripts/{CHECK_SCRIPT.name}" + ) + assert len(check_steps) == 1 + assert check_steps[0]["run"] == f"python .github/scripts/{CHECK_SCRIPT.name}" + assert check_steps[0]["env"] == {"GH_TOKEN": "${{ github.token }}"} + + +# --- Proof ---------------------------------------------------------------- + + +def _run_on_fixture( + fake_gh: FakeGh, fixture: str, issue: dict +) -> subprocess.CompletedProcess[str]: + return _run_script( + fake_gh.environment( + GITHUB_EVENT_PATH=str(PROOF_FIXTURES / fixture), + GITHUB_REPOSITORY=REPOSITORY, + **_gh_answers(issue), + ) + ) + + +def test_rule_proof_pr_tracking_issue(fake_gh): + """The check rejects bundle-update PR #4169's description and accepts it fixed. + + #4169's description starts with "## Summary". The good fixture adds the + line the fixed bundle-update script writes, Fixes #4170, and the fake gh + answers that #4170 is an open issue in this repository. + """ + results = { + fixture: _run_on_fixture(fake_gh, f"{fixture}_event.json", _issue(4170)) + for fixture in ("bad", "good") + } + + assert results["bad"].returncode == 1 + assert 'The first line is "## Summary"' in _error_annotation(results["bad"]) + assert results["good"].returncode == 0, results["good"].stdout + assert fake_gh.calls() == [ + ["gh", "api", "repos/PolicyEngine/policyengine-api/issues/4170"] + ] + + +@pytest.mark.parametrize( + ("fixture", "expected_message"), + [ + # PR #3776, whose Fixes line was on line 37; shortened. + ("bad_fixes_not_first_event.json", "Fixes #3775 is on line 10"), + ( + "bad_other_repository_event.json", + 'The first line is "Fixes PolicyEngine/policyengine-sim-api#862"', + ), + ], +) +def test_the_other_rejected_fixtures_fail_without_asking_github( + fake_gh, fixture, expected_message +): + result = _run_on_fixture(fake_gh, fixture, _issue(3775)) + + assert result.returncode == 1 + assert expected_message in _error_annotation(result) + assert fake_gh.calls() == [] From c5d555f31eb59c4b8d81bff2cff92090fb00bc6c Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 14:45:02 +0400 Subject: [PATCH 2/8] Check that every PR description starts with Fixes #N for an open issue Port the simulation API's PR description check (PolicyEngine/policyengine-sim-api#862). A new workflow, pr-description.yml, runs on every opened, edited, reopened and synchronize event of an open pull request, with no stack condition, and fails unless the description's first line is exactly Fixes #N and gh api, with the job's read-only token, reports N as an open issue in this repository. - Enforce api.process.pr-tracking-issue with the new job, with proof fixtures, and add pr-description.yml to the registry's discovered workflows. - The PolicyEngine bundle-update script now reuses the open "Update PolicyEngine bundle to " issue or opens one, and starts the pull request's description with Fixes #N on both paths that open a pull request. The daily relock script already did. - Let CI/CD scripts under .github run other programs (S603, S607), as command-line scripts under scripts/ already may. - Describe the check in the PR skill, AGENTS.md and the README. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/check_pr_description.py | 266 ++++++++++++++++++ .../scripts/update-policyengine-package.sh | 51 +++- .github/workflows/pr-description.yml | 52 ++++ AGENTS.md | 7 +- README.md | 2 +- changelog.d/4173.added.md | 1 + docs/engineering/skills/github-prs.md | 62 +++- docs/generated/rules.md | 22 +- pyproject.toml | 7 +- rules/registry.toml | 32 ++- 10 files changed, 470 insertions(+), 32 deletions(-) create mode 100644 .github/scripts/check_pr_description.py create mode 100644 .github/workflows/pr-description.yml create mode 100644 changelog.d/4173.added.md diff --git a/.github/scripts/check_pr_description.py b/.github/scripts/check_pr_description.py new file mode 100644 index 000000000..01af3fccc --- /dev/null +++ b/.github/scripts/check_pr_description.py @@ -0,0 +1,266 @@ +"""Check that a pull request's description names its tracking issue. + +Rule api.process.pr-tracking-issue: the first line of every pull request's +description is ``Fixes #ISSUE_NUMBER``, and ISSUE_NUMBER is an open issue, not a +pull request, in this repository. The "PR description" job in +.github/workflows/pr-description.yml runs this script; it prints one error +annotation and exits 1 when the rule is broken. + +Environment: + GITHUB_EVENT_PATH the pull_request event payload, which holds the + description (set by the GitHub Actions runner) + GITHUB_REPOSITORY owner/name of this repository (set by the runner) + GH_TOKEN read by gh, not by this script; the workflow sets it to + the job's github.token, which needs only issues: read + +It needs only the standard library and gh, so the workflow runs it without +installing the project. + +How the first line is read: + +- A line ends at a CRLF, a carriage return or a line feed, as in Markdown, so + descriptions edited in the GitHub web interface (which uses CRLF) pass. +- Trailing spaces and tabs are ignored: they don't show, and GitHub still reads + the closing keyword. +- Leading whitespace fails. Four leading spaces make the line a Markdown code + block, where GitHub doesn't read closing keywords, and any indent is a sign + the line isn't what the author meant. +- An empty or missing (``null``) description fails. +- The number is ASCII digits without a leading zero. + +``require_open_issue`` is the issue lookup on its own. Its errors say only what +is wrong; ``main`` adds the fix. +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +from pathlib import Path +from urllib.parse import urlsplit + +RULE_ID = "api.process.pr-tracking-issue" +HOW_TO_FIX = ( + "Edit the description so its first line is exactly Fixes #ISSUE_NUMBER, " + "where ISSUE_NUMBER is an open issue in this repository; editing the " + "description runs this check again. See docs/engineering/skills/github-prs.md " + f"#tracking-issues (rule {RULE_ID})." +) +RETRY_LOOKUP = ( + "Re-run this job; if it fails again, check the GH_TOKEN the workflow gives it." +) +REOPEN_ISSUE = ( + "If this pull request still fixes it, reopen it and re-run this job; " + "otherwise open an issue for this work and edit the first line to name it " + f"(rule {RULE_ID})." +) +FIXES_LINE = re.compile(r"Fixes #([1-9][0-9]*)") +# Markdown line endings: CRLF, a carriage return alone, or a line feed. +LINE_ENDING = re.compile(r"\r\n|\r|\n") +# How much of a rejected first line a message quotes. +SHOWN_FIRST_LINE_LENGTH = 80 +# gh api's error for a number GitHub has no issue or pull request for (404), +# or one whose issue was deleted (410). +MISSING_ISSUE = re.compile(r"\(HTTP (?:404|410)\)") +# The path of an issue's repository_url: [api/v3/]repos//. +REPOSITORY_PATH = re.compile(r"(?:.+/)?repos/([^/]+/[^/]+)") +ANNOTATION_TITLE = "PR description" + + +class DescriptionError(Exception): + """The description's first line does not name an issue as the rule requires.""" + + +class IssueLookupError(Exception): + """GitHub couldn't be asked about an issue, or its answer wasn't an issue. + + Running the lookup again may succeed. The message says what went wrong, + without a fix. + """ + + +class NotAnOpenIssueError(Exception): + """A number names no open issue in the repository. + + ``state`` is the issue's state, such as ``closed``, when the number names an + issue in the repository that isn't open, and None when it names no issue + there. The message says what the number names, without a fix. + """ + + def __init__(self, message: str, *, state: str | None = None) -> None: + """Keep ``message`` as the error's text, and the issue's ``state``.""" + super().__init__(message) + self.state = state + + +def _shown(line: str) -> str: + """Return ``line`` quoted for a message, shortened when it is long.""" + if len(line) > SHOWN_FIRST_LINE_LENGTH: + line = line[:SHOWN_FIRST_LINE_LENGTH].rstrip() + "..." + return f'"{line}"' + + +def issue_number_from_description(body: str | None) -> int: + """Return the issue number on the description's first line. + + Raise ``DescriptionError``, with a message that says how to fix the + description, unless the first line is ``Fixes #ISSUE_NUMBER``. + """ + if body is None or not body.strip(): + msg = f"The description is empty. {HOW_TO_FIX}" + raise DescriptionError(msg) + + lines = [line.rstrip(" \t") for line in LINE_ENDING.split(body)] + first_line = lines[0] + match = FIXES_LINE.fullmatch(first_line) + if match: + return int(match.group(1)) + + if FIXES_LINE.fullmatch(first_line.lstrip()): + msg = ( + f"The first line starts with whitespace: {_shown(first_line)}. {HOW_TO_FIX}" + ) + raise DescriptionError(msg) + for line_number, line in enumerate(lines[1:], start=2): + if FIXES_LINE.fullmatch(line.strip()): + msg = ( + f"{line.strip()} is on line {line_number}, but it must be the " + f"first line. {HOW_TO_FIX}" + ) + raise DescriptionError(msg) + msg = f"The first line is {_shown(first_line)}. {HOW_TO_FIX}" + raise DescriptionError(msg) + + +def fetch_issue(repository: str, number: int) -> dict[str, object]: + """Return GitHub's answer for issue ``number`` in ``repository``, from gh api. + + The issues API also answers for pull requests, and gh follows the redirect + GitHub sends for an issue transferred to another repository, so the answer + still has to be checked with ``check_issue``. Raise ``NotAnOpenIssueError`` + when GitHub has no issue ``number``, and ``IssueLookupError`` when the + lookup fails. + """ + completed = subprocess.run( + ["gh", "api", f"repos/{repository}/issues/{number}"], + capture_output=True, + text=True, + check=False, + ) + if completed.returncode != 0: + if MISSING_ISSUE.search(completed.stderr): + msg = f"There is no issue #{number} in {repository}." + raise NotAnOpenIssueError(msg) + reported = completed.stderr.strip() or f"exit status {completed.returncode}" + msg = f"Could not look up #{number} in {repository}: {reported}." + raise IssueLookupError(msg) + try: + issue = json.loads(completed.stdout) + except json.JSONDecodeError: + issue = None + if not isinstance(issue, dict): + msg = f"gh api did not print an issue for #{number}." + raise IssueLookupError(msg) + return issue + + +def _repository_of(repository_url: object) -> str | None: + """Return ``owner/name`` from an issue's ``repository_url``, if it has one.""" + if not isinstance(repository_url, str): + return None + match = REPOSITORY_PATH.fullmatch(urlsplit(repository_url).path.strip("/")) + return match.group(1) if match else None + + +def check_issue(repository: str, number: int, issue: dict[str, object]) -> None: + """Return quietly if ``issue``, GitHub's answer for ``number``, is open here. + + Raise ``NotAnOpenIssueError`` when it is a pull request, an issue moved to + another repository or an issue that isn't open, and ``IssueLookupError`` + when the answer names no repository. + """ + issue_repository = _repository_of(issue.get("repository_url")) + if issue_repository is None: + msg = ( + f"GitHub's answer for #{number} names no repository, so it can't be " + f"confirmed as an issue in {repository}." + ) + raise IssueLookupError(msg) + if issue_repository.lower() != repository.lower() or issue.get("number") != number: + msg = ( + f"#{number} is not an issue in {repository}: it moved to " + f"{issue_repository}#{issue.get('number')}." + ) + raise NotAnOpenIssueError(msg) + if issue.get("pull_request") is not None: + msg = f"#{number} is a pull request, not an issue." + raise NotAnOpenIssueError(msg) + state = issue.get("state") + if state != "open": + msg = f"Issue #{number} is {state}." + raise NotAnOpenIssueError(msg, state=str(state)) + + +def require_open_issue(repository: str, number: int) -> None: + """Return quietly if ``number`` is an open issue, not a pull request, here. + + Raise ``NotAnOpenIssueError`` or ``IssueLookupError`` otherwise; see + ``fetch_issue`` and ``check_issue``. + """ + check_issue(repository, number, fetch_issue(repository, number)) + + +def _required_environment(name: str) -> str: + value = os.environ.get(name) + if not value: + msg = f"{name} is required" + raise SystemExit(msg) + return value + + +def _description_from_event(event_path: str) -> str | None: + """Return the pull request description in the event payload at ``event_path``.""" + event = json.loads(Path(event_path).read_text(encoding="utf-8")) + pull_request = event.get("pull_request") if isinstance(event, dict) else None + if not isinstance(pull_request, dict): + msg = ( + f"The event payload at {event_path} has no pull_request; run this " + "check on pull_request events." + ) + raise SystemExit(msg) + body = pull_request.get("body") + return body if isinstance(body, str) else None + + +def _annotation_text(message: str) -> str: + """Escape ``message`` so the runner reads it as one annotation message.""" + return message.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + + +def main() -> int: + """Check the pull request in this run's event; return the exit status.""" + repository = _required_environment("GITHUB_REPOSITORY") + description = _description_from_event(_required_environment("GITHUB_EVENT_PATH")) + try: + number = issue_number_from_description(description) + require_open_issue(repository, number) + except DescriptionError as error: + message = str(error) + except IssueLookupError as error: + message = f"{error} {RETRY_LOOKUP}" + except NotAnOpenIssueError as error: + message = f"{error} {HOW_TO_FIX if error.state is None else REOPEN_ISSUE}" + else: + print( + f"The first line is Fixes #{number}, and #{number} is an open issue in " + f"{repository}." + ) + return 0 + print(f"::error title={ANNOTATION_TITLE}::{_annotation_text(message)}") + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/scripts/update-policyengine-package.sh b/.github/scripts/update-policyengine-package.sh index 12a47403e..8cdc75de7 100755 --- a/.github/scripts/update-policyengine-package.sh +++ b/.github/scripts/update-policyengine-package.sh @@ -5,8 +5,16 @@ # fragment, and open a single-version PR on branch # auto/update-policyengine-bundle-. # +# Every PR names its tracking issue on the first line of its description +# (rule api.process.pr-tracking-issue): the oldest open issue titled +# "Update PolicyEngine bundle to ", or a new one when none is open. +# Merging the PR closes it. +# # Environment: -# GH_TOKEN must be set for gh. +# GH_TOKEN must be set for gh: a token that can write issues and pull +# requests. +# GITHUB_REPOSITORY is owner/name of the repository the tracking issue is in +# (set by the runner; default: PolicyEngine/policyengine-api). # LATEST_OVERRIDE may be set to target an exact version (the # repository_dispatch trigger passes the just-released version here); # otherwise the latest version on PyPI is used. @@ -19,8 +27,47 @@ if [[ "${1:-}" == "--dry-run" ]]; then DRY_RUN=1 fi +REPOSITORY="${GITHUB_REPOSITORY:-PolicyEngine/policyengine-api}" +ISSUE_NUMBER="" + +# Set ISSUE_NUMBER to the oldest open tracking issue for LATEST, opening one +# when none is open. This sets a variable rather than printing, because bash +# turns off set -e inside $(...), and a failed lookup must not open a +# duplicate issue. +find_or_open_tracking_issue() { + local title="Update PolicyEngine bundle to ${LATEST}" + local issue_number + local issue_url + + issue_number="$( + gh issue list \ + --repo "${REPOSITORY}" \ + --state open \ + --limit 1000 \ + --json number,title \ + --jq "map(select(.title == \"${title}\")) | sort_by(.number) | .[0].number // empty" + )" + if [[ -z "${issue_number}" ]]; then + issue_url="$( + gh issue create \ + --repo "${REPOSITORY}" \ + --title "${title}" \ + --body "Tracks the automated update of the PolicyEngine .py bundle to ${LATEST} by .github/workflows/update-policyengine-bundle.yaml. The update pull request names this issue on the first line of its description and closes it when it merges." + )" + issue_number="${issue_url##*/}" + fi + + if [[ ! "${issue_number}" =~ ^[0-9]+$ ]]; then + echo "Could not resolve the tracking issue for PolicyEngine bundle ${LATEST}: ${issue_number}" >&2 + exit 1 + fi + ISSUE_NUMBER="${issue_number}" +} + create_pr_body() { cat </dev/null; then echo "Remote branch '${BRANCH}' already exists without an open PR. Creating PR." + find_or_open_tracking_issue gh pr create \ --base master \ --head "$BRANCH" \ @@ -169,6 +217,7 @@ git add pyproject.toml docker/Dockerfile uv.lock "$FRAGMENT" git commit -m "Update PolicyEngine bundle to ${LATEST}" git push -u origin "$BRANCH" +find_or_open_tracking_issue gh pr create \ --base master \ --title "Update PolicyEngine bundle to ${LATEST}" \ diff --git a/.github/workflows/pr-description.yml b/.github/workflows/pr-description.yml new file mode 100644 index 000000000..42bc5646d --- /dev/null +++ b/.github/workflows/pr-description.yml @@ -0,0 +1,52 @@ +name: PR description + +# Rule api.process.pr-tracking-issue: the first line of every pull request's +# description is "Fixes #ISSUE_NUMBER", naming an open issue in this +# repository. See docs/engineering/skills/github-prs.md#tracking-issues. +# +# A separate workflow from pr.yml, so that editing a description reruns only +# this check, not the whole of CI. It has no branches or paths filter and no +# stack condition: it is cheap, so every open pull request runs it, including +# each one in a stack. CI complete in pr.yml can't need a job in another +# workflow, so the master ruleset requires this job's check on its own. + +on: + pull_request: + types: [opened, edited, reopened, synchronize] + +# The check reads the description from the event payload and looks the issue +# up with the job's token, so it needs only read access. Pull requests from +# forks get a read-only token, which is enough. +permissions: + contents: read + issues: read + pull-requests: read + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + pr-description: + name: PR description + # GitHub also sends "edited" for closed pull requests. Merging closed the + # issue a merged pull request names, so checking one would fail. + if: ${{ github.event.pull_request.state == 'open' }} + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Checkout the check script + uses: actions/checkout@v4 + with: + sparse-checkout: .github/scripts/check_pr_description.py + sparse-checkout-cone-mode: false + # The script needs only the standard library and gh, which the runner + # image provides, so nothing is installed. + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + - name: Check the first line and the tracking issue + env: + GH_TOKEN: ${{ github.token }} + run: python .github/scripts/check_pr_description.py diff --git a/AGENTS.md b/AGENTS.md index 1f7b8c19e..f6c17dc22 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,7 +55,9 @@ code), or splitting a module into smaller ones, read Read `docs/engineering/skills/github-prs.md` before opening, replacing, or sharing any pull request. Every PR needs a tracking issue and -`Fixes #ISSUE_NUMBER` as the first line of its description. Every PR also adds +`Fixes #ISSUE_NUMBER` as the first line of its description; the +`PR description` job fails unless that issue is open in this repository. +Every PR also adds a changelog fragment of its own: a non-empty file directly in `changelog.d/`, named `changelog.d/..md`, where `` is `breaking`, `added`, `changed`, `fixed` or `removed` (see "Changelog fragments" in that skill). @@ -133,11 +135,10 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### Process and repository hygiene -- Checked: `api.agents.adapters-point-to-agents-md`, `api.process.changelog-fragment`, `api.process.cloud-run-image-builds`, `api.process.docker-image-builds`, `api.process.rules-registry`, `api.process.stacked-prs-full-ci-at-bottom` +- Checked: `api.agents.adapters-point-to-agents-md`, `api.process.changelog-fragment`, `api.process.cloud-run-image-builds`, `api.process.docker-image-builds`, `api.process.pr-tracking-issue`, `api.process.rules-registry`, `api.process.stacked-prs-full-ci-at-bottom` - `api.process.agents-never-bypass`: Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. (doc-only, not checked: follow it yourself) - `api.process.agents-suppress-as-last-resort`: Agents add an inline ruff suppression (noqa) only when neither a code change nor a ruff setting removes the finding, and a PR lists every inline suppression it adds, with the reason. (doc-only, not checked: follow it yourself) - `api.process.makefile-phony`: Command-only Makefile targets are declared in .PHONY, and .PHONY stays in sync when targets are renamed or removed. (doc-only, not checked: follow it yourself) -- `api.process.pr-tracking-issue`: Every PR has a tracking issue in this repository, and the first line of its description is Fixes #ISSUE_NUMBER. (doc-only, not checked: follow it yourself) - `api.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/guards/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. (doc-only, not checked: follow it yourself) diff --git a/README.md b/README.md index b95c00ee8..d833a4339 100644 --- a/README.md +++ b/README.md @@ -241,7 +241,7 @@ In the first line of your PR, please make sure to include the following: Fixes #{issue_number} ``` -This makes it much easier for us to maintain and prune our issue board. +This makes it much easier for us to maintain and prune our issue board. The `PR description` check fails unless the first line is exactly `Fixes #` followed by the number of an open issue in this repository; see [the PR skill](docs/engineering/skills/github-prs.md#tracking-issues). Please try to be detailed in your PRs about the changes you made and why you made them. You may find yourself looking back at them for reference in the future, or needing insight about someone else's changes. Save yourself a conversation and write it all in the PR! diff --git a/changelog.d/4173.added.md b/changelog.d/4173.added.md new file mode 100644 index 000000000..b40dbe837 --- /dev/null +++ b/changelog.d/4173.added.md @@ -0,0 +1 @@ +Check that every pull request description starts with `Fixes #N` for an open issue in this repository, and start automated PolicyEngine bundle-update pull requests with that line. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index ef00824a0..f8cab5741 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -11,6 +11,50 @@ Before opening a PR, all developers and agents must: 2. Put `Fixes #ISSUE_NUMBER` as the first line of the PR description, using the issue number from the previous step. The PR template starts with this line. +Rule `api.process.pr-tracking-issue`. The `PR description` job, in +`.github/workflows/pr-description.yml`, checks every open pull request whenever +it is opened, reopened or edited and on every push, including each pull request +in a stack. It skips closed pull requests. It reads the description from the +event that started the run and fails unless: + +- the first line is exactly `Fixes #ISSUE_NUMBER`; +- `ISSUE_NUMBER` is an open issue in this repository. A pull request's number, + an issue that was moved to another repository, a closed issue and a number + with no issue all fail. + +CRLF line endings, which the GitHub web editor uses, and trailing spaces are +fine. These fail: + +- an empty description, or the template's `Fixes #` left unfilled; +- a blank first line, or leading spaces before `Fixes`; +- other wording, such as `Fixes #3728.`, `Closes #3800.` or `Part of #3797`; +- a reference to another repository, such as + `Fixes PolicyEngine/policyengine-sim-api#862`. + +To fix a failure: + +- **Wrong first line:** edit the description. Saving the edit runs the check + again. +- **Closed issue:** if the pull request still fixes it, reopen the issue, then + re-run the job from the pull request's Checks tab. Reopening an issue starts + no run. Re-running checks the description as it was when the run started, so + after changing the description, rely on the run the edit starts. + +To run the check on a description locally, save a pull request event and pass +it in; the check needs only Python and an authenticated `gh`: + +```bash +gh pr view --json body --jq '{pull_request: {body: .body}}' > /tmp/event.json +GITHUB_EVENT_PATH=/tmp/event.json GITHUB_REPOSITORY=PolicyEngine/policyengine-api \ + python .github/scripts/check_pr_description.py +``` + +Automated pull requests follow the rule too. The PolicyEngine bundle update +(`update-policyengine-package.sh`) reuses the open "Update PolicyEngine bundle +to " issue or opens one. The daily relock (`open_relock_pull_request.sh`) +reuses the open "Relock dependencies" issue or opens one, and rewrites the +description of a relock pull request that is already open. + ## Changelog fragments Every PR adds at least one changelog fragment of its own @@ -76,13 +120,17 @@ human instead. In a stack of pull requests, only the lowest unmerged one runs the full PR workflow (`.github/workflows/pr.yml`). The ones above it run only the -`CI complete` job, which fails and says so. A stack therefore uses one set of -runners instead of one per layer. - -- **`CI complete` is the required check.** It needs every other job. On a - pull request that isn't stacked, or is the lowest unmerged one, it passes - only when every job passed. Skipped jobs would otherwise count as passing, so - a stack can only merge bottom-up, and nothing merges untested. +`CI complete` job, which fails and says so, and the `PR description` job (see +[Tracking Issues](#tracking-issues)), which every open pull request runs. A +stack therefore uses one set of runners instead of one per layer. + +- **`CI complete` is the required check for `pr.yml`.** It needs every other + job. On a pull request that isn't stacked, or is the lowest unmerged one, it + passes only when every job passed. Skipped jobs would otherwise count as + passing, so a stack can only merge bottom-up, and nothing merges untested. +- **`PR description` is in a workflow of its own,** so `CI complete` can't + need it, and it has no stack condition. The master ruleset requires it as a + separate check. - **When the pull request below merges,** GitHub rebases the next one onto the trunk, and that push starts its full run. If no run starts, push again (`gh stack rebase`, then `gh stack push`), or close and reopen the pull diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 98a079179..bf8654928 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 59 | 0 | 1 | 13 | 73 | +| mechanical | 60 | 0 | 1 | 12 | 73 | | metric | 1 | 0 | 0 | 0 | 1 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 60 of 75 rules. +**Blocking:** 61 of 75 rules. ## Gaps @@ -25,7 +25,6 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce - `api.process.agents-never-bypass`: Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. - `api.process.agents-suppress-as-last-resort`: Agents add an inline ruff suppression (noqa) only when neither a code change nor a ruff setting removes the finding, and a PR lists every inline suppression it adds, with the reason. - `api.process.makefile-phony`: Command-only Makefile targets are declared in .PHONY, and .PHONY stays in sync when targets are renamed or removed. -- `api.process.pr-tracking-issue`: Every PR has a tracking issue in this repository, and the first line of its description is Fixes #ISSUE_NUMBER. - `api.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/guards/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. - `api.routes.shared-query-parameters`: Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. - `api.v2.connectors-never-commit`: v2 connectors receive a session from the service and never open, commit, roll back or close it. @@ -327,7 +326,7 @@ flowchart LR | Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof | | --- | --- | --- | --- | --- | --- | --- | --- | | `api.quality.alembic-revisions-unused-imports` | Ruff doesn't lint Alembic revision files (lint.exclude in pyproject.toml), because they must stay exactly as Alembic generates them. | mechanical | advisory | no | nothing | — | — | -| `api.quality.ci-cd-scripts-ruff-exemptions` | CI/CD scripts under .github may print their results, fetch fixed URLs, have hyphenated file names and import the lazily loaded packages at module level (T201, S310, N999, TID253); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of N999, S310, T201, TID253 for `.github/**` | `pr.yml:lint` | — | +| `api.quality.ci-cd-scripts-ruff-exemptions` | CI/CD scripts under .github may print their results, run other programs, fetch fixed URLs, have hyphenated file names and import the lazily loaded packages at module level (T201, S603, S607, S310, N999, TID253); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of N999, S310, S603, S607, T201, TID253 for `.github/**` | `pr.yml:lint` | — | | `api.quality.command-line-modules-ruff-exemptions` | Application modules that also run as command-line programs (if __name__ == "__main__") may print their output (T201); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of T201 for `policyengine_api/{data/v2/catalog/initialization,data/v2/household_migration_qualification,data/v2/policy_migration_qualification,data/v2/privilege_reconciliation,data/v2/user_association_migration_qualification,worker_spm_release}.py` | `pr.yml:lint` | — | | `api.quality.command-line-scripts-ruff-exemptions` | Command-line scripts under scripts/ may print their output, run other programs, fetch fixed URLs, stamp today's date and pick random samples (T201, S603, S607, S310, DTZ011, S311); every other ruff rule applies to them. | mechanical | enforced | yes | ruff per-file ignore of DTZ011, S310, S311, S603, S607, T201 for `scripts/**` | `pr.yml:lint` | — | | `api.quality.environment-variables-load` | Required environment variables and credentials load in a deployed-like environment. | mechanical | enforced | yes | CI job `test_env_vars` | `pr.yml:test_env_vars` | — | @@ -345,7 +344,7 @@ flowchart LR r_api_quality_alembic_revisions_unused_imports("api.quality.alembic-revisions-unused-imports (advisory)") gap_unenforced>"not enforced"] r_api_quality_ci_cd_scripts_ruff_exemptions("api.quality.ci-cd-scripts-ruff-exemptions (enforced)") - e_ruff_per_file_ignore_of_N999__S310__T201__TID253_for__github___["ruff per-file ignore of N999, S310, T201, TID253 for .github/**"] + e_ruff_per_file_ignore_of_N999__S310__S603__S607__T201__TID253_for__github___["ruff per-file ignore of N999, S310, S603, S607, T201, TID253 for .github/**"] j_CI_pr_yml_lint["CI pr.yml:lint"] r_api_quality_command_line_modules_ruff_exemptions("api.quality.command-line-modules-ruff-exemptions (enforced)") e_ruff_per_file_ignore_of_T201_for_policyengine_api__data_v2_catalog_initialization_data_v2_household_migration_qualification_data_v2_policy_migration_qualification_data_v2_privilege_reconciliation_data_v2_user_association_migration_qualification_worker_spm_release__py["ruff per-file ignore of T201 for policyengine_api/{data/v2/catalog/initialization,data/v2/household_migration_qualification,data/v2/policy_migration_qualification,data/v2/privilege_reconciliation,data/v2/user_association_migration_qualification,worker_spm_release}.py"] @@ -375,8 +374,8 @@ flowchart LR r_api_quality_unittest_ruff_exemptions("api.quality.unittest-ruff-exemptions (enforced)") e_ruff_per_file_ignore_of_PT_for_scripts_rules_tests___["ruff per-file ignore of PT for scripts/rules/tests/**"] r_api_quality_alembic_revisions_unused_imports -.-> gap_unenforced - r_api_quality_ci_cd_scripts_ruff_exemptions --> e_ruff_per_file_ignore_of_N999__S310__T201__TID253_for__github___ - e_ruff_per_file_ignore_of_N999__S310__T201__TID253_for__github___ --> j_CI_pr_yml_lint + r_api_quality_ci_cd_scripts_ruff_exemptions --> e_ruff_per_file_ignore_of_N999__S310__S603__S607__T201__TID253_for__github___ + e_ruff_per_file_ignore_of_N999__S310__S603__S607__T201__TID253_for__github___ --> j_CI_pr_yml_lint r_api_quality_command_line_modules_ruff_exemptions --> e_ruff_per_file_ignore_of_T201_for_policyengine_api__data_v2_catalog_initialization_data_v2_household_migration_qualification_data_v2_policy_migration_qualification_data_v2_privilege_reconciliation_data_v2_user_association_migration_qualification_worker_spm_release__py e_ruff_per_file_ignore_of_T201_for_policyengine_api__data_v2_catalog_initialization_data_v2_household_migration_qualification_data_v2_policy_migration_qualification_data_v2_privilege_reconciliation_data_v2_user_association_migration_qualification_worker_spm_release__py --> j_CI_pr_yml_lint r_api_quality_command_line_scripts_ruff_exemptions --> e_ruff_per_file_ignore_of_DTZ011__S310__S311__S603__S607__T201_for_scripts___ @@ -474,7 +473,7 @@ flowchart LR | `api.process.cloud-run-image-builds` | The Cloud Run container image builds. | mechanical | enforced | yes | CI job `test_cloud_run_container_builds` | `pr.yml:test_cloud_run_container_builds` | — | | `api.process.docker-image-builds` | The API Docker image builds. | mechanical | enforced | yes | CI job `test_container_builds` | `pr.yml:test_container_builds` | — | | `api.process.makefile-phony` | Command-only Makefile targets are declared in .PHONY, and .PHONY stays in sync when targets are renamed or removed. | mechanical | doc-only | no | nothing | — | — | -| `api.process.pr-tracking-issue` | Every PR has a tracking issue in this repository, and the first line of its description is Fixes #ISSUE_NUMBER. | mechanical | doc-only | no | nothing | — | — | +| `api.process.pr-tracking-issue` | The first line of every PR's description is Fixes #ISSUE_NUMBER, naming an open issue, not a pull request, in this repository; automated PRs open or reuse such an issue. | mechanical | enforced | yes | CI job `pr-description` | `pr-description.yml:pr-description` | yes | | `api.process.rule-machinery-code-owned` | Changes to the rule machinery (rules/, scripts/rules/, scripts/guards/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. | mechanical | doc-only | no | nothing | — | — | | `api.process.rules-registry` | Every rule is registered in rules/registry.toml, is enforced where it claims to be, and its rendered views are current. | mechanical | enforced | yes | quality guard `rules-registry` | `pr.yml:quality-guards` | **unproven** | | `api.process.stacked-prs-full-ci-at-bottom` | In a stack of pull requests, only the lowest unmerged one runs the full PR workflow. Every pr.yml job is needed by the CI complete gate, which fails on the pull requests above it and otherwise passes only when every job passed, so a stack merges bottom-up and nothing merges untested. | mechanical | enforced | yes | pytest `tests/unit/test_ci_complete_gate.py` | `pr.yml:test`, `pr.yml:ci-complete` | yes | @@ -498,7 +497,9 @@ flowchart LR e_CI_job_test_container_builds["CI job test_container_builds"] j_CI_pr_yml_test_container_builds["CI pr.yml:test_container_builds"] r_api_process_makefile_phony("api.process.makefile-phony (doc-only)") - r_api_process_pr_tracking_issue("api.process.pr-tracking-issue (doc-only)") + r_api_process_pr_tracking_issue("api.process.pr-tracking-issue (enforced)") + e_CI_job_pr_description["CI job pr-description"] + j_CI_pr_description_yml_pr_description["CI pr-description.yml:pr-description"] r_api_process_rule_machinery_code_owned("api.process.rule-machinery-code-owned (doc-only)") r_api_process_rules_registry("api.process.rules-registry (enforced)") r_api_process_stacked_prs_full_ci_at_bottom("api.process.stacked-prs-full-ci-at-bottom (enforced)") @@ -517,7 +518,8 @@ flowchart LR r_api_process_docker_image_builds --> e_CI_job_test_container_builds e_CI_job_test_container_builds --> j_CI_pr_yml_test_container_builds r_api_process_makefile_phony -.-> gap_unenforced - r_api_process_pr_tracking_issue -.-> gap_unenforced + r_api_process_pr_tracking_issue --> e_CI_job_pr_description + e_CI_job_pr_description --> j_CI_pr_description_yml_pr_description r_api_process_rule_machinery_code_owned -.-> gap_unenforced r_api_process_rules_registry --> e_quality_guard_rules_registry r_api_process_stacked_prs_full_ci_at_bottom --> e_pytest_tests_unit_test_ci_complete_gate_py diff --git a/pyproject.toml b/pyproject.toml index ab4e87dce..9e95fe61f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -142,9 +142,10 @@ exclude = ["migrations/*/versions/*.py"] # The shared rules tool needs only the standard library, so its tests use # unittest on purpose; pytest-style rules don't apply to them. "scripts/rules/tests/**" = ["PT"] -# CI/CD scripts print their results, fetch fixed URLs, may have hyphenated -# file names and may import the lazily loaded packages at module level. -".github/**" = ["N999", "S310", "T201", "TID253"] +# CI/CD scripts print their results, run other programs (such as gh), fetch +# fixed URLs, may have hyphenated file names and may import the lazily loaded +# packages at module level. +".github/**" = ["N999", "S310", "S603", "S607", "T201", "TID253"] # Command-line scripts print their output, run other programs, fetch fixed # URLs, stamp today's date and pick random samples. "scripts/**" = [ diff --git a/rules/registry.toml b/rules/registry.toml index 04f5be4bf..4decc375d 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -56,7 +56,7 @@ id = "process" title = "Process and repository hygiene" [discovery] -workflows = [".github/workflows/pr.yml"] +workflows = [".github/workflows/pr.yml", ".github/workflows/pr-description.yml"] ignore_jobs = [ { job = "upload-test-coverage", reason = "sends the test shards' coverage reports to Codecov in one upload; it checks nothing" }, ] @@ -954,7 +954,7 @@ invocation = "ruff check ." [[rule]] id = "api.quality.ci-cd-scripts-ruff-exemptions" category = "quality" -statement = "CI/CD scripts under .github may print their results, fetch fixed URLs, have hyphenated file names and import the lazily loaded packages at module level (T201, S310, N999, TID253); every other ruff rule applies to them." +statement = "CI/CD scripts under .github may print their results, run other programs, fetch fixed URLs, have hyphenated file names and import the lazily loaded packages at module level (T201, S603, S607, S310, N999, TID253); every other ruff rule applies to them." rationale = "docs/engineering/skills/rules-registry.md#file-level-exceptions" scope = [".github/**/*.py"] tier = "mechanical" @@ -964,7 +964,7 @@ autofix = false kind = "ruff" mode = "per-file-ignore" pattern = ".github/**" -codes = ["N999", "S310", "T201", "TID253"] +codes = ["N999", "S310", "S603", "S607", "T201", "TID253"] [[rule.ci]] workflow = ".github/workflows/pr.yml" job = "lint" @@ -1387,14 +1387,32 @@ invocation = "unittest discover -s scripts/rules/tests" [[rule]] id = "api.process.pr-tracking-issue" category = "process" -statement = "Every PR has a tracking issue in this repository, and the first line of its description is Fixes #ISSUE_NUMBER." +statement = "The first line of every PR's description is Fixes #ISSUE_NUMBER, naming an open issue, not a pull request, in this repository; automated PRs open or reuse such an issue." rationale = "docs/engineering/skills/github-prs.md#tracking-issues" -scope = [".github/pull_request_template.md"] +scope = [ + ".github/pull_request_template.md", + ".github/workflows/pr-description.yml", + ".github/scripts/check_pr_description.py", + ".github/scripts/open_relock_pull_request.sh", + ".github/scripts/update-policyengine-package.sh", +] tier = "mechanical" -status = "doc-only" +status = "enforced" autofix = false [rule.enforcer] -kind = "none" +kind = "workflow-job" +step = "Check the first line and the tracking issue" +[[rule.ci]] +workflow = ".github/workflows/pr-description.yml" +job = "pr-description" +invocation = "check_pr_description.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.process.pr-tracking-issue/bad_event.json" +good_fixture = "rules/fixtures/api.process.pr-tracking-issue/good_event.json" +test = "tests/unit/test_pr_description_check.py::test_rule_proof_pr_tracking_issue" +workflow = ".github/workflows/pr.yml" +job = "test" +invocation = "make test" [[rule]] id = "api.process.changelog-fragment" From 33e8b9e8fb95a4867e46081c5efd32077f36739d Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:03:14 +0400 Subject: [PATCH 3/8] Test one shared tracking issue for bundle-update PRs (failing) 27 of the 32 bundle-update PRs closed since 2026-10-01 were closed without merging, so an issue per version would be left open for each of them. Test that every bundle-update PR names the oldest open issue titled "Update the PolicyEngine bundle", or a new one, and that issues with other titles, including the per-version title, are not reused. - The fake gh now applies the script's --jq expression to a list of open issues with jq, so the tests exercise the selection and sort order. - run_update no longer passes the shell's GITHUB_REPOSITORY to the script, which made a test fail wherever that variable names another repository; a test passes one explicitly to check the script uses it. - Correct the comment on the moved-issue test: #154 is the simulation API's transferred issue, not this repository's. Three tests fail: the script still uses a per-version title. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_bundle_update_pins.py | 75 +++++++++++++++++++++---- tests/unit/test_pr_description_check.py | 4 +- 2 files changed, 67 insertions(+), 12 deletions(-) diff --git a/tests/unit/test_bundle_update_pins.py b/tests/unit/test_bundle_update_pins.py index e07979403..e7792155e 100644 --- a/tests/unit/test_bundle_update_pins.py +++ b/tests/unit/test_bundle_update_pins.py @@ -31,6 +31,7 @@ def update_checkout(tmp_path): import json import os from pathlib import Path +import subprocess import sys name = Path(sys.argv[0]).name @@ -47,7 +48,15 @@ def update_checkout(tmp_path): if name == "gh" and args[:2] == ["issue", "list"]: if os.environ.get("BUNDLE_TEST_ISSUE_LIST_FAIL"): sys.exit(1) - print(os.environ.get("BUNDLE_TEST_OPEN_ISSUE", "")) + # Apply the script's --jq expression to the open issues, as gh does. + filtered = subprocess.run( + ["jq", "-r", args[args.index("--jq") + 1]], + input=os.environ.get("BUNDLE_TEST_OPEN_ISSUES", "[]"), + capture_output=True, + text=True, + check=True, + ) + sys.stdout.write(filtered.stdout) if name == "gh" and args[:2] == ["issue", "create"]: print("https://github.com/PolicyEngine/policyengine-api/issues/4321") if name == "uv" and args[:2] == ["lock", "--upgrade-package"]: @@ -72,11 +81,16 @@ def update_checkout(tmp_path): def run_update(root, *arguments, **environment): log = root / "calls.jsonl" + # The runner's GITHUB_REPOSITORY would choose the tracking issue's + # repository; tests that need one pass it. + inherited = { + name: value for name, value in os.environ.items() if name != "GITHUB_REPOSITORY" + } result = subprocess.run( ["bash", str(SCRIPT), *arguments], cwd=root, env={ - **os.environ, + **inherited, "PATH": f"{root / 'bin'}{os.pathsep}{os.environ['PATH']}", "LATEST_OVERRIDE": TARGET_VERSION, "BUNDLE_TEST_CALLS": str(log), @@ -207,7 +221,10 @@ def test_existing_pull_request_is_found_by_exact_branch(update_checkout): # --- The tracking issue (rule api.process.pr-tracking-issue) --------------- -ISSUE_TITLE = f"Update PolicyEngine bundle to {TARGET_VERSION}" +# Every bundle-update pull request names the same open issue, so a pull +# request closed without merging, as superseded ones are, leaves no issue +# behind: the next one to merge closes it. +ISSUE_TITLE = "Update the PolicyEngine bundle" def _calls_to(calls, *command): @@ -220,6 +237,15 @@ def _pull_request_description(calls): return pull_request[pull_request.index("--body") + 1] +def _open_issues(*issues): + """Environment for the fake gh to list ``issues`` as (number, title) pairs.""" + return { + "BUNDLE_TEST_OPEN_ISSUES": json.dumps( + [{"number": number, "title": title} for number, title in issues] + ) + } + + def test_a_new_pull_request_names_a_new_tracking_issue_first(update_checkout): result, calls = run_update(update_checkout) @@ -228,6 +254,7 @@ def test_a_new_pull_request_names_a_new_tracking_issue_first(update_checkout): assert issue_lookup[issue_lookup.index("--repo") + 1] == ( "PolicyEngine/policyengine-api" ) + assert issue_lookup[issue_lookup.index("--state") + 1] == "open" (issue,) = _calls_to(calls, "issue", "create") assert issue[issue.index("--repo") + 1] == "PolicyEngine/policyengine-api" assert issue[issue.index("--title") + 1] == ISSUE_TITLE @@ -236,22 +263,48 @@ def test_a_new_pull_request_names_a_new_tracking_issue_first(update_checkout): assert calls.index(issue) < calls.index(_calls_to(calls, "pr", "create")[0]) -def test_a_new_pull_request_reuses_the_open_tracking_issue(update_checkout): - result, calls = run_update(update_checkout, BUNDLE_TEST_OPEN_ISSUE="4300") +def test_a_new_pull_request_reuses_the_oldest_open_tracking_issue(update_checkout): + result, calls = run_update( + update_checkout, + **_open_issues( + (4310, ISSUE_TITLE), + (4300, ISSUE_TITLE), + (4100, "Relock dependencies"), + ), + ) assert result.returncode == 0, result.stderr assert _calls_to(calls, "issue", "create") == [] assert _pull_request_description(calls).startswith("Fixes #4300\n\n## Summary") -def test_the_issue_lookup_matches_this_versions_title_only(update_checkout): - result, calls = run_update(update_checkout) +def test_issues_with_other_titles_are_not_reused(update_checkout): + result, calls = run_update( + update_checkout, + **_open_issues( + (4200, f"Update PolicyEngine bundle to {TARGET_VERSION}"), + (4100, "Relock dependencies"), + (4000, f"{ISSUE_TITLE} later"), + ), + ) assert result.returncode == 0, result.stderr - (issue_lookup,) = _calls_to(calls, "issue", "list") - assert issue_lookup[issue_lookup.index("--state") + 1] == "open" - query = issue_lookup[issue_lookup.index("--jq") + 1] - assert f'.title == "{ISSUE_TITLE}"' in query + (issue,) = _calls_to(calls, "issue", "create") + assert issue[issue.index("--title") + 1] == ISSUE_TITLE + assert _pull_request_description(calls).startswith("Fixes #4321\n\n## Summary") + + +def test_the_tracking_issue_is_in_the_repository_the_workflow_runs_in( + update_checkout, +): + result, calls = run_update( + update_checkout, GITHUB_REPOSITORY="PolicyEngine/policyengine-api-copy" + ) + + assert result.returncode == 0, result.stderr + for command in (("issue", "list"), ("issue", "create")): + (call,) = _calls_to(calls, *command) + assert call[call.index("--repo") + 1] == "PolicyEngine/policyengine-api-copy" def test_a_pull_request_for_an_existing_branch_names_its_tracking_issue_first( diff --git a/tests/unit/test_pr_description_check.py b/tests/unit/test_pr_description_check.py index 47446356f..4233d7208 100644 --- a/tests/unit/test_pr_description_check.py +++ b/tests/unit/test_pr_description_check.py @@ -371,7 +371,9 @@ def test_a_missing_issue_fails(fake_gh, tmp_path, gh_error): def test_an_issue_moved_to_another_repository_fails(fake_gh, tmp_path): # gh api follows GitHub's redirect for an issue transferred to another - # repository and prints the moved issue, here #115 in policyengine.py. + # repository and prints the moved issue, as it does for + # PolicyEngine/policyengine-sim-api#154, now PolicyEngine/policyengine.py#115. + # The fake gh gives that answer for #154 here. moved_issue = _issue(115, state="open", repository="PolicyEngine/policyengine.py") result = _run_check(fake_gh, tmp_path, "Fixes #154\n", **_gh_answers(moved_issue)) From 706081b87a153819bf6c4354467fcd9c73f1892f Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:04:27 +0400 Subject: [PATCH 4/8] Name one shared tracking issue in every bundle-update PR update-policyengine-package.sh now reuses the oldest open issue titled "Update the PolicyEngine bundle", or opens one, instead of an issue per version. Most bundle-update PRs are closed without merging when a newer version arrives, and an issue per version stayed open for each of them; with one shared issue, the first PR to merge closes it and the next update opens a new one. The PR skill and pr-description.yml no longer say the master ruleset already requires the PR description check: they say the ruleset has to require it by name for it to block a merge. The skill also says that a bundle-update PR opened before the rule needs its description edited by hand. Co-Authored-By: Claude Opus 5.5 --- .../scripts/update-policyengine-package.sh | 16 ++++++++------- .github/workflows/pr-description.yml | 3 ++- docs/engineering/skills/github-prs.md | 20 ++++++++++++------- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/.github/scripts/update-policyengine-package.sh b/.github/scripts/update-policyengine-package.sh index 8cdc75de7..e135d06c3 100755 --- a/.github/scripts/update-policyengine-package.sh +++ b/.github/scripts/update-policyengine-package.sh @@ -7,8 +7,10 @@ # # Every PR names its tracking issue on the first line of its description # (rule api.process.pr-tracking-issue): the oldest open issue titled -# "Update PolicyEngine bundle to ", or a new one when none is open. -# Merging the PR closes it. +# "Update the PolicyEngine bundle", or a new one when none is open. Every +# bundle-update PR shares it, so one closed without merging, as superseded +# ones are, leaves no issue open: the first to merge closes it, and the next +# update opens a new one. # # Environment: # GH_TOKEN must be set for gh: a token that can write issues and pull @@ -30,12 +32,12 @@ fi REPOSITORY="${GITHUB_REPOSITORY:-PolicyEngine/policyengine-api}" ISSUE_NUMBER="" -# Set ISSUE_NUMBER to the oldest open tracking issue for LATEST, opening one -# when none is open. This sets a variable rather than printing, because bash +# Set ISSUE_NUMBER to the oldest open tracking issue, opening one when none +# is open. This sets a variable rather than printing, because bash # turns off set -e inside $(...), and a failed lookup must not open a # duplicate issue. find_or_open_tracking_issue() { - local title="Update PolicyEngine bundle to ${LATEST}" + local title="Update the PolicyEngine bundle" local issue_number local issue_url @@ -52,13 +54,13 @@ find_or_open_tracking_issue() { gh issue create \ --repo "${REPOSITORY}" \ --title "${title}" \ - --body "Tracks the automated update of the PolicyEngine .py bundle to ${LATEST} by .github/workflows/update-policyengine-bundle.yaml. The update pull request names this issue on the first line of its description and closes it when it merges." + --body "Tracks the automated PolicyEngine .py bundle updates by .github/workflows/update-policyengine-bundle.yaml. Every open bundle-update pull request names this issue on the first line of its description; the first one to merge closes it, and the next update opens a new one." )" issue_number="${issue_url##*/}" fi if [[ ! "${issue_number}" =~ ^[0-9]+$ ]]; then - echo "Could not resolve the tracking issue for PolicyEngine bundle ${LATEST}: ${issue_number}" >&2 + echo "Could not resolve the bundle-update tracking issue: ${issue_number}" >&2 exit 1 fi ISSUE_NUMBER="${issue_number}" diff --git a/.github/workflows/pr-description.yml b/.github/workflows/pr-description.yml index 42bc5646d..989b9355e 100644 --- a/.github/workflows/pr-description.yml +++ b/.github/workflows/pr-description.yml @@ -8,7 +8,8 @@ name: PR description # this check, not the whole of CI. It has no branches or paths filter and no # stack condition: it is cheap, so every open pull request runs it, including # each one in a stack. CI complete in pr.yml can't need a job in another -# workflow, so the master ruleset requires this job's check on its own. +# workflow, so for this check to block a merge, the master ruleset has to +# require it by name. on: pull_request: diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index f8cab5741..7b5e0bf49 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -49,11 +49,17 @@ GITHUB_EVENT_PATH=/tmp/event.json GITHUB_REPOSITORY=PolicyEngine/policyengine-ap python .github/scripts/check_pr_description.py ``` -Automated pull requests follow the rule too. The PolicyEngine bundle update -(`update-policyengine-package.sh`) reuses the open "Update PolicyEngine bundle -to " issue or opens one. The daily relock (`open_relock_pull_request.sh`) -reuses the open "Relock dependencies" issue or opens one, and rewrites the -description of a relock pull request that is already open. +Automated pull requests follow the rule too: + +- The PolicyEngine bundle update (`update-policyengine-package.sh`) reuses the + oldest open "Update the PolicyEngine bundle" issue or opens one. Every open + bundle-update pull request names it, so closing a superseded one leaves no + issue behind; the first to merge closes it. A bundle-update pull request + opened before the rule has no `Fixes` line, and rerunning the workflow + doesn't add one: edit its description. +- The daily relock (`open_relock_pull_request.sh`) reuses the open "Relock + dependencies" issue or opens one, and rewrites the description of a relock + pull request that is already open. ## Changelog fragments @@ -129,8 +135,8 @@ stack therefore uses one set of runners instead of one per layer. passes only when every job passed. Skipped jobs would otherwise count as passing, so a stack can only merge bottom-up, and nothing merges untested. - **`PR description` is in a workflow of its own,** so `CI complete` can't - need it, and it has no stack condition. The master ruleset requires it as a - separate check. + need it, and it has no stack condition. For it to block a merge, the master + ruleset has to require it by name, next to `CI complete`. - **When the pull request below merges,** GitHub rebases the next one onto the trunk, and that push starts its full run. If no run starts, push again (`gh stack rebase`, then `gh stack push`), or close and reopen the pull From 4f6bf90a34980bda6504f8fbca22b01e39a266cd Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:20:45 +0400 Subject: [PATCH 5/8] Test per-version bundle issues closed with their PRs (failing) A tracking issue shared by every bundle-update PR is closed by the first one to merge, so the other open ones name a closed issue and fail the PR description check on their next push (#3918 merged while #3925 was open, and #3953 while #3954 was). Go back to one issue per version, and test a new script that closes a bundle-update PR's issue when the PR is closed without merging, so superseded updates leave no issue open: - it closes the open issue titled for the branch's version that the first line of the description names, with reason "not planned"; - it leaves merged PRs, other branches, descriptions without a Fixes first line, and issues with another title, closed or a pull request alone; - its workflow runs on closed pull requests, only for unmerged bundle-update branches from this repository, with issues: write. Also test that the bundle-update script resolves the issue after the lock and before the push, so a failed lookup pushes no branch, and that it says which lookup failed. 25 cases fail: the script and workflow don't exist, and the bundle-update script uses the shared title and looks up the issue after pushing. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_bundle_update_pins.py | 36 ++- .../test_close_bundle_update_issue_script.py | 263 ++++++++++++++++++ 2 files changed, 292 insertions(+), 7 deletions(-) create mode 100644 tests/unit/test_close_bundle_update_issue_script.py diff --git a/tests/unit/test_bundle_update_pins.py b/tests/unit/test_bundle_update_pins.py index e7792155e..4a87d0eeb 100644 --- a/tests/unit/test_bundle_update_pins.py +++ b/tests/unit/test_bundle_update_pins.py @@ -221,10 +221,11 @@ def test_existing_pull_request_is_found_by_exact_branch(update_checkout): # --- The tracking issue (rule api.process.pr-tracking-issue) --------------- -# Every bundle-update pull request names the same open issue, so a pull -# request closed without merging, as superseded ones are, leaves no issue -# behind: the next one to merge closes it. -ISSUE_TITLE = "Update the PolicyEngine bundle" +# Each version's pull request names an issue of its own: merging closes it +# through the Fixes line, and close-bundle-update-issue.yml closes it when the +# pull request is closed without merging. No open pull request names an issue +# that another pull request's merge closed. +ISSUE_TITLE = f"Update PolicyEngine bundle to {TARGET_VERSION}" def _calls_to(calls, *command): @@ -260,7 +261,23 @@ def test_a_new_pull_request_names_a_new_tracking_issue_first(update_checkout): assert issue[issue.index("--title") + 1] == ISSUE_TITLE description = _pull_request_description(calls) assert description.split("\n")[:3] == ["Fixes #4321", "", "## Summary"] - assert calls.index(issue) < calls.index(_calls_to(calls, "pr", "create")[0]) + + +def test_the_tracking_issue_is_resolved_after_the_lock_and_before_the_push( + update_checkout, +): + """A failed lock opens no issue, and a failed lookup pushes no branch.""" + result, calls = run_update(update_checkout) + + assert result.returncode == 0, result.stderr + order = [ + calls.index(["uv", "lock", "--check"]), + calls.index(_calls_to(calls, "issue", "list")[0]), + calls.index(_calls_to(calls, "issue", "create")[0]), + next(index for index, call in enumerate(calls) if call[:2] == ["git", "push"]), + calls.index(_calls_to(calls, "pr", "create")[0]), + ] + assert order == sorted(order) def test_a_new_pull_request_reuses_the_oldest_open_tracking_issue(update_checkout): @@ -282,7 +299,8 @@ def test_issues_with_other_titles_are_not_reused(update_checkout): result, calls = run_update( update_checkout, **_open_issues( - (4200, f"Update PolicyEngine bundle to {TARGET_VERSION}"), + (4200, "Update PolicyEngine bundle to 4.0.9"), + (4150, "Update the PolicyEngine bundle"), (4100, "Relock dependencies"), (4000, f"{ISSUE_TITLE} later"), ), @@ -318,9 +336,13 @@ def test_a_pull_request_for_an_existing_branch_names_its_tracking_issue_first( assert _pull_request_description(calls).startswith("Fixes #4321\n\n## Summary") -def test_a_failed_issue_lookup_opens_no_issue_and_no_pull_request(update_checkout): +def test_a_failed_issue_lookup_pushes_nothing_and_opens_nothing(update_checkout): result, calls = run_update(update_checkout, BUNDLE_TEST_ISSUE_LIST_FAIL="1") assert result.returncode != 0 + assert "Could not list the open issues in PolicyEngine/policyengine-api" in ( + result.stderr + ) + assert not any(call[:2] in (["git", "commit"], ["git", "push"]) for call in calls) assert _calls_to(calls, "issue", "create") == [] assert _calls_to(calls, "pr", "create") == [] diff --git a/tests/unit/test_close_bundle_update_issue_script.py b/tests/unit/test_close_bundle_update_issue_script.py new file mode 100644 index 000000000..9c4018f79 --- /dev/null +++ b/tests/unit/test_close_bundle_update_issue_script.py @@ -0,0 +1,263 @@ +"""Tests for .github/scripts/close_bundle_update_issue.sh and its workflow. + +Each PolicyEngine bundle-update pull request names an issue of its own on the +first line of its description (rule api.process.pr-tracking-issue). Merging +the pull request closes that issue; when the pull request is closed without +merging, as superseded updates are, close-bundle-update-issue.yml runs the +script to close it. A fake ``gh`` first on PATH records the calls and answers +the issue lookup, so no test reaches GitHub. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +from pathlib import Path + +import pytest +import yaml + +REPO = Path(__file__).resolve().parents[2] +SCRIPT = REPO / ".github" / "scripts" / "close_bundle_update_issue.sh" +WORKFLOW = REPO / ".github" / "workflows" / "close-bundle-update-issue.yml" +JOB = "close-tracking-issue" +REPOSITORY = "PolicyEngine/policyengine-api" +BRANCH = "auto/update-policyengine-bundle-6.12.1" +ISSUE_TITLE = "Update PolicyEngine bundle to 6.12.1" +DESCRIPTION = "Fixes #4320\n\n## Summary\n\nUpdate PolicyEngine .py bundle.\n" + +# Records its arguments; for gh api, prints FAKE_GH_ISSUE, or fails when +# FAKE_GH_API_FAIL is set. +FAKE_GH = f"""#!{sys.executable} +import json +import os +import sys + +arguments = sys.argv[1:] +with open(os.environ["FAKE_GH_CALLS"], "a", encoding="utf-8") as calls: + calls.write(json.dumps(["gh", *arguments]) + "\\n") +if arguments[:1] == ["api"]: + if os.environ.get("FAKE_GH_API_FAIL"): + sys.stderr.write("gh: Bad credentials (HTTP 401)\\n") + sys.exit(1) + sys.stdout.write(os.environ.get("FAKE_GH_ISSUE", "{{}}")) +""" + + +def _issue( + number: int = 4320, *, title: str = ISSUE_TITLE, state: str = "open", **fields +) -> dict: + return {"number": number, "title": title, "state": state, **fields} + + +def _event( + *, + body: str | None = DESCRIPTION, + branch: str = BRANCH, + merged: bool = False, +) -> dict: + return { + "action": "closed", + "number": 4163, + "pull_request": { + "number": 4163, + "state": "closed", + "merged": merged, + "body": body, + "head": {"ref": branch, "repo": {"full_name": REPOSITORY}}, + }, + } + + +def _close( + tmp_path: Path, event: dict, issue: dict | None = None, **environment: str +) -> tuple[subprocess.CompletedProcess[str], list[list[str]]]: + """Run the script on ``event``; return the result and the gh calls.""" + binaries = tmp_path / "bin" + binaries.mkdir(exist_ok=True) + fake_gh = binaries / "gh" + fake_gh.write_text(FAKE_GH, encoding="utf-8") + fake_gh.chmod(0o755) + event_path = tmp_path / "event.json" + event_path.write_text(json.dumps(event), encoding="utf-8") + calls_log = tmp_path / "gh-calls.jsonl" + run_environment = { + name: value + for name, value in os.environ.items() + if not name.startswith(("GITHUB_", "FAKE_GH_", "GH_")) + } + run_environment.update( + { + "PATH": f"{binaries}{os.pathsep}{os.environ['PATH']}", + "FAKE_GH_CALLS": str(calls_log), + "FAKE_GH_ISSUE": json.dumps(issue if issue is not None else _issue()), + "GITHUB_EVENT_PATH": str(event_path), + "GITHUB_REPOSITORY": REPOSITORY, + **environment, + } + ) + result = subprocess.run( + ["bash", str(SCRIPT)], + env=run_environment, + text=True, + capture_output=True, + check=False, + ) + calls = ( + [json.loads(call) for call in calls_log.read_text().splitlines()] + if calls_log.exists() + else [] + ) + return result, calls + + +def _closes(calls: list[list[str]]) -> list[list[str]]: + return [call for call in calls if call[1:3] == ["issue", "close"]] + + +# --- Closing the issue ---------------------------------------------------- + + +def test_closes_the_issue_an_unmerged_bundle_update_names(tmp_path): + result, calls = _close(tmp_path, _event()) + + assert result.returncode == 0, result.stderr + assert calls[0] == ["gh", "api", f"repos/{REPOSITORY}/issues/4320"] + (close,) = _closes(calls) + assert close[:4] == ["gh", "issue", "close", "4320"] + assert close[close.index("--repo") + 1] == REPOSITORY + assert close[close.index("--reason") + 1] == "not planned" + assert "#4163 was closed without merging" in close[close.index("--comment") + 1] + assert "Closed issue #4320" in result.stdout + + +def test_a_crlf_description_names_the_issue_too(tmp_path): + result, calls = _close(tmp_path, _event(body="Fixes #4320 \r\n\r\n## Summary\r\n")) + + assert result.returncode == 0, result.stderr + assert len(_closes(calls)) == 1 + + +# --- Leaving the issue alone ---------------------------------------------- + + +@pytest.mark.parametrize( + "event", + [ + # Merging closed the issue through the Fixes line. + _event(merged=True), + # Not a bundle-update pull request. + _event(branch="pr-description-check"), + _event(branch="auto/relock-dependencies"), + ], +) +def test_other_closed_pull_requests_are_left_alone(tmp_path, event): + result, calls = _close(tmp_path, event) + + assert result.returncode == 0, result.stderr + assert calls == [] + + +@pytest.mark.parametrize( + "body", + [None, "", "## Summary\n\nUpdate PolicyEngine .py bundle.\n", "Fixes #4320."], +) +def test_a_description_without_a_fixes_first_line_closes_nothing(tmp_path, body): + result, calls = _close(tmp_path, _event(body=body)) + + assert result.returncode == 0, result.stderr + assert "names no tracking issue" in result.stdout + assert calls == [] + + +@pytest.mark.parametrize( + ("issue", "expected_message"), + [ + # Someone edited the line to name another version's issue. + ( + _issue(title="Update PolicyEngine bundle to 6.13.0"), + 'is titled "Update PolicyEngine bundle to 6.13.0"', + ), + (_issue(title="Relock dependencies"), 'is titled "Relock dependencies"'), + (_issue(state="closed"), "is already closed"), + ( + _issue(pull_request={"url": "https://api.github.com/pulls/4320"}), + "is a pull request", + ), + ], +) +def test_an_issue_that_isnt_this_versions_open_issue_is_left_open( + tmp_path, issue, expected_message +): + result, calls = _close(tmp_path, _event(), issue) + + assert result.returncode == 0, result.stderr + assert expected_message in result.stdout + assert _closes(calls) == [] + + +# --- Failures ------------------------------------------------------------- + + +def test_a_failed_issue_lookup_fails_and_closes_nothing(tmp_path): + result, calls = _close(tmp_path, _event(), FAKE_GH_API_FAIL="1") + + assert result.returncode != 0 + assert "Could not look up issue #4320" in result.stderr + assert _closes(calls) == [] + + +@pytest.mark.parametrize("variable", ["GITHUB_EVENT_PATH", "GITHUB_REPOSITORY"]) +def test_the_script_requires_its_environment(tmp_path, variable): + result, calls = _close(tmp_path, _event(), **{variable: ""}) + + assert result.returncode != 0 + assert f"{variable} is required" in result.stderr + assert calls == [] + + +# --- The workflow --------------------------------------------------------- + + +@pytest.fixture(scope="module") +def workflow() -> dict: + return yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + + +def test_the_workflow_runs_when_a_pull_request_closes(workflow): + # PyYAML reads the key "on" as True. + assert workflow.get("on", workflow.get(True)) == { + "pull_request": {"types": ["closed"]} + } + + +def test_the_job_runs_only_for_unmerged_bundle_updates_from_this_repository( + workflow, +): + assert list(workflow["jobs"]) == [JOB] + assert workflow["jobs"][JOB]["if"] == ( + "${{ !github.event.pull_request.merged" + " && startsWith(github.event.pull_request.head.ref," + " 'auto/update-policyengine-bundle-')" + " && github.event.pull_request.head.repo.full_name == github.repository }}" + ) + + +def test_the_job_token_can_write_issues_and_only_read_the_rest(workflow): + assert workflow["permissions"] == {"contents": "read", "issues": "write"} + assert "permissions" not in workflow["jobs"][JOB] + + +def test_the_job_runs_the_tested_script_with_the_job_token(workflow): + steps = workflow["jobs"][JOB]["steps"] + checkout = next( + step for step in steps if step.get("uses", "").startswith("actions/checkout@") + ) + script_steps = [step for step in steps if SCRIPT.name in step.get("run", "")] + + assert checkout["with"]["sparse-checkout"] == f".github/scripts/{SCRIPT.name}" + assert len(script_steps) == 1 + assert script_steps[0]["run"] == f"bash .github/scripts/{SCRIPT.name}" + assert script_steps[0]["env"] == {"GH_TOKEN": "${{ github.token }}"} From a66395629b4f28e71e9613ff54ed3cee73783970 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:23:13 +0400 Subject: [PATCH 6/8] Close a bundle-update PR's issue when the PR closes unmerged update-policyengine-package.sh goes back to one tracking issue per version, titled "Update PolicyEngine bundle to ". A shared issue was closed by the first bundle-update PR to merge, which left the other open ones naming a closed issue. - New workflow close-bundle-update-issue.yml runs on closed pull requests from auto/update-policyengine-bundle-* branches in this repository that weren't merged, and runs close_bundle_update_issue.sh with the job token (issues: write). The script closes the issue the description's first line names, as "not planned", only when it is open and titled for the branch's version, so superseded updates leave no issue open. - The bundle-update script resolves the issue after the lock and the version check and before it creates and pushes the branch, so a failed lookup no longer leaves a pushed branch without a PR, and it says which issue call failed. - The PR skill describes both, and the rule's scope lists the new files. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/close_bundle_update_issue.sh | 84 +++++++++++++++++++ .../scripts/update-policyengine-package.sh | 37 ++++---- .../workflows/close-bundle-update-issue.yml | 34 ++++++++ docs/engineering/skills/github-prs.md | 12 +-- rules/registry.toml | 2 + 5 files changed, 150 insertions(+), 19 deletions(-) create mode 100755 .github/scripts/close_bundle_update_issue.sh create mode 100644 .github/workflows/close-bundle-update-issue.yml diff --git a/.github/scripts/close_bundle_update_issue.sh b/.github/scripts/close_bundle_update_issue.sh new file mode 100755 index 000000000..0f37703da --- /dev/null +++ b/.github/scripts/close_bundle_update_issue.sh @@ -0,0 +1,84 @@ +#!/usr/bin/env bash + +# Close the tracking issue of a PolicyEngine bundle-update pull request that +# was closed without merging. +# +# Each bundle-update pull request names an issue of its own on the first line +# of its description (rule api.process.pr-tracking-issue; see +# update-policyengine-package.sh). Merging the pull request closes it through +# the Fixes line. Superseded updates are closed without merging, so this +# script, run by close-bundle-update-issue.yml, closes their issues instead. +# +# It closes the issue only when the pull request came from +# auto/update-policyengine-bundle-, was not merged, and the first +# line of its description is "Fixes #N" for an open issue titled +# "Update PolicyEngine bundle to ". Otherwise it says why it left +# the issue alone and exits 0. It exits 1 when the issue can't be read or +# closed. +# +# Environment: +# GITHUB_EVENT_PATH the pull_request event payload (set by the runner) +# GITHUB_REPOSITORY owner/name of this repository (set by the runner) +# GH_TOKEN read by gh: a token that can write issues + +set -euo pipefail + +: "${GITHUB_EVENT_PATH:?GITHUB_EVENT_PATH is required}" +: "${GITHUB_REPOSITORY:?GITHUB_REPOSITORY is required}" + +branch_prefix="auto/update-policyengine-bundle-" +# ASCII digits only, without a leading zero, as the PR description check reads +# them; a bracket range could match other digits in some locales. +fixes_line='^Fixes #([123456789][0123456789]*)$' + +pull_request="$(jq -r '.pull_request.number' "${GITHUB_EVENT_PATH}")" +merged="$(jq -r '.pull_request.merged' "${GITHUB_EVENT_PATH}")" +branch="$(jq -r '.pull_request.head.ref // ""' "${GITHUB_EVENT_PATH}")" +description="$(jq -r '.pull_request.body // ""' "${GITHUB_EVENT_PATH}")" + +if [[ "${merged}" == "true" ]]; then + echo "Pull request #${pull_request} was merged, which closed its issue." + exit 0 +fi +if [[ "${branch}" != "${branch_prefix}"* ]]; then + echo "Pull request #${pull_request} is not a bundle update (${branch}); nothing to close." + exit 0 +fi +title="Update PolicyEngine bundle to ${branch#"${branch_prefix}"}" + +# The first line ends at a line feed or a carriage return; trailing spaces +# and tabs don't count. +first_line="${description%%$'\n'*}" +first_line="${first_line%%$'\r'*}" +while [[ "${first_line}" == *[$' \t'] ]]; do + first_line="${first_line%?}" +done +if [[ ! "${first_line}" =~ ${fixes_line} ]]; then + echo "Pull request #${pull_request}'s description names no tracking issue on its first line; nothing to close." + exit 0 +fi +issue="${BASH_REMATCH[1]}" + +if ! answer="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${issue}")"; then + echo "Could not look up issue #${issue} in ${GITHUB_REPOSITORY}." >&2 + exit 1 +fi +if [[ "$(jq -r '.pull_request != null' <<<"${answer}")" == "true" ]]; then + echo "#${issue} is a pull request, not an issue; nothing to close." + exit 0 +fi +issue_title="$(jq -r '.title // ""' <<<"${answer}")" +if [[ "${issue_title}" != "${title}" ]]; then + echo "Issue #${issue} is titled \"${issue_title}\", not \"${title}\"; leaving it open." + exit 0 +fi +if [[ "$(jq -r '.state // ""' <<<"${answer}")" != "open" ]]; then + echo "Issue #${issue} is already closed." + exit 0 +fi + +gh issue close "${issue}" \ + --repo "${GITHUB_REPOSITORY}" \ + --reason "not planned" \ + --comment "The bundle-update pull request #${pull_request} was closed without merging, so this update won't happen. A later update has an issue of its own." +echo "Closed issue #${issue}, which #${pull_request} named." diff --git a/.github/scripts/update-policyengine-package.sh b/.github/scripts/update-policyengine-package.sh index e135d06c3..f41ca43ff 100755 --- a/.github/scripts/update-policyengine-package.sh +++ b/.github/scripts/update-policyengine-package.sh @@ -7,10 +7,12 @@ # # Every PR names its tracking issue on the first line of its description # (rule api.process.pr-tracking-issue): the oldest open issue titled -# "Update the PolicyEngine bundle", or a new one when none is open. Every -# bundle-update PR shares it, so one closed without merging, as superseded -# ones are, leaves no issue open: the first to merge closes it, and the next -# update opens a new one. +# "Update PolicyEngine bundle to ", or a new one when none is open. +# Each version has its own issue. Merging the PR closes it through the Fixes +# line; close-bundle-update-issue.yml closes it when the PR is closed without +# merging, as superseded updates are. The issue is resolved after the lock +# and before the branch is pushed, so a failed update opens no issue and a +# failed lookup pushes no branch. # # Environment: # GH_TOKEN must be set for gh: a token that can write issues and pull @@ -32,35 +34,41 @@ fi REPOSITORY="${GITHUB_REPOSITORY:-PolicyEngine/policyengine-api}" ISSUE_NUMBER="" -# Set ISSUE_NUMBER to the oldest open tracking issue, opening one when none -# is open. This sets a variable rather than printing, because bash +# Set ISSUE_NUMBER to the oldest open tracking issue for LATEST, opening one +# when none is open. This sets a variable rather than printing, because bash # turns off set -e inside $(...), and a failed lookup must not open a # duplicate issue. find_or_open_tracking_issue() { - local title="Update the PolicyEngine bundle" + local title="Update PolicyEngine bundle to ${LATEST}" local issue_number local issue_url - issue_number="$( + if ! issue_number="$( gh issue list \ --repo "${REPOSITORY}" \ --state open \ --limit 1000 \ --json number,title \ --jq "map(select(.title == \"${title}\")) | sort_by(.number) | .[0].number // empty" - )" + )"; then + echo "Could not list the open issues in ${REPOSITORY} to find \"${title}\"." >&2 + exit 1 + fi if [[ -z "${issue_number}" ]]; then - issue_url="$( + if ! issue_url="$( gh issue create \ --repo "${REPOSITORY}" \ --title "${title}" \ - --body "Tracks the automated PolicyEngine .py bundle updates by .github/workflows/update-policyengine-bundle.yaml. Every open bundle-update pull request names this issue on the first line of its description; the first one to merge closes it, and the next update opens a new one." - )" + --body "Tracks the automated update of the PolicyEngine .py bundle to ${LATEST} by .github/workflows/update-policyengine-bundle.yaml. The update pull request names this issue on the first line of its description. Merging the pull request closes this issue, and close-bundle-update-issue.yml closes it if the pull request is closed without merging." + )"; then + echo "Could not open the issue \"${title}\" in ${REPOSITORY}." >&2 + exit 1 + fi issue_number="${issue_url##*/}" fi if [[ ! "${issue_number}" =~ ^[0-9]+$ ]]; then - echo "Could not resolve the bundle-update tracking issue: ${issue_number}" >&2 + echo "Could not resolve the tracking issue \"${title}\": ${issue_number}" >&2 exit 1 fi ISSUE_NUMBER="${issue_number}" @@ -207,6 +215,8 @@ if [[ "$POLICYENGINE_VERSION" != "$LATEST" ]]; then exit 1 fi +find_or_open_tracking_issue + FRAGMENT="changelog.d/update-policyengine-bundle-${LATEST}.changed.md" echo "Update the PolicyEngine bundle to ${LATEST}." > "$FRAGMENT" @@ -219,7 +229,6 @@ git add pyproject.toml docker/Dockerfile uv.lock "$FRAGMENT" git commit -m "Update PolicyEngine bundle to ${LATEST}" git push -u origin "$BRANCH" -find_or_open_tracking_issue gh pr create \ --base master \ --title "Update PolicyEngine bundle to ${LATEST}" \ diff --git a/.github/workflows/close-bundle-update-issue.yml b/.github/workflows/close-bundle-update-issue.yml new file mode 100644 index 000000000..62939dec9 --- /dev/null +++ b/.github/workflows/close-bundle-update-issue.yml @@ -0,0 +1,34 @@ +name: Close superseded bundle-update issues + +# Each PolicyEngine bundle-update pull request names an issue of its own on +# the first line of its description (rule api.process.pr-tracking-issue). +# Merging the pull request closes it; superseded updates are closed without +# merging, so this job closes their issues. See +# .github/scripts/close_bundle_update_issue.sh. + +on: + pull_request: + types: [closed] + +# The job token closes the issue. Pull requests from forks get a read-only +# token, and the job's condition skips them anyway. +permissions: + contents: read + issues: write + +jobs: + close-tracking-issue: + name: Close the tracking issue of an unmerged bundle update + if: ${{ !github.event.pull_request.merged && startsWith(github.event.pull_request.head.ref, 'auto/update-policyengine-bundle-') && github.event.pull_request.head.repo.full_name == github.repository }} + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Checkout the script + uses: actions/checkout@v4 + with: + sparse-checkout: .github/scripts/close_bundle_update_issue.sh + sparse-checkout-cone-mode: false + - name: Close the issue the first line of the description names + env: + GH_TOKEN: ${{ github.token }} + run: bash .github/scripts/close_bundle_update_issue.sh diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 7b5e0bf49..4aa1f1330 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -52,11 +52,13 @@ GITHUB_EVENT_PATH=/tmp/event.json GITHUB_REPOSITORY=PolicyEngine/policyengine-ap Automated pull requests follow the rule too: - The PolicyEngine bundle update (`update-policyengine-package.sh`) reuses the - oldest open "Update the PolicyEngine bundle" issue or opens one. Every open - bundle-update pull request names it, so closing a superseded one leaves no - issue behind; the first to merge closes it. A bundle-update pull request - opened before the rule has no `Fixes` line, and rerunning the workflow - doesn't add one: edit its description. + oldest open "Update PolicyEngine bundle to " issue or opens one, so + each version's pull request has an issue of its own. Merging the pull + request closes it; closing the pull request without merging, as when a + newer version supersedes it, runs `close-bundle-update-issue.yml`, which + closes it too. A bundle-update pull request opened before the rule has no + `Fixes` line, and rerunning the workflow doesn't add one: edit its + description. - The daily relock (`open_relock_pull_request.sh`) reuses the open "Relock dependencies" issue or opens one, and rewrites the description of a relock pull request that is already open. diff --git a/rules/registry.toml b/rules/registry.toml index 4decc375d..77d9b17d8 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -1395,6 +1395,8 @@ scope = [ ".github/scripts/check_pr_description.py", ".github/scripts/open_relock_pull_request.sh", ".github/scripts/update-policyengine-package.sh", + ".github/scripts/close_bundle_update_issue.sh", + ".github/workflows/close-bundle-update-issue.yml", ] tier = "mechanical" status = "enforced" From bf6b6e4273f5744c11fa11679cb4e76d991d4060 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:41:56 +0400 Subject: [PATCH 7/8] Test the closing job on conflicting PRs and unused issues (failing) - GitHub doesn't run pull_request workflows while a pull request has a merge conflict, and a superseded bundle update usually conflicts with master: 14 of the 28 closed without merging since 2026-10-01 did when they were closed. Test that close-bundle-update-issue.yml listens for pull_request_target, checks out only the default branch's script, and can read pull requests, which gh issue close needs to look the number up. - Test that the bundle-update script closes the issue it opened when its push is rejected (as when a concurrent run for the version pushed first) or gh pr create fails, unless an open pull request for the branch names the issue, and never closes an issue it reused. - Test that the PR description check's message gives the skill's path and anchor as one link. 6 cases fail against the current workflow and scripts. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_bundle_update_pins.py | 75 +++++++++++++++++++ .../test_close_bundle_update_issue_script.py | 23 +++++- tests/unit/test_pr_description_check.py | 1 + 3 files changed, 96 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_bundle_update_pins.py b/tests/unit/test_bundle_update_pins.py index 4a87d0eeb..ee0002228 100644 --- a/tests/unit/test_bundle_update_pins.py +++ b/tests/unit/test_bundle_update_pins.py @@ -40,6 +40,24 @@ def update_checkout(tmp_path): stream.write(json.dumps([name, *args]) + "\\n") if name == "git" and args[:1] == ["ls-remote"]: sys.exit(0 if os.environ.get("BUNDLE_TEST_REMOTE_BRANCH") else 2) +if name == "git" and args[:1] == ["push"]: + sys.exit(1 if os.environ.get("BUNDLE_TEST_PUSH_FAIL") else 0) +if name == "gh" and args[:2] == ["pr", "create"]: + sys.exit(1 if os.environ.get("BUNDLE_TEST_PR_CREATE_FAIL") else 0) +if name == "gh" and args[:2] == ["pr", "view"] and "state,body" in args: + # The lookup after a failed gh pr create: apply its --jq expression to + # the pull request GitHub has for the branch, if any. + if not os.environ.get("BUNDLE_TEST_PR_AFTER_CREATE"): + sys.exit(1) + filtered = subprocess.run( + ["jq", "-r", args[args.index("--jq") + 1]], + input=os.environ["BUNDLE_TEST_PR_AFTER_CREATE"], + capture_output=True, + text=True, + check=True, + ) + sys.stdout.write(filtered.stdout) + sys.exit(0) if name == "gh" and args[:2] == ["pr", "view"]: if os.environ.get("BUNDLE_TEST_OPEN_PR"): print(os.environ["BUNDLE_TEST_OPEN_PR"]) @@ -346,3 +364,60 @@ def test_a_failed_issue_lookup_pushes_nothing_and_opens_nothing(update_checkout) assert not any(call[:2] in (["git", "commit"], ["git", "push"]) for call in calls) assert _calls_to(calls, "issue", "create") == [] assert _calls_to(calls, "pr", "create") == [] + + +# --- An issue no pull request will name ------------------------------------- + + +def test_a_rejected_push_closes_the_issue_this_run_opened(update_checkout): + """A concurrent run for the same version pushed first; its PR has an issue.""" + result, calls = run_update(update_checkout, BUNDLE_TEST_PUSH_FAIL="1") + + assert result.returncode != 0 + assert _calls_to(calls, "pr", "create") == [] + (close,) = _calls_to(calls, "issue", "close") + assert close[3] == "4321" + assert close[close.index("--repo") + 1] == "PolicyEngine/policyengine-api" + assert close[close.index("--reason") + 1] == "not planned" + + +def test_a_rejected_push_leaves_a_reused_issue_open(update_checkout): + result, calls = run_update( + update_checkout, + BUNDLE_TEST_PUSH_FAIL="1", + **_open_issues((4300, ISSUE_TITLE)), + ) + + assert result.returncode != 0 + assert _calls_to(calls, "issue", "close") == [] + + +@pytest.mark.parametrize("remote_branch", ["", "1"]) +def test_a_failed_pull_request_closes_the_issue_this_run_opened( + update_checkout, remote_branch +): + result, calls = run_update( + update_checkout, + BUNDLE_TEST_PR_CREATE_FAIL="1", + BUNDLE_TEST_REMOTE_BRANCH=remote_branch, + ) + + assert result.returncode != 0 + (close,) = _calls_to(calls, "issue", "close") + assert close[3] == "4321" + + +def test_a_failed_pull_request_keeps_an_issue_an_open_pull_request_names( + update_checkout, +): + """The pull request exists although gh pr create reported a failure.""" + result, calls = run_update( + update_checkout, + BUNDLE_TEST_PR_CREATE_FAIL="1", + BUNDLE_TEST_PR_AFTER_CREATE=json.dumps( + {"state": "OPEN", "body": "Fixes #4321\n\n## Summary\n"} + ), + ) + + assert result.returncode != 0 + assert _calls_to(calls, "issue", "close") == [] diff --git a/tests/unit/test_close_bundle_update_issue_script.py b/tests/unit/test_close_bundle_update_issue_script.py index 9c4018f79..aaecd6cf1 100644 --- a/tests/unit/test_close_bundle_update_issue_script.py +++ b/tests/unit/test_close_bundle_update_issue_script.py @@ -226,10 +226,18 @@ def workflow() -> dict: return yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) -def test_the_workflow_runs_when_a_pull_request_closes(workflow): +def test_the_workflow_runs_when_a_pull_request_closes_even_with_conflicts( + workflow, +): + """pull_request workflows don't run while a pull request has a conflict. + + A superseded bundle update usually conflicts with master, because the + newer one that merged changed the same pins, so the workflow listens for + pull_request_target, which GitHub runs anyway, from the default branch. + """ # PyYAML reads the key "on" as True. assert workflow.get("on", workflow.get(True)) == { - "pull_request": {"types": ["closed"]} + "pull_request_target": {"types": ["closed"]} } @@ -246,7 +254,12 @@ def test_the_job_runs_only_for_unmerged_bundle_updates_from_this_repository( def test_the_job_token_can_write_issues_and_only_read_the_rest(workflow): - assert workflow["permissions"] == {"contents": "read", "issues": "write"} + # gh issue close looks the number up as an issue or a pull request. + assert workflow["permissions"] == { + "contents": "read", + "issues": "write", + "pull-requests": "read", + } assert "permissions" not in workflow["jobs"][JOB] @@ -258,6 +271,10 @@ def test_the_job_runs_the_tested_script_with_the_job_token(workflow): script_steps = [step for step in steps if SCRIPT.name in step.get("run", "")] assert checkout["with"]["sparse-checkout"] == f".github/scripts/{SCRIPT.name}" + # pull_request_target checks out the default branch unless told otherwise; + # the job must never run the pull request's code with a token that writes. + assert "ref" not in checkout["with"] + assert "repository" not in checkout["with"] assert len(script_steps) == 1 assert script_steps[0]["run"] == f"bash .github/scripts/{SCRIPT.name}" assert script_steps[0]["env"] == {"GH_TOKEN": "${{ github.token }}"} diff --git a/tests/unit/test_pr_description_check.py b/tests/unit/test_pr_description_check.py index 4233d7208..d3af5de8d 100644 --- a/tests/unit/test_pr_description_check.py +++ b/tests/unit/test_pr_description_check.py @@ -143,6 +143,7 @@ def test_a_description_without_a_fixes_line_is_rejected(check_module): assert 'The first line is "## Summary"' in message assert "first line is exactly Fixes #ISSUE_NUMBER" in message + assert "docs/engineering/skills/github-prs.md#tracking-issues" in message assert "api.process.pr-tracking-issue" in message From 88a8a885bed53027a73be1a4e72a2ee23cb446c1 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:43:56 +0400 Subject: [PATCH 8/8] Close superseded bundle issues on conflicting PRs; close unused ones - close-bundle-update-issue.yml now runs on pull_request_target, which GitHub runs even when the pull request conflicts with master, as most superseded bundle updates do; pull_request workflows don't run then. It uses the default branch's workflow and script, checks out nothing from the pull request, and reads only the event payload. Its token can also read pull requests, which gh issue close needs to look the number up. - update-policyengine-package.sh closes the tracking issue it opened when its push is rejected or gh pr create fails, unless an open pull request for the branch names it, so a concurrent run for the same version or a failed run leaves no issue that no pull request names. It never closes an issue it reused. - The PR description check's message gives the skill's path and anchor as one link. - The PR skill says to reopen a bundle-update PR's issue along with the PR, and what a PR into a branch that predates the check sees. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/check_pr_description.py | 4 +- .github/scripts/close_bundle_update_issue.sh | 4 +- .../scripts/update-policyengine-package.sh | 55 +++++++++++++++++-- .../workflows/close-bundle-update-issue.yml | 15 ++++- docs/engineering/skills/github-prs.md | 18 ++++-- 5 files changed, 80 insertions(+), 16 deletions(-) diff --git a/.github/scripts/check_pr_description.py b/.github/scripts/check_pr_description.py index 01af3fccc..98235994d 100644 --- a/.github/scripts/check_pr_description.py +++ b/.github/scripts/check_pr_description.py @@ -45,8 +45,8 @@ HOW_TO_FIX = ( "Edit the description so its first line is exactly Fixes #ISSUE_NUMBER, " "where ISSUE_NUMBER is an open issue in this repository; editing the " - "description runs this check again. See docs/engineering/skills/github-prs.md " - f"#tracking-issues (rule {RULE_ID})." + "description runs this check again. See " + f"docs/engineering/skills/github-prs.md#tracking-issues (rule {RULE_ID})." ) RETRY_LOOKUP = ( "Re-run this job; if it fails again, check the GH_TOKEN the workflow gives it." diff --git a/.github/scripts/close_bundle_update_issue.sh b/.github/scripts/close_bundle_update_issue.sh index 0f37703da..bde943fe8 100755 --- a/.github/scripts/close_bundle_update_issue.sh +++ b/.github/scripts/close_bundle_update_issue.sh @@ -7,7 +7,9 @@ # of its description (rule api.process.pr-tracking-issue; see # update-policyengine-package.sh). Merging the pull request closes it through # the Fixes line. Superseded updates are closed without merging, so this -# script, run by close-bundle-update-issue.yml, closes their issues instead. +# script, run by close-bundle-update-issue.yml on pull_request_target, closes +# their issues instead. It reads only the event payload, never the pull +# request's code. # # It closes the issue only when the pull request came from # auto/update-policyengine-bundle-, was not merged, and the first diff --git a/.github/scripts/update-policyengine-package.sh b/.github/scripts/update-policyengine-package.sh index f41ca43ff..fafc9d831 100755 --- a/.github/scripts/update-policyengine-package.sh +++ b/.github/scripts/update-policyengine-package.sh @@ -12,7 +12,9 @@ # line; close-bundle-update-issue.yml closes it when the PR is closed without # merging, as superseded updates are. The issue is resolved after the lock # and before the branch is pushed, so a failed update opens no issue and a -# failed lookup pushes no branch. +# failed lookup pushes no branch. When the push or the PR fails after this +# run opened the issue, the script closes it again, unless an open PR for the +# branch names it. # # Environment: # GH_TOKEN must be set for gh: a token that can write issues and pull @@ -33,6 +35,8 @@ fi REPOSITORY="${GITHUB_REPOSITORY:-PolicyEngine/policyengine-api}" ISSUE_NUMBER="" +# 1 when this run opened ISSUE_NUMBER rather than reusing an open issue. +ISSUE_OPENED_HERE=0 # Set ISSUE_NUMBER to the oldest open tracking issue for LATEST, opening one # when none is open. This sets a variable rather than printing, because bash @@ -65,6 +69,7 @@ find_or_open_tracking_issue() { exit 1 fi issue_number="${issue_url##*/}" + ISSUE_OPENED_HERE=1 fi if [[ ! "${issue_number}" =~ ^[0-9]+$ ]]; then @@ -74,6 +79,35 @@ find_or_open_tracking_issue() { ISSUE_NUMBER="${issue_number}" } +# After a rejected push or a failed gh pr create, close the issue this run +# opened, so no open issue is left that no pull request names: a concurrent +# run for the same version, whose push won, has an issue of its own. Keep it +# when an open pull request for the branch names it after all, and never +# close an issue this run reused. +close_unused_tracking_issue() { + local first_line + + if [[ "${ISSUE_OPENED_HERE}" != "1" ]]; then + return 0 + fi + first_line="$( + gh pr view "$BRANCH" \ + --repo "${REPOSITORY}" \ + --json state,body \ + --jq 'select(.state == "OPEN") | .body | split("\n") | .[0]' \ + 2>/dev/null || true + )" + if [[ "${first_line%$'\r'}" == "Fixes #${ISSUE_NUMBER}" ]]; then + return 0 + fi + if ! gh issue close "${ISSUE_NUMBER}" \ + --repo "${REPOSITORY}" \ + --reason "not planned" \ + --comment "This run of the bundle update couldn't open a pull request for ${LATEST}, so nothing names this issue."; then + echo "Could not close the unused issue #${ISSUE_NUMBER}; close it by hand." >&2 + fi +} + create_pr_body() { cat </dev/null; then echo "Remote branch '${BRANCH}' already exists without an open PR. Creating PR." find_or_open_tracking_issue - gh pr create \ + if ! gh pr create \ --base master \ --head "$BRANCH" \ --title "Update PolicyEngine bundle to ${LATEST}" \ - --body "$(create_pr_body)" + --body "$(create_pr_body)"; then + close_unused_tracking_issue + exit 1 + fi echo "PR created for existing branch ${BRANCH}" exit 0 fi @@ -227,11 +264,17 @@ git checkout -b "$BRANCH" git add pyproject.toml docker/Dockerfile uv.lock "$FRAGMENT" git commit -m "Update PolicyEngine bundle to ${LATEST}" -git push -u origin "$BRANCH" +if ! git push -u origin "$BRANCH"; then + close_unused_tracking_issue + exit 1 +fi -gh pr create \ +if ! gh pr create \ --base master \ --title "Update PolicyEngine bundle to ${LATEST}" \ - --body "$(create_pr_body)" + --body "$(create_pr_body)"; then + close_unused_tracking_issue + exit 1 +fi echo "PR created: PolicyEngine bundle ${CURRENT} -> ${LATEST}" diff --git a/.github/workflows/close-bundle-update-issue.yml b/.github/workflows/close-bundle-update-issue.yml index 62939dec9..e4b480d95 100644 --- a/.github/workflows/close-bundle-update-issue.yml +++ b/.github/workflows/close-bundle-update-issue.yml @@ -5,16 +5,24 @@ name: Close superseded bundle-update issues # Merging the pull request closes it; superseded updates are closed without # merging, so this job closes their issues. See # .github/scripts/close_bundle_update_issue.sh. +# +# GitHub doesn't run pull_request workflows while a pull request has a merge +# conflict, and a superseded update usually conflicts with master, because +# the newer update that merged changed the same pins. pull_request_target +# runs anyway, with this workflow and the script from the default branch. The +# job reads only the event payload and never checks out the pull request. on: - pull_request: + pull_request_target: types: [closed] -# The job token closes the issue. Pull requests from forks get a read-only -# token, and the job's condition skips them anyway. +# The job token closes the issue; gh issue close also reads the number as an +# issue or a pull request. pull_request_target gives forks' pull requests the +# same token, so the job's condition skips any from another repository. permissions: contents: read issues: write + pull-requests: read jobs: close-tracking-issue: @@ -23,6 +31,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 5 steps: + # The default branch's script: no ref, so never the pull request's code. - name: Checkout the script uses: actions/checkout@v4 with: diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 4aa1f1330..4f8c38e54 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -40,6 +40,12 @@ To fix a failure: no run. Re-running checks the description as it was when the run started, so after changing the description, rely on the run the edit starts. +A pull request into a branch that doesn't contain +`.github/scripts/check_pr_description.py`, such as one stacked on a branch cut +before the check existed, fails with Python's `can't open file` message. Merge +master into that branch, or rely on the run that starts when GitHub retargets +the pull request to master. + To run the check on a description locally, save a pull request event and pass it in; the check needs only Python and an authenticated `gh`: @@ -54,11 +60,15 @@ Automated pull requests follow the rule too: - The PolicyEngine bundle update (`update-policyengine-package.sh`) reuses the oldest open "Update PolicyEngine bundle to " issue or opens one, so each version's pull request has an issue of its own. Merging the pull - request closes it; closing the pull request without merging, as when a + request closes it. Closing the pull request without merging, as when a newer version supersedes it, runs `close-bundle-update-issue.yml`, which - closes it too. A bundle-update pull request opened before the rule has no - `Fixes` line, and rerunning the workflow doesn't add one: edit its - description. + closes it too, even when the pull request conflicts with master (the + workflow runs on `pull_request_target`). If you reopen such a pull request, + reopen its issue as well, then re-run its `PR description` job. A run that + opens an issue but then can't push or open the pull request closes that + issue again. +- A bundle-update pull request opened before the rule has no `Fixes` line, + and rerunning the workflow doesn't add one: edit its description. - The daily relock (`open_relock_pull_request.sh`) reuses the open "Relock dependencies" issue or opens one, and rewrites the description of a relock pull request that is already open.