Skip to content

fix(github): avoid duplicate reviewer and mention triggers - #724

Open
neubig wants to merge 1 commit into
mainfrom
fix/single-review-request-path
Open

neubig wants to merge 1 commit into
mainfrom
fix/single-review-request-path

Conversation

@neubig

@neubig neubig commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

HUMAN:


AGENT:
This PR was created by an AI agent (OpenHands) on behalf of the user.

Why

On OpenHands/software-agent-sdk#5485, an agent requested all-hands-bot through the reviewer API and also posted an @mention asking for review. The two independent automations both ran: one submitted a formal approval, and the other posted a second review as a PR comment. This was duplicate work, not a retried review.

Live automation run records confirmed reviewer dispatch at 21:23:09 UTC and mention dispatch at 21:23:12 UTC. The resulting formal review and review-shaped comment appeared at 21:37:18 and 21:51:28 respectively.

Summary

  • Teach the GitHub skill to use one reviewer-assignment trigger, not assignment plus a mention.
  • Check pending requests/reviews before retrying; use a documented alternative only after assignment fails.
  • Update the skill README and regenerate the shipped skill catalog.

Issue Number

Incident investigated on OpenHands/software-agent-sdk#5485

How to Test

  • npm run build:skills succeeded.
  • python scripts/sync_extensions.py --check succeeded (existing non-blocking marketplace coverage warning).
  • git diff --check passed.
  • The documented gh pr edit ... --add-reviewer ... command was exercised against the incident PR and created the dedicated reviewer run. No extra live review was requested for validation.

Separately, the custom deployed mention responder now instructs review requests to defer to the dedicated reviewer, inspect pending/current-head reviews, and avoid review-shaped comments. Ordinary questions and fix requests retain their normal handling. Source compilation and actual Mention/prompt rendering passed; deployed tarball hash matched the local bundle. Called POST /api/automation/v1/git-sync/sync and confirmed dirty_count=0, last_error=null and completed sync commit d5b5b575361f7aa5930831cc2c835826ab0b1343 in OpenHands/oss-agent-canvas. That custom automation is not owned by this public skill repository and is not copied here.

Video/Screenshots

Not applicable; instruction change and API/run-history investigation.

Notes

This prevents the agent instruction pattern that caused the duplicate. The deployed mention routing is prompt-level, not an atomic cross-automation deduplication guarantee; no LLM behavior canary was run. No existing reviews were deleted or altered.

Tracking

Fixes #759 — fix(github): avoid duplicate reviewer and mention triggers

Added by an AI agent (OpenHands) on behalf of Graham Neubig to organize previously unlinked PRs.

@github-actions github-actions Bot added the type: fix A bug fix label Oct 3, 2026

@enyst enyst 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.

I'm an AI agent (Claude Code, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.

Leaving a comment rather than a verdict because this is a draft. The rule itself is good general advice for a default-enabled skill: trigger a review one way only, and check pending requests and reviews before retrying.

To get it ready:

  1. Conflicts with main. skills/github/README.md and skills/index.js conflict. Since this branch was cut, #696 added a "Connection selection" section to skills/github/SKILL.md and the README ("use authenticated GitHub MCP tools first when they are available"). Rebase and rerun npm run build:skills.
  2. Make the new section independent of the mechanism. SKILL.md:40-54 names gh pr edit ... --add-reviewer as the single path. After the rebase, the same skill tells agents to prefer MCP tools, so an agent with GitHub MCP gets two conflicting instructions. Something like this would cover both: "use the repository's documented review trigger once (a reviewer request through MCP, gh pr edit --add-reviewer, or the REST endpoint), and never add a second trigger." Labels also need to be covered. This repo's own plugins/pr-review starts a review on a reviewer request or on the review-this label (README "Automatic Triggers"), so adding the label on top of a reviewer request is the same double-trigger.
  3. PR gates for when you un-draft. The HUMAN: note is still the placeholder. ## Issue Number only has an SDK PR URL, but check_pr_description.py needs a #N reference to an extensions issue, labeled ready-for-dev if it was created after 2026-08-25.

The sync-sdk-skill failure on this head comes from the stale base and should clear after the rebase. The required checks are green.

Co-authored-by: openhands <openhands@all-hands.dev>
@neubig
neubig force-pushed the fix/single-review-request-path branch from 0eb19f8 to 5bc4da0 Compare October 5, 2026 21:11
@neubig

neubig commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Thanks — addressed points 1 and 2 in 5bc4da0 (rebased onto main).

  1. Conflicts resolved. Rebased onto main (25aa536) and reran npm run build:skills. The skills/index.js and README.md conflicts are gone; the "Connection selection" section from docs: prefer MCP tools for service integrations #696 and the new "Requesting a Review" section now coexist.

  2. Mechanism-independent, labels covered. The section no longer names gh pr edit --add-reviewer as the single path. It now says: trigger a review exactly once through one path — a reviewer request via MCP tools (preferred when available), gh pr edit --add-reviewer, or the REST requested_reviewers endpoint. It explicitly calls out that adding the review-this label on top of a reviewer request is the same double-trigger, citing the reviewer-request and label webhooks starting independent agents (matching plugins/pr-review README "Automatic Triggers" items 3 and 4). The @mention case remains covered.

Local validation: build:skills, sync_extensions.py --check, check_plugin_resources.py, sync_openhands_sdk_skill.py --check, and git diff --check all pass. The sync-sdk-skill failure was the stale base, as you predicted.

Not addressed, and left for the human author:

  • Point 3: the HUMAN: section is still the placeholder, and ## Issue Number references an SDK PR URL rather than a #N extensions issue. I am instructed not to edit the HUMAN section, and there is no extensions issue to reference, so this needs the author. Leaving the PR in draft.

This comment was posted by an AI agent (OpenHands) on behalf of the user.

@neubig
neubig marked this pull request as ready for review October 8, 2026 16:34
@neubig
neubig requested a review from all-hands-bot October 8, 2026 16:34

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This review was posted by an AI agent (OpenHands).

Scope

In scope. This is an instruction/documentation change to skills/github (SKILL.md + README.md) plus the regenerated skills/index.js, which matches this repo's rules ("one skill per change"; "edit the hand-authored source once and regenerate derived catalogs"). No repository move or product/architecture decision is required.

Verification

  • npm run build:skills regenerates skills/index.js with no diff on this head, so the shipped catalog matches the hand-authored SKILL.md.
  • uv run python scripts/sync_extensions.py --check passes (only the pre-existing, non-blocking marketplace coverage warning for plugins/issue-duplicate-checker).
  • uv run pytest tests/test_skills_catalog.py tests/test_skills_have_readme.py tests/test_sync_extensions.py tests/test_sdk_loading.py -> 98 passed; tests/test_github_pr_review_skill.py tests/test_github_skill_installation.py tests/test_pr_review_prompt.py -> 72 passed.
  • The claim is accurate against the workspace: plugins/pr-review/workflows/pr-review-by-openhands.yml triggers on both labeled (review-this) and review_requested, so the "label on top of a reviewer request" double-trigger the new section warns about is real, and the mechanism-independent phrasing (MCP / gh pr edit --add-reviewer / REST requested_reviewers) is correct.

Findings

  1. Blocking: the repository's own PR-description gate fails on this head. The PR is no longer a draft (draft: false), so .github/workflows/pr-description-check.yml runs, and Validate PR description concluded failure for 5bc4da0. Re-running .github/scripts/check_pr_description.py against the current body reproduces two errors: the HUMAN: note is still the placeholder (under 20 chars), and ## Issue Number contains no #N reference to an extensions issue. The author comment says point 3 was left for the human author and that the PR stays in draft, but the PR is currently marked ready, so the gate is active and red. This must be resolved (add the human note, and either link a ready-for-dev extensions issue or convert the PR back to a draft) before merge.
  2. Non-blocking: em dashes violate the repository punctuation convention. AGENTS.md states: use plain hyphens instead of em dashes in SKILL.md content and marketplace JSON. The new prose introduces two U+2014 characters, at skills/github/SKILL.md:79 and skills/github/README.md:52.

Verdict

The core guidance is correct, well-scoped, and the generated catalog is in sync. The merge is blocked by the failing PR-description gate, and the em-dash convention violation is a small but mechanical fix on the changed lines.

🔄 CHANGES REQUESTED

Comment thread skills/github/SKILL.md
request is the same double-trigger: a reviewer-request webhook and a
label webhook can each start an independent agent, producing a formal review
and a second review-shaped comment for the same request. Do not post an
`@reviewer please review` comment either — a mention responder can start yet

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking: this line introduces a U+2014 em dash. AGENTS.md ("Repository conventions") says to use plain hyphens instead of em dashes in SKILL.md content and marketplace JSON. The same applies to the em dash in skills/github/README.md:52.

This branch has not been deployed

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

Labels

type: fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(github): avoid duplicate reviewer and mention triggers

4 participants