From 394a060ac0236d7024eb0ed16dfebbbe34edadd4 Mon Sep 17 00:00:00 2001 From: domenico Date: Wed, 12 Aug 2026 20:59:27 +0200 Subject: [PATCH] fix: read the PR number from the event payload, not only from GITHUB_REF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pull_request run whose GITHUB_REF is not refs/pull//merge aborted with "Could not determine PR number from GITHUB_REF" and exit 1, before Layer 1 had measured anything. To the author that is indistinguishable from a coverage verdict: the check goes red having never looked at the diff. Observed on 2026-08-12 in Ostico/bruno-mcp-studio#180. Editing the PR body fired the `edited` activity type, which produced a second run on a head sha whose earlier run was green; that run failed this way, and because GitHub surfaces the latest run per check name, the merge button showed a red required gate for a commit that had passed. Every pull_request payload carries `.pull_request.number`, for every activity type. GITHUB_REF carries it only while GitHub has a merge ref to point the run at. So the payload at GITHUB_EVENT_PATH is now consulted first and GITHUB_REF remains the fallback, which keeps every case that worked before working. `_pr_number_from_event` returns None rather than raising for anything unexpected — no GITHUB_EVENT_PATH, an unreadable or malformed file, a payload for another event, a number that is not an int (`true` is valid JSON in that position, and bool is an int subclass). Each of those still has GITHUB_REF to fall back on, and raising would turn a recoverable run into a failed one. main.py's message now names both sources it tried and says outright that nothing was measured. As written it sent the reader to GITHUB_REF even once the payload was being consulted. One existing test needed a change for a reason worth stating: test_pr_number_from_env set GITHUB_REF and asserted 42, and this suite runs inside Actions, where GITHUB_EVENT_PATH points at a real payload naming a real PR. Reading the payload first means that test would have measured the CI run's own PR — passing locally and failing in CI. It now unsets GITHUB_EVENT_PATH explicitly. Twelve tests added. Each of the four ways to break this — never consulting the payload, letting the ref win, accepting a non-int number, letting a malformed payload raise — was verified to turn the suite red. --- src/config.py | 45 ++++++++++++++++++++++++-- src/main.py | 6 +++- tests/test_config.py | 75 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 4 deletions(-) diff --git a/src/config.py b/src/config.py index 74507a7..cf935d8 100644 --- a/src/config.py +++ b/src/config.py @@ -176,6 +176,40 @@ def _env_required(name: str) -> str: return val +def _pr_number_from_event() -> int | None: + """Read the PR number from the event payload GitHub writes for the run. + + Every pull_request payload carries `.pull_request.number`, for every + activity type, which GITHUB_REF does not. + + Returns None rather than raising for anything unexpected — no payload path, + an unreadable or malformed file, a payload for some other event, a number + that is not one. The caller still has GITHUB_REF to fall back on, and a + raise here would turn a recoverable case into a failed run. + + Returns: + The PR number, or None if the payload does not yield one. + """ + path = os.environ.get("GITHUB_EVENT_PATH") + if not path: + return None + try: + with open(path, encoding="utf-8") as handle: + payload = json.load(handle) + except (OSError, ValueError): + return None + if not isinstance(payload, dict): + return None + pull_request = payload.get("pull_request") + if not isinstance(pull_request, dict): + return None + number = pull_request.get("number") + # bool is an int subclass, and `true` is valid JSON in that position. + if isinstance(number, bool) or not isinstance(number, int): + return None + return number + + @dataclass(frozen=True) class Config: """Parsed and validated configuration from GitHub Actions inputs. @@ -305,10 +339,15 @@ def parse_config() -> Config: repo = _env_required("GITHUB_REPOSITORY") event_name = os.environ.get("GITHUB_EVENT_NAME", "unknown") - # Extract PR number from GITHUB_REF (refs/pull//merge) - pr_number = None + # The event payload first, GITHUB_REF only as a fallback. Every + # pull_request payload carries .pull_request.number, whereas GITHUB_REF is + # refs/pull//merge only while GitHub has a merge ref to point the + # run at. When it points elsewhere the run used to abort with "Could not + # determine PR number" having measured nothing, which reads to the author + # as a coverage verdict on code the action never looked at. + pr_number = _pr_number_from_event() github_ref = os.environ.get("GITHUB_REF", "") - match = re.search(r"refs/pull/(\d+)/", github_ref) + match = None if pr_number is not None else re.search(r"refs/pull/(\d+)/", github_ref) if match: pr_number = int(match.group(1)) diff --git a/src/main.py b/src/main.py index ca1b59a..3bfe70c 100644 --- a/src/main.py +++ b/src/main.py @@ -197,7 +197,11 @@ def main() -> None: return if config.pr_number is None: - print("::error::Could not determine PR number from GITHUB_REF.") + print( + "::error::Could not determine the PR number from either the event payload " + "at GITHUB_EVENT_PATH or GITHUB_REF. Nothing was measured, so this is not a " + "test-adequacy verdict." + ) sys.exit(1) try: diff --git a/tests/test_config.py b/tests/test_config.py index f2c6117..55b7e05 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,6 +1,8 @@ # pyright: reportUnknownParameterType=false, reportMissingParameterType=false, reportUnknownMemberType=false """Tests for configuration parsing.""" +import json + import pytest from src.config import parse_config @@ -217,9 +219,82 @@ def test_pr_number_from_env(self, monkeypatch): monkeypatch.setenv("GITHUB_REPOSITORY", "owner/repo") monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request") monkeypatch.setenv("GITHUB_REF", "refs/pull/42/merge") + # This suite runs inside Actions too, where GITHUB_EVENT_PATH points at a + # real payload naming a real PR. Without this the assertion below would + # measure that PR instead of the ref under test. + monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) cfg = parse_config() assert cfg.pr_number == 42 + def test_pr_number_from_event_payload_when_the_ref_has_none(self, monkeypatch, tmp_path): + # The case the action used to abort on: a pull_request event whose ref is + # not refs/pull//merge. Observed on the `edited` activity type, where + # the run failed with "Could not determine PR number" without having + # measured anything. + payload = tmp_path / "event.json" + payload.write_text(json.dumps({"pull_request": {"number": 180}}), encoding="utf-8") + monkeypatch.setenv("GITHUB_TOKEN", "ghp_fake") + monkeypatch.setenv("GITHUB_REPOSITORY", "owner/repo") + monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request") + monkeypatch.setenv("GITHUB_REF", "refs/heads/main") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(payload)) + assert parse_config().pr_number == 180 + + def test_the_event_payload_wins_over_the_ref(self, monkeypatch, tmp_path): + # The payload describes the event being processed; the ref is a pointer + # that may lag it. When they disagree, the payload is the one to trust. + payload = tmp_path / "event.json" + payload.write_text(json.dumps({"pull_request": {"number": 180}}), encoding="utf-8") + monkeypatch.setenv("GITHUB_TOKEN", "ghp_fake") + monkeypatch.setenv("GITHUB_REPOSITORY", "owner/repo") + monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request") + monkeypatch.setenv("GITHUB_REF", "refs/pull/42/merge") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(payload)) + assert parse_config().pr_number == 180 + + @pytest.mark.parametrize( + "payload_text", + [ + pytest.param('{"pull_request": {"number": ', id="truncated json"), + pytest.param("not json at all", id="not json"), + pytest.param('{"push": {"ref": "refs/heads/main"}}', id="another event"), + pytest.param('{"pull_request": null}', id="null pull_request"), + pytest.param('{"pull_request": {"number": "180"}}', id="number as a string"), + pytest.param('{"pull_request": {"number": true}}', id="number as a boolean"), + pytest.param('{"pull_request": {}}', id="no number"), + pytest.param("[]", id="payload is a list"), + ], + ) + def test_an_unusable_event_payload_falls_back_to_the_ref( + self, monkeypatch, tmp_path, payload_text + ): + # None of these may raise. Each still has a usable ref, and a raise here + # would turn a recoverable run into a failed one. + payload = tmp_path / "event.json" + payload.write_text(payload_text, encoding="utf-8") + monkeypatch.setenv("GITHUB_TOKEN", "ghp_fake") + monkeypatch.setenv("GITHUB_REPOSITORY", "owner/repo") + monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request") + monkeypatch.setenv("GITHUB_REF", "refs/pull/42/merge") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(payload)) + assert parse_config().pr_number == 42 + + def test_a_missing_event_file_falls_back_to_the_ref(self, monkeypatch, tmp_path): + monkeypatch.setenv("GITHUB_TOKEN", "ghp_fake") + monkeypatch.setenv("GITHUB_REPOSITORY", "owner/repo") + monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request") + monkeypatch.setenv("GITHUB_REF", "refs/pull/42/merge") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(tmp_path / "absent.json")) + assert parse_config().pr_number == 42 + + def test_no_pr_number_when_neither_source_has_one(self, monkeypatch): + monkeypatch.setenv("GITHUB_TOKEN", "ghp_fake") + monkeypatch.setenv("GITHUB_REPOSITORY", "owner/repo") + monkeypatch.setenv("GITHUB_EVENT_NAME", "push") + monkeypatch.setenv("GITHUB_REF", "refs/heads/main") + monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) + assert parse_config().pr_number is None + def test_coverage_threshold_range_validation(self, monkeypatch): monkeypatch.setenv("GITHUB_TOKEN", "ghp_fake") monkeypatch.setenv("GITHUB_REPOSITORY", "owner/repo")