diff --git a/.github/scripts/pr-auto-review/README.md b/.github/scripts/pr-auto-review/README.md index 106902768..b023632c4 100644 --- a/.github/scripts/pr-auto-review/README.md +++ b/.github/scripts/pr-auto-review/README.md @@ -19,7 +19,7 @@ Pure, side-effect-free helpers. Source the file, then call: |----------|-------|---------| | `pr_auto_review_required_contexts` | branch-rules JSON on stdin (`GET /repos/{owner}/{repo}/rules/branches/{branch}`) | prints a compact JSON array of required status-check context names (`[]` if none / non-array) | | `pr_auto_review_checks_ready REQUIRED_JSON SELF_NAME` | checks JSON on stdin (`gh pr checks --json bucket,name`) | prints a one-line reason; `0` ready, `1` not ready | -| `pr_auto_review_blocking_thread_count` | review-threads JSON on stdin (`reviewThreads(first:100){nodes{isResolved isOutdated}}`) | prints the count of **blocking** threads — unresolved AND not outdated | +| `pr_auto_review_blocking_thread_count` | review-threads JSON on stdin (`reviewThreads(first:100){nodes{isResolved isOutdated comments(first:100){nodes{author{__typename}}}}}`) | prints the count of **blocking** threads — unresolved AND not outdated AND not posted exclusively by advisory bots | | `pr_auto_review_ready STATE IS_DRAFT CHECKS_JSON REQUIRED_JSON SELF_NAME REVIEW_DECISION BLOCKING_THREAD_COUNT` | the PR facts the workflow gathers (all as arguments — no stdin) | prints the **decision class** on stdout; `0` ready, `1` not ready | ## `lib/sweep.sh` diff --git a/.github/scripts/pr-auto-review/lib/ready-check.sh b/.github/scripts/pr-auto-review/lib/ready-check.sh index d3f83614a..172392224 100644 --- a/.github/scripts/pr-auto-review/lib/ready-check.sh +++ b/.github/scripts/pr-auto-review/lib/ready-check.sh @@ -96,9 +96,10 @@ pr_auto_review_checks_ready() { # pr_auto_review_blocking_thread_count # Reads a review-threads GraphQL response on stdin — the payload of -# `reviewThreads(first:100){nodes{isResolved isOutdated}}` under +# `reviewThreads(first:100){nodes{isResolved isOutdated comments(first:100){nodes{author{__typename}}}}}` under # .data.repository.pullRequest — and prints the count of threads that should -# BLOCK auto-dispatch: those that are unresolved AND not outdated. +# BLOCK auto-dispatch: those that are unresolved AND not outdated AND not +# posted exclusively by advisory bots. # # Why isOutdated (issue #806): dev-lead's fix-review cycle often addresses an # advisory finding in a follow-up commit but never marks the thread resolved, @@ -110,15 +111,29 @@ pr_auto_review_checks_ready() { # unresolved-but-outdated thread as non-blocking clears the stall without the # producer having to resolve the thread first. # +# Why advisory-bot exclusion (issue #892): threads whose comments are all +# from advisory bots (author.__typename == "Bot") represent automated +# nitpick feedback, not human review requests. When CI is green and the only +# unresolved threads are advisory-bot posts, the PR should still be eligible +# for dispatch — otherwise the gate stalls on a human to resolve bot threads +# that dev-lead's fix-review correctly returns no-changes for (empty machine +# findings). A thread is advisory-bot-only when comments.nodes is non-empty +# and every author in that set has __typename == "Bot". +# # Fail-safe: only an explicit isOutdated == true makes a thread non-blocking; # a null / absent isOutdated on an unresolved thread still blocks, so a thread -# whose staleness we cannot confirm is never silently dropped. A GraphQL error -# body (no data / null nodes) yields 0. +# whose staleness we cannot confirm is never silently dropped. Similarly, a +# thread with absent or empty comments (cannot confirm all-bot) still blocks. +# A GraphQL error body (no data / null nodes) yields 0. pr_auto_review_blocking_thread_count() { jq -r ' + def is_advisory_bot_thread: + (.comments?.nodes? // []) as $c + | ($c | length) > 0 and ([$c[] | .author?.__typename? == "Bot"] | all); + [ (.data.repository.pullRequest.reviewThreads.nodes[]? | - select((.isResolved == false) and (.isOutdated != true)))? + select((.isResolved == false) and (.isOutdated != true) and (is_advisory_bot_thread | not)))? ] | length ' } diff --git a/.github/scripts/pr-auto-review/sweep-dispatch.sh b/.github/scripts/pr-auto-review/sweep-dispatch.sh index 2310c0bed..4843cfc81 100644 --- a/.github/scripts/pr-auto-review/sweep-dispatch.sh +++ b/.github/scripts/pr-auto-review/sweep-dispatch.sh @@ -104,7 +104,10 @@ evaluate_pr() { if [ -z "${required_json}" ]; then required_json="[]"; fi # shellcheck disable=SC2016 # $owner/$repo/$number are GraphQL variable refs, not shell vars - local gql='query($owner:String!,$repo:String!,$number:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$number){reviewThreads(first:100){nodes{isResolved isOutdated}}}}}' + local gql='query($owner:String!,$repo:String!,$number:Int!){repository(owner:$owner,name:$repo)' + # shellcheck disable=SC2016 + gql+='{pullRequest(number:$number){reviewThreads(first:100){nodes{isResolved isOutdated' + gql+=' comments(first:100){nodes{author{__typename}}}}}}}}' threads_json=$(gh api graphql \ -f "query=$gql" \ -f owner="${repo%%/*}" \ diff --git a/.github/workflows/pr-auto-review-reusable.yml b/.github/workflows/pr-auto-review-reusable.yml index 16bad0271..f6cda2fb3 100644 --- a/.github/workflows/pr-auto-review-reusable.yml +++ b/.github/workflows/pr-auto-review-reusable.yml @@ -211,7 +211,10 @@ jobs: # silently reporting 0 blocking threads and dispatching. The pure # function only has to tolerate a well-formed-but-empty payload. # shellcheck disable=SC2016 # $owner/$repo/$number are GraphQL variable refs, not shell vars - _GQL='query($owner:String!,$repo:String!,$number:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$number){reviewThreads(first:100){nodes{isResolved isOutdated}}}}}' + _GQL='query($owner:String!,$repo:String!,$number:Int!){repository(owner:$owner,name:$repo)' + # shellcheck disable=SC2016 + _GQL+='{pullRequest(number:$number){reviewThreads(first:100){nodes{isResolved isOutdated' + _GQL+=' comments(first:100){nodes{author{__typename}}}}}}}}' THREADS_JSON=$(gh api graphql \ -f "query=$_GQL" \ -f owner="${REPO%%/*}" \ diff --git a/node_modules/.package-lock.json b/node_modules/.package-lock.json index 5339ba19e..a80f4c2fb 100644 --- a/node_modules/.package-lock.json +++ b/node_modules/.package-lock.json @@ -1,5 +1,5 @@ { - "name": "pr-840", + "name": "pr-893", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/package-lock.json b/package-lock.json index beb0a3088..405cfe586 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "pr-840", + "name": "pr-893", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/test/workflows/pr-auto-review/blocking-threads.bats b/test/workflows/pr-auto-review/blocking-threads.bats index 4349fc09a..17be93ad8 100644 --- a/test/workflows/pr-auto-review/blocking-threads.bats +++ b/test/workflows/pr-auto-review/blocking-threads.bats @@ -150,3 +150,84 @@ resp() { [ "$status" -eq 0 ] [ "$output" = "0" ] } + +# ── issue #892: advisory-bot-only threads are non-blocking ─────────────────── +# A thread whose comments are exclusively from bots (author.__typename == "Bot") +# is advisory feedback, not a human review request. It should not block +# dispatch — otherwise the gate stalls on a human to resolve bot threads that +# dev-lead fix-review correctly returns no-changes for (empty machine findings). + +@test "blocking count: unresolved thread with all-bot comments → 0 (advisory bot, non-blocking)" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[{ + "isResolved":false,"isOutdated":false, + "comments":{"nodes":[{"author":{"__typename":"Bot"}}]} + }]')" + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +@test "blocking count: unresolved thread with User comment → 1 (human thread, still blocks)" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[{ + "isResolved":false,"isOutdated":false, + "comments":{"nodes":[{"author":{"__typename":"User"}}]} + }]')" + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +@test "blocking count: absent comments field → 1 (fail-safe: cannot confirm bot-only, still blocks)" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[{"isResolved":false,"isOutdated":false}]')" + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +@test "blocking count: empty comments nodes → 1 (fail-safe: no authors to confirm bot-only)" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[{ + "isResolved":false,"isOutdated":false, + "comments":{"nodes":[]} + }]')" + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +@test "blocking count: mixed bot and human threads — only human thread blocks" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[ + {"isResolved":false,"isOutdated":false,"comments":{"nodes":[{"author":{"__typename":"Bot"}}]}}, + {"isResolved":false,"isOutdated":false,"comments":{"nodes":[{"author":{"__typename":"User"}}]}}, + {"isResolved":false,"isOutdated":false,"comments":{"nodes":[{"author":{"__typename":"Bot"}}]}} + ]')" + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +@test "blocking count: thread with bot first comment and human reply → 1 (not purely advisory-bot)" { + # A thread opened by a bot but with a human follow-up must block. + # This pins the requirement that comments(first:100) is used in the GQL query + # rather than comments(first:1) — a first:1 query would only see the Bot + # comment and incorrectly classify the thread as advisory-bot-only. + run pr_auto_review_blocking_thread_count <<<"$(resp '[{ + "isResolved":false,"isOutdated":false, + "comments":{"nodes":[ + {"author":{"__typename":"Bot"}}, + {"author":{"__typename":"User"}} + ]} + }]')" + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +@test "blocking count: the #892 scenario — multiple advisory bot threads (gemini + codeant) → 0" { + # Scenario: CI green, CHANGES_REQUESTED from advisory bots (gemini-code-assist, + # codeant-ai), all 5 threads unresolved and not outdated. Without the fix, + # the gate counts 5 blocking threads and stalls dispatch even though every + # thread is advisory-bot feedback with no machine findings. + run pr_auto_review_blocking_thread_count <<<"$(resp '[ + {"isResolved":false,"isOutdated":false,"comments":{"nodes":[{"author":{"__typename":"Bot"}}]}}, + {"isResolved":false,"isOutdated":false,"comments":{"nodes":[{"author":{"__typename":"Bot"}}]}}, + {"isResolved":false,"isOutdated":false,"comments":{"nodes":[{"author":{"__typename":"Bot"}}]}}, + {"isResolved":false,"isOutdated":false,"comments":{"nodes":[{"author":{"__typename":"Bot"}}]}}, + {"isResolved":false,"isOutdated":false,"comments":{"nodes":[{"author":{"__typename":"Bot"}}]}} + ]')" + [ "$status" -eq 0 ] + [ "$output" = "0" ] +}