The Pull Request Checklist: 15 Things to Verify Before You Approve Code

The pull request lands at 4:40 on a Friday. Four hundred lines, eleven files, tests green, description reading "fix stuff". You have twenty minutes before your next meeting, so the temptation is to scroll to the bottom, approve, and get on with your day. Most reviewers have done that at least once. Most of them regretted it about three weeks later.

Reviewing code is not the same as reading it. A reviewer is hunting for the handful of things that actually break in production, and those things repeat themselves with almost boring regularity: a swallowed exception, a missing authorization check, a test that asserts nothing, a commit that buries a bug fix inside a directory rename.

A checklist is not bureaucracy. It is how you stop your attention from drifting toward whatever is most visible in the diff instead of what matters. Here is a version worth keeping in a second tab.

Readability: does this read like the rest of the codebase?

Consistency beats personal preference almost every time. If the project calls it getUserById and the new function is fetch_user, that is worth a comment, not because either style is wrong but because the next person will now have to remember two conventions.

Structure and size

Look for the happy path. A 60-line function with three nested conditionals usually means the author was thinking about edge cases before finishing the main flow, and small extractions would help. You do not need to demand a refactor, but ask the question: "What does this function do when everything goes right?" If the answer takes more than one sentence, the shape is off.

Check for duplication against code that already exists. Half of all review comments in mature repositories are some variation of "we have a helper for this in utils/". Point at the file rather than describing it from memory.

What the comments say

A comment like // increment the counter is noise and can be deleted. A missing comment is a problem when the code encodes a business rule, a workaround for a third-party bug, or a decision that looks wrong until you know the context. Those deserve a sentence and, ideally, a ticket reference. Future readers will not have the chat thread from six months ago.

Tests: green is not the same as covered

The CI badge proves the tests ran. It does not prove they check anything. Read the new tests and ask one question: if I broke this code on purpose, would the test fail?

Common shapes worth flagging: a test whose name is test_service_2; assertions that only verify a mock was called; sleep(2) used instead of waiting for a condition; a skipped test left in "temporarily"; .only accidentally committed so the rest of the suite silently stopped running.

Push on the edges. Empty list, single element, zero, negative number, null, very long string, unicode, a user who belongs to a different workspace. For bug fixes, ask for a regression test that fails on the old code. If the author cannot reproduce the bug in a test, that is a signal the root cause may still be unclear.

Error handling: follow the failure path

Read the diff as if every external call fails. Network drops, the database rejects the write, the third-party API returns a 500 with an HTML body, the queue is full.

Swallowed errors are the most expensive thing you will find in a review: empty catch blocks, except: pass, a .catch(() => {}) that turns a failure into a silent success. Check that errors are logged once, with enough context to debug them, and that they are not both logged and re-thrown three levels up.

Look at timeouts and retries. A fetch with no timeout can hang a worker indefinitely. A retry loop without backoff can turn one blip into an outage. A retry on a non-idempotent operation, like a payment or a message send, can produce a duplicate charge or a duplicate email. Finally, check what happens on partial failure: if the code writes to two systems in sequence, what state are you left in when the second one fails?

Security: a five-minute paranoid pass

You are not doing a full audit. You are looking for the mistakes that are easy to make and expensive to keep.

• Authorization, not just authentication. A new endpoint that checks the user is logged in but not that the user owns the record is a classic. Ask directly: can Alice read Bob's invoice by changing the id?
• Untrusted input. Validate at the boundary. Queries should be parameterized, output escaped, file uploads restricted by type and size.
• Secrets. No keys in code, fixtures, comments, or test configs. A secret committed once stays in the history forever.
• Logs. Tokens, passwords, full card numbers and personal data should not appear in log lines or error messages shown to users.
• New dependencies. Is it needed? Is it maintained? Is the version pinned?
• Dangerous defaults. Debug mode on, CORS set to a wildcard, permissive deserialization, a development bypass that ships to production.

If the change touches payments, health data or personal information, get a second reviewer from whoever owns that area. Reviewers who see a subsystem every day catch things the rest of us cannot.

Commit hygiene: can this be reverted cleanly?

A pull request should do one thing. When you see a bug fix, a dependency bump and a formatting pass in the same branch, ask for a split. It makes the diff easier to read now and makes the revert survivable later.

Commit messages matter more than people think. fix and wip are actively hostile to whoever runs git blame in a year. Ask for a line about why the change exists. If a rename or a large formatting change is mixed in, ask for it as its own commit so the real diff is visible. Last check: no commented-out code, no debug prints, no stray files or editor configs sneaking into the branch.

The 15-point checklist

Keep this short enough to finish in a couple of minutes on a normal pull request.

1. Names follow the conventions already used in the codebase.
2. Each function does one thing, and the happy path is visible without scrolling.
3. Comments explain why, not what, and reference tickets or upstream bugs where relevant.
4. The diff is scoped to a single change, and the description says what and why.
5. New behavior has tests, and each test would fail if the code were wrong.
6. Edge cases are covered: empty, zero, null, long input, wrong permissions.
7. Bug fixes include a regression test that fails on the previous code.
8. No sleeps, no arbitrary timeouts, no .only, no skipped tests left behind.
9. Every external call has a timeout and a defined failure path.
10. Errors are surfaced, not swallowed, and logged once with useful context.
11. Retries are limited, backed off, and used only on idempotent operations.
12. New endpoints check authorization per object, not just authentication.
13. Untrusted input is validated at the boundary and queries are parameterized.
14. No secrets, tokens or personal data in code, fixtures, logs or error messages.
15. Commits are atomic, messages explain why, and the branch can be reverted on its own.

Making it survive a deadline

Two passes work better than one. First, skim for scope and intent: do you understand what this change is for, and is it the size you can review properly? Then run the checklist against the details. If the pull request is so large that you cannot do both, ask for it to be split. The checklist gets skipped exactly when it is most needed.

Separate blocking comments from preferences. Prefix the small ones with "nit:" so the author knows what actually stops the merge, and approve with nits rather than opening another round. Put the list in a pull request template so authors self-check before requesting a review, and review small changes quickly. A fast, strict reviewer gets smaller pull requests over time; a slow, vague one gets bigger ones.

You are not looking for reasons to reject. You are looking for the two or three things that would otherwise cost someone a late night in six months, and then you are helping to ship.

Photo: Jakub Zerdzicki / Pexels