Code review that catches bugs: data flow, edge cases, errors, concurrency, security

We once went through six months of production incidents and looked at the pull requests that introduced each bug. Every one had been reviewed and approved. The review threads were not empty either: they averaged about seven comments, mostly about naming, formatting and import order. None of the comments touched the line that later broke. The reviewers were capable people looking at the wrong things.

This post is about reviewing for correctness. It is aimed at engineers who review regularly and want their time to catch the bugs that matter. You will get a reading order, five lenses with concrete questions, an annotated example, and the team habits (small PRs, automation, checklists) that make careful review possible.

Let machines own style

Formatting, import order, unused variables and naming conventions are real concerns, but a human should not spend attention on them. Every minute discussing a blank line is a minute not spent on the error path. Our rule: if a style issue can be expressed as a rule, it goes in a formatter, linter or CI check, and humans stop commenting on it.

# .github/workflows/checks.yml (excerpt)
jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - run: npm ci
      - run: npx prettier --check .
      - run: npx eslint . --max-warnings=0
      - run: npx tsc --noEmit

Static analysis tools such as CodeQL or Semgrep go further and catch some injection and data-flow issues automatically. They do not replace review, but they move the baseline up.

Read in the right order

Reading a diff top to bottom, file by file in alphabetical order, is how reviewers end up with opinions about a helper and none about the logic. We read in this order:

1. The description and linked issue. What should this change do? If you cannot answer that, ask before reading code.
2. The tests. They show what the author thinks the behaviour is and, by absence, what they did not consider.
3. The entry point. The route, job, handler or public function where data enters.
4. The data path. Follow values from the entry point to storage or output, opening files as the path leads you there.
5. Everything else. Helpers, renames and moves last.

Lens 1: follow the data

For each input, ask where it comes from, what shape it can have, and where it ends up.

• Can this value be null, empty, negative, huge or a different type than the code assumes?
• Is the unit consistent: cents or dollars, seconds or milliseconds, UTC or local time?
• Is anything computed twice in different ways that must agree (a total in the UI and in the API)?
• Is any data written that is never read, or read before it is written?

Lens 2: edge cases and boundaries

Bugs cluster at boundaries. We probe every loop, comparison and range with the same short list: zero, one, many; first and last; exactly at the limit, one above, one below; empty string versus missing; time zone and DST transitions; leap days; Unicode and right-to-left text; the largest value the type can hold.

Lens 3: error paths

Happy paths get tested by the author. Error paths usually do not.

• What happens if this call throws, times out or returns a partial result?
• Is an error swallowed (catch {} or except: pass) or logged and then treated as success?
• If the operation fails halfway, what state is left behind? Is there a transaction or compensation?
• Are retries safe, meaning is the operation idempotent?

Lens 4: concurrency and ordering

Any code that reads, decides and then writes is a candidate for a race if two requests, workers or replicas can run it at once.

• Is there a check-then-act sequence without a lock, unique constraint or atomic update?
• Can two jobs process the same record? Can messages arrive out of order or twice?
• Does a cache get invalidated before or after the write, and what does a reader see in between?

Lens 5: trust boundaries and security

Mark where untrusted data crosses into trusted code: request parameters, headers, uploaded files, webhook payloads, data from another tenant.

• Is authorisation checked on the object, not just authentication on the route?
• Is input passed to SQL, a shell, a template, a file path or a URL fetch without the right escaping or allowlist?
• Are secrets or personal data written to logs, errors or analytics?
• Does the change widen who can call something (a new public route, a relaxed CORS rule)?

An annotated example

Here is a trimmed version of a real pull request, a "redeem gift card" endpoint, with the comments that mattered:

// POST /api/gift-cards/redeem
export async function redeem(req: Request, res: Response) {
  const { code } = req.body;
  const card = await db.giftCard.findUnique({ where: { code } });

  if (!card || card.redeemedAt) {
    return res.status(400).json({ error: 'Invalid code' });
  }

  await db.wallet.update({
    where: { userId: req.user.id },
    data: { balanceCents: { increment: card.amountCents } },
  });
  await db.giftCard.update({ where: { code }, data: { redeemedAt: new Date() } });

  return res.json({ ok: true });
}

Review comments we left, one per lens:

• Data flow: code comes straight from the body. Codes are stored uppercase; a user typing lowercase gets "Invalid code". Normalise before lookup.
• Edge case: what about expired cards? The model has expiresAt, but it is never checked.
• Error path: if the second update fails, the wallet is credited and the card stays redeemable. Both writes need one transaction.
• Concurrency: two simultaneous requests can both read redeemedAt = null and both credit the wallet. Make the redemption a conditional update and check that it affected one row.
• Security: no rate limit on a guessable code space. Add per-user and per-IP limits.

The revised version:

export async function redeem(req: Request, res: Response) {
  const code = String(req.body.code ?? '').trim().toUpperCase();

  const result = await db.$transaction(async (tx) => {
    const claimed = await tx.giftCard.updateMany({
      where: { code, redeemedAt: null, expiresAt: { gt: new Date() } },
      data: { redeemedAt: new Date(), redeemedBy: req.user.id },
    });
    if (claimed.count !== 1) return null;

    const card = await tx.giftCard.findUniqueOrThrow({ where: { code } });
    await tx.wallet.update({
      where: { userId: req.user.id },
      data: { balanceCents: { increment: card.amountCents } },
    });
    return card.amountCents;
  });

  if (result === null) return res.status(400).json({ error: 'Invalid or used code' });
  return res.json({ creditedCents: result });
}

The conditional updateMany is the atomic claim: only one request can flip redeemedAt from null. None of these issues would have been found by a style review, and the race is the kind that passes every test the author writes.

Keep pull requests small

Review quality falls sharply with diff size. A 1,500-line PR gets skimmed and approved; a 150-line PR gets read. Our working limits:

Techniques for splitting: land refactors separately from behaviour changes; put new code behind a feature flag and merge it in slices; stack PRs so each builds on the previous one.

Make the author's job part of review

Reviewers catch more when authors give them context. Our PR template asks for:

## What and why
## How to test (commands, test accounts, screenshots for UI)
## Risky parts (where should the reviewer look hardest?)
## Rollout and rollback (flags, migrations, backfills)

"Risky parts" is the most valuable field. Authors usually know where the tricky bit is; asking them to say so directs review attention there.

A short checklist we actually use

Long checklists get ignored. Ours fits on one screen:

• I can explain what this change does and why.
• I traced at least one input from entry to storage or output.
• I checked empty, boundary and error cases, and the tests cover the important ones.
• Writes that must happen together are in one transaction; retries are safe.
• Shared state has no unprotected check-then-act.
• Authorisation is checked on the object; untrusted input is escaped or validated.
• Nothing sensitive is logged.

Common mistakes

Key takeaways

• Automate style so human review can focus on correctness.
• Read the description and tests first, then follow the data from the entry point.
• Apply five lenses: data flow, edge cases, error paths, concurrency, security.
• Keep pull requests small and ask authors to name the risky parts.
• A one-screen checklist used every time beats a long one used never.

References

• Google Engineering Practices: What to look for in a code review — https://google.github.io/eng-practices/review/reviewer/looking-for.html
• Google Engineering Practices: Small CLs — https://google.github.io/eng-practices/review/developer/small-cls.html
• GitHub Docs: About code owners — https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners
• GitHub Docs: About code scanning with CodeQL — https://docs.github.com/en/code-security/code-scanning/introduction-to-code-scanning/about-code-scanning-with-codeql
• Semgrep documentation — https://semgrep.dev/docs/