diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 795b3736f..28e4c8892 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -146,6 +146,7 @@ jobs: tests/test_validate_cases.bats \ tests/test_review_registry.bats \ tests/test_reviewer_sources.bats \ + tests/test_pr_approval_diagnostic.bats \ tests/test_deep_specialist.bats \ tests/test_pr_diff_dispatch.bats \ tests/test_plan_review.bats \ diff --git a/AGENTS.md b/AGENTS.md index e732fc21f..acd81ce64 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -638,6 +638,10 @@ run to `remediate=live` for the configured pilot. - Scripts must be POSIX-compatible shell (`#!/usr/bin/env bash` with `set -euo pipefail`). - No hardcoded tokens or secrets — use `$GITHUB_TOKEN` from the environment. +- **"Why isn't this PR approved?"** Run `bash scripts/pr-approval-diagnostic.sh ` + (profile: [`agents/why-not-approved.md`](./agents/why-not-approved.md)). It runs the real + pr-review gate chain read-only (`PR_REVIEW_DIAGNOSE=true`) and reports the first gate holding + the PR and what clears it. Do not reconstruct the gates by hand (#1894). ### Model selection — name a family, never pin a version diff --git a/agents/why-not-approved.md b/agents/why-not-approved.md new file mode 100644 index 000000000..492539085 --- /dev/null +++ b/agents/why-not-approved.md @@ -0,0 +1,75 @@ +--- +name: why-not-approved +description: > + Explains why pr-review has not approved a petry-projects pull request, and what + would change that, by running the real pr-review gate chain read-only. Invoke on + "why isn't PR N approved?", "what is blocking this PR?", or a PR stuck at + REVIEW_REQUIRED with green CI. +tools: ["read", "search", "execute"] +--- + +# Why isn't this PR approved? + +pr-review approves a PR only after a chain of gates passes, in this order: CI, +advisory-bot reviews, maintainer comments, maintainer review threads, standing +change requests, the already-reviewed-at-head check, the human-escalation hold and +cycle cap, and the per-PR automation budget. Then the model review runs and its verdict decides. +This skill finds the first gate holding the PR by running that real chain, not by +reading the PR and guessing. + +## Run the diagnostic + +The script lives in `petry-projects/.github-private`. If it is not in your checkout, +make a shallow clone of that repo first, through `gh` so the private repo is +authenticated: `gh repo clone petry-projects/.github-private -- --depth 1`. You need +`gh` authenticated with repo read on the PR's repository. + +```bash +bash scripts/pr-approval-diagnostic.sh https://github.com///pull/ +``` + +It runs `scripts/review-one-pr.sh` in diagnose mode (`PR_REVIEW_DIAGNOSE=true`). +That mode is read-only and never forced, and it stops before any model runs. The +output is a Markdown report: +- what pr-review would do now (`decision` and `reason`); +- what changes that; +- dev-lead hold labels (pr-review itself gates only on `needs-human-review`); +- approvals at the head; +- the gate log. + +Add `--json` for a machine-readable object. + +## Explain the result + +Lead with the blocking gate and the one action that clears it. The report's +"What changes that" line is the authoritative condition; restate it plainly. Only +the first holding gate is reported: say so when the user asks about others. + +Common reasons: + +| `reason` | What it means | What clears it | +|---|---|---| +| `ci-failing` / `ci-pending` | A required check is red or still running | Fix the check or wait for it; the sweep re-reviews | +| `waiting-for-advisory-bots` | Advisory reviewers have not all reported | Their review event re-triggers pr-review; a head-age timeout eventually proceeds without them | +| `changes-requested` | A reviewer requested changes and that review still stands | The author pushes a new commit (making that review stale), or @mentions the bot for a re-review | +| `undispositioned-pr-comment` | A PR comment has no verified disposition | dev-lead replies with a disposition and the comment is minimized RESOLVED | +| `unaddressed-maintainer-review-thread` | A maintainer review thread is open since the last push | Resolve the thread | +| `already-reviewed-at-head` | pr-review already gave a verdict at this commit | Push a commit, or change the PR metadata if the verdict was metadata-only | +| `human-escalated` | pr-review escalated to a human and `needs-human-review` is on | A human reviews and removes the label, or @mentions the bot | +| `max-cycles-reached` | Three review cycles without converging | A human removes `needs-human-review` for a fresh budget | +| `automation-budget-exhausted` | Too much automated activity since a human last acted | Any human comment or approval resets it | +| `automation-budget-unreadable` | The PR's activity could not be read (a GitHub API error) | Re-run the diagnostic | +| `gates-clear` (`proceed`) | Nothing holds the PR | The next pr-review run reviews it; its verdict decides | + +If the PR is already approved but not merging, the report's merge state says why +(for example `BLOCKED` by a required check, or `BEHIND`). + +## When the script cannot run + +Without a checkout or `gh` credentials, look at the PR's latest pr-review run. A +run that a gate stopped records `decision / reason / would_change` in its log and +step summary (#1552, #1894). A run that reached the model review answers with the +review it posted on the PR instead: read that review. If no review appears, read +that run's log for a model or posting failure. Do not reconstruct the +gates by hand: that copy drifts from the real ones, which is why #1902's first +version was replaced. diff --git a/scripts/lib/advisory-review-gate.sh b/scripts/lib/advisory-review-gate.sh index 9b7f792eb..4705d5def 100755 --- a/scripts/lib/advisory-review-gate.sh +++ b/scripts/lib/advisory-review-gate.sh @@ -165,14 +165,20 @@ readonly YELLOW='\033[1;33m' readonly GREEN='\033[0;32m' readonly NC='\033[0m' # No Color +# log_info +# Log informational message to stderr with advisory-gate prefix. log_info() { echo "[advisory-gate] $*" >&2 } +# log_warn +# Log warning message to stderr with yellow color and WARNING label. log_warn() { echo -e "${YELLOW}[advisory-gate] WARNING: $*${NC}" >&2 } +# log_success +# Log success message to stderr with green color. log_success() { echo -e "${GREEN}[advisory-gate] $*${NC}" >&2 } @@ -452,23 +458,11 @@ Advisory bots were rate-limited; auto-approval is withheld until they recover. p fi } -# Instant (non-blocking) check of advisory bot status -# -# DESIGN: This uses a re-trigger pattern instead of blocking waits: -# 1. On first pr-review trigger (check_suite completion): instant check -# 2. If return 0: bots ready → approve immediately -# 3. If return 1: bots not ready → skip (exit 100) -# 4. When bots submit: pull_request_review event fires -# 5. pr-review re-triggered → this check returns 0 → approve -# -# Returns: -# 0 = All detected participating bots have submitted (ready to approve) -# 1 = Waiting for bots (skip, will re-check on next review event) -# -# _record_partial_evidence — record approval on -# PARTIAL advisory evidence so the miss-rate metric counts it (#1596). Gate runs -# BEFORE the write, so with PARTIAL_EVIDENCE_STATE_FILE set we DEFER (record facts, -# post NOTHING); post-pr-review.sh announces only after verifying the review (#1874). +# _record_partial_evidence +# Record approval on PARTIAL advisory evidence so the miss-rate metric counts it +# (#1596). Gate runs BEFORE the write, so with PARTIAL_EVIDENCE_STATE_FILE set we +# DEFER (record facts, post NOTHING); post-pr-review.sh announces only after +# verifying the review (#1874). _record_partial_evidence() { if [ -n "${PARTIAL_EVIDENCE_STATE_FILE:-}" ]; then printf '%s %s %s\n' "$1" "$2" "$3" > "$PARTIAL_EVIDENCE_STATE_FILE" 2>/dev/null \ @@ -481,6 +475,12 @@ _record_partial_evidence() { || echo "::warning::partial-evidence marker post failed on ${PR_URL} — approval may be uncounted by the miss-rate metric (#1596)" } +# check_advisory_reviews +# Instant (non-blocking) check of advisory bot review status. Returns 0 when all +# detected advisory bots have submitted their reviews (or the head-age/quiescence +# timeouts (#1193) for absent bots apply); returns 1 when still waiting; returns 2 +# when the GitHub API query or jq parse fails — callers must fail closed, not treat +# it as "waiting". check_advisory_reviews() { local pr_url="${1:-}" if [[ -z "$pr_url" ]]; then @@ -555,7 +555,11 @@ check_advisory_reviews() { effective_total=$((total_advisory_bots - num_unavailable)) [[ "$effective_total" -lt 1 ]] && effective_total=1 if [[ "$num_unavailable" -gt 0 ]]; then - log_info "${num_unavailable} advisory bot(s) rate-limited/unsupported — required set reduced to ${effective_total}/${total_advisory_bots}" + # Name WHICH bots dropped and why so the reduced denominator is observable — the + # registry total and effective total must never differ silently (#1894 AC #2). + local unavailable_named + unavailable_named=$(echo "$current_states" | jq -rs '[.[] | select(.state == "RATE_LIMITED" or .state == "UNSUPPORTED") | "\(.bot)=\(.state)"] | unique | join(", ")') + log_info "${num_unavailable} advisory bot(s) rate-limited/unsupported — required set reduced to ${effective_total}/${total_advisory_bots}; dropped: ${unavailable_named}" fi # Require the effective advisory set to submit before approving. diff --git a/scripts/pr-approval-diagnostic.sh b/scripts/pr-approval-diagnostic.sh new file mode 100755 index 000000000..b659d1b25 --- /dev/null +++ b/scripts/pr-approval-diagnostic.sh @@ -0,0 +1,118 @@ +#!/usr/bin/env bash +# pr-approval-diagnostic.sh — "why isn't this PR approved?" (#1894) +# +# Answers from the REAL gate chain, not a model of it: runs scripts/review-one-pr.sh +# in diagnose mode (PR_REVIEW_DIAGNOSE=true — read-only, never forced, stops before +# any model runs) and reports the single verdict that run reaches — which gate holds +# the PR, and what would change the decision — together with the PR facts a reader +# needs (review decision, approvals at the head, hold labels, merge state). +# +# The previous attempt (#1902) re-implemented each gate in a separate library and +# drifted from the real ones (bot-comment handling, the unsupported-file regex, +# check-run participation, the thread-gate fail-closed path). Running the gate chain +# itself makes that drift impossible: a gate change is a diagnostic change. +# +# Usage: pr-approval-diagnostic.sh [--json] +# Needs the same environment a pr-review run needs to READ the PR (GH_TOKEN with +# repo read; BOT_USER defaults to donpetry-bot). Writes nothing to GitHub. +# Exit: 0 diagnosed · 2 no diagnosis: the gate chain produced no verdict (its log +# tail is printed), the PR facts could not be read, or the head moved while +# diagnosing (re-run) + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/hold-gate.sh +source "$SCRIPT_DIR/lib/hold-gate.sh" +PR_URL="${1:-}" +FORMAT="markdown" +[ "${2:-}" = "--json" ] && FORMAT="json" +if [[ ! "$PR_URL" =~ ^https://github\.com/[^/]+/[^/]+/pull/[0-9]+$ ]]; then + echo "usage: pr-approval-diagnostic.sh https://github.com///pull/ [--json]" >&2 + exit 2 +fi + +LOG="$(mktemp)" +trap 'rm -f "$LOG"' EXIT + +set +e +PR_REVIEW_DIAGNOSE=true GITHUB_STEP_SUMMARY="" bash "$SCRIPT_DIR/review-one-pr.sh" "$PR_URL" >"$LOG" 2>&1 +rc=$? +set -e + +# The run's verdict is its LAST emit_verdict line (a JSON object with .decision). +verdict=$(grep -E '^\{"pr":' "$LOG" | tail -n 1 || true) +if [ -z "$verdict" ] || ! jq -e '.decision' <<<"$verdict" >/dev/null 2>&1; then + echo "pr-approval-diagnostic: the gate chain produced no verdict (exit $rc); last lines:" >&2 + tail -n 20 "$LOG" >&2 + exit 2 +fi + +# Facts come from a second read of the PR. A failed read is reported as such — +# never as "no approvals, no holds" — and a head that moved since the gate run +# would describe a different commit than the verdict, so both stop the report. +pr_json=$(gh pr view "$PR_URL" --json state,isDraft,headRefOid,reviewDecision,mergeStateStatus,labels,reviews 2>/dev/null) \ + && jq -e '.headRefOid' <<<"$pr_json" >/dev/null 2>&1 \ + || { echo "pr-approval-diagnostic: could not read the PR's review facts (gh pr view failed)" >&2; exit 2; } +verdict_sha=$(jq -r '.sha // ""' <<<"$verdict") +if [ -n "$verdict_sha" ] && [ "$(jq -r '.headRefOid' <<<"$pr_json")" != "$verdict_sha" ]; then + echo "pr-approval-diagnostic: the PR head moved while diagnosing (gate run saw ${verdict_sha:0:8}); re-run" >&2 + exit 2 +fi +facts=$(jq -c --arg holds "$(hold_gate_labels | tr '\n' ' ')" ' + .headRefOid as $head + | {state, isDraft, reviewDecision, mergeStateStatus, + holds: [(.labels // [])[].name | select(. as $l | ($holds | split(" ") | index($l)))], + approvals_at_head: [(.reviews // [])[] + | select(.state == "APPROVED" and (.commit.oid // "") == $head) + | (.author.login // "?")] | unique}' <<<"$pr_json") + +# The gate log keeps the gates' own lines (indented) and the advisory gate's +# `[advisory-gate]` lines — e.g. which unavailable bots reduced the required set. +gate_log=$(sed 's/\x1b\[[0-9;]*m//g' "$LOG" | grep -E '^ |\[advisory-gate\]' || true) +report=$(jq -cn --argjson v "$verdict" --argjson f "$facts" --arg log "$gate_log" \ + '$v + {facts: $f, gate_log: $log}') + +if [ "$FORMAT" = "json" ]; then + printf '%s\n' "$report" + exit 0 +fi + +jq -r ' + def code: "`" + (. // "") + "`"; + .facts as $f + # GitHub can still show an approval while a gate would now hold the PR: lead + # with the gate, not "Approved". Every verdict holds except `proceed` and the + # one true no-op, `already-reviewed-at-head` (e.g. `noop human-escalated` holds). + | (if ($f.reviewDecision // "") == "APPROVED" and .decision != "proceed" and .reason != "already-reviewed-at-head" + then "**GitHub shows the PR approved, but pr-review would now hold it** at the " + (.reason | code) + " gate." + elif ($f.reviewDecision // "") == "APPROVED" + then "**Approved.** Merge state: " + (($f.mergeStateStatus // "unknown") | code) + "." + else "**Not approved** (review decision: " + (($f.reviewDecision // "none") | if . == "" then "none" else . end | code) + ")." + end) as $status + | [ "## pr-review approval diagnostic: " + .pr, + "", + $status, + "", + "| | |", + "|---|---|", + "| Head | " + ((.sha // "")[0:8] | code) + " |", + "| What pr-review would do now | " + (.decision | code) + " — " + (.reason | code) + " |", + "| What changes that | " + ((.would_change // "") | gsub("\\|"; "\\|")) + " |", + "| dev-lead hold labels | " + (if (($f.holds // []) | length) > 0 then ($f.holds | map(code) | join(", ")) else "none" end) + " |", + "| Approvals at head | " + (if (($f.approvals_at_head // []) | length) > 0 then ($f.approvals_at_head | join(", ")) else "none" end) + " |", + "", + "", + "_dev-lead hold labels stop dev-lead acting on the PR. Of them, pr-review gates only on `needs-human-review`, and when it holds the PR the verdict above names it._", + (if .decision == "proceed" + then "No gate holds this PR: the next pr-review run reviews it, and that review decides." + else "This is the first gate that holds the PR, in the order a pr-review run checks them. Gates after it were not evaluated." + end), + "", + "
Gate log", + "", + "```", + .gate_log, + "```", + "
" + ] | join("\n")' <<<"$report" diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index 066dbe8da..1ec267e5a 100755 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -120,6 +120,65 @@ source "$SCRIPT_DIR/lib/carry-forward.sh" PR_URL="${1:?usage: review-one-pr.sh }" export PR_URL +# Diagnose mode (#1894): answer "why isn't this PR approved?" by running the REAL +# gate chain below — the same functions, in the same order, on the same live +# state — and stopping before any model runs. It is read-only (DRY_RUN) and never +# forced: a diagnosis must report what a normal run would do, so the human +# break-glass and the orphan-rescue bypass are both off. The answer is the run's +# single emit_verdict line (decision, reason, would_change). Used by +# scripts/pr-approval-diagnostic.sh; never set by the review workflows. +# +# DRY_RUN alone is not a guarantee — a gate's write that predates its DRY_RUN +# guard (the advisory rate-limited marker did, #1902 review) would still post. +# So in diagnose mode `gh` itself refuses every write: the refused call fails +# (visible in the log, never silently "succeeds"), and reads pass through. +_diagnose_gh_is_write() { + local sub="${1:-}" verb="${2:-}" a prev="" method="" has_params=false query="" + case "$sub" in + pr|issue) case "$verb" in view|list|status|diff|checks) return 1 ;; *) return 0 ;; esac ;; + label|release|repo|workflow|run|secret|variable) + case "$verb" in view|list|download|watch) return 1 ;; *) return 0 ;; esac ;; + api) ;; + auth) case "$verb" in status|token) return 1 ;; *) return 0 ;; esac ;; + search|status|help|version|--version|--help) return 1 ;; + # Fail closed: any other subcommand (gist, codespace, ruleset, an extension…) + # is refused unless it is listed above as read-only. + *) return 0 ;; + esac + shift + for a in "$@"; do + case "$a" in + -X?*) method="${a#-X}" ;; + --method=*) method="${a#--method=}" ;; + esac + case "$prev" in + -X|--method) method="$a" ;; + -f|-F|--field|--raw-field) has_params=true; [[ "$a" == query=* ]] && query="${a#query=}" ;; + --input) has_params=true ;; + esac + prev="$a" + done + method=$(printf '%s' "$method" | tr '[:lower:]' '[:upper:]') + if [ "${1:-}" = "graphql" ]; then + # GraphQL is always POST; it writes only through a mutation. + [[ "$query" =~ ^[[:space:]]*mutation ]] && return 0 + return 1 + fi + [ -n "$method" ] && { [ "$method" != "GET" ]; return; } + $has_params # gh api defaults to POST when fields or an --input body are given +} +if [ "${PR_REVIEW_DIAGNOSE:-false}" = "true" ]; then + DRY_RUN=true FORCE_REVIEW=false FORCE_RE_REVIEW=false + export DRY_RUN FORCE_REVIEW FORCE_RE_REVIEW + gh() { + if _diagnose_gh_is_write "$@"; then + echo " diagnose: refused a GitHub write (gh $1 ${2:-}) — diagnose mode is read-only" >&2 + return 1 + fi + command gh "$@" + } +fi + # emit_verdict # Emit THE single structured verdict line for a run that DECLINES to review this # PR (skip / noop / escalate / error). It carries pr, head sha, decision, a @@ -138,6 +197,14 @@ emit_verdict() { --arg reason "$2" \ --arg would_change "$3" \ '{pr:$pr, sha:$sha, decision:$decision, reason:$reason, would_change:$would_change}' + # #1894: also record it in the step summary, so "why not approved?" is answered + # where an operator looks, not only in the run log. Best-effort. (A run that + # reaches the model cascade answers with the review it posts instead.) + if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then + local _sha="${PR_HEAD_SHA:-}" + printf -- '- **%s** at `%s`: `%s` (%s). Changes when: %s\n' \ + "$PR_URL" "${_sha:0:8}" "$1" "$2" "$3" >> "$GITHUB_STEP_SUMMARY" 2>/dev/null || true + fi } echo "==> $PR_URL" @@ -912,6 +979,29 @@ if [ "${DRY_RUN:-false}" != "true" ]; then fi exit 101 fi +elif [ "${PR_REVIEW_DIAGNOSE:-false}" = "true" ]; then + # Read-only: report an exhausted budget without escalating (enforce_pr_budget writes). + # An unreadable event list is not "budget remaining": report it, never gates-clear. + _diag_rc=0 + _diag_events=$(gather_pr_automation_events \ + "$(echo "$PR_URL" | sed -E 's|.*/pull/([0-9]+).*|\1|')" \ + "$(echo "$PR_URL" | sed -E 's|https://github.com/([^/]+/[^/]+)/pull/.*|\1|')") || _diag_rc=$? + if [ "$_diag_rc" -ne 0 ]; then + emit_verdict error automation-budget-unreadable "the PR's commits, comments and reviews become readable (a GitHub API error); re-run the diagnostic" + exit 100 + fi + if pr_budget_exhausted "$_diag_events"; then + emit_verdict escalate automation-budget-exhausted "a human interaction (comment or approval) resets the per-PR automation budget" + exit 100 + fi +fi + +# Diagnose mode stops here: every gate has passed, so a review run would now start +# the model cascade, whose own verdict (approve / fix-request / escalate) decides. +if [ "${PR_REVIEW_DIAGNOSE:-false}" = "true" ]; then + echo " diagnose: every gate passes — a review run would start the model cascade now" + emit_verdict proceed gates-clear "nothing gates this PR; the next pr-review run reviews it and its verdict decides (a review event, a push, or the pr-review sweep starts that run)" + exit 0 fi # Detect if the PR's repo org supports AI delegation for automated fix requests. diff --git a/tests/dev-lead/unit/test_advisory_review_gate.bats b/tests/dev-lead/unit/test_advisory_review_gate.bats index 8da245bfd..7c51d4fca 100755 --- a/tests/dev-lead/unit/test_advisory_review_gate.bats +++ b/tests/dev-lead/unit/test_advisory_review_gate.bats @@ -169,10 +169,12 @@ teardown() { @test "Advisory gate: script is minimal (non-blocking means fewer lines)" { local lines lines=$(wc -l < "$SCRIPT_DIR/lib/advisory-review-gate.sh") - # Budget raised 500→515 for the #1596 partial-evidence decision-point hooks. The + # Budget raised 500→515 for the #1596 partial-evidence decision-point hooks, then + # 515→520 for the #1894 AC #2 dropped-bot naming (four lines that make the reduced + # advisory denominator observable — which member fell out of "6 != 7" and why). The # heavy marker logic lives in scripts/lib/partial-evidence-marker.sh; only the # three irreducible timeout-branch hooks (deduplicated via _record_partial_evidence) - # are here. Raised 515→560 for the #1903 cubic registration (author-scoped + # are here. Raised 520→565 for the #1903 cubic registration (author-scoped # trial-ended detector, fallback list, notice bots list, pattern accessor, and # comprehensive test coverage for false positive protection). Raised 560→600 for # #2008: the section-aware CodeRabbit rate-limit scope (a shared jq def that both diff --git a/tests/test_pr_approval_diagnostic.bats b/tests/test_pr_approval_diagnostic.bats new file mode 100644 index 000000000..b3d226c21 --- /dev/null +++ b/tests/test_pr_approval_diagnostic.bats @@ -0,0 +1,287 @@ +#!/usr/bin/env bats +# #1894: "why isn't this PR approved?" — answered by the REAL gate chain. +# +# scripts/review-one-pr.sh in diagnose mode (PR_REVIEW_DIAGNOSE=true) runs every +# gate in order on the live snapshot, then stops before any model runs. It is +# read-only and never forced. scripts/pr-approval-diagnostic.sh wraps it and adds +# the PR facts. These tests drive both end to end with the same gh stub as +# tests/test_review_one_pr_force_re_review.bats, and assert the run never writes +# to GitHub and never starts an engine. + +setup() { + REPO_ROOT="$(cd "$(dirname "$BATS_TEST_FILENAME")/.." && pwd)" + export REVIEW_SCRIPT="$REPO_ROOT/scripts/review-one-pr.sh" + export DIAG_SCRIPT="$REPO_ROOT/scripts/pr-approval-diagnostic.sh" + + export SHA="1894a1b2c3d4e5f60718293a4b5c6d7e8f901234" + export PR_URL="https://github.com/petry-projects/.github-private/pull/1894" + + export TEST_DIR="$BATS_TEST_TMPDIR" + mkdir -p "$TEST_DIR/bin" + cd "$TEST_DIR" + + export SNAPSHOT="$TEST_DIR/snapshot.json" + export ENGINE_LOG="$TEST_DIR/engines.log"; : > "$ENGINE_LOG" + export GH_LOG="$TEST_DIR/gh_calls.log" + : > "$GH_LOG" + + # gh stub: honors `pr view --jq ` (so the idempotency block's own marker + # query works), returns the snapshot for a plain `pr view`, and answers the + # GraphQL shapes the gates use with old timestamps so they clear. + cat > "$TEST_DIR/bin/gh" <<'GHEOF' +#!/bin/bash +printf '%s\n' "$*" >> "$GH_LOG" +if [ "$1" = "pr" ] && [ "$2" = "view" ]; then + jqf=""; prev="" + for a in "$@"; do + [ "$prev" = "--jq" ] && jqf="$a" + prev="$a" + done + if [ -n "$jqf" ]; then jq -r "$jqf" "$SNAPSHOT"; else cat "$SNAPSHOT"; fi + exit 0 +fi +if [ "$1" = "api" ] && [ "$2" = "graphql" ]; then + case "$*" in + *reviewThreads*) printf '%s' '{"data":{"resource":{"reviewThreads":{"pageInfo":{"hasNextPage":false},"nodes":[]}}}}' ;; + *pushedDate*) printf '%s' '{"data":{"resource":{"commits":{"nodes":[{"commit":{"pushedDate":"2020-01-01T00:00:00Z","committer":{"date":"2020-01-01T00:00:00Z"}}}]}}}}' ;; + *) printf '%s\n' '2020-01-01T00:00:00Z' ;; + esac + exit 0 +fi +exit 0 +GHEOF + chmod +x "$TEST_DIR/bin/gh" + + # Stub engines so a bypassed (proceed) run can't block on a real CLI. + for e in claude copilot gemini; do + printf '#!/bin/bash\necho "%s" >> "$ENGINE_LOG"\nexit 0\n' "$e" > "$TEST_DIR/bin/$e" + chmod +x "$TEST_DIR/bin/$e" + done + + export PATH="$TEST_DIR/bin:$PATH" + export REVIEW_ENGINE="claude" GH_TOKEN="fake" + unset DRY_RUN GITHUB_STEP_SUMMARY + unset FORCE_REVIEW FORCE_RE_REVIEW +} + + +ROLLUP_PASS='[{"name":"CI / build","status":"COMPLETED","conclusion":"SUCCESS"}]' +ROLLUP_FAIL='[{"name":"CI / build","status":"COMPLETED","conclusion":"FAILURE"}]' +ROLLUP_PENDING='[{"name":"CI / build","status":"IN_PROGRESS","conclusion":null}]' + +# write_snap [labels-json] [comments-json] [reviews-json] [reviewDecision] +write_snap() { + jq -n --arg sha "$SHA" --argjson rollup "$1" --argjson labels "${2:-[]}" \ + --argjson comments "${3:-[]}" --argjson reviews "${4:-[]}" --arg rd "${5:-REVIEW_REQUIRED}" '{ + state: "OPEN", isDraft: false, mergeStateStatus: "BLOCKED", + headRefOid: $sha, baseRefName: "main", statusCheckRollup: $rollup, reviewDecision: $rd, + reviews: $reviews, labels: ($labels | map({name: .})), closingIssuesReferences: [], + body: "PR body", comments: $comments + }' > "$SNAPSHOT" +} + +# _assert_read_only — no GitHub write of any kind, and no engine ran. +_assert_read_only() { + # Explicit failure: a negated command is exempt from errexit, so `! grep` here + # would never fail the test. + if grep -qiE 'pr (comment|edit|review|merge|close)|issue (comment|edit|close|create|reopen)|gist |mutation|--method[= ](POST|PUT|PATCH|DELETE)|-X ?(POST|PUT|PATCH|DELETE)' "$GH_LOG"; then + cat "$GH_LOG" + return 1 + fi + [ ! -s "$ENGINE_LOG" ] +} + +_verdict() { grep -E '^\{"pr":' <<<"$output" | tail -n 1; } + +@test "diagnose: every gate passes → proceed/gates-clear, and the cascade never starts" { + write_snap "$ROLLUP_PASS" + PR_REVIEW_DIAGNOSE=true run timeout 25 bash "$REVIEW_SCRIPT" "$PR_URL" + [ "$status" -eq 0 ] + [ "$(_verdict | jq -r '.decision + "/" + .reason')" = "proceed/gates-clear" ] + [[ "$output" == *"diagnose: every gate passes"* ]] + _assert_read_only +} + +@test "diagnose: failing CI names the ci-failing gate and what clears it" { + write_snap "$ROLLUP_FAIL" + PR_REVIEW_DIAGNOSE=true run timeout 25 bash "$REVIEW_SCRIPT" "$PR_URL" + [ "$(_verdict | jq -r '.reason')" = "ci-failing" ] + [[ "$(_verdict | jq -r '.would_change')" == *"turn green"* ]] + _assert_read_only +} + +@test "diagnose: pending CI reports ci-pending without polling" { + write_snap "$ROLLUP_PENDING" + PR_REVIEW_DIAGNOSE=true run timeout 25 bash "$REVIEW_SCRIPT" "$PR_URL" + [[ "$(_verdict | jq -r '.reason')" == ci-pending* ]] + [[ "$output" != *"waiting"*"for checks to settle"* ]] + _assert_read_only +} + +@test "diagnose is never forced: FORCE_REVIEW in the environment does not bypass a gate" { + write_snap "$ROLLUP_FAIL" + FORCE_REVIEW=true FORCE_RE_REVIEW=true PR_REVIEW_DIAGNOSE=true run timeout 25 bash "$REVIEW_SCRIPT" "$PR_URL" + [ "$(_verdict | jq -r '.reason')" = "ci-failing" ] + [[ "$output" != *"bypassing the ci-failing gate"* ]] + _assert_read_only +} + +@test "diagnose: an escalated PR holding needs-human-review reports the human escalation" { + write_snap "$ROLLUP_PASS" '["needs-human-review"]' \ + '[{"author":{"login":"donpetry-bot"},"createdAt":"2026-10-09T04:44:00Z","body":"\nescalated"}]' + PR_REVIEW_DIAGNOSE=true run timeout 25 bash "$REVIEW_SCRIPT" "$PR_URL" + [ "$(_verdict | jq -r '.reason')" = "human-escalated" ] + _assert_read_only +} + +@test "diagnose: a verdict already at head reports already-reviewed-at-head" { + write_snap "$ROLLUP_PASS" '[]' \ + '[{"author":{"login":"donpetry-bot"},"createdAt":"2026-10-09T05:00:00Z","body":" "}]' + PR_REVIEW_DIAGNOSE=true run timeout 25 bash "$REVIEW_SCRIPT" "$PR_URL" + [ "$(_verdict | jq -r '.reason')" = "already-reviewed-at-head" ] + _assert_read_only +} + +@test "verdicts are also written to the step summary (#1894 — where an operator looks)" { + write_snap "$ROLLUP_FAIL" + export GITHUB_STEP_SUMMARY="$TEST_DIR/summary.md" + PR_REVIEW_DIAGNOSE=true run timeout 25 bash "$REVIEW_SCRIPT" "$PR_URL" + grep -q "$PR_URL"'.*`skip` (ci-failing)\. Changes when: ' "$GITHUB_STEP_SUMMARY" +} + +@test "wrapper: markdown report names the gate, what clears it, holds and approvals" { + write_snap "$ROLLUP_FAIL" '["dev-lead:hands-off","bug"]' '[]' \ + '[{"author":{"login":"cubic-dev-ai"},"state":"APPROVED","commit":{"oid":"'"$SHA"'"},"body":"ok"}]' + run timeout 30 bash "$DIAG_SCRIPT" "$PR_URL" + [ "$status" -eq 0 ] + [[ "$output" == *"## pr-review approval diagnostic: $PR_URL"* ]] + [[ "$output" == *'**Not approved** (review decision: `REVIEW_REQUIRED`).'* ]] + [[ "$output" == *'| What pr-review would do now | `skip` — `ci-failing` |'* ]] + [[ "$output" == *'| dev-lead hold labels | `dev-lead:hands-off` |'* ]] + [[ "$output" == *'pr-review gates only on `needs-human-review`'* ]] + [[ "$output" == *'| Approvals at head | cubic-dev-ai |'* ]] + [[ "$output" == *"This is the first gate that holds the PR"* ]] + _assert_read_only +} + +@test "wrapper: an approved PR that a gate would now hold leads with the gate, not 'Approved' (#1902 review)" { + write_snap "$ROLLUP_FAIL" '[]' '[]' \ + '[{"author":{"login":"donpetry-bot"},"state":"APPROVED","commit":{"oid":"'"$SHA"'"},"body":"ok"}]' APPROVED + run timeout 30 bash "$DIAG_SCRIPT" "$PR_URL" + [ "$status" -eq 0 ] + [[ "$output" == *'**GitHub shows the PR approved, but pr-review would now hold it** at the `ci-failing` gate.'* ]] + [[ "$output" != *'**Approved.**'* ]] +} + +@test "wrapper: the gate log keeps the advisory gate's lines, without colour codes (#1902 review)" { + write_snap "$ROLLUP_PASS" + # Inject an advisory-gate warning line (as log_warn prints it) into the gate run's output. + cat > "$TEST_DIR/bin/review-wrap" <<'EOF2' +#!/bin/bash +printf '\033[1;33m[advisory-gate] WARNING: required set reduced: dropped coderabbitai[bot] (RATE_LIMITED)\033[0m\n' >&2 +exec bash "$REAL_REVIEW" "$@" +EOF2 + chmod +x "$TEST_DIR/bin/review-wrap" + mkdir -p "$TEST_DIR/s"; cp "$DIAG_SCRIPT" "$TEST_DIR/s/pr-approval-diagnostic.sh"; cp -r "$REPO_ROOT/scripts/lib" "$TEST_DIR/s/lib" + printf '#!/bin/bash\nexec "%s" "$@"\n' "$TEST_DIR/bin/review-wrap" > "$TEST_DIR/s/review-one-pr.sh" + REAL_REVIEW="$REVIEW_SCRIPT" run timeout 30 bash "$TEST_DIR/s/pr-approval-diagnostic.sh" "$PR_URL" --json + [ "$status" -eq 0 ] + [[ "$(jq -r '.gate_log' <<<"$output")" == *"[advisory-gate] WARNING: required set reduced: dropped coderabbitai[bot] (RATE_LIMITED)"* ]] + [[ "$(jq -r '.gate_log' <<<"$output")" != *$'\033'* ]] +} + +@test "wrapper: an approved PR held by the human escalation (noop human-escalated) is reported as held (#1902 review)" { + write_snap "$ROLLUP_PASS" '["needs-human-review"]' \ + '[{"author":{"login":"donpetry-bot"},"createdAt":"2026-10-09T04:44:00Z","body":"\nescalated"}]' \ + '[{"author":{"login":"donpetry-bot"},"state":"APPROVED","commit":{"oid":"'"$SHA"'"},"body":"ok"}]' APPROVED + run timeout 30 bash "$DIAG_SCRIPT" "$PR_URL" + [ "$status" -eq 0 ] + [[ "$output" == *'pr-review would now hold it** at the `human-escalated` gate.'* ]] + [[ "$output" != *'**Approved.**'* ]] +} + +@test "wrapper: --json emits the verdict plus facts as one JSON object" { + write_snap "$ROLLUP_PASS" + run timeout 30 bash "$DIAG_SCRIPT" "$PR_URL" --json + [ "$status" -eq 0 ] + [ "$(jq -r '.decision' <<<"$output")" = "proceed" ] + [ "$(jq -r '.facts.reviewDecision' <<<"$output")" = "REVIEW_REQUIRED" ] + [ "$(jq -r '.facts.holds | length' <<<"$output")" = "0" ] +} + +@test "wrapper: an unreadable PR, or a head that moved since the gate run, is no diagnosis (#1902 review)" { + write_snap "$ROLLUP_FAIL" + # The gate run reads the snapshot; the facts read (the --json view without --jq) fails. + cat > "$TEST_DIR/bin/gh-facts" <<'EOF2' +#!/bin/bash +case "$*" in *"--json state,isDraft"*) exit 1 ;; esac +exec "$REAL_GH_STUB" "$@" +EOF2 + mv "$TEST_DIR/bin/gh" "$TEST_DIR/gh-stub"; mv "$TEST_DIR/bin/gh-facts" "$TEST_DIR/bin/gh" + chmod +x "$TEST_DIR/bin/gh" + REAL_GH_STUB="$TEST_DIR/gh-stub" run timeout 30 bash "$DIAG_SCRIPT" "$PR_URL" + [ "$status" -eq 2 ] + [[ "$output" == *"could not read the PR's review facts"* ]] + [[ "$output" != *"Approvals at head"* ]] + + # The facts read sees a different head than the gate run did. + cat > "$TEST_DIR/bin/gh" <<'EOF2' +#!/bin/bash +case "$*" in *"--json state,isDraft"*) jq '.headRefOid = "ffffffffffffffffffffffffffffffffffffffff"' "$SNAPSHOT"; exit 0 ;; esac +exec "$REAL_GH_STUB" "$@" +EOF2 + REAL_GH_STUB="$TEST_DIR/gh-stub" run timeout 30 bash "$DIAG_SCRIPT" "$PR_URL" + [ "$status" -eq 2 ] + [[ "$output" == *"head moved while diagnosing"* ]] +} + +@test "diagnose mode: gh itself refuses every write, whatever the spelling (#1902 review)" { + # Load the classifier and the diagnose-mode gh wrapper exactly as review-one-pr.sh defines them. + # shellcheck source=/dev/null + source <(sed -n '/^_diagnose_gh_is_write() {/,/^}/p' "$REVIEW_SCRIPT") + local w + for w in "pr comment $PR_URL --body x" "pr edit $PR_URL --add-label needs-human-review" \ + "pr review $PR_URL --approve" "issue comment 5 --body x" "label create x" \ + "api -X POST repos/o/r/issues/1/comments" "api -XPATCH repos/o/r/issues/comments/1" \ + "api --method=DELETE repos/o/r/issues/1/labels/x" "api --method put repos/o/r/x" \ + "api repos/o/r/issues/1/comments -f body=x" "api repos/o/r/dispatches --input -" \ + "api graphql -f query=mutation(\$id:ID!){x}" "issue close 5" "gist create f.txt" \ + "codespace create" "extension-cmd run"; do + # shellcheck disable=SC2086 + _diagnose_gh_is_write $w || { echo "not classified as a write: gh $w"; return 1; } + done + for w in "pr view $PR_URL --json labels" "pr list --state open" "api repos/o/r/pulls/1" \ + "api -X GET repos/o/r/pulls/1/reviews -f per_page=100" "api --paginate repos/o/r/issues/1/events" \ + "api graphql -f query=query{viewer{login}}" "auth status" "search prs is:open"; do + # shellcheck disable=SC2086 + if _diagnose_gh_is_write $w; then echo "read classified as a write: gh $w"; return 1; fi + done + # The wrapper refuses (non-zero, logged) without reaching gh; reads reach it. + # shellcheck source=/dev/null + source <(sed -n '/^ gh() {$/,/^ }$/p' "$REVIEW_SCRIPT") + [ "$(type -t gh)" = function ] + run gh pr comment "$PR_URL" --body x + [ "$status" -ne 0 ] + [[ "$output" == *"diagnose: refused a GitHub write (gh pr comment)"* ]] + [ ! -s "$GH_LOG" ] + run gh pr view "$PR_URL" --json labels --jq '.labels' + [ "$status" -eq 0 ] + grep -q '^pr view' "$GH_LOG" +} + +@test "wrapper: rejects a non-PR URL" { + run bash "$DIAG_SCRIPT" "https://github.com/o/r/issues/1" + [ "$status" -eq 2 ] + [[ "$output" == *"usage:"* ]] +} + +@test "agent profile: every reason in the why-not-approved table is one review-one-pr.sh actually emits" { + local skill="$REPO_ROOT/agents/why-not-approved.md" r + head -10 "$skill" | grep -q '^name: why-not-approved$' + head -10 "$skill" | grep -q '^description: ' + head -10 "$skill" | grep -q '^tools: ' + while IFS= read -r r; do + grep -qE "emit_verdict [a-z-]+ ${r} " "$REPO_ROOT/scripts/review-one-pr.sh" \ + || { echo "reason '$r' in SKILL.md is not emitted by review-one-pr.sh"; return 1; } + done < <(awk -F'|' '/^\| `/ {print $2}' "$skill" | grep -oE '`[a-z-]+`' | tr -d '`' | grep -vxE 'reason|proceed' | sort -u) +}