Skip to content

fix(review): gate agent launch on exact-head checks - #651

Merged
neubig merged 8 commits into
mainfrom
fix/644-reject-failed-checks
Sep 25, 2026
Merged

fix(review): gate agent launch on exact-head checks#651
neubig merged 8 commits into
mainfrom
fix/644-reject-failed-checks

Conversation

@all-hands-bot

@all-hands-bot all-hands-bot commented Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

HUMAN:
On 2026-09-22 a review request for all-hands-bot on OpenHands/OpenHands#17544 created a reviewer conversation even though the required Validate PR description check was already failing, spending an LLM slot on a head that deterministically could not be reviewed. Being conservative is also what the maintainer asked for after OpenHands/OpenHands#16223 was auto-approved over a failing current-head gate. The fix adds a deterministic check-run eligibility gate in front of agent dispatch. The evidence below is agent-run tests plus live API reads of the reported heads; I intentionally left the human-test checkbox unchecked because no human exercised the change.

  • A human has tested these changes.

AGENT:

Why

skills/github-pr-reviewer/scripts/worker.py dispatched an agent conversation for every eligible trigger without looking at the current head's check state. On 2026-09-22, requesting all-hands-bot on OpenHands/OpenHands#17544 created conversation f46343fd-9486-5943-a38f-fd8a279dd40f even though the head's required Validate PR description check was already COMPLETED / FAILURE. The reviewer should reserve agent work for heads that pass cheap deterministic eligibility checks.

Iterating on the live deployment exposed the two failure modes the gate must not have:

  • The check-run-only gate missed Actions workflows that fail before creating jobs. OpenHands/extensions#426 at head 41ffb9d had three failed pull_request workflow runs (Tests, Check Extensions, Deprecation deadlines), each with zero jobs, while the commit check-run rollup was green, so the reviewer launched anyway.
  • Classifying every check and workflow run then over-blocked. 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; the all-run classifier treated that optional failure as blocking.

The gate now classifies on GitHub's required-check signal in scheduled mode, and the explicit-request intake path is exempt.

Summary

  • skills/github/scripts/github_client.py
    • GitHubRepository.check_runs(sha) reads /repos/{owner}/{repo}/commits/{sha}/check-runs. That endpoint answers with an object rather than a list, so it paginates manually rather than through gh_pages.
    • GitHubRepository.workflow_runs(sha) reads /repos/{owner}/{repo}/actions/runs?head_sha={sha} the same way. A workflow that fails before any job reports leaves a failed check suite with no check runs under it, so only the workflow run reveals it.
    • GitHubRepository.required_check_contexts(number) runs one GraphQL query returning the head's status check rollup and keeps only the contexts isRequired(pullRequestNumber:) marks required. That field is the merge policy's own source of truth and is pull-request-scoped, so it stays correct for a stacked PR (#17200's symbolic base branch carries no branch rules) where a ruleset or branch-protection read would not.
  • skills/github-pr-reviewer/scripts/worker.py — the head-eligibility gate, evaluated immediately before dispatcher.deliver(...):
    • Scheduled discovery classifies only GitHub-required checks. A required context resolves to its latest exact-head check run; a required commit status resolves through statuses(). An optional workflow that fails - including one that fails before creating any check run - cannot block.
    • An explicit all-hands-bot review request bypasses the gate. The caller asked for that head by name, so _gate_head(pr, requested=True) returns green without reading checks and without leaving a gate comment. Draft, scope, exact-head, and delivery-deduplication safeguards still apply.
    • Fail closed. When the required signal cannot be read or is empty the classifier falls back to every current-head check run and workflow run, so a red head still blocks. This also covers #426: its required checks never started, so isRequired returns no node and the empty set falls back to the all-run rollup.
    • A completed run whose conclusion is not success, neutral, or skipped classifies as blocked; a run whose status is not completed, or a required context that has not reported a current-head run at all, classifies as waiting. An unrecognized conclusion fails closed rather than approving silently, and an unfulfilled required check is never approval.
    • Runs GitHub attributes to any other head are ignored, and only the latest run of each logical check or workflow counts (name + reporting app / workflow ID, by run ID then start time), so a stale or superseded failure cannot block the push that fixed it.
    • A non-green head prints a review-blocked / review-waiting disposition and continues before any conversation is created, without holding a worker slot to poll. An existing completed review still short-circuits first, so the gate never second-guesses a finished verdict.
    • The trigger is not consumed: a later scheduled scan, or a new explicit all-hands-bot request, starts the review once the head's required checks are non-blocking.
    • _gate_comment() leaves exactly one concise explanation on the PR. It upserts its own comment through a stable hidden marker carrying the head SHA and gate category (<!-- openhands-review-gate:{blocked|waiting}:{sha} -->), so a later run for a different head updates the marked comment instead of duplicating it. Only a marker this reviewer account authored counts as its own; any other marker is untrusted. If the repository workflow already posted a deterministic remediation comment naming the same reported checks, the gate adds nothing.
  • skills/github-pr-reviewer/SKILL.md, README.md — document the required-only scheduled gate, the explicit-request exemption, the blocked/waiting outcomes, and the retry contract.

No change to the review prompt, verdict parsing, scripts/maintainer_handoff.py, profile/secret selection, or the Actions-based plugins/pr-review.

Issue Number

Fixes #644

How to Test

  1. uv run --group test pytest -q tests/test_github_reviewer_delivery.py tests/test_github_automation_foundation.py — expect all pass.
  2. uv run --group test pytest -q — expect 1021 passed, 23 skipped.
  3. uv run python scripts/sync_extensions.py --check — expect clean (the pre-existing non-blocking issue-duplicate-checker coverage warning is unrelated).
  4. npm run build — expect no drift in automations/bundle-index.js or skills/index.js.

Regression coverage in tests/test_github_reviewer_delivery.py drives the real shipped worker.py entrypoint through the catalog bundle helper:

  • test_reviewer_ignores_an_optional_zero_job_workflow_on_a_green_required_head — the #17200 fixture: one required success check plus an optional zero-job startup_failure workflow yields exactly one dispatch and no gate comment.
  • test_reviewer_explicit_request_bypasses_the_ci_gate — a native review_requested event targeting all-hands-bot dispatches even with a failing required check, and leaves no gate comment.
  • test_reviewer_blocks_when_a_required_check_fails — a failed required check blocks and names only the required check, not the optional one.
  • test_reviewer_waits_for_a_pending_required_check, test_reviewer_waits_for_a_required_context_with_no_current_head_run — pending and unfulfilled required checks wait.
  • test_reviewer_falls_back_to_all_checks_when_required_reported_nothing, test_reviewer_falls_back_to_every_check_when_required_signal_is_unavailable — the #426 empty-required and signal-failure paths still block.
  • test_reviewer_ignores_an_optional_failure_but_reads_required_statuses — a required commit status is classified alongside required check runs.
  • test_reviewer_blocks_a_workflow_that_failed_with_no_check_runs, test_reviewer_does_not_double_report_a_workflow_that_has_check_runs, test_reviewer_waits_for_a_workflow_run_that_has_not_finished, test_reviewer_ignores_workflow_runs_from_an_obsolete_head, test_reviewer_lets_a_green_workflow_rerun_supersede_a_failed_one — the workflow-run fallback and its exact-head, dedupe, and supersession rules.
  • test_reviewer_blocking_current_head_check_creates_no_conversation, test_reviewer_green_current_head_creates_exactly_one_conversation, test_reviewer_pending_current_head_check_waits_without_a_conversation, test_reviewer_ignores_checks_from_an_obsolete_head, test_reviewer_fails_closed_on_an_unknown_conclusion — the core gate contract.
  • test_reviewer_does_not_duplicate_the_gate_comment_for_the_same_head, test_reviewer_updates_its_gate_comment_when_the_head_moves, test_reviewer_defers_to_an_equivalent_workflow_remediation_comment, test_reviewer_ignores_a_marked_comment_a_pr_author_wrote, test_reviewer_does_not_defer_to_a_workflow_comment_about_another_check — the comment-marker and dedupe contract.
  • test_reviewer_waiting_head_does_not_consume_a_later_green_request — a waiting stop does not consume the trigger.

tests/test_github_automation_foundation.py covers the transport: paginated object responses for check_runs and workflow_runs, and required_check_contexts keeping only required nodes, rejecting an over-long rollup, and surfacing GraphQL errors.

Live API reads

Required-check signal through the new helper with the reviewer's own token:

OpenHands/OpenHands 17200 -> [{'name': 'test-and-build (ubuntu)', 'kind': 'CheckRun'}]
OpenHands/extensions 426  -> []   # required checks never started; empty set falls back

Scheduled classification of the reported heads through the shipped classifier:

17200 (required-only) -> ('green', [])

#17200's optional release ready startup_failure no longer blocks; #426 still falls back to the all-run rollup and blocks.

Video/Screenshots

Not applicable: this changes automation worker dispatch logic, not a GUI.

Notes

_gate_comment reads the PR's existing comments on every blocked or waiting run. That is one paginated read on a path that already performs several, and it does not create an agent conversation. required_check_contexts is a single GraphQL query per candidate, and an explicit request skips it entirely.

Generated by OpenHands AI on behalf of the user.

@openhands-agent
fix(review): reject failed required checks before launching an agent
79e0a0b 
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>
@github-actions github-actions Bot added the type: fix A bug fix label Sep 22, 2026
@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

@all-hands-bot please rename this PR to reflect the implemented policy: the gate rejects non-green exact-head checks, not only checks marked required. A title such as "fix(review): gate agent launch on exact-head checks" would match the issue decisions and the code.

@all-hands-bot all-hands-bot mentioned this pull request Sep 22, 2026
Closed
19 tasks
@all-hands-bot all-hands-bot changed the title fix(review): reject failed required checks before launching an agent fix(review): gate agent launch on exact-head checks Sep 22, 2026
@all-hands-bot

all-hands-bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Renamed. The PR title is now:

fix(review): gate agent launch on exact-head checks

That matches the implemented policy: the gate classifies the exact head SHA's check runs, ignoring runs attributed to any other head, and blocks on any completed run whose conclusion is not success, neutral, or skipped — regardless of whether a check name is configured as required. A queued/in_progress head waits instead of launching. I did not touch the commits or description.

Generated by OpenHands AI on behalf of the user.

@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

@all-hands-bot please tighten the deterministic gate before human review:

  1. _gate_comment() must only treat a marked comment as managed when it was authored by the configured reviewer account. A PR author can currently put the marker in their own comment. The worker may then suppress its own explanation or attempt to PATCH that author's comment and fail. Treat all PR comments as untrusted input.
  2. The existing-workflow dedupe is too broad: any comment containing no AI was used to generate this comment suppresses the gate explanation, even when that comment is about a different check (for example the PR description or artifact workflow). Only suppress when the existing comment actually explains the same current-head blocking or waiting checks; otherwise post the reviewer's marked comment.
  3. The blocked heading says required checks failed, but this implementation intentionally gates on every reported check run. Please call them current-head checks or simply checks.

Please add narrow regression tests for the two ownership/deduplication cases and keep the patch focused. The current tests and CI are green, but these are live-path correctness gaps.

@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

@all-hands-bot I found a live edge case before rollout. GitHub's check-runs API lists both a failed and a later successful Validate PR description run on the same commit when the PR body is fixed without a code push. This is present on Extensions #647 (head 71b5028) and #649 (head 54683f5). The current _classify_check_runs() treats the historical failure as blocking forever even though the current validation succeeded.

Please investigate the API data and add a regression test for repeated runs of the same check on one SHA. I have asked the maintainer whether the gate should use only the latest run per check name; hold that behavior change until the policy decision arrives. This is a live-path blocker for deploying #651 as written.

@openhands-agent
fix(review): trust only owned gate comments and matching dedupe
ece3e88 
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.
@all-hands-bot

all-hands-bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Done on all three, plus narrow regression tests. Summary of the patch to skills/github-pr-reviewer/scripts/worker.py:

1. Ownership of managed comments. Added _owns_comment() and filtered the managed set so a comment with the gate marker only counts when its author matches self.github_login (the configured reviewer account). An author-supplied marker is now inert: it cannot suppress the explanation, and it is never selected for PATCH. Only the reviewer's own marked comment is updated for a moved head.

managed = [
    comment
    for comment in comments
    if CHECK_GATE_MARKER in (comment.get("body") or "")
    and self._owns_comment(comment)
]

2. Narrower workflow dedupe. _gate_comment() now takes the reported check names, and the disclosure-based short-circuit fires only when the unmarked deterministic comment actually names every check the gate is reporting (_explains_checks). A disclosure about a different check (PR description, artifact workflow, etc.) no longer suppresses the gate; the reviewer posts its own marked comment in that case.

if any(
    WORKFLOW_DISCLOSURE.lower() in (comment.get("body") or "").lower()
    and self._explains_checks(comment.get("body") or "", names)
    for comment in comments
    if CHECK_GATE_MARKER not in (comment.get("body") or "")
):
    return None

3. Heading. The blocked heading is now ### ⚠️ Review paused: current-head checks failed, matching the "every reported check run" policy. _gate_head() passes the names through, and SKILL.md documents the ownership and dedupe rules.

Tests (tests/test_github_reviewer_delivery.py):

  • test_reviewer_ignores_a_marked_comment_a_pr_author_wrote — a forged marker authored by a non-reviewer yields zero PATCHes and exactly one reviewer-posted marked comment.
  • test_reviewer_does_not_defer_to_a_workflow_comment_about_another_check — a disclosure naming only Validate PR description does not suppress the gate when the blocked check is lint; the reviewer posts its own comment.
  • test_reviewer_defers_to_an_equivalent_workflow_remediation_comment — updated so the workflow comment names the same Validate PR description check; it still suppresses.

uv run --group test pytest -q is green: 997 passed, 23 skipped. npm run build regenerated skills/index.js and automations/bundle-index.js with no drift, and scripts/sync_extensions.py --check is clean apart from the pre-existing non-blocking issue-duplicate-checker coverage warning.

I did not push commits, since I only push when explicitly asked; say the word and I will push this to fix/644-reject-failed-checks.

Separately, I see the follow-up about GitHub listing both a failed and a later successful Validate PR description run on one commit. Per that request I am holding any behavior change (latest-run-per-name) until the maintainer's policy decision arrives, and will add the repeated-run regression test at that point.

Generated by OpenHands AI on behalf of the user.

@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

@all-hands-bot please push the three tested fixes you described (managed-comment ownership, check-specific workflow dedupe, and accurate heading) to this PR now. Leave the repeated-run behavior unchanged while the latest-run policy decision is pending. After pushing, report the new head SHA and current CI results. Do not merge.

@all-hands-bot

all-hands-bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Pushed. The three tested fixes are now on this PR branch, and I did not merge anything.

New head SHA: ece3e88c47af5e3efed873c3f41785eb0430f81b (branch fix/644-reject-failed-checks, commit ece3e88 — fix(review): trust only owned gate comments and matching dedupe). Previously the branch was at 79e0a0b; this is a fast-forward, so the PR diff is unchanged apart from the patch below.

What landed:

  1. Managed-comment ownership — a comment with the gate marker only counts as our own when its author matches the configured reviewer account; an author-placed marker can neither suppress the explanation nor be selected for PATCH.
  2. Check-specific workflow dedupe — the disclosure short-circuit fires only when the existing unmarked comment names every check the gate is reporting; a disclosure about a different check no longer suppresses it.
  3. Accurate heading — ### ⚠️ Review paused: current-head checks failed.

No change to repeated-run behavior: _classify_check_runs() still treats a historical failure on the same SHA as blocking. I'm holding latest-run-per-name until the policy decision arrives, as you asked.

Current CI results on ece3e88 — all green (11 checks, 0 failing, 0 pending):

Check Workflow Result
test Tests ✅ success
validate-claude-code Check Extensions ✅ success
sync-extensions Check Extensions ✅ success
sync-sdk-skill Check Extensions ✅ success
check Deprecation deadlines ✅ success
Validate PR description PR Description Check ✅ success
check-pr-artifacts PR Artifacts ✅ success
pr-title / Lint PR title (conventional) pr ✅ success
pr-title / Apply conventional-type label pr ✅ success
cleanup-on-approval PR Artifacts ⏭️ skipped
cleanup-after-merge PR Artifacts ⏭️ skipped

Locally, uv run --group test pytest -q tests/test_github_reviewer_delivery.py is green (29 passed), and the ownership/dedupe regression tests described earlier are included. Merge state is BLOCKED pending review; no merge performed, as instructed.

Generated by OpenHands AI on behalf of the user.

@neubig neubig mentioned this pull request Sep 22, 2026
Open
11 tasks
@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

Deployment blocker found during live audit: the OSS reviewer automation has only review_requested and bot review.submitted event triggers; it has no scheduled scan. If this gate sees pending checks, no later CI-completion event reaches the worker, so the PR can remain unreviewed indefinitely unless a human re-requests the bot. The current waiting comment promises a scheduled retry that does not exist in this deployment. I opened #653 for an automatic pending-to-green retry path. Please keep #651 out of production until that path and the repeated-check-run policy are resolved; do not broaden this PR without first considering whether #653 is cleaner as a separate focused change.

@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

Policy decision for the repeated-run blocker: classify only the latest run of each logical check on the exact head. Use check name plus reporting app identity as the grouping key, and deterministic run start time/ID ordering; a later queued/in-progress rerun is waiting, a later success supersedes an earlier failure on the same SHA. This matches the live Validate PR description behavior on #647 and #649. Please add a regression test and update #651 before review. The automatic pending-to-green retry for the deployed reviewer is tracked separately in #653 and will stack on #651.

@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

@all-hands-bot please implement the latest-run policy recorded above on this PR now, using the live #647/#649 repeated Validate PR description runs as the regression case. Group by check name and reporting app identity, choose the latest run by start time and ID, and let a newer pending run make the head wait. Keep the change focused, run the relevant tests and full CI, and push the branch. Do not merge. #653 will stack on this PR after this base is green.

@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

One edge case in head 5261e282: _check_run_order() sorts (started_at or "", id). GitHub allows a queued/requested check run before it has a started_at timestamp. If an earlier successful run has a timestamp and a newer queued rerun has started_at: null, the earlier success wins and the reviewer can launch while the rerun is pending. Please add a regression with a newer queued run whose started_at is null, and make the ordering fall back to run ID when a start timestamp is absent. This must be fixed before deploying the gate. GitHub documents started_at as optional for check-run updates: https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28#update-a-check-run

@openhands-agent
fix(review): gate on the latest run of each exact-head check
336aaf2 
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>
@all-hands-bot
all-hands-bot force-pushed the fix/644-reject-failed-checks branch from 5261e28 to 336aaf2 Compare September 22, 2026 20:13
@all-hands-bot

all-hands-bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Implemented the latest-run policy you recorded and pushed it. I did not merge.

New head SHA: 336aaf2434c8815c4518ac0b343e0e881bbc0990 (branch fix/644-reject-failed-checks, commit 336aaf2 — fix(review): gate on the latest run of each exact-head check), fast-forwarded over ece3e88. I had to force-push once: the first commit amended the catalog index after npm run build, which left the embedded copy of worker.py in automations/bundle-index.js stale and failed the test check. Regenerated and re-pushed.

What changed

skills/github-pr-reviewer/scripts/worker.py — a new _latest_check_runs(sha) narrows the exact-head runs before _classify_check_runs() sees them:

  • Grouping key is (check name, reporting app identity), where the app identity is app.slug (falling back to app.id, then app.name). Two apps that use the same check name stay independent, so one cannot mask the other.
  • Latest run wins within a group, ordered by (started_at, run id). The run ID breaks a start-time tie, so ordering is deterministic rather than dependent on API list order.
  • A newer pending run makes the head wait. A later queued/in_progress re-run supersedes an earlier success; it is classified as pending, so the head is waiting, not green.
  • Superseded runs are dropped entirely: an earlier failure on the same SHA no longer blocks once a later run of that check exists.

_classify_check_runs() is otherwise unchanged — same fail-closed handling for unknown conclusions, same obsolete-head filtering.

SKILL.md documents the new rule under the head-eligibility bullet list.

Regression tests

Added to tests/test_github_reviewer_delivery.py, using a _run(...) helper that carries the real API fields (app, started_at, id):

  • test_reviewer_ignores_a_superseded_failure_for_the_same_check — the live case: Validate PR description failed at 13:04:07 then succeeded at 13:18:41 on one SHA; the head is green and exactly one conversation starts.
  • test_reviewer_prefers_run_id_to_break_a_start_time_tie — identical started_at, higher run ID wins.
  • test_reviewer_waits_for_a_newer_rerun_that_superseded_a_success — a later queued run turns a previously successful check into waiting.
  • test_reviewer_treats_same_name_from_different_apps_as_distinct_checks — same name from two apps; the newer success from github-actions cannot hide the custom-ci failure, so the head is blocked.

Verified against the live data

I ran the shipped gate methods against the actual check-run payloads for both heads, rather than trusting the fixtures alone. Both now classify green where they previously blocked forever, and the kept run is the later success:

#647 71b5028ec1d3f7cd9b57760776270823532473de: state=green  kept: success 2026-09-22T13:18:41Z id=106761769135
#649 54683f52bd3771891ed36478cd664d6aecbea1c1: state=green  kept: success 2026-09-22T14:56:31Z id=106800152225

In each case the superseded failure (106756418507, 106797051932) is dropped.

Local and CI results on 336aaf2

uv run --group test pytest -q — 1001 passed, 23 skipped. npm run build regenerated skills/index.js and automations/bundle-index.js with no remaining drift, and scripts/sync_extensions.py --check is clean apart from the pre-existing non-blocking issue-duplicate-checker coverage warning.

CI is fully green (9 success, 2 skipped, 0 failing, 0 pending):

Check Workflow Result
test Tests ✅ success
validate-claude-code Check Extensions ✅ success
sync-extensions Check Extensions ✅ success
sync-sdk-skill Check Extensions ✅ success
check Deprecation deadlines ✅ success
Validate PR description PR Description Check ✅ success
check-pr-artifacts PR Artifacts ✅ success
pr-title / Lint PR title (conventional) pr ✅ success
pr-title / Apply conventional-type label pr ✅ success
cleanup-on-approval PR Artifacts ⏭️ skipped
cleanup-after-merge PR Artifacts ⏭️ skipped

Merge state is BLOCKED pending review and the branch is MERGEABLE; nothing was merged. The pending-to-green retry gap you described stays out of this PR for #653 to stack on once this base is green.

Generated by OpenHands AI on behalf of the user.

@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

@all-hands-bot please address the queued-run edge case in my latest comment before this check gate is reviewed or deployed. Add a regression test for a newer queued run with no started_at and push the focused fix; do not merge.

@openhands-agent
fix(review): order a queued check run without a start time by its run ID
de8dd38 
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>
@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

The new ~{run_id} fallback fixes a newer queued run, but it creates the reverse error: an older queued/cancelled run with no started_at sorts after a newer successful run because ~ sorts after every ISO timestamp. That can keep a head blocked or waiting after the successful rerun. Since this code already assumes check-run IDs increase with creation order, please use the run ID as the primary ordering key (or a reliably present creation timestamp with ID tie-break), and add both directions as regressions: older null-start then newer success, and older success then newer null-start queued. This can remain a very small change. Do not merge.

@all-hands-bot

all-hands-bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Fixed and pushed. I did not merge.

New head SHA: de8dd38f888a85d8289eb92a715d7b89961e4fa0 (branch fix/644-reject-failed-checks, commit de8dd38 — fix(review): order a queued check run without a start time by its run ID), fast-forwarded over 336aaf2.

The bug

You are right, and it is exactly as you described. _check_run_order() was:

return (run.get("started_at") or "", int(run.get("id") or 0))

A queued/requested run that has no started_at yet got "", which sorts before any real timestamp. So for a group with an earlier completed/success at 2026-09-22T13:00:00Z (id 1) and a newer queued rerun with started_at: null (id 2):

success order: ('2026-09-22T13:00:00Z', 1)
queued  order: ('', 2)
queued wins?   False

The earlier success won, the queue was classified green, and the reviewer launched while the rerun was still pending.

The fix

run_id = int(run.get("id") or 0)
started_at = run.get("started_at")
return (started_at or f"~{run_id:020d}", run_id)

When a start time is absent, the run ID (monotonic per repository) fills in as the primary key, so a newer queued run always outranks an earlier completed run on the same logical check. Timestamped ordering is unchanged, and a timestamp tie still falls through to the run ID as before.

I re-checked the other orderings, and they all still hold: later timestamp beats an earlier one regardless of ID; two runs with no start time order by ID; two runs sharing a timestamp order by ID.

Regression test

test_reviewer_waits_for_a_newer_queued_run_without_a_start_time in tests/test_github_reviewer_delivery.py: an earlier completed/success with started_at=2026-09-22T13:00:00Z and a newer queued run with started_at=None. Asserts the reviewer does not launch and posts exactly one waiting gate comment naming ci. I confirmed it fails against the old ordering (queued wins? False path launches the agent) and passes with the fix.

SKILL.md and the _latest_check_runs() docstring now note that the run ID stands in for a queued run whose start time is still absent.

Verification on de8dd38

uv run --group test pytest -q — 1002 passed, 23 skipped. npm run build regenerated skills/index.js and automations/bundle-index.js with no remaining drift, and scripts/sync_extensions.py --check is clean apart from the pre-existing non-blocking issue-duplicate-checker coverage warning.

CI is fully green (9 pass, 2 skipped, 0 failing, 0 pending):

Check Workflow Result
test Tests ✅ pass
validate-claude-code Check Extensions ✅ pass
sync-extensions Check Extensions ✅ pass
sync-sdk-skill Check Extensions ✅ pass
check Deprecation deadlines ✅ pass
Validate PR description PR Description Check ✅ pass
check-pr-artifacts PR Artifacts ✅ pass
pr-title / Lint PR title (conventional) pr ✅ pass
pr-title / Apply conventional-type label pr ✅ pass
cleanup-on-approval PR Artifacts ⏭️ skipping
cleanup-after-merge PR Artifacts ⏭️ skipping

Merge state is BLOCKED pending review and the branch is MERGEABLE; nothing was merged, per your instruction. The change is scoped to the ordering helper, its docs, and the one regression test — the pending-to-green retry gap stays in #653 to stack on this base.

Generated by OpenHands AI on behalf of the user.

@neubig

neubig commented Sep 22, 2026

Copy link
Copy Markdown
Member

@all-hands-bot please fix the reverse-order case in my newest comment and add the regression. The current ~{run_id} fallback can incorrectly outrank a newer successful run. Use check-run ID as the primary order if that is the reliable creation sequence, then run tests and push. Do not merge.

@openhands-agent
fix(review): order exact-head check runs by run ID
0c4b297 
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.
@all-hands-bot

all-hands-bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Fixed and pushed. I did not merge.

New head SHA: 0c4b297f266e96a130c1990ce6a0b8a41fc5426b (branch fix/644-reject-failed-checks, commit 0c4b297 — fix(review): order exact-head check runs by run ID), fast-forwarded over de8dd38.

The bug

You are right. _check_run_order() was:

run_id = int(run.get("id") or 0)
started_at = run.get("started_at")
return (started_at or f"~{run_id:020d}", run_id)

~ sorts after every ISO timestamp, so an older queued/cancelled run with started_at: null outranked a newer successful run:

older queued  order: ('~00000000000000000001', 1)
newer success order: ('2026-09-22T13:10:00Z', 2)
queued wins?   True   # head stays blocked/waiting after the successful rerun

The fix

The run ID is the reliable creation sequence, so it is now the primary key and the start time only breaks a tie:

run_id = int(run.get("id") or 0)
return (run_id, run.get("started_at") or "")

I verified both directions against the old and new keys directly:

Case Old key New key
older null-start queued (id 1), newer success at 13:10 (id 2) queued wins ❌ success wins ✅
older success at 13:00 (id 1), newer null-start queued (id 2) queued wins ✅ queued wins ✅
same start time, higher run ID ID tie-break ✅ start-time tie-break ✅

The existing test_reviewer_waits_for_a_newer_queued_run_without_a_start_time still passes, and the superseded-failure and start-time-tie tests are unaffected. SKILL.md and the _latest_check_runs() docstring now state that the run ID is the primary order.

Regression

test_reviewer_launches_when_a_success_supersedes_an_older_null_start_run in tests/test_github_reviewer_delivery.py: an older queued run with started_at=None (id 1) and a newer completed/success at 2026-09-22T13:10:00Z (id 2). It asserts the reviewer launches exactly once and posts no gate comment. I confirmed it fails against the old ordering and passes with the fix.

Verification on 0c4b297

uv run --group test pytest -q — 1003 passed, 23 skipped. npm run build regenerated skills/index.js and automations/bundle-index.js with no drift, and scripts/sync_extensions.py --check is clean apart from the pre-existing non-blocking issue-duplicate-checker coverage warning.

CI is fully green (9 pass, 2 skipped, 0 failing, 0 pending) — including test:

Check Workflow Result
test Tests ✅ pass
validate-claude-code Check Extensions ✅ pass
sync-extensions Check Extensions ✅ pass
sync-sdk-skill Check Extensions ✅ pass
check Deprecation deadlines ✅ pass
Validate PR description PR Description Check ✅ pass
check-pr-artifacts PR Artifacts ✅ pass
pr-title / Lint PR title (conventional) pr ✅ pass
pr-title / Apply conventional-type label pr ✅ pass
cleanup-on-approval PR Artifacts ⏭️ skipping
cleanup-after-merge PR Artifacts ⏭️ skipping

Nothing was merged, and the pending-to-green retry remains in #653 to stack on this base.

Generated by OpenHands AI on behalf of the user.

@neubig
neubig requested a review from VascoSch92 September 22, 2026 20:31
@all-hands-bot all-hands-bot mentioned this pull request Sep 22, 2026
Merged
1 task
all-hands-bot pushed a commit that referenced this pull request Sep 22, 2026
@openhands-agent
fix(review): resume requested reviews on a scheduled check scan
1be2b44 
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>
This was referenced Sep 22, 2026
Open
Closed
@neubig

neubig commented Sep 23, 2026

Copy link
Copy Markdown
Member

@all-hands-bot Please update this PR to cover a current-head GitHub Actions workflow that fails before creating any check runs. This is within #644's existing goal of not launching a reviewer for red CI.

Concrete reproduction: #426 head 41ffb9d44ff187953034e32fe096607f77e506dd has a SUCCESS commit check-run rollup with only green pr-title checks, yet these pull_request workflow runs on that same head failed with zero jobs: Tests, Check Extensions, and Deprecation deadlines. The reviewer was launched and spent an agent slot before reporting those failures. gh pr checks also omitted the zero-job runs.

Please make the pre-dispatch gate consider current-head workflow-run conclusions as well as check runs, with the same blocked/waiting behavior and a focused regression test. Ignore obsolete-head runs, and avoid duplicating the existing gate/comment machinery. This should be part of #651 rather than a separate PR.

@neubig neubig mentioned this pull request Sep 23, 2026
Closed
14 tasks
@openhands-agent
fix(review): gate on current-head workflow runs too
9b012f1 
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>
@all-hands-bot

all-hands-bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Fixed and pushed. I did not merge.

New head SHA: 9b012f1 (branch fix/644-reject-failed-checks, commit fix(review): gate on current-head workflow runs too), fast-forwarded over 0c4b297.

The gap

You are right, and your reproduction is exact. The gate read only /commits/{sha}/check-runs, so a workflow that fails before any job reports contributed a failed check suite but no check runs. The commit rollup stayed green on the surviving pr-title checks, gh pr checks omitted the zero-job runs, and the reviewer launched into red CI. I re-read head 41ffb9d before changing anything and confirmed all three: the check-run rollup is 4 green pr-title entries, while the workflow runs on that SHA are Tests, Check Extensions, and Deprecation deadlines — each completed/failure, each with an empty suite.

The change

  • skills/github/scripts/github_client.py — add GitHubRepository.workflow_runs(sha), the shared transport for /actions/runs?head_sha={sha}. It answers with an object rather than a list, so it paginates manually exactly like the existing check_runs.
  • skills/github-pr-reviewer/scripts/worker.py — _classify_check_runs() now considers current-head workflow runs alongside check runs:
    • A workflow run whose check_suite_id already reported check runs is left to those runs; only a suite that reported no check runs (a workflow-level failure, or a pull_request run whose jobs never started) is added. A workflow is therefore never counted twice.
    • Workflow runs get the same blocked/waiting behavior and the same exact-head filter, so an obsolete-head run cannot block the push that fixed it.
    • The latest run of each workflow (name + workflow_id, ordered by run ID then run_started_at/created_at) supersedes its earlier attempts, so a green re-run clears a failed one.
    • The grouping is shared through one _latest_by_group helper, and the existing gate/comment machinery (_gate_comment, _gate_body, the marker upsert, the workflow-disclosure dedupe) is reused unchanged — nothing new to duplicate.
  • SKILL.md and the skill README.md document the workflow-run signal.

Regression test

test_reviewer_blocks_a_workflow_that_failed_with_no_check_runs in tests/test_github_reviewer_delivery.py reproduces #426: a green pr-title check run plus Tests, Check Extensions, and Deprecation deadlines failing as zero-job workflow runs. It asserts zero dispatches and exactly one blocked gate comment naming all three. Four supporting tests cover the dedupe (a workflow whose suite has check runs is not double-reported), waiting on an in-progress workflow, ignoring obsolete-head workflow runs, and a green re-run superseding a failed one. I confirmed the primary test fails against the check-run-only classifier (dispatcher.deliver is called) and passes with the fix. tests/test_github_automation_foundation.py gained the paginated-object test for workflow_runs.

Live verification

Running the shipped _classify_check_runs against the real API with the reviewer's own token:

41ffb9d44ff187953034e32fe096607f77e506dd -> ('blocked', ['Check Extensions', 'Deprecation deadlines', 'Tests'])
0c4b297f266e96a130c1990ce6a0b8a41fc5426b -> ('green', [])

The reproduced head is now blocked on the three zero-job workflows, and this PR's own head still reads green (its workflows all created check runs, so they are not double-counted).

uv run --group test pytest -q — 1009 passed, 23 skipped. npm run build regenerated skills/index.js and automations/bundle-index.js with no drift, and scripts/sync_extensions.py --check is clean apart from the pre-existing non-blocking issue-duplicate-checker coverage warning. Bundle version stays 1.5.0, consistent with the earlier commits on this branch.

Nothing was merged.

Generated by OpenHands AI on behalf of the user.

@neubig

neubig commented Sep 23, 2026

Copy link
Copy Markdown
Member

@all-hands-bot The workflow-run gate fix is pushed, but this PR now reports CONFLICTING with main. Please resolve only the merge conflict, preserve the exact-head check-run and workflow-run gate behavior, run focused tests plus CI, and push the branch. Do not merge.

@openhands-agent
Merge branch 'main' into fix/644-reject-failed-checks
6034e07 
Resolve the CONFLICTING state reported on #651. The only conflicted files
were the generated bundles (skills/index.js, automations/bundle-index.js),
which both sides had regenerated; they are resolved by re-running
`npm run build` over the auto-merged sources, so both the workflow-run gate
from this branch and main's refreshed-state prompt/docs are preserved.

Gate behavior is unchanged: exact-head check-run and workflow-run
classification, latest-run-per-logical-check ordering, owned-comment and
check-specific dedupe.
@all-hands-bot

all-hands-bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Conflict resolved and pushed. I did not merge.

New head SHA: 6034e07a3a095cd585efd7f6f87793271cdd9753 (branch fix/644-reject-failed-checks, merge commit Merge branch 'main' into fix/644-reject-failed-checks), over 9b012f1. mergeable is now MERGEABLE (was CONFLICTING); mergeStateStatus is BLOCKED, which is the pending review requirement, not a conflict.

The conflict

Only two files conflicted, both generated:

  • skills/index.js
  • automations/bundle-index.js

Both sides had regenerated them — this branch from the workflow-run gate, main from the refreshed-state work in #649 — so the conflict was in the inlined skill content and the inlined worker.py/main.py copies, not in any hand-written source. Every real source file auto-merged cleanly (worker.py, SKILL.md, README.md, main.py, both test files, integrations/catalog-index.js, plus main's new integrations/catalog/granola.json).

I resolved the two generated files by taking the merged sources and re-running npm run build, rather than hand-editing the bundles. That is the resolution the repo's own generator produces, and I confirmed the build is idempotent — a second npm run build over the committed tree leaves all three generated files byte-identical.

Gate behavior preserved

The merge is additive for this PR: git diff 9b012f1 <merged> over worker.py shows exactly one hunk, and it is main's prompt wording from #649. Nothing in the gate changed.

  • Exact-head check-run classification: intact.
  • Exact-head workflow-run classification, including the zero-job failure path: intact.
  • Latest-run-per-logical-check grouping by name + reporting app / workflow ID, run ID as primary order with start time as tie-break: intact.
  • Owned-comment filtering and check-specific workflow dedupe: intact.

I also ran the shipped _classify_check_runs against the live API on the new head and on your #426 reproduction:

6034e07a3a095cd585efd7f6f87793271cdd9753 -> state=green   names=[]
41ffb9d44ff187953034e32fe096607f77e506dd -> state=blocked names=['Check Extensions', 'Deprecation deadlines', 'Tests']

This PR's head still reads green, and the #426 head is still blocked on the three zero-job workflows, so the workflow-run gate survived the merge.

Tests

  • Focused: uv run --group test pytest -q tests/test_github_reviewer_delivery.py — 41 passed.
  • Full suite: uv run --group test pytest -q — 1011 passed, 23 skipped.
  • scripts/sync_extensions.py --check — clean apart from the pre-existing non-blocking issue-duplicate-checker coverage warning.
  • npm run build — no drift; skills/index.js, automations/bundle-index.js, and integrations/catalog-index.js are all stable across a rebuild.

CI on 6034e07 — all green (9 pass, 2 skipped, 0 failing, 0 pending)

Check Workflow Result
test Tests ✅ pass
validate-claude-code Check Extensions ✅ pass
sync-extensions Check Extensions ✅ pass
sync-sdk-skill Check Extensions ✅ pass
check Deprecation deadlines ✅ pass
Validate PR description PR Description Check ✅ pass
check-pr-artifacts PR Artifacts ✅ pass
pr-title / Lint PR title (conventional) pr ✅ pass
pr-title / Apply conventional-type label pr ✅ pass
cleanup-on-approval PR Artifacts ⏭️ skipping
cleanup-after-merge PR Artifacts ⏭️ skipping

Nothing was merged. The scope stayed at the conflict: I did not change gate policy or touch the #653 pending-to-green retry.

Generated by OpenHands AI on behalf of the user.

@all-hands-bot all-hands-bot mentioned this pull request Sep 23, 2026
Merged
1 task
all-hands-bot pushed a commit that referenced this pull request Sep 23, 2026
@openhands-agent
fix(review): resume requested reviews on a scheduled check scan
28f025f 
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>
This was referenced Sep 23, 2026
Merged
Closed
@neubig

neubig commented Sep 23, 2026

Copy link
Copy Markdown
Member

@all-hands-bot Please update the existing implementation in #651; do not open another PR.

The live deployment exposed two over-broad parts of the gate:

  1. Scheduled discovery should use GitHub-required checks, not every check/workflow on the head. feat: add automate setup split panel OpenHands#17200 currently passes its only required check (test-and-build (ubuntu)), which GitHub GraphQL reports with CheckRun.isRequired(pullRequestNumber: 17200) == true. The optional release ready workflow has a zero-job startup_failure; fix(review): gate agent launch on exact-head checks #651's all-workflow classifier incorrectly treated that optional failure as blocking. Use GitHub's required-check signal as the source of truth and retain exact-head, pending, and failed handling for required checks.
  2. An explicit request is the intake-policy exception. A native review_requested event targeting the configured reviewer should dispatch even if required CI is red or pending. The CI gate applies to scheduled/unrequested candidates. Draft, scope, exact-head, and delivery deduplication safeguards still apply.

Please update the focused tests and PR description accordingly. Add a regression fixture matching #17200: one successful required check plus an optional zero-job workflow startup_failure; scheduled classification is green, and an explicit request dispatches. Preserve the existing comment-marker and conversation-dispatch machinery.

@openhands-agent
fix(review): gate scheduled discovery on required checks only
9c62801 
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>
@all-hands-bot

all-hands-bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Updated and pushed. I did not merge.

New head SHA: 9c6280193f47229712e5cc4c9e682c7952ceac0d (branch fix/644-reject-failed-checks, commit fix(review): gate scheduled discovery on required checks only), over the merge commit 6034e07.

Both over-broad parts are fixed

1. Scheduled discovery now classifies only GitHub-required checks. I used the signal you verified: CheckRun.isRequired(pullRequestNumber:) in the head's status check rollup, as a single GraphQL query (GitHubRepository.required_check_contexts(number)). It is merge-policy source of truth and is pull-request-scoped, so it is correct for #17200 even though its symbolic base branch carries no rules of its own — a ruleset or branch-protection read returns nothing there. The required contexts resolve to their latest exact-head check runs (and required commit status contexts via statuses()); an optional workflow that fails is simply not in the set.

2. An explicit request bypasses the gate. _gate_head(pr, requested=True) returns green without reading checks and without leaving a gate comment, and run() passes requested=event_mode. Native review_requested events targeting all-hands-bot therefore dispatch even when required CI is red or pending. Draft, scope, exact-head, and delivery-deduplication safeguards are untouched.

Your #17200 reproduction

required set scheduled classification
#17200 (test-and-build (ubuntu) passes, optional release ready startup_failure) [{name: "test-and-build (ubuntu)", kind: "CheckRun"}] ('green', []) → dispatches
explicit review_requested request not read (bypass) dispatches, no gate comment

A regression fixture matching it, test_reviewer_ignores_an_optional_zero_job_workflow_on_a_green_required_head, asserts exactly one dispatch and no gate comment; test_reviewer_explicit_request_bypasses_the_ci_gate asserts the request dispatches with a failing required check and posts no comment. Both fail against the previous all-run classifier and event-gated code, and pass now.

The #426 case still blocks

isRequired is a field on each context that actually reported, so a required check that failed before creating any check run yields no node and the required set comes back empty. An empty or unreadable set falls back to the full current-head check run + workflow run rollup, so #426 still blocks rather than launching:

OpenHands/extensions 426 required -> []
OpenHands/extensions 426 scheduled -> ('blocked', ['Check Extensions', 'Deprecation deadlines', 'Tests'])
OpenHands/OpenHands 17200 scheduled -> ('green', [])
OpenHands/extensions 651 scheduled -> ('green', [])

That fallback is also the fail-closed path: a GraphQL error logs required_check_signal: unavailable and classifies the full rollup, so a required-check read failure never approves silently.

Preserved machinery

Exact-head filtering, latest-run-per-logical-check/workflow supersession (run ID primary, start time tie-break), fail-closed conclusions, the openhands-review-gate comment marker with its ownership filter and check-specific dedupe, and the conversation dispatch path are all unchanged. _classify_runs now holds the shared status/conclusion logic, with _classify_all_runs and _classify_required_runs as the two callers.

Verification

  • Focused: uv run --group test pytest -q tests/test_github_reviewer_delivery.py tests/test_github_automation_foundation.py — 60 passed.
  • Full suite: uv run --group test pytest -q — 1021 passed, 23 skipped.
  • scripts/sync_extensions.py --check — clean apart from the pre-existing non-blocking issue-duplicate-checker warning.
  • npm run build — no drift.

CI on 9c62801 — all green (9 pass, 2 skipped, 0 failing, 0 pending)

Check Workflow Result
test Tests ✅ pass
validate-claude-code Check Extensions ✅ pass
sync-extensions Check Extensions ✅ pass
sync-sdk-skill Check Extensions ✅ pass
check Deprecation deadlines ✅ pass
Validate PR description PR Description Check ✅ pass
check-pr-artifacts PR Artifacts ✅ pass
pr-title / Lint PR title (conventional) pr ✅ pass
pr-title / Apply conventional-type label pr ✅ pass
cleanup-on-approval PR Artifacts ⏭️ skipping
cleanup-after-merge PR Artifacts ⏭️ skipping

The PR description is updated to match: required-only scheduled classification, the explicit-request exemption, the fail-closed fallback, and the new tests. Nothing was merged.

Generated by OpenHands AI on behalf of the user.

all-hands-bot pushed a commit that referenced this pull request Sep 23, 2026
@openhands-agent
fix(review): resume requested reviews on a scheduled check scan
12048c6 
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>
@neubig

neubig commented Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Live Canvas evidence (2026-09-23):

  • Git Sync deployed composite extensions head 7100eb4 to the enabled OSS reviewer from private config commit ce27ad2.
  • An explicit all-hands-bot request on feat: add automate setup split panel OpenHands#17200 dispatched automation run cd47d825-a327-408d-ae27-e5d3fcc68153 even though optional/older check runs were failing.
  • The run callback recorded conversation cb12f25b-2a32-530d-98c2-c81fc4a5e2f8; the conversation finished normally.
  • The agent posted a COMMENTED review with a clear CHANGES REQUESTED verdict and two inline findings: feat: add automate setup split panel OpenHands#17200 (review)
  • The review explicitly recognized the successful required/current checks and treated the optional release ready startup failure plus superseded validation failures as nonblocking.

No merge was performed.

VascoSch92
VascoSch92 approved these changes Sep 24, 2026
View reviewed changes

@VascoSch92 VascoSch92 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepting as from the evidences it seems to work fine

@neubig
neubig merged commit 84459c5 into main Sep 25, 2026
17 checks passed
@neubig
neubig deleted the fix/644-reject-failed-checks branch September 25, 2026 20:52
@openhands-release-bot openhands-release-bot Bot mentioned this pull request Sep 25, 2026
Merged
neubig pushed a commit that referenced this pull request Sep 25, 2026
@openhands-agent
fix(review): resume requested reviews on a scheduled check scan
de21349 
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>
neubig pushed a commit that referenced this pull request Sep 25, 2026
@all-hands-bot @openhands-agent
fix(review): resume requested reviews on a scheduled check scan (#654)
c7e3a7d 
* 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>
@openhands-release-bot openhands-release-bot Bot added the released: v0.25.0 Shipped in v0.25.0 label Sep 27, 2026
@openhands-release-bot

openhands-release-bot Bot commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

🚀 Released in v0.25.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@VascoSch92 VascoSch92 VascoSch92 approved these changes

Assignees

No one assigned

Labels

released: v0.25.0 Shipped in v0.25.0 type: fix A bug fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(review): reject failed required checks before launching an agent

4 participants

@all-hands-bot @neubig @VascoSch92 @openhands-agent