Skip to content

fix(pr-review): post reviews via Pull Request Reviews API, not issue comments#1

Merged
m0nk111 merged 1 commit into
mainfrom
pr-review/format-issues-as-review-threads
Jun 16, 2026
Merged

fix(pr-review): post reviews via Pull Request Reviews API, not issue comments#1
m0nk111 merged 1 commit into
mainfrom
pr-review/format-issues-as-review-threads

Conversation

@m0nk111

@m0nk111 m0nk111 commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Problem

On real PRs, the github-pr-review agent posts a single gh pr comment (issue-level blob) instead of a POST /repos/{owner}/{repo}/pulls/{n}/reviews call with a comments[] array. That bypasses inline diff anchoring, breaks the suggestion-block UX, and produces no per-finding threads.

Concrete observation: on m0nklabs/cryptotrader#379, the github-code-quality bot left 16 inline review threads (one per finding, each with a Commit suggestion button). The OpenHands agent on the same PR left one Markdown blob in the PR conversation, with zero inline threads — i.e. it used the wrong API.

The previous SKILL.md and prompt did say to bundle findings into one Reviews API call, but the agent routinely ignores the spec and falls back to gh pr comment. The likely cause is that the prompt's primary instruction is "keep the review body brief unless your active review instructions require a longer structured format", which is weak and easily overridden by the agent's default behaviour. The spec was hidden several layers down inside the skill.

Fix

Two changes:

  1. plugins/pr-review/scripts/prompt.py — insert a ## How to Post the Review (CRITICAL — read first) block directly under the /github-pr-review trigger. It mandates the Reviews API, shows a worked gh api + JSON example with one entry per finding, and explicitly forbids gh pr comment / POST /issues/{n}/comments. The block is positioned before the existing "keep the review body brief" line so the strongest directive fires first.

    The example uses {{owner}}/{{repo}}/{{n}} escapes (rendering as literal {owner}/{repo}/{n}) so the LLM sees a realistic-looking gh command. The real pr_number placeholder stays substituted.

  2. plugins/pr-review/skills/github-pr-review/SKILL.md — rewrite to align the spec with the github-code-quality[bot] format. New sections: Pre-Review Checks, Key Rule: One PR Review / One API Call / Many Inline Comments, Inline Comment Body Format, Worked example, Multi-Line Suggestion Blocks, How Suggestions Actually Work (Mandatory Verification), When to Use a Suggestion Block, Good vs Bad Inline Comments (borrowed from Aider reviewer_tier1.md), Edge Cases, Summary Checklist.

  3. skills/index.js — auto-regenerated by scripts/build-skills-catalog.mjs to mirror the new SKILL.md content.

Tests

All 33 tests in test_pr_review_prompt.py, test_github_pr_review_skill.py, test_pr_review_feedback.py, and test_pr_review_diff_payload.py pass. The full non-env-blocked suite (266 tests) is green.

How to verify on PR #379

After this PR is merged, re-trigger the review on m0nklabs/cryptotrader#379 by adding the review-this label. The new review should:

  • Land as a single PR Review (visible in the "Reviews" section, not the "Conversation" section).
  • Contain one inline review thread per finding, each anchored to a path:line with side: "RIGHT".
  • Each thread body should follow the ## <Priority> <Category> shape, with 🔴 Critical / 🟠 Important / 🟡 Suggestion.
  • Concrete fixes should end with a ```suggestion ``` block where the fix is ≤ 5 lines and contiguous.

Note

This is a copy of OpenHands#339 on my fork.

Co-authored-by

Generated by an AI agent (OpenHands) on behalf of the maintainer.

…comments

The github-pr-review skill and the agent prompt both documented the
correct API (POST /repos/{owner}/{repo}/pulls/{n}/reviews with a
comments[] array), but on real PRs the agent kept producing a single
gh pr comment (issue-level blob) instead of a proper review with
inline threads and suggestion blocks.

Two changes:

1. plugins/pr-review/scripts/prompt.py: insert a '## How to Post the
   Review (CRITICAL — read first)' block right under the /github-pr-review
   trigger. It includes a worked bash + JSON example, mandates the
   Reviews API, and explicitly forbids gh pr comment / POST
   /issues/{n}/comments. The {owner}/{repo}/{n} tokens inside the
   example are escaped ({{...}}) so they survive str.format()
   substitution; the existing pr_number placeholder stays as a real
   substitution target.

2. skills/github-pr-review/SKILL.md: rewrite to align the spec with
   the github-code-quality[bot] format. New sections: Pre-Review
   Checks, 'One PR Review, One API Call, Many Inline Comments',
   Inline Comment Body Format (## <Priority> <Category> + one-line
   statement + --- separator + 'Best fix in this file' anchor + scope
   confirmation + optional suggestion block), worked example, Good
   vs Bad Inline Comments, edge cases, and a Summary Checklist.
   The 'How Suggestions Actually Work' section heading is preserved
   (test_skill_explains_replace_semantics depends on it).

3. skills/index.js: auto-regenerated by scripts/build-skills-catalog.mjs
   to reflect the updated SKILL.md.

All 33 tests in test_pr_review_prompt.py, test_github_pr_review_skill.py,
test_pr_review_feedback.py, and test_pr_review_diff_payload.py pass.

Reference target: m0nklabs/cryptotrader#379
@m0nk111 m0nk111 merged commit 0e42505 into main Jun 16, 2026
m0nk111-post pushed a commit that referenced this pull request Jun 17, 2026
…te guard + deployment-neutral config

Consolidates the work that was previously proposed as PRs #1, #2 and #3
into a single reviewable change.

## What this does

1. Steers the github-pr-review skill and the agent prompt at the GitHub
   Pull Request Reviews API (POST /repos/{owner}/{repo}/pulls/{n}/reviews
   with a comments[] array) instead of the issue-comments API. Inline
   threads, suggestion blocks, and one-click-apply now actually work.

2. Adds the agent-canvas-automation cron runner that watches one repo
   for a trigger label, forks a fresh OpenHands conversation per
   (PR, label_event_id), waits for the conversation to finish, parses
   a ###REVIEW_JSON### contract out of the final response, and posts
   the result as a single Pull Request Review. v2.7 and v2.8 are
   shipped together; the difference between them is the duplicate-review
   guard (see below).

3. Makes the duplicate-review guard robust: before posting a parsed
   payload, the script queries GitHub for an existing review by the
   bot user at the same commit_id. If one is found, the script closes
   the state without re-posting — that fixes the race where the agent
   posts via the GitHub MCP and the script also posts from the parsed
   JSON, producing two reviews with identical content.

4. Removes every hardcoded reference to the original test deployment
   from this repo. The cron runner is generic now: REPO is an empty
   string with a TODO pointing at /pr-reviewer:setup, the bot-user
   check is resolved at runtime via GET /user with the same token that
   posts the review, and the SKILL.md / README references are
   deployment-neutral.

## Files

M  plugins/pr-review/scripts/prompt.py
M  skills/github-pr-review/SKILL.md
M  skills/index.js
A  plugins/pr-review/agent-canvas-automation/README.md
A  plugins/pr-review/agent-canvas-automation/v2/README.md
A  plugins/pr-review/agent-canvas-automation/v2.7/main.py
A  plugins/pr-review/agent-canvas-automation/v2.8/main.py

## Behavioural impact

- New env vars: none.
- One extra GET /user per cron tick (cached thereafter). That's well
  within the 5000/hr PAT budget.
- Until REPO is filled in by /pr-reviewer:setup, _verify_token_and_repo
  will fail with a 404 against an empty path — fail-fast by design.

## Verification

- python3 -c 'import ast; ast.parse(...)' on v2.7 + v2.8 main.py -> OK
- grep -rnE 'm0nklabs|cryptotrader|m0nk111-post|pull/379|PR 379|PR #379'
  on the whole tree -> zero hits (the previous test-deployment literal
  that used to live in REPO = ... is gone; REPO is now empty).
- node scripts/build-skills-catalog.mjs regenerated skills/index.js
  cleanly.

Co-authored-by: openhands <openhands@all-hands.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants