From cdb17bd791113b62ba1c8ea468a55de21ce970a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 17:29:50 +0000 Subject: [PATCH 1/2] =?UTF-8?q?pr-review-sweep:=20event-driven=20re-trigge?= =?UTF-8?q?r=20to=20close=20the=20ci-pending=E2=86=92green=20latency=20gap?= =?UTF-8?q?=20(#898)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reframes #898 from its (incorrect) marker hypothesis to the real cause. The ci-pending skip in review-one-pr.sh exits 100 BEFORE the idempotency check and writes no marker, so it is already non-terminal; the selection logic that re-reviews a stuck-green PR (REVIEW_REQUIRED + CI green + no marker) is correct and already covered by tests. What stalled #892 was purely LATENCY: the only thing that re-reviews a PR which goes green after the last trigger fired was the hourly stuck-review sweep (#573), so the PR could sit green-but-unreviewed for up to an hour (the human force-reviewed before the next tick). Close the gap with two robust, layered triggers on pr-review-sweep.yml: - Scheduled cron tightened from hourly to every 15 min — the GUARANTEED backstop bounding worst-case re-review latency, with zero fragility. - A `workflow_run: completed` fast path keyed on the broadly-run CI workflows. When CI finishes, the sweep runs SCOPED to just that run's PR(s) — sweep-stuck-reviews.sh derives them from the event payload (`prs_from_workflow_run_event`, pure/unit-tested) instead of enumerating the fleet — so a PR that just went green is re-reviewed in seconds. Per-branch `cancel-in-progress` concurrency collapses the multi-workflow burst so the sweep effectively runs once, after the last-finishing workflow (most likely to observe green). The fast path can never strand a PR: if it matches nothing (fork PR, branch push, CI still pending, or a workflow rename) the scheduled sweep still catches it, and the same REVIEW_REQUIRED + green + no-marker gate decides in every path. Tests: 5 new cases in tests/test_sweep_stuck_reviews.bats exercise the workflow_run path — ci-pending→green dispatches (scoped to the event), empty pull_requests is a clean no-op, a too-early fire (CI still pending) does not dispatch, an already-reviewed-at-head PR is not re-dispatched, and a multi-PR run evaluates each independently. shellcheck clean; existing 21 sweep tests unchanged and green. AGENTS.md exception note updated to describe both triggers. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr --- .github/workflows/pr-review-sweep.yml | 29 ++++++++- AGENTS.md | 6 +- scripts/sweep-stuck-reviews.sh | 32 ++++++++++ tests/test_sweep_stuck_reviews.bats | 88 +++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 5 deletions(-) diff --git a/.github/workflows/pr-review-sweep.yml b/.github/workflows/pr-review-sweep.yml index 89cf5838d..81b74b369 100644 --- a/.github/workflows/pr-review-sweep.yml +++ b/.github/workflows/pr-review-sweep.yml @@ -12,6 +12,14 @@ name: PR Review Agent — Stuck-Review Sweep # lifting (CI gating, idempotency) still lives in review-one-pr.sh; the sweep # only decides WHAT to re-trigger via pr-review-trigger.yml. # +# Latency (#898): the scheduled sweep is the GUARANTEED backstop (≤15 min). On +# top of it, a `workflow_run: completed` trigger gives a near-instant fast path — +# when a core CI workflow finishes, the sweep runs SCOPED to just that run's PR(s) +# (scripts/sweep-stuck-reviews.sh derives them from the event payload), so a PR +# that just went green is re-reviewed in seconds instead of waiting for the next +# cron tick. The fast path can never strand a PR: if it matches nothing (fork PR, +# push to a branch, or CI still pending) the scheduled sweep still catches it. +# # NOTE: the re-dispatch runs as GH_PAT_WORKFLOWS (a PAT with workflow / # actions:write scope), NOT the default GITHUB_TOKEN — GitHub will not start a # new run from a workflow_dispatch fired with GITHUB_TOKEN (loop prevention). @@ -26,8 +34,20 @@ name: PR Review Agent — Stuck-Review Sweep on: schedule: - # Hourly, offset from the top of the hour to avoid contending with other crons. - - cron: '17 * * * *' + # Every 15 min (offset to avoid contending with other crons) — the guaranteed + # backstop bounding worst-case re-review latency when the fast path misses. + - cron: '2,17,32,47 * * * *' + workflow_run: + # Event-driven fast path (#898): a core CI workflow finishing is the signal + # that a PR may have just gone green. The sweep run is scoped to that PR by the + # script, so firing on several workflows is cheap and idempotent — and the + # per-branch `cancel-in-progress` concurrency below collapses the burst so the + # sweep effectively runs ONCE, after the last-finishing keyed workflow (the one + # most likely to observe green). Keyed on broadly-run, stable workflow names + # (must match each workflow's `name:`); the scheduled sweep backstops any + # rename/miss, so this list is best-effort, not load-bearing. + workflows: ["CI", "Tests", "Holdout Guard", "SonarCloud Analysis", "Lint"] + types: [completed] workflow_dispatch: inputs: dry_run: @@ -45,7 +65,10 @@ permissions: contents: read concurrency: - group: pr-review-sweep + # Per-trigger/branch grouping so an event-driven sweep for one branch never + # cancels the scheduled full sweep (or another branch's sweep); cancel-in-progress + # still collapses a burst of CI completions on the SAME branch into one run. + group: pr-review-sweep-${{ github.event.workflow_run.head_branch || github.event_name }} cancel-in-progress: true jobs: diff --git a/AGENTS.md b/AGENTS.md index c2b6fad88..5e980f3ec 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,9 +50,11 @@ This is the `.github-private` org infrastructure repo for `petry-projects`. It c reviewable decision. The other half is the **"require branches up to date before merging"** ruleset on `main` (already enabled). Together they close the gap behind #655, where a PR merged from a stale base reverted a shipped fix and deleted its regression test in the same green-CI diff. Do not remove either guard. -- **Exception:** `pr-review-sweep.yml` (scheduled stuck-review sweep, #573) is a documented repo-specific +- **Exception:** `pr-review-sweep.yml` (stuck-review sweep, #573/#898) is a documented repo-specific workflow with no corresponding org template in `standards/workflows/`. It re-dispatches reviews for PRs - that went green after a ci-pending/ci-failing skip. It must not be removed by template syncs. If the org + that went green after a ci-pending/ci-failing skip, via two triggers: a scheduled cron (the guaranteed + ≤15-min backstop) and a `workflow_run: completed` fast path (#898) that scopes the sweep to the + completing CI run's PR(s) for near-instant re-review. It must not be removed by template syncs. If the org template gains an equivalent re-trigger sweep, remove this exception and defer to the template instead. - **Exception:** The `bats` test list in `lint.yml` is extended with repo-specific test files (e.g. `tests/test_push_protection.bats`) beyond the org template baseline. When adding new test files to this diff --git a/scripts/sweep-stuck-reviews.sh b/scripts/sweep-stuck-reviews.sh index e811cb3b2..14e0a962e 100755 --- a/scripts/sweep-stuck-reviews.sh +++ b/scripts/sweep-stuck-reviews.sh @@ -72,8 +72,40 @@ fi candidates_file="$(mktemp)" trap 'rm -f "$candidates_file"' EXIT +# prs_from_workflow_run_event +# Event-driven fast path: when the sweep is kicked by a `workflow_run: completed` +# event (a CI workflow just finished), scope the candidate set to the PR(s) +# attached to that run instead of enumerating the whole fleet. Emits one PR +# html_url per line from `.workflow_run.pull_requests[]`. Pure (reads a JSON file, +# no network), so it is unit-testable. +# +# GitHub populates `pull_requests` only for SAME-repo head branches; fork-PR runs +# and plain pushes to a branch carry an empty array and yield nothing — those are +# covered by the scheduled (cron) sweep, and this repo never self-reviews forks. +prs_from_workflow_run_event() { + local event_path="${1:-}" + [ -n "$event_path" ] && [ -r "$event_path" ] || return 0 + local repo + repo="$(jq -r '.repository.full_name // empty' "$event_path" 2>/dev/null || true)" + [ -n "$repo" ] || return 0 + jq -r --arg repo "$repo" ' + (.workflow_run.pull_requests // []) + | map(select(.number != null) | .number) + | unique + | .[] + | "https://github.com/\($repo)/pull/\(.)" + ' "$event_path" 2>/dev/null || true +} + if [[ -n "${SWEEP_PRS_FILE:-}" && -r "${SWEEP_PRS_FILE}" ]]; then grep -v '^[[:space:]]*$' "$SWEEP_PRS_FILE" > "$candidates_file" || true +elif [[ "${GITHUB_EVENT_NAME:-}" == "workflow_run" && -n "${GITHUB_EVENT_PATH:-}" ]]; then + # CI-completion kick (#898): inspect only the completing run's PR(s). The + # REVIEW_REQUIRED + CI-green + not-reviewed-at-head gate below still decides, so + # a too-early fire (some checks still pending) simply skips and the next + # completing workflow re-fires. The scheduled sweep remains the guaranteed + # backstop, so this fast path can never strand a PR if it matches nothing. + prs_from_workflow_run_event "$GITHUB_EVENT_PATH" > "$candidates_file" || true else bash "$SCRIPT_DIR/list-prs.sh" > "$candidates_file" || true fi diff --git a/tests/test_sweep_stuck_reviews.bats b/tests/test_sweep_stuck_reviews.bats index f945c04e1..1a9b53e92 100644 --- a/tests/test_sweep_stuck_reviews.bats +++ b/tests/test_sweep_stuck_reviews.bats @@ -301,3 +301,91 @@ FUTURE_RESET='2999-01-01T00:00:00Z' [ ! -s "$GH_LOG" ] [[ "$output" == *"$(url_for 717)"* ]] } + +# --------------------------------------------------------------------------- +# Event-driven fast path (#898): a `workflow_run: completed` kick scopes the +# sweep to the completing run's PR(s) via the event payload, so a PR that just +# went green is re-reviewed in seconds instead of waiting for the cron backstop. +# The same REVIEW_REQUIRED + CI-green + not-reviewed-at-head gate still decides. +# --------------------------------------------------------------------------- + +# write_event +write_event() { + local file="$1" repo="$2" prs="$3" + jq -n --arg repo "$repo" --argjson prs "$prs" \ + '{repository:{full_name:$repo}, workflow_run:{pull_requests: ($prs | map({number: .}))}}' \ + > "$file" +} + +# In the event path the script derives the PR url as +# https://github.com//pull/; the gh mock keys the fixture off the +# trailing , so pr_.json must exist. +ghp_event_url() { echo "https://github.com/petry-projects/.github-private/pull/$1"; } + +@test "workflow_run kick: ci-pending→green PR (REVIEW_REQUIRED + green + no marker) is dispatched, scoped to the event" { + unset SWEEP_PRS_FILE + local ev="$FIXTURE_DIR/event.json" + write_event "$ev" "petry-projects/.github-private" '[898]' + export GITHUB_EVENT_NAME=workflow_run + export GITHUB_EVENT_PATH="$ev" + write_pr 898 "REVIEW_REQUIRED" "$ROLLUP_PASS" "greensha" + + run bash "$SCRIPT" + [ "$status" -eq 0 ] + grep -qF "workflow run pr-review-trigger.yml" "$GH_LOG" + grep -qF -- "-f pr_url=$(ghp_event_url 898)" "$GH_LOG" +} + +@test "workflow_run kick: empty pull_requests (fork PR / branch push) is a clean no-op" { + unset SWEEP_PRS_FILE + local ev="$FIXTURE_DIR/event.json" + write_event "$ev" "petry-projects/.github-private" '[]' + export GITHUB_EVENT_NAME=workflow_run + export GITHUB_EVENT_PATH="$ev" + + run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ ! -s "$GH_LOG" ] +} + +@test "workflow_run kick: event PR with CI still pending is NOT dispatched (too-early fire backstopped by cron)" { + unset SWEEP_PRS_FILE + local ev="$FIXTURE_DIR/event.json" + write_event "$ev" "petry-projects/.github-private" '[899]' + export GITHUB_EVENT_NAME=workflow_run + export GITHUB_EVENT_PATH="$ev" + write_pr 899 "REVIEW_REQUIRED" "$ROLLUP_PENDING" "pendsha" + + run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ ! -s "$GH_LOG" ] +} + +@test "workflow_run kick: already-reviewed-at-head event PR is NOT re-dispatched" { + unset SWEEP_PRS_FILE + local ev="$FIXTURE_DIR/event.json" + write_event "$ev" "petry-projects/.github-private" '[900]' + export GITHUB_EVENT_NAME=workflow_run + export GITHUB_EVENT_PATH="$ev" + local comments='[{"body":" reviewed"}]' + write_pr 900 "REVIEW_REQUIRED" "$ROLLUP_PASS" "donesha" "[]" "$comments" + + run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ ! -s "$GH_LOG" ] +} + +@test "workflow_run kick: multiple PRs on the run are each evaluated" { + unset SWEEP_PRS_FILE + local ev="$FIXTURE_DIR/event.json" + write_event "$ev" "petry-projects/.github-private" '[901,902]' + export GITHUB_EVENT_NAME=workflow_run + export GITHUB_EVENT_PATH="$ev" + write_pr 901 "REVIEW_REQUIRED" "$ROLLUP_PASS" "g901" # green → dispatch + write_pr 902 "REVIEW_REQUIRED" "$ROLLUP_FAIL" "f902" # failing → skip + + run bash "$SCRIPT" + [ "$status" -eq 0 ] + grep -qF -- "-f pr_url=$(ghp_event_url 901)" "$GH_LOG" + ! grep -qF -- "-f pr_url=$(ghp_event_url 902)" "$GH_LOG" +} From 752af093a5cf73b5b255e5c3b92e3213d0188a56 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 17:35:14 +0000 Subject: [PATCH 2/2] sweep: fold prs_from_workflow_run_event into a single jq pass (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address gemini-code-assist's medium-priority note on #914: parse the workflow_run event payload exactly once. Bind .repository.full_name inside the jq program and `select` it out (empty stream) when absent, instead of a separate jq invocation + shell guard — one process, one read, same behavior. The positional-arg guard (`${1:-}`, set -u-safe) and the URL output are unchanged; all 26 sweep tests still pass and shellcheck is clean. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr --- scripts/sweep-stuck-reviews.sh | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/scripts/sweep-stuck-reviews.sh b/scripts/sweep-stuck-reviews.sh index 14e0a962e..aea29ea51 100755 --- a/scripts/sweep-stuck-reviews.sh +++ b/scripts/sweep-stuck-reviews.sh @@ -85,11 +85,12 @@ trap 'rm -f "$candidates_file"' EXIT prs_from_workflow_run_event() { local event_path="${1:-}" [ -n "$event_path" ] && [ -r "$event_path" ] || return 0 - local repo - repo="$(jq -r '.repository.full_name // empty' "$event_path" 2>/dev/null || true)" - [ -n "$repo" ] || return 0 - jq -r --arg repo "$repo" ' - (.workflow_run.pull_requests // []) + # Single jq pass: bind the repo, drop out (empty stream) when it is absent, and + # format the PR urls in one go — one process, one read of the payload. + jq -r ' + (.repository.full_name // "") as $repo + | select($repo != "") + | (.workflow_run.pull_requests // []) | map(select(.number != null) | .number) | unique | .[]