/dev-review - Code Review

Skill Awareness: See skills/_registry.md for all available skills.

• Before: After /dev-coding implementation
• If issues: Fix with /dev-coding, then review again
• If major changes: Create CR via /debrief

Review code changes for quality, security, and adherence to standards.

When to Use

• After implementing a feature
• Before merging a PR
• To get feedback on approach
• To catch issues before production

Usage

/dev-review                      # Review uncommitted changes
/dev-review --staged             # Review staged changes only
/dev-review src/auth/            # Review specific directory
/dev-review UC-AUTH-001          # Review changes for specific UC

Input

Reviews can be based on:

1. Git diff - Uncommitted or staged changes
2. File list - Specific files to review
3. UC reference - Files changed for a use case (from spec)

Output

## Review Summary

**Verdict**: ✅ Approve | ⚠️ Request Changes | ❓ Needs Discussion

**Stats**: X files, Y additions, Z deletions

### Issues Found

#### 🔴 Critical (must fix)
- [security] SQL injection risk in `src/api/users.ts:45`
- [bug] Null pointer in `src/utils/parse.ts:23`

#### 🟡 Important (should fix)
- [performance] N+1 query in `src/api/posts.ts:67`
- [error-handling] Unhandled promise rejection

#### 🔵 Suggestions (nice to have)
- [style] Consider extracting to helper function
- [naming] `data` is too generic, suggest `userProfile`

### By File
- `src/api/auth.ts` - 2 issues
- `src/components/Form.tsx` - 1 suggestion

### Positives
- Good error handling in login flow
- Clean separation of concerns

Workflow

Phase 1: Gather Context

1. Get changes to review
   → git diff (uncommitted)
   → git diff --staged (staged only)
   → Read specific files

2. Read project conventions
   → plans/scout/README.md (Conventions section)
   → CLAUDE.md
   → .eslintrc, tsconfig.json

3. Read related spec (if UC provided)
   → plans/features/{feature}/specs/{UC}/README.md

Phase 2: Analyze Changes

For each changed file:

1. Understand the change
   - What was added/modified/removed?
   - What is the intent?

2. Check against spec (if available)
   - Does implementation match spec?
   - Any missing requirements?

3. Run through checklists
   - Security
   - Performance
   - Error handling
   - Style/conventions
   - Testing

Phase 3: Security Review

## Security Checklist

[ ] **Input Validation**
    - User input sanitized?
    - SQL/NoSQL injection prevented?
    - XSS prevented (HTML escaped)?

[ ] **Authentication**
    - Auth required where needed?
    - Token validated correctly?
    - Session handled securely?

[ ] **Authorization**
    - Permissions checked?
    - Can't access others' data?
    - Admin functions protected?

[ ] **Data Protection**
    - Passwords hashed?
    - Sensitive data not logged?
    - No secrets in code?

[ ] **API Security**
    - Rate limiting present?
    - CORS configured?
    - No sensitive data in URLs?

Common Security Issues:

// BAD: SQL injection
const query = `SELECT * FROM users WHERE id = ${userId}`;

// GOOD: Parameterized
const query = `SELECT * FROM users WHERE id = $1`;
await db.query(query, [userId]);

// BAD: XSS vulnerable
element.innerHTML = userInput;

// GOOD: Escaped
element.textContent = userInput;

// BAD: Hardcoded secret
const apiKey = "sk-1234567890";

// GOOD: Environment variable
const apiKey = process.env.API_KEY;

Phase 4: Quality Review

## Quality Checklist

[ ] **Error Handling**
    - Errors caught and handled?
    - User-friendly error messages?
    - Errors logged for debugging?
    - No swallowed errors?

[ ] **Performance**
    - No N+1 queries?
    - Large lists paginated?
    - Heavy operations async?
    - No memory leaks?

[ ] **Maintainability**
    - Code readable?
    - Functions not too long?
    - No magic numbers/strings?
    - DRY (no unnecessary duplication)?

[ ] **Testing**
    - New code has tests?
    - Edge cases covered?
    - Tests actually test something?

Common Quality Issues:

// BAD: N+1 query
const posts = await getPosts();
for (const post of posts) {
  post.author = await getUser(post.authorId); // Query per post!
}

// GOOD: Batch query
const posts = await getPosts({ include: { author: true } });

// BAD: Swallowed error
try {
  await doSomething();
} catch (e) {
  // Nothing - error disappears!
}

// GOOD: Handle or rethrow
try {
  await doSomething();
} catch (e) {
  logger.error('Failed to do something', e);
  throw new AppError('Operation failed', e);
}

// BAD: Magic number
if (retries > 3) { ... }

// GOOD: Named constant
const MAX_RETRIES = 3;
if (retries > MAX_RETRIES) { ... }

Phase 5: Convention Review

## Convention Checklist (from scout)

[ ] **Naming**
    - Variables: {convention from scout}
    - Files: {convention from scout}
    - Components: {convention from scout}

[ ] **Structure**
    - File in correct location?
    - Follows project patterns?
    - Imports organized?

[ ] **Style**
    - Matches .prettierrc / .eslintrc?
    - Consistent with codebase?
    - No linting errors?

[ ] **Git**
    - Commit message format correct?
    - No unrelated changes?
    - No debug code / console.log?

Phase 6: Spec Compliance (if UC provided)

## Spec Compliance

### Requirements Met
- [x] Login endpoint created
- [x] Returns token on success
- [x] Returns error on invalid credentials

### Requirements Not Met
- [ ] Rate limiting not implemented (spec said 5 attempts/min)

### Not in Spec
- Added "remember me" checkbox (is this approved?)

Phase 7: Generate Review

Compile findings into review output format.

Severity Levels:

Level	Icon	Meaning	Action
Critical	🔴	Security risk, bug, breaks functionality	Must fix before merge
Important	🟡	Performance, maintainability issues	Should fix
Suggestion	🔵	Style, improvements	Nice to have
Positive	✅	Good practice noted	Encouragement

Review Verdicts:

Verdict	When
✅ Approve	No critical/important issues
⚠️ Request Changes	Has critical or multiple important issues
❓ Needs Discussion	Unclear requirements, architectural concerns

Review Best Practices

Be Constructive

// BAD
"This code is bad"

// GOOD
"This could cause a SQL injection. Consider using parameterized queries:
```sql
SELECT * FROM users WHERE id = $1
```"

Explain Why

// BAD
"Don't use var"

// GOOD
"Use const/let instead of var - var has function scope which can lead to
unexpected behavior. const also signals intent that the value won't change."

Suggest Alternatives

// Issue + Solution
"The N+1 query here will cause performance issues with many posts.

Consider using an include/join:
```typescript
const posts = await db.posts.findMany({
  include: { author: true }
});
```"

Acknowledge Good Work

### Positives
- Clean separation of API and business logic
- Good error messages for users
- Comprehensive input validation

Tools Used

Tool	Purpose
Bash	git diff, git log
Read	Read changed files
Grep	Search for patterns
Glob	Find related files

Integration

Skill	Relationship
/dev-coding	Review after implementation
/dev-scout	Get project conventions
/dev-specs	Check spec compliance

Example Review

User: /dev-review UC-AUTH-001

Phase 1: Gather
- Get git diff for UC-AUTH-001 files
- Read scout conventions
- Read UC-AUTH-001 spec

Phase 2-6: Analyze
- src/api/auth/login.ts: Clean ✓
- src/components/LoginForm.tsx: 1 issue
- src/lib/api.ts: 1 suggestion

Phase 7: Output

## Review Summary

**Verdict**: ⚠️ Request Changes

**Stats**: 3 files, +245 additions, -12 deletions

### Issues Found

#### 🔴 Critical
None

#### 🟡 Important
- [error-handling] `src/components/LoginForm.tsx:34`
  Promise rejection not handled. If API fails, user sees nothing.
  ```tsx
  // Add error state
  .catch(err => setError(err.message))

🔵 Suggestions

• [naming] src/lib/api.ts:12 data is generic. Consider credentials for clarity.

Spec Compliance

• POST /api/auth/login works
• Returns token
• Validates input
• Missing: Rate limiting (spec requirement)

Positives

• Good validation on both client and server
• Clean component structure
• Proper TypeScript types