| name | review |
| description | Use when reviewing pull requests, PRs, code changes, or code quality.
Covers architecture reviews, security audits, mentoring through review feedback,
and establishing review standards. Triggers on: "review", "code review", "PR review",
"check this code", "audit", "security review".
|
Review
Transform code reviews from gatekeeping to knowledge sharing through constructive feedback, systematic analysis, and collaborative improvement.
Architecture — Read This First
Main agent (YOU — running this skill)
├── Phase 1: Dispatch explore subagent(s) for context gathering
│ ├── explore: gather files, diff, related code, CI status ← pure leaf
│ └── (optional) explore: additional angles (tests, docs) ← pure leaf
├── Phase 2: Synthesize context → classify change, determine scope
├── Phase 3: Dispatch reviewer subagent with enriched brief
│ └── reviewer: reads code, runs checks, returns report ← pure leaf
├── Phase 4: Main agent presents report → calls ask() ← YOU interact
├── Phase 5: (user responds) Main agent fixes selected issues
└── Phase 6: Re-verify fixes, recommend merge
You (the main agent) own orchestration AND user interaction. Two subagent types, both pure leaves:
- explore — gathers context: files to review, diffs, related code, test coverage, CI status, docs. Returns a concise context brief.
- reviewer — receives the context brief + review scope from the main agent. Reads code, runs lint/test commands, performs analysis, returns a categorized report.
Subagents never interact with the user. The reviewer returns its findings as a report. The main agent presents the report to the user and calls ask(). No subagent ever fixes code or calls ask().
Phase 1: Dispatch explore subagent(s)
Use subagent with agent: "explore" to gather context. Dispatch one or more in parallel:
subagent({
agent: "explore",
task: "Gather review context for [target]. Find all changed files, " +
"related imports/callers, test coverage, and any CI/lint status. " +
"Return a concise brief: files, scope, languages, and risk areas."
})
For large changes, dispatch multiple explore subagents in parallel:
- One for the changed files and their dependencies
- One for tests and CI status
- One for docs and related ADRs/plans
Phase 2: Synthesize and classify
The main agent collects all explore results, classifies the change (bug fix, feature, refactor, security), determines review depth, and identifies focus areas.
Phase 3: Dispatch reviewer subagent
Use subagent with agent: "reviewer" and pass the synthesized context:
subagent({
agent: "reviewer",
task: "Review the following for [scope]: [files/context from explore]. " +
"Change type: [classification]. Depth: [quick/standard/deep]. " +
"Focus areas: [correctness, security, perf, etc.]. " +
"Follow the review skill reviewer checklist. " +
"Consult [language] guide. Return a report categorized by severity. " +
"Do NOT call ask() — just return the report."
})
The reviewer subagent returns a report only — it never calls ask() and never fixes code.
Phase 4: Present report and ask
The main agent receives the reviewer's report, displays it to the user, and calls ask() to let the user choose what to fix. This is the only point of user interaction.
When to use subagents vs. review inline
| Scenario | Approach |
|---|
| Single file, < 100 lines, trivial change | Inline review (no subagents) |
| Multiple files or > 100 lines | explore → reviewer → present → ask pipeline |
| Cross-cutting concern (security, perf, architecture) | explore → reviewer with focused scope |
| User says "review" without specifics | explore → reviewer → present → ask pipeline |
| Large change (> 400 lines) | Multiple explore in parallel → reviewer → present → ask |
When to Use This Skill
- Reviewing pull requests and code changes
- Establishing code review standards for teams
- Mentoring junior developers through reviews
- Conducting architecture reviews
- Creating review checklists and guidelines
- Improving team collaboration
- Reducing code review cycle time
- Maintaining code quality standards
Core Principles
1. The Review Mindset
Goals of Code Review:
- Catch bugs and edge cases — Prevent production issues before they ship
- Ensure code maintainability — Make future changes easier, not harder
- Share knowledge — Spread understanding across the team, reduce bus factor
- Enforce standards — Consistency reduces cognitive load for everyone
- Improve design and architecture — Catch structural problems early
- Build team culture — Reviews are conversations, not verdicts
Not the Goals:
- Show off knowledge or prove you're smarter
- Nitpick formatting (use linters/formatters)
- Block progress unnecessarily
- Rewrite code to your personal preference
- A substitute for thorough testing
2. Effective Feedback
Good Feedback is:
- Specific — Point to exact lines, explain the impact
- Actionable — Suggest concrete fixes or alternatives
- Educational — Explain why, not just what
- Focused on code — Never attack the person
- Balanced — Praise good work, not just problems
- Prioritized — Distinguish must-fix from nice-to-have
❌ Bad: "This is wrong."
✅ Good: "This could cause a race condition when multiple users
access simultaneously. Consider using a mutex here."
❌ Bad: "Why didn't you use X pattern?"
✅ Good: "Have you considered the Repository pattern? It would
make this easier to test. Here's an example: [link]"
❌ Bad: "Rename this variable."
✅ Good: "[nit] Consider `userCount` instead of `uc` for
clarity. Not blocking if you prefer to keep it."
❌ Bad: "This is messy."
✅ Good: "This function does 5 different things. Consider extracting
the validation logic into a separate function for clarity."
3. Review Depth Guidelines
Match your review depth to the change:
| Change Type | Depth | Focus Areas |
|---|
| Bug fix (< 50 lines) | Quick | Correctness, edge cases, test coverage |
| Feature addition (< 200 lines) | Standard | Logic, design, tests, docs |
| Major refactor (> 400 lines) | Deep | Architecture, migration safety, rollback plan |
| Security-sensitive code | Thorough | All security checklist items |
| Performance-critical path | Detailed | Algorithm complexity, allocations, memory |
| New dependency | Careful | Security, license, maintenance, alternatives |
Rule of thumb: Spend more time on changes that affect more code, touch critical paths, or introduce new dependencies.
4. Review Scope
What to Review (Deep):
- Logic correctness and edge cases
- Security vulnerabilities (injection, auth, data exposure)
- Performance implications (algorithm complexity, allocations, memory)
- Test coverage and quality (edge cases, meaningful assertions)
- Error handling (complete, informative, recoverable)
- Documentation (README, inline comments, API docs)
- API design and naming (clear, consistent, hard to misuse)
- Architectural fit (patterns, separation of concerns, coupling)
- Concurrency and async safety (race conditions, cancellation safety)
What Not to Review Manually (delegate to tools):
- Code formatting (use Prettier, Black, cargo fmt, etc.)
- Import organization (use import sorters)
- Linting violations (use ESLint, clippy, pylint, etc.)
- Simple typos (unless in user-facing text)
- Style preferences that are already codified in linters
Review Process
See Architecture for the high-level flow. This section expands each phase with detailed checklists.
Explore Subagent — What to Gather
The explore subagent collects:
- Changed files and their contents
- Related imports, callers, and dependencies
- Test coverage for affected code
- CI/lint/build status
- PR description, linked issues, design docs
For large changes (> 400 lines), dispatch multiple explore subagents in parallel:
- explore 1: Changed files + dependencies + related patterns
- explore 2: Tests + CI status + coverage gaps
- explore 3: Docs + ADRs + migration notes
Main Agent — Classification Checklist
After collecting explore results, classify:
- Change type — bug fix, feature, refactor, security, perf
- Review depth — quick (< 100 lines), standard (100-400), deep (> 400)
- Focus areas — correctness, security, performance, architecture
- Risk level — based on affected code paths and test coverage
Checklist:
Reviewer Checklist (what the reviewer subagent does)
The reviewer subagent follows this checklist for each changed file:
Architecture & Design
Performance
- Algorithm complexity (Big-O)
- Memory usage patterns
- N+1 queries or excessive API calls
- Unnecessary allocations
- Consult Performance Review Guide
Security
Correctness
- Logic errors or off-by-one mistakes
- Edge case handling (empty inputs, null values, boundary conditions)
- Error handling completeness
- Concurrency issues (race conditions, deadlocks, TOCTOU)
- State machine transitions complete and valid
Testing
- Are tests covering edge cases?
- Are tests meaningful or just exercising code?
- Is there test coverage for error paths?
File Organization
- Are new files in the right places?
- Is there logical grouping?
- Are there unnecessary files?
Maintainability
- Clear naming conventions (express intent, not implementation)
- Single responsibility principle (one reason to change)
- DRY violations (duplicate logic)
- Comment quality and relevance (why, not what)
- API design (hard to misuse, clear contracts)
- Error messages (helpful for debugging)
Documentation
- README updated if needed
- Inline comments explain non-obvious logic
- API docs for public functions/types
- Migration guides for breaking changes
- Changelog entries for user-facing changes
Main agent presents report and calls ask() — HARD STOP
🛑 HARD STOP POINT — READ THIS FIRST
After the reviewer subagent returns its report, the main agent presents the findings to the user and calls ask(). This is the ONLY place in the entire review process where execution pauses for user input.
The reviewer subagent never calls ask(). It returns a report. The main agent presents it and asks.
EXECUTION FLOW (MUST follow this exact sequence):
reviewer returns report → main agent presents to user → main agent calls ask() → [STOP — WAIT] → (user responds) → main agent fixes
↑
└── HARD STOP.
Main agent PRE-FLIGHT CHECKLIST (Complete ALL before proceeding):
❌ NEVER do any of these:
- Present findings and then immediately start fixing issues
- Say "What would you like to do?" in plain text instead of using
ask()
- Begin fixes before receiving a response from
ask()
- Assume the user wants everything fixed — let them choose
- Have the reviewer subagent call
ask() — only the main agent calls ask()
✅ ALWAYS do this — IN EXACT ORDER:
- Receive reviewer's report
- Present the findings to the user (count and categorize)
- Call
ask() with the question format below
- STOP. Wait for the user's response.
This is a hard break in execution. After ask() the main agent waits — nothing happens until the user responds.
VIOLATION DETECTION: If you find yourself about to fix, suggest, or modify code after presenting findings, you have violated this rule. Go back and call ask() first.
Report format
Tally findings from the reviewer's report:
let countBlocking = 0;
let countImportant = 0;
let countNit = 0;
let countSuggestions = 0;
let countLearning = 0;
let countPraise = 0;
Present the summary to the user
## Code Review Summary
**🔴 Blocking (N):**
1. [Issue description] - `file.ts:42`
- Impact: [What breaks if unfixed]
- Fix: [Suggested approach]
**🟡 Important (N):**
1. [Issue description] - `file.ts:100`
- Impact: [What degrades if unfixed]
**🟢 Nit (N):**
1. [Issue description] - `file.ts:150`
- Impact: [Minor improvement]
**💡 Suggestions (N):**
1. [Suggestion] - `file.ts:200`
- Why: [Benefit of change]
**📚 Learning (N):**
1. [Educational note] - `file.ts:250`
**🎉 Praise (N):**
1. [Positive feedback] - `file.ts:300`
Call ask() immediately after presenting
ask({
questions: [{
id: "fix-priority",
question: `Found ${countBlocking + countImportant + countNit + countSuggestions} issues. What would you like to fix?`,
options: [
{ label: `Fix 🔴 blocking only (${countBlocking})` },
{ label: `Fix 🔴 + 🟡 (${countBlocking + countImportant})` },
{ label: `Fix all (blocking + important + nit)` },
{ label: `Review all findings without fixing` }
],
description: `**🔴 Blocking:** Must fix before merge\n**🟡 Important:** Should fix, discuss if disagree\n**🟢 Nit:** Nice to have, not blocking\n**💡 Suggestions:** Alternative approach to consider\n**📚 Learning:** Educational note, no action needed\n**🎉 Praise:** Good work, keep it up!`
}]
})
STOP and wait for user response
After calling ask(), the review is complete. Do nothing else. Do not start fixing. Do not suggest fixes. Wait for the user to respond, then proceed to Phase 5 based on their answer.
If you skip this step or bypass ask(), you have violated a core rule of the review process.
Phase 5: Main agent fixes issues (Based on User Choice)
⚠️ The main agent handles all fixes — the reviewer subagent never fixes code.
After ask() returns with the user's selection, the main agent fixes issues ONE AT A TIME:
- Explain the problem clearly
- Show the fix
- Verify with tests/linting
- Move to next issue
Phase 6: Re-review (If Issues Were Fixed)
After fixes are applied:
- Re-read the fixed code
- Verify the fix addresses the issue
- Check for any new issues introduced
- If issues persist, escalate to user
- If all clear, recommend merge
Severity Classification Guide
🔴 Blocking — Must Fix Before Merge
Issues that will cause:
- Runtime crashes or panics
- Data loss or corruption
- Security vulnerabilities (injection, auth bypass, data exposure)
- Build failures
- Test failures
- Breaking changes without migration path
- Deadlocks or race conditions in production code
🟡 Important — Should Fix, Discuss if Disagree
Issues that will cause:
- Incorrect behavior in edge cases
- Performance degradation (significant)
- Memory leaks
- Missing error handling
- Test coverage gaps for critical paths
- API design that's hard to use correctly
- Code that will be difficult to maintain
🟢 Nit — Nice to Have, Not Blocking
Issues that are:
- Minor style inconsistencies
- Unnecessary complexity that could be simplified
- Naming that could be clearer
- Redundant code that could be extracted
- Missing documentation that would help future readers
💡 Suggestions — Alternative Approaches
Ideas for:
- Different patterns that might work better
- Libraries or tools that could help
- Architectural improvements
- Performance optimizations
- Code organization improvements
📚 Learning — Educational, No Action Needed
Notes for:
- Alternative approaches worth knowing
- Language/framework features not being used
- Best practices that apply elsewhere
- "Nice to know" information
🎉 Praise — Good Work, Keep It Up
Recognition for:
- Well-designed components
- Excellent test coverage
- Clear documentation
- Creative solutions to hard problems
- Consistent adherence to patterns
Review Techniques
Technique 1: The Checklist Method
Use checklists for consistent, thorough reviews. Start with the language-specific guide, then apply the appropriate domain guide (security, performance, architecture).
Technique 2: The Question Approach
Instead of stating problems, ask questions that guide the author to the solution:
❌ "This will fail if the list is empty."
✅ "What happens if `items` is an empty array? Should we handle that case?"
❌ "You need error handling here."
✅ "How should this behave if the API call fails? Should we retry or show an error?"
❌ "This is too complex."
✅ "This function does validation, transformation, and persistence. Could we split these into separate functions?"
Technique 3: Suggest, Don't Command
Use collaborative language that invites discussion:
❌ "You must change this to use async/await"
✅ "Suggestion: async/await might make this more readable. What do you think?"
❌ "Extract this into a function"
✅ "This logic appears in 3 places. Would it make sense to extract it into a shared function?"
❌ "This is a bad pattern"
✅ "Have you considered using the Strategy pattern here? It would make testing easier."
Technique 4: The Impact Framework
For each finding, explain the impact:
🟡 **Important: Missing error handling in API endpoint** - `api/users.rs:42`
**Impact:** If the database is unreachable, the API returns a 500 with no context.
Users see a generic error, and we lose visibility into the failure.
**Suggested fix:** Add error context with `.with_context()` and return a proper
error response with a user-friendly message.
Technique 5: The Before/After Pattern
Show concrete before/after examples:
💡 **Suggestion: Extract validation logic**
**Before:**
```rust
fn process_order(order: &str) -> Result<()> {
if order.is_empty() {
return Err("empty".into());
}
if !order.chars().all(|c| c.is_alphanumeric()) {
return Err("invalid".into());
}
// ... 50 more lines
}
After:
fn validate_order_id(order: &str) -> Result<()> {
if order.is_empty() || !order.chars().all(|c| c.is_alphanumeric()) {
return Err("invalid order ID".into());
}
Ok(())
}
fn process_order(order: &str) -> Result<()> {
validate_order_id(order)?;
}
## Language-Specific Guides
When reviewing code in a specific language/framework, consult the corresponding detailed guide:
| Language/Framework | Reference File | Key Topics |
|-------------------|----------------|------------|
| **React** | [React Guide](languages/lang-react.md) | Hooks, useEffect, React 19 Actions, RSC, Suspense, TanStack Query v5 |
| **Vue 3** | [Vue Guide](languages/lang-vue.md) | Composition API, Reactivity System, Props/Emits, Watchers, Composables |
| **Rust** | [Rust Guide](languages/lang-rust.md) | Ownership/Borrowing, Unsafe Review, Async Code, Error Handling, Cancellation Safety, Testing, Macros |
| **TypeScript** | [TypeScript Guide](languages/lang-typescript.md) | Type Safety, async/await, Immutability, Generics |
| **Python** | [Python Guide](languages/lang-python.md) | Mutable Default Args, Exception Handling, Class Attributes, Context Managers |
| **Java** | [Java Guide](languages/lang-java.md) | Java 21/25 Features, Spring Boot 4, Virtual Threads, Stream/Optional |
| **Go** | [Go Guide](languages/lang-go.md) | Error Handling, goroutine/channel, context, Interface Design |
| **C** | [C Guide](languages/lang-c.md) | Pointers/Buffers, Memory Safety, UB, Error Handling |
| **C++** | [C++ Guide](languages/lang-cpp.md) | RAII, Lifetime, Rule of 0/3/5, Exception Safety |
| **SQL/PostgreSQL** | [SQL/PG Guide](languages/lang-sql-pg.md) | SQL Injection Prevention, EXPLAIN ANALYZE, Indexing, Concurrency |
| **CSS/Less/Sass** | [CSS Guide](languages/lang-css-less-sass.md) | Variable Conventions, !important, Performance Optimization, Responsive Design |
| **Qt** | [Qt Guide](languages/lang-qt.md) | Object Model, Signals/Slots, Memory Management, Thread Safety |
| **WASM/Frontend** | [WASM Guide](languages/lang-wasm.md) | Memory management, JS interop, bundle size, accessibility, browser compat |
## Additional Resources
- [Architecture Review Guide](guides/guide-architecture-review.md) - Architecture design review guide (SOLID, anti-patterns, coupling)
- [Performance Review Guide](guides/guide-performance-review.md) - Performance review guide (Web Vitals, N+1, complexity)
- [Security Review Guide](guides/guide-security-review.md) - Security review guide (OWASP, injection, auth)
- [Code Review Best Practices](guides/guide-code-review-best-practices.md) - Code review best practices
- [Common Bugs Checklist](checklists/checklist-common-bugs.md) - Common bugs by language
- [PR Review Template](assets/pr-review-template.md) - Standard PR review template
- [Review Checklist](assets/review-checklist.md) - Quick reference checklist
