name	code-review
scope	generic
description	How to review PRs effectively - what to look for, how to give feedback, and when to approve. Use this when reviewing code or preparing for review.
version	1.2.0
triggers	code review, review pr, pull request review, review feedback, pr checklist
requires
triggered_by	python-engineer
gates	[object Object]
session_persistence	[object Object]

Code Review

You are reviewing code to catch bugs, improve quality, and share knowledge. Good reviews are thorough but respectful, specific but not nitpicky. Focus on correctness, security, and maintainability.

Your goal: Catch real issues while helping the author improve.

Part 1: Review Checklist

1.1 Core Checklist

CORRECTNESS
[ ] Does the code do what it claims?
[ ] Are edge cases handled?
[ ] Are error conditions handled?
[ ] Does it match the requirements/issue?

SECURITY
[ ] No hardcoded secrets/credentials
[ ] Input validation present
[ ] No SQL injection vulnerabilities
[ ] No XSS vulnerabilities
[ ] Authentication/authorization correct

DESIGN
[ ] Follows project architecture
[ ] No unnecessary complexity
[ ] Single responsibility principle
[ ] Appropriate abstraction level
[ ] No code duplication

TESTING
[ ] Tests cover new functionality
[ ] Tests cover edge cases
[ ] Tests are readable
[ ] No flaky tests introduced

MAINTAINABILITY
[ ] Code is readable
[ ] Naming is clear
[ ] Comments explain "why" not "what"
[ ] No magic numbers/strings

1.2 Review Priorities

Priority	Focus	Time
P0	Security vulnerabilities	Stop everything
P1	Bugs and correctness	Must fix before merge
P2	Design issues	Should fix
P3	Code quality	Nice to fix
P4	Style/formatting	Optional (automate)

Part 2: What to Automate

2.1 Automate These

# These should fail in CI, not human review

Formatting:
  - ruff format --check
  - black --check

Linting:
  - ruff check
  - mypy --strict

Security:
  - bandit -r src/
  - safety check

Tests:
  - pytest --cov

Commit format:
  - commitlint

2.2 Don't Waste Time On

Category	Instead
Formatting	Enforce with ruff/black
Import order	Enforce with isort
Type annotations	Enforce with mypy
Unused imports	Enforce with ruff
Line length	Enforce in config

2.3 Focus Human Review On

Focus	Why
Logic correctness	Computers can't verify intent
Design decisions	Requires context and experience
Security implications	Requires threat modeling
Edge cases	Requires domain knowledge
Readability	Subjective but important

Part 3: Human Review Focus

3.1 Logic Review

# Questions to ask:
# - What happens when input is empty?
# - What happens when input is very large?
# - What happens when external service fails?
# - What happens concurrently?

# Example review comment:
# "What happens if `users` is empty? Line 42 will raise IndexError."

3.2 Security Review

# Check for:

# SQL Injection
# BAD: f"SELECT * FROM users WHERE id = {user_id}"
# GOOD:
cursor.execute("SELECT * FROM users WHERE id = ?", (user_id,))

# Command Injection
# BAD: shell commands with user input interpolation
# GOOD: use subprocess with list arguments
subprocess.run(["process", user_input], check=True)

# Path Traversal
# BAD: open(f"/data/{filename}")
# GOOD:
safe_path = (Path("/data") / filename).resolve()
if not safe_path.is_relative_to(Path("/data")):
    raise ValueError("Invalid path")

3.3 Design Review

Questions to ask:
- Is this the right place for this code?
- Will this scale if we have 100x data?
- Is this abstraction reusable or premature?
- Does this couple unrelated components?
- Is there a simpler way?

3.4 Test Review

# Check for:

# Missing assertions
def test_create_user():
    user = create_user("alice")
    # Missing: assert user.name == "alice"

# Testing implementation, not behavior
def test_internal_state():
    obj._internal_list.append(1)  # Don't test internals
    assert obj._internal_list == [1]

# No edge cases
def test_divide():
    assert divide(10, 2) == 5
    # Missing: what about divide(10, 0)?

Part 4: Giving Feedback

4.1 Comment Types

Prefix	Meaning	Action Required
`blocking:`	Must fix before merge	Yes
`suggestion:`	Consider this improvement	Optional
`question:`	Need clarification	Respond required
`nit:`	Minor style issue	Optional
`praise:`	Good work!	None

4.2 Good vs Bad Comments

# BAD: Vague
"This is confusing"

# GOOD: Specific with suggestion
"This function is hard to follow because it does three things.
Consider splitting into: validate_input(), process_data(), format_output()"

# BAD: Demanding
"Fix this. Use a dict instead."

# GOOD: Explaining why
"suggestion: A dict would be O(1) lookup vs O(n) for this list.
Since we look up by ID frequently, a dict might be faster."

# BAD: Personal
"You always do this wrong"

# GOOD: About the code
"This pattern can lead to memory leaks. Here's a safer approach: ..."

4.3 The Sandwich (When Needed)

For significant feedback:

1. Acknowledge what works
   "The overall approach looks good, especially the error handling."

2. Give constructive feedback
   "I noticed a potential issue with concurrent access..."

3. End positively
   "Once that's addressed, this is ready to merge."

4.4 Asking Questions

# To understand:
"Could you explain the reasoning behind using X here?
I'm wondering if Y would be simpler."

# To prompt thinking:
"What happens if this external call times out?"

# To suggest without demanding:
"Have you considered using Z? It might handle edge case A better."

Part 5: Review Process

5.1 Before Starting Review

1. Read the PR description
   - What problem does it solve?
   - What's the approach?

2. Check the linked issue
   - Do requirements match implementation?

3. Note the scope
   - How many files changed?
   - Is it too big? (> 400 LOC = request split)

5.2 During Review

1. First pass: Understand
   - Read through all changes
   - Build mental model
   - Don't comment yet

2. Second pass: Analyze
   - Check logic correctness
   - Look for security issues
   - Verify design choices

3. Third pass: Comment
   - Add actionable feedback
   - Categorize by priority
   - Praise good patterns

5.3 Review Decisions

Situation	Action
All good	Approve
Minor issues (P3/P4)	Approve with comments
Design concerns (P2)	Request changes
Bugs or security (P0/P1)	Request changes
Need more context	Comment, don't decide

5.4 After Review

# If author responds:
1. Re-review changed files
2. Verify issues are addressed
3. Update your review status

# If no response in 2 days:
1. Ping the author
2. Offer to discuss synchronously

Part 6: Being Reviewed

6.1 Preparing for Review

Before requesting review:
[ ] Self-review your own PR
[ ] Tests pass
[ ] CI passes
[ ] Description is clear
[ ] PR is < 400 LOC
[ ] No unrelated changes

6.2 Responding to Feedback

# Addressed: Explain what you did
"Fixed! Moved the validation to the entry point as suggested."

# Disagree: Explain why respectfully
"I considered that approach, but X won't work here because..."

# Need discussion: Suggest synchronous chat
"This is nuanced - want to hop on a quick call?"

# Grateful: Acknowledge good catches
"Great catch! I completely missed that edge case."

6.3 What Not to Do

Don't	Instead
Take it personally	It's about the code
Dismiss feedback	Consider it seriously
Ghost the review	Respond within 1 day
Make unrelated changes	Keep scope focused
Push back on everything	Pick your battles

STOP GATES

STOP GATE 1: Checklist Complete

Check: Have you gone through the review checklist? Pass: All sections reviewed Fail: STOP. Complete the checklist.

STOP GATE 2: No Security Issues

Check: Are there security vulnerabilities? Pass: No injection, auth, or data exposure issues Fail: STOP. Request changes immediately.

STOP GATE 3: Tests Exist

Check: Is new code tested? Pass: Tests cover new functionality Fail: STOP. Request tests before merge.

STOP GATE 4: PR Size Reasonable

Check: Is PR < 400 LOC? Pass: Reviewable size Fail: STOP. Request PR split.

STOP GATE 5: Feedback is Actionable

Check: Can author act on your comments? Pass: Specific, with suggestions Fail: STOP. Rewrite vague comments.

Quick Reference

+-------------------------------------------------------------------+
| REVIEW CHECKLIST (COPY/PASTE)                                     |
+-------------------------------------------------------------------+
| **Correctness**                                                   |
| - [ ] Logic is correct                                            |
| - [ ] Edge cases handled                                          |
| - [ ] Error handling present                                      |
|                                                                   |
| **Security**                                                      |
| - [ ] No hardcoded secrets                                        |
| - [ ] Input validated                                             |
| - [ ] No injection vulnerabilities                                |
|                                                                   |
| **Design**                                                        |
| - [ ] Follows architecture                                        |
| - [ ] Not over-engineered                                         |
| - [ ] No code duplication                                         |
|                                                                   |
| **Tests**                                                         |
| - [ ] Tests added for new code                                    |
| - [ ] Edge cases tested                                           |
| - [ ] Tests are readable                                          |
|                                                                   |
| **Decision**: [ ] Approve  [ ] Request Changes  [ ] Comment       |
+-------------------------------------------------------------------+

+-------------------------------------------------------------------+
| COMMENT PREFIXES                                                  |
+-------------------------------------------------------------------+
| blocking:   Must fix before merge                                 |
| suggestion: Consider this change                                  |
| question:   Need clarification                                    |
| nit:        Minor style issue                                     |
| praise:     Nice work!                                            |
+-------------------------------------------------------------------+

+-------------------------------------------------------------------+
| REVIEW TIMELINE                                                   |
+-------------------------------------------------------------------+
| < 200 LOC:  Same day                                              |
| 200-400 LOC: 1-2 days                                             |
| > 400 LOC:  Request split                                         |
+-------------------------------------------------------------------+

Anti-Patterns

If you're doing...	STOP. Do this instead...
Rubber stamping	Actually review the code
Only style comments	Focus on logic and design
Vague feedback	Be specific with examples
Personal attacks	Comment on code, not person
Blocking on style	Automate style checks
Approving > 400 LOC PRs	Request split
Ignoring tests	Require test coverage
Not explaining "why"	Give rationale for changes
Reviewing without context	Read issue/description first
Delaying response	Review within 1-2 days

code-review

Install Skill

SKILL.md