Skip to content

Conversation

jerop
Copy link
Contributor

@jerop jerop commented Aug 20, 2025

This commit shortens and simplifies the prompt for PR reviews. The previous prompt was long and complex, making it difficult to adapt to meet own needs. The new prompt is more concise and focused on the most important aspects of the review.

The new prompt is divided into the following sections:

  • Setup: Instructions for the model to get the necessary information.
  • Review Focus: The key areas to focus on during the review.
  • Constraints: Rules that the model must follow.
  • Review Process: The steps to take to complete the review.
  • Comments Format: The format for the review comments.

This change should improve the quality of PR reviews and make them more consistent.

This change also makes this workflow consistent with other workflows which are all concise.

Fixes #208

@jerop jerop requested review from a team as code owners August 20, 2025 20:09
This commit shortens and simplifies the prompt for PR reviews. The previous prompt was long and complex, making it difficult to adapt to meet own needs. The new prompt is more concise and focused on the most important aspects of the review.

The new prompt is divided into the following sections:

- Setup: Instructions for the model to get the necessary information.
- Review Focus: The key areas to focus on during the review.
- Constraints: Rules that the model must follow.
- Review Process: The steps to take to complete the review.
- Comments Format: The format for the review comments.

This change should improve the quality of PR reviews and make them more consistent.

Fixes #208
@jerop jerop force-pushed the workflows/pr-review branch from 4f8821b to 51b378e Compare August 20, 2025 20:12
@jerop jerop changed the title feat: shorten and simplify the PR review prompt feat: simplify the PR review prompt Aug 20, 2025
@jerop
Copy link
Contributor Author

jerop commented Aug 20, 2025

@jerop jerop mentioned this pull request Aug 20, 2025
@dan-ri
Copy link

dan-ri commented Aug 20, 2025

This is an improvement. The most serious problems that it doesn't address are:

  • Precise determination of line number and side information
  • Problems with inconsistent tool calling
  • The model sometimes marking a PR as approved or blocked

Solving these problems requires adding an additional step that preprocesses diffs and provides them to the model in a format that makes it trivial for it to select the line number and side information and consistently provide the correct tool inputs.

I would also recommend moving the prompt to a separate file for ease of editing.

@jerop
Copy link
Contributor Author

jerop commented Aug 20, 2025

@dan-ri thank you for the input! do you have an example of how you did this in a public repo?

I would also recommend moving the prompt to a separate file for ease of editing.

agreed, this is an issue we're tracking in #76 and I have a POC in #89 -- hoping we land on a solution soon

@dan-ri
Copy link

dan-ri commented Aug 20, 2025

I don't, but the idea is simply to add a couple of steps to the workflow like:

- name: Generate diff index
  env:
    GITHUB_TOKEN: ${{ steps.generate_token.outputs.token || secrets.GITHUB_TOKEN }}
    PR_URL: ${{ steps.get_pr_context.outputs.pr_url }}
  run: |
    python .github/scripts/generate_diff_index.py --pr "${PR_URL}" --output diff_index.json

- name: Load prompt
  run: |
    {
      echo "GEMINI_PROMPT<<EOF"
      cat .github/data/gemini-pr-review-prompt.md
      echo "EOF"
    } >> "$GITHUB_ENV"

Here get_pr_context is a unified step that collects all of the PR data (replacing the multiple conditional steps in the original workflow) and generate_diff_index.py is a simple Python script that retrieves the PR diffs with gh and outputs a JSON file with a simple structure that includes side, line number and change type data for every line.

This needs to be paired with prompt changes that direct the model to exclusively use the diff index for line number and side information, and prescribe its exact inputs to the GitHub MCP tools.

@jerop
Copy link
Contributor Author

jerop commented Aug 20, 2025

@dan-ri so helpful, let me experiment with this 🙏🏾

@dan-ri
Copy link

dan-ri commented Aug 20, 2025

A bit more detail on the kind of index format that works well:

{
  "version": 1,
  "pr": "https://github.com/owner/repo/pull/123",
  "files": [
    {
      "path": "relative/file/path.py",
      "additions": 10,
      "deletions": 2,
      "hunks": [
        {
          "old_start": 38, "old_len": 7,
          "new_start": 38, "new_len": 9,
          "changes": [
            {"t": "+", "side": "RIGHT", "line": 41, "preview": "    def foo(...", "hash": "a1b2c3d4"},
            {"t": "-", "side": "LEFT",  "line": 42, "preview": "old line text",  "hash": "deadbeef"}
          ]
        }
      ]
    }
  ]
}

Notes:

  • Only changed lines (+ or -) are included inside "changes" to keep size small.
  • Each change includes a short preview (trimmed) and an 8-hex hash prefix (sha1(content)) for stable reference if needed.
  • The model is instructed to use ONLY these entries for inline comments; context lines can be inspected via gh / cat if necessary.

@vivekkairi
Copy link
Contributor

One more suggestion -

gh cli has a bug where it doesn't return changed files list always. Issue being tracked here.

Can we use git to fill the CHANGED_FILES key?

@dan-ri
Copy link

dan-ri commented Aug 21, 2025

Can we use git to fill the CHANGED_FILES key?

CHANGED_FILES is redundant if we're using the index.

@jerop
Copy link
Contributor Author

jerop commented Aug 21, 2025

changes by @sethvargo in #212 simplified the prompt, will close this for now, and separately look into handling diffs -- the change also switched to using an mcp tool for fetching diff, wonder if that changes the diff handling? will test with latest state and explore solutions in this thread -- @dan-ri @vivekkairi let's continue the discussions in #151

@jerop jerop closed this Aug 21, 2025
@jerop jerop deleted the workflows/pr-review branch August 25, 2025 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Simplify PR Review Prompt
3 participants