Code Review (Google eng-practices style)

When to Use

Use this skill when you are:

• Reviewing a pull request, merge request, CL, patchset, or any peer code change intended for merge / approval / submission
• Asked to "review", "take a pass on", or "approve" a code change for merging
• About to write LGTM or leave review comments
• Authoring a PR/MR/CL that will be reviewed
• Self-reviewing your own diff before sending it for review
• Writing or revising a CL description, title, or commit message
• Assessing whether a change is reviewable (size, scope, splittability)
• Splitting a large change into reviewable pieces
• Responding to reviewer feedback, or pushing back on a comment
• Stuck in disagreement with a reviewer or author
• Considering whether something qualifies as an emergency CL
• Pair-programming or doing in-person review

Not for: exploratory debugging of someone else's code that isn't being reviewed; just reading code to learn it.

Trigger keywords (EN/KO): PR, MR, CL, patchset, patch for review, LGTM, pull request, merge request, code review, peer review, PR description, review my diff, self-review, nitpick, Nit, pushback, rubber-stamp, emergency CL, 코드 리뷰, PR 리뷰, MR 리뷰.

Quick Start (the runbook)

1. Identify your role — reviewer, author, or both (responding to a review = both).
2. Reviewer: Navigate (description → main file → rest) → run checklist (Design first; every line read) → write labeled comments → respond within 1 business day → push back fine; insist on cleanup; escalate, never stall.
3. Author: Keep CL to ~100 lines, one self-contained thing → imperative first-line description + body explaining why → self-review → tests in the same CL → address every comment by fixing the code first.
4. Emergencies are rare — verify the named category applies before bypassing the normal bar.
5. Hold the line on non-negotiables: small CLs, real descriptions, cleanup before submit, code-not-developer comments, every-line-read, one-business-day response, labeled severity, never let a CL stall.

Before LGTM — reviewer gate

Before you type LGTM (or approve), confirm each:

• [ ] You read every line of human-written code, or you said in a comment which parts you reviewed and which you didn't.
• [ ] You explicitly checked the design (it's the most important thing).
• [ ] You opened the whole file for context, not only the diff.
• [ ] You labeled severity on non-blocking comments (Nit: / Optional: / FYI:) — Google says "Consider labeling," so it's a strong recommendation, not absolute.
• [ ] If LGTM-with-comments: you stated which of the 3 conditions applies when not obvious, and labeled each unresolved comment so the author can prioritize.
• [ ] Any "I'll clean it up later" → you insisted on cleanup now, or you required an assigned bug (the bug is required; a TODO in code referencing it is encouraged but optional) before approving.
• [ ] Any specialized concern — Google's explicit call-outs (privacy, security, concurrency, accessibility, internationalization) and high-risk areas the same logic covers in practice (auth/crypto/PII, security boundaries, DB migrations, infra/CI/CD, legal) — a qualified reviewer is on the CL. CC alone is not enough.
• [ ] Tests appropriate to the change live in this CL — behavior/logic changes need new or updated tests; pure refactors have coverage (existing or added). Unless this is a verified emergency.
• [ ] If the CL is large enough that you couldn't realistically read every line: you asked the author to split, not rubber-stamped.

Before submit — author gate

Before you ask for review (or merge a reviewed CL), confirm each:

• [ ] First line of the description: imperative, complete sentence, stands alone.
• [ ] Body explains why: problem, approach, shortcomings, links (with enough context inline that future readers don't depend on the links).
• [ ] CL is ~100 lines or you've consciously decided larger is necessary (see can't make it small enough).
• [ ] Non-trivial refactors are in their own CL (small cleanups like a local variable rename inside a feature CL are fine — judgment call).
• [ ] Major reformatting is in its own CL (mixing with functional changes makes diffs and rollbacks hard).
• [ ] Tests appropriate to the change are in this CL — behavior/logic changes have new or updated tests; refactors have coverage (existing or added). Unless verified emergency.
• [ ] You self-reviewed the diff as if you were the reviewer.
• [ ] If revising after review: you fixed the code, not just the review thread; you addressed every comment.

The Standard (one rule that resolves most arguments)

In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn't perfect.

"In general" is a real qualifier: a reviewer can deny a well-designed CL that adds a feature they don't want in the system — but they must state the specific ownership/product/design objection (not vague "I don't like it") and route disputes through team process if challenged. Otherwise: "improves code health" — not "is perfect" — is the bar.

Do not wait for perfect code. "A key point here is that there is no such thing as 'perfect' code — there is only better code." Pursue continuous improvement, not perfection. Reviewers are always free to leave comments noting that something could be better — prefix non-essential polish with Nit:.

Nothing here justifies merging CLs that worsen code health. The only exception is an emergency.

Reviewers carry ownership and responsibility for the code they review — the codebase staying consistent and maintainable is your responsibility, not just the author's.

Principles (use these to resolve disagreement)

1. Technical facts and data overrule opinions and personal preferences.
2. On matters of style, the style guide is the absolute authority. Whitespace and similar with no style-guide rule = personal preference; be consistent with what's already there; accept the author's choice if there's no prior style.
3. Aspects of software design are almost never a pure style issue or just a personal preference. Design is technical. If the author can demonstrate (data or principle) that several approaches are equally valid, accept the author's choice. Otherwise the choice follows standard software-design principles.
4. If no other rule applies, the reviewer may ask the author to be consistent with the existing codebase — as long as that doesn't worsen overall code health.

Re-read these when stuck.

Reviewer Workflow

Picking reviewers (if you're triaging)

Choose the reviewer who can give the most thorough, correct review within a reasonable response time — typically the actual code owner, who may or may not be in the OWNERS file. Different parts of a CL may go to different reviewers. If the ideal reviewer is unavailable, CC them — but CC is not a substitute for a qualified owner actually reviewing the change.

In-person review or pair programming with a qualified peer counts as review. You can also do in-person reviews where the reviewer asks questions and the author only speaks when spoken to.

Specialized-concern reviewers

Google's source-explicit call-outs — make sure a qualified reviewer is on the CL for:

• Privacy
• Security
• Concurrency
• Accessibility (a11y)
• Internationalization (i18n)

The same logic applies in practice (not in source, but consistent with it) to other high-risk areas:

• Authentication / authorization — login, sessions, tokens, ACLs
• Cryptography — signing, hashing, key handling, custom crypto
• PII / data handling — user-data storage, logging, retention
• Security boundaries — input validation, sandboxing, request handling, trust transitions
• Concurrency primitives — locks, channels, atomics, async/await, goroutines, parallel pipelines
• Database migrations / schema changes
• Infrastructure / deployment / CI/CD configuration
• Legal / compliance / licensing

If you yourself are not qualified in the area: find a co-reviewer who is. Don't approve silently.

Step 1: Navigate the CL

1. Take a broad view. Does the change make sense? Does it have a good description? A good description has: imperative first line, blank line, body covering problem / approach / known shortcomings / links with inline context. If the description fails this bar, ask for a rewrite before you start the deep review — you can't reason about a CL whose intent you don't know. If the change itself shouldn't have happened, respond immediately with why — courteously — and suggest what they should have done instead. If you keep getting CLs you don't want, re-work the team's development process so unwanted work is caught with conversation before someone writes it.
2. Examine the main parts first. Find the file(s) with the largest logical change. If the CL is too large to identify which parts are major, ask the author what to look at first — or ask them to [split](#splitting-strategies) the CL. If you spot major design problems, send those comments immediately, before reviewing the rest — because (a) the author is likely already building follow-up work on top of this CL and (b) major rework takes time; they need to start now.
3. Read the rest in a sensible order. After the major files, file-order is usually fine. Sometimes reading tests first helps — it tells you what the change is supposed to do.

Step 2: Reviewer mechanics (process gates)

These are how you review — apply them on every review.

• Read every line of human-written code you've been assigned. Scan data files / generated code (but see the rationalization counter — don't silently skip).
• Open the whole file, not just the diff — four new lines might live inside a 50-line method that now needs to be broken up.
• Think about system-wide effect. Don't accept CLs that degrade code health, even via small additions that compound quietly.
• *If you can't read or understand a block, ask the author to clarify it in the code before you continue.* If you can't read it, future readers can't either; asking helps them too.
• Tell the author what they did well, with reasoning — clean algorithm, exemplary tests, elegant fix, something you learned. Reinforcement is part of mentoring; sometimes more valuable than corrections.
• If you're only one of several reviewers and reviewed only part: note in a comment which parts you reviewed. If your LGTM depends on other reviewers covering other parts, state that explicitly so expectations are clear.

Step 3: Quality dimensions (the checklist)

*Design is the most important thing to cover.* Start there. Apply each below, in priority order:

Step 4: Speed — respond within one business day

• If you're not in focused work, review shortly after the CL comes in. *One business day is the maximum response time*, not the target. Typical CLs should get multiple rounds within a single day.
• Optimize for team velocity, not your own. Slow reviews → team velocity drops, developers protest, pressure to merge worse code grows. Most complaints about the code review process are actually resolved by making it faster.
• Don't interrupt your own deep coding to start a review. Interrupting a coder costs the team more than making a reviewer wait. Wait for a natural break (task end, lunch, post-meeting).
• Fast individual responses > fast overall process. Even multi-round reviews stay civil if each round is quick. Too busy for a full pass? Send a quick note: when you'll get to it, suggest other reviewers, or leave broad initial comments.
• LGTM must mean "this code meets [our standards](#the-standard-one-rule-that-resolves-most-arguments)." Don't trade quality for speed.
• Cross-timezone: finish your review before the author's next work day starts.
• Strict + fast reviews compound over time (developers send better CLs; reviewers respond faster) — but never compromise on standards for imagined speed gains.

#### LGTM with comments

Approve while leaving unresolved comments only when at least one applies:

1. You trust the author to address all remaining comments.
2. The comments are optional / don't have to be addressed.
3. The suggestions are minor (sort imports, fix a typo, remove an unused dep).

State which condition applies when not obvious from context. Label each unresolved comment so the author can prioritize. Especially worth using when timezones would otherwise cost a full day. Do not use LGTM-with-comments to defer a blocking issue.

#### Large CLs

If a CL is too big to review in reasonable time, ask the author to split it. If splitting is genuinely impossible and you can't review fully, leave broad design comments and send back — never rubber-stamp. Goal: always unblock the author without sacrificing code health.

Heuristics that suggest a split (Google says there are no hard size rules — reviewer judgment governs; these are signals, not absolute triggers):

• You cannot summarize the CL as one logical change in one sentence.
• You cannot identify the "main" file or area quickly.
• You cannot realistically read every line in a single sitting.
• The CL bundles unrelated concerns (e.g. refactor + feature).

Writing comments

• Be kind. Explain your reasoning. Make comments about the code, not the developer. You don't always have to phrase things this way — but always do so when saying something that might otherwise be upsetting or contentious.
• ❌ "Why did you use threads here when there's obviously no benefit to be gained from concurrency?"
• ✅ "The concurrency model here is adding complexity to the system without any actual performance benefit that I can see. Because there's no performance benefit, it's best for this code to be single-threaded instead of using multiple threads."
• Balance pointing-out vs prescribing. Fixing a CL is the developer's job, not yours — when you are acting only as reviewer, do not take over implementation unless explicitly asked. Pointing out the problem often produces better solutions (they're closer to the code) and teaches more. Sometimes direct suggestions or code are more efficient — judgment call.
• Label severity on every comment that isn't blocking:
• Nit: — minor; technically should but won't hugely impact things
• Optional: / Consider: — may be a good idea, not strictly required
• FYI: — not for this CL, but interesting for the future
• Followup: — will track separately (require an assigned bug)
• (unlabeled = blocking; please address before merge — make this expectation explicit when ambiguous)
• *Encourage clarification in the code, not the review tool. If you ask "what does this do?", the right result is the author rewriting the code (or adding a code comment) — explanations written only in the review tool don't help future code readers.* Accept review-tool-only explanations only in narrow circumstances (you're unfamiliar with the area; the author explains what normal readers would already know).

Mentoring is part of review. Code review teaches language, framework, and design knowledge — share it generously. Purely educational comments not critical to meeting the standard get a `Nit:` prefix so the author knows they're not mandatory for this CL.

Handling pushback

• Are they right? They're often closer to the code. Does their argument hold up from a code-health standpoint? If yes, say so plainly and drop it.
• If you're right, keep advocating. Re-explain in a way that shows you understood their reply and adds new information. Improving code health tends to happen in small steps. Multiple rounds may be needed before it sinks in.
• Stay polite even when firm. Let them know you hear them, you just don't agree. Worry about upsetting the developer is usually overblown — upsets come from how comments are written more than from insistence on quality.
• Cleanup: insist on it now. A common pushback: "I'll clean it up in a follow-up CL." Experience shows that unless the cleanup happens immediately, it doesn't — not because developers are irresponsible, but because new work crowds it out. "Letting people 'clean things up later' is a common way for codebases to degenerate." *It is usually best to insist the developer clean up the CL now, before the code is "done" and in the codebase. If the CL introduces new complexity, it must be cleaned up before submission — unless it's an [emergency](#emergencies).*
• If a cleanup truly must wait: the cleanup must be for a pre-existing surrounding problem that this CL exposes but didn't introduce. The author files a bug and assigns it to themselves; the author may also add a TODO in the code referencing the bug. The assigned bug is required; the TODO is optional but encouraged (the source language is "optionally also write a TODO").
• General complaints about strictness usually fade once review speed improves. It can take months. Loud protesters often become the strongest supporters once they see the quality benefit.

Resolving conflicts

1. Try to reach consensus, grounded in this document and the rest of the eng-practices guide.
2. If consensus is hard, face-to-face or video helps — but record the discussion result as a comment on the CL so future readers have the rationale.
3. If still unresolved, escalate: broader team discussion, technical lead, code maintainer, engineering manager.
4. Don't let a CL sit because the author and reviewer can't agree.

Author Workflow

Small CLs

Keep each CL to one self-contained change — typically one part of a feature, not the whole feature. Include related test code. Everything the reviewer needs is in the CL, its description, the existing codebase, or a CL they've already reviewed. The system keeps working after the CL submits.

There are no hard rules about size, but the rough guidance is:

• 100 lines is usually a reasonable size for a CL.
• 1,000 lines is usually too large — but the reviewer's judgment governs.
• File count matters too: a 200-line change in one file might be fine; spread across 50 files it would usually be too large.
• Err toward CLs that are too small over too large. Work with your reviewer on what an acceptable size is. Reviewers rarely complain about CLs being too small.
• Don't go so small that implications are unclear — if you add a new API, include a usage of it in the same CL (this also prevents checking in unused APIs).

Reviewers may reject your CL outright purely for being too large. Easier to write small from the start than to argue or split after.

When large CLs are less bad — either case applies (these are independent exceptions, not joint requirements):

• Entire-file deletions count as roughly one line of review.
• CLs generated by an automated refactoring tool you trust completely — the reviewer's job is just to verify the intent (not every line). "Trust completely" in practice means: the transform is reproducible, no manual edits were mixed in, and tests still pass. Merging and testing caveats still apply.

Writing small CLs efficiently

Don't sit blocked waiting for review. Options:

• Work on multiple projects in parallel.
• Find reviewers who agree to be immediately available.
• Do in-person reviews or pair programming (itself counts as review).
• [Split](#splitting-strategies) so you can continue on the next CL while the first is in review.

Splitting strategies

• Stack: send CL A; immediately start CL B based on A.
• By files / sub-areas: independent file groups → different reviewers in parallel (e.g. proto CL + the code that uses the proto). Inform both sets of reviewers about the related CL so they have context.
• Horizontally: by layer (model → service → API → client), with shared stubs/protos to abstract layers.
• Vertically: independent end-to-end feature slices that can move in parallel.
• Grid: combine — each cell in the (feature × layer) matrix is a standalone CL.

Can't make it small enough?

This is very rarely actually true. Before sending a large CL:

• Consider a refactoring-only CL first to pave the way for a smaller implementation CL.
• Talk to teammates — someone may see a decomposition you don't.
• If a large CL is truly unavoidable, get reviewer consent in advance. Expect a long review, be vigilant about bugs, be extra diligent about tests.

Separate refactorings from feature changes

Refactor CL + feature CL is two CLs, not one. Moving and renaming a class is its own CL, not bundled with a bug fix in that class. Tiny cleanups (a local variable rename) inside a feature CL are fine. Judgment call on what's "tiny."

Tests in the same CL

• A CL that adds or changes logic must include new or updated tests for the new behavior.
• Pure refactoring CLs should also be covered by tests (ideally pre-existing; add if missing).
• Independent test-only modifications can go in separate CLs first — useful for: validating already-submitted code with new tests before refactoring it; refactoring the test code itself; introducing larger test framework code.

Don't break the build

When stacking dependent CLs, ensure each one keeps the system working after submission. Don't break the build for everyone else between your CL submissions.

CL descriptions

A CL description is a permanent public record, read potentially by hundreds of people over years. Future engineers will search for your CL based on its description — sometimes that future engineer is you. Reading the source reveals what; the description reveals why. Without the why, future devs can't tell whether they can move Chesterton's fence.

First line:

• Short summary of what is being done.
• Complete sentence, imperative. Say "Delete the FizzBuzz RPC and replace it with the new system." instead of "Deleting the FizzBuzz RPC and replacing it with the new system."
• Stands alone (shows up in git log summaries).
• Followed by a blank line.

Body:

• Problem being solved.
• Why this approach (briefly mention alternatives if relevant).
• Known shortcomings.
• Links to bug, design doc, benchmarks, screenshots.
• Even small CLs deserve context.
• External links may not be visible to future readers due to access restrictions or retention policies — include enough context inline.

Bad descriptions (some are real): "Fix bug", "Fix build.", "Add patch.", "Moving code from A to B.", "Phase 1.", "Add convenience functions.", "kill weird URLs." — short, vague, leave future readers stranded.

Good description shape (Google's example):

RPC: Remove size limit on RPC server message freelist.

>

Servers like FizzBuzz have very large messages and would benefit from reuse. Make the freelist larger, and add a goroutine that frees the freelist entries slowly over time, so that idle servers eventually release all freelist entries.

First line states what. Body covers problem, why this is a good solution, implementation context.

Tags ([tag], #tag, tag:) are optional. Keep them short; they must not obscure the standalone first-line summary.

Tool-generated and AI-generated descriptions follow the same standard. Verify them before submitting — generated descriptions often miss the why, the trade-offs, or get the what wrong.

Review the description before submitting. CLs evolve during review — the description should still reflect what the CL actually does.

Handling reviewer comments

• Don't take it personally. Review aims to maintain code quality, not attack you. Even when reviewers express frustration poorly, ask "what's the constructive thing they're trying to communicate?" and respond to that.
• Never respond in anger. It lives forever in the review tool. Walk away if needed.
• Fix the code, not the review thread. If a reviewer says they don't understand something, your first response should be to make the code clearer. If the code can't be clarified, add a code comment. Only if that itself would be pointless should you reply only in the review tool.
• Address every comment — change the code or reply substantively.
• Courtesy and respect should always be a first priority. When you disagree, think collaboratively, not defensively:
• ❌ "No, I'm not going to do that."
• ✅ "I went with X because of [these pros/cons] with [these tradeoffs]. My understanding is that using Y would be worse because of [these reasons]. Are you suggesting that Y better serves the original tradeoffs, that we should weigh the tradeoffs differently, or something else?"
• Disagreement path: make sure you understand the comment → if you understand and disagree, explain trade-offs with technical facts → fix the code where appropriate → discuss to reach consensus.
• If a reviewer is consistently non-constructive or impolite, raise it in person or via private email first. Escalate to a manager only if that fails.

Emergencies

An emergency CL is a small change that fits a named category:

• Lets a major launch continue instead of rolling back
• Fixes a bug significantly affecting users in production
• Handles a pressing legal issue
• Closes a major security hole
• Meets a hard contractual or hardware-ship-window deadline that would cause disastrous outcome if missed

Even in an emergency, the Google standard still requires:

• The change is small (not a refactor or feature dressed up as urgent).
• The reviewer reasons about correctness — "does this actually resolve the emergency?"
• The emergency CL takes priority over all other code reviews when it comes up.
• After the emergency resolves, the CL gets a more thorough re-review.

(Local extensions worth adopting — not in the Google source, but help prevent abuse of "emergency":)

• State the qualifying category in the CL description.
• Include a rollback / follow-up plan in case the fix goes wrong.

NOT emergencies:

• Wanting to launch this week instead of next (unless an actual contractual hard deadline)
• Sunk cost — the author has worked on this for a long time and wants it merged
• Reviewer is in another timezone or away
• End of Friday and the author wants it in before the weekend
• A manager says "today, soft deadline"
• Rolling back a CL that's breaking tests or builds

A hard deadline = something disastrous happens if you miss it. Missing a conference is usually not disastrous. Most "deadlines" are soft — important but not worth sacrificing code health. *If your team is routinely invoking near-cycle-end emergencies to bypass review, change the release process* so large features land early in the cycle.

Common Mistakes — Reviewer

Common Mistakes — Author

Red Flags — Stop and Reconsider

You're about to violate the spirit of this skill when you catch yourself thinking:

As reviewer:

• "It's mostly fine, LGTM" → did you read every line? Check the design?
• "CI passes, so I don't need to reason about correctness" → CI is evidence, not a substitute for design/functionality review
• "It's just config / data / generated code, skim it" → still verify intent and blast radius; note in a comment what you skipped
• "The CL description was AI-generated, looks fine" → verify the why, the trade-offs, and that it still matches the current diff
• "I don't want another round" → only OK if code health is improved as is
• "They can clean it up later" → usually no; insist now, or require an assigned bug + TODO
• "It's huge but I'll power through" → ask to split
• "I don't fully understand this part but I'll approve anyway" → add a qualified co-reviewer, or have the author clarify the code
• "I'm in deep work; I'll skim it" → wait for a break point; either review properly or send "I'll get to it after X"
• "Just CC'ing the security person is enough" → no; CC is not review

As author:

• "I'll explain in the comment thread" → fix the code; thread-only explanations don't help future readers
• "I'll address tests in a follow-up" → tests go in the same CL (unless verified emergency)
• "This reviewer is being annoying" → focus on the technical point; never reply in anger
• "I've worked on this so long, just merge it" → sunk cost is not an emergency

Either side:

• "This deadline makes it urgent" → soft deadlines do not relax the standard; name the qualifying emergency category or it isn't one
• "The AI wrote a reasonable-looking description, ship it" → verify it before relying on it

References

Faithful summary of Google's open-source eng-practices guide (CC-BY 3.0). Original text:

Reviewer:

• The Standard of Code Review
• What to look for in a code review
• Navigating a CL in Review
• Speed of Code Reviews
• How to write code review comments
• Handling pushback in code reviews

Author:

• Writing good CL descriptions
• Small CLs
• How to handle reviewer comments

Cross-cutting:

• Emergencies
• Overview (picking reviewers, in-person reviews, mentoring)