fix(pr-validation): do not re-ask when the approval already stands

.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:

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

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")
@@ -1122,6 +1127,37 @@ 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.
+                #
+                # 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
         # change alone must not put the same question back in their queue —