The Shape of a Pull Request You Can Trust

· 6 min read
development best-practices github testing
The Shape of a Pull Request You Can Trust

I merge a lot of pull requests now, more than I can read line by line. That volume forced me to work out what actually makes a description trustworthy, as opposed to merely thorough, and the answer turned out to be a specific skeleton with six parts.

This applies whether a person or an agent wrote the diff. The reviewer’s problem is identical: decide whether to believe a change without redoing the work.

1. The problem, with a reproduction

Start with what was wrong, stated concretely enough that I could observe it myself. Not “fixed a bug in the cache,” but:

Attaching a cache backend changed results, not just speed. @multicache built its key from kwargs only, so any argument passed positionally was invisible to the key and collapsed to "None". Two calls with different branches returned the first branch’s data.

The test I apply: could a reader reproduce this in five minutes from the description alone? If not, the author may have been guessing, and I have no way to tell.

This section is also where I catch the failure I fear most, which is a plausible fix to a misdiagnosed problem. Those pass every check. The only defense is a clearly stated diagnosis that I can sanity-check against the symptom independently of the fix.

2. Real numbers

Anything measurable gets a before and after. Not “improved performance,” but the actual values:

A 2500 versus 1500 matchup returned 0.504; it now returns 0.979.

Rate limiting blocked 0 of 200 rotating-header requests; it now blocks 45.

1,267 of 1,616 nodes were degree-1 leaves; now 8% of that node type is.

Numbers do two jobs. They prove the author ran the thing, and they give me a magnitude to judge whether the change was worth its risk. A description with no numbers anywhere is usually a description of intent.

3. The change, at file level

What changed and where, in prose, organized by file or by concern. Enough that I can predict the diff before I open it, and notice if the diff contains something the description didn’t mention.

The strongest version of this section names what was deliberately not touched:

The beat/tied update equations are intentionally left alone; this PR only corrects expected_score.

An explicit scope boundary is how I know the author thought about the blast radius rather than stopping when the tests went green.

4. Verification, with commands and output

The commands, and what they printed. Not “tests pass,” but:

make test    - 235 passed, 6 skipped
make lint    - passed
make typecheck - passed (24 source files)

And when something did not pass, say so plainly:

make quality-check stops on a pre-existing unsorted import in a smoke-test file this PR does not touch. The CI lint scope above passes.

That admission raises my trust rather than lowering it. A description where absolutely everything is green is either a small change or an incomplete report, and I’d rather know which.

If a check could not be run at all, name that too, along with why:

Not executed here (no deno available): the type check and the launchd reload path were verified by review against existing code.

5. The mutation check

This is the section most descriptions lack and the one that changes how I read everything else.

A regression test written alongside a fix has never been asked the question it exists to answer. So revert the fix, run the new test, and record that it fails:

Mutation check: restored the previous adjusted-odds formula and ran the numeric regressions; four parameter cases failed as expected, then the corrected formula was restored and the full suite passed.

Mutation check: changing the winner mean multiplier back from sigma^2/c to sigma^2/c^2 failed test_beat_with_known_values (25.333621 versus 29.395741).

With that paragraph present, I can skip reading the test carefully, because the author has demonstrated it binds to the change. Without it, I have to read the test and judge whether it would catch a regression, which is most of the work of reviewing.

For a change with two independently load-bearing halves, I want each half mutated separately. Reverting only one and staying green is the tell for an untested branch hiding as belt-and-braces redundancy.

6. What was not done

The last section, and the one that most reliably predicts a good author:

Not done: Framework, Dependency, and Datastore still triple-book the same label (27 such cases). That needs a decision about which kind wins, so it’s left for a follow-up; the new merge: verb makes it fixable by hand meanwhile.

Three things happen here. The author demonstrates they saw the larger problem and chose a boundary. The reviewer stops wondering whether an obvious omission was an oversight. And the follow-up work gets recorded in the one place someone will actually look for it later.

A close cousin is the “things worth a look before merge” section, which flags the parts where the author made a judgment call they’re unsure about. That’s where I spend my review attention, because the author has already told me where the risk is.

Why this is a spec, not a wish

The reason I’m confident this skeleton is teachable is that I put it in a prompt and got it back consistently, across hundreds of pull requests, in seventy-odd repositories.

Which means it’s a spec you can hand to anything. Put it in CONTRIBUTING.md, put it in a pull request template, put it in your agent instructions. The six headings are the whole artifact:

## Problem
## Numbers
## What changed
## Verification
## Mutation check
## Not done

How I actually review with it

Since the volume forced me to triage, here’s the honest order:

  • Read the problem statement first, and check the diagnosis against the symptom. If those two do not line up, nothing else matters.
  • Triage by blast radius, not by diff size. Anything touching math, money, authorization, or a data migration gets read line by line no matter how small. A 400-line CI configuration change gets skimmed.
  • Use the mutation check to decide how hard to read the tests. Present and specific: skim. Absent: read the tests as carefully as the code.
  • Read the “not done” section as the real scope statement. It tells me what the next pull request should be, and often whether this one stopped in a coherent place.

The pleasant discovery in all of this is that the skeleton makes human pull requests better too. Every section is a question a reviewer was going to ask anyway. Answering them in the description costs the author ten minutes and saves the reviewer an hour of inference, and inference is exactly where review goes wrong.