Skip to main content

Code Review Best Practices: How to Catch the Bugs Outside the Diff

git diff gives you three lines of context. Your function might have forty callers.

Updated 10 min readMarkdown
Code Review Best Practices: How to Catch the Bugs Outside the Diff

By default, git diff shows three lines of context around every change. That's the window most code reviews happen through.

Three lines is plenty to catch a typo, an off-by-one, or a confusing name. It's almost never enough to catch the bug that actually reaches production, because that bug usually lives in a file the pull request didn't touch.

This post covers why that happens, the five places those bugs hide, and a short review pass (with the Git commands for it) that finds most of them.

A two-line change that breaks three files

Here's the whole pull request:

 def get_user(user_id):
     row = db.find(user_id)
     if row is None:
-        raise NotFound(user_id)
+        return None
     return User(row)

The intent is reasonable: a missing user is a normal outcome for a lookup, not an exceptional one. The function's own tests get updated. Two reviewers approve.

Meanwhile, in files nobody opened:

# workers/digest.py
user = get_user(event.user_id)
send_digest(user.email)  # AttributeError when user is None

# web/password_reset.py
try:
    user = get_user(form.user_id)
except NotFound:
    return render("no_account_found.html")  # never runs anymore

Every changed line is correct. The function's contract changed, and the callers that depended on the old contract weren't part of the diff. The failures only show up on the rare path (a deleted or missing user), which is exactly the path local testing and demos skip.

A two-line diff marked approved, with arrows to five places the reviewer never saw: callers, deleted lines, existing data, sibling copies, silent tests

What code review actually catches (the data)

Two Microsoft studies line up on this:

  • Bacchelli & Bird (ICSE 2013) surveyed 873 developers and testers and classified 570 review comments. Finding defects was the top motivation for review for 44% of developers, but only 14% of comments were about defects. Code-improvement comments were the most common. Interviewees named understanding the change as the main challenge, and 91% said unfamiliar files take longer to review. (source)
  • Czerwonka et al. (2015), "Code Reviews Do Not Find Bugs" reported that about 15% of comments indicate a possible defect. Reviewers seeing code for the first time had about a third of their comments rated useful by the author, rising to about two-thirds by their third review of the same code. Usefulness also dropped noticeably once changes reached 20 or more files. (source)

The reading that fits both: review quality tracks how much of the surrounding code the reviewer already knows. The diff format doesn't supply that surrounding code, so most reviewers review what's on screen.

The five places bugs hide outside the diff

Edge What goes wrong How to check
Callers Return value, exceptions, latency, units, or ordering changed; callers still assume the old behavior git grep every changed function; read each call site's assumptions
Deleted lines A "redundant" check, retry, or branch was protecting a path the author never runs git log -S on the removed line to find why it was added
Existing data New code assumes a shape that rows written under old rules don't have One query against a replica or staging snapshot
Sibling copies The fix lands in one of several copies of the same logic Search for the distinctive constant, regex, or message, not the function name
Silent tests Behavior changed, but no existing test failed, so nothing asserted the old behavior Ask for one test that fails on the old code

The fifth row deserves emphasis. Green CI after a refactor is reassurance. Green CI after a behavior change is a list of things nothing tests.

Why "keep PRs small" doesn't cover this

Comparison: big diff with small reach (renames, reformatting, uncalled endpoints) versus tiny diff with wide reach (raise becomes return None, changed defaults, deleted null checks, renamed enums, removed retries)

Small PRs are easier to read, and the data supports that. But size and risk are separate axes:

  • A 40-file rename is large and usually low-risk.
  • A one-line change to a default value is tiny and can affect every request.

Small diffs carry an extra trap: nothing in them looks dangerous, so reviewers relax at the moment the reach is widest. Sort review effort by reach, not line count.

The Outside Pass: a 10-minute code review checklist for contract changes

The Outside Pass: five numbered questions to ask before approving a pull request

Read the diff once for intent. Then, before any comments on naming or style:

  1. Who calls this? Find every caller of each function whose behavior changed.
  2. What did the red lines do? Treat each deletion as a removed feature. Name it and who relied on it.
  3. What data already exists? Check whether production already holds rows that break the new assumption.
  4. Where else does this logic live? Search for the pattern, not the file.
  5. Which test should have failed? If none did, request one that fails on the old code.

The commands that do most of the work:

# Every caller of a changed function
git grep -n "get_user("

# Show the whole enclosing function as context, not 3 lines
git diff -W main...HEAD        # -W = --function-context

# Find the commit that introduced (or removed) a specific line
git log -S "raise NotFound" --oneline

# Then read why it was added
git show <commit>

For the data question, a single count is often enough:

-- Does production already contain rows the new code assumes can't exist?
SELECT COUNT(*) FROM users WHERE email IS NULL;

When to run it

Run the full pass when the change touches a contract:

  • return values, raised exceptions, or default values
  • deleted conditionals, validations, guards, or retries
  • validation rules or schema assumptions
  • shared utilities with many callers
  • renamed constants, enum values, or config keys

Skip it for formatting, test-only changes, and new code with no callers yet.

Move half the work to the author

The author already knows most of the answers. A short section in the PR description saves every reviewer from rediscovering them. For the get_user change, done properly:

## Outside the diff
- Callers: 4 found with `git grep`. 3 updated in this PR;
  admin/report.py already handles None.
- Removed: the `NotFound` raise. Introduced for the password-reset
  flow (see `git log -S`); reset now checks for None.
- Data: no schema or data assumptions changed.
- Copies: same lookup in workers/sync.py, updated here.
- Test that fails on the old code: test_get_user_missing_returns_none

Add it to your PR template and it becomes a habit. A wrong entry in that list is much easier to spot than a missing caller nobody thought to search for.

Reviewers can match it with scoped approvals: "Approved: logic and tests in the diff, plus callers in api/. Did not check workers/." An approval that states its coverage is a record, not a blanket guarantee.

Any reviewer that receives only the patch, human or tool, has the same blind spot. It's the same reason AI-generated code that looks right slips through: correctness inside the window says nothing about what's outside it.

Key Takeaways

  • A diff answers "is this change correct?" Production bugs ask "is everything that depended on the old behavior still correct?"
  • In Microsoft's research, only about 14–15% of review comments concern defects, and reviewers unfamiliar with the code are far less effective.
  • Outside-the-diff bugs cluster in five edges: callers, deleted lines, existing data, sibling copies, and silent tests.
  • Diff size measures typing; reach measures risk. Tiny contract changes deserve the most scrutiny.
  • Run the Outside Pass on contract changes, put an "Outside the diff" section in PR descriptions, and scope your approvals.

FAQ

Why does code review miss bugs?

Because review is done through the diff, and the diff shows changed lines plus a few lines of context. Many production bugs come from the change interacting with code that wasn't changed, such as callers, old data, or duplicated logic, so they're invisible in the review view unless the reviewer goes looking.

What should I look for in a code review besides the changed lines?

Five things: callers of any function whose behavior changed, the purpose of every deleted line, data that already exists in production, other copies of the same logic, and whether any existing test should have failed but didn't.

Are small pull requests safer?

They're easier to review, but not automatically safer. Risk depends on reach, meaning how much code and data depends on what changed. A one-line change to a default value or an exception can have wider reach than a 40-file rename.

How do I find all callers of a function before approving a PR?

Use your IDE's "find references," or run git grep -n "function_name(" across the repository. For dynamic languages, also search for the name as a string, since it may be called through reflection, routing tables, or task queues.

What does it mean if CI passes after a behavior change?

If behavior changed and no existing test failed, no existing test was asserting the old behavior. Ask for at least one test that fails against the old code so the new behavior is protected going forward.

What should go in a pull request description to help reviewers?

Beyond what changed and why, add an "Outside the diff" section: callers you checked, the purpose of deleted lines, any data assumptions, other copies of the logic you updated, and the test that fails on the old code.

The bottom line

The lines in a pull request are usually fine; someone was staring right at them when they were written. The expensive bugs live in what those lines touch: the caller three folders away, the guard someone deleted as noise, the rows written under last year's rules.

Before your first comment about naming, ask the question the diff can't answer: who was relying on the way this used to work?

New here? Start with Start Here: A Plain-English Map.

Adam Jaber is a software engineer who writes Simply Explained: complex topics, made simple. No jargon, no hype. Get one of these in your inbox each week: subscribe on Substack.

About the author

  • I'm a software engineer who spends most days building systems that solve real problems. When I'm not shipping code, I'm either untangling a tricky problem or writing about what I learned doing it. Currently exploring AI on the side.

No comments yet. Be the first to comment.