From fdb735ac69ba593ff3d68c3862a075e6656ab87e Mon Sep 17 00:00:00 2001 From: openhands Date: Wed, 17 Jun 2026 13:48:10 +0000 Subject: [PATCH] chore(pr-review): resolve bot login at runtime via /user API This commit supersedes the previous commit on this branch (b5496a8). The previous one introduced two new environment variables (PR_REVIEW_TARGET_REPO, PR_REVIEW_BOT_LOGIN) that were both unnecessary: - PR_REVIEW_TARGET_REPO duplicates a value that the /pr-reviewer:setup skill's Step 6 already substitutes into REPO at build time. The tarball is intentionally deployment-specific; baking REPO in is by design. So REPO is back to being a hardcoded string at the top of v2.7/main.py and v2.8/main.py, with a comment explaining that the setup skill rewrites this line before packaging. - PR_REVIEW_BOT_LOGIN duplicates information that is already available from GITHUB_PERSONAL_ACCESS_TOKEN (the same token that posts the review). The dedup guard now resolves the bot user at runtime via a cached GET /user call (see _get_bot_login). That works for both GITHUB_TOKEN and GITHUB_PERSONAL_ACCESS_TOKEN, with no extra config. Other than that the commit covers the same ground as b5496a8: - skills/github-pr-review/SKILL.md: drop the reference URL to m0nklabs/cryptotrader/pull/379 and the 'Worked example (matches the format on PR #379)' header. - skills/index.js: regenerated from the fixed SKILL.md. - plugins/pr-review/agent-canvas-automation/README.md: drop the deployment name from the intro. - plugins/pr-review/agent-canvas-automation/v2/README.md: drop the 7 direct cryptotrader URLs from the demo table, the hardcoded automation ID in the deploy steps, and the m0nk111-post ref in the MCP-detection row. Co-authored-by: openhands --- .../agent-canvas-automation/README.md | 5 +- .../agent-canvas-automation/v2.7/main.py | 38 +++++++++++- .../agent-canvas-automation/v2.8/main.py | 60 +++++++++++++++---- .../agent-canvas-automation/v2/README.md | 26 ++++---- skills/github-pr-review/SKILL.md | 4 +- skills/index.js | 2 +- 6 files changed, 101 insertions(+), 34 deletions(-) diff --git a/plugins/pr-review/agent-canvas-automation/README.md b/plugins/pr-review/agent-canvas-automation/README.md index f3dc2864..11ec8853 100644 --- a/plugins/pr-review/agent-canvas-automation/README.md +++ b/plugins/pr-review/agent-canvas-automation/README.md @@ -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 diff --git a/plugins/pr-review/agent-canvas-automation/v2.7/main.py b/plugins/pr-review/agent-canvas-automation/v2.7/main.py index bfd28792..87da27c1 100644 --- a/plugins/pr-review/agent-canvas-automation/v2.7/main.py +++ b/plugins/pr-review/agent-canvas-automation/v2.7/main.py @@ -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" @@ -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. @@ -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```" ) diff --git a/plugins/pr-review/agent-canvas-automation/v2.8/main.py b/plugins/pr-review/agent-canvas-automation/v2.8/main.py index 23d4d3b8..bb0c6f10 100644 --- a/plugins/pr-review/agent-canvas-automation/v2.8/main.py +++ b/plugins/pr-review/agent-canvas-automation/v2.8/main.py @@ -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. """ @@ -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" @@ -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. @@ -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) @@ -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```" ) @@ -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" ) diff --git a/plugins/pr-review/agent-canvas-automation/v2/README.md b/plugins/pr-review/agent-canvas-automation/v2/README.md index 9131ade4..60336efa 100644 --- a/plugins/pr-review/agent-canvas-automation/v2/README.md +++ b/plugins/pr-review/agent-canvas-automation/v2/README.md @@ -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`). @@ -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 diff --git a/skills/github-pr-review/SKILL.md b/skills/github-pr-review/SKILL.md index 4e93bec9..7f751d0a 100644 --- a/skills/github-pr-review/SKILL.md +++ b/skills/github-pr-review/SKILL.md @@ -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 `## ` heading, one-line statement, `---` separator, and prose fix guidance). - ## Pre-Review Checks (run before drafting the review) 1. **PR is still open:** @@ -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 diff --git a/skills/index.js b/skills/index.js index e80fde60..67dd55a2 100644 --- a/skills/index.js +++ b/skills/index.js @@ -177,7 +177,7 @@ export const SKILLS_CATALOG = [ "triggers": [ "/github-pr-review" ], - "content": "# GitHub PR Review\n\nPost 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.\n\nReference 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 `## ` heading, one-line statement, `---` separator, and prose fix guidance).\n\n## Pre-Review Checks (run before drafting the review)\n\n1. **PR is still open:**\n ```bash\n gh pr view {pr_number} --json state,mergedAt,merged\n ```\n - `state: OPEN` → proceed.\n - `state: MERGED` → stop, nothing to review.\n - `state: CLOSED` and `merged: false` → stop, do not review. The PR was abandoned; route back to planning.\n\n2. **Scope guardrail:** review only within the issue/PR scope and regressions introduced by that scope. Do not flag unrelated cleanup, refactors, or wishlist improvements. If a finding is out of scope, skip it.\n\n3. **Severity filter:** only report findings that are medium severity or higher, have a concrete failure mechanism, and tie to user-visible impact, correctness risk, or metric distortion. Style nits belong to linters, not reviews.\n\n4. **Cap the number of findings.** A focused review with 5 strong findings beats a noisy review with 20 weak ones. If you have more than ~10, keep the top-severity ones and silently drop the rest.\n\n5. **Order by severity:** 🔴 Critical → 🟠 Important → 🟡 Suggestion. Within the same priority, the most user-visible issue goes first.\n\n## Key Rule: One PR Review, One API Call, Many Inline Comments\n\nPost exactly **one** Pull Request Review (`POST /repos/{owner}/{repo}/pulls/{pr_number}/reviews`) whose `comments[]` array contains **one entry per finding**. Each entry becomes a separate inline thread anchored to a `path` + `line` + `side`.\n\n**Do NOT:**\n- Post a single big issue/PR comment that contains all findings as numbered sections. That bypasses the inline diff anchoring, breaks the suggestion-block UX, and is not what `github-code-quality[bot]` does.\n- Post each finding as a separate API call. That fragments the review into N pending reviews and pollutes the timeline.\n\n## Inline Comment Body Format\n\nEach `comments[i].body` string follows this exact template (this is the `github-code-quality[bot]` shape, with the priority label folded into the heading and an optional one-click suggestion block appended):\n\n```markdown\n## \n\n\n\n---\n\n\n\nBest fix in this file (`\n\n\n\n```suggestion\n\n```\n```\n\nRules:\n- **Heading** is `## `. The priority prefix is one of:\n - `🔴 Critical:` — must fix: security, data loss, broken invariants.\n - `🟠 Important:` — should fix: logic errors, performance, missing error handling.\n - `🟡 Suggestion:` — worth considering: clarity, maintainability.\n - **Never** `🟢 Nit` or `🟢 Acceptable`. If the code is fine, do not comment.\n- **One-line statement** names the specific defect at that file/line. No \"consider\", no \"could be better\" — state what is wrong.\n- **`---`** is a literal Markdown horizontal rule. It separates the diagnosis from the fix guidance and is what makes the format scannable in the GitHub UI.\n- **General fix** is one or two sentences explaining the approach (e.g., \"remove only the unused symbol from the import list while keeping used imports intact\").\n- **Best fix here** is anchored to the actual file path and line number in backticks. It is the *one* edit the reviewer should make.\n- **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.\n- **` ```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.\n\n### Worked example (matches the format on PR #379)\n\n```\n🟠 Important: Unused import\n\nImport of 'generate_macd_signal' is not used.\n\n---\n\nTo fix an unused import without changing behavior, remove only the unused symbol from the import statement and keep the rest intact.\n\nBest fix in this file (`core/analysis/indicator_correlation.py`): update line 23 so it imports only `compute_macd`, and remove `generate_macd_signal` from that line. No other code changes are needed, assuming the symbol is indeed unused throughout the file.\n\n```suggestion\nfrom core.indicators.macd import compute_macd\n```\n```\n\n## Posting the Review\n\nUse the GitHub CLI (`gh`) with a JSON input file. The `GITHUB_TOKEN` is automatically available.\n\n**Important**: Always use `--input` with a JSON file instead of `-F` flags. This avoids shell-quoting issues with backticks, quotes, and newlines inside suggestion blocks.\n\n### Step 1: Create the JSON file\n\n```bash\ncat > /tmp/review.json << 'EOF'\n{\n \"commit_id\": \"{commit_sha}\",\n \"event\": \"COMMENT\",\n \"body\": \"Review summary: 2 important findings + 1 suggestion. Inline comments below.\",\n \"comments\": [\n {\n \"path\": \"core/analysis/indicator_correlation.py\",\n \"line\": 23,\n \"side\": \"RIGHT\",\n \"body\": \"🟠 Important: Unused import\\n\\nImport of 'generate_macd_signal' is not used.\\n\\n---\\n\\nTo fix an unused import without changing behavior, remove only the unused symbol from the import statement and keep the rest intact.\\n\\nBest fix in this file (`core/analysis/indicator_correlation.py`): update line 23 so it imports only `compute_macd`, and remove `generate_macd_signal` from that line. No other code changes are needed, assuming the symbol is indeed unused throughout the file.\\n\\n```suggestion\\nfrom core.indicators.macd import compute_macd\\n```\"\n },\n {\n \"path\": \"api/routes/health.py\",\n \"line\": 42,\n \"side\": \"RIGHT\",\n \"body\": \"🟠 Important: Division by None\\n\\n`uptime_seconds` divides by `(now - start_time)` without checking whether `start_time` is set, raising `TypeError` on first startup.\\n\\n---\\n\\nGuard the division with a `None` check and return `0` until the service is fully initialized.\\n\\nBest fix in this file (`api/routes/health.py`): add the guard at line 42 so the function returns `uptime_seconds=0` cleanly when `start_time` is None.\\n\\nNo new imports, methods, or external dependencies are needed.\\n\\n```suggestion\\nif start_time is None:\\n uptime_seconds = 0\\nelse:\\n uptime_seconds = int((now - start_time).total_seconds())\\n```\"\n }\n ]\n}\nEOF\n```\n\n### Step 2: Post the review\n\n```bash\ngh api -X POST repos/{owner}/{repo}/pulls/{pr_number}/reviews --input /tmp/review.json\n```\n\n### Parameters\n\n| Parameter | Description |\n|-----------|-------------|\n| `commit_id` | Commit SHA to comment on (use `git rev-parse HEAD`). |\n| `event` | `COMMENT` (leave as comments), `APPROVE`, or `REQUEST_CHANGES`. |\n| `body` | Brief 1–3 sentence summary. Keep it short — every detail belongs in an inline comment. |\n| `comments[].path` | File path exactly as shown in the diff. |\n| `comments[].line` | 1-based line number in the NEW version (right side of diff). |\n| `comments[].side` | `RIGHT` for new/added lines, `LEFT` for deleted lines. |\n| `comments[].body` | The formatted Markdown described in \"Inline Comment Body Format\" above. |\n| `comments[].start_line` | (Optional) For multi-line suggestion ranges, see below. |\n\n## Multi-Line Suggestion Blocks\n\nFor a fix that spans multiple lines, set `start_line` to the first line of the range. **`start_line`/`line` together define the range that will be REPLACED** when the reviewer clicks \"Commit suggestion\".\n\n```json\n{\n \"path\": \"api/routes/health.py\",\n \"start_line\": 41,\n \"line\": 43,\n \"side\": \"RIGHT\",\n \"body\": \"🟠 Important: ...\\n\\n---\\n\\n...\\n\\n```suggestion\\nif start_time is None:\\n uptime_seconds = 0\\nelse:\\n uptime_seconds = int((now - start_time).total_seconds())\\n```\"\n}\n```\n\n## How Suggestions Actually Work (Mandatory Verification)\n\nA ` ```suggestion ``` ` block **replaces** the targeted range with its contents. The replaced range is:\n\n- `line` only → the single line `line` (replaces 1 line).\n- `start_line` + `line` → the inclusive range `start_line..line` (replaces `line − start_line + 1` lines).\n\nThe suggestion body can be any number of lines — 0 (deletion), 1, or many. It does **not** have to match the range size.\n\n| Intent | `start_line` | `line` | Suggestion body must contain |\n|--------|--------------|--------|-------------------------------|\n| Change line N | omit | N | the new content for line N |\n| Change lines N..M | N | M | the new content for the whole block |\n| **Add** a line **after** line N (keep line N) | omit | N | line N's exact current text, then the new line(s) |\n| **Add** a line **before** line N (keep line N) | omit | N | the new line(s), then line N's exact current text |\n| **Insert** lines inside range N..M (keep N..M) | N | M | every original line in N..M plus the new lines, in the final desired order |\n| **Delete** line N | omit | N | empty body (just an empty ` ```suggestion ``` ` block) |\n| **Delete** lines N..M | N | M | empty body |\n\n### Common mistakes that silently corrupt code\n\n1. **Duplicated lines.** You copy a neighboring line into the suggestion body as \"context\" — but that line is still present in the file outside the replaced range, so accepting the suggestion inserts a second copy. Fix: only include lines that fall within the targeted range, plus any genuinely new content.\n2. **Disappearing lines.** You target `start_line=10, line=12` to comment on a 3-line block, but your suggestion body only contains 1 line because you \"only want to change line 11\". Accepting that suggestion deletes lines 10 and 12. Fix: narrow the range to just line 11, or include lines 10 and 12 verbatim in the body.\n3. **Description does not match the suggestion.** The prose says \"rename this variable\" but the suggestion replaces an entire function. Fix: re-read the prose after writing the suggestion and confirm the resulting file matches it line-for-line.\n\n### Mandatory verification before posting\n\nFor every inline comment that contains a ` ```suggestion ``` ` block, do this check before adding it to the review JSON:\n\n1. Read the actual file lines that will be replaced: `sed -n ',p' ` (or `sed -n 'p' ` for a single-line target).\n2. Mentally apply the suggestion: drop those lines, splice in the suggestion body, and look at the result in context.\n3. Confirm the resulting code matches **exactly** what your prose description promises — no extra duplicated line above/below, no original line accidentally dropped, no off-by-one.\n4. If the change cannot be expressed cleanly as a contiguous replacement (non-adjacent lines, depends on edits elsewhere), do **not** use a suggestion block — describe the change in prose instead.\n\nIf you are not 100% sure the suggestion will produce the exact code you described, drop the ` ```suggestion ``` ` block and leave a regular inline comment. A correct prose comment is always better than a one-click suggestion that silently corrupts the file.\n\n## When to Use a Suggestion Block (and When Not To)\n\nUse ` ```suggestion ``` ` for: small concrete changes — renames, typos, type hints, docstrings, 1–5 line refactors, removing a single unused import, adding one guard clause.\n\nSkip ` ```suggestion ``` ` for: large refactors, architectural changes, multi-region fixes, ambiguous improvements, anything that requires context from other parts of the file. Describe the fix in prose and let the human implement it.\n\n## Finding Line Numbers\n\n```bash\n# From a diff header: @@ -old_start,old_count +new_start,new_count @@\n# Count from new_start for added/modified lines.\n\ngrep -n \"pattern\" filename # Find the line number\nsed -n '40,50p' filename # Verify the surrounding context\n```\n\n## Good vs Bad Inline Comments\n\n### ✅ Good (specific failure mode, file/line anchored, actionable)\n\n```\n🟠 Important: JWT signature not verified\n\nThe token is decoded without verifying the signature, allowing any forged token to pass authentication.\n\n---\n\n`jwt.decode()` must be called with both the secret key and the allowed `algorithms` list, otherwise signature verification is skipped entirely.\n\nBest fix in this file (`api/auth.py`): pass the secret and the algorithm whitelist to `jwt.decode()` at line 87.\n\nNo additional methods, definitions, or external imports are required.\n\n```suggestion\ntoken_data = jwt.decode(token, SECRET_KEY, algorithms=[\"HS256\"])\n```\n```\n\n### ❌ Bad (vague, no failure mode, no anchor)\n\n```\nThis could be improved. Consider refactoring.\n```\n\n### ❌ Bad (style nit — belongs to a linter, not a review)\n\n```\nVariable should be named `user_id` instead of `uid`.\n```\n\n### ❌ Bad (out of scope — skip it)\n\n```\nUnrelated to this PR, but the README has a typo on line 12.\n```\n\n### ❌ Bad (no suggestion block where one is needed, or one where it isn't)\n\nFor a 30-line architectural refactor: do not paste a 30-line ` ```suggestion ``` ` block. Describe the change in prose and let the human do the refactor in a follow-up.\n\nFor a one-line unused-import removal: do not skip the suggestion block. The reviewer should be able to apply the fix with one click.\n\n## Edge Cases\n\n### Dependency-only PRs\n\nFor PRs that only update `package-lock.json`, `requirements.txt`, or similar:\n- Do not flag normal metadata churn (dev/devOptional flags, version bumps).\n- Only flag issues that break install, build, or runtime loading.\n- Only flag known security vulnerabilities in dependencies.\n\n### Documentation-only PRs\n\nFor PRs that only modify `.md` files:\n- Check for broken links and incorrect code examples.\n- Do not flag typos or grammar unless they change meaning.\n\n### Test-only PRs\n\nFor PRs that only add/modify tests:\n- Verify the tests actually test what they claim.\n- Check for test interdependencies.\n- Do not flag test refactoring without functional impact.\n\n## Fallback: curl\n\nIf `gh` is unavailable, use curl with the same JSON file:\n\n```bash\ncurl -X POST \\\n -H \"Authorization: token $GITHUB_TOKEN\" \\\n -H \"Accept: application/vnd.github+json\" \\\n \"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}/reviews\" \\\n -d @/tmp/review.json\n```\n\n## Summary Checklist\n\n1. Pre-review checks passed (PR open, scope ok, severity filtered, ≤ 10 findings, severity-ordered).\n2. Review data written to `/tmp/review.json` with one entry per finding in `comments[]`.\n3. Each `comments[i].body` follows the `## ` template.\n4. Each suggestion block has been verified by mentally applying it to the file.\n5. Post **ONE** review with `gh api --input /tmp/review.json`.\n6. If no actionable findings: post a short approval with `\"event\": \"APPROVE\"` and **no** `comments[]` entries." + "content": "# GitHub PR Review\n\nPost 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.\n\n## Pre-Review Checks (run before drafting the review)\n\n1. **PR is still open:**\n ```bash\n gh pr view {pr_number} --json state,mergedAt,merged\n ```\n - `state: OPEN` → proceed.\n - `state: MERGED` → stop, nothing to review.\n - `state: CLOSED` and `merged: false` → stop, do not review. The PR was abandoned; route back to planning.\n\n2. **Scope guardrail:** review only within the issue/PR scope and regressions introduced by that scope. Do not flag unrelated cleanup, refactors, or wishlist improvements. If a finding is out of scope, skip it.\n\n3. **Severity filter:** only report findings that are medium severity or higher, have a concrete failure mechanism, and tie to user-visible impact, correctness risk, or metric distortion. Style nits belong to linters, not reviews.\n\n4. **Cap the number of findings.** A focused review with 5 strong findings beats a noisy review with 20 weak ones. If you have more than ~10, keep the top-severity ones and silently drop the rest.\n\n5. **Order by severity:** 🔴 Critical → 🟠 Important → 🟡 Suggestion. Within the same priority, the most user-visible issue goes first.\n\n## Key Rule: One PR Review, One API Call, Many Inline Comments\n\nPost exactly **one** Pull Request Review (`POST /repos/{owner}/{repo}/pulls/{pr_number}/reviews`) whose `comments[]` array contains **one entry per finding**. Each entry becomes a separate inline thread anchored to a `path` + `line` + `side`.\n\n**Do NOT:**\n- Post a single big issue/PR comment that contains all findings as numbered sections. That bypasses the inline diff anchoring, breaks the suggestion-block UX, and is not what `github-code-quality[bot]` does.\n- Post each finding as a separate API call. That fragments the review into N pending reviews and pollutes the timeline.\n\n## Inline Comment Body Format\n\nEach `comments[i].body` string follows this exact template (this is the `github-code-quality[bot]` shape, with the priority label folded into the heading and an optional one-click suggestion block appended):\n\n```markdown\n## \n\n\n\n---\n\n\n\nBest fix in this file (`\n\n\n\n```suggestion\n\n```\n```\n\nRules:\n- **Heading** is `## `. The priority prefix is one of:\n - `🔴 Critical:` — must fix: security, data loss, broken invariants.\n - `🟠 Important:` — should fix: logic errors, performance, missing error handling.\n - `🟡 Suggestion:` — worth considering: clarity, maintainability.\n - **Never** `🟢 Nit` or `🟢 Acceptable`. If the code is fine, do not comment.\n- **One-line statement** names the specific defect at that file/line. No \"consider\", no \"could be better\" — state what is wrong.\n- **`---`** is a literal Markdown horizontal rule. It separates the diagnosis from the fix guidance and is what makes the format scannable in the GitHub UI.\n- **General fix** is one or two sentences explaining the approach (e.g., \"remove only the unused symbol from the import list while keeping used imports intact\").\n- **Best fix here** is anchored to the actual file path and line number in backticks. It is the *one* edit the reviewer should make.\n- **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.\n- **` ```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.\n\n### Worked example (one finding, full template)\n\n```\n🟠 Important: Unused import\n\nImport of 'generate_macd_signal' is not used.\n\n---\n\nTo fix an unused import without changing behavior, remove only the unused symbol from the import statement and keep the rest intact.\n\nBest fix in this file (`core/analysis/indicator_correlation.py`): update line 23 so it imports only `compute_macd`, and remove `generate_macd_signal` from that line. No other code changes are needed, assuming the symbol is indeed unused throughout the file.\n\n```suggestion\nfrom core.indicators.macd import compute_macd\n```\n```\n\n## Posting the Review\n\nUse the GitHub CLI (`gh`) with a JSON input file. The `GITHUB_TOKEN` is automatically available.\n\n**Important**: Always use `--input` with a JSON file instead of `-F` flags. This avoids shell-quoting issues with backticks, quotes, and newlines inside suggestion blocks.\n\n### Step 1: Create the JSON file\n\n```bash\ncat > /tmp/review.json << 'EOF'\n{\n \"commit_id\": \"{commit_sha}\",\n \"event\": \"COMMENT\",\n \"body\": \"Review summary: 2 important findings + 1 suggestion. Inline comments below.\",\n \"comments\": [\n {\n \"path\": \"core/analysis/indicator_correlation.py\",\n \"line\": 23,\n \"side\": \"RIGHT\",\n \"body\": \"🟠 Important: Unused import\\n\\nImport of 'generate_macd_signal' is not used.\\n\\n---\\n\\nTo fix an unused import without changing behavior, remove only the unused symbol from the import statement and keep the rest intact.\\n\\nBest fix in this file (`core/analysis/indicator_correlation.py`): update line 23 so it imports only `compute_macd`, and remove `generate_macd_signal` from that line. No other code changes are needed, assuming the symbol is indeed unused throughout the file.\\n\\n```suggestion\\nfrom core.indicators.macd import compute_macd\\n```\"\n },\n {\n \"path\": \"api/routes/health.py\",\n \"line\": 42,\n \"side\": \"RIGHT\",\n \"body\": \"🟠 Important: Division by None\\n\\n`uptime_seconds` divides by `(now - start_time)` without checking whether `start_time` is set, raising `TypeError` on first startup.\\n\\n---\\n\\nGuard the division with a `None` check and return `0` until the service is fully initialized.\\n\\nBest fix in this file (`api/routes/health.py`): add the guard at line 42 so the function returns `uptime_seconds=0` cleanly when `start_time` is None.\\n\\nNo new imports, methods, or external dependencies are needed.\\n\\n```suggestion\\nif start_time is None:\\n uptime_seconds = 0\\nelse:\\n uptime_seconds = int((now - start_time).total_seconds())\\n```\"\n }\n ]\n}\nEOF\n```\n\n### Step 2: Post the review\n\n```bash\ngh api -X POST repos/{owner}/{repo}/pulls/{pr_number}/reviews --input /tmp/review.json\n```\n\n### Parameters\n\n| Parameter | Description |\n|-----------|-------------|\n| `commit_id` | Commit SHA to comment on (use `git rev-parse HEAD`). |\n| `event` | `COMMENT` (leave as comments), `APPROVE`, or `REQUEST_CHANGES`. |\n| `body` | Brief 1–3 sentence summary. Keep it short — every detail belongs in an inline comment. |\n| `comments[].path` | File path exactly as shown in the diff. |\n| `comments[].line` | 1-based line number in the NEW version (right side of diff). |\n| `comments[].side` | `RIGHT` for new/added lines, `LEFT` for deleted lines. |\n| `comments[].body` | The formatted Markdown described in \"Inline Comment Body Format\" above. |\n| `comments[].start_line` | (Optional) For multi-line suggestion ranges, see below. |\n\n## Multi-Line Suggestion Blocks\n\nFor a fix that spans multiple lines, set `start_line` to the first line of the range. **`start_line`/`line` together define the range that will be REPLACED** when the reviewer clicks \"Commit suggestion\".\n\n```json\n{\n \"path\": \"api/routes/health.py\",\n \"start_line\": 41,\n \"line\": 43,\n \"side\": \"RIGHT\",\n \"body\": \"🟠 Important: ...\\n\\n---\\n\\n...\\n\\n```suggestion\\nif start_time is None:\\n uptime_seconds = 0\\nelse:\\n uptime_seconds = int((now - start_time).total_seconds())\\n```\"\n}\n```\n\n## How Suggestions Actually Work (Mandatory Verification)\n\nA ` ```suggestion ``` ` block **replaces** the targeted range with its contents. The replaced range is:\n\n- `line` only → the single line `line` (replaces 1 line).\n- `start_line` + `line` → the inclusive range `start_line..line` (replaces `line − start_line + 1` lines).\n\nThe suggestion body can be any number of lines — 0 (deletion), 1, or many. It does **not** have to match the range size.\n\n| Intent | `start_line` | `line` | Suggestion body must contain |\n|--------|--------------|--------|-------------------------------|\n| Change line N | omit | N | the new content for line N |\n| Change lines N..M | N | M | the new content for the whole block |\n| **Add** a line **after** line N (keep line N) | omit | N | line N's exact current text, then the new line(s) |\n| **Add** a line **before** line N (keep line N) | omit | N | the new line(s), then line N's exact current text |\n| **Insert** lines inside range N..M (keep N..M) | N | M | every original line in N..M plus the new lines, in the final desired order |\n| **Delete** line N | omit | N | empty body (just an empty ` ```suggestion ``` ` block) |\n| **Delete** lines N..M | N | M | empty body |\n\n### Common mistakes that silently corrupt code\n\n1. **Duplicated lines.** You copy a neighboring line into the suggestion body as \"context\" — but that line is still present in the file outside the replaced range, so accepting the suggestion inserts a second copy. Fix: only include lines that fall within the targeted range, plus any genuinely new content.\n2. **Disappearing lines.** You target `start_line=10, line=12` to comment on a 3-line block, but your suggestion body only contains 1 line because you \"only want to change line 11\". Accepting that suggestion deletes lines 10 and 12. Fix: narrow the range to just line 11, or include lines 10 and 12 verbatim in the body.\n3. **Description does not match the suggestion.** The prose says \"rename this variable\" but the suggestion replaces an entire function. Fix: re-read the prose after writing the suggestion and confirm the resulting file matches it line-for-line.\n\n### Mandatory verification before posting\n\nFor every inline comment that contains a ` ```suggestion ``` ` block, do this check before adding it to the review JSON:\n\n1. Read the actual file lines that will be replaced: `sed -n ',p' ` (or `sed -n 'p' ` for a single-line target).\n2. Mentally apply the suggestion: drop those lines, splice in the suggestion body, and look at the result in context.\n3. Confirm the resulting code matches **exactly** what your prose description promises — no extra duplicated line above/below, no original line accidentally dropped, no off-by-one.\n4. If the change cannot be expressed cleanly as a contiguous replacement (non-adjacent lines, depends on edits elsewhere), do **not** use a suggestion block — describe the change in prose instead.\n\nIf you are not 100% sure the suggestion will produce the exact code you described, drop the ` ```suggestion ``` ` block and leave a regular inline comment. A correct prose comment is always better than a one-click suggestion that silently corrupts the file.\n\n## When to Use a Suggestion Block (and When Not To)\n\nUse ` ```suggestion ``` ` for: small concrete changes — renames, typos, type hints, docstrings, 1–5 line refactors, removing a single unused import, adding one guard clause.\n\nSkip ` ```suggestion ``` ` for: large refactors, architectural changes, multi-region fixes, ambiguous improvements, anything that requires context from other parts of the file. Describe the fix in prose and let the human implement it.\n\n## Finding Line Numbers\n\n```bash\n# From a diff header: @@ -old_start,old_count +new_start,new_count @@\n# Count from new_start for added/modified lines.\n\ngrep -n \"pattern\" filename # Find the line number\nsed -n '40,50p' filename # Verify the surrounding context\n```\n\n## Good vs Bad Inline Comments\n\n### ✅ Good (specific failure mode, file/line anchored, actionable)\n\n```\n🟠 Important: JWT signature not verified\n\nThe token is decoded without verifying the signature, allowing any forged token to pass authentication.\n\n---\n\n`jwt.decode()` must be called with both the secret key and the allowed `algorithms` list, otherwise signature verification is skipped entirely.\n\nBest fix in this file (`api/auth.py`): pass the secret and the algorithm whitelist to `jwt.decode()` at line 87.\n\nNo additional methods, definitions, or external imports are required.\n\n```suggestion\ntoken_data = jwt.decode(token, SECRET_KEY, algorithms=[\"HS256\"])\n```\n```\n\n### ❌ Bad (vague, no failure mode, no anchor)\n\n```\nThis could be improved. Consider refactoring.\n```\n\n### ❌ Bad (style nit — belongs to a linter, not a review)\n\n```\nVariable should be named `user_id` instead of `uid`.\n```\n\n### ❌ Bad (out of scope — skip it)\n\n```\nUnrelated to this PR, but the README has a typo on line 12.\n```\n\n### ❌ Bad (no suggestion block where one is needed, or one where it isn't)\n\nFor a 30-line architectural refactor: do not paste a 30-line ` ```suggestion ``` ` block. Describe the change in prose and let the human do the refactor in a follow-up.\n\nFor a one-line unused-import removal: do not skip the suggestion block. The reviewer should be able to apply the fix with one click.\n\n## Edge Cases\n\n### Dependency-only PRs\n\nFor PRs that only update `package-lock.json`, `requirements.txt`, or similar:\n- Do not flag normal metadata churn (dev/devOptional flags, version bumps).\n- Only flag issues that break install, build, or runtime loading.\n- Only flag known security vulnerabilities in dependencies.\n\n### Documentation-only PRs\n\nFor PRs that only modify `.md` files:\n- Check for broken links and incorrect code examples.\n- Do not flag typos or grammar unless they change meaning.\n\n### Test-only PRs\n\nFor PRs that only add/modify tests:\n- Verify the tests actually test what they claim.\n- Check for test interdependencies.\n- Do not flag test refactoring without functional impact.\n\n## Fallback: curl\n\nIf `gh` is unavailable, use curl with the same JSON file:\n\n```bash\ncurl -X POST \\\n -H \"Authorization: token $GITHUB_TOKEN\" \\\n -H \"Accept: application/vnd.github+json\" \\\n \"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}/reviews\" \\\n -d @/tmp/review.json\n```\n\n## Summary Checklist\n\n1. Pre-review checks passed (PR open, scope ok, severity filtered, ≤ 10 findings, severity-ordered).\n2. Review data written to `/tmp/review.json` with one entry per finding in `comments[]`.\n3. Each `comments[i].body` follows the `## ` template.\n4. Each suggestion block has been verified by mentally applying it to the file.\n5. Post **ONE** review with `gh api --input /tmp/review.json`.\n6. If no actionable findings: post a short approval with `\"event\": \"APPROVE\"` and **no** `comments[]` entries." }, { "name": "github-pr-reviewer",