Skip to content

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

Closed
#682
Closed
pr-auto-review ready-check counts non-required/cancelled checks → false 'not passing', blocks auto-dispatch#680
#682
Labels
dev-leadFor dev-lead agent pickup

Description

@don-petry
don-petry
opened on Jul 12, 2026

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.

Activity

  1. don-petry
    added
    dev-leadFor dev-lead agent pickup
    on Jul 12, 2026

  2. don-petry commented on Jul 12, 2026

    @don-petry
    don-petry
    on Jul 12, 2026
    ContributorAuthor

    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 Command

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

  3. don-petry commented on Jul 12, 2026

    @don-petry
    don-petry
    on Jul 12, 2026
    ContributorAuthor

    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.
  4. don-petry
    closed this as completedin #682on Jul 12, 2026
  5. don-petry
    added a commit that references this issue on Jul 12, 2026

    feat: implement issue #680 — pr-auto-review ready-check counts non-re…

    4ab3812
  6. don-petry
    added a commit that references this issue on Aug 7, 2026

    feat: implement issue #680 — pr-auto-review ready-check counts non-re…

    89bc10d
  7. don-petry
    added a commit that references this issue on Aug 31, 2026

    feat: implement issue #680 — pr-auto-review ready-check counts non-re…

    1b0175b
  8. don-petry
    added a commit that references this issue on Sep 7, 2026

    feat: implement issue #680 — pr-auto-review ready-check counts non-re…

    abb31de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    dev-leadFor dev-lead agent pickup

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions