fix(harvest-contributors): only credit PRs from the subject line, not body refs

.github/workflows/script-tests.yml

@@ -0,0 +1,33 @@
+name: Script Tests
+
+# Runs shell self-tests for scripts/ that ship one (currently the
+# harvest-contributors PR-number extraction guard). Standalone job: the
+# skill-repo-skill reusables validate skill *structure*, not script *behavior*.
+# Third-party actions are SHA-pinned (githubactions:S7637); permissions are
+# minimal and declared at the call site.
+
+on:
+  push:
+    branches: [main]
+  pull_request:
+
+permissions: {}
+
+jobs:
+  shell-tests:
+    name: Shell self-tests
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+    steps:
+      - name: Harden Runner
+        uses: step-security/harden-runner@9af89fc71515a100421586dfdb3dc9c984fbf411 # v2.19.4
+        with:
+          egress-policy: audit
+
+      - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
+        with:
+          persist-credentials: false
+
+      - name: harvest-contributors self-test
+        run: bash skills/github-release/scripts/tests/harvest-contributors.test.sh

skills/github-release/scripts/harvest-contributors.sh

@@ -18,33 +18,20 @@
 # before applying with `gh release edit vX.Y.Z --notes-file notes.md`.
 set -euo pipefail
 
-REPO=""; FROM=""; TO=""
-while [ $# -gt 0 ]; do
-  case "$1" in
-    --repo) REPO="$2"; shift 2 ;;
-    --from) FROM="$2"; shift 2 ;;
-    --to) TO="$2"; shift 2 ;;
-    -h | --help) sed -n '2,18p' "$0"; exit 0 ;;
-    *) echo "unknown arg: $1" >&2; exit 2 ;;
-  esac
-done
-
-command -v gh >/dev/null || { echo "gh (authenticated) required" >&2; exit 1; }
-command -v jq >/dev/null || { echo "jq required" >&2; exit 1; }
-
-[ -n "$REPO" ] || REPO="$(gh repo view --json nameWithOwner --jq .nameWithOwner)"
-[ -n "$FROM" ] && [ -n "$TO" ] || { echo "both --from and --to are required (e.g. --from v0.25.1 --to v0.26.0)" >&2; exit 2; }
+# ---- pure helpers (sourced by scripts/tests/harvest-contributors.test.sh) ----
 
-owner="${REPO%/*}"; name="${REPO#*/}"
-
-# PR numbers from the compare range's commit messages (squash "(#N)" + merge refs),
-# sourced from the API so this works from any directory.
-# Run gh api directly (not inside the process substitution) so a failure — auth,
-# rate limit, bad range — trips `set -e` instead of silently yielding an empty
-# list and a misleading "No PRs found".
-commit_messages="$(gh api --paginate "repos/${REPO}/compare/${FROM}...${TO}" --jq '.commits[].commit.message')"
-mapfile -t PRS < <(grep -oE '#[0-9]+' <<<"$commit_messages" | tr -d '#' | sort -un || true)
-[ "${#PRS[@]}" -gt 0 ] || { echo "No PRs found in ${REPO} ${FROM}...${TO}." >&2; exit 0; }
+# PR numbers from commit SUBJECT lines (stdin), one per line, sorted-unique.
+# Match ONLY the place GitHub stamps the PR number: a squash-merge "… (#N)"
+# suffix or a "Merge pull request #N" subject.
+#
+# Do NOT grep the full multi-line message for "#N": commit BODIES cross-reference
+# unrelated issues/PRs ("follow-up to #114"), and dependency-bump commits
+# (Renovate/Dependabot) embed upstream changelogs full of foreign "#123" refs and
+# the "&#8203;" HTML entity (grepped as "#8203") — all of which would be looked up
+# against THIS repo's numbering and falsely credit whoever owns that number.
+extract_pr_numbers() {
+  grep -oE '\(#[0-9]+\)[[:space:]]*$|^Merge pull request #[0-9]+' | grep -oE '[0-9]+' | sort -un || true
+}
 
 # GitHub bot logins end in "[bot]"; also drop known automation by bare name.
 is_bot() {
@@ -55,52 +42,91 @@ is_bot() {
   esac
 }
 
-declare -A CODE      # login -> 1 (merged-PR authors)
-declare -A REPORTERS # login -> "12 34" (space-separated issue numbers, deduped at emit)
-for pr in "${PRS[@]}"; do
-  # shellcheck disable=SC2016 # $o/$n/$pr are GraphQL variables, expanded server-side
-  json="$(gh api graphql -f query='
-    query($o:String!,$n:String!,$pr:Int!){
-      repository(owner:$o,name:$n){
-        pullRequest(number:$pr){
-          author{login}
-          closingIssuesReferences(first:30){nodes{number author{login}}}
+main() {
+  local REPO="" FROM="" TO=""
+  while [ $# -gt 0 ]; do
+    case "$1" in
+      --repo) REPO="$2"; shift 2 ;;
+      --from) FROM="$2"; shift 2 ;;
+      --to) TO="$2"; shift 2 ;;
+      -h | --help) sed -n '2,18p' "$0"; exit 0 ;;
+      *) echo "unknown arg: $1" >&2; exit 2 ;;
+    esac
+  done
+
+  command -v gh >/dev/null || { echo "gh (authenticated) required" >&2; exit 1; }
+  command -v jq >/dev/null || { echo "jq required" >&2; exit 1; }
+
+  [ -n "$REPO" ] || REPO="$(gh repo view --json nameWithOwner --jq .nameWithOwner)"
+  [ -n "$FROM" ] && [ -n "$TO" ] || { echo "both --from and --to are required (e.g. --from v0.25.1 --to v0.26.0)" >&2; exit 2; }
+
+  local owner="${REPO%/*}" name="${REPO#*/}"
+
+  # Subjects only (first line of each commit), then extract via the helper above.
+  # Run gh api directly (not inside the process substitution) so a failure — auth,
+  # rate limit, bad range — trips `set -e` instead of silently yielding an empty
+  # list and a misleading "No PRs found".
+  local subjects
+  subjects="$(gh api --paginate "repos/${REPO}/compare/${FROM}...${TO}" --jq '.commits[].commit.message | split("\n")[0]')"
+  local -a PRS
+  mapfile -t PRS < <(printf '%s\n' "$subjects" | extract_pr_numbers)
+  [ "${#PRS[@]}" -gt 0 ] || { echo "No PRs found in ${REPO} ${FROM}...${TO}." >&2; exit 0; }
+
+  local -A CODE      # login -> 1 (merged-PR authors)
+  local -A REPORTERS # login -> "12 34" (space-separated issue numbers, deduped at emit)
+  local pr json a inum ilogin
+  for pr in "${PRS[@]}"; do
+    # shellcheck disable=SC2016 # $o/$n/$pr are GraphQL variables, expanded server-side
+    json="$(gh api graphql -f query='
+      query($o:String!,$n:String!,$pr:Int!){
+        repository(owner:$o,name:$n){
+          pullRequest(number:$pr){
+            author{login}
+            closingIssuesReferences(first:30){nodes{number author{login}}}
+          }
         }
-      }
-    }' -F o="$owner" -F n="$name" -F pr="$pr" 2>/dev/null)" || continue
-  # A number that is an issue (not a PR) returns pullRequest: null. jq tolerates
-  # indexing null (yields null -> // empty), and .nodes[]? guards the iteration,
-  # so the null case is handled without erroring under set -e.
-  a="$(jq -r '.data.repository.pullRequest.author.login // empty' <<<"$json")"
-  if [ -n "$a" ] && ! is_bot "$a"; then CODE["$a"]=1; fi
+      }' -F o="$owner" -F n="$name" -F pr="$pr" 2>/dev/null)" || continue
+    # A number that is an issue (not a PR) returns pullRequest: null. jq tolerates
+    # indexing null (yields null -> // empty), and .nodes[]? guards the iteration,
+    # so the null case is handled without erroring under set -e.
+    a="$(jq -r '.data.repository.pullRequest.author.login // empty' <<<"$json")"
+    if [ -n "$a" ] && ! is_bot "$a"; then CODE["$a"]=1; fi
+
+    while IFS=$'\t' read -r inum ilogin; do
+      if [ -n "$ilogin" ] && ! is_bot "$ilogin"; then
+        REPORTERS["$ilogin"]="${REPORTERS["$ilogin"]:+${REPORTERS["$ilogin"]} }${inum}"
+      fi
+    done < <(jq -r '.data.repository.pullRequest.closingIssuesReferences.nodes[]?
+                    | "\(.number)\t\(.author.login // "")"' <<<"$json")
+  done
 
-  while IFS=$'\t' read -r inum ilogin; do
-    if [ -n "$ilogin" ] && ! is_bot "$ilogin"; then
-      REPORTERS["$ilogin"]="${REPORTERS["$ilogin"]:+${REPORTERS["$ilogin"]} }${inum}"
-    fi
-  done < <(jq -r '.data.repository.pullRequest.closingIssuesReferences.nodes[]?
-                  | "\(.number)\t\(.author.login // "")"' <<<"$json")
-done
+  echo "## 👥 Contributors"
+  echo
+  echo "Thanks to everyone who made this release — code and reports alike:"
+  echo
+  local k joined line any
+  if [ "${#CODE[@]}" -gt 0 ]; then
+    joined=""
+    for k in $(printf '%s\n' "${!CODE[@]}" | sort -f); do joined="${joined}@${k}, "; done
+    echo "- **Code:** ${joined%, }"
+  fi
+  # "Reported" credits humans who reported a fixed issue but are NOT already in
+  # Code — so each person is credited once and this line highlights the reporters
+  # (typically community members) who didn't also author a PR. This also drops a
+  # maintainer's self-reported issues, since maintainers appear under Code.
+  line="- **Reported issues fixed here:** "; any=0
+  for k in $(printf '%s\n' "${!REPORTERS[@]}" | sort -f); do
+    [ -n "${CODE["$k"]:-}" ] && continue
+    # paste -d takes the delimiter chars cyclically, so -d', ' would alternate
+    # "," and " " between fields — join with a single "," then expand to ", ".
+    refs="$(printf '%s\n' "${REPORTERS["$k"]}" | tr ' ' '\n' | grep . | sort -un | sed 's/^/#/' | paste -sd, - | sed 's/,/, /g')"
+    line="${line}@${k} (${refs}), "; any=1
+  done
+  [ "$any" -eq 1 ] && echo "${line%, }"
+  return 0
+}
 
-echo "## 👥 Contributors"
-echo
-echo "Thanks to everyone who made this release — code and reports alike:"
-echo
-if [ "${#CODE[@]}" -gt 0 ]; then
-  joined=""
-  for k in $(printf '%s\n' "${!CODE[@]}" | sort -f); do joined="${joined}@${k}, "; done
-  echo "- **Code:** ${joined%, }"
+# Run main only when executed directly; when sourced (tests) expose the helpers.
+if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
+  main "$@"
 fi
-# "Reported" credits humans who reported a fixed issue but are NOT already in
-# Code — so each person is credited once and this line highlights the reporters
-# (typically community members) who didn't also author a PR. This also drops a
-# maintainer's self-reported issues, since maintainers appear under Code.
-line="- **Reported issues fixed here:** "; any=0
-for k in $(printf '%s\n' "${!REPORTERS[@]}" | sort -f); do
-  [ -n "${CODE["$k"]:-}" ] && continue
-  # paste -d takes the delimiter chars cyclically, so -d', ' would alternate
-  # "," and " " between fields — join with a single "," then expand to ", ".
-  refs="$(printf '%s\n' "${REPORTERS["$k"]}" | tr ' ' '\n' | grep . | sort -un | sed 's/^/#/' | paste -sd, - | sed 's/,/, /g')"
-  line="${line}@${k} (${refs}), "; any=1
-done
-[ "$any" -eq 1 ] && echo "${line%, }"

skills/github-release/scripts/tests/harvest-contributors.test.sh

@@ -0,0 +1,61 @@
+#!/usr/bin/env bash
+# Self-test for harvest-contributors.sh pure helpers — no network.
+#
+# Pins the regression fixed alongside it: PR numbers must come ONLY from the
+# squash-merge "(#N)" subject suffix or a "Merge pull request #N" subject — never
+# from body cross-references, dependency-bump changelog excerpts, or the
+# "​" HTML entity (which used to be grepped as PR "#8203").
+set -uo pipefail
+
+HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+# shellcheck source=../harvest-contributors.sh
+source "$HERE/../harvest-contributors.sh"
+set +e # drive assertions ourselves; sourcing turned on `set -e`
+
+fail=0
+check() { # check <name> <expected> <actual>
+  if [ "$2" = "$3" ]; then
+    printf 'ok   - %s\n' "$1"
+  else
+    printf 'FAIL - %s\n       expected: [%s]\n       actual:   [%s]\n' "$1" "$2" "$3"
+    fail=1
+  fi
+}
+
+ex() { printf '%s\n' "$@" | extract_pr_numbers | paste -sd, -; }
+
+# --- extract_pr_numbers ---
+check "squash suffix"                 "842" "$(ex 'docs(readme): restructure (#842)')"
+check "two refs -> trailing PR only"  "847" "$(ex 'fix: aspect ratio (#846) (#847)')"
+check "merge-commit subject"          "853" "$(ex 'Merge pull request #853 from netresearch/x')"
+check "trailing space after (#N)"     "843" "$(ex 'feat: y (#843) ')"
+check "trailing tab after (#N)"       "844" "$(printf 'feat: z (#844)\t' | extract_pr_numbers | paste -sd, -)"
+
+# Regression: body / changelog-excerpt / entity lines must yield NOTHING.
+check "changelog '&#8203;' + foreign #refs -> none" "" \
+  "$(ex 'to v4]  -  by [@hi-ogawa] in &#8203; #10450' '[#&#8203;2439](https://github.com/actions/checkout/pull/2439)')"
+check "body cross-ref -> none"        "" "$(ex 'follow-up to #114, see #837')"
+check "plain subject -> none"         "" "$(ex 'docs: update readme')"
+
+# Realistic mixed set: only the real PR stamps survive, sorted-unique.
+check "mixed set -> 842,847,853,855" "842,847,853,855" \
+  "$(ex 'docs(readme): restructure (#842)' \
+        'chore(deps): bump undici 7.25.0 to 7.28.0 &#8203; #10450 (#855)' \
+        'fix: aspect ratio (#846) (#847)' \
+        'Merge pull request #853 from x' \
+        'follow-up to #114')"
+
+# --- is_bot ---
+for b in 'dependabot[bot]' 'renovate' 'github-actions' 'copilot-pull-request-reviewer' 'gemini-code-assist'; do
+  is_bot "$b"; check "is_bot: $b" "0" "$?"
+done
+for h in 'marekskopal' 'CybotTM' 'dkochc'; do
+  is_bot "$h"; check "human: $h" "1" "$?"
+done
+
+if [ "$fail" -eq 0 ]; then
+  echo "All harvest-contributors self-tests passed."
+else
+  echo "Some harvest-contributors self-tests FAILED." >&2
+fi
+exit "$fail"