diff --git a/.github/workflows/on-pull-request.yml b/.github/workflows/on-pull-request.yml index 4aa5c82ef3..73e958ba7c 100644 --- a/.github/workflows/on-pull-request.yml +++ b/.github/workflows/on-pull-request.yml @@ -9,10 +9,25 @@ on: description: 'PR number to review' required: true type: number + review_mode: + description: 'Review mode (all = security+plan+outsider+deep)' + required: false + type: choice + options: + - all + - security + - plan + - outsider + - deep + default: all + linked_issue: + description: 'Linked issue number (for plan verification mode)' + required: false + type: number jobs: review: - name: AI Code Review + name: AI Code Review (${{ matrix.mode }}) runs-on: ubuntu-latest # Skip draft PRs and PRs with [skip-review] in title # For workflow_dispatch, always run (manual trigger implies intent) @@ -26,8 +41,21 @@ jobs: contents: read pull-requests: write + # Run all four default review modes in parallel + # For workflow_dispatch with specific mode, only that mode runs + strategy: + fail-fast: false + matrix: + mode: >- + ${{ + github.event_name == 'workflow_dispatch' && + github.event.inputs.review_mode != 'all' + && fromJson(format('["{0}"]', github.event.inputs.review_mode)) + || fromJson('["security", "plan", "outsider", "deep"]') + }} + concurrency: - group: egg-review-${{ github.event.pull_request.number || github.event.inputs.pr_number }} + group: egg-review-${{ github.event.pull_request.number || github.event.inputs.pr_number }}-${{ matrix.mode }} cancel-in-progress: true steps: @@ -62,10 +90,17 @@ jobs: - name: Build review prompt id: prompt - run: bash action/build-review-prompt.sh + run: | + if [[ "$REVIEW_MODE" == "deep" ]]; then + bash action/build-deep-review-prompt.sh + else + bash action/build-review-prompt.sh + fi env: GH_TOKEN: ${{ steps.bot-token.outputs.token }} PR_NUMBER: ${{ github.event.pull_request.number || github.event.inputs.pr_number }} + REVIEW_MODE: ${{ matrix.mode }} + LINKED_ISSUE: ${{ github.event.inputs.linked_issue }} - name: Checkout PR branch uses: actions/checkout@v4 @@ -89,7 +124,8 @@ jobs: bot-app-private-key: ${{ secrets.BOT_APP_PRIVATE_KEY }} bot-app-installation-id: ${{ secrets.BOT_APP_INSTALLATION_ID }} bot-username: james-in-a-box - timeout: "10" + # Deep review gets 30min timeout, standard review gets 10min + timeout: ${{ steps.prompt.outputs.timeout || '10' }} - name: Post review comments if: always() && steps.egg.outputs.exit-code == '0' diff --git a/action/action.yml b/action/action.yml index 9014b15096..99f5891017 100644 --- a/action/action.yml +++ b/action/action.yml @@ -47,6 +47,10 @@ inputs: description: Docker image tag to use (e.g., v1.0.0 or latest) required: false default: latest + review-mode: + description: "Review mode: standard, security, plan, outsider, or deep" + required: false + default: standard outputs: exit-code: @@ -78,5 +82,6 @@ runs: INPUT_TIMEOUT: ${{ inputs.timeout }} INPUT_MODEL: ${{ inputs.model }} INPUT_IMAGE_TAG: ${{ inputs.image-tag }} + INPUT_REVIEW_MODE: ${{ inputs.review-mode }} GITHUB_EVENT_REPOSITORY_VISIBILITY: ${{ github.event.repository.visibility }} run: ${{ github.action_path }}/entrypoint.sh diff --git a/action/build-deep-review-prompt.sh b/action/build-deep-review-prompt.sh new file mode 100755 index 0000000000..0a6c8fe645 --- /dev/null +++ b/action/build-deep-review-prompt.sh @@ -0,0 +1,433 @@ +#!/usr/bin/env bash +# build-deep-review-prompt.sh — Build a deep review prompt for multi-turn AI analysis +# +# Unlike the standard review prompt, deep review gives the bot direct PR access +# for exploratory investigation, test execution, and direct comment posting. +# +# Environment variables: +# PR_NUMBER — Pull request number to review +# GITHUB_REPOSITORY — owner/repo +# GH_TOKEN — GitHub token for API access +# RUNNER_TEMP — Temp directory for large prompt file +# REVIEW_MODE — (optional) security|plan|outsider (loads specialized prompt) +# LINKED_ISSUE — (optional) Issue number for plan verification mode +# +# Output: +# Sets 'prompt-file', 'model', and 'mode' in $GITHUB_OUTPUT + +set -euo pipefail + +# --------------------------------------------------------------------------- +# Configuration +# --------------------------------------------------------------------------- + +MAX_CONTEXT_CHARS=50000 # Context limit for deep review + +# Files to skip (same as build-review-prompt.sh) +SKIP_PATTERNS=( + '\.lock$' + '\.min\.js$' + '\.min\.css$' + 'package-lock\.json$' + 'yarn\.lock$' + 'pnpm-lock\.yaml$' + 'Pipfile\.lock$' + 'poetry\.lock$' + 'Gemfile\.lock$' + 'composer\.lock$' + 'go\.sum$' + 'Cargo\.lock$' + '\.pyc$' + '\.pyo$' + '__pycache__' + '\.class$' + '\.jar$' + '\.war$' + '\.so$' + '\.dylib$' + '\.dll$' + '\.exe$' + '\.bin$' + '\.png$' + '\.jpg$' + '\.jpeg$' + '\.gif$' + '\.ico$' + '\.svg$' + '\.woff' + '\.ttf$' + '\.eot$' + '\.pdf$' + '\.zip$' + '\.tar' + '\.gz$' +) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +truncate_text() { + local text="$1" + local max_chars="$2" + if [[ ${#text} -gt $max_chars ]]; then + echo "${text:0:$max_chars} + +... (truncated, ${#text} total chars)" + else + echo "$text" + fi +} + +should_skip_file() { + local filename="$1" + for pattern in "${SKIP_PATTERNS[@]}"; do + if [[ "$filename" =~ $pattern ]]; then + return 0 + fi + done + return 1 +} + +# Safe gh api wrapper +gh_api_safe() { + local stderr_file + stderr_file=$(mktemp) + local output + local cmd_display + cmd_display=$(printf "'gh api %s'" "$*") + + if output=$(gh api "$@" 2>"$stderr_file"); then + rm -f "$stderr_file" + echo "$output" + else + local rc=$? + local stderr_content + stderr_content=$(cat "$stderr_file") + rm -f "$stderr_file" + echo "WARNING: ${cmd_display} failed (exit $rc): ${stderr_content}" >&2 + return 0 + fi +} + +# --------------------------------------------------------------------------- +# Fetch PR data +# --------------------------------------------------------------------------- + +fetch_pr_details() { + gh_api_safe "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" \ + --jq '{ + title: .title, + body: .body, + state: .state, + base: .base.ref, + head: .head.ref, + head_sha: .head.sha, + user: .user.login, + html_url: .html_url + }' +} + +fetch_pr_files_summary() { + # Returns a summary of changed files (less detail than standard review) + gh_api_safe "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/files" \ + --jq '[.[] | { + filename: .filename, + status: .status, + additions: .additions, + deletions: .deletions, + changes: .changes + }]' +} + +fetch_issue_content() { + local issue_number="$1" + if [[ -z "$issue_number" ]]; then + echo "" + return + fi + + gh_api_safe "repos/${GITHUB_REPOSITORY}/issues/${issue_number}" \ + --jq '{title: .title, body: .body}' 2>/dev/null || echo "" +} + +fetch_review_rules() { + # Try to fetch .egg/review-rules.md from the repo + local b64_content + b64_content=$(gh_api_safe "repos/${GITHUB_REPOSITORY}/contents/.egg/review-rules.md?ref=main" \ + --jq '.content // empty' 2>/dev/null) + + local content="" + if [[ -n "$b64_content" ]]; then + content=$(echo "$b64_content" | base64 -d 2>/dev/null) || content="" + fi + + if [[ -z "$content" ]]; then + cat <<'EOF' +## Default Review Rules + +Focus on: +- Security issues (vulnerabilities, unsafe patterns, credential leaks) +- Correctness (logic errors, edge cases, error handling gaps) +- Code quality (readability, maintainability, naming) + +Skip: +- Style issues handled by linters (formatting, import order) +- Type annotation completeness (type checkers handle this) +- Auto-generated files (migrations, lock files) +EOF + else + echo "$content" + fi +} + +# --------------------------------------------------------------------------- +# Build the prompt +# --------------------------------------------------------------------------- + +build_deep_review_prompt() { + local pr_details + pr_details=$(fetch_pr_details) + + local title body base head user html_url + title=$(echo "$pr_details" | jq -r '.title // "Untitled"') + body=$(echo "$pr_details" | jq -r '.body // ""') + base=$(echo "$pr_details" | jq -r '.base // "main"') + head=$(echo "$pr_details" | jq -r '.head // "unknown"') + user=$(echo "$pr_details" | jq -r '.user // "unknown"') + html_url=$(echo "$pr_details" | jq -r '.html_url // ""') + + # Fetch changed files summary + local files_json + files_json=$(fetch_pr_files_summary) + + local file_count + file_count=$(echo "$files_json" | jq 'length') + + # Build changed files summary + local files_summary="" + local security_sensitive_files=() + + while IFS= read -r file_entry; do + local filename status additions deletions + filename=$(echo "$file_entry" | jq -r '.filename') + status=$(echo "$file_entry" | jq -r '.status') + additions=$(echo "$file_entry" | jq -r '.additions') + deletions=$(echo "$file_entry" | jq -r '.deletions') + + if should_skip_file "$filename"; then + continue + fi + + files_summary="${files_summary}- ${filename} (${status}, +${additions}/-${deletions})\n" + + # Track security-sensitive files + if [[ "$filename" =~ (auth|middleware|security|password|token|secret|cred|api|endpoint|route|handler|docker|workflow|yml|yaml) ]]; then + security_sensitive_files+=("$filename") + fi + done < <(echo "$files_json" | jq -c '.[]') + + # Fetch review rules (not used for outsider mode) + local review_rules="" + if [[ "${REVIEW_MODE:-}" != "outsider" ]]; then + review_rules=$(fetch_review_rules) + fi + + # Fetch linked issue content for plan verification mode + local linked_content="" + if [[ "${REVIEW_MODE:-}" == "plan" ]] && [[ -n "${LINKED_ISSUE:-}" ]]; then + local issue_data + issue_data=$(fetch_issue_content "$LINKED_ISSUE") + if [[ -n "$issue_data" ]]; then + local issue_title issue_body + issue_title=$(echo "$issue_data" | jq -r '.title // ""') + issue_body=$(echo "$issue_data" | jq -r '.body // ""') + linked_content="### Issue #${LINKED_ISSUE}: ${issue_title} + +${issue_body}" + fi + fi + + # Determine which prompt template to use + local mode_prompt="" + local script_dir + script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + + case "${REVIEW_MODE:-}" in + security) + if [[ -f "${script_dir}/prompts/security-review.md" ]]; then + mode_prompt=$(cat "${script_dir}/prompts/security-review.md") + fi + ;; + plan) + if [[ -f "${script_dir}/prompts/plan-verify.md" ]]; then + mode_prompt=$(cat "${script_dir}/prompts/plan-verify.md") + fi + ;; + outsider) + if [[ -f "${script_dir}/prompts/outsider-review.md" ]]; then + mode_prompt=$(cat "${script_dir}/prompts/outsider-review.md") + fi + ;; + esac + + # Assemble the deep review prompt + local prompt + prompt="You are performing a **deep review** of PR #${PR_NUMBER}: \"${title}\" in ${GITHUB_REPOSITORY}. + +Author: ${user} +Branch: ${head} -> ${base} +URL: ${html_url} + +## PR Description + +${body:-No description provided.} +" + + # Add linked content for plan verification + if [[ -n "$linked_content" ]]; then + prompt="${prompt} +## Linked Issue/Plan + +${linked_content} +" + fi + + # Add review rules (except for outsider mode) + if [[ -n "$review_rules" ]]; then + prompt="${prompt} +## Review Rules + +${review_rules} +" + fi + + # Add changed files summary + prompt="${prompt} +## Changed Files (${file_count} files) + +$(echo -e "$files_summary") +" + + # Add security-sensitive files note if applicable + if [[ ${#security_sensitive_files[@]} -gt 0 ]]; then + prompt="${prompt} +### Security-Sensitive Files in This PR + +$(printf '%s\n' "${security_sensitive_files[@]}" | sed 's/^/- /') + +Pay extra attention to these files. +" + fi + + # Add mode-specific instructions or default deep review instructions + if [[ -n "$mode_prompt" ]]; then + # Substitute placeholders in mode prompt + mode_prompt="${mode_prompt//\{pr_number\}/$PR_NUMBER}" + mode_prompt="${mode_prompt//\{title\}/$title}" + mode_prompt="${mode_prompt//\{owner\}/${GITHUB_REPOSITORY%%/*}}" + mode_prompt="${mode_prompt//\{repo\}/${GITHUB_REPOSITORY##*/}}" + mode_prompt="${mode_prompt//\{pr_description\}/$body}" + mode_prompt="${mode_prompt//\{linked_content\}/$linked_content}" + mode_prompt="${mode_prompt//\{changed_files\}/$(echo -e "$files_summary")}" + mode_prompt="${mode_prompt//\{file_contents\}/[Use the Read tool to view file contents as needed]}" + + prompt="${prompt} +--- + +${mode_prompt} +" + else + # Default deep review instructions + prompt="${prompt} +## Deep Review Instructions + +You have **full access** to the repository and can perform multi-turn analysis. + +### Available Capabilities + +1. **Read any file** - Use the Read tool to examine files beyond the diff +2. **Run tests** - Use Bash to run \`pytest\`, \`jest\`, \`make test\`, etc. +3. **Post comments directly** - Use \`gh pr review\` to post inline comments +4. **Explore the codebase** - Follow chains of investigation + +### Review Process + +1. Start by reading the changed files in full +2. Investigate each concern by: + - Reading related files for context + - Running tests to validate suspected issues + - Checking for similar patterns elsewhere +3. Post inline comments as you find issues +4. For concrete fixes, use GitHub suggestion blocks + +### Comment Format + +For inline comments, use \`gh pr review ${PR_NUMBER} --comment\` with this body format: + +\`\`\` +**[severity]** (category): Description + +[If you have a fix, include a suggestion block:] +\`\`\`suggestion +corrected code here +\`\`\` +\`\`\` + +### Guardrails + +- Maximum 10 inline comments per review +- 30-minute time limit +- Do NOT modify code or push commits +- Do NOT approve or request changes, only comment +- Focus on the most important issues + +### Focus Areas + +1. **Security vulnerabilities** - Auth bypasses, injection, data exposure +2. **Correctness bugs** - Logic errors, edge cases, race conditions +3. **Significant quality issues** - Not style, but structural problems + +Do NOT comment on: +- Style issues (linters handle this) +- Minor improvements +- Things that static analyzers catch + +### Getting Started + +Begin by reading the changed files to understand what this PR does: +$(echo -e "$files_summary" | head -10 | sed 's/^/1. Read /') + +Then investigate any concerns by exploring related code and running tests. +" + fi + + # Truncate overall prompt if needed + prompt=$(truncate_text "$prompt" "$MAX_CONTEXT_CHARS") + + # Write prompt to temp file + local prompt_file="${RUNNER_TEMP:-/tmp}/deep-review-prompt-${PR_NUMBER}.txt" + echo "$prompt" > "$prompt_file" + + # Deep review always uses opus and has longer timeout + local model="opus" + + # Write outputs + { + echo "prompt-file=${prompt_file}" + echo "model=${model}" + echo "mode=deep-review" + echo "timeout=30" + } >> "${GITHUB_OUTPUT:-/dev/null}" + + echo "Deep review prompt built: ${#prompt} chars, ${file_count} files, mode=${REVIEW_MODE:-default}" +} + +# --------------------------------------------------------------------------- +# Main +# --------------------------------------------------------------------------- + +: "${PR_NUMBER:?PR_NUMBER is required}" +: "${GITHUB_REPOSITORY:?GITHUB_REPOSITORY is required}" + +build_deep_review_prompt diff --git a/action/build-review-prompt.sh b/action/build-review-prompt.sh index 879bd82bd3..9cb1749fe2 100755 --- a/action/build-review-prompt.sh +++ b/action/build-review-prompt.sh @@ -9,9 +9,11 @@ # GITHUB_REPOSITORY — owner/repo # GH_TOKEN — GitHub token for API access # RUNNER_TEMP — Temp directory for large prompt file +# REVIEW_MODE — (optional) security|plan|outsider for specialized modes +# LINKED_ISSUE — (optional) Issue number for plan verification mode # # Output: -# Sets 'prompt-file' and 'model' in $GITHUB_OUTPUT +# Sets 'prompt-file', 'model', and 'review-mode' in $GITHUB_OUTPUT set -euo pipefail @@ -23,6 +25,27 @@ MAX_DIFF_CHARS=15000 # Per-file diff limit MAX_FILE_CHARS=30000 # Per-file content limit MAX_PROMPT_CHARS=100000 # Overall prompt limit +# Security-sensitive file patterns (triggers auto security mode if many match) +SECURITY_PATTERNS=( + 'auth' + 'login' + 'password' + 'token' + 'secret' + 'cred' + 'middleware' + 'permission' + 'role' + 'api' + 'endpoint' + 'route' + 'handler' + 'docker' + 'workflow' + 'ci/' + '.github/' +) + # Files to skip (generated, binary, lock files) SKIP_PATTERNS=( '\.lock$' @@ -165,6 +188,64 @@ fetch_file_content() { fi } +is_security_sensitive() { + local filename="$1" + for pattern in "${SECURITY_PATTERNS[@]}"; do + if [[ "$filename" =~ $pattern ]]; then + return 0 + fi + done + return 1 +} + +count_security_sensitive_files() { + local files_json="$1" + local count=0 + while IFS= read -r file_entry; do + local filename + filename=$(echo "$file_entry" | jq -r '.filename') + if is_security_sensitive "$filename"; then + ((count++)) || true + fi + done < <(echo "$files_json" | jq -c '.[]') + echo "$count" +} + +fetch_issue_content() { + local issue_number="$1" + if [[ -z "$issue_number" ]]; then + echo "" + return + fi + + gh_api_safe "repos/${GITHUB_REPOSITORY}/issues/${issue_number}" \ + --jq '{title: .title, body: .body}' 2>/dev/null || echo "" +} + +load_specialized_prompt() { + local mode="$1" + local script_dir + script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + + case "$mode" in + security) + if [[ -f "${script_dir}/prompts/security-review.md" ]]; then + cat "${script_dir}/prompts/security-review.md" + fi + ;; + plan) + if [[ -f "${script_dir}/prompts/plan-verify.md" ]]; then + cat "${script_dir}/prompts/plan-verify.md" + fi + ;; + outsider) + if [[ -f "${script_dir}/prompts/outsider-review.md" ]]; then + cat "${script_dir}/prompts/outsider-review.md" + fi + ;; + esac +} + fetch_review_rules() { # Try to fetch .egg/review-rules.md from the repo local b64_content @@ -223,9 +304,44 @@ build_prompt() { # Always use opus for reviews local model="opus" - # Fetch review rules - local review_rules - review_rules=$(fetch_review_rules) + # Determine review mode (can be set via env or auto-detected) + local review_mode="${REVIEW_MODE:-}" + local security_sensitive_count + security_sensitive_count=$(count_security_sensitive_files "$files_json") + + # Auto-detect security mode if many security-sensitive files changed + if [[ -z "$review_mode" ]] && [[ "$security_sensitive_count" -ge 3 ]]; then + review_mode="security" + model="opus" # Use opus for security reviews + echo "Auto-detected security mode: ${security_sensitive_count} security-sensitive files" + fi + + # Fetch review rules (skip for outsider mode which deliberately ignores context) + local review_rules="" + if [[ "$review_mode" != "outsider" ]]; then + review_rules=$(fetch_review_rules) + fi + + # Fetch linked issue content for plan verification mode + local linked_content="" + if [[ "$review_mode" == "plan" ]] && [[ -n "${LINKED_ISSUE:-}" ]]; then + local issue_data + issue_data=$(fetch_issue_content "$LINKED_ISSUE") + if [[ -n "$issue_data" ]]; then + local issue_title issue_body + issue_title=$(echo "$issue_data" | jq -r '.title // ""') + issue_body=$(echo "$issue_data" | jq -r '.body // ""') + linked_content="### Issue #${LINKED_ISSUE}: ${issue_title} + +${issue_body}" + fi + fi + + # Load specialized prompt template if applicable + local specialized_prompt="" + if [[ -n "$review_mode" ]]; then + specialized_prompt=$(load_specialized_prompt "$review_mode") + fi # Build changed files section local changed_files_section="" @@ -273,7 +389,24 @@ $(truncate_text "$content" "$MAX_FILE_CHARS") # Assemble the full prompt local prompt - prompt="You are reviewing PR #${PR_NUMBER}: \"${title}\" in ${GITHUB_REPOSITORY}. + + # If we have a specialized prompt, use it with substitutions + if [[ -n "$specialized_prompt" ]]; then + # Build the prompt using the specialized template + prompt="$specialized_prompt" + + # Substitute placeholders + prompt="${prompt//\{pr_number\}/$PR_NUMBER}" + prompt="${prompt//\{title\}/$title}" + prompt="${prompt//\{owner\}/${GITHUB_REPOSITORY%%/*}}" + prompt="${prompt//\{repo\}/${GITHUB_REPOSITORY##*/}}" + prompt="${prompt//\{pr_description\}/${body:-No description provided.}}" + prompt="${prompt//\{linked_content\}/$linked_content}" + prompt="${prompt//\{changed_files\}/$changed_files_section}" + prompt="${prompt//\{file_contents\}/$file_contents_section}" + else + # Use default prompt structure + prompt="You are reviewing PR #${PR_NUMBER}: \"${title}\" in ${GITHUB_REPOSITORY}. Author: ${user} Branch: ${head} -> ${base} @@ -282,33 +415,49 @@ URL: ${html_url} ## PR Description ${body:-No description provided.} +" + + # Add linked content if available (for plan mode) + if [[ -n "$linked_content" ]]; then + prompt="${prompt} +## Linked Issue/Plan +${linked_content} +" + fi + + # Add review rules (skip for outsider mode) + if [[ -n "$review_rules" ]]; then + prompt="${prompt} ## Review Rules ${review_rules} +" + fi + prompt="${prompt} ## Changed Files (${file_count} files) ${changed_files_section} " - # Add file contents section if we have any - if [[ -n "$file_contents_section" ]]; then - prompt="${prompt} + # Add file contents section if we have any + if [[ -n "$file_contents_section" ]]; then + prompt="${prompt} ## Full File Context ${file_contents_section} " - fi + fi - # Add skipped files note - if [[ -n "$skipped_files" ]]; then - prompt="${prompt} + # Add skipped files note + if [[ -n "$skipped_files" ]]; then + prompt="${prompt} ## Skipped Files $(echo -e "$skipped_files") " - fi + fi - # Add review instructions - prompt="${prompt} + # Add review instructions + prompt="${prompt} ## Instructions Review this PR for: @@ -353,6 +502,7 @@ If the PR looks good with no significant issues, output: {\"summary\": \"No significant issues found. The changes look good.\", \"verdict\": \"approve\", \"comments\": []} \`\`\` " + fi # Truncate overall prompt if needed prompt=$(truncate_text "$prompt" "$MAX_PROMPT_CHARS") @@ -365,9 +515,10 @@ If the PR looks good with no significant issues, output: { echo "prompt-file=${prompt_file}" echo "model=${model}" + echo "review-mode=${review_mode:-standard}" } >> "${GITHUB_OUTPUT:-/dev/null}" - echo "Review prompt built: ${#prompt} chars, ${file_count} files, model=${model}" + echo "Review prompt built: ${#prompt} chars, ${file_count} files, model=${model}, mode=${review_mode:-standard}" } # --------------------------------------------------------------------------- diff --git a/action/prompts/outsider-review.md b/action/prompts/outsider-review.md new file mode 100644 index 0000000000..41981c39e3 --- /dev/null +++ b/action/prompts/outsider-review.md @@ -0,0 +1,104 @@ +# Outsider Review Mode + +You are performing an "outsider" code review of PR #{pr_number}: "{title}" in {owner}/{repo}. + +## PR Description + +{pr_description} + +## Changed Files + +{changed_files} + +## Full File Context + +{file_contents} + +## Outsider Review Instructions + +You are deliberately reviewing this code **without any project-specific context**. Pretend you are a competent engineer who has never seen this codebase before. + +Your goal is to identify issues that would confuse or mislead someone unfamiliar with the project. + +### Focus Areas + +1. **Code Clarity** + - Would a new team member understand this code? + - Are variable/function names self-explanatory? + - Is the intent of the code obvious? + - Are magic numbers or strings explained? + +2. **Documentation Gaps** + - Are complex algorithms or business logic explained? + - Do public APIs have adequate documentation? + - Are non-obvious design decisions documented? + - Would someone know how to use/modify this code? + +3. **Implicit Knowledge** + - What assumptions does this code make that aren't documented? + - Are there hidden dependencies or requirements? + - What context is needed to understand this code? + - Are there "gotchas" that only insiders would know? + +4. **Maintainability** + - Could someone fix bugs here without breaking things? + - Is the code structure logical and predictable? + - Are there tight couplings or hidden dependencies? + - Is error handling clear and consistent? + +5. **Naming & Structure** + - Do names accurately describe behavior? + - Are similar things named consistently? + - Is the file/module structure intuitive? + - Are abstractions at the right level? + +### What NOT to Focus On + +- Project-specific conventions (you don't know them) +- Style issues (linters handle this) +- Optimal implementation (focus on clarity, not performance) +- Domain expertise (assume you don't have it) + +### Output Format + +For each issue found, output a structured JSON block: +```json +{ + "file": "path/to/file", + "line": , + "severity": "warning|suggestion", + "category": "clarity", + "type": "unclear_naming|missing_docs|implicit_knowledge|confusing_logic|hidden_dependency", + "comment": "What confused you and what would help" +} +``` + +At the end, provide a summary: +```json +{ + "summary": "Outsider review summary", + "verdict": "approve|request_changes|comment", + "readability_score": "excellent|good|fair|poor", + "key_concerns": ["List of main clarity issues"], + "comments": [] +} +``` + +### Guidelines + +- Be specific: "What does 'ctx' mean here?" not "Variable names unclear" +- Ask the questions a newcomer would ask +- Suggest concrete documentation or naming improvements +- Note when code IS clear and well-documented (positive feedback helps) +- Don't assume insider knowledge - if you have to guess, flag it + +If the code is clear and well-documented: +```json +{ + "summary": "Code is clear and accessible to newcomers.", + "verdict": "approve", + "readability_score": "excellent", + "key_concerns": [], + "comments": [] +} +``` diff --git a/action/prompts/plan-verify.md b/action/prompts/plan-verify.md new file mode 100644 index 0000000000..1b143a9180 --- /dev/null +++ b/action/prompts/plan-verify.md @@ -0,0 +1,89 @@ +# Plan Verification Review Mode + +You are verifying that PR #{pr_number}: "{title}" in {owner}/{repo} implements the stated plan correctly. + +## PR Description + +{pr_description} + +## Linked Plan/Issue + +{linked_content} + +## Changed Files + +{changed_files} + +## Full File Context + +{file_contents} + +## Verification Instructions + +Compare the PR changes against the linked plan/issue. Your task is to verify: + +1. **Completeness**: Does the PR implement everything specified in the plan? +2. **Scope**: Are there changes that weren't in the plan (scope creep)? +3. **Correctness**: Does the implementation match the design described? +4. **Missing Items**: What planned items are missing from the PR? + +### Analysis Approach + +For each item in the plan: +- [ ] Identify what was supposed to be implemented +- [ ] Find the corresponding code changes (or note if missing) +- [ ] Verify the implementation matches the specification +- [ ] Flag any deviations or omissions + +For each change in the PR: +- [ ] Verify it corresponds to a planned item +- [ ] Flag any unplanned additions (scope creep) +- [ ] Note if changes go beyond what was specified + +### Output Format + +For each discrepancy found, output a structured JSON block: +```json +{ + "file": "path/to/file", + "line": , + "severity": "warning|suggestion", + "category": "plan", + "type": "missing|scope_creep|deviation|incomplete", + "planned_item": "Description of what was planned", + "comment": "Description of the discrepancy" +} +``` + +At the end, provide a verification summary: +```json +{ + "summary": "Plan verification summary", + "verdict": "approve|request_changes|comment", + "plan_compliance": { + "implemented": ["list of implemented items"], + "missing": ["list of missing items"], + "scope_creep": ["list of unplanned additions"], + "deviations": ["list of implementation deviations"] + }, + "comments": [] +} +``` + +### Guidelines + +- Be specific about which plan items are implemented vs missing +- Quote the exact plan text when noting deviations +- Distinguish between acceptable variations and problematic deviations +- Scope creep isn't always bad - note if additions are valuable +- Missing items may be intentional (phased approach) - note but don't over-emphasize + +If the PR fully implements the plan: +```json +{ + "summary": "PR implements the plan as specified.", + "verdict": "approve", + "plan_compliance": {"implemented": ["all items"], "missing": [], "scope_creep": [], "deviations": []}, + "comments": [] +} +``` diff --git a/action/prompts/security-review.md b/action/prompts/security-review.md new file mode 100644 index 0000000000..081a15e3b3 --- /dev/null +++ b/action/prompts/security-review.md @@ -0,0 +1,121 @@ +# Security-Focused Review Mode + +You are performing a **security-focused code review** of PR #{pr_number}: "{title}" in {owner}/{repo}. + +## PR Description + +{pr_description} + +## Changed Files + +{changed_files} + +## Full File Context + +{file_contents} + +## Security Review Instructions + +Perform a deep security analysis of this PR. Focus exclusively on security vulnerabilities and risks. + +### Primary Focus Areas + +1. **Authentication & Authorization** + - Auth bypass vulnerabilities + - Privilege escalation paths + - Session management flaws + - JWT/token handling issues + - Missing or incorrect access controls + +2. **Injection Vulnerabilities** + - SQL injection (including ORM misuse) + - Command injection + - XSS (stored, reflected, DOM-based) + - LDAP/XML/XPATH injection + - Template injection + +3. **Data Security** + - Credential leaks (API keys, passwords, tokens) + - Sensitive data exposure + - Missing encryption + - Insecure data transmission + - PII handling violations + +4. **Input Validation & Sanitization** + - Missing input validation + - Insufficient output encoding + - Path traversal vulnerabilities + - Regex denial of service (ReDoS) + - Unvalidated redirects + +5. **Cryptographic Issues** + - Weak algorithms (MD5, SHA1, DES) + - Hardcoded secrets + - Insecure random number generation + - Missing or improper TLS validation + +6. **Race Conditions & TOCTOU** + - Time-of-check to time-of-use flaws + - Race conditions in concurrent code + - Atomicity violations + +7. **Resource Management** + - Resource leaks (file handles, connections) + - Denial of service vectors + - Unbounded operations + +8. **Dependencies & Supply Chain** + - Known vulnerable dependencies + - Suspicious package sources + - Dependency confusion risks + +### Security-Sensitive Files + +Pay extra attention to changes in: +- Authentication handlers and middleware +- Authorization/permission checks +- API endpoint definitions +- Database query construction +- File upload/download handlers +- Cryptographic operations +- CI/CD workflows and Dockerfiles +- Configuration files with secrets + +### Output Format + +For each security issue found, output a structured JSON block: +```json +{ + "file": "path/to/file", + "line": , + "severity": "critical|warning", + "category": "security", + "cwe": "CWE-XXX (optional)", + "owasp": "A01-A10 (optional)", + "comment": "Description of the vulnerability, attack vector, and remediation" +} +``` + +At the end, provide a summary: +```json +{ + "summary": "Security assessment of the PR", + "verdict": "approve|request_changes|comment", + "risk_level": "none|low|medium|high|critical", + "comments": [] +} +``` + +### Guidelines + +- Only report actual security vulnerabilities, not theoretical concerns +- Explain the attack vector: how could an attacker exploit this? +- Rate severity based on exploitability and impact +- Suggest specific remediation for each issue +- Do NOT flag issues that static analyzers like bandit, gitleaks, or trufflehog would catch +- Focus on logic-level security issues that require human judgment + +If no security issues are found: +```json +{"summary": "No security vulnerabilities found.", "verdict": "approve", "risk_level": "none", "comments": []} +``` diff --git a/docs/plans/ai-code-review-bots-plan.md b/docs/plans/ai-code-review-bots-plan.md new file mode 100644 index 0000000000..45bf9ac379 --- /dev/null +++ b/docs/plans/ai-code-review-bots-plan.md @@ -0,0 +1,754 @@ +# Plan: AI-Powered Code Review Bots + +**Issue:** [#134](https://github.com/jwbron/egg/issues/134) +**Related:** #70 (security linters in CI), #77 (autofixers in CI) + +## Executive Summary + +Implement AI-powered code review as a GitHub Action that runs automatically +on PRs. Start with a single-agent reviewer that combines security, +standards, and quality checks in one pass. Use the existing egg Action +infrastructure (`action/action.yml`, gateway + sandbox orchestration) to +avoid building new bot infrastructure. + +## Current State + +egg already has: +- A production GitHub Action (`action/`) that orchestrates gateway + sandbox +- An @mention workflow (`on-mention.yml`) that builds context-rich prompts + from GitHub events and runs Claude Code +- Gateway API allowlists for all PR/review endpoints (files, comments, + reviews, reactions) +- Claude Code runner with streaming output, timeout handling, and error + classification +- `build-mention-prompt.sh` that fetches PR metadata, changed files, and + recent comments + +What's missing: +- A workflow that triggers automatically on PR open/update (not just + @mention) +- A review-specific prompt template +- Review comment posting logic (inline + summary) +- Configuration for review scope and behavior + +## Design Principles + +1. **Single-agent architecture** — one reviewer that handles multiple + concerns per pass. Multi-agent pipelines add complexity without + proportional benefit for code review. +2. **GitHub Actions-native** — no webhook servers, no persistent + infrastructure. Triggered by `pull_request` events. +3. **Reuse existing infrastructure** — the egg Action already handles + container orchestration, credential isolation, and Claude Code execution. +4. **Incremental value** — ship the simplest useful thing first, iterate + based on real usage. +5. **Low false-positive tolerance** — a noisy reviewer gets ignored. Bias + toward fewer, higher-signal comments. + +## Architecture + +### Single-Agent Reviewer (Recommended Starting Point) + +``` +PR opened/updated + │ + ▼ +on-pull-request.yml workflow + │ + ├── Build review prompt (action/build-review-prompt.sh) + │ ├── Fetch PR diff (gh api pulls/{id}/files) + │ ├── Fetch PR description & metadata + │ ├── Fetch file contents for changed files (full context) + │ ├── Load review rules from .egg/review-rules.md (if exists) + │ └── Assemble structured prompt + │ + ├── Run egg Action (jwbron/egg/action@main) + │ └── Claude Code reviews diff with review prompt + │ + └── Post results + ├── Inline comments on specific lines (gh api) + └── Summary comment on PR +``` + +### Why Not Multi-Agent + +The issue references Anthropic's Bughunter (find → verify → promote +pipeline). Multi-agent makes sense when verification is expensive or when +specialization improves accuracy significantly. For code review: + +- A single Claude pass can handle security, standards, and quality together +- Verification of review comments doesn't need a separate agent — the + reviewer can self-verify by reading surrounding code +- Multi-agent adds latency (sequential passes), cost (multiple API calls), + and debugging complexity +- If review quality proves insufficient, we can add a verification pass + later without changing the trigger/posting infrastructure + +### Why Not a Webhook Server + +The issue asks whether this should be a GitHub Action, pre-commit hook, or +egg-native feature. Recommendation: **GitHub Action**. + +- egg already runs as a GitHub Action — the infrastructure exists +- No persistent server to maintain, no webhook endpoint to secure +- GitHub Actions handles concurrency, retries, and audit logging +- Same security model (gateway sidecar, credential isolation) applies +- Pre-commit hooks run locally and can't access the full PR context + (description, linked issues, other files). They're better for formatting + and simple lint (already handled by ruff, shellcheck, etc. in + `.pre-commit-config.yaml`) + +## Implementation Plan + +### Phase 1: Single-Pass Reviewer (MVP) + +**Goal:** Automatically review every PR with a single Claude pass that +covers security, standards, and quality. + +#### 1.1 Review Prompt Builder (`action/build-review-prompt.sh`) + +New script that constructs the review prompt. Separate from +`build-mention-prompt.sh` because the context and instructions differ. + +**Inputs (from GitHub event context):** +- PR number, title, description +- Base and head branches +- Changed files with diffs (from `gh api pulls/{id}/files`) +- Full file contents for changed files (for surrounding context) +- Repository-level review rules (`.egg/review-rules.md` if present) + +**Prompt structure:** +``` +You are reviewing PR #{number}: "{title}" in {owner}/{repo}. + +## PR Description +{description} + +## Review Rules +{contents of .egg/review-rules.md, or default rules} + +## Changed Files +{for each file: filename, status (added/modified/deleted), patch/diff} + +## Full File Context +{for each modified file: complete current file contents} + +## Instructions + +Review this PR for: +1. Security issues (vulnerabilities, unsafe patterns, credential leaks) +2. Correctness (logic errors, edge cases, error handling gaps) +3. Code quality (readability, maintainability, naming) +4. Standards compliance (project conventions per review rules) + +For each issue found, output a structured JSON block: +{ + "file": "path/to/file", + "line": , + "severity": "critical|warning|suggestion", + "category": "security|correctness|quality|standards", + "comment": "Description of the issue and suggested fix" +} + +The "line" field must be the actual line number in the file (as shown +in the GitHub file viewer on the HEAD commit), NOT a diff-relative +position. + +If the PR looks good, output: +{"summary": "No significant issues found.", "comments": []} + +Rules for comments: +- Only comment on things that are actually wrong or risky +- Do not comment on style preferences already handled by linters +- Do not repeat what ruff, mypy, shellcheck, or bandit would catch +- Focus on issues that require human judgment to detect +- Be specific: reference the exact line and explain why it's a problem +- Suggest a fix when possible +``` + +**Truncation limits:** +- Individual file diffs: 15,000 chars (skip very large generated files) +- Full file contents: 30,000 chars per file +- Overall prompt: 100,000 chars (Claude's context window is large, but + cost scales with input size) +- Skip binary files, lock files, generated files (`.lock`, `.min.js`, + `package-lock.json`, etc.) + +**Prompt output mechanism:** The assembled prompt can be large (up to +100K chars). `$GITHUB_OUTPUT` uses multiline heredoc syntax and has +practical size limits (~1 MB) where large multiline values become +fragile. Instead of writing the prompt to `$GITHUB_OUTPUT`, the script +writes it to a temp file (`$RUNNER_TEMP/review-prompt.txt`) and outputs +only the file path: +```bash +PROMPT_FILE="$RUNNER_TEMP/review-prompt.txt" +# ... assemble prompt into $PROMPT_FILE ... +echo "prompt-file=$PROMPT_FILE" >> "$GITHUB_OUTPUT" +``` +The workflow step then passes the file path to the egg Action, which +reads the prompt from the file. This avoids `GITHUB_OUTPUT` size limits +entirely and matches how `build-mention-prompt.sh` handles large +prompts. The egg Action's `prompt` input would accept either inline text +or a `file://` path (or a new `prompt-file` input could be added). + +#### 1.2 Review Workflow (`.github/workflows/on-pull-request.yml`) + +```yaml +name: "egg: Code Review" + +on: + pull_request: + types: [opened, synchronize, ready_for_review, reopened] + +jobs: + review: + name: AI Code Review + runs-on: ubuntu-latest + # Skip draft PRs, bot PRs, and PRs with [skip-review] in title + if: >- + !github.event.pull_request.draft && + github.event.pull_request.user.login != 'james-in-a-box' && + github.event.pull_request.user.login != 'james-in-a-box[bot]' && + !contains(github.event.pull_request.title, '[skip-review]') + + permissions: + contents: read + pull-requests: write + + concurrency: + group: egg-review-${{ github.event.pull_request.number }} + cancel-in-progress: true # Cancel stale reviews on new pushes + + steps: + - name: Generate bot token + id: bot-token + uses: actions/create-github-app-token@v1 + with: + app-id: ${{ secrets.BOT_APP_ID }} + private-key: ${{ secrets.BOT_APP_PRIVATE_KEY }} + + - name: Checkout (trusted main for prompt building) + uses: actions/checkout@v4 + with: + ref: main + + - name: Build review prompt + id: prompt + run: bash action/build-review-prompt.sh + env: + GH_TOKEN: ${{ steps.bot-token.outputs.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + + - name: Checkout PR branch + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.ref }} + + - name: Run egg review + id: egg + uses: jwbron/egg/action@main + with: + prompt-file: ${{ steps.prompt.outputs.prompt-file }} + model: ${{ steps.prompt.outputs.model }} + anthropic-oauth-token: ${{ secrets.ANTHROPIC_OAUTH_TOKEN }} + bot-app-id: ${{ secrets.BOT_APP_ID }} + bot-app-private-key: ${{ secrets.BOT_APP_PRIVATE_KEY }} + bot-app-installation-id: ${{ secrets.BOT_APP_INSTALLATION_ID }} + bot-username: james-in-a-box + timeout: 10 # Reviews should be fast + + - name: Post review comments + if: always() && steps.egg.outputs.exit-code == '0' + env: + GH_TOKEN: ${{ steps.bot-token.outputs.token }} + run: bash action/post-review-comments.sh +``` + +**Key design choices:** +- **`cancel-in-progress: true`** — when a new push arrives, cancel the + running review. The old review is stale anyway. +- **Separate from on-mention.yml** — different trigger, different prompt, + different purpose. Keeps workflows focused. +- **Skip bot's own PRs** — prevent self-review loops. +- **`[skip-review]`** — escape hatch for trivial PRs (docs-only, typo + fixes). +- **10-minute timeout** — reviews should complete fast. If Claude needs + more than 10 minutes, the PR is probably too large to review well. +- **Trusted checkout for prompt building** — same security pattern as + `on-mention.yml`. Build the prompt from main branch code, then checkout + the PR branch for the actual review. + +#### 1.3 Review Comment Poster (`action/post-review-comments.sh`) + +Parses Claude's JSON output and posts review comments via GitHub API. + +**Approach:** +1. Read Claude's output from the log file (`${{ steps.egg.outputs.log-file }}`) +2. Extract JSON blocks (structured review comments) +3. For each comment with a file and line number, post as an inline PR + review comment via `gh api` +4. Post a summary comment with overall assessment + +**Log file format and parsing:** The egg Action's `log-file` output +contains Claude's raw response text. Claude may wrap JSON in markdown +code fences (`` ```json ... ``` ``), include preamble/postamble text, +or produce multiple JSON blocks. The parser must handle all of these: + +```bash +# Extract JSON blocks from Claude's output, handling: +# 1. JSON wrapped in ```json ... ``` code fences +# 2. Bare JSON objects/arrays in the output +# 3. Preamble/postamble text around JSON +# Use grep + jq to find and validate JSON blocks: +grep -Pzo '(?s)\{[^{}]*"file"[^{}]*\}' "$LOG_FILE" | \ + jq -s '.' 2>/dev/null || echo '[]' +``` + +For robustness, the prompt asks Claude to wrap all review output in a +single top-level JSON object with a `comments` array. The parser first +tries to extract this structured object. If that fails (Claude deviated +from the format), it falls back to scanning for individual JSON comment +blocks. If all JSON extraction fails, the raw output is posted as a +plain PR comment (the fallback described below). + +**GitHub API calls:** + +The `gh api` CLI does not support array construction via repeated `-f` +flags. Instead, construct a JSON payload with `jq` and pass it via +`--input`: + +```bash +# Build the review payload as JSON +REVIEW_PAYLOAD=$(jq -n \ + --arg event "COMMENT" \ + --arg body "$SUMMARY" \ + --argjson comments "$COMMENTS_JSON" \ + '{event: $event, body: $body, comments: $comments}') + +# Post the review +echo "$REVIEW_PAYLOAD" | gh api repos/{owner}/{repo}/pulls/{pr}/reviews \ + -X POST \ + --input - +``` + +Where `$COMMENTS_JSON` is a JSON array built by the parser: +```json +[ + { + "path": "src/auth.py", + "line": 42, + "side": "RIGHT", + "body": "**security:** This input is not sanitized before..." + } +] +``` + +**Line numbers vs. diff positions:** The GitHub Pull Request Reviews API +supports two modes for positioning inline comments: + +1. **`position`** (legacy) — the line index within the diff hunk. This + requires mapping file line numbers to diff-relative offsets, which is + fragile and error-prone. +2. **`subject_type: "line"` with `line` and `side`** (available since + 2022) — accepts actual file line numbers. `side: "RIGHT"` refers to + the new version of the file (head commit), `side: "LEFT"` refers to + the old version (base commit). + +This plan uses the newer `line`/`side` approach. The prompt instructs +Claude to output the file line number (not diff position), and the +comment poster sets `side: "RIGHT"` for all comments (since reviews +comment on the proposed code). This avoids the diff-position mapping +entirely. + +The prompt's JSON output schema is updated accordingly: +```json +{ + "file": "path/to/file", + "line": 42, + "severity": "critical|warning|suggestion", + "category": "security|correctness|quality|standards", + "comment": "Description of the issue and suggested fix" +} +``` +Where `line` is the **file line number in the head commit** (the number +shown in the GitHub file viewer), not a diff-relative position. + +Using `event: COMMENT` (not `REQUEST_CHANGES` or `APPROVE`) — the bot +provides information, not blocking decisions. Humans decide whether to +act on the feedback. + +**Fallback:** If JSON parsing fails (Claude didn't follow the format), +post the raw output as a single PR comment. This ensures the review is +never silently lost. + +#### 1.4 Repository Review Rules (`.egg/review-rules.md`) + +Optional per-repository configuration file that customizes review behavior. + +```markdown +# Review Rules + +## Focus Areas +- Security: Pay special attention to input validation in API handlers +- We use Django ORM exclusively — flag any raw SQL queries +- All new API endpoints must have rate limiting + +## Ignore +- Don't comment on import ordering (ruff handles this) +- Don't comment on type annotations (mypy handles this) +- Migrations files are auto-generated — skip them + +## Project Context +- This is a Django/React application +- Authentication uses Django REST Framework JWT +- We deploy on GCP Cloud Run +``` + +This file lives in the reviewed repository (not in egg), so each repo +can customize its review rules. The prompt builder reads it if present, +or uses sensible defaults. + +### Phase 2: Specialized Review Modes *(Implemented)* + +After the MVP is running and generating useful feedback, add specialized +modes that can be triggered explicitly or run as additional passes. + +**Implementation Status:** Phase 2 is complete. The following have been added: + +**Default Behavior:** On PR open/update, three review modes run in parallel +by default: security, plan verification, and outsider review. Deep review +remains manually triggered only. Specific modes can be run individually via +workflow_dispatch. +- `action/prompts/security-review.md` — Security-focused review prompt +- `action/prompts/plan-verify.md` — Plan verification review prompt +- `action/prompts/outsider-review.md` — Outsider/clarity review prompt +- `action/build-deep-review-prompt.sh` — Deep review prompt builder +- Updated `build-review-prompt.sh` to support specialized modes +- Updated `on-pull-request.yml` with workflow dispatch inputs for modes +- Updated `action.yml` with review-mode input + +#### 2.1 Security-Focused Review + +A review mode specifically tuned for security: +- Deeper analysis of authentication/authorization changes +- OWASP Top 10 pattern matching +- Dependency vulnerability awareness (cross-reference with + `pip-audit`/`npm audit`) +- Secrets detection (complementing `trufflehog`/`gitleaks`) + +**Trigger:** Automatically for PRs that touch security-sensitive files +(auth, middleware, API handlers, Dockerfiles, CI workflows), or manually +via `@egg security-review`. + +**Integration with #70:** The security linters issue covers traditional +SAST tools (bandit, etc.). This AI review catches what static analysis +can't: logic-level auth bypasses, TOCTOU issues, subtle injection +vectors, and insecure-by-design patterns. + +#### 2.2 Plan Verification Review + +Compares PR changes against the stated plan (issue description, JIRA +ticket, or linked plan document): +- Does the PR implement what was planned? +- Are there changes that weren't in the plan (scope creep)? +- Are planned items missing from the PR? + +**Trigger:** When PR description links to an issue or JIRA ticket, fetch +the linked content and include it in the prompt. + +#### 2.3 Bounded-Context "Outsider" Review + +A reviewer that deliberately operates without internal project knowledge: +- No `.egg/review-rules.md` loaded +- No project-specific context beyond what's in the diff +- Reviews from first principles: "Would a competent engineer unfamiliar + with this codebase understand this code?" +- Surfaces documentation gaps, unclear naming, implicit assumptions + +**Trigger:** Manual via `@egg outsider-review`. Useful for code that will +be maintained by people outside the original team. + +#### 2.4 Deep Review Mode + +A review mode that gives the bot direct PR access for multi-turn analysis +and exploratory investigation, rather than constraining it to structured +JSON output. + +**Rationale:** The Phase 1 structured output approach is predictable and +testable, but constrains what the bot can do. Some reviews benefit from: +- Running tests to validate suspected issues +- Exploring the codebase to understand impact +- Cross-referencing related files not in the diff +- Multi-turn analysis: "I found X, let me check if Y is also affected..." +- Posting inline code suggestions using GitHub's suggestion blocks + +**Capabilities:** +- **Test execution:** Run `make test` or specific test files to validate + concerns (e.g., "This change might break the auth flow — let me run + the auth tests to confirm") +- **Codebase exploration:** Read files outside the diff to understand + context, check for similar patterns, or verify assumptions +- **Multi-turn reasoning:** Follow chains of investigation rather than + producing a single-pass output +- **Inline suggestions:** Post GitHub suggestion blocks for concrete fixes: + ```suggestion + with open(path) as f: + data = f.read() + ``` +- **Direct PR interaction:** Post comments directly via `gh pr review` + rather than through post-processing + +**Architecture:** + +``` +PR opened/updated (with deep-review trigger) + │ + ▼ +on-pull-request.yml workflow (deep-review mode) + │ + ├── Build deep-review prompt (action/build-deep-review-prompt.sh) + │ ├── Fetch PR diff and metadata + │ ├── Load review rules + │ └── Assemble prompt with direct-posting instructions + │ + └── Run egg Action with extended permissions + └── Claude Code with full PR access: + ├── Reads files beyond the diff + ├── Runs tests to validate concerns + ├── Posts comments directly via gh + └── Can do multi-turn exploration +``` + +**Key differences from Phase 1:** +| Aspect | Phase 1 (Structured) | Deep Review | +|--------|---------------------|-------------| +| Output | JSON → post-processor | Direct `gh` calls | +| Scope | Changed files only | Full codebase access | +| Analysis | Single pass | Multi-turn exploration | +| Test execution | No | Yes (read-only) | +| Suggestions | Schema extension | Native GitHub blocks | +| Predictability | High | Lower | +| Cost | Lower (faster) | Higher (longer runs) | +| Debugging | Inspect JSON output | Review action logs | + +**Guardrails:** +- **Time limit:** 30-minute timeout (vs 10 minutes for structured review) +- **Comment limit:** Maximum 10 inline comments per review to prevent spam +- **Test scope:** Can only run tests, not modify code or push commits +- **No self-approval:** Cannot approve or request changes, only comment + +**Trigger:** Manual via `@egg deep-review`. Not automatic — use for: +- Complex PRs that touch many subsystems +- PRs where the structured review flagged potential issues worth + investigating +- Security-sensitive changes that warrant deeper analysis +- PRs from new contributors where extra scrutiny is valuable + +**Prompt structure:** +``` +You are performing a deep review of PR #{number}: "{title}" in {owner}/{repo}. + +## PR Description +{description} + +## Changed Files +{diff summary} + +## Instructions + +You have full access to the repository and can: +1. Read any file in the codebase (use the Read tool) +2. Run tests to validate concerns (use Bash with pytest/jest) +3. Post review comments directly (use `gh pr review`) + +Review this PR thoroughly. For each issue you find: +1. Investigate to confirm it's a real problem (check related code, run tests) +2. Post an inline comment explaining the issue +3. If you have a concrete fix, use a GitHub suggestion block + +For suggestions, use this format in your comment: +\`\`\`suggestion +corrected code here +\`\`\` + +Limit yourself to the 10 most important findings. Focus on: +- Security vulnerabilities +- Correctness bugs +- Significant code quality issues + +Do NOT comment on style issues or things linters would catch. +``` + +**Implementation notes:** +- Uses the same egg Action infrastructure but with a longer timeout +- Prompt builder sets `mode: deep-review` which the action recognizes +- The action grants additional tool permissions (Read for all files, Bash + for test execution) +- Comment posting happens inline during the review, not as post-processing +- The workflow captures the action log for debugging but doesn't parse it + +### Phase 3: Review Infrastructure Improvements + +#### 3.1 False Positive Management + +Track which review comments get resolved vs. dismissed: +- Store review feedback outcomes (resolved, won't fix, false positive) +- Use feedback to refine prompts over time +- Add a reaction-based feedback mechanism: maintainer adds thumbs-down + to false positive comments + +**Implementation:** A simple JSON file in the repo +(`.egg/review-feedback.json`) or a GitHub issue that accumulates feedback +patterns. The prompt builder reads this to add "do not flag" patterns. + +#### 3.2 Incremental Review + +On `synchronize` events (new push to PR), only review the new changes: +- Diff between previous head and new head +- Skip files that haven't changed since last review +- Reference previous review comments to avoid repeating + +The `pull_request.synchronize` event payload includes `before` and +`after` SHAs, which provide the previous and new head commits directly. +This means no external state storage is needed between runs — the +incremental diff can be computed as `git diff ` using +values from the event payload (`github.event.before` and +`github.event.after`). + +This reduces cost and noise for iterative PRs. + +#### 3.3 Review Metrics Dashboard + +Track review effectiveness: +- Number of comments per PR +- Comment resolution rate (acted on vs. dismissed) +- False positive rate +- Categories of issues found (security vs. quality vs. standards) +- Time to review + +Surface via a periodic summary (weekly Slack notification or GitHub +issue). + +### Phase 4: AI Lintbot Integration + +For checks that are too nuanced for static analysis but too formulaic for +full review: + +#### 4.1 Semantic Naming Checker +- Flag variables/functions with misleading names +- Detect name/behavior mismatches (e.g., `is_valid()` that returns a + string) +- Suggest more descriptive names for single-letter variables in non-trivial + scope + +#### 4.2 Logic Correctness Beyond Types +- Off-by-one errors in loops +- Null/undefined paths that type systems miss +- Race conditions in async code +- Resource leaks (open files, unclosed connections) + +#### 4.3 API Usage Antipatterns +- N+1 query patterns in ORM code +- Blocking calls in async contexts +- Unbounded queries without pagination +- Cache invalidation gaps + +**Implementation:** These run as additional prompt modes within the same +GitHub Action infrastructure. Each is a specialized prompt template +(`action/prompts/naming.md`, `action/prompts/logic.md`, etc.) that the +workflow selects based on configuration. + +## Integration with Existing Issues + +### #70 — Security Linters in CI + +Issue #70 covers adding traditional security linters (bandit, custom +scripts) to `make lint`. AI review complements this: +- Static linters catch syntactic patterns (hardcoded passwords, dangerous + function calls) +- AI review catches semantic patterns (logic-level auth bypasses, insecure + design decisions) +- No overlap: the AI reviewer is explicitly instructed to skip issues that + bandit/ruff would catch + +### #77 — Autofixers in CI + +Issue #77 covers automated fixing of lint issues. AI review could feed +into this: +- Phase 1: Review comments are informational only +- Future: For high-confidence suggestions (e.g., "this variable should be + renamed from `x` to `user_count`"), the bot could open a fix PR + against the reviewed PR's branch +- This requires careful scoping — autofixes should be limited to + mechanical changes, not logic rewrites + +## Deliverables Summary + +| Phase | Deliverable | New Files | Modifies | Status | +|-------|------------|-----------|----------|--------| +| 1 | Review prompt builder | `action/build-review-prompt.sh` | — | Done | +| 1 | Review workflow | `.github/workflows/on-pull-request.yml` | — | Done | +| 1 | Comment poster | `action/post-review-comments.sh` | — | Done | +| 1 | Review rules spec | Documented convention for `.egg/review-rules.md` | — | Done | +| 2 | Security review mode | `action/prompts/security-review.md` | `build-review-prompt.sh` | Done | +| 2 | Plan verification mode | `action/prompts/plan-verify.md` | `build-review-prompt.sh` | Done | +| 2 | Outsider review mode | `action/prompts/outsider-review.md` | `build-review-prompt.sh` | Done | +| 2 | Deep review mode | `action/build-deep-review-prompt.sh` | `on-pull-request.yml`, `action.yml` | Done | +| 3 | Feedback tracking | `.egg/review-feedback.json` convention | `build-review-prompt.sh` | — | +| 3 | Incremental review | — | `on-pull-request.yml`, `build-review-prompt.sh` | — | +| 3 | Metrics dashboard | `action/review-metrics.sh` | — | — | +| 4 | AI lintbot prompts | `action/prompts/naming.md`, etc. | `on-pull-request.yml` | — | + +## Open Questions + +1. **Cost management:** Each review costs an API call. For active repos + with many PRs, this adds up. Should we add a daily/weekly budget cap? + Or limit reviews to PRs from certain authors? + +2. **Review comment threading:** ~~Should the bot reply to its own previous + comments when updating a review (on new push), or delete old comments + and post fresh?~~ **Resolved:** On new pushes, dismiss (resolve) the + bot's previous review and post a fresh review. The GitHub API supports + dismissing reviews via `PUT /repos/{owner}/{repo}/pulls/{pr}/reviews/{id}/dismissals`. + This keeps the PR timeline clean (old reviews are collapsed/resolved) + while preserving history (dismissed reviews remain visible in the + timeline if someone wants to see them). The comment poster should + query for existing bot reviews and dismiss them before posting the new + one. This is simpler than threading (no need to match old comments to + new ones) and avoids the clutter of accumulated stale reviews. + +3. **Blocking vs. advisory:** The MVP uses `COMMENT` review events + (advisory only). Should critical security findings use + `REQUEST_CHANGES` to block merge? This would give the AI reviewer + veto power, which is a significant trust escalation. + +4. **Model selection:** ~~Reviews don't need the most powerful model for + every PR.~~ **Resolved:** Use the existing `model` input on the egg + Action to parameterize model selection. Default to Haiku for PRs with + 5 or fewer changed files, Opus for larger PRs. The threshold is + configurable via `.egg/review-rules.md` (e.g., + `model-threshold-files: 10`). The prompt builder counts changed files + and sets the model accordingly in the workflow output. This keeps + costs low for small PRs while using a more capable model when the + review is likely to benefit from it. + +5. **Review of egg's own PRs:** Should the egg repo itself use this + reviewer? Dogfooding would be valuable, but the bot reviewing its own + codebase creates a feedback loop that needs careful handling. + +## Recommendation + +Start with Phase 1 (single-pass reviewer via GitHub Action). This: +- Leverages existing infrastructure (egg Action, gateway, Claude Code) +- Requires only 3 new files + 1 workflow +- Delivers value immediately on every PR +- Provides a foundation for all later phases +- Is reversible (disable the workflow if it's not useful) + +Phase 1 implementation can be done as a single PR. The deliverables are: +1. `action/build-review-prompt.sh` +2. `action/post-review-comments.sh` +3. `.github/workflows/on-pull-request.yml` +4. Documentation for `.egg/review-rules.md` convention + +Authored-by: egg