feat: implement issue #806 — dev-lead fix-review addresses advisory findings in code but never resolves the review thread → PRs stall on the advisory gate

.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

.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,15 +143,15 @@ 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
 #   required-checks gate (#2) is delegated verbatim to 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,9 @@ 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).
+  [ -z "$blocking_thread_count" ] && blocking_thread_count="0"
+  if [ "$blocking_thread_count" -gt 0 ]; then
     echo "skip-unresolved-threads"
     return 1
   fi

.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

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" ]
+}