Sentry Code Review

Sentry Code Review Guidelines

When to Use This Skill

• Reviewing pull requests
• Performing security code reviews
• Checking code quality before merge
• Auditing existing code for issues

Review Checklist

1. Code Quality

## Quality Checks
- [ ] Code follows project style guide
- [ ] Functions are single-purpose and well-named
- [ ] No unnecessary complexity
- [ ] Proper error handling
- [ ] Adequate test coverage
- [ ] No dead code or commented-out blocks
- [ ] Dependencies are necessary and up-to-date

2. Security Review (OWASP)

## Security Checks

### Input Validation
- [ ] All user inputs are validated
- [ ] Input length limits enforced
- [ ] Allowlists used over blocklists
- [ ] No direct SQL query construction

### Authentication & Authorization
- [ ] Auth checks on all protected routes
- [ ] Session management is secure
- [ ] Password handling follows best practices
- [ ] No hardcoded credentials

### Data Protection
- [ ] Sensitive data is encrypted
- [ ] No secrets in code or logs
- [ ] PII handling follows regulations
- [ ] Proper data sanitization

### Injection Prevention
- [ ] Parameterized queries used
- [ ] No eval() or similar
- [ ] Template escaping enabled
- [ ] Safe deserialization

### Error Handling
- [ ] Errors don't leak sensitive info
- [ ] Proper logging (no secrets)
- [ ] Graceful degradation
- [ ] Rate limiting on sensitive endpoints

3. Performance

## Performance Checks
- [ ] No N+1 queries
- [ ] Appropriate caching
- [ ] Async operations where beneficial
- [ ] No blocking I/O in hot paths
- [ ] Database queries optimized
- [ ] Memory usage considered

4. Maintainability

## Maintainability Checks
- [ ] Clear documentation
- [ ] Intuitive naming
- [ ] Consistent patterns
- [ ] Easy to test in isolation
- [ ] No magic numbers/strings
- [ ] Proper abstraction level

Review Process

Step 1: Understand Context

# Get PR details
gh pr view <number>

# See changed files
gh pr diff <number>

# Check related issues
gh pr view <number> --json body | jq -r '.body'

Step 2: Review Changes

## For Each File, Ask:

1. **Purpose**: Does this change make sense?
2. **Correctness**: Will it work as intended?
3. **Edge Cases**: What could go wrong?
4. **Security**: Any vulnerabilities introduced?
5. **Performance**: Any bottlenecks?
6. **Tests**: Is it properly tested?

Step 3: Provide Feedback

## Feedback Categories

### 🔴 Blocking (Must Fix)
- Security vulnerabilities
- Data loss risks
- Breaking changes without migration
- Critical bugs

### 🟡 Should Fix
- Performance issues
- Code quality problems
- Missing tests
- Documentation gaps

### 🟢 Suggestions
- Style improvements
- Alternative approaches
- Future considerations
- Nice-to-haves

Common Issues to Watch For

Python

# BAD: SQL Injection
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")

# GOOD: Parameterized query
cursor.execute("SELECT * FROM users WHERE id = %s", (user_id,))

# BAD: Insecure deserialization
data = pickle.loads(user_input)

# GOOD: Safe parsing
data = json.loads(user_input)

# BAD: Path traversal
with open(f"/data/{filename}") as f:

# GOOD: Validate path
safe_path = os.path.join("/data", os.path.basename(filename))

JavaScript/TypeScript

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

// GOOD: Safe text content
element.textContent = userInput;

// BAD: Prototype pollution
Object.assign(target, userInput);

// GOOD: Validate and sanitize
const safeData = sanitize(userInput);

// BAD: Open redirect
window.location.href = req.query.redirect;

// GOOD: Validate URL
const url = new URL(req.query.redirect, window.location.origin);
if (url.origin === window.location.origin) {
  window.location.href = url.href;
}

React

// BAD: dangerouslySetInnerHTML with user input
<div dangerouslySetInnerHTML={{__html: userInput}} />

// GOOD: Use text content or sanitize
import DOMPurify from 'dompurify';
<div dangerouslySetInnerHTML={{__html: DOMPurify.sanitize(userInput)}} />

// BAD: Sensitive data in client bundle
const API_KEY = process.env.API_KEY; // Exposed!

// GOOD: Server-side only
// In API route, not client component

Review Comment Templates

Request Changes

## Changes Requested

### Security Issue
This code is vulnerable to [ISSUE TYPE].

**Current:**
```code
[problematic code]

Suggested:

[fixed code]

Why: [explanation of the risk]

### Approve with Suggestions

```markdown
## Approved with Suggestions

Nice work! A few optional improvements:

1. Consider [suggestion] for better [benefit]
2. [Another suggestion]

These aren't blocking - feel free to address in a follow-up.

Integration with CI

# .github/workflows/review.yml
name: Automated Review
on: [pull_request]

jobs:
  security-scan:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Run Semgrep
        uses: returntocorp/semgrep-action@v1
        with:
          config: p/security-audit

  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Lint
        run: npm run lint

Best Practices

• Review in small batches (< 400 lines)
• Take breaks between large reviews
• Test locally when unsure
• Ask clarifying questions
• Be constructive and specific
• Acknowledge good work too
• Follow up on blocking issues