Repository navigation
fix(review): bound scheduled PR review intake across repositories - #659
Conversation
014659e to
fbfece9
Compare
|
@all-hands-bot #659 now conflicts with its updated base #654 (head fbfece9). Please restack this focused bounded-intake PR onto that head, preserve its behavior, run full CI, and push. #660 is stacked on #659, so keep its dependent commits out of this PR and do not merge. The clean combined canary branch in OpenHands/extensions at factory/reviewer-continuous-canary may help compare the intended result. |
23ee3e7 to
b55d263
Compare
|
@all-hands-bot The restack is mergeable and CI green. Please remove |
|
Restacked #659 onto #654's head What the restack contains
The old head's first two commits ( One conflict resolution, and it is the one the canary branch impliesThree files conflicted: The one substantive adjustment was in Behavior preservedSame bound, same semantics as before: at most CI
GitHub CI on the new head: I did not merge, and did not touch #660. Generated by OpenHands AI on behalf of the user. |
|
Done — the canary harness is out of the PR and the checks are green. Change: pushed
Live result kept. The PR description still carries the recorded live-API evidence: the read-only scan over the four repositories ( One correction while I was in there. The Checks rerun on
Locally I also confirmed Not merging, as asked. Two things for your awareness, neither of which I changed:
Generated by OpenHands AI on behalf of the user. |
|
@all-hands-bot After extensions #654 is updated, update this stacked PR onto the current head of its base branch |
fbfece9 to
d63d9d7
Compare
A scheduled scan resumed every outstanding all-hands-bot request it found and started one conversation for each, so a first scan over the OSS backlog could start an agent for hundreds of eligible pull requests at once and exhaust the deployment. The scan now starts at most `max_new_per_run` new review conversations, defaulting to 2, counted across every configured repository rather than reset per repository. Delivery reuses the existing machinery: eligible candidates are collected with the request event and head SHA #654 already keys deliveries on, ordered by the oldest outstanding request and then by repository and pull-request number, and started through the same `dispatcher.deliver`. The bound counts conversations a scan starts, so a deduplicated or already-running delivery reuses its runtime and consumes no slot, and a candidate whose exact-head checks are pending or failing posts its gate disposition and consumes none either. Reaching the bound still evaluates the remaining candidates and their gate comments, and a dispatch that raises is reported without aborting the scan. The explicit reviewer-request event path and the trigger-label scan are unchanged. Wires the bound through the worker's rendered config with the `max_new_per_run` key the other automations already use, documents it in SKILL.md and README.md, bumps the catalog entry and bundle 1.6.0 -> 1.7.0, and regenerates the catalogs. Part of #656. Stacked on #654. Co-authored-by: openhands <openhands@all-hands.dev> (cherry picked from commit cb1b625)
Loads the shipped catalog bundle and drives the real scheduled scan and shared-intake drain against the live GitHub API. It starts no agent and posts no comment: the dispatcher and the gate comment upsert record what would have happened, so the canary is read-only. Development-only, kept in .pr/. Co-authored-by: openhands <openhands@all-hands.dev> (cherry picked from commit 9396c24)
The 197-line read-only canary was evidence for #656, not part of the shipped automation, so it does not belong in this focused per-scan bound PR. The live result stays recorded in the PR description; the harness itself is preserved on factory/reviewer-continuous-canary. Co-authored-by: openhands <openhands@all-hands.dev>
7609cf1 to
50fb986
Compare
|
Rebased #659 onto the current head of What the base move changed#654 was force-pushed while I was working: its head went
I rebased the branch's three commits onto How the two behaviors were reconciled
The Three files conflicted:
The docs now describe both behaviors: the base's "Gates each scheduled review on the current head's GitHub-required checks … an explicit Validation
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.
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.
* fix(review): bound scheduled PR review intake across repositories A scheduled scan resumed every outstanding all-hands-bot request it found and started one conversation for each, so a first scan over the OSS backlog could start an agent for hundreds of eligible pull requests at once and exhaust the deployment. The scan now starts at most `max_new_per_run` new review conversations, defaulting to 2, counted across every configured repository rather than reset per repository. Delivery reuses the existing machinery: eligible candidates are collected with the request event and head SHA #654 already keys deliveries on, ordered by the oldest outstanding request and then by repository and pull-request number, and started through the same `dispatcher.deliver`. The bound counts conversations a scan starts, so a deduplicated or already-running delivery reuses its runtime and consumes no slot, and a candidate whose exact-head checks are pending or failing posts its gate disposition and consumes none either. Reaching the bound still evaluates the remaining candidates and their gate comments, and a dispatch that raises is reported without aborting the scan. The explicit reviewer-request event path and the trigger-label scan are unchanged. Wires the bound through the worker's rendered config with the `max_new_per_run` key the other automations already use, documents it in SKILL.md and README.md, bumps the catalog entry and bundle 1.6.0 -> 1.7.0, and regenerates the catalogs. Part of #656. Stacked on #654. Co-authored-by: openhands <openhands@all-hands.dev> (cherry picked from commit cb1b625) * chore(review): add read-only bounded-intake canary for #656 Loads the shipped catalog bundle and drives the real scheduled scan and shared-intake drain against the live GitHub API. It starts no agent and posts no comment: the dispatcher and the gate comment upsert record what would have happened, so the canary is read-only. Development-only, kept in .pr/. Co-authored-by: openhands <openhands@all-hands.dev> (cherry picked from commit 9396c24) * chore(review): drop the one-off bounded-intake canary harness The 197-line read-only canary was evidence for #656, not part of the shipped automation, so it does not belong in this focused per-scan bound PR. The live result stays recorded in the PR description; the harness itself is preserved on factory/reviewer-continuous-canary. Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: all-hands-bot <all-hands-bot@users.noreply.github.com> Co-authored-by: openhands <openhands@all-hands.dev>
* fix(review): resume requested reviews on a scheduled check scan The OSS reviewer's only trigger was the GitHub review_requested event. #651 stopped dispatching an agent while the exact head's checks were pending or failing, and its waiting comment promised a scheduled retry that deployment did not have, so a request arriving during CI could be left unreviewed. Reuse the existing worker and one automation record. In scheduled mode the scan now also considers every open, non-draft PR that still holds an outstanding all-hands-bot review request, keyed by that request's own review_requested event so repeated scans reuse one conversation and one review. The explicit request event path is unchanged for event-only deployments. The waiting and blocked comments now name the retry the deployment actually has: a scheduled scan where a cron trigger exists, and another review request where only the event trigger does. Bumps the catalog entry and bundle to 1.6.0, documents the retry contract, and adds scheduled-scan, duplicate-scan, draft, and gate-message tests. Part of #653. Stacked on #651. Co-authored-by: openhands <openhands@all-hands.dev> * fix(review): reword the managed gate comment across a trigger change A pending PR first gated by an event-only deployment kept its 'request all-hands-bot again' comment after the automation was switched to a cron scan, because the gate returned early on a matching state:sha marker without comparing the body. The comment therefore named a retry that deployment no longer had. The gate now compares the full managed body for the same marker and rewrites the comment in place when it changed, so an event-only comment becomes the scheduled one once the scan exists and an identical body stays a no-op. No second comment is created. The event-only wording also makes the retry explicit: the outstanding request must be removed and re-requested, since GitHub will not accept a second request for a reviewer who is already requested. Adds a focused event-then-cron regression plus a stale-body rewrite test, and documents the in-place reword in SKILL.md and README.md. Part of #653. Co-authored-by: openhands <openhands@all-hands.dev> * fix(review): bound scheduled PR review intake across repositories (#659) * fix(review): bound scheduled PR review intake across repositories A scheduled scan resumed every outstanding all-hands-bot request it found and started one conversation for each, so a first scan over the OSS backlog could start an agent for hundreds of eligible pull requests at once and exhaust the deployment. The scan now starts at most `max_new_per_run` new review conversations, defaulting to 2, counted across every configured repository rather than reset per repository. Delivery reuses the existing machinery: eligible candidates are collected with the request event and head SHA #654 already keys deliveries on, ordered by the oldest outstanding request and then by repository and pull-request number, and started through the same `dispatcher.deliver`. The bound counts conversations a scan starts, so a deduplicated or already-running delivery reuses its runtime and consumes no slot, and a candidate whose exact-head checks are pending or failing posts its gate disposition and consumes none either. Reaching the bound still evaluates the remaining candidates and their gate comments, and a dispatch that raises is reported without aborting the scan. The explicit reviewer-request event path and the trigger-label scan are unchanged. Wires the bound through the worker's rendered config with the `max_new_per_run` key the other automations already use, documents it in SKILL.md and README.md, bumps the catalog entry and bundle 1.6.0 -> 1.7.0, and regenerates the catalogs. Part of #656. Stacked on #654. Co-authored-by: openhands <openhands@all-hands.dev> (cherry picked from commit cb1b625) * chore(review): add read-only bounded-intake canary for #656 Loads the shipped catalog bundle and drives the real scheduled scan and shared-intake drain against the live GitHub API. It starts no agent and posts no comment: the dispatcher and the gate comment upsert record what would have happened, so the canary is read-only. Development-only, kept in .pr/. Co-authored-by: openhands <openhands@all-hands.dev> (cherry picked from commit 9396c24) * chore(review): drop the one-off bounded-intake canary harness The 197-line read-only canary was evidence for #656, not part of the shipped automation, so it does not belong in this focused per-scan bound PR. The live result stays recorded in the PR description; the harness itself is preserved on factory/reviewer-continuous-canary. Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: all-hands-bot <all-hands-bot@users.noreply.github.com> Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: openhands <openhands@all-hands.dev> Co-authored-by: all-hands-bot <all-hands-bot@users.noreply.github.com>
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.
fix(review): bound the unrequested scan to a rotating KV-backed window 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. Co-authored-by: openhands <openhands@all-hands.dev>
3 participants
Why
The scheduled reviewer-request scan from #654 drains every outstanding
all-hands-botrequest it finds, so one scan can start a conversation for every eligible PR. The four main OSS repositories currently have hundreds of open non-draft PRs lacking a review on their current head. Requesting all of them at once overloaded the 15 GiB OSS Agent Canvas VM: four simultaneous agent jobs plus retained Docker runtimes caused an OOM restart on 2026-09-22.Summary
max_new_per_run, defaulting to 2 and counted across every configured repository rather than reset per repository. Eligible candidates are collected with the reviewer-request event and head SHA fix(review): resume requested reviews on a scheduled check scan #654 already keys deliveries on, ordered by the oldest outstanding request and then by repository and pull-request number, and started through the existingdispatcher.deliver. The bound counts conversations a scan starts, so a delivery that only deduplicates or reports an already-running conversation consumes no slot, and a candidate whose exact-head checks are pending or failing posts its gate disposition and consumes none either.review_requestedevent path and the trigger-label scan keep their current unbounded behavior.max_new_per_runkey the other automations already use, document it inSKILL.md/README.md, and bump the catalog entry and bundle1.6.0→1.7.0with regenerated catalogs.Details:
skills/github-pr-reviewer/scripts/worker.pyReviewIntakeholds the per-scan budget and a queue of eligible candidates.drain()sorts by(created_at, repository, number)and starts at mostmax_new_per_runof them, reusingdispatcher.deliverand its keyed dedupe. A dispatch that raises is collected and reported after the drain so later candidates still get their turn.run_scan(dispatcher)is the shipped entrypoint: it creates one sharedReviewIntake, runs every configured repository, then drains once. The drain runs even when a repository raises, and the first failure is what the scan reports.PullRequestReviewer.intakereturns the shared scan intake whenrun_scanset one, otherwise a private lazily-created intake, so a standalonerun()still bounds its own scan. In scheduled mode a green candidate is registered with the intake; in event mode it dispatches immediately and is never bounded.skills/github-pr-reviewer/scripts/main.py—MAX_NEW_PER_RUN = 2default plusmax_new_per_runconfig parsing, rejecting a boolean (a Pythonboolis anint) and any value below 1.automations/catalog/github-pr-reviewer/manifest.json—maxNewPerRunsetup field (min 1, default 2), version bump, description/example/message update;automations/bundle-index.js,skills/index.js, andtests/fixtures/automations/github-pr-reviewer.jsonregenerated withnpm run build.Stacking
Base is
fix/653-scheduled-review-retry, the head branch of #654, so GitHub records this as a stacked PR and it will retarget tomainwhen #654 merges. #651 and #654 must merge first. This PR resumes the branchopenhands/issue-656after the earlier resolver pushed it but timed out before opening a PR; the unrelatedintegrations/catalog/granola.jsonandintegrations/catalog-index.jschanges that had been picked up frommainwere removed, so the diff below is only the bounded intake and its tests/docs.Issue Number
Part of #656
How to Test
uv run pytest -q tests/test_github_reviewer_delivery.py— expect 62 passed.uv run pytest -q— expect 1032 passed, 23 skipped (the basefbfece9runs 1020 passed, 23 skipped; the difference is this PR's new cases).uv run 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.Live Canvas / live-API validation
The Canvas automation service on this host was not reachable for a create/dispatch mutation (
127.0.0.1:8001returned403for the available key), so there is no live Canvas scheduled run to report. A read-only one-off harness instead loaded the shipped bundle fromautomations/catalog/github-pr-reviewer/manifest.jsonand drove the realPullRequestReviewer.run()scheduled path plus 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 no agent started and no comment was posted. That harness was evidence for #656, not part of the shipped automation, so it is not in this PR; the live result it produced is recorded below. The harness itself remains available onfactory/reviewer-continuous-canary.The live scan is honest about the gate, so only one candidate was green at the time of that run and the bound was not the limiting factor on live data. A second pass over the same live PRs, request events, head SHAs, and repositories forced each outstanding request eligible so the real drain is shown stopping at the bound on real data (only the gate flag is overridden; ordering, delivery keys, and the drain are the shipped code):
That is the bounded intake: one scan starts no more than the configured number of conversations, in oldest-request order, and a later scan reaches the remainder. The remaining acceptance evidence — a review requested while a check is pending and completed after it goes green on a live deployment — requires the
oss-agent-canvasautomation to switch to the cron trigger, which is post-live-test deployment work and out of scope here.Video/Screenshots
Not applicable: this changes automation worker dispatch logic, not a GUI.
Notes
RuntimeErrorafter the drain, so a flaky agent server does not silently drop the rest of the backlog.Generated by OpenHands AI on behalf of the user.