fix(review): resume requested reviews when checks become green

[neubig]

Problem

The live OSS PR reviewer on oss-agent-canvas (f36aba99-070d-40a9-9f77-627f9113347a) has only a GitHub review_requested / bot review.submitted event trigger. Extensions #651 correctly stops before launching an agent while current-head checks are pending or failing, but its message promises a scheduled retry that this deployment does not have. A request arriving during CI can therefore be left unreviewed after CI turns green unless a human removes and re-requests all-hands-bot. This undermines automatic coverage of non-draft PRs.

Desired behavior

Keep the cheap current-head eligibility gate from #651, then automatically revisit a pending requested review when its current-head checks reach a terminal green state. Reuse the existing reviewer automation and conversation-dispatch machinery. Choose the smallest reliable trigger mechanism supported by Canvas Automations; do not launch an agent or consume a slot while checks are pending or failing.

Acceptance criteria

• A native all-hands-bot review request made before CI finishes results in one review after the exact requested head's checks are green, without a second manual review request.
• A failing current-head check still prevents review, and a later fix on the same head (such as editing a PR description) or a new head can be reconsidered without a stale failure blocking it.
• Repeated check-completion events or scans do not create duplicate conversations or reviews; an already completed review is not restarted.
• The deployed OSS reviewer configuration uses the retry mechanism on all four repos, and the user-facing waiting comment names the actual retry condition instead of promising a nonexistent scan.
• Tests cover pending-to-green, failed-to-green on the same SHA, changed head, and duplicate notifications. Live Canvas evidence shows a review requested while a check is pending and completed after it becomes green.

Related: #644 and #651. The currently live reviewer remains on the tested #652 bundle; do not merge or deploy #651 until its repeated-check-run policy and this retry path are resolved.

---

OpenHands AI triage

The following comments and acceptance criteria were added by the OpenHands AI agent.

Triage

The maintainer resolved the earlier design questions, so the scope is now bounded and does not need a further decision. The reusable reviewer behavior belongs in this registry, reusing the existing skills/github-pr-reviewer worker and conversation-dispatch machinery, and stacking on #651 (which must merge first). The OSS deployment gets its retry path by adding a five-minute cron trigger to the same reviewer automation record, scanning open non-draft PRs that hold an outstanding all-hands-bot review request, while the explicit-request event path is retained for other deployments.

Non-goals: no new GitHub webhook event types or check_run parsing (Canvas Automations does not parse that payload today, and adding it belongs to OpenHands/automation); no second automation record for the OSS deployment; no deployment-specific code in OpenHands/extensions. Updating the live oss-agent-canvas automation to the cron trigger on all four repos and syncing OpenHands/oss-agent-canvas is validation and deployment work performed after the PR is live-tested, not part of this change. Neither PR is merged or deployed automatically.

Acceptance Criteria

• The github-pr-reviewer automation can run on a recurring cron trigger and, on each run, considers open non-draft PRs that carry an outstanding all-hands-bot review request, in addition to the existing trigger-label scan and the explicit-request event path.
• A native all-hands-bot review request made before CI finishes results in exactly one review after the requested head's checks become green, with no second manual review request.
• While the exact head has pending or failing checks, the run creates no review conversation and consumes no agent slot; it exits with a clear waiting/blocked disposition.
• A completed failing or cancelled check on the exact head still prevents review, and the head is reconsidered once checks pass.
• Current check state is determined from the latest run per logical check on the exact head, grouped by check name together with the reporting app identity and ordered deterministically by run start time and ID; a later successful run replaces an earlier failure on the same SHA, and a later pending rerun makes the head wait. Unrelated apps sharing a display name do not mask one another.
• Checks attributed to any other head do not block the current head, and a changed head is evaluated on its own checks.
• Repeated cron scans or duplicate notifications do not create duplicate conversations or reviews, and an already completed exact-head review is not restarted (existing keyed subject/delivery and completed-review dedupe reused).
• The user-facing waiting comment names the actual retry condition (the scheduled scan, or a new review request) rather than promising a mechanism the deployment does not have, and a moved head updates or replaces the previous gate comment instead of stacking a duplicate.
• Existing review verdict, maintainer-handoff, profile, and secret-handling behavior is unchanged, and no new secrets or elevated permissions are introduced.
• Automated tests cover pending-to-green, failed-to-green on the same SHA (latest-run-per-check), changed head, and duplicate notifications/scans.
• Live Canvas evidence demonstrates a review requested while a check is pending and completed after the check becomes green, and the documented SKILL.md/README.md retry contract matches the deployed cron behavior.

[neubig]

Decisions for triage:

1. Use the existing cron mode in Extensions. Make the same OSS reviewer automation run on a five-minute cron and scan open non-draft PRs with an outstanding all-hands-bot review request; keep the existing keyed conversation and completed-review dedupe. This avoids adding a second automation record or new GitHub webhook types. GitHub's current hooks on the four repos do not subscribe to check_run, and OpenHands/automation does not parse that payload today. The five-minute polling delay is acceptable. Keep the explicit-request event path in the extension for other deployments, but the OSS deployment can use cron.

2. Treat only the latest run of a logical check on the exact head as its current state, including a later pending rerun. A historical failed Validate PR description run must not block after a newer successful run on the same SHA. Group by check name and reporting app identity so unrelated apps with the same display name cannot mask one another; use the run's start time and ID for deterministic ordering. This resolves the policy question on fix(review): gate agent launch on exact-head checks #651 and lets the same-SHA acceptance criterion work.

3. Implement the reusable reviewer behavior in OpenHands/extensions, stacked on fix(review): gate agent launch on exact-head checks #651 with fix(review): gate agent launch on exact-head checks #651 merging first. Updating the live oss-agent-canvas automation to the cron trigger on all four repos and syncing it to OpenHands/oss-agent-canvas is validation/deployment work after the PR is live-tested; it does not belong as deployment-specific code in Extensions. Do not merge either PR automatically.

Please incorporate these decisions into the issue body and mark ready-for-dev if no other design gap remains.

[neubig]

@all-hands-bot please implement this ready-for-dev issue as a focused PR stacked on #651 using GitHub’s stacked PR relationship. Reuse the existing reviewer worker and one reviewer automation record: the OSS deployment should switch its event trigger to a five-minute cron scan, while the existing event handler stays usable in other deployments. Do not add a second automation record or backend machinery. #651 is receiving one small check-ordering fix, so update your base when that lands. Run the relevant tests and full CI, report live Canvas validation, and do not merge.

[all-hands-bot]

Implemented as a stacked PR: #654, based on fix/644-reject-failed-checks (the head branch of #651), so GitHub records the stack and it retargets to main when #651 merges. Not merged.

What I did

I reused the existing skills/github-pr-reviewer worker and one reviewer automation record. No second automation record, no new GitHub webhook types, no check_run parsing, and no deployment-specific code in the registry.

• _outstanding_review_request(pr) reads requested_reviewers off the /pulls list the scheduled scan already fetches. It is the live set, so an answered or withdrawn request simply drops out; open, non-draft PRs only.
• Scheduled mode now also considers any open, non-draft PR holding an outstanding all-hands-bot request, alongside the existing trigger-label path. Such a PR is keyed by its own review_requested event, so repeated scans produce the identical delivery ({event_id}:{sha}) and the existing keyed subject/delivery dedupe reuses one conversation and one review.
• The event path is unchanged and stays usable in other deployments — including the submitted-is-not-a-trigger rule and its completion handling.
• The waiting/blocked comment now names the real retry. A scheduled run proves a scan exists, so it says the next scan retries. An event-only run says to request all-hands-bot again instead of promising a scan it does not have.
• The eligibility gate, latest-run-per-logical-check ordering, keyed dedupe, completed-review short-circuit, verdict parsing, maintainer handoff, and secret handling are untouched.
• Bumped the catalog entry and setup.bundle.version 1.5.0 → 1.6.0, moved the catalog default schedule to */5 * * * *, regenerated automations/bundle-index.js / skills/index.js with npm run build, and documented the retry contract in SKILL.md / README.md.

#651's check-ordering fix has landed (branch head is now de8dd38), and my branch is based on exactly that commit, so the base is already current.

Tests and CI

• uv run pytest -q — 1010 passed, 23 skipped.
• PYTHONPATH=/workspace/repo uv run --group test pytest tests/ (the exact tests.yml command) — 1010 passed, 23 skipped.
• uv run pytest -q tests/test_github_reviewer_delivery.py ... tests/test_interface_manifest.py — 209 passed, 17 skipped.
• uv run python scripts/sync_extensions.py --check — clean (the pre-existing non-blocking issue-duplicate-checker coverage warning is unrelated).
• uv run python scripts/check_deprecations.py — clean.
• npm run build — no drift.

New regression tests cover pending-to-green across two scans, duplicate scans reusing one delivery key, a draft-with-request, an unlabeled+unrequested PR, the unchanged label path, and both event-path gate comments refusing to promise a scan.

One caveat on CI, worth knowing before #651 merges. On #654 GitHub only ran 3 checks (Validate PR description, and the two pr-title jobs). That is because tests.yml, check-extensions.yml, and workflow_sync are filtered with branches: ["*"], which does not match a base branch containing a slash (fix/644-reject-failed-checks). Those jobs did run on #651 because its base is main. So for this stacked PR I ran the tests.yml job's exact command locally (above) rather than relying on a green check that never fired. Once #651 merges and #654 retargets to main, the full suite will run on the PR itself.

Live Canvas validation

The Canvas automation service on this host was not reachable for a create/dispatch mutation, so I have no live Canvas run to report: 127.0.0.1:8001 returns 403 for the available key, and ${OPENHANDS_URL} resolves to an ERR_NGORK_3200 tunnel.

Instead I loaded the shipped bundle from automations/catalog/github-pr-reviewer/manifest.json and drove the real PullRequestReviewer scan against the live GitHub API with a read-only reviewer (no dispatch, no comment posted), across all four configured repos:

=== OpenHands/extensions ===      open PRs: 83   outstanding all-hands-bot requests: 0
=== OpenHands/OpenHands ===       open PRs: 434  outstanding all-hands-bot requests: 1
    #17584 draft=False head=abbbf032 state=green names=[]
=== OpenHands/software-agent-sdk === open PRs: 256  outstanding all-hands-bot requests: 1
    #5058 draft=False head=adfeb8fd state=blocked names=['Check OpenAPI Schema','agent-server-tests','pre-commit','sdk-tests']
=== OpenHands/automation ===      open PRs: 48   outstanding all-hands-bot requests: 1
    #374 draft=False head=27175d12 state=green names=[]

This exercises the new scan on live data: requested_reviewers is populated on real PRs, drafts are excluded, and each outstanding request resolves to a real exact head and a real gate disposition (green / blocked).

The remaining acceptance evidence — a review requested while a check is pending and completed after it goes green on a live deployment — needs the oss-agent-canvas automation switched to the cron trigger on all four repos. Per the issue's own non-goals that is validation/deployment work after this PR is live-tested, not part of the change, so I have not touched the live automation or OpenHands/oss-agent-canvas.

Open judgement call

Review verdicts on real bot reviews may not always end with the parseable verdict line, so like #651 this scan can occasionally re-dispatch a stable, keyed conversation (one slot). I chose spending one slot over silently skipping an outstanding request; say the word if you would rather it fail closed.

Generated by OpenHands AI on behalf of the user.