thoreinstein/agents/code-review

Code Review Mode

Perform a comprehensive code review with structured analysis across multiple dimensions.

When to Use This Skill

• When reviewing pull requests or merge requests
• Before merging feature branches into main/trunk
• When asked to review specific files or changes
• During pair programming review sessions
• For post-implementation quality checks

Workflow

Phase 1: Context Gathering

1. Identify the scope:

• Determine which files/commits to review (diff, specific files, or branch comparison)
• Get the diff: git diff for the relevant range

1. Understand the change:

• Review commit messages and PR description (if available)
• Check git history for related recent changes
• Identify the purpose and expected behavior of the change

1. Map dependencies:

• Identify what other code depends on the changed files
• Check for related test files
• Note any configuration or schema changes

Phase 2: Multi-Dimensional Review

Evaluate the code against each dimension in the Review Dimensions section below. For each dimension:

1. Walk through the relevant checklist
2. Note any issues with severity (Critical/High/Medium/Low)
3. Record the specific location (file:line)
4. Document suggested fixes

Phase 3: Synthesis

Produce a structured review following the template in references/code-review-template.md.

1. Determine verdict:

• APPROVE - No critical/high issues, code is ready to merge
• NEEDS WORK - Has issues that must be addressed before merge
• BLOCK - Has critical issues, security vulnerabilities, or fundamental design problems

1. Organize findings by severity
2. Acknowledge strengths - what's done well
3. Provide actionable fixes - not just problems, but solutions

Review Dimensions

Correctness

• [ ] Logic is correct and handles all expected cases
• [ ] Edge cases are handled (null, empty, boundary values)
• [ ] Assumptions are documented or validated
• [ ] Error states are handled appropriately
• [ ] Types are correct and conversions are safe

Security

• [ ] No injection vulnerabilities (SQL, command, XSS)
• [ ] Input is validated and sanitized
• [ ] No secrets, credentials, or keys in code
• [ ] Authentication/authorization checks are correct
• [ ] Dependencies are secure and up-to-date

Performance

• [ ] No unnecessary loops or redundant operations
• [ ] Algorithms are appropriate for the data size
• [ ] Memory usage is reasonable
• [ ] No N+1 queries or unbounded fetches
• [ ] Caching is used appropriately

Reliability

• [ ] Errors are caught and handled gracefully
• [ ] Failures don't leave system in bad state
• [ ] Retry logic has backoff and limits
• [ ] Resources are properly cleaned up (connections, files, etc.)
• [ ] Timeouts are set for external calls

Maintainability

• [ ] Code is readable and self-documenting
• [ ] Functions/methods have single responsibility
• [ ] Naming is clear and consistent
• [ ] No unnecessary duplication
• [ ] Follows project conventions and patterns

Testing

• [ ] Tests exist for new/changed behavior
• [ ] Edge cases and error paths are tested
• [ ] Tests are deterministic (not flaky)
• [ ] Test names clearly describe what they verify
• [ ] Mocks/stubs are appropriate and not excessive

Escalation Criteria

Flag for additional human review when:

Constraints

• Review only what's in scope - don't expand to unrelated code unless it's directly affected
• Be specific - reference exact file:line locations for all findings
• Provide fixes, not just criticisms - every issue should have a suggested resolution
• Calibrate severity appropriately - not everything is critical
• Acknowledge good work - positive feedback matters too

Examples

Example: Reviewing a New API Endpoint

Scope: PR adds POST /api/users endpoint in internal/api/users.go

Phase 1 output:

Files changed:
- internal/api/users.go (new handler)
- internal/api/routes.go (route registration)
- internal/db/users.go (new repository method)
 
Related tests: internal/api/users_test.go (new)
Purpose: Add user creation endpoint for onboarding flow

Phase 2 findings:

Phase 3 verdict:

Verdict: NEEDS WORK
 
Critical/High Issues:
- SQL injection vulnerability must be fixed before merge
 
What's Done Well:
- Clean separation between handler and repository
- Comprehensive input validation struct
- Good test coverage for happy path
 
Action Items:
1. [High] Fix SQL injection in db/users.go:45
2. [Medium] Add rate limiting to endpoint
3. [Low] Sanitize error messages returned to client

Begin by gathering context on the target files/commits before conducting the multi-dimensional review.