pr-review-sweep: event-driven re-trigger to close the ci-pending→green latency gap (#898) - #914
Conversation
…n latency gap (#898) 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an event-driven fast path to ChangesEvent-driven stuck-review sweep
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-06-23T18:31:34Z. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an event-driven fast path to the stuck-review sweep script, triggered by 'workflow_run: completed' events, allowing PRs that just went green to be re-reviewed quickly. It also adds comprehensive unit tests and updates the documentation. The reviewer suggested optimizing the 'prs_from_workflow_run_event' helper function to parse the JSON file with a single 'jq' pipeline instead of two, and to explicitly validate positional parameters to prevent unbound variable errors.
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y3xW3cnB5wscv4SmjSNEr
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #914 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-06-23T19:28:53Z. |
|
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #914 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-06-23T20:29:15Z. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 4dfe4284c56a7f4f2386f6cf1f9828848ce24a66
Review mode: triage-approved (single reviewer)
Summary
Adds an event-driven fast path to the stuck-review sweep: a workflow_run:completed trigger (keyed on core CI workflows) re-runs the sweep scoped to just the completing run's PR(s), tightens the cron backstop from hourly to every 15 min, and makes the concurrency group per-branch/event. A new pure, unit-tested helper prs_from_workflow_run_event derives PR URLs from the event payload. Authoritative gating in review-one-pr.sh is unchanged. Confirms the triage low-risk assessment.
Linked issue analysis
Closes #898. The PR substantively addresses the issue by reframing its (incorrect) marker hypothesis to the real cause — re-review latency — and closing the gap with two layered triggers (15-min cron backstop + workflow_run fast path). Linked-issue gate satisfied.
Findings
No blocking findings.
- Security: workflow_run runs against the trusted default-branch workflow definition and checks out only the agent repo (no untrusted PR-code execution). permissions remain contents: read. PR numbers are taken from .workflow_run.pull_requests[].number, validated as integers via jq — no injection vector. github.event.workflow_run.head_branch is used only as a YAML concurrency-group key, never in a shell context. Fork-PR/branch-push runs carry an empty pull_requests array and are correctly excluded; the scheduled sweep backstops any miss.
- Review threads: gemini-code-assist's medium-priority note (single jq pass + set -u-safe positional guard) was addressed in commit 752af09. CodeRabbit APPROVED at head; SonarCloud Quality Gate passed.
- Advisory bots: the earlier 'rate-limited' markers were Codex usage-limit notices (external billing), not code findings.
- Secret-scan MCP tool unavailable in this environment; not run (diff contains no secret material; gitleaks CI check passed).
- Tests: 5 new bats cases cover dispatch-on-green, empty no-op, too-early/CI-pending skip, already-reviewed-at-head skip, and multi-PR evaluation; shellcheck clean.
CI status
All required checks green (CI/Lint/ShellCheck/bats/unit-tests, CodeQL, AgentShield, SonarCloud, gitleaks, Holdout Guard, Test-Deletion Guard, Dev-Lead); dependency-audit ecosystem jobs SKIPPED as expected. No failing or cancelled checks. mergeStateStatus is BLOCKED solely on the REVIEW_REQUIRED gate (org-leads).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
3 participants
Closes #898. Part of the pr-review reliability work (related: #573, #619, #463, #617).
TL;DR — the issue's root cause was wrong; this fixes the real one
I traced the pr-review state machine against the #892 evidence (details in the investigation comment on #898):
ci-pendingskip exits100atreview-one-pr.sh:184, before the idempotency check at:290, and writes nopr-review-agent v1 sha=…marker. So it's already non-terminal — the prescribed "don't persist the marker forreason=ci-pending" fix would be a no-op.review-batch.sh:256's generic label for any exit-100, not a persisted marker.REVIEW_REQUIRED+ CI green + no marker) is correct and already covered (test_sweep_stuck_reviews.bats:74).What actually stalled #892 was latency. The only thing that re-reviews a PR which goes green after the last trigger fired is the stuck-review sweep (#573), which ran hourly — so the PR could sit green-but-unreviewed for up to an hour. The human force-reviewed before the next tick. (I watched this same rate-limit/sweep machinery carry #905 to merge.)
The fix — two layered, robust triggers on
pr-review-sweep.ymlworkflow_run: completedon the core CI workflowssweep-stuck-reviews.shgains a pure, unit-tested helperprs_from_workflow_run_eventthat derives the PR(s) from the event payload (.workflow_run.pull_requests[]), so the event path inspects one PR, not the whole fleet.cancel-in-progressconcurrency collapses the multi-workflow burst, so the sweep effectively runs once, after the last-finishing keyed workflow (the one most likely to observe green).REVIEW_REQUIRED + green + no-markergate (and the rate-limited-marker handling) decides in every path.Safety / cost notes for review
pr-review-trigger.yml(thin caller stub) or toreview-one-pr.sh's authoritative gating — only the sweep's when and candidate scoping.GH_PAT_WORKFLOWS(GITHUB_TOKEN can't start dispatched runs); unchanged.workflows:name list is best-effort (must match each workflow'sname:); it's not load-bearing because the cron backstops any miss.Tests
tests/test_sweep_stuck_reviews.batsfor theworkflow_runpath: ci-pending→green dispatches (scoped to the event), emptypull_requestsis 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 --severity=warning -xclean; workflow YAML validated.AGENTS.mdexception note updated to describe both triggers.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Chores
Tests