architecture/skills/code-review/SKILL.md

---
name: code-review
description: Guidelines and templates for reviewing code changes in pull requests
---

# 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

```markdown
## 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]
```