architecture/skills/code-review/SKILL.md

Code Review

Guidelines for reviewing code changes in pull requests.

Review Categories

Code Quality

Look for:

• Readability: Clear naming, logical structure, appropriate comments
• Maintainability: Easy to modify, follows DRY, single responsibility
• Complexity: Avoid deep nesting, overly long functions, complex conditionals
• Dead code: Unused variables, unreachable code, commented-out blocks

Questions to ask:

• Can someone unfamiliar with this code understand it quickly?
• Would I be comfortable maintaining this code?
• Does this follow existing patterns in the codebase?

Potential Bugs

Look for:

• Edge cases: Empty arrays, null values, boundary conditions
• Logic errors: Off-by-one, incorrect operators, inverted conditions
• Race conditions: Async operations, shared state
• Resource leaks: Unclosed connections, missing cleanup
• Error handling: Unhandled exceptions, silent failures

Questions to ask:

• What happens with unexpected input?
• Are all error paths handled appropriately?
• Could concurrent execution cause issues?

Security Concerns

Look for:

• Injection: SQL, command, XSS vulnerabilities
• Authentication: Bypasses, weak validation
• Authorization: Missing permission checks
• Data exposure: Logging secrets, exposing internals
• Dependencies: Known vulnerabilities in imports

Questions to ask:

• Could an attacker exploit this?
• Is user input properly validated and sanitized?
• Are secrets properly protected?

Style & Consistency

Look for:

• Naming conventions: Match existing codebase style
• Formatting: Consistent indentation, spacing
• File organization: Logical grouping, appropriate location
• Import order: Following project conventions

Note: Style issues are lower priority than functional concerns.

Test Coverage

Look for:

• Missing tests: New functionality without tests
• Edge cases: Boundary conditions not tested
• Error paths: Exception handling not verified
• Integration: Component interactions not covered

Questions to ask:

• Would these tests catch a regression?
• Are the assertions meaningful?
• Do tests cover the acceptance criteria?

Review Process

1. Understand context: Read PR description and linked issues
2. High-level scan: Understand overall structure and approach
3. Detailed review: Go through changes file by file
4. Consider impact: Think about side effects and dependencies
5. Provide feedback: Be constructive and specific

Writing Review Comments

Be Constructive

• Explain why something is an issue
• Suggest alternatives when possible
• Distinguish between blocking issues and suggestions

Be Specific

• Reference exact lines or code blocks
• Provide concrete examples
• Link to relevant documentation or patterns

Be Kind

• Phrase feedback as questions when appropriate
• Acknowledge good solutions
• Remember there's a person receiving this feedback

Verdict Criteria

LGTM (Looks Good To Me)

• No blocking issues
• Code meets quality standards
• Tests are adequate
• Ready to merge

Needs Changes

• Minor issues that should be addressed
• Style improvements
• Missing tests for edge cases
• Not blocking, but worth fixing

Blocking Issues

• Security vulnerabilities
• Logic errors that would cause bugs
• Missing critical functionality
• Breaking changes without migration

Review Comment Template

## AI Code Review

> This is an automated review generated by the code-reviewer agent.

### Summary
[1-2 sentence overall assessment of the changes]

### Findings

#### Code Quality
- [Issue with specific file:line reference and explanation]
- [Or "Code is well-structured and readable"]

#### Potential Bugs
- [Bug risk with explanation]
- [Or "No obvious issues found"]

#### Security Concerns
- [Security issue with severity]
- [Or "No security concerns identified"]

#### Style Notes
- [Style improvement suggestion]
- [Or "Consistent with codebase conventions"]

#### Test Coverage
- [Missing test scenario]
- [Or "Tests adequately cover changes"]

### Verdict
**[LGTM / Needs Changes / Blocking Issues]**

[Brief explanation of verdict]