Reviewing code you didn't write

Most of my last couple of weeks have been spent on code reviews and planning. Everyone can write code fast and cheaply, teammates and agents both, so the wall of PRs mounts quicker than anyone can read it. Reviewing is how I stay a contributor to all that code: reading a change properly, even just its core path, means I know what’s going on in that part of the system and I can still challenge the approach with everything I know about the rest of it. Skip the review and I’m not really involved in that code any more, just adjacent to it.

Reviewing is a separate skill from writing code. The job is to find the issue that changes whether the work should ship, then make its priority obvious.

Reconstruct the change in context #

When you write code, you already know why the abstraction looks that way and which compromises you made. A reviewer turns up with a diff and a description. Before judging the change, you have to rebuild enough of that context to understand what the author intended.

A browser diff is enough for a small, contained fix. For a behavioural change, I check out the branch and read the code where it lives. I follow its callers and look at what else depends on it. Then I read the tests and run them. The diff shows me what changed, and the checkout shows me the system it changed. Reading a change with the rest of the repo around it is what keeps me a contributor to code I didn’t write.

The browser view only shows the changed lines, and a change can be tidy inside the diff while duplicating something the repo already has. The individual lines all read fine, so the problem only shows up when you look at the rest of the system.

I read the ticket or design note as well. A change can be internally consistent and still solve the wrong problem, especially when the PR description only says what changed. If the intent isn’t written down, I ask before filling the gap with my own assumptions.

I also come away from a review understanding more of the system. On the next change, I can talk it through with another engineer or question an agent’s patch without learning the same area again.

If the PR changes something a user can see, I run the feature too. I watch it do what the description claims, then try the awkward path the happy-path test skipped.

Before leaving a comment, I ask whether the choice breaks behaviour or makes the next change harder. If neither is true, it’s probably taste and I leave it alone.

Review the core path first #

The approach comes first. Does the system already solve this problem somewhere else? Does the change fit the way the surrounding code works? By the time a PR exists, the expensive decisions have already been made. A duplicated retry mechanism matters more than the name of the function containing it.

Here’s the shape I mean:

def fetch_price(sku: str) -> Price:
    try:
        return client.get(f"/prices/{sku}")
    except TransportError:
        time.sleep(1)
        return client.get(f"/prices/{sku}")

Every line reads fine and the tests pass. The comment worth leaving isn’t about any of the lines: the shared client already retries with backoff through tenacity, so this stacks retries on top of retries and the whole function should be return client.get(...). Nothing inside the diff points at that, and no linter flags it.

For a larger change, I try to find its spine: the path where the behaviour lives and the state changes. I read that first. An admin panel might make up most of the diff and be easy to click through. The smaller state transition behind it is the part every caller depends on.

The order matters because later comments may disappear once the approach changes. There’s little value polishing tests around a second retry mechanism if the right answer is to delete it and use the client the system already has.

After the approach, I work down through the behaviour:

• Correctness. What happens with an empty input or a repeated call?
• Failure. What happens when a dependency times out or a queue delivers the same message again?
• Security. Trace untrusted input and check the permission against the operation.
• Evidence. Do the tests prove public behaviour rather than mirror private calls? Have I run the changed path myself?

Formatting and lint should already be automated, and I expect static application security testing (SAST) tools such as Semgrep to catch known patterns before a person opens the diff. No scanner tells me whether the change duplicates something or uses the right design. Small naming preferences can be worth mentioning when they make the code harder to follow, but they belong at the bottom of the review.

Tests deserve the same scrutiny as the implementation. A test that mirrors private calls can pass while the user-visible behaviour is wrong, and it breaks during harmless refactors.

Make the priority clear #

A review with fifteen comments gives every issue the same visual weight, even when only one should block the merge. I built lgtmaybe around the same problem after watching an LLM reviewer bury useful findings under confident speculation.

If something should block the merge, say why. If it’s a preference or small improvement, prefix it with nit: so the author knows it can wait. When you’re unsure, ask a question. “What happens if this is called twice?” gives the author room to show context you may have missed.

Not every real issue needs to hold up the PR. I block when the change is wrong or the design will be expensive to unwind. If it can wait, I mark it as non-blocking and say why.

If one issue dominates the review, I put it in the summary. Approach problems rarely belong to one line anyway, so I use line comments to point at the evidence.

Comments should also explain the consequence. “This is not idempotent” names a property. “The queue can deliver this twice, so we may charge the customer twice” explains why the property matters. The second version gives the author something concrete to fix.

Reviewing AI-generated changes #

I give AI-generated code less benefit of the doubt. A model is just as fluent when it has invented an API, so tidy naming and grammatical comments don’t reassure me. I check the finished-looking parts as closely as the rough ones.

When the patch calls an unfamiliar API, I check the library documentation or the installed source. Plausible method names are cheap for a model to produce. The same applies to configuration flags that look right and happen to be ignored by the version in the project.

Generated tests can encode the implementation without proving its behaviour. If the function chooses a value through flawed logic, the test may encode the same choice and pass. I run the feature against the requirement, especially when the test suite looks unusually complete for the size of the change.

AI-generated patches often include caching layers for workloads that don’t need them and interfaces with one implementation. Those shapes are common in mature systems, so they look reasonable in a finished patch. I ask what measured problem the cache fixes or which second implementation needs the interface now. If there’s no answer, I ask for simpler code.

Before reading the patch line by line, I compare the changed files with the task. If it contains an unrelated refactor or is too broad to verify in one pass, I ask the agent to split it by behaviour first.

I put recurring project rules in AGENTS.md. It cuts down repeat mistakes, but I still check whether the agent followed them in the patch.

Getting better at review #

I still read merged pull requests in codebases I know well. I start with the description and diff, then compare the review conversation with the final patch. What changed tells me which comments mattered.

I also ask another engineer to look at my review comments occasionally. They can tell me which issue I missed and which comment only expressed my taste. That feedback is more useful than counting how many findings I left.

If one comment changes the approach, I put it in the summary and stop reviewing the code that will be deleted.