From e3fe8e1d434d62ca285161403aa22642cde6015e Mon Sep 17 00:00:00 2001 From: Bibek Chaudhary Date: Wed, 15 Jul 2026 13:27:39 +0545 Subject: [PATCH] Fix Dependabot CI gate misreading transient check-read as "no CI" gated_merge.sh discovered the PR's checks via `gh pr checks` (the GraphQL status rollup) and mapped empty stdout to "no checks reported". Under a simultaneous multi-PR Dependabot batch the rollup can transiently return nothing, so a check that was attached and already green was misread as "this repo has no CI" -- and after the 300s APPEAR window the gate parked healthy patch/minor bumps with `needs-manual-review` (realrate/.github#11). Read checks authoritatively from the REST check-runs + commit-status APIs for the PR head SHA instead, and distinguish an API failure from a genuine empty result: a failed read now yields a "__ERR__" sentinel so the loops retry rather than concluding "no CI". The genuine-no-CI, red-CI, and exit-0-on-every-path guarantees are preserved. Adds test_gated_merge.sh (stubbed `gh`) covering: API blips then green -> merge, no CI -> human, red CI -> human, self-run exclusion; and a tooling-tests workflow running shellcheck + the tests on tooling changes. Closes #11 Co-Authored-By: Claude Opus 4.8 --- .github/review-tooling/gated_merge.sh | 77 +++++++++++-- .github/review-tooling/test_gated_merge.sh | 120 +++++++++++++++++++++ .github/workflows/tooling-tests.yaml | 31 ++++++ 3 files changed, 219 insertions(+), 9 deletions(-) create mode 100755 .github/review-tooling/test_gated_merge.sh create mode 100644 .github/workflows/tooling-tests.yaml diff --git a/.github/review-tooling/gated_merge.sh b/.github/review-tooling/gated_merge.sh index a6b702e..47a21de 100755 --- a/.github/review-tooling/gated_merge.sh +++ b/.github/review-tooling/gated_merge.sh @@ -48,8 +48,22 @@ if [ "$CI_GATE_ENABLED" != "true" ]; then exit $? fi +# OWNER/REPO from the PR URL, host-agnostic: strip scheme, then host, then the +# /pull/N suffix. Used for the REST check-runs/status queries below. +_repo_tail="${PR_URL#*://}"; _repo_tail="${_repo_tail#*/}"; REPO="${_repo_tail%%/pull/*}" + pr_state() { gh pr view "$PR_URL" --json state --jq .state 2>/dev/null || echo ""; } +# The PR head commit SHA (fixed for a run -- a rebase/synchronize cancels this +# run and starts a fresh one). Prints it, or returns non-zero if it can't be +# read so the caller retries rather than treating the blip as "no checks". +resolve_head_sha() { + local sha + sha="$(gh pr view "$PR_URL" --json headRefOid --jq .headRefOid 2>/dev/null)" || return 1 + [ -n "$sha" ] || return 1 + printf '%s' "$sha" +} + route_to_human() { gh pr comment "$PR_URL" --body "🤖 **Auto-merge held:** $1 Routed to manual review." || true gh label create needs-manual-review --color FBCA04 \ @@ -78,16 +92,44 @@ do_merge() { route_to_human "the merge was rejected: $(printf '%s' "$err" | tail -1)" } -# The PR's checks minus THIS workflow's own run, matched by run id inside the -# check link (rename-proof). Captures stdout even when `gh pr checks` exits -# non-zero (it does when checks are pending/failing); only an empty result -- no -# checks reported yet -- collapses to []. +# The PR head SHA's checks minus THIS workflow's own run, as a JSON array of +# {name,bucket,link}. Read from the REST check-runs + commit-status APIs rather +# than `gh pr checks` (the GraphQL status rollup): under a simultaneous multi-PR +# Dependabot batch the rollup can transiently return nothing, and the old code +# mapped that empty stdout to "no checks" -- so a green check the REST API still +# reported was misread as "no CI", parking healthy patch/minor bumps for a human +# (realrate/.github#11). The REST endpoints return a well-formed body with a +# reliable count even while checks are pending, and a real non-zero exit on a +# genuine API error. +# +# Crucially this DISTINGUISHES an API failure from a genuine empty result: on any +# failed read it prints the sentinel "__ERR__" so callers retry instead of +# concluding "no checks". Self-exclusion matches this run's id in the check-run +# URL (`/runs//`), rename-proof. Commit statuses carry no run id, but our +# own workflow reports a check-run (not a status), so it never appears there. other_checks() { - local out - out="$(gh pr checks "$PR_URL" --json name,bucket,link 2>/dev/null)" - [ -z "$out" ] && { echo '[]'; return; } - echo "$out" | jq --arg rid "${GITHUB_RUN_ID:-0}" \ - '[.[] | select((.link // "") | contains("/runs/" + $rid + "/") | not)]' + local sha runs statuses rid="${GITHUB_RUN_ID:-0}" + sha="$(resolve_head_sha)" || { printf '__ERR__'; return; } + runs="$(gh api "repos/$REPO/commits/$sha/check-runs?per_page=100" 2>/dev/null)" || { printf '__ERR__'; return; } + statuses="$(gh api "repos/$REPO/commits/$sha/status" 2>/dev/null)" || { printf '__ERR__'; return; } + jq -n --argjson runs "$runs" --argjson st "$statuses" --arg rid "$rid" ' + [ $runs.check_runs[]? + | select(((.html_url // .details_url) // "") | contains("/runs/" + $rid + "/") | not) + | { name: .name, + link: (.html_url // .details_url // ""), + bucket: (if .status != "completed" then "pending" + elif (.conclusion == "success" or .conclusion == "neutral") then "pass" + elif .conclusion == "skipped" then "skip" + elif .conclusion == "cancelled" then "cancel" + else "fail" end) } ] + + + [ $st.statuses[]? + | { name: .context, + link: (.target_url // ""), + bucket: (if .state == "pending" then "pending" + elif .state == "success" then "pass" + else "fail" end) } ] + ' 2>/dev/null || printf '__ERR__' } start="$SECONDS" @@ -97,8 +139,17 @@ start="$SECONDS" # would skip the gate exactly when checks are merely delayed (and an immediate # merge before the base-branch policy's requirements exist gets rejected # anyway). If nothing registers, route to a human rather than merge blind. +# A __ERR__ read (transient API failure) is NOT "no checks": keep waiting so a +# blip during a busy batch can't be mistaken for a CI-less repo (see #11). while :; do checks="$(other_checks)" + if [ "$checks" = "__ERR__" ]; then + if [ $((SECONDS - start)) -ge "$TIMEOUT" ]; then + route_to_human "could not read CI status from the API within $((TIMEOUT / 60)) minutes." + exit 0 + fi + sleep "$POLL"; continue + fi [ "$(echo "$checks" | jq 'length')" -gt 0 ] && break if [ $((SECONDS - start)) -ge "$APPEAR" ]; then route_to_human "no CI checks registered within ${APPEAR}s (checks delayed, or the repo has none) -- not auto-merging without a green signal." @@ -110,6 +161,14 @@ done # 2) Wait for every non-self check to reach a terminal state. while :; do checks="$(other_checks)" + if [ "$checks" = "__ERR__" ]; then + if [ "$(pr_state)" != "OPEN" ]; then echo "PR no longer open; nothing to do."; exit 0; fi + if [ $((SECONDS - start)) -ge "$TIMEOUT" ]; then + route_to_human "could not read CI status from the API within $((TIMEOUT / 60)) minutes." + exit 0 + fi + sleep "$POLL"; continue + fi [ "$(echo "$checks" | jq '[.[] | select(.bucket=="pending")] | length')" -eq 0 ] && break if [ "$(pr_state)" != "OPEN" ]; then echo "PR no longer open; nothing to do."; exit 0; fi if [ $((SECONDS - start)) -ge "$TIMEOUT" ]; then diff --git a/.github/review-tooling/test_gated_merge.sh b/.github/review-tooling/test_gated_merge.sh new file mode 100755 index 0000000..7bf8634 --- /dev/null +++ b/.github/review-tooling/test_gated_merge.sh @@ -0,0 +1,120 @@ +#!/usr/bin/env bash +# Regression tests for gated_merge.sh, driven by a stubbed `gh` on PATH. +# +# The bug these guard (realrate/.github#11): a transient failure to READ the +# PR's checks used to be indistinguishable from "the repo has no CI", so healthy +# patch/minor Dependabot bumps were parked for a human. The gate now reads the +# REST check-runs/status APIs and distinguishes an API error (retry) from a +# genuine empty result (route to human). Scenarios below pin that behaviour. +# +# No network: `gh` is replaced by a fake that returns canned JSON per scenario. +set -uo pipefail + +HERE="$(cd "$(dirname "$0")" && pwd)" +SCRIPT="$HERE/gated_merge.sh" +FAILURES=0 + +# --- the fake `gh` ----------------------------------------------------------- +BIN="$(mktemp -d)" +cat > "$BIN/gh" <<'FAKE' +#!/usr/bin/env bash +# Minimal `gh` stand-in. Logs every call to $GH_LOG and answers from env-provided +# fixtures. BLIP_COUNT makes the first N check-runs reads fail (exit 1) to +# simulate a transient API error during a busy batch. +set -uo pipefail +echo "gh $*" >> "$GH_LOG" +sub="${1:-}"; shift || true +case "$sub" in + pr) + action="${1:-}"; shift || true + case "$action" in + view) + if printf '%s ' "$@" | grep -q headRefOid; then echo "${FAKE_SHA:-deadbeef}"; exit 0; fi + if printf '%s ' "$@" | grep -q state; then echo "${FAKE_PR_STATE:-OPEN}"; exit 0; fi + exit 0 ;; + merge) echo "merged"; exit 0 ;; + comment|edit|review|checks) exit 0 ;; + *) exit 0 ;; + esac ;; + api) + path="${1:-}" + case "$path" in + *check-runs*) + if [ "${BLIP_COUNT:-0}" -gt 0 ]; then + c="$(cat "$COUNTER" 2>/dev/null || echo 0)"; c=$((c + 1)); echo "$c" > "$COUNTER" + [ "$c" -le "$BLIP_COUNT" ] && exit 1 + fi + cat "$CHECK_RUNS_JSON"; exit 0 ;; + *status*) cat "$STATUS_JSON"; exit 0 ;; + *) echo '{}'; exit 0 ;; + esac ;; + label) exit 0 ;; + *) exit 0 ;; +esac +FAKE +chmod +x "$BIN/gh" + +FIX="$(mktemp -d)" +# Our own auto-merge run is id 999; the gate must exclude it from the checks it +# waits on. It is left "in_progress" so a failure to exclude it would hang the +# terminal-state loop until timeout (surfacing the bug). +SELF_RUN='{"name":"auto-merge","status":"in_progress","conclusion":null,"html_url":"https://github.com/o/r/actions/runs/999/job/2","details_url":"https://github.com/o/r/actions/runs/999/job/2"}' +tests_run() { # $1 = conclusion + printf '{"name":"tests","status":"completed","conclusion":"%s","html_url":"https://github.com/o/r/actions/runs/222/job/1","details_url":"https://github.com/o/r/actions/runs/222/job/1"}' "$1" +} +echo "{\"total_count\":2,\"check_runs\":[$(tests_run success),$SELF_RUN]}" > "$FIX/green.json" +echo "{\"total_count\":2,\"check_runs\":[$(tests_run failure),$SELF_RUN]}" > "$FIX/red.json" +echo "{\"total_count\":1,\"check_runs\":[$SELF_RUN]}" > "$FIX/noci.json" +echo '{"state":"pending","total_count":0,"statuses":[]}' > "$FIX/status.json" + +# --- harness ----------------------------------------------------------------- +run_case() { # $1 name; remaining: VAR=VAL env for the run. Populates $LOG/$OUT. + # Use `env` so the caller's VAR=VAL args (which arrive via "$@" expansion, and + # so are NOT recognised as shell assignments) are applied to the environment. + local name="$1"; shift + LOG="$(mktemp)"; OUT="$(mktemp)"; COUNTER_FILE="$(mktemp)"; : > "$COUNTER_FILE" + env \ + "PATH=$BIN:$PATH" "GH_LOG=$LOG" "COUNTER=$COUNTER_FILE" \ + "STATUS_JSON=$FIX/status.json" FAKE_SHA="cafef00d" FAKE_PR_STATE="OPEN" \ + GITHUB_RUN_ID="999" PR_URL="https://github.com/o/r/pull/1" GITHUB_TOKEN="x" \ + CI_GATE_ENABLED="true" MERGE_METHOD="rebase" GATE_POLL_SECONDS="1" \ + "$@" \ + bash "$SCRIPT" > "$OUT" 2>&1 + echo " [case] $name" +} + +assert_grep() { if grep -qF "$1" "$2"; then echo " ok: found '$1'"; else echo " FAIL: expected '$1'"; FAILURES=$((FAILURES + 1)); fi; } +assert_no_grep() { if grep -qF "$1" "$2"; then echo " FAIL: unexpected '$1'"; FAILURES=$((FAILURES + 1)); else echo " ok: absent '$1'"; fi; } + +echo "== 1. API blips then green -> MERGE (the #11 regression) ==" +run_case "blip_then_green" \ + BLIP_COUNT="3" CHECK_RUNS_JSON="$FIX/green.json" \ + GATE_APPEAR_SECONDS="5" GATE_TIMEOUT_SECONDS="30" +assert_grep "pr merge --rebase" "$LOG" +assert_no_grep "no CI checks registered" "$LOG" +assert_no_grep "could not read CI status" "$LOG" + +echo "== 2. Green immediately -> MERGE (self-run excluded) ==" +run_case "green" \ + CHECK_RUNS_JSON="$FIX/green.json" \ + GATE_APPEAR_SECONDS="5" GATE_TIMEOUT_SECONDS="30" +assert_grep "pr merge --rebase" "$LOG" +assert_no_grep "pr comment" "$LOG" + +echo "== 3. Genuinely no CI -> ROUTE TO HUMAN (fail-safe preserved) ==" +run_case "no_ci" \ + CHECK_RUNS_JSON="$FIX/noci.json" \ + GATE_APPEAR_SECONDS="2" GATE_TIMEOUT_SECONDS="30" +assert_grep "no CI checks registered" "$LOG" +assert_no_grep "pr merge --rebase" "$LOG" + +echo "== 4. Red CI -> ROUTE TO HUMAN ==" +run_case "red_ci" \ + CHECK_RUNS_JSON="$FIX/red.json" \ + GATE_APPEAR_SECONDS="5" GATE_TIMEOUT_SECONDS="30" +assert_grep "CI is not passing" "$LOG" +assert_no_grep "pr merge --rebase" "$LOG" + +echo +if [ "$FAILURES" -eq 0 ]; then echo "ALL PASS"; else echo "$FAILURES ASSERTION(S) FAILED"; fi +exit "$FAILURES" diff --git a/.github/workflows/tooling-tests.yaml b/.github/workflows/tooling-tests.yaml new file mode 100644 index 0000000..43dc41d --- /dev/null +++ b/.github/workflows/tooling-tests.yaml @@ -0,0 +1,31 @@ +name: Review tooling tests + +# Lints and tests the shared Dependabot auto-merge tooling so a change to the CI +# gate can't silently regress (e.g. realrate/.github#11, where a transient +# check-read was misread as "no CI"). Runs on PRs and pushes that touch it. + +on: + pull_request: + paths: + - ".github/review-tooling/**" + - ".github/workflows/tooling-tests.yaml" + push: + branches: [main] + paths: + - ".github/review-tooling/**" + - ".github/workflows/tooling-tests.yaml" + +permissions: + contents: read + +jobs: + tooling-tests: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 + + - name: shellcheck + run: shellcheck .github/review-tooling/gated_merge.sh .github/review-tooling/test_gated_merge.sh + + - name: gated_merge regression tests + run: bash .github/review-tooling/test_gated_merge.sh