Reorganize: Move all skills to skills/ folder
- Created skills/ directory - Moved 272 skills to skills/ subfolder - Kept agents/ at root level - Kept installation scripts and docs at root level Repository structure: - skills/ - All 272 skills from skills.sh - agents/ - Agent definitions - *.sh, *.ps1 - Installation scripts - README.md, etc. - Documentation Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
444
skills/code-review-checklist/skill.md
Normal file
444
skills/code-review-checklist/skill.md
Normal file
@@ -0,0 +1,444 @@
|
||||
---
|
||||
name: code-review-checklist
|
||||
description: "Comprehensive checklist for conducting thorough code reviews covering functionality, security, performance, and maintainability"
|
||||
---
|
||||
|
||||
# Code Review Checklist
|
||||
|
||||
## Overview
|
||||
|
||||
Provide a systematic checklist for conducting thorough code reviews. This skill helps reviewers ensure code quality, catch bugs, identify security issues, and maintain consistency across the codebase.
|
||||
|
||||
## When to Use This Skill
|
||||
|
||||
- Use when reviewing pull requests
|
||||
- Use when conducting code audits
|
||||
- Use when establishing code review standards for a team
|
||||
- Use when training new developers on code review practices
|
||||
- Use when you want to ensure nothing is missed in reviews
|
||||
- Use when creating code review documentation
|
||||
|
||||
## How It Works
|
||||
|
||||
### Step 1: Understand the Context
|
||||
|
||||
Before reviewing code, I'll help you understand:
|
||||
- What problem does this code solve?
|
||||
- What are the requirements?
|
||||
- What files were changed and why?
|
||||
- Are there related issues or tickets?
|
||||
- What's the testing strategy?
|
||||
|
||||
### Step 2: Review Functionality
|
||||
|
||||
Check if the code works correctly:
|
||||
- Does it solve the stated problem?
|
||||
- Are edge cases handled?
|
||||
- Is error handling appropriate?
|
||||
- Are there any logical errors?
|
||||
- Does it match the requirements?
|
||||
|
||||
### Step 3: Review Code Quality
|
||||
|
||||
Assess code maintainability:
|
||||
- Is the code readable and clear?
|
||||
- Are names descriptive?
|
||||
- Is it properly structured?
|
||||
- Are functions/methods focused?
|
||||
- Is there unnecessary complexity?
|
||||
|
||||
### Step 4: Review Security
|
||||
|
||||
Check for security issues:
|
||||
- Are inputs validated?
|
||||
- Is sensitive data protected?
|
||||
- Are there SQL injection risks?
|
||||
- Is authentication/authorization correct?
|
||||
- Are dependencies secure?
|
||||
|
||||
### Step 5: Review Performance
|
||||
|
||||
Look for performance issues:
|
||||
- Are there unnecessary loops?
|
||||
- Is database access optimized?
|
||||
- Are there memory leaks?
|
||||
- Is caching used appropriately?
|
||||
- Are there N+1 query problems?
|
||||
|
||||
### Step 6: Review Tests
|
||||
|
||||
Verify test coverage:
|
||||
- Are there tests for new code?
|
||||
- Do tests cover edge cases?
|
||||
- Are tests meaningful?
|
||||
- Do all tests pass?
|
||||
- Is test coverage adequate?
|
||||
|
||||
## Examples
|
||||
|
||||
### Example 1: Functionality Review Checklist
|
||||
|
||||
```markdown
|
||||
## Functionality Review
|
||||
|
||||
### Requirements
|
||||
- [ ] Code solves the stated problem
|
||||
- [ ] All acceptance criteria are met
|
||||
- [ ] Edge cases are handled
|
||||
- [ ] Error cases are handled
|
||||
- [ ] User input is validated
|
||||
|
||||
### Logic
|
||||
- [ ] No logical errors or bugs
|
||||
- [ ] Conditions are correct (no off-by-one errors)
|
||||
- [ ] Loops terminate correctly
|
||||
- [ ] Recursion has proper base cases
|
||||
- [ ] State management is correct
|
||||
|
||||
### Error Handling
|
||||
- [ ] Errors are caught appropriately
|
||||
- [ ] Error messages are clear and helpful
|
||||
- [ ] Errors don't expose sensitive information
|
||||
- [ ] Failed operations are rolled back
|
||||
- [ ] Logging is appropriate
|
||||
|
||||
### Example Issues to Catch:
|
||||
|
||||
**❌ Bad - Missing validation:**
|
||||
\`\`\`javascript
|
||||
function createUser(email, password) {
|
||||
// No validation!
|
||||
return db.users.create({ email, password });
|
||||
}
|
||||
\`\`\`
|
||||
|
||||
**✅ Good - Proper validation:**
|
||||
\`\`\`javascript
|
||||
function createUser(email, password) {
|
||||
if (!email || !isValidEmail(email)) {
|
||||
throw new Error('Invalid email address');
|
||||
}
|
||||
if (!password || password.length < 8) {
|
||||
throw new Error('Password must be at least 8 characters');
|
||||
}
|
||||
return db.users.create({ email, password });
|
||||
}
|
||||
\`\`\`
|
||||
```
|
||||
|
||||
### Example 2: Security Review Checklist
|
||||
|
||||
```markdown
|
||||
## Security Review
|
||||
|
||||
### Input Validation
|
||||
- [ ] All user inputs are validated
|
||||
- [ ] SQL injection is prevented (use parameterized queries)
|
||||
- [ ] XSS is prevented (escape output)
|
||||
- [ ] CSRF protection is in place
|
||||
- [ ] File uploads are validated (type, size, content)
|
||||
|
||||
### Authentication & Authorization
|
||||
- [ ] Authentication is required where needed
|
||||
- [ ] Authorization checks are present
|
||||
- [ ] Passwords are hashed (never stored plain text)
|
||||
- [ ] Sessions are managed securely
|
||||
- [ ] Tokens expire appropriately
|
||||
|
||||
### Data Protection
|
||||
- [ ] Sensitive data is encrypted
|
||||
- [ ] API keys are not hardcoded
|
||||
- [ ] Environment variables are used for secrets
|
||||
- [ ] Personal data follows privacy regulations
|
||||
- [ ] Database credentials are secure
|
||||
|
||||
### Dependencies
|
||||
- [ ] No known vulnerable dependencies
|
||||
- [ ] Dependencies are up to date
|
||||
- [ ] Unnecessary dependencies are removed
|
||||
- [ ] Dependency versions are pinned
|
||||
|
||||
### Example Issues to Catch:
|
||||
|
||||
**❌ Bad - SQL injection risk:**
|
||||
\`\`\`javascript
|
||||
const query = \`SELECT * FROM users WHERE email = '\${email}'\`;
|
||||
db.query(query);
|
||||
\`\`\`
|
||||
|
||||
**✅ Good - Parameterized query:**
|
||||
\`\`\`javascript
|
||||
const query = 'SELECT * FROM users WHERE email = $1';
|
||||
db.query(query, [email]);
|
||||
\`\`\`
|
||||
|
||||
**❌ Bad - Hardcoded secret:**
|
||||
\`\`\`javascript
|
||||
const API_KEY = 'sk_live_abc123xyz';
|
||||
\`\`\`
|
||||
|
||||
**✅ Good - Environment variable:**
|
||||
\`\`\`javascript
|
||||
const API_KEY = process.env.API_KEY;
|
||||
if (!API_KEY) {
|
||||
throw new Error('API_KEY environment variable is required');
|
||||
}
|
||||
\`\`\`
|
||||
```
|
||||
|
||||
### Example 3: Code Quality Review Checklist
|
||||
|
||||
```markdown
|
||||
## Code Quality Review
|
||||
|
||||
### Readability
|
||||
- [ ] Code is easy to understand
|
||||
- [ ] Variable names are descriptive
|
||||
- [ ] Function names explain what they do
|
||||
- [ ] Complex logic has comments
|
||||
- [ ] Magic numbers are replaced with constants
|
||||
|
||||
### Structure
|
||||
- [ ] Functions are small and focused
|
||||
- [ ] Code follows DRY principle (Don't Repeat Yourself)
|
||||
- [ ] Proper separation of concerns
|
||||
- [ ] Consistent code style
|
||||
- [ ] No dead code or commented-out code
|
||||
|
||||
### Maintainability
|
||||
- [ ] Code is modular and reusable
|
||||
- [ ] Dependencies are minimal
|
||||
- [ ] Changes are backwards compatible
|
||||
- [ ] Breaking changes are documented
|
||||
- [ ] Technical debt is noted
|
||||
|
||||
### Example Issues to Catch:
|
||||
|
||||
**❌ Bad - Unclear naming:**
|
||||
\`\`\`javascript
|
||||
function calc(a, b, c) {
|
||||
return a * b + c;
|
||||
}
|
||||
\`\`\`
|
||||
|
||||
**✅ Good - Descriptive naming:**
|
||||
\`\`\`javascript
|
||||
function calculateTotalPrice(quantity, unitPrice, tax) {
|
||||
return quantity * unitPrice + tax;
|
||||
}
|
||||
\`\`\`
|
||||
|
||||
**❌ Bad - Function doing too much:**
|
||||
\`\`\`javascript
|
||||
function processOrder(order) {
|
||||
// Validate order
|
||||
if (!order.items) throw new Error('No items');
|
||||
|
||||
// Calculate total
|
||||
let total = 0;
|
||||
for (let item of order.items) {
|
||||
total += item.price * item.quantity;
|
||||
}
|
||||
|
||||
// Apply discount
|
||||
if (order.coupon) {
|
||||
total *= 0.9;
|
||||
}
|
||||
|
||||
// Process payment
|
||||
const payment = stripe.charge(total);
|
||||
|
||||
// Send email
|
||||
sendEmail(order.email, 'Order confirmed');
|
||||
|
||||
// Update inventory
|
||||
updateInventory(order.items);
|
||||
|
||||
return { orderId: order.id, total };
|
||||
}
|
||||
\`\`\`
|
||||
|
||||
**✅ Good - Separated concerns:**
|
||||
\`\`\`javascript
|
||||
function processOrder(order) {
|
||||
validateOrder(order);
|
||||
const total = calculateOrderTotal(order);
|
||||
const payment = processPayment(total);
|
||||
sendOrderConfirmation(order.email);
|
||||
updateInventory(order.items);
|
||||
|
||||
return { orderId: order.id, total };
|
||||
}
|
||||
\`\`\`
|
||||
```
|
||||
|
||||
## Best Practices
|
||||
|
||||
### ✅ Do This
|
||||
|
||||
- **Review Small Changes** - Smaller PRs are easier to review thoroughly
|
||||
- **Check Tests First** - Verify tests pass and cover new code
|
||||
- **Run the Code** - Test it locally when possible
|
||||
- **Ask Questions** - Don't assume, ask for clarification
|
||||
- **Be Constructive** - Suggest improvements, don't just criticize
|
||||
- **Focus on Important Issues** - Don't nitpick minor style issues
|
||||
- **Use Automated Tools** - Linters, formatters, security scanners
|
||||
- **Review Documentation** - Check if docs are updated
|
||||
- **Consider Performance** - Think about scale and efficiency
|
||||
- **Check for Regressions** - Ensure existing functionality still works
|
||||
|
||||
### ❌ Don't Do This
|
||||
|
||||
- **Don't Approve Without Reading** - Actually review the code
|
||||
- **Don't Be Vague** - Provide specific feedback with examples
|
||||
- **Don't Ignore Security** - Security issues are critical
|
||||
- **Don't Skip Tests** - Untested code will cause problems
|
||||
- **Don't Be Rude** - Be respectful and professional
|
||||
- **Don't Rubber Stamp** - Every review should add value
|
||||
- **Don't Review When Tired** - You'll miss important issues
|
||||
- **Don't Forget Context** - Understand the bigger picture
|
||||
|
||||
## Complete Review Checklist
|
||||
|
||||
### Pre-Review
|
||||
- [ ] Read the PR description and linked issues
|
||||
- [ ] Understand what problem is being solved
|
||||
- [ ] Check if tests pass in CI/CD
|
||||
- [ ] Pull the branch and run it locally
|
||||
|
||||
### Functionality
|
||||
- [ ] Code solves the stated problem
|
||||
- [ ] Edge cases are handled
|
||||
- [ ] Error handling is appropriate
|
||||
- [ ] User input is validated
|
||||
- [ ] No logical errors
|
||||
|
||||
### Security
|
||||
- [ ] No SQL injection vulnerabilities
|
||||
- [ ] No XSS vulnerabilities
|
||||
- [ ] Authentication/authorization is correct
|
||||
- [ ] Sensitive data is protected
|
||||
- [ ] No hardcoded secrets
|
||||
|
||||
### Performance
|
||||
- [ ] No unnecessary database queries
|
||||
- [ ] No N+1 query problems
|
||||
- [ ] Efficient algorithms used
|
||||
- [ ] No memory leaks
|
||||
- [ ] Caching used appropriately
|
||||
|
||||
### Code Quality
|
||||
- [ ] Code is readable and clear
|
||||
- [ ] Names are descriptive
|
||||
- [ ] Functions are focused and small
|
||||
- [ ] No code duplication
|
||||
- [ ] Follows project conventions
|
||||
|
||||
### Tests
|
||||
- [ ] New code has tests
|
||||
- [ ] Tests cover edge cases
|
||||
- [ ] Tests are meaningful
|
||||
- [ ] All tests pass
|
||||
- [ ] Test coverage is adequate
|
||||
|
||||
### Documentation
|
||||
- [ ] Code comments explain why, not what
|
||||
- [ ] API documentation is updated
|
||||
- [ ] README is updated if needed
|
||||
- [ ] Breaking changes are documented
|
||||
- [ ] Migration guide provided if needed
|
||||
|
||||
### Git
|
||||
- [ ] Commit messages are clear
|
||||
- [ ] No merge conflicts
|
||||
- [ ] Branch is up to date with main
|
||||
- [ ] No unnecessary files committed
|
||||
- [ ] .gitignore is properly configured
|
||||
|
||||
## Common Pitfalls
|
||||
|
||||
### Problem: Missing Edge Cases
|
||||
**Symptoms:** Code works for happy path but fails on edge cases
|
||||
**Solution:** Ask "What if...?" questions
|
||||
- What if the input is null?
|
||||
- What if the array is empty?
|
||||
- What if the user is not authenticated?
|
||||
- What if the network request fails?
|
||||
|
||||
### Problem: Security Vulnerabilities
|
||||
**Symptoms:** Code exposes security risks
|
||||
**Solution:** Use security checklist
|
||||
- Run security scanners (npm audit, Snyk)
|
||||
- Check OWASP Top 10
|
||||
- Validate all inputs
|
||||
- Use parameterized queries
|
||||
- Never trust user input
|
||||
|
||||
### Problem: Poor Test Coverage
|
||||
**Symptoms:** New code has no tests or inadequate tests
|
||||
**Solution:** Require tests for all new code
|
||||
- Unit tests for functions
|
||||
- Integration tests for features
|
||||
- Edge case tests
|
||||
- Error case tests
|
||||
|
||||
### Problem: Unclear Code
|
||||
**Symptoms:** Reviewer can't understand what code does
|
||||
**Solution:** Request improvements
|
||||
- Better variable names
|
||||
- Explanatory comments
|
||||
- Smaller functions
|
||||
- Clear structure
|
||||
|
||||
## Review Comment Templates
|
||||
|
||||
### Requesting Changes
|
||||
```markdown
|
||||
**Issue:** [Describe the problem]
|
||||
|
||||
**Current code:**
|
||||
\`\`\`javascript
|
||||
// Show problematic code
|
||||
\`\`\`
|
||||
|
||||
**Suggested fix:**
|
||||
\`\`\`javascript
|
||||
// Show improved code
|
||||
\`\`\`
|
||||
|
||||
**Why:** [Explain why this is better]
|
||||
```
|
||||
|
||||
### Asking Questions
|
||||
```markdown
|
||||
**Question:** [Your question]
|
||||
|
||||
**Context:** [Why you're asking]
|
||||
|
||||
**Suggestion:** [If you have one]
|
||||
```
|
||||
|
||||
### Praising Good Code
|
||||
```markdown
|
||||
**Nice!** [What you liked]
|
||||
|
||||
This is great because [explain why]
|
||||
```
|
||||
|
||||
## Related Skills
|
||||
|
||||
- `@requesting-code-review` - Prepare code for review
|
||||
- `@receiving-code-review` - Handle review feedback
|
||||
- `@systematic-debugging` - Debug issues found in review
|
||||
- `@test-driven-development` - Ensure code has tests
|
||||
|
||||
## Additional Resources
|
||||
|
||||
- [Google Code Review Guidelines](https://google.github.io/eng-practices/review/)
|
||||
- [OWASP Top 10](https://owasp.org/www-project-top-ten/)
|
||||
- [Code Review Best Practices](https://github.com/thoughtbot/guides/tree/main/code-review)
|
||||
- [How to Review Code](https://www.kevinlondon.com/2015/05/05/code-review-best-practices.html)
|
||||
|
||||
---
|
||||
|
||||
**Pro Tip:** Use a checklist template for every review to ensure consistency and thoroughness. Customize it for your team's specific needs!
|
||||
Reference in New Issue
Block a user