An agent that reviews pull requests

What you end up with

4 artefacts, and only one of them has any substance:

• .github/review-brief.md — our standard review brief, which communicates what qualifies as a finding and what never will (for example, it's not a finding if there's no concrete input for which the change breaks sth)

• .github/findings-schema.json — the schema that describes the shape of the response we expect

• scripts/pr-review.py — the script which calculates the diff, starts the run of the agent using your chosen harness, and then ignores most of its output to post only a single comment with the rest of it

• .github/workflows/pr-review.yml — the workflow file, in which we define a proper trigger (PR created/synchronised/reopened) and set up the right permissions (the GITHUB_TOKEN with contents:read and pull-requests:write permissions)

It takes 35 minutes, and if you built the issue triager in the previous lesson, you can just base your workflow file on that one so only the brief and the filter are new. The majority of time goes into the "don't" part, not the "do" part — the models are already good at spotting things in a diff, what's much harder is picking only 3 out of 30 of them worth your time. That way you decide if this tool will still be useful here in a month

The bot people stop reading

A lot of people ask us why other companies drop their review bots. Well, we think it's mostly because how the engineering landscape has changed recently — writing the first draft of a piece of code is really cheap nowadays, understanding what a change actually does and what it'll cost you in the future is not. That's where the review comes into play, and that's what a review bot tries to address.

Here's an example of what we mean by this; say you create a PR with a small change in it. Then you run a generic "Review this PR" prompt on it — in theory you should get a list of findings, but in practice you get a long wall of text which is mostly the code itself, with a few ideas like:

• This function is missing a docstring

• You could extract a helper function for this piece of code

• Consider renaming this variable

• Can you write some tests for it?

And then at the very end there are two actual findings:

• The type of this variable is not correct for this input

• This line does not handle this edge case

It's not a big deal, there's no chaos, you just need to scroll a little, and after a while you stop scrolling and collapse it. And that's it — among the rest of this wall of text, those two actual findings go down too, the bot gets disabled, and the conclusion is that "AI review doesn't work". But it does work, we just need to build 3 things rather than prompt for them:

• Defining what the agent can see

• Defining what kind of information the agent can return

• Defining how much of this information will reach you

Feed it the diff, not the repo

Let's start with defining what the agent can see. It needs to be the diff, not the entire repository. There are two potential pitfalls here.

The first one is a diff against main with two dots, which compares the branch with the current state of main, including the changes other people have made since you've created the branch. So the bot comments on code you haven't even touched.

git diff main HEAD

To address this, you need to use merge-base instead.

git diff --merge-base main HEAD

Another thing is that if you include everything in the diff, you will burn a lot of tokens to review a bunch of lockfiles, which will scatter your focus and won't return anything valuable anyway. That's why you need to exclude some files from the diff using pathspecs.

git diff --merge-base main HEAD -- \
  ':(exclude,glob)**/*.lock' \
  ':(exclude,glob)**/package-lock.json' \
  ':(exclude,glob)**/vendor/**' \
  ':(exclude,glob)**/*.generated.*'

What's more, when using pathspecs, you need to remember about one thing — if you use asterisks inside an exclude pathspec, the keyword glob is required (which we've used here), and it's really easy to forget about it. Without glob, that double asterisk only matches when there's a real directory in front of the filename — so a lockfile sitting in the root of your repository quietly slips through, which is the one case you cared about

With these exclusions, we reduced the diff in our test repository from 8 files to one.

The other thing is setting up an upper limit. After a certain point, every change is so big that it doesn't make sense to review it — if sb creates a PR with 3 thousand lines of changes, the best move is to tell them to split it, not to point out typos in it. That's why you need to make sure your script stops and informs you if a diff is too big.

But there's one exception to this. The agent needs to have access to the repository during the review — it might be that a particular hunk looks good on its own, but you can't see if sb returns sth from it; in such case, the bot needs to be able to open the file and check it. So make sure your script includes both the diff and the ability to open files (read-only)

Three of the five harnesses we've used have a documented flag for this. The other two don't — we've described which is which, and what to do instead, in the variant below.

Make it return data, not an essay

OK, so now we know what the agent can see. The next thing is defining what it can return.

What we mean here is that it should return structured data, not a wall of text. If you ask an LLM for a list of findings, it'll give you a list of findings. But if you ask it for a list of findings in JSON format, it'll give you a list of findings in JSON format — and this way we can further process it

The schema we've created defines the structure that each finding needs to have — it's very simple, and has the following fields:

• file — the path to the file

• line — the line number

• severity — one of blocking, important, or nit

• category — a free-form string, like security, bug, etc.

• claim — the actual finding

• breaks_when — what input this change breaks on

{
  "type": "object",
  "properties": {
    "findings": {
      "type": "array",
      "items": {
        "type": "object",
        "properties": {
          "file": {"type": "string"},
          "line": {"type": "integer"},
          "severity": {"type": "string", "enum": ["blocking", "important", "nit"]},
          "category": {"type": "string"},
          "claim": {"type": "string"},
          "breaks_when": {"type": "string"}
        },
        "required": ["file", "line", "severity", "category", "claim", "breaks_when"],
        "additionalProperties": false
      }
    }
  },
  "required": ["findings"],
  "additionalProperties": false
}

This last field is the most important one — it's about naming the input, the order of arguments, or the state that leads to a bug. That way we can exclude things like "It'd be good to add a test for it", because there's no input we can use here; it's also better than saying "Be concise" — you can even go as far as making sure every finding has this field populated with a sensible value, and if it doesn't or is vague, drop it during the run of the script

This is what we call the brief:

• It's a relatively short document, with a big part of it being a list of things you don't want to see

• It basically says "Think as if you were the on-call engineer that got paged about this change", and then goes on to say that unless there's a concrete input for which the change breaks sth, it's not a finding; and even then, it's never good to communicate about:

  • Style

  • Naming

  • Formatting

  • The absence of comments

  • Any ideas around extracting smth into separate functions

  • General complaints about test coverage

  • Anything that a linter can find

It also says that if the bot finds sth, it needs to communicate it in a specific structure — JSON, as defined by the schema. And lastly, it explicitly says that an empty array is a valid response.

Review the diff below as the engineer who will be paged when it breaks.

Report only what you can state as a fact about this change: a specific input,
sequence or state that makes it behave wrong. If you cannot name the input that
breaks it, it is not a finding.

Do not report: style, naming, formatting, missing comments, "consider
extracting", test coverage in the abstract, or anything a linter already catches.

Severity is one of:
  blocking  - data loss, a security hole, or a broken contract someone depends on
  important - wrong under an input that will actually occur
  nit       - real but survivable

Answer with nothing but the JSON object described by the schema.
An empty findings array is a valid, expected answer.

The "it's valid" part here is really important. If you don't include it, the only success criterion you've defined for the model is finding sth

The severity floor and the comment budget

OK, so now we know what the agent can see and what it can return. The last thing is defining how much of its output will reach you.

There are two things here:

• Minimum severity — don't include anything below "important" in the output

• Maximum number of comments — five, with the most important ones at the beginning

These are two constants that we set in our script, not in the prompt. If the model gives you 30 findings, it'll return 3 out of them, and drop the remaining 27 on the Python level — which means you can inspect them, rather than having your attention span wasted on them; the script will also tell you how many it's dropped, so you'll have data to adjust the numbers later

We're aware that this way we lose real findings. But five actual findings with a silent 6th one is better than 30 findings that include five of them, because in the latter case people stop paying attention and then it doesn't surface anything at all — you don't want a tool that doesn't work well, you want a tool that works

Also, we know you might be tempted to say "tell me about maximum 5", but if you do this, you'll make the model responsible for ranking these findings, which will be invisible, not logged, and not easily adjustable without editing the prompt. It's better to have it be a simple line in our script that always does the same thing.

One more thing about severity — it's very important to remember that it's not an attribute of a piece of code, it's the model's judgment. Once it's an enum in a schema it stops looking like one. When we ran the same diff through two harnesses while writing this, one of them graded a falsy-default bug "important" and the other graded the exact same line a "nit". Neither of them is wrong

So set your minimum severity against your own harness and your own history, not by reading what somebody else set theirs to

Post it once, not once per push

The last thing is making sure we post a single updating comment instead of one per push. If we were to append new lines to an existing comment every time sb pushes, we'd end up with a log of the bot's history — instead, we post a single comment using gh pr comment with the edit-last and create-if-none commands:

• edit-last targets the bot's previous comment

• create-if-none is there for the first run

gh pr comment "$PR" --edit-last --create-if-none --body-file -

Together, they form an idempotent operation that costs less than any custom API solution.

Alternatively, you can use line-anchored inline comments, which are more readable but require more maintenance — for this, you'll need to use the pull request reviews REST endpoint with a "comments" array containing things like:

• path

• line

• side

• body

But there are two potential issues with the line-anchored approach:

• The first one is that every line anchor is an assertion — it says "there's code on this line", so once the code moves, you own the re-anchoring and the de-duplication that edit-last was doing for you

• The second thing is that if you set the event to REQUEST_CHANGES instead of COMMENT, the reviews endpoint will accept it — but you don't want to set up a bot that blocks merges

So if you ask us, start with the single updating comment and switch to inline once you're confident in what your agent returns

The fork problem

The last thing is forked PRs. This is where a reasonable-looking workflow becomes a security issue, so be careful here.

If you create a workflow that runs on forked PRs, GitHub will only give your GITHUB_TOKEN read-only permissions, and it won't share any of your secrets with the runner — so no comment posting, and no model API key. In theory, this means the entire workflow is doomed to fail if you run it on forked PRs.

But there's a potential solution. If you change the workflow's event to pull_request_target, it will run in the base repository's default branch context, so you'll get back your GITHUB_TOKEN and secrets — which means that you can post comments and use your model API key. But there's a catch — the code under review is on the fork, so if you check it out, you run some stranger's code with the GITHUB_TOKEN and your model API key; GitHub's own warning about this is not subtle: "Running untrusted code on the pull_request_target trigger may lead to security vulnerabilities. These vulnerabilities include cache poisoning and granting unintended access to write privileges or secrets."

There are two ways you can address this.

The first one is not reviewing PRs from forks — just add an "if" statement that compares the full name of the head repository with your own, and if they don't match, exit early. This is perfect if every single PR comes from a branch in your own repository, but make sure to check this, rather than assume it

if: github.event.pull_request.head.repo.full_name == github.repository

Another solution is a 2-workflow split — this is what GitHub Security Lab recommends:

• Create a pull_request workflow that uses the read-only GITHUB_TOKEN and doesn't have access to any of your secrets. This one will run on forks, and will create an artifact with the bot's output

• Then, create another workflow that runs on workflow_run events. In this one you can use the regular GITHUB_TOKEN and have access to your secrets — it'll download the artifact from the first workflow, and post a comment with its contents

This way, you never check out any code from forks in an environment where your secrets are present.

One last thing worth mentioning is what the GitHub Security Lab says about this: "Artifacts resulting from untrusted PR data are themselves untrusted and should be treated as such when handled in privileged contexts." This is especially important to keep in mind if you decide to go with the 2-workflow split — even though a diff is text rather than a binary, you don't want to do anything else with it than giving it to the model. Make sure your privileged workflow only reads the artifact and passes it on

This solution is more complex than the "if" statement, but it's worth it if you actually accept external contributions

Two lines that stop it costing money

OK, so these are two ways of dealing with forked PRs — let's have a look at the last cost controls.

The first one is setting up a concurrency group based on the PR number, with cancel-in-progress enabled. If sb creates a PR and pushes 5 times in 4 minutes to fix a typo, they'll trigger this workflow 5 times. With a concurrency group in place, the queue will already take care of this — but if you enable cancel-in-progress, it'll also terminate any running jobs, which are the ones using model tokens

concurrency:
  group: pr-review-${{ github.event.pull_request.number }}
  cancel-in-progress: true

Otherwise, you'll spend your tokens to review a target that keeps changing, four times in a row. We'd say this is even more impactful than picking a particular model

The second thing is setting a job timeout to 10 minutes, and making sure your harness has its own budget or timeout flag. This varies between tools, so it's in the variant below

timeout-minutes: 10

The workflow

And that's it — here's what the resulting workflow looks like:

• It runs on PR created/synchronised/reopened events

• If the head repository is not the one you own, it does nothing

• It uses an Ubuntu runner with a 10-minute timeout

• It uses actions/checkout to check out the code with its entire history (so we can use merge-base)

• It installs your chosen agent's harness

• Lastly, it runs our script with the following environment variables:

  • BASE_REF — the name of the base branch

  • PR — the number of the PR

  • GH_TOKEN — the token with contents:read and pull-requests:write permissions

  • AGENT_CMD — the command that starts the agent's harness

name: pr-review

on:
  pull_request:
    types: [opened, synchronize, reopened]

permissions:
  contents: read
  pull-requests: write

concurrency:
  group: pr-review-${{ github.event.pull_request.number }}
  cancel-in-progress: true

jobs:
  review:
    # Fork PRs get a read-only token and no secrets. Skip them until you build the split.
    if: github.event.pull_request.head.repo.full_name == github.repository
    runs-on: ubuntu-latest
    timeout-minutes: 10
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0   # --merge-base needs the history, not just the tip

      # Install your harness here -- see the variant below.

      - run: python3 scripts/pr-review.py
        env:
          BASE_REF: origin/${{ github.event.pull_request.base.ref }}
          PR: ${{ github.event.pull_request.number }}
          GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
          AGENT_CMD: ${{ vars.AGENT_CMD }}

The fetch-depth 0 is important here — by default, actions/checkout fetches a single commit, and with 0 it fetches the entire history. Leave it at the default and merge-base has nothing to compare against — it fails with fatal: no merge base found, which reads like a git problem rather than the config mistake it is

The AGENT_CMD is a repository variable so we can easily change the agent in the future without modifying the script (or revert the change if we want to try another one)

Read it yourself for two weeks

This is the most important part of the whole build. Because at the end of the day, you don't know if it works until you see for yourself. And the only way to see for yourself is to read every single comment the bot posts for two weeks, and count how many times you acted on them

If most of these comments are things you'd usually ignore, it means the problem is with the brief, not with the model — add these things to your never-report list, or update the "breaks_when" section of the brief. Almost every bad review bot we've seen would become a good one if its brief were a paragraph longer

You can always adjust the budget and the severity threshold — but only if you're sure it's right and if you regularly hit this limit. You can promote one very specific thing to "blocking", for example hardcoded credentials or migrations missing rollbacks, but first check if you can't create a script that does it instead — it'll be faster and more cost-effective. And if after a month nobody reacts to it, disagrees with it, or fixes their code based on it, remove it altogether, because it's become scenery, and scenery in CI costs money

That's the evergreen version of the tool — a few comments per week, usually worth reading, none of them blocking. That's what we set out to achieve