Reviewing code you didn't write · coles.codes

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&rsquo;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&rsquo;m not really involved in that code any more, just adjacent to it.<br>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.<br>Reconstruct the change in context #<br>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.<br>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&rsquo;t write.<br>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.<br>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&rsquo;t written down, I ask before filling the gap with my own assumptions.<br>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&rsquo;s patch without learning the same area again.<br>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.<br>Before leaving a comment, I ask whether the choice breaks behaviour or makes the next change harder. If neither is true, it&rsquo;s probably taste and I leave it alone.<br>Review the core path first #<br>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.<br>Here&rsquo;s the shape I mean:<br>def fetch_price(sku: str) -> Price:<br>try:<br>return client.get(f"/prices/{sku}")<br>except TransportError:<br>time.sleep(1)<br>return client.get(f"/prices/{sku}")

Every line reads fine and the tests pass. The comment worth leaving isn&rsquo;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.<br>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.<br>The order matters because later comments may disappear once the approach changes. There&rsquo;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.<br>After the approach, I work down through the behaviour:<br>Correctness. What happens with an empty input or a repeated call?<br>Failure. What happens when a dependency times out or a queue delivers the same message again?<br>Security. Trace untrusted input and check the permission against the operation.<br>Evidence. Do the tests prove public behaviour rather than mirror private calls? Have I run the changed path myself?<br>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.<br>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.<br>Make the priority clear #<br>A review with fifteen comments gives every issue the same visual weight, even when only one...