> ## Documentation Index
> Fetch the complete documentation index at: https://lib.findy.co.jp/llms.txt
> Use this file to discover all available pages before exploring further.
# How to do code review: what to check, how to comment
> Why code review matters, what to check for each type of requirement, how tooling has taken over the checking, and how to self-review and write comments.
## Overview
Code review is the practice of having someone other than the author examine
a proposed change before it merges, and decide — based on that examination —
whether the change is ready to become part of the codebase.
This page starts from the value review brings an organization, works
through what a reviewer actually checks, then looks at how tooling has
taken over more of that checking over time, and closes with two practical habits: reviewing your own change
first, and writing comments that are easy to act on. It assumes the pull
request itself — its granularity, its template, its title and commit
conventions, and the basic request-to-merge cycle — is already in place;
see [Pull Requests](/development/pull-request#review-first) for that layer,
and [Test Code](/development/testing) for what makes a change verifiable in
the first place.
## The value review brings
Catching defects before they ship is the obvious reason to review a
change, and it is real — but a team that stops there is leaving most of
review's value on the table.
Examining a change forces someone besides the author to understand it,
so knowledge of why it exists and how it works is never trapped in one
person's head.
A convention starts as a one-off comment in a review — "we don't do it
this way" — and, repeated across enough reviews, becomes a rule the next
author already knows. The team's shared standard grows one review at a
time.
Response time, security exposure, and compliance with rules outside the
codebase rarely show up as a failing test. Review is often the only
checkpoint that asks about them at all.
A change that already works can still be reviewed toward a better one —
a reviewer who suggests a cleaner approach raises the bar past "does it
work," not just enforces it.
## What review verifies
| Observation | What it checks | Example question |
| --------------------------- | ------------------------------------------------------------------------------------------------------------ | ----------------------------------------------------------------------------------------- |
| Functional requirements | The change does what was asked, across normal and edge cases | Does this satisfy the acceptance criteria in the issue? |
| Non-functional requirements | The change meets constraints beyond behavior — performance, security, compliance with rules outside the code | Does this stay within the expected response time? Does it introduce a new attack surface? |
| Design & architecture | The change fits the existing structure without adding unnecessary coupling | Does this belong in this module, or somewhere else? |
| Test suite | The behavior the change claims is backed by a test that fails without it | If I revert the fix, does a test go red? |
| Readability & convention | The next reader can follow the change without asking the author | Does this naming match how the surrounding code reads? |
Functional requirements are usually explicit — an issue's acceptance
criteria, a ticket's description — so checking them is mostly a matter of
comparing the diff against what was actually asked, not just what compiles.
Non-functional requirements are easy to miss precisely because they rarely
announce themselves: a slow query does not fail the build, and a compliance
gap does not throw an exception.
Design fit, the test suite, and readability round out the check: design fit
protects the codebase the requirement is being added to, the test suite
protects whoever touches this code next, and readability keeps the
change's true cost — the cost of maintaining it — from staying hidden until
long after it merged.
## How tooling has taken on review's mechanical burden
Historically, every row in the table above was checked by a human, and
review meant catching all of it by eye.
A review drags on for several reasons, and one of the least essential is
when it re-derives things a machine could already have told the author.
Lint violations, missing tests, and a naming mismatch a style guide already
settles are the typical cases. Worse, re-deriving the easy items alongside
the hard ones splits a reviewer's attention, making the judgment calls that
actually need it more likely to slip through.
Linters and formatters became the standard fix for exactly this: enforcing
naming, formatting, and a growing set of known-bug patterns automatically,
before a human reviewer ever opens the diff. That is the premise most teams
build on today.
A reviewer's time no longer goes to catching a missing semicolon or a
naming mismatch, because a tool already did.
AI extends the same idea further. Where a linter can only check against
fixed, generic rules, an AI-assisted pass can also check a change against a
team's own coding conventions and context — the kind of judgment that used
to require someone who already knew the codebase.
Correct answers a rule, or a team's own conventions, can decide.
* Adherence to naming and coding conventions
* Test coverage for the changed lines
* Known bug and vulnerability patterns matched against a fixed rule set
Judgments whose correct answer depends on context.
* Whether the design and architecture fit the surrounding system
* Whether the change actually satisfies the requirement
* Security risk acceptance and the final merge decision
```mermaid theme={null}
flowchart LR
PR[Pull request opened] --> AUTO[Mechanical checks
convention, coverage, known patterns]
AUTO -->|findings fixed| HUMAN[Human review
design, requirement fit, security judgment]
HUMAN --> MERGE[Merge]
```
At Findy, this mechanical pass is delegated to AI-assisted self-review,
run before the pull request is even opened. What matters is keeping the
rule-checkable part separate from the part that needs human judgment. See
[Agentic Workflow](/ai/agentic-workflow#responsibility-boundaries) for how
the same principle governs AI-generated code more broadly.
The boundary is not fixed forever. As conventions solidify and the checks a
team trusts grow more reliable, more rows can move from the human side to
the mechanical side — but that migration should be a decision a team makes
on purpose, not something that drifts unexamined.
## Reviewing your own change first
Before requesting a reviewer, read the diff as if you had never seen it.
Distance changes what you notice: a mistake invisible while writing becomes
obvious once you view the change as a stranger would, and every one you
catch here costs a local fix instead of a round trip through someone
else's attention.
Reading the diff in the order the reviewer will see it — top to bottom,
file by file, without editing along the way — surfaces the same rough edges
a reviewer would notice, instead of just the first thing you trip over.
```markdown Example self-review checklist theme={null}
## Before requesting review
- [ ] Every changed line is covered by a test that fails without it
- [ ] No hard-coded secret, credential, or environment value is committed
- [ ] Naming and structure match the surrounding code
- [ ] No leftover debug output, commented-out code, or TODO without an issue link
- [ ] The change matches the acceptance criteria stated in the issue
```
Keep the checklist itself under version control and revise it like any
other convention. When a self-review pass misses something a human later
catches, add the missed case so the same gap does not recur.
## Writing review comments
A comment's job is to get the right change made with the fewest round
trips, which means the author must be able to act on it without guessing
what it means or how urgent it is.
Vague reactions push that guessing work onto the author: "this looks off"
does not say what is wrong, where, or whether it blocks the merge. Naming
the specific location and the reasoning — not just the reaction — turns a
comment into something the author can act on directly instead of a puzzle
to solve first.
Prefixing a comment with a short label that states its weight closes the
other gap. A common set of labels:
| Label | Meaning |
| ------ | -------------------------------------------------------------- |
| `must` | Blocking; must be addressed before merge |
| `imo` | The reviewer's opinion; the author decides whether to apply it |
| `nits` | A minor, non-blocking point (typos, style) |
| `ask` | A genuine question, not yet a judgment |
```text Vague theme={null}
This looks off.
```
```text Labeled and specific theme={null}
[must] This mutates the shared `order` object in place, so the caller's
copy changes too. Return a new object instead of mutating the argument.
```
Where possible, propose the fix rather than only naming the problem — it
turns a round of discussion into a round of confirmation. Where the right
answer genuinely is not clear, prefer a question over an assertion; framing
an uncertain point as a question invites the context the author has and
the reviewer does not, instead of forcing a debate over a claim that may
already be wrong.
## Related pages
Where review is requested and applied — granularity, template, and merge mechanics.
What makes a change verifiable in the first place, and the patterns review checks test code against.
The broader AI/human responsibility split for AI-generated code that the mechanical/judgment split above draws on.
Why the human stays responsible for verifying AI-generated code before it ever reaches review.