architecture/old/skills/code-review/SKILL.md

name, model, description, user-invocable

name	model	description	user-invocable
code-review	haiku	Review code for quality, bugs, security, and style issues. Use when reviewing pull requests, checking code quality, looking for bugs or security vulnerabilities, or when the user asks for a code review.	false

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

Example Review Comments

Code Quality

Good:

src/utils/parser.ts:45 - This function is doing three things: parsing, validating, and transforming. Consider splitting into parse(), validate(), and transform() to improve testability and make each responsibility clear.

Bad:

This code is messy, please clean it up.

Potential Bugs

Good:

src/api/users.ts:23 - users.find() returns undefined when no match is found, but line 25 accesses user.id without a null check. This will throw when the user doesn't exist. Consider: const user = users.find(...); if (!user) return null;

Bad:

This might crash.

Security

Good:

src/routes/search.ts:12 - The query parameter is interpolated directly into the SQL string, which allows SQL injection. Use parameterized queries instead: db.query('SELECT * FROM items WHERE name = ?', [query])

Bad:

Security issue here.

Style

Good:

src/components/Button.tsx:8 - Minor: The codebase uses camelCase for event handlers (e.g., handleClick), but this uses on_click. Consider renaming for consistency.

Bad:

Wrong naming convention.

Test Coverage

Good:

The happy path is well tested. Consider adding a test for when fetchUser() rejects - the error handling in line 34 isn't currently covered.

Bad:

Need more tests.

Positive Feedback

Good:

Nice use of the builder pattern here - it makes the configuration much more readable than the previous approach with multiple boolean flags.

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]