pr-review-loop

3 files shipped with this skill. Inline text is the catalog copy. The upstream link is the source repository.

• dev/skills/pr-review-loop/SKILL.md11 KBe9253a862c24upstream ↗

  ---
  name: pr-review-loop
  description: >-
    Use when a PR against main is open, updated, or failing — the mechanics of
    running its review loop. Covers reading a Copilot review without missing the
    suppressed bucket, the API traps that silently report a clean round, the
    separate-agent substitute when Copilot does not review, and the pointer to
    delegate-routing for three blind checks. Load it when you are actually
    operating the loop; the trigger and the decision rules are always-on in the
    repository operating policy.
  ---
  
  # PR review loop — mechanics
  
  The operating policy owns WHEN to do these things and WHOSE job it is. This
  skill owns HOW, because the how is a pile of API traps that each produce a
  confident wrong answer, and none of it is needed until a PR actually exists.
  
  ## Reading a Copilot review
  
  Copilot writes findings in TWO places and only one creates a thread.
  
  **Bucket 1 — inline comments** become resolvable threads and gate the merge.
  Read them with GraphQL, not the REST comments endpoint:
  
  ```bash
  gh api graphql -f query='
  query { repository(owner:"OWNER", name:"REPO") {
    pullRequest(number:N) { reviewThreads(first:100) {
      nodes { id isResolved path comments(first:1){nodes{author{login} body}} } } } } }' \
    --jq '.data.repository.pullRequest.reviewThreads.nodes[]|select(.isResolved==false)'
  ```
  
  `first:100` is the maximum page size, and it is a CAP, not a guarantee. A PR
  with more threads than that returns a truncated set with no error — which is
  this document's own failure mode, a query that reports a clean round it never
  established. Check `pageInfo.hasNextPage` and page through `endCursor` before
  concluding a PR has no unresolved threads.
  
  REST attributes the same output to two DIFFERENT logins —
  `copilot-pull-request-reviewer[bot]` on `/pulls/N/reviews`, plain `Copilot` on
  `/pulls/N/comments` — so filtering comments by the bot login returns zero while
  gating threads are open. Measured on PR #614: four unresolved threads invisible,
  and since threads block merge it presented as a PR that would not land. GraphQL
  sidesteps it and returns the `isResolved` and thread id you need anyway.
  
  **Bucket 2 — a `<details>` block in the review BODY.** No thread, nothing to
  resolve, invisible to every thread query.
  
  ```bash
  gh api --paginate "repos/OWNER/REPO/pulls/N/reviews" \
    --jq '[.[]|select(.user.login=="copilot-pull-request-reviewer[bot]")]|last|.body'
  ```
  
  **Print the whole body. Never grep it for a heading.** The block's title is not
  stable — `Comments suppressed due to low confidence (N)` and plain
  `Suppressed comments (N)` have both shipped. A phrase grep that returns empty is
  indistinguishable from a clean bucket, which makes the failure silent and
  self-confirming. On PR #568 the suppressed bucket produced 7 genuine findings
  against the gating bucket's 2; the confidence signal was inverted.
  
  `--paginate` is load-bearing: the endpoint pages at 30 and `last` on page one is
  an OLD review that reads exactly like a fresh clean one.
  
  **"generated no new comments" describes bucket 1 only.** It has appeared in the
  same body as a suppressed finding.
  
  ## Watching a PR
  
  Every wait on a PR uses one watcher, `scripts/pr-watch.sh` in this skill's
  directory (in this repository, `dev/skills/pr-review-loop/scripts/pr-watch.sh`).
  Run it in the background (`run_in_background`) for every PR you own or wait on,
  all PRs of a stack in one call, so its exit is the one notification:
  
  ```bash
  scripts/pr-watch.sh [--repo OWNER/REPO] [--interval 30] [--settle 60] [--timeout 21600] PR [PR...]
  ```
  
  It snapshots each PR at start and exits 0 on the first poll that sees a change,
  printing every change from that poll as `#<pr> <event>`: `merged <sha>`,
  `closed`, `reopened`, `ready`, `draft`, `head-moved <old>-><new>`,
  `base <old>-><new>`, `auto-merge armed <method>`, `auto-merge disarmed`,
  `dirty`, `check-failed <name> <link>`, `checks-green`, `rerun-cancelled <run>`,
  `review <login> <STATE> on <sha>: <first line>` and
  `thread-new <path>:<line> <first line>`. Exit 2 means ten polls in a row failed
  to read GitHub, 3 a timeout and 64 a usage error.
  
  The baseline is the state at start, so re-arm after each wake once you have
  acted on it, and stop the watcher when the PR is done. Act on each event:
  
  - **`check-failed`**: read that job's log, fix, push and re-arm. It fires on the
    FIRST failure while slow Darwin legs still run; `gh pr checks --watch` and
    `--fail-fast` wait for pending checks or ignore cancellations.
  - **`dirty`**: rebase. A conflicted PR gets no checks, so it reads like slow CI.
  - **`review`**: read it as below. Copilot's quota and error replies arrive as
    `COMMENTED` reviews, and the first line shows which one this is.
  - **`rerun-cancelled`** appears once the other checks are green, even right
    after re-arming: run `gh run rerun <run>` so the superseded run's required
    contexts clear, then re-arm. Runs older than GitHub's 30-day rerun window are
    never asked for; push or dispatch to get fresh checks instead.
  
  The guards each close a way a hand-written watcher reported the wrong thing:
  
  - **Checks and reviews belong to the head.** It reads checks for the PR's
    current head commit, so the previous commit's finished checks never pass for a
    new push, and a moved head is its own event. A review of an older commit is
    not an event; every new review of the head is, two from one author included.
  - **No checks is not green.** `checks-green` needs at least one check and none
    pending. A stacked PR may have none at all; that is silence, not an event.
  - **Cancelled duplicates.** A cancelled check whose Actions run has a newer run
    of the same workflow on the same head, still running or successful, is not a
    failure. A failed job, even inside that cancelled run, still wakes you.
  - **A failed read is never an event.** An empty, failed or hung `gh` call is
    retried next poll; it cannot read as "head moved" or "checks gone". Each call
    is cut off after 60 s or at `--timeout`, whichever comes first, and killed 2 s
    later if it ignores the signal.
  - **Late checks.** Jobs that wait on other jobs create their checks later, so a
    settled-looking snapshot can cover only the first batch. `checks-green` waits
    until the checks have stayed green for `--settle` seconds. Checks that were
    green at the start report `checks-green` only after a non-green poll.
  
  `UNSTABLE` on the PR is not a failure — it means "not all green yet" and covers
  pending. `DIRTY` is the one that means stop waiting and rebase.
  
  ## Watching a dispatched CI run
  
  A stacked PR (base is another PR's branch) gets no automatic checks, so an empty
  check list there is expected, not a failure. After a substantive change, not a
  pure rebase, dispatch CI and watch that one run with the script below, beside
  `pr-watch.sh`. It is separate because it waits on one Actions run, not on the
  PR: any other check on the commit, such as Copilot's, would satisfy
  `checks-green` before the dispatched run exists.
  
  ```bash
  #!/usr/bin/env bash
  set -euETo pipefail
  shopt -s inherit_errexit 2>/dev/null || :
  repo=OWNER/REPO
  branch=$(git branch --show-current) head=$(git rev-parse HEAD)
  gh() { timeout 60 gh "$@"; } # runs the real gh; it sets no HTTP timeout
  # gh prints the new run's URL: watch that run, never one picked from a list.
  url=$(gh workflow run ci.yml --repo "$repo" --ref "$branch")
  run=${url##*/}
  [[ $run =~ ^[0-9]+$ ]] || { echo "no run URL from gh: $url"; exit 1; }
  sha=$(gh run view "$run" --repo "$repo" --json headSha --jq .headSha)
  [ "$sha" = "$head" ] || { echo "run $run is for $sha, not $head"; exit 1; }
  deadline=$((SECONDS + 3 * 3600))
  while [ "$SECONDS" -lt "$deadline" ]; do
    view=$(gh run view "$run" --repo "$repo" --json status,conclusion,jobs)
    failed=$(jq -r '[.jobs[] | select(.conclusion | IN("failure", "cancelled",
      "timed_out", "action_required", "startup_failure")) | .name] | join(", ")' <<<"$view")
    [ -z "$failed" ] || { echo "run $run failed: $failed"; exit 1; }
    if [ "$(jq -r .status <<<"$view")" = completed ]; then
      [ "$(jq -r .conclusion <<<"$view")" = success ] && exit 0
      echo "run $run ended $(jq -r .conclusion <<<"$view")"
      exit 1
    fi
    sleep 30
  done
  echo "run $run still running after 3 hours"
  exit 1
  ```
  
  It watches the run `gh workflow run` created, checked against the pushed commit,
  so no other run can pass for it. It stops on the first failed job instead of
  waiting for the slowest one, and every `gh` call has a time limit.
  
  ## Deciding whether a review is yours to read
  
  Three states look alike and only one means "reviewed, nothing found".
  
  ```bash
  # did a review run on THIS commit? gate on commit_id, never a timestamp
  gh api --paginate "repos/OWNER/REPO/pulls/N/reviews" \
    --jq '[.[]|select(.user.login=="copilot-pull-request-reviewer[bot]")]|last|.commit_id'
  # did the reviewer run at all on the head SHA?
  gh api --paginate "repos/OWNER/REPO/commits/<head-sha>/check-runs" \
    --jq '.check_runs[]|select(.name=="copilot-pull-request-reviewer")|.status'
  ```
  
  A review of an OLDER commit still advances `submitted_at`, so a timestamp
  baseline reports a stale review as fresh. Absent run = nothing ran. `completed`
  plus an error body = it ran and produced nothing.
  
  **A zero-finding round is clean ONLY if the body says the review ran.** Both
  "Copilot encountered an error and was unable to review" and the quota refusal
  arrive as `state: COMMENTED` with zero threads — every mechanical signal reads
  "reviewed, nothing found".
  
  ## Re-requesting
  
  Only ever after a significant change since the last run (the operating policy
  defines significant). Request the review through REST with the real reviewer
  login:
  
  ```bash
  gh api --method POST "repos/OWNER/REPO/pulls/N/requested_reviewers" \
    -f 'reviewers[]=copilot-pull-request-reviewer[bot]'
  ```
  
  The request is not the confirmation. The POST returns the entire pull request
  object on success, so HTTP 200 proves nothing on its own. Poll for the
  `copilot-pull-request-reviewer` check run on the head SHA. A request issued
  while a review is in flight is silently dropped.
  
  GraphQL has no `requestCopilotReview` mutation. In a session that actually has
  the GitHub MCP server configured, its `request_copilot_review` operation is an
  alternative. The identifier shown by the client is PREFIXED and varies by MCP
  client config, so match on the trailing segment rather than the full name.
  
  ## Separate-agent review
  
  The repository operating policy owns the triggers and scope. For local review,
  follow the delegate-routing skill's **Work and review** workflow, including its
  Subtractive rubric, single-reviewer path and three blind checks. Keep reviewers
  read-only: forbid mutating git commands because sibling worktrees hold live
  edits. Record dismissed findings by title and count, and their dispositions in
  the PR body.
  
  Findings are unbounded, so bound the contest before adding lenses: a 94-line
  docs PR once produced 28 findings and a 59-agent run.
  

• dev/skills/pr-review-loop/scripts/pr-watch.sh11 KB9bdb7ab39977upstream ↗

  #!/usr/bin/env bash
  # cspell:words fromdateiso
  # shellcheck disable=SC2016 # $ in the GraphQL and jq programs is theirs
  # pr-watch.sh — exit on the first change to any watched pull request.
  #
  #   pr-watch.sh [--repo OWNER/REPO] [--interval S] [--settle S] [--timeout S] PR [PR...]
  #
  # Takes a baseline per PR, polls, and prints one "#<pr> <event>" line per
  # change seen in the poll that found any. Exit 0 on events, 2 after ten
  # consecutive failed polls, 3 on timeout, 64 on a usage error.
  set -euETo pipefail
  shopt -s inherit_errexit 2>/dev/null || :
  
  usage() {
    echo "usage: pr-watch.sh [--repo OWNER/REPO] [--interval S] [--settle S] [--timeout S] PR [PR...]" >&2
    exit 64
  }
  
  repo='' interval=${PR_WATCH_INTERVAL:-30} timeout=${PR_WATCH_TIMEOUT:-21600}
  settle=${PR_WATCH_SETTLE:-60} prs=()
  while [ $# -gt 0 ]; do
    case $1 in
    --repo | --interval | --settle | --timeout) [ $# -ge 2 ] || usage ;;&
    --repo) repo=$2 ;;
    --interval) interval=$2 ;;
    --settle) settle=$2 ;;
    --timeout) timeout=$2 ;;
    [0-9]*) prs+=("$1") && shift && continue ;;
    *) usage ;;
    esac
    shift 2
  done
  for n in "${prs[@]}" "$interval" "$settle" "$timeout"; do [[ $n =~ ^[0-9]+$ ]] || usage; done
  [ "${#prs[@]}" -gt 0 ] || usage
  
  # The deadline is in milliseconds: SECONDS ticks on wall-clock seconds, so a
  # short --timeout could expire milliseconds after the start.
  now_ms() {
    local t=${EPOCHREALTIME//[!0-9]/}
    echo $((10#$t / 1000))
  }
  left_ms() { echo $((deadline - $(now_ms))); }
  secs() { printf '%d.%03d' $(($1 / 1000)) $(($1 % 1000)); } # MS -> S.mmm
  deadline=$(($(now_ms) + timeout * 1000))
  
  err_file=$(mktemp)
  trap 'rm -f "$err_file"' EXIT
  
  timed_out() {
    echo "pr-watch: no change on ${prs[*]} after ${timeout}s" >&2
    exit 3
  }
  
  # gh sets no HTTP timeout, so bound every call by 60 s and by the time left: a
  # hung call fails its poll instead of outliving --timeout. timeout(1) without
  # --foreground signals gh's whole process group; -k kills a call that ignores
  # TERM, so a call ends at most 2 s past its limit.
  gh_bounded() {
    local left
    left=$(left_ms) || return 1
    [ "$left" -gt 0 ] || { echo "deadline reached: gh $1 $2" >"$err_file" && return 1; }
    timeout -k 2 "$(secs $((left < 60000 ? left : 60000)))" gh "$@"
  }
  
  if [ -z "$repo" ]; then
    if ! repo=$(gh_bounded repo view --json nameWithOwner --jq .nameWithOwner 2>"$err_file"); then
      [ "$(left_ms)" -gt 0 ] || timed_out
      usage
    fi
  fi
  [[ $repo == */* ]] || usage
  owner=${repo%%/*} name=${repo#*/}
  
  pr_query='query($owner:String!,$name:String!,$pr:Int!){repository(owner:$owner,name:$name){
    pullRequest(number:$pr){state isDraft headRefOid baseRefName mergeStateStatus
      autoMergeRequest{mergeMethod} reviews(last:100){nodes{id author{login} state commit{oid} body}}}}}'
  threads_query='query($owner:String!,$name:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$name){
    pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor}
      nodes{id isResolved path line comments(first:1){nodes{body}}}}}}}'
  checks_query='query($owner:String!,$name:String!,$oid:GitObjectID!,$endCursor:String){repository(owner:$owner,name:$name){
    object(oid:$oid){...on Commit{statusCheckRollup{contexts(first:100,after:$endCursor){pageInfo{hasNextPage endCursor}
      nodes{__typename ...on CheckRun{name status conclusion detailsUrl checkSuite{workflowRun{databaseId}}}
        ...on StatusContext{context state targetUrl}}}}}}}}'
  
  # An empty or failed call is "unknown", never a state: callers retry next poll.
  call() {
    local out
    out=$(gh_bounded "$@" 2>"$err_file") || return 1
    [ -n "$out" ] || { echo "empty response: gh $1 $2" >"$err_file" && return 1; }
    printf '%s\n' "$out"
  }
  
  # snapshot PR -> one normalized JSON state. Runs under `if`, where errexit is
  # off, so every step returns explicitly.
  snapshot() {
    local pr=$1 meta head threads checks runs='{}'
    meta=$(call api graphql -f query="$pr_query" -f owner="$owner" -f name="$name" -F pr="$pr") || return 1
    head=$(jq -er '.data.repository.pullRequest.headRefOid | strings' <<<"$meta") || return 1
    threads=$(call api graphql --paginate -f query="$threads_query" -f owner="$owner" -f name="$name" -F pr="$pr") || return 1
    jq -se 'all(.[]; .data.repository.pullRequest.reviewThreads.nodes | type == "array")' <<<"$threads" >/dev/null || return 1
    checks=$(call api graphql --paginate -f query="$checks_query" -f owner="$owner" -f name="$name" -f oid="$head") || return 1
    jq -se 'all(.[]; .data.repository.object | type == "object")' <<<"$checks" >/dev/null || return 1
    # Actions runs are only needed to classify a failed or cancelled check.
    if jq -se 'any(.[].data.repository.object.statusCheckRollup.contexts.nodes[]?;
        .conclusion != null and (.conclusion | IN("SUCCESS", "NEUTRAL", "SKIPPED") | not))' \
      <<<"$checks" >/dev/null; then
      runs=$(call api --paginate "repos/$repo/actions/runs?head_sha=$head&per_page=100") || return 1
    fi
    jq -n --argjson meta "$meta" --slurpfile threads <(printf '%s' "$threads") \
      --slurpfile checks <(printf '%s' "$checks") --slurpfile runs <(printf '%s' "$runs") '
      def bucket: if .__typename == "StatusContext" then
          ({SUCCESS: "pass", PENDING: "pending", EXPECTED: "pending"}[.state] // "fail")
        elif .status != "COMPLETED" then "pending"
        else ({SUCCESS: "pass", NEUTRAL: "pass", SKIPPED: "pass", CANCELLED: "cancel"}[.conclusion] // "fail") end;
      $meta.data.repository.pullRequest as $p |
      [$runs[].workflow_runs[]? | select(.head_sha == $p.headRefOid)] as $r |
      # A cancelled run with a newer run of the same workflow on this head that is
      # still going or succeeded is a superseded duplicate, not a failure.
      def replacements: (.run as $id | $r | map(select(.id == $id)) | first) as $run |
        if $run.conclusion == "cancelled" then [$r[] | select(.workflow_id == $run.workflow_id
          and .id > $run.id and (.status != "completed" or .conclusion == "success"))] else [] end;
      [$checks[].data.repository.object.statusCheckRollup.contexts.nodes[]? |
        {name: (.name // .context), link: (.detailsUrl // .targetUrl // ""),
         run: .checkSuite.workflowRun.databaseId, bucket: bucket}] as $c |
      # Only a cancelled check can be superseded; a failed job stays a failure even
      # when its run was later cancelled and replaced.
      [$c[] | select(.bucket == "fail" or .bucket == "cancel") |
        . + {repl: (if .bucket == "cancel" then replacements else [] end)}] as $bad |
      {head: $p.headRefOid, state: $p.state, draft: $p.isDraft, base: $p.baseRefName,
       automerge: ($p.autoMergeRequest.mergeMethod // "none"), dirty: ($p.mergeStateStatus == "DIRTY"),
       failed: [$bad[] | select(.repl == []) | {name, link}],
       # Green needs a check, and every non-pass check superseded by a success.
       green: (($c | length) > 0 and all($c[]; .bucket != "pending")
         and all($bad[]; any(.repl[]; .conclusion == "success"))),
       # GitHub refuses to rerun a run older than 30 days, so only ask for newer ones.
       rerun: [$bad[] | select(.repl != []) | .run | select(. as $id | $r[] | select(.id == $id) |
         (.created_at // "") | . == "" or fromdateiso8601 > now - 30 * 86400)] | unique,
       # Each review of the head, oldest first. A review of an older commit is not
       # news about this head. The query reads the newest 100, so only more than
       # 100 new reviews between two polls could hide one.
       reviews: [$p.reviews.nodes[] | select(.commit.oid == $p.headRefOid) |
         {id, login: (.author.login // "ghost"), state, commit: .commit.oid, body: (.body // "")}],
       threads: [$threads[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved | not) |
         {id, path, line, body: (.comments.nodes[0].body // "")}]}'
  }
  
  # events PR BASELINE CURRENT SETTLED WAS_GREEN -> "#<pr> <event>" lines.
  events() {
    jq -nr --arg pr "$1" --argjson a "$2" --argjson b "$3" --argjson settled "$4" --argjson was_green "$5" '
      def short: .[0:8];
      def first_line: [splits("\r?\n") | select(test("\\S"))] | (first // "(no body)") | .[0:160];
      (if $a.state != $b.state then
         {MERGED: "merged \($b.head | short)", CLOSED: "closed", OPEN: "reopened"}[$b.state] else empty end),
      (if $a.draft != $b.draft then (if $b.draft then "draft" else "ready" end) else empty end),
      (if $a.head != $b.head then "head-moved \($a.head | short)->\($b.head | short)" else empty end),
      (if $a.base != $b.base then "base \($a.base)->\($b.base)" else empty end),
      (if $a.automerge != $b.automerge then (if $b.automerge == "none" then "auto-merge disarmed"
         else "auto-merge armed \($b.automerge | ascii_downcase)" end) else empty end),
      (if $b.dirty and ($a.dirty | not) then "dirty" else empty end),
      ($b.failed[] | select(. as $f | any($a.failed[]; . == $f) | not) | "check-failed \(.name) \(.link)"),
      (if $b.green and $settled and ($was_green | not) and $a.head == $b.head then
         "checks-green" else empty end),
      # A superseded cancelled run needs a rerun whatever the baseline was, so a
      # re-armed watcher still asks for it.
      (if $b.green and $settled then ($b.rerun[] | "rerun-cancelled \(.)") else empty end),
      ($b.reviews[] | select(.id as $id | any($a.reviews[]; .id == $id) | not) |
         "review \(.login) \(.state) on \(.commit | short): \(.body | first_line)"),
      ($b.threads[] | select(.id as $id | any($a.threads[]; .id == $id) | not) |
         "thread-new \(.path):\(.line // "?") \(.body | first_line)")
      | "#\($pr) \(.)"'
  }
  
  # was_green: the checks have been green at every poll since the baseline, so a
  # green now is not news. Any non-green poll clears it, so green -> pending ->
  # green reports again.
  declare -A baseline green_since was_green
  fails=0
  while :; do
    report='' failed=''
    for pr in "${prs[@]}"; do
      if ! snap=$(snapshot "$pr"); then
        failed=1
        continue
      fi
      if [ -z "${baseline[$pr]:-}" ]; then
        baseline[$pr]=$snap
        was_green[$pr]=$(jq .green <<<"$snap")
        continue
      fi
      # Jobs that wait on other jobs add their checks later, so green must hold
      # for $settle seconds before it counts.
      if jq -e .green <<<"$snap" >/dev/null; then
        green_since[$pr]=${green_since[$pr]:-$SECONDS}
      else
        unset 'green_since[$pr]'
        was_green[$pr]=false
      fi
      settled=false
      if [ -n "${green_since[$pr]:-}" ] && [ $((SECONDS - green_since[$pr])) -ge "$settle" ]; then
        settled=true
      fi
      # A moved head's checks are not settled, whatever the old head's were.
      if [ "$(jq -r .head <<<"$snap")" != "$(jq -r .head <<<"${baseline[$pr]}")" ]; then
        settled=false
      fi
      out=$(events "$pr" "${baseline[$pr]}" "$snap" "$settled" "${was_green[$pr]}")
      [ -z "$out" ] || report+=$out$'\n'
    done
    if [ -n "$report" ]; then
      printf '%s' "$report"
      exit 0
    fi
    if [ -n "$failed" ]; then
      fails=$((fails + 1))
      if [ "$fails" -ge 10 ]; then
        echo "pr-watch: $fails consecutive polls failed; last error: $(head -c 300 "$err_file")" >&2
        exit 2
      fi
    else
      fails=0
    fi
    left=$(left_ms)
    [ "$left" -gt 0 ] || timed_out
    sleep "$(secs $((interval * 1000 < left ? interval * 1000 : left)))"
  done
  

• README.mdupstream ↗

  Contents are not inlined in the catalog. Open the upstream file.

Plain-text copy, no login: /skill-bundles/higherorderfunctor-nix-agentic-tools-dev-skills-pr-review-loop.md