From 90cf84b8bb61bb77a2c1809cbbd86029c0587fdc Mon Sep 17 00:00:00 2001 From: Mohammad Faiz Date: Thu, 22 Jan 2026 17:01:49 +0530 Subject: [PATCH] Add files via upload --- skills/code-review-checklist/SKILL.md | 489 ++++++++++++++++++++++---- 1 file changed, 412 insertions(+), 77 deletions(-) diff --git a/skills/code-review-checklist/SKILL.md b/skills/code-review-checklist/SKILL.md index 3b20b610..a00a47b5 100644 --- a/skills/code-review-checklist/SKILL.md +++ b/skills/code-review-checklist/SKILL.md @@ -1,109 +1,444 @@ --- name: code-review-checklist -description: Code review guidelines covering code quality, security, and best practices. -allowed-tools: Read, Glob, Grep +description: "Comprehensive checklist for conducting thorough code reviews covering functionality, security, performance, and maintainability" --- # Code Review Checklist -## Quick Review Checklist +## Overview -### Correctness -- [ ] Code does what it's supposed to do -- [ ] Edge cases handled -- [ ] Error handling in place -- [ ] No obvious bugs +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 -- [ ] Input validated and sanitized -- [ ] No SQL/NoSQL injection vulnerabilities -- [ ] No XSS or CSRF vulnerabilities -- [ ] No hardcoded secrets or sensitive credentials -- [ ] **AI-Specific:** Protection against Prompt Injection (if applicable) -- [ ] **AI-Specific:** Outputs are sanitized before being used in critical sinks +- [ ] No SQL injection vulnerabilities +- [ ] No XSS vulnerabilities +- [ ] Authentication/authorization is correct +- [ ] Sensitive data is protected +- [ ] No hardcoded secrets ### Performance -- [ ] No N+1 queries -- [ ] No unnecessary loops -- [ ] Appropriate caching -- [ ] Bundle size impact considered +- [ ] No unnecessary database queries +- [ ] No N+1 query problems +- [ ] Efficient algorithms used +- [ ] No memory leaks +- [ ] Caching used appropriately ### Code Quality -- [ ] Clear naming -- [ ] DRY - no duplicate code -- [ ] SOLID principles followed -- [ ] Appropriate abstraction level +- [ ] Code is readable and clear +- [ ] Names are descriptive +- [ ] Functions are focused and small +- [ ] No code duplication +- [ ] Follows project conventions -### Testing -- [ ] Unit tests for new code -- [ ] Edge cases tested -- [ ] Tests readable and maintainable +### Tests +- [ ] New code has tests +- [ ] Tests cover edge cases +- [ ] Tests are meaningful +- [ ] All tests pass +- [ ] Test coverage is adequate ### Documentation -- [ ] Complex logic commented -- [ ] Public APIs documented -- [ ] README updated if needed +- [ ] Code comments explain why, not what +- [ ] API documentation is updated +- [ ] README is updated if needed +- [ ] Breaking changes are documented +- [ ] Migration guide provided if needed -## AI & LLM Review Patterns (2025) +### Git +- [ ] Commit messages are clear +- [ ] No merge conflicts +- [ ] Branch is up to date with main +- [ ] No unnecessary files committed +- [ ] .gitignore is properly configured -### Logic & Hallucinations -- [ ] **Chain of Thought:** Does the logic follow a verifiable path? -- [ ] **Edge Cases:** Did the AI account for empty states, timeouts, and partial failures? -- [ ] **External State:** Is the code making safe assumptions about file systems or networks? +## Common Pitfalls -### Prompt Engineering Review +### 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 -// ❌ Vague prompt in code -const response = await ai.generate(userInput); +**Issue:** [Describe the problem] -// ✅ Structured & Safe prompt -const response = await ai.generate({ - system: "You are a specialized parser...", - input: sanitize(userInput), - schema: ResponseSchema -}); +**Current code:** +\`\`\`javascript +// Show problematic code +\`\`\` + +**Suggested fix:** +\`\`\`javascript +// Show improved code +\`\`\` + +**Why:** [Explain why this is better] ``` -## Anti-Patterns to Flag +### Asking Questions +```markdown +**Question:** [Your question] -```typescript -// ❌ Magic numbers -if (status === 3) { ... } +**Context:** [Why you're asking] -// ✅ Named constants -if (status === Status.ACTIVE) { ... } - -// ❌ Deep nesting -if (a) { if (b) { if (c) { ... } } } - -// ✅ Early returns -if (!a) return; -if (!b) return; -if (!c) return; -// do work - -// ❌ Long functions (100+ lines) -// ✅ Small, focused functions - -// ❌ any type -const data: any = ... - -// ✅ Proper types -const data: UserData = ... +**Suggestion:** [If you have one] ``` -## Review Comments Guide +### Praising Good Code +```markdown +**Nice!** [What you liked] +This is great because [explain why] ``` -// Blocking issues use 🔴 -🔴 BLOCKING: SQL injection vulnerability here -// Important suggestions use 🟡 -🟡 SUGGESTION: Consider using useMemo for performance +## Related Skills -// Minor nits use 🟢 -🟢 NIT: Prefer const over let for immutable variable +- `@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 -// Questions use ❓ -❓ QUESTION: What happens if user is null here? -``` +## 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!