Code Review

Code Review

Coverage

• Pre-review fact-gathering: understanding the PR's stated purpose, the linked issue, the size of the diff, and any context the author called out
• Read-order strategy: tests first (do they describe the change correctly?), then the implementation, then the call sites that consume the changed surface
• Severity-grading rubric: blocker / change-requested / suggestion / nit / praise — and when each is appropriate
• Comment-phrasing discipline: how to ask questions instead of make accusations, how to cite line numbers and references, how to distinguish an objective rule from a stylistic preference
• The no-rubber-stamp rule for AI-generated diffs: deliberate verification of the generated code's claims, especially around tests, error handling, and security
• Self-review pass: how to review your own diff before opening the PR, catching the obvious issues so the human reviewer can focus on the non-obvious
• Tools that complement the review: lint output, type-check output, test results, and how to interpret each in context
• The merge decision: when "approve", "request changes", and "close without merge" are appropriate

Philosophy

A code review is a conversation, not a verdict. The reviewer's job is not to prove they could have written the code differently; it is to ensure the change ships in a state the team can maintain. Reviews fail when they are either rubber-stamped (no verification, just a thumbs-up) or weaponised (every review becomes a referendum on the author's competence). The reviewer's leverage comes from reading code the author has been staring at for hours — the reviewer sees the obvious mistakes the author cannot.

For AI-generated diffs the bar is higher, not lower. Karpathy's "vibe hangover" is real: AI-generated code typically looks correct, has reasonable variable names, and compiles cleanly — and contains 1.7-2.74× more security vulnerabilities than human-authored equivalents. A reviewer rubber-stamping an AI diff is not saving time; they are deferring debugging cost to whichever colleague debugs the production failure.

Workflow

The review is six phases. Skipping any phase is a rubber stamp; doing them out of order produces low-signal comments that confuse the author.

Phase 1 — Pre-review fact-gathering (5 minutes)

1. Read the PR title and description. Identify the stated purpose in one sentence.
2. Read the linked issue or task. Confirm the diff matches the issue's acceptance criteria.
3. Note the diff size: <50 lines is a quick review, 50-300 is a normal review, 300-1000 is a slow review, 1000+ is a "should this have been split?" review.
4. Skim the list of changed files. Form a hypothesis: "this change should touch X, Y, Z." If the diff touches files outside that hypothesis, ask why before reading the code.

Phase 2 — Read tests first (10-30 minutes)

1. Open the test files BEFORE the implementation files.
2. For each new or modified test, read the assertion — what does the test claim is true?
3. For each new or modified test, read the setup — what state does the test put the code in?
4. The tests should describe the change in plain language. If the tests are unclear, the implementation will be too.
5. Missing tests for new behaviour is a change-requested signal at minimum, often a blocker for production paths.

Phase 3 — Read implementation (30-90 minutes)

Read in the order the diff is presented (chronological by file path). Apply the rubric below to each diff hunk.

Phase 4 — Read call sites (10-30 minutes)

For every changed public surface (exported function, route, schema), grep for callers. Confirm the diff's contract matches what callers expect. The most expensive bugs are introduced at the contract layer because the implementation looked fine in isolation.

Phase 5 — Author the review (15-30 minutes)

Write comments in the order: blockers, change-requested, suggestions, nits, praise. Each comment must:

• Cite the line number explicitly.
• State the concern before the fix: "this returns null on missing input — should it throw? callers do not currently null-check."
• Distinguish an objective rule (cite the rule) from a preference ("style nit").
• Avoid ad-hominem. The diff is the subject, not the author.

For AI-generated diffs, add deliberate verification comments: "AI-generated — confirmed test exists for the happy path. Did the author verify the empty-input case? It's not in the diff."

Phase 6 — The merge decision

A review without a verdict is incomplete. Don't leave PRs open with comments and no decision.

Self-Review (Before Opening the PR)

Run the same six phases on your own diff before opening the PR. The author's self-review catches 60-80% of the issues a reviewer would otherwise raise, leaving the reviewer's attention free for the non-obvious. Self-review is not optional — it is the cheapest place to catch the obvious mistakes.

Evals

This skill ships a comprehension-eval artifact at examples/evals/code-review.json. The checklist below is the reviewer gate for proposed changes; the eval file is the grader surface.

Verification

• PR description matches the diff scope (no surprise files or scope creep)
• Tests exist for new behaviour and pass locally / in CI
• All blockers and change-requested items have line-cited comments
• AI-generated content is verified, not rubber-stamped
• Naming, security, and convention concerns are graded by severity, not lumped together
• The merge decision is explicit (approve / request-changes / close)

Do NOT Use When

Source: jacob-balslev/skills — distributed by TomeVault.