feat: implement issue #868 — [#850] pr-auto-review dispatch strands PRs under bulk convergence — add a catch-up sweep + churn tolerance #869
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| ' | ||
| } | ||
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,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 \ | ||
|
There was a problem hiding this comment. 1. Silent search failure no-ops The sweep suppresses gh search prs errors and ignores its exit status, then treats empty output as an empty PR set, so auth/rate-limit/network failures can make the workflow succeed while dispatching nothing (leaving PRs stranded). This undermines the sweep’s purpose because failures become indistinguishable from “no open PRs.” Agent Prompt
Contributor
Author
There was a problem hiding this comment. Fixed in |
||
| --owner "$SEARCH_OWNER" \ | ||
| --label "$SWEEP_LABEL" \ | ||
| --state open \ | ||
| --sort created \ | ||
| --order asc \ | ||
| --limit 100 \ | ||
| --json url,isDraft 2>/dev/null || true) | ||
|
Comment on lines
+55
to
+62
There was a problem hiding this comment. Suggestion: The search is hard-limited to 100 PRs with no pagination, so labeled open PRs beyond the first page are never considered by the sweep and can remain stranded indefinitely. Add pagination (or raise and iterate limits) so the sweep can eventually inspect the full open set. [incomplete implementation] Severity Level: Major
|
||
| 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') | ||
|
There was a problem hiding this comment. 2. Jq parse can abort sweep total_open is computed by piping PR_LIST into jq without guarding parse errors, so unexpected non-JSON/truncated stdout can terminate the sweep under set -e. This can prevent any candidates from being evaluated/processed in that run. Agent Prompt
Contributor
Author
There was a problem hiding this comment. Fixed in |
||
| [ "$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 \ | ||
|
There was a problem hiding this comment. 3. Dispatch failure stops batch A single failing gh api ... /dispatches call will terminate the entire sweep run due to set -e, skipping remaining ready PRs in the capped batch. This reduces sweep throughput and can keep PRs stuck until a later cycle even when they were ready now. Agent Prompt
Contributor
Author
There was a problem hiding this comment. Fixed in |
||
| --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)." | ||
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,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 |
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The candidate cap is applied before readiness is checked, so older PRs that stay non-ready can repeatedly consume all slots and prevent newer ready PRs from ever being evaluated. Move the cap to the dispatch stage (cap successful dispatches), or evaluate a larger window and stop once MAX dispatches are reached. [incorrect condition logic]
Severity Level: Major⚠️
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in
.github/scripts/pr-auto-review/lib/sweep.shandsweep-dispatch.sh: removed the[:]slice frompr_auto_review_sweep_candidates(the function now emits all non-draft PRs in input order). The back-pressure cap moves to the dispatch loop insweep-dispatch.shwith[ "$dispatched" -ge "$MAX_PER_RUN" ] && break, so non-ready older PRs can no longer consume all candidate slots and starve newer ready ones. Tests intest/workflows/pr-auto-review/sweep.batsupdated to reflect the new contract.