LLM-assisted code review in pull requests: the rules we gave it, and where we want it to stay quiet

We wired an LLM-based reviewer into the pull request flow a few months ago, with one narrow goal: stop repetitive comments -an unused import, a fix applied in one file but not mirrored in another file of the same PR, new business logic with no test covering it- from eating up a human reviewer's time. The point was never to replace the human reviewer. It was to filter before the PR reaches that person, so what's left to discuss is exactly what a model is bad at judging: whether a given decision makes sense inside the whole system.

That goal forced us to draw an explicit, written line between what the automated reviewer is allowed to flag and what it isn't. We didn't arrive at that line up front; the first version had no restrictions, and we had to walk part of it back within days.

What we trust it with: style, repeated patterns, test coverage

The reviewer runs as a step in the CI pipeline, over the pull request's diff, and posts its comments as ordinary review comments on GitHub - not as an approval or a merge block.

# .github/workflows/llm-review.yml
name: llm-review
on:
  pull_request:
    types: [opened, synchronize]

jobs:
  review:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0
      - name: Run review
        run: |
          diff=$(git diff origin/${{ github.base_ref }}...HEAD)
          ./scripts/llm-review.sh --diff "$diff" --rules rules/review-scope.yml
      - name: Post comments
        run: ./scripts/post-review-comments.sh

The file rules/review-scope.yml does more to define the reviewer's actual behavior than the prompt itself. It lists, by category, what kind of finding it's allowed to raise:

allowed_categories:
  - style             # naming, unused imports, dead code, local duplication
  - repeated_pattern   # a fix applied in one file but not in a similar one in the same diff
  - missing_tests      # new business logic with no test covering it in the same PR
  - null_safety        # unchecked access to values already typed as nullable

forbidden_categories:
  - architecture             # framework choice, library choice, service boundaries
  - schema_design             # table shape, migrations, normalization
  - dependency_choice         # adding or swapping a third-party dependency
  - naming_of_domain_concepts # names of domain entities already established in the code

repeated_pattern turned out to be the most valuable category in practice, because it's exactly the kind of thing a tired human reviewer skips over: the model compares the different files touched in the same diff and flags when a fix - say, adding validation for a required field - landed in one controller but not in another that follows the same structure within the same PR. That's a local, diff-verifiable finding that needs no context about why the system is shaped the way it is.

missing_tests runs on a simple rule: if the diff adds or changes a non-trivial method (more than one line of logic, with some conditional branch) and no test file touched in the same PR references that method, it gets flagged as a comment, not a block. Whether that specific method actually needs a test is still a call the human reviewer makes; the bot just keeps the case from slipping through unnoticed.

Where we decided it shouldn't have a say

The forbidden_categories list isn't a list of topics where the model tends to be wrong more often - in several cases, its architecture suggestions were reasonable when read in isolation. It's a list of topics where the reasoning behind the decision doesn't live in the diff, or anywhere in the repository's code, but in a conversation that happened somewhere else: why that particular boundary between services was chosen, what alternative got ruled out and why, what roadmap constraint made a simpler solution unworkable at the time.

An LLM reviewing a diff has no access to any of that. It sees the resulting code, not the process that produced that shape. When it comments on an architecture decision, it's evaluating that decision against a generic notion of best practice, not against the project's actual constraints, and the comment reads with the same confidence whether it's right or wrong. That apparent confidence is the real problem: a poorly grounded style comment gets dismissed quickly, but a poorly grounded architecture comment, delivered in a confident tone, pushes the reviewer to second-guess a decision that was already made with information the bot never had.

What didn't work: letting it comment on everything, even as comment-only

The first version of rules/review-scope.yml had no forbidden_categories. Our reasoning was that since the bot only posted comments and never blocked the merge, there was no real risk in letting it flag anything: if it opined on architecture and got it wrong, the human reviewer would just ignore it.

That's not how it played out. The bot started routinely commenting on decisions that had already been settled - suggesting extracting a service that had deliberately been kept together with another for a concrete operational reason, or proposing swapping a library without knowing the original choice came from a licensing constraint. No individual comment blocked anything, but each of these cases triggered the same sequence: someone on the team had to write a reply explaining context to a comment no human had written, and on PRs opened by people less familiar with the project's history, those comments generated genuine doubt about whether the original decision still held, even when it did.

The cost wasn't the time the bot spent being wrong. It was the human time spent defusing, PR after PR, the implicit authority that any confidently-worded comment carries, whether or not it comes from someone with real context. We fixed this by adding the explicit forbidden-categories list, and the comment volume dropped noticeably - but what remained was, overwhelmingly, actionable without any extra context.

The rule we ended up with

The question we now use to decide whether something belongs in allowed_categories isn't whether the model tends to get that kind of finding right. It's whether the reasoning for accepting or rejecting the finding is fully contained in the diff and the repository's code, or whether it depends on a conversation, a business constraint, or a prior decision written down nowhere the model can read. The first case is safe ground for an automated reviewer. The second is, and will keep being, ground that belongs to the person who holds context the diff can't contain.