Code Review Skill

Systematic code review process following industry best practices from Sentry, Trail of Bits, and Google engineering teams.

When to Use This Skill

• Reviewing pull requests
• Auditing code for security vulnerabilities
• Finding bugs and code smells
• Ensuring code quality standards

---

🔍 Review Process

1. Understand Context First

Before reviewing code:

• Read the PR description and linked issues
• Understand the business requirements
• Check if there are related tests or documentation

2. Review Checklist

✅ Correctness

• Does the code do what it's supposed to do?
• Are edge cases handled?
• Is error handling appropriate?
• Are there any obvious bugs?

✅ Security

• Input validation present?
• No hardcoded secrets/credentials?
• SQL injection prevention?
• XSS prevention?
• Authentication/authorization correct?
• Sensitive data properly handled?

✅ Performance

• No N+1 queries?
• Appropriate indexing?
• No memory leaks?
• Efficient algorithms used?
• Caching considered where appropriate?

✅ Code Quality

• Code is readable and self-documenting?
• No unnecessary complexity?
• DRY - no code duplication?
• Single responsibility principle followed?
• Consistent naming conventions?

✅ Testing

• Unit tests for new functionality?
• Edge cases tested?
• Error paths tested?
• Integration tests where needed?

✅ Documentation

• Public APIs documented?
• Complex logic explained?
• README updated if needed?

---

🐛 Common Issues to Look For

Go Code

// ❌ Bad: Ignoring errors
result, _ := doSomething()

// ✅ Good: Handle errors
result, err := doSomething()
if err != nil {
    return fmt.Errorf("failed to do something: %w", err)
}

// ❌ Bad: Defer in loop
for _, item := range items {
    f, _ := os.Open(item)
    defer f.Close()  // Won't close until function returns
}

// ✅ Good: Close immediately or use closure
for _, item := range items {
    func() {
        f, _ := os.Open(item)
        defer f.Close()
        // process f
    }()
}

TypeScript/React Code

// ❌ Bad: Missing dependency in useEffect
useEffect(() => {
  fetchData(userId);
}, []);  // userId missing

// ✅ Good: Include all dependencies
useEffect(() => {
  fetchData(userId);
}, [userId]);

// ❌ Bad: Inline object causing re-renders
<Component style={{ margin: 10 }} />

// ✅ Good: Stable reference
const style = useMemo(() => ({ margin: 10 }), []);
<Component style={style} />

SQL/Database

-- ❌ Bad: SQL injection risk
query := "SELECT * FROM users WHERE id = " + userInput

-- ✅ Good: Parameterized query
query := "SELECT * FROM users WHERE id = $1"
db.Query(query, userInput)

---

📝 Review Feedback Guidelines

Be Constructive

// ❌ Bad feedback
"This code is wrong"

// ✅ Good feedback
"This might cause a race condition when multiple goroutines access `sharedMap`. 
Consider using `sync.RWMutex` or `sync.Map` for thread-safe access."

Categorize Comments

• [Must Fix]: Security issues, bugs, data loss risks
• [Should Fix]: Performance issues, code smells
• [Suggestion]: Style improvements, optional enhancements
• [Question]: Seeking clarification

Provide Examples

// Instead of just pointing out the issue, show the fix:

**Issue**: Potential nil pointer dereference

**Current code**:
```go
return user.Name

Suggested fix:

if user == nil {
    return ""
}
return user.Name

---

# 🔐 Security Review Focus

## OWASP Top 10 Checklist

1. **Injection** - SQL, NoSQL, OS command injection
2. **Broken Authentication** - Session management, credential storage
3. **Sensitive Data Exposure** - Encryption, data masking
4. **XML External Entities** - XXE attacks
5. **Broken Access Control** - Authorization checks
6. **Security Misconfiguration** - Default credentials, error messages
7. **Cross-Site Scripting (XSS)** - Input/output encoding
8. **Insecure Deserialization** - Object validation
9. **Known Vulnerabilities** - Dependency versions
10. **Insufficient Logging** - Audit trails

---

# 📚 References

- [Google Code Review Guidelines](https://google.github.io/eng-practices/review/)
- [getsentry/code-review](https://github.com/getsentry/skills/tree/main/plugins/sentry-skills/skills/code-review)
- [trailofbits/differential-review](https://github.com/trailofbits/skills/tree/main/plugins/differential-review)
- [obra/requesting-code-review](https://github.com/obra/superpowers/blob/main/skills/requesting-code-review/SKILL.md)