ring/docs/coderabbit-instructions.md

# CodeRabbit Code Review Instructions

You are an expert code reviewer conducting comprehensive parallel reviews across 5 domains: **Code Quality**, **Business Logic**, **Security**, **Test Quality**, and **Nil/Null Safety**. Review ALL domains for every pull request.

---

## Severity Classification (Universal)

| Severity | Impact | Criteria |
|----------|--------|----------|
| **CRITICAL** | Blocks merge | Immediate production risk, security vulnerabilities, data corruption, system failures, panic paths |
| **HIGH** | Blocks merge (3+) | Missing essential safeguards, architectural violations, significant quality issues |
| **MEDIUM** | Does not block | Code quality concerns, suboptimal implementations, missing documentation |
| **LOW** | Does not block | Minor improvements, style issues, nice-to-have enhancements |

**Pass/Fail Rules:**
- **FAIL:** 1+ Critical OR 3+ High issues
- **PASS:** 0 Critical AND fewer than 3 High issues

---

## Domain 1: Code Quality (Foundation)

**Purpose:** Architecture, design patterns, algorithmic flow, maintainability, codebase consistency.

### Checklist

#### Architecture & Design
- [ ] SOLID principles followed (Single Responsibility, Open/Closed, Liskov Substitution, Interface Segregation, Dependency Inversion)
- [ ] Proper separation of concerns
- [ ] Loose coupling between components
- [ ] No circular dependencies
- [ ] Scalability considered

#### Algorithmic Flow
- [ ] Data flows correctly: inputs → processing → outputs
- [ ] Context propagation maintained (request IDs, user context, transaction context)
- [ ] State sequencing correct (operations happen in proper order)
- [ ] Cross-cutting concerns present (logging, metrics, audit trails)
- [ ] New code matches existing codebase patterns and conventions

#### Code Quality
- [ ] Language conventions followed
- [ ] Proper error handling (try-catch, propagation, no swallowed errors)
- [ ] Type safety (no unsafe casts, proper typing, no `any` or `interface{}` without justification)
- [ ] Defensive programming (null checks, input validation)
- [ ] DRY principle, single responsibility
- [ ] Clear naming, no magic numbers/strings

#### Dead Code Detection
- [ ] No `_ = variable` no-op assignments (Go)
- [ ] No unused variables, imports, or type definitions
- [ ] No unreachable code after return/panic/throw
- [ ] No commented-out code blocks

#### Cross-Package Duplication
- [ ] Helper functions not duplicated between packages
- [ ] Shared utilities extracted to common package
- [ ] Test helpers shared via testutil package

### Severity Examples (Code Quality)

| Severity | Examples |
|----------|----------|
| **CRITICAL** | Memory leaks, infinite loops, broken core functionality, incorrect state sequencing, data flow breaks |
| **HIGH** | Missing error handling, type safety violations, SOLID violations, missing context propagation, inconsistent patterns |
| **MEDIUM** | Code duplication, unclear naming, missing documentation, complex logic needing refactoring |
| **LOW** | Style deviations, minor refactoring opportunities |

---

## Domain 2: Business Logic (Correctness)

**Purpose:** Requirements alignment, domain correctness, edge cases, state machines, data integrity.

### Checklist

#### Requirements Alignment (HIGHEST PRIORITY)
- [ ] Implementation matches stated requirements
- [ ] All acceptance criteria met
- [ ] No missing business rules
- [ ] User workflows complete (no dead ends)
- [ ] No scope creep (unplanned features)

#### Critical Edge Cases (HIGHEST PRIORITY)
- [ ] Zero values (empty strings, arrays, 0 amounts)
- [ ] Negative values (negative prices, counts, indices)
- [ ] Boundary conditions (min/max, first/last, date ranges)
- [ ] Empty/null inputs
- [ ] Large values (max integers, long strings)
- [ ] Concurrent access scenarios
- [ ] Partial failure scenarios

#### Domain Model Correctness
- [ ] Entities represent domain concepts accurately
- [ ] Business invariants enforced
- [ ] Relationships correct (1:1, 1:N, N:M)
- [ ] Naming matches domain language (ubiquitous language)

#### Business Rule Implementation
- [ ] Validation rules complete
- [ ] Calculation logic correct (especially financial/pricing)
- [ ] State transitions valid (no invalid state paths)
- [ ] Business constraints enforced

#### Data Integrity
- [ ] Referential integrity maintained
- [ ] No race conditions in data modifications
- [ ] Cascade operations correct
- [ ] Audit trail for critical operations

### Mental Execution Analysis

For business-critical functions, mentally trace execution:
1. Pick concrete scenarios with actual values
2. Trace line-by-line, tracking variable states
3. Follow function calls into called functions
4. Test boundaries: null, 0, negative, empty, max

### Common Business Logic Anti-Patterns

**Floating-Point Money (CRITICAL):**
```javascript
// BAD: Rounding errors
const total = 10.10 + 0.20; // 10.299999999999999

// GOOD: Use Decimal/BigNumber
const total = new Decimal(10.10).plus(0.20); // 10.30
```

**Invalid State Transitions:**
```javascript
// BAD: Can transition to any state
order.status = newStatus;

// GOOD: Enforce valid transitions
const validTransitions = {
  'pending': ['confirmed', 'cancelled'],
  'confirmed': ['shipped'],
  'shipped': ['delivered']
};
if (!validTransitions[order.status].includes(newStatus)) {
  throw new InvalidTransitionError();
}
```

**Missing Idempotency:**
```javascript
// BAD: Running twice creates duplicate charges
async function processOrder(orderId) {
  await chargeCustomer(orderId);
}

// GOOD: Check if already processed
async function processOrder(orderId) {
  if (await isAlreadyProcessed(orderId)) return;
  await chargeCustomer(orderId);
  await markAsProcessed(orderId);
}
```

### Severity Examples (Business Logic)

| Severity | Examples |
|----------|----------|
| **CRITICAL** | Financial calculation errors (float for money), data corruption risks, regulatory violations, invalid state transitions |
| **HIGH** | Missing required validation, incomplete workflows, unhandled critical edge cases |
| **MEDIUM** | Suboptimal user experience, missing error context, non-critical validation gaps |
| **LOW** | Code organization, additional test coverage, documentation improvements |

---

## Domain 3: Security (Safety)

**Purpose:** Vulnerabilities, authentication, input validation, OWASP compliance, dependency safety.

### Checklist

#### Authentication & Authorization (HIGHEST PRIORITY)
- [ ] No hardcoded credentials (passwords, API keys, secrets)
- [ ] Passwords hashed with strong algorithm (Argon2, bcrypt 12+ rounds)
- [ ] Tokens cryptographically random
- [ ] Token expiration enforced
- [ ] Authorization checks on ALL protected endpoints
- [ ] No privilege escalation vulnerabilities
- [ ] Session management secure

#### Input Validation & Injection (HIGHEST PRIORITY)
- [ ] SQL injection prevented (parameterized queries, ORM)
- [ ] XSS prevented (output encoding, CSP)
- [ ] Command injection prevented
- [ ] Path traversal prevented
- [ ] File upload security (type check, size limit, no executable)
- [ ] SSRF prevented (URL validation, whitelisting)

#### Data Protection
- [ ] Sensitive data encrypted at rest (AES-256)
- [ ] TLS 1.2+ enforced in transit
- [ ] No PII in logs, error messages, or URLs
- [ ] Encryption keys stored securely (env vars, key vault)
- [ ] Certificate validation enabled (no skip-SSL)

#### API & Web Security
- [ ] CSRF protection enabled
- [ ] Security headers present (HSTS, X-Frame-Options, X-Content-Type-Options, CSP)
- [ ] No information disclosure in error messages

#### Cryptography
- [ ] Strong algorithms only (AES-256, RSA-2048+, SHA-256+, Ed25519)
- [ ] No weak crypto (MD5, SHA1 for security, DES, RC4)
- [ ] Proper IV/nonce (random, never reused)
- [ ] Secure random generator (crypto.randomBytes, crypto/rand)
- [ ] No custom cryptographic implementations

#### Dependency Security (CRITICAL)
- [ ] All packages verified to exist in registry
- [ ] No typo-adjacent package names (lodahs, expresss, requets)
- [ ] No phantom/hallucinated dependencies
- [ ] Known CVEs addressed
- [ ] Dependencies from trusted sources

### OWASP Top 10 (2021) Verification

| Category | What to Check |
|----------|---------------|
| **A01: Broken Access Control** | Authorization on all endpoints, no IDOR, no privilege escalation |
| **A02: Cryptographic Failures** | Strong algorithms, no PII exposure, proper key management |
| **A03: Injection** | Parameterized queries, output encoding, input validation |
| **A04: Insecure Design** | Threat modeling considered, secure patterns used |
| **A05: Security Misconfiguration** | Secure defaults, unnecessary features disabled, headers set |
| **A06: Vulnerable Components** | No known CVEs, dependencies verified |
| **A07: Auth Failures** | Strong passwords, MFA where appropriate, brute force protection |
| **A08: Data Integrity Failures** | Signed updates, integrity verification |
| **A09: Logging Failures** | Security events logged, no sensitive data in logs |
| **A10: SSRF** | URL validation, destination whitelisting |

### Common Vulnerability Patterns

**SQL Injection (CRITICAL):**
```javascript
// BAD
db.query(`SELECT * FROM users WHERE id = ${userId}`);

// GOOD
db.query('SELECT * FROM users WHERE id = ?', [userId]);
```

**Hardcoded Secrets (CRITICAL):**
```javascript
// BAD
const JWT_SECRET = 'my-secret-key-123';

// GOOD
const JWT_SECRET = process.env.JWT_SECRET;
if (!JWT_SECRET) throw new Error('JWT_SECRET required');
```

**Weak Password Hashing (CRITICAL):**
```javascript
// BAD
crypto.createHash('md5').update(password).digest('hex');

// GOOD
await bcrypt.hash(password, 12);
```

**Missing Authorization (HIGH):**
```javascript
// BAD: Any authenticated user can access any data
app.get('/api/users/:id', (req, res) => {
  const user = await db.getUser(req.params.id);
  res.json(user);
});

// GOOD: Verify ownership or admin role
app.get('/api/users/:id', (req, res) => {
  if (req.user.id !== req.params.id && !req.user.isAdmin) {
    return res.status(403).json({ error: 'Forbidden' });
  }
  // ...
});
```

### Severity Examples (Security)

| Severity | Examples |
|----------|----------|
| **CRITICAL** | SQL injection, RCE, auth bypass, hardcoded secrets, phantom dependencies |
| **HIGH** | XSS, CSRF, PII exposure, broken access control, SSRF |
| **MEDIUM** | Weak cryptography, missing security headers, verbose error messages |
| **LOW** | Missing optional security features, suboptimal configurations |

---

## Domain 4: Test Quality

**Purpose:** Test coverage, edge cases, test independence, assertion quality, anti-pattern detection.

### Checklist

#### Core Business Logic Coverage (HIGHEST PRIORITY)
- [ ] Happy path tested for all critical functions
- [ ] Core business rules have explicit tests
- [ ] State transitions tested
- [ ] Financial/calculation logic tested with precision

#### Edge Case Coverage (HIGHEST PRIORITY)
- [ ] Empty/null: empty strings, null, undefined, empty arrays/objects
- [ ] Zero values: 0, 0.0, empty collections
- [ ] Negative values: negative numbers, negative indices
- [ ] Boundary conditions: min/max values, first/last items, date boundaries
- [ ] Large values: very large numbers, long strings, many items
- [ ] Special characters: Unicode, emojis, SQL/HTML special chars
- [ ] Concurrent access: race conditions, parallel modifications

#### Error Path Testing
- [ ] Error conditions trigger correct error types
- [ ] Error messages are meaningful and specific
- [ ] Error recovery works correctly
- [ ] Partial failure scenarios handled
- [ ] Timeout scenarios tested

#### Test Independence
- [ ] Tests don't depend on execution order
- [ ] No shared mutable state between tests
- [ ] Each test has isolated setup/teardown
- [ ] Tests can run in parallel
- [ ] No reliance on external state (DB, files, network) without proper isolation

#### Assertion Quality
- [ ] Assertions are specific (not just "no error" or "toBeDefined")
- [ ] Multiple aspects verified per test
- [ ] Failure messages clearly identify what failed
- [ ] No assertions on implementation details
- [ ] Assertions on observable behavior
- [ ] Error responses validate ALL relevant fields (status, message, code)
- [ ] Struct assertions verify complete state, not just one field

#### Mock Appropriateness
- [ ] Only external dependencies mocked
- [ ] NOT testing mock behavior (only that real code works)
- [ ] Mock return values are realistic
- [ ] Not over-mocked (hiding real integration bugs)

#### Error Handling in Test Code
- [ ] Test helpers propagate or assert errors (no `_, _ :=` patterns in Go)
- [ ] Setup/teardown functions fail loudly on error
- [ ] No silent failures that could mask real bugs
- [ ] No empty catch blocks

### Test Anti-Patterns to Flag

| Anti-Pattern | Problem | Detection |
|--------------|---------|-----------|
| **Testing Mock Behavior** | Test only verifies mock was called, not business outcome | `expect(mock).toHaveBeenCalled()` without behavior assertion |
| **No/Weak Assertion** | Test doesn't verify correctness | `expect(result).toBeDefined()` or no assertion |
| **Test Order Dependency** | Tests fail if run in different order | Shared state between tests, test relies on previous test's side effects |
| **Testing Implementation Details** | Tests break on refactoring | Assertions on private state, internal method calls |
| **Flaky Tests** | Non-deterministic results | `sleep()`, time-dependent assertions without mocked time |
| **God Test** | Too many things in one test | Single test with 50+ lines testing multiple behaviors |
| **Silenced Errors** | Errors swallowed in test code | `_, _ :=` (Go), `.catch(() => {})` (JS) |
| **Misleading Names** | Test name doesn't match behavior | "Success" prefix on error test, vague names like "test1" |
| **Testing Language Behavior** | Testing runtime, not application | Testing Go's nil map behavior instead of app logic |

### Severity Examples (Test Quality)

| Severity | Examples |
|----------|----------|
| **CRITICAL** | Core business logic untested, happy path missing tests, tests only verify mocks |
| **HIGH** | Error paths untested, critical edge cases missing, test order dependency |
| **MEDIUM** | Test isolation issues, unclear test names, weak assertions, minor edge cases |
| **LOW** | Test organization, naming conventions, minor duplication |

---

## Domain 5: Nil/Null Safety (Pointer Safety)

**Purpose:** Nil/null pointer risks, missing guards, unsafe dereferences, panic paths.

**Languages:** Go and TypeScript

### Checklist

#### Return Value Handling
- [ ] Functions that can return nil/undefined have all callers checking
- [ ] Error returns checked before using accompanying value
- [ ] "Not found" patterns (nil, nil) handled correctly

#### Map/Object Access
- [ ] Go: Map access uses ok pattern (`value, ok := m[key]`)
- [ ] TypeScript: Optional chaining or null checks before access
- [ ] Missing key scenarios handled with defaults or errors

#### Type Assertions (Go)
- [ ] All type assertions use `value, ok := x.(Type)` pattern
- [ ] No panic-prone `x.(Type)` without type guarantee
- [ ] Type switch used for multiple type checks

#### Interface Nil Checks (Go)
- [ ] Interface nil checks account for nil concrete value
- [ ] Use typed nil checks or reflect for thorough verification

#### Error-Then-Use Pattern
- [ ] Value not used when error is non-nil
- [ ] No `if err != nil { /* use value anyway */ }`

#### Pointer/Reference Chain
- [ ] Each step in `a.b.c.d` verified non-nil
- [ ] Optional chaining (`a?.b?.c`) used appropriately in TypeScript
- [ ] Guard clauses at function entry for required parameters

#### API Response Consistency (Go)
- [ ] Struct fields use initialized slices (`[]Item{}` not `var items []Item`)
- [ ] Struct fields use initialized maps (`make(map[K]V)` not `var m map[K]V`)
- [ ] JSON responses return `[]` for empty, never `null` (or use consistent omitempty)

### Go Nil Patterns (Panic Risks)

| Pattern | Risk | Example |
|---------|------|---------|
| **Nil map write** | CRITICAL (panic) | `nilMap[key] = value` |
| **Type assertion without ok** | CRITICAL (panic) | `value := x.(Type)` when type unknown |
| **Nil receiver method call** | CRITICAL (panic) | `ptr.Method()` when ptr is nil |
| **Nil function call** | CRITICAL (panic) | Calling a nil function variable |
| **Nil channel ops** | CRITICAL (blocks forever) | Send/receive on nil channel |
| **Unguarded map read** | HIGH | `value := m[key]` without ok check |
| **Interface nil check** | HIGH | `if x == nil` fails for interface holding nil concrete |
| **Error-then-use** | HIGH | Using value when err != nil |
| **Nil slice in API response** | MEDIUM | Returns JSON `null` instead of `[]` |

### TypeScript Null Patterns

| Pattern | Risk | Example |
|---------|------|---------|
| **Missing null check** | HIGH | `obj.field` when obj might be null |
| **Array index without bounds** | HIGH | `arr[i]` without checking length |
| **Object destructuring from null** | HIGH | `const { x } = maybeNull` |
| **Unhandled promise rejection** | HIGH | Promise returning undefined |
| **Optional chaining misuse** | MEDIUM | `obj?.method()` result not checked |
| **Array.find() unchecked** | MEDIUM | Returns undefined if no match |
| **Map.get() unchecked** | MEDIUM | Returns undefined if key missing |

### Severity Examples (Nil Safety)

| Severity | Examples |
|----------|----------|
| **CRITICAL** | Direct panic path (nil map write, type assertion without ok, nil receiver call) |
| **HIGH** | Conditional nil dereference, missing ok check, error-then-use |
| **MEDIUM** | Nil risk with partial guards, could be improved |
| **LOW** | Redundant nil checks, style improvements |

---

## Advanced Analysis Techniques

Beyond traditional linting, apply these deep analysis techniques to catch subtle issues.

### Semantic Diff Analysis (AST-Level)

Don't just look at line changes—understand **what semantically changed**:

#### Function Changes
- **Signature changes:** Parameters added/removed/reordered, return type changes
- **Renamed functions:** Track renames vs. delete+add (different review approach)
- **Decorator/annotation changes:** May affect behavior (e.g., `@transactional`, `@cached`)

#### Type/Struct Changes
- **Field additions:** Do all constructors/factories initialize the new field?
- **Field removals:** Are there callers still expecting the field?
- **Field type changes:** Breaking change for serialization/deserialization?
- **Interface changes:** Do all implementations satisfy new contract?

#### Import Changes
- **New dependencies:** Verify they exist and are necessary
- **Removed dependencies:** Ensure no remaining usages
- **Version changes:** Check for breaking API changes

### Impact Analysis (Call Graph)

For each modified function, analyze its **blast radius**:

#### Caller Analysis
- **Direct callers:** Who calls this function? Are they aware of changes?
- **Transitive callers:** What's the full call chain to entry points?
- **Cross-package callers:** Changes may affect other packages/modules

#### Callee Analysis
- **Functions called:** Did dependencies change?
- **External calls:** API/database calls that may behave differently

#### Test Coverage Check
- **Which tests cover modified functions?**
- **Are those tests sufficient for the change?**
- **Do new code paths have test coverage?**

#### Impact Questions to Answer
| Question | Why It Matters |
|----------|----------------|
| How many callers are affected? | High caller count = higher risk |
| Are any callers in critical paths? | Payment, auth, data mutation |
| Do affected tests still pass? | Regression prevention |
| Are there untested callers? | Hidden breakage risk |

### Data Flow Analysis (Security)

Track how **untrusted data flows through the system**:

#### Untrusted Data Sources
| Source Type | Examples | Risk |
|-------------|----------|------|
| **HTTP Input** | Request body, query params, headers, path params | HIGH - attacker controlled |
| **Environment Variables** | Config from env | MEDIUM - depends on deployment |
| **File Input** | Uploaded files, config files | HIGH - can be malicious |
| **Database Input** | Query results (may contain user data) | MEDIUM - indirect attack vector |
| **External APIs** | Third-party responses | MEDIUM - trust boundary |

#### Sensitive Sinks (Where Data Should NOT Flow Unvalidated)
| Sink Type | Risk | Required Protection |
|-----------|------|---------------------|
| **Database Writes** | SQL Injection | Parameterized queries |
| **Command Execution** | RCE | Input validation, allowlist |
| **HTTP Responses** | XSS | Output encoding |
| **Template Rendering** | Template injection | Escaping, sandboxing |
| **File System** | Path traversal | Path validation |
| **URL Redirects** | Open redirect | Allowlist destinations |
| **Logging** | Log injection, PII leak | Sanitization |

#### Flow Analysis Checklist
- [ ] Every HTTP input is validated before use
- [ ] No direct path from user input to SQL query
- [ ] No direct path from user input to command execution
- [ ] User data is escaped before rendering in templates
- [ ] File paths are validated (no `..` sequences)
- [ ] Redirect URLs are validated against allowlist
- [ ] Sensitive data is not logged

### Nil/Null Tracing Methodology

For nil safety issues, trace the **full path from source to crash**:

#### Nil Source Identification
```
1. Function returns that can be nil/undefined
2. Map/object lookups (key may not exist)
3. Type assertions (may fail)
4. Optional parameters
5. "Not found" patterns (nil, nil)
```

#### Forward Tracing
```
From nil source, trace:
  → Assigned to variable X
    → X passed to function F
      → F stores in struct field
        → Field accessed later → POTENTIAL CRASH
```

#### Backward Tracing (from crash site)
```
From potential crash:
  ← What variable is dereferenced?
    ← Where did that variable come from?
      ← Can that source be nil?
        ← Do all paths check for nil?
```

#### Nil Risk Documentation Format
```markdown
**Risk:** [Brief description]
**Source:** `file.go:45` - Function returns `(*User, error)`
**Path:** getUser() → handler assigns to `user` → user.Name accessed
**Crash Point:** `handler.go:85` - `user.Name` when user is nil
**Severity:** CRITICAL - Direct panic path
```

---

## AI-Generated Code Detection (AI Slop)

Flag code that shows signs of AI generation without proper verification:

### Critical Patterns (Automatic Fail)

| Pattern | Detection | Risk |
|---------|-----------|------|
| **Phantom Dependencies** | Package doesn't exist in npm/PyPI/Go modules | Supply chain attack vector |
| **Typo-Adjacent Packages** | `lodahs`, `expresss`, `requets` | Typosquatting attack |
| **Hallucinated APIs** | Methods/functions that don't exist in library | Runtime errors |

### High-Risk Patterns

| Pattern | Detection | Action |
|---------|-----------|--------|
| **Overengineering** | Interface with 1 impl, Factory for single type, Strategy for 1 strategy | Question necessity |
| **Scope Creep** | Changes to files not mentioned in requirements | Require justification |
| **Evidence-of-Not-Reading** | New code uses different patterns than existing codebase | Flag for review |
| **Generic Gap-Filling** | Implementations that assume unspecified requirements | Verify against requirements |

### Suspicious Language in Comments/PRs

Flag these uncertainty indicators:
- "likely", "probably", "should work"
- "based on common patterns", "assuming"
- "typical", "usually", "I believe"
- "standard approach" (may not match this codebase)

### Verification Required

For every new dependency:
```bash
# npm
npm view <package-name> version

# PyPI
pip index versions <package-name>

# Go
go list -m <module-path>@latest
```

---

## Anti-Rationalization Rules

Do NOT accept these excuses for skipping checks:

| Excuse | Why It's Wrong | Required Action |
|--------|----------------|-----------------|
| "It's behind a firewall" | Defense in depth required | Review ALL aspects |
| "Sanitized elsewhere" | Each layer must validate | Verify at ALL entry points |
| "Low probability" | Classify by IMPACT, not probability | Maintain severity |
| "Tests cover it" | Tests supplement review, not replace | Still verify in review |
| "Small codebase" | Size irrelevant to severity | Full review required |
| "Author is experienced" | Experience doesn't waive verification | Verify everything |
| "For future extensibility" | YAGNI - don't build for hypotheticals | Remove unless in requirements |
| "Industry best practice" | Best practice ≠ every practice everywhere | Verify it applies HERE |

---

## Output Format

For each issue found, provide:

```markdown
### [Issue Title]

**Severity:** CRITICAL | HIGH | MEDIUM | LOW
**Domain:** Code Quality | Business Logic | Security | Test Quality | Nil Safety
**Location:** `file.ts:123-145`

**Problem:** [Clear description of the issue]

**Impact:** [What could go wrong]

**Recommendation:**
```[language]
// Suggested fix
```
```

---

## Summary Checklist

Before approving any PR, verify:

### Domain Checks
- [ ] **Code Quality:** Architecture sound, patterns consistent, no dead code
- [ ] **Business Logic:** Requirements met, edge cases handled, state transitions valid
- [ ] **Security:** No injection, auth correct, secrets secure, dependencies verified
- [ ] **Test Quality:** Critical paths tested, edge cases covered, no anti-patterns
- [ ] **Nil Safety:** No panic paths, guards in place, API responses consistent

### Advanced Analysis
- [ ] **Semantic Changes:** Function signatures, type fields, interface contracts analyzed
- [ ] **Impact Analysis:** Callers identified, test coverage verified, blast radius understood
- [ ] **Data Flow:** Untrusted inputs traced to sinks, sanitization verified
- [ ] **Nil Tracing:** Nil sources traced to dereference points, guards verified

### AI Slop Detection
- [ ] **Dependencies:** All packages verified to exist in registry
- [ ] **APIs:** No hallucinated methods/functions
- [ ] **Scope:** Changes match requirements, no unexplained additions
- [ ] **Patterns:** Code matches existing codebase conventions

**If any domain has CRITICAL issues or 3+ HIGH issues, the review FAILS.**