fix(pr-validation): close two paths past the breaking-change gate

.github/workflows/breaking-change-approval.yml

@@ -405,7 +405,7 @@ jobs:
       contents: read
       pull-requests: write
     outputs:
-      already-approved: ${{ steps.prior.outputs.already-approved }}
+      already-approved: ${{ steps.reuse.outputs.already-approved }}
       approved-by: ${{ steps.prior.outputs.approved-by }}
     steps:
       # Has this exact breaking change already been approved on this pull
@@ -435,7 +435,8 @@ jobs:
         run: |
           set -euo pipefail
 
-          already=false
+          candidate_run=""
+          candidate_head=""
           approved_by=""
           approving_run=""
           unverified=""
@@ -498,10 +499,17 @@ jobs:
             # Matched on the branch, because `pull_requests` on a run comes
             # back empty in practice — verified against run 38967637198, which
             # really did carry an approval for this gate.
-            run_branch=$(
+            #
+            # The run's own head is read here and nowhere else. It is the only
+            # unforgeable statement of *what* that approval covered: the
+            # fingerprint in the record says what the writer claims it covered,
+            # and anybody who can read the repository can write a record.
+            run_facts=$(
               gh api "repos/${REPO}/actions/runs/${approving_run}" \
-                --jq '.head_branch // ""' 2>/dev/null || true
+                --jq '[(.head_branch // ""), (.head_sha // "")] | @tsv' 2>/dev/null || true
             )
+            run_branch=$(printf '%s' "$run_facts" | cut -f1)
+            run_head=$(printf '%s' "$run_facts" | cut -f2)
 
             if [ -z "$run_branch" ] || [ "$run_branch" != "$HEAD_REF" ]; then
               unverified="${unverified:+$unverified }${approving_run}"
@@ -516,8 +524,12 @@ jobs:
                       | .user.login' 2>/dev/null || true
             )
 
-            if [ -n "$approvers" ]; then
-              already=true
+            if [ -n "$approvers" ] && [ -n "$run_head" ]; then
+              # A candidate, not a verdict. What this run approved is still
+              # unknown — the next step resolves the breaking set as it stood
+              # at `run_head` and only then is the record believed.
+              candidate_run="$approving_run"
+              candidate_head="$run_head"
               approved_by="$approvers"
               break
             fi
@@ -534,9 +546,10 @@ jobs:
             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"
+            echo "  candidate run     : ${candidate_run:-none}${approved_by:+ (by ${approved_by})}"
+            echo "  candidate head    : ${candidate_head:-none}"
+          elif [ -n "$candidate_run" ]; then
+            echo "::notice::Run ${candidate_run} carried an approval for this branch — checking what it approved"
           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
@@ -558,10 +571,77 @@ jobs:
           approved_by=$(printf '%s' "$approved_by" | awk 'NF' | paste -sd, -)
 
           {
-            echo "already-approved=$already"
+            echo "candidate-run=${candidate_run}"
+            echo "candidate-head=${candidate_head}"
             echo "approved-by=${approved_by}"
           } >> "$GITHUB_OUTPUT"
 
+      # What did that run actually approve? Resolved from the run's own head,
+      # which nobody can write, rather than from the fingerprint in the record,
+      # which anybody who can read the repository can write.
+      #
+      # @fredcamaral demonstrated the gap on #926. The fingerprint is derived
+      # from public commit SHAs, so it is computable in advance: a member posts
+      # a record carrying fp(A,B) and pointing at the run that approved only A,
+      # then pushes the breaking commit B. Every check up to here passes — the
+      # record is from a MEMBER, the run is on this branch, the run really did
+      # carry an approval — and nothing asked what it approved. The verdict
+      # came back `approved`, exit 0, no reviewer involved.
+      - name: Resolve what the approving run approved
+        id: approved-set
+        if: steps.prior.outputs.candidate-head != ''
+        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: ${{ needs.detect.outputs.pr-number }}
+          at-sha: ${{ steps.prior.outputs.candidate-head }}
+          base-ref: ${{ needs.detect.outputs.base-ref }}
+
+      - name: Confirm the approval covers this change
+        id: reuse
+        env:
+          CANDIDATE_RUN: ${{ steps.prior.outputs.candidate-run }}
+          CANDIDATE_HEAD: ${{ steps.prior.outputs.candidate-head }}
+          APPROVED_BY: ${{ steps.prior.outputs.approved-by }}
+          APPROVED_FINGERPRINT: ${{ steps.approved-set.outputs.fingerprint }}
+          RESOLVE_OUTCOME: ${{ steps.approved-set.outcome }}
+          FINGERPRINT: ${{ needs.detect.outputs.fingerprint }}
+          PR_NUMBER: ${{ needs.detect.outputs.pr-number }}
+          DRY_RUN: ${{ inputs.dry_run && 'true' || 'false' }}
+        run: |
+          set -euo pipefail
+
+          already=false
+
+          # Three ways to not reuse, and all of them ask the reviewer again.
+          # Asking twice costs attention; reusing an approval that covered
+          # something else costs the gate.
+          if [ -z "$CANDIDATE_RUN" ]; then
+            reason="no verified approval on record"
+          elif [ "$RESOLVE_OUTCOME" != "success" ] || [ -z "$APPROVED_FINGERPRINT" ]; then
+            reason="could not resolve what run ${CANDIDATE_RUN} approved"
+          elif [ "$APPROVED_FINGERPRINT" != "$FINGERPRINT" ]; then
+            reason="run ${CANDIDATE_RUN} approved ${APPROVED_FINGERPRINT}, this is ${FINGERPRINT}"
+          else
+            already=true
+            reason="run ${CANDIDATE_RUN} approved ${APPROVED_FINGERPRINT}, unchanged"
+          fi
+
+          if [ "$DRY_RUN" = "true" ]; then
+            echo "::notice::DRY RUN — approval reuse"
+            echo "  this change       : ${FINGERPRINT:-none}"
+            echo "  approved at       : ${CANDIDATE_HEAD:-none}"
+            echo "  that change was   : ${APPROVED_FINGERPRINT:-unresolved}"
+            echo "  reusing           : ${already} — ${reason}"
+          elif [ "$already" = "true" ]; then
+            echo "::notice::PR #${PR_NUMBER} — ${reason}; not asking ${APPROVED_BY} again"
+          elif [ -n "$CANDIDATE_RUN" ]; then
+            echo "::warning::Not reusing the approval on PR #${PR_NUMBER} — ${reason}."
+          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.
@@ -628,7 +708,7 @@ jobs:
           needs.detect.result == 'success'
           && needs.detect.outputs.in-scope == 'true'
           && needs.detect.outputs.has-breaking-changes == 'true'
-          && steps.prior.outputs.already-approved != 'true'
+          && steps.reuse.outputs.already-approved != 'true'
           && !inputs.dry_run
         uses: LerianStudio/github-actions-shared-workflows/src/notify/breaking-change-approval-reporter@develop
         with:
@@ -801,23 +881,39 @@ jobs:
             #
             # One head branch can carry more than one pull request — same
             # branch, different bases — so the workflow scope alone still lets
-            # another pull request's run stand in for this one's. `.pull_requests`
-            # carries the run's own pull request and settles it. It comes back
-            # empty for runs from forks, and there the branch scope is the best
-            # evidence left: a run of this workflow, on this head branch, newer
-            # than this one.
+            # another pull request's run stand in for this one's.
+            # `.pull_requests` carries the run's own pull request and settles
+            # it.
+            #
+            # And only a `pull_request` run counts. Without that filter any
+            # newer run of this workflow on the branch qualified, including one
+            # somebody started by hand: dispatch a run, cancel the pending
+            # approval, and the verdict reads `superseded`, which exits 0 — a
+            # green gate with no approval, available to anybody with write
+            # access. Reported by @fredcamaral and reproduced; the `event`
+            # filter is applied twice, once as a query parameter and once in
+            # the selection, because the parameter alone is the server's
+            # promise and this decides whether a merge is allowed.
+            #
+            # `.pull_requests` comes back empty for runs from forks. Those are
+            # matched on the head instead, which still ties the run to this
+            # revision rather than to the branch at large — the empty arm used
+            # to accept any run with no pull request attached, which is what
+            # let a dispatched run through.
             workflow_id=$(
               gh api "repos/${REPO}/actions/runs/${RUN_ID}" --jq '.workflow_id' 2>/dev/null || true
             )
 
             newer=0
             if [ -n "$workflow_id" ]; then
               newer=$(
-                gh api "repos/${REPO}/actions/workflows/${workflow_id}/runs?branch=${HEAD_REF}&per_page=50" \
+                gh api "repos/${REPO}/actions/workflows/${workflow_id}/runs?branch=${HEAD_REF}&event=pull_request&per_page=50" \
                   --jq "[.workflow_runs[]
                           | select(.id > ${RUN_ID})
-                          | select((.pull_requests | length) == 0
-                                   or any(.pull_requests[]; .number == ${PR_NUMBER}))]
+                          | select(.event == \"pull_request\")
+                          | select(any(.pull_requests[]?; .number == ${PR_NUMBER})
+                                   or ((.pull_requests | length) == 0
+                                       and .head_sha == \"${HEAD_SHA}\"))]
                          | length" 2>/dev/null || echo 0
               )
             fi

docs/breaking-change-approval.md

@@ -300,19 +300,35 @@ 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:
+So three things must hold before an approval is reused:
 
 1. the record sits in a comment from an **owner or member** of the
    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
+   Environment, read from the Environment's own approval log, and that run is
+   on this head branch
+3. the **set that run approved** is this set — resolved from the run's own
+   `head_sha`, never from the fingerprint written in the record
 
 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.
 
+The third was missing until @fredcamaral demonstrated what it cost. The first
+two establish that *an* approval happened on this branch; neither says what it
+approved. Since the fingerprint is derived from public commit SHAs it is
+computable before the commit exists, so a member could post a record carrying
+`fp(A,B)` pointing at the run that approved only `A`, then push the breaking
+commit `B` — and the gate would read "already approved" and return success with
+no reviewer involved. Resolving the approved set from the run's own head closes
+it: the record now says only *which run to ask*, and the run answers.
+
+When that resolution fails — a deleted run, an unreadable comparison, a pull
+request past the commit cap — the approval is not reused. Not knowing is not
+evidence that it still applies.
+
 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

src/validate/breaking-change-commits/README.md

@@ -87,13 +87,41 @@ 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.
 
+## Resolving at a past revision
+
+`at-sha` (with `base-ref`) resolves the set as it stood at that revision
+instead of at the pull request's current head, by comparing `base-ref...at-sha`
+rather than reading the pull request's commits — which always report the
+current state.
+
+The gate uses it for one question: *what did that approval approve?* The
+fingerprint written in the approval record answers a different one — what the
+writer claims it covered — and anybody who can read the repository can write a
+record. Since the fingerprint is derived from public commit SHAs it is also
+computable in advance, so a record naming a real approved run could be posted
+before the breaking commit it claims to cover even exists. Resolving from the
+approving run's own head closes that: the head is not writable.
+
+The 250 cap applies here too, measured against `total_commits`, and is refused
+the same way.
+
+**This path needs `contents: read`.** The comparison API is repository
+contents, not pull request data, so a token carrying only `pull-requests: read`
+fails every comparison — and a failed comparison reads as "cannot confirm what
+that approval covered", which means the approval is not reused and the reviewer
+is asked again on every push. Nothing says why, so grant the scope.
+`breaking-change-approval.yml` already does for its own `request` job; this
+applies to anything calling the composite directly.
+
 ## Inputs
 
 | Input | Description | Required | Default |
 |-------|-------------|----------|---------|
-| `github-token` | Token used to read the commits. Needs `pull-requests: read`. | Yes | — |
+| `github-token` | Token used to read the commits. Needs `pull-requests: read` — and `contents: read` as well when `at-sha` is given. | Yes | — |
 | `pr-number` | Pull request whose commits are listed. | Yes | — |
 | `repository` | Repository owning it, as `owner/name`. | No | `${{ github.repository }}` |
+| `at-sha` | Resolve the set as it stood at this revision instead of at the current head. Requires `base-ref`. | No | `''` |
+| `base-ref` | Base of the comparison when `at-sha` is given. | No | `''` |
 
 ## Outputs
 

src/validate/breaking-change-commits/action.yml

@@ -3,7 +3,13 @@ description: "Lists the commits in a pull request that carry a breaking change,
 
 inputs:
   github-token:
-    description: 'Token used to read the pull request commits. Needs `pull-requests: read`.'
+    description: >-
+      Token used to read the commits. Needs `pull-requests: read`, and
+      `contents: read` as well when `at-sha` is given — that path reads the
+      comparison API, which is repository contents rather than pull request
+      data. Granting only the first makes every comparison fail, and the gate
+      reads a failed comparison as "cannot confirm", so it asks the reviewer
+      again on every push without saying why.
     required: true
   pr-number:
     description: Pull request whose commits are listed.
@@ -12,6 +18,18 @@ inputs:
     description: 'Repository owning the pull request, as `owner/name`.'
     required: false
     default: ${{ github.repository }}
+  at-sha:
+    description: >-
+      Resolve the set as it stood at this revision instead of at the pull
+      request's current head. Requires `base-ref`, and a token carrying
+      `contents: read`. The gate uses it to ask what an approval actually
+      approved, rather than believing a fingerprint written in a comment.
+    required: false
+    default: ''
+  base-ref:
+    description: 'Base of the comparison when `at-sha` is given — the pull request''s base branch.'
+    required: false
+    default: ''
 
 outputs:
   count:
@@ -37,6 +55,8 @@ runs:
         GH_TOKEN: ${{ inputs.github-token }}
         REPO: ${{ inputs.repository }}
         PR_NUMBER: ${{ inputs.pr-number }}
+        AT_SHA: ${{ inputs.at-sha }}
+        BASE_REF: ${{ inputs.base-ref }}
       run: |
         set -euo pipefail
         bash "${GITHUB_ACTION_PATH}/resolve.sh" >> "${GITHUB_OUTPUT}"

src/validate/breaking-change-commits/resolve.sh

@@ -23,6 +23,13 @@ set -euo pipefail
 GH_TOKEN=${GH_TOKEN:-}
 REPO=${REPO:-}
 PR_NUMBER=${PR_NUMBER:-}
+# Optional. Resolves the set as it stood at a given revision instead of at the
+# pull request's current head — which is what lets the gate ask the question it
+# actually needs answered: what did that approval approve? Reading the
+# fingerprint out of a comment answers a different one, and a comment is
+# writable by anybody who can read the repository.
+BASE_REF=${BASE_REF:-}
+AT_SHA=${AT_SHA:-}
 
 export GH_TOKEN
 
@@ -38,7 +45,34 @@ for binary in gh jq; do
   fi
 done
 
-if ! commits=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/commits" --paginate \
+if [[ -n "${AT_SHA}" ]]; then
+  if [[ -z "${BASE_REF}" ]]; then
+    printf 'Breaking Change Commits: BASE_REF is required with AT_SHA.\n' >&2
+    exit 1
+  fi
+
+  # `compare` answers for a revision that is no longer the head. The pull
+  # request's own commits endpoint cannot: it always reports the current state.
+  if ! comparison=$(gh api "repos/${REPO}/compare/${BASE_REF}...${AT_SHA}" 2>/dev/null); then
+    printf 'Breaking Change Commits: could not compare %s...%s in %s.\n' \
+      "${BASE_REF}" "${AT_SHA}" "${REPO}" >&2
+    exit 1
+  fi
+
+  # Same cap, same refusal. `compare` returns at most 250 commits and declares
+  # the real total separately, so a truncated answer is detectable — and
+  # answering from one would be worse here than anywhere else, since this
+  # decides whether an approval is reused.
+  declared_total=$(jq -r '.total_commits // 0' <<< "${comparison}")
+  returned_total=$(jq -r '.commits | length' <<< "${comparison}")
+  if [[ "${declared_total}" -ne "${returned_total}" ]]; then
+    printf 'Breaking Change Commits: %s...%s has %s commits but the comparison returned %s.\n' \
+      "${BASE_REF}" "${AT_SHA}" "${declared_total}" "${returned_total}" >&2
+    exit 1
+  fi
+
+  commits=$(jq -r '.commits[] | [.sha, .commit.message] | @base64' <<< "${comparison}")
+elif ! 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
@@ -53,10 +87,15 @@ fi
 # covered.
 #
 # Refuse instead. A gate that cannot see the whole change says so.
+#
+# Only for the pull-request path: the comparison above does its own version of
+# this check, against `total_commits`, because it has no pull request to ask.
 returned=$(grep -c . <<< "${commits}" || true)
 [[ -z "${commits}" ]] && returned=0
 
-if ! declared=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.commits' 2>/dev/null); then
+if [[ -n "${AT_SHA}" ]]; then
+  declared="${returned}"
+elif ! declared=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.commits' 2>/dev/null); then
   printf 'Breaking Change Commits: could not read the commit count of PR #%s in %s.\n' \
     "${PR_NUMBER}" "${REPO}" >&2
   exit 1