From 2d1c1f8efa664f28b6445647f2d52f3fffd9325d Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Tue, 20 Oct 2026 11:55:22 -0300 Subject: [PATCH] fix(pr-validation): say which half of the gate failed, not just that it did The blocked comment said only that the gate did not reach a verdict and linked the run. True, and no help: the author opens the run and hunts for the red job. Measured on ungoliant-controller, whose first breaking change produced exactly that comment. The cause was three steps away in the log - no catch-all CODEOWNERS rule, no fallback reviewer, so the Environment could not be created - and nothing on the pull request pointed at any of it. The verdict already knew which half failed; it was writing it to the log and telling nobody else. It now emits the stage, the outcome forwards it, and the comment names it. The request case also names the cause that actually occurs, since it is the one a reader cannot guess. Empty keeps the neutral wording, so a caller that does not pass it is no worse off. @radagast-lerian offered both options on #926 and I took the cheaper one; this is the other. --- .../workflows/breaking-change-approval.yml | 19 ++++++++ .../README.md | 3 +- .../action.yml | 27 ++++++++++-- .../breaking-change-guard/test-workflow.py | 44 +++++++++++++++++++ 4 files changed, 89 insertions(+), 4 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 9183030b..4715d715 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -869,6 +869,7 @@ jobs: approved=false result=failure state=unresolved + stage="" rejecters="$REJECTERS" emit() { @@ -876,6 +877,7 @@ jobs: echo "approved=$approved" echo "result=$result" echo "state=$state" + echo "stage=$stage" echo "rejecters=$rejecters" } >> "$GITHUB_OUTPUT" } @@ -971,6 +973,19 @@ jobs: if [ "$DETECT_RESULT" != "success" ] || [ "$DETECTION_SUCCEEDED" != "true" ] \ || [ "$REQUEST_RESULT" != "success" ]; then state=blocked + + # Which half failed, carried through to the comment. It used to say + # only "the gate did not reach a verdict" and point at the run — + # true, and no help to the author, who then opens the run and hunts + # for the red job. The verdict already knew; it was telling the log + # and nobody else. Measured on a real repository: this is exactly + # what ungoliant-controller's first breaking change produced. + if [ "$DETECT_RESULT" != "success" ] || [ "$DETECTION_SUCCEEDED" != "true" ]; then + stage=detection + else + stage=request + fi + emit echo "::error::Breaking change gate could not run (detect=${DETECT_RESULT}, request=${REQUEST_RESULT}, detection_succeeded=${DETECTION_SUCCEEDED})" exit 0 @@ -1067,6 +1082,10 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} pr-number: ${{ needs.detect.outputs.pr-number }} state: ${{ steps.verdict.outputs.state }} + # Which half of the gate failed. Without it the comment can only say + # "it did not reach a verdict" and point at the run, leaving the + # author to open it and find the red job. + blocked-stage: ${{ steps.verdict.outputs.stage }} # Only a run whose Environment approval actually happened may write a # record naming itself. A reused approval reports `approved` with the # gate job skipped, and a record pointing at that run would assert diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 45cd2152..f8c36ac5 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -24,7 +24,7 @@ comment, and it ends as the record of who decided what — nothing is deleted. | `approved` | Approved by whom, and that the next release will be a major version | | `rejected` | Rejected by whom, the reason they gave, and what to do next | | `unresolved` | The approval ended without a recorded decision — open the run | -| `blocked` | The gate could not reach a verdict at all — detection or the request stage failed, and the check fails closed. Deliberately neutral about which: `blocked` is reached from both. The only state that does **not** apply the fact label, because nothing was established — labels already on the pull request are left alone | +| `blocked` | The gate could not reach a verdict — detection or the request stage failed, and the check fails closed. Pass `blocked-stage` to say which; without it the wording stays neutral, since `blocked` is reached from both. The only state that does **not** apply the fact label, because nothing was established — labels already on the pull request are left alone | Any other value fails the step. A typo in `state` must not produce a confidently worded comment about the wrong thing. @@ -117,6 +117,7 @@ side effect in the caller is what makes this safe to run with | `pr-number` | Pull request to report on. | Yes | — | | `repository` | Repository owning it, as `owner/name`. | No | `${{ github.repository }}` | | `state` | `waiting`, `acknowledgement`, `approved`, `rejected`, `unresolved` or `blocked`. | Yes | — | +| `blocked-stage` | With `state: blocked`, which half failed — `detection` or `request`. Named in the comment so the author knows where to look. | No | `''` | | `reason` | Why the pull request is gated, from `pr-gate-scope`. | No | `''` | | `head-sha` | Head commit the report refers to. | No | `''` | | `acknowledged` | Whether the author acknowledgement line is present. | No | `false` | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 386bf4d5..736a8249 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -60,6 +60,14 @@ inputs: resolver first. required: false default: '' + blocked-stage: + description: >- + With `state: blocked`, which half of the gate failed — `detection` or + `request`. Named in the comment so the author knows where to look + instead of opening the run and hunting for the red job. Empty keeps the + generic wording. + required: false + default: '' fingerprint: description: 'Digest of the breaking-change commit set, from `breaking-change-commits`. Recorded in the comment when the state is `approved`, alongside the run that carried the approval, so a later run can verify it against the Environment log rather than trusting the text. Previous records are preserved.' required: false @@ -121,6 +129,7 @@ runs: HEAD_SHA: ${{ inputs.head-sha }} ACKNOWLEDGED: ${{ inputs.acknowledged }} SUBJECTS: ${{ inputs.subjects }} + BLOCKED_STAGE: ${{ inputs.blocked-stage }} ACK_LINE: ${{ inputs.author-acknowledgement }} RUN_URL: ${{ inputs.run-url }} REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-reviewers || inputs.reviewers }} @@ -360,9 +369,21 @@ runs: printf '> [!CAUTION]\n' printf '> ## 💥 Breaking change gate could not run\n' printf '>\n' - printf '> The gate did not reach a verdict, so this check fails closed — an unknown is not the same as "no breaking changes".' - [ -n "$RUN_URL" ] && printf ' [Open the run](%s) to see which step failed.' "$RUN_URL" - printf '\n\n' + case "$BLOCKED_STAGE" in + detection) + printf '> **Detection did not complete**, so the gate does not know whether this pull request carries a breaking change — and an unknown is not the same as "no breaking changes". This check fails closed.\n' + ;; + request) + printf '> The break was detected, but **the approval could not be requested**, so there is nothing for a reviewer to answer. This check fails closed.\n' + printf '>\n' + printf '> Most often the `breaking-approval` Environment does not exist and could not be created — either no reviewer could be resolved (the repository CODEOWNERS has no catch-all `*` rule and no fallback is configured), or the token cannot create Environments. The job log names which.\n' + ;; + *) + printf '> The gate did not reach a verdict, so this check fails closed — an unknown is not the same as "no breaking changes".\n' + ;; + esac + [ -n "$RUN_URL" ] && printf '>\n> [Open the run](%s) for the failing step.\n' "$RUN_URL" + printf '\n' # `ack_followup` is a no-op unless the acknowledgement was # actually evaluated: the gate sends an empty value, not # `false`, when nothing read the description. diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 1cb0b6fc..f770c507 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -261,6 +261,7 @@ def render_reporter(state, **overrides): # composite reads the commits itself and `MOCK_SUBJECTS` feeds the # stubbed API. Pass `SUBJECTS` to exercise the forwarded set instead. "SUBJECTS": "", + "BLOCKED_STAGE": "", "MOCK_SUBJECTS": "", } env.update(overrides) @@ -478,6 +479,7 @@ def run_reporter_comment( "ACKNOWLEDGED": "true", "ACK_LINE": "Breaking change acknowledged: …", "SUBJECTS": "", + "BLOCKED_STAGE": "", "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", "REVIEWERS": "team:devops", "SERVER_URL": "https://github.com", @@ -592,6 +594,7 @@ def run_reporter_labels(state, **overrides): "PENDING_LABEL": "awaiting-breaking-approval", "ENABLED": "false", "SUBJECTS": "", + "BLOCKED_STAGE": "", "FINGERPRINT": "", "RUN_ID": "1", "GH_TOKEN": "stub", @@ -2265,6 +2268,47 @@ def test_no_instruction_is_better_than_an_empty_one(self): # The break is still announced — only the unusable instruction is gone. self.assertIn("acknowledgement missing", rendered) + def test_a_blocked_gate_names_the_half_that_failed(self): + # Measured on a real repository: ungoliant-controller's first breaking + # change hit this, and the comment said only "did not reach a verdict" + # with a link to the run. True, and no help — the author has to open + # the run and find the red job. The verdict already knew which half + # failed; it was telling the log and nobody else. + detection = render_reporter("blocked", ACKNOWLEDGED="", BLOCKED_STAGE="detection") + self.assertIn("Detection did not complete", detection) + self.assertNotIn("could not be requested", detection) + + request = render_reporter("blocked", ACKNOWLEDGED="", BLOCKED_STAGE="request") + self.assertIn("the approval could not be requested", request) + self.assertNotIn("Detection did not complete", request) + # And the cause that actually happens, named rather than implied. + self.assertIn("breaking-approval", request) + self.assertIn("catch-all", request) + + # Empty keeps the generic wording — a caller that does not pass it is + # no worse off than before. + generic = render_reporter("blocked", ACKNOWLEDGED="", BLOCKED_STAGE="") + self.assertIn("did not reach a verdict", generic) + + # The verdict resolves the stage, and the outcome forwards it. + _, from_detection = run_verdict(DETECTION_SUCCEEDED="false") + self.assertEqual("detection", from_detection["stage"]) + _, from_request = run_verdict(REQUEST_RESULT="failure") + self.assertEqual("request", from_request["stage"]) + # Not set when nothing is blocked — it describes a failure, not a state. + _, approved = run_verdict(APPROVAL_RESULT="success") + self.assertEqual("", approved["stage"]) + + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + step = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Report the outcome on the pull request" + ) + self.assertEqual( + "${{ steps.verdict.outputs.stage }}", step["with"]["blocked-stage"] + ) + def test_a_blocked_gate_says_so_on_the_pull_request(self): # Detection failing fails the check. It used to do that silently: a red # check with no comment reads as a flake and gets re-run rather than