feat(review): review unrequested green PRs on the scheduled scan - #660
Conversation
e9be523 to
c01bac7
Compare
|
@all-hands-bot Live canary of #660 exposed a blocking scalability bug. The scheduled run started at 04:23 UTC and, before dispatching any reviewer, posted at least 99 managed CI-gate comments in OpenHands/OpenHands alone (example: OpenHands/OpenHands#16821 (comment)). It was still scanning after 10 minutes. I stopped and disabled the live reviewer to prevent a mass-comment/VM/API-rate incident. Please amend this PR so each scheduled run examines a bounded, fairly rotating window of unrequested PRs across all four repos, using the existing Automation KV facility to retain per-repository scan position. Explicit all-hands-bot requests/trigger labels should keep priority regardless of the window. Do not post gate comments for unrequested PRs that are merely red or pending; keep the managed gate comment for explicit requests. Only eligible green candidates should reach the bounded dispatch queue. Add focused tests for rotation, explicit priority, no unrequested gate-comment storm, and bounded GitHub API reads. Keep the change within this reviewer automation, and do not merge. |
23ee3e7 to
b55d263
Compare
|
@all-hands-bot When you restack #660 on updated #659, please keep the PR limited to the continuous unrequested-review behavior and its bounded scan fix. Remove the one-off |
The scheduled scan examined every open, non-draft PR. Classifying each unrequested head costs a review read plus the exact-head check and workflow reads, so one run read one list per PR across the largest repository and posted a managed gate comment for every red or pending head, which the live canary of #660 exposed as a blocking scalability bug. - The unrequested part of a scheduled scan is now a bounded, rotating window: at most SCAN_WINDOW (10) unrequested PRs per repository, starting where the previous scan stopped. The position is retained in the existing Automation KV store under a per-repository review-scan:{owner}__{repo} key, so successive scans rotate through the whole backlog instead of reading one pull request per open PR. With no KV store the position is kept in memory. - Explicit all-hands-bot review requests and trigger labels are never subject to the window: every explicit candidate is examined on every scan. - A gate stop on an unrequested head posts no managed comment, so a scan over a large backlog cannot storm the PRs with comments. The managed comment stays for explicit requests, which still get their blocked or waiting explanation. - Remove the one-off .pr canary script and bump the bundle to 1.9.0. Tests cover rotation and KV persistence, explicit priority over the window, the absence of unrequested gate comments, and the bounded pull-request reads.
c01bac7 to
ca83635
Compare
|
Restacked #660 onto the current #659 head ( What was wrongThe scheduled scan read every open, non-draft PR and, before the bounded drain, classified each one's exact head. Classification costs a review-list read plus the check-run and workflow-run reads, so one run spent its whole API budget in the largest repository, and every red or pending head got a managed gate comment. That is the 99-comment, 10-minute run the canary caught. What changedAll within
Tests added (focused, in
|
|
Done. #660 is restacked onto the current #659 head and limited to the continuous unrequested-review behavior plus its bounded scan fix. State
Canary harnessThe one-off Its live read-only results are retained in the PR description, as asked. Diff (only the reviewer automation)The bounded scan fix is the rotating, KV-backed window: Verification on the pushed head
DescriptionThe PR body still claimed the deleted canary was committed and described the workflow-run gate as part of this PR, so I corrected it in place: live results kept, canary references replaced with the bounded-window fix, and the base/head recorded. Nothing was pushed beyond the existing branch, and no merge was performed. Generated by OpenHands AI on behalf of the user. |
|
@all-hands-bot After extensions #659 is updated, update this stacked PR onto the current head of its base branch |
7609cf1 to
50fb986
Compare
Restacked onto the current #659 head (openhands/issue-656). This keeps the continuous unrequested green-PR discovery and the corrected base behavior it builds on: scheduled candidates gate on GitHub-required checks (falling back to every current-head check and workflow run when the required signal is unavailable), and an explicit all-hands-bot request bypasses the CI gate. The scheduled scan examined every open, non-draft PR. Classifying each unrequested head costs a review read plus the exact-head check and workflow reads, so one run read one list per PR across the largest repository and posted a managed gate comment for every red or pending head, which the live canary of #660 exposed as a blocking scalability bug. - The unrequested part of a scheduled scan is now a bounded, rotating window: at most SCAN_WINDOW (10) unrequested PRs per repository, starting where the previous scan stopped. The position is retained in the existing Automation KV store under a per-repository review-scan:{owner}__{repo} key, so successive scans rotate through the whole backlog instead of reading one pull request per open PR. With no KV store the position is kept in memory. - Explicit all-hands-bot review requests and trigger labels are never subject to the window: every explicit candidate is examined on every scan, and an explicit request still bypasses the CI gate. - A gate stop on an unrequested head posts no managed comment, so a scan over a large backlog cannot storm the PRs with comments. The managed comment stays for explicit requests and labeled heads, which still get their blocked or waiting explanation. - Remove the one-off .pr canary script and bump the catalog entry and bundle 1.7.0 -> 1.8.0. Tests cover rotation and KV persistence, explicit priority over the window, the absence of unrequested gate comments, and the bounded pull-request reads, alongside the base required-check and explicit-request-bypass tests.
ca83635 to
7100eb4
Compare
|
Restacked #660 onto the current #659 head and force-pushed the existing branch. Not merged. State
What the rebase changed#659 had moved on to
I also corrected a stale version skip the old stack carried: the catalog entry and bundle now bump Verification on the pushed head
The base gate regressions still pass unchanged, including Diff and descriptionThe shipped diff is limited to the reviewer automation: I updated the PR description to match: base/head, the Generated by OpenHands AI on behalf of the user. |
Restacked onto the current #659 head (openhands/issue-656). This keeps the continuous unrequested green-PR discovery and the corrected base behavior it builds on: scheduled candidates gate on GitHub-required checks (falling back to every current-head check and workflow run when the required signal is unavailable), and an explicit all-hands-bot request bypasses the CI gate. The scheduled scan examined every open, non-draft PR. Classifying each unrequested head costs a review read plus the exact-head check and workflow reads, so one run read one list per PR across the largest repository and posted a managed gate comment for every red or pending head, which the live canary of #660 exposed as a blocking scalability bug. - The unrequested part of a scheduled scan is now a bounded, rotating window: at most SCAN_WINDOW (10) unrequested PRs per repository, starting where the previous scan stopped. The position is retained in the existing Automation KV store under a per-repository review-scan:{owner}__{repo} key, so successive scans rotate through the whole backlog instead of reading one pull request per open PR. With no KV store the position is kept in memory. - Explicit all-hands-bot review requests and trigger labels are never subject to the window: every explicit candidate is examined on every scan, and an explicit request still bypasses the CI gate. - A gate stop on an unrequested head posts no managed comment, so a scan over a large backlog cannot storm the PRs with comments. The managed comment stays for explicit requests and labeled heads, which still get their blocked or waiting explanation. - Remove the one-off .pr canary script and bump the catalog entry and bundle 1.7.0 -> 1.8.0. Tests cover rotation and KV persistence, explicit priority over the window, the absence of unrequested gate comments, and the bounded pull-request reads, alongside the base required-check and explicit-request-bypass tests.
7100eb4 to
7957433
Compare
0f3a2df to
a8936f4
Compare
Restacked onto the current #659 head (openhands/issue-656). This keeps the continuous unrequested green-PR discovery and the corrected base behavior it builds on: scheduled candidates gate on GitHub-required checks (falling back to every current-head check and workflow run when the required signal is unavailable), and an explicit all-hands-bot request bypasses the CI gate. The scheduled scan examined every open, non-draft PR. Classifying each unrequested head costs a review read plus the exact-head check and workflow reads, so one run read one list per PR across the largest repository and posted a managed gate comment for every red or pending head, which the live canary of - The unrequested part of a scheduled scan is now a bounded, rotating window: at most SCAN_WINDOW (10) unrequested PRs per repository, starting where the previous scan stopped. The position is retained in the existing Automation KV store under a per-repository review-scan:{owner}__{repo} key, so successive scans rotate through the whole backlog instead of reading one pull request per open PR. With no KV store the position is kept in memory. - Explicit all-hands-bot review requests and trigger labels are never subject to the window: every explicit candidate is examined on every scan, and an explicit request still bypasses the CI gate. - A gate stop on an unrequested head posts no managed comment, so a scan over a large backlog cannot storm the PRs with comments. The managed comment stays for explicit requests and labeled heads, which still get their blocked or waiting explanation. - Remove the one-off .pr canary script and bump the catalog entry and bundle 1.7.0 -> 1.8.0. Tests cover rotation and KV persistence, explicit priority over the window, the absence of unrequested gate comments, and the bounded pull-request reads, alongside the base required-check and explicit-request-bypass tests.
7957433 to
dcea1ae
Compare
|
🚀 Released in v0.25.0. |
3 participants
HUMAN:
This is a focused change to the existing scheduled reviewer automation, stacked on #656 (PR #659). It adds the continuous unrequested-PR scan and its bounded, rotating scan window; no host-local script or second automation is added. The evidence below is agent-run focused tests plus the read-only live API reads captured before the one-off canary harness was removed from the tree; I intentionally left the human-test checkbox unchecked because no human exercised the change.
AGENT:
Why
The scheduled GitHub PR reviewer retries outstanding
all-hands-botrequests (#654) and bounds how many new conversations one scan may start (#656). Once those requests are exhausted the automation stops making progress even while open, non-draft PRs with passing CI still lack a review on their current head. A one-off batch of requests would not cover later PRs.This PR makes the existing scheduled reviewer continuously discover eligible CI-passing PRs in its configured repositories and review them at a bounded pace, reusing the reviewer's scan, gate, keyed delivery, native review, and maintainer handoff. No separate daemon or automation.
The base #659 carries the corrected exact-head gate this PR builds on: scheduled candidates are classified on the GitHub-required checks only (read through
isRequired, falling back to every current-head check run and workflow run when the required signal is unavailable or empty), and an explicitall-hands-botrequest bypasses the CI gate entirely. This PR does not re-add or change that logic; it preserves both behaviors.Summary
skills/github-pr-reviewer/scripts/worker.py— in scheduled mode the scan now also considers open, non-draft PRs whose current head carries no completed review by the reviewer account, even with no trigger label and no request. The candidate filter is the exact negative of the completion handler's predicate, so a review the scan starts and one it finds already present are one set; an already-reviewed head is reconciled (verdict parsing and maintainer handoff) instead of restarted. Unrequested heads use the stablescan:{repository}:{number}:{head}delivery key, so repeated scans create neither a duplicate conversation nor a duplicate native review, and a changed head is eligible again under its new SHA.SCAN_WINDOW(10) unrequested PRs per repository, starting where the previous scan stopped. The position is retained in the existing Automation KV facility — the sameAUTOMATION_KV_TOKEN/AUTOMATION_API_URLthatmain.pyalready uses for review state — under a per-repository keyreview-scan:{owner}__{repo}({"cursor": N}). The window wraps at the end of the backlog, so the whole backlog is covered over successive scans. No new secret, store, or permission. When the KV store is unavailable (a local run or the tests) the position is kept in memory, and a KV read/write failure degrades to the in-memory cursor rather than aborting the scan.all-hands-botreview request or a trigger label is never subject to the window: every explicit candidate is examined on every scan, whatever the stored position is, and explicit candidates drain first through the existingpriorityordering. An explicit request still bypasses the CI gate (requested=True), so it dispatches on a red or pending head without a gate comment._classify_check_runs(pr)→required_check_contexts→isRequired), falling back to every current-head check and workflow run when the required set cannot be read or is empty._gate_head(..., explain=False)for unrequested candidates, so a merely red or pending unrequested PR is skipped without starting an agent and without posting a managed gate comment. The blocked/waiting managed comment is kept for explicit requests and labeled heads.max_new_per_runbound (default 2, configurable) orders candidates explicitly: an explicit request or trigger label first, then the oldest candidate — the request time for a requested PR, the PR's own creation time (oldest first) for an unrequested one — with repository and PR number as stable tie-breakers. An ineligible candidate (blocked or waiting head) consumes no slot and does not block the candidates behind it.event: COMMENTreview that keeps the approved verdict, instead of being skipped because GitHub ignores a self-review request.SKILL.md,README.md,references/state-schema.md,automations/catalog/github-pr-reviewer/manifest.json,tests/fixtures/automations/github-pr-reviewer.json— document the unrequested scan, the bounded rotating window and its KV cursor, the delivery key, and the per-run maximum; bump the catalog entry and bundle version1.7.0→1.8.0; regenerateautomations/bundle-index.jsandskills/index.jswithnpm run build.No new secret, token scope, or GitHub permission is required beyond what the current reviewer uses. No change to the review prompt, verdict parsing, maintainer handoff, or secret selection.
Stacking
Base is
openhands/issue-656, the head branch of #656 (PR #659, current head50fb986), so GitHub records this as a stacked PR and it will retarget tomainwhen the stack merges. #659 must merge first. This branch was rebased onto the updated #659 head: the earlier commit that added the read-only canary harness and its docstring fix were dropped, and the conflict in the reviewer's gate was resolved to keep the base's required-check gating and explicit-request bypass alongside the new unrequested scan.Issue Number
Fixes #658
How to Test
uv run --group test pytest -q tests/test_github_reviewer_delivery.py— expect 86 passed.uv run --group test pytest -q tests/test_github_reviewer_delivery.py tests/test_github_automation_foundation.py tests/test_automation_setup.py— expect 230 passed, 17 skipped.uv run --group test pytest -q— expect 1059 passed, 23 skipped.uv run --group test python scripts/sync_extensions.py --check— expect clean (the pre-existing non-blockingissue-duplicate-checkercoverage warning is unrelated).npm run build— expect no drift inautomations/bundle-index.jsorskills/index.js.Focused tests added for the new behavior in
tests/test_github_reviewer_delivery.py, driving the real shippedworker.pyentrypoint through the catalog bundle helper and the shippedrun_scancontrol flow:test_unrequested_scan_reviews_a_green_pr_with_no_request_or_label— the core new behavior: an unrequested green PR starts a review.test_scan_examines_a_bounded_rotating_window_of_unrequested_prs— rotation across successive scans, including wrap-around.test_the_scan_position_is_persisted_per_repository_in_the_kv_store— the cursor is written underreview-scan:owner__one.test_explicit_requests_are_examined_regardless_of_the_rotation_window— an explicit request is examined even when the window sits elsewhere.test_unrequested_red_and_pending_prs_post_no_gate_comments— no unrequested gate comment, while the green unrequested head still reaches the queue.test_scan_reads_a_bounded_number_of_pull_requests_per_repository— one list page plus exactlySCAN_WINDOWfull PR reads over a 40-PR backlog.test_an_explicit_request_still_gets_its_managed_gate_comment— the explicit-request gate comment is preserved.test_unrequested_scan_does_not_duplicate_across_repeated_scans,test_unrequested_scan_reviews_a_changed_head_again_under_a_new_key— the stable key dedupes a repeat scan and re-reviews a changed head once.test_unrequested_scan_bound_spans_repositories_with_explicit_priority,test_unrequested_scan_reviews_the_oldest_eligible_prs_under_the_cap,test_unrequested_scan_still_gates_a_blocked_pr_past_the_cap— the global cap, oldest-first order, and a blocked candidate that consumes no slot.test_unrequested_scan_reconciles_a_completed_review_and_hands_off,test_unrequested_scan_marks_a_self_authored_pr_for_the_comment_verdict— reconciliation plus handoff, and the bot-authoredCOMMENT-verdict path.test_unrequested_scan_blocks_a_failed_zero_job_workflow,test_unrequested_scan_waits_on_a_pending_workflow— the gate on an unrequested head through the workflow-run path.The base #659 gate regressions continue to pass unchanged, including
test_reviewer_explicit_request_bypasses_the_ci_gate,test_reviewer_ignores_an_optional_zero_job_workflow_on_a_green_required_head,test_reviewer_blocks_when_a_required_check_fails,test_reviewer_waits_for_a_pending_required_check, andtest_reviewer_falls_back_to_every_check_when_required_signal_is_unavailable.Live evidence (read-only, no agent started, no comment posted)
The run below was performed with a one-off read-only harness that has since been removed from the tree (it was development-only evidence, never part of the shipped bundle); the numbers are retained here.
GITHUB_TOKEN_REVIEWauthenticated asall-hands-bot. I loaded the shipped bundle fromautomations/catalog/github-pr-reviewer/manifest.jsonand drove the realPullRequestReviewer.run()scheduled path and the real shared-intake drain against the live GitHub API. The dispatcher recorded what would have been delivered and the gate-comment upsert was recorded instead of written, so the run is read-only. The live candidate list was bounded to the oldest six non-draft PRs per repository (a full scan would issue one/reviewsread per PR); everything after candidate selection is the shipped code.A scheduled scan over all four configured repositories,
max_new_per_run=2:The real drain stopping at the bound on live data across more than one repository (only the gate flag is forced eligible; ordering, keys, and the drain are the shipped code):
The shipped exact-head classification against live heads confirms the gate, including the
#426reproduction head whose check-run rollup is green while three zero-job workflows failed:Video/Screenshots
Not applicable: this changes automation worker dispatch logic, not a GUI.
Notes
.pr/artifacts: the read-only canary harness was removed, and fix(review): bound scheduled PR review intake across repositories #659's own.pr/canary_bounded_intake.pyis not present either.Generated by OpenHands AI on behalf of the user.