Repository navigation
fix(pr-review): do not restart completed reviews on delayed submitted events - #647
Conversation
A pull_request_review.submitted delivery is created after the review it reports, so comparing that review's submitted_at against the selected review_requested timestamp rejected the very review the event announced. The submitted event then fell through without completing the review, so the configured maintainer handoff never ran for it. Accept the exact review a submitted delivery names (matched by id) regardless of timing, and evaluate a submitted delivery even when no review_requested issue event is in the window. A submitted review still never acts as a fresh trigger, so a later explicit review_requested event starts a new exact-head review.
|
@all-hands-bot Please update this PR description so the repository's Validate PR description check passes. Preserve the substantive problem/change/test evidence, use the required HUMAN/AGENT markers and section headings, do not claim human testing, and then verify the check is green. |
|
Done — the PR description is updated and the What the check was failing on: the PR body used What I changed in the description:
Verification: I ran the base-SHA validator locally against the new body first ( Generated by OpenHands AI on behalf of the user. |
smolpaws
left a comment
smolpaws
left a comment
There was a problem hiding this comment.
I am an AI agent helping my human, Engel Nyst (@enyst), with repository maintenance.
Taste: good. Reviewed head 71b5028. No material correctness, security, compatibility, or acceptance-criterion findings.
Acceptance criteria for #646:
- Submitted events select the reported review by ID while retaining the bot-author and current-head checks, including when no request event is available.
- Decisive completion retains the configured maintainer handoff and no-roster behavior; non-decisive and wrong-head submissions do not dispatch work or hand off.
- Later explicit review requests can dispatch a new review, and scheduled label behavior remains covered.
- Generated automation bundle matches its source.
Validation: independently ran python -m pytest -q tests/test_github_reviewer_delivery.py tests/test_maintainer_handoff.py in a fresh test environment: 29 passed. npm run build:automations produced no tracked diff; scripts/sync_extensions.py --check passed with the existing non-blocking marketplace coverage warning. Current-head test and extension checks passed, and the later successful PR-description check supersedes its earlier failure. Tests exercise the shipped worker with mocked GitHub/dispatch boundaries; I did not perform a live deployment or webhook test.
[RISK ASSESSMENT]
[Overall PR] Risk Assessment: 🟢 LOW. This is a bounded completion-selection fix using existing identity checks and handoff behavior, with no dependency or runtime-contract changes.
Architectural insight: the submitted review's identity supplies the completion signal, while explicit review requests retain timestamp-based selection for new work.
✅ APPROVED
GitHub lists every run for a commit, so a check re-run after a fix kept contributing its superseded failure forever. Group check runs by name and reporting app identity, pick the latest by start time and then run ID, and classify only that run. A later queued or in-progress re-run supersedes an earlier success, so the head waits instead of being reviewed. Adds regression tests for the live #647/#649 repeated Validate PR description runs, the start-time tie broken by run ID, a newer pending re-run, and same-name checks from different apps. Co-authored-by: openhands <openhands@all-hands.dev>
GitHub lists every run for a commit, so a check re-run after a fix kept contributing its superseded failure forever. Group check runs by name and reporting app identity, pick the latest by start time and then run ID, and classify only that run. A later queued or in-progress re-run supersedes an earlier success, so the head waits instead of being reviewed. Adds regression tests for the live #647/#649 repeated Validate PR description runs, the start-time tie broken by run ID, a newer pending re-run, and same-name checks from different apps. Co-authored-by: openhands <openhands@all-hands.dev>
* fix(review): reject failed required checks before launching an agent The reviewer dispatched an agent conversation for every eligible trigger without reading the current head's check state, so a head with an already failing required check still spent an LLM slot (OpenHands/OpenHands#17544 created conversation f46343fd despite a failing Validate PR description). Add a deterministic head-eligibility gate in front of dispatcher.deliver: - GitHubRepository.check_runs(sha) reads the check runs the reviewer's own token can already see, with no ruleset or branch-protection access. - A completed run on the exact head blocks unless its conclusion is success, neutral, or skipped, so an unknown conclusion fails closed. - A queued or in_progress run exits as waiting on checks without holding a slot to poll, and never approves. - Runs attributed to another head are ignored. - The trigger is not consumed: the next scan or explicit review request starts the review once the head is green. - One explanation is left per head, upserted through a marker carrying the head SHA and gate category, and an equivalent repository workflow remediation comment is not duplicated. Bump the bundle version to 1.5.0 and regenerate the catalog artifacts. Co-authored-by: openhands <openhands@all-hands.dev> * fix(review): trust only owned gate comments and matching dedupe Only treat a marked comment as managed when the configured reviewer account authored it, so a marker placed by a PR author can neither suppress the gate explanation nor be PATCHed. Narrow the workflow dedupe to a disclosure comment that names the same current-head checks. Rename the blocked heading to current-head checks. Add narrow regression tests for the ownership and dedupe cases. * fix(review): gate on the latest run of each exact-head check GitHub lists every run for a commit, so a check re-run after a fix kept contributing its superseded failure forever. Group check runs by name and reporting app identity, pick the latest by start time and then run ID, and classify only that run. A later queued or in-progress re-run supersedes an earlier success, so the head waits instead of being reviewed. Adds regression tests for the live #647/#649 repeated Validate PR description runs, the start-time tie broken by run ID, a newer pending re-run, and same-name checks from different apps. Co-authored-by: openhands <openhands@all-hands.dev> * fix(review): order a queued check run without a start time by its run ID A queued or requested check run can exist before GitHub sets started_at, which the API reports as null. Ordering used the empty string for a missing start time, so an earlier completed success with a timestamp outranked the newer queued rerun and the reviewer could launch while the rerun was still pending. Fall back to the run ID, which increases monotonically, whenever a start time is absent. Co-authored-by: openhands <openhands@all-hands.dev> * fix(review): order exact-head check runs by run ID The `~{run_id}` fallback sorted a run without a `started_at` past every real timestamp, so an older queued/cancelled run could outrank a newer successful run on the same check and keep a head blocked or waiting. The check-run ID is the monotonic creation sequence, so make it the primary order key and use the start time only as a tie-break. Adds the reverse regression alongside the existing null-start case. * fix(review): gate on current-head workflow runs too The head-eligibility gate read only check runs, so a workflow that failed before creating any check run left the commit's check-run rollup green and the reviewer launched for red CI. On #426 head 41ffb9d, `Tests`, `Check Extensions`, and `Deprecation deadlines` failed with zero jobs, so only the green `pr-title` checks were visible; the reviewer spent an agent slot before reporting the failures, and `gh pr checks` omitted the zero-job runs as well. Read the current head's Actions workflow runs as well as its check runs: - GitHubRepository.workflow_runs(sha) reads `/actions/runs?head_sha=<sha>`, which answers with an object and so paginates manually like check_runs. - A workflow run whose check suite already reported check runs is left to those runs, so a workflow is never counted twice. Only a run whose suite reported no check runs - a workflow-level failure, or a `pull_request` run whose jobs never started - is added to the gate. - Workflow runs get the same blocked/waiting behavior and the same current-head filter as check runs, and the latest run of each workflow (name + workflow ID, by run ID then start time) supersedes its superseded attempts. - The two grouping paths share one `_latest_by_group` helper and the one existing classifier and comment machinery. Co-authored-by: openhands <openhands@all-hands.dev> * fix(review): gate scheduled discovery on required checks only The live deployment exposed two over-broad behaviors in the exact-head eligibility gate. OpenHands/OpenHands#17200 passes `test-and-build (ubuntu)`, its only GitHub-required check, while the optional `release ready` workflow has a zero-job `startup_failure`. Classifying every check and workflow run treated that optional failure as blocking, so scheduled discovery paused a mergeable head. An explicit `all-hands-bot` review request is the intake-policy exception: the caller asked for that head by name, so it must dispatch even when required CI is red or pending. - GitHubRepository.required_check_contexts(number) adds one GraphQL query that returns the head's status check rollup and keeps only the contexts isRequired(pullRequestNumber:) marks required. That field is merge-policy source of truth and is pull-request-scoped, so it is correct for a stacked PR whose symbolic base branch has no rules of its own. - Scheduled classification uses only those required contexts: a required check run by name, a required commit status by context. An optional failure cannot block. A required context with no current-head run is expected, so it waits. - When the required signal cannot be read or is empty the gate falls back to every current-head check run and workflow run, so a red head still blocks. This keeps the #426 zero-job shape blocked: its required checks never started, isRequired returns no node, and the empty set falls back. - _gate_head(pr, requested=True) returns green without reading checks and without a gate comment, and run() passes requested=event_mode. Exact-head filtering, latest-run supersession, fail-closed conclusions, the comment marker and its ownership/dedupe rules, and the conversation dispatch path are unchanged. Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review of head 71b5028
Scope: This is the reusable skills/github-pr-reviewer extension in its owning repository (per AGENTS.md, reusable skills/automations live in OpenHands/extensions; scheduling, webhooks, and dispatch lifecycle belong in OpenHands/automation). The change does not cross that boundary, so it passes the scope gate.
What the change does: _finish_completed_review() previously required an exact-head bot review whose submitted_at was later than the selected review_requested trigger's created_at. A pull_request_review.submitted delivery is created after the review it announces, so that review was rejected and the maintainer handoff never ran. The fix threads the event's review object through and matches it by id regardless of timing, and also evaluates a submitted delivery when no review_requested event is in the window.
Findings: No material correctness, security, or design issues found.
- The new
_review_completes_request()keeps the exact-head and bot-author filters in_finish_completed_review(), so the id match only broadens the time window; it cannot accept a wrong-head or non-bot review. - The
trigger is Nonepath now substitutes an empty trigger only when asubmittedreview is present. Withtriggered_at == ""and no id included in the delivery,_review_completes_request()returnsFalse, which is the conservative direction (no false completion). The priorcontinuefor non-submitted runs is preserved. - The retained
submittedguard means a non-decisive or superseded-head submission neither dispatches nor hands off, and a later explicitreview_requestedstill dispatches a new review (deliverykeyed on the new trigger id). Scheduled label mode is untouched. - The maintainer handoff remains idempotent:
request_maintainer_review()parses the roster, returns early if a roster member is already requested or has a standing head review, and is a no-op with no roster, so repeatedsubmitteddeliveries converge. automations/bundle-index.jsis regenerated: I re-rannpm run build:automationsanduv run python scripts/sync_extensions.py --checkon the checked-out head and confirmed no drift (only the pre-existing non-blocking marketplace coverage warning for./plugins/issue-duplicate-checker).
Verification: Head is 71b5028ec1d3f7cd9b57760776270823532473de; issue #646 is open with ready-for-dev and priority:high. uv run pytest -q tests/test_github_reviewer_delivery.py tests/test_maintainer_handoff.py passes (29 passed). The current-head GitHub Actions results are green for test, check, sync-extensions, sync-sdk-skill, validate-claude-code, check-pr-artifacts, and pr-title; the Validate PR description run failed once at 13:04 but a later re-run at 13:18 succeeded, so there is no open failure on this head. The new tests exercise the shipped worker: delayed submitted-event completion with handoff, completion with no request event, the no-roster no-op, a subsequent explicit review_requested dispatch, and the non-decisive / superseded-head guards. I did not perform a live webhook deployment.
✅ APPROVED
Main reworked worker.py since this branch was opened. Take main's version here; the next commit re-applies the fix on top of it.
…nt main Port the fix for #646 onto the restructured worker. A submitted delivery now passes the review it reports into both completion paths (the already-reviewed-head path added in #699 and the regular path), which match that review by id regardless of timing, and a submitted delivery with no visible review_requested event is still reconciled instead of skipped. Unrequested scheduled scans keep the any-head-review rule. The follow-up request test now uses a moved head: since #699 an explicit request on an unchanged, already-reviewed head intentionally does not start a second review. Bump the reviewer bundle to 1.9.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… on main Port both fixes onto the current dispatcher and reviewer gate. - A matched delivery whose conversation is in ERROR is sent again and reported as `retried` on both the local Agent Server path and the OpenHands API path. Retries are bounded (two per delivery, counted in the KV record and reset by a new delivery), so a conversation that fails the same way every time is not re-run on every scan. - A completed run with conclusion `action_required` is waiting, not blocking. It is listed as "(awaiting maintainer approval)", and the gate comment asks a maintainer to approve the workflow runs instead of saying no action is needed, which addresses the review on this PR. The fail-closed test now uses a genuinely unknown conclusion. - Bump the bundles that ship these files: github-pr-reviewer 1.9.2 (after #647's 1.9.1), github-issue-to-pr 1.2.2, github-issue-triage 1.3.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A submitted delivery for an earlier approval can arrive after this account requested changes on the same head. Anchor completion on the reported review but also count this account's later reviews of the head, so the newest verdict decides, and never complete on a dismissed review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
I'm an AI agent (Claude Code, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.
Approving 7ff0ee8. Since the earlier approvals, main reworked worker.py (#649, #651, #654, #660, #699, #703, #692, #652), so I merged main in and re-applied the #646 fix on top of it:
- A
submitteddelivery passes the review it reports into both completion paths, including the already-reviewed-head path from #699. That review is found by id regardless of the request window, and a delivery with no visiblereview_requestedevent is still reconciled instead of skipped. - An independent pass found that matching only the reported review could hand off on a superseded verdict (a late delivery for an earlier approval after a newer change request on the same head). Completion now also counts this account's later reviews of the head, so the newest verdict decides, and dismissed reviews never complete.
- The follow-up-request test now uses a moved head: since #699, an explicit request on an unchanged, already-reviewed head intentionally starts nothing.
Tests: 107 reviewer/handoff tests and the full suite (1145 passed, 23 skipped). The five new completion tests fail against main's worker. CI is green on this head, and the bundle is bumped to 1.9.1.
Bring in #647 and #688. Mechanical resolution: - agent_conversation.py: keep both constant groups (Cloud sandbox states and `_MAX_ERROR_RETRIES`); `_deliver_local` takes both the PR's `subject` and #688's `can_retry`; `deliver` writes #688's `error_retries` into the record, then keeps the PR's `_kv_put` and registry bookkeeping. The retry branches are not gated by the in-flight cap yet; that follows in a separate commit. - worker.py: `ReviewIntake.drain` keeps the PR's stop on `deferred` and #688's count of `created` and `retried` against the per-scan bound. - test_agent_conversation_dispatch.py: keep #688's Cloud retry tests and the PR's Cloud in-flight section. - github-pr-reviewer manifest and fixture: keep the PR's 1.10.0 over main's 1.9.2. - automations/bundle-index.js regenerated with `npm run build`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🚀 Released in v0.28.0. |
Bring the dev branch up to OpenHands/extensions 0.28.0, which is 16 commits ahead and includes upstream fixes to the GitHub PR reviewer itself: OpenHands#647 stops a delayed submitted review event from restarting a completed review, OpenHands#688 retries errored deliveries and waits on action_required heads, and OpenHands#652 requires live UI evidence before approval. Those land in the shared reviewer the compound-engineering entry reuses, so dev was carrying known bugs until this. Merged rather than rebased: fork/dev is what agent_canvas_extensions_ref points the running server at, so its history is deployed. A rebase would rewrite a published ref and force a push onto the branch the server is reading. Three files needed resolution. README.md conflicted only in its auto-generated catalog section, and the conflict was the extension counts, so the markers were dropped and sync_extensions.py regenerated the section authoritatively rather than hand-picking a side: 78 extensions, which is the union of upstream's new simplified-technical-english skill and our compound-engineering plugin. marketplaces/openhands-extensions.json and automations/bundle-index.js auto-merged, both additive and in different regions. uv run pytest -q: 1163 passed, 23 skipped. sync_extensions.py --check exits 0 and the catalog and bundle indexes are regenerated, so the derived files match their sources on the merged tree.
5 participants
HUMAN:
No human has tested these changes. The checkbox below is intentionally left unchecked, and all evidence in this description comes from agent-run automated tests and generated-artifact checks. This description was written by an AI agent at the maintainer's request.
AGENT:
Why
_finish_completed_review()in the reusablegithub-pr-reviewerextension decides completion by comparing each exact-head bot review'ssubmitted_atagainst the selected trigger'screated_at. In event mode the selected trigger is the latestreview_requestedissue event, and the review apull_request_review.submitteddelivery reports can predate that event, so the review the delivery announced is not recognized as completion and the configured maintainer handoff does not run. When noreview_requestedissue event is visible, the delivery is skipped entirely.Summary
skills/github-pr-reviewer/scripts/worker.py(re-applied on currentmainon 2026-10-04, after #649, #651, #654, #660, #699, #703, #692 and #652 reworked the worker):run()reads thereviewobject from asubmittedpayload and passes it to both completion paths: the already-reviewed-head path added in fix(review): stop repeat reviews of an unchanged head #699 and the regular completion check._review_completes_request(review, triggered_at, submitted_review)matches the reported review byidregardless of timing; every other run keeps the trigger-time window, and unrequested scheduled scans keep the any-exact-head-review rule.submitteddelivery with no visiblereview_requestedevent is reconciled instead of skipped. The existing "a submitted review is never a fresh trigger" guard is unchanged, so a non-decisive, wrong-head, or different review neither dispatches nor hands off.1.9.1(manifest and fixture),automations/bundle-index.jsregenerated.Issue Number
Closes #646
How to Test
New tests in
tests/test_github_reviewer_delivery.py:test_reviewer_delayed_submitted_event_completes_prior_request- a reported review that predates the latest request completes it and runs the handoff once.test_reviewer_submitted_event_completes_without_request_event- completion with noreview_requestedevent visible.test_reviewer_submitted_event_matches_only_the_reported_review- another decisive review on the head is not the one the delivery reports, so nothing happens.test_reviewer_delayed_submitted_event_without_roster_is_a_noop.test_reviewer_subsequent_review_request_for_new_head_starts_new_review- a later explicit request on a moved head still dispatches (43:head-3). Since fix(review): stop repeat reviews of an unchanged head #699 an explicit request on an unchanged, already-reviewed head intentionally does not start a second review.The first three fail against
main'sworker.pyand pass with the fix.Video/Screenshots
Not applicable: this changes automation worker logic, not a GUI.
Notes
No live webhook deployment or human verification was performed; the behavior is covered by the unit tests above.
Generated by OpenHands AI on behalf of the user; re-applied on current
mainby an AI agent (Claude Code, Opus 5.5) helping Engel Nyst (@enyst).