What Senior Engineers Actually Do During Code Reviews (It's Not Just Finding Bugs)

A developer's blog post argues that senior engineers' primary role in code reviews is not merely finding bugs but assessing whether code belongs in the system, citing research from Microsoft and Google showing that most review comments focus on maintainability rather than defects. The post illustrates with a composite example where a currency conversion change in a payment service is locally correct but systemically wrong, creating a second pricing system.

The best code review comment I have learned to leave is not: This has a bug. It is: Should this code exist here at all? A change can be locally correct and systemically wrong. The code is clean. The tests pass. The author's reasoning makes sense. But the change puts a new responsibility in the wrong service, duplicates a rule owned elsewhere, or establishes a pattern the team will spend years undoing. Those changes are dangerous precisely because they look good in the diff. Finding them requires a different model of code review. The job is not only to ask whether the implementation works. It is to ask whether the implementation belongs. That is where experienced reviewers create the most leverage. Bug finding matters. But research suggests that it is an incomplete description of what code review delivers. In a large Microsoft study, Alberto Bacchelli and Christian Bird observed 17 developers across 16 teams, manually classified 570 review comments, and surveyed 165 managers and 873 developers and testers. Finding defects was the top-ranked motivation https://www.microsoft.com/en-us/research/publication/expectations-outcomes-and-challenges-of-modern-code-review/ for 44% of the developers surveyed. Yet only 14% of the comments in the researchers' sample were classified as defect-related. Code improvements, such as removing unnecessary code or improving readability, were the largest category at 29%. The measures are not identical, but the gap is useful: what teams say review is for and what review actually produces are not quite the same thing. A later Microsoft paper made the argument provocative enough to put it in the title: "Code Reviews Do Not Find Bugs." https://www.microsoft.com/en-us/research/publication/code-reviews-do-not-find-bugs-how-the-current-code-review-best-practice-slows-us-down/ Its actual claim was narrower than the headline. The authors reported that about 15% of review comments indicated a possible defect, while at least 50% concerned long-term maintainability. Google's experience points in the same direction. A 2018 case study combined 12 interviews, 44 survey responses, and logs from 9 million reviewed changes https://research.google/pubs/modern-code-review-a-case-study-at-google/ . The researchers concluded that review gave developers a place to teach one another, maintain the integrity of their codebases, and build norms around readability and consistency. The research does not say "stop looking for bugs." It says bug finding is too small a model for the work review is already doing. The senior reviewer's job is to see the change as part of a system, a team, and a sequence of future decisions. Consider this composite scenario. The details are invented, but the pattern is common. A payment service processes a charge through four handlers: php validate - authorize - capture - settle Each handler owns one stage. A change adds currency conversion to authorize . If a customer pays in EUR and the merchant settles in USD, the handler converts the amount before calling the payment processor. The implementation is tidy. It handles rounding. It has unit tests. The motivation sounds reasonable: authorize the amount in the currency that will eventually be settled. Reviewing only for correctness, this is easy to approve. But the settlement amount is already calculated by an upstream pricing component and included with the order. Recalculating it inside authorize creates three system-level problems: authorize can change pricing, the next change can add fees, tax adjustments, or discounts. A handler with one responsibility slowly becomes a second pricing system.No syntax rule exposes those problems. More tests around the new conversion code do not solve them either. The implementation can be perfectly tested and still be the wrong implementation. The review comment should make that distinction clear: The implementation looks solid, so my concern is not the conversion logic. It is ownership. The settlement amount is already calculated upstream and is available on the order. Recalculating it here couples the authorization path to exchange-rate policy and creates a second result that can drift from the invoiced amount. Can we consume the existing value instead and keep conversion with its current owner? That comment has four useful parts: The best outcome is deletion. No replacement abstraction. No new helper. The new handler logic simply does not need to exist. You do not need the whole architecture in your head to notice responsibility creep. Start with four signals. A small change acquires a nearby cleanup, validation, cache, or optimization. Each addition may be reasonable on its own, but convenience is not an architectural reason. Ask: Would we still choose to make this change here if it required its own proposal? A service begins calling another service from a path that did not call it before. A domain package imports an infrastructure client. A synchronous request now waits on a second network hop. Ask: Which boundary changed, and who owns the new failure mode? The diff validates, calculates, normalizes, or authorizes something that another component already decides. Ask: Are we protecting against bad input, or creating a second source of truth? Hooks, plugins, middleware, and callbacks are natural places to put "just one more thing." That flexibility is useful, but it can hide the gradual transfer of business responsibility into infrastructure. Ask: If every future feature followed this pattern, what would this extension point own in a year? These questions are not requests for theoretical purity. They are ways to expose future cost while the cheapest fix is still deleting a few lines. Not every change deserves the same level of scrutiny. Spending twenty minutes debating a private helper while skimming a schema migration is not thoroughness. It is poor allocation of attention. I use three factors to decide how slowly to review: This is not a numerical formula, but the heuristic is useful: review attention ~= blast radius x irreversibility x novelty A copy change behind a feature flag is narrow and reversible. Review it, then move on. A new retry policy in a shared client deserves more thought. It can amplify traffic and alter latency across every caller. An authorization change, irreversible data migration, or public API contract deserves a slower review, explicit failure scenarios, and someone who understands the affected domain. Seniority in review is not demonstrated by finding something to say on every line. It is demonstrated by spending attention where a mistake would be expensive. The Microsoft study found that 91% of surveyed developers said unfamiliar files took longer to review. More importantly, 82% said reviewers familiar with the files gave different feedback: deeper, more conceptual, and more likely to identify subtle issues. That finding should change how teams assign reviews. The most senior available engineer is not automatically the best reviewer. A mid-level engineer who knows the subsystem, its invariants, and the last failed migration may provide more value than a distinguished engineer seeing the code for the first time. For risky changes, look for context in three forms: One person may cover all three. Often they do not. The point is not to add reviewers mechanically. It is to make sure the decision has the context it requires. Here is a practical workflow for the next change you review. Before opening the diff, read the change description and answer: If you cannot explain the intent in one sentence, the review is not ready for line-by-line feedback. Now inspect the shape of the change: This is the pass most likely to produce "should this exist here?" Only then go deep on the implementation: This order matters. There is little value in perfecting tests for code the system should not own. A strong review is not measured by comment count. Sometimes the right output is a detailed concern about ownership. Sometimes it is a question that brings in the person with missing context. Sometimes it is a quick approval because the change is low-risk, reversible, and aligned. The standard I try to apply is simple: A code review should make the system more coherent, not just the diff more correct. Bug finding remains part of the job. It is simply not the ceiling. On your next review, ask one question before reading the implementation: If this code is correct, could it still be the wrong change? That question will not produce more comments. It should produce better decisions. Where does your team draw the line between reviewing implementation and reviewing architecture? What signals tell you that correct code is landing in the wrong place?