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..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(): @@ -968,6 +1030,63 @@ 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", + ) + + 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",