From 1b4ea5ff2080af1d3506a673dfcb83acaac0c06a Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 22:15:12 -0300 Subject: [PATCH 1/2] fix(pr-validation): treat a cancelled approval as superseded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught on #906. Editing the pull request description fires `edited`, which queues a new run on the very same commit. The concurrency group cancelled the previous pending approval, correctly — but the superseded guard only compared head SHAs, and the head had not moved. So the dead run fell through to "decision not recorded", overwrote the live revision's comment and failed its check, while the new run sat waiting with the real request. A cancelled approval now counts as superseded on its own, and is checked first because it is the broader signal: nothing cancels it except the concurrency group replacing the run or a human doing it by hand, and neither is a decision worth reporting. The head comparison stays for whatever slips past that. Test pins both the new branch and its order ahead of the comparison. --- .../workflows/breaking-change-approval.yml | 25 +++++++++++++++---- docs/breaking-change-approval.md | 12 +++++++-- .../breaking-change-guard/test-workflow.py | 13 ++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 708d714e..769e3c5a 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -509,11 +509,26 @@ jobs: } >> "$GITHUB_OUTPUT" } - # A superseded run must not touch the pull request. When a push cancels - # the pending approval, this job still runs — and without this guard it - # would overwrite the current revision's comment with "the approval did - # not complete", which is true of the dead run and misleading about the - # live one. + # A superseded run must not touch the pull request. When something + # cancels the pending approval, this job still runs — and without this + # guard it would overwrite the live revision's comment with "the + # approval did not complete", which is true of the dead run and + # misleading about the live one. + # + # A cancelled approval is the first and broader signal. Nothing cancels + # it except the concurrency group replacing this run, or a human + # cancelling by hand, and neither is a decision to report. It matters + # on its own because the head does not have to change for a run to be + # replaced: editing the description fires `edited`, which queues a new + # run on the very same commit — and the comparison below would see + # nothing wrong. + if [ "$APPROVAL_RESULT" = "cancelled" ]; then + state=superseded + emit + echo "::notice::This run's approval was cancelled — another run is carrying the decision; leaving PR #${PR_NUMBER} alone" + exit 0 + fi + if [ -n "${PR_NUMBER:-}" ] && [ -n "${HEAD_SHA:-}" ]; then live_sha=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefOid --jq '.headRefOid' 2>/dev/null || true) if [ -n "$live_sha" ] && [ "$live_sha" != "$HEAD_SHA" ]; then diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index d144c8e9..963dc71b 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -266,8 +266,16 @@ revision, and the next one has to earn its own. Nothing inherits it. **A cancelled run never touches the pull request.** The verdict job still runs when its approval is cancelled, and without a guard it would overwrite the live revision's comment with *"the approval did not complete"* — true of the dead run, -misleading about the live one. It compares the commit it covers against the -pull request's current head and, when they differ, leaves everything alone. +misleading about the live one. + +Two signals, and the order matters. A **cancelled approval** is the broad one: +nothing cancels it except the concurrency group replacing the run or a human +doing it by hand, and neither is a decision to report. A **changed head** is the +narrower one, for anything that slipped past the first. + +The cancelled check cannot be dropped in favour of the head comparison, because +a run can be replaced without the head moving at all: editing the description +fires `edited`, which queues a new run on the very same commit. ## Fails closed diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 7cc94227..599df228 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -968,6 +968,19 @@ def test_a_superseded_run_leaves_the_pull_request_alone(self): # overwrite the live revision's comment with "did not complete". self.assertIn("state=superseded", body) self.assertIn('"$live_sha" != "$HEAD_SHA"', body) + # A changed head is not the only way to be superseded: `edited` queues a + # new run on the very same commit, so a cancelled approval has to count + # on its own. Observed on #906 — editing the description replaced the + # run, and the comparison above saw nothing wrong while the comment was + # overwritten with "decision not recorded". + self.assertIn('if [ "$APPROVAL_RESULT" = "cancelled" ]; then', body) + cancelled_at = body.index('"$APPROVAL_RESULT" = "cancelled"') + head_compare_at = body.index('"$live_sha" != "$HEAD_SHA"') + self.assertLess( + cancelled_at, + head_compare_at, + "the cancelled check must come first — it is the broader signal", + ) # And `superseded` must drive neither the comment nor the close. for step_name in ( "Report the outcome on the pull request", From b134e61fba686252c3f1b6db6586ff10f5924e20 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 09:08:55 -0300 Subject: [PATCH 2/2] test(pr-validation): run the verdict instead of reading it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #912. The cancellation test checked the branch and its ordering but not its `exit 0`. Without that exit a cancelled run falls through, and a recorded rejection downstream wins: the verdict comes back `rejected` and the pull request is closed — because a concurrency cancellation raced a reviewer. Closing a pull request by accident is the most destructive thing this workflow does, so "trivial" undersells it. Asserting the characters `exit 0` are present would prove nothing about whether the branch stops, and would be the fifth text assertion in this series. `run_verdict()` executes the step with `gh` stubbed instead, and the test hands it a rejection to fall through onto. Removing the exit makes it fail with 'superseded' != 'rejected' — which is how the report was confirmed in the first place. Also covers the whole state table by running it: approved, rejected, superseded from a cancellation, superseded from a moved head, unresolved, none, blocked. Those cases existed only in a scratch harness before. --- .../breaking-change-guard/test-workflow.py | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 599df228..f6e2ab6b 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -280,6 +280,68 @@ def render_reporter(state, **overrides): return result.stdout +def run_verdict(**overrides): + """Run the gate's verdict step and return its outputs. + + Asserting that the cancellation branch contains `exit 0` would only prove + the characters are there. Running it with a recorded rejection in hand is + what proves the branch actually stops: without the exit, the rejection + downstream wins and the pull request gets closed. + """ + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + step = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Resolve the verdict" + ) + + env = { + "GH_TOKEN": "stub", + "REPO": "LerianStudio/example", + "HEAD_SHA": "abc123", + "DETECT_RESULT": "success", + "REQUEST_RESULT": "success", + "APPROVAL_RESULT": "success", + "IN_SCOPE": "true", + "REASON": "rule develop <- '*'", + "PR_NUMBER": "42", + "HAS_BREAKING_CHANGES": "true", + "DETECTION_SUCCEEDED": "true", + "APPROVERS": "", + "REJECTERS": "", + "API_READABLE": "true", + "DRY_RUN": "false", + "LIVE_SHA": "abc123", + } + env.update(overrides) + + with tempfile.TemporaryDirectory() as temp_dir: + stub = Path(temp_dir) / "gh" + stub.write_text( + "#!/usr/bin/env bash\n" + 'case "$*" in\n' + ' *headRefOid*) printf \'%s\\n\' "$LIVE_SHA" ;;\n' + " *) exit 1 ;;\n" + "esac\n", + encoding="utf-8", + ) + stub.chmod(0o755) + output_path = Path(temp_dir) / "github-output" + output_path.touch() + env["GITHUB_OUTPUT"] = str(output_path) + env["GITHUB_STEP_SUMMARY"] = str(Path(temp_dir) / "summary") + env["PATH"] = f"{temp_dir}:{os.environ.get('PATH', '/usr/bin:/bin')}" + result = subprocess.run( + ["bash", "-c", step["run"]], + cwd=temp_dir, + env=env, + text=True, + capture_output=True, + check=False, + ) + return result, parse_github_output(output_path.read_text(encoding="utf-8")) + + def parse_github_output(output): values = {} for line in output.splitlines(): @@ -981,6 +1043,50 @@ def test_a_superseded_run_leaves_the_pull_request_alone(self): head_compare_at, "the cancelled check must come first — it is the broader signal", ) + + def test_a_cancelled_approval_stops_before_anything_can_act_on_it(self): + # Run it, with a recorded rejection sitting right there. The branch has + # to stop: if it falls through, the rejection downstream wins, the + # verdict comes back `rejected`, and the pull request gets closed — + # because a concurrency cancellation raced a reviewer. + _, outputs = run_verdict(APPROVAL_RESULT="cancelled", REJECTERS="dave") + self.assertEqual("superseded", outputs["state"]) + self.assertNotEqual("rejected", outputs["state"]) + + def test_the_verdict_states_are_reachable_as_documented(self): + # The whole table in one place, run rather than read. + for description, env, expected in ( + ("approved", {"APPROVAL_RESULT": "success"}, "approved"), + ( + "rejected", + {"APPROVAL_RESULT": "failure", "REJECTERS": "dave"}, + "rejected", + ), + ( + "cancelled, same head", + {"APPROVAL_RESULT": "cancelled"}, + "superseded", + ), + ( + "head moved under us", + {"LIVE_SHA": "def456"}, + "superseded", + ), + ( + "failed with no recorded decision", + {"APPROVAL_RESULT": "failure"}, + "unresolved", + ), + ("nothing to approve", {"HAS_BREAKING_CHANGES": "false"}, "none"), + ( + "detection never completed", + {"DETECTION_SUCCEEDED": "false"}, + "blocked", + ), + ): + with self.subTest(case=description): + _, outputs = run_verdict(**env) + self.assertEqual(expected, outputs["state"]) # And `superseded` must drive neither the comment nor the close. for step_name in ( "Report the outcome on the pull request",