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