fix(pr-validation): make the breaking-change comment read as information

.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'
@@ -392,6 +393,8 @@ 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 }}
+          actual-reviewers: ${{ steps.environment.outputs.reviewers }}
           reason: ${{ needs.detect.outputs.reason }}
           head-sha: ${{ needs.detect.outputs.head-sha }}
           acknowledged: ${{ needs.detect.outputs.acknowledged }}

src/notify/breaking-change-approval-reporter/README.md

@@ -27,6 +27,36 @@ 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`](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](https://github.com/LerianStudio/example/actions/runs/1)
+>
+> 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 `<sub>` 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.
+
+**`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
@@ -46,6 +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 a `team:<slug>` / `user:<login>` list. | No | `''` |
+| `actual-reviewers` | Reviewers read from the Environment itself. Preferred over `reviewers` when present. | No | `''` |
+| `server-url` | Base URL used to build commit links. | No | `${{ github.server_url }}` |
+| `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 | `''` |

src/notify/breaking-change-approval-reporter/action.yml

@@ -31,6 +31,22 @@ inputs:
     description: Link to the run carrying the pending deployment.
     required: false
     default: ''
+  reviewers:
+    description: 'Who can answer, as a `team:<slug>` / `user:<login>` 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:
+    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 +92,9 @@ runs:
         HEAD_SHA: ${{ inputs.head-sha }}
         ACKNOWLEDGED: ${{ inputs.acknowledged }}
         RUN_URL: ${{ inputs.run-url }}
+        REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-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 +115,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 '<!-- breaking-change-approval -->\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 '<sub>%s</sub>\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 '<sub>Not intended? Drop the `!` and any `BREAKING CHANGE:` footer from the commits and push again — the gate clears itself. '
+            printf '[How this works](%s)</sub>\n' "$DOCS_URL"
+          fi
         } > "$body_file"
 
         comment_id=""

src/validate/breaking-change-guard/test-workflow.py

@@ -821,6 +821,47 @@ 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.
+        #
+        # 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_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)

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
@@ -90,6 +99,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:<slug>`/`user:<login>`. Read back from the Environment, so it reflects one that already existed with different reviewers than were asked for. |
 
 ## Usage
 

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:<slug>`/`user:<login>`. 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
@@ -57,9 +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.
+        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:<slug>` / `user:<login>` / `<login>` into the
@@ -95,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
@@ -151,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
@@ -171,6 +200,7 @@ runs:
         {
           echo "protected=true"
           echo "created=$created"
+          echo "reviewers=$(reviewers_of)"
         } >> "$GITHUB_OUTPUT"
 
         if [ "$created" != "true" ]; then