Code Review Skill

Use this skill when reviewing code changes, PRs, or evaluating code quality.

Review Process

1. •Understand the context - What problem is being solved? What's the expected behavior?
2. •Read the code - Understand the implementation before critiquing.
3. •Check for correctness - Does it do what it's supposed to do?
4. •Evaluate quality - Is it maintainable, readable, and well-structured?
5. •Provide actionable feedback - Be specific, constructive, and prioritize issues.

Providing Feedback

Be Specific

Bad: "This is confusing." Good: "The variable x doesn't describe its purpose. Consider renaming to userCount."

Explain Why

Bad: "Don't do this." Good: "This approach can cause a race condition if two requests arrive simultaneously."

Suggest Alternatives

Bad: "This is wrong." Good: "Consider using a map lookup instead of a loop here for O(1) access."

Prioritize Issues

• •Blocking: Must fix before merge (bugs, security issues, breaking changes)
• •Important: Should fix, but not blocking (maintainability, performance)
• •Nit: Minor suggestions (style, naming preferences)

Label your feedback clearly: [blocking], [important], [nit]

Checklist

Correctness

• • Does the code correctly implement the intended functionality?
• • Are edge cases handled (null, empty, boundary values)?
• • Are error conditions handled appropriately?
• • Are there any off-by-one errors or incorrect loop bounds?
• • Is the logic correct for all input combinations?

Error Handling

• • Are errors caught and handled, not silently ignored?
• • Are error messages informative and actionable?
• • Do errors propagate correctly to callers?
• • Are resources cleaned up on error (files, connections, locks)?

Readability

• • Are names descriptive and consistent?
• • Is the code self-documenting, or are comments needed for clarity?
• • Is the control flow easy to follow?
• • Are magic numbers replaced with named constants?
• • Is nesting depth reasonable (generally 3 levels or fewer)?

Maintainability

• • Is the code DRY without being over-abstracted?
• • Are functions and classes appropriately sized?
• • Is there a single responsibility per function/class?
• • Are dependencies explicit and minimal?
• • Would a new team member understand this code?

Performance

• • Are there any obvious inefficiencies (N+1 queries, unnecessary loops)?
• • Is the algorithmic complexity appropriate for the data size?
• • Are resources (memory, connections) managed efficiently?
• • Could any operations be batched or cached?

Testing

• • Are there tests covering the new/changed functionality?
• • Do tests cover edge cases and error conditions?
• • Are tests readable and maintainable?
• • Do tests actually assert the correct behavior?

API Design

• • Are function signatures intuitive and consistent?
• • Are return types clear and predictable?
• • Is the public API minimal (no unnecessary exposure)?
• • Are breaking changes avoided or clearly documented?

Security

• • Is user input validated and sanitized?
• • Are there any hardcoded secrets or credentials?
• • Are permissions/authorization checked appropriately?
• • See security-check skill for comprehensive security review.

Concurrency

• • Are shared resources protected from race conditions?
• • Are database transactions used correctly (appropriate isolation levels)?
• • Are locks held for minimal time?
• • Is there potential for deadlocks?
• • Are async operations properly awaited?

Observability

• • Are key operations logged with appropriate levels?
• • Is sensitive data excluded from logs?
• • Are errors logged with enough context for debugging?
• • Are request/correlation IDs propagated?

Dependencies

• • Are new dependencies necessary and well-maintained?
• • Is the dependency license compatible with the project?
• • Are there known vulnerabilities in new dependencies?

Backward Compatibility

• • Are API contracts preserved (no breaking changes without versioning)?
• • Are database migrations reversible or forward-compatible?
• • Are feature flags used for risky changes?