Code Review Checklist: What to Check Before You Approve

A code review checklist is the short list of questions a reviewer answers before approving a change. Check seven things, in this order: the intent (you know what the change is for and it does only that), design (it belongs where it was put), correctness (it works, including the edges), tests (they prove the change and would fail without it), security (who can reach it and what it trusts), readability (the next person can follow it), and rollout (it can ship and be undone). Anything a tool already checks, such as formatting, lint and a passing build, stays with the tool. Your time goes on what a machine cannot judge.

This list is for one change at a time. Checking a whole release before it ships is a different job, covered in the QA checklist for web apps. If you are choosing an AI reviewer to run before the human one, AI code review tools compared covers that; this piece is about what the person who approves should look at.

Before you read the diff

Most review time is wasted by starting at line one of the first file. Spend two minutes on these first, and a surprising number of reviews end here with a question rather than forty comments.

• Find the task. Every change should point to the work item it is for. If you cannot say in a sentence what the change is supposed to do, ask before reading the code.
• Check the size. A change that mixes a refactor, a feature and a dependency bump is three reviews. Asking for it to be split is a legitimate review comment, not an obstruction.
• Check that CI has run on the latest commit. Reviewing code that does not build wastes everyone’s time.
• Read the description. The author should have said what changed, why, how it was tested and what is risky. What reviewers need in a pull request template lists the sections.

Design and architecture

Google’s published guide to what to look for in a code review↗ (opens in a new tab) puts design first, asking whether the pieces of the change make sense together and whether it “belongs in your codebase, or in a library”. An architectural code review checklist for a small team does not need to be longer than this:

• Is the change in the right layer? Business rules in a UI component, or SQL in a request handler, cost more later than they save now.
• Does it reuse what exists? A second date formatter or a second HTTP client is a common sign the author did not know about the first.
• Is it only what was needed? Code written for a future nobody has asked for is extra surface to test and maintain.
• Does it change a contract, such as an API response, a database schema or a public function signature? If so, who calls it, and are they updated or still compatible?
• Is there a decision behind any surprising choice? If the answer lives only in the author’s head, ask for it in writing, for example as an architecture decision record.

Correctness

Read the code as the input would travel through it. The bugs that reach production are rarely in the main path the author tested; they are in the branch nobody took.

• Edges: empty lists, zero, negative numbers, null or missing fields, very long strings, time zones and daylight saving changes, the first and last item.
• Errors: what happens when the network call fails, the file is missing or the record was deleted a second ago. Is the error handled, logged, or silently swallowed?
• Concurrency: two users saving at once, a double click, a retried request. Is anything written twice or lost?
• Callers: if a function’s behavior changed, search for every place it is used, not just the ones in the diff.
• Data changes: migrations run in the right order, handle existing rows, and can be rolled back or have a written plan if they cannot.

Tests

A green check tells you the tests passed. It does not tell you they test anything. Read the test changes before the code changes; they tell you what the author thinks the code does.

• Would these tests fail if the change were reverted? A test that passes either way proves nothing.
• Is there a test for the edge you worried about above? If not, ask for one rather than trusting a comment that says it is handled.
• Were any existing tests deleted, skipped or loosened? Each one needs a reason in the description.
• For a bug fix: is there a test that reproduces the bug? That is what stops it coming back, and it belongs on the regression testing checklist for the next release.

Security

You do not need to be a security specialist to catch the common problems. The OWASP Top 10:2025↗ (opens in a new tab) ranks Broken Access Control first, then Security Misconfiguration and Software Supply Chain Failures, with Injection at fifth. Those translate into review questions:

• Access: does every new route, query or action check who is asking, and that they may see or change this record?
• Input: is anything from the user put into a query, a shell command, HTML or a file path without being parameterized or escaped?
• Secrets: no keys, tokens or passwords in code, config, tests or logs.
• Dependencies: is each new package real, intended, maintained and pinned in the lockfile?
• Data: does the change log or return personal data it does not need?

For anything touching sign-in, payments or permissions, go deeper. The OWASP Code Review Guide↗ (opens in a new tab) is free and written for exactly that review.

Readability and maintainability

• Names say what a thing is or does. If you had to read the body to understand the name, suggest a better one.
• Comments explain why, not what. A comment restating the next line adds nothing; a comment explaining a workaround saves the next person an afternoon.
• Complexity: could you explain this function to a new teammate quickly? If not, ask whether it can be simpler.
• Style: if it is not in the linter or the team’s written norms, it is a preference. Mark it as optional or leave it out.

Rollout and operations

• Can it be turned off or rolled back without a second deploy? If not, is that acceptable for this change?
• Will you know if it breaks? Errors are logged with enough context to act on, and anything new that can fail has a way to be noticed.
• Docs, config and release notes are updated if the change affects how people build, run or use the software.

The checklist to copy

BEFORE READING
[ ] I know which task this is for; the change does that and no more
[ ] Small enough to review in one sitting, or split requested
[ ] CI passed on the latest commit
[ ] Description says what, why, how tested, and what is risky

DESIGN
[ ] In the right layer; reuses what exists
[ ] No speculative code for needs nobody has
[ ] Contract changes (API, schema, signatures) have callers updated

CORRECTNESS
[ ] Edges: empty, zero, null, long, time zones, first/last
[ ] Failures handled and logged, not swallowed
[ ] Double submit / concurrent writes considered
[ ] Every caller of a changed function checked
[ ] Migrations handle existing data; rollback written down

TESTS
[ ] Tests would fail if the change were reverted
[ ] The risky edge has a test
[ ] No test deleted, skipped or loosened without a reason
[ ] Bug fixes include a test that reproduces the bug

SECURITY
[ ] Access checked on every new route, query and action
[ ] User input parameterized or escaped
[ ] No secrets in code, config, tests or logs
[ ] New dependencies real, intended, pinned

READABILITY
[ ] Names clear; comments say why
[ ] Simple enough to explain quickly

ROLLOUT
[ ] Can be turned off or rolled back
[ ] Failures will be visible in logs or alerts
[ ] Docs and release notes updated if needed

When an AI assistant wrote the change

The same checklist applies, with more weight on three lines. GitHub’s own guidance on reviewing AI-generated code↗ (opens in a new tab) tells reviewers to look for “hallucinated APIs, ignored constraints, or incorrect logic” and to “watch for tests that are deleted or skipped, instead of fixed.” So check that every function and package the code calls actually exists, read the test diff before anything else, and compare the diff against the task, because an assistant asked to fix one thing will sometimes tidy three others. The full routine, with the failure patterns and a longer list, is in how to review AI-generated code.

What to do with what you find

Findings that block the merge go in the review as comments and get fixed there. The ones that get lost are the real problems outside the change: a bug you noticed in a neighboring function, a missing index, a test that has been flaky for weeks. Do not hold the pull request hostage for them, and do not trust yourself to remember them. File each one.

On a fenbs board each becomes a bug in To Do with a BUG- ref, the file and line in its note, and a priority from 1 to 10, where 1 is the most urgent. An AI assistant connected over MCP can file them for you with fenbs_create_item; if a matching open task already exists, fenbs answers with the likely match instead of filing a duplicate, and the assistant comments on that one. When the fix lands, the task’s test status and test notes record how it was checked, so the next reviewer can see whether “fixed” was ever verified. fenbs does not review code or connect to your repository; it keeps the list of what the review found once the pull request has closed.