diff --git a/.github/scripts/pr-auto-review/README.md b/.github/scripts/pr-auto-review/README.md index d3699d715..affd4d802 100644 --- a/.github/scripts/pr-auto-review/README.md +++ b/.github/scripts/pr-auto-review/README.md @@ -19,7 +19,8 @@ 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_ready STATE IS_DRAFT CHECKS_JSON REQUIRED_JSON SELF_NAME REVIEW_DECISION UNRESOLVED_COUNT` | the PR facts the workflow gathers (all as arguments — no stdin) | prints the **decision class** on stdout; `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_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 | ### The unified decision core — `pr_auto_review_ready` @@ -65,6 +66,31 @@ gate merge: `SELF_NAME` is this workflow's own check-run name; it is excluded from the gate so an in-progress run never blocks itself. +### Unresolved-thread semantics (issue #806) + +Criterion #4 blocks dispatch on open review threads, but not all open threads +should block. dev-lead's fix-review cycle frequently **addresses an advisory +finding (Copilot / Gemini / CodeRabbit) in a follow-up commit but never marks +the thread resolved**. The code is fixed and CI is green, yet the PR sits +`REVIEW_REQUIRED` until a human resolves the thread by hand. + +`pr_auto_review_blocking_thread_count` is the consumer-side, defense-in-depth +fix: it counts a thread as **blocking only when it is unresolved AND not +outdated**. GitHub sets `reviewThread.isOutdated == true` exactly when the diff +position the thread anchors to no longer exists at the current HEAD (the line +changed or the file moved) — a heuristic that the diff anchor shifted, not a +guarantee the concern was resolved. So an +unresolved-but-outdated thread — the signature of a fix that changed the flagged +line without resolving the thread — is treated as non-blocking, and the PR +converges without manual thread resolution. + +Fail-safe: only an explicit `isOutdated == true` makes a thread non-blocking. A +`null` or absent `isOutdated` on an unresolved thread still blocks, so a thread +whose staleness cannot be confirmed is never silently dropped. This is +defense-in-depth: the producer side (dev-lead resolving the threads it fixes) is +still the preferred fix; this gate just stops forgotten resolutions from +stalling otherwise-mergeable PRs. + ### Context name matching Rulesets store the bare context (e.g. a job name `Lint`, or a third-party status diff --git a/.github/scripts/pr-auto-review/lib/ready-check.sh b/.github/scripts/pr-auto-review/lib/ready-check.sh index 0d21d6538..d3f83614a 100644 --- a/.github/scripts/pr-auto-review/lib/ready-check.sh +++ b/.github/scripts/pr-auto-review/lib/ready-check.sh @@ -94,8 +94,37 @@ pr_auto_review_checks_ready() { [[ "$decision" == "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 +# .data.repository.pullRequest — and prints the count of threads that should +# BLOCK auto-dispatch: those that are unresolved AND not outdated. +# +# 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, +# so the unresolved-threads gate stalls the PR even though the code is fixed. +# GitHub sets reviewThread.isOutdated == true exactly when the diff position +# the thread anchors to no longer exists at HEAD (the line changed / file +# moved) — a heuristic that the diff anchor shifted, not a guarantee the +# underlying concern was resolved. Treating an +# unresolved-but-outdated thread as non-blocking clears the stall without the +# producer having to resolve the thread first. +# +# 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. +pr_auto_review_blocking_thread_count() { + jq -r ' + [ + (.data.repository.pullRequest.reviewThreads.nodes[]? | + select((.isResolved == false) and (.isOutdated != true)))? + ] | length + ' +} + # pr_auto_review_ready STATE IS_DRAFT CHECKS_JSON REQUIRED_JSON SELF_NAME \ -# REVIEW_DECISION UNRESOLVED_COUNT +# REVIEW_DECISION BLOCKING_THREAD_COUNT # Unified, pure readiness core for the pr-auto-review reusable workflow. Given # the PR facts gathered by the workflow's I/O glue, it evaluates all four # readiness criteria in gate order and PRINTS the decision class on stdout — @@ -114,7 +143,7 @@ pr_auto_review_checks_ready() { # pr_auto_review_required_contexts; may be []). # SELF_NAME this workflow's own check-run name, excluded from the gate. # REVIEW_DECISION effective review decision (gh: .reviewDecision; may be ""). -# UNRESOLVED_COUNT number of unresolved review threads (may be "" → 0). +# BLOCKING_THREAD_COUNT count of blocking threads — unresolved AND not outdated (may be "" → 0). # # Criteria are evaluated in order, so an earlier skip wins over a later one # (e.g. a draft PR that also has CHANGES_REQUESTED reports skip-draft). The @@ -122,7 +151,7 @@ pr_auto_review_checks_ready() { # so the required-vs-non-required behaviour (issue #680) is unchanged. pr_auto_review_ready() { local state="$1" is_draft="$2" checks_json="${3:-[]}" required_json="${4:-[]}" \ - self_name="$5" review_decision="$6" unresolved_count="${7:-0}" + self_name="$5" review_decision="$6" blocking_thread_count="${7:-0}" # 1. PR must be open and not a draft. if [ "$state" != "OPEN" ] || [ "$is_draft" = "true" ]; then @@ -148,9 +177,8 @@ pr_auto_review_ready() { return 1 fi - # 4. No unresolved review threads. - [ -z "$unresolved_count" ] && unresolved_count="0" - if [ "$unresolved_count" -gt 0 ]; then + # 4. No blocking review threads (unresolved AND not outdated). + if [ "$blocking_thread_count" -gt 0 ]; then echo "skip-unresolved-threads" return 1 fi diff --git a/.github/workflows/pr-auto-review-reusable.yml b/.github/workflows/pr-auto-review-reusable.yml index 6d3514e66..16bad0271 100644 --- a/.github/workflows/pr-auto-review-reusable.yml +++ b/.github/workflows/pr-auto-review-reusable.yml @@ -9,7 +9,10 @@ # failing / cancelled). Non-required advisory contexts are ignored — see # .github/scripts/pr-auto-review/lib/ready-check.sh (issue #680). # 3. Effective review decision is not CHANGES_REQUESTED -# 4. No unresolved review threads +# 4. No BLOCKING review threads. A thread blocks only when it is unresolved +# AND not outdated; an unresolved-but-outdated thread (its anchored code +# changed at HEAD — a heuristic, not a guarantee the concern was resolved) is non-blocking — see +# pr_auto_review_blocking_thread_count in ready-check.sh (issue #806). # # Triggered by (events forwarded from the thin caller): # workflow_run:completed — a named CI workflow finished green @@ -187,27 +190,41 @@ jobs: "/repos/${{ github.repository }}/actions/runs/${{ github.run_id }}/jobs" \ --jq '.jobs[0].name // empty' 2>/dev/null || echo "") - # Count unresolved review threads. + # Count BLOCKING review threads — unresolved AND not outdated. # REST API has no resolved field on review comments; GraphQL is # required. \$owner/\$repo/\$number are GraphQL variable references; # the backslash-dollar escaping prevents shell expansion while # keeping the literal $ that GraphQL expects. + # isOutdated (issue #806): a thread whose anchored diff position no + # longer exists at HEAD (line changed / file moved) is treated as + # non-blocking, so a dev-lead fix that addresses a finding in code + # but leaves the thread unresolved no longer stalls the gate. The + # unresolved-but-outdated → non-blocking rule lives in the pure, + # unit-tested pr_auto_review_blocking_thread_count (test/workflows/ + # pr-auto-review/blocking-threads.bats); this glue only passes the + # raw GraphQL response through it. # Known limitation: only the first 100 review threads are inspected; # PRs with more than 100 threads are not fully paginated. Accepted # as-is — such PRs are vanishingly rare across this org's repos. - UNRESOLVED=$(gh api graphql \ - -f "query=query(\$owner:String!,\$repo:String!,\$number:Int!){repository(owner:\$owner,name:\$repo){pullRequest(number:\$number){reviewThreads(first:100){nodes{isResolved}}}}}" \ + # Keep the transport fail-closed (no `|| true`): a gh/network/auth + # failure aborts the step under `set -e` — as before — rather than + # 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}}}}}' + THREADS_JSON=$(gh api graphql \ + -f "query=$_GQL" \ -f owner="${REPO%%/*}" \ -f repo="${REPO##*/}" \ - -F number="${PR_NUMBER}" \ - --jq "[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false)] | length") + -F number="${PR_NUMBER}") + BLOCKING_THREAD_COUNT=$(printf '%s' "$THREADS_JSON" | pr_auto_review_blocking_thread_count) # ── Decide (pure core) ─────────────────────────────────────────────── # The lib returns the decision class on stdout and exit 0 iff ready; # the glue only echoes it to $GITHUB_OUTPUT (Layer 2 telemetry reads it). if DECISION=$(pr_auto_review_ready \ "$STATE" "$IS_DRAFT" "$CHECKS" "$REQUIRED_JSON" \ - "$SELF_CHECK" "$REVIEW_DECISION" "$UNRESOLVED"); then + "$SELF_CHECK" "$REVIEW_DECISION" "$BLOCKING_THREAD_COUNT"); then READY=true else READY=false diff --git a/node_modules/.package-lock.json b/node_modules/.package-lock.json index 824cabc3d..5339ba19e 100644 --- a/node_modules/.package-lock.json +++ b/node_modules/.package-lock.json @@ -1,5 +1,5 @@ { - "name": "pr-837", + "name": "pr-840", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/package-lock.json b/package-lock.json index 8539506bd..beb0a3088 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "pr-837", + "name": "pr-840", "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 new file mode 100644 index 000000000..4349fc09a --- /dev/null +++ b/test/workflows/pr-auto-review/blocking-threads.bats @@ -0,0 +1,152 @@ +#!/usr/bin/env bats +# Tests for pr_auto_review_blocking_thread_count in +# .github/scripts/pr-auto-review/lib/ready-check.sh +# +# Pins issue #806: dev-lead's fix-review cycle addresses an advisory finding in a +# follow-up commit but frequently never marks the corresponding review thread +# resolved, so the PR stalls REVIEW_REQUIRED on the unresolved-threads gate even +# though the code is fixed and CI is green. +# +# Consumer-side, defense-in-depth: a review thread that is unresolved but +# OUTDATED (its anchored diff position no longer exists at HEAD — line changed / +# file moved) no longer blocks auto-dispatch. GitHub sets reviewThread.isOutdated +# when the diff anchor shifts (a heuristic, not proof the concern is fixed), so +# unresolved-but-outdated threads stop blocking without requiring the producer to +# resolve them. +# +# The function reads the `gh api graphql` reviewThreads response on stdin (each +# node exposing .isResolved and .isOutdated) and prints the count of *blocking* +# threads — unresolved AND not outdated. + +load 'helpers/setup' + +setup() { + # shellcheck source=/dev/null + . "${TT_SCRIPTS_DIR}/lib/ready-check.sh" +} + +# Build a GraphQL-shaped response from a raw nodes array, matching the shape the +# reusable workflow passes: .data.repository.pullRequest.reviewThreads.nodes +resp() { + printf '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":%s}}}}}' "$1" +} + +# ── empty / trivial ────────────────────────────────────────────────────────── + +@test "blocking count: no threads → 0" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[]')" + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +@test "blocking count: all resolved → 0" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[{"isResolved":true,"isOutdated":false},{"isResolved":true,"isOutdated":true}]')" + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +# ── the blocking case: still applies to HEAD ───────────────────────────────── + +@test "blocking count: unresolved and NOT outdated → 1 (still blocks)" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[{"isResolved":false,"isOutdated":false}]')" + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +# ── the #806 fix: outdated thread (diff anchor shifted at HEAD) → non-blocking ─ + +@test "blocking count: unresolved but OUTDATED → 0 (diff anchor shifted, non-blocking)" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[{"isResolved":false,"isOutdated":true}]')" + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +@test "blocking count: the #805 scenario — three fixed advisory threads all outdated → 0" { + # Copilot dep-check + two Gemini temp-file findings, each fixed in a follow-up + # commit that changed the anchored line, so all three threads are outdated. + run pr_auto_review_blocking_thread_count <<<"$(resp '[ + {"isResolved":false,"isOutdated":true}, + {"isResolved":false,"isOutdated":true}, + {"isResolved":false,"isOutdated":true} + ]')" + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +# ── mixed sets: only unresolved-and-current threads count ───────────────────── + +@test "blocking count: mixed set counts only unresolved-and-current threads" { + # resolved(current), resolved(outdated), unresolved+outdated, unresolved+current + run pr_auto_review_blocking_thread_count <<<"$(resp '[ + {"isResolved":true,"isOutdated":false}, + {"isResolved":true,"isOutdated":true}, + {"isResolved":false,"isOutdated":true}, + {"isResolved":false,"isOutdated":false} + ]')" + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +@test "blocking count: several unresolved-and-current threads → their count" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[ + {"isResolved":false,"isOutdated":false}, + {"isResolved":false,"isOutdated":false}, + {"isResolved":false,"isOutdated":true} + ]')" + [ "$status" -eq 0 ] + [ "$output" = "2" ] +} + +# ── fail-safe: missing / null isOutdated on an unresolved thread still blocks ─ + +@test "blocking count: unresolved thread with null isOutdated → 1 (fail safe: still blocks)" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[{"isResolved":false,"isOutdated":null}]')" + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +@test "blocking count: unresolved thread with absent isOutdated → 1 (fail safe: still blocks)" { + run pr_auto_review_blocking_thread_count <<<"$(resp '[{"isResolved":false}]')" + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +# ── robustness: GraphQL error / missing-data bodies yield 0 ────────────────── + +@test "blocking count: GraphQL error body (no data) → 0" { + run pr_auto_review_blocking_thread_count <<<'{"errors":[{"message":"Could not resolve to a Repository"}]}' + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +@test "blocking count: null nodes → 0" { + run pr_auto_review_blocking_thread_count <<<"$(resp 'null')" + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +# ── null-safety: absent / null intermediate fields → 0 ────────────────────── + +@test "blocking count: absent .data key → 0" { + run pr_auto_review_blocking_thread_count <<<'{}' + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +@test "blocking count: null .data → 0" { + run pr_auto_review_blocking_thread_count <<<'{"data":null}' + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +@test "blocking count: absent .data.repository → 0" { + run pr_auto_review_blocking_thread_count <<<'{"data":{}}' + [ "$status" -eq 0 ] + [ "$output" = "0" ] +} + +@test "blocking count: null .data.repository → 0" { + run pr_auto_review_blocking_thread_count <<<'{"data":{"repository":null}}' + [ "$status" -eq 0 ] + [ "$output" = "0" ] +}