diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c17e056..cb5c4d31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +### Skills + +- **`release`: `poll-pr-reviews.sh` counted zero Copilot inline comments on every PR** — Copilot authors its review as `copilot-pull-request-reviewer[bot]` but its inline comments as `Copilot`, and the counter matched comments against the review login, so `inline_comments.copilot` was a constant `0` — vacuously satisfying Step 7's "every inline comment has a reply" merge gate and letting real Copilot findings merge unanswered. Comment counting now matches a set of logins per reviewer (`GH_AW_COMMENT_LOGINS` / `COPILOT_COMMENT_LOGINS` at the top of the script); gh-aw and `dismiss-stale-reviews.sh` were unaffected. Full motivation: PR #180. + ## 0.3.92 — 2026-07-17 ### Rules diff --git a/skills/release/poll-pr-reviews.sh b/skills/release/poll-pr-reviews.sh index 87b84d58..b109763d 100755 --- a/skills/release/poll-pr-reviews.sh +++ b/skills/release/poll-pr-reviews.sh @@ -32,6 +32,25 @@ set -euo pipefail +# Bot logins, by surface. A reviewer does NOT necessarily author its reviews +# and its inline comments under the same login: +# +# surface gh-aw Copilot +# --------------- --------------------- -------------------------------- +# review github-actions[bot] copilot-pull-request-reviewer[bot] +# inline comment github-actions[bot] Copilot +# +# Counting Copilot's comments against its REVIEW login matches nothing, so +# `inline_comments.copilot` reads 0 on every PR — which vacuously satisfies the +# release skill's Step 7 "every inline comment has a reply" merge gate and lets +# a real Copilot finding merge unanswered. Comment counting therefore matches a +# SET of logins per reviewer. A comment carries exactly one author, so listing +# both logins cannot double-count. +GH_AW_REVIEW_LOGIN="github-actions[bot]" +COPILOT_REVIEW_LOGIN="copilot-pull-request-reviewer[bot]" +GH_AW_COMMENT_LOGINS=("github-actions[bot]") +COPILOT_COMMENT_LOGINS=("Copilot" "copilot-pull-request-reviewer[bot]") + # `--paginate` is mandatory: GitHub's default per-page is 30, and a PR # with more than that many reviews/comments would otherwise return only # the first page. The script's `last` filter would then pick the last @@ -52,12 +71,18 @@ latest_review_by() { else {state, submitted_at, body} end' } +# Count top-level (non-reply) inline comments authored by ANY of . +# Variadic on login so one reviewer's multiple author identities collapse to a +# single count — see the login table at the top of this file. toplevel_comments_by() { - local owner="$1" repo="$2" pr="$3" login="$4" + local owner="$1" repo="$2" pr="$3"; shift 3 + local logins_json + logins_json=$(jq -n '$ARGS.positional' --args "$@") \ + || { echo "error: failed to encode login list for comment count" >&2; return 1; } gh api --paginate "repos/${owner}/${repo}/pulls/${pr}/comments?per_page=100" \ - | jq -s --arg login "$login" ' + | jq -s --argjson logins "$logins_json" ' (add // []) - | [.[] | select(.user.login == $login) | select(.in_reply_to_id == null)] + | [.[] | select(.in_reply_to_id == null) | select(.user.login | IN($logins[]))] | length' } @@ -99,13 +124,13 @@ main() { || { echo "error: failed to fetch merge state for ${owner}/${repo}#${pr_number} — run 'gh auth status' to verify auth, then retry 'gh pr view ${pr_number} --repo ${owner}/${repo} --json mergeStateStatus,mergeable' to inspect the failing call directly" >&2; exit 1; } local gh_aw_review copilot_review gh_aw_comments copilot_comments - gh_aw_review=$(latest_review_by "$owner" "$repo" "$pr_number" "github-actions[bot]") \ + gh_aw_review=$(latest_review_by "$owner" "$repo" "$pr_number" "$GH_AW_REVIEW_LOGIN") \ || { echo "error: failed to fetch gh-aw review state" >&2; exit 1; } - copilot_review=$(latest_review_by "$owner" "$repo" "$pr_number" "copilot-pull-request-reviewer[bot]") \ + copilot_review=$(latest_review_by "$owner" "$repo" "$pr_number" "$COPILOT_REVIEW_LOGIN") \ || { echo "error: failed to fetch Copilot review state" >&2; exit 1; } - gh_aw_comments=$(toplevel_comments_by "$owner" "$repo" "$pr_number" "github-actions[bot]") \ + gh_aw_comments=$(toplevel_comments_by "$owner" "$repo" "$pr_number" "${GH_AW_COMMENT_LOGINS[@]}") \ || { echo "error: failed to count gh-aw inline comments" >&2; exit 1; } - copilot_comments=$(toplevel_comments_by "$owner" "$repo" "$pr_number" "copilot-pull-request-reviewer[bot]") \ + copilot_comments=$(toplevel_comments_by "$owner" "$repo" "$pr_number" "${COPILOT_COMMENT_LOGINS[@]}") \ || { echo "error: failed to count Copilot inline comments" >&2; exit 1; } jq -n \ diff --git a/skills/release/tests/test_poll_pr_reviews.sh b/skills/release/tests/test_poll_pr_reviews.sh index e306becd..8e6f2f93 100755 --- a/skills/release/tests/test_poll_pr_reviews.sh +++ b/skills/release/tests/test_poll_pr_reviews.sh @@ -234,6 +234,46 @@ t_toplevel_comments_by_returns_zero_for_no_comments() { assert_eq "comments count for empty" "0" "$count" } +# Copilot authors its REVIEW as `copilot-pull-request-reviewer[bot]` but its +# INLINE COMMENTS as `Copilot`. Counting comments against the review login +# matched nothing, so `inline_comments.copilot` read 0 on every PR — vacuously +# satisfying the release skill's Step 7 "every inline comment has a reply" gate +# and letting a real Copilot finding merge unanswered. +t_toplevel_comments_by_counts_copilot_login() { + MOCK_COMMENTS_BODY='[{"user":{"login":"Copilot"},"in_reply_to_id":null,"path":"a/b.md","body":"Real finding."}]' + local count + count=$(toplevel_comments_by "owner" "repo" "1" "${COPILOT_COMMENT_LOGINS[@]}") + assert_eq "Copilot-authored comment is counted" "1" "$count" +} + +# The review login must stay in the comment set: a regression that swapped one +# login for the other instead of matching both would pass the test above and +# still lose comments on any PR where Copilot posts under the review login. +t_toplevel_comments_by_matches_either_copilot_login() { + MOCK_COMMENTS_BODY='[{"user":{"login":"Copilot"},"in_reply_to_id":null},{"user":{"login":"copilot-pull-request-reviewer[bot]"},"in_reply_to_id":null}]' + local count + count=$(toplevel_comments_by "owner" "repo" "1" "${COPILOT_COMMENT_LOGINS[@]}") + assert_eq "both Copilot logins counted" "2" "$count" +} + +t_toplevel_comments_by_excludes_replies_and_other_logins() { + MOCK_COMMENTS_BODY='[{"user":{"login":"Copilot"},"in_reply_to_id":null},{"user":{"login":"Copilot"},"in_reply_to_id":991},{"user":{"login":"some-human"},"in_reply_to_id":null}]' + local count + count=$(toplevel_comments_by "owner" "repo" "1" "${COPILOT_COMMENT_LOGINS[@]}") + assert_eq "replies and foreign logins excluded" "1" "$count" +} + +# End-to-end through main() — the path the Step 7 merge gate actually reads. +# gh-aw and Copilot counts must not bleed into each other. +t_main_counts_copilot_comments_in_snapshot() { + MOCK_MERGE_STATE=clean + MOCK_COMMENTS_BODY='[{"user":{"login":"Copilot"},"in_reply_to_id":null},{"user":{"login":"github-actions[bot]"},"in_reply_to_id":null},{"user":{"login":"github-actions[bot]"},"in_reply_to_id":null}]' + local out counts + out=$(main "owner" "repo" "1") + counts=$(echo "$out" | jq -r '.inline_comments | "\(.gh_aw)|\(.copilot)"') + assert_eq "inline_comments {gh_aw|copilot}" "2|1" "$counts" +} + # --- driver --- echo "== poll-pr-reviews.sh tests ==" @@ -248,6 +288,10 @@ run "latest_review_by surfaces the review body text" t_latest_r run "latest_review_by ignores other logins across pages" t_latest_review_by_filters_other_logins_across_pages run "toplevel_comments_by sums counts across pages (issue #83)" t_toplevel_comments_by_sums_across_pages run "toplevel_comments_by returns 0 for empty comments" t_toplevel_comments_by_returns_zero_for_no_comments +run "toplevel_comments_by counts the 'Copilot' comment login" t_toplevel_comments_by_counts_copilot_login +run "toplevel_comments_by matches either Copilot login" t_toplevel_comments_by_matches_either_copilot_login +run "toplevel_comments_by excludes replies and foreign logins" t_toplevel_comments_by_excludes_replies_and_other_logins +run "main counts Copilot comments in the snapshot" t_main_counts_copilot_comments_in_snapshot echo "== summary: ${PASS_COUNT} passed, ${FAIL_COUNT} failed ==" [[ "$FAIL_COUNT" -eq 0 ]]