{"skill":{"slug":"code-review","displayName":"Code Review","summary":"Systematic code review patterns covering security, performance, maintainability, correctness, and testing — with severity levels, structured feedback guidance, review process, and anti-patterns to avoid. Use when reviewing PRs, establishing review standards, or improving review quality.","description":"---\nname: code-review\nmodel: reasoning\ncategory: testing\ndescription: Systematic code review patterns covering security, performance, maintainability, correctness, and testing — with severity levels, structured feedback guidance, review process, and anti-patterns to avoid. Use when reviewing PRs, establishing review standards, or improving review quality.\nversion: 1.0\n---\n\n# Code Review Checklist\n\nThorough, structured approach to reviewing code. Work through each dimension systematically rather than scanning randomly.\n\n\n## Installation\n\n### OpenClaw / Moltbot / Clawbot\n\n```bash\nnpx clawhub@latest install code-review\n```\n\n\n---\n\n## Review Dimensions\n\n| Dimension | Focus | Priority |\n|-----------|-------|----------|\n| Security | Vulnerabilities, auth, data exposure | Critical |\n| Performance | Speed, memory, scalability bottlenecks | High |\n| Correctness | Logic errors, edge cases, data integrity | High |\n| Maintainability | Readability, structure, future-proofing | Medium |\n| Testing | Coverage, quality, reliability of tests | Medium |\n| Accessibility | WCAG compliance, keyboard nav, screen readers | Medium |\n| Documentation | Comments, API docs, changelog entries | Low |\n\n---\n\n## Security Checklist\n\nReview every change for these vulnerabilities:\n\n- [ ] **SQL Injection** — All queries use parameterized statements or an ORM; no string concatenation with user input\n- [ ] **XSS** — User-provided content is escaped/sanitized before rendering; `dangerouslySetInnerHTML` or equivalent is justified and safe\n- [ ] **CSRF Protection** — State-changing requests require valid CSRF tokens; SameSite cookie attributes are set\n- [ ] **Authentication** — Every protected endpoint verifies the user is authenticated before processing\n- [ ] **Authorization** — Resource access is scoped to the requesting user's permissions; no IDOR vulnerabilities\n- [ ] **Input Validation** — All external input (params, headers, body, files) is validated for type, length, format, and range on the server side\n- [ ] **Secrets Management** — No API keys, passwords, tokens, or credentials in source code; secrets come from environment variables or a vault\n- [ ] **Dependency Safety** — New dependencies are from trusted sources, actively maintained, and free of known CVEs\n- [ ] **Sensitive Data** — PII, tokens, and secrets are never logged, included in error messages, or returned in API responses\n- [ ] **Rate Limiting** — Public and auth endpoints have rate limits to prevent brute-force and abuse\n- [ ] **File Upload Safety** — Uploaded files are validated for type and size, stored outside the webroot, and served with safe Content-Type headers\n- [ ] **HTTP Security Headers** — Content-Security-Policy, X-Content-Type-Options, Strict-Transport-Security are set\n\n---\n\n## Performance Checklist\n\n- [ ] **N+1 Queries** — Database access patterns are batched or joined; no loops issuing individual queries\n- [ ] **Unnecessary Re-renders** — Components only re-render when their relevant state/props change; memoization is applied where measurable\n- [ ] **Memory Leaks** — Event listeners, subscriptions, timers, and intervals are cleaned up on unmount/disposal\n- [ ] **Bundle Size** — New dependencies are tree-shakeable; large libraries are loaded dynamically; no full-library imports for a single function\n- [ ] **Lazy Loading** — Heavy components, routes, and below-the-fold content use lazy loading / code splitting\n- [ ] **Caching Strategy** — Expensive computations and API responses use appropriate caching (memoization, HTTP cache headers, Redis)\n- [ ] **Database Indexing** — Queries filter/sort on indexed columns; new queries have been checked with EXPLAIN\n- [ ] **Pagination** — List endpoints and queries use pagination or cursor-based fetching; no unbounded SELECT *\n- [ ] **Async Operations** — Long-running tasks are offloaded to background jobs or queues rather than blocking request threads\n- [ ] **Image & Asset Optimization** — Images are properly sized, use modern formats (WebP/AVIF), and leverage CDN delivery\n\n---\n\n## Correctness Checklist\n\n- [ ] **Edge Cases** — Empty arrays, empty strings, zero values, negative numbers, and maximum values are handled\n- [ ] **Null/Undefined Handling** — Nullable values are checked before access; optional chaining or guards prevent runtime errors\n- [ ] **Off-by-One Errors** — Loop bounds, array slicing, pagination offsets, and range calculations are verified\n- [ ] **Race Conditions** — Concurrent access to shared state uses locks, transactions, or atomic operations\n- [ ] **Timezone Handling** — Dates are stored in UTC; display conversion happens at the presentation layer\n- [ ] **Unicode & Encoding** — String operations handle multi-byte characters; text encoding is explicit (UTF-8)\n- [ ] **Integer Overflow / Precision** — Arithmetic on large numbers or currency uses appropriate types (BigInt, Decimal)\n- [ ] **Error Propagation** — Errors from async calls and external services are caught and handled; promises are never silently swallowed\n- [ ] **State Consistency** — Multi-step mutations are transactional; partial failures leave the system in a valid state\n- [ ] **Boundary Validation** — Values at the boundaries of valid ranges (min, max, exactly-at-limit) are tested\n\n---\n\n## Maintainability Checklist\n\n- [ ] **Naming Clarity** — Variables, functions, and classes have descriptive names that reveal intent\n- [ ] **Single Responsibility** — Each function/class/module does one thing; changes to one concern don't ripple through unrelated code\n- [ ] **DRY** — Duplicated logic is extracted into shared utilities; copy-pasted blocks are consolidated\n- [ ] **Cyclomatic Complexity** — Functions have low branching complexity; deeply nested chains are refactored\n- [ ] **Error Handling** — Errors are caught at appropriate boundaries, logged with context, and surfaced meaningfully\n- [ ] **Dead Code Removal** — Commented-out code, unused imports, unreachable branches, and obsolete feature flags are removed\n- [ ] **Magic Numbers & Strings** — Literal values are extracted into named constants with clear semantics\n- [ ] **Consistent Patterns** — New code follows the conventions already established in the codebase\n- [ ] **Function Length** — Functions are short enough to understand at a glance; long functions are decomposed\n- [ ] **Dependency Direction** — Dependencies point inward (infrastructure to domain); core logic does not import from UI or framework layers\n\n---\n\n## Testing Checklist\n\n- [ ] **Test Coverage** — New logic paths have corresponding tests; critical paths have both happy-path and failure-case tests\n- [ ] **Edge Case Tests** — Tests cover boundary values, empty inputs, nulls, and error conditions\n- [ ] **No Flaky Tests** — Tests are deterministic; no reliance on timing, external services, or shared mutable state\n- [ ] **Test Independence** — Each test sets up its own state and tears it down; test order does not affect results\n- [ ] **Meaningful Assertions** — Tests assert on behavior and outcomes, not implementation details\n- [ ] **Test Readability** — Tests follow Arrange-Act-Assert; test names describe the scenario and expected outcome\n- [ ] **Mocking Discipline** — Only external boundaries (network, DB, filesystem) are mocked\n- [ ] **Regression Tests** — Bug fixes include a test that reproduces the original bug and proves it is resolved\n\n---\n\n## Review Process\n\nWork through the code in three passes. Do not try to catch everything in one read.\n\n| Pass | Focus | Time | What to Look For |\n|------|-------|------|------------------|\n| First | High-level structure | 2-5 min | Architecture fit, file organization, API design, overall approach |\n| Second | Line-by-line detail | Bulk | Logic errors, security issues, performance problems, edge cases |\n| Third | Edge cases & hardening | 5 min | Failure modes, concurrency, boundary values, missing tests |\n\n### First Pass (2-5 minutes)\n\n1. Read the PR description and linked issue\n2. Scan the file list — does the change scope make sense?\n3. Check the overall approach — is this the right solution to the problem?\n4. Verify the change does not introduce architectural drift\n\n### Second Pass (bulk of review time)\n\n1. Read each file diff top to bottom\n2. Check every function change against the checklists above\n3. Verify error handling at every I/O boundary\n4. Flag anything that makes you pause — trust your instincts\n\n### Third Pass (5 minutes)\n\n1. Think about what could go wrong in production\n2. Check for missing tests on the code paths you flagged\n3. Verify rollback safety — can this change be reverted without data loss?\n4. Confirm documentation and changelog are updated if needed\n\n---\n\n## Severity Levels\n\nClassify every comment by severity so the author knows what blocks merge.\n\n| Level | Label | Meaning | Blocks Merge? |\n|-------|-------|---------|---------------|\n| Critical | `[CRITICAL]` | Security vulnerability, data loss, or crash in production | Yes |\n| Major | `[MAJOR]` | Bug, logic error, or significant performance regression | Yes |\n| Minor | `[MINOR]` | Improvement that would reduce future maintenance cost | No |\n| Nitpick | `[NIT]` | Style preference, naming suggestion, or trivial cleanup | No |\n\nAlways prefix your review comment with the severity label. This removes ambiguity about what matters.\n\n---\n\n## Giving Feedback\n\n### Principles\n\n- **Be specific** — Point to the exact line and explain the issue, not just \"this is wrong\"\n- **Explain why** — State the risk or consequence, not just the rule\n- **Suggest a fix** — Offer a concrete alternative or code snippet when possible\n- **Ask, don't demand** — Use questions for subjective points: \"What do you think about...?\"\n- **Acknowledge good work** — Call out clean solutions, clever optimizations, or thorough tests\n- **Separate blocking from non-blocking** — Use severity labels so the author knows what matters\n\n### Example Comments\n\n**Bad:**\n> This is wrong. Fix it.\n\n**Good:**\n> `[MAJOR]` This query interpolates user input directly into the SQL string (line 42), which is vulnerable to SQL injection. Consider using a parameterized query:\n> ```sql\n> SELECT * FROM users WHERE id = $1\n> ```\n\n**Bad:**\n> Why didn't you add tests?\n\n**Good:**\n> `[MINOR]` The new `calculateDiscount()` function has a few branching paths — could we add tests for the zero-quantity and negative-price edge cases to prevent regressions?\n\n**Bad:**\n> I would have done this differently.\n\n**Good:**\n> `[NIT]` This works well. An alternative approach could be extracting the retry logic into a shared `withRetry()` wrapper — but that's optional and could be a follow-up.\n\n---\n\n## Review Anti-Patterns\n\nAvoid these common traps that waste time and damage team trust:\n\n| Anti-Pattern | Description |\n|--------------|-------------|\n| **Rubber-Stamping** | Approving without reading. Creates false confidence and lets bugs through. |\n| **Bikeshedding** | Spending 30 minutes debating a variable name while ignoring a race condition. |\n| **Blocking on Style** | Refusing to approve over formatting that a linter should enforce automatically. |\n| **Gatekeeping** | Requiring your personal preferred approach when the submitted one is correct. |\n| **Drive-by Reviews** | Leaving one vague comment and disappearing. Commit to following through. |\n| **Scope Creep Reviews** | Requesting unrelated refactors that should be separate PRs. |\n| **Stale Reviews** | Letting PRs sit for days. Review within 24 hours or hand off to someone else. |\n| **Emotional Language** | \"This is terrible\" or \"obviously wrong.\" Critique the code, not the person. |\n\n---\n\n## NEVER Do\n\n1. **NEVER approve without reading every changed line** — rubber-stamping is worse than no review\n2. **NEVER block a PR solely for style preferences** — use a linter; humans review logic\n3. **NEVER leave feedback without a severity level** — ambiguity causes wasted cycles\n4. **NEVER request changes without explaining why** — \"fix this\" teaches nothing\n5. **NEVER review more than 400 lines in one sitting** — comprehension drops sharply; break large PRs into sessions\n6. **NEVER skip the security checklist** — one missed vulnerability outweighs a hundred style nits\n7. **NEVER make it personal** — review the code, never the coder; assume good intent\n","tags":{"latest":"1.0.0"},"stats":{"comments":0,"downloads":17244,"installsAllTime":234,"installsCurrent":234,"stars":19,"versions":1},"createdAt":1770729869941,"updatedAt":1778486991803},"latestVersion":{"version":"1.0.0","createdAt":1770729869941,"changelog":"Initial release of the code-review skill with comprehensive, structured checklists for code review.\n\n- Provides detailed checklists for security, performance, correctness, maintainability, and testing.\n- Suggests severity and priority for each review dimension.\n- Outlines best practices, anti-patterns to avoid, and a step-by-step review process.\n- Includes sample installation instructions.\n- Aims to standardize and improve code review quality for any team.","license":null},"metadata":null,"owner":{"handle":"wpank","userId":"s17bjjbnm7xjckd29e1h2641ks8849md","displayName":"wpank","image":"https://avatars.githubusercontent.com/u/9498646?v=4"},"moderation":null}