Comprehensive code review standards covering security, quality, performance, testing, and documentation. Includes checklists, common issues, and best practices for thorough code reviews.
Triggers when reviewing pull requests to catch security vulnerabilities, performance issues, and code quality problems before production. Provides comprehensive checklists for authentication, input validation, database optimization, testing requirements, and documentation standards.
/plugin marketplace add squirrelsoft-dev/agency/plugin install agency@squirrelsoft-dev-toolsThis skill inherits all available tools. When active, it can use any tool Claude has access to.
examples/code-quality-examples.mdexamples/review-comments.mdreferences/performance-review.mdreferences/test-review.mdMaster comprehensive code review practices that catch critical issues before they reach production. This skill covers security vulnerabilities, code quality metrics, performance optimization, testing requirements, and documentation standards to ensure every pull request meets professional engineering standards.
Code review is your last line of defense against bugs, security vulnerabilities, and technical debt. A thorough review process prevents production incidents, maintains code quality, and transfers knowledge across the team.
Review Philosophy:
When to Review vs. Auto-Approve:
Security vulnerabilities are the highest priority in code review. Check these categories systematically.
For comprehensive security review guidance with examples, see:
What to Check:
Review Questions:
What to Check:
Review Questions:
What to Check:
Review Questions:
For comprehensive security review, verify the code doesn't have these OWASP vulnerabilities:
See references/security-review.md for detailed examples and remediation patterns.
Beyond security, code must be maintainable, readable, and follow best practices.
Cyclomatic Complexity: Aim for functions with complexity < 10. High complexity (>15) is a code smell.
❌ Bad: High complexity (15+ branches)
function processOrder(order, user, inventory) {
if (order.type === 'standard') {
if (user.isPremium) {
if (inventory.stock > 0) {
if (order.quantity < inventory.stock) {
if (user.paymentMethod === 'card') {
// ... 10 more nested conditions
}
}
}
}
}
// Complexity: 20+, impossible to test
}
✅ Good: Extracted functions (complexity < 10 each)
function processOrder(order, user, inventory) {
validateOrder(order);
checkInventory(order, inventory);
processPayment(order, user);
updateStock(order, inventory);
}
function validateOrder(order) {
if (!order.type || !order.quantity) {
throw new ValidationError("Invalid order");
}
}
// Each function: complexity 3-5, easily testable
Review Questions:
Common Smells to Flag:
❌ Duplicate Code: Same logic repeated in multiple places
// In component A
const total = items.reduce((sum, item) => sum + item.price * item.quantity, 0);
// In component B
const total = items.reduce((sum, item) => sum + item.price * item.quantity, 0);
✅ Good: Extracted to shared utility
// utils/cart.ts
export function calculateTotal(items) {
return items.reduce((sum, item) => sum + item.price * item.quantity, 0);
}
❌ Magic Numbers: Unexplained constants
if (user.age > 65) { ... } // Why 65?
setTimeout(pollAPI, 5000); // Why 5 seconds?
✅ Good: Named constants
const RETIREMENT_AGE = 65;
const API_POLL_INTERVAL_MS = 5000;
if (user.age > RETIREMENT_AGE) { ... }
setTimeout(pollAPI, API_POLL_INTERVAL_MS);
❌ Long Parameter Lists: > 4 parameters is a smell
function createUser(name, email, password, age, country, city, zipCode) { ... }
✅ Good: Object parameter
function createUser({ name, email, password, profile }: CreateUserParams) { ... }
❌ God Objects/Functions: Classes/functions doing too much
class UserService {
authenticate() { ... }
sendEmail() { ... }
processPayment() { ... }
generateReport() { ... } // Too many responsibilities
}
✅ Good: Single Responsibility Principle
class AuthService { authenticate() { ... } }
class EmailService { send() { ... } }
class PaymentService { process() { ... } }
class ReportService { generate() { ... } }
Variables and Functions:
isValid, hasPermission, canEditgetUserById, calculateTotal, validateEmail❌ Bad: Vague names
const d = new Date(); // What does 'd' mean?
function proc(x) { ... } // What is proc? What is x?
const temp = fetchData(); // Temporary for what?
✅ Good: Descriptive names
const currentDate = new Date();
function processPayment(order) { ... }
const userData = await fetchUserProfile();
Single Responsibility: Each class/function should have one reason to change
Open/Closed: Open for extension, closed for modification (use composition/inheritance)
Liskov Substitution: Subtypes should be substitutable for base types
Interface Segregation: Many specific interfaces > one general interface
Dependency Inversion: Depend on abstractions, not concretions
Review Questions:
See examples/code-quality-examples.md for 15-20 refactoring examples.
Look for performance issues that could cause production problems under load.
N+1 Query Problem: Most common performance issue
❌ Bad: N+1 queries (1 for posts + N for authors)
const posts = await db.query('SELECT * FROM posts');
for (const post of posts) {
post.author = await db.query('SELECT * FROM users WHERE id = ?', [post.authorId]);
// This executes N additional queries!
}
✅ Good: Single query with JOIN
const posts = await db.query(`
SELECT p.*, u.name as authorName, u.email as authorEmail
FROM posts p
JOIN users u ON p.authorId = u.id
`);
Missing Indexes: Check for queries on unindexed columns
❌ Bad: Full table scan
SELECT * FROM orders WHERE customer_email = 'user@example.com';
-- If email isn't indexed, this scans all rows
✅ Good: Indexed column
CREATE INDEX idx_orders_email ON orders(customer_email);
SELECT * FROM orders WHERE customer_email = 'user@example.com';
Large Result Sets: Always paginate
❌ Bad: Returning all results
const allUsers = await db.query('SELECT * FROM users'); // Could be millions!
✅ Good: Pagination
const users = await db.query(
'SELECT * FROM users LIMIT ? OFFSET ?',
[pageSize, page * pageSize]
);
See references/performance-review.md for comprehensive database optimization patterns.
Unnecessary Re-renders:
❌ Bad: Creating new objects in render
function Component() {
const options = { key: 'value' }; // New object every render!
return <Child options={options} />; // Causes Child to re-render
}
✅ Good: Memoization
function Component() {
const options = useMemo(() => ({ key: 'value' }), []);
return <Child options={options} />;
}
Bundle Size: Check for large dependencies
❌ Bad: Importing entire libraries
import _ from 'lodash'; // 70KB+ for one function
import moment from 'moment'; // 68KB for date formatting
✅ Good: Tree-shakeable imports
import debounce from 'lodash/debounce'; // 2KB
import { formatDistance } from 'date-fns'; // 2-10KB
Efficient Data Transfer:
❌ Bad: No pagination, overfetching
app.get('/api/products', async (req, res) => {
const products = await db.getAllProducts(); // All 10,000 products!
res.json(products);
});
✅ Good: Pagination + selective fields
app.get('/api/products', async (req, res) => {
const { page = 1, limit = 20, fields = 'id,name,price' } = req.query;
const products = await db.getProducts({ page, limit, fields });
res.json(products);
});
Review Questions:
Code review should verify comprehensive test coverage.
Minimum Coverage Targets:
Coverage is Necessary but Not Sufficient: 100% coverage doesn't guarantee bug-free code. Test quality matters more than quantity.
Good tests follow Arrange, Act, Assert:
❌ Bad: Testing implementation details
test('counter increments', () => {
const counter = new Counter();
counter.value = 5; // Directly setting internal state
expect(counter.value).toBe(5); // Testing implementation, not behavior
});
✅ Good: Testing behavior
test('counter increments when button clicked', () => {
// Arrange
const counter = new Counter();
// Act
counter.increment();
counter.increment();
// Assert
expect(counter.getValue()).toBe(2);
});
❌ Bad: Multiple assertions without clear meaning
test('user creation', async () => {
const user = await createUser({ name: 'John' });
expect(user).toBeDefined();
expect(user.id).toBeTruthy();
expect(user.name).toBe('John');
expect(user.createdAt).toBeTruthy();
// What is actually being tested?
});
✅ Good: Focused tests with clear intent
test('createUser assigns unique ID to new user', async () => {
const user = await createUser({ name: 'John' });
expect(user.id).toMatch(/^[a-f0-9-]{36}$/); // UUID format
});
test('createUser sets createdAt timestamp', async () => {
const before = Date.now();
const user = await createUser({ name: 'John' });
const after = Date.now();
expect(user.createdAt).toBeGreaterThanOrEqual(before);
expect(user.createdAt).toBeLessThanOrEqual(after);
});
When to Require Each Type:
Unit Tests (always required):
Integration Tests (required for):
E2E Tests (required for):
Review Questions:
See references/test-review.md for comprehensive testing guidance.
Documentation should explain why, not just what.
Always Document:
Optional Documentation:
Function/Method Documentation:
❌ Bad: Redundant comments
// Gets the user by ID
function getUserById(id) { ... }
✅ Good: Meaningful documentation
/**
* Fetches user profile with eager-loaded preferences.
*
* @param id - User UUID
* @returns User with preferences, or null if not found
* @throws DatabaseError if connection fails
*
* Note: This function caches results for 5 minutes to reduce DB load.
* Use getUserByIdFresh() if you need latest data.
*/
async function getUserById(id: string): Promise<User | null> { ... }
Complex Logic Documentation:
❌ Bad: No explanation
const price = base * (1 + tax) * (1 - discount) + shipping;
✅ Good: Explained calculation
// Calculate final price including tax (additive), discount (multiplicative), and shipping
// Formula: (base × (1 + tax%) × (1 - discount%)) + shipping
// Example: $100 × 1.08 × 0.90 + $5 = $102.20
const price = base * (1 + tax) * (1 - discount) + shipping;
Use appropriate format for context:
Review Questions:
Be Constructive:
Ask Questions:
Explain Reasoning:
Acknowledge Good Work:
See examples/review-comments.md for template review comments.
Security:
Code Quality:
Performance:
Testing:
✅ Approve:
🔄 Request Changes:
💬 Comment Only (no approval/rejection):
testing-strategy - Test pyramid and comprehensive testing patternsgithub-workflow-best-practices - PR workflow and review processagency-workflow-patterns - Multi-agent code review coordination (Reality Checker agent)This skill should be used when the user asks to "create an agent", "add an agent", "write a subagent", "agent frontmatter", "when to use description", "agent examples", "agent tools", "agent colors", "autonomous agent", or needs guidance on agent structure, system prompts, triggering conditions, or agent development best practices for Claude Code plugins.
This skill should be used when the user asks to "create a slash command", "add a command", "write a custom command", "define command arguments", "use command frontmatter", "organize commands", "create command with file references", "interactive command", "use AskUserQuestion in command", or needs guidance on slash command structure, YAML frontmatter fields, dynamic arguments, bash execution in commands, user interaction patterns, or command development best practices for Claude Code.
This skill should be used when the user asks to "create a hook", "add a PreToolUse/PostToolUse/Stop hook", "validate tool use", "implement prompt-based hooks", "use ${CLAUDE_PLUGIN_ROOT}", "set up event-driven automation", "block dangerous commands", or mentions hook events (PreToolUse, PostToolUse, Stop, SubagentStop, SessionStart, SessionEnd, UserPromptSubmit, PreCompact, Notification). Provides comprehensive guidance for creating and implementing Claude Code plugin hooks with focus on advanced prompt-based hooks API.