“It compiles and the tests pass” is the floor, not the bar. What else are you reading a diff for?
Student edition · understand, apply, verify
Review intent, correctness, maintainability, and operational/security risk with a proportionate attention budget.
What you should be able to do: Explain a blocking concern through a concrete failure and separate it from preferences.
A context-dependent practice guide, not a universal policy or verified personal history. Examples and numbers are illustrative. Use genuine, authorized evidence; personnel/privacy/legal decisions follow applicable organization policies and qualified review.
Concepts explained in this guide
Read these definitions first, or use the linked terms as you go. The examples and limitations are local to this guide; companion pages are optional.
Preconditions and postconditions
A precondition states required inputs/state before an operation. A postcondition states the result promised when those requirements hold and execution terminates normally. Together they make requirements testable.
Example. For sum_first(b,n), require integer 0≤n≤len(b) and numeric elements; promise the sum of b[0:n].
Boundary. State exceptions, mutation, numeric domains, and empty cases. A mathematical array range does not automatically validate a runtime input.
Decision records and reversible experiments
Record the problem, options, evidence, constraints, owner, chosen tradeoff, and reconsideration trigger. A reversible test reduces uncertainty before large commitment.
Example. Try one tenant with success/abort metrics and a data-safe rollback plan.
Boundary. Timeboxing does not bypass compliance/safety. Reversible means effects and data can actually recover, not merely that a flag exists.
SBI feedback and observable behavior
Situation–Behavior–Impact frames feedback around context, observed action, and its effect. Ask for perspective and agree a concrete change.
Example. 'In Tuesday's review, you interrupted two explanations; we missed rollback discussion. What was happening, and how can we finish those discussions next time?'
Outputs are produced things; outcomes are changes for users/business/team. Useful metrics state definition, denominator, population, period, and attribution limits.
Example. 'Shipped a tool' is output. 'Median onboarding fell ten to six days in this cohort' is an outcome with sample/context caveats.
Boundary. Before/after does not prove sole causation. Avoid gaming, selection bias, hidden costs, and unlike-team comparisons.
Ownership, accountability, and decision rights
Ownership names who drives follow-through; accountability names who answers for outcomes. Decision rights specify who chooses/approves and who is consulted.
Example. A migration has a coordinating owner, service owners, and named release authority.
Boundary. DRI/RACI/DACI are local conventions, not guarantees. Role labels do not replace authority, capacity, and follow-through.
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.
The spine of the answer: correctness is step one, then design, readability, tests, shared context, security/privacy, and operability — while style gets automated away rather than argued line by line.
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
Risk-weight correctness, including invariants, boundaries, failure, and concurrency. Tests/types help but do not justify moving past unproved behavior; design, maintainability, and operability complement this reasoning.
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.
What the interviewer is looking for
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 automation for the checks it actually performs, but verify coverage, semantics, and risk-sensitive outcomes. Green CI is evidence, not a universal correctness proof.
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.
Evidence to collect with your approved tools
A decision/review record with the problem, constraints, options, and your contribution.
Relevant tests, dashboards, or measurements with definitions, denominators, dates, and uncertainty.
Incident or delivery records showing actual outcomes and unresolved limitations.
Stakeholder agreements and follow-up actions with owners and review dates.
Only authorized, minimized, safe-to-share evidence. Do not copy confidential personnel notes into a public story.
Worked example: green CI but an untested failure boundary
This is an illustrative situation. The point is the reasoning and evidence, not copying the numbers or first-person story.
Sequence for this example. The table explains the evidence and decision at each step.
Step
Evidence, decision, and expected result
Understand the change
A new enrichment call is added to a hot event path; successful unit tests do not establish timeout/replay behavior.
State the invariant
Accepted events must have a documented durable/dedup path, and fallback behavior must be explicit.
Request evidence
Ask for failure tests, dependency deadlines, authorized telemetry, and an explanation of the chosen synchronous boundary.
Prioritize comments
Label invariant/security violations as blocking under the agreed policy; label style/preferences optional. Correctness remains central, not something to move past quickly.
Check your understanding: Does green CI prove the change is correct?
No. Tests/type checks cover selected properties; review still reasons about contracts, coverage, context, and failure behavior.
Practice with your own situation
Choose one relevant real situation. Write its context, constraints, your role, considered alternatives, chosen actions, measured result, uncertainty, and follow-up. Explain one tradeoff and what evidence would change your decision.
Use genuine, safe-to-share evidence. Example numbers here illustrate reasoning; they are not claims about your work or a guaranteed outcome.
