Code Review

Code Review

Review code against structured checklists for correctness, security, performance, and maintainability. Give actionable, prioritized feedback.

When to Use

• User asks for a code review, PR review, or feedback on code
• User wants to check code quality, security, or style before merging
• User is auditing a file or module

Workflow

1. Understand context: What is the change for? (issue, feature, refactor)
2. Read the diff: What actually changed?
3. Collect all issues—never stop at first error: Review the entire scope and return every issue you find. Do not stop when you hit the first bug or suggestion. Use manage_todos, a scratch file, or spawn_subagent (for large PRs) to track issues as you go, then output the complete list.
4. Gather usage context: Before judging changes in isolation, gather enough context to give complete feedback:
   • Surrounding code: Read the function/module containing the change, not just the modified lines. What invariants, preconditions, or patterns exist?
   • Call sites & usage: How is this code called? Search for callers, imports, or usages. Are there edge cases or misuse patterns at call sites?
   • Integration points: Does this touch APIs, DBs, external services, or shared state? How does it fit into the larger flow?
   • Tests: If tests exist for this area, read them to understand expected behavior and coverage gaps.
5. Run checklist: Logic, security, performance, style, tests (using the gathered context)
6. Consider language & framework best practices: Does the code follow idiomatic patterns for the stack?
7. Prioritize: Critical → must fix; Suggestion → consider; Nice-to-have → optional
8. Respond: Summary + categorized comments + concrete suggestions + what was done well

Using git_diff Efficiently

The git_diff tool truncates large diffs (default 500 lines). For PRs with many files or 1k+ lines of changes:

1. Get the file list first: Call git_diff with commit (e.g. main...HEAD) and nameOnly: true. You receive paths — the full list of changed files.
2. Fetch content by batch:
   • Small PR (few files, under ~500 lines total): Call git_diff with just commit for the full diff.
   • Large PR: Call git_diff with commit and paths set to 5–10 files per batch (e.g. paths: ["src/foo.ts", "src/bar.ts"]). Use maxLines: 2000 if batches are large. Iterate until you've reviewed all files.
3. Prioritize: Review highest-risk areas first (auth, input handling, external calls). Security-sensitive files warrant extra attention.

Don't assume a single git_diff call shows everything. If the result says truncated or you only see a few files, use the batched workflow above.

Review Checklist

Correctness & Logic

• Does it do what it claims? Edge cases?
• Off-by-one, null/undefined, empty inputs?
• Error handling: failures caught and handled?
• Concurrency: races, deadlocks, shared state?

Security

• User input validated and sanitized?
• No secrets in code or logs?
• Auth/authz checked where needed?
• Dangerous functions (eval, exec, SQL concatenation) avoided?

Performance

• Obvious inefficiency? (e.g. loop in loop when avoidable)
• Large data: streaming, pagination, or limits?
• Caching or repeated work that could be reused?

Maintainability

• Naming clear? Functions do one thing?
• Duplication that could be factored?
• Magic numbers/strings that should be constants?
• Comments only where needed (why, not what)?

Tests

• New behavior covered by tests?
• Tests readable and stable (no flake)?
• Mocks/fixtures appropriate?

Style & Conventions

• Matches project style (lint, formatter)?
• Imports organized? Dead code removed?

Feedback Format

Use a consistent severity and format:

## Code Review: [PR/File name]

### Summary
[1-2 sentences: overall assessment and main concerns]

### Well done
- [Highlight specific things done very well—clear naming, solid error handling, idiomatic patterns, etc.]
- [Recognize good use of language/framework features.]

### Critical (must address)
- **[Location]** [Issue]. [Suggestion or fix.]
- ...

### Suggestions (consider)
- **[Location]** [Issue]. [Optional improvement.]
- ...

### Nice to have
- [Minor polish or future improvement.]

Location: file path, function name, or "line N" (if line numbers available). Be specific so the author can jump to it.

Suggestion: Prefer concrete fix (code snippet or exact change) over vague "consider improving X."

Tone

• Respectful and constructive
• Assume good intent; explain the "why" behind requests
• Distinguish "this is wrong" vs "this could be clearer"
• No nitpicking without value; batch trivial style nits

What Not to Do

• Don't skip the "Well done" section—acknowledging good work motivates and reinforces best practices
• Don't demand personal style preferences unless they're project convention
• Don't repeat what the diff already shows ("you added a function")
• Don't leave only "LGTM" without at least a one-line summary
• Don't mix severity: critical items must be clearly marked

Small vs Large PRs

• Small: Full checklist, quick pass. One round of feedback.
• Large: Summarize by area (e.g. "Auth logic", "UI components"). Call out highest-risk areas first. Suggest splitting if it would help.

Security-Sensitive Areas

Extra scrutiny for:

• Auth (login, sessions, permissions)
• Input handling (forms, API params, file upload)
• External calls (HTTP, DB, shell)
• Crypto and secrets

For these, explicitly note: "No issues found" or list concrete concerns.

Anti-Patterns

• ❌ Reviewing the diff in isolation without checking surrounding code or call sites—context-free feedback is often incomplete or wrong
• ❌ Vague feedback ("this could be better")
• ❌ Only praising without actionable items when issues exist
• ❌ Blocking on style nits that aren't in the style guide
• ❌ Missing the main bug or security issue while commenting on formatting

Converted and distributed by TomeVault — claim your Tome and manage your conversions.