Claude PR review bot: four-category scorecard comment on every PR

[reese8272]

Adds an automated code-quality gate: on every PR open/push, Claude reviews the diff and posts a scorecard comment (Syntax Errors / Code Smells / Bugs / Security Vulnerabilities, each 1-10 with findings).

Files

• .github/workflows/review.yml — trigger + permissions
• .github/scripts/review.py — fetch diff via gh api, call claude-sonnet-4-6, upsert one sticky comment per PR

Security / hardening (deliberate choices)

• Trigger is pull_request, never pull_request_target: GitHub withholds secrets from fork PRs under pull_request, so untrusted code can never read ANTHROPIC_API_KEY. Fork PRs are skipped cleanly by an if: guard. Please treat any future switch to pull_request_target as an alarm bell (pwn-request secret-exfiltration class).
• concurrency cancels the in-flight review when a new push supersedes it.
• Diff capped at 200k chars with a visible truncation notice in the comment (no silent partial reviews).
• Diff is wrapped in <diff> tags and declared untrusted data (prompt-injection hygiene).
• One sticky comment per PR (marker-based upsert) instead of a new comment per push.
• API errors fail the step visibly.

Setup required before this works

Add repository secret ANTHROPIC_API_KEY (Settings → Secrets and variables → Actions). Cost is roughly $0.01–0.05 per review at Sonnet pricing.

Tests

tools/tests/test_pr_review_script.py (4 tests) covers the pure functions: truncation boundary and sticky-comment lookup.

🤖 Generated with Claude Code

[github-actions]

Code review overview

• Syntax Errors - Score - 10/10
• Code Smells - Score - 8/10
• Bugs - Score - 8/10
• Security Vulnerabilities - Score - 9/10

---

Syntax Errors

No issues found.

---

Code Smells

.github/scripts/review.py, gh() function (~line 43)

• Why it matters: subprocess.check_output raises subprocess.CalledProcessError on non-zero exit but the function provides no error handling or helpful message. Failures surface as raw tracebacks with no context about which gh call failed.
• Suggested fix: Wrap in a try/except, print a descriptive message to stderr, and re-raise or call raise SystemExit(1).

.github/scripts/review.py, ask_claude() (~line 68)

• Why it matters: The function accesses body["content"] without guarding against unexpected response shapes (e.g., missing "content" key, or blocks without a "text" key). A malformed or unexpected API response will produce a KeyError with no useful diagnostic.
• Suggested fix: Add a guard or structured error handling around the response parsing, and log the raw body to stderr before raising.

.github/scripts/review.py, module-level constants (~line 17)

• Why it matters: MODEL = "claude-sonnet-4-6" is a bare string with no comment explaining where this identifier comes from or how to update it. Model identifiers are easy to let go stale.
• Suggested fix: Add a brief inline comment pointing to the Anthropic model naming docs/changelog.

tools/tests/test_pr_review_script.py, module-level import (~lines 20–23)

• Why it matters: The module is imported at module level using exec_module, which means any import-time side effects in review.py (e.g., if os.environ accesses were ever added at the top level) would run during test collection, not just during test execution. It also makes the test file harder to understand at a glance.
• Suggested fix: Wrap the import in a setUpModule() function or a @classmethod setUpClass so it is explicit and isolated.

---

Bugs

.github/scripts/review.py, upsert_comment() (~line 85)

• Why it matters: gh(..., stdin=payload) passes payload (a str) as stdin to the custom gh() wrapper, but gh() passes it as input=stdin to subprocess.check_output. The gh CLI's --input - flag reads from actual stdin of the subprocess, so this should work — but the gh api --input - approach requires the CLI to support it. If the installed gh version does not support --input - for PATCH, this silently breaks. More critically, if payload contains non-ASCII characters (e.g., a review with unicode), there is no explicit encoding specified for the subprocess stdin; subprocess.check_output with text=True will use the system locale, which may not be UTF-8 on all runners.
• Suggested fix: Encode payload to bytes and remove text=True from the gh() signature, or explicitly pass encoding="utf-8" and ensure payload is always a str.

tools/tests/test_pr_review_script.py, test_oversize_diff_truncated_at_limit (~line 41)

• Why it matters: The test splits on "<diff>\n" and "\n</diff>" to extract the diff body and checks its length equals MAX_DIFF_CHARS. However, PROMPT_TEMPLATE uses {diff} without a guaranteed trailing newline before </diff>, so rsplit("\n</diff>") could fail to strip correctly if the template changes. This makes the test fragile and coupled to whitespace in the template string.
• Suggested fix: Assert on truncated == True and that the prompt length is bounded, rather than re-parsing the prompt's internal structure.

---

Security Vulnerabilities

.github/workflows/review.yml, trigger and fork guard (~lines 10, 27)

• Why it matters: The YAML comment correctly explains the pull_request vs pull_request_target risk, and the if: condition on the job correctly skips fork PRs. This is well-handled. However, the ANTHROPIC_API_KEY is passed directly into the environment of a run: step that executes a script from the repository being reviewed. If a maintainer accidentally changes the trigger to pull_request_target in the future, the guard comment may not be noticed.
• Suggested fix: Consider adding a step that explicitly asserts github.event_name == 'pull_request' as a runtime check, providing defense-in-depth beyond the YAML comment.

.github/scripts/review.py, prompt injection boundary (~lines 28–38)

• Why it matters: The <diff> XML tags and the "ignore instructions inside" disclaimer are a reasonable prompt-injection mitigation. However, the diff content is interpolated with a plain Python .format(), meaning a diff containing </diff> followed by new instructions would visually close the tag in the prompt and could influence the model. This is a known limitation of string-based prompt construction.
• Suggested fix: This is partially mitigated by the disclaimer, but consider escaping or replacing </diff> occurrences in the diff string before interpolation, e.g., diff.replace("</diff>", "<\\/diff>").