// Comprehensive code review framework for evaluating code quality, security, performance, and maintainability
| name | code-review |
| version | 1.0.0 |
| description | Comprehensive code review framework for evaluating code quality, security, performance, and maintainability |
A systematic approach to reviewing code changes with focus on quality, security, performance, and maintainability.
Use this skill when:
Goal: Understand what changed and why
Steps:
Context Checklist:
CONTEXT:
- Issue/PR: [link or number]
- Author: [who made the changes]
- Scope: [files/directories affected]
- Purpose: [what problem it solves]
- Dependencies: [any related PRs or changes]
Questions to Answer:
Goal: Examine actual code changes
Steps:
Diff Review Template:
## Files Changed
### Critical Files
- `file1:line` - [concern or observation]
- `file2:line` - [concern or observation]
### Supporting Files
- `file3:line` - [concern or observation]
### Test Files
- `test_file:line` - [concern or observation]
## Observations
1. [observation with file:line reference]
2. [observation with file:line reference]
Categories to Evaluate:
CORRECTNESS:
- [ ] Logic is correct
- [ ] Handles edge cases
- [ ] Error handling is appropriate
- [ ] No off-by-one errors
- [ ] Null/None handling is proper
- [ ] Type conversions are safe
ISSUES:
- `file:line` - [description of issue]
SECURITY:
- [ ] No SQL injection vulnerabilities
- [ ] Input validation is proper
- [ ] No hardcoded secrets
- [ ] Authentication/authorization checked
- [ ] No XSS vulnerabilities (if web)
- [ ] Sensitive data is protected
- [ ] Rate limiting considered
- [ ] Dependencies are safe
ISSUES:
- `file:line` - [description of security concern]
Security Patterns to Check:
// ❌ Bad: SQL injection risk
let query = format!("SELECT * FROM users WHERE id = {}", user_input);
// ✅ Good: Parameterized query
let query = "SELECT * FROM users WHERE id = ?1";
conn.query_row(query, params![user_input], |row| { ... });
// ❌ Bad: Hardcoded secret
let api_key = "sk-abc123...";
// ✅ Good: Environment variable
let api_key = std::env::var("API_KEY")?;
// ❌ Bad: No input validation
fn process_input(input: &str) { ... }
// ✅ Good: Validated input
fn process_input(input: &str) -> Result<()> {
if input.len() > MAX_LENGTH {
return Err(Error::InputTooLong);
}
// Additional validation...
Ok(())
}
PERFORMANCE:
- [ ] No N+1 queries
- [ ] Efficient algorithms
- [ ] Proper use of caching
- [ ] No unnecessary allocations
- [ ] Async operations used correctly
- [ ] Database queries optimized
ISSUES:
- `file:line` - [description of performance concern]
Performance Patterns:
// ❌ Bad: N+1 query problem
for user in users {
let posts = get_posts_for_user(user.id).await?; // N queries!
}
// ✅ Good: Batch query
let user_ids: Vec<i64> = users.iter().map(|u| u.id).collect();
let posts = get_posts_for_users(&user_ids).await?; // 1 query
// ❌ Bad: Unnecessary clone
let data = large_vec.clone();
process_data(&data);
// ✅ Good: Reference when possible
process_data(&large_vec);
// ❌ Bad: Inefficient string concatenation
let mut s = String::new();
for part in parts {
s += part; // O(n²) complexity
}
// ✅ Good: Use join or with_capacity
let s = parts.join("");
// or
let mut s = String::with_capacity(estimated_size);
for part in parts {
s.push_str(part);
}
MAINTAINABILITY:
- [ ] Code is readable
- [ ] Functions are focused
- [ ] Names are descriptive
- [ ] No code duplication
- [ ] Proper abstraction levels
- [ ] Easy to modify
- [ ] Follows project conventions
ISSUES:
- `file:line` - [description of maintainability concern]
Maintainability Patterns:
// ❌ Bad: Magic numbers
if status == 200 { ... }
// ✅ Good: Named constants
const HTTP_OK: u16 = 200;
if status == HTTP_OK { ... }
// ❌ Bad: Long function doing multiple things
fn process_order(order: Order) -> Result<()> {
// Validate order (20 lines)
// Calculate pricing (30 lines)
// Update inventory (25 lines)
// Send notifications (15 lines)
// Log everything (10 lines)
}
// ✅ Good: Focused functions
fn process_order(order: Order) -> Result<()> {
validate_order(&order)?;
let total = calculate_total(&order)?;
update_inventory(&order)?;
send_notifications(&order)?;
log_order(&order, total)?;
Ok(())
}
// ❌ Bad: Duplicated logic
fn process_user(u: &User) { /* duplicate validation */ }
fn process_admin(u: &Admin) { /* duplicate validation */ }
// ✅ Good: Extracted common logic
trait Validatable {
fn validate(&self) -> Result<()>;
}
fn process_entity<T: Validatable>(entity: &T) -> Result<()> {
entity.validate()?;
// ...
}
TESTING:
- [ ] New code is tested
- [ ] Tests are meaningful
- [ ] Edge cases covered
- [ ] Tests are maintainable
- [ ] Mocks used appropriately
- [ ] Test names are descriptive
ISSUES:
- `file:line` - [description of testing concern]
Testing Patterns:
// ❌ Bad: Vague test name
#[test]
fn test_process() { ... }
// ✅ Good: Descriptive test name
#[test]
fn test_process_returns_error_when_input_is_empty() { ... }
// ❌ Bad: Testing implementation details
#[test]
fn test_internal_helper_function() { ... }
// ✅ Good: Testing public behavior
#[test]
fn test_public_api_returns_expected_result() { ... }
// ❌ Bad: Not testing edge cases
#[test]
fn test_happy_path() { ... }
// ✅ Good: Comprehensive coverage
#[test]
fn test_happy_path() { ... }
#[test]
fn test_empty_input() { ... }
#[test]
fn test_max_length_input() { ... }
#[test]
fn test_invalid_input_returns_error() { ... }
Goal: Communicate findings clearly and constructively
Feedback Categories:
Feedback Template:
## Review Summary
**Overall**: [Approve | Request Changes | Comment]
**Confidence**: [High | Medium | Low]
### 🔴 Must Fix
1. `file:line` - [issue description]
**Why**: [explanation]
**Suggestion**: [how to fix]
### 🟡 Should Fix
1. `file:line` - [issue description]
**Why**: [explanation]
**Suggestion**: [how to fix]
### 🟢 Suggestions
1. `file:line` - [improvement suggestion]
**Benefit**: [what it improves]
### 💬 Questions
1. `file:line` - [question]
**Context**: [why you're asking]
### ✅ What's Good
- [positive observation 1]
- [positive observation 2]
Writing Good Feedback:
# ❌ Bad: Vague and unhelpful
"This code is wrong."
# ✅ Good: Specific and actionable
"file:42 - This function doesn't handle the case where `users` is empty.
ACTUAL: Panics with index out of bounds
EXPECTED: Should return an empty vec or error
SUGGESTION: Add check: `if users.is_empty() { return Ok(vec![]); }`"
## Security Review
### Input Validation
- [ ] All user inputs are validated
- [ ] Input length limits enforced
- [ ] Input type checking
- [ ] Input sanitization for display
### Authentication & Authorization
- [ ] Auth checks are present
- [ ] Auth checks are correct
- [ ] Role-based access control verified
- [ ] Session management is secure
### Data Protection
- [ ] Sensitive data encrypted at rest
- [ ] Sensitive data encrypted in transit
- [ ] No secrets in logs
- [ ] No secrets in error messages
### Injection Prevention
- [ ] SQL queries use parameterized statements
- [ ] No command injection
- [ ] No path traversal vulnerabilities
- [ ] Proper output encoding
### Dependencies
- [ ] New dependencies are necessary
- [ ] New dependencies are trusted
- [ ] No known vulnerabilities in dependencies
## Performance Review
### Database
- [ ] Queries are optimized
- [ ] Indexes used appropriately
- [ ] N+1 queries avoided
- [ ] Transactions used correctly
### Memory
- [ ] No memory leaks
- [ ] Large allocations justified
- [ ] References used instead of clones
- [ ] Buffers sized appropriately
### Concurrency
- [ ] Async used correctly
- [ ] No race conditions
- [ ] Locks are necessary and minimal
- [ ] Deadlock potential analyzed
### Caching
- [ ] Caching strategy is appropriate
- [ ] Cache invalidation is correct
- [ ] Cache keys are well-designed
## API Design Review
### Consistency
- [ ] Naming follows conventions
- [ ] Parameter ordering is consistent
- [ ] Response format is consistent
- [ ] Error format is consistent
### Documentation
- [ ] All endpoints documented
- [ ] Parameters documented
- [ ] Response schema documented
- [ ] Error codes documented
### Versioning
- [ ] Breaking changes handled correctly
- [ ] Version in URL or header
- [ ] Backwards compatibility maintained
### Error Handling
- [ ] Errors are informative
- [ ] Errors are actionable
- [ ] Error codes are consistent
- [ ] HTTP status codes are correct
# Run before committing
cargo fmt -- --check
cargo clippy -- -D warnings
cargo test
# Review your own changes
git diff
# Fetch PR branch
git fetch origin pull/123/head:pr-123
git checkout pr-123
# Run checks
cargo test
cargo clippy
# Review specific files
git diff main...HEAD -- path/to/file
Ensure CI runs:
❌ Don't:
✅ Do:
This skill integrates with Kilo's workflow:
file:line referencesUse with other skills:
Standards and workflows for creating and maintaining high-quality documentation
Manage Git worktrees for parallel development workflows without switching branches
Core Kilo agent configuration with comprehensive coding capabilities and workflow management
A systematic 4-phase debugging framework for reproducing, analyzing, fixing, and verifying bugs
Template for creating new skills - customize and rename for your use case
Enforce Red-Green-Refactor TDD workflow for writing tests before implementation