From e6106f6e1ebaba39ad54a11ca0b0baed47f6ffc4 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:10:57 -0300 Subject: [PATCH 01/47] fix(pr-validation): make the breaking-change comment read as information MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The waiting comment opened with a siren and spent four paragraphs explaining itself. A breaking change is often deliberate, and a comment that shouts teaches people to scroll past it — which is the one thing a gate cannot afford. Now: what was found, a link straight to the decision, and who can make it. Everything else — the scope rule, the acknowledgement state, how to undo the break — is one small line at the bottom. Eighteen lines down to nine. Four things are links now where there were none: the offending commit, the pending deployment, the reviewers, and the docs. "Who can approve this" was the obvious next question and the answer was buried in Environment settings nobody thinks to open. Reviewers render as links, never as @mentions: the pending deployment already notifies exactly those people. --- .../workflows/breaking-change-approval.yml | 1 + .../README.md | 27 +++++ .../action.yml | 106 ++++++++++++------ .../breaking-change-guard/test-workflow.py | 18 +++ 4 files changed, 120 insertions(+), 32 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 6112fdea..215c0a51 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -392,6 +392,7 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} pr-number: ${{ needs.detect.outputs.pr-number }} state: waiting + reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} reason: ${{ needs.detect.outputs.reason }} head-sha: ${{ needs.detect.outputs.head-sha }} acknowledged: ${{ needs.detect.outputs.acknowledged }} diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 516ff460..ff4d28f7 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -27,6 +27,30 @@ 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. +## It reads as information, not as a block + +A breaking change is often deliberate, and a comment that opens with a siren +teaches people to scroll past it. The waiting comment states what was found, +links straight to the decision, and names who can make it: + +> ### Breaking change — needs a decision +> +> [`f8d5506`](#) carries a breaking change, so merging makes the next release a +> **major** version. That is often deliberate — this just asks someone to +> confirm it. +> +> ### ➡️ [Approve or reject](#) +> +> Who can: [@LerianStudio/g_github_devops](#) + +Everything else — the scope rule, the acknowledgement state, how to undo the +break — is one `` line at the bottom, present for whoever needs it and out +of the way of whoever does not. + +**Reviewers are rendered as links, never as @mentions.** The pending deployment +already notifies exactly these people; a mention would ping the whole team a +second time for the same thing. + ## What it does not do It never closes, merges or approves anything. `close-on-rejection` only changes @@ -46,6 +70,9 @@ side effect in the caller is what makes this safe to run with | `head-sha` | Head commit the report refers to. | No | `''` | | `acknowledged` | Whether the author acknowledgement line is present. | No | `false` | | `run-url` | Link to the run carrying the pending deployment. | No | `''` | +| `reviewers` | Who can answer, as the `team:` / `user:` list the Environment was given. | No | `''` | +| `server-url` | Base URL used to build commit links. | No | `${{ github.server_url }}` | +| `docs-url` | Link to the explanation of how the gate works. | No | the gate's docs page | | `approvers` | Comma-separated logins that approved. | No | `''` | | `rejecters` | Comma-separated logins that rejected. | No | `''` | | `rejection-comment` | Comment left with the rejection. | No | `''` | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 19b33ac0..e5f5b79f 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -31,6 +31,18 @@ inputs: description: Link to the run carrying the pending deployment. required: false default: '' + reviewers: + description: 'Who can answer, as the `team:` / `user:` list the Environment was given. Rendered as GitHub mentions so the reader knows who to ask instead of hunting through settings.' + required: false + default: '' + server-url: + description: Base URL used to build commit links. + required: false + default: ${{ github.server_url }} + docs-url: + description: Link to the explanation of how the gate works. + required: false + default: 'https://github.com/LerianStudio/github-actions-shared-workflows/blob/main/docs/breaking-change-approval.md' approvers: description: Comma-separated logins that approved. required: false @@ -76,6 +88,9 @@ runs: HEAD_SHA: ${{ inputs.head-sha }} ACKNOWLEDGED: ${{ inputs.acknowledged }} RUN_URL: ${{ inputs.run-url }} + REVIEWERS: ${{ inputs.reviewers }} + SERVER_URL: ${{ inputs.server-url }} + DOCS_URL: ${{ inputs.docs-url }} APPROVERS: ${{ inputs.approvers }} REJECTERS: ${{ inputs.rejecters }} REJECTION_COMMENT: ${{ inputs.rejection-comment }} @@ -96,58 +111,85 @@ runs: body_file=$(mktemp) trap 'rm -f "$body_file"' EXIT INT TERM + owner=${REPO%%/*} + + # `team:x,user:y` -> linked names. The reader's next question is always + # "who can unblock this", and the answer is sitting in the Environment + # where nobody thinks to look. + # + # Links, deliberately not @mentions: the pending deployment already + # notifies these very people, and a mention would ping the whole team a + # second time for the same thing. + mentions="" + while IFS= read -r entry; do + [ -z "$entry" ] && continue + case "$entry" in + team:*) + slug=${entry#team:} + mentions="${mentions:+$mentions, }[@${owner}/${slug}](${SERVER_URL}/orgs/${owner}/teams/${slug})" + ;; + *) + login=${entry#user:} + mentions="${mentions:+$mentions, }[@${login}](${SERVER_URL}/${login})" + ;; + esac + done < <(tr ',' '\n' <<< "$REVIEWERS" | xargs -n1 2>/dev/null) + + commit_link="" + if [ -n "$HEAD_SHA" ]; then + commit_link="[\`${HEAD_SHA:0:7}\`](${SERVER_URL}/${REPO}/commit/${HEAD_SHA})" + fi + # shellcheck disable=SC2016 # the backticks below are Markdown code spans, not command substitutions { printf '\n\n' case "$STATE" in waiting) - printf '### 🚨 Breaking change — approval required\n\n' - printf 'A breaking-change commit was found on this branch, so merging it will force the next release to a **major** version.\n\n' - printf 'This pull request is held until a reviewer answers one question: **is this breaking change necessary?**\n\n' - [ -n "$RUN_URL" ] && printf '👉 **[Review the pending deployment](%s)** — *Review deployments* → `breaking-approval`.\n\n' "$RUN_URL" + 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" ;; approved) printf '### ✅ Breaking change approved\n\n' - printf 'Approved by **%s** — the break is considered necessary and the next release will be a major version.\n\n' \ + printf 'Approved by **%s**. The next release will be a major version.\n\n' \ "${APPROVERS:-a reviewer}" ;; rejected) - printf '### ⛔ Breaking change rejected\n\n' - printf 'Rejected by **%s** — the break is not considered necessary.\n\n' "${REJECTERS:-a reviewer}" - [ -n "$REJECTION_COMMENT" ] && printf '> %s\n\n' "$REJECTION_COMMENT" + printf '### Breaking change rejected\n\n' + printf 'Rejected by **%s**' "${REJECTERS:-a reviewer}" + [ -n "$REJECTION_COMMENT" ] && printf ' — *%s*' "$REJECTION_COMMENT" + printf '.\n\n' + if [ "$CLOSE_ON_REJECTION" = "true" ]; then + printf 'This pull request was closed. The branch is untouched — rework it without the break and reopen.\n\n' + fi ;; unresolved) - printf '### 🚨 Breaking change — approval required\n\n' + printf '### Breaking change — decision not recorded\n\n' printf 'The approval did not complete.' - [ -n "$RUN_URL" ] && printf ' [Open the run](%s) and answer the pending deployment.' "$RUN_URL" + [ -n "$RUN_URL" ] && printf ' [Open the run](%s) to answer it.' "$RUN_URL" printf '\n\n' ;; esac - printf '| | |\n|---|---|\n' - [ -n "$REASON" ] && printf '| Gated via | `%s` |\n' "$REASON" - [ -n "$HEAD_SHA" ] && printf '| Head commit | `%s` |\n' "${HEAD_SHA:0:7}" - printf '| Author acknowledgement in the description | %s |\n' \ - "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" - printf '\n' + # One compact line of context instead of a table of three rows. + context="" + [ -n "$REASON" ] && context="Gated by \`${REASON}\`" + if [ "$STATE" = "waiting" ]; then + if [ "$ACKNOWLEDGED" = "true" ]; then + context="${context:+$context · }author acknowledged" + else + context="${context:+$context · }author acknowledgement missing" + fi + fi + [ -n "$context" ] && printf '%s\n\n' "$context" - case "$STATE" in - waiting) - printf '**Approving** records that the approvers consider the break necessary, and releases the merge.\n\n' - if [ "$CLOSE_ON_REJECTION" = "true" ]; then - printf '**Rejecting** records that it is not, and **closes this pull request** so the change can be reworked without the break. Reopening is one click and the branch is untouched.\n\n' - else - printf '**Rejecting** records that it is not, and fails this check.\n\n' - fi - printf 'If the break was unintentional, reword the commits — drop the `!` after the type and remove any `BREAKING CHANGE:` footer — and push again. No approval is needed then.\n' - ;; - rejected) - if [ "$CLOSE_ON_REJECTION" = "true" ]; then - printf 'This pull request was closed. Rework the change so it does not break the contract — drop the `!` and any `BREAKING CHANGE:` footer once it no longer applies — then reopen it. The branch is untouched.\n' - fi - ;; - esac + 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 } > "$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 3a1cdb77..058c3a07 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -821,6 +821,24 @@ def test_reviewers_come_from_codeowners_on_the_base_ref(self): preflight, ) + def test_the_waiting_comment_links_instead_of_mentioning(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + # Links to the commit, the decision, the reviewers and the docs. + for fragment in ( + "${SERVER_URL}/${REPO}/commit/${HEAD_SHA}", + "[Approve or reject](%s)", + "${SERVER_URL}/orgs/${owner}/teams/${slug}", + "[How this works](%s)", + ): + with self.subTest(fragment=fragment): + self.assertIn(fragment, reporter) + # The pending deployment already notifies the reviewers; a bare @mention + # would ping the whole team again for the same thing. + self.assertNotIn('}@${owner}/', 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) From 8db3d0256c0b62ce5a4c8fd6a64a206a83d1beda Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:26:28 -0300 Subject: [PATCH 02/47] fix(pr-validation): name the Environment's real reviewers in the comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #907. environment-protection now reads back who the Environment is actually gated by and exposes it, and the reporter prefers that over the configured list. The two differ whenever the Environment already existed or was edited since — and naming the wrong approvers is worse than naming none, which is what the comment does when neither is available. Also from the review: the `reviewers` description still said "rendered as GitHub mentions", written before the switch to links and never updated the README gave `docs-url` as "the gate's docs page" where .coderabbit.yml asks for input tables to match action.yml exactly the example's links all pointed at `(#)` the no-bare-mention test asserted one forbidden spelling, so it passed by accident and would keep passing if the mention were written any other way. It now checks that every `@${owner}/` sits inside Markdown link text — verified by injecting a bare mention and watching it fail --- .../workflows/breaking-change-approval.yml | 2 ++ .../README.md | 21 ++++++++++++------- .../action.yml | 8 +++++-- .../breaking-change-guard/test-workflow.py | 13 +++++++++++- src/validate/environment-protection/README.md | 1 + .../environment-protection/action.yml | 17 +++++++++++++++ 6 files changed, 52 insertions(+), 10 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 215c0a51..d26bb4c3 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -369,6 +369,7 @@ jobs: ref: ${{ needs.detect.outputs.base-ref }} - name: Verify the breaking-approval Environment is protected + id: environment if: >- needs.detect.result == 'success' && needs.detect.outputs.has-breaking-changes == 'true' @@ -393,6 +394,7 @@ jobs: pr-number: ${{ needs.detect.outputs.pr-number }} state: waiting reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} + actual-reviewers: ${{ steps.environment.outputs.reviewers }} reason: ${{ needs.detect.outputs.reason }} head-sha: ${{ needs.detect.outputs.head-sha }} acknowledged: ${{ needs.detect.outputs.acknowledged }} diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index ff4d28f7..2092f48e 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -35,13 +35,14 @@ links straight to the decision, and names who can make it: > ### Breaking change — needs a decision > -> [`f8d5506`](#) carries a breaking change, so merging makes the next release a -> **major** version. That is often deliberate — this just asks someone to -> confirm it. +> [`f8d5506`](https://github.com/LerianStudio/example/commit/f8d5506) carries a +> breaking change, so merging makes the next release a **major** version. That is +> often deliberate — this just asks someone to confirm it. > -> ### ➡️ [Approve or reject](#) +> ### ➡️ [Approve or reject](https://github.com/LerianStudio/example/actions/runs/1) > -> Who can: [@LerianStudio/g_github_devops](#) +> Who can: +> [@LerianStudio/g_github_devops](https://github.com/orgs/LerianStudio/teams/g_github_devops) Everything else — the scope rule, the acknowledgement state, how to undo the break — is one `` line at the bottom, present for whoever needs it and out @@ -51,6 +52,11 @@ 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. +**`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 +rather than guessed. + ## What it does not do It never closes, merges or approves anything. `close-on-rejection` only changes @@ -70,9 +76,10 @@ side effect in the caller is what makes this safe to run with | `head-sha` | Head commit the report refers to. | No | `''` | | `acknowledged` | Whether the author acknowledgement line is present. | No | `false` | | `run-url` | Link to the run carrying the pending deployment. | No | `''` | -| `reviewers` | Who can answer, as the `team:` / `user:` list the Environment was given. | No | `''` | +| `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 }}` | -| `docs-url` | Link to the explanation of how the gate works. | No | the gate's docs page | +| `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 | `''` | | `rejection-comment` | Comment left with the rejection. | No | `''` | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index e5f5b79f..42a6bf76 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -32,7 +32,11 @@ inputs: required: false default: '' reviewers: - description: 'Who can answer, as the `team:` / `user:` list the Environment was given. Rendered as GitHub mentions so the reader knows who to ask instead of hunting through settings.' + description: 'Who can answer, as a `team:` / `user:` list. Rendered as links, never as @mentions — the pending deployment already notifies these people, so a mention would ping them twice for the same thing.' + required: false + default: '' + actual-reviewers: + description: 'Reviewers read from the Environment itself, in the same format. Preferred over `reviewers` when present: 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.' required: false default: '' server-url: @@ -88,7 +92,7 @@ runs: HEAD_SHA: ${{ inputs.head-sha }} ACKNOWLEDGED: ${{ inputs.acknowledged }} RUN_URL: ${{ inputs.run-url }} - REVIEWERS: ${{ inputs.reviewers }} + REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-reviewers || inputs.reviewers }} SERVER_URL: ${{ inputs.server-url }} DOCS_URL: ${{ inputs.docs-url }} APPROVERS: ${{ inputs.approvers }} diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 058c3a07..6239f0fa 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -837,7 +837,18 @@ def test_the_waiting_comment_links_instead_of_mentioning(self): self.assertIn(fragment, reporter) # The pending deployment already notifies the reviewers; a bare @mention # would ping the whole team again for the same thing. - self.assertNotIn('}@${owner}/', reporter) + # + # Checked as "every occurrence sits inside Markdown link text", not as + # one forbidden spelling: an assertion pinned to the exact characters + # around `@` passes the day somebody writes the mention differently, + # which is worse than no assertion at all. + for match in re.finditer(r"@\$\{owner\}/", reporter): + preceding = reporter[: match.start()] + self.assertTrue( + preceding.endswith("["), + f"bare team mention at offset {match.start()}: " + f"...{reporter[max(0, match.start() - 40):match.end() + 20]}...", + ) 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. diff --git a/src/validate/environment-protection/README.md b/src/validate/environment-protection/README.md index a34557ab..ac97bcb6 100644 --- a/src/validate/environment-protection/README.md +++ b/src/validate/environment-protection/README.md @@ -90,6 +90,7 @@ Environments API takes numeric ids only. |--------|-------------| | `protected` | `true` when the rule is present. The step fails before it could report `false`. | | `created` | `true` when this run created the Environment. | +| `reviewers` | Who the Environment is actually gated by, as `team:`/`user:`. Read back from the Environment, so it reflects one that already existed with different reviewers than were asked for. | ## Usage diff --git a/src/validate/environment-protection/action.yml b/src/validate/environment-protection/action.yml index 47dd32c8..8f2fdde2 100644 --- a/src/validate/environment-protection/action.yml +++ b/src/validate/environment-protection/action.yml @@ -36,6 +36,9 @@ outputs: created: description: "Whether this run created the Environment (true/false)." value: ${{ steps.ensure.outputs.created }} + reviewers: + description: "Reviewers the Environment actually carries, as `team:`/`user:`. Read back from the Environment, so it reflects an existing one that is gated by different people than the caller asked for." + value: ${{ steps.ensure.outputs.reviewers }} runs: using: composite @@ -62,6 +65,19 @@ runs: --jq '[.protection_rules[].type] | join(",")' 2>/dev/null } + # Who the Environment is actually gated by, which is not necessarily who + # the caller asked for: it may have existed already, or been edited since. + # Callers that name the reviewers to a human should prefer this. + read_reviewers() { + gh api "repos/${REPO}/environments/${ENVIRONMENT}" \ + --jq '[.protection_rules[] + | select(.type == "required_reviewers") + | .reviewers[]? + | if .type == "Team" then "team:" + .reviewer.slug + else "user:" + .reviewer.login end] + | join(",")' 2>/dev/null || true + } + # Resolves `team:` / `user:` / `` into the # {type,id} pairs the Environments API expects. It takes numeric ids # only, so every entry costs a lookup. @@ -171,6 +187,7 @@ runs: { echo "protected=true" echo "created=$created" + echo "reviewers=$(read_reviewers)" } >> "$GITHUB_OUTPUT" if [ "$created" != "true" ]; then From 84a9db4f439d01e486a365fa0b361d51d65318b7 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 18:31:55 -0300 Subject: [PATCH 03/47] fix(pr-validation): read the Environment once, so reviewers cannot be unknown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the second CodeRabbit review on #907. The composite read the Environment twice: once for the protection rules, once for the reviewers, with the second call swallowing failure. That allowed a state of "protected, reviewers unknown" — and an empty reviewer list is indistinguishable from "this Environment has none", so the reporter fell back to the configured list and could name people who cannot actually approve. Which is the very thing the previous commit set out to fix. The suggestion was to carry the read failure as a distinct status. Removing the state is better than reporting it: one GET now answers both questions, so either both are known or the step fails. It also drops a call to an API this job already talks to twice. Test pins it to exactly one read. --- .../breaking-change-guard/test-workflow.py | 12 ++++ src/validate/environment-protection/README.md | 9 +++ .../environment-protection/action.yml | 59 +++++++++++-------- 3 files changed, 57 insertions(+), 23 deletions(-) diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 6239f0fa..941ec476 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -850,6 +850,18 @@ def test_the_waiting_comment_links_instead_of_mentioning(self): f"...{reporter[max(0, match.start() - 40):match.end() + 20]}...", ) + 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", + # so a caller would fall back to whoever it had configured and name the + # wrong people. One read, both answers, no such state. + reads = ENVIRONMENT_PROTECTION.count( + 'gh api "repos/${REPO}/environments/${ENVIRONMENT}"' + ) + self.assertEqual(1, reads, "the Environment must be read exactly once") + self.assertIn("reviewers_of()", ENVIRONMENT_PROTECTION) + self.assertIn("rules_of()", ENVIRONMENT_PROTECTION) + 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) diff --git a/src/validate/environment-protection/README.md b/src/validate/environment-protection/README.md index ac97bcb6..b2a0a1af 100644 --- a/src/validate/environment-protection/README.md +++ b/src/validate/environment-protection/README.md @@ -46,6 +46,15 @@ drop a wait timer or a deployment branch policy somebody set on purpose, so an existing Environment is reported and left untouched. Creating from nothing is a different act from overwriting somebody's configuration. +### One read, both answers + +The rules and the reviewers come from a single `GET`. Two calls would allow a +state of "protected, reviewers unknown" — and an empty reviewer list is +indistinguishable from *this Environment has none*, so a caller naming the +approvers to a human would quietly fall back to whoever it had configured and +name the wrong people. With one read there is no such state: either both +answers are known, or the step fails. + ### A 404 and a 403 are not the same Only "does not exist" is this composite's to fix. If the Environment merely diff --git a/src/validate/environment-protection/action.yml b/src/validate/environment-protection/action.yml index 8f2fdde2..cfdbe555 100644 --- a/src/validate/environment-protection/action.yml +++ b/src/validate/environment-protection/action.yml @@ -60,22 +60,34 @@ runs: created=false owner=${REPO%%/*} - read_rules() { - gh api "repos/${REPO}/environments/${ENVIRONMENT}" \ - --jq '[.protection_rules[].type] | join(",")' 2>/dev/null + error_file=$(mktemp) + trap 'rm -f "$error_file"' EXIT INT TERM + environment_json='' + + # One read, both answers. Fetching the rules and the reviewers in + # separate calls would create a state where the first succeeded and the + # second did not — "protected, reviewers unknown" — and an empty + # reviewer list is indistinguishable from "this Environment has none". + # A caller would then fall back to whoever it had configured and name + # the wrong people. There is no such state with a single read. + read_environment() { + environment_json=$(gh api "repos/${REPO}/environments/${ENVIRONMENT}" 2>"$error_file") + } + + rules_of() { + jq -r '[.protection_rules[].type] | join(",")' <<< "$environment_json" } # Who the Environment is actually gated by, which is not necessarily who # the caller asked for: it may have existed already, or been edited since. # Callers that name the reviewers to a human should prefer this. - read_reviewers() { - gh api "repos/${REPO}/environments/${ENVIRONMENT}" \ - --jq '[.protection_rules[] - | select(.type == "required_reviewers") - | .reviewers[]? - | if .type == "Team" then "team:" + .reviewer.slug - else "user:" + .reviewer.login end] - | join(",")' 2>/dev/null || true + reviewers_of() { + jq -r '[.protection_rules[] + | select(.type == "required_reviewers") + | .reviewers[]? + | if .type == "Team" then "team:" + .reviewer.slug + else "user:" + .reviewer.login end] + | join(",")' <<< "$environment_json" } # Resolves `team:` / `user:` / `` into the @@ -111,19 +123,18 @@ runs: printf '%s' "$resolved" } - if ! rules=$(read_rules); then + if ! read_environment; then # Distinguish "does not exist" from "cannot read". Only the first is # ours to fix; the second is a token problem, and creating on top of # it would be guessing. - if ! error=$(gh api "repos/${REPO}/environments/${ENVIRONMENT}" 2>&1 >/dev/null); then - case "$error" in - *"HTTP 404"* | *"Not Found"* ) ;; - * ) - echo "Could not read the '${ENVIRONMENT}' Environment in ${REPO}: ${error}" >&2 - exit 1 - ;; - esac - fi + error=$(cat "$error_file") + case "$error" in + *"HTTP 404"* | *"Not Found"* ) ;; + * ) + echo "Could not read the '${ENVIRONMENT}' Environment in ${REPO}: ${error}" >&2 + exit 1 + ;; + esac if [ "$CREATE_IF_MISSING" != "true" ]; then echo "The '${ENVIRONMENT}' Environment does not exist in ${REPO}, and create-if-missing is off." >&2 @@ -167,12 +178,14 @@ runs: created=true echo "::notice::Created the '${ENVIRONMENT}' Environment in ${REPO} with reviewers: ${REVIEWERS}" - if ! rules=$(read_rules); then + if ! read_environment; then echo "Created the '${ENVIRONMENT}' Environment but could not read it back." >&2 exit 1 fi fi + rules=$(rules_of) + # Reached whether the Environment was just created or already existed. # An existing one that is unprotected is NOT repaired here: a PUT # replaces the whole configuration, so it would silently drop a wait @@ -187,7 +200,7 @@ runs: { echo "protected=true" echo "created=$created" - echo "reviewers=$(read_reviewers)" + echo "reviewers=$(reviewers_of)" } >> "$GITHUB_OUTPUT" if [ "$created" != "true" ]; then 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 04/47] 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) From ce7decfe6e077b39988d3f8bf3c67d195dfb1225 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 18:57:06 -0300 Subject: [PATCH 05/47] feat(pr-validation): mark the breaking change so it reads at a glance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The alert block said what happened in prose, which meant reading it to know. A 💥 in an `##` heading inside the block makes the state legible from the timeline, before anyone opens anything — and the table gains marks on the three rows that matter, so it can be scanned instead of read. Each state now gets the alert that fits it: IMPORTANT while a decision is pending, NOTE once approved, CAUTION on a rejection that closed the pull request, WARNING when the decision was never recorded. Colour and icon come from GitHub; the emoji says what the block is about. --- .../action.yml | 35 ++++++++++++------- .../breaking-change-guard/test-workflow.py | 18 ++++++++++ 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 9768c32d..1b126aeb 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -173,9 +173,9 @@ runs: case "$STATE" in waiting) printf '> [!IMPORTANT]\n' - printf '> **Breaking change detected — this pull request needs a decision**\n' + printf '> ## 💥 Breaking change detected\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' + printf '> **This pull request needs a decision.** Merging it makes the next release a **major** version — often deliberate, so 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 @@ -190,28 +190,37 @@ runs: 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' \ + [ -n "$commit_link" ] && printf '| 💥 **Breaking commit** | %s |\n' "$commit_link" + [ -n "$mentions" ] && printf '| 👤 **Who can decide** | %s |\n' "$mentions" + printf '| %s **Author acknowledgement** | %s |\n\n' \ + "$([ "$ACKNOWLEDGED" = "true" ] && echo "✅" || echo "⬜")" \ "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" ;; approved) - printf '### ✅ Breaking change approved\n\n' - printf 'Approved by **%s**. The next release will be a major version.\n\n' \ + printf '> [!NOTE]\n' + printf '> ## ✅ Breaking change approved\n' + printf '>\n' + printf '> Approved by **%s**. 💥 The next release will be a major version.\n\n' \ "${APPROVERS:-a reviewer}" ;; rejected) - printf '### Breaking change rejected\n\n' - printf 'Rejected by **%s**' "${REJECTERS:-a reviewer}" + printf '> [!CAUTION]\n' + printf '> ## ⛔ Breaking change rejected\n' + printf '>\n' + printf '> Rejected by **%s**' "${REJECTERS:-a reviewer}" [ -n "$REJECTION_COMMENT" ] && printf ' — *%s*' "$REJECTION_COMMENT" - printf '.\n\n' + printf '.\n' if [ "$CLOSE_ON_REJECTION" = "true" ]; then - printf 'This pull request was closed. The branch is untouched — rework it without the break and reopen.\n\n' + printf '>\n' + printf '> This pull request was closed. The branch is untouched — rework it without the break and reopen.\n' fi + printf '\n' ;; unresolved) - printf '### Breaking change — decision not recorded\n\n' - printf 'The approval did not complete.' + printf '> [!WARNING]\n' + printf '> ## 💥 Breaking change — decision not recorded\n' + printf '>\n' + printf '> The approval did not complete.' [ -n "$RUN_URL" ] && printf ' [Open the run](%s) to answer it.' "$RUN_URL" printf '\n\n' ;; diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 72cee214..30b081e7 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -904,6 +904,24 @@ def test_the_button_degrades_when_the_badge_cannot_load(self): # And an empty badge-url falls back to a plain heading link. self.assertIn('elif [ -n "$RUN_URL" ]; then', reporter) + def test_each_state_carries_its_own_alert_and_mark(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + # GitHub alerts colour the block and supply the icon; the emoji marks + # what the block is about, so the state is readable from the timeline + # without opening anything. + for alert, mark in ( + ("[!IMPORTANT]", "## 💥 Breaking change detected"), + ("[!NOTE]", "## ✅ Breaking change approved"), + ("[!CAUTION]", "## ⛔ Breaking change rejected"), + ("[!WARNING]", "## 💥 Breaking change — decision not recorded"), + ): + with self.subTest(alert=alert): + self.assertIn(alert, reporter) + self.assertIn(mark, reporter) + def test_the_undo_example_uses_the_real_commit_subjects(self): reporter = ( REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" From 1c162b2006a83e1035fcc8a398e0cec362847dd2 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:04:29 -0300 Subject: [PATCH 06/47] test(pr-validation): render the comment instead of grepping the script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #909. The state assertions searched the whole reporter script, so every alert and heading was present somewhere no matter which branch used it. Swapping two states' alerts would have kept the suite green while every comment said the wrong thing. The suggested fix pulled each `case` branch out with a regex anchored to the exact indentation. That trades one fragility for another — any reindentation breaks the pattern and the test reports a problem that is not there — and it still reads the source rather than the output. So the tests now run the composite and read the Markdown it produced. `render_reporter(state)` executes the body with `gh` stubbed, and each state is checked for its own alert and mark *and* for the absence of the other three; exclusivity is what catches a swap. Verified by swapping IMPORTANT for NOTE on `waiting` and watching it fail. The link assertions moved the same way: they matched shell fragments like `${SERVER_URL}/${REPO}/commit/...` and now match rendered URLs, so the no-bare-mention rule holds however the composite spells it. --- .../breaking-change-guard/test-workflow.py | 142 +++++++++++++----- 1 file changed, 105 insertions(+), 37 deletions(-) diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 30b081e7..6172af4f 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -8,6 +8,8 @@ import unittest from pathlib import Path +import yaml + REPO_ROOT = Path(__file__).resolve().parents[3] WORKFLOW_PATH = REPO_ROOT / ".github" / "workflows" / "pr-validation.yml" @@ -213,6 +215,71 @@ def execute_body(body, env): return result, parse_github_output(output) +def render_reporter(state, **overrides): + """Render the reporter's comment for one state and return the Markdown. + + Asserting against the composite's source only proves a string exists + somewhere in it — swapping two states' alerts would keep every string + present and every assertion green. Running the thing and reading what it + produced is the only check that ties a state to its own output. + """ + action = yaml.safe_load( + ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + ) + # Everything up to the posting is the part that builds the Markdown. + body = action["runs"]["steps"][0]["run"].split('comment_id=""')[0] + + env = { + "REPO": "LerianStudio/example", + "PR_NUMBER": "42", + "STATE": state, + "REASON": "rule develop <- '*'", + "HEAD_SHA": "abc1234567", + "ACKNOWLEDGED": "false", + "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", + "REVIEWERS": "team:devops", + "SERVER_URL": "https://github.com", + "BADGE_URL": "https://img.shields.io/badge/x-1f6feb", + "DOCS_URL": "https://example.invalid/docs", + "APPROVERS": "carol", + "REJECTERS": "dave", + "REJECTION_COMMENT": "", + "CLOSE_ON_REJECTION": "true", + "LABEL": "breaking-change", + "ENABLED": "true", + "GH_TOKEN": "stub", + } + env.update(overrides) + + with tempfile.TemporaryDirectory() as stub_dir: + stub = Path(stub_dir) / "gh" + stub.write_text( + "#!/usr/bin/env bash\n" + "case \"$*\" in\n" + " *\"/commits\"*) printf '%s\\n'" + " 'feat(scope)!: a breaking subject' 'chore: an ordinary one' ;;\n" + " *) exit 1 ;;\n" + "esac\n", + encoding="utf-8", + ) + stub.chmod(0o755) + env["PATH"] = f"{stub_dir}:{os.environ.get('PATH', '/usr/bin:/bin')}" + result = subprocess.run( + ["bash", "-c", body + '\ncat "$body_file"'], + cwd=stub_dir, + env=env, + text=True, + capture_output=True, + check=False, + ) + if result.returncode != 0: + raise AssertionError(f"reporter failed for state {state!r}: {result.stderr}") + return result.stdout + + def parse_github_output(output): values = {} for line in output.splitlines(): @@ -822,32 +889,24 @@ def test_reviewers_come_from_codeowners_on_the_base_ref(self): ) def test_the_waiting_comment_links_instead_of_mentioning(self): - reporter = ( - REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" - / "action.yml" - ).read_text(encoding="utf-8") + rendered = render_reporter("waiting") # Links to the commit, the decision, the reviewers and the docs. for fragment in ( - "${SERVER_URL}/${REPO}/commit/${HEAD_SHA}", - "[![Review the pending deployment](%s)](%s)", - "${SERVER_URL}/orgs/${owner}/teams/${slug}", - "[How this gate works](${DOCS_URL})", + "](https://github.com/LerianStudio/example/commit/abc1234567)", + "[![Review the pending deployment](https://img.shields.io/badge/", + "](https://github.com/orgs/LerianStudio/teams/devops)", + "[How this gate works](https://example.invalid/docs)", ): with self.subTest(fragment=fragment): - self.assertIn(fragment, reporter) + self.assertIn(fragment, rendered) + # The pending deployment already notifies the reviewers; a bare @mention - # would ping the whole team again for the same thing. - # - # Checked as "every occurrence sits inside Markdown link text", not as - # one forbidden spelling: an assertion pinned to the exact characters - # around `@` passes the day somebody writes the mention differently, - # which is worse than no assertion at all. - for match in re.finditer(r"@\$\{owner\}/", reporter): - preceding = reporter[: match.start()] + # would ping the whole team again for the same thing. Checked on the + # rendered comment, so it holds however the composite spells it. + for match in re.finditer(r"@LerianStudio/", rendered): self.assertTrue( - preceding.endswith("["), - f"bare team mention at offset {match.start()}: " - f"...{reporter[max(0, match.start() - 40):match.end() + 20]}...", + rendered[: match.start()].endswith("["), + f"bare team mention: ...{rendered[max(0, match.start() - 40):match.end() + 30]}...", ) def test_only_one_approval_request_stays_open_per_pull_request(self): @@ -904,23 +963,32 @@ def test_the_button_degrades_when_the_badge_cannot_load(self): # And an empty badge-url falls back to a plain heading link. self.assertIn('elif [ -n "$RUN_URL" ]; then', reporter) - def test_each_state_carries_its_own_alert_and_mark(self): - reporter = ( - REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" - / "action.yml" - ).read_text(encoding="utf-8") - # GitHub alerts colour the block and supply the icon; the emoji marks - # what the block is about, so the state is readable from the timeline - # without opening anything. - for alert, mark in ( - ("[!IMPORTANT]", "## 💥 Breaking change detected"), - ("[!NOTE]", "## ✅ Breaking change approved"), - ("[!CAUTION]", "## ⛔ Breaking change rejected"), - ("[!WARNING]", "## 💥 Breaking change — decision not recorded"), - ): - with self.subTest(alert=alert): - self.assertIn(alert, reporter) - self.assertIn(mark, reporter) + # GitHub alerts colour the block and supply the icon; the emoji marks what + # the block is about, so the state is readable from the timeline without + # opening anything. + STATE_MARKS = ( + ("waiting", "[!IMPORTANT]", "## 💥 Breaking change detected"), + ("approved", "[!NOTE]", "## ✅ Breaking change approved"), + ("rejected", "[!CAUTION]", "## ⛔ Breaking change rejected"), + ("unresolved", "[!WARNING]", "## 💥 Breaking change — decision not recorded"), + ) + + def test_each_state_renders_its_own_alert_and_mark(self): + # Rendered, not grepped. Asserting against the source only proves a + # string exists somewhere in it, so swapping two states' alerts would + # keep every assertion green while every comment said the wrong thing. + for state, alert, mark in self.STATE_MARKS: + with self.subTest(state=state): + rendered = render_reporter(state) + self.assertIn(alert, rendered) + self.assertIn(mark, rendered) + + # Exclusivity is what actually catches a swap. + for other_state, other_alert, other_mark in self.STATE_MARKS: + if other_state == state: + continue + self.assertNotIn(other_alert, rendered) + self.assertNotIn(other_mark, rendered) def test_the_undo_example_uses_the_real_commit_subjects(self): reporter = ( From 72ee2dd2b6ba8baec5b09b63318c7514e8450389 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:09:23 -0300 Subject: [PATCH 07/47] test(pr-validation): assert the alert block, not two loose strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the third CodeRabbit review on #909. The state test read the rendered comment but checked `[!IMPORTANT]` and `## 💥 …` as separate strings. A GitHub alert only renders inside a blockquote, so dropping the `>` would leave both assertions green while the comment rendered as plain text — getting the content right and the presentation wrong, which is the thing this PR is about. Now asserted as adjacent quoted lines. Verified by removing the `>` from `waiting` and watching it fail. --- src/validate/breaking-change-guard/test-workflow.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 6172af4f..80c63546 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -980,8 +980,11 @@ def test_each_state_renders_its_own_alert_and_mark(self): for state, alert, mark in self.STATE_MARKS: with self.subTest(state=state): rendered = render_reporter(state) - self.assertIn(alert, rendered) - self.assertIn(mark, rendered) + # As adjacent blockquote lines, not as two strings that happen + # to be somewhere: a GitHub alert only renders inside `>`, so + # dropping the quote would leave both assertions green and the + # comment looking like plain text. + self.assertIn(f"> {alert}\n> {mark}", rendered) # Exclusivity is what actually catches a swap. for other_state, other_alert, other_mark in self.STATE_MARKS: From 4addf976d09b7d888541cce83023a3df2e819c90 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 21:49:24 -0300 Subject: [PATCH 08/47] fix(pr-validation): run the advisory checks even when a blocking one fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR Size, Auto Labels and PR Metadata were gated on `needs.blocking-checks.result == 'success'`, so a wrong title or a missing signature took them down too — the summary reported three `skipped` rows next to the failure. That came from the 2-tier fail-fast model and saved a runner at the expense of the pull request that most needed the help. None of the three can block a merge, all three are cheap, and a pull request already in trouble is exactly where a size label and an assignee are worth having. `needs:` stays for ordering; the verdict is no longer consulted. Drafts and non-pull_request events still skip, as before. Safe because none of the three can fail the job — each is continue-on-error — and the summary and report jobs already run with always(), so an advisory checkout failure still gets reported. Tests pin all three facts. --- .github/workflows/pr-validation.yml | 11 ++++++- docs/pr-validation.md | 13 ++++++++ .../breaking-change-guard/test-workflow.py | 33 +++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index b0d45a69..6d416f17 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -402,11 +402,20 @@ jobs: commit-signatures-outcome: ${{ steps.commit-signatures.outcome || 'skipped' }} # ----------------- Tier 2: Advisory Checks (shared checkout, depends on Tier 1) ----------------- + # Advisory by definition, so a blocking failure must not take them with it. + # They used to be gated on `needs.blocking-checks.result == 'success'`, from + # the 2-tier fail-fast model, which saved a runner at the cost of the exact + # pull request that needed the help: a wrong title or a missing signature + # meant no size label, no auto-labels and no assignee either. Those three are + # cheap, none of them can block, and they are most useful on a pull request + # that is already in trouble. + # + # `needs:` stays for ordering only. The verdict is no longer consulted. advisory-checks: name: Advisory Checks runs-on: ${{ vars.GENERAL_RUNNERS || inputs.runner_type }} needs: [blocking-checks] - if: always() && github.event_name == 'pull_request' && needs.blocking-checks.result == 'success' && github.event.pull_request.draft != true + if: always() && github.event_name == 'pull_request' && github.event.pull_request.draft != true outputs: metadata-result: ${{ steps.collect.outputs.metadata }} size-result: ${{ steps.collect.outputs.size }} diff --git a/docs/pr-validation.md b/docs/pr-validation.md index 99703145..2abd19cc 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -225,6 +225,19 @@ For every non-draft live run, including detector failure or cancellation, the wo feat fix docs style refactor perf test chore ci build revert ``` +### Advisory checks always run + +PR Size, Auto Labels and PR Metadata are advisory — none of them can block a +merge. They are therefore **not** gated on the blocking tier passing. + +They used to be. The 2-tier fail-fast model skipped them whenever a blocking +check failed, which saved a runner at the cost of the pull request that most +needed the help: a wrong title or a missing signature also meant no size label, +no auto-labels and no assignee. A pull request already in trouble is exactly +where that information is worth having. + +They still skip on drafts, and on anything that is not a `pull_request` event. + ## Breaking change approval The author-acknowledgement guard above asks the author to declare the break. The diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 80c63546..c7d00d14 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -778,6 +778,39 @@ def test_analysis_workflows_expose_scoped_test_runner_overrides(self): ) +class AdvisoryChecksTests(unittest.TestCase): + def test_advisory_checks_do_not_cascade_off_a_blocking_failure(self): + job = extract_job(WORKFLOW, "advisory-checks") + condition = next( + line for line in job.splitlines() if line.strip().startswith("if:") + ) + # Advisory by definition: a wrong title or a missing signature must not + # also cost the pull request its size label, its auto-labels and its + # assignee. Those are most useful precisely on a pull request that is + # already in trouble. + self.assertNotIn("needs.blocking-checks.result", condition) + self.assertIn("always()", condition) + self.assertIn("github.event.pull_request.draft != true", condition) + + def test_advisory_checks_cannot_become_a_new_blocker(self): + job = extract_job(WORKFLOW, "advisory-checks") + # Running them unconditionally is only safe while none of them can fail + # the job — otherwise this change would turn three advisory checks into + # a blocking one. + for step_name in ("Check PR metadata", "Check PR size", "Auto-label PR"): + with self.subTest(step=step_name): + self.assertIn("continue-on-error: true", extract_step(job, step_name)) + + def test_the_reports_survive_an_advisory_failure(self): + # The checkout in advisory-checks has no continue-on-error, so the job + # can still fail. Everything that reports must run anyway. + for job_id in ("pr-checks-summary", "pr-validation-report"): + with self.subTest(job=job_id): + job = extract_job(WORKFLOW, job_id) + self.assertIn("advisory-checks", job) + self.assertIn("always()", job) + + class ApprovalGateStructureTests(unittest.TestCase): """The human approval gate — wiring that must not drift. From 28c54ad9eefc7c1f910a8a052b8555af45f711e9 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 21:58:32 -0300 Subject: [PATCH 09/47] docs(pr-validation): correct the advisory conditions left saying otherwise MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #911. The new section described advisory checks running regardless of the blocking result while two other places in the same file still described the old behaviour — the Jobs table and the Tier 2 bullet. Contradictory documentation is worse than none: whoever reads the table believes it. A third one was not flagged and was wrong too: the Tier 1 bullet said a blocking failure prevents advisory checks from running. Also fixes the test that was supposed to make this change safe. It asserted over the whole job text, so a step — or the comment above the assertion, which contains the word always() — satisfied it while the job-level needs and if said something else. It now reads the parsed job. Verified by dropping advisory-checks from a needs list and watching it fail. --- docs/pr-validation.md | 8 +++++--- src/validate/breaking-change-guard/test-workflow.py | 10 +++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/docs/pr-validation.md b/docs/pr-validation.md index 2abd19cc..f01a9d6f 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -284,7 +284,7 @@ Full detail: [`breaking-change-approval.md`](breaking-change-approval.md). |-----|------|------------|-----------| | `breaking-change-guard` | mandatory detection | `breaking-change-guard@v1` | always, including drafts | | `blocking-checks` | 1 (fail-fast) | guard enforcement, `pr-source-branch`, `pr-title`, `pr-description`, `pr-commit-signatures` | always; drafts run guard enforcement only | -| `advisory-checks` | 2 (informational) | `pr-metadata`, `pr-size`, `pr-labels` | non-draft, blocking-checks passed | +| `advisory-checks` | 2 (informational) | `pr-metadata`, `pr-size`, `pr-labels` | non-draft, regardless of the blocking result | | `pr-checks-summary` | — | `pr-checks-summary` | always (writes to step summary) | | `breaking-change-comment` | — | `actions/github-script` | every non-draft live run, including detector failure or cancellation | | `pr-validation-report` | — | `pr-validation-reporter` | non-draft (upserts single PR comment) | @@ -295,7 +295,7 @@ Full detail: [`breaking-change-approval.md`](breaking-change-approval.md). - Always enforce both the guard job state and its normalized output, including on drafts - Skip existing source branch, title, description, commit signature, and collector steps on drafts; their non-draft behavior is unchanged - On non-drafts, every validation runs even if one fails (`continue-on-error` per step) -- Job fails if the guard job, normalized guard output, or **any** existing blocking check fails, preventing advisory checks from running +- Job fails if the guard job, normalized guard output, or **any** existing blocking check fails. Advisory checks run either way - In dry-run mode, guard state is logged but does not fail `Blocking Checks` ### Fail-closed detection @@ -338,7 +338,9 @@ The step runs with read-only permissions and emits no commit metadata beyond wha ### Advisory checks (Tier 2) - Share a single `checkout` with `fetch-depth: 0` -- Only run if all blocking checks passed +- Run on every non-draft pull request, **regardless of the blocking result** — a + pull request that failed a blocking check still gets its size label, its + auto-labels and its assignee - Never block merge — informational only ## Dry Run Behavior diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index c7d00d14..7cc94227 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -804,11 +804,15 @@ def test_advisory_checks_cannot_become_a_new_blocker(self): def test_the_reports_survive_an_advisory_failure(self): # The checkout in advisory-checks has no continue-on-error, so the job # can still fail. Everything that reports must run anyway. + # + # Read from the parsed job, not from its text: `assertIn` over the whole + # job body is satisfied by a step, or even by this comment, while the + # job-level `needs` and `if` say something else entirely. + jobs = yaml.safe_load(WORKFLOW)["jobs"] for job_id in ("pr-checks-summary", "pr-validation-report"): with self.subTest(job=job_id): - job = extract_job(WORKFLOW, job_id) - self.assertIn("advisory-checks", job) - self.assertIn("always()", job) + self.assertIn("advisory-checks", jobs[job_id]["needs"]) + self.assertIn("always()", jobs[job_id]["if"]) class ApprovalGateStructureTests(unittest.TestCase): From 1b4ea5ff2080af1d3506a673dfcb83acaac0c06a Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 22:15:12 -0300 Subject: [PATCH 10/47] fix(pr-validation): treat a cancelled approval as superseded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught on #906. Editing the pull request description fires `edited`, which queues a new run on the very same commit. The concurrency group cancelled the previous pending approval, correctly — but the superseded guard only compared head SHAs, and the head had not moved. So the dead run fell through to "decision not recorded", overwrote the live revision's comment and failed its check, while the new run sat waiting with the real request. A cancelled approval now counts as superseded on its own, and is checked first because it is the broader signal: nothing cancels it except the concurrency group replacing the run or a human doing it by hand, and neither is a decision worth reporting. The head comparison stays for whatever slips past that. Test pins both the new branch and its order ahead of the comparison. --- .../workflows/breaking-change-approval.yml | 25 +++++++++++++++---- docs/breaking-change-approval.md | 12 +++++++-- .../breaking-change-guard/test-workflow.py | 13 ++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 708d714e..769e3c5a 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -509,11 +509,26 @@ 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. + # A superseded run must not touch the pull request. When something + # cancels the pending approval, this job still runs — and without this + # guard it would overwrite the live revision's comment with "the + # approval did not complete", which is true of the dead run and + # misleading about the live one. + # + # A cancelled approval is the first and broader signal. Nothing cancels + # it except the concurrency group replacing this run, or a human + # cancelling by hand, and neither is a decision to report. It matters + # on its own because the head does not have to change for a run to be + # replaced: editing the description fires `edited`, which queues a new + # run on the very same commit — and the comparison below would see + # nothing wrong. + if [ "$APPROVAL_RESULT" = "cancelled" ]; then + state=superseded + emit + echo "::notice::This run's approval was cancelled — another run is carrying the decision; leaving PR #${PR_NUMBER} alone" + exit 0 + fi + 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 diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index d144c8e9..963dc71b 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -266,8 +266,16 @@ 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. +misleading about the live one. + +Two signals, and the order matters. A **cancelled approval** is the broad one: +nothing cancels it except the concurrency group replacing the run or a human +doing it by hand, and neither is a decision to report. A **changed head** is the +narrower one, for anything that slipped past the first. + +The cancelled check cannot be dropped in favour of the head comparison, because +a run can be replaced without the head moving at all: editing the description +fires `edited`, which queues a new run on the very same commit. ## Fails closed diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 7cc94227..599df228 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -968,6 +968,19 @@ def test_a_superseded_run_leaves_the_pull_request_alone(self): # overwrite the live revision's comment with "did not complete". self.assertIn("state=superseded", body) self.assertIn('"$live_sha" != "$HEAD_SHA"', body) + # A changed head is not the only way to be superseded: `edited` queues a + # new run on the very same commit, so a cancelled approval has to count + # on its own. Observed on #906 — editing the description replaced the + # run, and the comparison above saw nothing wrong while the comment was + # overwritten with "decision not recorded". + self.assertIn('if [ "$APPROVAL_RESULT" = "cancelled" ]; then', body) + cancelled_at = body.index('"$APPROVAL_RESULT" = "cancelled"') + head_compare_at = body.index('"$live_sha" != "$HEAD_SHA"') + self.assertLess( + cancelled_at, + head_compare_at, + "the cancelled check must come first — it is the broader signal", + ) # And `superseded` must drive neither the comment nor the close. for step_name in ( "Report the outcome on the pull request", From b134e61fba686252c3f1b6db6586ff10f5924e20 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 09:08:55 -0300 Subject: [PATCH 11/47] test(pr-validation): run the verdict instead of reading it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #912. The cancellation test checked the branch and its ordering but not its `exit 0`. Without that exit a cancelled run falls through, and a recorded rejection downstream wins: the verdict comes back `rejected` and the pull request is closed — because a concurrency cancellation raced a reviewer. Closing a pull request by accident is the most destructive thing this workflow does, so "trivial" undersells it. Asserting the characters `exit 0` are present would prove nothing about whether the branch stops, and would be the fifth text assertion in this series. `run_verdict()` executes the step with `gh` stubbed instead, and the test hands it a rejection to fall through onto. Removing the exit makes it fail with 'superseded' != 'rejected' — which is how the report was confirmed in the first place. Also covers the whole state table by running it: approved, rejected, superseded from a cancellation, superseded from a moved head, unresolved, none, blocked. Those cases existed only in a scratch harness before. --- .../breaking-change-guard/test-workflow.py | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 599df228..f6e2ab6b 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -280,6 +280,68 @@ def render_reporter(state, **overrides): return result.stdout +def run_verdict(**overrides): + """Run the gate's verdict step and return its outputs. + + Asserting that the cancellation branch contains `exit 0` would only prove + the characters are there. Running it with a recorded rejection in hand is + what proves the branch actually stops: without the exit, the rejection + downstream wins and the pull request gets closed. + """ + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + step = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Resolve the verdict" + ) + + env = { + "GH_TOKEN": "stub", + "REPO": "LerianStudio/example", + "HEAD_SHA": "abc123", + "DETECT_RESULT": "success", + "REQUEST_RESULT": "success", + "APPROVAL_RESULT": "success", + "IN_SCOPE": "true", + "REASON": "rule develop <- '*'", + "PR_NUMBER": "42", + "HAS_BREAKING_CHANGES": "true", + "DETECTION_SUCCEEDED": "true", + "APPROVERS": "", + "REJECTERS": "", + "API_READABLE": "true", + "DRY_RUN": "false", + "LIVE_SHA": "abc123", + } + env.update(overrides) + + with tempfile.TemporaryDirectory() as temp_dir: + stub = Path(temp_dir) / "gh" + stub.write_text( + "#!/usr/bin/env bash\n" + 'case "$*" in\n' + ' *headRefOid*) printf \'%s\\n\' "$LIVE_SHA" ;;\n' + " *) exit 1 ;;\n" + "esac\n", + encoding="utf-8", + ) + stub.chmod(0o755) + output_path = Path(temp_dir) / "github-output" + output_path.touch() + env["GITHUB_OUTPUT"] = str(output_path) + env["GITHUB_STEP_SUMMARY"] = str(Path(temp_dir) / "summary") + env["PATH"] = f"{temp_dir}:{os.environ.get('PATH', '/usr/bin:/bin')}" + result = subprocess.run( + ["bash", "-c", step["run"]], + cwd=temp_dir, + env=env, + text=True, + capture_output=True, + check=False, + ) + return result, parse_github_output(output_path.read_text(encoding="utf-8")) + + def parse_github_output(output): values = {} for line in output.splitlines(): @@ -981,6 +1043,50 @@ def test_a_superseded_run_leaves_the_pull_request_alone(self): head_compare_at, "the cancelled check must come first — it is the broader signal", ) + + def test_a_cancelled_approval_stops_before_anything_can_act_on_it(self): + # Run it, with a recorded rejection sitting right there. The branch has + # to stop: if it falls through, the rejection downstream wins, the + # verdict comes back `rejected`, and the pull request gets closed — + # because a concurrency cancellation raced a reviewer. + _, outputs = run_verdict(APPROVAL_RESULT="cancelled", REJECTERS="dave") + self.assertEqual("superseded", outputs["state"]) + self.assertNotEqual("rejected", outputs["state"]) + + def test_the_verdict_states_are_reachable_as_documented(self): + # The whole table in one place, run rather than read. + for description, env, expected in ( + ("approved", {"APPROVAL_RESULT": "success"}, "approved"), + ( + "rejected", + {"APPROVAL_RESULT": "failure", "REJECTERS": "dave"}, + "rejected", + ), + ( + "cancelled, same head", + {"APPROVAL_RESULT": "cancelled"}, + "superseded", + ), + ( + "head moved under us", + {"LIVE_SHA": "def456"}, + "superseded", + ), + ( + "failed with no recorded decision", + {"APPROVAL_RESULT": "failure"}, + "unresolved", + ), + ("nothing to approve", {"HAS_BREAKING_CHANGES": "false"}, "none"), + ( + "detection never completed", + {"DETECTION_SUCCEEDED": "false"}, + "blocked", + ), + ): + with self.subTest(case=description): + _, outputs = run_verdict(**env) + self.assertEqual(expected, outputs["state"]) # And `superseded` must drive neither the comment nor the close. for step_name in ( "Report the outcome on the pull request", From 32a82923e2a71514129ac49649f601f5191911aa Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 09:39:50 -0300 Subject: [PATCH 12/47] fix(pr-validation): give the comment job permission to comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The breaking change guard comment has never been posted. Every run since 2026-08-06 took a 403 and reported success: RequestError [HttpError]: Resource not accessible by integration POST /repos/{owner}/{repo}/issues/{n}/comments The job declared `pull-requests: read` with `issues: write`. The step posts through the issue-comments endpoint, but the subject is a pull request, and GitHub authorises those as pull requests — `issues: write` covers real issues only. A job-level block replaces the workflow's rather than merging with it, so declaring `read` there was not least privilege; it was below what the step needs to do anything. The cost was not cosmetic. An author who omits the acknowledgement line sees Blocking Checks fail with no explanation, because the comment that would explain it is the one that never arrives. Two months of silence were possible for two reasons, both addressed: the step is continue-on-error, so the 403 never surfaced. It stays — losing a comment should not fail validation — but a failed post now emits a ::warning:: instead of vanishing a test pinned the permission set to exactly {pull-requests: read, issues: write}. It did not merely miss the bug, it held it in place. Swept the other workflows for the same shape: frontend-pr-analysis and go-pr-coverage-comment both comment on pull requests and both inherit `pull-requests: write` from their workflow level, so they are fine. A test now covers every job that comments, so the next one cannot repeat this. --- .github/workflows/pr-validation.yml | 22 ++++++- docs/pr-validation.md | 2 +- .../breaking-change-guard/test-workflow.py | 62 ++++++++++++++++--- 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index 6d416f17..36f1c6e1 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -490,12 +490,21 @@ jobs: runs-on: ${{ vars.GENERAL_RUNNERS || inputs.runner_type }} needs: [breaking-change-guard] if: always() && github.event_name == 'pull_request' && github.event.pull_request.draft != true && !inputs.dry_run + # `pull-requests: write`, not `issues: write`. The step posts through the + # issue-comments endpoint, but the subject is a pull request — and GitHub + # authorises those as pull requests. `issues: write` covers real issues + # only, so this job spent from 2026-08-06 taking a 403 on every run and + # reporting success, because the step is continue-on-error. + # + # The job-level block narrows the workflow's own `pull-requests: write`, so + # declaring `read` here was not least privilege — it was below what the step + # needs to work at all. permissions: - pull-requests: read - issues: write + pull-requests: write steps: - name: Manage breaking change guard comment + id: comment continue-on-error: true uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: @@ -574,6 +583,15 @@ jobs: const body = `${marker}\n## Breaking change guard\n\n**Status:** ${state}\n\n${detail}\n\n\`${acknowledgement}\``; await upsertOwned(body); + # Cosmetic enough to keep continue-on-error — losing the comment should + # not fail validation — but not so cosmetic that it should fail in + # silence. Swallowing it is what let a 403 run for two months while every + # job reported green. + - name: Surface a failed comment + if: always() && steps.comment.outcome == 'failure' + run: | + echo "::warning::Could not post the breaking change guard comment. The validation verdict is unaffected, but the author gets no explanation of what to add. Check the step log above — a 403 here usually means the job lacks 'pull-requests: write'." + # ----------------- PR Validation Summary Comment ----------------- pr-validation-report: name: PR Validation Report diff --git a/docs/pr-validation.md b/docs/pr-validation.md index f01a9d6f..af9d86cd 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -286,7 +286,7 @@ Full detail: [`breaking-change-approval.md`](breaking-change-approval.md). | `blocking-checks` | 1 (fail-fast) | guard enforcement, `pr-source-branch`, `pr-title`, `pr-description`, `pr-commit-signatures` | always; drafts run guard enforcement only | | `advisory-checks` | 2 (informational) | `pr-metadata`, `pr-size`, `pr-labels` | non-draft, regardless of the blocking result | | `pr-checks-summary` | — | `pr-checks-summary` | always (writes to step summary) | -| `breaking-change-comment` | — | `actions/github-script` | every non-draft live run, including detector failure or cancellation | +| `breaking-change-comment` | — | `actions/github-script` | every non-draft live run, including detector failure or cancellation. Needs `pull-requests: write` — commenting on a PR is authorised as a pull request, not as an issue | | `pr-validation-report` | — | `pr-validation-reporter` | non-draft (upserts single PR comment) | | `notify` | — | `slack-notify.yml` | non-draft, `!dry_run` | diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index f6e2ab6b..05545707 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -686,15 +686,20 @@ def test_draft_pull_requests_still_reach_guard_enforcement(self): def test_comment_job_is_write_minimal_and_does_not_checkout(self): job = extract_job(WORKFLOW, "breaking-change-comment") - permissions = re.search( - r"(?m)^ permissions:\n(?P(?: [^\n]+\n?)+)", job - ) - self.assertIsNotNone(permissions) - self.assertEqual( - {"pull-requests: read", "issues: write"}, - {line.strip() for line in permissions.group("body").splitlines()}, - ) - self.assertNotIn("contents:", permissions.group("body")) + permissions = yaml.safe_load(WORKFLOW)["jobs"]["breaking-change-comment"][ + "permissions" + ] + # `pull-requests: write` is the minimum that works, not a widening. + # This set used to be {pull-requests: read, issues: write}, which looks + # narrower and is simply wrong: commenting on a pull request is + # authorised as a pull request, so the step took a 403 on every run + # from 2026-08-06 and reported success anyway. + # + # The lesson is in the shape of the old assertion, not just its value — + # it pinned a permission set nobody had watched work, and so kept the + # bug in place rather than catching it. + self.assertEqual({"pull-requests": "write"}, permissions) + self.assertNotIn("contents", permissions) self.assertNotIn("actions/checkout", job) self.assertIn("github-token: ${{ github.token }}", job) @@ -840,6 +845,45 @@ def test_analysis_workflows_expose_scoped_test_runner_overrides(self): ) +class CommentPermissionTests(unittest.TestCase): + def test_jobs_that_comment_on_a_pull_request_can_write_to_it(self): + # Commenting on a pull request needs `pull-requests: write`. The + # issue-comments endpoint is the one used, but GitHub authorises the + # subject as a pull request, so `issues: write` is not enough — it + # covers real issues only. The mistake fails as a 403 at runtime, and + # these steps are continue-on-error, so it reports success. + workflow = yaml.safe_load(WORKFLOW) + top_level = workflow.get("permissions") or {} + + for job_id, job in workflow["jobs"].items(): + if not isinstance(job, dict): + continue + body = yaml.safe_dump(job) + if not any( + marker in body + for marker in ("createComment", "updateComment", "issues/comments") + ): + continue + + with self.subTest(job=job_id): + # A job-level block replaces the workflow's, it does not merge, + # so an explicit block has to carry the permission itself. + effective = job.get("permissions") or top_level + self.assertEqual( + "write", + effective.get("pull-requests"), + f"{job_id} comments on a pull request and would take a 403", + ) + + def test_a_failed_comment_is_visible_rather_than_swallowed(self): + job = extract_job(WORKFLOW, "breaking-change-comment") + # continue-on-error is right for a cosmetic comment, but silence is how + # a 403 ran for two months with every job green. + self.assertIn("continue-on-error: true", job) + self.assertIn("::warning::", job) + self.assertIn("steps.comment.outcome == 'failure'", job) + + class AdvisoryChecksTests(unittest.TestCase): def test_advisory_checks_do_not_cascade_off_a_blocking_failure(self): job = extract_job(WORKFLOW, "advisory-checks") From 8c1e9c2d53acf019a34c3ae53229c3513d912f3d Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 10:04:34 -0300 Subject: [PATCH 13/47] feat(pr-validation): remember which breaking change was approved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit only runs once every check has passed, and that now includes the approval gate. So each round of approve → review → fix → push needed a fresh human approval before the next review could happen — and the reviewer was answering the identical question about an unchanged breaking change. A question asked five times gets a reflex answer by the third. The gate asks about a specific breaking change, so it now remembers which one was approved. New composite breaking-change-commits lists the commits carrying the break and digests their SHAs; the approval comment records that digest, and a later run with the same digest skips the Environment entirely. typo fix, ordinary commit approval stands reworded breaking commit asks again another breaking commit asks again Built from SHAs, so a reword counts: the approval was given to the text that was read, and the subject is what the release parser acts on. The records accumulate in the comment and are carried across rewrites — every run rebuilds the body, so without that the approval would be forgotten by the very next push. The composite also replaces the partial copy of the rule that lived in the reporter, which only matched the `!` form. Its tests cover the same negative cases as the detector: `BREAKING-CHANGE` with a hyphen and prose mentioning the phrase are not breaking. --- .../workflows/breaking-change-approval.yml | 65 ++++++++++ docs/breaking-change-approval.md | 45 ++++++- .../action.yml | 27 +++++ .../breaking-change-commits/README.md | 96 +++++++++++++++ .../breaking-change-commits/action.yml | 42 +++++++ .../breaking-change-commits/resolve.sh | 106 +++++++++++++++++ src/validate/breaking-change-commits/test.sh | 111 ++++++++++++++++++ .../breaking-change-guard/test-workflow.py | 37 ++++++ 8 files changed, 527 insertions(+), 2 deletions(-) create mode 100644 src/validate/breaking-change-commits/README.md create mode 100644 src/validate/breaking-change-commits/action.yml create mode 100755 src/validate/breaking-change-commits/resolve.sh create mode 100755 src/validate/breaking-change-commits/test.sh diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 769e3c5a..b184b6df 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -212,6 +212,8 @@ jobs: author: ${{ steps.scope.outputs.author }} reason: ${{ steps.scope.outputs.reason }} has-breaking-changes: ${{ steps.normalize.outputs.has-breaking-changes }} + fingerprint: ${{ steps.commits.outputs.fingerprint }} + subjects: ${{ steps.commits.outputs.subjects }} acknowledged: ${{ steps.normalize.outputs.acknowledged }} detection-succeeded: ${{ steps.normalize.outputs.detection-succeeded }} @@ -280,6 +282,18 @@ jobs: breaking-change-acknowledgement: ${{ inputs.author_acknowledgement }} acknowledgement-match-mode: exact-visible-line + # Which commits tripped the gate, and a digest of the set. The digest is + # what lets an approval survive a push that did not touch the breaking + # change — see the `request` job. + - name: List the breaking-change commits + id: commits + if: steps.scope.outputs.in-scope == 'true' + continue-on-error: true + uses: LerianStudio/github-actions-shared-workflows/src/validate/breaking-change-commits@develop + with: + github-token: ${{ secrets.MANAGE_TOKEN || github.token }} + pr-number: ${{ steps.scope.outputs.pr-number }} + - name: Normalize detection result id: normalize if: always() @@ -351,7 +365,42 @@ jobs: permissions: contents: read pull-requests: write + outputs: + already-approved: ${{ steps.prior.outputs.already-approved }} steps: + # Has this exact breaking change already been approved on this pull + # request? The approval comment records the fingerprint it approved, so a + # push that leaves the breaking commits alone — a typo fix, a review + # follow-up — does not send the reviewer the same question again. + # + # Asking again for unchanged content is how a gate becomes a formality: + # the second time costs attention, the fifth time gets a reflex click. + - name: Check for a prior approval of the same change + id: prior + if: >- + needs.detect.result == 'success' + && needs.detect.outputs.has-breaking-changes == 'true' + && needs.detect.outputs.fingerprint != '' + && !inputs.dry_run + env: + GH_TOKEN: ${{ secrets.MANAGE_TOKEN || github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ needs.detect.outputs.pr-number }} + FINGERPRINT: ${{ needs.detect.outputs.fingerprint }} + run: | + set -euo pipefail + + already=false + marker="" + + if gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ + --jq '.[].body' 2>/dev/null | grep -qF "$marker"; then + already=true + echo "::notice::This breaking change (${FINGERPRINT}) was already approved on PR #${PR_NUMBER} — not asking again" + fi + + echo "already-approved=$already" >> "$GITHUB_OUTPUT" + # Read from the BASE ref, never the head: a CODEOWNERS taken from the pull # request branch would let an author edit who is allowed to approve their # own breaking change. @@ -393,6 +442,7 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} pr-number: ${{ needs.detect.outputs.pr-number }} state: waiting + fingerprint: ${{ needs.detect.outputs.fingerprint }} reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} actual-reviewers: ${{ steps.environment.outputs.reviewers }} reason: ${{ needs.detect.outputs.reason }} @@ -416,6 +466,7 @@ jobs: needs.detect.result == 'success' && needs.request.result == 'success' && needs.detect.outputs.has-breaking-changes == 'true' + && needs.request.outputs.already-approved != 'true' && !inputs.dry_run environment: breaking-approval # One open request per pull request. Every push creates another pending @@ -480,6 +531,7 @@ jobs: DETECT_RESULT: ${{ needs.detect.result }} REQUEST_RESULT: ${{ needs.request.result }} APPROVAL_RESULT: ${{ needs.approval.result }} + ALREADY_APPROVED: ${{ needs.request.outputs.already-approved }} IN_SCOPE: ${{ needs.detect.outputs.in-scope }} REASON: ${{ needs.detect.outputs.reason }} PR_NUMBER: ${{ needs.detect.outputs.pr-number }} @@ -515,6 +567,18 @@ jobs: # approval did not complete", which is true of the dead run and # misleading about the live one. # + # An approval already given to this exact breaking change stands. The + # gate job was skipped on purpose, so there is no Environment decision + # to read — the decision is the one recorded on the pull request. + if [ "$ALREADY_APPROVED" = "true" ]; then + approved=true + result=success + state=approved + emit + echo "::notice::PR #${PR_NUMBER} carries a breaking change that was already approved — nothing changed about it" + exit 0 + fi + # A cancelled approval is the first and broader signal. Nothing cancels # it except the concurrency group replacing this run, or a human # cancelling by hand, and neither is a decision to report. It matters @@ -597,6 +661,7 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} pr-number: ${{ needs.detect.outputs.pr-number }} state: ${{ steps.verdict.outputs.state }} + fingerprint: ${{ needs.detect.outputs.fingerprint }} reason: ${{ needs.detect.outputs.reason }} head-sha: ${{ needs.detect.outputs.head-sha }} acknowledged: ${{ needs.detect.outputs.acknowledged }} diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 963dc71b..57d7d704 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -53,6 +53,7 @@ own: |---|---| | [`pr-gate-scope`](../src/validate/pr-gate-scope/README.md) | Is this pull request in scope, and why — base → head rules, label overrides, author vetoes | | [`breaking-change-guard`](../src/validate/breaking-change-guard/README.md) | Does the branch carry a breaking-change commit | +| [`breaking-change-commits`](../src/validate/breaking-change-commits/README.md) | Which commits carry the break, and a digest of the set | | [`codeowners-reviewers`](../src/validate/codeowners-reviewers/README.md) | Who owns this repository, per its CODEOWNERS | | [`environment-protection`](../src/validate/environment-protection/README.md) | Does `breaking-approval` exist with required reviewers — creating it when it does not | | [`environment-approvals`](../src/validate/environment-approvals/README.md) | Who approved or rejected, and could we read that at all | @@ -249,6 +250,44 @@ 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. +## An approval survives a push that did not change the breaking change + +The gate asks about a specific breaking change, not about a pull request. So it +remembers which one was approved, and only asks again when that changes. + +| The author… | Asked again? | +|---|---| +| fixes a typo the review pointed out | no | +| adds an ordinary commit | no | +| rewords the breaking commit | **yes** — it is different text now | +| adds another breaking commit | **yes** | + +This matters more than convenience, because of how the review gate is wired: +CodeRabbit only runs once every check has passed, which includes this one. So +without the memory, every iteration of *approve → review → fix → push* would +need a fresh human approval to get the next review — and the reviewer would be +answering the identical question each time. A question asked five times gets a +reflex answer by the third. + +**How it is remembered.** `breaking-change-commits` digests the SHAs of the +breaking commits. When an approval lands, the comment records that digest: + +```html + +``` + +Every later run rebuilds the digest and looks for its record. A match skips the +Environment gate entirely; anything else asks again. Records accumulate, so +returning to a previously approved set is still approved. + +Built from SHAs, so a reword counts as a change — the approval was given to the +text that was read, and the subject is what the release parser acts on. + +**It is a comment, not a signed record.** Forging one means editing a bot's +comment, which needs maintainer rights — and an admin can already bypass the +Environment outright (`can_admins_bypass`). It does not widen the threat model, +but it is state in text, and worth knowing as such. + ## One open request, and approvals expire on a push Every push queues another pending deployment, and GitHub does not retire the @@ -260,8 +299,9 @@ 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. +That is what retires a *pending* request. An approval that already landed is a +different matter — see above: it is remembered against the change it approved, +and only a change to that change asks again. **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 @@ -414,6 +454,7 @@ correct fix, since the commit message is what the release reads. ```bash bash src/validate/pr-gate-scope/test.sh +bash src/validate/breaking-change-commits/test.sh bash src/validate/codeowners-reviewers/test.sh bash src/validate/breaking-change-guard/test.sh python3 src/validate/breaking-change-guard/test-workflow.py diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 1b126aeb..c1cc7737 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -47,6 +47,10 @@ inputs: 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' + fingerprint: + description: 'Digest of the breaking-change commit set, from `breaking-change-commits`. Recorded in the comment when the state is `approved`, so a later run can tell that the same change was already approved and not ask again. Previous records are preserved.' + required: false + default: '' docs-url: description: Link to the explanation of how the gate works. required: false @@ -99,6 +103,7 @@ runs: REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-reviewers || inputs.reviewers }} SERVER_URL: ${{ inputs.server-url }} BADGE_URL: ${{ inputs.badge-url }} + FINGERPRINT: ${{ inputs.fingerprint }} DOCS_URL: ${{ inputs.docs-url }} APPROVERS: ${{ inputs.approvers }} REJECTERS: ${{ inputs.rejecters }} @@ -261,6 +266,28 @@ runs: existing=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ --jq "[.[] | select(.body | contains(\"${marker}\")) | .id] | first // empty") + # The approval record outlives the comment body. Every run rewrites + # the body, so without carrying these forward an approval would be + # forgotten by the next push — which is the whole thing the record + # exists to prevent. + previous_records="" + if [ -n "$existing" ]; then + previous_records=$(gh api "repos/${REPO}/issues/comments/${existing}" \ + --jq '.body' 2>/dev/null | grep -oE '' || true) + fi + + if [ "$STATE" = "approved" ] && [ -n "$FINGERPRINT" ]; then + previous_records=$(printf '%s\n' \ + "$previous_records" "$FINGERPRINT") + fi + + if [ -n "$(printf '%s' "$previous_records" | tr -d '[:space:]')" ]; then + { + printf '\n' + printf '%s\n' "$previous_records" | awk 'NF' | sort -u + } >> "$body_file" + fi + if [ -n "$existing" ]; then gh api --method PATCH "repos/${REPO}/issues/comments/${existing}" \ -F body=@"$body_file" >/dev/null diff --git a/src/validate/breaking-change-commits/README.md b/src/validate/breaking-change-commits/README.md new file mode 100644 index 00000000..d9d76d40 --- /dev/null +++ b/src/validate/breaking-change-commits/README.md @@ -0,0 +1,96 @@ + + + + + +
Lerian

Breaking Change Commits

+ +> **Unreleased.** Until this ships in a release, reference it as `@develop` — +> `@v1` is the previous release and does not contain it yet. + +Lists the commits in a pull request that carry a breaking change, and +fingerprints the set. + +## Why a fingerprint + +An approval is given to a specific breaking change, not to a pull request. Push +anything afterwards and the question becomes: *is this still the thing that was +approved?* + +Without an answer, the only safe default is to ask again on every push — so a +reviewer who approved a breaking change gets asked the identical question +minutes later because the author fixed a typo. That is how a gate turns into a +formality people click through. + +The fingerprint is built from the **SHAs** of the breaking commits, so: + +| The author… | Fingerprint | +|---|---| +| fixes a typo in an unrelated file | unchanged | +| adds an ordinary commit | unchanged | +| rewords the breaking commit | changes — it is a different commit now | +| adds another breaking commit | changes | + +Rewording counts as a change on purpose: the approval was given to the text that +was read, and the subject is what the release parser acts on. + +## Which commits count + +The same two forms the +[`breaking-change-guard`](../breaking-change-guard/README.md) detector accepts, +and no others: + +- a subject with `!` after the type — `feat(scope)!: …` +- a message line that is a `BREAKING CHANGE` / `BREAKING CHANGES` footer, + optionally prefixed by whitespace, `*` or `|` + +`BREAKING-CHANGE` with a hyphen does not count, and neither does prose that +mentions the phrase mid-sentence — because the release parser treats neither as +breaking. + +**This is a view, not a verdict.** The detector runs over the real commit range +and decides; this reads the API because the jobs that need it have no checkout. +They must agree, which is why the patterns are kept identical — and the tests +here cover the same negative cases. + +## Inputs + +| Input | Description | Required | Default | +|-------|-------------|----------|---------| +| `github-token` | Token used to read the commits. Needs `pull-requests: read`. | Yes | — | +| `pr-number` | Pull request whose commits are listed. | Yes | — | +| `repository` | Repository owning it, as `owner/name`. | No | `${{ github.repository }}` | + +## Outputs + +| Output | Description | +|--------|-------------| +| `count` | How many commits carry a breaking change. | +| `shas` | Comma-separated SHAs, in commit order. | +| `subjects` | Their subjects, one per line. | +| `fingerprint` | Short digest of the SHA set. Empty when there is none. | + +## Usage + +```yaml +- name: List the breaking-change commits + id: breaking + uses: LerianStudio/github-actions-shared-workflows/src/validate/breaking-change-commits@develop + with: + github-token: ${{ secrets.MANAGE_TOKEN || github.token }} + pr-number: ${{ github.event.pull_request.number }} + +- name: Ask only if this is not what was already approved + if: steps.breaking.outputs.fingerprint != steps.previous.outputs.approved_fingerprint + run: echo "needs a decision" +``` + +## Local test + +```bash +bash src/validate/breaking-change-commits/test.sh +``` + +13 cases against a stubbed `gh`: both breaking forms, the two lookalikes that +must not count, ordering, and the fingerprint staying put when an ordinary +commit is added while changing when a breaking one is. diff --git a/src/validate/breaking-change-commits/action.yml b/src/validate/breaking-change-commits/action.yml new file mode 100644 index 00000000..089b4122 --- /dev/null +++ b/src/validate/breaking-change-commits/action.yml @@ -0,0 +1,42 @@ +name: Breaking Change Commits +description: "Lists the commits in a pull request that carry a breaking change, and fingerprints the set so an approval can be matched against what it approved." + +inputs: + github-token: + description: 'Token used to read the pull request commits. Needs `pull-requests: read`.' + required: true + pr-number: + description: Pull request whose commits are listed. + required: true + repository: + description: 'Repository owning the pull request, as `owner/name`.' + required: false + default: ${{ github.repository }} + +outputs: + count: + description: "How many commits carry a breaking change." + value: ${{ steps.resolve.outputs.count }} + shas: + description: "Comma-separated SHAs of those commits, in commit order." + value: ${{ steps.resolve.outputs.shas }} + subjects: + description: "Their subjects, one per line, for showing the author their own commits." + value: ${{ steps.resolve.outputs.subjects }} + fingerprint: + description: "Short digest of the SHA set. Equal fingerprints mean the same breaking change; empty when there is none. Use it to tell an approval that still applies from one that does not." + value: ${{ steps.resolve.outputs.fingerprint }} + +runs: + using: composite + steps: + - name: List the breaking-change commits + id: resolve + shell: bash + env: + GH_TOKEN: ${{ inputs.github-token }} + REPO: ${{ inputs.repository }} + PR_NUMBER: ${{ inputs.pr-number }} + run: | + set -euo pipefail + bash "${GITHUB_ACTION_PATH}/resolve.sh" >> "${GITHUB_OUTPUT}" diff --git a/src/validate/breaking-change-commits/resolve.sh b/src/validate/breaking-change-commits/resolve.sh new file mode 100755 index 00000000..4b52e2f1 --- /dev/null +++ b/src/validate/breaking-change-commits/resolve.sh @@ -0,0 +1,106 @@ +#!/usr/bin/env bash + +# Lists the commits in a pull request that carry a breaking change, and +# fingerprints the set. +# +# Two consumers need this and for different reasons: +# +# the comment, to show the author their own subjects rather than a made-up +# example +# +# the gate, to tell "the approved breaking change is still the same one" from +# "something new arrived". Without that, every push expires an approval given +# minutes earlier for identical content, and a reviewer is asked the same +# question again — which is how a gate becomes a formality. +# +# Read through the API rather than Git, because the jobs that need it have no +# checkout. The canonical detector remains `breaking-change-guard`, which runs +# over the real commit range; this must agree with it, and the patterns below +# are kept identical to the ones pinned there. + +set -euo pipefail + +GH_TOKEN=${GH_TOKEN:-} +REPO=${REPO:-} +PR_NUMBER=${PR_NUMBER:-} + +export GH_TOKEN + +if [[ -z "${REPO}" || -z "${PR_NUMBER}" ]]; then + printf 'Breaking Change Commits: REPO and PR_NUMBER are required.\n' >&2 + exit 1 +fi + +for binary in gh jq; do + if ! command -v "${binary}" >/dev/null 2>&1; then + printf 'Breaking Change Commits: %s is not available.\n' "${binary}" >&2 + exit 1 + fi +done + +if ! commits=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/commits" --paginate \ + --jq '.[] | [.sha, .commit.message] | @base64' 2>/dev/null); then + printf 'Breaking Change Commits: could not read the commits of PR #%s in %s.\n' \ + "${PR_NUMBER}" "${REPO}" >&2 + exit 1 +fi + +shas='' +subjects='' +count=0 + +# `!` after the type, or a `BREAKING CHANGE` footer. Same two forms the +# detector accepts, and deliberately no others: `BREAKING-CHANGE` and `-` +# bullets are not breaking to the release parser either. +is_breaking() { + local message=$1 subject line + + subject=$(head -n1 <<< "${message}") + if [[ "${subject}" =~ ^[A-Za-z0-9_]*(\([^\)]*\))?!:\ ]]; then + return 0 + fi + + while IFS= read -r line; do + line=${line%$'\r'} + if [[ "${line}" =~ ^[[:space:]\|\*]*[Bb][Rr][Ee][Aa][Kk][Ii][Nn][Gg][[:space:]][Cc][Hh][Aa][Nn][Gg][Ee][Ss]?([:[:space:]]) ]]; then + return 0 + fi + done <<< "${message}" + + return 1 +} + +while IFS= read -r encoded; do + [[ -z "${encoded}" ]] && continue + + entry=$(base64 --decode <<< "${encoded}" 2>/dev/null || true) + [[ -z "${entry}" ]] && continue + + sha=$(jq -r '.[0]' <<< "${entry}" 2>/dev/null || true) + message=$(jq -r '.[1]' <<< "${entry}" 2>/dev/null || true) + [[ -z "${sha}" ]] && continue + + if is_breaking "${message}"; then + shas="${shas:+${shas},}${sha}" + subjects="${subjects}$(head -n1 <<< "${message}")"$'\n' + count=$((count + 1)) + fi +done <<< "${commits}" + +# The fingerprint answers one question: is this the same set that was approved? +# Built from the SHAs, so rewording or reordering produces a different one — +# the commit is not the same commit, and the approval was for the old text. +fingerprint='' +if [[ -n "${shas}" ]]; then + fingerprint=$(printf '%s' "${shas}" | shasum -a 256 2>/dev/null | cut -c1-16) \ + || fingerprint=$(printf '%s' "${shas}" | sha256sum | cut -c1-16) +fi + +printf 'count=%s\n' "${count}" +printf 'shas=%s\n' "${shas}" +printf 'fingerprint=%s\n' "${fingerprint}" +{ + printf 'subjects< "${TEST_ROOT}/bin/gh" <<'STUB' +#!/usr/bin/env bash +case "$*" in + *"/commits"*) printf '%s\n' ${COMMITS_B64} ;; + *) exit 1 ;; +esac +STUB +chmod +x "${TEST_ROOT}/bin/gh" +PATH="${TEST_ROOT}/bin:${PATH}" + +# encode +encode() { + jq -cn --arg s "$1" --arg m "$2" '[$s, $m]' | base64 | tr -d '\n' +} + +run() { + env GH_TOKEN=stub REPO=LerianStudio/example PR_NUMBER=42 COMMITS_B64="$1" \ + bash "${RESOLVER}" 2>/dev/null +} + +check() { + local description=$1 commits=$2 expected_count=$3 expected_shas=$4 + local output count shas + output=$(run "${commits}") + count=$(sed -n 's/^count=//p' <<< "${output}") + shas=$(sed -n 's/^shas=//p' <<< "${output}") + + if [[ "${count}" == "${expected_count}" && "${shas}" == "${expected_shas}" ]]; then + PASSED=$((PASSED + 1)) + printf 'ok - %s\n' "${description}" + else + FAILED=$((FAILED + 1)) + printf 'not ok - %s (count=%s shas=%s; wanted %s / %s)\n' \ + "${description}" "${count}" "${shas}" "${expected_count}" "${expected_shas}" + fi +} + +BANG=$(encode aaa1111 'feat(scope)!: breaks the contract') +PLAIN=$(encode bbb2222 'fix(scope): ordinary work') +FOOTER=$(encode ccc3333 'feat(scope): looks ordinary + +BREAKING CHANGE: the field was removed') +BULLET=$(encode ddd4444 'refactor: cleanup + +* BREAKING CHANGE: moved') +HYPHEN=$(encode eee5555 'refactor: cleanup + +BREAKING-CHANGE: moved') +PROSE=$(encode fff6666 'docs: explain the policy + +We document how a BREAKING CHANGE is handled here.') +NOTYPE=$(encode aaa7777 '!: no type at all') + +check 'a bang subject is breaking' "${BANG}" 1 aaa1111 +check 'an ordinary commit is not' "${PLAIN}" 0 '' +check 'a BREAKING CHANGE footer is breaking' "${FOOTER}" 1 ccc3333 +check 'a bullet-prefixed footer is breaking' "${BULLET}" 1 ddd4444 +check 'BREAKING-CHANGE with a hyphen is not' "${HYPHEN}" 0 '' +check 'prose mentioning the phrase mid-line is not' "${PROSE}" 0 '' +check 'a bang with no type is breaking' "${NOTYPE}" 1 aaa7777 +check 'several breaking commits are all listed' "${BANG} ${PLAIN} ${FOOTER}" 2 aaa1111,ccc3333 +check 'order follows the commit list' "${FOOTER} ${BANG}" 2 ccc3333,aaa1111 + +# ----------------- Fingerprint ----------------- +fingerprint_of() { run "$1" | sed -n 's/^fingerprint=//p'; } + +same_a=$(fingerprint_of "${BANG} ${PLAIN}") +same_b=$(fingerprint_of "${BANG} ${PLAIN}") +added=$(fingerprint_of "${BANG} ${PLAIN} ${FOOTER}") +ordinary_added=$(fingerprint_of "${BANG} ${PLAIN} ${PLAIN}") +none=$(fingerprint_of "${PLAIN}") + +assert() { + if [[ "$2" == "$3" ]]; then + PASSED=$((PASSED + 1)); printf 'ok - %s\n' "$1" + else + FAILED=$((FAILED + 1)); printf 'not ok - %s (%s vs %s)\n' "$1" "$2" "$3" + fi +} +refute() { + if [[ "$2" != "$3" ]]; then + PASSED=$((PASSED + 1)); printf 'ok - %s\n' "$1" + else + FAILED=$((FAILED + 1)); printf 'not ok - %s (both %s)\n' "$1" "$2" + fi +} + +assert 'the same set fingerprints the same' "${same_a}" "${same_b}" +assert 'adding an ordinary commit does not change it' "${same_a}" "${ordinary_added}" +refute 'adding a breaking commit changes it' "${same_a}" "${added}" +assert 'no breaking commits means no fingerprint' "${none}" '' + +printf '\n%s passed, %s failed\n' "${PASSED}" "${FAILED}" +[[ ${FAILED} -eq 0 ]] diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index f6e2ab6b..3d2be928 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -50,6 +50,7 @@ "src/validate/environment-approvals", "src/notify/breaking-change-approval-reporter", "src/validate/codeowners-reviewers", + "src/validate/breaking-change-commits", ) ENVIRONMENT_PROTECTION = ( REPO_ROOT / "src" / "validate" / "environment-protection" / "action.yml" @@ -312,6 +313,7 @@ def run_verdict(**overrides): "API_READABLE": "true", "DRY_RUN": "false", "LIVE_SHA": "abc123", + "ALREADY_APPROVED": "false", } env.update(overrides) @@ -1053,6 +1055,41 @@ def test_a_cancelled_approval_stops_before_anything_can_act_on_it(self): self.assertEqual("superseded", outputs["state"]) self.assertNotEqual("rejected", outputs["state"]) + def test_an_approval_already_given_is_not_asked_for_again(self): + # The reviewer said yes to a breaking change. A push that leaves that + # change alone must not put the same question back in their queue — + # asking repeatedly for unchanged content is how a gate stops being + # read. The approval job is skipped, so there is no Environment + # decision; the verdict comes from the record on the pull request. + _, outputs = run_verdict(ALREADY_APPROVED="true", APPROVAL_RESULT="skipped") + self.assertEqual("approved", outputs["state"]) + self.assertEqual("success", outputs["result"]) + + job = extract_job(APPROVAL_WORKFLOW, "approval") + self.assertIn( + "needs.request.outputs.already-approved != 'true'", + job, + "the Environment gate must be skipped when the change was already approved", + ) + + def test_the_approval_record_survives_the_next_run(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + # Every run rewrites the comment body. Without carrying the records + # forward, the approval would be forgotten by the very next push, which + # is the one thing the record exists to prevent. + self.assertIn("previous_records", reporter) + self.assertIn("breaking-change-approved: [a-f0-9]+", reporter) + # Only an approval writes a new record. + self.assertIn('if [ "$STATE" = "approved" ] && [ -n "$FINGERPRINT" ]', reporter) + + def test_the_fingerprint_comes_from_the_breaking_commits(self): + detect = extract_job(APPROVAL_WORKFLOW, "detect") + self.assertIn("src/validate/breaking-change-commits@", detect) + self.assertIn("fingerprint: ${{ steps.commits.outputs.fingerprint }}", detect) + def test_the_verdict_states_are_reachable_as_documented(self): # The whole table in one place, run rather than read. for description, env, expected in ( From d5ba39d409ae30ec77953ab6d1ff7aeb251ea0a3 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 10:15:33 -0300 Subject: [PATCH 14/47] feat(pr-validation): add the breaking-change-commits composite Lists the commits in a pull request that carry a breaking change, and digests their SHAs so an approval can be matched against what it approved. Landed on its own, consumed by nothing yet. A composite cannot be used at @develop by the pull request that introduces it, and now that the approval gate is a required check that is no longer a cosmetic problem: the pull request adding the composite would be blocked by the gate that cannot resolve it. Two pull requests, the same way breaking-change-guard was introduced. The patterns match the detector exactly, including what must NOT count: BREAKING-CHANGE with a hyphen, and prose mentioning the phrase mid-line. Those are the cases where a view that disagreed with the verdict would be most confusing. --- .../breaking-change-commits/README.md | 96 +++++++++++++++ .../breaking-change-commits/action.yml | 42 +++++++ .../breaking-change-commits/resolve.sh | 106 +++++++++++++++++ src/validate/breaking-change-commits/test.sh | 111 ++++++++++++++++++ 4 files changed, 355 insertions(+) create mode 100644 src/validate/breaking-change-commits/README.md create mode 100644 src/validate/breaking-change-commits/action.yml create mode 100755 src/validate/breaking-change-commits/resolve.sh create mode 100755 src/validate/breaking-change-commits/test.sh diff --git a/src/validate/breaking-change-commits/README.md b/src/validate/breaking-change-commits/README.md new file mode 100644 index 00000000..d9d76d40 --- /dev/null +++ b/src/validate/breaking-change-commits/README.md @@ -0,0 +1,96 @@ + + + + + +
Lerian

Breaking Change Commits

+ +> **Unreleased.** Until this ships in a release, reference it as `@develop` — +> `@v1` is the previous release and does not contain it yet. + +Lists the commits in a pull request that carry a breaking change, and +fingerprints the set. + +## Why a fingerprint + +An approval is given to a specific breaking change, not to a pull request. Push +anything afterwards and the question becomes: *is this still the thing that was +approved?* + +Without an answer, the only safe default is to ask again on every push — so a +reviewer who approved a breaking change gets asked the identical question +minutes later because the author fixed a typo. That is how a gate turns into a +formality people click through. + +The fingerprint is built from the **SHAs** of the breaking commits, so: + +| The author… | Fingerprint | +|---|---| +| fixes a typo in an unrelated file | unchanged | +| adds an ordinary commit | unchanged | +| rewords the breaking commit | changes — it is a different commit now | +| adds another breaking commit | changes | + +Rewording counts as a change on purpose: the approval was given to the text that +was read, and the subject is what the release parser acts on. + +## Which commits count + +The same two forms the +[`breaking-change-guard`](../breaking-change-guard/README.md) detector accepts, +and no others: + +- a subject with `!` after the type — `feat(scope)!: …` +- a message line that is a `BREAKING CHANGE` / `BREAKING CHANGES` footer, + optionally prefixed by whitespace, `*` or `|` + +`BREAKING-CHANGE` with a hyphen does not count, and neither does prose that +mentions the phrase mid-sentence — because the release parser treats neither as +breaking. + +**This is a view, not a verdict.** The detector runs over the real commit range +and decides; this reads the API because the jobs that need it have no checkout. +They must agree, which is why the patterns are kept identical — and the tests +here cover the same negative cases. + +## Inputs + +| Input | Description | Required | Default | +|-------|-------------|----------|---------| +| `github-token` | Token used to read the commits. Needs `pull-requests: read`. | Yes | — | +| `pr-number` | Pull request whose commits are listed. | Yes | — | +| `repository` | Repository owning it, as `owner/name`. | No | `${{ github.repository }}` | + +## Outputs + +| Output | Description | +|--------|-------------| +| `count` | How many commits carry a breaking change. | +| `shas` | Comma-separated SHAs, in commit order. | +| `subjects` | Their subjects, one per line. | +| `fingerprint` | Short digest of the SHA set. Empty when there is none. | + +## Usage + +```yaml +- name: List the breaking-change commits + id: breaking + uses: LerianStudio/github-actions-shared-workflows/src/validate/breaking-change-commits@develop + with: + github-token: ${{ secrets.MANAGE_TOKEN || github.token }} + pr-number: ${{ github.event.pull_request.number }} + +- name: Ask only if this is not what was already approved + if: steps.breaking.outputs.fingerprint != steps.previous.outputs.approved_fingerprint + run: echo "needs a decision" +``` + +## Local test + +```bash +bash src/validate/breaking-change-commits/test.sh +``` + +13 cases against a stubbed `gh`: both breaking forms, the two lookalikes that +must not count, ordering, and the fingerprint staying put when an ordinary +commit is added while changing when a breaking one is. diff --git a/src/validate/breaking-change-commits/action.yml b/src/validate/breaking-change-commits/action.yml new file mode 100644 index 00000000..089b4122 --- /dev/null +++ b/src/validate/breaking-change-commits/action.yml @@ -0,0 +1,42 @@ +name: Breaking Change Commits +description: "Lists the commits in a pull request that carry a breaking change, and fingerprints the set so an approval can be matched against what it approved." + +inputs: + github-token: + description: 'Token used to read the pull request commits. Needs `pull-requests: read`.' + required: true + pr-number: + description: Pull request whose commits are listed. + required: true + repository: + description: 'Repository owning the pull request, as `owner/name`.' + required: false + default: ${{ github.repository }} + +outputs: + count: + description: "How many commits carry a breaking change." + value: ${{ steps.resolve.outputs.count }} + shas: + description: "Comma-separated SHAs of those commits, in commit order." + value: ${{ steps.resolve.outputs.shas }} + subjects: + description: "Their subjects, one per line, for showing the author their own commits." + value: ${{ steps.resolve.outputs.subjects }} + fingerprint: + description: "Short digest of the SHA set. Equal fingerprints mean the same breaking change; empty when there is none. Use it to tell an approval that still applies from one that does not." + value: ${{ steps.resolve.outputs.fingerprint }} + +runs: + using: composite + steps: + - name: List the breaking-change commits + id: resolve + shell: bash + env: + GH_TOKEN: ${{ inputs.github-token }} + REPO: ${{ inputs.repository }} + PR_NUMBER: ${{ inputs.pr-number }} + run: | + set -euo pipefail + bash "${GITHUB_ACTION_PATH}/resolve.sh" >> "${GITHUB_OUTPUT}" diff --git a/src/validate/breaking-change-commits/resolve.sh b/src/validate/breaking-change-commits/resolve.sh new file mode 100755 index 00000000..4b52e2f1 --- /dev/null +++ b/src/validate/breaking-change-commits/resolve.sh @@ -0,0 +1,106 @@ +#!/usr/bin/env bash + +# Lists the commits in a pull request that carry a breaking change, and +# fingerprints the set. +# +# Two consumers need this and for different reasons: +# +# the comment, to show the author their own subjects rather than a made-up +# example +# +# the gate, to tell "the approved breaking change is still the same one" from +# "something new arrived". Without that, every push expires an approval given +# minutes earlier for identical content, and a reviewer is asked the same +# question again — which is how a gate becomes a formality. +# +# Read through the API rather than Git, because the jobs that need it have no +# checkout. The canonical detector remains `breaking-change-guard`, which runs +# over the real commit range; this must agree with it, and the patterns below +# are kept identical to the ones pinned there. + +set -euo pipefail + +GH_TOKEN=${GH_TOKEN:-} +REPO=${REPO:-} +PR_NUMBER=${PR_NUMBER:-} + +export GH_TOKEN + +if [[ -z "${REPO}" || -z "${PR_NUMBER}" ]]; then + printf 'Breaking Change Commits: REPO and PR_NUMBER are required.\n' >&2 + exit 1 +fi + +for binary in gh jq; do + if ! command -v "${binary}" >/dev/null 2>&1; then + printf 'Breaking Change Commits: %s is not available.\n' "${binary}" >&2 + exit 1 + fi +done + +if ! commits=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/commits" --paginate \ + --jq '.[] | [.sha, .commit.message] | @base64' 2>/dev/null); then + printf 'Breaking Change Commits: could not read the commits of PR #%s in %s.\n' \ + "${PR_NUMBER}" "${REPO}" >&2 + exit 1 +fi + +shas='' +subjects='' +count=0 + +# `!` after the type, or a `BREAKING CHANGE` footer. Same two forms the +# detector accepts, and deliberately no others: `BREAKING-CHANGE` and `-` +# bullets are not breaking to the release parser either. +is_breaking() { + local message=$1 subject line + + subject=$(head -n1 <<< "${message}") + if [[ "${subject}" =~ ^[A-Za-z0-9_]*(\([^\)]*\))?!:\ ]]; then + return 0 + fi + + while IFS= read -r line; do + line=${line%$'\r'} + if [[ "${line}" =~ ^[[:space:]\|\*]*[Bb][Rr][Ee][Aa][Kk][Ii][Nn][Gg][[:space:]][Cc][Hh][Aa][Nn][Gg][Ee][Ss]?([:[:space:]]) ]]; then + return 0 + fi + done <<< "${message}" + + return 1 +} + +while IFS= read -r encoded; do + [[ -z "${encoded}" ]] && continue + + entry=$(base64 --decode <<< "${encoded}" 2>/dev/null || true) + [[ -z "${entry}" ]] && continue + + sha=$(jq -r '.[0]' <<< "${entry}" 2>/dev/null || true) + message=$(jq -r '.[1]' <<< "${entry}" 2>/dev/null || true) + [[ -z "${sha}" ]] && continue + + if is_breaking "${message}"; then + shas="${shas:+${shas},}${sha}" + subjects="${subjects}$(head -n1 <<< "${message}")"$'\n' + count=$((count + 1)) + fi +done <<< "${commits}" + +# The fingerprint answers one question: is this the same set that was approved? +# Built from the SHAs, so rewording or reordering produces a different one — +# the commit is not the same commit, and the approval was for the old text. +fingerprint='' +if [[ -n "${shas}" ]]; then + fingerprint=$(printf '%s' "${shas}" | shasum -a 256 2>/dev/null | cut -c1-16) \ + || fingerprint=$(printf '%s' "${shas}" | sha256sum | cut -c1-16) +fi + +printf 'count=%s\n' "${count}" +printf 'shas=%s\n' "${shas}" +printf 'fingerprint=%s\n' "${fingerprint}" +{ + printf 'subjects< "${TEST_ROOT}/bin/gh" <<'STUB' +#!/usr/bin/env bash +case "$*" in + *"/commits"*) printf '%s\n' ${COMMITS_B64} ;; + *) exit 1 ;; +esac +STUB +chmod +x "${TEST_ROOT}/bin/gh" +PATH="${TEST_ROOT}/bin:${PATH}" + +# encode +encode() { + jq -cn --arg s "$1" --arg m "$2" '[$s, $m]' | base64 | tr -d '\n' +} + +run() { + env GH_TOKEN=stub REPO=LerianStudio/example PR_NUMBER=42 COMMITS_B64="$1" \ + bash "${RESOLVER}" 2>/dev/null +} + +check() { + local description=$1 commits=$2 expected_count=$3 expected_shas=$4 + local output count shas + output=$(run "${commits}") + count=$(sed -n 's/^count=//p' <<< "${output}") + shas=$(sed -n 's/^shas=//p' <<< "${output}") + + if [[ "${count}" == "${expected_count}" && "${shas}" == "${expected_shas}" ]]; then + PASSED=$((PASSED + 1)) + printf 'ok - %s\n' "${description}" + else + FAILED=$((FAILED + 1)) + printf 'not ok - %s (count=%s shas=%s; wanted %s / %s)\n' \ + "${description}" "${count}" "${shas}" "${expected_count}" "${expected_shas}" + fi +} + +BANG=$(encode aaa1111 'feat(scope)!: breaks the contract') +PLAIN=$(encode bbb2222 'fix(scope): ordinary work') +FOOTER=$(encode ccc3333 'feat(scope): looks ordinary + +BREAKING CHANGE: the field was removed') +BULLET=$(encode ddd4444 'refactor: cleanup + +* BREAKING CHANGE: moved') +HYPHEN=$(encode eee5555 'refactor: cleanup + +BREAKING-CHANGE: moved') +PROSE=$(encode fff6666 'docs: explain the policy + +We document how a BREAKING CHANGE is handled here.') +NOTYPE=$(encode aaa7777 '!: no type at all') + +check 'a bang subject is breaking' "${BANG}" 1 aaa1111 +check 'an ordinary commit is not' "${PLAIN}" 0 '' +check 'a BREAKING CHANGE footer is breaking' "${FOOTER}" 1 ccc3333 +check 'a bullet-prefixed footer is breaking' "${BULLET}" 1 ddd4444 +check 'BREAKING-CHANGE with a hyphen is not' "${HYPHEN}" 0 '' +check 'prose mentioning the phrase mid-line is not' "${PROSE}" 0 '' +check 'a bang with no type is breaking' "${NOTYPE}" 1 aaa7777 +check 'several breaking commits are all listed' "${BANG} ${PLAIN} ${FOOTER}" 2 aaa1111,ccc3333 +check 'order follows the commit list' "${FOOTER} ${BANG}" 2 ccc3333,aaa1111 + +# ----------------- Fingerprint ----------------- +fingerprint_of() { run "$1" | sed -n 's/^fingerprint=//p'; } + +same_a=$(fingerprint_of "${BANG} ${PLAIN}") +same_b=$(fingerprint_of "${BANG} ${PLAIN}") +added=$(fingerprint_of "${BANG} ${PLAIN} ${FOOTER}") +ordinary_added=$(fingerprint_of "${BANG} ${PLAIN} ${PLAIN}") +none=$(fingerprint_of "${PLAIN}") + +assert() { + if [[ "$2" == "$3" ]]; then + PASSED=$((PASSED + 1)); printf 'ok - %s\n' "$1" + else + FAILED=$((FAILED + 1)); printf 'not ok - %s (%s vs %s)\n' "$1" "$2" "$3" + fi +} +refute() { + if [[ "$2" != "$3" ]]; then + PASSED=$((PASSED + 1)); printf 'ok - %s\n' "$1" + else + FAILED=$((FAILED + 1)); printf 'not ok - %s (both %s)\n' "$1" "$2" + fi +} + +assert 'the same set fingerprints the same' "${same_a}" "${same_b}" +assert 'adding an ordinary commit does not change it' "${same_a}" "${ordinary_added}" +refute 'adding a breaking commit changes it' "${same_a}" "${added}" +assert 'no breaking commits means no fingerprint' "${none}" '' + +printf '\n%s passed, %s failed\n' "${PASSED}" "${FAILED}" +[[ ${FAILED} -eq 0 ]] From 1823fb553a699225b75f8d2da50205b8d2a0667f Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 10:41:48 -0300 Subject: [PATCH 15/47] fix(pr-validation): stop a commit subject from forging the output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #915. The first one is a gate bypass. Commit subjects are written to GITHUB_OUTPUT as a multiline value with a fixed delimiter, and whoever opens the pull request writes them. A commit whose subject IS that delimiter closes the block, and every line after it becomes an output assignment — including `fingerprint=`, which decides whether a breaking change counts as already approved. Two commits are enough to skip the approval entirely: subject: BREAKING_SUBJECTS_EOF + a BREAKING CHANGE footer subject: fingerprint= + a BREAKING CHANGE footer The delimiter is now random per run and checked against the subjects before anything is written. The suite runs that attack, and the check is parsed the way the runner parses it — lines inside the block are a value, so grepping the raw text would have called a harmless line a breach. The patterns also did not match the detector, despite the header saying they did: [[:space:]\|\*] — a backslash inside a bracket expression is a literal backslash, so the class quietly accepted a leading `\` \([^)]*\) for the scope where the detector has \(.*\), so a scope containing a closing paren was breaking there and ordinary here Both now transcribe the detector exactly, held in variables so a bare `|` is not parsed as an operator. Six cases cover the forms where a slip shows: lowercase, mixed case, plural, space separator, uppercase subject, and a scope with a paren in it. And a resolver failure now reports `not ok` instead of aborting the suite at the assignment, where `set -e` took the summary with it. --- .../breaking-change-commits/README.md | 23 +++++- .../breaking-change-commits/resolve.sh | 40 ++++++++-- src/validate/breaking-change-commits/test.sh | 73 ++++++++++++++++++- 3 files changed, 126 insertions(+), 10 deletions(-) diff --git a/src/validate/breaking-change-commits/README.md b/src/validate/breaking-change-commits/README.md index d9d76d40..d7d94a4c 100644 --- a/src/validate/breaking-change-commits/README.md +++ b/src/validate/breaking-change-commits/README.md @@ -48,6 +48,18 @@ and no others: mentions the phrase mid-sentence — because the release parser treats neither as breaking. +## Commit subjects are untrusted text + +The subjects go into `GITHUB_OUTPUT` as a multiline value, and whoever opens the +pull request writes them. With a fixed delimiter, a commit whose subject *is* +that delimiter closes the block early, and every line after it is read as a new +output assignment — `fingerprint=` included, which is what decides whether a +breaking change counts as already approved. + +So the delimiter is random per run and checked against the subjects before +anything is written. The test suite includes the attack: two commits, one +closing the block and one assigning a forged fingerprint. + **This is a view, not a verdict.** The detector runs over the real commit range and decides; this reads the API because the jobs that need it have no checkout. They must agree, which is why the patterns are kept identical — and the tests @@ -72,6 +84,10 @@ here cover the same negative cases. ## Usage +Testing, while this is unreleased — `@develop` is the only ref that resolves +today. After it ships, pin a tier channel (`@tier-0` / `@tier-1` / `@tier-2`) +and never `@develop`, which moves under the caller. + ```yaml - name: List the breaking-change commits id: breaking @@ -91,6 +107,7 @@ here cover the same negative cases. bash src/validate/breaking-change-commits/test.sh ``` -13 cases against a stubbed `gh`: both breaking forms, the two lookalikes that -must not count, ordering, and the fingerprint staying put when an ordinary -commit is added while changing when a breaking one is. +20 cases against a stubbed `gh`: both breaking forms, the two lookalikes that +must not count, ordering, the fingerprint staying put when an ordinary commit is +added while changing when a breaking one is, parity with the detector on case +and on scopes containing parens, and the output-injection attack. diff --git a/src/validate/breaking-change-commits/resolve.sh b/src/validate/breaking-change-commits/resolve.sh index 4b52e2f1..a25698e5 100755 --- a/src/validate/breaking-change-commits/resolve.sh +++ b/src/validate/breaking-change-commits/resolve.sh @@ -52,17 +52,34 @@ count=0 # `!` after the type, or a `BREAKING CHANGE` footer. Same two forms the # detector accepts, and deliberately no others: `BREAKING-CHANGE` and `-` # bullets are not breaking to the release parser either. +# Transcribed from the detector's awk, which lowercases the line first and then +# applies: +# +# subject ^[a-z0-9_]*(\(.*\))?!: .*$ +# footer ^[[:space:]|*]*breaking changes?(:|[[:space:]])+ +# +# Held in variables because a bare `|` inside `[[ =~ ]]` is parsed as an +# operator, and because the two must be read side by side with the detector to +# stay honest. `\(.*\)` for the scope, not `\([^)]*\)`: a scope containing a +# closing paren matches there, so it has to match here. +BANG_SUBJECT_RE='^[a-z0-9_]*(\(.*\))?!: ' +# `[[:space:]|*]` — inside a bracket expression a backslash is a literal +# backslash, so escaping `|` and `*` there would quietly widen the class to +# accept a leading `\`. +FOOTER_RE='^[[:space:]|*]*breaking changes?(:|[[:space:]])' + is_breaking() { - local message=$1 subject line + local message=$1 subject line lowered - subject=$(head -n1 <<< "${message}") - if [[ "${subject}" =~ ^[A-Za-z0-9_]*(\([^\)]*\))?!:\ ]]; then + subject=$(printf '%s' "${message}" | head -n1 | tr '[:upper:]' '[:lower:]') + if [[ "${subject}" =~ ${BANG_SUBJECT_RE} ]]; then return 0 fi while IFS= read -r line; do line=${line%$'\r'} - if [[ "${line}" =~ ^[[:space:]\|\*]*[Bb][Rr][Ee][Aa][Kk][Ii][Nn][Gg][[:space:]][Cc][Hh][Aa][Nn][Gg][Ee][Ss]?([:[:space:]]) ]]; then + lowered=$(printf '%s' "${line}" | tr '[:upper:]' '[:lower:]') + if [[ "${lowered}" =~ ${FOOTER_RE} ]]; then return 0 fi done <<< "${message}" @@ -99,8 +116,19 @@ fi printf 'count=%s\n' "${count}" printf 'shas=%s\n' "${shas}" printf 'fingerprint=%s\n' "${fingerprint}" +# The subjects are commit-controlled text going into GITHUB_OUTPUT. With a +# fixed delimiter, a commit whose subject IS that delimiter closes the block +# early and every line after it becomes a new output assignment — including +# `fingerprint=`, which is what decides whether this breaking change was +# already approved. That is a gate bypass written by whoever opens the pull +# request, so the delimiter is random per run and verified not to appear. +delimiter="BREAKING_SUBJECTS_$(head -c 16 /dev/urandom | od -An -tx1 | tr -d ' \n')" +while grep -Fqx -- "${delimiter}" <<< "${subjects}"; do + delimiter="BREAKING_SUBJECTS_$(head -c 16 /dev/urandom | od -An -tx1 | tr -d ' \n')" +done + { - printf 'subjects</dev/null + bash "${RESOLVER}" 2>/dev/null || true } check() { @@ -78,6 +81,74 @@ check 'a bang with no type is breaking' "${NOTYPE}" 1 aaa7777 check 'several breaking commits are all listed' "${BANG} ${PLAIN} ${FOOTER}" 2 aaa1111,ccc3333 check 'order follows the commit list' "${FOOTER} ${BANG}" 2 ccc3333,aaa1111 +# ----------------- Parity with the detector ----------------- +# The detector lowercases before matching, and allows any scope content. These +# are the forms where a transcription slip would make this view disagree with +# the verdict. +LOWER=$(encode aab1111 'feat(scope): looks ordinary + +breaking change: lowercase footer') +MIXED=$(encode aab2222 'feat(scope): looks ordinary + +Breaking Change: mixed case footer') +PLURAL=$(encode aab3333 'feat(scope): looks ordinary + +BREAKING CHANGES: plural') +SPACED=$(encode aab4444 'feat(scope): looks ordinary + +BREAKING CHANGE the separator is a space') +NESTED=$(encode aab5555 'feat(a)b)!: a scope containing a closing paren') +UPPER_SUBJECT=$(encode aab6666 'FEAT(SCOPE)!: an uppercase subject') + +check 'a lowercase footer is breaking' "${LOWER}" 1 aab1111 +check 'a mixed-case footer is breaking' "${MIXED}" 1 aab2222 +check 'the plural form is breaking' "${PLURAL}" 1 aab3333 +check 'a space separator is breaking' "${SPACED}" 1 aab4444 +check 'a scope containing a paren still matches' "${NESTED}" 1 aab5555 +check 'an uppercase subject is breaking' "${UPPER_SUBJECT}" 1 aab6666 + +# ----------------- Output injection ----------------- +# The subjects are commit-controlled and go into GITHUB_OUTPUT as a multiline +# value. A fixed delimiter would let a commit close the block and write its own +# assignments — `fingerprint=` among them, which decides whether this change +# counts as already approved. +INJECT_CLOSE=$(encode aac1111 'BREAKING_SUBJECTS_EOF + +BREAKING CHANGE: closes the block above') +INJECT_VALUE=$(encode aac2222 'fingerprint=deadbeefdeadbeef + +BREAKING CHANGE: and assigns a forged one') + +# Parsed the way the runner parses GITHUB_OUTPUT: lines inside a multiline +# block are a value, not assignments. Grepping the raw text would find the +# forged line sitting harmlessly inside the block and call it a breach. +single_line_assignments() { + local line delimiter='' in_block=0 + while IFS= read -r line; do + if [[ ${in_block} -eq 1 ]]; then + [[ "${line}" == "${delimiter}" ]] && in_block=0 + continue + fi + if [[ "${line}" == *'<<'* ]]; then + delimiter=${line#*<<} + in_block=1 + continue + fi + printf '%s\n' "${line}" + done +} + +injected=$(run "${INJECT_CLOSE} ${INJECT_VALUE}") +forged=$(single_line_assignments <<< "${injected}" \ + | grep -c '^fingerprint=deadbeefdeadbeef$' || true) +if [[ "${forged}" == '0' ]]; then + PASSED=$((PASSED + 1)) + printf 'ok - a commit subject cannot close the output block and forge a value\n' +else + FAILED=$((FAILED + 1)) + printf 'not ok - a commit subject forged an output assignment\n' +fi + # ----------------- Fingerprint ----------------- fingerprint_of() { run "$1" | sed -n 's/^fingerprint=//p'; } From 8ebfab110abb6f8073cd9adf9535d9f3c3eb7fef Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 11:10:10 -0300 Subject: [PATCH 16/47] fix(pr-validation): verify the approval record instead of believing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #914. The first finding is a gate bypass, and the documentation I wrote got the threat model wrong. The lookup accepted the fingerprint marker from any comment on the pull request. Anyone with read access can comment, and the fingerprint is computable from public data — the commit SHAs are on the pull request and the digest is defined in this repository. So a stranger could paste a marker and skip the Environment entirely. The docs claimed forging "means editing a bot's comment, which needs maintainer rights". Nothing had to be edited; creating a comment was enough. Two independent conditions now hold before an approval is reused: the record sits in a comment from an OWNER or MEMBER the run it names really was approved for the Environment, read from the Environment's own approval log The second carries the weight. Writing the text is easy; producing a run whose approval actually happened is not. Verification needs `actions: read`; without it the approval is not reused and the reviewer is asked again — asking twice costs attention, trusting an unverified record costs the gate. Three more from the same review: `grep -qF` could exit on the first match while `--paginate` was still writing, and under pipefail the broken pipe read as "no match" — so an approval already given was asked for again. A second `grep -q` had the same hazard and was not flagged; both are gone. the reused-approval branch sat ahead of the cancelled, superseded and request-result guards, so a run whose preflight had failed reported success. A standing approval is not a reason to skip the checks that say whether this run may speak at all. a failed comment read turned into "there were no records", and the rewrite that followed erased every approval on the pull request. It now fails instead of rewriting blind. --- .../workflows/breaking-change-approval.yml | 80 +++++++++++++++---- docs/breaking-change-approval.md | 34 ++++++-- .../action.yml | 30 +++++-- .../breaking-change-guard/test-workflow.py | 63 ++++++++++++++- 4 files changed, 178 insertions(+), 29 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index b184b6df..8211ed4a 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -391,12 +391,55 @@ jobs: set -euo pipefail already=false - marker="" - if gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ - --jq '.[].body' 2>/dev/null | grep -qF "$marker"; then - already=true - echo "::notice::This breaking change (${FINGERPRINT}) was already approved on PR #${PR_NUMBER} — not asking again" + # Two things have to be true, because the comment alone is not + # evidence. Anyone who can read the repository can comment on a pull + # request, and the fingerprint is computable from public data — the + # commit SHAs are on the pull request and the digest is in this + # repository. A marker on its own is therefore forgeable by a + # stranger, which would hand them the approval gate. + # + # 1. the record sits in a comment from someone inside the + # organisation, which is where the reporter writes + # 2. the run it names really was approved for this Environment + # + # The second is the one that matters: forging the text is easy, + # producing a run whose Environment approval actually happened is not. + # + # `grep -F ... >/dev/null`, not `grep -qF`: with `pipefail`, `-q` can + # exit on the first match while `--paginate` is still writing, and the + # broken pipe would read as "no match" — asking for an approval that + # was already given. + records=$( + gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ + --jq '.[] + | select(.author_association == "OWNER" + or .author_association == "MEMBER") + | .body' 2>/dev/null \ + | grep -oE "" \ + || true + ) + + if [ -n "$records" ]; then + approving_run=$(head -n1 <<< "$records" | grep -oE 'run=[0-9]+' | cut -d= -f2) + + # Verified against the Environment's own approval log. Needs + # `actions: read`; when that is missing the check cannot be made, so + # the approval is not reused and the reviewer is asked again. Asking + # twice is a cost, trusting an unverified record is a hole. + approvers=$( + gh api "repos/${REPO}/actions/runs/${approving_run}/approvals" \ + --jq '.[] | select(.state == "approved") + | select((.environments // []) | map(.name) | index("breaking-approval")) + | .user.login' 2>/dev/null || true + ) + + if [ -n "$approvers" ]; then + already=true + echo "::notice::This breaking change (${FINGERPRINT}) was approved on run ${approving_run} — not asking again" + else + echo "::warning::A record claims run ${approving_run} approved ${FINGERPRINT}, but that approval could not be verified. Asking again." + fi fi echo "already-approved=$already" >> "$GITHUB_OUTPUT" @@ -567,18 +610,6 @@ jobs: # approval did not complete", which is true of the dead run and # misleading about the live one. # - # An approval already given to this exact breaking change stands. The - # gate job was skipped on purpose, so there is no Environment decision - # to read — the decision is the one recorded on the pull request. - if [ "$ALREADY_APPROVED" = "true" ]; then - approved=true - result=success - state=approved - emit - echo "::notice::PR #${PR_NUMBER} carries a breaking change that was already approved — nothing changed about it" - exit 0 - fi - # A cancelled approval is the first and broader signal. Nothing cancels # it except the concurrency group replacing this run, or a human # cancelling by hand, and neither is a decision to report. It matters @@ -613,6 +644,20 @@ jobs: exit 0 fi + # Only now, with the guards behind us. Placed any earlier this would + # report success for a run whose `request` job failed — the preflight + # that proves the Environment is real lives there — or for a run the + # head has moved out from under. An approval that still applies is not + # a reason to skip the checks that say whether this run may speak. + if [ "$ALREADY_APPROVED" = "true" ]; then + approved=true + result=success + state=approved + emit + echo "::notice::PR #${PR_NUMBER} carries a breaking change that was already approved — nothing changed about it" + exit 0 + fi + if [ "$IN_SCOPE" != "true" ] || [ "$HAS_BREAKING_CHANGES" != "true" ]; then approved=true result=success @@ -662,6 +707,7 @@ jobs: pr-number: ${{ needs.detect.outputs.pr-number }} state: ${{ steps.verdict.outputs.state }} fingerprint: ${{ needs.detect.outputs.fingerprint }} + run-id: ${{ github.run_id }} reason: ${{ needs.detect.outputs.reason }} head-sha: ${{ needs.detect.outputs.head-sha }} acknowledged: ${{ needs.detect.outputs.acknowledged }} diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 751b0fc8..81c101a0 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -270,10 +270,11 @@ answering the identical question each time. A question asked five times gets a reflex answer by the third. **How it is remembered.** `breaking-change-commits` digests the SHAs of the -breaking commits. When an approval lands, the comment records that digest: +breaking commits. When an approval lands, the comment records that digest +together with the run that carried the approval: ```html - + ``` Every later run rebuilds the digest and looks for its record. A match skips the @@ -283,10 +284,31 @@ returning to a previously approved set is still approved. Built from SHAs, so a reword counts as a change — the approval was given to the text that was read, and the subject is what the release parser acts on. -**It is a comment, not a signed record.** Forging one means editing a bot's -comment, which needs maintainer rights — and an admin can already bypass the -Environment outright (`can_admins_bypass`). It does not widen the threat model, -but it is state in text, and worth knowing as such. +### The record is checked, not believed + +A comment is not evidence. Anyone who can read the repository can comment on a +pull request, and the fingerprint is computable from public data: the commit +SHAs are on the pull request and the digest is defined in this repository. A +record on its own is therefore something a stranger can write. + +So two things must hold before an approval is reused: + +1. the record sits in a comment from an **owner or member** of the + organisation, which is where the reporter writes +2. the **run it names really was approved** for the `breaking-approval` + Environment, read from the Environment's own approval log + +The second is what carries the weight. Writing the text is easy; producing a run +whose Environment approval actually happened is not. + +Verifying needs `actions: read`. Where that is unavailable the check cannot be +made, so the approval is **not** reused and the reviewer is asked again — asking +twice costs attention, trusting an unverified record costs the gate. + +**The records are never rewritten blind.** If the comment cannot be read back, +the reporter fails rather than replacing it, because a rewrite without the +records would erase approvals already given and send reviewers questions they +had already answered. ## One open request, and approvals expire on a push diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index c1cc7737..8ee8a41c 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -48,9 +48,13 @@ inputs: required: false default: 'https://img.shields.io/badge/%E2%9E%A1%EF%B8%8F%20Review%20pending%20deployment-1f6feb?style=for-the-badge' fingerprint: - description: 'Digest of the breaking-change commit set, from `breaking-change-commits`. Recorded in the comment when the state is `approved`, so a later run can tell that the same change was already approved and not ask again. Previous records are preserved.' + 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 default: '' + run-id: + description: Run recorded with the approval, so the next run can verify that this run's Environment approval actually happened. + required: false + default: ${{ github.run_id }} docs-url: description: Link to the explanation of how the gate works. required: false @@ -104,6 +108,7 @@ runs: SERVER_URL: ${{ inputs.server-url }} BADGE_URL: ${{ inputs.badge-url }} FINGERPRINT: ${{ inputs.fingerprint }} + RUN_ID: ${{ inputs.run-id }} DOCS_URL: ${{ inputs.docs-url }} APPROVERS: ${{ inputs.approvers }} REJECTERS: ${{ inputs.rejecters }} @@ -272,13 +277,28 @@ runs: # exists to prevent. previous_records="" if [ -n "$existing" ]; then - previous_records=$(gh api "repos/${REPO}/issues/comments/${existing}" \ - --jq '.body' 2>/dev/null | grep -oE '' || true) + # A failed read must not look like "there were none". `|| true` + # here would hand back an empty set, the PATCH below would rewrite + # the comment without the records, and every approval on this pull + # request would be silently erased by a transient API error — the + # reviewers asked again for changes they had already cleared. + if ! current_body=$(gh api "repos/${REPO}/issues/comments/${existing}" --jq '.body'); then + echo "Could not read the existing comment (${existing}) to carry its approval records forward." >&2 + echo "Refusing to rewrite it: that would drop approvals already given." >&2 + exit 1 + fi + + # A successful read with no markers is a legitimately empty set. + previous_records=$(grep -oE '' \ + <<< "$current_body" || true) fi if [ "$STATE" = "approved" ] && [ -n "$FINGERPRINT" ]; then - previous_records=$(printf '%s\n' \ - "$previous_records" "$FINGERPRINT") + # The run is recorded alongside the fingerprint so the next run can + # verify the approval against the Environment's log rather than + # taking this text at its word. + previous_records=$(printf '%s\n' \ + "$previous_records" "$FINGERPRINT" "${RUN_ID}") fi if [ -n "$(printf '%s' "$previous_records" | tr -d '[:space:]')" ]; then diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 3d2be928..ac7a1aa4 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1072,6 +1072,67 @@ def test_an_approval_already_given_is_not_asked_for_again(self): "the Environment gate must be skipped when the change was already approved", ) + def test_a_forged_approval_record_is_not_believed(self): + step = extract_step( + extract_job(APPROVAL_WORKFLOW, "request"), + "Check for a prior approval of the same change", + ) + # Anyone with read access can comment, and the fingerprint is computable + # from public data — so the record alone is writable by a stranger. + # Two independent conditions must hold before an approval is reused. + self.assertIn('select(.author_association == "OWNER"', step) + self.assertIn('or .author_association == "MEMBER")', step) + # And the run it names has to carry a real Environment approval. + self.assertIn("actions/runs/${approving_run}/approvals", step) + self.assertIn('select(.state == "approved")', step) + self.assertIn('index("breaking-approval")', step) + + def test_the_prior_lookup_consumes_the_whole_comment_stream(self): + step = extract_step( + extract_job(APPROVAL_WORKFLOW, "request"), + "Check for a prior approval of the same change", + ) + # `grep -q` can exit on the first match while `--paginate` is still + # writing; under pipefail the broken pipe reads as "no match", and the + # reviewer is asked for an approval they already gave. + # + # Comment lines are stripped first: the step explains this very hazard + # in prose, and matching that prose would make the assertion pass or + # fail on the explanation rather than on the code. + code = "\n".join( + line for line in step.splitlines() if not line.strip().startswith("#") + ) + self.assertNotIn("grep -q", code) + self.assertIn("grep -oE", code) + + def test_a_reused_approval_does_not_skip_the_other_guards(self): + body = extract_run_body( + extract_job(APPROVAL_WORKFLOW, "outcome"), "Resolve the verdict" + ) + already_at = body.index('"$ALREADY_APPROVED" = "true"') + for guard in ( + '"$APPROVAL_RESULT" = "cancelled"', # superseded by a newer run + '"$live_sha" != "$HEAD_SHA"', # head moved under us + '"$REQUEST_RESULT" != "success"', # preflight never proved the gate + ): + with self.subTest(guard=guard): + self.assertLess( + body.index(guard), + already_at, + "a standing approval is not a reason to skip the checks that " + "say whether this run may speak at all", + ) + + def test_the_records_are_never_rewritten_blind(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + # A failed read must not look like "there were none": the rewrite would + # erase approvals already given. + self.assertIn("Refusing to rewrite it", reporter) + self.assertIn("if ! current_body=$(gh api", reporter) + def test_the_approval_record_survives_the_next_run(self): reporter = ( REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" @@ -1081,7 +1142,7 @@ def test_the_approval_record_survives_the_next_run(self): # forward, the approval would be forgotten by the very next push, which # is the one thing the record exists to prevent. self.assertIn("previous_records", reporter) - self.assertIn("breaking-change-approved: [a-f0-9]+", reporter) + self.assertIn("breaking-change-approved: [a-f0-9]+ run=[0-9]+", reporter) # Only an approval writes a new record. self.assertIn('if [ "$STATE" = "approved" ] && [ -n "$FINGERPRINT" ]', reporter) From be6797454c7ce51e4171043388b5df12c46aaad0 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 11:22:04 -0300 Subject: [PATCH 17/47] fix(pr-validation): make the approval record usable and honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three from the second CodeRabbit review on #914. The author filter dropped `github-actions[bot]`, for which GitHub can report `author_association: NONE`. That is the identity the reporter writes under whenever no MANAGE_TOKEN is passed — most callers — so the filter I added to close the forgery hole quietly disabled the feature for them. Accepted by exact login now: nobody but Actions can author under it, so it costs nothing. Only the first record was checked. Runs are deleted on the repository's retention schedule, so the oldest record for a fingerprint goes unverifiable first while a newer one is still good — and the lookup would ask for an approval that was on file. Every record is tried now. And a reused approval was writing a record naming the current run, whose Environment approval never happened. That record asserts what no log can confirm, and accumulates with each reuse until the only records left are the unverifiable ones. A record is written only when `needs.approval` actually succeeded. --- .../workflows/breaking-change-approval.yml | 37 +++++++++++++++---- docs/breaking-change-approval.md | 13 ++++++- .../breaking-change-guard/test-workflow.py | 33 +++++++++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 8211ed4a..902a5f8e 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -410,18 +410,32 @@ jobs: # exit on the first match while `--paginate` is still writing, and the # broken pipe would read as "no match" — asking for an approval that # was already given. + # `github-actions[bot]` is accepted by login because GitHub can report + # `author_association: NONE` for it — and that is the identity the + # reporter writes under whenever no MANAGE_TOKEN is passed, which is + # most callers. Filtering it out would quietly disable the whole + # feature for them. It is not a weakening: nobody but Actions can + # author a comment under that login. records=$( gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ --jq '.[] | select(.author_association == "OWNER" - or .author_association == "MEMBER") + or .author_association == "MEMBER" + or .user.login == "github-actions[bot]") | .body' 2>/dev/null \ | grep -oE "" \ || true ) - if [ -n "$records" ]; then - approving_run=$(head -n1 <<< "$records" | grep -oE 'run=[0-9]+' | cut -d= -f2) + # Every record, not just the first. The reporter keeps them all, and + # runs are deleted on the repository's retention schedule — 90 days by + # default — so the oldest record for a fingerprint is the first to + # become unverifiable while a newer one is still perfectly good. + # Stopping at the first would ask for an approval that is on file. + while IFS= read -r record; do + [ -z "$record" ] && continue + approving_run=$(grep -oE 'run=[0-9]+' <<< "$record" | cut -d= -f2) + [ -z "$approving_run" ] && continue # Verified against the Environment's own approval log. Needs # `actions: read`; when that is missing the check cannot be made, so @@ -436,10 +450,15 @@ jobs: if [ -n "$approvers" ]; then already=true - echo "::notice::This breaking change (${FINGERPRINT}) was approved on run ${approving_run} — not asking again" - else - echo "::warning::A record claims run ${approving_run} approved ${FINGERPRINT}, but that approval could not be verified. Asking again." + echo "::notice::This breaking change (${FINGERPRINT}) was approved on run ${approving_run} by ${approvers} — not asking again" + break fi + + echo "::notice::A record names run ${approving_run} for ${FINGERPRINT}, but its approval could not be verified — trying any others" + done <<< "$records" + + if [ -n "$records" ] && [ "$already" != "true" ]; then + echo "::warning::No recorded approval of ${FINGERPRINT} could be verified against the Environment log. Asking again." fi echo "already-approved=$already" >> "$GITHUB_OUTPUT" @@ -706,7 +725,11 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} pr-number: ${{ needs.detect.outputs.pr-number }} state: ${{ steps.verdict.outputs.state }} - fingerprint: ${{ needs.detect.outputs.fingerprint }} + # 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 + # something no log can confirm — then outlive the real one. + fingerprint: ${{ needs.approval.result == 'success' && needs.detect.outputs.fingerprint || '' }} run-id: ${{ github.run_id }} reason: ${{ needs.detect.outputs.reason }} head-sha: ${{ needs.detect.outputs.head-sha }} diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 81c101a0..2e417324 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -294,10 +294,21 @@ record on its own is therefore something a stranger can write. So two things must hold before an approval is reused: 1. the record sits in a comment from an **owner or member** of the - organisation, which is where the reporter writes + organisation, or from `github-actions[bot]` — the identity the reporter + writes under when no `MANAGE_TOKEN` is passed, and one nobody but Actions + can author as 2. the **run it names really was approved** for the `breaking-approval` Environment, read from the Environment's own approval log +Every record for the fingerprint is tried, not only the first: runs are deleted +on the repository's retention schedule, so the oldest record goes unverifiable +while a newer one is still good. + +Only a run whose Environment approval actually happened writes a record. A +reused approval reports `approved` with the gate job skipped, and a record +naming *that* run would assert something no log can confirm — then outlive the +real one. + The second is what carries the weight. Writing the text is easy; producing a run whose Environment approval actually happened is not. diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index ac7a1aa4..a64f7f37 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1087,6 +1087,39 @@ def test_a_forged_approval_record_is_not_believed(self): self.assertIn('select(.state == "approved")', step) self.assertIn('index("breaking-approval")', step) + def test_the_default_reporter_identity_is_accepted(self): + step = extract_step( + extract_job(APPROVAL_WORKFLOW, "request"), + "Check for a prior approval of the same change", + ) + # Without MANAGE_TOKEN the reporter writes as `github-actions[bot]`, + # for which GitHub can report `author_association: NONE`. Filtering on + # association alone would drop those comments and quietly disable the + # whole feature for every caller on the default token. + self.assertIn('.user.login == "github-actions[bot]"', step) + + def test_every_record_is_tried_not_just_the_first(self): + step = extract_step( + extract_job(APPROVAL_WORKFLOW, "request"), + "Check for a prior approval of the same change", + ) + # Runs are deleted on the repository's retention schedule, so the oldest + # record for a fingerprint goes unverifiable first while a newer one is + # still good. Stopping at the first would ask for an approval on file. + self.assertIn("while IFS= read -r record", step) + self.assertNotIn("head -n1 <<< \"$records\"", step) + + def test_only_a_real_approval_writes_a_record(self): + job = extract_job(APPROVAL_WORKFLOW, "outcome") + step = extract_step(job, "Report the outcome on the pull request") + # A reused approval reports `approved` with the gate job skipped. A + # record naming that run would assert something no log can confirm, and + # would outlive the real one. + self.assertIn( + "needs.approval.result == 'success' && needs.detect.outputs.fingerprint", + step, + ) + def test_the_prior_lookup_consumes_the_whole_comment_stream(self): step = extract_step( extract_job(APPROVAL_WORKFLOW, "request"), From dd281a1603649788b9170f60987f22f09f3da724 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 11:23:00 -0300 Subject: [PATCH 18/47] fix(pr-validation): repair the assertion the last commit broke MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The author filter grew a third clause, so the closing paren moved to the next line and `test_a_forged_approval_record_is_not_believed` stopped matching. I pushed that commit without rerunning the suite — the test guarding the forgery fix was red on the branch. Also drops a comment left describing a `grep -F ... >/dev/null` that no longer exists; the code captures output instead. A stale comment about a hazard is worse than none, because the next reader checks the comment rather than the code. --- .github/workflows/breaking-change-approval.yml | 8 ++++---- src/validate/breaking-change-guard/test-workflow.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 902a5f8e..a792271e 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -406,10 +406,10 @@ jobs: # The second is the one that matters: forging the text is easy, # producing a run whose Environment approval actually happened is not. # - # `grep -F ... >/dev/null`, not `grep -qF`: with `pipefail`, `-q` can - # exit on the first match while `--paginate` is still writing, and the - # broken pipe would read as "no match" — asking for an approval that - # was already given. + # No `grep -q` anywhere below: with `pipefail`, it can exit on the + # first match while `--paginate` is still writing, and the broken pipe + # would read as "no match" — asking for an approval already given. + # # `github-actions[bot]` is accepted by login because GitHub can report # `author_association: NONE` for it — and that is the identity the # reporter writes under whenever no MANAGE_TOKEN is passed, which is diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index a64f7f37..f6d52afb 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1081,7 +1081,7 @@ def test_a_forged_approval_record_is_not_believed(self): # from public data — so the record alone is writable by a stranger. # Two independent conditions must hold before an approval is reused. self.assertIn('select(.author_association == "OWNER"', step) - self.assertIn('or .author_association == "MEMBER")', step) + self.assertIn('or .author_association == "MEMBER"', step) # And the run it names has to carry a real Environment approval. self.assertIn("actions/runs/${approving_run}/approvals", step) self.assertIn('select(.state == "approved")', step) From b2c8ad1962e493c196b26946cce91bffad4db98f Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:58:42 -0300 Subject: [PATCH 19/47] fix(pr-validation): hold the prior-approval lookup to the dry_run contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #914. The repository's rule is that a real run stays silent beyond one summary line, and this step printed a notice per unverifiable record on top of its verdict. The other half of that rule was broken too, and was not flagged: the step carried `&& !inputs.dry_run`, so it never ran under dry_run at all. A preview that cannot say whether the approval would be reused is not previewing the decision this gate turns on. It only reads, so it now runs either way. Real run: one `::notice::` when an approval is reused, one `::warning::` when records exist but none verify — naming the runs it tried, which is the thing someone debugging needs — and nothing when there is no record. Dry run: the fingerprint, how many records were found, which runs failed to verify, and the resulting decision. --- .../workflows/breaking-change-approval.yml | 27 +++++++++++++---- .../breaking-change-guard/test-workflow.py | 29 +++++++++++++++++++ 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index a792271e..ebd7156e 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -377,20 +377,25 @@ jobs: # the second time costs attention, the fifth time gets a reflex click. - name: Check for a prior approval of the same change id: prior + # Runs under dry_run too: it only reads, and a dry run that cannot say + # whether the approval would be reused is not previewing the decision + # that matters most here. if: >- needs.detect.result == 'success' && needs.detect.outputs.has-breaking-changes == 'true' && needs.detect.outputs.fingerprint != '' - && !inputs.dry_run env: GH_TOKEN: ${{ secrets.MANAGE_TOKEN || github.token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ needs.detect.outputs.pr-number }} FINGERPRINT: ${{ needs.detect.outputs.fingerprint }} + DRY_RUN: ${{ inputs.dry_run && 'true' || 'false' }} run: | set -euo pipefail already=false + approved_by="" + unverified="" # Two things have to be true, because the comment alone is not # evidence. Anyone who can read the repository can comment on a pull @@ -450,15 +455,27 @@ jobs: if [ -n "$approvers" ]; then already=true - echo "::notice::This breaking change (${FINGERPRINT}) was approved on run ${approving_run} by ${approvers} — not asking again" + approved_by="$approvers" break fi - echo "::notice::A record names run ${approving_run} for ${FINGERPRINT}, but its approval could not be verified — trying any others" + unverified="${unverified:+$unverified }${approving_run}" + [ "$DRY_RUN" = "true" ] && echo " record run=${approving_run} : approval could not be verified" done <<< "$records" - if [ -n "$records" ] && [ "$already" != "true" ]; then - echo "::warning::No recorded approval of ${FINGERPRINT} could be verified against the Environment log. Asking again." + # One line on a real run, per the dry_run contract. Everything else is + # for the preview, where being able to predict the decision is the + # entire point. + if [ "$DRY_RUN" = "true" ]; then + echo "::notice::DRY RUN — approval reuse lookup" + echo " fingerprint : ${FINGERPRINT}" + echo " records found : $(grep -c . <<< "$records" || true)" + echo " unverified runs : [${unverified:-}]" + echo " already approved : ${already}${approved_by:+ (by ${approved_by})}" + elif [ "$already" = "true" ]; then + echo "::notice::This breaking change was approved on run ${approving_run} by ${approved_by} — not asking again" + elif [ -n "$records" ]; then + echo "::warning::A recorded approval of ${FINGERPRINT} exists but could not be verified against the Environment log (runs: ${unverified:-none}). Asking again." fi echo "already-approved=$already" >> "$GITHUB_OUTPUT" diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index f6d52afb..e5ee272f 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1098,6 +1098,35 @@ def test_the_default_reporter_identity_is_accepted(self): # whole feature for every caller on the default token. self.assertIn('.user.login == "github-actions[bot]"', step) + def test_the_prior_lookup_honours_the_dry_run_contract(self): + step = extract_step( + extract_job(APPROVAL_WORKFLOW, "request"), + "Check for a prior approval of the same change", + ) + # The repository's contract: a real run is silent beyond one summary, + # a dry run prints the resolved values. Both halves matter here — + # a preview that cannot say whether the approval would be reused is + # not previewing the decision this gate turns on. + self.assertIn('DRY_RUN: ${{ inputs.dry_run', step) + self.assertNotIn("&& !inputs.dry_run", step) + + code = "\n".join( + line for line in step.splitlines() if not line.strip().startswith("#") + ) + # Every diagnostic beyond the single summary sits behind the dry-run + # branch; count them to keep a future one from slipping out. + for line in code.splitlines(): + if "echo " not in line or "GITHUB_OUTPUT" in line: + continue + stripped = line.strip() + guarded = ( + stripped.startswith('[ "$DRY_RUN" = "true" ] &&') + or "::notice::" in stripped + or "::warning::" in stripped + or stripped.startswith('echo " ') + ) + self.assertTrue(guarded, f"ungated diagnostic on a real run: {stripped}") + def test_every_record_is_tried_not_just_the_first(self): step = extract_step( extract_job(APPROVAL_WORKFLOW, "request"), From 4a06ad2bc9cbce5be75edd80b81d0a7295465383 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 15:42:43 -0300 Subject: [PATCH 20/47] docs(pr-validation): state the continue-on-error exception to the dry_run rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #913, which flagged the ::warning:: this PR adds as a breach of "dry_run: false must be silent". Keeping the warning: the rule's own wording is "let failures surface via exit codes", and a continue-on-error step has deliberately given that channel up. With no exit code and no annotation a failure is invisible — which is precisely how the 403 this PR fixes ran for two months with every job green. Twenty-two steps already in this repository annotate failures on a real run, so the practice predates the question. What the review did expose is that the rule never says any of this. It does now, with the case that proves it. --- .agents/skills/workflow/SKILL.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.agents/skills/workflow/SKILL.md b/.agents/skills/workflow/SKILL.md index d48ed4bd..552a3c91 100644 --- a/.agents/skills/workflow/SKILL.md +++ b/.agents/skills/workflow/SKILL.md @@ -211,6 +211,21 @@ The two modes have opposite goals — design them accordingly: **Real run (`false`):** no extra `echo`, no debug flags, let failures surface via exit codes, one `::notice::` summary on success at most. +**Exception — `continue-on-error` steps must annotate their own failures.** The +rule above leans on the exit code to carry a failure. A step that deliberately +swallows its exit code has no such channel, so a failure there is invisible: the +job reports success and nobody looks at the log. Emit `::warning::` on the +failure path. + +This is not hypothetical. `pr-validation.yml`'s breaking-change comment took a +403 on every run from 2026-08-06 and reported success each time, because the +step was `continue-on-error` and said nothing. The comment it posts explains to +an author what to add; for two months it never arrived, and the only symptom was +a blocking check failing with no explanation. + +Gate the annotation on the failure itself (`if: steps..outcome == 'failure'`) +so a successful run stays silent. + ## Step section titles When a job has more than one logical group of steps, separate each group with a titled section comment: From c8a971928179287a95050fa186308fe9736e312f Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:14:31 -0300 Subject: [PATCH 21/47] feat(pr-validation): one comment for the breaking change, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported on #906: two bot comments about the same breaking change, one headed "Breaking change guard — ✅ Author acknowledged" and one asking for a decision. The ✅ reads as the approval. It is not: it says the author declared the break, and the small print saying so is small print. The second comment only appeared yesterday, when #913 fixed the 403 that had kept it silent since August. Fixing the permission put a ✅ next to a pending request without my noticing the two would collide. `breaking-change-comment` is removed. The gate's comment already carries the acknowledgement state, with the decision as the headline and the acknowledgement as a row — the hierarchy the two controls actually have. To replace what the removed comment covered, the gate now comments wherever a breaking change is found, not only inside its approval scope. Outside it there is no decision to make, so the comment reports the acknowledgement and the exact line to paste. The waiting comment carries that line too, folded away, since Blocking Checks fails on a missing acknowledgement whether or not an approval is pending. pr-validation now passes the acknowledgement literal to the gate instead of relying on the default to coincide. They were equal by hand across two files; a drift would have had the gate teaching a line that fails the check it was explaining. --- .../workflows/breaking-change-approval.yml | 35 +++++- .github/workflows/pr-validation.yml | 117 ++---------------- docs/pr-validation.md | 13 +- .../README.md | 4 + .../action.yml | 35 +++++- .../breaking-change-guard/test-workflow.py | 84 +++---------- 6 files changed, 109 insertions(+), 179 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index ebd7156e..b36887d6 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -314,9 +314,18 @@ jobs: detection_succeeded=false if [ "$IN_SCOPE" != "true" ]; then - # Nothing was scanned, and nothing needs to be: an out-of-scope pull - # request is a successful no-op, not a failed detection. + # Out of scope means no approval is required — it does not mean + # nothing was found. When the caller already detected, keep that + # answer: the acknowledgement is still enforced everywhere, and the + # author needs to be told which line to add. Scanning here is what + # would be pointless, not knowing. detection_succeeded=true + case "$SUPPLIED_HAS_BREAKING_CHANGES" in + true | false ) + has_breaking_changes="$SUPPLIED_HAS_BREAKING_CHANGES" + [ "$SUPPLIED_ACKNOWLEDGED" = "true" ] && acknowledged=true + ;; + esac elif [ -n "$SUPPLIED_HAS_BREAKING_CHANGES" ]; then # The caller already ran the same composite over the same commits. # Anything but a clean boolean is a wiring mistake, and a wiring @@ -510,6 +519,27 @@ jobs: create-if-missing: ${{ inputs.create_approval_environment && 'true' || 'false' }} reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} + # Out of scope, so no approval is coming — but the acknowledgement is + # enforced on every pull request, and an author who omits it otherwise + # sees Blocking Checks fail with nothing explaining what to add. + - name: Report the acknowledgement requirement + if: >- + needs.detect.result == 'success' + && needs.detect.outputs.in-scope != 'true' + && needs.detect.outputs.has-breaking-changes == 'true' + && !inputs.dry_run + continue-on-error: true + uses: LerianStudio/github-actions-shared-workflows/src/notify/breaking-change-approval-reporter@develop + with: + github-token: ${{ secrets.MANAGE_TOKEN || github.token }} + pr-number: ${{ needs.detect.outputs.pr-number }} + state: acknowledgement + head-sha: ${{ needs.detect.outputs.head-sha }} + acknowledged: ${{ needs.detect.outputs.acknowledged }} + author-acknowledgement: ${{ inputs.author_acknowledgement }} + label: '' + enabled: ${{ inputs.enable_comment && 'true' || 'false' }} + - name: Request the approval on the pull request if: >- needs.detect.result == 'success' @@ -521,6 +551,7 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} pr-number: ${{ needs.detect.outputs.pr-number }} state: waiting + author-acknowledgement: ${{ inputs.author_acknowledgement }} fingerprint: ${{ needs.detect.outputs.fingerprint }} reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} actual-reviewers: ${{ steps.environment.outputs.reviewers }} diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index 36f1c6e1..ac4c5338 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -296,7 +296,17 @@ jobs: uses: ./.github/workflows/breaking-change-approval.yml with: detected_breaking_changes: ${{ needs.breaking-change-guard.result == 'success' && needs.breaking-change-guard.outputs.detection-succeeded == 'true' && needs.breaking-change-guard.outputs.has-breaking-changes || 'unknown' }} + # The acknowledgement state travels with it, so one comment can speak for + # both controls. It replaces `breaking-change-comment`, removed here: two + # bot comments about the same breaking change, one headed by a ✅ for the + # author's acknowledgement and one asking for a decision, read as a + # contradiction. The ✅ was taken for the approval. detected_acknowledged: ${{ needs.breaking-change-guard.outputs.approved }} + # Passed explicitly rather than left to the gate's default. The comment + # tells the author which line to paste, and the detector decides whether + # what they pasted counts — two literals in two files, previously kept + # equal by hand. A drift would have the gate teaching a line that fails. + author_acknowledgement: 'Breaking change acknowledged: I understand that this PR intentionally introduces a breaking change and requires the next release to be a major version.' scope_rules: ${{ inputs.breaking_change_scope_rules }} additional_scope_rules: ${{ inputs.breaking_change_additional_scope_rules }} force_labels: ${{ inputs.breaking_change_force_labels }} @@ -485,113 +495,6 @@ jobs: dry-run: ${{ inputs.dry_run && 'true' || 'false' }} # ----------------- Breaking Change Feedback Comment (best effort) ----------------- - breaking-change-comment: - name: Breaking Change Comment - runs-on: ${{ vars.GENERAL_RUNNERS || inputs.runner_type }} - needs: [breaking-change-guard] - if: always() && github.event_name == 'pull_request' && github.event.pull_request.draft != true && !inputs.dry_run - # `pull-requests: write`, not `issues: write`. The step posts through the - # issue-comments endpoint, but the subject is a pull request — and GitHub - # authorises those as pull requests. `issues: write` covers real issues - # only, so this job spent from 2026-08-06 taking a 403 on every run and - # reporting success, because the step is continue-on-error. - # - # The job-level block narrows the workflow's own `pull-requests: write`, so - # declaring `read` here was not least privilege — it was below what the step - # needs to work at all. - permissions: - pull-requests: write - - steps: - - name: Manage breaking change guard comment - id: comment - continue-on-error: true - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - GUARD_JOB_RESULT: ${{ needs.breaking-change-guard.result }} - DETECTION_SUCCEEDED: ${{ needs.breaking-change-guard.outputs.detection-succeeded }} - HAS_BREAKING_CHANGES: ${{ needs.breaking-change-guard.outputs.has-breaking-changes }} - BREAKING_CHANGE_ACKNOWLEDGED: ${{ needs.breaking-change-guard.outputs.approved }} - EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - ACKNOWLEDGEMENT: 'Breaking change acknowledged: I understand that this PR intentionally introduces a breaking change and requires the next release to be a major version.' - with: - github-token: ${{ github.token }} - script: | - const marker = ''; - const guardJobSucceeded = process.env.GUARD_JOB_RESULT === 'success'; - const detectionSucceeded = process.env.DETECTION_SUCCEEDED === 'true'; - const hasBreakingChanges = process.env.HAS_BREAKING_CHANGES === 'true'; - const acknowledged = process.env.BREAKING_CHANGE_ACKNOWLEDGED === 'true'; - const eventHeadSha = process.env.EVENT_HEAD_SHA; - const acknowledgement = process.env.ACKNOWLEDGEMENT; - - const current = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.issue.number, - }); - if (!eventHeadSha || current.data.head.sha !== eventHeadSha) { - core.warning(`Skipping stale breaking change comment update: event head ${eventHeadSha || 'missing'}, current head ${current.data.head.sha}.`); - return; - } - - const comments = await github.paginate(github.rest.issues.listComments, { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - per_page: 100, - }); - const owned = comments.filter(comment => - comment.user?.login === 'github-actions[bot]' - && (comment.body === marker || comment.body?.startsWith(`${marker}\n`)) - ); - const deleteComment = comment => github.rest.issues.deleteComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: comment.id, - }); - const upsertOwned = async body => { - const params = { owner: context.repo.owner, repo: context.repo.repo, body }; - - if (owned.length > 0) { - await github.rest.issues.updateComment({ ...params, comment_id: owned[0].id }); - await Promise.all(owned.slice(1).map(deleteComment)); - return; - } - - await github.rest.issues.createComment({ - ...params, - issue_number: context.issue.number, - }); - }; - - if (!guardJobSucceeded || !detectionSucceeded) { - const body = `${marker}\n## Breaking change guard\n\n**Status:** ⚠️ Detection failed\n\nThe workflow could not determine whether this pull request contains breaking changes. Blocking Checks failed closed.`; - await upsertOwned(body); - return; - } - - if (!hasBreakingChanges) { - await Promise.all(owned.map(deleteComment)); - return; - } - - const state = acknowledged ? '✅ Author acknowledged' : '🚫 Awaiting author acknowledgement'; - const detail = acknowledged - ? 'The exact visible author acknowledgement line is present in the pull request description. It records intentional awareness and does not grant maintainer permission.' - : 'The PR author can acknowledge intentional awareness by adding this exact visible line to the pull request description:'; - const body = `${marker}\n## Breaking change guard\n\n**Status:** ${state}\n\n${detail}\n\n\`${acknowledgement}\``; - await upsertOwned(body); - - # Cosmetic enough to keep continue-on-error — losing the comment should - # not fail validation — but not so cosmetic that it should fail in - # silence. Swallowing it is what let a 403 run for two months while every - # job reported green. - - name: Surface a failed comment - if: always() && steps.comment.outcome == 'failure' - run: | - echo "::warning::Could not post the breaking change guard comment. The validation verdict is unaffected, but the author gets no explanation of what to add. Check the step log above — a 403 here usually means the job lacks 'pull-requests: write'." - # ----------------- PR Validation Summary Comment ----------------- pr-validation-report: name: PR Validation Report diff --git a/docs/pr-validation.md b/docs/pr-validation.md index af9d86cd..2bb1a90e 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -238,6 +238,18 @@ where that information is worth having. They still skip on drafts, and on anything that is not a `pull_request` event. +## One comment, not two + +The acknowledgement guard used to post its own comment. It no longer does: the +breaking-change gate's comment carries both controls, because two bot comments +about the same breaking change read as a contradiction — one headed by a ✅ for +the author's acknowledgement, one asking for a decision. The ✅ gets taken for +the approval. + +The gate now comments wherever a breaking change is found, including on branches +outside its approval scope. There it reports only the acknowledgement and the +exact line to paste, since that is the only thing being enforced. + ## Breaking change approval The author-acknowledgement guard above asks the author to declare the break. The @@ -286,7 +298,6 @@ Full detail: [`breaking-change-approval.md`](breaking-change-approval.md). | `blocking-checks` | 1 (fail-fast) | guard enforcement, `pr-source-branch`, `pr-title`, `pr-description`, `pr-commit-signatures` | always; drafts run guard enforcement only | | `advisory-checks` | 2 (informational) | `pr-metadata`, `pr-size`, `pr-labels` | non-draft, regardless of the blocking result | | `pr-checks-summary` | — | `pr-checks-summary` | always (writes to step summary) | -| `breaking-change-comment` | — | `actions/github-script` | every non-draft live run, including detector failure or cancellation. Needs `pull-requests: write` — commenting on a PR is authorised as a pull request, not as an issue | | `pr-validation-report` | — | `pr-validation-reporter` | non-draft (upserts single PR comment) | | `notify` | — | `slack-notify.yml` | non-draft, `!dry_run` | diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 2050fcb2..4af911ef 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -20,6 +20,7 @@ comment, and it ends as the record of who decided what — nothing is deleted. | `state` | What the comment says | |---|---| | `waiting` | A breaking change was found; here is the link to the pending deployment, and what approving or rejecting will do | +| `acknowledgement` | A breaking change was found on a branch outside the approval scope — no decision is needed, only the author's acknowledgement, with the exact line to paste | | `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 | @@ -99,6 +100,9 @@ side effect in the caller is what makes this safe to run with | `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` | +| `author-acknowledgement` | The exact line the author must add when it is missing. | No | `''` | +| `fingerprint` | Digest of the breaking-change commit set. Recorded when the state is `approved`, with the run that carried the approval. | No | `''` | +| `run-id` | Run recorded alongside the fingerprint, so a later run can verify the approval against the Environment log. | No | `${{ github.run_id }}` | | `run-url` | Link to the run carrying the pending deployment. | No | `''` | | `reviewers` | Who can answer, as a `team:` / `user:` list. | No | `''` | | `actual-reviewers` | Reviewers read from the Environment itself. Preferred over `reviewers` when present. | No | `''` | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 8ee8a41c..3349ed7c 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -27,6 +27,10 @@ inputs: description: Whether the author acknowledgement line is present in the description (true/false). Shown to the reviewer as context. required: false default: 'false' + author-acknowledgement: + description: 'The exact line the author must add to the description. Shown when it is missing — a check that fails without saying what to add is a check people route around.' + required: false + default: '' run-url: description: Link to the run carrying the pending deployment. required: false @@ -103,6 +107,7 @@ runs: REASON: ${{ inputs.reason }} HEAD_SHA: ${{ inputs.head-sha }} ACKNOWLEDGED: ${{ inputs.acknowledged }} + ACK_LINE: ${{ inputs.author-acknowledgement }} RUN_URL: ${{ inputs.run-url }} REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-reviewers || inputs.reviewers }} SERVER_URL: ${{ inputs.server-url }} @@ -120,9 +125,9 @@ runs: set -euo pipefail case "$STATE" in - waiting | approved | rejected | unresolved ) ;; + waiting | approved | rejected | unresolved | acknowledgement ) ;; * ) - echo "state must be one of: waiting, approved, rejected, unresolved; got '${STATE}'." >&2 + echo "state must be one of: waiting, approved, rejected, unresolved, acknowledgement; got '${STATE}'." >&2 exit 1 ;; esac @@ -205,6 +210,15 @@ runs: printf '| %s **Author acknowledgement** | %s |\n\n' \ "$([ "$ACKNOWLEDGED" = "true" ] && echo "✅" || echo "⬜")" \ "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" + + # Blocking Checks fails on a missing acknowledgement whether or + # not an approval is pending, so the line to add belongs here too. + if [ "$ACKNOWLEDGED" != "true" ] && [ -n "$ACK_LINE" ]; then + printf '
\nBlocking Checks also needs the author acknowledgement\n\n' + printf 'Add this exact line to the pull request description:\n\n' + printf '```\n%s\n```\n\n' "$ACK_LINE" + printf '
\n\n' + fi ;; approved) printf '> [!NOTE]\n' @@ -226,6 +240,23 @@ runs: fi printf '\n' ;; + acknowledgement) + # No approval is required here — only the author's own + # acknowledgement, which Blocking Checks enforces everywhere. + if [ "$ACKNOWLEDGED" = "true" ]; then + printf '> [!NOTE]\n' + printf '> ## 💥 Breaking change acknowledged\n' + printf '>\n' + printf '> The author has declared this break in the description. No approval is required on this branch.\n\n' + else + printf '> [!IMPORTANT]\n' + printf '> ## 💥 Breaking change — acknowledgement missing\n' + printf '>\n' + printf '> This pull request carries a breaking change, so the next release becomes a **major** version. Add this exact line to the description:\n' + printf '>\n' + printf '> ```\n> %s\n> ```\n\n' "$ACK_LINE" + fi + ;; unresolved) printf '> [!WARNING]\n' printf '> ## 💥 Breaking change — decision not recorded\n' diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 04ae60fc..0f1c6ce5 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -240,6 +240,9 @@ def render_reporter(state, **overrides): "REASON": "rule develop <- '*'", "HEAD_SHA": "abc1234567", "ACKNOWLEDGED": "false", + "ACK_LINE": "Breaking change acknowledged: I understand that this PR " + "intentionally introduces a breaking change and requires the next " + "release to be a major version.", "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", "REVIEWERS": "team:devops", "SERVER_URL": "https://github.com", @@ -644,22 +647,30 @@ def test_guard_uses_fixed_author_acknowledgement_exact_mode_and_released_action( self.assertIn("does not grant maintainer permission", outputs) def test_detector_and_comment_acknowledgement_literals_are_identical(self): + # One literal tells the author what to paste, the other decides whether + # what they pasted counts. A drift between them leaves the gate teaching + # a line that fails the check it is explaining. detector_step = extract_step( extract_job(WORKFLOW, "breaking-change-guard"), "Detect breaking changes" ) - comment_job = extract_job(WORKFLOW, "breaking-change-comment") detector_literal = re.search( r"(?m)^\s*breaking-change-acknowledgement: '(?P[^']*)'$", detector_step, ) - comment_literal = re.search( - r"(?m)^\s*ACKNOWLEDGEMENT: '(?P[^']*)'$", comment_job - ) self.assertIsNotNone(detector_literal) - self.assertIsNotNone(comment_literal) self.assertEqual(ACKNOWLEDGEMENT, detector_literal.group("value")) + + # The gate is handed the same literal explicitly — relying on its + # default to coincide is what made these drift-prone in the first place. + gate_job = extract_job(WORKFLOW, "breaking-change-approval") + gate_literal = re.search( + r"(?m)^\s*author_acknowledgement: '(?P[^']*)'$", gate_job + ) + self.assertIsNotNone( + gate_literal, "pr-validation must pass the acknowledgement explicitly" + ) self.assertEqual( - detector_literal.group("value"), comment_literal.group("value") + detector_literal.group("value"), gate_literal.group("value") ) def test_guard_checkout_has_full_history_without_persisted_credentials(self): @@ -686,58 +697,6 @@ def test_draft_pull_requests_still_reach_guard_enforcement(self): self.assertRegex(enforcement, r"(?m)^ if: always\(\)$") self.assertIn("github.event.pull_request.draft != true", collection) - def test_comment_job_is_write_minimal_and_does_not_checkout(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - permissions = yaml.safe_load(WORKFLOW)["jobs"]["breaking-change-comment"][ - "permissions" - ] - # `pull-requests: write` is the minimum that works, not a widening. - # This set used to be {pull-requests: read, issues: write}, which looks - # narrower and is simply wrong: commenting on a pull request is - # authorised as a pull request, so the step took a 403 on every run - # from 2026-08-06 and reported success anyway. - # - # The lesson is in the shape of the old assertion, not just its value — - # it pinned a permission set nobody had watched work, and so kept the - # bug in place rather than catching it. - self.assertEqual({"pull-requests": "write"}, permissions) - self.assertNotIn("contents", permissions) - self.assertNotIn("actions/checkout", job) - self.assertIn("github-token: ${{ github.token }}", job) - - def test_comment_job_owns_only_exact_bot_marker_comments(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - self.assertIn("comment.user?.login === 'github-actions[bot]'", job) - self.assertIn("comment.body === marker", job) - self.assertIn("comment.body?.startsWith(`${marker}\\n`)", job) - self.assertNotIn("comment.body?.includes(marker)", job) - self.assertEqual(2, job.count("const body = `${marker}\\n")) - - def test_comment_job_uses_author_acknowledgement_not_authorization(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - self.assertIn("'✅ Author acknowledged'", job) - self.assertIn("'🚫 Awaiting author acknowledgement'", job) - self.assertIn("does not grant maintainer permission", job) - self.assertNotIn("'✅ Approved'", job) - self.assertNotIn("'🚫 Awaiting approval'", job) - - def test_comment_job_rejects_stale_head_updates(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - self.assertIn("EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }}", job) - self.assertIn("github.rest.pulls.get", job) - self.assertIn("current.data.head.sha !== eventHeadSha", job) - - def test_comment_job_reconciles_detector_failure(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - self.assertIn("if (!guardJobSucceeded || !detectionSucceeded)", job) - self.assertIn("**Status:** ⚠️ Detection failed", job) - self.assertIn("Blocking Checks failed closed.", job) - failure_branch = job.index("if (!guardJobSucceeded || !detectionSucceeded)") - no_breaking_branch = job.index("if (!hasBreakingChanges)") - self.assertIn( - "await upsertOwned(body);", job[failure_branch:no_breaking_branch] - ) - def test_reporter_and_summary_receive_guard_and_blocking_runtime_results(self): for job_id, step_name in ( ("pr-checks-summary", "PR Checks Summary"), @@ -877,15 +836,6 @@ def test_jobs_that_comment_on_a_pull_request_can_write_to_it(self): f"{job_id} comments on a pull request and would take a 403", ) - def test_a_failed_comment_is_visible_rather_than_swallowed(self): - job = extract_job(WORKFLOW, "breaking-change-comment") - # continue-on-error is right for a cosmetic comment, but silence is how - # a 403 ran for two months with every job green. - self.assertIn("continue-on-error: true", job) - self.assertIn("::warning::", job) - self.assertIn("steps.comment.outcome == 'failure'", job) - - class AdvisoryChecksTests(unittest.TestCase): def test_advisory_checks_do_not_cascade_off_a_blocking_failure(self): job = extract_job(WORKFLOW, "advisory-checks") From 121604a89bcf6821a254737e4fa656d055f030a9 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:27:28 -0300 Subject: [PATCH 22/47] refactor(pr-validation): detect through the API, check out nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL flagged `actions/untrusted-checkout` at the second checkout, and the alert was fair. The gate runs on `pull_request`, so it is not privileged — but it was still placing a contributor's tree on disk in a job that carries MANAGE_TOKEN. Nothing executed that tree, and nothing needed it: the only thing read from it was commit messages, which the API serves. The `breaking-change-commits` composite was already running here for the fingerprint, reading exactly those commits through the API with patterns kept identical to the detector. It now does the detecting too, when the caller has not detected already. Two checkouts, the Git-reading composite, the tree, the token exposure and the alert all go together. `breaking-change-guard` remains the canonical detector and still runs in pr-validation over the real commit range — which is the verdict this gate is handed in the path that matters. One consequence worth stating: on the standalone path the acknowledgement is no longer evaluated, because the detector that reads the description lives in pr-validation. The state is reported as unknown rather than missing, and the comment omits the row. Telling an author to add a line that no check is asking for is worse than saying nothing. --- .../workflows/breaking-change-approval.yml | 71 ++++++------------- docs/breaking-change-approval.md | 19 +++-- .../action.yml | 16 +++-- .../breaking-change-guard/test-workflow.py | 17 +++++ 4 files changed, 65 insertions(+), 58 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index b36887d6..25cf40e2 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -246,49 +246,25 @@ jobs: echo "::notice::PR #${PR_NUMBER} (${HEAD_REF} → ${BASE_REF}) is not gated — ${REASON}" fi - # The composite reads Git history, so it needs `origin/` and a HEAD - # that is the pull request. The second checkout exists because the first - # one leaves HEAD on the event's commit, which is only the pull request - # under `pull_request`. `clean: false` keeps what the first one fetched. - - name: Check out full history - id: checkout - if: steps.scope.outputs.in-scope == 'true' && inputs.detected_breaking_changes == '' - continue-on-error: true - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v6 - with: - fetch-depth: 0 - persist-credentials: false - - - name: Move HEAD onto the pull request head - id: checkout-head - if: steps.scope.outputs.in-scope == 'true' && steps.checkout.outcome == 'success' - continue-on-error: true - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v6 - with: - ref: refs/pull/${{ steps.scope.outputs.pr-number }}/head - fetch-depth: 0 - clean: false - persist-credentials: false - - # Nothing from the checked-out tree is executed — the composite only reads - # commit messages — so fork code never runs here. - - name: Detect breaking changes - id: guard - if: steps.scope.outputs.in-scope == 'true' && steps.checkout-head.outcome == 'success' && inputs.detected_breaking_changes == '' - continue-on-error: true - uses: LerianStudio/github-actions-shared-workflows/src/validate/breaking-change-guard@v1 - with: - base-ref: ${{ steps.scope.outputs.base-ref }} - breaking-change-acknowledgement: ${{ inputs.author_acknowledgement }} - acknowledgement-match-mode: exact-visible-line - # Which commits tripped the gate, and a digest of the set. The digest is # what lets an approval survive a push that did not touch the breaking # change — see the `request` job. + # + # This also detects, when the caller did not. It replaced a pair of + # checkouts and the Git-reading composite: CodeQL flagged them as + # `actions/untrusted-checkout`, and it was right to — a non-privileged + # workflow was putting a contributor's tree on disk beside MANAGE_TOKEN. + # Nothing executed it, but nothing needed it either. Reading the API + # removes the tree, the token exposure and the alert together. + # + # `breaking-change-guard` stays the canonical detector and still runs in + # pr-validation over the real commit range; the patterns here are kept + # identical to it and tested against the same negative cases. - name: List the breaking-change commits id: commits if: steps.scope.outputs.in-scope == 'true' continue-on-error: true + # No checkout: it reads the pull request through the API. uses: LerianStudio/github-actions-shared-workflows/src/validate/breaking-change-commits@develop with: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} @@ -301,11 +277,8 @@ jobs: IN_SCOPE: ${{ steps.scope.outputs.in-scope }} SUPPLIED_HAS_BREAKING_CHANGES: ${{ inputs.detected_breaking_changes }} SUPPLIED_ACKNOWLEDGED: ${{ inputs.detected_acknowledged }} - CHECKOUT_OUTCOME: ${{ steps.checkout.outcome }} - CHECKOUT_HEAD_OUTCOME: ${{ steps.checkout-head.outcome }} - GUARD_OUTCOME: ${{ steps.guard.outcome }} - RAW_HAS_BREAKING_CHANGES: ${{ steps.guard.outputs.has-breaking-changes }} - RAW_ACKNOWLEDGED: ${{ steps.guard.outputs.approved }} + COMMITS_OUTCOME: ${{ steps.commits.outcome }} + BREAKING_COUNT: ${{ steps.commits.outputs.count }} run: | set -euo pipefail @@ -345,15 +318,17 @@ jobs: echo "detected_breaking_changes must be 'true', 'false', 'unknown' or empty; got '${SUPPLIED_HAS_BREAKING_CHANGES}'." >&2 ;; esac - elif [ "$CHECKOUT_OUTCOME" = "success" ] && [ "$CHECKOUT_HEAD_OUTCOME" = "success" ] \ - && [ "$GUARD_OUTCOME" = "success" ] \ - && { [ "$RAW_HAS_BREAKING_CHANGES" = "true" ] || [ "$RAW_HAS_BREAKING_CHANGES" = "false" ]; } \ - && { [ "$RAW_ACKNOWLEDGED" = "true" ] || [ "$RAW_ACKNOWLEDGED" = "false" ]; }; then - has_breaking_changes="$RAW_HAS_BREAKING_CHANGES" - acknowledged="$RAW_ACKNOWLEDGED" + elif [ "$COMMITS_OUTCOME" = "success" ] && [ -n "$BREAKING_COUNT" ]; then + [ "$BREAKING_COUNT" -gt 0 ] && has_breaking_changes=true detection_succeeded=true + # Left empty on purpose, not false. Without the caller's verdict + # the acknowledgement was never evaluated here — the detector that + # reads the description lives in pr-validation — and reporting + # "missing" for something nobody looked at would send the author + # after a line no check is asking for. + acknowledged="" else - echo "Breaking change detection failed: checkout=${CHECKOUT_OUTCOME} checkout_head=${CHECKOUT_HEAD_OUTCOME} guard=${GUARD_OUTCOME}" >&2 + echo "Breaking change detection failed: commits=${COMMITS_OUTCOME} count=${BREAKING_COUNT:-missing}" >&2 fi { diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 2e417324..be87e051 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -52,7 +52,7 @@ own: | Composite | Responsibility | |---|---| | [`pr-gate-scope`](../src/validate/pr-gate-scope/README.md) | Is this pull request in scope, and why — base → head rules, label overrides, author vetoes | -| [`breaking-change-guard`](../src/validate/breaking-change-guard/README.md) | Does the branch carry a breaking-change commit | +| [`breaking-change-guard`](../src/validate/breaking-change-guard/README.md) | The canonical detector, over the real commit range. Runs in `pr-validation.yml`, whose verdict this gate is handed | | [`breaking-change-commits`](../src/validate/breaking-change-commits/README.md) | Which commits carry the break, and a digest of the set | | [`codeowners-reviewers`](../src/validate/codeowners-reviewers/README.md) | Who owns this repository, per its CODEOWNERS | | [`environment-protection`](../src/validate/environment-protection/README.md) | Does `breaking-approval` exist with required reviewers — creating it when it does not | @@ -177,10 +177,19 @@ jobs: MANAGE_TOKEN: ${{ secrets.MANAGE_TOKEN }} ``` -Called this way the workflow detects on its own, with two checkouts: a plain one -for the `origin/` remote-tracking refs, then `refs/pull//head` with -`clean: false` to move `HEAD` onto the pull request. Nothing from the tree is -executed — only commit messages are read — so fork code never runs. +Called this way the workflow detects on its own, through the API — it checks +out nothing at all. + +It used to use two checkouts and read Git directly. CodeQL flagged that as +`actions/untrusted-checkout`, and the alert was fair: a non-privileged workflow +was placing a contributor's tree on disk next to `MANAGE_TOKEN`. Nothing ever +executed it, but nothing needed it either, and the tree, the token exposure and +the alert all went away together. + +The acknowledgement is **not** evaluated on this path — the detector that reads +the pull request description runs in `pr-validation.yml`. The comment omits that +row rather than reporting it missing, because telling an author to add a line no +check is asking for is worse than saying nothing. ### Testing before adoption diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 3349ed7c..e6412c68 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -24,7 +24,7 @@ inputs: required: false default: '' acknowledged: - description: Whether the author acknowledgement line is present in the description (true/false). Shown to the reviewer as context. + description: 'Whether the author acknowledgement line is present in the description — `true`, `false`, or empty when nobody evaluated it. Empty omits the row entirely: reporting "missing" for something unchecked sends the author after a line no check is asking for.' required: false default: 'false' author-acknowledgement: @@ -207,13 +207,19 @@ runs: printf '| | |\n|:--|:--|\n' [ -n "$commit_link" ] && printf '| 💥 **Breaking commit** | %s |\n' "$commit_link" [ -n "$mentions" ] && printf '| 👤 **Who can decide** | %s |\n' "$mentions" - printf '| %s **Author acknowledgement** | %s |\n\n' \ - "$([ "$ACKNOWLEDGED" = "true" ] && echo "✅" || echo "⬜")" \ - "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" + # Empty means nobody evaluated it — only the caller that enforces + # the acknowledgement can say. Printing "missing" there would send + # the author after a line no check is asking for. + if [ -n "$ACKNOWLEDGED" ]; then + printf '| %s **Author acknowledgement** | %s |\n' \ + "$([ "$ACKNOWLEDGED" = "true" ] && echo "✅" || echo "⬜")" \ + "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" + fi + printf '\n' # Blocking Checks fails on a missing acknowledgement whether or # not an approval is pending, so the line to add belongs here too. - if [ "$ACKNOWLEDGED" != "true" ] && [ -n "$ACK_LINE" ]; then + if [ "$ACKNOWLEDGED" = "false" ] && [ -n "$ACK_LINE" ]; then printf '
\nBlocking Checks also needs the author acknowledgement\n\n' printf 'Add this exact line to the pull request description:\n\n' printf '```\n%s\n```\n\n' "$ACK_LINE" diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 0f1c6ce5..d731c86c 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1202,6 +1202,23 @@ def test_the_approval_record_survives_the_next_run(self): # Only an approval writes a new record. self.assertIn('if [ "$STATE" = "approved" ] && [ -n "$FINGERPRINT" ]', reporter) + def test_the_gate_checks_out_nothing(self): + # CodeQL flagged `actions/untrusted-checkout` here, and it was right: + # a non-privileged workflow was putting a contributor's tree on disk + # beside MANAGE_TOKEN. Nothing executed it, but nothing needed it — + # the commits are readable through the API. + self.assertNotIn("actions/checkout", APPROVAL_WORKFLOW) + + def test_an_unevaluated_acknowledgement_is_not_reported_as_missing(self): + # The detector that reads the description lives in pr-validation. Where + # it did not run, the state is unknown — and "missing" would send the + # author after a line no check is asking for. + rendered = render_reporter("waiting", ACKNOWLEDGED="") + self.assertNotIn("Author acknowledgement", rendered) + rendered = render_reporter("waiting", ACKNOWLEDGED="false") + self.assertIn("Author acknowledgement", rendered) + self.assertIn("missing", rendered) + def test_the_fingerprint_comes_from_the_breaking_commits(self): detect = extract_job(APPROVAL_WORKFLOW, "detect") self.assertIn("src/validate/breaking-change-commits@", detect) From 6fda01b233f1c0620edf2d1587570cc17efa8b29 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:41:22 -0300 Subject: [PATCH 23/47] docs(pr-validation): drop the description of a job that no longer exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #918. "Feedback comments" still described, in ten lines of detail, how the acknowledgement guard creates, updates and deletes its comment — a job this PR removes. It sat directly above the section explaining that the gate owns the single comment, so the file contradicted itself. This is the same mistake I flagged and fixed on #911: adding a section for the new behaviour and leaving the old description in place, where whoever reads it believes it. Also lists `acknowledgement` in the `state` input description. The README already documented it; the action's own contract did not. --- docs/pr-validation.md | 15 +++++++-------- .../breaking-change-approval-reporter/action.yml | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/docs/pr-validation.md b/docs/pr-validation.md index 2bb1a90e..9dbb5dbd 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -210,14 +210,13 @@ The guard is mandatory on every target branch. There is no opt-out. The action s ### Feedback comments -For every non-draft live run, including detector failure or cancellation, the workflow uses `github.token` to manage one best-effort comment owned only when the author is exactly `github-actions[bot]` and the body starts with the exact marker line: - -- A breaking change creates or updates the comment with `Author acknowledged` or `Awaiting author acknowledgement` state and the exact acknowledgement. -- Detection or guard-job failure replaces stale acknowledgement with one `Detection failed` comment and deletes duplicate owned comments. -- Duplicate owned marker comments are deleted. -- When breaking changes disappear, all owned guard comments are deleted. -- Before any comment mutation, the workflow fetches the current PR and stops if its current head SHA differs from the event head SHA. -- Fork token write restrictions and comment API failures never affect enforcement. The workflow never falls back to another bot identity. +The guard posts nothing of its own. Everything an author needs to read about a +breaking change — including the exact acknowledgement line to paste — lives in +the single comment described under [One comment, not two](#one-comment-not-two), +written by the breaking-change gate. + +Enforcement never depends on that comment: a comment API failure leaves the +verdict untouched, and the author still sees `Blocking Checks` fail. ### Default PR Title Types diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index e6412c68..977bddf3 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -13,7 +13,7 @@ inputs: required: false default: ${{ github.repository }} state: - description: "What to report — `waiting`, `approved`, `rejected`, or `unresolved` when the approval ended without a recorded decision. Any other value fails the step." + description: "What to report — `waiting`, `approved`, `rejected`, `unresolved` when the approval ended without a recorded decision, or `acknowledgement` where no approval is required and only the author's acknowledgement is at stake. Any other value fails the step." required: true reason: description: Why the pull request is gated, as resolved by `pr-gate-scope`. From 4590dfe2e9cbffecc7204067ec3ab30842b2657e Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:59:30 -0300 Subject: [PATCH 24/47] docs(pr-validation): stop promising a failure that does not always happen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #918. The sentence said a comment API failure leaves the verdict untouched "and the author still sees Blocking Checks fail" — but an acknowledged breaking change passes it, as the same file states four paragraphs earlier and `pr-validation.yml:266` implements. The point was that enforcement does not depend on the comment. It now says that, without inventing a consequence that holds in one case out of two. I wrote the sentence in the previous round, while fixing a different inaccuracy in the same section. --- docs/pr-validation.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/pr-validation.md b/docs/pr-validation.md index 9dbb5dbd..ffcd66c1 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -215,8 +215,9 @@ breaking change — including the exact acknowledgement line to paste — lives the single comment described under [One comment, not two](#one-comment-not-two), written by the breaking-change gate. -Enforcement never depends on that comment: a comment API failure leaves the -verdict untouched, and the author still sees `Blocking Checks` fail. +Enforcement never depends on that comment. A comment API failure leaves the +verdict exactly as it was — passing or failing on the acknowledgement alone, +which is what `Blocking Checks` decides either way. ### Default PR Title Types From 241d656e43d06f82206d9eb54b2205911affcc30 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:23:05 -0300 Subject: [PATCH 25/47] fix(pr-validation): keep the breaking-change label after the approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The label was being removed the moment the change was approved, which gets it backwards: `breaking-change` describes the commits, and the break does not stop being a break because somebody agreed to it. The repository's own labels.yml says as much — "Callers must update their workflow configuration after this change" is a fact about the change, not a step in a workflow. Removing it erased the record exactly when it started to matter: now the break is both real and agreed. Two labels now, expiring at different times: breaking-change a fact. Applied throughout, removed only when the breaking commits are gone awaiting-breaking-approval a state. On while a decision is pending, off the moment one is made, either way Found while implementing this: the cleanup for the fact label did not exist at all. An earlier rewrite of the verdict job dropped it, so a reword that removed the `!` left `breaking-change` on the pull request for good. Restored, and now removing both. awaiting-breaking-approval added to .github/labels.yml. Both are cosmetic — a repository that never declared them still gets its gate. --- .github/labels.yml | 4 +++ .../workflows/breaking-change-approval.yml | 34 +++++++++++++++++-- docs/breaking-change-approval.md | 12 +++++-- .../README.md | 18 +++++++++- .../action.yml | 31 ++++++++++++++--- .../breaking-change-guard/test-workflow.py | 27 +++++++++++++++ 6 files changed, 116 insertions(+), 10 deletions(-) diff --git a/.github/labels.yml b/.github/labels.yml index 8498552c..6e04c717 100644 --- a/.github/labels.yml +++ b/.github/labels.yml @@ -82,6 +82,10 @@ color: "b60205" description: Callers must update their workflow configuration after this change +- name: awaiting-breaking-approval + color: "fbca04" + description: A breaking change on this PR is waiting on a human decision + # ── Capabilities ────────────────────────────────────────────────────────────── - name: build diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 25cf40e2..bd443ec9 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -144,10 +144,23 @@ on: type: boolean default: true label: - description: Label applied while the pull request waits for its approval, and removed once resolved. Cosmetic — failure to apply it never fails the job. Empty disables it. + description: >- + Label stating that this pull request carries a breaking change. A fact + about the change, so it survives the approval — the break does not go + away because somebody agreed to it. Removed only when the breaking + commits are gone. Empty disables it. required: false type: string default: breaking-change + pending_label: + description: >- + Label stating that the breaking change is waiting on a decision. A + state, so it comes off once approved or rejected. Separate from + `label` because the two stop being true at different moments. + Empty disables it. + required: false + type: string + default: awaiting-breaking-approval enable_comment: description: Post (and keep updated) a single comment carrying the request, the approval link and the outcome. required: false @@ -512,7 +525,8 @@ jobs: head-sha: ${{ needs.detect.outputs.head-sha }} acknowledged: ${{ needs.detect.outputs.acknowledged }} author-acknowledgement: ${{ inputs.author_acknowledgement }} - label: '' + label: ${{ inputs.label }} + pending-label: '' enabled: ${{ inputs.enable_comment && 'true' || 'false' }} - name: Request the approval on the pull request @@ -536,6 +550,7 @@ jobs: run-url: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} close-on-rejection: ${{ inputs.close_on_rejection && 'true' || 'false' }} label: ${{ inputs.label }} + pending-label: ${{ inputs.pending_label }} enabled: ${{ inputs.enable_comment && 'true' || 'false' }} # ----------------- The gate itself ----------------- @@ -625,6 +640,8 @@ jobs: APPROVERS: ${{ steps.decisions.outputs.approvers }} REJECTERS: ${{ steps.decisions.outputs.rejecters }} API_READABLE: ${{ steps.decisions.outputs.api-readable }} + LABEL: ${{ inputs.label }} + PENDING_LABEL: ${{ inputs.pending_label }} DRY_RUN: ${{ inputs.dry_run && 'true' || 'false' }} run: | set -euo pipefail @@ -705,6 +722,18 @@ jobs: result=success state=none emit + + # The only moment the break actually stopped existing, so the only + # place the fact label comes off. A reword dropping the `!` left + # `breaking-change` on the pull request for good — the cleanup was + # lost in an earlier rewrite of this job and is restored here. + if [ "$DRY_RUN" != "true" ] && [ -n "${PR_NUMBER:-}" ]; then + for stale in "$LABEL" "$PENDING_LABEL"; do + [ -n "$stale" ] || continue + gh pr edit "$PR_NUMBER" --repo "$REPO" --remove-label "$stale" >/dev/null 2>&1 || true + done + fi + echo "::notice::Nothing to approve on PR #${PR_NUMBER} — ${REASON}" exit 0 fi @@ -763,6 +792,7 @@ jobs: rejection-comment: ${{ steps.decisions.outputs.rejection-comment }} close-on-rejection: ${{ inputs.close_on_rejection && 'true' || 'false' }} label: ${{ inputs.label }} + pending-label: ${{ inputs.pending_label }} enabled: ${{ inputs.enable_comment && 'true' || 'false' }} # Only on a recorded rejection, never on a cancellation. Closing a pull diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index be87e051..942d373b 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -408,8 +408,16 @@ ends as the record of who decided what: > 👉 **[Review the pending deployment](#)** — *Review deployments* → > `breaking-approval`. -The `breaking-change` label is applied while it waits and removed once resolved — -cosmetic, and a repository that never declared it still gets its gate. +Two labels, and they expire at different times. `breaking-change` states a fact +about the commits and **survives the approval** — the break does not stop being a +break because somebody agreed to it, and a search for that label months later +should still find this pull request. It comes off only when the breaking commits +are gone. `awaiting-breaking-approval` states that a decision is pending, and +comes off the moment one is made. + +Both are cosmetic: a repository that never declared them still gets its gate. +Declare them with [`labels-sync`](labels-sync.md), or pass `label: ''` / +`pending_label: ''` to turn them off. ## Inputs diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 4af911ef..0d3cc91c 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -82,6 +82,21 @@ may be gated by different people than the caller configured, and naming the wron ones is worse than naming none — with neither available, the line is omitted rather than guessed. +## Two labels, because they expire at different times + +`breaking-change` is a **fact about the commits**. Approving does not make the +break go away, and somebody filtering for that label three months from now wants +this pull request in the results. It is applied in every state this composite +handles, and only the caller removes it — at the one moment the fact stops being +true, when the breaking commits are gone. + +`awaiting-breaking-approval` is a **state**. It goes on while a decision is +pending and comes off the moment one is made, either way. + +Collapsing the two is tempting and wrong: removing `breaking-change` on approval +erases the record exactly when it starts to matter, because now the break is +both real and agreed. + ## What it does not do It never closes, merges or approves anything. `close-on-rejection` only changes @@ -113,7 +128,8 @@ side effect in the caller is what makes this safe to run with | `rejecters` | Comma-separated logins that rejected. | No | `''` | | `rejection-comment` | Comment left with the rejection. | No | `''` | | `close-on-rejection` | Whether the caller closes on rejection. Wording only. | No | `true` | -| `label` | Applied while waiting, removed once resolved. Cosmetic. | No | `''` | +| `label` | States that the PR carries a breaking change. A fact — stays once applied. Cosmetic. | No | `''` | +| `pending-label` | States that the breaking change is waiting on a decision. Removed once approved or rejected. | No | `''` | | `enabled` | `false` skips commenting while still managing the label. | No | `true` | ## Outputs diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 977bddf3..bb948c30 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -80,7 +80,11 @@ inputs: required: false default: 'true' label: - description: Label applied while waiting and removed once resolved. Cosmetic — failing to apply it never fails the step. Empty disables it. + description: 'Label stating that this pull request carries a breaking change. A fact about the change, so it stays once applied — approving does not make the break go away. The caller removes it when the breaking commits are gone. Cosmetic; failing to apply it never fails the step. Empty disables it.' + required: false + default: '' + pending-label: + description: 'Label stating that the breaking change is waiting on a decision. A state, so it is removed once approved or rejected. Kept separate from `label` because "this breaks things" and "nobody has decided yet" stop being true at different moments. Empty disables it.' required: false default: '' enabled: @@ -120,6 +124,7 @@ runs: REJECTION_COMMENT: ${{ inputs.rejection-comment }} CLOSE_ON_REJECTION: ${{ inputs.close-on-rejection }} LABEL: ${{ inputs.label }} + PENDING_LABEL: ${{ inputs.pending-label }} ENABLED: ${{ inputs.enabled }} run: | set -euo pipefail @@ -356,14 +361,30 @@ runs: fi # Cosmetic and deliberately last — a repository that never declared the - # label still gets its gate. + # labels still gets its gate. + # + # Two of them, because they stop being true at different moments. The + # breaking change is a property of the commits: approving it does not + # make the break go away, and somebody filtering for `breaking-change` + # three months from now wants this pull request in the results. Waiting + # on a decision is a state, and it ends. + # + # Every state reaching this composite means a breaking change is + # present — the caller skips it entirely when there is none — so the + # fact label is applied throughout and only the caller removes it. if [ -n "$LABEL" ]; then + if ! gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "$LABEL" >/dev/null 2>&1; then + echo "::notice::Label '${LABEL}' not applied — it is not declared in ${REPO}. Cosmetic only." + fi + fi + + if [ -n "$PENDING_LABEL" ]; then if [ "$STATE" = "waiting" ]; then - if ! gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "$LABEL" >/dev/null 2>&1; then - echo "::notice::Label '${LABEL}' not applied — it is not declared in ${REPO}. Cosmetic only." + if ! gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "$PENDING_LABEL" >/dev/null 2>&1; then + echo "::notice::Label '${PENDING_LABEL}' not applied — it is not declared in ${REPO}. Cosmetic only." fi else - gh pr edit "$PR_NUMBER" --repo "$REPO" --remove-label "$LABEL" >/dev/null 2>&1 || true + gh pr edit "$PR_NUMBER" --repo "$REPO" --remove-label "$PENDING_LABEL" >/dev/null 2>&1 || true fi fi diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index d731c86c..92810945 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1219,6 +1219,33 @@ def test_an_unevaluated_acknowledgement_is_not_reported_as_missing(self): self.assertIn("Author acknowledgement", rendered) self.assertIn("missing", rendered) + def test_the_fact_label_outlives_the_decision(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + # `breaking-change` describes the commits, not the process: approving + # does not make the break go away, and somebody filtering for it months + # later wants this pull request in the results. Only the pending label + # is a state, and only it comes off on a decision. + self.assertIn("PENDING_LABEL", reporter) + add_fact = reporter.index('--add-label "$LABEL"') + self.assertNotIn('--remove-label "$LABEL"', reporter) + self.assertIn('--remove-label "$PENDING_LABEL"', reporter) + self.assertGreater(add_fact, 0) + + def test_both_labels_go_when_the_break_does(self): + body = extract_run_body( + extract_job(APPROVAL_WORKFLOW, "outcome"), "Resolve the verdict" + ) + # The one moment the fact stops being true. This cleanup was lost in an + # earlier rewrite, leaving `breaking-change` on a pull request whose + # breaking commit had been reworded away. + self.assertIn('for stale in "$LABEL" "$PENDING_LABEL"', body) + none_at = body.index("state=none") + cleanup_at = body.index('for stale in "$LABEL" "$PENDING_LABEL"') + self.assertLess(none_at, cleanup_at) + def test_the_fingerprint_comes_from_the_breaking_commits(self): detect = extract_job(APPROVAL_WORKFLOW, "detect") self.assertIn("src/validate/breaking-change-commits@", detect) From 23403cd27364cbd2cbd5d4098d896e9a0950ed29 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:40:26 -0300 Subject: [PATCH 26/47] test(pr-validation): run the label lifecycle instead of reading it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #919. The label test asserted on the composite's text, which cannot say which state made which call. Moving `--add-label "$LABEL"` into the `waiting` branch would have left every string in place and stopped the fact label from surviving a decision — the one thing this PR exists to fix. `run_reporter_labels(state)` executes the reporter with `gh` stubbed and returns the `gh pr edit` calls. Each state is checked for the fact label being added and never removed, and for the pending label being added only while waiting. Verified by that exact mutation: three subtests fail. Worth saying plainly: I ran this check by hand while implementing, and pasted its output as evidence. Running it once and versioning it are not the same thing, and only the second survives the next refactor. Also documents the behaviour change for callers who set `label` as a temporary waiting marker — move it to `pending_label` and clear `label`. Callers on the default need nothing. --- .../workflows/breaking-change-approval.yml | 4 + docs/breaking-change-approval.md | 8 ++ .../action.yml | 2 +- .../breaking-change-guard/test-workflow.py | 117 ++++++++++++++++-- 4 files changed, 117 insertions(+), 14 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index bd443ec9..08ab0dcf 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -149,6 +149,10 @@ on: about the change, so it survives the approval — the break does not go away because somebody agreed to it. Removed only when the breaking commits are gone. Empty disables it. + + Behaviour change: this label used to come off on approval. A caller + that set it as a temporary waiting marker should move that value to + `pending_label` and set this one to an empty string. required: false type: string default: breaking-change diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 942d373b..38e5aebb 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -419,6 +419,14 @@ Both are cosmetic: a repository that never declared them still gets its gate. Declare them with [`labels-sync`](labels-sync.md), or pass `label: ''` / `pending_label: ''` to turn them off. +> **Behaviour change.** `label` used to be removed once the change was approved +> or rejected. If you set it as a temporary waiting marker, move that value to +> `pending_label` and set `label: ''` — otherwise the marker now stays on the +> pull request after the decision. +> +> Callers on the default need nothing: `breaking-change` staying put is the +> point, and `awaiting-breaking-approval` takes over the transient role. + ## Inputs | Input | Type | Default | Description | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index bb948c30..b21f6e18 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -80,7 +80,7 @@ inputs: required: false default: 'true' label: - description: 'Label stating that this pull request carries a breaking change. A fact about the change, so it stays once applied — approving does not make the break go away. The caller removes it when the breaking commits are gone. Cosmetic; failing to apply it never fails the step. Empty disables it.' + description: 'Label stating that this pull request carries a breaking change. A fact about the change, so it stays once applied — approving does not make the break go away. The caller removes it when the breaking commits are gone. Cosmetic; failing to apply it never fails the step. Empty disables it. Behaviour change: it used to be removed on a decision — a caller using it as a temporary waiting marker should move that value to `pending-label` and clear this one.' required: false default: '' pending-label: diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 92810945..23c8255f 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -347,6 +347,79 @@ def run_verdict(**overrides): return result, parse_github_output(output_path.read_text(encoding="utf-8")) +def run_reporter_labels(state, **overrides): + """Run the reporter end to end and return the `gh pr edit` calls it made. + + `render_reporter` stops before the label block, so asserting on the + composite's text was the only coverage the lifecycle had — and text cannot + tell which state made which call. Moving `--add-label "$LABEL"` into the + `waiting` branch would keep every string in place and break the behaviour. + """ + action = yaml.safe_load( + ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + ) + body = action["runs"]["steps"][0]["run"] + + env = { + "REPO": "LerianStudio/example", + "PR_NUMBER": "42", + "STATE": state, + "REASON": "rule develop <- '*'", + "HEAD_SHA": "abc1234567", + "ACKNOWLEDGED": "false", + "ACK_LINE": "Breaking change acknowledged: …", + "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", + "REVIEWERS": "team:devops", + "SERVER_URL": "https://github.com", + "BADGE_URL": "https://img.shields.io/badge/x-1f6feb", + "DOCS_URL": "https://example.invalid/docs", + "APPROVERS": "carol", + "REJECTERS": "dave", + "REJECTION_COMMENT": "", + "CLOSE_ON_REJECTION": "true", + "LABEL": "breaking-change", + "PENDING_LABEL": "awaiting-breaking-approval", + "ENABLED": "false", + "FINGERPRINT": "", + "RUN_ID": "1", + "GH_TOKEN": "stub", + } + env.update(overrides) + + with tempfile.TemporaryDirectory() as temp_dir: + calls = Path(temp_dir) / "calls" + calls.touch() + stub = Path(temp_dir) / "gh" + stub.write_text( + "#!/usr/bin/env bash\n" + 'case "$*" in\n' + " *\"pr edit\"*)" + " printf '%s\\n' \"$*\" | grep -oE '(add|remove)-label [a-z-]+'" + f' >> "{calls}" ;;\n' + " *\"/commits\"*) printf '%s\\n' 'feat(x)!: breaks' ;;\n" + " *) exit 1 ;;\n" + "esac\n", + encoding="utf-8", + ) + stub.chmod(0o755) + env["PATH"] = f"{temp_dir}:{os.environ.get('PATH', '/usr/bin:/bin')}" + env["GITHUB_OUTPUT"] = str(Path(temp_dir) / "out") + result = subprocess.run( + ["bash", "-c", body], + cwd=temp_dir, + env=env, + text=True, + capture_output=True, + check=False, + ) + if result.returncode != 0: + raise AssertionError(f"reporter failed for {state!r}: {result.stderr}") + return calls.read_text(encoding="utf-8").split() + + def parse_github_output(output): values = {} for line in output.splitlines(): @@ -1220,19 +1293,37 @@ def test_an_unevaluated_acknowledgement_is_not_reported_as_missing(self): self.assertIn("missing", rendered) def test_the_fact_label_outlives_the_decision(self): - reporter = ( - REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" - / "action.yml" - ).read_text(encoding="utf-8") - # `breaking-change` describes the commits, not the process: approving - # does not make the break go away, and somebody filtering for it months - # later wants this pull request in the results. Only the pending label - # is a state, and only it comes off on a decision. - self.assertIn("PENDING_LABEL", reporter) - add_fact = reporter.index('--add-label "$LABEL"') - self.assertNotIn('--remove-label "$LABEL"', reporter) - self.assertIn('--remove-label "$PENDING_LABEL"', reporter) - self.assertGreater(add_fact, 0) + # Run it, per state, and read the `gh pr edit` calls. Asserting on the + # composite's text cannot tell which state made which call: moving + # `--add-label "$LABEL"` into the `waiting` branch would leave every + # string in place and stop the fact label from surviving a decision. + for state in ("waiting", "approved", "rejected", "acknowledgement"): + with self.subTest(state=state): + calls = run_reporter_labels(state) + pairs = list(zip(calls[::2], calls[1::2])) + + self.assertIn( + ("add-label", "breaking-change"), + pairs, + "the fact label belongs on every state the gate reports", + ) + self.assertNotIn( + ("remove-label", "breaking-change"), + pairs, + "approving does not make the break go away", + ) + + expected = "add-label" if state == "waiting" else "remove-label" + self.assertIn((expected, "awaiting-breaking-approval"), pairs) + + def test_the_two_labels_can_be_turned_off_independently(self): + # Cosmetic throughout: a repository that declared neither still gets + # its gate, and one that wants only the fact can say so. + self.assertEqual([], run_reporter_labels("waiting", LABEL="", PENDING_LABEL="")) + self.assertEqual( + ["add-label", "breaking-change"], + run_reporter_labels("waiting", PENDING_LABEL=""), + ) def test_both_labels_go_when_the_break_does(self): body = extract_run_body( From 28ef1f2b9c5fb5b0e2472d50d4b9f30be48823bc Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 18:22:38 -0300 Subject: [PATCH 27/47] fix(pr-validation): do not re-ask when the approval already stands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seen on #920. With the approval already on file, the run still posted "needs a decision", added awaiting-breaking-approval, and the verdict removed both seconds later. The timeline showed the label going on and off for a decision that was never in question. The request step checked whether a breaking change exists, but not whether it had already been approved — which the previous step had just worked out. One condition. Also links the decider instead of bolding them: `Approved by [@carol](https://github.com/carol)`. It reads as the @name people expect and is clickable, without notifying — the person named is the one who just acted, so a mention would tell them what they already know. Same reasoning already applied to the reviewer list. --- .../workflows/breaking-change-approval.yml | 5 ++++ .../action.yml | 23 +++++++++++++++--- .../breaking-change-guard/test-workflow.py | 24 +++++++++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 08ab0dcf..9a0f50c8 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -533,11 +533,16 @@ jobs: pending-label: '' enabled: ${{ inputs.enable_comment && 'true' || 'false' }} + # Nothing to request when the approval already stands. Without this the + # run announces "needs a decision", adds the pending label, and then the + # verdict takes both back seconds later — a flicker in the timeline for a + # decision that was never in question. - name: Request the approval on the pull request if: >- needs.detect.result == 'success' && needs.detect.outputs.in-scope == 'true' && needs.detect.outputs.has-breaking-changes == 'true' + && steps.prior.outputs.already-approved != 'true' && !inputs.dry_run uses: LerianStudio/github-actions-shared-workflows/src/notify/breaking-change-approval-reporter@develop with: diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index b21f6e18..986b7979 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -181,6 +181,23 @@ runs: ) fi + # `carol` -> `[@carol](https://github.com/carol)`. Linked, not a bare + # mention: the person being named is the one who just acted, so a + # notification tells them what they already know. Same reasoning as the + # reviewer list above. + link_people() { + local out="" person + while IFS= read -r person; do + person=$(printf '%s' "$person" | tr -d ' ') + [ -z "$person" ] && continue + out="${out:+$out, }[@${person}](${SERVER_URL}/${person})" + done < <(tr ',' '\n' <<< "$1") + printf '%s' "$out" + } + + linked_approvers=$(link_people "$APPROVERS") + linked_rejecters=$(link_people "$REJECTERS") + commit_link="" if [ -n "$HEAD_SHA" ]; then commit_link="[\`${HEAD_SHA:0:7}\`](${SERVER_URL}/${REPO}/commit/${HEAD_SHA})" @@ -235,14 +252,14 @@ runs: printf '> [!NOTE]\n' printf '> ## ✅ Breaking change approved\n' printf '>\n' - printf '> Approved by **%s**. 💥 The next release will be a major version.\n\n' \ - "${APPROVERS:-a reviewer}" + printf '> Approved by %s. 💥 The next release will be a major version.\n\n' \ + "${linked_approvers:-a reviewer}" ;; rejected) printf '> [!CAUTION]\n' printf '> ## ⛔ Breaking change rejected\n' printf '>\n' - printf '> Rejected by **%s**' "${REJECTERS:-a reviewer}" + printf '> Rejected by %s' "${linked_rejecters:-a reviewer}" [ -n "$REJECTION_COMMENT" ] && printf ' — *%s*' "$REJECTION_COMMENT" printf '.\n' if [ "$CLOSE_ON_REJECTION" = "true" ]; then diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 23c8255f..07372e8b 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1122,6 +1122,30 @@ def test_a_cancelled_approval_stops_before_anything_can_act_on_it(self): self.assertEqual("superseded", outputs["state"]) self.assertNotEqual("rejected", outputs["state"]) + def test_a_standing_approval_skips_the_request_entirely(self): + step = extract_step( + extract_job(APPROVAL_WORKFLOW, "request"), + "Request the approval on the pull request", + ) + # Observed on #920: with the approval already standing, the run still + # announced "needs a decision" and added the pending label, and the + # verdict took both back seconds later. A flicker in the timeline for a + # decision that was never in question. + self.assertIn("steps.prior.outputs.already-approved != 'true'", step) + + def test_the_decider_is_linked_not_mentioned(self): + for state, env, who in ( + ("approved", {"APPROVERS": "carol"}, "carol"), + ("rejected", {"REJECTERS": "dave"}, "dave"), + ): + with self.subTest(state=state): + rendered = render_reporter(state, **env) + self.assertIn(f"[@{who}](https://github.com/{who})", rendered) + # Linked, not mentioned: the person named is the one who just + # acted, so a notification tells them what they already know. + for match in re.finditer(rf"@{who}", rendered): + self.assertTrue(rendered[: match.start()].endswith("[")) + def test_an_approval_already_given_is_not_asked_for_again(self): # The reviewer said yes to a breaking change. A push that leaves that # change alone must not put the same question back in their queue — From 62e4a5dbd09ff4b13031e3ac9b125a503822c802 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Fri, 16 Oct 2026 18:29:40 -0300 Subject: [PATCH 28/47] test(pr-validation): require the whole link, not a leading bracket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the CodeRabbit review on #921. The no-bare-mention check asserted that each `@name` was preceded by `[`, which a bare `[@carol]` with no target satisfies — bracketed, unlinked, and notifying nobody only by accident. The separate assertion proved one valid link existed somewhere, not that every occurrence was one. Both checks now delete the complete `[@name](url)` links and require nothing to remain, so every occurrence has to be a real link. Applied to the reviewer-list test too, which had the identical weakness and was not flagged. Verified by breaking the link into `[@name] (url)`: two subtests fail where the old form passed. --- .../breaking-change-guard/test-workflow.py | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 07372e8b..ff7e8bdb 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1069,13 +1069,18 @@ def test_the_waiting_comment_links_instead_of_mentioning(self): self.assertIn(fragment, rendered) # The pending deployment already notifies the reviewers; a bare @mention - # would ping the whole team again for the same thing. Checked on the - # rendered comment, so it holds however the composite spells it. - for match in re.finditer(r"@LerianStudio/", rendered): - self.assertTrue( - rendered[: match.start()].endswith("["), - f"bare team mention: ...{rendered[max(0, match.start() - 40):match.end() + 30]}...", - ) + # would ping the whole team again for the same thing. + # + # Same technique as the decider test: delete the complete links, then + # require nothing to remain. This assertion used to check only that an + # `@` was preceded by `[`, which a bare `[@LerianStudio/devops]` with no + # target satisfies. + without_links = re.sub( + r"\[@LerianStudio/\w+\]\(https://github\.com/orgs/LerianStudio/teams/\w+\)", + "", + rendered, + ) + self.assertNotIn("@LerianStudio/", without_links) def test_only_one_approval_request_stays_open_per_pull_request(self): job = extract_job(APPROVAL_WORKFLOW, "approval") @@ -1141,10 +1146,17 @@ def test_the_decider_is_linked_not_mentioned(self): with self.subTest(state=state): rendered = render_reporter(state, **env) self.assertIn(f"[@{who}](https://github.com/{who})", rendered) + # Linked, not mentioned: the person named is the one who just # acted, so a notification tells them what they already know. - for match in re.finditer(rf"@{who}", rendered): - self.assertTrue(rendered[: match.start()].endswith("[")) + # + # Checked by deleting the complete links and requiring nothing + # to remain. "preceded by `[`" would accept a bare `[@carol]` + # with no target — present, bracketed, and not a link. + without_links = re.sub( + rf"\[@{who}\]\(https://github\.com/{who}\)", "", rendered + ) + self.assertNotIn(f"@{who}", without_links) def test_an_approval_already_given_is_not_asked_for_again(self): # The reviewer said yes to a breaking change. A push that leaves that From 96010aa6ff8579a586d26937ad0ab6ead062c4fe Mon Sep 17 00:00:00 2001 From: Fred Amaral Date: Sun, 18 Oct 2026 20:24:30 +0000 Subject: [PATCH 29/47] feat(coderabbit-gate): skip authors reviewed by radagast The gate now reads the organization variable RADAGAST_AUTHORS and vetoes those authors right after ignore_authors, with the same login normalisation, so it stops posting `@coderabbitai review` on pull requests Radagast reviews. The skip notice names Radagast, and dry_run prints the value the run saw. Additive: inputs and defaults are unchanged, and an unset or empty variable vetoes nobody, which is every organization except LerianStudio. tests/coderabbit-gate runs the step as written against a stub gh and is wired into self PR validation. X-Lerian-Ref: 0x1 --- .github/workflows/coderabbit-gate.yml | 31 +++++++--- .github/workflows/self-pr-validation.yml | 20 ++++++ docs/coderabbit-gate.md | 8 +++ tests/coderabbit-gate/test-workflow.sh | 79 ++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 tests/coderabbit-gate/test-workflow.sh diff --git a/.github/workflows/coderabbit-gate.yml b/.github/workflows/coderabbit-gate.yml index 2f9e9bfc..047c4d6a 100644 --- a/.github/workflows/coderabbit-gate.yml +++ b/.github/workflows/coderabbit-gate.yml @@ -30,6 +30,10 @@ name: CodeRabbit Gate # human work and would otherwise spend the hourly review allowance on diffs that # are a version number. # +# The organization variable `RADAGAST_AUTHORS` (comma-separated logins) is a second +# veto: Radagast reviews those authors instead. Unset or empty, it vetoes nobody — +# which is the case in every organization that does not define it. +# # NOTE for callers: the caller's own `pull_request` trigger bounds this. A base # branch listed here but absent from `on.pull_request.branches` never sees the # workflow run at all, so the command is never posted. The effective scope is the @@ -111,6 +115,7 @@ jobs: REVIEW_BASE_BRANCHES: ${{ inputs.review_base_branches }} REVIEW_HEAD_PATTERNS: ${{ inputs.review_head_patterns }} IGNORE_AUTHORS: ${{ inputs.ignore_authors }} + RADAGAST_AUTHORS: ${{ vars.RADAGAST_AUTHORS }} LABEL: ${{ inputs.label }} DRY_RUN: ${{ inputs.dry_run }} run: | @@ -138,6 +143,7 @@ jobs: echo " scope bases : [${REVIEW_BASE_BRANCHES}]" echo " scope head globs : [${REVIEW_HEAD_PATTERNS}]" echo " ignored authors : [${IGNORE_AUTHORS}]" + echo " radagast authors : [${RADAGAST_AUTHORS}]" fi # `checks_passed` describes one specific commit. Without concurrency @@ -178,13 +184,24 @@ jobs: author_key=$(normalise_login "$author") - while IFS= read -r entry; do - [ -z "$entry" ] && continue - if [ "$author_key" = "$(normalise_login "$entry")" ]; then - echo "::notice::PR #${PR_NUMBER} was opened by ${author}, an ignored author — not requesting a review" - exit 0 - fi - done < <(tr ',' '\n' <<< "$IGNORE_AUTHORS" | xargs -n1 2>/dev/null) + listed() { + local entry + while IFS= read -r entry; do + [ -z "$entry" ] && continue + [ "$author_key" = "$(normalise_login "$entry")" ] && return 0 + done < <(tr ',' '\n' <<< "$1" | xargs -n1 2>/dev/null) + return 1 + } + + if listed "$IGNORE_AUTHORS"; then + echo "::notice::PR #${PR_NUMBER} was opened by ${author}, an ignored author — not requesting a review" + exit 0 + fi + + if listed "$RADAGAST_AUTHORS"; then + echo "::notice::PR #${PR_NUMBER} was opened by ${author}, whose pull requests are reviewed by Radagast — not requesting a CodeRabbit review" + exit 0 + fi matched="" diff --git a/.github/workflows/self-pr-validation.yml b/.github/workflows/self-pr-validation.yml index 7a6c7948..11fbf267 100644 --- a/.github/workflows/self-pr-validation.yml +++ b/.github/workflows/self-pr-validation.yml @@ -221,6 +221,25 @@ jobs: - name: Run coderabbit-collapse behavioural tests run: python3 src/notify/coderabbit-collapse/test.py + # ----------------- CodeRabbit Gate Tests ----------------- + coderabbit-gate-tests: + name: CodeRabbit Gate Tests + runs-on: ${{ vars.GENERAL_RUNNERS || 'blacksmith-4vcpu-ubuntu-2404' }} + permissions: + contents: read + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v6 + with: + persist-credentials: false + + # The shellcheck job lints `run:` blocks inside YAML, not standalone scripts. + - name: Lint coderabbit-gate test scripts + run: shellcheck tests/coderabbit-gate/*.sh + + - name: Run coderabbit-gate author veto tests + run: bash tests/coderabbit-gate/test-workflow.sh + # ----------------- Lerian Lib Version Advisory Tests ----------------- lerian-lib-version-tests: name: Lerian Lib Version Advisory Tests @@ -587,6 +606,7 @@ jobs: - permission-manifest-publish-tests - pr-commit-signatures-tests - coderabbit-collapse-tests + - coderabbit-gate-tests - prerelease-check-tests - socket-app-gate-tests - non-doc-changes-tests diff --git a/docs/coderabbit-gate.md b/docs/coderabbit-gate.md index e4bda7c3..d477cb84 100644 --- a/docs/coderabbit-gate.md +++ b/docs/coderabbit-gate.md @@ -138,6 +138,14 @@ them match: REST returns `dependabot[bot]`, GraphQL returns `dependabot`, and stripped before comparing, so `dependabot`, `dependabot[bot]` and `app/dependabot` are the same entry. +### Authors reviewed by Radagast + +The variable `RADAGAST_AUTHORS` (comma-separated logins, spelled as for +`ignore_authors`) is a second veto, checked right after it: Radagast reviews those +authors, so the gate never asks CodeRabbit for them. It is read from the caller's +organization or repository, so changing it needs no input and no promotion. Unset +or empty, it vetoes nobody. A `dry_run` prints the value the run saw. + ### The caller's trigger bounds this A base branch listed in `review_base_branches` but absent from the caller's diff --git a/tests/coderabbit-gate/test-workflow.sh b/tests/coderabbit-gate/test-workflow.sh new file mode 100644 index 00000000..16c3cb03 --- /dev/null +++ b/tests/coderabbit-gate/test-workflow.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash + +# Runs the decision step of .github/workflows/coderabbit-gate.yml as written, under +# the runner's own shell flags and with a stub `gh` on PATH, and asserts for which +# authors it posts `@coderabbitai review`. No network, no GitHub. + +set -euo pipefail + +CDPATH='' +WORKFLOW="$(cd -- "$(dirname -- "$0")/../.." && pwd)/.github/workflows/coderabbit-gate.yml" +WORK_DIR=$(mktemp -d "${TMPDIR:-/tmp}/coderabbit-gate.XXXXXX") +trap 'rm -rf "${WORK_DIR}"' EXIT INT TERM +PASSED=0 +FAILED=0 + +# The job has a single step: print its `run: |` body dedented to column zero. +awk ' + !inrun && /^[[:space:]]*run: \|[[:space:]]*$/ { match($0, /^ */); indent = RLENGTH; inrun = 1; next } + inrun { + if ($0 ~ /^[[:space:]]*$/) { print ""; next } + match($0, /^ */) + if (RLENGTH <= indent) exit + print substr($0, indent + 3) + } +' "${WORKFLOW}" >"${WORK_DIR}/gate.sh" +[[ -s ${WORK_DIR}/gate.sh ]] || { + printf 'not ok - could not extract the run block from %s\n' "${WORKFLOW}" >&2 + exit 1 +} + +# `gh pr view` answers with an in-scope, green pull request opened by STUB_AUTHOR; +# `gh pr comment` is recorded; every other call succeeds and prints nothing. +mkdir "${WORK_DIR}/bin" +cat >"${WORK_DIR}/bin/gh" <<'STUB' +#!/usr/bin/env bash +case "$1 $2" in + 'pr view') printf 'develop feat/x abc123 %s\n' "${STUB_AUTHOR}" ;; + 'pr comment') printf '%s\n' "$*" >>"${STUB_COMMENTS}" ;; +esac +STUB +chmod +x "${WORK_DIR}/bin/gh" + +# assert