How I Review PRs I Don’t Own

2026/03/04

BUILDcidevex

Most people review PRs by opening the diff and reading top to bottom. I used to do this too. It’s like reading a novel starting from chapter 7 — you can follow the words, but you have no idea what’s going on.

I developed a different method. Six steps, story-first, code-second. It sounds like more work. It’s actually less, because you stop re-reading the same diff three times trying to figure out why a file changed.

The problem with reading diffs cold

A diff tells you what changed. It doesn’t tell you why. And without the why, you’re pattern-matching blindly — looking for things that look wrong without knowing what right looks like.

This is especially brutal when the PR is in a part of the codebase you don’t own. You don’t have the mental model. You don’t know the invariants. You’re just staring at green and red lines hoping something jumps out.

For me there’s an extra layer. English is my second language, I have ADHD, and reading dense technical text under time pressure is the worst possible combination of those things. I needed a system that reduced the cognitive load — not by skipping work, but by doing it in the right order.

The method

Six steps. I’ll walk through each one using a real PR I reviewed recently — #23087, which replaced a grep pipeline in CI with Python AST parsing for finding GPU test files.

Step 1: Fetch everything first

Don’t start reading yet. Pull the full PR metadata — title, description, author, changed files, additions/deletions count. Get the complete diff loaded.

This sounds trivial, but the point is to resist the urge to start reading the moment you see code. You’re building the map before hiking the trail.

Step 2: Tell the story

This is the most important step.

Before reading any code, answer these questions:

For #23087, the story was: “The CI used grep pipelines to find pytest-marked test files. Grep can’t tell real code from string literals and can’t handle conftest.py marker inheritance. Replace with Python AST parsing.”

That took maybe two minutes to construct from the PR description. But now when I read the code, I already know what I’m looking for. I’m not pattern-matching — I’m verifying a narrative. Huge difference.

This is the step that changes everything. Every line of code you read after this has context. You know the shape of the solution before you see it.

Step 3: File map with reading order

Don’t just look at the file list. Create a reading plan. Group files by purpose, explain why each one changed, and figure out which to read first.

Separate “files that matter” from “files you can skip.” Version bumps, auto-generated stuff, test fixture boilerplate — acknowledge these exist, then ignore them.

For #23087, the reading order was:

  1. START HEREfind_marked_tests.py (the core logic, AST parsing, 235 lines)
  2. INTEGRATIONrun_gpu_tests.yml (where the old grep was, where the new script gets called)
  3. SKIMtest_find_marked_tests.py (344 lines of tests, check for coverage gaps)

This ordering matters. If you read the YAML first, you’d see a script invocation with no context. If you read the tests first, you’d see assertions about behavior you don’t understand yet. Core logic first, always.

Step 4: Before/after flow diagrams

Draw what the system looked like before and after. For CI/pipeline PRs this is crucial — YAML is basically impossible to hold in your head.

ASCII diagrams work fine:

BEFORE:
  git ls-files
    → grep "pytestmark="
    → grep "pytest.mark.X"
    → sort → jq
    → gpu_tests.json

AFTER:
  find_marked_tests.py $MARKER --all --prefix
    → walk repo for test files
    → ast.parse() each file
    → check conftest.py inheritance
    → gpu_tests.json

Now the diff isn’t abstract. You can see the shape of the change: a shell pipeline became a Python script. Same input (repo), same output (JSON file), different middle.

More importantly, you can see what’s structurally different. The old pipeline used git ls-files — which respects .gitignore. The new one walks the filesystem — which doesn’t. That’s not a detail you’d notice from reading the diff. It falls out of the diagram.

Step 5: Review notes in three tiers

Not all review comments are equal. Tier them:

The tier system prevents you from being That Reviewer who leaves 47 nit comments on a well-written PR. It forces a decision: “Is this worth the author’s time to read and respond to?”

If you can’t tier it, you probably don’t understand it well enough to comment on it.

Step 6: Confidence check

Be honest about what you understand and what’s fuzzy. Rate yourself HIGH, MEDIUM, or LOW for different areas of the PR. List terms or patterns you’re unfamiliar with.

This step exists for one specific reason: to prevent the thing where you leave a review comment that reveals you didn’t understand the PR. We’ve all seen those comments. We’ve all been afraid of writing them.

If your confidence is HIGH, comment with conviction. If it’s LOW, ask questions instead of making assertions. The confidence check gives you permission to do either.

What this actually caught

The PR #23087 review surfaced two real issues. Neither jumped out from reading the diff. Both emerged from the story and the flow diagram.

Decorator-based markers get silently missed. The new AST parser only detects pytestmark = [...] assignments at module level. It won’t catch @pytest.mark.falcon_gpu_pr on individual test functions. The old grep did catch those. So there’s a class of tests that will silently disappear from the GPU test list after this PR. Future footgun.

This came from the story step — knowing that the PR replaces grep means you need behavioral equivalence. If the new thing misses cases the old thing caught, that’s a regression.

Filesystem walk vs git ls-files. The old pipeline started with git ls-files, which only returns tracked files. The new script walks the filesystem with os.walk, which will pick up test files in .venv/, build/, or any other gitignored directory. Probably a small risk in CI (clean checkout), but a real one for local runs.

This came from the flow diagram. Drawing the before/after made the structural difference obvious.

Why this works (the real reason)

Three things.

It separates understanding from evaluation. Reading a diff cold forces you to do two things simultaneously — figure out what the code is doing AND decide if it’s good. That’s a lot of cognitive load. The story step separates these. You understand first. You evaluate second. One thing at a time.

It prevents “I read every line but understood nothing” syndrome. ADHD makes it easy to hyperfocus on diff details — indentation, variable names, individual lines — without building the big picture. The structured steps force big-picture-first, details-second. By the time you’re reading code, you already know the plot.

It builds confidence for commenting. As a non-native English speaker, there’s always a moment of “what if I misunderstood?” before posting a review comment. The confidence check makes you explicitly assess your understanding. It converts a vague anxiety into a concrete question: do I know enough to say this? Usually the answer is yes. And when it’s no, you ask a question instead of guessing — which is the correct move anyway.

The universal version

The method works for anyone, not just ESL engineers with ADHD. Here’s why.

Most people review PRs backwards. They start with the diff and try to reverse-engineer the intent. This is like watching a movie starting from the climax and trying to figure out the first act from context clues.

Flip it. Start with the intent, then verify the diff delivers it.

Build the story. Draw the flow. Then read the code.

You’ll find better bugs, leave better comments, and spend less time re-reading the same diff wondering what it means.