bootstrap/guides/CODE-REVIEW.md

# Code Review Guide

## Hard Requirement

If an agent modifies source code, code review is REQUIRED before completion.
Do not mark code-change tasks done until review is completed and blockers are resolved or explicitly tracked.
If code/config/API contract/auth behavior changed and required docs are missing, this is a BLOCKER.
If tests pass but acceptance criteria are not verified by situational evidence, this is a BLOCKER.
If implementation diverges from `docs/PRD.md` or `docs/PRD.json` without PRD updates, this is a BLOCKER.

Merge strategy enforcement (HARD RULE):
- PR target for delivery is `main`.
- Direct pushes to `main` are prohibited.
- Merge to `main` MUST be squash-only.
- Use `~/.config/mosaic/tools/git/pr-merge.sh -n {PR_NUMBER} -m squash` (or PowerShell equivalent).

## Review Checklist

### 1. Correctness
- [ ] Code does what the issue/PR description says
- [ ] Code aligns with active PRD requirements
- [ ] Acceptance criteria are mapped to concrete verification evidence
- [ ] Edge cases are handled
- [ ] Error conditions are managed properly
- [ ] No obvious bugs or logic errors

### 2. Security
- [ ] No hardcoded secrets or credentials
- [ ] Input validation at boundaries
- [ ] SQL injection prevention (parameterized queries)
- [ ] XSS prevention (output encoding)
- [ ] Authentication/authorization checks present
- [ ] Sensitive data not logged
- [ ] Secrets follow Vault structure (see `docs/vault-secrets-structure.md`)

### 2a. OWASP Coverage (Required)
- [ ] OWASP Top 10 categories were reviewed for change impact
- [ ] Access control checks verified on protected actions
- [ ] Cryptographic handling validated (keys, hashing, TLS assumptions)
- [ ] Injection risks reviewed for all untrusted inputs
- [ ] Security misconfiguration risks reviewed (headers, CORS, defaults)
- [ ] Dependency/component risk reviewed (known vulnerable components)
- [ ] Authentication/session flows reviewed for failure paths
- [ ] Logging/monitoring preserves detection without leaking sensitive data

### 3. Testing
- [ ] Tests exist for new functionality
- [ ] Tests cover happy path AND error cases
- [ ] Situational tests cover all impacted change surfaces (primary gate)
- [ ] Tests validate required behavior/outcomes, not only internal implementation details
- [ ] TDD was applied when required by `~/.config/mosaic/guides/QA-TESTING.md`
- [ ] Coverage meets 85% minimum
- [ ] Tests are readable and maintainable
- [ ] No flaky tests introduced

### 4. Code Quality
- [ ] Follows Google Style Guide for the language
- [ ] Functions are focused and reasonably sized
- [ ] No unnecessary complexity
- [ ] DRY - no significant duplication
- [ ] Clear naming for variables and functions
- [ ] No dead code or commented-out code

### 4a. TypeScript Strict Typing (see `TYPESCRIPT.md`)
- [ ] **NO `any` types** — explicit types required everywhere
- [ ] **NO lazy `unknown`** — only for error catches with immediate narrowing
- [ ] **Explicit return types** on all exported/public functions
- [ ] **Explicit parameter types** — never implicit any
- [ ] **No type assertions** (`as Type`) — use type guards instead
- [ ] **No non-null assertions** (`!`) — use proper null handling
- [ ] **Interfaces for objects** — not inline types
- [ ] **Discriminated unions** for variant types
- [ ] **DTO files used at boundaries** — module/API contracts are in `*.dto.ts`, not inline payload types

### 5. Documentation
- [ ] Complex logic has explanatory comments
- [ ] Required docs updated per `~/.config/mosaic/guides/DOCUMENTATION.md`
- [ ] Public APIs are documented
- [ ] Private/internal APIs are documented
- [ ] API input/output schemas are documented
- [ ] API permissions/auth requirements are documented
- [ ] Site map updates are present when navigation changed
- [ ] README updated if needed
- [ ] Breaking changes noted

### 6. Performance
- [ ] No obvious N+1 queries
- [ ] No blocking operations in hot paths
- [ ] Resource cleanup (connections, file handles)
- [ ] Reasonable memory usage

### 7. Dependencies
- [ ] No deprecated packages
- [ ] No unnecessary new dependencies
- [ ] Dependency versions pinned appropriately

## Review Process

Use `~/.config/mosaic/templates/docs/DOCUMENTATION-CHECKLIST.md` whenever code/API/auth/infra changes are present.

### Getting Context
```bash
# List the issue being addressed
~/.config/mosaic/tools/git/issue-list.sh -i {issue-number}

# View the changes
git diff main...HEAD
```

### Providing Feedback
- Be specific: point to exact lines/files
- Explain WHY something is problematic
- Suggest alternatives when possible
- Distinguish between blocking issues and suggestions
- Be constructive, not critical of the person

### Feedback Categories
- **Blocker**: Must fix before merge (security, bugs, test failures)
- **Should Fix**: Important but not blocking (code quality, minor issues)
- **Suggestion**: Optional improvements (style preferences, nice-to-haves)
- **Question**: Seeking clarification

### Review Comment Format
```
[BLOCKER] Line 42: SQL injection vulnerability
The user input is directly interpolated into the query.
Use parameterized queries instead:
`db.query("SELECT * FROM users WHERE id = ?", [userId])`

[SUGGESTION] Line 78: Consider extracting to helper
This pattern appears in 3 places. A shared helper would reduce duplication.
```

## After Review
1. Update issue with review status
2. If changes requested, assign back to author
3. If approved, note approval in issue comments
4. For merges, ensure CI passes first
5. Merge PR to `main` with squash strategy only