code-review

name	code-review
description	Use when the user asks to "review this code", "give me feedback on a PR", "what should I look for in code review", "how do I review a pull request", or wants to improve their code review process. Provides a structured, senior-engineer-level code review framework.

Code Review

Code review is the highest-leverage quality gate in engineering. A review that catches a design flaw before merge is worth 10x a review that catches a typo. The discipline is knowing what matters, in what order, and how to communicate findings that actually improve the code.

When to Activate

• Reviewing a pull request
• Writing code review guidelines for a team
• Training engineers on effective review
• Receiving feedback on submitted code
• Evaluating whether a PR is merge-ready

Review Priority Order

Not all issues are equal. Review top-down by impact:

1. Correctness — does this code do what it claims? Does it handle edge cases?
2. Design — is this the right abstraction? Does it fit the existing architecture?
3. Security — does this introduce vulnerabilities?
4. Performance — are there obvious algorithmic or query-level problems?
5. Maintainability — will this be easy to change in 6 months?
6. Style — formatting, naming, comments (lowest priority; automate where possible)

Stop blocking on style if an automated linter handles it. Focus review time on 1-4.

What to Look For

Correctness

• Does the logic handle the null/empty/zero case?
• Are race conditions possible? (concurrent writes, shared mutable state)
• Is error handling complete? Are errors swallowed or ignored?
• Does the code match the PR description?
• Are there off-by-one errors in loops and range operations?
• Are all code paths reachable? Is there dead code?

Design

• Is this the right place for this logic?
• Does the abstraction hold under reasonable extensions?
• Is the interface clear from the call site without reading the implementation?
• Does this duplicate logic that already exists?
• Is the complexity justified? Could a simpler approach solve the problem?
• Does this increase coupling between components that should be independent?

Security

Apply OWASP Top 10 as a checklist:

• Are inputs validated and sanitized? (injection, XSS)
• Are authentication and authorization applied correctly? (broken auth, broken access control)
• Is sensitive data encrypted at rest and in transit?
• Are third-party dependencies known-safe? (vulnerable components)
• Is error output safe? (no stack traces or internal paths in user-facing errors)
• Is logging free of PII and secrets?

Performance

• Are N+1 query patterns introduced?
• Is there an O(n²) algorithm where O(n log n) or O(n) exists?
• Are large objects held in memory unnecessarily?
• Are there unbounded loops or recursions?
• Are database queries using indexes? (check EXPLAIN ANALYZE on new queries)

Maintainability

• Can you understand what this code does without reading the tests?
• Are variable and function names descriptive enough?
• Is the function doing one thing?
• Are magic numbers or strings given named constants?
• Is complex logic commented with why, not what?
• Are tests present and testing behavior, not implementation?

How to Communicate Feedback

Severity Labels

Use explicit severity to remove ambiguity:

Label	Meaning
[blocking]	Must be fixed before merge
[suggestion]	Non-blocking improvement you'd recommend
[question]	Seeking understanding, not requesting a change
[nitpick]	Minor style/preference, author may ignore
[praise]	Acknowledge something done particularly well

Feedback Principles

• Be specific: "this is confusing" → "this variable name d doesn't communicate intent; consider document_id"
• Explain why: link to the principle, not just state the problem
• Suggest don't prescribe: "one option would be X" not "you must do X"
• Separate concerns: a blocking correctness issue and a nitpick naming issue are different conversations
• Acknowledge good work: reviews that only find problems train authors to be defensive

Turnaround Time

Code review is a blocking activity for the author. Establish team norms:

• First response within 1 business day
• Do not start a review you cannot finish that day
• Prefer async batched feedback over multiple small rounds

Reviewing Your Own Code

Before submitting a PR:

1. Read the diff as if you are a reviewer who has no context
2. Run the tests and read the test output
3. Check your own PR against this checklist
4. Write a clear description: what changed, why, how to test, any risks

Self-review catches 30-40% of issues before another engineer sees the code.

What NOT to Block On

• Personal style preferences not covered by the team's agreed linter rules
• Minor naming disagreements where both names are clear
• Hypothetical future requirements ("what if we need to add X later?")
• Approaches that differ from how you would do it but are not objectively worse

Block on correctness, security, and significant design issues. Suggest on everything else.

Large PRs

A PR over 400 changed lines is harder to review effectively. Request the author:

1. Split into logical commits that can be reviewed individually
2. Provide a review guide: which files to read first, what the intent of each commit is

Do not rubber-stamp large PRs because the review feels overwhelming.

Gotchas

1. Reviewing style instead of substance: if your review comments are 80% naming and formatting, something is wrong — automate the style layer and focus on design.

2. Approving without reading: "LGTM" from a reviewer who did not read the code provides false assurance and undermines the entire review culture.

3. Design review as PR review: large architectural changes should be reviewed before implementation, not after. If a PR introduces a major design decision, the discussion should have happened earlier.

4. Personal feedback instead of code feedback: "you always do this" is personal. "This pattern has caused bugs in services X and Y" is specific and actionable.

5. Blocking on perfection: a PR that is 90% right and ships is better than a PR that is 100% right and is stuck in review for two weeks. Apply the Pareto principle to blocking issues.

6. Missing the test coverage: reviewing implementation without reviewing tests misses half the PR. Tests document expected behavior — a test that does not test behavior is not a test.

Integration

• testing-strategy — evaluate test coverage and quality as part of every review
• security-engineering — apply the threat model to every PR that touches auth, data access, or external interfaces
• debugging-methodology — use when a review surfaces a bug that needs tracing to root cause
• system-design — evaluate design decisions in the context of the broader architecture

References

• Code Review Checklist — printable checklist for consistent reviews

Skill Metadata

Created: 2026-04-10 Version: 1.0.0