Code Review Beyond Correctness
“It compiles and the tests pass” is the floor, not the bar. What else are you reading a diff for?
This is the opening question of any code-review screen for a senior engineer, and it separates the people who treat review as a pass/fail gate from the people who treat it as the highest-leverage quality and mentorship tool a team has. The interviewer wants to hear that when you open a diff, “does it work” is the first thing you check and the least interesting. Everything that determines whether this code is still a good decision in two years — the design, the readability, the tests, whether anyone but the author understands it, the security and privacy exposure, whether it can be operated at 2 AM — is what an experienced reviewer actually spends attention on. Get this answer right and you set the frame for the whole loop.
What this question is really testing
Whether you have a layered mental model of review and the judgment to spend your (and the author's) limited attention on what matters. Junior engineers review line by line for bugs and style. Senior engineers review the change against its intent, the system it lands in, and the humans who will maintain it. The tell is whether you name the axes beyond correctness and the discipline to push the mechanical ones into tooling so humans review substance.
How to answer- Correctness is the floor, so start there and move on fast. Logic, edge cases, error handling, off-by-ones — yes, you check them, but say plainly that tests and types should catch most of this so your human attention goes higher.
- Design and approach. The highest-value review question is often “is this the right way to solve this at all?” — the right abstraction, the right boundary, whether it fits the existing architecture, whether it's solving the actual problem or a symptom. A correct implementation of the wrong design is still wrong.
- Readability and maintainability. Code is read far more than it's written. You review for the next engineer — naming, clarity, cohesion, whether the diff is understandable without the author in the room. Cleverness that needs a comment to decode is usually a defect.
- Tests. Not just “are there tests” but do they cover the risk in this change — the edge cases, the failure paths, the concurrency — and would they actually fail if the code regressed. A test that can't fail is decoration.
- Shared context and bus-factor. Review spreads knowledge of the codebase across the team. You use review to make sure a system isn't understood by exactly one person, and to onboard yourself and others into unfamiliar corners.
- Mentorship. Review is your highest-frequency teaching moment. Explaining the why behind a comment is how a mid-level engineer grows into the bar — feedback backed by a principle teaches, feedback backed by taste breeds resentment.
- Security, privacy, and operability. Some risk classes are exactly what a human is there to catch — an unvalidated input, PII in a log line, a missing timeout, no metric on a new code path. Ask “can an on-call debug this when it breaks?”
- Automate style away. Close on the seniority tell: if two people are arguing about formatting or import order, the linter has failed, not the author. Push mechanical checks into CI so review is about judgment, not whitespace.
- A layered answer — correctness named quickly, then design, readability, tests, context, security, operability.
- Review framed as a mentorship and knowledge-sharing lever, not just a defect gate.
- A clear instinct to automate the mechanical so humans review substance.
- Attention budgeted deliberately — you don't nitpick a diff to death; you spend it where the risk is.
- Empathy for the author and the future maintainer, not gatekeeping.
Concrete example — the diff that “worked.” An engineer sends a diff that adds a new enrichment step to an ad-event ingestion path. It's correct, it has a unit test, CI is green. A line-by-line reviewer approves it. A senior reviewer asks four different questions: Design — why a synchronous call to the enrichment service on the hot path instead of joining against the already-loaded dimension table? Tests — there's a happy-path test, but what happens when enrichment times out; is the event dropped, retried, or emitted un-enriched? Operability — there's no metric or log for the new failure mode, so an on-call would be blind. Privacy — the enrichment payload now carries a user identifier into a log line at DEBUG. Same diff; the difference is entirely in what the reviewer chose to look at.
Common follow-ups
How do you decide how deep to go on a given diff?
How to answer- Risk-weight your attention. A one-line config change to a load-bearing path deserves more scrutiny than a 400-line test-only diff. Blast radius, not line count, sets the depth.
- Read the intent first. Understand what the change is trying to do before judging how it does it — a good description and a linked task change how you review.
- Trust the tooling on the mechanical. If CI, types, and lint are green, don't re-audit them; spend your budget on design and failure modes.
Isn't a lot of this the author's job, not the reviewer's?
How to answer- Yes — and review is the safety net, not the primary control. The goal is authors who self-review to this bar; review verifies and teaches, it doesn't substitute for care.
- Push the bar upstream. Checklists, templates, and a written standard mean authors catch most of this before you see it.
- A recurring reviewer catch is a process gap. If you keep finding the same class of issue, encode it as a check or a guideline rather than relying on vigilance.
What do you deliberately not comment on?
How to answer- Anything a machine should own. Formatting, import order, trailing whitespace — if it's in a human comment, it belongs in a linter.
- Pure taste, unless labeled optional. Style preferences get a “nit:” prefix and the author decides; they never block a land.
- Rewrites of correct, working code to match how you'd have written it. That's ego, not review.
How does AI-assisted review change this?
How to answer- Great at the mechanical tier. AI is a strong first pass on obvious bugs, missing tests, and boilerplate — the same tier you'd want automated anyway.
- It doesn't own judgment or accountability. Design intent, business context, and the security sign-off still need a human who is responsible for what lands.
- Use it to free attention, not to skip humans. Let it triage and pre-comment so reviewers focus on the hard, contextual calls; verify its suggestions rather than trusting blindly.
- Phabricator — pull review comments where you raised a design question or caught a correctness/operability issue, not a nit, to prove you review for substance.
- Internal wiki review guidelines — the written standard or checklist that encodes what “good” review covers for your team.
- SEV / postmortems — an escaped defect whose class a stronger review would have caught, and the guardrail you added.
- Code search — where review spread ownership of a system beyond a single author, reducing bus-factor.
- GSD — the quality or velocity goal the review practice was tied to.