thoreinstein/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

2. 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

3. 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

2. Organize findings by severity

3. Acknowledge strengths - what's done well

4. 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:

| Concern | Escalation Trigger | | ---------------- | -------------------------------------------------------------------------------- | | Security | Any auth changes, crypto usage, user data handling, or potential vulnerabilities | | Reliability | Changes to error handling, retry logic, or failure recovery paths | | Performance | Changes to hot paths, database queries, or algorithms with scale concerns | | Architecture | New patterns, significant structural changes, or cross-cutting concerns | | Testing | Reduced coverage, disabled tests, or changes to test infrastructure |

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:

| Severity | Issue | Location | Fix | | -------- | -------------------------------------- | --------------- | ------------------------------ | | High | SQL injection via unsanitized email | db/users.go:45 | Use parameterized query | | Medium | Missing rate limiting | api/users.go:23 | Add rate limiter middleware | | Low | Error message exposes internal details | api/users.go:67 | Return generic error to client |

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.