<!-- .github/pull_request_template.md -->

## What does this PR do?
<!-- One sentence summary. What problem does it solve? -->

## Why is this approach right?
<!-- Why this solution over alternatives? Link to any design docs or tickets. -->

## What should reviewers focus on?
<!-- Highlight the tricky parts, areas of uncertainty, or where you want specific feedback. -->

## Testing
- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] Manually tested: [describe what you tested and how]
- [ ] No test needed: [explain why]

## Checklist
- [ ] CHANGELOG updated (if user-facing change)
- [ ] Documentation updated (if behavior changed)
- [ ] Feature flag added (if gradual rollout needed)
- [ ] Migration is reversible (if database change)
- [ ] No secrets or credentials in diff

## Code Review Checklist

### Correctness
- [ ] The code does what the PR description says it does
- [ ] Edge cases are handled (empty inputs, nil/null, zero values, max values)
- [ ] Error paths are handled and return meaningful errors
- [ ] Concurrent access is handled correctly (no race conditions, proper locking)
- [ ] External service failures are handled (timeouts, retries, circuit breakers)
- [ ] No silent failures (errors logged or returned, not swallowed)

### Security
- [ ] No secrets, credentials, or PII in code or logs
- [ ] User input is validated and sanitized before use
- [ ] SQL queries use parameterized statements, not string formatting
- [ ] Authorization checks are present (not just authentication)
- [ ] New dependencies checked for known vulnerabilities
- [ ] No new attack surfaces opened (SSRF, path traversal, etc.)

### Performance
- [ ] No N+1 queries (use bulk fetches or JOINs where needed)
- [ ] Database queries use appropriate indexes
- [ ] No blocking operations in hot paths
- [ ] Memory allocations are reasonable (no unbounded slices/maps)
- [ ] Caching used appropriately (not over-cached, TTLs set)

### Maintainability
- [ ] Code is readable without needing the author to explain it
- [ ] Functions do one thing (< 50 lines is a reasonable guide, not a rule)
- [ ] Variable and function names communicate intent
- [ ] Complex logic has comments explaining *why*, not *what*
- [ ] No dead code or commented-out code
- [ ] Constants used instead of magic numbers/strings

### Testing
- [ ] Tests cover the happy path
- [ ] Tests cover error/failure cases
- [ ] Tests are deterministic (no time.Sleep, no external dependencies)
- [ ] Test names describe the scenario being tested

### Operations
- [ ] New configuration values have documented defaults
- [ ] Logging is structured (not fmt.Println / console.log)
- [ ] Metrics/tracing added for new paths (if applicable)
- [ ] Database migrations are idempotent and reversible
- [ ] Feature flagged if risky or needing gradual rollout

<!-- .github/pull_request_template.md -->

## Type of change
- [ ] Bug fix (non-breaking, fixes an issue)
- [ ] New feature (non-breaking, adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to break)
- [ ] Refactoring (no functional changes)
- [ ] Documentation update

## How was this tested?
<!-- Describe your testing approach -->

## Reviewer Notes
<!-- Anything specific you want reviewers to focus on -->

---

### Author Checklist
- [ ] I have tested this change locally
- [ ] I have added/updated unit tests
- [ ] I have updated documentation if needed
- [ ] I have not committed secrets or credentials
- [ ] Database migrations are reversible

### Reviewer Checklist (delete items not applicable)
- [ ] Correctness: edge cases, error handling, race conditions
- [ ] Security: input validation, auth checks, no exposed secrets
- [ ] Performance: no N+1 queries, appropriate indexes
- [ ] Readability: naming, comments on complex logic
- [ ] Tests: coverage of failure cases, deterministic

## Review Comment Tags

**[BLOCK]** — Must be addressed before merge. Correctness or security issue.
Example: `[BLOCK] This query is vulnerable to SQL injection — use parameterized query`

**[SUGGEST]** — Strong recommendation, but reviewer won't block merge.
Example: `[SUGGEST] Consider extracting this into a separate function for testability`

**[NIT]** — Minor style/naming preference. Author's call.
Example: `[NIT] 'userData' is more idiomatic than 'user_data' in Go`

**[QUESTION]** — Asking for understanding, not necessarily requesting change.
Example: `[QUESTION] Why does this use a goroutine instead of sequential processing?`

**[FYI]** — Sharing context, no action needed.
Example: `[FYI] There's a related issue in the payments module you might want to track`

## Code Review SLA

| PR Size | First Review | Final Approval |
|---------|-------------|----------------|
| Small (<100 lines) | 4 hours | 8 hours |
| Medium (100-500 lines) | 8 hours | 24 hours |
| Large (>500 lines) | 24 hours | 48 hours |
| Hotfix | 1 hour | 2 hours |

### Rules
- PRs under 400 lines require 1 approval. Over 400 lines require 2.
- Authors respond to review comments within 4 business hours.
- If a reviewer doesn't respond within SLA, author may request reassignment.
- Reviewers who can't review within SLA must reassign promptly.
- Do not merge without at least one approval, even for small changes.