Implement Phase 1: AI-powered code review

[james-in-a-box]

Summary

Implements Phase 1 of AI-powered code review per the plan in #140.

• Add action/build-review-prompt.sh to construct review prompts from PR
  metadata, diffs, and full file contents
• Add action/post-review-comments.sh to parse Claude's JSON output and
  post GitHub PR reviews with inline comments
• Add prompt-file input to the egg action for large prompts
• Add .github/workflows/on-pull-request.yml workflow (see note below)

Key features

• Respects .egg/review-rules.md for per-repo customization
• Selects model automatically (haiku for small PRs, opus for larger ones)
• Dismisses previous bot reviews to avoid clutter
• Skips draft PRs, bot PRs, and PRs with [skip-review] in title
• Uses cancel-in-progress to abort stale reviews on new pushes

Workflow file limitation

The workflow file (.github/workflows/on-pull-request.yml) is in this
branch but could not be pushed due to GitHub App permission
restrictions. Options:

1. A human can cherry-pick or copy the workflow file after merging
2. Grant workflows permission to the GitHub App
3. Manually create the workflow file from the content in this PR

Issue: #134
Related: #140 (plan), #70 (security linters), #77 (autofixers)

Test plan

• Review script logic for prompt construction and comment parsing
• Verify action.yml changes support both prompt and prompt-file inputs
• Test workflow locally with act or on a test repository
• Confirm inline comments are posted correctly on a real PR

Authored-by: egg

[jwbron]

@james-in-a-box rlecting on this a bit more, does it actually make more sense to have the structured output that we pass along as a PR review, or to just let the bot post on its own seeing as it has full read write access to the PR?

For example, the bot can do a broader analysis of the codebase or integrate internal research, run tests to validate its findings, etc.

Do we really want to limit the bot by sticking with this structured output format? Can we support a sufficiently broad use case with this? Can we support inline comments, change suggestions, etc?

[james-in-a-box]

Good question — this gets at a fundamental architectural choice. Let me lay out the trade-offs:

Current approach: Structured output → post-processing

Pros:

• Predictable, testable output format
• Easy to parse, validate, and filter comments
• Can enforce policies (e.g., max comments, severity thresholds)
• Debugging is straightforward — just inspect the JSON
• Works with any model (including faster/cheaper ones for small PRs)

Cons:

• Constrains what the bot can do
• Can't easily support multi-turn analysis or codebase exploration
• Inline suggestions require careful line-number mapping (fragile)

Alternative: Let the bot post directly

Pros:

• Bot can run tests, explore the codebase, cross-reference files
• Multi-turn analysis: "I found X, let me check if Y is also affected..."
• Can post inline suggestions using gh pr review with suggestion blocks
• More flexible — bot decides how to structure feedback
• Could integrate with other tools (run linters, check dependencies)

Cons:

• Less predictable — harder to test and validate behavior
• Could spam PRs if not carefully constrained
• Debugging is harder (what did the bot actually do?)
• More expensive (longer runs, more API calls)
• Need guardrails to prevent runaway comments

My recommendation

I think the current structured approach is the right starting point for Phase 1, but we should design it to be extensible:

1. Keep structured output for the core review flow — it's simpler, testable, and sufficient for 80% of cases (security issues, correctness bugs, quality feedback)

2. Add a "deep review" mode for Phase 2 that gives the bot direct PR access for:

   • Running tests to validate concerns
   • Cross-referencing related files
   • Posting multi-line suggestions
   • More exploratory analysis

3. For inline suggestions specifically — we can support them in the structured format by extending the schema:

{
  "file": "path/to/file",
  "line": 42,
  "severity": "suggestion",
  "category": "quality",
  "comment": "Consider using a context manager here",
  "suggestion": "with open(path) as f:\n    data = f.read()"
}

GitHub's review API supports suggestion blocks:

\`\`\`suggestion
with open(path) as f:
    data = f.read()
\`\`\`

This keeps the structured approach but enables richer feedback.

Bottom line: I'd ship Phase 1 as-is (structured output) since it works and is testable. Then we can add a deep-review mode in Phase 2 that gives the bot direct access for cases that need codebase exploration or test validation.

Does this framing help? Happy to adjust the Phase 1 implementation if you want to go a different direction.

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[jwbron]

@james-in-a-box makes sense and agreed. Go ahead and add a seep-review mode to the plan PR.

[jwbron]

@james-in-a-box review this PR

[james-in-a-box]

Done — I've added a deep-review mode to the plan PR (#140) as section 2.4 in Phase 2.

Summary of what's documented:

• Rationale: The Phase 1 structured output is predictable and testable, but constrains what the bot can do. Deep review unlocks multi-turn analysis for cases that need it.

• Capabilities:

  • Run tests to validate suspected issues
  • Explore files outside the diff
  • Multi-turn investigation chains
  • Post inline code suggestions using GitHub suggestion blocks
  • Direct PR interaction via gh pr review

• Guardrails:

  • 30-minute timeout (vs 10 minutes for structured)
  • Maximum 10 inline comments to prevent spam
  • Can only run tests, not modify code
  • Cannot approve or request changes, only comment

• Trigger: Manual via @egg deep-review — not automatic, for use on complex PRs or when the structured review flags issues worth investigating.

The deliverables table now includes deep review mode with its own prompt builder (action/build-deep-review-prompt.sh) and modifications to the workflow and action.yml.

See the updated plan: https://github.com/jwbron/egg/pull/140/files

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[james-in-a-box]

Code Review

This is a solid Phase 1 implementation of AI-powered code review. The architecture is clean and the scripts are well-structured. A few issues to address:

Issues Found

1. Line number mapping may fail for inline comments (post-review-comments.sh:183-189)

The GitHub review API expects line numbers relative to the diff, not absolute file line numbers. The prompt instructs Claude to provide "actual line number in the file" but the API uses diff-relative positions. This will cause inline comments to either fail or land on wrong lines.

Options:

• Convert absolute line numbers to diff positions using the patch hunks
• Or change the prompt to request diff-relative line numbers
• Or use the subject_type: "file" approach which doesn't require line mapping

2. Regex pattern in JSON extraction is fragile (post-review-comments.sh:70)

The pattern [^{}]* won't handle nested objects correctly. If Claude's comment contains {} characters (e.g., explaining code), the regex will break.

The Python fallback at line 81-103 is more robust but the regex patterns above it may incorrectly match partial JSON.

3. Race condition in dismiss_previous_reviews (post-review-comments.sh:129)

The while read loop with gh_api_safe inside creates a subshell, so errors won't propagate. Also, if dismissal fails mid-loop, partial state remains. Consider collecting all review IDs first, then processing.

4. Potential command injection in gh_api_safe (build-review-prompt.sh:103)

The error message interpolates $* directly. While currently used safely, if file paths ever contain special characters this could be problematic in logs. Consider quoting: "gh api $*"

5. Missing error handling for base64 decode (build-review-prompt.sh:144)

If the GitHub API returns malformed base64, the || echo "" suppresses the error silently. This could lead to empty file contents being passed to review without indication.

Suggestions

Model selection threshold (build-review-prompt.sh:25-26): Consider also factoring in total diff size, not just file count. A single file with 2000+ changed lines may warrant opus.

Workflow file: Since the workflow couldn't be pushed, consider adding a docs/setup-review-workflow.md with the workflow content and manual setup instructions.

What looks good

• Comprehensive skip patterns for generated/binary files
• Sensible truncation limits to avoid token overflow
• Fallback to regular comment if review API fails
• Dismissal of previous bot reviews to reduce clutter
• Clean separation between prompt building and comment posting

Inline Comments

• action/post-review-comments.sh:188 [warning/correctness] GitHub's review API expects diff-relative line numbers (position within the patch), not absolute file line numbers. The prompt asks Claude for absolute line numbers, so inline comments may fail or land on wrong lines.

• action/post-review-comments.sh:70 [warning/correctness] The regex [^{}]* won't handle nested JSON objects. If Claude's comment contains braces (e.g., code snippets), this pattern will fail.

• action/build-review-prompt.sh:308 [suggestion/correctness] This instruction asks for "actual line number in the file" but GitHub's review comment API expects diff-relative positions. This mismatch will cause inline comments to fail.

---

This is an automated review. Please evaluate suggestions carefully.

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[jwbron]

@james-in-a-box address the above feedback

[james-in-a-box]

I've addressed all the feedback from the code review:

1. Line number mapping (Issue #1)

Added get_diff_position() function that converts absolute file line numbers to diff-relative positions. The function:

• Parses diff hunk headers to track line numbers
• Properly handles context lines, additions, and deletions
• Returns the correct position for GitHub's review API

Comments on lines not in the diff (e.g., context lines) are now gracefully handled - they're included in the review body under "Additional comments (lines not in diff)" instead of failing silently.

2. Fragile JSON regex (Issue #2)

Replaced the [^{}]* regex patterns with a robust Python-based JSON extractor that:

• Uses proper bracket matching to handle nested objects
• Correctly handles escaped characters in strings
• First checks markdown code blocks, then bare JSON
• Falls back gracefully if no valid JSON found

3. Race condition in dismiss_previous_reviews (Issue #3)

Refactored to collect all review IDs into arrays first, then process them. This:

• Avoids subshell issues with while read pipelines
• Properly tracks failed dismissals
• Reports a summary of failures at the end

4. Command injection in gh_api_safe (Issue #4)

Changed from direct $* interpolation to using printf with proper quoting:

cmd_display=$(printf "'gh api %s'" "$*")

5. Base64 decode error handling (Issue #5)

Added explicit error handling in fetch_file_content() and fetch_review_rules():

• Logs warnings when content fetch returns empty
• Logs warnings when base64 decode fails
• Returns empty string with clear indication of what failed

Also updated the prompt instructions to clarify that line number conversion is handled automatically by the posting script.

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[jwbron]

@james-in-a-box the workflow has been added. Review it.

[james-in-a-box]

Workflow Review

The workflow file has been added and looks good overall. Here's my review:

What looks good

1. Security model is correct - Using a trusted checkout from main for build-review-prompt.sh before checking out the PR branch is the right approach. This prevents a malicious PR from modifying the prompt builder to exfiltrate secrets.

2. Proper skip conditions - The if condition correctly skips:

   • Draft PRs
   • Bot PRs (both james-in-a-box and james-in-a-box[bot])
   • PRs with [skip-review] in title

3. Concurrency handling - Using cancel-in-progress: true with a PR-specific group prevents stale reviews from accumulating on rapid pushes.

4. Minimal permissions - Only requests contents: read and pull-requests: write, which is the minimum needed.

5. Conditional comment posting - The if: always() && steps.egg.outputs.exit-code == '0' ensures we only post if Claude succeeded.

Issues to address

1. HEAD_SHA may not match after checkout (line 74)

The workflow captures HEAD_SHA from github.event.pull_request.head.sha, then checks out github.event.pull_request.head.ref (the branch name). If someone pushed to the branch between workflow trigger and checkout, these won't match. The review comments would be posted to the wrong commit.

Fix: After checking out the PR branch, capture the actual HEAD:

- name: Capture actual HEAD SHA
  id: head
  run: echo "sha=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"

- name: Post review comments
  ...
  env:
    HEAD_SHA: ${{ steps.head.outputs.sha }}

2. Missing repository context for fork PRs (lines 45-48, 70-76)

When a PR comes from a fork, GITHUB_REPOSITORY is the base repo but the head.ref may not exist there. The workflow uses actions/checkout@v4 with just ref: which works for same-repo PRs but may fail for forks.

Fix: For fork PRs, you need to explicitly specify the repository:

- name: Checkout PR branch
  uses: actions/checkout@v4
  with:
    repository: ${{ github.event.pull_request.head.repo.full_name }}
    ref: ${{ github.event.pull_request.head.ref }}

However, this introduces a security consideration: the "Run egg review" step would then execute code from an untrusted fork. The current design (checkout trusted main → build prompt → checkout PR branch → run Claude) is intentional to keep prompt building secure, but Claude would still see untrusted code. This is acceptable since Claude only reviews, doesn't execute arbitrary code from the PR.

3. Token scope for fork PRs

The create-github-app-token step generates a token for the base repo. For PRs from forks, this token can still read the fork (public forks) or may fail (private forks). This is probably fine for your use case but worth documenting.

Suggestions

Add workflow_dispatch for manual testing:

on:
  pull_request:
    types: [opened, synchronize, ready_for_review, reopened]
  workflow_dispatch:
    inputs:
      pr_number:
        description: 'PR number to review'
        required: true

This makes it easier to re-run reviews or test on specific PRs.

Summary

The workflow is well-designed with proper security boundaries. The two issues above (HEAD_SHA race and fork support) should be addressed before relying on this for production reviews. The HEAD_SHA issue can cause inline comments to fail; the fork issue will cause reviews to fail entirely on fork PRs.

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[jwbron]

@james-in-a-box address the feedback in the last comment

[james-in-a-box]

Workflow Update Required

I attempted to address the feedback from the workflow review, but the push failed due to GitHub App permission restrictions (no workflows permission). The changes need to be applied manually.

Changes made (commit 46297d9)

1. Added workflow_dispatch trigger for manual testing:

on:
  pull_request:
    types: [opened, synchronize, ready_for_review, reopened]
  workflow_dispatch:
    inputs:
      pr_number:
        description: 'PR number to review'
        required: true
        type: number

2. Added fork PR support with repository parameter:

- name: Checkout PR branch
  uses: actions/checkout@v4
  with:
    repository: ${{ github.event.pull_request.head.repo.full_name }}
    ref: ${{ github.event.pull_request.head.ref }}

3. Fixed HEAD_SHA race condition by capturing actual HEAD after checkout:

- name: Capture actual HEAD SHA
  id: head
  run: echo "sha=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"

And updated the Post review comments step to use the captured SHA:

HEAD_SHA: ${{ steps.head.outputs.sha }}

Options to apply these changes

1. Grant workflows permission to the GitHub App, then I can push
2. Manually update .github/workflows/on-pull-request.yml with the changes above
3. Cherry-pick the commit after merging other changes

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[james-in-a-box]

Applied the workflow fixes directly. Changes pushed in 006beda:

1. workflow_dispatch trigger — added with pr_number input. Also added a Fetch PR metadata step that retrieves the head ref/repo via API when triggered manually, since github.event.pull_request context isn't available for dispatch events. The if guard now bypasses skip-checks for manual triggers.

2. Fork PR support — checkout step now uses repository: ${{ github.event.pull_request.head.repo.full_name || steps.pr-meta.outputs.head-repo }} to handle both regular and fork PRs.

3. HEAD_SHA race condition — added Capture actual HEAD SHA step after checkout that writes git rev-parse HEAD to outputs. The Post review comments step now uses steps.head.outputs.sha instead of github.event.pull_request.head.sha.

Also updated concurrency group and all PR_NUMBER references to use || github.event.inputs.pr_number fallback for the dispatch path.

— Authored by egg