Code Review Standards

What this skill does

# Code Review Standards Skill

## Overview

Code review is a critical quality control process that catches bugs, ensures standards compliance, and facilitates knowledge sharing.

**Review Objectives:** Ensure code quality and maintainability, catch bugs and logic errors, validate security practices, enforce project standards, share knowledge.

## Review Process

### Four-Pass Review Method

**Pass 1: High-Level Understanding**
- Read PR description and ticket context
- Understand what the code is supposed to do
- Review architectural approach
- Identify scope and boundaries

**Pass 2: Line-by-Line Detailed Review**
- Read every changed line
- Check logic correctness
- Identify potential bugs
- Note style issues

**Pass 3: Standards Validation**
- Validate against AGENTS.md quality gates
- Verify constitution.md principles
- Ensure project standards followed

**Pass 4: Architectural Assessment**
- Evaluate design decisions
- Check for code smells
- Assess maintainability
- Consider alternatives

## Review Checklist

### Code Quality

**Readability:**
- [ ] Code is self-documenting
- [ ] Variable names are descriptive
- [ ] Functions appropriately sized (<50 lines ideal)
- [ ] Complex logic has explanatory comments
- [ ] No commented-out code

**Structure:**
- [ ] Proper separation of concerns
- [ ] DRY principle followed (no duplication)
- [ ] Single Responsibility Principle
- [ ] Appropriate abstraction levels
- [ ] Consistent code style

**Error Handling:**
- [ ] All errors handled appropriately
- [ ] Error messages are informative
- [ ] No silent failures
- [ ] Edge cases considered
- [ ] Graceful degradation

### Logic and Correctness

**Functionality:**
- [ ] Implements requirements correctly
- [ ] Edge cases handled
- [ ] Boundary conditions tested
- [ ] No off-by-one errors
- [ ] Correct algorithm complexity

**Data Handling:**
- [ ] Null/undefined checks
- [ ] Type safety maintained
- [ ] Data validation present
- [ ] Proper data transformations
- [ ] No data loss scenarios

### Security

**OWASP Top 10 Security Checklist (Detailed)**

For every PR, validate:

1. **Injection**:
   - [ ] SQL queries use parameterized statements or ORMs
   - [ ] NoSQL queries don't use string concatenation
   - [ ] OS commands don't use unsanitized user input
   - [ ] LDAP queries are parameterized

2. **Broken Authentication**:
   - [ ] Passwords are hashed (bcrypt, Argon2)
   - [ ] Session tokens are secure, random, and expire
   - [ ] Multi-factor authentication is implemented (if required)
   - [ ] No credentials in code or config

3. **Sensitive Data Exposure**:
   - [ ] No API keys, passwords, or secrets in code
   - [ ] Sensitive data encrypted in transit (HTTPS/TLS)
   - [ ] Sensitive data encrypted at rest
   - [ ] No sensitive data in logs or error messages

4. **XML External Entities (XXE)**:
   - [ ] XML parsing disables external entity processing
   - [ ] XML libraries are configured securely

5. **Broken Access Control**:
   - [ ] Authorization checks before sensitive operations
   - [ ] Users can't access others' data without permission
   - [ ] Admin functions require admin privileges
   - [ ] CORS policies are restrictive

6. **Security Misconfiguration**:
   - [ ] No default passwords or credentials
   - [ ] Error messages don't leak sensitive info
   - [ ] Security headers are set (CSP, X-Frame-Options, etc.)
   - [ ] Unnecessary features/services are disabled

7. **Cross-Site Scripting (XSS)**:
   - [ ] User input is escaped before rendering
   - [ ] HTML sanitization is applied to rich content
   - [ ] Content Security Policy is used
   - [ ] No `dangerouslySetInnerHTML` or equivalent without sanitization

8. **Insecure Deserialization**:
   - [ ] Deserialization is from trusted sources only
   - [ ] Input validation before deserialization
   - [ ] Type checks on deserialized objects

9. **Using Components with Known Vulnerabilities**:
   - [ ] Dependencies are up-to-date
   - [ ] No known CVEs in dependencies
   - [ ] Dependency versions are locked

10. **Insufficient Logging & Monitoring**:
    - [ ] Security events are logged
    - [ ] Errors are logged (without sensitive data)
    - [ ] Audit trail for sensitive operations

**Input Validation:**
- [ ] All external input validated
- [ ] Proper sanitization
- [ ] Type checking
- [ ] Length/size limits enforced
- [ ] Whitelist validation where possible

**Authentication & Authorization:**
- [ ] Proper authentication checks
- [ ] Authorization verified
- [ ] Session management secure
- [ ] No hardcoded credentials
- [ ] Secure password handling

**Data Protection:**
- [ ] Sensitive data encrypted
- [ ] No secrets in code
- [ ] Proper key management
- [ ] HTTPS enforced
- [ ] Secure headers present

### Performance

**Efficiency:**
- [ ] No unnecessary computations
- [ ] Appropriate data structures
- [ ] Efficient algorithms
- [ ] No N+1 query problems
- [ ] Proper indexing

**Resource Usage:**
- [ ] No memory leaks
- [ ] File handles closed
- [ ] Database connections managed
- [ ] Caching implemented where beneficial
- [ ] Batch operations used appropriately

### Testing

**Coverage:**
- [ ] Meets minimum coverage requirement (typically 80%)
- [ ] Critical paths fully tested
- [ ] Edge cases covered
- [ ] Error conditions tested
- [ ] Integration tests present

**Test Quality:**
- [ ] Tests are clear and maintainable
- [ ] Tests are deterministic (no flakiness)
- [ ] Good test data
- [ ] Appropriate mocking
- [ ] Tests run fast

### Documentation

**Code Documentation:**
- [ ] Public APIs documented
- [ ] Complex logic explained
- [ ] Assumptions stated
- [ ] TODOs tracked
- [ ] Examples provided where helpful

**External Documentation:**
- [ ] README updated if needed
- [ ] API docs updated
- [ ] Migration guide if breaking changes
- [ ] Changelog entry
- [ ] Configuration documented

### Project Standards

**AGENTS.md Compliance:**
- [ ] Quality gates pass
- [ ] File patterns follow conventions
- [ ] Code review standards met
- [ ] Testing requirements satisfied
- [ ] Architectural patterns followed
- [ ] Preferred libraries used
- [ ] File organization correct
- [ ] Coding style consistent

**constitution.md Compliance:**
- [ ] Core principles upheld
- [ ] Constraints respected
- [ ] Security policies followed
- [ ] Performance requirements met

## Providing Feedback

### Feedback Structure

**Standard format:**
```markdown
**[Severity]** [Category]: [Issue]

[Explanation]

[Suggestion]

[Example or reference]
```

**Severity levels:**
- **P0 (Critical):** Security vulnerabilities, data loss, breaking changes
- **P1 (Important):** Logic errors, quality gate violations, architecture issues
- **P2 (Minor):** Style issues, optimization opportunities, refactoring suggestions
- **Suggestion:** Nice-to-haves, alternative approaches, learning opportunities

**Reference:** See k2-dev-reference.md#review-severity-levels

### Example Feedback

**P0 - Security Issue:**
```markdown
**P0** Security: SQL Injection Vulnerability

The search query is directly concatenated into the SQL statement, allowing injection attacks.

\```typescript
// Current (vulnerable)
const query = `SELECT * FROM users WHERE name = '${searchTerm}'`;

// Fix: Use parameterized queries
const query = 'SELECT * FROM users WHERE name = ?';
const results = await db.query(query, [searchTerm]);
\```

Reference: OWASP SQL Injection Prevention Cheat Sheet
```

**P1 - Logic Error:**
```markdown
**P1** Logic: Off-by-One Error in Pagination

The pagination logic will skip the last item on each page due to incorrect boundary condition.

\```typescript
// Current (incorrect)
const end = start + pageSize;  // Should be exclusive

// Fix
const end = Math.min(start + pageSize, totalItems);
\```

Add test case TC-045 to catch this.
```

### Tone and Approach

**DO:**
✅ Be specific and objective
✅ Explain the reasoning
✅ Provide examples and references
✅ Suggest solutions, not just problems
✅ Distinguish between must-fix and nice-to-have
✅ Ack