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 hour, finds the ones waiting on a review, and queues them. Someone still clicks to start it.
• The two labels will confuse people if nobody says this out loud. Same rules, same lenses, same log. Only the thing driving it differs, and both are scored together.
• One thing it asks of you, and it is the only one: a change to an interface has to show that its accessibility behaviour was checked. The bar is low and the reason is in the numbers, which the notes on the measurement slide carry.
• 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.
• Not every change gets six. A diff with no product code in it, a spec file on its own, now gets one. Running the full set on it cost about 216k tokens to produce two notes that both lost at the gates.

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 limits, and they are not going to move

• The distinction that matters to anyone worried about this: the left column is behaviour and the right column is a wall. Behaviour can drift, and a wall cannot.
• Say why it is enforced rather than written: a session was once told to commit and push and got as far as a push on a branch here. Nothing bad shipped, and a rule that depends on the model remembering it is not a rule.
• On approve specifically: merging needs human approvals, and an approval from the reviewing account would satisfy the exact gate that exists to require a person.
• It posts without me reading it first, and it can do none of these. Those are the same decision from both ends, and the wall is what makes the posting safe.
• A hold does not clear itself when you push, so it is spent only 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.
• This is also where the accessibility ask came from. Those findings scored the worst ignore ratio of any theme, and the lens was not the weak part: it carries twenty-three mined patterns, each with the pull request that taught it. Finding more defects does not move an ignore rate. The author answering does.

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.

Every comment ends up in one of these, and they do not all count

• The two fractions are the whole instrument, and keeping them apart is the lesson. Fused, an author merging past a correct comment looks identical to the reviewer being wrong.
• The dashed row is the honest part. "You explained and I accept that" is not a win or a loss, and a PR that got abandoned and rebuilt elsewhere left comments on code that never shipped. Counting either would flatter the numbers.
• Every fraction is read per topic, never as one average. A reviewer can be excellent on database changes and noisy on frontend speed, and the mean of those looks fine.
• If asked who decides: nothing here is automatic. A score under half is surfaced with a proposed cause and changed only when I agree.

Eight pieces, and only three of them run on their own

• The split is the governance story in one picture: the cheap watching is automatic, everything that costs money or makes a judgement waits for a person.
• Each piece owns exactly one job and the boundaries are written down, because eight things touching one pull request is how you get two of them replying at once.
• Only the reviewer may invent a finding. The answering one is explicitly forbidden from it: if it spots something new while checking your fix, it says so and stops.
• Two of these can reach the same PR, so they take a lock first. They differ on what to do when it is held, and that difference is deliberate.

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.