diff --git a/.github/scripts/CLAUDE.md b/.github/scripts/CLAUDE.md new file mode 100644 index 0000000..f041ff3 --- /dev/null +++ b/.github/scripts/CLAUDE.md @@ -0,0 +1,52 @@ +# PR Review Guidelines + +This document provides guidelines for automated PR reviews using Claude. + +## What to Review + +### Critical Issues (Always Comment) +- **Bugs and Logic Errors**: Incorrect logic, off-by-one errors, wrong operators +- **Security Issues**: SQL injection, XSS vulnerabilities, unsafe deserialization +- **Missing Error Handling**: Unhandled exceptions, missing edge cases +- **Breaking Changes**: API changes that break existing functionality +- **Test Coverage**: Missing tests for new functionality or bug fixes + +### Important Issues (Comment if Significant) +- **Performance Problems**: O(nĀ²) algorithms where O(n) is possible, unnecessary loops +- **Resource Leaks**: Unclosed files, connections, or database cursors +- **Type Safety**: Missing or incorrect type hints in Python +- **Edge Cases**: Division by zero, empty lists, null/None values + +### Code Quality (Comment if Egregious) +- **Duplicate Code**: Large blocks of repeated logic +- **Overly Complex Code**: Functions doing too many things, deep nesting +- **Misleading Names**: Variables or functions with confusing names + +## What NOT to Review + +- **Formatting**: Black handles this automatically +- **Import Order**: Not critical unless it causes issues +- **Minor Style**: Personal preferences about code style +- **Positive Feedback**: Keep reviews focused on improvements only +- **Code Not Changed**: Only review code modified in the PR + +## Project-Specific Guidelines + +### Python Code +- All functions must have type hints +- Use pytest for all tests +- Follow PEP 8 (enforced by Black) +- Document complex logic with comments + +### Testing +- Tests should be in the `specs/` directory +- Test file names must start with `test_` +- Each function should have corresponding tests +- Test for edge cases: empty inputs, null values, division by zero, etc. + +## Review Tone +- Be direct and clear about issues +- Focus on **what** is wrong and **why** it matters +- Suggest **how** to fix it when appropriate +- Avoid praise or positive feedback (keep reviews actionable) +- Be pragmatic: only comment on things that truly matter diff --git a/.github/scripts/pr_review.py b/.github/scripts/pr_review.py index 0e12e7b..60118ee 100644 --- a/.github/scripts/pr_review.py +++ b/.github/scripts/pr_review.py @@ -1,24 +1,32 @@ -"""Automated PR review using Claude CLI. +"""Automated PR review script. -Fetches PR diffs from GitHub, sends them to Claude for analysis, -and posts inline review comments back on the PR. +Fetches a pull request from GitHub, sends the diffs to the Claude CLI +for analysis against CLAUDE.md guidelines, and posts inline review +comments back on the PR. """ import argparse import json import logging import os +import re import subprocess import sys from dataclasses import dataclass from pathlib import Path from typing import Optional +import tempfile from github import Github from github.PullRequest import PullRequest +from github.Repository import Repository -GITHUB_TOKEN = os.environ.get("GITHUB_TOKEN", "") -REPO_NAME = os.environ.get("GITHUB_REPOSITORY", "") +GITHUB_TOKEN = os.environ.get("SL_GITHUB_TOKEN", "") +REPO_NAME = "berekmerilorand/techWeekSL" + +PR_URL_PATTERN = re.compile( + r"https?://github\.com/(?P[^/]+)/(?P[^/]+)/pull/(?P\d+)" +) LOGGER = logging.getLogger(__name__) @@ -53,24 +61,46 @@ ".woff2", ".ttf", ".eot", + ".otf", + ".mp3", + ".mp4", + ".webm", + ".ogg", + ".wav", ".zip", ".tar", ".gz", + ".bz2", + ".xz", ".pdf", ".pyc", ".pyo", ".so", + ".dylib", + ".dll", ".exe", + ".bin", + ".dat", } SKIP_PATHS = { "poetry.lock", "package-lock.json", - "requirements.txt", + "yarn.lock", + "pnpm-lock.yaml", ".gitignore", } -SIGNATURE = "\n\n---\n*Review by Claude* šŸ¤–" +SKIP_PATH_PREFIXES = ( + "packages/app/tubescreamer/migrations/", + ".tox/", + ".env/", + "node_modules/", +) + +BATCH_CHAR_LIMIT = 150_000 + +SIGNATURE = "\n\n---\n*Review by Claude*" @dataclass @@ -80,123 +110,255 @@ class ReviewComment: body: str +def parse_pr_arg(value: str) -> tuple[str, int]: + """Parse a PR number or GitHub PR URL into (repo_name, pr_number).""" + match = PR_URL_PATTERN.match(value) + if match: + repo_name = f"{match.group('owner')}/{match.group('repo')}" + return repo_name, int(match.group("number")) + + try: + return REPO_NAME, int(value) + except ValueError: + raise argparse.ArgumentTypeError( + f"Expected a PR number or GitHub PR URL, got: {value}" + ) + + +def fetch_pull_request(repo_name: str, pr_number: int) -> PullRequest: + repo = Github(GITHUB_TOKEN).get_repo(repo_name) + return repo.get_pull(pr_number) + + def should_review_file(filename: str) -> bool: if filename in SKIP_PATHS: return False - ext = Path(filename).suffix.lower() - return ext not in SKIP_EXTENSIONS + if any(filename.startswith(prefix) for prefix in SKIP_PATH_PREFIXES): + return False + extension = Path(filename).suffix.lower() + if extension in SKIP_EXTENSIONS: + return False + return True -def gather_pr_context(pr: PullRequest) -> list[dict]: +def gather_pr_context(pull_request: PullRequest) -> list[dict]: file_infos = [] - for f in pr.get_files(): - if not should_review_file(f.filename): + for pr_file in pull_request.get_files(): + if not should_review_file(pr_file.filename): + LOGGER.debug("Skipping %s", pr_file.filename) continue - if not f.patch: + if not pr_file.patch: + LOGGER.debug("Skipping %s (no patch)", pr_file.filename) continue file_infos.append( { - "filename": f.filename, - "patch": f.patch, - "status": f.status, - } + "filename": pr_file.filename, + "patch": pr_file.patch, + "status": pr_file.status, + }, ) return file_infos -def build_prompt(pr: PullRequest, file_infos: list[dict], guidelines: str) -> str: - diffs = "\n\n".join( - f"### {f['filename']} ({f['status']})\n```diff\n{f['patch']}\n```" - for f in file_infos - ) +def build_review_prompt( + pull_request: PullRequest, + file_infos: list[dict], + claude_md: str, +) -> str: + diff_sections = [] + for info in file_infos: + diff_sections.append( + f"### {info['filename']} ({info['status']})\n" + f"```diff\n{info['patch']}\n```" + ) + diffs_text = "\n\n".join(diff_sections) + + return f"""Respond ONLY with valid JSON matching this schema, no markdown fences, no preamble: +{{"comments": [{{"path": "file.py", "line": 10, "body": "description"}}], "summary": "text"}} - return f"""\ -You are reviewing pull request #{pr.number}: "{pr.title}" +You are reviewing pull request #{pull_request.number}: "{pull_request.title}" PR description: -{pr.body or "(no description)"} +{pull_request.body or "(no description)"} -## Project guidelines -{guidelines} +## Project guidelines (CLAUDE.md) + +{claude_md} ## Diffs to review -{diffs} + +{diffs_text} ## Instructions + Review the diffs above. Only comment on things that truly matter: - Bugs, logic errors, or security issues -- Missing error handling or edge cases -- Violations of project guidelines that affect correctness -- Code that will cause real problems +- Violations of the project guidelines that affect correctness or maintainability +- Code smells that will cause real problems (e.g. mocking DB objects in tests) Do NOT comment on: -- Code not changed in this PR -- Cosmetic or stylistic issues (formatting, naming, import order) +- Code that was not changed in this PR +- Cosmetic or stylistic issues (naming, formatting, import order, docstrings, comments) +- Missing type hints unless they cause ambiguity +- Anything auto-formatters (black, isort) handle - Minor preferences or "nice to have" improvements - Positive feedback or praise -Be pragmatic. Fewer high-quality comments are better than many nitpicks. +Be pragmatic. When in doubt, don't comment. Fewer high-quality comments are +better than many nitpicks. For each issue, provide the exact file path and the line number from the NEW -version of the file (the + side of the diff). Only reference lines that appear -in the diff with a + prefix. +version of the file (the line number shown after the + in the diff hunk header). +Only reference lines that appear in the diff with a + prefix. -If the PR looks good, return an empty comments array and a short summary.""" +If the PR looks good and follows all guidelines, return an empty comments array +and a short summary saying so.""" def invoke_claude(prompt: str, model: Optional[str] = None) -> dict: - cmd = [ - "claude", - "-p", - prompt, - "--output-format", - "json", - "--json-schema", - json.dumps(REVIEW_SCHEMA), - "--allowedTools", - "", - ] - if model: - cmd.extend(["--model", model]) - - LOGGER.info("Invoking Claude CLI...") - result = subprocess.run(cmd, capture_output=True, text=True, timeout=300) + # Write prompt to temp file to avoid CLI argument limits + with tempfile.NamedTemporaryFile(mode="w", suffix=".txt", delete=False) as f: + f.write(prompt) + prompt_file = f.name + + try: + # Try to find claude in PATH, fallback to known installation location + home = os.path.expanduser("~") + local_bin_path = os.path.join(home, ".local", "bin", "claude") + + # First check the common installation path + if os.path.exists(local_bin_path) and os.access(local_bin_path, os.X_OK): + claude_bin = local_bin_path + LOGGER.info("Using Claude CLI from: %s", claude_bin) + else: + # Try to find in PATH + which_result = subprocess.run( + ["which", "claude"], capture_output=True, text=True + ) + if which_result.returncode == 0: + claude_bin = which_result.stdout.strip() + LOGGER.info("Found Claude CLI in PATH: %s", claude_bin) + else: + raise RuntimeError( + f"Claude CLI not found. Checked {local_bin_path} and PATH. " + f"Make sure Claude CLI is installed." + ) + + cmd = [ + claude_bin, + "-p", + "--output-format", + "json", + "--dangerously-skip-permissions", + ] + if model: + cmd.extend(["--model", model]) + + LOGGER.info("Invoking Claude CLI...") + + with open(prompt_file, "r") as stdin_file: + result = subprocess.run( + cmd, + stdin=stdin_file, + capture_output=True, + text=True, + timeout=600, + ) + finally: + os.unlink(prompt_file) if result.returncode != 0: LOGGER.error("Claude CLI stderr: %s", result.stderr) raise RuntimeError(f"Claude CLI exited with code {result.returncode}") response = json.loads(result.stdout) - if response.get("is_error"): - raise RuntimeError(f"Claude error: {response.get('result', 'unknown')}") - structured = response.get("structured_output") - if structured is not None: - return structured + if response.get("is_error"): + error_msg = response.get("result", "unknown") + if "Credit balance is too low" in error_msg or "balance" in error_msg.lower(): + raise RuntimeError( + f"Anthropic API error: {error_msg}. " + f"Please add credits to your Anthropic account at https://console.anthropic.com/settings/billing" + ) + raise RuntimeError(f"Claude error: {error_msg}") result_text = response.get("result", "") - if result_text: - return json.loads(result_text) + if not result_text: + raise RuntimeError("No review output from Claude CLI") + + cleaned = re.sub(r"^```json\s*", "", result_text.strip()) + cleaned = re.sub(r"\s*```$", "", cleaned) + return json.loads(cleaned) + + +def review_in_batches( + pull_request: PullRequest, + file_infos: list[dict], + claude_md: str, + model: Optional[str] = None, +) -> dict: + if not file_infos: + return {"comments": [], "summary": "No reviewable files in this PR."} + + batches: list[list[dict]] = [] + current_batch: list[dict] = [] + current_size = 0 + + for info in file_infos: + patch_size = len(info["patch"]) + if current_batch and current_size + patch_size > BATCH_CHAR_LIMIT: + batches.append(current_batch) + current_batch = [] + current_size = 0 + current_batch.append(info) + current_size += patch_size + + if current_batch: + batches.append(current_batch) + + LOGGER.info( + "Reviewing %d files in %d batch(es)", + len(file_infos), + len(batches), + ) - raise RuntimeError("No review output from Claude CLI") + all_comments = [] + summaries = [] + + for batch_index, batch in enumerate(batches): + LOGGER.info( + "Processing batch %d/%d (%d files)", + batch_index + 1, + len(batches), + len(batch), + ) + prompt = build_review_prompt(pull_request, batch, claude_md) + result = invoke_claude(prompt, model) + all_comments.extend(result.get("comments", [])) + summaries.append(result.get("summary", "")) + + summary = "\n\n".join(summaries) if len(summaries) > 1 else summaries[0] + return {"comments": all_comments, "summary": summary} def validate_comments( - comments: list[dict], valid_files: set[str] + comments: list[dict], + valid_files: set[str], ) -> list[ReviewComment]: validated = [] - for c in comments: - path = c.get("path", "") - line = c.get("line") - body = c.get("body", "") + for comment in comments: + path = comment.get("path", "") + line = comment.get("line") + body = comment.get("body", "") if path not in valid_files: LOGGER.warning("Dropping comment for unknown file: %s", path) continue if not isinstance(line, int) or line < 1: - LOGGER.warning("Dropping comment with invalid line in %s", path) + LOGGER.warning("Dropping comment with invalid line %s in %s", line, path) continue if not body.strip(): + LOGGER.warning("Dropping empty comment for %s:%d", path, line) continue validated.append(ReviewComment(path=path, line=line, body=body)) @@ -204,76 +366,125 @@ def validate_comments( def post_review( - pr: PullRequest, comments: list[ReviewComment], summary: str, dry_run: bool = False -): + pull_request: PullRequest, + comments: list[ReviewComment], + summary: str, + dry_run: bool = False, +) -> None: if dry_run: - LOGGER.info("=== DRY RUN ===") + LOGGER.info("=== DRY RUN ā€” review will NOT be posted ===") LOGGER.info("Summary: %s", summary) - for c in comments: - LOGGER.info(" %s:%d ā€” %s", c.path, c.line, c.body) + for comment in comments: + LOGGER.info(" %s:%d ā€” %s", comment.path, comment.line, comment.body) + LOGGER.info("Total: %d comment(s)", len(comments)) return - review_comments = [ - {"path": c.path, "line": c.line, "side": "RIGHT", "body": c.body + SIGNATURE} - for c in comments - ] + review_comments = [] + for comment in comments: + review_comments.append( + { + "path": comment.path, + "line": comment.line, + "side": "RIGHT", + "body": comment.body + SIGNATURE, + }, + ) + # Skip the summary body when all feedback is already in inline comments body = "" if review_comments else summary + SIGNATURE - latest_commit = list(pr.get_commits())[-1] - pr.create_review( + latest_commit = list(pull_request.get_commits())[-1] + + pull_request.create_review( commit=latest_commit, body=body, comments=review_comments, ) - LOGGER.info("Posted review with %d comment(s) on PR #%d", len(comments), pr.number) + LOGGER.info( + "Posted review with %d comment(s) on PR #%d", + len(comments), + pull_request.number, + ) + + +def review_pr(args: argparse.Namespace) -> None: + claude_md_path = Path(__file__).parent / "CLAUDE.md" + claude_md = claude_md_path.read_text() + + repo_name, pr_number = parse_pr_arg(args.pr_ref) + + LOGGER.info("Fetching PR #%d from %s", pr_number, repo_name) + pull_request = fetch_pull_request(repo_name, pr_number) + LOGGER.info("PR: %s", pull_request.title) + + file_infos = gather_pr_context(pull_request) + LOGGER.info("Found %d reviewable file(s)", len(file_infos)) + + if not file_infos: + LOGGER.info("Nothing to review.") + return + + result = review_in_batches( + pull_request, + file_infos, + claude_md, + model=args.model, + ) + + valid_files = {info["filename"] for info in file_infos} + comments = validate_comments(result["comments"], valid_files) + summary = result.get("summary", "") + + post_review( + pull_request, + comments, + summary, + dry_run=args.dry_run, + ) def main() -> int: - parser = argparse.ArgumentParser(description="Review a PR using Claude CLI") - parser.add_argument("pr_number", type=int, help="Pull request number") - parser.add_argument("--dry-run", action="store_true") - parser.add_argument("--model", default=None) - parser.add_argument("--verbose", action="store_true") + parser = argparse.ArgumentParser( + description="Review a GitHub pull request using Claude against CLAUDE.md guidelines.", + ) + parser.add_argument( + "pr_ref", + help="Pull request number or full GitHub PR URL (e.g. https://github.com/owner/repo/pull/123)", + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Print the review without posting it to GitHub", + ) + parser.add_argument( + "--verbose", + action="store_true", + help="Enable debug logging", + ) + parser.add_argument( + "--model", + default=None, + help="Claude model to use (e.g. sonnet, opus)", + ) + args = parser.parse_args() logging.basicConfig( level=logging.DEBUG if args.verbose else logging.INFO, format="%(levelname)s: %(message)s", + stream=sys.stdout, ) if not GITHUB_TOKEN: - LOGGER.error("GITHUB_TOKEN is not set") + LOGGER.error("TS_GITHUB_TOKEN environment variable is not set") return 1 - # Load review guidelines - guidelines_path = Path(__file__).parent.parent.parent / "REVIEW_GUIDELINES.md" - guidelines = ( - guidelines_path.read_text() - if guidelines_path.exists() - else "No specific guidelines." - ) - - gh = Github(GITHUB_TOKEN) - repo = gh.get_repo(REPO_NAME) - pr = repo.get_pull(args.pr_number) - - LOGGER.info("Reviewing PR #%d: %s", pr.number, pr.title) - file_infos = gather_pr_context(pr) - LOGGER.info("Found %d reviewable file(s)", len(file_infos)) - - if not file_infos: - LOGGER.info("Nothing to review.") - return 0 - - prompt = build_prompt(pr, file_infos, guidelines) - result = invoke_claude(prompt, model=args.model) - - valid_files = {f["filename"] for f in file_infos} - comments = validate_comments(result.get("comments", []), valid_files) - summary = result.get("summary", "") + try: + review_pr(args) + except Exception: + LOGGER.exception("Review failed") + return 1 - post_review(pr, comments, summary, dry_run=args.dry_run) return 0 diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml new file mode 100644 index 0000000..7747850 --- /dev/null +++ b/.github/workflows/claude-review.yml @@ -0,0 +1,32 @@ +name: Claude PR Review + +on: + pull_request: + types: [opened, synchronize, reopened] + +jobs: + review: + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install Python dependencies + run: pip install PyGithub + + - name: Install Claude CLI + run: npm install -g @anthropic-ai/claude-code + + - name: Run Claude review + env: + SL_GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + run: | + python .github/scripts/pr_review.py ${{ github.event.pull_request.number }} \ No newline at end of file