Something new on your pull requests

Every Time It Was Wrong

What the reviewer does to your PR, and how we know whether it is any good.

Tjakoen Stolk

• Shape it in one sentence: fifteen minutes, three questions. What is it, can you trust it, what do you do when it is wrong.
• Interrupt whenever. This is a working session, not a presentation.
• The title is about the log and it lands at the end. Do not explain it now.

When your PR is sitting waiting for review

• Land the three facts and stop. No defence yet and no mechanism.
• Answer the obvious question before it is asked: not every PR, and not instantly. A check runs every half hour, finds the ones waiting on a review, and queues them. Someone still clicks to start it.
• If someone asks why it cannot approve, say "two slides from now" and move on.

First, the boring question

• Say the number flatly. It is not a boast, it is a way of clearing accuracy off the table so the rest of the session is about the real problem.
• Flag it as a September snapshot and say it moves.
• Do not explain the grading yet. That is the last third.

Because we have all been sent the accurate one

• Show of hands on the review-bomb. Everyone has one.
• The point in a sentence: the hard problem was never finding things. It is everything it finds and should not say.

So most of the work is throwing things away

• Point at the taper, not the boxes. The design is subtractive and that is the whole trick.
• The cap is hard: six comments, three on a small change. Over the cap the weakest one is deleted rather than all of them being shortened.
• The six, only if asked: security, tests, migrations, access, design, frontend.

Three ways a comment dies before you ever see it

• The third one is a good rule for people too. "Delete this and every caller reads the wrong customer's data" is a comment. "This looks wrong" is not.
• The bottom line is the design in one sentence. A wrong comment sticks around and costs the next real one its audience. A missed one costs one PR.
• If asked what else it checks: it proves a convention by counting across the whole service rather than asserting it, and it will not accept the file it is reviewing as evidence about itself. That is its own session.

Then whatever survived gets attacked on purpose

• Hiding the original argument is the part worth explaining. Show a checker the case that was made and it tends to agree with it.
• Default-to-no is a deliberate bias. It loses real findings, and that is the trade, because the other mistake is louder and lasts longer.
• One exception if asked: a fact a script already proved skips this. Sending a grep result to something that says no when unsure just deletes your most reliable finding.

It is not fire and forget

• Row one matters most. An open thread ending in your question makes a correct finding worse than no finding, because now the PR is blocked on it.
• Row two: never on the strength of a "done" reply. It checks the actual code at the current commit.
• Row three is how most people will use it, and reading a silent close as "it was wrong" would have taught it five wrong lessons. Every silent close we checked turned out to be a real fix.

The one limit that is not going to move

• This is a control, not a preference. No accuracy number would change it.
• It posts without me reading it first, and it cannot approve. Those are the same decision from both ends, and the second is what makes the first safe.
• Worth flagging: a hold does not clear itself when you push, so it is only used where we mean to stop the merge until a person acts.

And when something goes wrong on its side

• Every run is on a clock, and anything still going when its phase is up gets stopped rather than waited on forever. That is where this case comes from.
• The line to land: partial beats nothing, and partial that reads as complete is worse than both. The cost is paid by whoever trusts it more than it deserves.
• It is enforced rather than remembered. A check refuses a review claiming full coverage while carrying something nobody verified.

Now the part we did not expect

• Top half first: both groups were right. This is not an accuracy story, which is why slide three cleared accuracy off the table.
• Most of what it said was landing in the summary, which is the part nobody reads. So the repair was placement, not a stricter reviewer.
• The trap worth naming: we had "was it right" and "did anyone act" as one number, so every time you ignored a correct comment the dashboard told us to make it stricter. It was learning to say less for being read less.

We only know that because of this loop

• This is the slide the title is about. Every comment it has ever left is written down and graded afterwards.
• The bar is the point. Making itself stricter, quieter or narrower always stops and asks. Making itself noisier does not, because that failure is one you would all see and this one is invisible.
• Per topic, never one average: it can be excellent on database changes and noisy on frontend performance, and the mean of those looks fine.
• Not every comment resolves cleanly. A PR that gets abandoned and reimplemented somewhere else leaves comments on code that never shipped, and those are marked as such rather than counted either way. Guessing would poison the numbers in the one direction nobody would notice.

Everything you have just seen, joined up

• Do not narrate the boxes. Say the three rows, then point at two things.
• First: only the top row is on a clock, and it is the only part talking to your pull request while you are waiting. Everything below it happens after you have moved on.
• Second: the dashed line back is the whole argument. Without it the top row is just a reviewer with opinions and no way of finding out whether any of them were good.
• The filled box is the only place it is allowed to change itself, and a person is standing in front of it.

Nobody starts any of this by hand

• The filled rows are the ones you will feel: you type, and it is already reacting. The outlined rows are the safety net running on a timer.
• Row four is the important one and it is why the timers still exist. Closing a thread in silence moves nothing, so nothing can tell us, and that is how most of you will agree with a comment.
• Row three is deliberately not a question. A push means code no reviewer has seen, which is the normal life of a PR, so asking permission every time would break the loop at its most common event.
• Row one is the answer to "will this happen to everything". It starts from the ticket being ready for review, not from the branch existing.

And none of it was designed up front

• Offer this as the real takeaway for anyone building something similar. None of it could have been designed up front.
• Row two is the one with teeth and anyone can make it: a metric that fuses "was I right" with "did anyone listen" will always tell you to shut up.
• Open the floor here rather than at the end.

That is the whole thing

It will be wrong sometimes. Tell it, and the next one gets better.

Reply the way you would to a person. That reply is what gets graded, and it is the only thing that says which reviewer to fix.

The judgment is human. The typing, by design, is not.

• End on the ask rather than the architecture: disagree with it out loud, in the thread, because that is the input.
• A written version with the full mechanism is coming. Offer it rather than putting a URL on the wall.
• Take questions.