pr-auto-review ready-check counts non-required/cancelled checks → false 'not passing', blocks auto-dispatch

[don-petry]

Bug

The pr-auto-review ready-check (pr-auto-review-reusable.yml, check-and-dispatch job) gates dispatch on every check passing:

CHECKS=$(gh pr checks "$PR_URL" --json bucket,name ...)
# map(select(.name != $self)) | map(select(.bucket != "pass" and .bucket != "skipping")) | length

This counts non-required and cancelled check contexts as "not passing", so it refuses to dispatch the review agent on PRs that are actually mergeable.

Repro (observed on #678)

dev-lead's review-changes commits carry [skip ci-relay], and dev-lead's per-PR concurrency cancels superseded runs. Each leaves stale dev-lead / ci-relay + dev-lead / dispatch check runs in state CANCELLED. These contexts are not in the branch ruleset's required set (required = SonarCloud, CodeQL, AgentShield, Detect ecosystems, Lint, ShellCheck, Agent Security Scan). Yet the ready-check reported 4 of 24 check(s) are not yet passing — skipping and never dispatched, even though the PR was mergeable: MERGEABLE with all required checks green, review-approved-ready, zero unresolved threads.

Impact: the auto-approval path silently fails for exactly the PRs that go through a review round-trip (multiple dev-lead runs → concurrency cancellations). Had to fall back to manually dispatching the review agent + a normal merge.

Fix options (pick one, prefer A)

A. Only count REQUIRED checks. Fetch the branch's required contexts (ruleset rules/branches/<branch> → required_status_checks) and evaluate the passing-gate against that set, not all contexts. Most correct — mirrors what actually blocks merge.

B. Treat cancel (and skipping, already handled) as non-blocking. Add .bucket != "cancel" to the filter. Simpler, but still counts non-required failing checks. A cancelled advisory run is not a signal.

C. Use mergeStateStatus. Gate on BLOCKED vs CLEAN/UNSTABLE from the GraphQL merge state — UNSTABLE (non-required failing) is mergeable. Leans on GitHub's own required-vs-not determination.

Recommend A (explicit, testable) with B as a cheap immediate mitigation. Whatever the choice, extract the passing-gate into a pure helper + bats (the house pattern — Layer 0 of #668) so the decision matrix is pinned.

Acceptance

• A PR with all required checks green + approved-ready + no unresolved threads dispatches the review agent even when a non-required or cancelled advisory context (e.g. dev-lead / ci-relay) is present.
• Required-check failure/pending still blocks (unchanged).
• Decision logic unit-tested.

[don-petry]

Dev-Lead Implementation Plan

Issue: #680 — pr-auto-review ready-check counts non-required/cancelled checks → false 'not passing', blocks auto-dispatch

Scope

Extract the pr-auto-review passing-gate into a pure, bats-tested helper and switch it to evaluate only the branch's required status-check contexts (fix option A, with option B semantics as the no-required-set fallback), so non-required/cancelled advisory contexts (e.g. dev-lead / ci-relay) no longer block auto-dispatch.

Implementation Checklist

• .github/scripts/pr-auto-review/lib/ready-check.sh — new pure helpers:
  • pr_auto_review_required_contexts — jq transform of GET /repos/{o}/{r}/rules/branches/{b} → JSON array of required context names
  • pr_auto_review_checks_ready REQUIRED_JSON SELF_NAME — reads gh pr checks --json bucket,name on stdin; gates on required contexts only (fallback to fail/pending-only when no required set); prints reason, returns 0 ready / 1 not-ready
• .github/scripts/pr-auto-review/README.md — document the helper contract (house pattern, mirrors auto-rebase)
• .github/workflows/pr-auto-review-reusable.yml — add a checkout step (authenticated with the existing GH_PAT_WORKFLOWS, so no caller permission change is needed), source the lib, fetch required contexts for the PR base branch, and replace the inline NOT_PASSING gate with pr_auto_review_checks_ready
• .github/workflows/pr-auto-review-tests.yml — new CI gate: shellcheck + bats for the helper (mirrors auto-rebase-tests.yml)

Tests to Write

• test/workflows/pr-auto-review/helpers/setup.bash — sets TT_SCRIPTS_DIR
• test/workflows/pr-auto-review/ready-check.bats:
  • required all green + a cancelled non-required context present → ready (the feat: implement issue #677 — canary #668 increment 3: opt-in human confirmation at ring1→stable (require_confirmation, dev-lead) #678 repro)
  • required check failing / pending / cancelled / missing → not ready
  • non-required failing/cancelled context ignored
  • self check excluded
  • no required set → fallback: block only on fail/pending; cancel/skipping/pass ok
  • pr_auto_review_required_contexts extraction from a rules payload (and empty/none)

Test Command

bats --print-output-on-failure test/workflows/pr-auto-review/

[don-petry]

Dev-Lead: Implementation Complete

Fix option A (gate on required status-check contexts only), with option B semantics as the no-required-set fallback. The passing-gate is now a pure, bats-pinned helper (house pattern / Layer 0 of #668).

Plan Execution

• New pure helper .github/scripts/pr-auto-review/lib/ready-check.sh (pr_auto_review_required_contexts + pr_auto_review_checks_ready)
• Helper contract documented in .github/scripts/pr-auto-review/README.md
• Reusable workflow sources the helper (checkout authenticated with the existing GH_PAT_WORKFLOWS — no caller permission change required), resolves the base branch's required contexts, and replaces the inline all-checks gate
• New CI gate .github/workflows/pr-auto-review-tests.yml (shellcheck + bats)
• 17 bats tests written test-first (red → green)

Test Results

1..17
ok 1 required_contexts: extracts contexts from a required_status_checks rule
ok 2 required_contexts: no required_status_checks rule → empty array
ok 3 required_contexts: empty rules array → empty array
ok 4 required_contexts: non-array API error body → empty array
ok 5 ready: all required green, a CANCELLED non-required advisory present → ready
ok 6 ready: non-required FAILING advisory does not block
ok 7 not ready: a required check is failing
ok 8 not ready: a required check is pending
ok 9 not ready: a required check is cancelled
ok 10 not ready: a required context has no check run reported yet
ok 11 ready: a required check that is skipping counts as satisfied
ok 12 ready: bare required context matches an exact status check name
ok 13 ready: bare required job name matches a 'workflow / job' check name
ok 14 ready: this workflow's own pending check is excluded via SELF_NAME
ok 15 fallback: no required set, only cancel/skipping/pass non-self checks → ready
ok 16 fallback: no required set, a FAILING check → not ready
ok 17 fallback: no required set, a PENDING check → not ready

Also green: shellcheck -x on the lib, actionlint + yamllint on both workflow files, markdownlint on the README, and .dev-lead/scripts/dev-lead-lint.sh.

Files Changed

• .github/scripts/pr-auto-review/lib/ready-check.sh (new): pure passing-gate decision logic. Required set non-empty → block only on a required context that is missing, failing, pending, or cancelled; ignore every non-required/cancelled advisory context. Required set empty → fallback that blocks only on fail/pending.
• .github/scripts/pr-auto-review/README.md (new): helper contract + name-matching rules.
• .github/workflows/pr-auto-review-reusable.yml: checkout + source the helper; fetch baseRefName; resolve required contexts via GET /repos/{o}/{r}/rules/branches/{branch}; call pr_auto_review_checks_ready.
• .github/workflows/pr-auto-review-tests.yml (new): shellcheck + bats gate scoped to the pr-auto-review paths.
• test/workflows/pr-auto-review/ready-check.bats + helpers/setup.bash (new): the decision matrix, incl. the exact feat: implement issue #677 — canary #668 increment 3: opt-in human confirmation at ring1→stable (require_confirmation, dev-lead) #678 repro (all required green + a cancelled dev-lead / ci-relay → dispatches).

Acceptance

• ✅ All-required-green + a non-required/cancelled advisory context present → ready (bats test 5).
• ✅ Required-check failure/pending/cancelled/missing still blocks (tests 7–10).
• ✅ Decision logic unit-tested (17 cases).

Notes

• Name matching: rulesets store bare contexts (e.g. Lint, SonarCloud Code Analysis) while gh pr checks renders Actions checks as "<workflow> / <job>"; the helper matches on equality or a " / <name>" suffix in either direction, biased toward not false-blocking.
• Fail-open bias: a required context whose name can't be matched is treated as non-required (ignored) rather than blocking — the downstream merge/branch-protection gate still enforces it, so a mismatch can't silently merge an unready PR but also can't reintroduce the pr-auto-review ready-check counts non-required/cancelled checks → false 'not passing', blocks auto-dispatch #680 deadlock.
• Rubber-duck review caught and fixed an output-doubling bug: gh api writes its body to stdout on a 4xx, so $(gh api ... || echo "[]") would have produced two JSON values; the ruleset fetch now captures gh's exit status inside an if before transforming.
• No thin-caller or standards/workflows/pr-auto-review.yml change needed — authenticating the checkout with GH_PAT_WORKFLOWS avoids requiring a new contents: permission from callers.