Repository navigation
feat(#1894): answer "why isn't this PR approved?" from the real pr-review gate chain (+ why-not-approved agent) #1902
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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/<owner>/<repo>/pull/<n> | ||
| ``` | ||
|
|
||
| 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <pr-url> [--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/<owner>/<repo>/pull/<n> [--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 | ||
|
don-petry marked this conversation as resolved.
|
||
| | {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) + " |", | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| "| 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), | ||
| "", | ||
| "<details><summary>Gate log</summary>", | ||
| "", | ||
| "```", | ||
| .gate_log, | ||
| "```", | ||
| "</details>" | ||
| ] | join("\n")' <<<"$report" | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.