diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index d44fdbda2..20fe8e361 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -34,7 +34,7 @@ jobs: run: sudo apt-get install -y bats - name: Run tests - run: bats tests/fleet_report.bats tests/token_report.bats tests/model_pricing.bats tests/test_review_cycle.bats + run: bats tests/fleet_report.bats tests/token_report.bats tests/model_pricing.bats tests/test_review_cycle.bats tests/test_ci_status.bats validate-agent-profiles: runs-on: ubuntu-latest diff --git a/scripts/lib/ci-status.sh b/scripts/lib/ci-status.sh new file mode 100644 index 000000000..e940edffe --- /dev/null +++ b/scripts/lib/ci-status.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# ci-status.sh — compute CI gate status from a statusCheckRollup JSON array, +# filtering out the PR Review Agent's own check runs so the cascade never +# blocks on itself (issue #469). +# +# Usage: source this file, then call compute_ci_status with the rollup array. + +# compute_ci_status +# +# Inputs: +# $1 — JSON array (the .statusCheckRollup field from `gh pr view`) +# +# Outputs (stdout): one of "passing", "pending", "failing" +# +# Classification rules (after filtering own checks): +# passing — empty rollup, or every item is SUCCESS/SKIPPED/NEUTRAL/SUCCESS state +# pending — any item is IN_PROGRESS/QUEUED/WAITING/PENDING/EXPECTED or +# COMPLETED with null/empty conclusion +# failing — anything else (FAILURE, ACTION_REQUIRED, TIMED_OUT, etc.) +# +# Own-check filter — excludes entries belonging to the PR Review cascade. +# `gh pr view --json statusCheckRollup` exposes the check/job *name* but not +# the workflow name (gh/cli#9091), so the PR Review Agent's single job appears +# with its bare job name in the rollup: "review". +# This is matched explicitly to prevent the cascade from blocking on its own +# pending check run. Compound "workflow / job" forms are also matched for +# reusable-workflow callers where that format does appear. +# Dev-Lead bare checks ("dispatch", "ci-relay") are NOT filtered because +# Dev-Lead can commit back to the PR branch; filtering them could cause a review +# to race an in-progress branch-mutating run. +# Matched: +# • Bare job name: "review" +# • "PR Review Agent / *" +# • "PR Review Reusable / *" +# • "PR Review — Mention Trigger / *" +compute_ci_status() { + local rollup_json="${1:-[]}" + jq -r ' + def is_pending: + .status == "IN_PROGRESS" or .status == "QUEUED" or .status == "WAITING" or + .state == "PENDING" or .state == "EXPECTED" or + (.status == "COMPLETED" and (.conclusion == null or .conclusion == "")); + def is_success: + .conclusion == "SUCCESS" or .conclusion == "SKIPPED" or .conclusion == "NEUTRAL" or + .state == "SUCCESS"; + def is_own_check: + (.name // .context // "") as $n | + ($n == "review") or + ($n | test("^PR Review (Agent|Reusable) /")) or + ($n | test("^PR Review — Mention Trigger /")); + if (. == null or (type != "array")) then "passing" + else + (map(select(is_own_check | not))) as $ext | + if ($ext | length) == 0 then "passing" + elif ([$ext[] | select(is_pending)] | length) > 0 then "pending" + elif ([$ext[] | select(is_success)] | length) == ($ext | length) then "passing" + else "failing" + end + end + ' <<< "$rollup_json" 2>/dev/null || echo "passing" +} diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index 84fae0b72..b15d340f9 100644 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -28,6 +28,9 @@ source "$SCRIPT_DIR/engine.sh" # Cycle-cap helpers: compute_review_cycle, has_escalation_marker (issue #467). # shellcheck source=lib/review-cycle.sh source "$SCRIPT_DIR/lib/review-cycle.sh" +# CI gate: compute_ci_status filters own check runs before classifying (issue #469). +# shellcheck source=lib/ci-status.sh +source "$SCRIPT_DIR/lib/ci-status.sh" PR_URL="${1:?usage: review-one-pr.sh }" export PR_URL @@ -48,20 +51,7 @@ PR_HEAD_SHA=$(echo "$PR_SNAPSHOT" | jq -r '.headRefOid') export PR_HEAD_SHA echo " head SHA: $PR_HEAD_SHA" -CI_STATUS=$(echo "$PR_SNAPSHOT" | jq -r ' - def is_pending: - .status == "IN_PROGRESS" or .status == "QUEUED" or .status == "WAITING" or - .state == "PENDING" or .state == "EXPECTED" or - (.status == "COMPLETED" and (.conclusion == null or .conclusion == "")); - def is_success: - .conclusion == "SUCCESS" or .conclusion == "SKIPPED" or .conclusion == "NEUTRAL" or - .state == "SUCCESS"; - if (.statusCheckRollup | length) == 0 then "passing" - elif ([.statusCheckRollup[] | select(is_pending)] | length) > 0 then "pending" - elif ([.statusCheckRollup[] | select(is_success)] | length) == (.statusCheckRollup | length) then "passing" - else "failing" - end -') +CI_STATUS=$(compute_ci_status "$(jq '.statusCheckRollup' <<< "$PR_SNAPSHOT")") echo " CI status: $CI_STATUS" REVIEW_DECISION=$(echo "$PR_SNAPSHOT" | jq -r '.reviewDecision // ""') @@ -75,10 +65,76 @@ if [ "$CI_STATUS" = "failing" ]; then echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"ci-failing\"}" exit 100 fi + +# Pending CI gate: for normal runs, skip immediately. +# For FORCE_REVIEW=true (mention-triggered), poll briefly so transient check +# churn (caused by the mention burst itself) doesn't silently swallow the run +# (issue #469). Defaults: 10 polls × 30 s = up to 5 minutes. if [ "$CI_STATUS" = "pending" ]; then - echo " skip: CI checks still in progress — will re-evaluate when checks complete" - echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"ci-pending\"}" - exit 100 + if [ "${FORCE_REVIEW:-false}" = "true" ]; then + _FORCE_POLL_MAX="${FORCE_REVIEW_POLL_ATTEMPTS:-10}" + _FORCE_POLL_SEC="${FORCE_REVIEW_POLL_INTERVAL_SEC:-30}" + # Validate that the env-var overrides are positive integers; fall back to + # the defaults if they are not so that sleep and the numeric test don't fail. + case "$_FORCE_POLL_MAX" in + ''|*[!0-9]*) _FORCE_POLL_MAX=10 ;; + esac + case "$_FORCE_POLL_SEC" in + ''|*[!0-9]*) _FORCE_POLL_SEC=30 ;; + esac + [ "$_FORCE_POLL_MAX" -gt 0 ] || _FORCE_POLL_MAX=10 + [ "$_FORCE_POLL_SEC" -gt 0 ] || _FORCE_POLL_SEC=30 + _poll=1 + while [ "$_poll" -le "$_FORCE_POLL_MAX" ] && [ "$CI_STATUS" = "pending" ]; do + 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 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 + echo " gh API rate limited during polling — skipping PR" + echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"gh-rate-limited\"}" + exit 100 + fi + echo "::error::gh pr view failed during polling for $PR_URL" + exit 1 + fi + 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")") + echo " CI status (poll ${_poll}): $CI_STATUS" + _poll=$((_poll + 1)) + done + unset _poll _FORCE_POLL_MAX _FORCE_POLL_SEC _gh_poll_err _gh_poll_err_content + + if [ "$CI_STATUS" = "failing" ]; then + echo " skip: CI checks are failing (detected after polling) — will re-evaluate after fixes are pushed" + echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"ci-failing\"}" + exit 100 + fi + + if [ "$CI_STATUS" = "pending" ]; then + echo " force-review: CI still pending after polling — posting visible comment" + if [ "${DRY_RUN:-false}" != "true" ]; then + _msg=$(printf '\n\nCI checks on this PR are still running. Once they complete, re-mention `@%s` to trigger a fresh review.\n\n_Posted by the %s PR-review cascade._' \ + "${BOT_USER:-donpetry-bot}" "${BOT_USER:-donpetry-bot}") + gh pr comment "$PR_URL" --body "$_msg" || echo " warn: could not post ci-pending-ack comment" + unset _msg + fi + echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"ci-pending-after-poll\"}" + exit 100 + fi + + # CI settled — update REVIEW_DECISION from the refreshed snapshot + REVIEW_DECISION=$(jq -r '.reviewDecision // ""' <<< "$PR_SNAPSHOT") + echo " review decision (after CI poll): $REVIEW_DECISION" + else + echo " skip: CI checks still in progress — will re-evaluate when checks complete" + echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"ci-pending\"}" + exit 100 + fi fi # Skip when a human has requested changes, with two guards: # 1. FORCE_REVIEW bypasses the skip — mention-triggered runs always proceed so diff --git a/tests/test_ci_status.bats b/tests/test_ci_status.bats new file mode 100644 index 000000000..db7cdf871 --- /dev/null +++ b/tests/test_ci_status.bats @@ -0,0 +1,342 @@ +#!/usr/bin/env bats +# Unit tests for compute_ci_status in scripts/lib/ci-status.sh +# (issue #469: own check runs must never cause ci-pending/ci-failing classification) +# +# Run with: bats tests/test_ci_status.bats + +setup() { + source "$(dirname "$BATS_TEST_FILENAME")/../scripts/lib/ci-status.sh" +} + +# --------------------------------------------------------------------------- +# Helpers to build statusCheckRollup items +# --------------------------------------------------------------------------- + +# check_run [conclusion] +# Models a GitHub Actions CheckRun in the statusCheckRollup array. +check_run() { + local name="$1" status="$2" conclusion="${3:-null}" + if [ "$conclusion" = "null" ]; then + jq -n --arg name "$name" --arg status "$status" \ + '{name: $name, status: $status, conclusion: null}' + else + jq -n --arg name "$name" --arg status "$status" --arg conclusion "$conclusion" \ + '{name: $name, status: $status, conclusion: $conclusion}' + fi +} + +# status_ctx +# Models a commit StatusContext in the statusCheckRollup array. +status_ctx() { + jq -n --arg context "$1" --arg state "$2" \ + '{context: $context, state: $state}' +} + +# rollup ... +# Wraps items into a JSON array. +rollup() { + printf '%s\n' "$@" | jq -s '.' +} + +# --------------------------------------------------------------------------- +# Baseline: empty / all-passing rollups +# --------------------------------------------------------------------------- + +@test "empty rollup returns passing" { + run compute_ci_status '[]' + [ "$status" -eq 0 ] + [ "$output" = "passing" ] +} + +@test "single passing external check returns passing" { + local r + r=$(rollup "$(check_run "Lint" "COMPLETED" "SUCCESS")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +@test "single skipped external check returns passing" { + local r + r=$(rollup "$(check_run "Path-filtered" "COMPLETED" "SKIPPED")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +@test "single neutral external check returns passing" { + local r + r=$(rollup "$(check_run "Informational" "COMPLETED" "NEUTRAL")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +# --------------------------------------------------------------------------- +# External pending / failing without own checks +# --------------------------------------------------------------------------- + +@test "single external IN_PROGRESS check returns pending" { + local r + r=$(rollup "$(check_run "Build" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +@test "single external QUEUED check returns pending" { + local r + r=$(rollup "$(check_run "Test" "QUEUED")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +@test "single external failing check returns failing" { + local r + r=$(rollup "$(check_run "Test" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" + [ "$output" = "failing" ] +} + +# --------------------------------------------------------------------------- +# PR Review Agent bare job name IS filtered (actual rollup shape) +# `gh pr view --json statusCheckRollup` exposes the job name only (not the +# workflow name, per gh/cli#9091), so the PR Review Agent's own job appears as +# a bare name: "review". This is filtered to prevent the cascade from blocking +# on its own pending check run. +# Dev-Lead bare job names ("dispatch", "ci-relay") are NOT filtered because +# Dev-Lead can commit back to the PR branch. +# --------------------------------------------------------------------------- + +@test "bare PR Review Agent check 'review' (IN_PROGRESS) is filtered → passing" { + local r + r=$(rollup "$(check_run "review" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +@test "bare Dev-Lead check 'dispatch' (IN_PROGRESS) is NOT filtered → pending" { + local r + r=$(rollup "$(check_run "dispatch" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +@test "bare Dev-Lead check 'ci-relay' (IN_PROGRESS) is NOT filtered → pending" { + local r + r=$(rollup "$(check_run "ci-relay" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +@test "bare check named 'pr-review-mention' (IN_PROGRESS) is NOT filtered → pending" { + local r + r=$(rollup "$(check_run "pr-review-mention" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +@test "bare check named 'mention-ack' (IN_PROGRESS) is NOT filtered → pending" { + local r + r=$(rollup "$(check_run "mention-ack" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +# --------------------------------------------------------------------------- +# Known PR Review workflow checks ARE filtered — workflow / job-name form +# --------------------------------------------------------------------------- + +@test "'PR Review Agent / review' (IN_PROGRESS) is filtered out → passing" { + local r + r=$(rollup "$(check_run "PR Review Agent / review" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +@test "'PR Review Reusable / review' (IN_PROGRESS) is filtered out → passing" { + local r + r=$(rollup "$(check_run "PR Review Reusable / review" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +@test "'PR Review — Mention Trigger / pr-review-mention' (IN_PROGRESS) is filtered out → passing" { + local r + r=$(rollup "$(check_run "PR Review — Mention Trigger / pr-review-mention" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +# --------------------------------------------------------------------------- +# Dev-lead and generic workflow/job-suffix checks are NOT filtered +# Dev-lead can commit back to the PR branch, so it must remain a real CI gate. +# A generic 'workflow / review' suffix (e.g., 'Build / review') must also block +# so external required checks are never silently bypassed. +# --------------------------------------------------------------------------- + +@test "'dev-lead / dispatch' (IN_PROGRESS) is NOT filtered → pending" { + local r + r=$(rollup "$(check_run "dev-lead / dispatch" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +@test "'dev-lead / ci-relay' (IN_PROGRESS) is NOT filtered → pending" { + local r + r=$(rollup "$(check_run "dev-lead / ci-relay" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +@test "'Build / review' (IN_PROGRESS) is NOT filtered → pending" { + local r + r=$(rollup "$(check_run "Build / review" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +# --------------------------------------------------------------------------- +# Failure classification: PR Review checks (bare and compound) are filtered +# --------------------------------------------------------------------------- + +@test "bare PR Review Agent check 'review' (COMPLETED/FAILURE) is filtered → passing" { + local r + r=$(rollup "$(check_run "review" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +@test "'PR Review Agent / review' (COMPLETED/FAILURE) is filtered out → passing" { + local r + r=$(rollup "$(check_run "PR Review Agent / review" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +# --------------------------------------------------------------------------- +# Mix: PR Review workflow pending + external passing → passing (filtered) +# --------------------------------------------------------------------------- + +@test "mix: 'PR Review Agent / review' pending + external passing → passing" { + local r + r=$(rollup \ + "$(check_run "PR Review Agent / review" "IN_PROGRESS")" \ + "$(check_run "CI / build" "COMPLETED" "SUCCESS")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +# --------------------------------------------------------------------------- +# Mix: bare PR Review Agent checks filtered; external result determines status +# --------------------------------------------------------------------------- + +@test "mix: bare 'review' filtered + external passing → passing" { + local r + r=$(rollup \ + "$(check_run "review" "IN_PROGRESS")" \ + "$(check_run "Lint" "COMPLETED" "SUCCESS")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +@test "mix: bare 'dispatch' NOT filtered + external passing → pending" { + local r + r=$(rollup \ + "$(check_run "dispatch" "IN_PROGRESS")" \ + "$(check_run "Build" "COMPLETED" "SUCCESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +# --------------------------------------------------------------------------- +# Mix: bare PR Review Agent check filtered + external pending → pending +# --------------------------------------------------------------------------- + +@test "mix: bare 'review' filtered + external pending → pending" { + local r + r=$(rollup \ + "$(check_run "review" "IN_PROGRESS")" \ + "$(check_run "Build" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +# --------------------------------------------------------------------------- +# Mix: bare PR Review Agent check filtered + external failing → failing +# --------------------------------------------------------------------------- + +@test "mix: bare 'review' filtered + external failing → failing" { + # 'review' is filtered out; only 'Test' (COMPLETED/FAILURE) remains in $ext. + local r + r=$(rollup \ + "$(check_run "review" "IN_PROGRESS")" \ + "$(check_run "Test" "COMPLETED" "FAILURE")") + run compute_ci_status "$r" + [ "$output" = "failing" ] +} + +# --------------------------------------------------------------------------- +# Rollup with only PR Review workflow checks → passing (all filtered) +# --------------------------------------------------------------------------- + +@test "rollup with only PR Review workflow/job form checks returns passing" { + local r + r=$(rollup \ + "$(check_run "PR Review Agent / review" "IN_PROGRESS")" \ + "$(check_run "PR Review — Mention Trigger / pr-review-mention" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +# --------------------------------------------------------------------------- +# Rollup with only PR Review Agent bare job names → passing (all filtered) +# --------------------------------------------------------------------------- + +@test "rollup: 'review' filtered, Dev-Lead 'dispatch' + 'ci-relay' NOT filtered → pending" { + local r + r=$(rollup \ + "$(check_run "review" "IN_PROGRESS")" \ + "$(check_run "dispatch" "IN_PROGRESS")" \ + "$(check_run "ci-relay" "IN_PROGRESS")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +# --------------------------------------------------------------------------- +# StatusContext checks are unaffected by the own-check filter +# --------------------------------------------------------------------------- + +@test "status context with PENDING state still causes pending classification" { + local r + r=$(rollup "$(status_ctx "sonarcloud" "PENDING")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +@test "status context with FAILURE state still causes failing classification" { + local r + r=$(rollup "$(status_ctx "sonarcloud" "FAILURE")") + run compute_ci_status "$r" + [ "$output" = "failing" ] +} + +@test "status context named 'external-ci' (non-own-check) with PENDING state still causes pending" { + # StatusContext entries that do not match any own-check prefix must still block + # as pending — verifies the filter doesn't accidentally swallow external statuses. + local r + r=$(rollup "$(status_ctx "external-ci" "PENDING")") + run compute_ci_status "$r" + [ "$output" = "pending" ] +} + +# --------------------------------------------------------------------------- +# Malformed / edge input +# --------------------------------------------------------------------------- + +@test "malformed JSON input returns passing (safe default)" { + run compute_ci_status "not-json" + [ "$status" -eq 0 ] + [ "$output" = "passing" ] +} + +@test "null JSON input returns passing (safe default)" { + run compute_ci_status "null" + [ "$output" = "passing" ] +}