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)