Skip to content

feat: implement issue #872 — [#850] harden pr-auto-review catch-up sweep (starvation ordering, pagination, error handling) #873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
don-petry wants to merge 1 commit into from
Closed

feat: implement issue #872 — [#850] harden pr-auto-review catch-up sweep (starvation ordering, pagination, error handling) #873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions .github/scripts/pr-auto-review/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
132 changes: 132 additions & 0 deletions .github/scripts/pr-auto-review/lib/sweep.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}
Comment on lines +44 to +55

@gemini-code-assist gemini-code-assist Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

To ensure the function explicitly and reliably returns a success status when the search payload is valid, it is best practice to add an explicit return 0 at the end of the function rather than relying on the implicit exit status of the last executed command.

Suggested change
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_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
return 0
}
References
  1. In Bash functions, use an explicit 'return 0' (or other specific exit code) instead of a bare 'return' to make the function's intent self-documenting and to prevent the return value from implicitly inheriting the exit status of the preceding command.


# 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] | .[]
'
Comment on lines +128 to +131

@codeant-ai codeant-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: MAX_PER_RUN is passed straight into jq slicing without validating that it is a non-negative integer. If it is set to a negative value (for example -1), jq slice semantics (.[0:-1]) will dispatch almost the entire ready list instead of dispatching none, violating the run cap. Validate and clamp MAX_PER_RUN to >= 0 before calling jq. [logic error]

Severity Level: Major ⚠️ 
⚠️ Sweep may dispatch more PRs than configured.
⚠️ Cap misconfiguration silently breaks fairness guarantees.
Steps of Reproduction ✅ 
1. Configure the environment variable MAX_PER_RUN to "-1" before running the sweep
orchestrator `.github/scripts/pr-auto-review/sweep.sh` (MAX_PER_RUN is read from the
environment at `.github/scripts/pr-auto-review/sweep.sh:52`).

2. Run the sweep so it builds the readiness verdict list `verdicts` for candidate PRs in
`.github/scripts/pr-auto-review/sweep.sh:159-184`, producing an array of `{number, ready}`
objects.

3. Observe that the planner `pr_auto_review_sweep_plan` defined in
`.github/scripts/pr-auto-review/lib/sweep.sh:127-131` receives `max=-1` and executes the
jq slice `[ .[] | select(.ready == true) | .number ] | .[0:$max] | .[]`, where `$max` is
the negative value.

4. jq’s negative slice (`.[0:-1]`) selects all but the last ready PR instead of none, so
the dispatch loop in `.github/scripts/pr-auto-review/sweep.sh:190-204` sends reviews for
almost every ready PR in the list, violating the configured per-run limit.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** .github/scripts/pr-auto-review/lib/sweep.sh
**Line:** 128:131
**Comment:**
	*Logic Error: `MAX_PER_RUN` is passed straight into jq slicing without validating that it is a non-negative integer. If it is set to a negative value (for example `-1`), jq slice semantics (`.[0:-1]`) will dispatch almost the entire ready list instead of dispatching none, violating the run cap. Validate and clamp `MAX_PER_RUN` to `>= 0` before calling jq.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

}
204 changes: 204 additions & 0 deletions .github/scripts/pr-auto-review/sweep.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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:-}}"

@qodo-code-review qodo-code-review Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. sweep_per_page not validated 📘 Rule violation ⛨ Security

Environment variables such as MAX_PER_RUN, SWEEP_PER_PAGE, and SWEEP_MAX_PAGES are consumed as
numeric inputs without type/range validation even though they drive numeric comparisons and API
parameters. In particular, a non-numeric MAX_PER_RUN can cause the jq --argjson-based planner to
fail in a way that may not reliably propagate through process substitution, resulting in a
misleading “dispatch 0” no-op run instead of a hard failure.
Agent Prompt 
## Issue description
Environment variables used as numeric controls (`MAX_PER_RUN`, `SWEEP_PER_PAGE`, `SWEEP_MAX_PAGES`) are not validated for type/range before being used in numeric comparisons and GitHub API parameters. A bad `MAX_PER_RUN` value can also make the planner’s `jq --argjson` fail, and because the sweep captures planner output via process substitution into `mapfile`, that failure may not reliably fail the parent script, leading to a silent no-op (e.g., “dispatch 0”) instead of an explicit error.

## Issue Context
These variables are external inputs that influence pagination behavior and dispatch limits, so configuration mistakes should fail loudly and predictably. The planner uses `jq --argjson max "$max"`, which requires valid JSON/numeric input; combined with process substitution, planner failures may not be deterministically propagated under `set -e`, reducing reliability of this periodic mechanism.

## Fix Focus Areas
- .github/scripts/pr-auto-review/sweep.sh[50-57]
- .github/scripts/pr-auto-review/sweep.sh[118-125]
- .github/scripts/pr-auto-review/sweep.sh[186-187]
- .github/scripts/pr-auto-review/lib/sweep.sh[127-132]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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

@qodo-code-review qodo-code-review Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Fallback [] hides fetch failures 📘 Rule violation ☼ Reliability

sweep_pr_facts substitutes empty arrays when GitHub API calls or parsing fail, without any
warning/degraded flag. This can cause the sweep to treat missing/failed fact gathering as “no
required contexts / no checks,” potentially dispatching reviews for PRs that are not actually ready.
Agent Prompt 
## Issue description
`sweep_pr_facts` falls back to `[]` (empty checks/required contexts) when `gh`/`jq` work fails, but does not log/flag the degraded state. This can silently misclassify readiness.

## Issue Context
Current code uses patterns like `2>/dev/null`, `|| true`, and `|| echo "[]"` that convert failures into empty arrays.

## Fix Focus Areas
- .github/scripts/pr-auto-review/sweep.sh[80-90]
- .github/scripts/pr-auto-review/sweep.sh[82-83]
- .github/scripts/pr-auto-review/sweep.sh[85-90]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

# 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
Comment on lines +115 to +116

@gemini-code-assist gemini-code-assist Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

When creating temporary files in shell scripts, it is safer and more robust to use a global array and a single array-safe EXIT trap to register and clean up all temporary files. This ensures they are properly cleaned up even if the script terminates prematurely or if multiple files are created.

Suggested change
pages_file="$(mktemp)"
trap 'rm -f "$pages_file"' EXIT
declare -a tmpfiles=()
cleanup() {
rm -f "${tmpfiles[@]+"${tmpfiles[@]}"}"
}
trap cleanup EXIT
pages_file="$(mktemp)"
tmpfiles+=("$pages_file")
References
  1. When creating temporary files or directories in shell scripts, use a global array and a single array-safe EXIT trap to register and clean up all temporary files, ensuring they are removed even if the script terminates prematurely or if multiple files are created.


page=1

@qodo-code-review qodo-code-review Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

5. Silent page-ceiling truncation 🐞 Bug ☼ Reliability

The pagination loop stops after SWEEP_MAX_PAGES even if the last fetched page was full, so
additional pages (and their PRs) are skipped with no warning/error. This is especially likely to
truncate below the Search API’s 1000-result ceiling when SWEEP_PER_PAGE is reduced but
SWEEP_MAX_PAGES is left at its default.
Agent Prompt 
### Issue description
`sweep.sh` pages while `page <= SWEEP_MAX_PAGES`, but it never detects/report that it stopped due to the configured ceiling rather than because the result set ended. If the last page was full, more pages may exist, yet the script proceeds as if the candidate set is complete.

### Issue Context
This is a catch-up mechanism intended to avoid missing PRs; silently stopping early defeats that goal when configuration is too small (most notably when `SWEEP_PER_PAGE` is lowered).

### Fix Focus Areas
- .github/scripts/pr-auto-review/sweep.sh[118-156]

### Proposed fix
- Track whether the last fetched page was full.
- After the loop, if `page > SWEEP_MAX_PAGES` (i.e., ceiling hit) **and** the last page was full, emit `::error::` (or at least `::warning::`) stating results may be truncated and suggesting increasing `SWEEP_MAX_PAGES` and/or `SWEEP_PER_PAGE`.
- Consider defaulting `SWEEP_MAX_PAGES` based on `SWEEP_PER_PAGE` (e.g., enough pages to reach 1000 results), if that’s consistent with your intended API-cap behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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)

@codeant-ai codeant-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The search response is treated as complete as long as .items is an array, but GitHub search responses can set incomplete_results=true when results are truncated. Ignoring that flag can silently drop eligible PRs and break catch-up guarantees; detect incomplete_results and retry or abort with an error. [api mismatch]

Severity Level: Major ⚠️ 
⚠️ Sweep may miss ready PRs when search incomplete.
⚠️ Catch-up automation unreliable for large or busy repositories.
Steps of Reproduction ✅ 
1. Run `.github/scripts/pr-auto-review/sweep.sh` in a repository with enough open
auto-review PRs that GitHub Search can legitimately return `incomplete_results: true` for
the query composed at `.github/scripts/pr-auto-review/sweep.sh:112-114` (`repo:${REPO}
is:pr is:open` with optional label).

2. The script issues the search request via `gh api -X GET /search/issues` at
`.github/scripts/pr-auto-review/sweep.sh:121-125` and stores the full JSON response, which
includes both `items` and the `incomplete_results` flag.

3. At `.github/scripts/pr-auto-review/sweep.sh:131`, only the `.items` array is extracted
into `items=$(printf '%s' "$response" | jq -c '.items' 2>/dev/null || true)`, and
subsequent validation at lines 134-137 (`pr_auto_review_sweep_valid_search`) checks solely
that `items` is a well-formed JSON array, ignoring `incomplete_results`.

4. Pagination and candidate construction in
`.github/scripts/pr-auto-review/sweep.sh:146-155` proceed based on `items` alone, so any
PRs omitted because `incomplete_results=true` never enter `candidates` and are silently
skipped by the readiness evaluation and dispatch planning that follow, reducing the
catch-up sweep’s guarantee that all eligible PRs eventually receive auto-review.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** .github/scripts/pr-auto-review/sweep.sh
**Line:** 131:131
**Comment:**
	*Api Mismatch: The search response is treated as complete as long as `.items` is an array, but GitHub search responses can set `incomplete_results=true` when results are truncated. Ignoring that flag can silently drop eligible PRs and break catch-up guarantees; detect `incomplete_results` and retry or abort with an error.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


# 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')
Comment on lines +162 to +184

@gemini-code-assist gemini-code-assist Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Spawning multiple jq processes inside a loop (6 times for field extraction + 1 time for appending to the array per candidate) creates a significant performance bottleneck. We can optimize this by:

  1. Extracting all fields in a single jq call using @tsv formatting and reading them into variables with IFS=$'\t' read -r.
  2. Streaming the JSON objects from the loop and collecting them into a single array with jq -sc '.' at the end, completely avoiding process spawns for array appending.
Suggested change
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')
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")
# Extract all fields in a single jq call to avoid spawning multiple processes per iteration.
IFS=$'\t' read -r f_state f_draft f_review f_blocking f_checks f_required < <(
printf '%s' "$facts" | jq -r '[.state, .isDraft, (.reviewDecision // ""), .blocking, (.checks|@json), (.required|@json)] | @tsv'
)
# 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}" >&2
printf '{"number":%d,"ready":%s}\n' "$num" "$ready"
done < <(printf '%s' "$candidates" | jq -r '.[].number') | jq -sc '.'
)


mapfile -t to_dispatch < <(printf '%s' "$verdicts" | pr_auto_review_sweep_plan "$MAX_PER_RUN")

@codeant-ai codeant-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The planner is executed inside process substitution for mapfile, which does not reliably propagate producer exit failures under set -e; if the planner errors (for example due to invalid MAX_PER_RUN), the script can continue with an empty dispatch list and silently no-op. Run the planner in a checked command path and fail explicitly on non-zero exit. [logic error]

Severity Level: Major ⚠️ 
⚠️ Planner failures cause silent no-op instead of error.
⚠️ Misconfigured run appears successful while skipping all PRs.
Steps of Reproduction ✅ 
1. Configure MAX_PER_RUN to a non-numeric value (for example `MAX_PER_RUN=foo`) before
running `.github/scripts/pr-auto-review/sweep.sh`; the script reads this environment value
at line 52.

2. During a sweep run, the script builds the `verdicts` JSON array of `{number, ready}`
objects from candidate PRs in `.github/scripts/pr-auto-review/sweep.sh:159-184`.

3. At `.github/scripts/pr-auto-review/sweep.sh:186`, `mapfile -t to_dispatch < <(printf
'%s' "$verdicts" | pr_auto_review_sweep_plan "$MAX_PER_RUN")` invokes
`pr_auto_review_sweep_plan` (implemented in
`.github/scripts/pr-auto-review/lib/sweep.sh:127-131`), where jq’s `--argjson max
"$MAX_PER_RUN"` fails to parse the non-numeric value and exits non-zero without emitting a
list.

4. Because the planner runs inside process substitution, its non-zero exit status does not
abort the main shell under `set -e`; `mapfile` simply receives no lines, `to_dispatch`
becomes an empty array, and the dispatch loop in
`.github/scripts/pr-auto-review/sweep.sh:190-204` silently no-ops while the script reports
“dispatching 0 of up to foo slot(s)” instead of surfacing an explicit error.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** .github/scripts/pr-auto-review/sweep.sh
**Line:** 186:186
**Comment:**
	*Logic Error: The planner is executed inside process substitution for `mapfile`, which does not reliably propagate producer exit failures under `set -e`; if the planner errors (for example due to invalid `MAX_PER_RUN`), the script can continue with an empty dispatch list and silently no-op. Run the planner in a checked command path and fail explicitly on non-zero exit.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

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
Loading
Loading