From e28b2e7f1241307c3c38ffdc8d36959f21606231 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sat, 19 Sep 2026 13:12:20 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20implement=20issue=20#1795=20?= =?UTF-8?q?=E2=80=94=20Non-required=20failing=20check=20silently=20deadloc?= =?UTF-8?q?ks=20every=20merge:=20template-drift=20fails=20on=20all=20PRs,?= =?UTF-8?q?=20review=20agent=20skips=20as=20ci-failing,=20no=20approval=20?= =?UTF-8?q?can=20land?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/lint.yml | 1 + scripts/lib/ci-status.sh | 100 +++++++++--- scripts/review-one-pr.sh | 81 +++++++++- tests/test_ci_status.bats | 142 ++++++++++++++++++ .../test_review_one_pr_nonrequired_gate.bats | 139 +++++++++++++++++ 5 files changed, 442 insertions(+), 21 deletions(-) create mode 100644 tests/test_review_one_pr_nonrequired_gate.bats diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 0f23c415d..2767ba39b 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -155,6 +155,7 @@ jobs: 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 \ diff --git a/scripts/lib/ci-status.sh b/scripts/lib/ci-status.sh index 1c919bc26..846588c54 100644 --- a/scripts/lib/ci-status.sh +++ b/scripts/lib/ci-status.sh @@ -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 +# compute_ci_status [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 @@ -135,9 +148,14 @@ _ci_status_agent_roles_json() { 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. + required_names="$(jq -c 'if type == "array" then . else [] end' <<< "$required_names" 2>/dev/null || echo '[]')" + 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 +177,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 +212,52 @@ compute_ci_status() { " <<< "$rollup_json" 2>/dev/null || echo "passing" } +# ci_nonrequired_failures [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)" + required_names="$(jq -c 'if type == "array" then . else [] end' <<< "$required_names" 2>/dev/null || echo '[]')" + 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 [now_epoch] # # Bounded-deferral guard (issue #1427, AC #3/#4). Given the same external checks diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index 8229b5d00..dd877963b 100755 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -159,6 +159,42 @@ echo " artifact_type=$ARTIFACT_TYPE content_ref=$CONTENT_REF" echo " rubric=$REVIEW_RUBRIC" echo " output_channel=$REVIEW_OUTPUT_CHANNEL" +# ruleset_required_checks +# +# 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. +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" +} + # 1. Current head SHA + CI gate — single API call for both fields. # Strict CI classification: # pending — any item still running (IN_PROGRESS/QUEUED/WAITING/PENDING/EXPECTED @@ -168,12 +204,28 @@ echo " output_channel=$REVIEW_OUTPUT_CHANNEL" # NEUTRAL covers informational checks that don't gate merging. # failing — anything else (FAILURE, ACTION_REQUIRED, TIMED_OUT, CANCELLED, # STALE, STARTUP_FAILURE, or unknown conclusions) -PR_SNAPSHOT=$(gh pr view "$PR_URL" --json headRefOid,statusCheckRollup,reviewDecision,reviews,labels,comments,body,closingIssuesReferences) +PR_SNAPSHOT=$(gh pr view "$PR_URL" --json headRefOid,baseRefName,statusCheckRollup,reviewDecision,reviews,labels,comments,body,closingIssuesReferences) PR_HEAD_SHA=$(echo "$PR_SNAPSHOT" | jq -r '.headRefOid') export PR_HEAD_SHA echo " head SHA: $PR_HEAD_SHA" -CI_STATUS=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$PR_SNAPSHOT")") +# Read the branch ruleset's required-check set once (base branch is stable for the +# life of the PR). Passed into every compute_ci_status call so a failing check the +# ruleset does not require cannot by itself produce `ci-failing` (#1795 AC #2). +# Fail closed: an unreadable/empty set leaves REQUIRED_CHECKS_JSON empty and +# compute_ci_status reverts to gating on all failing checks (#1795 AC #3). +PR_BASE_REF=$(jq -r '.baseRefName // ""' <<< "$PR_SNAPSHOT") +_OWNER_REPO=$(printf '%s' "$PR_URL" | sed -E 's#^https?://[^/]+/([^/]+/[^/]+)/pull/[0-9]+.*#\1#') +REQUIRED_CHECKS_JSON=$(ruleset_required_checks "$_OWNER_REPO" "$PR_BASE_REF" || true) +if [ -n "$REQUIRED_CHECKS_JSON" ] && [ "$REQUIRED_CHECKS_JSON" != "[]" ]; then + echo " required checks (branch ruleset): $REQUIRED_CHECKS_JSON" +else + echo " required checks: none readable — failing closed (every failing check blocks)" + REQUIRED_CHECKS_JSON="" +fi +unset _OWNER_REPO + +CI_STATUS=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$PR_SNAPSHOT")" "$REQUIRED_CHECKS_JSON") echo " CI status: $CI_STATUS" REVIEW_DECISION=$(echo "$PR_SNAPSHOT" | jq -r '.reviewDecision // ""') @@ -258,7 +310,7 @@ if [ "$CI_STATUS" = "pending" ]; then echo " force-review: CI pending, waiting ${_FORCE_POLL_SEC}s for checks to settle (attempt ${_poll}/${_FORCE_POLL_MAX})" sleep "$_FORCE_POLL_SEC" _gh_poll_err=$(mktemp 2>/dev/null || echo "/tmp/cascade/gh-poll-$$.err") - if ! PR_SNAPSHOT=$(gh pr view "$PR_URL" --json headRefOid,statusCheckRollup,reviewDecision,reviews,labels,comments,body,closingIssuesReferences 2>"$_gh_poll_err"); then + if ! PR_SNAPSHOT=$(gh pr view "$PR_URL" --json headRefOid,baseRefName,statusCheckRollup,reviewDecision,reviews,labels,comments,body,closingIssuesReferences 2>"$_gh_poll_err"); then _gh_poll_err_content=$(cat "$_gh_poll_err" 2>/dev/null || true) rm -f "$_gh_poll_err" if is_rate_limited "$_gh_poll_err_content"; then @@ -272,7 +324,7 @@ if [ "$CI_STATUS" = "pending" ]; then rm -f "$_gh_poll_err" PR_HEAD_SHA=$(jq -r '.headRefOid' <<< "$PR_SNAPSHOT") export PR_HEAD_SHA - CI_STATUS=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$PR_SNAPSHOT")") + CI_STATUS=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$PR_SNAPSHOT")" "$REQUIRED_CHECKS_JSON") echo " CI status (poll ${_poll}): $CI_STATUS" _poll=$((_poll + 1)) done @@ -315,6 +367,20 @@ if [ "$CI_STATUS" = "pending" ]; then fi fi +# CI gate cleared. If any NON-required check is red, the gate deliberately let the +# review proceed past it (#1795 AC #2) — but downgrading a signal must not delete +# it (AC #4). Name each such advisory failure here (log) and export it so the +# review prompt surfaces it in the review body: "reviewed despite a red advisory +# check" stays visible rather than silent. Empty when nothing non-required is red. +NON_REQUIRED_CI_FAILURES=$( + ci_nonrequired_failures "$(jq '.statusCheckRollup' <<< "$PR_SNAPSHOT")" "$REQUIRED_CHECKS_JSON" \ + | paste -sd ',' - 2>/dev/null || true +) +if [ -n "$NON_REQUIRED_CI_FAILURES" ]; then + echo "::notice::proceeding past non-required failing check(s) the branch ruleset does not require: ${NON_REQUIRED_CI_FAILURES} — the merge gate still blocks on any failing REQUIRED check (#1795)" + export NON_REQUIRED_CI_FAILURES +fi + # Advisory bot review gate — instant check for advisory bot reviews (Gemini, Copilot, SonarCloud, Codex) # This ensures valid code reviews are incorporated before pr-review posts approval (issue #457). # @@ -979,6 +1045,13 @@ TRIAGE_PROMPT_FILE="/tmp/cascade/triage-prompt.md" else printf '\nADVISORY_BOT_FEEDBACK: (none)\n' fi + # #1795 AC #4: name any red NON-required check the CI gate proceeded past, so the + # review body records "reviewed despite a red advisory check" rather than hiding + # it. These do NOT gate merge (the ruleset does not require them); do not escalate + # or withhold approval solely because one is red — just name it. + if [ -n "${NON_REQUIRED_CI_FAILURES:-}" ]; then + printf '\nNON_REQUIRED_CI_FAILURES (advisory checks that are RED but NOT required by the branch ruleset — name these in your review body; they do NOT block merge and are not a reason to withhold approval or escalate):\n%s\n' "$NON_REQUIRED_CI_FAILURES" + fi # Inline the DOWNSTREAM_IMPACT block (no-op when the Story 5 flag is off, so # the triage prompt stays byte-identical to pre-feature behavior). downstream_impact_triage_section diff --git a/tests/test_ci_status.bats b/tests/test_ci_status.bats index 07a94ebd4..b168f0ff7 100644 --- a/tests/test_ci_status.bats +++ b/tests/test_ci_status.bats @@ -915,3 +915,145 @@ status_ctx_req() { run compute_ci_status "$r" [ "$output" = "passing" ] } + +# --------------------------------------------------------------------------- +# Ruleset-derived required-set gating (#1795). The rollup's own .isRequired field +# is not reliably populated for ruleset-protected branches (a ruleset, not classic +# protection, so the field is often absent — the #1795 deadlock: the fallback then +# reverts to all-external and template-drift blocks every PR again). The authoritative +# signal is the branch ruleset's required-status-check contexts, read from the API +# and passed to compute_ci_status as a second arg. When that set is NON-EMPTY the +# gate is the UNION of (name ∈ ruleset set) OR (.isRequired == true) — never narrower +# than either source, so a genuinely-required check can never slip the gate. When the +# set is empty/absent (API unreadable — fail closed), the existing .isRequired-then-all +# fallback applies (today's behaviour). +# --------------------------------------------------------------------------- + +# The five contexts the main ruleset requires today (issue #1795). Passed as the +# required-name set; template-drift is deliberately NOT among them. +REQUIRED_SET='["SonarCloud","CodeQL","agent-shield / AgentShield","dependency-audit / Detect ecosystems","duplicate-decl-gate"]' + +@test "#1795 ruleset gating: all required green + non-required 'template-drift' FAILURE → passing" { + local r + r=$(rollup \ + "$(check_run "SonarCloud" "COMPLETED" "SUCCESS")" \ + "$(check_run "CodeQL" "COMPLETED" "SUCCESS")" \ + "$(check_run "agent-shield / AgentShield" "COMPLETED" "SUCCESS")" \ + "$(check_run "dependency-audit / Detect ecosystems" "COMPLETED" "SUCCESS")" \ + "$(check_run "duplicate-decl-gate" "COMPLETED" "SUCCESS")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" "$REQUIRED_SET" + [ "$output" = "passing" ] +} + +@test "#1795 ruleset gating: a red REQUIRED check (name in ruleset set) → failing" { + local r + r=$(rollup \ + "$(check_run "SonarCloud" "COMPLETED" "FAILURE")" \ + "$(check_run "CodeQL" "COMPLETED" "SUCCESS")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" "$REQUIRED_SET" + [ "$output" = "failing" ] +} + +@test "#1795 ruleset gating: a pending REQUIRED check → pending (non-required red ignored)" { + local r + r=$(rollup \ + "$(check_run "CodeQL" "IN_PROGRESS")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" "$REQUIRED_SET" + [ "$output" = "pending" ] +} + +# The rollup does NOT carry .isRequired (the real ruleset-branch shape), yet the +# ruleset set still correctly gates only on the required contexts. This is the exact +# case #1549's .isRequired fallback could not clear. +@test "#1795 ruleset gating: no .isRequired anywhere, ruleset set supplied, only non-required red → passing" { + local r + r=$(rollup \ + "$(check_run "SonarCloud" "COMPLETED" "SUCCESS")" \ + "$(check_run "duplicate-decl-gate" "COMPLETED" "SUCCESS")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" "$REQUIRED_SET" + [ "$output" = "passing" ] +} + +# Fail closed: an empty required set (API unreadable) must NOT pass a genuine failure — +# it reverts to today's all-external behaviour. +@test "#1795 ruleset gating: empty required set (fail closed) + red non-required → failing" { + local r + r=$(rollup \ + "$(check_run "SonarCloud" "COMPLETED" "SUCCESS")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" '[]' + [ "$output" = "failing" ] +} + +# Union safety: even if the ruleset name set is stale/misses a context, a rollup +# entry GitHub flagged .isRequired:true is still gated (never weakens what blocks). +@test "#1795 ruleset gating: union honours .isRequired even when name not in ruleset set → failing" { + local r + r=$(rollup \ + "$(check_run_req "some-new-required-check" "COMPLETED" "FAILURE" true)" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" "$REQUIRED_SET" + [ "$output" = "failing" ] +} + +@test "#1795 ruleset gating: own/agent checks still filtered when ruleset set supplied" { + local r + r=$(rollup \ + "$(check_run "review / review" "COMPLETED" "FAILURE")" \ + "$(check_run "dev-lead / dispatch" "COMPLETED" "FAILURE")" \ + "$(check_run "SonarCloud" "COMPLETED" "SUCCESS")" \ + "$(check_run "CodeQL" "COMPLETED" "SUCCESS")" \ + "$(check_run "agent-shield / AgentShield" "COMPLETED" "SUCCESS")" \ + "$(check_run "dependency-audit / Detect ecosystems" "COMPLETED" "SUCCESS")" \ + "$(check_run "duplicate-decl-gate" "COMPLETED" "SUCCESS")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" "$REQUIRED_SET" + [ "$output" = "passing" ] +} + +# --------------------------------------------------------------------------- +# ci_nonrequired_failures (#1795 AC #4) — names the failing external checks that +# are neither in the ruleset required set nor flagged .isRequired, so a review that +# proceeds past a red advisory check can name it (never silently ignore it). +# --------------------------------------------------------------------------- + +@test "ci_nonrequired_failures: names a red non-required check" { + local r + r=$(rollup \ + "$(check_run "SonarCloud" "COMPLETED" "SUCCESS")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run ci_nonrequired_failures "$r" "$REQUIRED_SET" + [ "$output" = "template-drift" ] +} + +@test "ci_nonrequired_failures: a red REQUIRED check is NOT named (it blocks, not ignored)" { + local r + r=$(rollup \ + "$(check_run "SonarCloud" "COMPLETED" "FAILURE")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run ci_nonrequired_failures "$r" "$REQUIRED_SET" + [ "$output" = "template-drift" ] +} + +@test "ci_nonrequired_failures: nothing failing → empty" { + local r + r=$(rollup \ + "$(check_run "SonarCloud" "COMPLETED" "SUCCESS")" \ + "$(check_run "template-drift" "COMPLETED" "SUCCESS")") + run ci_nonrequired_failures "$r" "$REQUIRED_SET" + [ -z "$output" ] +} + +@test "ci_nonrequired_failures: own/agent red checks are not named" { + local r + r=$(rollup \ + "$(check_run "review / review" "COMPLETED" "FAILURE")" \ + "$(check_run "dev-lead / dispatch" "COMPLETED" "FAILURE")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run ci_nonrequired_failures "$r" "$REQUIRED_SET" + [ "$output" = "template-drift" ] +} diff --git a/tests/test_review_one_pr_nonrequired_gate.bats b/tests/test_review_one_pr_nonrequired_gate.bats new file mode 100644 index 000000000..985170d94 --- /dev/null +++ b/tests/test_review_one_pr_nonrequired_gate.bats @@ -0,0 +1,139 @@ +#!/usr/bin/env bats +# Regression guard for #1795: a failing NON-required check must not deadlock merge. +# +# The repo protects `main` with a ruleset (not classic protection), so the rollup's +# .isRequired field is frequently absent and the #1549 fallback reverted to gating +# on ALL external checks — meaning a red `template-drift` (a non-required advisory +# check that fails whenever the standards baseline moves under repo-template) skipped +# every review as `ci-failing`, and no approval could ever land. +# +# review-one-pr.sh now reads the required-check set from the branch ruleset API and +# passes it to compute_ci_status, so: +# • a red non-required check with all required green → reviews (no ci-failing skip) +# • a red REQUIRED check → still skips ci-failing (the gate is never weakened) +# • an unreadable ruleset → fails closed (every failing check blocks) +# • the non-required failure is NAMED in the output (never silently ignored) +# +# Offline + stubbed — no network. +# +# Run with: bats tests/test_review_one_pr_nonrequired_gate.bats + +setup() { + REPO_ROOT="$(cd "$(dirname "$BATS_TEST_FILENAME")/.." && pwd)" + export REVIEW_SCRIPT="$REPO_ROOT/scripts/review-one-pr.sh" + export SHA="cafebabecafebabecafebabecafebabecafebabe" + export PR_URL="https://github.com/petry-projects/.github-private/pull/1783" + export TEST_DIR="$BATS_TEST_TMPDIR" + mkdir -p "$TEST_DIR/bin"; cd "$TEST_DIR" + + # The five contexts the `main` ruleset requires today (issue #1795). template-drift + # is deliberately NOT among them. + cat > "$TEST_DIR/ruleset.json" <<'EOF' +["SonarCloud","CodeQL","agent-shield / AgentShield","dependency-audit / Detect ecosystems","duplicate-decl-gate"] +EOF + + export GH_LOG="$TEST_DIR/gh_calls.log"; : > "$GH_LOG" + + # Stub gh. `pr view` returns the snapshot; the ruleset API returns the pre-filtered + # required-context array (real gh applies --jq itself; the stub emits the result). + # RULESET_MODE=fail makes the ruleset (and classic-protection fallback) unreadable. + cat > "$TEST_DIR/bin/gh" <> "$GH_LOG" +if [ "\$1" = "pr" ] && [ "\$2" = "view" ]; then cat "$TEST_DIR/snapshot.json"; exit 0; fi +if [ "\$1" = "api" ]; then + case "\$2" in + *rules/branches*) + if [ "\${RULESET_MODE:-ok}" = "fail" ]; then exit 1; fi + cat "$TEST_DIR/ruleset.json"; exit 0 ;; + *protection*) exit 1 ;; # classic protection 404s on this ruleset-only branch + esac +fi +exit 0 +EOF + chmod +x "$TEST_DIR/bin/gh" + for e in claude copilot gemini; do printf '#!/bin/bash\nexit 0\n' > "$TEST_DIR/bin/$e"; chmod +x "$TEST_DIR/bin/$e"; done + export PATH="$TEST_DIR/bin:$PATH" + export REVIEW_ENGINE="claude" GH_TOKEN="fake" DRY_RUN="true" + unset FORCE_REVIEW RULESET_MODE +} +teardown() { rm -rf "$TEST_DIR"; } + +# All five required checks green, only the non-required template-drift red. +write_nonrequired_red_snapshot() { + cat > "$TEST_DIR/snapshot.json" <&2; echo "$output" >&2 + # The CI gate must NOT record a ci-failing skip. + [[ "$output" != *'"reason":"ci-failing"'* ]] + [[ "$output" != *"skip: CI checks are failing"* ]] +} + +@test "#1795: the non-required failure is NAMED in the output (never silently ignored)" { + write_nonrequired_red_snapshot + run timeout 30 bash "$REVIEW_SCRIPT" "$PR_URL" + echo "$output" >&2 + [[ "$output" == *"proceeding past non-required failing check"* ]] + [[ "$output" == *"template-drift"* ]] +} + +@test "#1795: a red REQUIRED check still skips with ci-failing (gate never weakened)" { + cat > "$TEST_DIR/snapshot.json" <&2; echo "$output" >&2 + [ "$status" -eq 100 ] + [[ "$output" == *'"reason":"ci-failing"'* ]] +} + +@test "#1795: unreadable ruleset → fails closed (non-required red then blocks as ci-failing)" { + export RULESET_MODE=fail + write_nonrequired_red_snapshot + run timeout 30 bash "$REVIEW_SCRIPT" "$PR_URL" + echo "status=$status" >&2; echo "$output" >&2 + # With no readable required set, every failing check blocks (today's behaviour). + [[ "$output" == *"failing closed"* ]] + [ "$status" -eq 100 ] + [[ "$output" == *'"reason":"ci-failing"'* ]] +} + +@test "#1795: the required set is read from the ruleset API, not a literal" { + write_nonrequired_red_snapshot + run timeout 30 bash "$REVIEW_SCRIPT" "$PR_URL" + # The ruleset endpoint must have been queried for the base branch. + grep -q 'api repos/petry-projects/.github-private/rules/branches/main' "$GH_LOG" +} From 956d806daf69da445f530d4c335e3bef1e9606b9 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sat, 19 Sep 2026 13:26:42 +0000 Subject: [PATCH 2/2] chore: dev-lead update (review-changes) [skip ci-relay] --- scripts/lib/ci-status.sh | 57 ++++++++++++++++++++-- scripts/pr_mergeready_scan.sh | 20 ++++---- scripts/pr_stall_scan.sh | 9 +++- scripts/review-one-pr.sh | 86 +++++++++++++++------------------- scripts/sweep-stuck-reviews.sh | 16 ++++++- tests/test_ci_status.bats | 57 ++++++++++++++++++++++ 6 files changed, 182 insertions(+), 63 deletions(-) diff --git a/scripts/lib/ci-status.sh b/scripts/lib/ci-status.sh index 846588c54..8f214d335 100644 --- a/scripts/lib/ci-status.sh +++ b/scripts/lib/ci-status.sh @@ -146,6 +146,47 @@ _ci_status_agent_roles_json() { printf '%s' "$roles_json" } +# ruleset_required_checks +# +# 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:-[]}" @@ -153,8 +194,14 @@ compute_ci_status() { agent_roles="$(_ci_status_agent_roles_json)" # 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. - required_names="$(jq -c 'if type == "array" then . else [] end' <<< "$required_names" 2>/dev/null || echo '[]')" + # 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 @@ -226,7 +273,11 @@ ci_nonrequired_failures() { local required_names="${2:-[]}" local agent_roles agent_roles="$(_ci_status_agent_roles_json)" - required_names="$(jq -c 'if type == "array" then . else [] end' <<< "$required_names" 2>/dev/null || echo '[]')" + 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 != \"\"); diff --git a/scripts/pr_mergeready_scan.sh b/scripts/pr_mergeready_scan.sh index b7f6b193f..70e4da278 100644 --- a/scripts/pr_mergeready_scan.sh +++ b/scripts/pr_mergeready_scan.sh @@ -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 '[]') diff --git a/scripts/pr_stall_scan.sh b/scripts/pr_stall_scan.sh index 0436830f4..dee1270b6 100644 --- a/scripts/pr_stall_scan.sh +++ b/scripts/pr_stall_scan.sh @@ -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 # ``; a marker at the current head means diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index dd877963b..182c38da8 100755 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -159,41 +159,9 @@ echo " artifact_type=$ARTIFACT_TYPE content_ref=$CONTENT_REF" echo " rubric=$REVIEW_RUBRIC" echo " output_channel=$REVIEW_OUTPUT_CHANNEL" -# ruleset_required_checks -# -# 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. -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" -} +# ruleset_required_checks now lives in lib/ci-status.sh (sourced above) so every +# compute_ci_status caller — this script, the stuck-review sweep, and the +# stall/merge-ready scans — gates on the same authoritative required set (#1795). # 1. Current head SHA + CI gate — single API call for both fields. # Strict CI classification: @@ -209,21 +177,32 @@ PR_HEAD_SHA=$(echo "$PR_SNAPSHOT" | jq -r '.headRefOid') export PR_HEAD_SHA echo " head SHA: $PR_HEAD_SHA" -# Read the branch ruleset's required-check set once (base branch is stable for the -# life of the PR). Passed into every compute_ci_status call so a failing check the -# ruleset does not require cannot by itself produce `ci-failing` (#1795 AC #2). +# owner/repo from the PR URL — pure Bash regex, no printf|sed subprocess. +_OWNER_REPO="" +if [[ "$PR_URL" =~ ^https?://[^/]+/([^/]+/[^/]+)/pull/[0-9]+ ]]; then + _OWNER_REPO="${BASH_REMATCH[1]}" +fi + +# resolve_required_checks — set REQUIRED_CHECKS_JSON from the branch +# ruleset for . Passed into every compute_ci_status call so a failing check +# the ruleset does not require cannot by itself produce `ci-failing` (#1795 AC #2). # Fail closed: an unreadable/empty set leaves REQUIRED_CHECKS_JSON empty and -# compute_ci_status reverts to gating on all failing checks (#1795 AC #3). +# compute_ci_status reverts to gating on all failing checks (#1795 AC #3). Re-callable +# so the poll loop can refresh it if the PR is retargeted to a different base branch +# (the required set is base-branch-specific — a stale set would gate on the old +# ruleset). Only re-invoked when the base actually changes, so no redundant API read. +resolve_required_checks() { + local _base_ref="$1" + REQUIRED_CHECKS_JSON=$(ruleset_required_checks "$_OWNER_REPO" "$_base_ref" || true) + if [ -n "$REQUIRED_CHECKS_JSON" ] && [ "$REQUIRED_CHECKS_JSON" != "[]" ]; then + echo " required checks (branch ruleset for $_base_ref): $REQUIRED_CHECKS_JSON" + else + echo " required checks ($_base_ref): none readable — failing closed (every failing check blocks)" + REQUIRED_CHECKS_JSON="" + fi +} PR_BASE_REF=$(jq -r '.baseRefName // ""' <<< "$PR_SNAPSHOT") -_OWNER_REPO=$(printf '%s' "$PR_URL" | sed -E 's#^https?://[^/]+/([^/]+/[^/]+)/pull/[0-9]+.*#\1#') -REQUIRED_CHECKS_JSON=$(ruleset_required_checks "$_OWNER_REPO" "$PR_BASE_REF" || true) -if [ -n "$REQUIRED_CHECKS_JSON" ] && [ "$REQUIRED_CHECKS_JSON" != "[]" ]; then - echo " required checks (branch ruleset): $REQUIRED_CHECKS_JSON" -else - echo " required checks: none readable — failing closed (every failing check blocks)" - REQUIRED_CHECKS_JSON="" -fi -unset _OWNER_REPO +resolve_required_checks "$PR_BASE_REF" CI_STATUS=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$PR_SNAPSHOT")" "$REQUIRED_CHECKS_JSON") echo " CI status: $CI_STATUS" @@ -324,10 +303,21 @@ if [ "$CI_STATUS" = "pending" ]; then rm -f "$_gh_poll_err" PR_HEAD_SHA=$(jq -r '.headRefOid' <<< "$PR_SNAPSHOT") export PR_HEAD_SHA + # If the PR was retargeted mid-poll, the required-check set is base-branch- + # specific, so refresh it against the new base before classifying — otherwise + # CI is evaluated with the old base's ruleset. Only refetched when the base + # actually changes, so an unchanged base costs no extra API read. + _poll_base_ref=$(jq -r '.baseRefName // ""' <<< "$PR_SNAPSHOT") + if [ "$_poll_base_ref" != "$PR_BASE_REF" ]; then + echo " force-review: PR retargeted $PR_BASE_REF → $_poll_base_ref — refreshing required checks" + PR_BASE_REF="$_poll_base_ref" + resolve_required_checks "$PR_BASE_REF" + fi CI_STATUS=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$PR_SNAPSHOT")" "$REQUIRED_CHECKS_JSON") echo " CI status (poll ${_poll}): $CI_STATUS" _poll=$((_poll + 1)) done + unset _poll_base_ref unset _poll _FORCE_POLL_MAX _FORCE_POLL_SEC _gh_poll_err _gh_poll_err_content if [ "$CI_STATUS" = "failing" ]; then diff --git a/scripts/sweep-stuck-reviews.sh b/scripts/sweep-stuck-reviews.sh index 59d822fe7..3126e836f 100755 --- a/scripts/sweep-stuck-reviews.sh +++ b/scripts/sweep-stuck-reviews.sh @@ -199,7 +199,7 @@ while IFS= read -r pr_url; do inspected=$((inspected + 1)) if ! snapshot=$(gh pr view "$pr_url" \ - --json headRefOid,statusCheckRollup,reviewDecision,reviews,comments,labels 2>/dev/null); then + --json headRefOid,baseRefName,statusCheckRollup,reviewDecision,reviews,comments,labels 2>/dev/null); then echo " skip $pr_url — could not fetch PR (deleted, no access, or rate-limited)" continue fi @@ -334,7 +334,19 @@ while IFS= read -r pr_url; do continue fi - ci_status=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$snapshot")") + # Gate on the branch ruleset's REQUIRED checks (#1795) — same authoritative set + # review-one-pr.sh uses. Without it this sweep reverts to blocking on every red + # external check, so a non-required `template-drift` failure would keep a green PR + # stuck and never re-dispatched (the deadlock #1795 fixes, on the sweep path too). + # Fail closed: an unreadable set leaves it empty and compute_ci_status gates on all + # failing checks. + base_ref=$(jq -r '.baseRefName // ""' <<< "$snapshot") + owner_repo="" + if [[ "$pr_url" =~ ^https?://[^/]+/([^/]+/[^/]+)/pull/[0-9]+ ]]; then + owner_repo="${BASH_REMATCH[1]}" + fi + required_checks=$(ruleset_required_checks "$owner_repo" "$base_ref" || true) + ci_status=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$snapshot")" "$required_checks") if [ "$ci_status" != "passing" ]; then echo " skip $pr_url — CI '$ci_status' (not green yet)" continue diff --git a/tests/test_ci_status.bats b/tests/test_ci_status.bats index b168f0ff7..9fa9ab961 100644 --- a/tests/test_ci_status.bats +++ b/tests/test_ci_status.bats @@ -1057,3 +1057,60 @@ REQUIRED_SET='["SonarCloud","CodeQL","agent-shield / AgentShield","dependency-au run ci_nonrequired_failures "$r" "$REQUIRED_SET" [ "$output" = "template-drift" ] } + +# --------------------------------------------------------------------------- +# ruleset_required_checks (#1795): the required set now lives in this lib so every +# compute_ci_status caller (review-one-pr.sh, the stuck-review sweep, the +# stall/merge-ready scans) reads the SAME authoritative set — closing the deadlock +# on the callers that previously passed only the rollup and so reverted to blocking +# on red non-required checks. +# --------------------------------------------------------------------------- + +# Install a gh stub on PATH that returns a required-context array from the ruleset +# endpoint (and 404s the classic-protection fallback) and records each invocation. +_install_gh_ruleset_stub() { + GH_CALL_LOG="$BATS_TEST_TMPDIR/gh_calls.log"; : > "$GH_CALL_LOG" + mkdir -p "$BATS_TEST_TMPDIR/bin" + cat > "$BATS_TEST_TMPDIR/bin/gh" <> "$GH_CALL_LOG" +if [ "\$1" = "api" ]; then + case "\$2" in + *rules/branches*) printf '%s' '["SonarCloud","CodeQL"]'; exit 0 ;; + *protection*) exit 1 ;; + esac +fi +exit 0 +EOF + chmod +x "$BATS_TEST_TMPDIR/bin/gh" + export PATH="$BATS_TEST_TMPDIR/bin:$PATH" +} + +@test "ruleset_required_checks: reads the ruleset API and returns its contexts" { + _install_gh_ruleset_stub + run ruleset_required_checks "petry-projects/.github-private" "main" + [ "$output" = '["SonarCloud","CodeQL"]' ] + grep -q 'api repos/petry-projects/.github-private/rules/branches/main' "$GH_CALL_LOG" +} + +@test "ruleset_required_checks: empty repo or branch returns empty (no API call)" { + _install_gh_ruleset_stub + run ruleset_required_checks "" "main" + [ -z "$output" ] + [ ! -s "$GH_CALL_LOG" ] +} + +# End-to-end #1795 for a loop caller: its required set (from ruleset_required_checks) +# fed into compute_ci_status lets a green-required PR pass even with a red +# non-required template-drift — the deadlock closed on the sweep/scan paths. +@test "ruleset_required_checks + compute_ci_status: red non-required check does not block" { + _install_gh_ruleset_stub + local req r + req=$(ruleset_required_checks "petry-projects/.github-private" "main") + r=$(rollup \ + "$(check_run "SonarCloud" "COMPLETED" "SUCCESS")" \ + "$(check_run "CodeQL" "COMPLETED" "SUCCESS")" \ + "$(check_run "template-drift" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" "$req" + [ "$output" = "passing" ] +}