diff --git a/plugins/pr-review/scripts/prompt.py b/plugins/pr-review/scripts/prompt.py index 3cf931af..c71bb946 100644 --- a/plugins/pr-review/scripts/prompt.py +++ b/plugins/pr-review/scripts/prompt.py @@ -74,7 +74,37 @@ PROMPT = """{skill_trigger} /github-pr-review -When posting a review, keep the review body brief unless your active review instructions require a longer structured format. +## How to Post the Review (CRITICAL ā€” read first) + +You MUST post your review via the **GitHub Pull Request Reviews API**, NOT the Issue Comments API. Use exactly one API call whose `comments[]` array contains one entry per finding: + +```bash +gh api -X POST repos/{{owner}}/{{repo}}/pulls/{pr_number}/reviews --input /tmp/review.json +``` + +where `/tmp/review.json` has this shape (one entry per finding, all findings bundled into the same call): + +```json +{{ + "commit_id": "", + "event": "COMMENT", + "body": "Brief 1ā€“3 sentence summary. Inline comments below.", + "comments": [ + {{ + "path": "api/auth.py", + "line": 87, + "side": "RIGHT", + "body": "## šŸŸ  Important: JWT signature not verified\n\nForged tokens pass because `jwt.decode()` is called without the secret.\n\n---\n\n`jwt.decode()` requires both the secret and the allowed `algorithms` list.\n\nBest fix in this file (`api/auth.py:87`): pass `SECRET_KEY` and `algorithms=[\"HS256\"]`.\n\nNo new methods or dependencies needed.\n\n```suggestion\ntoken_data = jwt.decode(token, SECRET_KEY, algorithms=[\"HS256\"])\n```" + }} + ] +}} +``` + +Each `comments[i].body` starts with `## <šŸ”“ Critical|šŸŸ  Important|šŸŸ” Suggestion> `, then a one-line statement of the issue, a `---` separator, a general fix explanation, a "Best fix in this file (``)" anchor, a scope confirmation ("No logic changes, new methods, or dependency changes are needed."), and ends with a ` ```suggestion ``` ` block for one-click apply. Never use `šŸŸ¢` priority labels ā€” if the code is fine, do not comment on it. + +**Do NOT** use `gh pr comment`, `POST /issues/{{n}}/comments`, or any path that produces a single issue-level comment. That yields a blob in the PR conversation with no diff anchoring, no suggestion blocks, and no per-finding threading ā€” it is a bug, not a stylistic choice. The Pull Request Reviews API is the only acceptable submission path. Use the `line` (1-based) and `side: "RIGHT"` of the new file as shown in the diff. + +When posting a review, keep the top-level `body` brief unless your active review instructions require a longer structured format. For dependency update PRs, do **NOT** approve a target version that was published less than 7 days ago. diff --git a/skills/github-pr-review/SKILL.md b/skills/github-pr-review/SKILL.md index 706c5716..4e93bec9 100644 --- a/skills/github-pr-review/SKILL.md +++ b/skills/github-pr-review/SKILL.md @@ -7,38 +7,115 @@ triggers: # GitHub PR Review -Post structured code review feedback using the GitHub API with inline comments on specific lines. +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. -## Key Rule: One API Call +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). -Bundle ALL comments into a **single review API call**. Do not post comments individually. +## Pre-Review Checks (run before drafting the review) -## Posting a Review +1. **PR is still open:** + ```bash + gh pr view {pr_number} --json state,mergedAt,merged + ``` + - `state: OPEN` ā†’ proceed. + - `state: MERGED` ā†’ stop, nothing to review. + - `state: CLOSED` and `merged: false` ā†’ stop, do not review. The PR was abandoned; route back to planning. + +2. **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. + +3. **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. + +4. **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. + +5. **Order by severity:** šŸ”“ Critical ā†’ šŸŸ  Important ā†’ šŸŸ” Suggestion. Within the same priority, the most user-visible issue goes first. + +## Key Rule: One PR Review, One API Call, Many Inline Comments + +Post 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`. + +**Do NOT:** +- 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. +- Post each finding as a separate API call. That fragments the review into N pending reviews and pollutes the timeline. + +## Inline Comment Body Format + +Each `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): + +```markdown +## + + + +--- + + + +Best fix in this file (` + + + +```suggestion + +``` +``` + +Rules: +- **Heading** is `## `. The priority prefix is one of: + - `šŸ”“ Critical:` ā€” must fix: security, data loss, broken invariants. + - `šŸŸ  Important:` ā€” should fix: logic errors, performance, missing error handling. + - `šŸŸ” Suggestion:` ā€” worth considering: clarity, maintainability. + - **Never** `šŸŸ¢ Nit` or `šŸŸ¢ Acceptable`. If the code is fine, do not comment. +- **One-line statement** names the specific defect at that file/line. No "consider", no "could be better" ā€” state what is wrong. +- **`---`** 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. +- **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"). +- **Best fix here** is anchored to the actual file path and line number in backticks. It is the *one* edit the reviewer should make. +- **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) + +``` +šŸŸ  Important: Unused import + +Import of 'generate_macd_signal' is not used. + +--- + +To fix an unused import without changing behavior, remove only the unused symbol from the import statement and keep the rest intact. + +Best 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. + +```suggestion +from core.indicators.macd import compute_macd +``` +``` + +## Posting the Review Use the GitHub CLI (`gh`) with a JSON input file. The `GITHUB_TOKEN` is automatically available. -**Important**: Always use `--input` with a JSON file instead of `-F` flags. This avoids shell quoting issues with special characters in comment bodies (quotes, backticks, newlines, etc.) and eliminates the need for complex heredoc scripts. +**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. -### Step 1: Create a JSON file +### Step 1: Create the JSON file ```bash cat > /tmp/review.json << 'EOF' { "commit_id": "{commit_sha}", "event": "COMMENT", - "body": "Brief 1-3 sentence summary.", + "body": "Review summary: 2 important findings + 1 suggestion. Inline comments below.", "comments": [ { - "path": "path/to/file.py", - "line": 42, + "path": "core/analysis/indicator_correlation.py", + "line": 23, "side": "RIGHT", - "body": "šŸŸ  Important: Your comment here." + "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```" }, { - "path": "another/file.js", - "line": 15, + "path": "api/routes/health.py", + "line": 42, "side": "RIGHT", - "body": "šŸŸ” Suggestion: Another comment." + "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```" } ] } @@ -55,75 +132,37 @@ gh api -X POST repos/{owner}/{repo}/pulls/{pr_number}/reviews --input /tmp/revie | Parameter | Description | |-----------|-------------| -| `commit_id` | Commit SHA to comment on (use `git rev-parse HEAD`) | -| `event` | `COMMENT`, `APPROVE`, or `REQUEST_CHANGES` | -| `path` | File path as shown in the diff | -| `line` | Line number in the NEW version (right side of diff) | -| `side` | `RIGHT` for new/added lines, `LEFT` for deleted lines | -| `body` | Comment text with priority label | +| `commit_id` | Commit SHA to comment on (use `git rev-parse HEAD`). | +| `event` | `COMMENT` (leave as comments), `APPROVE`, or `REQUEST_CHANGES`. | +| `body` | Brief 1ā€“3 sentence summary. Keep it short ā€” every detail belongs in an inline comment. | +| `comments[].path` | File path exactly as shown in the diff. | +| `comments[].line` | 1-based line number in the NEW version (right side of diff). | +| `comments[].side` | `RIGHT` for new/added lines, `LEFT` for deleted lines. | +| `comments[].body` | The formatted Markdown described in "Inline Comment Body Format" above. | +| `comments[].start_line` | (Optional) For multi-line suggestion ranges, see below. | -### Multi-Line Comments +## Multi-Line Suggestion Blocks -For comments spanning multiple lines, add `start_line` to specify the range: +For 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". ```json { - "path": "path/to/file.py", - "start_line": 10, - "line": 12, + "path": "api/routes/health.py", + "start_line": 41, + "line": 43, "side": "RIGHT", - "body": "šŸŸ” Suggestion: Refactor this block:\n\n```suggestion\nline_one = \"new\"\nline_two = \"code\"\nline_three = \"here\"\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```" } ``` -**`start_line`/`line` define the range that will be REPLACED.** The suggestion block may have any number of lines ā€” it does **not** have to match the range size. See the next section for the exact semantics; getting this wrong is how suggestions silently delete or duplicate code. - -## Priority Labels +## How Suggestions Actually Work (Mandatory Verification) -Start each comment with a priority label. **Minimize nits** - leave minor style issues to linters. +A ` ```suggestion ``` ` block **replaces** the targeted range with its contents. The replaced range is: -| Label | When to Use | -|-------|-------------| -| šŸ”“ **Critical** | Must fix: security vulnerabilities, bugs, data loss risks | -| šŸŸ  **Important** | Should fix: logic errors, performance issues, missing error handling | -| šŸŸ” **Suggestion** | Worth considering: significant improvements to clarity or maintainability | +- `line` only ā†’ the single line `line` (replaces 1 line). +- `start_line` + `line` ā†’ the inclusive range `start_line..line` (replaces `line āˆ’ start_line + 1` lines). -**Do NOT post šŸŸ¢ Nit or šŸŸ¢ Acceptable comments.** If code is fine, simply don't comment on it. Inline comments that say "this looks good" or "acceptable trade-off" are noise ā€” they create review threads that must be resolved without providing actionable value. - -**Example:** -``` -šŸŸ  Important: This function doesn't handle None, which could cause an AttributeError. - -```suggestion -if user is None: - raise ValueError("User cannot be None") -``` -``` - -## GitHub Suggestions - -For small code changes, use the suggestion syntax for one-click apply: - -~~~ -```suggestion -improved_code_here() -``` -~~~ - -Use suggestions for: renaming, typos, small refactors (1-5 lines), type hints, docstrings. - -Avoid for: large refactors, architectural changes, ambiguous improvements. - -### How Suggestions Actually Work (READ THIS BEFORE WRITING ONE) - -A suggestion block **replaces** the targeted range with its contents. The replaced range is: - -- `line` only ā†’ the single line `line` (replaces 1 line) -- `start_line` + `line` ā†’ the inclusive range `start_line..line` (replaces `line - start_line + 1` lines) - -The suggestion content can be **any number of lines** ā€” 0 (deletion), 1, or many. It does not have to match the range size. Whatever is between the ` ```suggestion ` and closing ` ``` ` fences becomes the new content of those lines. - -Writing the wrong combination of `start_line`/`line` and suggestion body is what causes accepted suggestions to **duplicate** or **delete** code. Use the table below as your contract: +The suggestion body can be any number of lines ā€” 0 (deletion), 1, or many. It does **not** have to match the range size. | Intent | `start_line` | `line` | Suggestion body must contain | |--------|--------------|--------|-------------------------------| @@ -135,36 +174,110 @@ Writing the wrong combination of `start_line`/`line` and suggestion body is what | **Delete** line N | omit | N | empty body (just an empty ` ```suggestion ``` ` block) | | **Delete** lines N..M | N | M | empty body | -### Common Mistakes That Break Code +### Common mistakes that silently corrupt code -1. **Duplicated lines.** You copy a neighboring line (N-1 or N+1) into the suggestion body as context ā€” that line is still present in the file outside the replaced range, so accepting the suggestion inserts a second copy of it. Fix: only include lines that fall within the targeted range, plus any genuinely new content. -2. **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: either narrow the range to just line 11, or include lines 10 and 12 verbatim in the body. -3. **Description does not match the suggestion.** The prose says "rename this variable" but the suggestion replaces an entire function. Or the prose says "add a None check" but the suggestion only contains the check (deleting the original code). Fix: after writing the suggestion, re-read the prose and confirm the resulting file would match it line-for-line. +1. **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. +2. **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. +3. **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. -### Mandatory Verification Before Posting +### Mandatory verification before posting -For every comment that contains a ` ```suggestion ``` ` block, do this check before adding it to the review JSON: +For every inline comment that contains a ` ```suggestion ``` ` block, do this check before adding it to the review JSON: 1. Read the actual file lines that will be replaced: `sed -n ',p' ` (or `sed -n 'p' ` for a single-line target). 2. Mentally apply the suggestion: drop those lines, splice in the suggestion body, and look at the result in context. 3. 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. -4. If the change cannot be expressed cleanly as a contiguous replacement (e.g., it touches non-adjacent lines, or it depends on edits elsewhere in the file), do **not** use a suggestion block ā€” describe the change in prose instead. +4. 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. If 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. +## When to Use a Suggestion Block (and When Not To) + +Use ` ```suggestion ``` ` for: small concrete changes ā€” renames, typos, type hints, docstrings, 1ā€“5 line refactors, removing a single unused import, adding one guard clause. + +Skip ` ```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. + ## Finding Line Numbers ```bash -# From diff header: @@ -old_start,old_count +new_start,new_count @@ -# Count from new_start for added/modified lines +# From a diff header: @@ -old_start,old_count +new_start,new_count @@ +# Count from new_start for added/modified lines. + +grep -n "pattern" filename # Find the line number +sed -n '40,50p' filename # Verify the surrounding context +``` + +## Good vs Bad Inline Comments + +### āœ… Good (specific failure mode, file/line anchored, actionable) + +``` +šŸŸ  Important: JWT signature not verified + +The token is decoded without verifying the signature, allowing any forged token to pass authentication. + +--- + +`jwt.decode()` must be called with both the secret key and the allowed `algorithms` list, otherwise signature verification is skipped entirely. + +Best fix in this file (`api/auth.py`): pass the secret and the algorithm whitelist to `jwt.decode()` at line 87. + +No additional methods, definitions, or external imports are required. + +```suggestion +token_data = jwt.decode(token, SECRET_KEY, algorithms=["HS256"]) +``` +``` + +### āŒ Bad (vague, no failure mode, no anchor) + +``` +This could be improved. Consider refactoring. +``` + +### āŒ Bad (style nit ā€” belongs to a linter, not a review) + +``` +Variable should be named `user_id` instead of `uid`. +``` + +### āŒ Bad (out of scope ā€” skip it) -grep -n "pattern" filename # Find line number -head -n 42 filename | tail -1 # Verify line content ``` +Unrelated to this PR, but the README has a typo on line 12. +``` + +### āŒ Bad (no suggestion block where one is needed, or one where it isn't) + +For 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. + +For a one-line unused-import removal: do not skip the suggestion block. The reviewer should be able to apply the fix with one click. + +## Edge Cases + +### Dependency-only PRs + +For PRs that only update `package-lock.json`, `requirements.txt`, or similar: +- Do not flag normal metadata churn (dev/devOptional flags, version bumps). +- Only flag issues that break install, build, or runtime loading. +- Only flag known security vulnerabilities in dependencies. + +### Documentation-only PRs + +For PRs that only modify `.md` files: +- Check for broken links and incorrect code examples. +- Do not flag typos or grammar unless they change meaning. + +### Test-only PRs + +For PRs that only add/modify tests: +- Verify the tests actually test what they claim. +- Check for test interdependencies. +- Do not flag test refactoring without functional impact. ## Fallback: curl -If `gh` is unavailable, use curl with the JSON file: +If `gh` is unavailable, use curl with the same JSON file: ```bash curl -X POST \ @@ -174,13 +287,11 @@ curl -X POST \ -d @/tmp/review.json ``` -## Summary +## Summary Checklist -1. Analyze the code and identify important issues (minimize nits) -2. Write review data to a JSON file (e.g., `/tmp/review.json`) -3. Post **ONE** review using `gh api --input /tmp/review.json` -4. Use priority labels (šŸ”“šŸŸ šŸŸ”) on every comment -5. Do NOT post comments for code that is acceptable ā€” only comment when action is needed -6. Use suggestion syntax for concrete code changes, but only after verifying the resulting code matches your description (see "How Suggestions Actually Work") -7. Keep the review body brief (details go in inline comments) -8. If no issues: post a short approval message with no inline comments +1. Pre-review checks passed (PR open, scope ok, severity filtered, ā‰¤ 10 findings, severity-ordered). +2. Review data written to `/tmp/review.json` with one entry per finding in `comments[]`. +3. Each `comments[i].body` follows the `## ` template. +4. Each suggestion block has been verified by mentally applying it to the file. +5. Post **ONE** review with `gh api --input /tmp/review.json`. +6. If no actionable findings: post a short approval with `"event": "APPROVE"` and **no** `comments[]` entries. diff --git a/skills/index.js b/skills/index.js index 0737c2a2..fc716f02 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 structured code review feedback using the GitHub API with inline comments on specific lines.\n\n## Key Rule: One API Call\n\nBundle ALL comments into a **single review API call**. Do not post comments individually.\n\n## Posting a 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 special characters in comment bodies (quotes, backticks, newlines, etc.) and eliminates the need for complex heredoc scripts.\n\n### Step 1: Create a JSON file\n\n```bash\ncat > /tmp/review.json << 'EOF'\n{\n \"commit_id\": \"{commit_sha}\",\n \"event\": \"COMMENT\",\n \"body\": \"Brief 1-3 sentence summary.\",\n \"comments\": [\n {\n \"path\": \"path/to/file.py\",\n \"line\": 42,\n \"side\": \"RIGHT\",\n \"body\": \"šŸŸ  Important: Your comment here.\"\n },\n {\n \"path\": \"another/file.js\",\n \"line\": 15,\n \"side\": \"RIGHT\",\n \"body\": \"šŸŸ” Suggestion: Another comment.\"\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`, `APPROVE`, or `REQUEST_CHANGES` |\n| `path` | File path as shown in the diff |\n| `line` | Line number in the NEW version (right side of diff) |\n| `side` | `RIGHT` for new/added lines, `LEFT` for deleted lines |\n| `body` | Comment text with priority label |\n\n### Multi-Line Comments\n\nFor comments spanning multiple lines, add `start_line` to specify the range:\n\n```json\n{\n \"path\": \"path/to/file.py\",\n \"start_line\": 10,\n \"line\": 12,\n \"side\": \"RIGHT\",\n \"body\": \"šŸŸ” Suggestion: Refactor this block:\\n\\n```suggestion\\nline_one = \\\"new\\\"\\nline_two = \\\"code\\\"\\nline_three = \\\"here\\\"\\n```\"\n}\n```\n\n**`start_line`/`line` define the range that will be REPLACED.** The suggestion block may have any number of lines ā€” it does **not** have to match the range size. See the next section for the exact semantics; getting this wrong is how suggestions silently delete or duplicate code.\n\n## Priority Labels\n\nStart each comment with a priority label. **Minimize nits** - leave minor style issues to linters.\n\n| Label | When to Use |\n|-------|-------------|\n| šŸ”“ **Critical** | Must fix: security vulnerabilities, bugs, data loss risks |\n| šŸŸ  **Important** | Should fix: logic errors, performance issues, missing error handling |\n| šŸŸ” **Suggestion** | Worth considering: significant improvements to clarity or maintainability |\n\n**Do NOT post šŸŸ¢ Nit or šŸŸ¢ Acceptable comments.** If code is fine, simply don't comment on it. Inline comments that say \"this looks good\" or \"acceptable trade-off\" are noise ā€” they create review threads that must be resolved without providing actionable value.\n\n**Example:**\n```\nšŸŸ  Important: This function doesn't handle None, which could cause an AttributeError.\n\n```suggestion\nif user is None:\n raise ValueError(\"User cannot be None\")\n```\n```\n\n## GitHub Suggestions\n\nFor small code changes, use the suggestion syntax for one-click apply:\n\n~~~\n```suggestion\nimproved_code_here()\n```\n~~~\n\nUse suggestions for: renaming, typos, small refactors (1-5 lines), type hints, docstrings.\n\nAvoid for: large refactors, architectural changes, ambiguous improvements.\n\n### How Suggestions Actually Work (READ THIS BEFORE WRITING ONE)\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 content can be **any number of lines** ā€” 0 (deletion), 1, or many. It does not have to match the range size. Whatever is between the ` ```suggestion ` and closing ` ``` ` fences becomes the new content of those lines.\n\nWriting the wrong combination of `start_line`/`line` and suggestion body is what causes accepted suggestions to **duplicate** or **delete** code. Use the table below as your contract:\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 Break Code\n\n1. **Duplicated lines.** You copy a neighboring line (N-1 or N+1) into the suggestion body as context ā€” that line is still present in the file outside the replaced range, so accepting the suggestion inserts a second copy of it. 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: either 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. Or the prose says \"add a None check\" but the suggestion only contains the check (deleting the original code). Fix: after writing the suggestion, re-read the prose and confirm the resulting file would match it line-for-line.\n\n### Mandatory Verification Before Posting\n\nFor every 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 (e.g., it touches non-adjacent lines, or it depends on edits elsewhere in the file), 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## Finding Line Numbers\n\n```bash\n# From 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 line number\nhead -n 42 filename | tail -1 # Verify line content\n```\n\n## Fallback: curl\n\nIf `gh` is unavailable, use curl with the 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\n\n1. Analyze the code and identify important issues (minimize nits)\n2. Write review data to a JSON file (e.g., `/tmp/review.json`)\n3. Post **ONE** review using `gh api --input /tmp/review.json`\n4. Use priority labels (šŸ”“šŸŸ šŸŸ”) on every comment\n5. Do NOT post comments for code that is acceptable ā€” only comment when action is needed\n6. Use suggestion syntax for concrete code changes, but only after verifying the resulting code matches your description (see \"How Suggestions Actually Work\")\n7. Keep the review body brief (details go in inline comments)\n8. If no issues: post a short approval message with no inline comments" + "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." }, { "name": "github-pr-reviewer",