From 252496736aafce6bc8b831215c334409439e19d4 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Wed, 22 Jul 2026 05:45:32 +0000 Subject: [PATCH] =?UTF-8?q?feat:=20implement=20issue=20#868=20=E2=80=94=20?= =?UTF-8?q?[#850]=20pr-auto-review=20dispatch=20strands=20PRs=20under=20bu?= =?UTF-8?q?lk=20convergence=20=E2=80=94=20add=20a=20catch-up=20sweep=20+?= =?UTF-8?q?=20churn=20tolerance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/scripts/pr-auto-review/README.md | 53 +++++++ .github/scripts/pr-auto-review/lib/sweep.sh | 48 ++++++ .../scripts/pr-auto-review/sweep-dispatch.sh | 143 ++++++++++++++++++ .github/workflows/pr-auto-review-sweep.yml | 70 +++++++++ .github/workflows/pr-auto-review-tests.yml | 4 +- test/workflows/pr-auto-review/sweep.bats | 104 +++++++++++++ 6 files changed, 421 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/pr-auto-review/lib/sweep.sh create mode 100644 .github/scripts/pr-auto-review/sweep-dispatch.sh create mode 100644 .github/workflows/pr-auto-review-sweep.yml create mode 100644 test/workflows/pr-auto-review/sweep.bats diff --git a/.github/scripts/pr-auto-review/README.md b/.github/scripts/pr-auto-review/README.md index affd4d802..106902768 100644 --- a/.github/scripts/pr-auto-review/README.md +++ b/.github/scripts/pr-auto-review/README.md @@ -22,6 +22,18 @@ Pure, side-effect-free helpers. Source the file, then call: | `pr_auto_review_blocking_thread_count` | review-threads JSON on stdin (`reviewThreads(first:100){nodes{isResolved isOutdated}}`) | prints the count of **blocking** threads — unresolved AND not outdated | | `pr_auto_review_ready STATE IS_DRAFT CHECKS_JSON REQUIRED_JSON SELF_NAME REVIEW_DECISION BLOCKING_THREAD_COUNT` | the PR facts the workflow gathers (all as arguments — no stdin) | prints the **decision class** on stdout; `0` ready, `1` not ready | +## `lib/sweep.sh` + +Pure candidate-selection logic for the catch-up sweep (issue #868). + +| Function | Input | Returns | +|----------|-------|---------| +| `pr_auto_review_sweep_candidates MAX` | PR-list JSON on stdin (`gh search prs --json url,isDraft`) | prints ≤`MAX` non-draft PR URLs, one per line, in input order | + +## `sweep-dispatch.sh` + +The catch-up sweep orchestrator (issue #868). See "The catch-up sweep" below. + ### The unified decision core — `pr_auto_review_ready` `pr_auto_review_ready` is the single pure core the reusable workflow calls. It @@ -98,3 +110,44 @@ context `SonarCloud Code Analysis`), while `gh pr checks` renders Actions checks as `" / "` (e.g. `CI / Lint`). A check matches a required context when the names are equal, or when either ends with `" / "`, so both forms resolve to the same required check. + +## The catch-up sweep (issue #868) + +The event-driven ready-check has **no catch-up**. Under a bulk `standards-sync` +convergence (Epic #850 / #857) it fires while CI is still mid-flight +(`"N of M checks not yet passing — skipping"`) and, once everything goes green, +**no further event re-evaluates it** — the PR strands `BLOCKED` with all required +checks green and no code-owner approval, indefinitely. Manually re-running the +ready-check often lands during transient CI re-runs and skips again, so a clean +"all-green + fresh event" window is unreliable while bots are active. + +`sweep-dispatch.sh` is the missing catch-up, run by the +`PR Auto-Review — Catch-up Sweep` workflow on a schedule (every 15 min) and on +`workflow_dispatch`: + +1. Enumerate the open, non-draft PRs carrying the sweep label + (`standards-sync`) org-wide with a single `gh search prs`. +2. Select a **bounded** set via `pr_auto_review_sweep_candidates MAX_PER_RUN` + (back-pressure — see below). +3. For each candidate, gather the same PR facts the event path gathers and + delegate the decision to `pr_auto_review_ready`. Dispatch the review agent + for the ones that come back `dispatched`. + +Because the decision is delegated verbatim, the sweep inherits the #680 +required-vs-non-required tolerance for free: a cancelled/superseded **non-required** +context (a `dev-lead / ci-relay` / `dev-lead / dispatch` run cancelled by per-PR +concurrency) never keeps a ready PR from dispatching. Periodic re-evaluation is +also the **debounce**: a PR skipped during a transient *required* re-run is +re-swept next cycle, so no exact settle window has to be caught. + +### Back-pressure (donpetry-bot capacity) + +donpetry-bot approvals drain at a limited rate (agent/token capacity), so a +10-PR burst that fires every dispatch at once just queues them all behind the +same cap. `MAX_PER_RUN` (default 8) bounds the dispatches per run so a burst +drains over a few cycles instead. `pr_auto_review_sweep_candidates` enforces the +bound and is fail-safe: a non-positive or non-numeric `MAX` selects **nothing**, +so a misconfigured cap can never turn into an unbounded dispatch burst. + +Manual `workflow_dispatch` runs default to **dry-run** (log intended dispatches, +fire nothing); scheduled runs are live. diff --git a/.github/scripts/pr-auto-review/lib/sweep.sh b/.github/scripts/pr-auto-review/lib/sweep.sh new file mode 100644 index 000000000..10c5765a2 --- /dev/null +++ b/.github/scripts/pr-auto-review/lib/sweep.sh @@ -0,0 +1,48 @@ +#!/usr/bin/env bash +# Candidate-selection logic for the pr-auto-review catch-up sweep (issue #868). +# +# Pure, side-effect-free — unit-tested with bats +# (test/workflows/pr-auto-review/sweep.bats). The sweep workflow gathers the +# open standards-sync PR list via `gh search prs` and hands it here to pick the +# bounded set of PRs to (re-)evaluate this cycle. The per-PR readiness decision +# is NOT re-implemented: the orchestrator (sweep-dispatch.sh) feeds each URL +# through pr_auto_review_ready, so the #680 required-vs-non-required tolerance +# is inherited verbatim. +# +# Contract: see .github/scripts/pr-auto-review/README.md +# Pins issue #868. + +# pr_auto_review_sweep_candidates MAX +# Reads a PR-list JSON array on stdin — the response of +# `gh search prs --json url,isDraft` (each element has .url and .isDraft) — +# and prints, one per line, up to MAX candidate PR URLs to evaluate this +# sweep cycle. Draft PRs (`.isDraft == true`) are dropped; the rest are +# emitted in input order, capped at MAX for per-run back-pressure. +# +# MAX the maximum number of PRs to process this run (back-pressure). A +# non-positive or non-numeric MAX emits nothing — the sweep does no +# work rather than fire an unbounded burst of dispatches. +# +# A missing `.isDraft` is treated as non-draft (fail-open on the field, since +# `gh search prs --json isDraft` always populates it; a bare `{url}` from a +# hand-built payload should still be swept). Non-array input (e.g. a +# `{"message": "Not Found"}` error body) or empty stdin emits nothing. +# Always returns 0; the caller decides what an empty candidate set means. +pr_auto_review_sweep_candidates() { + local max="${1:-0}" + + # Back-pressure guard: only a positive integer bounds the run. Anything else + # (0, negative, non-numeric) selects nothing so a misconfigured cap can never + # fire an unbounded dispatch burst. + if ! [[ "$max" =~ ^[0-9]+$ ]] || [ "$max" -le 0 ]; then + return 0 + fi + + jq -r --argjson max "$max" ' + if type == "array" then + [ .[] | select(.isDraft != true) | .url | select(type == "string") ][:$max][] + else + empty + end + ' +} diff --git a/.github/scripts/pr-auto-review/sweep-dispatch.sh b/.github/scripts/pr-auto-review/sweep-dispatch.sh new file mode 100644 index 000000000..2310c0bed --- /dev/null +++ b/.github/scripts/pr-auto-review/sweep-dispatch.sh @@ -0,0 +1,143 @@ +#!/usr/bin/env bash +# sweep-dispatch.sh — catch-up sweep for the pr-auto-review ready-check (#868). +# +# Why this exists (Epic #850 / #857 fleet convergence): the event-driven +# ready-check (pr-auto-review-reusable.yml) has no catch-up. When a bulk +# standards-sync convergence opens many PRs at once, the ready-check fires while +# CI is still mid-flight ("N of M checks not yet passing — skipping") and, once +# everything goes green, no further event re-evaluates it — the PR sits BLOCKED +# with all required checks green and no code-owner approval, indefinitely. +# +# This scheduled/manual sweep enumerates the open standards-sync PRs org-wide +# and, for each one that satisfies the SAME readiness gate as the event path +# (delegated verbatim to pr_auto_review_ready — so the #680 cancelled/superseded +# -non-required tolerance is inherited), re-invokes the dispatch → review-agent +# path. It is the missing catch-up for the missed-event case and removes the +# need for manual `gh run rerun` nudges. Periodic re-evaluation is also the +# debounce: a PR skipped during a transient required re-run is re-swept next +# cycle, so no clean "fresh event + all green" window has to be caught. +# +# Back-pressure (donpetry-bot token/capacity, acceptance criterion): at most +# MAX_PER_RUN PRs are dispatched per run so a 10-PR burst drains at a throttled +# rate over a few cycles instead of firing every dispatch at once. +# +# Idempotent: dispatching an already-approved / already-merged PR is a no-op on +# the review-agent side. Honours DRY_RUN=1 (logs intended dispatches, mutates +# nothing). +# +# Env: +# GH_TOKEN classic PAT with repo scope — API reads + dispatch (required) +# SEARCH_OWNER org to scan for open PRs (default: petry-projects) +# SWEEP_LABEL PR label to sweep (default: standards-sync) +# MAX_PER_RUN max PRs to dispatch per run (default: 8) +# DISPATCH_REPO repository_dispatch target repo (default: petry-projects/.github-private) +# DRY_RUN "1" → log intended dispatches only +set -euo pipefail + +_dir="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=.github/scripts/pr-auto-review/lib/ready-check.sh +. "${_dir}/lib/ready-check.sh" +# shellcheck source=.github/scripts/pr-auto-review/lib/sweep.sh +. "${_dir}/lib/sweep.sh" + +SEARCH_OWNER="${SEARCH_OWNER:-petry-projects}" +SWEEP_LABEL="${SWEEP_LABEL:-standards-sync}" +MAX_PER_RUN="${MAX_PER_RUN:-8}" +DISPATCH_REPO="${DISPATCH_REPO:-petry-projects/.github-private}" +DRY_RUN="${DRY_RUN:-0}" + +# ── Enumerate open, non-draft PRs carrying the sweep label, org-wide ────────── +# `gh search prs` spans every repo the token can see in one call, so the sweep +# runs centrally without an App token or a per-repo installation walk. +# Oldest-first (created asc) so back-pressure drains fairly: each capped run +# takes the oldest waiting PRs, and a merged PR leaves the set so the next-oldest +# advances next cycle — no PR is starved by newer arrivals under best-match order. +PR_LIST=$(gh search prs \ + --owner "$SEARCH_OWNER" \ + --label "$SWEEP_LABEL" \ + --state open \ + --sort created \ + --order asc \ + --limit 100 \ + --json url,isDraft 2>/dev/null || true) +if [ -z "${PR_LIST}" ]; then + PR_LIST="[]" +fi + +# Selection + back-pressure (pure, unit-tested): drops drafts, caps at MAX_PER_RUN. +mapfile -t CANDIDATES < <(printf '%s' "$PR_LIST" | pr_auto_review_sweep_candidates "$MAX_PER_RUN") + +total_open=$(printf '%s' "$PR_LIST" | jq 'if type == "array" then length else 0 end') +[ "$DRY_RUN" = "1" ] && dry_note=" (DRY_RUN)" || dry_note="" +echo "Sweep: ${total_open} open '${SWEEP_LABEL}' PR(s) in ${SEARCH_OWNER}; evaluating ${#CANDIDATES[@]} this run (MAX_PER_RUN=${MAX_PER_RUN})${dry_note}." + +if [ "${#CANDIDATES[@]}" -eq 0 ]; then + echo "No candidate PRs to evaluate — nothing to do." + exit 0 +fi + +# evaluate_pr PR_URL +# Gathers the same PR facts the event-path glue gathers and returns the +# pr_auto_review_ready decision class on stdout; exit 0 iff ready to dispatch. +# Mirrors the "Check PR readiness criteria" step of pr-auto-review-reusable.yml +# (there is no self-check to exclude in a sweep — the sweep is not a PR check). +evaluate_pr() { + local pr_url="$1" repo pr_meta state is_draft pr_number review_decision base_branch + local checks required_json rules_json threads_json blocking_thread_count + repo=$(printf '%s' "$pr_url" | sed 's|https://github.com/||; s|/pull/.*||') + + pr_meta=$(gh pr view "$pr_url" --json state,isDraft,number,reviewDecision,baseRefName) + state=$(printf '%s' "$pr_meta" | jq -r '.state') + is_draft=$(printf '%s' "$pr_meta" | jq -r '.isDraft') + pr_number=$(printf '%s' "$pr_meta" | jq -r '.number') + review_decision=$(printf '%s' "$pr_meta" | jq -r '.reviewDecision // ""') + base_branch=$(printf '%s' "$pr_meta" | jq -r '.baseRefName') + + checks=$(gh pr checks "$pr_url" --json bucket,name 2>/dev/null || true) + if [ -z "${checks}" ]; then checks="[]"; fi + + if rules_json=$(gh api "/repos/${repo}/rules/branches/${base_branch}" 2>/dev/null); then + required_json=$(printf '%s' "$rules_json" | pr_auto_review_required_contexts 2>/dev/null || echo "[]") + else + required_json="[]" + fi + if [ -z "${required_json}" ]; then required_json="[]"; fi + + # shellcheck disable=SC2016 # $owner/$repo/$number are GraphQL variable refs, not shell vars + local gql='query($owner:String!,$repo:String!,$number:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$number){reviewThreads(first:100){nodes{isResolved isOutdated}}}}}' + threads_json=$(gh api graphql \ + -f "query=$gql" \ + -f owner="${repo%%/*}" \ + -f repo="${repo##*/}" \ + -F number="${pr_number}") + blocking_thread_count=$(printf '%s' "$threads_json" | pr_auto_review_blocking_thread_count) + + pr_auto_review_ready \ + "$state" "$is_draft" "$checks" "$required_json" \ + "" "$review_decision" "$blocking_thread_count" +} + +dispatched=0 +for pr_url in "${CANDIDATES[@]}"; do + [ -z "$pr_url" ] && continue + echo "::group::${pr_url}" + if decision=$(evaluate_pr "$pr_url"); then + if [ "$DRY_RUN" = "1" ]; then + echo "[dry-run] would dispatch review agent for ${pr_url} (decision=${decision})" + else + gh api \ + --method POST \ + --header "Accept: application/vnd.github+json" \ + "/repos/${DISPATCH_REPO}/dispatches" \ + --field event_type=pr-review-mention \ + --field "client_payload[pr_url]=${pr_url}" + echo "::notice::Sweep dispatched auto-review for ${pr_url}" + fi + dispatched=$((dispatched + 1)) + else + echo "Not ready (decision=${decision}) — skipping ${pr_url}" + fi + echo "::endgroup::" +done + +echo "Sweep complete — dispatched ${dispatched} of ${#CANDIDATES[@]} evaluated PR(s)." diff --git a/.github/workflows/pr-auto-review-sweep.yml b/.github/workflows/pr-auto-review-sweep.yml new file mode 100644 index 000000000..18a0ac23e --- /dev/null +++ b/.github/workflows/pr-auto-review-sweep.yml @@ -0,0 +1,70 @@ +# PR Auto-Review — Catch-up Sweep (issue #868, surfaced by Epic #850 / #857). +# +# The event-driven ready-check (pr-auto-review-reusable.yml) has no catch-up: +# under a bulk standards-sync convergence it fires while CI is still mid-flight +# and, once everything goes green, no further event re-evaluates the PR — it +# strands BLOCKED with all required checks green and no code-owner approval. +# +# This scheduled/manual sweep enumerates the open `standards-sync` PRs org-wide +# and re-invokes the SAME readiness gate → dispatch path for the ready ones +# (delegated to pr_auto_review_ready, so the #680 cancelled/superseded-non- +# required tolerance is inherited). It is the missing catch-up for the missed- +# event case and removes the need for manual `gh run rerun` nudges. A bounded +# MAX_PER_RUN provides back-pressure so a burst drains over a few cycles rather +# than firing every dispatch at once (donpetry-bot token/capacity). +# +# Runs centrally in petry-projects/.github (the scripts live here); a single +# `gh search prs` call spans every repo the PAT can see. +# +# Requires: GH_PAT_WORKFLOWS org secret (classic PAT, repo scope) for API reads +# and the repository_dispatch to petry-projects/.github-private. +name: PR Auto-Review — Catch-up Sweep + +on: + workflow_dispatch: + inputs: + label: + description: "PR label to sweep" + type: string + default: "standards-sync" + max_per_run: + description: "Max PRs to dispatch this run (back-pressure)" + type: string + default: "8" + dry_run: + description: "Log intended dispatches without firing them" + type: boolean + default: true + schedule: + # Every 15 min — the catch-up interval for a missed-event stall. A PR that + # goes all-required-green with no fresh event is picked up within one cycle. + - cron: "*/15 * * * *" + +permissions: {} + +concurrency: + group: pr-auto-review-sweep + cancel-in-progress: false + +jobs: + sweep: + runs-on: ubuntu-latest + timeout-minutes: 20 + permissions: + contents: read + steps: + - name: Checkout sweep tooling + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + fetch-depth: 1 + persist-credentials: false + + - name: Sweep open standards-sync PRs and dispatch the ready ones + env: + GH_TOKEN: ${{ secrets.GH_PAT_DON_PETRY || secrets.GH_PAT_WORKFLOWS }} + SEARCH_OWNER: ${{ github.repository_owner }} + SWEEP_LABEL: ${{ inputs.label || 'standards-sync' }} + MAX_PER_RUN: ${{ inputs.max_per_run || '8' }} + # Manual runs default to dry-run; scheduled runs are live. + DRY_RUN: ${{ (github.event_name == 'workflow_dispatch' && inputs.dry_run) && '1' || '0' }} + run: bash .github/scripts/pr-auto-review/sweep-dispatch.sh diff --git a/.github/workflows/pr-auto-review-tests.yml b/.github/workflows/pr-auto-review-tests.yml index 325f7e260..f9887367f 100644 --- a/.github/workflows/pr-auto-review-tests.yml +++ b/.github/workflows/pr-auto-review-tests.yml @@ -60,7 +60,9 @@ jobs: run: | set -euo pipefail shellcheck -x \ - .github/scripts/pr-auto-review/lib/ready-check.sh + .github/scripts/pr-auto-review/lib/ready-check.sh \ + .github/scripts/pr-auto-review/lib/sweep.sh \ + .github/scripts/pr-auto-review/sweep-dispatch.sh - name: Run bats suite run: bats --print-output-on-failure test/workflows/pr-auto-review/ diff --git a/test/workflows/pr-auto-review/sweep.bats b/test/workflows/pr-auto-review/sweep.bats new file mode 100644 index 000000000..d14c51da8 --- /dev/null +++ b/test/workflows/pr-auto-review/sweep.bats @@ -0,0 +1,104 @@ +#!/usr/bin/env bats +# Tests for pr_auto_review_sweep_candidates in +# .github/scripts/pr-auto-review/lib/sweep.sh +# +# The catch-up sweep (issue #868) enumerates open standards-sync PRs and +# re-invokes the ready-check → dispatch path for the ones that went green +# without a fresh event. pr_auto_review_sweep_candidates is the pure, +# side-effect-free selection + back-pressure core: given the PR list the sweep +# workflow fetched (`gh search prs --json url,isDraft`), it emits the bounded +# set of candidate PR URLs — drafts dropped, capped at MAX per run so a bulk +# convergence drains at a throttled rate rather than firing all dispatches at +# once (back-pressure, acceptance criterion for donpetry-bot token capacity). +# +# The per-PR readiness decision itself is NOT re-implemented here: the sweep +# reuses pr_auto_review_ready, so the #680 cancelled/superseded-non-required +# tolerance is inherited verbatim (see ready.bats / ready-check.bats). + +load 'helpers/setup' + +setup() { + # shellcheck source=/dev/null + . "${TT_SCRIPTS_DIR}/lib/sweep.sh" +} + +# Three open PRs, the middle one a draft. +LIST='[ + {"url":"https://github.com/petry-projects/repo-a/pull/1","isDraft":false}, + {"url":"https://github.com/petry-projects/repo-b/pull/2","isDraft":true}, + {"url":"https://github.com/petry-projects/repo-c/pull/3","isDraft":false} +]' + +# ── draft exclusion ────────────────────────────────────────────────────────── + +@test "candidates: drafts are excluded, non-drafts kept in input order" { + run pr_auto_review_sweep_candidates 10 <<<"$LIST" + [ "$status" -eq 0 ] + [ "${lines[0]}" = "https://github.com/petry-projects/repo-a/pull/1" ] + [ "${lines[1]}" = "https://github.com/petry-projects/repo-c/pull/3" ] + [ "${#lines[@]}" -eq 2 ] +} + +# ── back-pressure: bounded number per run ──────────────────────────────────── + +@test "candidates: capped at MAX (back-pressure), taking the first N non-drafts" { + run pr_auto_review_sweep_candidates 1 <<<"$LIST" + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 1 ] + [ "${lines[0]}" = "https://github.com/petry-projects/repo-a/pull/1" ] +} + +@test "candidates: MAX larger than the candidate count returns all non-drafts" { + run pr_auto_review_sweep_candidates 99 <<<"$LIST" + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 2 ] +} + +@test "candidates: MAX of 0 (or negative) emits nothing" { + run pr_auto_review_sweep_candidates 0 <<<"$LIST" + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 0 ] + + run pr_auto_review_sweep_candidates -5 <<<"$LIST" + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 0 ] +} + +@test "candidates: non-numeric MAX emits nothing (defensive)" { + run pr_auto_review_sweep_candidates "abc" <<<"$LIST" + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 0 ] +} + +# ── empty / malformed input ────────────────────────────────────────────────── + +@test "candidates: empty PR list emits nothing" { + run pr_auto_review_sweep_candidates 10 <<<'[]' + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 0 ] +} + +@test "candidates: empty stdin emits nothing" { + run pr_auto_review_sweep_candidates 10 < /dev/null + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 0 ] +} + +@test "candidates: non-array API error body emits nothing" { + run pr_auto_review_sweep_candidates 10 <<<'{"message":"Not Found"}' + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 0 ] +} + +@test "candidates: an object missing isDraft is treated as non-draft" { + run pr_auto_review_sweep_candidates 10 <<<'[{"url":"https://github.com/petry-projects/repo-a/pull/7"}]' + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 1 ] + [ "${lines[0]}" = "https://github.com/petry-projects/repo-a/pull/7" ] +} + +@test "candidates: all drafts → nothing" { + run pr_auto_review_sweep_candidates 10 <<<'[{"url":"https://github.com/petry-projects/repo-a/pull/1","isDraft":true}]' + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 0 ] +}