What To Look For, In What Order

Reviewing your own diff for the things that matter, and the check that silently does nothing.

3 min read

Not yet reviewed

You will review far more of your own code than anybody else's. Doing it well is a reading order rather than a talent: follow the untrusted value, then check the decisions, then check the checks.

One — follow the value

Find everything in the diff that came from outside — a request body, a query string, a header, a filename, a webhook, a file somebody else wrote — and follow each one to where it stops being a string. That path is where almost every serious bug lives.

1Does it reach a query?        -> parameterised?
2Does it reach a document?     -> encoded for that context?
3Does it reach the filesystem? -> is the path yours?
4Does it reach a command?      -> is there a shell involved at all?
5Does it decide something?     -> is that decision the server's?

Line 5The one people miss. A value that never touches a database or a page can still be the thing your code branched on — and if the branch was "is this user an admin", the value was the whole security model.

Two — check the decisions

For every new endpoint or public function, ask who is allowed to call it and where that is decided. If the answer is "the interface does not show the button", it is not decided.

Three — check the checks

This is the step almost nobody does, and it is the one this platform has most reason to insist on. A check that silently does nothing looks exactly like a check that passes.

Looked fine

A coverage threshold configured but never run
A lint rule whose regex was broken by escaping
An end-to-end test that skipped itself on a 404
An assertion that a page contains no "/" — on a page of ASCII art slashes

Actually true

Every one of those was found by deliberately breaking
something and watching whether the gate complained.

All four are real, from this codebase. Three of them sat green for weeks. None was noticed by reading the code, because reading a broken check and reading a working one feel identical.

Take care

When you add a gate, prove it fails. Break the thing it protects, watch it go red, then put the thing back. It costs two minutes and it is the only evidence that the check exists.

Wrenlearner

That feels excessive for a lint rule.

Rookmentor

It felt excessive here too, until a rule that had "protected" a convention for a month turned out to have a regex that matched nothing. The cost of the habit is two minutes per gate. The cost of not having it is that you do not know which of your gates are real, and you cannot tell by looking.

Four — read the diff as somebody else

Last, read it as though you are being paid to find one thing wrong. Not to confirm it works — you already know it works, that is why you are pushing it. The useful question is narrower: what is the worst input this could receive, and what happens then?

You add a lint rule that forbids a dangerous pattern and it reports no violations. What should you do next?

Tip

Keep the order. Following the value first means the other three steps happen on the code that matters, rather than on the whole diff — which is how a review stays short enough that you actually do it.