code-review
Resources
scripts/
validate-code-review.sh
references/
review-patterns.md
Code Review Quality Skill
This skill teaches you how to perform thorough, enterprise-grade code reviews using GoodVibes precision tools. A systematic code review catches issues early, ensures consistency, validates security and performance, and maintains high quality standards across the codebase.
When to Use This Skill
Load this skill when:
- Reviewing pull requests or merge requests
- Performing code quality audits
- Validating security and performance of implementations
- Checking adherence to architectural patterns
- Assessing test coverage and quality
- Evaluating accessibility compliance
- Providing structured feedback to developers
Trigger phrases: "review this code", "check the PR", "quality audit", "security review", "performance review", "validate implementation".
Core Workflow
Phase 1: Discovery - Understand the Change
Before reviewing, understand what changed, why, and the scope of impact.
Step 1.1: Gather Change Context
Use discover to map all changed files and identify the type of change.
discover:
queries:
- id: changed_files
type: glob
patterns: ["src/**/*"] # List changed files for review
- id: new_files
type: glob
patterns: ["**/*"]
- id: test_files
type: glob
patterns: ["**/*.test.ts", "**/*.test.tsx", "**/*.spec.ts", "**/*.spec.tsx"]
verbosity: files_only
Extract from git:
precision_exec:
commands:
- cmd: "git diff --name-only HEAD~1..HEAD"
verbosity: minimal
What this reveals:
- All files modified in the changeset
- New vs modified vs deleted files
- Test files included (or missing)
- Scope of change (frontend, backend, full-stack)
Step 1.2: Analyze Changed Code
Use precision_read to examine the actual changes.
precision_read:
files:
- path: "src/api/user-routes.ts" # Example changed file
extract: content
- path: "src/components/UserProfile.tsx"
extract: content
output:
max_per_item: 200
verbosity: standard
Get diff context:
precision_exec:
commands:
- cmd: "git diff HEAD~1..HEAD -- src/api/user-routes.ts"
verbosity: standard
Step 1.3: Find Related Code
Use discover to find imports, exports, and usage of changed symbols.
discover:
queries:
- id: function_exports
type: grep
pattern: "export (function|const|class) (createUser|updateUser|deleteUser)"
glob: "**/*.{ts,tsx,js,jsx}"
- id: usage_sites
type: grep
pattern: "(createUser|updateUser|deleteUser)\\("
glob: "**/*.{ts,tsx,js,jsx}"
- id: related_tests
type: grep
pattern: "(createUser|updateUser|deleteUser)"
glob: "**/*.test.{ts,tsx}"
verbosity: locations
What this reveals:
- Where changed functions are used
- Potential breaking changes
- Test coverage for changed code
- Ripple effects across the codebase
Phase 2: Security Review
Security issues are critical and must be caught in review.
Step 2.1: Check for Common Vulnerabilities
Use discover to find security anti-patterns.
discover:
queries:
# SQL Injection
- id: sql_injection
type: grep
pattern: '(query|execute|sql).*[`$].*\$\{'
glob: "**/*.{ts,tsx,js,jsx}"
# XSS vulnerabilities
- id: dangerous_html
type: grep
pattern: "(dangerouslySetInnerHTML|innerHTML|outerHTML)"
glob: "**/*.{ts,tsx,jsx}"
# Hardcoded secrets
- id: hardcoded_secrets
type: grep
pattern: '(password|secret|api[_-]?key|token)\s*=\s*["''][^"'']+["'']'
glob: "**/*.{ts,tsx,js,jsx,json}"
# Missing authentication
- id: unauthed_routes
type: grep
pattern: "export (async )?function (GET|POST|PUT|DELETE|PATCH)"
glob: "src/app/api/**/*.ts"
verbosity: locations
Critical patterns to check:
| Pattern | Severity | Why It Matters |
|---|---|---|
| SQL injection | Critical | Allows attackers to read/modify database |
| XSS vulnerabilities | Critical | Allows script injection, session hijacking |
| Hardcoded secrets | Critical | Exposes credentials in source code |
| Missing auth checks | Critical | Exposes protected resources |
| Unsafe deserialization | Critical | Remote code execution |
| CORS misconfiguration | Major | Allows unauthorized origins |
| Weak password rules | Major | Account compromise |
| Missing input validation | Major | Data corruption, injection |
Step 2.2: Validate Input Handling
All external input must be validated.
discover:
queries:
# Check for validation schemas
- id: zod_schemas
type: grep
pattern: "z\\.(object|string|number|array|enum)"
glob: "**/*.{ts,tsx}"
# Check for direct request.json() without validation
- id: unvalidated_input
type: grep
pattern: "(await request\\.json\\(\\)|req\\.body)(?!.*safeParse)"
glob: "src/app/api/**/*.ts"
# Check for SQL parameterization
- id: parameterized_queries
type: grep
pattern: "(db\\.(query|execute)|prisma\\.|sql`)"
glob: "**/*.{ts,js}"
verbosity: locations
Best practices to validate:
- All API routes validate input with Zod, Joi, or equivalent
- All database queries use parameterized queries (Prisma, prepared statements)
- File uploads validate content type and size
- URLs and redirects are validated against allowlists
Step 2.3: Check Authentication & Authorization
Verify that protected resources require authentication.
discover:
queries:
# Find auth middleware usage
- id: auth_middleware
type: grep
pattern: "(getServerSession|auth\\(\\)|requireAuth|withAuth)"
glob: "src/app/api/**/*.ts"
# Find resource ownership checks
- id: ownership_checks
type: grep
pattern: "(userId|authorId|ownerId)\s*===\s*(session|user|currentUser)"
glob: "**/*.{ts,tsx}"
# Find RBAC checks
- id: rbac_checks
type: grep
pattern: "(role|permission|can)\s*===\s*"
glob: "**/*.{ts,tsx}"
verbosity: locations
Critical checks:
- Protected API routes call auth middleware
- User can only modify their own resources (ownership checks)
- Admin-only actions check for admin role
- JWTs are validated with proper secret
- Session tokens are rotated and have expiration
Phase 3: Performance Review
Performance issues cause poor UX and cost scalability.
Step 3.1: Check for N+1 Queries
N+1 queries are the #1 database performance killer.
discover:
queries:
# Find loops with database calls
- id: n_plus_one
type: grep
pattern: "(for|forEach|map).*await.*(prisma|db|query|find)"
glob: "**/*.{ts,tsx,js,jsx}"
# Find Prisma include usage
- id: prisma_includes
type: grep
pattern: "(findMany|findUnique|findFirst).*include:"
glob: "**/*.{ts,js}"
verbosity: locations
How to fix:
- Use
includeto eager load related records (Prisma) - Use joins or
SELECT INfor batch loading - Implement DataLoader for GraphQL
- Cache frequently accessed data
Step 3.2: Check for Unnecessary Re-renders
React re-render issues harm frontend performance.
discover:
queries:
# Find inline object/array creation in JSX
- id: inline_objects
type: grep
pattern: "(onClick|onChange|style)=\\{\\{|=\\{\\["
glob: "**/*.{tsx,jsx}"
# Find missing useMemo/useCallback
- id: missing_memoization
type: grep
pattern: "(map|filter|reduce)\\("
glob: "**/*.{tsx,jsx}"
# Find useEffect without dependencies
- id: missing_deps
type: grep
pattern: "useEffect\\([^)]+\\)\\s*$"
glob: "**/*.{tsx,jsx}"
verbosity: locations
Common issues:
| Anti-pattern | Fix |
|---|---|
| Inline object in props | Extract to constant or useMemo |
| Inline function in props | Wrap in useCallback |
| Large list without key | Add stable key prop |
| useEffect missing deps | Add all used variables to deps array |
| Context re-renders everything | Split context or use state managers |
Step 3.3: Check Database Indexes
Missing indexes cause slow queries.
precision_read:
files:
- path: "prisma/schema.prisma"
extract: content
output:
max_per_item: 500
verbosity: standard
Validate indexes exist for:
- Foreign keys (userId, postId, etc.)
- Frequently filtered fields (status, createdAt, published)
- Fields used in WHERE clauses
- Compound indexes for multi-column queries
Phase 4: Code Quality Review
Code quality affects maintainability and reliability.
Step 4.1: Check Type Safety
TypeScript should catch errors at compile time, not runtime.
discover:
queries:
# Find any types
- id: any_usage
type: grep
pattern: ":\s*any(\\s|;|,|\\))"
glob: "**/*.{ts,tsx}"
# Find type assertions (as)
- id: type_assertions
type: grep
pattern: "as (unknown|any|string|number)"
glob: "**/*.{ts,tsx}"
# Find non-null assertions (!)
- id: non_null_assertions
type: grep
pattern: "![.;,)\\]]"
glob: "**/*.{ts,tsx}"
# Find unsafe member access
- id: unsafe_access
type: grep
pattern: "\\?\\."
glob: "**/*.{ts,tsx}"
verbosity: locations
Type safety issues:
| Issue | Severity | Fix |
|---|---|---|
any type usage |
Major | Use proper types or unknown |
as any assertions |
Major | Fix the underlying type issue |
! non-null assertion |
Minor | Add null checks |
| Missing return types | Minor | Explicitly type function returns |
| Implicit any params | Major | Add parameter types |
Step 4.2: Check Error Handling
All async operations and API calls must handle errors.
discover:
queries:
# Find floating promises
- id: floating_promises
type: grep
pattern: "^\\s+[a-z][a-zA-Z]*\\(.*\\);$"
glob: "**/*.{ts,tsx,js,jsx}"
# Find empty catch blocks
- id: empty_catch
type: grep
pattern: "catch.*\\{\\s*\\}"
glob: "**/*.{ts,tsx,js,jsx}"
# Find console.error (should use logger)
- id: console_error
type: grep
pattern: "console\\.(error|warn|log)"
glob: "**/*.{ts,tsx,js,jsx}"
verbosity: locations
Error handling checklist:
- All promises have
.catch()ortry/catch - Errors are logged with context (user ID, request ID)
- User-facing errors are sanitized (no stack traces)
- API errors return proper HTTP status codes
- Retry logic for transient failures
Step 4.3: Check Code Organization
Large files and high complexity make code hard to maintain.
precision_exec:
commands:
- cmd: "find src -not -path '*/node_modules/*' -not -path '*/dist/*' -name '*.ts' -o -name '*.tsx' -print0 | xargs -0 wc -l | sort -rn | head -20"
verbosity: standard
Code organization rules:
- Files should be < 300 lines (exception: generated code)
- Functions should be < 50 lines
- Cyclomatic complexity < 10
- Max nesting depth: 3 levels
- Extract helpers to separate files
Phase 5: Testing Review
Tests validate correctness and prevent regressions.
Step 5.1: Check Test Coverage
Every changed file should have tests.
discover:
queries:
# Find test files
- id: test_files
type: glob
patterns: ["**/*.test.{ts,tsx}", "**/*.spec.{ts,tsx}"]
# Find files without tests
- id: source_files
type: glob
patterns: ["src/**/*.{ts,tsx}"]
# Check test imports
- id: test_imports
type: grep
pattern: "from ['\"].*/(api|lib|components)/"
glob: "**/*.test.{ts,tsx}"
verbosity: files_only
Compare source files to test files:
// Pseudo-logic (implement with precision tools)
const sourceFiles = results.source_files.files;
const testFiles = results.test_files.files;
const missingTests = sourceFiles.filter(f => !testFiles.some(t => t.includes(f.replace('.ts', ''))));
Step 5.2: Check Test Quality
Tests should test behavior, not implementation.
discover:
queries:
# Find skipped tests
- id: skipped_tests
type: grep
pattern: "(it\\.skip|test\\.skip|describe\\.skip)"
glob: "**/*.test.{ts,tsx}"
# Find focused tests (.only)
- id: focused_tests
type: grep
pattern: "(it\\.only|test\\.only|describe\\.only)"
glob: "**/*.test.{ts,tsx}"
# Find expect assertions
- id: assertions
type: grep
pattern: "expect\\("
glob: "**/*.test.{ts,tsx}"
# Find mock usage
- id: mocks
type: grep
pattern: "(vi\\.mock|jest\\.mock|vi\\.fn)"
glob: "**/*.test.{ts,tsx}"
verbosity: locations
Test quality checklist:
- No
.skipor.only(should be removed before merge) - Each test has at least one assertion
- Tests have descriptive names ("it should...", "when...then...")
- Tests are isolated (no shared state)
- Mocks are used sparingly (prefer real implementations)
- Edge cases are tested (null, empty, large values)
Phase 6: Architecture Review
Architecture violations create technical debt.
Step 6.1: Check Dependency Direction
Dependencies should flow from outer layers to inner layers.
discover:
queries:
# Find domain imports in UI
- id: ui_imports_domain
type: grep
pattern: "from ['\"].*/(domain|core|lib)/"
glob: "src/components/**/*.{ts,tsx}"
# Find UI imports in domain
- id: domain_imports_ui
type: grep
pattern: "from ['\"].*/(components|pages|app)/"
glob: "src/domain/**/*.{ts,tsx}"
# Find circular dependencies
- id: imports
type: grep
pattern: "^import.*from"
glob: "src/**/*.{ts,tsx}"
verbosity: locations
Dependency rules:
- UI components can import hooks, utils, domain logic
- Domain logic should NOT import UI components
- API routes can import domain logic, not UI
- Shared utils should have zero dependencies
Step 6.2: Check for Layering Violations
discover:
queries:
# Database access in components
- id: db_in_components
type: grep
pattern: "(prisma|db\\.(query|execute))"
glob: "src/components/**/*.{ts,tsx}"
# Business logic in API routes
- id: logic_in_routes
type: grep
pattern: "export (async )?function (GET|POST)"
glob: "src/app/api/**/*.ts"
verbosity: files_only
Read the route handlers to check:
- Route handlers should be < 30 lines (delegate to services)
- No database queries in components (use server actions or API routes)
- No business logic in routes (extract to service layer)
Phase 7: Accessibility Review
Accessibility ensures your app is usable by everyone.
Step 7.1: Check Semantic HTML
Use proper HTML elements for accessibility.
discover:
queries:
# Find div buttons (should be <button>)
- id: div_buttons
type: grep
pattern: "<div.*(onClick|onKeyDown)"
glob: "**/*.{tsx,jsx}"
# Find missing alt text
- id: missing_alt
type: grep
pattern: "<img(?![^>]*alt=)"
glob: "**/*.{tsx,jsx}"
# Find missing labels
- id: missing_labels
type: grep
pattern: "<input(?![^>]*aria-label)(?![^>]*id=)"
glob: "**/*.{tsx,jsx}"
# Find missing ARIA roles
- id: missing_roles
type: grep
pattern: "<(nav|header|footer|main)(?![^>]*role=)"
glob: "**/*.{tsx,jsx}"
verbosity: locations
Accessibility checklist:
- Buttons use
<button>, not<div onClick> - Images have descriptive
alttext - Form inputs have labels or
aria-label - Focus states are visible (no
outline: nonewithout replacement) - Color contrast meets WCAG AA (4.5:1 for text)
- Keyboard navigation works (tab order, Enter/Space)
Step 7.2: Check ARIA Patterns
discover:
queries:
# Find custom components
- id: custom_components
type: grep
pattern: "(Accordion|Dialog|Dropdown|Tabs|Tooltip)"
glob: "src/components/**/*.{tsx,jsx}"
verbosity: files_only
Read components to validate:
- Modals trap focus and close on Escape
- Dropdowns support arrow keys
- Tabs support arrow keys and Home/End
- Tooltips are accessible (not just on hover)
Phase 8: Review Scoring
Use the review-scoring skill to provide structured feedback.
See plugins/goodvibes/skills/protocol/review-scoring/SKILL.md for the full rubric.
The 10 Dimensions (weighted):
- Correctness (15%) - Does it work? Does it meet requirements?
- Type Safety (10%) - Proper TypeScript usage, no
any - Security (15%) - No vulnerabilities, input validation, auth checks
- Performance (10%) - No N+1 queries, proper indexes, memoization
- Error Handling (10%) - All errors caught, logged, and handled
- Testing (10%) - Adequate test coverage, quality assertions
- Code Quality (10%) - Readable, maintainable, follows patterns
- Architecture (10%) - Proper layering, no violations
- Accessibility (5%) - Semantic HTML, ARIA, keyboard nav
- Documentation (5%) - JSDoc, README, inline comments where needed
Score each dimension 1-10:
- 1-3: Needs major work
- 4-6: Needs improvement
- 7-8: Good, minor issues
- 9-10: Excellent
Overall score = weighted average
Pass/fail thresholds:
- 9.5+: Excellent (approve immediately)
- 8.0-9.4: Good (approve with minor suggestions)
- 7.0-7.9: Acceptable (request changes - minor issues)
- 5.0-6.9: Needs work (request changes - major issues)
- < 5.0: Reject (needs significant rework)
Phase 9: Automated Validation
Run automated checks to catch issues.
precision_exec:
commands:
# Type check
- cmd: "npm run typecheck"
# Lint
- cmd: "npm run lint"
# Tests
- cmd: "npm run test"
# Security audit
- cmd: "npm audit --audit-level=moderate"
verbosity: standard
All checks must pass before approval.
Phase 10: Provide Feedback
Structured feedback is actionable and specific.
Review output format:
## Review Summary
**Overall Score**: 8.2/10
**Verdict**: APPROVE with suggestions
**What changed**: Added user profile API with authentication
**Files reviewed**: 8 files (5 source, 3 test)
## Dimension Scores
1. Correctness: 9/10
2. Type Safety: 7/10
3. Security: 9/10
4. Performance: 8/10
5. Error Handling: 7/10
6. Testing: 8/10
7. Code Quality: 9/10
8. Architecture: 8/10
9. Accessibility: 8/10
10. Documentation: 7/10
## Issues Found
### Major (should fix)
- **FILE:LINE** - Type safety: Function `updateProfile` has implicit `any` return type
- Fix: Add explicit return type `Promise<User>`
- Impact: TypeScript can't catch type errors in callers
### Minor (nice to fix)
- **src/api/profile.ts:42** - Error handling: Empty catch block swallows errors
- Fix: Log error with context before re-throwing
- Impact: Makes debugging harder
## What Was Done Well
- Excellent input validation with Zod schemas
- Comprehensive test coverage (95%)
- Proper authentication checks on all routes
- Clean separation of concerns (route -> service -> repository)
Feedback guidelines:
- Be specific: Include file path and line number
- Be actionable: Explain exactly how to fix
- Explain impact: Why does this matter?
- Acknowledge good work: Highlight what was done well
- Categorize by severity: Critical > Major > Minor
Common Review Patterns
See references/review-patterns.md for detailed anti-patterns organized by category.
Quick reference:
Security Patterns
- SQL injection: String concatenation in queries
- XSS: Unsafe HTML rendering
- Auth bypass: Missing auth checks
- Secrets exposure: Hardcoded API keys
Performance Patterns
- N+1 queries: Loops with database calls
- Missing indexes: WHERE clauses without indexes
- Unnecessary re-renders: Inline objects in React
- Memory leaks: Event listeners not cleaned up
Code Quality Patterns
- Type unsafety:
anytypes, type assertions - Error handling: Floating promises, empty catches
- Complexity: Functions > 50 lines, nesting > 3
- Duplication: Copy-paste code
Testing Patterns
- Missing tests: Changed files without tests
- Poor assertions:
expect(result).toBeTruthy() - Test pollution: Shared state between tests
- Skipped tests:
.skipor.onlyleft in
Precision Tools for Review
Discover Tool
Run multiple grep/glob queries in parallel to find patterns.
Example: Find security issues
discover:
queries:
- id: sql_injection
type: grep
pattern: 'query.*\$\{'
- id: hardcoded_secrets
type: grep
pattern: 'api[_-]?key\s*=\s*["''][^"'']+'
- id: xss
type: grep
pattern: 'dangerouslySetInnerHTML'
verbosity: locations
Precision Grep
Search with context, multiline support, and token limits.
Example: Find error handling
precision_grep:
queries:
- id: catch_blocks
pattern: "try\\s*\\{[\\s\\S]*?\\}\\s*catch"
output:
format: context
context_after: 3
context_before: 1
verbosity: standard
Precision Exec
Run validation commands.
precision_exec:
commands:
- cmd: "npm run typecheck"
- cmd: "npm run lint"
- cmd: "npm test -- --coverage"
verbosity: standard
Validation Script
Use scripts/validate-code-review.sh to validate review completeness.
./scripts/validate-code-review.sh /path/to/review-output.md
The script checks:
- Security patterns were checked
- Performance patterns were checked
- Test coverage was reviewed
- All changed files were reviewed
- Review includes scoring with rubric
- Feedback is specific with file/line references
Quick Reference
Review Checklist
Before reviewing:
- Understand the change (read PR description, commits)
- Get the diff (
git diffor GitHub PR) - Identify changed files and their purpose
During review:
- Security: Check for vulnerabilities, auth, validation
- Performance: Check for N+1, indexes, re-renders
- Type safety: Check for
any, assertions, return types - Error handling: Check for floating promises, empty catches
- Testing: Check coverage, quality, skipped tests
- Architecture: Check layering, dependencies, organization
- Accessibility: Check semantic HTML, ARIA, keyboard nav
After review:
- Run automated validation (typecheck, lint, test)
- Score all 10 dimensions
- Calculate weighted score
- Provide verdict (approve/request changes/reject)
- Write actionable feedback with file/line references
Severity Guidelines
Critical (must fix before merge):
- Security vulnerabilities (SQL injection, XSS, auth bypass)
- Data loss bugs
- Compilation/runtime errors
- Breaking changes to public API
Major (should fix before merge):
- Type safety issues (
anyusage, missing types) - Performance issues (N+1 queries, missing indexes)
- Missing error handling
- Missing tests for critical paths
- Architecture violations
Minor (nice to fix, or address in follow-up):
- Code style inconsistencies
- Missing documentation
- Non-critical accessibility issues
- Refactoring opportunities
- Low test coverage on edge cases
Common Mistakes to Avoid
Score inflation:
- Don't give 9-10 scores when issues exist
- Be honest about quality gaps
- Critical issues should fail review
Vague feedback:
- BAD: "Fix the types"
- GOOD: "src/api/user.ts:42 - Add return type
Promise<User>to functiongetUser"
Ignoring positives:
- Always acknowledge what was done well
- Positive feedback motivates improvement
Inconsistent severity:
- Security issues are always Critical
- Missing tests are Major, not Minor
- Style issues are Minor, not Major
Advanced Techniques
Batch Review with Discover
Review multiple PRs or files in parallel.
discover:
queries:
- id: all_changes
type: grep
pattern: ".*"
path: "src/"
verbosity: files_only
Then read files in batch:
precision_read:
files:
- path: "src/changed-file-1.ts"
- path: "src/changed-file-2.ts"
extract: content
output:
max_per_item: 100
verbosity: standard
Contextual Review
Use precision_grep with context to understand surrounding code.
precision_grep:
queries:
- id: auth_checks
pattern: "getServerSession|auth\\(\\)"
output:
format: context
context_before: 5
context_after: 10
verbosity: standard
Differential Review
Focus on changed lines only.
precision_exec:
commands:
- cmd: "git diff HEAD~1..HEAD --unified=5"
verbosity: standard
Parse the diff and review only changed sections.
Automated Pattern Detection
Create a batch of security/performance checks.
discover:
queries:
# Security
- { id: sql_injection, type: grep, pattern: 'query.*\$\{' }
- { id: xss, type: grep, pattern: 'dangerouslySetInnerHTML' }
- { id: secrets, type: grep, pattern: 'password\s*=\s*["''][^"'']+' }
# Performance
- { id: n_plus_one, type: grep, pattern: 'for.*await.*prisma' }
- { id: inline_objects, type: grep, pattern: 'onClick=\{\{', glob: '**/*.tsx' }
# Quality
- { id: any_usage, type: grep, pattern: ':\s*any', glob: '**/*.ts' }
- { id: empty_catch, type: grep, pattern: 'catch.*\{\s*\}' }
verbosity: locations
Aggregate results and score based on findings.
Integration with Other Skills
- Use review-scoring for structured feedback with the 10-dimension rubric
- Use error-recovery when fix attempts fail during review remediation
- Use precision-mastery for advanced precision tool usage
- Use testing-strategy to validate test quality
- Use component-architecture when reviewing UI components
- Use api-design when reviewing API routes
- Use database-layer when reviewing database queries and schemas
Resources
references/review-patterns.md- Common anti-patterns by categoryscripts/validate-code-review.sh- Automated review validationplugins/goodvibes/skills/protocol/review-scoring/SKILL.md- Scoring rubric details- OWASP Top 10 - https://owasp.org/www-project-top-ten/
- WCAG Guidelines - https://www.w3.org/WAI/WCAG21/quickref/