diff --git a/.github/scripts/check_pr_description.py b/.github/scripts/check_pr_description.py new file mode 100644 index 000000000..7d16f9682 --- /dev/null +++ b/.github/scripts/check_pr_description.py @@ -0,0 +1,213 @@ +"""Check that a pull request's description names its tracking issue. + +Rule sim.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" workflow job 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 + +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. +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +from pathlib import Path +from urllib.parse import urlsplit + +RULE_ID = "sim.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"(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, or the issue it names, does not satisfy the rule.""" + + +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``. + """ + 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}. {HOW_TO_FIX}" + raise DescriptionError(msg) + reported = completed.stderr.strip() or f"exit status {completed.returncode}" + msg = ( + f"Could not look up #{number} in {repository}: {reported}. Re-run this " + "job; if it fails again, check the GH_TOKEN the workflow gives it." + ) + raise DescriptionError(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}. Re-run this job." + raise DescriptionError(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: + """Raise ``DescriptionError`` unless ``issue`` is open issue ``number`` here.""" + 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}. Re-run this job." + ) + raise DescriptionError(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')}. {HOW_TO_FIX}" + ) + raise DescriptionError(msg) + if issue.get("pull_request") is not None: + msg = f"#{number} is a pull request, not an issue. {HOW_TO_FIX}" + raise DescriptionError(msg) + if issue.get("state") != "open": + msg = ( + f"Issue #{number} is {issue.get('state')}. If this pull request still " + "fixes it, reopen it and re-run this job; otherwise open an issue for " + f"this work and edit the first line to name it (rule {RULE_ID})." + ) + raise DescriptionError(msg) + + +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) + check_issue(repository, number, fetch_issue(repository, number)) + except DescriptionError as error: + print(f"::error title={ANNOTATION_TITLE}::{_annotation_text(str(error))}") + return 1 + print( + f"The first line is Fixes #{number}, and #{number} is an open issue in " + f"{repository}." + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/scripts/dependency-update-create-pr.sh b/.github/scripts/dependency-update-create-pr.sh index 1084c03e8..b35d4d1df 100755 --- a/.github/scripts/dependency-update-create-pr.sh +++ b/.github/scripts/dependency-update-create-pr.sh @@ -1,14 +1,121 @@ #!/usr/bin/env bash -# Open a dependency-update PR after a new update branch has been pushed. +# After an update branch has been pushed, make sure an open dependency-update +# PR exists and that the first line of its description names its tracking +# issue, as rule sim.process.pr-tracking-issue requires of every PR. +# +# The tracking issue is the oldest open issue titled "Update dependencies", or +# a new one when none is open. "Fixes #N" closes it when the PR merges, so the +# next update PR gets a new issue. +# +# - Without an open update PR, open one whose description starts with +# "Fixes #N". +# - With one, make "Fixes #N" its first line: replace an earlier "Fixes #" +# line, or add the line above a description that has none (such as a PR +# opened before the rule). +# +# Environment: +# GITHUB_REPOSITORY owner/name of this repository (set by the runner) +# GH_TOKEN read by gh: a token that can read and write issues and +# pull requests set -euo pipefail : "${GITHUB_REPOSITORY:?GITHUB_REPOSITORY is required}" -gh pr create \ +ISSUE_TITLE="Update dependencies" +ISSUE_BODY="Tracks the automated daily dependency updates from update-dependencies.yml. The update pull request names this issue on the first line of its description and closes it when it merges." +PR_SUMMARY="Automated daily dependency updates. Subsequent runs append commits to this branch while the pull request remains open." + +# Set ISSUE_NUMBER to the oldest open "Update dependencies" issue, opening one +# when none is open. The issues API lists pull requests too; they have a +# pull_request field. This sets a variable rather than printing, because bash +# turns off set -e inside $(...), and a failed lookup must not open an issue. +find_or_open_tracking_issue() { + local open_issues + local issue_number + local issue_url + + open_issues="$( + gh api --paginate \ + "repos/${GITHUB_REPOSITORY}/issues?state=open&per_page=100" \ + --jq ".[] | select(.pull_request == null and .title == \"${ISSUE_TITLE}\") | .number" + )" + if [[ -n "${open_issues}" ]]; then + issue_number="$(sort -n <<<"${open_issues}")" + issue_number="${issue_number%%$'\n'*}" + else + issue_url="$( + gh issue create \ + --repo "${GITHUB_REPOSITORY}" \ + --title "${ISSUE_TITLE}" \ + --body "${ISSUE_BODY}" + )" + issue_number="${issue_url##*/}" + fi + + if [[ ! "${issue_number}" =~ ^[0-9]+$ ]]; then + echo "Could not resolve the dependency-update tracking issue: ${issue_number}" >&2 + exit 1 + fi + ISSUE_NUMBER="${issue_number}" +} + +# Print the description with "Fixes #" as its first line. +description_naming_issue() { + local description="$1" + local issue_number="$2" + local first_line="${description%%$'\n'*}" + local fixes_line="Fixes #${issue_number}" + + if [[ "${first_line}" =~ ^Fixes\ \#[0-9]+[[:space:]]*$ ]]; then + if [[ "${description}" == *$'\n'* ]]; then + printf '%s\n%s' "${fixes_line}" "${description#*$'\n'}" + else + printf '%s' "${fixes_line}" + fi + else + printf '%s\n\n%s' "${fixes_line}" "${description}" + fi +} + +open_pr="$( + gh pr list \ + --repo "${GITHUB_REPOSITORY}" \ + --base main \ + --head update-dependencies \ + --state open \ + --json number \ + --jq '.[0].number // empty' +)" +ISSUE_NUMBER="" +find_or_open_tracking_issue + +if [[ -z "${open_pr}" ]]; then + gh pr create \ + --repo "${GITHUB_REPOSITORY}" \ + --title "Update dependencies" \ + --body-file - \ + --base main \ + --head update-dependencies \ + <<<"Fixes #${ISSUE_NUMBER}"$'\n\n'"${PR_SUMMARY}" + exit 0 +fi + +description="$( + gh pr view "${open_pr}" \ + --repo "${GITHUB_REPOSITORY}" \ + --json body \ + --jq '.body' +)" +linked_description="$(description_naming_issue "${description}" "${ISSUE_NUMBER}")" +if [[ "${linked_description}" == "${description}" ]]; then + echo "Pull request #${open_pr} already names tracking issue #${ISSUE_NUMBER}." + exit 0 +fi + +gh pr edit "${open_pr}" \ --repo "${GITHUB_REPOSITORY}" \ - --title "Update dependencies" \ - --body "Automated daily dependency updates. Subsequent runs append commits to this branch while the pull request remains open." \ - --base main \ - --head update-dependencies + --body-file - \ + <<<"${linked_description}" +echo "Pull request #${open_pr} now names tracking issue #${ISSUE_NUMBER}." diff --git a/.github/workflows/pr-description.yml b/.github/workflows/pr-description.yml new file mode 100644 index 000000000..f6734077a --- /dev/null +++ b/.github/workflows/pr-description.yml @@ -0,0 +1,52 @@ +name: PR description + +# Rule sim.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. +# +# 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. + +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@v6 + with: + sparse-checkout: .github/scripts/check_pr_description.py + sparse-checkout-cone-mode: false + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.13' + + - 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/.github/workflows/update-dependencies.yml b/.github/workflows/update-dependencies.yml index caf7b0142..3c4d1d265 100644 --- a/.github/workflows/update-dependencies.yml +++ b/.github/workflows/update-dependencies.yml @@ -61,8 +61,11 @@ jobs: HAS_OPEN_PR: ${{ steps.branch.outputs.has_open_pr }} run: .github/scripts/dependency-update-commit-and-push.sh - - name: Create pull request - if: steps.branch.outputs.has_open_pr == 'false' && steps.push.outputs.branch_pushed == 'true' + # Opens the update PR, or, when one is open, makes sure the first line + # of its description names the tracking issue (rule + # sim.process.pr-tracking-issue). + - name: Open or update the pull request + if: steps.push.outputs.branch_pushed == 'true' env: GH_TOKEN: ${{ steps.app-token.outputs.token }} run: .github/scripts/dependency-update-create-pr.sh diff --git a/AGENTS.md b/AGENTS.md index 498175b75..aa0bf0098 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,7 +83,9 @@ same-repository draft PR. - API integration tests live in `projects/policyengine-apis-integ`. - PR CI runs simulation unit tests, Ruff format and lint checks, Pyright for every package, quality guards (including the rules registry), Docker build, - and local integration tests. + and local integration tests. A separate `PR description` job checks that + every open PR's description starts with `Fixes #ISSUE_NUMBER` for an open + issue in this repository. - There is currently no repository-wide changelog fragment requirement. ## Safety Boundaries @@ -150,9 +152,8 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### Process and repository hygiene -- Checked: `sim.agents.adapters-point-to-agents-md`, `sim.process.rules-registry`, `sim.process.stacked-prs-full-ci-at-bottom` +- Checked: `sim.agents.adapters-point-to-agents-md`, `sim.process.pr-tracking-issue`, `sim.process.rules-registry`, `sim.process.stacked-prs-full-ci-at-bottom` - `sim.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) -- `sim.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) - `sim.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. (doc-only, not checked: follow it yourself) - `sim.process.same-repo-draft-prs`: PRs are opened as drafts from branches in PolicyEngine/policyengine-sim-api, never from forks. (doc-only, not checked: follow it yourself) diff --git a/README.md b/README.md index 3a3b8dcd8..df4db6273 100644 --- a/README.md +++ b/README.md @@ -147,8 +147,9 @@ See [AGENTS.md](AGENTS.md) and for the canonical same-repository draft PR workflow. 1. Create a feature branch (never commit to `main`). -2. Open a GitHub issue for non-trivial work; put `Fixes #ISSUE_NUMBER` as the - first line of the PR description. +2. Open a GitHub issue for the work; put `Fixes #ISSUE_NUMBER` as the first + line of the PR description. Every PR needs one: the `PR description` check + fails otherwise. 3. Make changes and run the relevant tests locally. 4. Open a same-repository draft PR: `make push-pr-branch`, then `gh pr create --draft`. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 2dce64f4c..77288057a 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -15,7 +15,8 @@ Before creating or sharing a PR: 2. Open a GitHub issue for the work, or verify that an appropriate issue already exists. 3. Put `Fixes #ISSUE_NUMBER` as the first line of the PR description, using the - issue number from the issue created or found in the previous step. + issue number from the issue created or found in the previous step. The + `PR description` check fails otherwise; see [The `Fixes` Line](#the-fixes-line). 4. Run formatting and the most relevant tests for the changed surface. If an expected check cannot be run, say so in the PR body. 5. Push the current branch to the canonical repository: @@ -44,7 +45,8 @@ human instead. In a stack of pull requests, only the lowest unmerged one runs the full PR workflow (`.github/workflows/pr.yml`) and the Modal image smoke. The ones above -it run only the `CI complete` job, which fails and says so. A stack therefore +it run only the `CI complete` job, which fails and says so, and the +`PR description` job, 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.** It needs every other job in @@ -63,6 +65,42 @@ uses one set of runners instead of one per layer. `projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py` checks both. +## The `Fixes` Line + +Rule `sim.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; +- a blank first line, or leading spaces before `Fixes`; +- other wording, such as `Fixes #615.`, `Closes #12` or `Refs #12`; +- a reference to another repository, such as `Fixes PolicyEngine/policyengine-api#12`. + +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. + +Automated pull requests follow the rule too. The policyengine package update +(`update-policyengine-package.sh`) opens an issue for each version. The daily +dependency update (`dependency-update-create-pr.sh`) reuses the open "Update +dependencies" issue or opens one, and also puts its `Fixes` line first on an +update pull request that is already open. + ## PR Title Do not add `[codex]`, `[claude]`, `[copilot]`, or other agent labels to PR diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 7fa416bbc..719c83ca6 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-sim-api` is listed here with how it is enf | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 49 | 5 | 0 | 15 | 69 | +| mechanical | 50 | 5 | 0 | 14 | 69 | | metric | 0 | 0 | 0 | 0 | 0 | | judgment | 0 | 0 | 0 | 0 | 0 | -**Blocking:** 54 of 69 rules. +**Blocking:** 55 of 69 rules. ## Gaps @@ -28,7 +28,6 @@ Every rule that governs `policyengine-sim-api` is listed here with how it is enf - `sim.observability.id-is-diagnostic-only`: observability_id has diagnostic meaning only: never use it for idempotency, authorization, database identity, routing or business logic. - `sim.observability.registered-stages-only`: Span names come from the stage plans in policyengine_simulation_observability.stages; runtime code never adds span-name string literals. - `sim.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. -- `sim.process.pr-tracking-issue`: Every PR has a tracking issue in this repository, and the first line of its description is Fixes #ISSUE_NUMBER. - `sim.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. - `sim.process.same-repo-draft-prs`: PRs are opened as drafts from branches in PolicyEngine/policyengine-sim-api, never from forks. - `sim.quality.every-project-tested`: Every project and library with unit tests runs them in the PR test matrix. @@ -430,7 +429,7 @@ flowchart LR | --- | --- | --- | --- | --- | --- | --- | --- | | `sim.agents.adapters-point-to-agents-md` | CLAUDE.md and .github/copilot-instructions.md only point to AGENTS.md; skills and rules are listed in AGENTS.md alone. | mechanical | enforced | yes | quality guard `rules-registry` | `pr.yml:quality-guards` | yes | | `sim.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. | mechanical | doc-only | no | nothing | — | — | -| `sim.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 | — | — | +| `sim.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 | | `sim.process.rule-machinery-code-owned` | Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. | mechanical | doc-only | no | nothing | — | — | | `sim.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** | | `sim.process.same-repo-draft-prs` | PRs are opened as drafts from branches in PolicyEngine/policyengine-sim-api, never from forks. | mechanical | doc-only | no | nothing | — | — | @@ -443,7 +442,9 @@ flowchart LR j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] r_sim_process_agents_never_bypass("sim.process.agents-never-bypass (doc-only)") gap_unenforced>"not enforced"] - r_sim_process_pr_tracking_issue("sim.process.pr-tracking-issue (doc-only)") + r_sim_process_pr_tracking_issue("sim.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_sim_process_rule_machinery_code_owned("sim.process.rule-machinery-code-owned (doc-only)") r_sim_process_rules_registry("sim.process.rules-registry (enforced)") r_sim_process_same_repo_draft_prs("sim.process.same-repo-draft-prs (doc-only)") @@ -454,7 +455,8 @@ flowchart LR r_sim_agents_adapters_point_to_agents_md --> e_quality_guard_rules_registry e_quality_guard_rules_registry --> j_CI_pr_yml_quality_guards r_sim_process_agents_never_bypass -.-> gap_unenforced - r_sim_process_pr_tracking_issue -.-> gap_unenforced + r_sim_process_pr_tracking_issue --> e_CI_job_pr_description + e_CI_job_pr_description --> j_CI_pr_description_yml_pr_description r_sim_process_rule_machinery_code_owned -.-> gap_unenforced r_sim_process_rules_registry --> e_quality_guard_rules_registry r_sim_process_same_repo_draft_prs -.-> gap_unenforced diff --git a/projects/policyengine-simulation-entry/tests/test_pr_description_check.py b/projects/policyengine-simulation-entry/tests/test_pr_description_check.py new file mode 100644 index 000000000..659f356c6 --- /dev/null +++ b/projects/policyengine-simulation-entry/tests/test_pr_description_check.py @@ -0,0 +1,518 @@ +"""Tests for .github/scripts/check_pr_description.py. + +Rule sim.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 subprocess +import sys +from typing import TYPE_CHECKING + +import pytest +import yaml +from workflow_script_support import ( + REPOSITORY_ROOT, + SCRIPTS_DIR, + WORKFLOWS_DIR, + FakeCommands, +) + +if TYPE_CHECKING: + from pathlib import Path + from types import ModuleType + +CHECK_SCRIPT = SCRIPTS_DIR / "check_pr_description.py" +CHECK_WORKFLOW = WORKFLOWS_DIR / "pr-description.yml" +CHECK_JOB = "pr-description" +PROOF_FIXTURES = ( + REPOSITORY_ROOT / "rules" / "fixtures" / "sim.process.pr-tracking-issue" +) +REPOSITORY = "PolicyEngine/policyengine-sim-api" +VALID_BODY = "Fixes #861\n\n## Summary\n\n- Adds a check.\n" + +# 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 = """\ +printf '%s' "${FAKE_GH_STDOUT:-}" +if [[ -n "${FAKE_GH_STDERR:-}" ]]; then + printf '%s\\n' "$FAKE_GH_STDERR" >&2 +fi +exit "${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)" + + +@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 #861", + "Fixes #861\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) == 861 + + +@pytest.mark.parametrize( + "body", + [ + # Descriptions edited in the GitHub web interface use CRLF. + "Fixes #861\r\n\r\n## Summary\r\n", + # A carriage return on its own also ends a line in Markdown. + "Fixes #861\r## Summary", + ], +) +def test_carriage_return_line_endings_end_the_first_line(check_module, body): + assert check_module.issue_number_from_description(body) == 861 + + +def test_trailing_whitespace_after_the_number_is_ignored(check_module): + # From PR #405, edited in the web interface: a space, then CRLF. + body = "Fixes #404 \r\n\r\n## Summary\r\n" + + assert check_module.issue_number_from_description(body) == 404 + + +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 "sim.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 #861\n") + + assert "Fixes #861 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 #861\r\n") + + assert "Fixes #861 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 #861\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 #616. + "Fixes #615.", + # From PR #543. + "Refs #434", + # From PR #750. + "Fixes # (no tracking issue exists yet.)", + "fixes #861", + "Closes #861", + "Fixes 861", + "Fixes #", + "Fixes #0", + "Fixes #0861", + "Fixes #861 and #862", + "Fixes PolicyEngine/policyengine-api#3000", + "Fixes https://github.com/PolicyEngine/policyengine-sim-api/issues/861", + # 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 #743. " + "Gate releases on a sentinel. " * 10 + + message = _rejection(check_module, first_line) + + assert 'The first line is "Draft, stacked on #743.' in message + assert first_line not in message + + +# --- The issue lookup ----------------------------------------------------- + + +@pytest.fixture +def fakes(tmp_path: Path) -> FakeCommands: + commands = FakeCommands.create(tmp_path) + commands.add("gh", FAKE_GH) + return commands + + +def _issue( + number: int = 861, *, state: str = "open", repository: str = REPOSITORY, **fields +) -> 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": 862, "pull_request": {"body": body}} + + +def _run_check( + fakes: FakeCommands, 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( + fakes.env( + 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(fakes, tmp_path): + result = _run_check(fakes, tmp_path, **_gh_answers(_issue())) + + assert result.returncode == 0, result.stdout + result.stderr + assert fakes.calls("gh") == [ + ["gh", "api", "repos/PolicyEngine/policyengine-sim-api/issues/861"] + ] + assert result.stdout == ( + "The first line is Fixes #861, and #861 is an open issue in " + "PolicyEngine/policyengine-sim-api.\n" + ) + + +def test_a_rejected_description_is_reported_without_asking_github(fakes, tmp_path): + result = _run_check(fakes, tmp_path, "## Summary\n\nFixes #861\n") + + assert result.returncode == 1 + assert "Fixes #861 is on line 3" in _error_annotation(result) + assert fakes.calls("gh") == [] + + +def test_a_null_description_in_the_event_fails(fakes, tmp_path): + result = _run_check(fakes, tmp_path, None) + + assert result.returncode == 1 + assert "The description is empty." in _error_annotation(result) + assert fakes.calls("gh") == [] + + +def test_a_crlf_description_from_the_event_passes(fakes, tmp_path): + result = _run_check( + fakes, tmp_path, "Fixes #861\r\n\r\n## Summary\r\n", **_gh_answers(_issue()) + ) + + assert result.returncode == 0, result.stdout + result.stderr + + +def test_a_closed_issue_fails(fakes, tmp_path): + result = _run_check(fakes, tmp_path, **_gh_answers(_issue(state="closed"))) + + assert result.returncode == 1 + annotation = _error_annotation(result) + assert "Issue #861 is closed." in annotation + assert "reopen it and re-run this job" in annotation + + +def test_a_pull_request_number_fails(fakes, tmp_path): + pull_request = _issue( + 857, + pull_request={ + "url": f"https://api.github.com/repos/{REPOSITORY}/pulls/857", + }, + ) + + result = _run_check(fakes, tmp_path, "Fixes #857\n", **_gh_answers(pull_request)) + + assert result.returncode == 1 + assert "#857 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(fakes, tmp_path, gh_error): + result = _run_check( + fakes, + 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-sim-api." + in _error_annotation(result) + ) + + +def test_an_issue_moved_to_another_repository_fails(fakes, tmp_path): + # Issue #154 was transferred to policyengine.py, where it is #115; gh api + # follows GitHub's redirect and prints the moved issue. + moved_issue = _issue(115, state="closed", repository="PolicyEngine/policyengine.py") + + result = _run_check(fakes, tmp_path, "Fixes #154\n", **_gh_answers(moved_issue)) + + assert result.returncode == 1 + annotation = _error_annotation(result) + assert ( + "#154 is not an issue in PolicyEngine/policyengine-sim-api: it moved to " + "PolicyEngine/policyengine.py#115." + ) in annotation + + +def test_an_issue_in_a_repository_whose_name_differs_only_in_case_passes( + fakes, tmp_path +): + issue = _issue(repository="policyengine/PolicyEngine-Sim-API") + + result = _run_check(fakes, 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(fakes, tmp_path, repository_url): + issue = _issue() + issue["repository_url"] = repository_url + + result = _run_check(fakes, tmp_path, **_gh_answers(issue)) + + assert result.returncode == 1 + assert "GitHub's answer for #861 names no repository" in _error_annotation(result) + + +def test_a_failed_lookup_fails_with_what_gh_reported(fakes, tmp_path): + result = _run_check( + fakes, + 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 #861 in PolicyEngine/policyengine-sim-api" in annotation + assert "gh: Bad credentials (HTTP 401)" in annotation + + +@pytest.mark.parametrize("gh_stdout", ["not JSON", "[]"]) +def test_an_answer_that_is_not_an_issue_object_fails(fakes, tmp_path, gh_stdout): + result = _run_check(fakes, tmp_path, FAKE_GH_STDOUT=gh_stdout) + + assert result.returncode == 1 + assert "gh api did not print an issue for #861" in _error_annotation(result) + + +def test_messages_stay_on_one_annotation_line(fakes, tmp_path): + result = _run_check( + fakes, + 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(fakes, tmp_path, variable): + event_path = _write_event(tmp_path, _pull_request_event(VALID_BODY)) + env = fakes.env(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 fakes.calls("gh") == [] + + +def test_the_check_requires_a_pull_request_event(fakes, tmp_path): + event_path = _write_event(tmp_path, {"ref": "refs/heads/main"}) + + result = _run_script( + fakes.env(GITHUB_EVENT_PATH=str(event_path), GITHUB_REPOSITORY=REPOSITORY) + ) + + assert result.returncode == 1 + assert "has no pull_request" in result.stderr + assert fakes.calls("gh") == [] + + +# --- 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 test_rule_proof_pr_tracking_issue(fakes): + """The check rejects PR #616's first line, "Fixes #615.", and accepts it fixed. + + Both fixtures are pull_request event payloads; the fake gh answers that + #615 is an open issue in this repository. + """ + results = { + fixture: _run_script( + fakes.env( + GITHUB_EVENT_PATH=str(PROOF_FIXTURES / f"{fixture}_event.json"), + GITHUB_REPOSITORY=REPOSITORY, + **_gh_answers(_issue(615)), + ) + ) + for fixture in ("bad", "good") + } + + assert results["bad"].returncode == 1 + assert 'The first line is "Fixes #615."' in _error_annotation(results["bad"]) + assert results["good"].returncode == 0, results["good"].stdout + assert fakes.calls("gh") == [ + ["gh", "api", "repos/PolicyEngine/policyengine-sim-api/issues/615"] + ] diff --git a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py index aaa0a816b..a3cae0fd7 100644 --- a/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py +++ b/projects/policyengine-simulation-entry/tests/test_workflow_scripts.py @@ -10,6 +10,7 @@ from pathlib import Path import pytest +import yaml from workflow_script_support import ( REPOSITORY_ROOT, WORKFLOWS_DIR, @@ -253,15 +254,81 @@ def test_commit_and_push_fails_when_the_push_fails(fakes, tmp_path): assert github_output(output) == {} -def test_create_pr_opens_the_update_pr(fakes): - fakes.add("gh") - +# The create-PR script finds the open "Update dependencies" issue, or opens +# one, and names it on the first line of the update PR's description (rule +# sim.process.pr-tracking-issue). The fake gh answers each call from the +# environment and writes each description it is given to FAKE_PR_BODY_LOG. +FAKE_GH_FOR_CREATE_PR = """\ +case "$1 $2" in + "pr list") printf '%s' "${FAKE_OPEN_PR:-}" ;; + "pr view") printf '%s' "${FAKE_OPEN_PR_BODY:-}" ;; + "pr create" | "pr edit") cat >> "$FAKE_PR_BODY_LOG" ;; + "issue create") printf '%s\\n' "${FAKE_CREATED_ISSUE_URL:-}" ;; + "api --paginate") + printf '%s' "${FAKE_OPEN_ISSUES:-}" + exit "${FAKE_GH_API_EXIT:-0}" + ;; +esac +""" +UPDATE_PR_SUMMARY = ( + "Automated daily dependency updates. Subsequent runs append commits to " + "this branch while the pull request remains open." +) +LEGACY_UPDATE_PR_BODY = ( + "Automated dependency updates\n\n" + "This PR was automatically created by the dependency update workflow." +) +OPEN_UPDATE_PR_LOOKUP = [ + "gh", + "pr", + "list", + "--repo", + "PolicyEngine/policyengine-sim-api", + "--base", + "main", + "--head", + "update-dependencies", + "--state", + "open", + "--json", + "number", + "--jq", + ".[0].number // empty", +] +OPEN_TRACKING_ISSUE_LOOKUP = [ + "gh", + "api", + "--paginate", + "repos/PolicyEngine/policyengine-sim-api/issues?state=open&per_page=100", + "--jq", + '.[] | select(.pull_request == null and .title == "Update dependencies") | .number', +] + + +def _create_pr(fakes: FakeCommands, tmp_path: Path, **env: str): + """Run the create-PR script; return its result and the descriptions it set.""" + body_log = tmp_path / "pr-bodies" + fakes.add("gh", FAKE_GH_FOR_CREATE_PR) result = run_script( - CREATE_PR, env=fakes.env(GITHUB_REPOSITORY="PolicyEngine/policyengine-sim-api") + CREATE_PR, + env=fakes.env( + GITHUB_REPOSITORY="PolicyEngine/policyengine-sim-api", + FAKE_PR_BODY_LOG=str(body_log), + **env, + ), ) + # Read bytes: read_text would turn the CRLF line endings under test into LF. + descriptions = body_log.read_bytes().decode("utf-8") if body_log.exists() else "" + return result, descriptions + + +def test_create_pr_opens_the_update_pr_naming_the_open_tracking_issue(fakes, tmp_path): + result, descriptions = _create_pr(fakes, tmp_path, FAKE_OPEN_ISSUES="870\n") assert result.returncode == 0, result.stderr assert fakes.calls() == [ + OPEN_UPDATE_PR_LOOKUP, + OPEN_TRACKING_ISSUE_LOOKUP, [ "gh", "pr", @@ -270,15 +337,159 @@ def test_create_pr_opens_the_update_pr(fakes): "PolicyEngine/policyengine-sim-api", "--title", "Update dependencies", - "--body", - "Automated daily dependency updates. Subsequent runs append commits " - "to this branch while the pull request remains open.", + "--body-file", + "-", "--base", "main", "--head", "update-dependencies", - ] + ], + ] + assert descriptions == f"Fixes #870\n\n{UPDATE_PR_SUMMARY}\n" + + +def test_create_pr_names_the_oldest_open_tracking_issue(fakes, tmp_path): + result, descriptions = _create_pr( + fakes, tmp_path, FAKE_OPEN_ISSUES="875\n870\n1001\n" + ) + + assert result.returncode == 0, result.stderr + assert descriptions.startswith("Fixes #870\n\n") + + +def test_create_pr_opens_a_tracking_issue_when_none_is_open(fakes, tmp_path): + result, descriptions = _create_pr( + fakes, + tmp_path, + FAKE_CREATED_ISSUE_URL=( + "https://github.com/PolicyEngine/policyengine-sim-api/issues/871" + ), + ) + + assert result.returncode == 0, result.stderr + assert fakes.calls()[2] == [ + "gh", + "issue", + "create", + "--repo", + "PolicyEngine/policyengine-sim-api", + "--title", + "Update dependencies", + "--body", + "Tracks the automated daily dependency updates from " + "update-dependencies.yml. The update pull request names this issue on " + "the first line of its description and closes it when it merges.", + ] + assert fakes.calls()[3][:3] == ["gh", "pr", "create"] + assert descriptions == f"Fixes #871\n\n{UPDATE_PR_SUMMARY}\n" + + +def test_create_pr_fails_when_the_new_issue_has_no_number(fakes, tmp_path): + result, descriptions = _create_pr( + fakes, tmp_path, FAKE_CREATED_ISSUE_URL="could not create the issue" + ) + + assert result.returncode == 1 + assert "Could not resolve the dependency-update tracking issue" in result.stderr + assert not any(call[1:3] == ["pr", "create"] for call in fakes.calls()) + assert descriptions == "" + + +def test_create_pr_fails_when_the_issue_lookup_fails(fakes, tmp_path): + result, descriptions = _create_pr(fakes, tmp_path, FAKE_GH_API_EXIT="1") + + assert result.returncode == 1 + assert fakes.calls() == [OPEN_UPDATE_PR_LOOKUP, OPEN_TRACKING_ISSUE_LOOKUP] + assert descriptions == "" + + +def test_create_pr_puts_the_tracking_issue_first_on_an_open_update_pr(fakes, tmp_path): + """An update PR opened before the rule, such as #583, gets a Fixes line.""" + result, descriptions = _create_pr( + fakes, + tmp_path, + FAKE_OPEN_PR="583", + FAKE_OPEN_PR_BODY=LEGACY_UPDATE_PR_BODY, + FAKE_OPEN_ISSUES="870\n", + ) + + assert result.returncode == 0, result.stderr + assert fakes.calls() == [ + OPEN_UPDATE_PR_LOOKUP, + OPEN_TRACKING_ISSUE_LOOKUP, + [ + "gh", + "pr", + "view", + "583", + "--repo", + "PolicyEngine/policyengine-sim-api", + "--json", + "body", + "--jq", + ".body", + ], + [ + "gh", + "pr", + "edit", + "583", + "--repo", + "PolicyEngine/policyengine-sim-api", + "--body-file", + "-", + ], ] + assert descriptions == f"Fixes #870\n\n{LEGACY_UPDATE_PR_BODY}\n" + + +@pytest.mark.parametrize("line_ending", ["\n", "\r\n"]) +def test_create_pr_replaces_an_earlier_fixes_line_on_the_open_update_pr( + fakes, tmp_path, line_ending +): + """An open update PR whose issue was closed names the open one instead.""" + rest = f"{line_ending}{UPDATE_PR_SUMMARY}" + result, descriptions = _create_pr( + fakes, + tmp_path, + FAKE_OPEN_PR="900", + FAKE_OPEN_PR_BODY=f"Fixes #860 {line_ending}{rest}", + FAKE_OPEN_ISSUES="870\n", + ) + + assert result.returncode == 0, result.stderr + assert descriptions == f"Fixes #870\n{rest}\n" + + +def test_create_pr_replaces_a_fixes_line_that_is_the_whole_description(fakes, tmp_path): + result, descriptions = _create_pr( + fakes, + tmp_path, + FAKE_OPEN_PR="900", + FAKE_OPEN_PR_BODY="Fixes #860", + FAKE_OPEN_ISSUES="870\n", + ) + + assert result.returncode == 0, result.stderr + assert descriptions == "Fixes #870\n" + + +def test_create_pr_leaves_an_open_update_pr_that_names_the_tracking_issue( + fakes, tmp_path +): + result, descriptions = _create_pr( + fakes, + tmp_path, + FAKE_OPEN_PR="900", + FAKE_OPEN_PR_BODY=f"Fixes #870\n\n{UPDATE_PR_SUMMARY}", + FAKE_OPEN_ISSUES="870\n", + ) + + assert result.returncode == 0, result.stderr + assert not any( + call[1:3] in (["pr", "edit"], ["pr", "create"]) for call in fakes.calls() + ) + assert descriptions == "" def test_create_pr_fails_when_gh_fails(fakes): @@ -289,6 +500,7 @@ def test_create_pr_fails_when_gh_fails(fakes): ) assert result.returncode == 1 + assert fakes.calls() == [OPEN_UPDATE_PR_LOOKUP] def test_create_pr_requires_the_repository(fakes): @@ -303,6 +515,20 @@ def test_create_pr_requires_the_repository(fakes): assert fakes.calls() == [] +def test_update_dependencies_workflow_updates_the_pr_whenever_it_pushes(): + """With or without an open update PR, a push runs the create-PR script.""" + workflow = yaml.safe_load( + (WORKFLOWS_DIR / "update-dependencies.yml").read_text(encoding="utf-8") + ) + steps = workflow["jobs"]["update-dependencies"]["steps"] + create_pr = next( + step for step in steps if step.get("run") == f".github/scripts/{CREATE_PR}" + ) + + assert create_pr["if"] == "steps.push.outputs.branch_pushed == 'true'" + assert create_pr["env"] == {"GH_TOKEN": "${{ steps.app-token.outputs.token }}"} + + # --- pr.yml --------------------------------------------------------------- START_LOCAL_SERVICES = "start-local-services.sh" diff --git a/rules/fixtures/sim.process.pr-tracking-issue/bad_event.json b/rules/fixtures/sim.process.pr-tracking-issue/bad_event.json new file mode 100644 index 000000000..5ffa821c1 --- /dev/null +++ b/rules/fixtures/sim.process.pr-tracking-issue/bad_event.json @@ -0,0 +1,12 @@ +{ + "action": "opened", + "number": 616, + "pull_request": { + "number": 616, + "title": "Return report-shaped US congressional district impacts", + "body": "Fixes #615.\n\nSummary:\n- Maps congressional district results to canonical district IDs like `AL-01`.\n- Returns district impacts only for US national and state-level reports.\n" + }, + "repository": { + "full_name": "PolicyEngine/policyengine-sim-api" + } +} diff --git a/rules/fixtures/sim.process.pr-tracking-issue/good_event.json b/rules/fixtures/sim.process.pr-tracking-issue/good_event.json new file mode 100644 index 000000000..f3b34f646 --- /dev/null +++ b/rules/fixtures/sim.process.pr-tracking-issue/good_event.json @@ -0,0 +1,12 @@ +{ + "action": "opened", + "number": 616, + "pull_request": { + "number": 616, + "title": "Return report-shaped US congressional district impacts", + "body": "Fixes #615\n\nSummary:\n- Maps congressional district results to canonical district IDs like `AL-01`.\n- Returns district impacts only for US national and state-level reports.\n" + }, + "repository": { + "full_name": "PolicyEngine/policyengine-sim-api" + } +} diff --git a/rules/registry.toml b/rules/registry.toml index dac2bb1a7..4e2f60794 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -52,7 +52,7 @@ id = "process" title = "Process and repository hygiene" [discovery] -workflows = [".github/workflows/pr.yml", ".github/workflows/pr-image-smoke.yml"] +workflows = [".github/workflows/pr.yml", ".github/workflows/pr-image-smoke.yml", ".github/workflows/pr-description.yml"] guard_test_globs = [ "projects/*/tests/test_*boundar*.py", "libs/*/tests/test_*boundar*.py", @@ -1485,14 +1485,27 @@ invocation = "unittest discover -s scripts/rules/tests" [[rule]] id = "sim.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." -rationale = "docs/engineering/skills/github-prs.md#same-repository-draft-prs-only" -scope = [".github/pull_request_template.md"] +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#the-fixes-line" +scope = [".github/pull_request_template.md", ".github/workflows/pr-description.yml", ".github/scripts/check_pr_description.py", ".github/scripts/dependency-update-create-pr.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/sim.process.pr-tracking-issue/bad_event.json" +good_fixture = "rules/fixtures/sim.process.pr-tracking-issue/good_event.json" +project = "projects/policyengine-simulation-entry" +test = "tests/test_pr_description_check.py::test_rule_proof_pr_tracking_issue" +workflow = ".github/workflows/pr.yml" +job = "test" +invocation = "uv run --frozen pytest tests/" [[rule]] id = "sim.process.same-repo-draft-prs"