Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions plugins/pr-review/agent-canvas-automation/README.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
# agent-canvas-automation

This directory is the **source-of-truth copy** of the OpenHands Automations
tarball that powers the `github-pr-review` automation on the
`m0nklabs/cryptotrader` repo, run by `agent-canvas` on the `m0nk111-post`
bot user.
tarball that powers a `github-pr-review`-style cron automation on a single
agent-canvas deployment.

## What this is for

Expand Down
38 changes: 35 additions & 3 deletions plugins/pr-review/agent-canvas-automation/v2.7/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@
from pathlib import Path
from urllib.parse import urlencode

# REPO is the deployment-specific target. It is set by the `/pr-reviewer:setup`
# skill's Step 6 ("Apply exactly five constant substitutions near the top of
# the file"), which substitutes this string with the operator's chosen
# `owner/repo` before the tarball is uploaded. The value below is the source
# of truth that the setup skill operates on, not a runtime override.
REPO = "m0nklabs/cryptotrader"
TRIGGER_LABEL = "openhands-review"
REVIEW_TONE = "thorough"
Expand Down Expand Up @@ -282,6 +287,32 @@ def _list_existing_reviews(
return []


_BOT_LOGIN_CACHE: str | None = None


def _get_bot_login(token: str) -> str:
"""Resolve the GitHub login that owns `token` (the bot user).

Used by the duplicate-review guard to recognise MCP-direct posts: any
review whose `user.login` matches the token owner is "one of ours". This
works for both `GITHUB_TOKEN` and `GITHUB_PERSONAL_ACCESS_TOKEN` because
both authenticate as the bot user that posts the review.
"""
global _BOT_LOGIN_CACHE
if _BOT_LOGIN_CACHE:
return _BOT_LOGIN_CACHE
try:
user, _ = _github_request(token, "GET", "/user")
if isinstance(user, dict):
login = (user.get("login") or "").strip()
if login:
_BOT_LOGIN_CACHE = login
return login
except Exception as exc:
print(f" Warning: could not resolve bot login from token: {exc}")
return ""


def _resolve_event(token: str, pr: dict, requested: str) -> str:
"""Coalesce REQUEST_CHANGES → COMMENT when the reviewer is the PR author.

Expand Down Expand Up @@ -783,21 +814,22 @@ def _check_conversation_completion(
if payload is None:
print(f" PR #{pr_number}: agent did not emit REVIEW_JSON block in final response")
existing = _list_existing_reviews(github_token, REPO, pr_number)
bot_login = _get_bot_login(github_token)
already_posted = any(
(r.get("user") or {}).get("login", "").lower() == "m0nk111-post"
(r.get("user") or {}).get("login", "").lower() == bot_login.lower()
for r in existing
)

if already_posted:
print(
f" PR #{pr_number}: m0nk111-post review(s) already present "
f" PR #{pr_number}: {bot_login} review(s) already present "
f"(agent used MCP directly) — closing"
)
else:
msg = (
"⚠️ **OpenHands completed the review for commit "
f"`{reviewed_sha[:12]}`** but did not produce a parseable "
"`###REVIEW_JSON###` block and no `m0nk111-post` review was "
f"`###REVIEW_JSON###` block and no `{bot_login}` review was "
"found on the PR. Falling back to issue comment.\n\n"
f"```\n{(final or '').strip()[:6000]}\n```"
)
Expand Down
60 changes: 47 additions & 13 deletions plugins/pr-review/agent-canvas-automation/v2.8/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,17 @@
contiguous.

This is the v2.8 release. The only change vs v2.7 is a duplicate-review
guard: the script now also checks for existing m0nk111-post reviews at the
same commit_id BEFORE posting a parsed payload, in addition to the
"no JSON" path. v2.7 only did the check in the no-JSON path, so a run
where the agent posted via the GitHub MCP AND the script also posted
from the parsed JSON resulted in two reviews with identical content.

The v2.8 fix: if `_list_existing_reviews()` shows a m0nk111-post review
at the same `commit_id` we are about to post at, skip posting and close
the state. The script logs which path was taken ("MCP-direct" or
guard: the script now also checks for existing reviews posted by the bot
user (the GitHub login that owns `GITHUB_TOKEN` / `GITHUB_PERSONAL_ACCESS_TOKEN`,
resolved at runtime via `/user`) at the same commit_id BEFORE posting a
parsed payload, in addition to the "no JSON" path. v2.7 only did the check
in the no-JSON path, so a run where the agent posted via the GitHub MCP
AND the script also posted from the parsed JSON resulted in two reviews
with identical content.

The v2.8 fix: if `_list_existing_reviews()` shows a review by the bot
user at the same `commit_id` we are about to post at, skip posting and
close the state. The script logs which path was taken ("MCP-direct" or
"script-posted") so the run log is unambiguous.
"""

Expand All @@ -34,6 +36,11 @@
from pathlib import Path
from urllib.parse import urlencode

# REPO is the deployment-specific target. It is set by the `/pr-reviewer:setup`
# skill's Step 6 ("Apply exactly five constant substitutions near the top of
# the file"), which substitutes this string with the operator's chosen
# `owner/repo` before the tarball is uploaded. The value below is the source
# of truth that the setup skill operates on, not a runtime override.
REPO = "m0nklabs/cryptotrader"
TRIGGER_LABEL = "openhands-review"
REVIEW_TONE = "thorough"
Expand Down Expand Up @@ -314,6 +321,32 @@ def _existing_bot_review_for_commit(
return None


_BOT_LOGIN_CACHE: str | None = None


def _get_bot_login(token: str) -> str:
"""Resolve the GitHub login that owns `token` (the bot user).

Used by the duplicate-review guard to recognise MCP-direct posts: any
review whose `user.login` matches the token owner is "one of ours". This
works for both `GITHUB_TOKEN` and `GITHUB_PERSONAL_ACCESS_TOKEN` because
both authenticate as the bot user that posts the review.
"""
global _BOT_LOGIN_CACHE
if _BOT_LOGIN_CACHE:
return _BOT_LOGIN_CACHE
try:
user, _ = _github_request(token, "GET", "/user")
if isinstance(user, dict):
login = (user.get("login") or "").strip()
if login:
_BOT_LOGIN_CACHE = login
return login
except Exception as exc:
print(f" Warning: could not resolve bot login from token: {exc}")
return ""


def _resolve_event(token: str, pr: dict, requested: str) -> str:
"""Coalesce REQUEST_CHANGES → COMMENT when the reviewer is the PR author.

Expand Down Expand Up @@ -810,8 +843,9 @@ def _check_conversation_completion(
# BEFORE deciding what to do — this prevents duplicates whether
# the agent emitted the JSON contract or used the MCP directly.
existing_reviews = _list_existing_reviews(github_token, REPO, pr_number)
bot_login = _get_bot_login(github_token)
existing_bot = _existing_bot_review_for_commit(
existing_reviews, "m0nk111-post", reviewed_sha
existing_reviews, bot_login, reviewed_sha
)

payload = _parse_review_payload(final)
Expand All @@ -821,14 +855,14 @@ def _check_conversation_completion(
if existing_bot is not None:
print(
f" PR #{pr_number}: agent did not emit REVIEW_JSON block but "
f"m0nk111-post review #{existing_bot['id']} already exists at "
f"{bot_login} review #{existing_bot['id']} already exists at "
f"commit {reviewed_sha[:12]} (agent used MCP directly) — closing"
)
else:
msg = (
"⚠️ **OpenHands completed the review for commit "
f"`{reviewed_sha[:12]}`** but did not produce a parseable "
"`###REVIEW_JSON###` block and no `m0nk111-post` review was "
f"`###REVIEW_JSON###` block and no `{bot_login}` review was "
"found on the PR. Falling back to issue comment.\n\n"
f"```\n{(final or '').strip()[:6000]}\n```"
)
Expand All @@ -845,7 +879,7 @@ def _check_conversation_completion(
# produce a duplicate Pull Request Review with identical content.
if existing_bot is not None:
print(
f" PR #{pr_number}: m0nk111-post review #{existing_bot['id']} "
f" PR #{pr_number}: {bot_login} review #{existing_bot['id']} "
f"already exists at commit {reviewed_sha[:12]} (likely agent used "
f"the GitHub MCP directly) — skipping script post to avoid duplicate"
)
Expand Down
26 changes: 15 additions & 11 deletions plugins/pr-review/agent-canvas-automation/v2/README.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
# agent-canvas-automation — v2

This directory holds the **standalone OpenHands automation** that the
agent-canvas `github-pr-review` automation runs on cron inside the
`m0nklabs/cryptotrader` repo. It is the bit of glue that:
agent-canvas `github-pr-review` automation runs on cron. It is the bit
of glue that:

1. **Polls** `m0nklabs/cryptotrader` for open PRs carrying the
1. **Polls** the configured repo for open PRs carrying the
`openhands-review` label (configurable via `TRIGGER_LABEL`).
2. **Forks a fresh OpenHands conversation** per `(PR, label_event_id)` and
feeds it the prompt template (see `_build_review_prompt` in `main.py`).
Expand Down Expand Up @@ -78,18 +78,22 @@ the result-poster. Concretely:
| Path validation | n/a | Inline comments with `path` not in the PR diff are dropped with a log line (GitHub returns 422 for unknown paths) |
| Author-equals-reviewer | n/a | `REQUEST_CHANGES` is auto-downgraded to `COMMENT` when the bot user is the PR author (GitHub returns 422 otherwise) |
| Re-review guard | n/a | Already-closed `(PR, label_event_id)` pairs are skipped on the next cron tick instead of starting a new conversation |
| MCP-direct detection | n/a | If the agent posts the review via the GitHub MCP instead of the JSON contract, the script queries GitHub for existing `m0nk111-post` reviews and closes the state without double-posting |
| MCP-direct detection | n/a | If the agent posts the review via the GitHub MCP instead of the JSON contract, the script queries GitHub for existing reviews by the bot user (resolved at runtime via `GET /user` with the same token that posts the review) and closes the state without double-posting |
| `###REVIEW_JSON###` parser | n/a | Brace-counting parser that handles both fenced ```json` and raw inline JSON, with a "last-marker wins" rule so descriptive prose mentioning the marker doesn't shadow the real JSON contract |

### Real-world demonstration

| PR | Outcome | Reference |
|---|---|---|
| PR 379 (old, before v1 was uploaded) | Used the new format because v1 wasn't yet the prompt | https://github.com/m0nklabs/cryptotrader/pull/379 |
| PR 380, 381 (with v1 tarball) | Old format (single issue-comment blob) | https://github.com/m0nklabs/cryptotrader/issues/4718914883 (the bad blob) |
| PR 380, 381, 387 (with v2.5/2.6) | New format with inline review threads | https://github.com/m0nklabs/cryptotrader/pull/381#pullrequestreview-4507050239 |
| PR 400 (with v2.7) | New format — but **2 reviews with identical content** landed on the PR (agent's MCP-direct post + script's post from the parsed JSON). The v2.7 script's MCP-detection only ran in the "no JSON" path. | https://github.com/m0nklabs/cryptotrader/pull/400 |
| PR 402 (clean e2e test, with v2.8) | New format, **exactly 1 review**. The v2.8 duplicate-review guard runs in BOTH the "no JSON" and "have JSON" paths. | https://github.com/m0nklabs/cryptotrader/pull/402 |
The version timeline below is the source of truth for what each release
changed. For concrete observed behaviour (PR-by-PR), see the deployment's
own review history — this README deliberately avoids hardcoded links to
specific PRs/threads so it stays portable across deployments.

| Version stage | Expected behaviour on a fresh e2e test |
|---|---|
| v1 | Agent outputs a single Markdown blob; script posts it as one issue comment (no inline threads, no suggestion blocks). |
| v2.1–v2.6 | Agent emits `###REVIEW_JSON###`; script posts a single Pull Request Review with one inline thread per finding. Path-validation and PR-author coalescing are wired up. |
| v2.7 | Last-marker parser means agent prose that mentions the marker no longer shadows the JSON contract. MCP-direct post is detected in the "no JSON" path. |
| v2.8 | Duplicate-review guard runs in **both** the "no JSON" and "have JSON" paths. A run where the agent posts via MCP and the script also posts from the parsed JSON produces exactly one review, not two. |

## How to apply this locally

Expand Down
4 changes: 1 addition & 3 deletions skills/github-pr-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ triggers:

Post a single **Pull Request Review** with one **inline comment per finding** — never a single blob comment. The body of each inline comment follows the `github-code-quality[bot]` format so reviewers can scan, discuss, and one-click-apply suggestions directly on the diff line.

Reference example of the target format: https://github.com/m0nklabs/cryptotrader/pull/379 (16 inline review threads, each on its own file/line, each with a `## <Category>` heading, one-line statement, `---` separator, and prose fix guidance).

## Pre-Review Checks (run before drafting the review)

1. **PR is still open:**
Expand Down Expand Up @@ -72,7 +70,7 @@ Rules:
- **Scope confirmation** explicitly says no other code changes / no new imports / no new methods are needed. This is what differentiates a focused review from a sprawl.
- **` ```suggestion ``` ` block** is appended at the end when, and only when, the change can be expressed as a contiguous replacement of ≤ 5 lines on the new (right) side. For multi-region, architectural, or ambiguous changes, omit the block — describe the fix in prose instead.

### Worked example (matches the format on PR #379)
### Worked example (one finding, full template)

```
🟠 Important: Unused import
Expand Down
Loading