Compare a pull request with its merge commit's first parent in the Aggregate impact report

.github/workflows/aggregate-impact.yaml

@@ -1,6 +1,8 @@
 # Measure a pull request's base and head on the certified dataset and post
 # the comparison: receipts by tax head, spending and caseloads, poverty,
 # inequality, deciles and the standard reform costings, 2025-26 to 2031-32.
+# For a pull request, the head is GitHub's merge commit and the base is that
+# commit's first parent, so the report is exactly what merging would change.
 #
 # Both sides run the pull request head's copy of the measurement tool
 # (policyengine_uk/utils/aggregate_impact), each in an environment synced
@@ -66,16 +68,15 @@ jobs:
         id: commits
         env:
           EVENT: ${{ github.event_name }}
-          PR_BASE: ${{ github.event.pull_request.base.sha }}
           PR_BASE_REF: ${{ github.event.pull_request.base.ref }}
           PR_NUMBER: ${{ github.event.pull_request.number }}
           INPUT_BASE: ${{ inputs.base }}
           INPUT_HEAD: ${{ inputs.head }}
         run: |
-          # A pull request's head is its merge commit, whose first parent is
-          # the base, so the comparison is exactly what merging would change.
+          # A pull request's head is its merge commit. Its base is read from
+          # that commit once the head is checked out (Choose the base).
           if [ "$EVENT" = "pull_request" ]; then
-            base="$PR_BASE"; head="$GITHUB_SHA"
+            base=""; head="$GITHUB_SHA"
             base_name="$PR_BASE_REF"; head_name="#$PR_NUMBER merged into $PR_BASE_REF"
           else
             base="${INPUT_BASE:-main}"; head="${INPUT_HEAD:-$GITHUB_SHA}"
@@ -93,11 +94,39 @@ jobs:
         with:
           ref: ${{ steps.commits.outputs.head }}
           path: head
+          # A pull request's merge commit and its parents, so that the next
+          # step can read the base from it.
+          fetch-depth: 2
+
+      - name: Choose the base
+        id: base
+        env:
+          EVENT: ${{ github.event_name }}
+          BASE: ${{ steps.commits.outputs.base }}
+        run: |
+          # A pull request's base is its merge commit's first parent: the tip
+          # of the branch it merges into, as GitHub made the merge commit for
+          # this run. The comparison is then exactly what merging would
+          # change. The event's pull_request.base.sha is not that commit: it
+          # lags the branch once the branch has moved on, and the report then
+          # counted the branch's newer merges as the pull request's impact
+          # (#2516's took in 26 commits from main).
+          if [ "$EVENT" = "pull_request" ]; then
+            # The merge commit, then its parents.
+            line=$(git -C head rev-list --parents -n 1 HEAD)
+            read -r -a commit <<< "$line"
+            if [ "${#commit[@]}" -ne 3 ]; then
+              echo "::error::The head ${commit[0]} has $((${#commit[@]} - 1)) parents, not 2, so it is not a pull request's merge commit and its base is unknown."
+              exit 1
+            fi
+            BASE="${commit[1]}"
+          fi
+          echo "sha=$BASE" >> "$GITHUB_OUTPUT"
 
       - name: Check out the base
         uses: actions/checkout@v6
         with:
-          ref: ${{ steps.commits.outputs.base }}
+          ref: ${{ steps.base.outputs.sha }}
           path: base
 
       - name: Install uv

changelog.d/aggregate-impact-merge-parent.fixed.md

@@ -0,0 +1 @@
+- The Aggregate impact report now compares a pull request's merge commit with that commit's first parent, the tip of the branch it merges into. It used the event's `pull_request.base.sha`, which lags that branch once it moves on, so newer merges to `main` were counted as the pull request's impact.

policyengine_uk/tests/code_health/test_aggregate_impact_workflow.py

@@ -1,17 +1,41 @@
-"""Data protection: only aggregates may leave the aggregate-impact job.
+"""What the aggregate-impact job compares, and what may leave it.
 
-The certified dataset is licensed survey microdata. The measurement keeps
-household-level detail for the decile comparison under ``private/``; these
-tests fail if the workflow could publish it, or could put the dataset where
-other workflows can read it.
+Comparison base. For a pull request the head is GitHub's merge commit and
+the base is that commit's first parent, so the report is exactly what
+merging would change. The event's ``pull_request.base.sha`` lags the base
+branch once the branch moves on, and a report against it counted main's
+newer merges as the pull request's impact (#2516's took in 26 commits from
+main). These tests run the workflow's own step scripts on real histories,
+with the head fetched as actions/checkout fetches it at the workflow's
+fetch depth, and hold the job to:
+
+1. The base is the merge commit's first parent for every shape of history,
+   so no commit of the base branch is counted as the pull request's.
+2. A head that is not a merge of two commits fails the job with an error
+   and sets no base.
+3. No step reads ``pull_request.base.sha``, and the base checkout uses the
+   base the job chose.
+4. A manual dispatch compares the commits it names, as before.
+
+Data protection. Only aggregates may leave the job. The certified dataset
+is licensed survey microdata. The measurement keeps household-level detail
+for the decile comparison under ``private/``; these tests fail if the
+workflow could publish it, or could put the dataset where other workflows
+can read it.
 """
 
 import json
+import os
+import re
+import subprocess
+import tempfile
 from pathlib import Path
 
 import numpy as np
 import pytest
 import yaml
+from hypothesis import HealthCheck, given, settings
+from hypothesis import strategies as st
 
 from policyengine_uk.utils.aggregate_impact.measure import (
     HOUSEHOLDS_FILE,
@@ -95,3 +119,243 @@ def test_a_failed_run_always_replaces_an_earlier_comment():
     assert post["if"] == "always() && github.event_name == 'pull_request'"
     assert "if [ ! -s report/comment.md ]" in post["run"]
     assert "FAIL" in post["run"]
+
+
+# The comparison base.
+
+GIT_ENV = {
+    **os.environ,
+    "GIT_CONFIG_GLOBAL": os.devnull,
+    "GIT_CONFIG_NOSYSTEM": "1",
+    "GIT_AUTHOR_NAME": "t",
+    "GIT_AUTHOR_EMAIL": "t@example.com",
+    "GIT_COMMITTER_NAME": "t",
+    "GIT_COMMITTER_EMAIL": "t@example.com",
+}
+SHA = "0123456789abcdef0123456789abcdef01234567"
+
+
+def step_named(name):
+    return next(s for s in steps() if s.get("name") == name)
+
+
+def git(repo, *args):
+    return subprocess.run(
+        ["git", "-C", str(repo), *args],
+        check=True,
+        capture_output=True,
+        text=True,
+        env=GIT_ENV,
+    ).stdout.strip()
+
+
+def run_step(name, workspace, **env):
+    """Run a step's script in ``workspace`` as Actions runs it.
+
+    Each variable in the step's ``env`` is set, empty unless given, as an
+    expression with no value is. Returns the process and the outputs the
+    step wrote to ``GITHUB_OUTPUT``.
+    """
+    step = step_named(name)
+    outputs = Path(workspace) / "github_output"
+    outputs.write_text("")
+    variables = {key: "" for key in step.get("env", {})}
+    variables.update(env)
+    process = subprocess.run(
+        ["bash", "--noprofile", "--norc", "-eo", "pipefail", "-c", step["run"]],
+        cwd=workspace,
+        env={**GIT_ENV, **variables, "GITHUB_OUTPUT": str(outputs)},
+        capture_output=True,
+        text=True,
+    )
+    written = dict(line.split("=", 1) for line in outputs.read_text().splitlines())
+    return process, written
+
+
+class Origin:
+    """A repository on GitHub's side, holding commits made directly."""
+
+    def __init__(self, path):
+        path.mkdir(parents=True)
+        git(path, "init", "-q")
+        # actions/checkout fetches the head by its SHA.
+        git(path, "config", "uploadpack.allowAnySHA1InWant", "true")
+        self.path = path
+        self.tree = git(path, "write-tree")
+        self.made = 0
+
+    def commit(self, *parents):
+        self.made += 1
+        flags = [flag for parent in parents for flag in ("-p", parent)]
+        return git(
+            self.path, "commit-tree", self.tree, *flags, "-m", f"commit {self.made}"
+        )
+
+    def attributed(self, base, head):
+        """The commits a comparison of ``base`` with ``head`` counts."""
+        return set(git(self.path, "rev-list", f"{base}..{head}").split())
+
+
+def check_out_the_head(workspace, origin, sha):
+    """Fetch ``sha`` into ``workspace/head`` as the workflow's checkout does."""
+    depth = step_named("Check out the head")["with"].get("fetch-depth", 1)
+    head = Path(workspace) / "head"
+    head.mkdir(parents=True)
+    git(head, "init", "-q")
+    git(head, "fetch", "-q", "--no-tags", f"--depth={depth}", origin.path.as_uri(), sha)
+    git(head, "checkout", "-q", "--detach", sha)
+
+
+def choose_the_base(workspace, origin, merge):
+    check_out_the_head(workspace, origin, merge)
+    return run_step("Choose the base", workspace, EVENT="pull_request")
+
+
+def test_a_pull_request_is_compared_with_its_merge_commits_first_parent(tmp_path):
+    origin = Origin(tmp_path / "origin")
+    # The pull request branches from main at `forked`. The event's
+    # pull_request.base.sha can still name it after main has moved on.
+    forked = origin.commit(origin.commit())
+    change = origin.commit(forked)
+    # Main moves on: two later merges.
+    tip = origin.commit(origin.commit(forked))
+    # GitHub's merge commit for the run: main's tip, then the pull request.
+    merge = origin.commit(tip, change)
+
+    process, outputs = choose_the_base(tmp_path / "runner", origin, merge)
+
+    assert process.returncode == 0, process.stdout + process.stderr
+    assert outputs == {"sha": tip}
+    assert origin.attributed(outputs["sha"], merge) == {merge, change}
+    # Against `forked`, main's two later merges counted as the pull request's.
+    assert len(origin.attributed(forked, merge) - {merge, change}) == 2
+
+
+@pytest.mark.parametrize("parents", [0, 1, 3])
+def test_a_head_that_does_not_merge_two_commits_has_no_base(tmp_path, parents):
+    origin = Origin(tmp_path / "origin")
+    head = origin.commit(*[origin.commit() for _ in range(parents)])
+
+    process, outputs = choose_the_base(tmp_path / "runner", origin, head)
+
+    assert process.returncode != 0
+    assert f"::error::The head {head} has {parents} parents, not 2" in process.stdout
+    assert outputs == {}
+
+
+def test_the_events_base_sha_is_not_the_comparison_base():
+    expressions = re.findall(r"\$\{\{(.*?)\}\}", WORKFLOW.read_text(), re.DOTALL)
+    assert expressions
+    assert not [e for e in expressions if "pull_request.base.sha" in e]
+    names = [s.get("name") for s in steps()]
+    order = [
+        names.index("Choose the commits"),
+        names.index("Check out the head"),
+        names.index("Choose the base"),
+        names.index("Check out the base"),
+    ]
+    assert order == sorted(order)
+    head = step_named("Check out the head")["with"]
+    assert head["ref"] == "${{ steps.commits.outputs.head }}"
+    choose = step_named("Choose the base")
+    assert choose["id"] == "base"
+    assert choose["env"]["BASE"] == "${{ steps.commits.outputs.base }}"
+    assert step_named("Check out the base")["with"]["ref"] == (
+        "${{ steps.base.outputs.sha }}"
+    )
+
+
+def test_a_pull_request_head_is_its_merge_commit(tmp_path):
+    process, outputs = run_step(
+        "Choose the commits",
+        tmp_path,
+        EVENT="pull_request",
+        GITHUB_SHA=SHA,
+        PR_BASE_REF="main",
+        PR_NUMBER="2516",
+    )
+    assert process.returncode == 0, process.stdout + process.stderr
+    assert outputs == {
+        "base": "",
+        "head": SHA,
+        "base_name": "main",
+        "head_name": "#2516 merged into main",
+    }
+
+
+@pytest.mark.parametrize(
+    "inputs, expected",
+    [
+        (
+            {},
+            {"base": "main", "head": SHA, "base_name": "main", "head_name": "dev"},
+        ),
+        (
+            {"INPUT_BASE": "2.139.0", "INPUT_HEAD": "fix"},
+            {
+                "base": "2.139.0",
+                "head": "fix",
+                "base_name": "2.139.0",
+                "head_name": "fix",
+            },
+        ),
+    ],
+)
+def test_a_dispatch_compares_the_commits_it_names(tmp_path, inputs, expected):
+    process, outputs = run_step(
+        "Choose the commits",
+        tmp_path,
+        EVENT="workflow_dispatch",
+        GITHUB_SHA=SHA,
+        GITHUB_REF_NAME="dev",
+        **inputs,
+    )
+    assert process.returncode == 0, process.stdout + process.stderr
+    assert outputs == expected
+    # The base passes through, with no head to read it from.
+    process, outputs = run_step(
+        "Choose the base", tmp_path, EVENT="workflow_dispatch", BASE=expected["base"]
+    )
+    assert process.returncode == 0, process.stdout + process.stderr
+    assert outputs == {"sha": expected["base"]}
+
+
+@settings(
+    max_examples=25,
+    deadline=None,
+    derandomize=True,
+    suppress_health_check=[HealthCheck.too_slow],
+)
+@given(
+    before=st.integers(0, 2),
+    events=st.lists(st.sampled_from(["main", "pull request", "update"]), max_size=8),
+)
+def test_no_commit_of_the_base_branch_counts_as_the_pull_requests(before, events):
+    """Invariant 1, for any history: main's commits before and after the
+    fork, the pull request's commits, and merges of main into the pull
+    request ("Update branch")."""
+    with tempfile.TemporaryDirectory() as directory:
+        origin = Origin(Path(directory) / "origin")
+        main = origin.commit()
+        for _ in range(before):
+            main = origin.commit(main)
+        pull_request = origin.commit(main)
+        own = {pull_request}
+        merged = main
+        for event in events:
+            if event == "main":
+                main = origin.commit(main)
+            elif event == "pull request":
+                pull_request = origin.commit(pull_request)
+                own.add(pull_request)
+            elif main != merged:
+                pull_request = origin.commit(pull_request, main)
+                own.add(pull_request)
+                merged = main
+        merge = origin.commit(main, pull_request)
+
+        process, outputs = choose_the_base(Path(directory) / "runner", origin, merge)
+
+        assert process.returncode == 0, process.stdout + process.stderr
+        assert outputs == {"sha": main}
+        assert origin.attributed(outputs["sha"], merge) == own | {merge}