Skip to content

feat(pr-validation): button, real undo example, one live approval #908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bedatty merged 1 commit into from
Oct 15, 2026
Merged

feat(pr-validation): button, real undo example, one live approval #908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion .github/workflows/breaking-change-approval.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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 }}
Expand Down Expand Up @@ -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" ] \
Expand Down Expand Up @@ -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

Expand Down
20 changes: 20 additions & 0 deletions docs/breaking-change-approval.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 25 additions & 0 deletions src/notify/breaking-change-approval-reporter/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -79,6 +103,7 @@ side effect in the caller is what makes this safe to run with
| `reviewers` | Who can answer, as a `team:<slug>` / `user:<login>` 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 | `''` |
Expand Down
80 changes: 64 additions & 16 deletions src/notify/breaking-change-approval-reporter/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 }}
Expand Down Expand Up @@ -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})"
Expand All @@ -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 '<sub>Actions → **Review deployments** → `breaking-approval`</sub>\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'
Expand All @@ -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 '<details>\n'
printf '<summary><b>Not intentional?</b> — how to clear this without an approval</summary>\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 '</details>\n\n'
fi
[ -n "$context" ] && printf '<sub>%s</sub>\n\n' "$context"

if [ "$STATE" = "waiting" ]; then
printf '<sub>Not intended? Drop the `!` and any `BREAKING CHANGE:` footer from the commits and push again — the gate clears itself. '
printf '[How this works](%s)</sub>\n' "$DOCS_URL"
fi
context=""
[ -n "$REASON" ] && context="Gated by \`${REASON}\`"
context="${context:+$context · }[How this gate works](${DOCS_URL})"
printf '<sub>%s</sub>\n' "$context"
} > "$body_file"

comment_id=""
Expand Down
57 changes: 55 additions & 2 deletions src/validate/breaking-change-guard/test-workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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",
Expand All @@ -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)
Expand Down
Loading