Monday, 10:37. Andrei opens a pull request: a payments refactor, 847 lines, three days. At 11:42 Dima stamps LGTM. Thirty seconds. At 12:15 I stamp Approved after skimming files and green tests. Two minutes. At 14:00 it lands on main. At 14:47 production stops taking money.
The system used to retry the provider three times. After the refactor it cancelled the transaction at once. Four hours, about 40% of payments missed, $127,000 of lost revenue, six hours of hotfix, 230 support tickets.
Two experienced people missed the hole because review was theatre. A checkbox before merge. That was four years ago. I no longer treat speed of first approve as a win.
How to see the process is already a play
A review under a minute on two hundred lines, no comments, or comments about spaces. A war over camelCase, architecture silent. A pull request sits three days, the author moved to another task, context died. Lines like "this is terrible" and "why would you". Nobody knows how many bugs review catches or how many pull requests are older than a day.
A person thinks: I must pass review to land. They should think: I must hear it while it is still cheap. No criteria, each person looks at their own thing. The dashboard holds velocity and closed tickets. Review lives nowhere. The author fears critique and cuts experiments. The reviewer fears offence and stamps LGTM.
What to look at, and in what order
The machine first. Tests, linter, format, secret and dependency scan, coverage not down without a note. If a human on review is arguing about indent or running tests by hand, the process is broken. A red CI does not start a review.
Then behaviour, 5-10 minutes. Is the ticket closed? What happens on null, an empty list, zero, a huge number? Where does it die without a handler? A VIP discount with no check on the user or the amount is Friday production.
function calculateDiscount(user, total) {
if (!user || total <= 0) throw new Error("Invalid input");
return total * (user.isVIP ? 0.2 : 0.1);
}Then how it sits in the system. Is there a second email validator in the next method. Is a dependency upside down. Did a new god object just get born. N+1 lives here too: a thousand users and an orders query in a loop is a thousand and one trips to the database.
# This kills the report
for user in User.objects.all():
orders = Order.objects.filter(user_id=user.id).count()
# This walks once
User.objects.annotate(orders_count=Count("orders"))Then security, short. A query string with email inside, a key in the repo, a check for "logged in" instead of "allowed", raw input on a public API.
# email = "' OR '1'='1" → the whole table
query = f"SELECT * FROM users WHERE email = '{email}'"
query = "SELECT * FROM users WHERE email = ?"
db.execute(query, [email])Then reading it in six months. function p(u, a) with magic 1 and 0.15 should not live in production. A name, a constant, a short function. A comment where the move is not obvious, not where the code already speaks.
Then tests. Happy path, edge, error. One assert on a VIP and a hundred dollars does not hold the hole.
Before the request the author reads the diff as a stranger, writes why this move, links the ticket, marks breaking changes. The reviewer template has the same shelves: behaviour, edge, fit, speed, a security hole, reading, tests. A question at the bottom if they are unsure themselves.
## What this PR changes
## Ticket
## Type: bug / feature / breaking / refactor
## Author: I read it, tests ran, no secrets
## Reviewer: behaviour, edge, fit, speed, hole, reading
## Where I am unsure myselfThere is no person in the comment. "You wrote terrible code" kills the next experiment. "This retry with no pause will flatten the API, here is exponential backoff, what do you think?" leaves work.
Prefixes save blood.
[CRITICAL] SQL via f-string on line 47. Parameters, not glue.
[MAJOR] N+1 on 10k rows. Need select_related.
[MINOR] data is too generic a name.
[NITPICK] I prefer a guard clause, taste.
[QUESTION] Why setTimeout, not a promise?
[PRAISE] Memoisation belongs here, I would have missed it.
Critical blocks the merge. A nit does not. Praise is work too. Otherwise the feed is only holes.
You do not have to answer feedback in the same minute with a shield. If the critique is right, fix it. If it is arguable, write why you left it. If you did not get it, ask. "Rewrite" with no example can be sent back with a request to show the move.
Format and lint live in pre-commit and CI, not in a senior's head. The toolkit is secondary. What matters is that red never reaches a human.
What to count so theatre is visible
Time to first look: for a pull request under 200 lines I want hours, not days. For 847 lines an approve in 30 seconds is forbidden by the physics of reading. After the payments case we set a floor: more than 200 lines cannot be approved sooner than 30 minutes.
Share of the diff with a living comment, not "LGTM". Bugs caught here versus bugs on production. How many pull requests are older than a day. Size: a stack of 800 lines is almost always read on a diagonal. Better to cut.
A team of eight before that case: first look in 15 minutes (felt like a win), comments on 12% of the diff, 8 production bugs a month. After the checklist, the time floor, a target above 80% comment coverage and a rule of two reviewers with at least three notes per hundred lines, a quarter later: first look 4 hours, coverage 78%, production bugs 2, review catches 12 a month. Slower to merge. Cheaper than four hours of a dead till.
A second case the same year. Senior Pavel wrote to Andrei: terrible code, rewrite, how did you think this would work. Andrei shrank to minimal diffs, left in two months. A talk with Pavel, prefixes, a ban on grading the person. Two months later a new junior called Pavel the best mentor. Tone moves slower than an action in CI. Without tone the checklist becomes a new play.
A third: a pull request five days with no look. The author is already in another task and comes back to their own code as a stranger. Rule: if a day passes with no owner, ping the channel, put a slot on the calendar like a meeting. Review is work. You plan it. You do not snatch it from the air at six on Friday.
A rollout for me is not a quarter of courses. A week: agree that LGTM without reading is dead, hang the template and the prefixes. A week: lint, tests and secrets in CI, red is not reviewed. Two weeks: a time floor on big diffs, a rotation so the same two people are not ticking each other. Then watch production bugs and pull-request age every two weeks and cut whatever became theatre again.
A third case that same year I already named: five days with no look. Here is the mechanic we used after. If a day passes with no owner, a bot in the channel writes the on-call name from the rotation. A 45-minute slot sits on the calendar like a meeting. A pull request over 400 lines we ask to split, otherwise people read it on a diagonal, like those 847. The author may ping after a day without feeling they are begging.
The person on review is not an examiner and not a notary. They are the second pair of eyes that can catch a cancelled payment before 230 clients do. If after their look everyone knows what to do with the code, the slot paid. If after their look there is only a thumb, the slot already sent a bill. We paid that bill once.
