pr-review CI gate should gate on REQUIRED checks only — non-required CANCELLED *and* FAILED checks falsely block reviews

[don-petry]

Summary

compute_ci_status (scripts/lib/ci-status.sh) classifies any check that is not SUCCESS/SKIPPED/NEUTRAL/pending as failing — which sweeps in CANCELLED checks. dev-lead's own orchestration jobs (dev-lead / dispatch, dev-lead / ci-relay) are routinely cancelled by dev-lead's concurrency (superseded runs), leaving CANCELLED check-runs on the PR. The gate then reads the PR as ci-failing and pr-review skips it — even though nothing actually failed. FORCE_REVIEW=true does not bypass this (the CI gate precedes the force path).

Root cause

scripts/lib/ci-status.sh:

def is_success:
  .conclusion == "SUCCESS" or .conclusion == "SKIPPED" or .conclusion == "NEUTRAL" or .state == "SUCCESS";
...
elif ([$ext[] | select(is_success)] | length) == ($ext | length) then "passing"
else "failing"   # <-- CANCELLED, TIMED_OUT, etc. all land here

A CANCELLED check is neither pending nor success → falls through to failing.

Evidence (2026-06-12)

Of the PRs skipped as ci-failing in batch run 27376131558, only one has a real failure:

PR	real FAILUREs	CANCELLED	reality
#570	3	0	genuinely failing ✓ correct skip
#549	0	2	only cancelled dev-lead checks — false block
#521	0	2	false block
#528	0	4	false block
#483	0	6	false block

The cancelled checks are dev-lead orchestration jobs:

dev-lead / dispatch    COMPLETED  CANCELLED
dev-lead / ci-relay    COMPLETED  CANCELLED

This is currently blocking PR #606 (the fix for #574) — its only non-green checks are two CANCELLED dev-lead jobs, so the force-review run logged:

{"pr":".../pull/606","decision":"skip","reason":"ci-failing"}

Impact

A large share of the "PRs pending review" backlog is not genuinely failing CI — it's this false-positive. dev-lead's concurrency cancellations are a normal, frequent occurrence, so many PRs carry CANCELLED orchestration checks and are silently un-reviewable. Force-review can't rescue them.

Proposed fix (pick one or combine)

1. Treat CANCELLED as non-blocking in is_success (or as pending) — a cancelled check is not a failed check. Simplest, broad fix.
2. Extend is_own_check to also exclude dev-lead's orchestration jobs (dev-lead / dispatch, dev-lead / ci-relay, ^Dev-Lead) the same way it excludes the pr-review cascade's own checks — these aren't real CI signal for merge-readiness.
3. Distinguish "cancelled because superseded" (ignore) from "cancelled because the suite was aborted" (treat as pending/failing) if finer granularity is wanted.

Option 1 + 2 together is safest: ignore dev-lead's own orchestration checks, and don't treat a bare CANCELLED as a hard failure.

Related

• pr-review: PRs skipped as ci-pending are never re-reviewed after CI goes green #573 (re-trigger gap), pr-review: PRs with >300 changed files hard-fail the cascade (gh pr diff 406); define org standard for oversized PRs #574 (oversized-PR diff), advisory-bot gate strands PRs forever: pushedDate=null disables both absent-bot timeout fallbacks #577 (advisory-gate pushedDate) — all surfaced in the same "pr-review not working" investigation.

---

Filed from an investigation into "pr-review agent not working — many PRs pending review."

[don-petry]

Dev-Lead Implementation Plan

Issue: #608 — pr-review CI gate treats CANCELLED checks as failing

Scope

Treat CANCELLED check conclusions as non-blocking in compute_ci_status so dev-lead's concurrency-cancelled orchestration jobs no longer falsely classify a PR as ci-failing.

Approach

Implementing Option 1 from the issue (treat CANCELLED as non-blocking). Option 2 (filtering dev-lead's dispatch/ci-relay jobs) is intentionally not adopted: the existing design (and tests #21, #22, #28, #32) deliberately keeps in-progress dev-lead checks blocking because dev-lead can commit back to the PR branch, and filtering them risks a review racing a branch-mutating run. A CANCELLED check is terminal, not in-progress, so Option 1 fixes exactly the reported false-positive without weakening that invariant. CANCELLED is classified as non-blocking (success-equivalent), not pending, so the PR reaches passing and becomes reviewable (pending would leave it perpetually skipped, since superseded checks never complete).

Implementation Checklist

• scripts/lib/ci-status.sh — classify CANCELLED conclusion as non-blocking; update header doc comment
• tests/test_ci_status.bats — add CANCELLED test cases

Tests to Write

• Single CANCELLED external check → passing
• Single CANCELLED dev-lead dispatch check → passing
• CANCELLED + external SUCCESS → passing
• CANCELLED + external FAILURE → failing (regression guard)
• CANCELLED + external IN_PROGRESS → pending (regression guard)
• Rollup of only CANCELLED dev-lead orchestration checks (dispatch + ci-relay) → passing

Test Command

bats tests/test_ci_status.bats (full suite); lint via bash .dev-lead/scripts/dev-lead-lint.sh

[don-petry]

Dev-Lead: Implementation Complete

Plan Execution

• scripts/lib/ci-status.sh — CANCELLED conclusions now classify as non-blocking (success-equivalent), not failing; header + inline docs updated
• tests/test_ci_status.bats — added 8 CANCELLED test cases (TDD: written failing first, then made green)

Approach

Implemented Option 1 only (treat CANCELLED as non-blocking). Added an is_cancelled predicate and folded it into the "passing" condition (select(is_success or is_cancelled)). Because is_pending is evaluated first, a CANCELLED check among in-progress checks still yields pending, and among a real FAILURE still yields failing — only an all-cancelled / cancelled+success rollup now reaches passing.

Option 2 (filtering dev-lead's dispatch/ci-relay) was intentionally not adopted. The existing is_own_check design deliberately keeps in-progress dev-lead checks blocking (header lines 28-30, tests #21/#22/#28/#32) because dev-lead can commit back to the PR branch and a review must not race a branch-mutating run. Option 1 fixes the exact reported false-positive (terminal CANCELLED checks) without weakening that invariant. CANCELLED is non-blocking rather than pending so the PR reaches passing and is reviewable — a superseded check never completes, so pending would leave it permanently un-reviewable.

Test Results

$ bats tests/test_ci_status.bats
...
ok 33 single external CANCELLED check returns passing
ok 34 CANCELLED dev-lead 'dispatch' check returns passing
ok 35 CANCELLED 'dev-lead / dispatch' check returns passing
ok 36 CANCELLED check alongside external passing → passing
ok 37 CANCELLED check alongside external FAILURE → failing (regression guard)
ok 38 CANCELLED check alongside external IN_PROGRESS → pending (regression guard)
ok 39 rollup of only CANCELLED dev-lead orchestration checks → passing (issue #608 repro)
ok 40 status context CANCELLED-equivalent: real TIMED_OUT conclusion still → failing
...
45 tests, 0 failures

All 45 ci-status tests pass (37 prior + 8 new). bash .dev-lead/scripts/dev-lead-lint.sh passes (shellcheck on 45 files + agent-profile validation).

Files Changed

• scripts/lib/ci-status.sh: add is_cancelled predicate; classify CANCELLED as non-blocking in the passing condition; update doc comments.
• tests/test_ci_status.bats: add 8 CANCELLED-handling tests incl. the pr-review CI gate should gate on REQUIRED checks only — non-required CANCELLED *and* FAILED checks falsely block reviews #608 repro and TIMED_OUT/FAILURE/IN_PROGRESS regression guards.

Notes

• Repro case (chore(deps): bump the actions group across 1 directory with 6 updates #570/feat: implement issue #69 — Compliance: check-suite-auto-trigger-347564 #549/feat: implement issue #244 — [Fleet Monitor] petry-projects/.github-private — ci.yml #521/feat: implement issue #405 — petry-projects — workflow failures detected 2026-05-28 #528/feat: implement issue #397 — Compliance: copilot-setup-steps-invalid-job-name #483, PR feat: implement issue #574 — pr-review: PRs with >300 changed files hard-fail the cascade (gh pr diff 406); define org standard for oversized PRs #606): a rollup of only CANCELLED dev-lead orchestration checks now classifies passing (test [LOW] Surface log-retrieval failures in health report #39), so pr-review no longer skips them as ci-failing.
• Other non-success terminal conclusions (TIMED_OUT, ACTION_REQUIRED, FAILURE) remain failing — only CANCELLED is whitelisted (test fix: harden rebase, review dismissal, and health log diagnostics #40).
• Out of scope: .dev-lead/scripts/lib/ci-status.sh is a gitignored runtime snapshot and was not modified.
• One unrelated pre-existing failure in tests/test_batch_fallback.bats (engine fallback, fix(engine): reorder cross-provider fallback to claude → copilot → gemini #571 area) exists on the base commit independent of this change — not touched.

[don-petry]

Broadening scope: this is the FAILED variant too, not just CANCELLED

The root cause in scripts/lib/ci-status.sh (compute_ci_status) is that it (a) treats any non-SUCCESS/SKIPPED/NEUTRAL/pending check as failing, and (b) considers all checks rather than only the required ones. CANCELLED is one symptom; genuinely-failed non-required checks are another, and they caused a hard self-host deadlock:

Evidence — #571 (engine-fallback fix): the dev-lead / dispatch and dev-lead / ci-relay checks had conclusion=failure (the dev-lead agent running the old claude→gemini→copilot order hit the depleted Gemini → 429). Those checks are not required by any ruleset, yet the gate read the PR as ci-failing and pr-review skipped approving it — so the fix for the bug was blocked by the bug. It only merged via admin-merge.

Proposed fix (generalises this issue)

Gate on required checks only — derive the required set from the branch ruleset / protection (the same set that actually blocks merge) and ignore the conclusion of non-required checks entirely. This covers CANCELLED and FAILED non-required checks in one change, and would have prevented the #571 deadlock outright. (Fallback if required-set derivation is hard: at minimum treat CANCELLED and non-required FAILURE as non-blocking, and make FORCE_REVIEW=true actually bypass the CI gate.)

Related

• PR Review Agent never reviews at green CI — skips on ci-pending with no CI-completion re-trigger (clean PRs stuck at REVIEW_REQUIRED) #463 / pr-review: PRs skipped as ci-pending are never re-reviewed after CI goes green #573 — the companion friction: PRs skipped as ci-pending are never re-reviewed when CI goes green (no CI-completion re-trigger). Together these are the bulk of agent-PR merge friction.