diff --git a/.github/scripts/pr-auto-review/README.md b/.github/scripts/pr-auto-review/README.md index affd4d80..d5753916 100644 --- a/.github/scripts/pr-auto-review/README.md +++ b/.github/scripts/pr-auto-review/README.md @@ -91,6 +91,56 @@ defense-in-depth: the producer side (dev-lead resolving the threads it fixes) is still the preferred fix; this gate just stops forgotten resolutions from stalling otherwise-mergeable PRs. +## The catch-up sweep — `sweep.sh` + `lib/sweep.sh` + +The event-driven reusable workflow reviews a PR when its triggering event +(CI green, review submitted, …) fires. A dropped or missed event — or a PR that +went green while no event was in flight — can leave a mergeable PR un-reviewed. +`sweep.sh` is a periodically-run **catch-up sweep**: it re-scans open PRs and +dispatches the review agent for the ones the event path missed. + +`sweep.sh` is thin gh I/O glue; every decision lives in the pure, unit-tested +cores it sources — `lib/ready-check.sh` (per-PR readiness) and `lib/sweep.sh` +(candidate selection, `test/workflows/pr-auto-review/sweep.bats`). + +### `lib/sweep.sh` + +Pure, side-effect-free helpers. Source the file, then call: + +| Function | Input | Returns | +|----------|-------|---------| +| `pr_auto_review_sweep_valid_search` | a search payload on stdin | `0` if it is a valid JSON array (incl. empty `[]`); non-zero + stderr otherwise | +| `pr_auto_review_sweep_extract` | a search payload (JSON array of PR objects) on stdin | prints a compact `[{number,updatedAt}]`; **non-zero** (not an abort) on a malformed / non-array payload | +| `pr_auto_review_sweep_page_full COUNT PER_PAGE` | — | `0` if `COUNT >= PER_PAGE` (more pages may exist), `1` otherwise | +| `pr_auto_review_sweep_merge_pages` | one-or-more candidate arrays concatenated on stdin | prints one array, deduped by `.number` | +| `pr_auto_review_sweep_order` | a candidate array on stdin | prints it sorted oldest-first by `updatedAt`, ties by `number` asc | +| `pr_auto_review_sweep_plan MAX_PER_RUN` | an ordered `[{number,ready}]` array on stdin | prints the PR numbers to dispatch — ready-only, capped at **MAX dispatched** | + +### Robustness properties (issue #872) + +The sweep is hardened against four failure modes; each maps to a pure helper so +the behaviour is unit-tested without a live gh: + +1. **Cap on the number _dispatched_, not considered.** Readiness is evaluated for + every candidate *before* the per-run cap is applied + (`pr_auto_review_sweep_plan` counts only ready PRs), so a run of older + non-ready PRs can never consume all slots and starve newer ready ones. +2. **Full pagination.** The labeled-PR search is paged while + `pr_auto_review_sweep_page_full` reports a full page; pages are combined with + `pr_auto_review_sweep_merge_pages`, so PRs beyond page 1 are swept rather than + dropped at the 100-item cliff. +3. **A failed search surfaces, never no-ops.** A non-zero gh exit *or* a payload + `pr_auto_review_sweep_valid_search` rejects (empty string, error object, + malformed JSON) aborts the run with an error, instead of being read as an + empty candidate set / "nothing to do". +4. **A malformed payload is guarded.** `pr_auto_review_sweep_extract` catches a + jq parse failure and returns non-zero rather than letting an unguarded jq + error abort the whole sweep under `set -euo pipefail`. + +Plus deterministic **oldest-first ordering** (`pr_auto_review_sweep_order`): when +more PRs are ready than one run's cap allows, the longest-waiting ready PR drains +first, so no ready PR is perpetually starved across runs. + ### Context name matching Rulesets store the bare context (e.g. a job name `Lint`, or a third-party status 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 00000000..8c7f5c1b --- /dev/null +++ b/.github/scripts/pr-auto-review/lib/sweep.sh @@ -0,0 +1,132 @@ +#!/usr/bin/env bash +# Pure decision core for the pr-auto-review catch-up sweep. +# +# The event-driven pr-auto-review reusable workflow dispatches a review agent for +# a single PR when its triggering event (CI green, review submitted, …) fires. A +# missed or dropped event, or a PR that went green while no event was in flight, +# can leave a mergeable PR un-reviewed. The catch-up sweep periodically re-scans +# open PRs and dispatches the review agent for the ones the event path missed. +# +# These functions are pure and side-effect-free (like lib/ready-check.sh) so the +# sweep's robustness properties (issue #872) are unit-testable without a live gh: +# +# * pr_auto_review_sweep_plan — cap on the number DISPATCHED (ready), not +# candidates considered, so a run of older +# non-ready PRs cannot starve newer ready +# ones. (#872 finding 1) +# * pr_auto_review_sweep_order — deterministic oldest-first ordering so the +# longest-waiting ready PR drains first and +# is not perpetually starved. (#872 ordering) +# * pr_auto_review_sweep_page_full — pagination predicate: a full page means +# more may exist. (#872 finding 2) +# * pr_auto_review_sweep_merge_pages — merge + dedupe accumulated pages so PRs +# beyond page 1 are swept. (#872 finding 2) +# * pr_auto_review_sweep_valid_search — a failed search is surfaced, not +# treated as an empty result. (#872 finding 3) +# * pr_auto_review_sweep_extract — guarded parse: a malformed payload returns +# non-zero instead of aborting the run under +# set -e. (#872 finding 4) +# +# Contract: see .github/scripts/pr-auto-review/README.md. +# The orchestrator that supplies the gh I/O is sweep.sh alongside this file. + +# pr_auto_review_sweep_valid_search +# Reads a search payload on stdin and validates it is a well-formed JSON array +# (the shape `gh search prs --json …` / a REST search `.items` returns). Exit 0 +# when valid — INCLUDING a legitimately empty `[]`. Exit 1 with a message on +# stderr otherwise: an empty string, malformed JSON, or a non-array such as an +# API error object (`{"message":"Bad credentials"}`). +# +# Why (#872 finding 3): an unchecked `gh search` failure yields an empty set, +# which the sweep would read as "nothing to do" and silently skip every PR. The +# caller pairs this with gh's own exit status so a transport/API failure is +# surfaced as an error instead of a no-op. +pr_auto_review_sweep_valid_search() { + local payload + payload=$(cat) + if [ -z "$payload" ]; then + echo "sweep: empty search payload (search likely failed)" >&2 + return 1 + fi + if ! printf '%s' "$payload" | jq -e 'type == "array"' >/dev/null 2>&1; then + echo "sweep: search payload is not a JSON array (search likely failed)" >&2 + return 1 + fi +} + +# pr_auto_review_sweep_extract +# Reads a raw search payload on stdin and emits a compact, normalized candidate +# array `[{ "number": N, "updatedAt": "…" }]` on stdout. Returns 0 on success. +# +# Guarded (#872 finding 4): a malformed or non-array payload makes jq fail; the +# failure is caught and turned into a non-zero RETURN with an empty stdout, +# rather than propagating as an uncaught jq error that would abort the whole +# sweep under `set -euo pipefail`. The caller checks the return value and can +# surface the bad payload without losing the candidates it already parsed. +pr_auto_review_sweep_extract() { + local payload out + payload=$(cat) + if ! out=$(printf '%s' "$payload" | jq -c ' + if type == "array" then + [ .[] | { number: .number, updatedAt: (.updatedAt // .updated_at) } ] + else + error("not an array") + end + ' 2>/dev/null); then + echo "sweep: could not parse search payload — skipping it" >&2 + return 1 + fi + printf '%s\n' "$out" +} + +# pr_auto_review_sweep_page_full COUNT PER_PAGE +# Pagination predicate. Exit 0 when the page was full (COUNT >= PER_PAGE), so +# another page may exist and should be fetched; exit 1 when the page was short +# or empty (the last page). +# +# Why (#872 finding 2): the labeled-PR search was hard-limited to one page of +# 100, so PRs beyond page 1 were never swept. The orchestrator loops fetching +# pages while this predicate is true. +pr_auto_review_sweep_page_full() { + local count="${1:-0}" per_page="${2:-100}" + [ "$count" -ge "$per_page" ] +} + +# pr_auto_review_sweep_merge_pages +# Reads one-or-more candidate JSON arrays concatenated on stdin (one per page) +# and emits a single compact array on stdout, deduped by `.number` (a PR can +# appear on two pages if the underlying set shifts between fetches). (#872 +# finding 2) +pr_auto_review_sweep_merge_pages() { + jq -sc 'add // [] | unique_by(.number)' +} + +# pr_auto_review_sweep_order +# Reads a candidate JSON array on stdin and emits it sorted oldest-first by +# `updatedAt`, ties broken by `.number` ascending, on stdout. +# +# Why (#872 ordering): when more PRs are ready than a single run's dispatch cap +# allows, draining the longest-waiting (oldest) ready PR first guarantees no +# ready PR is perpetually starved across successive runs (FIFO fairness). +pr_auto_review_sweep_order() { + jq -c 'sort_by(.updatedAt, .number)' +} + +# pr_auto_review_sweep_plan MAX_PER_RUN +# Reads an ORDERED candidate JSON array on stdin, each element +# `{ "number": N, "ready": true|false }`, and prints the PR numbers to dispatch +# — one per line — walking candidates in order and emitting a ready one until +# MAX_PER_RUN of them have been emitted. Non-ready candidates are skipped and +# consume NO slot. Exit 0. +# +# Why (#872 finding 1): the cap must apply to the number DISPATCHED (ready), not +# to candidates considered. Capping on candidates lets a leading run of older +# non-ready PRs consume every slot and starve the ready ones behind them; gating +# on dispatched count means readiness is checked first and only ready PRs count +# against the cap. +pr_auto_review_sweep_plan() { + local max="${1:-0}" + jq -r --argjson max "$max" ' + [ .[] | select(.ready == true) | .number ] | .[0:$max] | .[] + ' +} diff --git a/.github/scripts/pr-auto-review/sweep.sh b/.github/scripts/pr-auto-review/sweep.sh new file mode 100755 index 00000000..184388ec --- /dev/null +++ b/.github/scripts/pr-auto-review/sweep.sh @@ -0,0 +1,204 @@ +#!/usr/bin/env bash +# pr-auto-review catch-up sweep — orchestrator. +# +# The event-driven pr-auto-review reusable workflow reviews a PR when its +# triggering event fires. A dropped/missed event (or a PR that went green with no +# event in flight) can leave a mergeable PR un-reviewed. This sweep periodically +# re-scans open PRs and dispatches the review agent for the ones the event path +# missed. +# +# This is the thin gh I/O glue; every decision lives in the pure, unit-tested +# cores it sources — lib/ready-check.sh (per-PR readiness) and lib/sweep.sh +# (candidate selection). It implements the four robustness properties of #872: +# +# 1. cap on the number DISPATCHED (ready), not candidates considered +# → readiness is evaluated first, then pr_auto_review_sweep_plan caps on +# the ready ones, so a run of older non-ready PRs cannot starve newer +# ready ones. +# 2. full pagination of the labeled-PR search +# → the /search/issues call is paged while pr_auto_review_sweep_page_full +# reports a full page; pages are merged with pr_auto_review_sweep_merge_pages. +# 3. a failed search surfaces an error, is not treated as "nothing to do" +# → a non-zero gh exit OR a payload pr_auto_review_sweep_valid_search rejects +# aborts the run instead of yielding an empty candidate set. +# 4. a malformed payload is guarded, does not abort mid-jq +# → pr_auto_review_sweep_extract catches a jq parse failure and returns +# non-zero, which this script surfaces explicitly. +# +# Configuration (all via environment): +# REPO owner/repo to sweep (default: $GITHUB_REPOSITORY) +# SWEEP_LABEL only sweep PRs carrying this label (default: auto-review; +# set empty to sweep every open PR) +# MAX_PER_RUN max review agents to DISPATCH per run (default: 10) +# SWEEP_PER_PAGE search page size, max 100 (default: 100) +# SWEEP_MAX_PAGES page ceiling — REST search caps at 1000 results (default: 10) +# DISPATCH_REPO repo receiving the repository_dispatch (default: +# petry-projects/.github-private) +# DISPATCH_EVENT repository_dispatch event_type (default: pr-review-mention) +# DRY_RUN when "true", log the plan but do not dispatch (default: false) +# +# Requires: GH_TOKEN with repo scope (for search, pr view/checks, GraphQL, and +# the dispatch). +set -euo pipefail + +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=/dev/null +. "${SCRIPT_DIR}/lib/ready-check.sh" +# shellcheck source=/dev/null +. "${SCRIPT_DIR}/lib/sweep.sh" + +REPO="${REPO:-${GITHUB_REPOSITORY:-}}" +SWEEP_LABEL="${SWEEP_LABEL-auto-review}" +MAX_PER_RUN="${MAX_PER_RUN:-10}" +SWEEP_PER_PAGE="${SWEEP_PER_PAGE:-100}" +SWEEP_MAX_PAGES="${SWEEP_MAX_PAGES:-10}" +DISPATCH_REPO="${DISPATCH_REPO:-petry-projects/.github-private}" +DISPATCH_EVENT="${DISPATCH_EVENT:-pr-review-mention}" +DRY_RUN="${DRY_RUN:-false}" + +if [ -z "$REPO" ]; then + echo "::error::sweep: REPO (or GITHUB_REPOSITORY) is required" >&2 + exit 2 +fi + +# ── Gather one PR's readiness facts via gh (fail-closed) ────────────────────── +# Mirrors the reusable workflow's fact-gathering and prints them as one JSON +# object. Called as a plain top-level assignment in the loop below, so under +# `set -e` a gh/network failure aborts the whole sweep — the sweep never scores a +# PR ready on incomplete data. The pure readiness decision is made by the caller. +sweep_pr_facts() { + local num="$1" pr_meta state is_draft review_decision base_branch + local checks required_json rules_json threads_json blocking gql + + pr_meta=$(gh pr view "$num" --repo "$REPO" \ + --json state,isDraft,number,reviewDecision,baseRefName) + state=$(printf '%s' "$pr_meta" | jq -r '.state') + is_draft=$(printf '%s' "$pr_meta" | jq -r '.isDraft') + review_decision=$(printf '%s' "$pr_meta" | jq -r '.reviewDecision // ""') + base_branch=$(printf '%s' "$pr_meta" | jq -r '.baseRefName') + + # gh pr checks exits non-zero when checks are failing/pending but still writes + # the JSON payload; || true keeps that output under set -e. + checks=$(gh pr checks "$num" --repo "$REPO" --json bucket,name 2>/dev/null || true) + [ -z "$checks" ] && checks="[]" + + 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 + [ -z "$required_json" ] && required_json="[]" + + # shellcheck disable=SC2016 # $owner/$repo/$number are GraphQL variable refs + 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="$num") + blocking=$(printf '%s' "$threads_json" | pr_auto_review_blocking_thread_count) + + jq -cn \ + --arg state "$state" \ + --arg isDraft "$is_draft" \ + --argjson checks "$checks" \ + --argjson required "$required_json" \ + --arg reviewDecision "$review_decision" \ + --arg blocking "$blocking" \ + '{state:$state, isDraft:$isDraft, checks:$checks, required:$required, reviewDecision:$reviewDecision, blocking:$blocking}' +} + +# ── 1. Paginated, surfaced, guarded candidate search (#872 findings 2,3,4) ──── +query="repo:${REPO} is:pr is:open" +[ -n "$SWEEP_LABEL" ] && query="${query} label:\"${SWEEP_LABEL}\"" + +pages_file="$(mktemp)" +trap 'rm -f "$pages_file"' EXIT + +page=1 +while [ "$page" -le "$SWEEP_MAX_PAGES" ]; do + # Fail-closed on the transport: a non-zero gh exit is surfaced, never treated + # as an empty result set (#872 finding 3). + if ! response=$(gh api -X GET /search/issues \ + --raw-field q="$query" \ + -F per_page="$SWEEP_PER_PAGE" \ + -F page="$page" 2>/dev/null); then + echo "::error::sweep: search API call failed on page ${page} — aborting rather than treating as 'nothing to do'" >&2 + exit 1 + fi + + # Unwrap .items defensively; a malformed response yields "" and is rejected below. + items=$(printf '%s' "$response" | jq -c '.items' 2>/dev/null || true) + + # Distinguish a valid (possibly empty) result from a failed/garbage one (#872 finding 3). + if ! printf '%s' "$items" | pr_auto_review_sweep_valid_search; then + echo "::error::sweep: search returned an invalid payload on page ${page} — aborting" >&2 + exit 1 + fi + + # Guarded parse: a jq failure here returns non-zero instead of aborting mid-pipe (#872 finding 4). + if ! page_candidates=$(printf '%s' "$items" | pr_auto_review_sweep_extract); then + echo "::error::sweep: could not parse search results on page ${page} — aborting" >&2 + exit 1 + fi + printf '%s\n' "$page_candidates" >> "$pages_file" + + page_count=$(printf '%s' "$items" | jq 'length') + # Stop when the page was short; keep paging while it was full (#872 finding 2). + pr_auto_review_sweep_page_full "$page_count" "$SWEEP_PER_PAGE" || break + page=$((page + 1)) +done + +# Merge pages (dedupe) and order oldest-first so the longest-waiting ready PR +# drains first and is never perpetually starved (#872 ordering). +candidates=$(pr_auto_review_sweep_merge_pages < "$pages_file" | pr_auto_review_sweep_order) +candidate_count=$(printf '%s' "$candidates" | jq 'length') +echo "sweep: ${candidate_count} open candidate PR(s) in ${REPO}${SWEEP_LABEL:+ with label \"$SWEEP_LABEL\"}" + +# ── 2. Evaluate readiness, THEN cap on dispatched (#872 finding 1) ──────────── +# Build a {number,ready} verdict list in candidate order. Readiness is checked +# for every candidate BEFORE the cap is applied, so non-ready PRs consume no +# dispatch slot and cannot starve ready ones. +verdicts='[]' +while IFS= read -r num; do + [ -z "$num" ] && continue + # Plain assignment: a gh failure inside aborts the sweep under set -e (fail-closed). + facts=$(sweep_pr_facts "$num") + # Field extraction is pure jq (no gh), so these assignments cannot mask an error. + f_state=$(printf '%s' "$facts" | jq -r '.state') + f_draft=$(printf '%s' "$facts" | jq -r '.isDraft') + f_checks=$(printf '%s' "$facts" | jq -c '.checks') + f_required=$(printf '%s' "$facts" | jq -c '.required') + f_review=$(printf '%s' "$facts" | jq -r '.reviewDecision') + f_blocking=$(printf '%s' "$facts" | jq -r '.blocking') + # Pure decision — safe to test in `if` (SELF_CHECK empty: the sweep is not a + # check run on the PR). + if decision=$(pr_auto_review_ready "$f_state" "$f_draft" "$f_checks" \ + "$f_required" "" "$f_review" "$f_blocking"); then + ready=true + else + ready=false + fi + echo "sweep: PR #${num} → ${decision}" + verdicts=$(printf '%s' "$verdicts" | jq -c --argjson n "$num" --argjson r "$ready" '. + [{number:$n, ready:$r}]') +done < <(printf '%s' "$candidates" | jq -r '.[].number') + +mapfile -t to_dispatch < <(printf '%s' "$verdicts" | pr_auto_review_sweep_plan "$MAX_PER_RUN") +echo "sweep: dispatching ${#to_dispatch[@]} of up to ${MAX_PER_RUN} slot(s)" + +# ── 3. Dispatch the review agent for the planned PRs ────────────────────────── +for pr in "${to_dispatch[@]}"; do + [ -z "$pr" ] && continue + pr_url="https://github.com/${REPO}/pull/${pr}" + if [ "$DRY_RUN" = "true" ]; then + echo "::notice::[dry-run] would dispatch auto-review for ${pr_url}" + continue + fi + gh api \ + --method POST \ + --header "Accept: application/vnd.github+json" \ + "/repos/${DISPATCH_REPO}/dispatches" \ + --field event_type="$DISPATCH_EVENT" \ + --field "client_payload[pr_url]=${pr_url}" + echo "::notice::sweep dispatched auto-review for ${pr_url}" +done diff --git a/.github/workflows/pr-auto-review-tests.yml b/.github/workflows/pr-auto-review-tests.yml index 325f7e26..2c394d74 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.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 00000000..e2c74690 --- /dev/null +++ b/test/workflows/pr-auto-review/sweep.bats @@ -0,0 +1,198 @@ +#!/usr/bin/env bats +# Tests for the pure catch-up-sweep decision core in +# .github/scripts/pr-auto-review/lib/sweep.sh +# +# The catch-up sweep re-evaluates open PRs the event-driven pr-auto-review path +# may have missed and dispatches the review agent for the ready ones. These +# functions are the pure, side-effect-free core (mirroring lib/ready-check.sh), +# so the four robustness properties from issue #872 are unit-testable without a +# live gh: +# +# 1. cap on the number DISPATCHED (ready), not candidates considered +# → pr_auto_review_sweep_plan (anti-starvation) +# 2. full pagination of the labeled-PR search +# → pr_auto_review_sweep_page_full / pr_auto_review_sweep_merge_pages +# 3. a failed search surfaces an error, is not treated as "nothing to do" +# → pr_auto_review_sweep_valid_search +# 4. a malformed payload is guarded and does not abort the whole run +# → pr_auto_review_sweep_extract +# +# plus deterministic oldest-first ordering so ready PRs are not starved +# → pr_auto_review_sweep_order + +load 'helpers/setup' + +setup() { + # shellcheck source=/dev/null + . "${TT_SCRIPTS_DIR}/lib/sweep.sh" + # Export so `bash -c` subshells used to drive stdin/pipes see the functions. + export -f pr_auto_review_sweep_valid_search pr_auto_review_sweep_extract \ + pr_auto_review_sweep_page_full pr_auto_review_sweep_merge_pages \ + pr_auto_review_sweep_order pr_auto_review_sweep_plan +} + +# ── #872 finding 1: cap on DISPATCHED, not candidates considered ────────────── + +# The starvation bug: MAX_PER_RUN applied to candidates BEFORE readiness means a +# run of older non-ready PRs consumes every slot and starves newer ready ones. +# The fix caps on the count dispatched, so non-ready candidates consume no slot. +@test "plan: older non-ready candidates do not consume dispatch slots (anti-starvation)" { + input='[{"number":10,"ready":false},{"number":11,"ready":false},{"number":20,"ready":true},{"number":21,"ready":true}]' + run bash -c 'printf "%s" '"'$input'"' | pr_auto_review_sweep_plan 2' + [ "$status" -eq 0 ] + # Both ready PRs dispatch even though 2 non-ready ones preceded them. + [ "${lines[0]}" = "20" ] + [ "${lines[1]}" = "21" ] + [ "${#lines[@]}" -eq 2 ] +} + +@test "plan: dispatches at most MAX ready PRs" { + input='[{"number":1,"ready":true},{"number":2,"ready":true},{"number":3,"ready":true},{"number":4,"ready":true}]' + run bash -c 'printf "%s" '"'$input'"' | pr_auto_review_sweep_plan 2' + [ "$status" -eq 0 ] + [ "${lines[0]}" = "1" ] + [ "${lines[1]}" = "2" ] + [ "${#lines[@]}" -eq 2 ] +} + +@test "plan: fewer ready than MAX dispatches all ready" { + input='[{"number":1,"ready":false},{"number":2,"ready":true},{"number":3,"ready":false}]' + run bash -c 'printf "%s" '"'$input'"' | pr_auto_review_sweep_plan 5' + [ "$status" -eq 0 ] + [ "${lines[0]}" = "2" ] + [ "${#lines[@]}" -eq 1 ] +} + +@test "plan: no ready candidates dispatches nothing" { + input='[{"number":1,"ready":false},{"number":2,"ready":false}]' + run bash -c 'printf "%s" '"'$input'"' | pr_auto_review_sweep_plan 3' + [ "$status" -eq 0 ] + [ "$output" = "" ] +} + +@test "plan: MAX of 0 dispatches nothing" { + input='[{"number":1,"ready":true},{"number":2,"ready":true}]' + run bash -c 'printf "%s" '"'$input'"' | pr_auto_review_sweep_plan 0' + [ "$status" -eq 0 ] + [ "$output" = "" ] +} + +@test "plan: empty candidate array dispatches nothing" { + run bash -c 'printf "[]" | pr_auto_review_sweep_plan 5' + [ "$status" -eq 0 ] + [ "$output" = "" ] +} + +# ── #872 finding 5 (ordering): oldest-first so ready PRs are not starved ────── + +@test "order: sorts candidates oldest updatedAt first" { + input='[{"number":2,"updatedAt":"2026-07-20T00:00:00Z"},{"number":1,"updatedAt":"2026-07-18T00:00:00Z"},{"number":3,"updatedAt":"2026-07-22T00:00:00Z"}]' + run bash -c 'printf "%s" '"'$input'"' | pr_auto_review_sweep_order | jq -c "[.[].number]"' + [ "$status" -eq 0 ] + [ "$output" = "[1,2,3]" ] +} + +@test "order: ties on updatedAt break by number ascending" { + input='[{"number":9,"updatedAt":"2026-07-20T00:00:00Z"},{"number":4,"updatedAt":"2026-07-20T00:00:00Z"}]' + run bash -c 'printf "%s" '"'$input'"' | pr_auto_review_sweep_order | jq -c "[.[].number]"' + [ "$status" -eq 0 ] + [ "$output" = "[4,9]" ] +} + +# ── #872 finding 2: full pagination (no 100-item cliff) ─────────────────────── + +@test "page_full: a full page signals more pages may exist (exit 0)" { + run pr_auto_review_sweep_page_full 100 100 + [ "$status" -eq 0 ] +} + +@test "page_full: a short page signals the last page (exit 1)" { + run pr_auto_review_sweep_page_full 37 100 + [ "$status" -eq 1 ] +} + +@test "page_full: an empty page signals the last page (exit 1)" { + run pr_auto_review_sweep_page_full 0 100 + [ "$status" -eq 1 ] +} + +@test "merge_pages: concatenated pages merge and dedupe by number" { + page1='[{"number":1,"updatedAt":"2026-07-18T00:00:00Z"},{"number":2,"updatedAt":"2026-07-19T00:00:00Z"}]' + page2='[{"number":2,"updatedAt":"2026-07-19T00:00:00Z"},{"number":3,"updatedAt":"2026-07-20T00:00:00Z"}]' + run bash -c 'printf "%s\n%s" '"'$page1'"' '"'$page2'"' | pr_auto_review_sweep_merge_pages | jq -c "[.[].number]"' + [ "$status" -eq 0 ] + [ "$output" = "[1,2,3]" ] +} + +@test "merge_pages: a single page passes through unchanged in count" { + page='[{"number":5,"updatedAt":"2026-07-18T00:00:00Z"}]' + run bash -c 'printf "%s" '"'$page'"' | pr_auto_review_sweep_merge_pages | jq "length"' + [ "$status" -eq 0 ] + [ "$output" = "1" ] +} + +# ── #872 finding 3: a failed search surfaces, not silently empty ────────────── + +@test "valid_search: a valid (even empty) JSON array is accepted" { + run bash -c 'printf "[]" | pr_auto_review_sweep_valid_search' + [ "$status" -eq 0 ] + run bash -c 'printf "%s" "[{\"number\":1}]" | pr_auto_review_sweep_valid_search' + [ "$status" -eq 0 ] +} + +@test "valid_search: an API error object is rejected (surface, do not no-op)" { + run bash -c 'printf "%s" "{\"message\":\"Bad credentials\"}" | pr_auto_review_sweep_valid_search' + [ "$status" -ne 0 ] +} + +@test "valid_search: an empty string is rejected" { + run bash -c 'printf "" | pr_auto_review_sweep_valid_search' + [ "$status" -ne 0 ] +} + +@test "valid_search: malformed JSON is rejected" { + run bash -c 'printf "%s" "{not json" | pr_auto_review_sweep_valid_search' + [ "$status" -ne 0 ] +} + +# ── #872 finding 4: a malformed payload is guarded, does not abort the run ──── + +@test "extract: a well-formed search payload yields number+updatedAt candidates" { + payload='[{"number":7,"updatedAt":"2026-07-20T00:00:00Z","title":"x","author":{"login":"a"}}]' + run bash -c 'printf "%s" '"'$payload'"' | pr_auto_review_sweep_extract' + [ "$status" -eq 0 ] + echo "$output" | jq -e '.[0].number == 7 and .[0].updatedAt == "2026-07-20T00:00:00Z"' +} + +@test "extract: accepts the REST search field name updated_at" { + payload='[{"number":8,"updated_at":"2026-07-19T00:00:00Z","title":"y"}]' + run bash -c 'printf "%s" '"'$payload'"' | pr_auto_review_sweep_extract' + [ "$status" -eq 0 ] + echo "$output" | jq -e '.[0].number == 8 and .[0].updatedAt == "2026-07-19T00:00:00Z"' +} + +@test "extract: a malformed payload returns non-zero instead of aborting" { + run bash -c 'printf "%s" "{oops" | pr_auto_review_sweep_extract' + [ "$status" -ne 0 ] +} + +@test "extract: a non-array payload returns non-zero" { + run bash -c 'printf "%s" "{\"message\":\"Not Found\"}" | pr_auto_review_sweep_extract' + [ "$status" -ne 0 ] +} + +# The guard's purpose: even under `set -e`, a malformed payload must be catchable +# so the caller can continue rather than have the whole sweep aborted by jq. +@test "extract: guarded call under set -e lets the caller continue past a bad payload" { + run bash -c ' + set -euo pipefail + . "'"${TT_SCRIPTS_DIR}"'/lib/sweep.sh" + if printf "%s" "{bad" | pr_auto_review_sweep_extract >/dev/null 2>&1; then + echo "unexpected-success" + else + echo "handled-and-continued" + fi + ' + [ "$status" -eq 0 ] + [ "$output" = "handled-and-continued" ] +}