don-petry · Sep 19, 2026 · Sep 19, 2026 · Sep 19, 2026
@@ -155,6 +155,7 @@
             tests/test_review_one_pr_ci_pending.bats \
             tests/test_review_one_pr_decline_verdict.bats \
             tests/test_review_one_pr_force_ci_failing.bats \
+            tests/test_review_one_pr_nonrequired_gate.bats \
             tests/test_review_one_pr_force_re_review.bats \
             tests/test_review_one_pr_metadata_rearm.bats \
             tests/test_review_one_pr_claim_observe.bats \
@@ -257,7 +258,7 @@
    timeout-minutes: 5
    steps:
      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1  # v7.0.1
        # NOTE: persist-credentials is intentionally left at the default (true) —
        # vci_resolve_reusable needs `git fetch origin` to authenticate against this
        # private repo when resolving same-repo channel tags. Setting
        # persist-credentials: false would silently push every such caller into the

@@ -10,21 +10,34 @@
 # (../../interaction-contracts) relative to the script, independent of CWD.
 _CI_STATUS_LIB_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 
-# compute_ci_status <rollup_json>
+# compute_ci_status <rollup_json> [required_names_json]
 #
 # Inputs:
 #   $1 — JSON array (the .statusCheckRollup field from `gh pr view`)
+#   $2 — (optional) JSON array of the branch ruleset's required status-check
+#        contexts, read from GET /repos/{owner}/{repo}/rules/branches/{branch}
+#        (#1795). When non-empty this is the AUTHORITATIVE required set.
 #
 # Outputs (stdout): one of "passing", "pending", "failing"
 #
-# Required-check gating (#1549): after own/agent filtering, if the rollup marks
-# any remaining check required (its .isRequired field, which GitHub computes from
-# the branch's rulesets AND classic protection), classification gates ONLY on
-# those required checks — a red NON-required check (a superseded
-# 'dev-lead / dispatch', or 'template-drift' from a repo-template Dependabot bump)
-# no longer blocks the review that would approve an otherwise-green PR. When no
-# remaining check is flagged required (isRequired absent, or no protection), the
-# gate falls back to evaluating every external check (fail-safe, unchanged).
+# Required-check gating (#1549, #1795): after own/agent filtering, classification
+# gates ONLY on the REQUIRED checks — a red NON-required check (a superseded
+# 'dev-lead / dispatch', or 'template-drift' from a repo-template Dependabot bump /
+# a standards baseline move) no longer blocks the review that would approve an
+# otherwise-green PR.
+#
+# The required set is the UNION of two signals, so it is never narrower than either:
+#   • ruleset names ($2) — the authoritative source read from the branch ruleset
+#     API by the caller (review-one-pr.sh). This repo protects `main` with a
+#     ruleset, not classic protection, so the rollup's own .isRequired field is
+#     frequently ABSENT (the #1795 deadlock: with nothing flagged required the
+#     #1549 fallback reverts to all-external and template-drift blocks every PR).
+#   • .isRequired == true — GitHub's per-entry verdict, honoured whenever present
+#     so a genuinely-required check can never slip the gate even if the ruleset
+#     name set is stale or a context string does not match.
+# When BOTH are empty (caller passed nothing / the ruleset API was unreadable —
+# fail closed), the gate falls back to evaluating EVERY external check (today's
+# behaviour, fail-safe): a genuine failure is never silently passed.
 #
 # Classification rules (after filtering own checks, over the gated set):
 #   passing — empty rollup, or every item is SUCCESS/SKIPPED/NEUTRAL/CANCELLED
@@ -133,11 +146,63 @@ _ci_status_agent_roles_json() {
   printf '%s' "$roles_json"
 }
 
+# ruleset_required_checks <owner/repo> <branch>
+#
+# Emit a JSON array of the branch's REQUIRED status-check contexts (#1795), read
+# from the ruleset API — the authoritative source compute_ci_status gates on so a
+# red NON-required check (e.g. `template-drift`) never yields `ci-failing`. Names
+# are NEVER hardcoded (a literal list would drift the same way the template
+# baseline did — that is the bug, not the fix).
+#
+# This repo protects `main` with a **ruleset**, so `GET /repos/{o}/{r}/rules/
+# branches/{branch}` is the source; the classic `/branches/{branch}/protection`
+# endpoint returns 404 here but is queried as a fallback for branches that still
+# use classic protection ("read the ruleset, or handle both").
+#
+# On ANY failure (both endpoints error or return nothing) it emits an EMPTY string.
+# The caller treats that as FAIL CLOSED: compute_ci_status then reverts to gating
+# on every failing check (today's behaviour), so an API hiccup can never silently
+# widen what gets approved — same posture as the router's unreadable-label guard.
+#
+# Lives here (not in review-one-pr.sh) so every compute_ci_status caller — the
+# stuck-review sweep, the stall/merge-ready scans — gates on the same authoritative
+# required set, closing the #1795 deadlock on those paths too (they previously
+# passed only the rollup and so reverted to blocking on red non-required checks).
+ruleset_required_checks() {
+  local repo="$1" branch="$2" out=""
+  [ -n "$repo" ] && [ -n "$branch" ] || return 0
+  # Ruleset API (primary): collect every required_status_checks rule's contexts.
+  out=$(gh api "repos/$repo/rules/branches/$branch" \
+          --jq '[.[] | select(.type == "required_status_checks")
+                      | .parameters.required_status_checks[]?.context]' 2>/dev/null) || out=""
+  if [ -z "$out" ] || [ "$out" = "[]" ]; then
+    # Classic branch-protection fallback (404 on ruleset-only branches like main).
+    local classic
+    classic=$(gh api "repos/$repo/branches/$branch/protection/required_status_checks" \
+                --jq '[.checks[]?.context] // []' 2>/dev/null) || classic=""
+    if [ -n "$classic" ] && [ "$classic" != "[]" ]; then
+      out="$classic"
+    fi
+  fi
+  printf '%s' "$out"
+}
+
 compute_ci_status() {
   local rollup_json="${1:-[]}"
+  local required_names="${2:-[]}"
   local agent_roles
   agent_roles="$(_ci_status_agent_roles_json)"
-  jq -r --argjson agent_roles "$agent_roles" "
+  # Normalise the ruleset required-name set: anything that is not a JSON array
+  # (empty string, malformed) becomes [] so the union degrades to the .isRequired
+  # fallback (fail closed) rather than erroring. Skip the jq subprocess for the
+  # common empty/[] case (rulesets unconfigured or the API read empty) — this runs
+  # inside the caller's polling loop.
+  if [ -z "$required_names" ] || [ "$required_names" = "[]" ]; then
+    required_names="[]"
+  else
+    required_names="$(jq -c 'if type == "array" then . else [] end' <<< "$required_names" 2>/dev/null || echo '[]')"
+  fi
+  jq -r --argjson agent_roles "$agent_roles" --argjson required_names "$required_names" "
     # A check carrying a non-empty conclusion is terminal, whatever its status
     # says (#1427, AC #3): GitHub can report a zombie check as IN_PROGRESS while
     # already carrying a terminal conclusion (observed on PR #1426:
@@ -159,14 +224,16 @@ compute_ci_status() {
     # not a real merge-readiness signal.
     def is_cancelled:
       .conclusion == \"CANCELLED\";
-    # A rollup entry is required iff GitHub marks it required for THIS PR — the
-    # .isRequired field on the statusCheckRollup entry, which reflects the target
-    # branch's rulesets AND classic branch protection combined (#1549). Gating on
-    # it means a red NON-required check (a superseded 'dev-lead / dispatch', or
-    # 'template-drift' from a repo-template Dependabot bump) no longer blocks the
-    # review that would post the approval — the circular skip in #1549.
+    # A rollup entry is required iff EITHER the branch ruleset names it (its
+    # .name/.context is in \$required_names, read from the ruleset API — #1795) OR
+    # GitHub flags the entry .isRequired == true (#1549). The union is never
+    # narrower than either signal, so a genuinely-required check can never slip the
+    # gate; a red NON-required check ('dev-lead / dispatch', or 'template-drift'
+    # from a baseline move / repo-template Dependabot bump) no longer blocks the
+    # review that would post the approval — the circular skip in #1549/#1795.
     def is_required:
-      (.isRequired == true);
+      (.isRequired == true) or
+      (((.name // .context // \"\") as \$n | (\$required_names | index(\$n)) != null));
     # Classify a list of checks: pending dominates, then all-green/cancelled,
     # else failing. An empty set is passing (nothing left to gate on).
     def classify(\$set):
@@ -192,6 +259,56 @@ compute_ci_status() {
   " <<< "$rollup_json" 2>/dev/null || echo "passing"
 }
 
+# ci_nonrequired_failures <rollup_json> [required_names_json]
+#
+# Surfacing helper for #1795 AC #4: emit (one per line) the names of external
+# (non-own, non-agent) checks that are FAILING but NOT required — i.e. neither in
+# the ruleset required-name set ($2) nor flagged .isRequired. These are the red
+# advisory checks a review proceeds PAST; the caller logs them and names them in
+# the review body so "ignored" never means "unmentioned". A failing check is any
+# terminal/non-success entry that is not pending, success/skipped/neutral, or
+# cancelled — mirroring compute_ci_status's classification so the two never drift.
+ci_nonrequired_failures() {
+  local rollup_json="${1:-[]}"
+  local required_names="${2:-[]}"
+  local agent_roles
+  agent_roles="$(_ci_status_agent_roles_json)"
+  if [ -z "$required_names" ] || [ "$required_names" = "[]" ]; then
+    required_names="[]"
+  else
+    required_names="$(jq -c 'if type == "array" then . else [] end' <<< "$required_names" 2>/dev/null || echo '[]')"
+  fi
+  jq -r --argjson agent_roles "$agent_roles" --argjson required_names "$required_names" "
+    def is_terminal:
+      (.conclusion != null and .conclusion != \"\");
+    def is_pending:
+      (is_terminal | not) and (
+        .status == \"IN_PROGRESS\" or .status == \"QUEUED\" or .status == \"WAITING\" or
+        .status == \"COMPLETED\"  or
+        .state  == \"PENDING\"     or .state  == \"EXPECTED\"
+      );
+    def is_success:
+      .conclusion == \"SUCCESS\" or .conclusion == \"SKIPPED\" or .conclusion == \"NEUTRAL\" or
+      .state == \"SUCCESS\";
+    def is_cancelled:
+      .conclusion == \"CANCELLED\";
+    def is_required:
+      (.isRequired == true) or
+      (((.name // .context // \"\") as \$n | (\$required_names | index(\$n)) != null));
+    $_CI_STATUS_JQ_IS_OWN_CHECK
+    $_CI_STATUS_JQ_IS_AGENT_CHECK
+    if (. == null or (type != \"array\")) then empty
+    else
+      .[]
+      | select((is_own_check or is_agent_check) | not)
+      | select(is_required | not)
+      | select((is_pending or is_success or is_cancelled) | not)
+      | (.name // .context // \"\")
+      | select(length > 0)
+    end
+  " <<< "$rollup_json" 2>/dev/null || true
+}
+
 # ci_pending_age_exceeded <rollup_json> <max_age_sec> [now_epoch]
 #
 # Bounded-deferral guard (issue #1427, AC #3/#4). Given the same external checks

@@ -153,7 +153,7 @@ while IFS= read -r pr; do
   # One snapshot gives review decision, mergeability, CI rollup (with check-run
   # timestamps), labels, title, url, and updatedAt.
   if ! snapshot=$(gh pr view "$pr" --repo "$REPO" \
-        --json mergeable,reviewDecision,statusCheckRollup,labels,title,url,updatedAt 2>/dev/null); then
+        --json mergeable,reviewDecision,statusCheckRollup,baseRefName,labels,title,url,updatedAt 2>/dev/null); then
     echo "  skip PR #${pr} — could not fetch (deleted, no access, or rate-limited)"
     scan_incomplete=true
     continue
@@ -165,13 +165,17 @@ while IFS= read -r pr; do
   html_url=$(jq -r '.url // ""' <<< "$snapshot")
   updated_at=$(jq -r '.updatedAt // ""' <<< "$snapshot")
 
-  # Pass the COMPLETE rollup: compute_ci_status now does the #1549 required-check
-  # gating internally (and its fail-safe fallback to all external checks when the
-  # rollup flags nothing required). Prefiltering to isRequired==true here would
-  # hand it an empty set whenever required metadata is absent (older gh CLI, or
-  # not yet propagated), which classifies as passing and silently bypasses the
-  # fallback — misreading a genuinely-failing PR as green.
-  ci_status=$(compute_ci_status "$(jq '.statusCheckRollup // []' <<< "$snapshot")")
+  # Pass the COMPLETE rollup PLUS the branch ruleset's REQUIRED-check set (#1795):
+  # this repo protects `main` with a ruleset, so the rollup's `.isRequired` field is
+  # frequently absent and gating on the rollup alone reverts to blocking on every red
+  # external check — a non-required `template-drift` failure would then hide an
+  # otherwise merge-ready PR from this scan. The ruleset set is the authoritative
+  # required signal (union'd with `.isRequired` inside compute_ci_status). Fail closed:
+  # an unreadable set leaves it empty and compute_ci_status gates on all failing
+  # checks (never misreading a genuinely-failing PR as green).
+  base_ref=$(jq -r '.baseRefName // ""' <<< "$snapshot")
+  required_checks=$(ruleset_required_checks "$REPO" "$base_ref" || true)
+  ci_status=$(compute_ci_status "$(jq '.statusCheckRollup // []' <<< "$snapshot")" "$required_checks")
 
   # Human-gate exclusions (AC#2). Only the current label state is consulted.
   labels_json=$(jq -c '[.labels[]?.name]' <<< "$snapshot" 2>/dev/null || echo '[]')

@@ -101,7 +101,7 @@ while IFS= read -r pr; do
   # One snapshot gives CI rollup, review decision, head, review/comment bodies,
   # labels, title, and url — the same shape sweep-stuck-reviews.sh consumes.
   if ! snapshot=$(gh pr view "$pr" --repo "$REPO" \
-        --json headRefOid,statusCheckRollup,reviewDecision,reviews,comments,labels,title,url,updatedAt,autoMergeRequest 2>/dev/null); then
+        --json headRefOid,baseRefName,statusCheckRollup,reviewDecision,reviews,comments,labels,title,url,updatedAt,autoMergeRequest 2>/dev/null); then
     echo "  skip PR #${pr} — could not fetch (deleted, no access, or rate-limited)"
     scan_incomplete=true
     continue
@@ -113,7 +113,12 @@ while IFS= read -r pr; do
   html_url=$(jq -r '.url // ""' <<< "$snapshot")
   updated_at=$(jq -r '.updatedAt // ""' <<< "$snapshot")
 
-  ci_status=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$snapshot")")
+  # Gate on the branch ruleset's REQUIRED checks (#1795) so a red non-required check
+  # (e.g. template-drift) is not mistaken for a genuine stall signal. Fail closed:
+  # an unreadable set gates on all failing checks.
+  base_ref=$(jq -r '.baseRefName // ""' <<< "$snapshot")
+  required_checks=$(ruleset_required_checks "$REPO" "$base_ref" || true)
+  ci_status=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$snapshot")" "$required_checks")
 
   # Already reviewed at this exact head? The cascade stamps each review with
   # `<!-- pr-review-agent v1 sha=<HEAD> -->`; a marker at the current head means