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 1/2] 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 2/2] 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