Repository navigation
fix(review): stop repeat reviews of an unchanged head - #699
Merged
Merged
Conversation
A review was published up to three times on one head (#698 got three APPROVED reviews on c9a6c7d). Two independent causes: 1. The delivery key counts the trigger event, so every fresh review_requested or re-applied label produced a new key and started another review at the same head. The dispatcher now also dedupes on the head it records, and a conversation already running that head is reported in-progress rather than restarted. 2. The reviewer itself only recognized a completed review as its own result when the review followed the trigger event. A review that predated the event re-dispatched. The worker now treats a review on the current head with no clarifying comment since as done, and says so in the log. The prompt carries the same rule, since the agent is what reads GitHub before publishing: do not publish a second review of a head already reviewed, unless the head moved or someone else commented after that review. Callers with no revision identity pass no head and keep the delivery-key-only behavior. Regenerate automations/bundle-index.js for the inlined dispatcher.
16 tasks
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: This change belongs in OpenHands/extensions - it fixes the bundled github-pr-reviewer automation's dispatcher/worker, the extension content this repository owns. No product/architecture decision is required.
What I verified on head e4e4dce
- Required status checks for the
mainruleset all pass:test,sync-extensions,validate-claude-code. npm run build:automationsis a no-op, and the inlinedagent_conversation.py/worker.pyinautomations/bundle-index.jsmatch the sources byte-for-byte, so the staleness test is satisfied.- Full suite in the workspace:
1074 passed, 23 skipped(your stated count differs; the delta is environmental, not a failure). - The new regression tests fail against the pre-change source (
TypeError: deliver() got an unexpected keyword argument 'head'and wrong-disposition assertions) and pass after, so they pin the fix rather than the implementation.
Assessment
The fix is correct and well-targeted. Both root causes are addressed: the revision is now part of the KV record so a repeated delivery key at an unchanged head reports in_progress instead of starting a second turn, and the pre-dispatch guard stops a fresh trigger from re-reviewing a head this account already reviewed. The backward-compatible head="" default leaves the issue triage / issue-to-PR callers on their previous behavior, which I confirmed at skills/github-issue-triage/scripts/worker.py and skills/github-issue-to-pr/scripts/worker.py.
One behavioral gap is worth addressing before merge - see the inline comment: the guard treats a GitHub-dismissed review as a standing review, so an unchanged head can become permanently unreviewable.
Non-blocking note: the Validate PR description check is red on this head because the description does not open with the HUMAN: section now required by the template on main (added by #698). It is not one of the ruleset's required checks, so it does not block merge, but you may want to add the human/agent sections for consistency.
Verdict:
🔄 CHANGES REQUESTED
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Re-established state at head e4e4dceff1b70ecc4e4b2d341a5bf02be79d808c: PR open, label type: fix, no prior reviews or comments, and every CI check green (test, sync-extensions, validate-claude-code, Validate PR description, etc.). Linked issue #700 is open with ready-for-dev and priority:high. In the workspace I ran the focused tests (uv run pytest -q tests/test_github_reviewer_delivery.py tests/test_agent_conversation_dispatch.py -> 98 passed) and the full suite (uv run pytest -q -> 1074 passed, 23 skipped), and npm run build:automations produced no drift in automations/bundle-index.js.
The two root causes from #700 are addressed: the dispatcher persists the revision head and returns in_progress for a head already under review, and the worker's pre-dispatch _reviewed_current_head_without_clarification gate suppresses a repeat review_requested / re-applied label on an already-reviewed head, while _clarified_since re-enables it on another account's later comment. Backward compatibility for no-head callers is preserved, and the new tests pin the #698 sequence.
One correctness/state issue in the new gate (see inline comment):
- The guard's call to
_finish_completed_review(pr, trigger, trigger_label)is annotated as clearing the label, but it cannot do that in the exact scenario the guard targets._finish_completed_reviewonly treats a review as this trigger's result whensubmitted_at > trigger.created_at; the #698 ordering is a review that predates the re-applied trigger, so it computes no completed review and returns before theDELETE /issues/{number}/labels/{label}. The guard thencontinues, so on a label deployment the label is never removed and every later scan re-reads the PR and re-logsreview-already-publishedindefinitely.
Non-blocking: skills/github-pr-reviewer/SKILL.md ("Each explicit review request - a re-applied trigger label, or a new reviewer request in event mode - is a fresh review") and README.md ("Supports re-review by requesting the bot again or re-applying the label") now contradict the enforced behavior for an unchanged, already-reviewed head. The linked issue's acceptance criteria call for these docs to describe the same-head dedup rule and the clarifying-comment path, so that documentation update is still outstanding.
🔄 CHANGES REQUESTED
Two gaps in the unchanged-head guard, both raised in review of #699: - The guard counted a dismissed review as standing. GitHub keeps a dismissed review in /pulls/{n}/reviews with its commit_id, so a maintainer who dismissed the verdict and re-requested a review on the same head got `review-already-published` forever. Skip DISMISSED reviews, matching maintainer_handoff._reviewer_states. - The `review-already-published` branch claimed to clear the label but delegated to _finish_completed_review, which returns early when the review predates the trigger (#698 ordering) and never reaches its DELETE. Clear the label directly in that case so a later scan does not re-read the same head forever. Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
approved these changes
Sep 29, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: This change belongs in OpenHands/extensions — it fixes the bundled github-pr-reviewer automation's dispatcher and worker, the extension content this repository owns. No product/architecture decision is required.
What I verified on head 74d4878
- Required
mainruleset checks all pass:test,sync-extensions,validate-claude-code. npm run build:automationsis a no-op, and the inlinedagent_conversation.py/worker.pyinautomations/bundle-index.jsmatch the sources byte-for-byte, so the drift gate is satisfied.- Full suite:
1076 passed, 23 skipped(the description's1074is the pre-rebase count; not a failure). - Both new tests fail against
e4e4dce(the prior head) and pass on74d4878, so they pin the fixes rather than the implementation.
Prior findings are resolved. My earlier review on the previous head raised two issues, and both are fixed in this head:
- The dismissed-review gap —
_reviewed_current_head_without_clarificationnow skips reviews whosestateisDISMISSED, matchingmaintainer_handoff._reviewer_states. I reproduced a dismissed✅ APPROVEDreview on an unchanged head: the previous head suppressed dispatch; this head reviews again. - The stale-trigger label leak — when the review predates the trigger,
_finish_completed_reviewreturns False and no longer leaves the label on the PR; the guard now deletes it directly, so later scans do not re-read and re-log the same head forever.
Assessment. The core fix is correct and well-targeted. The revision is now part of the KV record, so a repeated delivery key at an unchanged head reports in_progress rather than starting a second turn, and the pre-dispatch guard prevents a fresh trigger from re-reviewing an already-reviewed head. The head="" default keeps the issue triage / issue-to-PR callers (skills/github-issue-triage/scripts/worker.py, skills/github-issue-to-pr/scripts/worker.py) on their prior delivery-key-only behavior. The two follow-up fixes are properly tested.
Non-blocking notes (no action required for merge):
- The
_has_current_head_reviewpredicate on the unrequested scheduled-scan path (line ~791) still counts a dismissed review as a standing review, so an unrequested PR whose only bot review was dismissed is skipped by the scan. Impact is bounded: the dismissed state does not persist on GitHub — a dismissal is followed either by a new review request/label (handled by the guard) or by a new head on the next push (the old-commit review no longer matches), so the head is not permanently stuck. Aligning this predicate with the sameDISMISSEDrule would make the two paths consistent. - The description's
HUMAN:section explicitly notes no human has tested the change. The gate behavior is exercised by tests, but the live duplicate-review symptom is only observable in a real deployment, so the rollout follow-up on the oss-agent-canvas VM matters for confirming the fix end-to-end.
Verdict:
✅ APPROVED
1 task
hieptl
added a commit
that referenced
this pull request
Oct 1, 2026
…ared scripts The previous commit gave a minor version to every bundle that ships the shared GitHub scripts. A bundle's version records the shape of its files and config, not their contents, and #699 changed the same scripts without a bump. Restore those five versions and the fixture that pins one of them.
hieptl
added a commit
that referenced
this pull request
Oct 2, 2026
…d Enterprise (#712) * feat(jira-issue-to-pr): run the Jira to GitHub PR automation on OpenHands Cloud The skill's deploy instructions hardcoded http://localhost:8000 with the local session key, and its poller script only worked on a local agent server: it read AGENT_SERVER_URL, which the automation service injects in local mode only, authenticated its completion callback with AUTOMATION_CALLBACK_API_KEY (also local-only), fetched LLM settings from the sandbox's agent server, and started conversations inside the run's sandbox, which the service tears down once the run reports completion. - Branch the script on AGENT_SERVER_URL, the signal the service's presets use. On OpenHands Cloud and Enterprise it reads secrets from /api/v1/sandboxes/{SANDBOX_ID}/settings/secrets/{name}, authenticates the callback with OPENHANDS_API_KEY, requires the KV store (each run starts in a fresh sandbox), and starts each conversation through POST /api/v1/app-conversations, which runs it in its own sandbox with the user's settings, secrets and connected git provider. The Jira comment links the canvas conversation URL. The local path is unchanged. - Document the cloud host and auth in SKILL.md and switch the curl steps to OPENHANDS_HOST and AUTH_HEADER; regenerate skills/index.js. - Add tests that run the script against a stub server in both modes. Refs OHE-3160 * feat(automations): run the template bundles and skills on OpenHands Cloud The automation templates were written against a local Agent Canvas run: their scripts read AGENT_SERVER_URL and a local callback key, the three agent-profile bundles import the SDK, and the skills tell the agent to find the automation backend in <RUNTIME_SERVICES>. A run on OpenHands Cloud or Enterprise gets OPENHANDS_CLOUD_API_URL, OPENHANDS_API_KEY and SANDBOX_ID instead, executes on a bare sandbox Python, and its sandbox is removed once it reports completion. - agent_conversation.py: when no Agent Server URL is given, deliver through the OpenHands API. Conversations start in a sandbox of their own with the automation's agent profile, a paused one is resumed before the next turn is sent, and one whose sandbox is gone is replaced, with the KV record following the current id. The SDK imports are optional so the module loads on a sandbox without it. - github_client.py: read secrets from the run's sandbox on cloud, fall back to the connected GitHub integration when the named secret is not saved (and name GITHUB_TOKEN to the agent), and default the workspace. - AGENTS.md maintainer, GitLab issue to MR and news digest: on cloud, start and follow conversations through the OpenHands API, authenticate the callback with the run's API key, require the KV store, and link the Canvas conversation. The agent clones the project itself; a conversation that ends without a pull or merge request is reported rather than finished from a clone the run no longer has. GitLab requests carry the token as a bearer token there, which an integration's OAuth token needs. - Skills that create a prompt-preset automation, and the three bundle skills above, pick the API host and auth for Cloud (<HOST> and Authorization: Bearer $OPENHANDS_API_KEY) or a local Agent Canvas. - Bump the three changed bundles to 1.1.0 and regenerate the indexes. The local path is unchanged. Refs OHE-3160 * docs(automations): document Jira Cloud webhooks for event-triggered automations An agent asked for an event-based Jira automation had nothing to go on: the automation skill listed no Jira settings, so it fell back to the generic signature header and wrote a label filter for a payload shape Jira does not send. The result was a webhook Jira could not deliver to and a trigger that never matched. - openhands-automation: add a Jira Cloud example and table row. Jira signs with X-Hub-Signature, names the event in webhookEvent, sends labels as plain strings, and only an admin-registered webhook can carry a secret. The example filter fires when an issue is created with the label or when an update changes its labels, so later edits do not start another run. - jira-issue-to-pr: add an event-based alternative to the poller, pointing at that example and stating what the poller's script no longer does. * feat(jira-issue-to-pr): set up the Jira webhook for the user Setting up the event-based automation meant opening Jira and pasting a webhook URL and secret by hand. Jira Cloud has a REST endpoint for the same admin webhook (POST /rest/webhooks/1.0/webhook), so the skill can do it. scripts/setup_webhook.py registers the custom webhook source in OpenHands and the matching webhook in Jira, limited to the label by JQL, and sends one signed request to prove the pair works. The signing secret is generated or rotated inside the script and never printed, so it stays out of the conversation. Running it again updates both sides in place; `delete` removes the Jira webhook. An account without the Administer Jira permission exits with code 3 and the skill falls back to the manual steps, which are kept. The skill has the agent show a dry run and wait for the user's confirmation before it changes their Jira site. The automation skill's rule against registering webhooks gets an exception for skills that ship such a script. * fix(automations): correct the cloud paths found in review Conversations on OpenHands Cloud and Enterprise: - OpenHands initializes a Git repository in a conversation's working directory, so `git clone ... .` and `gh repo clone <repo> .` fail there. The GitLab, AGENTS.md and issue-to-PR prompts now clone into a new directory, and the shared GitHub instructions stop naming the run's own workspace path, which means nothing in the conversation's sandbox. - The dispatcher started a second conversation for a subject when a new delivery arrived before the API listed the first. While a start is in its grace window every delivery now waits, with the record left for the next trigger. - The Jira poller marked an issue done as soon as the API accepted the start request, so a start that failed afterwards was lost. The next run now confirms each start, makes a failed one again up to three times, and then reports it on the issue. - `is_cloud_run()` now means what `IS_CLOUD` means in the bundle scripts. Jira webhook setup script: - An existing pair keeps its signing secret: Jira is updated without one, so a failure half way no longer leaves the two sides with different secrets. `--rotate-secret` replaces it on request. - The deployment URL is checked for HTTPS and Jira's webhook API is queried before anything is written. The bundles that ship the shared GitHub scripts get a minor version, and the changed behavior is covered by tests. * docs(automations): say what changes on OpenHands Cloud and Enterprise A conversation started through the OpenHands API runs with the user's own settings, so the news digest's empty secret allow-list and the GitLab template's single-secret allow-list hold only on a local Agent Canvas. The skill text, READMEs and catalog entry now say so, and describe the other cloud differences: the agent clones for itself, an unfinished conversation is reported rather than completed by the script, and a connected git integration stands in for a missing token secret. Also corrects the Jira label filter's description: it still fires when another label changes on an issue that carries the trigger label. * chore(automations): keep the versions of bundles that only pick up shared scripts The previous commit gave a minor version to every bundle that ships the shared GitHub scripts. A bundle's version records the shape of its files and config, not their contents, and #699 changed the same scripts without a bump. Restore those five versions and the fixture that pins one of them. * fix(jira-issue-to-pr): be conservative where the OpenHands API says nothing - A start task the API no longer has is settled like a started one. Treating it as a failure would start a second conversation for an issue whose first one may well have run. - An answer that is not the expected list leaves the pending starts for the next run. - The setup script asks Jira for its webhooks before it registers anything in OpenHands, also when the OpenHands webhook does not exist yet, so an account Jira does not serve the webhook API leaves nothing behind. * fix(github-stale-ci-pr-closer): report completion on cloud runs The closer reported its run with AUTOMATION_CALLBACK_API_KEY, which a run on OpenHands Cloud or Enterprise is not given. The callback was refused, the entrypoint raised, and a run that had done its work ended as failed. It now falls back to the run's API key, as the other bundles that report their own completion do. * fix(jira-issue-to-pr): keep polling when earlier starts cannot be confirmed A failing call to the start-task API ended the run before it polled Jira, so one pending start could stall the automation for as long as the call failed. The run now warns and leaves the pending starts for the next one. --------- Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
🚀 Released in v0.26.0. |
enyst
added a commit
that referenced
this pull request
Oct 4, 2026
…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>
1 task
enyst
added a commit
that referenced
this pull request
Oct 4, 2026
Resolve the hand-written conflicts against main's Cloud/local dispatcher split (#712), unchanged-head dedupe (#699), and full-backlog scan (#703): - agent_conversation.py: keep main's `deliver` -> `_deliver_cloud` / `_deliver_local` structure. The PR's admission gate moves into `_deliver_local` (create, resume of an idle/paused conversation, and a new-revision turn), which now returns a "deferred" disposition that `deliver` reports without writing a delivery record. `deliver` keeps the PR's registry bookkeeping (`_remember`) and reads/writes through `_kv_value` / `_kv_put` because `_kv_request` now returns the whole body. The Cloud path is unchanged here; how the cap applies to it follows in a separate commit. - test_agent_conversation_dispatch.py: keep both new sections, the PR's in-flight/admission tests and main's Cloud tests. - SKILL.md: troubleshooting rows describe the cap on top of main's full-backlog scan (no rotating window). - manifest.json: main's descriptions plus the PR's in-flight cap clause. - A deferred-drain test stub accepts main's `head=` keyword; the ReviewIntake blank line left by main's ScanCursor removal is dropped; fixture `max_in_flight` lines take their neighbours' indentation. - skills/index.js and automations/bundle-index.js regenerated with `npm run build`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
The reviewer published the same review up to three times on one unchanged head. On #698,
all-hands-botposted three separate APPROVED reviews against the identical headc9a6c7d. Re-reviewing identical code costs a full agent runtime per duplicate, and the repeated reviews bury any genuine follow-up in noise.Timeline on #698, from the issue timeline API:
reviewed— commentedreview_requested(neubig)reviewed— approvedreview_requested(neubig)reviewed— approvedreview_requested(all-hands-bot)review_requested(neubig)reviewed— approvedTwo independent causes:
"{pr_number}:label:{label_event_id}"(or thereview_requestedevent id), so every fresh trigger at the same head produced a distinct delivery key. Each request in the table above started its own review ofc9a6c7d, with no dedup on the revision under review. An in-flight review of the same head was likewise unrecognized, so a second conversation could run beside the first._finish_completed_reviewonly treated a review as this run's result when the review followed the selected trigger, so a review that predated a re-applied label or repeat request re-dispatched.Summary
AgentConversationDispatcher.deliver()now takes the revisionheadand persists it in the KV record. A conversation already reviewing that head reportsin_progressand is left alone, so a second delivery key at an unchanged head cannot start a second review beside a live one.PullRequestReviewergained_reviewed_current_head_without_clarification()/_clarified_since(). Before dispatch, an account-published review on the current head with no later comment from anyone else is treated as done, and the worker logsdisposition: "review-already-published". A moved head, or a comment from another account after the review, makes the head eligible again under the normal rules.automations/bundle-index.js(npm run build:automations) — the bundled dispatcher is inlined there, and the repo's own staleness test fails without this.Callers with no revision identity (issue triage, issue-to-PR) pass no
headand keep the previous delivery-key-only behavior, so this is backward compatible.Issue Number
Fixes #700
How to Test
End-to-end evidence, not only unit tests:
1. The generated bundle is in sync with the sources (this is a real drift gate, and it fails if
bundle-index.jsis stale):2. The full suite, on the rebased branch cut from current
main:1074 passedhere is the count on currentmain. The five added tests are the regression coverage:test_reviewer_does_not_re_review_a_head_after_a_repeat_request— replays the fix(ci): adopt HUMAN/AGENT sections in the PR template #698 sequence (a review lands, then a later request arrives at the same head) and asserts exactly one dispatch.test_reviewer_does_not_redeliver_a_label_for_an_already_reviewed_head— a label re-applied after the bot's own handoff dispatches nothing.test_new_delivery_on_a_running_same_head_is_not_a_second_review— a fresh delivery key against a head already under review returnsin_progress.test_new_delivery_on_a_running_new_head_still_sends_a_turn— a moved head still resumes the conversation with a new turn.test_reviewer_redelivers_a_label_when_a_human_clarified_since— a human comment after the review re-enables the head.3. Each new test was confirmed to fail against the pre-change source, so the tests pin the defect rather than the implementation. Reverting
agent_conversation.pyandworker.pytoorigin/mainand re-running the five names produces failures on all five; restoring the change makes them pass.Video/Screenshots
No UI change; the evidence above is terminal output. The observable behavior is a JSON log line and a dispatch count, both asserted in the tests.
Notes
Behavior change worth an explicit decision. The fix makes a repeat
review_requestedon an unchanged, already-reviewed head a no-op. That is unambiguously correct for the bot's own re-requests and for re-applied labels, and it is what stops #698's duplicate reviews. It also means a maintainer cannot force a re-review of an unmoved head by re-requesting; a clarifying comment does re-enable it. If forcing a re-review deliberately should stay possible, that needs a separate escape hatch and this PR should be adjusted — flagged rather than assumed.Related, not overlapping. #646 addresses the delayed
pull_request_review.submittedcompletion path (PR #647); #687 addresses bounded dispatcher state. This PR touches neither.Rollout follow-up. The reviewer automations running on the oss-agent-canvas VM execute the previously-synced code, so duplicate reviews continue there until that deployment picks up this change. I could not reach that VM from this environment to verify or trigger the sync.