From 1b21fb38e90ee4934186a39f00a940e80c01c500 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 18:48:38 -0300 Subject: [PATCH] feat(pr-validation): button, real undo example, one live approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things, from reviewing the comment on #906 in place. The call to action is now a shields.io badge wrapped in the link, which is the only thing in Markdown that reads as a button. That puts a third party in the path of a release control, so the alt text carries the whole call to action: a blocked image leaves a described link, not a broken rectangle. Setting badge-url to empty drops the dependency and falls back to a heading link. The "not intentional?" example now shows this pull request's own commit subjects in a diff block instead of a made-up feat(scope)!: subject. It matches only the `!` form, because that is the form the example illustrates — a view, never a verdict. The canonical rule stays in the detector, and with nothing matched the wording goes generic. And only one approval request stays open. Every push queued another pending deployment while the old ones kept sitting in the reviewers' queue, each tied to a commit that is no longer the head — #906 had two at once. Approving one of those approves a revision nobody is merging. A concurrency group keyed to the pull request cancels the previous request, which is also what expires an approval: the yes was given to a specific revision and the next one has to earn its own. The cancelled run still reaches the verdict, so it now checks whether it still covers the head and leaves the pull request alone when it does not. Otherwise it would overwrite the live comment with "the approval did not complete". --- .../workflows/breaking-change-approval.yml | 34 +++++++- docs/breaking-change-approval.md | 20 +++++ .../README.md | 25 ++++++ .../action.yml | 80 +++++++++++++++---- .../breaking-change-guard/test-workflow.py | 57 ++++++++++++- 5 files changed, 197 insertions(+), 19 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index d26bb4c3..708d714e 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -418,6 +418,18 @@ jobs: && needs.detect.outputs.has-breaking-changes == 'true' && !inputs.dry_run environment: breaking-approval + # One open request per pull request. Every push creates another pending + # deployment, and the old ones do not go away on their own — they sit in the + # reviewers' queue, each tied to a commit that is no longer the head. The + # risk is not clutter: approving a superseded one approves a revision nobody + # is merging, while the current one is still waiting. + # + # Cancelling the previous request is also what expires an approval. A push + # kills the pending job, so the next revision has to be approved again + # rather than inheriting a yes given to different code. + concurrency: + group: breaking-approval-${{ github.repository }}-${{ needs.detect.outputs.pr-number }} + cancel-in-progress: true permissions: contents: read steps: @@ -462,6 +474,9 @@ jobs: - name: Resolve the verdict id: verdict env: + GH_TOKEN: ${{ secrets.MANAGE_TOKEN || github.token }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ needs.detect.outputs.head-sha }} DETECT_RESULT: ${{ needs.detect.result }} REQUEST_RESULT: ${{ needs.request.result }} APPROVAL_RESULT: ${{ needs.approval.result }} @@ -494,6 +509,21 @@ 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. + 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 + state=superseded + emit + echo "::notice::Superseded — this run covers ${HEAD_SHA:0:7} but PR #${PR_NUMBER} now heads at ${live_sha:0:7}; leaving the pull request alone" + exit 0 + fi + fi + # Detection failing is not "no breaking changes" — it is "we do not # know", and the point of the gate is that an unknown blocks. if [ "$DETECT_RESULT" != "success" ] || [ "$DETECTION_SUCCEEDED" != "true" ] \ @@ -606,7 +636,9 @@ jobs: } >> "$GITHUB_STEP_SUMMARY" # dry_run never fails: it reports what a real run would have done. - if [ "$DRY_RUN" = "true" ] || [ "$RESULT" = "success" ]; then + # Neither does a superseded run — its verdict is about a commit that is + # no longer the head, and the live run is the one that decides. + if [ "$DRY_RUN" = "true" ] || [ "$RESULT" = "success" ] || [ "$STATE" = "superseded" ]; then exit 0 fi diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index cd86d24b..d144c8e9 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -249,6 +249,26 @@ them match: REST returns `dependabot[bot]`, GraphQL returns `dependabot`, and `gh pr view --json author` — what this job reads — returns `app/dependabot`. Both affixes are stripped before comparing. +## One open request, and approvals expire on a push + +Every push queues another pending deployment, and GitHub does not retire the +previous one. Left alone, a reviewer's queue fills with requests tied to commits +that are no longer the head — and approving one of those approves a revision +nobody is merging, while the live one still waits. + +The approval job is therefore under a `concurrency` group keyed to the pull +request, with `cancel-in-progress`. A push cancels the pending request and +replaces it with one for the new head. + +That is also what expires an approval: the yes was given to a specific +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. + ## Fails closed Detection failing is not "no breaking changes" — it is "we do not know", and an diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 2092f48e..2050fcb2 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -27,6 +27,22 @@ comment, and it ends as the record of who decided what — nothing is deleted. Any other value fails the step. A typo in `state` must not produce a confidently worded comment about the wrong thing. +## The button is a badge, and the alt text is the fallback + +Markdown has no button, so the call to action is a [shields.io](https://shields.io) +badge wrapped in the link. That is a third party in the path of a release +control, which is worth stating plainly — so the alt text carries the entire +call to action: + +```markdown +[![Review the pending deployment](https://img.shields.io/badge/...)](run-url) +``` + +A reader whose network blocks the image, or a day when shields is down, sees a +described link rather than a broken rectangle. Setting `badge-url` to empty +falls back to a plain `### ➡️ [Review the pending deployment](…)` heading, with +no external dependency at all. + ## It reads as information, not as a block A breaking change is often deliberate, and a comment that opens with a siren @@ -52,6 +68,14 @@ of the way of whoever does not. already notifies exactly these people; a mention would ping the whole team a second time for the same thing. +**The undo example uses this pull request's own commits.** The composite lists +the pull request's commits and shows the real subjects in a `diff` block, +instead of a made-up `feat(scope)!: subject`. Only the `!` form is matched +there, because that is the form the example illustrates — it is a view, never a +verdict. The canonical rule lives in +[`breaking-change-guard`](../../validate/breaking-change-guard/README.md); when +the two disagree the detector wins and this falls back to generic wording. + **`actual-reviewers` wins over `reviewers`.** An Environment that already existed may be gated by different people than the caller configured, and naming the wrong ones is worse than naming none — with neither available, the line is omitted @@ -79,6 +103,7 @@ side effect in the caller is what makes this safe to run with | `reviewers` | Who can answer, as a `team:` / `user:` list. | No | `''` | | `actual-reviewers` | Reviewers read from the Environment itself. Preferred over `reviewers` when present. | No | `''` | | `server-url` | Base URL used to build commit links. | No | `${{ github.server_url }}` | +| `badge-url` | Image used as the call-to-action button. Empty falls back to a plain heading link. | No | `https://img.shields.io/badge/%E2%9E%A1%EF%B8%8F%20Review%20pending%20deployment-1f6feb?style=for-the-badge` | | `docs-url` | Link to the explanation of how the gate works. | No | `https://github.com/LerianStudio/github-actions-shared-workflows/blob/main/docs/breaking-change-approval.md` | | `approvers` | Comma-separated logins that approved. | No | `''` | | `rejecters` | Comma-separated logins that rejected. | No | `''` | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 42a6bf76..9768c32d 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -43,6 +43,10 @@ inputs: description: Base URL used to build commit links. required: false default: ${{ github.server_url }} + badge-url: + description: 'Image used as the call-to-action button. Markdown has no button, so this is a badge image wrapped in the link. Its alt text carries the full call to action, so a reader whose network blocks the image still sees a described link. Set to empty for a plain text link instead.' + required: false + default: 'https://img.shields.io/badge/%E2%9E%A1%EF%B8%8F%20Review%20pending%20deployment-1f6feb?style=for-the-badge' docs-url: description: Link to the explanation of how the gate works. required: false @@ -94,6 +98,7 @@ runs: RUN_URL: ${{ inputs.run-url }} REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-reviewers || inputs.reviewers }} SERVER_URL: ${{ inputs.server-url }} + BADGE_URL: ${{ inputs.badge-url }} DOCS_URL: ${{ inputs.docs-url }} APPROVERS: ${{ inputs.approvers }} REJECTERS: ${{ inputs.rejecters }} @@ -139,6 +144,23 @@ runs: esac done < <(tr ',' '\n' <<< "$REVIEWERS" | xargs -n1 2>/dev/null) + # The subjects that actually tripped the gate, so the "how to undo it" + # example shows this pull request's own commits rather than a made-up + # `feat(scope)!: subject`. + # + # Only the `!` form is matched here, because that is the form the diff + # below illustrates. This is a view, never a verdict: the canonical rule + # lives in the breaking-change-guard composite, and when the two disagree + # the detector wins — this just falls back to generic wording. + breaking_subjects="" + if [ "$STATE" = "waiting" ]; then + breaking_subjects=$( + gh api "repos/${REPO}/pulls/${PR_NUMBER}/commits" --paginate \ + --jq '.[] | .commit.message | split("\n")[0]' 2>/dev/null \ + | grep -E '^[A-Za-z0-9_]*(\([^)]*\))?!: ' || true + ) + fi + commit_link="" if [ -n "$HEAD_SHA" ]; then commit_link="[\`${HEAD_SHA:0:7}\`](${SERVER_URL}/${REPO}/commit/${HEAD_SHA})" @@ -150,11 +172,28 @@ runs: case "$STATE" in waiting) - printf '### Breaking change — needs a decision\n\n' - printf '%s carries a breaking change, so merging makes the next release a **major** version. That is often deliberate — this just asks someone to confirm it.\n\n' \ - "${commit_link:-This branch}" - [ -n "$RUN_URL" ] && printf '### ➡️ [Approve or reject](%s)\n\n' "$RUN_URL" - [ -n "$mentions" ] && printf 'Who can: %s\n\n' "$mentions" + printf '> [!IMPORTANT]\n' + printf '> **Breaking change detected — this pull request needs a decision**\n' + printf '>\n' + printf '> Merging it makes the next release a **major** version. That is often deliberate; this just asks someone to confirm it before it ships.\n\n' + + # A shields.io badge renders as an actual button, which nothing in + # GitHub-flavoured Markdown does on its own. The alt text carries + # the full call to action on purpose: if the image fails to load — + # shields down, or a network that blocks it — the reader still sees + # a described link rather than a broken rectangle. + if [ -n "$RUN_URL" ] && [ -n "$BADGE_URL" ]; then + printf '[![Review the pending deployment](%s)](%s)\n\n' "$BADGE_URL" "$RUN_URL" + printf 'Actions → **Review deployments** → `breaking-approval`\n\n' + elif [ -n "$RUN_URL" ]; then + printf '### ➡️ [Review the pending deployment](%s)\n\n' "$RUN_URL" + fi + + printf '| | |\n|:--|:--|\n' + [ -n "$commit_link" ] && printf '| **Commit** | %s |\n' "$commit_link" + [ -n "$mentions" ] && printf '| **Who can decide** | %s |\n' "$mentions" + printf '| **Author acknowledgement** | %s |\n\n' \ + "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" ;; approved) printf '### ✅ Breaking change approved\n\n' @@ -178,22 +217,31 @@ runs: ;; esac - # One compact line of context instead of a table of three rows. - context="" - [ -n "$REASON" ] && context="Gated by \`${REASON}\`" + # The escape hatch is folded away: it matters enormously to whoever + # broke the contract by accident, and not at all to whoever meant to. if [ "$STATE" = "waiting" ]; then - if [ "$ACKNOWLEDGED" = "true" ]; then - context="${context:+$context · }author acknowledged" + printf '
\n' + printf 'Not intentional? — how to clear this without an approval\n\n' + if [ -n "$breaking_subjects" ]; then + printf 'Drop the `!` from the commit subject, then push again. The gate re-evaluates and clears itself.\n\n' + printf '```diff\n' + while IFS= read -r subject; do + [ -z "$subject" ] && continue + printf -- '- %s\n' "$subject" + printf -- '+ %s\n' "$(sed -E 's/^([A-Za-z0-9_]*(\([^)]*\))?)!:/\1:/' <<< "$subject")" + done <<< "$breaking_subjects" + printf '```\n\n' + printf 'Also remove any `BREAKING CHANGE:` footer from the commit bodies, if there is one.\n\n' else - context="${context:+$context · }author acknowledgement missing" + printf 'Remove the `BREAKING CHANGE:` footer from the commit bodies, and drop any `!` after the type in the subjects. Push again and the gate re-evaluates.\n\n' fi + printf '
\n\n' fi - [ -n "$context" ] && printf '%s\n\n' "$context" - if [ "$STATE" = "waiting" ]; then - printf 'Not intended? Drop the `!` and any `BREAKING CHANGE:` footer from the commits and push again — the gate clears itself. ' - printf '[How this works](%s)\n' "$DOCS_URL" - fi + context="" + [ -n "$REASON" ] && context="Gated by \`${REASON}\`" + context="${context:+$context · }[How this gate works](${DOCS_URL})" + printf '%s\n' "$context" } > "$body_file" comment_id="" diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 941ec476..72cee214 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -829,9 +829,9 @@ def test_the_waiting_comment_links_instead_of_mentioning(self): # Links to the commit, the decision, the reviewers and the docs. for fragment in ( "${SERVER_URL}/${REPO}/commit/${HEAD_SHA}", - "[Approve or reject](%s)", + "[![Review the pending deployment](%s)](%s)", "${SERVER_URL}/orgs/${owner}/teams/${slug}", - "[How this works](%s)", + "[How this gate works](${DOCS_URL})", ): with self.subTest(fragment=fragment): self.assertIn(fragment, reporter) @@ -850,6 +850,37 @@ def test_the_waiting_comment_links_instead_of_mentioning(self): f"...{reporter[max(0, match.start() - 40):match.end() + 20]}...", ) + def test_only_one_approval_request_stays_open_per_pull_request(self): + job = extract_job(APPROVAL_WORKFLOW, "approval") + # Every push queues another pending deployment, and the old ones do not + # expire on their own: approving a superseded one approves a revision + # nobody is merging. Cancelling is also what expires an approval, so the + # next revision cannot inherit a yes given to different code. + self.assertIn("concurrency:", job) + self.assertIn( + "group: breaking-approval-${{ github.repository }}" + "-${{ needs.detect.outputs.pr-number }}", + job, + ) + self.assertIn("cancel-in-progress: true", job) + + def test_a_superseded_run_leaves_the_pull_request_alone(self): + body = extract_run_body( + extract_job(APPROVAL_WORKFLOW, "outcome"), "Resolve the verdict" + ) + # The cancelled run still reaches the verdict. Without this it would + # overwrite the live revision's comment with "did not complete". + self.assertIn("state=superseded", body) + self.assertIn('"$live_sha" != "$HEAD_SHA"', body) + # And `superseded` must drive neither the comment nor the close. + for step_name in ( + "Report the outcome on the pull request", + "Close the pull request on rejection", + ): + with self.subTest(step=step_name): + step = extract_step(extract_job(APPROVAL_WORKFLOW, "outcome"), step_name) + self.assertNotIn("superseded", step) + def test_the_environment_is_read_exactly_once(self): # Two reads would allow "rules fetched, reviewers not" — and an empty # reviewer list is indistinguishable from "this Environment has none", @@ -862,6 +893,28 @@ def test_the_environment_is_read_exactly_once(self): self.assertIn("reviewers_of()", ENVIRONMENT_PROTECTION) self.assertIn("rules_of()", ENVIRONMENT_PROTECTION) + def test_the_button_degrades_when_the_badge_cannot_load(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + # The badge is an external image. Its alt text carries the whole call to + # action, so a blocked image leaves a described link, not a broken box. + self.assertIn("[![Review the pending deployment]", reporter) + # And an empty badge-url falls back to a plain heading link. + self.assertIn('elif [ -n "$RUN_URL" ]; then', reporter) + + def test_the_undo_example_uses_the_real_commit_subjects(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + self.assertIn('repos/${REPO}/pulls/${PR_NUMBER}/commits', reporter) + self.assertIn("```diff", reporter) + # A view, never a verdict: with no subject matched it must fall back to + # generic wording rather than invent an example. + self.assertIn('if [ -n "$breaking_subjects" ]; then', reporter) + def test_a_missing_environment_is_told_apart_from_an_unreadable_one(self): # Creating on top of a 403 would be guessing; only a 404 is ours to fix. self.assertIn('*"HTTP 404"* | *"Not Found"* )', ENVIRONMENT_PROTECTION)