dev-review

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

```

/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

```

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)

```markdown

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

```

Phase 1: Gather Context

```

1. Get changes to review

→ git diff (uncommitted)

→ git diff --staged (staged only)

→ Read specific files

1. Read project conventions

→ plans/scout/README.md (Conventions section)

→ CLAUDE.md

→ .eslintrc, tsconfig.json

1. 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?

1. Check against spec (if available)

- Does implementation match spec?

- Any missing requirements?

1. Run through checklists

- Security

- Performance

- Error handling

- Style/conventions

- Testing

```

Phase 3: Security Review

```markdown

[ ] 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:

```typescript

// 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

```markdown

[ ] 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:

```typescript

// 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

```markdown

[ ] 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)

```markdown

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 |

Be Constructive

```markdown

// 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

```markdown

// 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

```markdown

// 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

```markdown

Positives

• Clean separation of API and business logic
• Good error messages for users
• Comprehensive input validation

```

| Tool | Purpose |

|------|---------|

| Bash | git diff, git log |

| Read | Read changed files |

| Grep | Search for patterns |

| Glob | Find related files |

| Skill | Relationship |

|-------|--------------|

| /dev-coding | Review after implementation |

| /dev-scout | Get project conventions |

| /dev-specs | Check spec compliance |

```

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

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

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

Positives

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

```