# Code Review Standards

## Purpose

Code review ensures quality, security, and maintainability before code is merged. This rule is the **canonical definition of review lanes, severity, and the merge gate** — other review rules and commands reference it rather than restating.

## Review lanes (non-overlapping)

Review is split so no two reviewers check the same thing — this keeps each reviewer focused and makes synthesis dedupe trivial.

| Lane | Owner | Covers |
|------|-------|--------|
| **Architecture & Correctness** | inline (main session) | SAD/ADR/layer-boundary conformance, logic correctness, edge cases, null handling, error-handling strategy, dead code |
| **Language** | `{lang_agent}` (csharp/python/typescript-reviewer) | language idioms, naming conventions, async mechanics, type safety, framework-usage footguns, design tokens (UI) |
| **Security** | `security-reviewer` agent | OWASP Top 10, injection, secrets, auth/authz, data exposure — see `security.md` + the agent's rubric |
| **Schema** | `database-reviewer` agent | schema design, indexes, migration safety, query performance, data integrity |
| **AWS infra** | `aws-reviewer` agent | IAM/S3/Lambda/networking security, cost, reliability (when IaC in scope) |

The inline reviewer does **not** do language style, security, or schema — those are agent lanes. Don't duplicate them.

## Review criteria (Architecture & Correctness lane — inline)

Priority order for the inline reviewer (the other lanes are defined in their agents):

1. **Architecture** — violations of SAD, ADR decisions, layer boundaries
2. **Correctness** — logic errors, edge cases, null handling, error-handling strategy
3. **Conventions** — structure, commit/branch format per CLAUDE.md (language idioms → `{lang_agent}`)
4. **Cohesion** — functions focused (<50 lines), files cohesive (<800 lines), no deep nesting (>4 levels), no dead/commented-out code

Every finding MUST reference a specific `file:line` and propose a fix. No general comments.

## Severity & merge gate (canonical)

| Level | Meaning | Gate action |
|-------|---------|-------------|
| **Critical** | Security vuln or data-loss risk | **BLOCK** — must fix before merge/Done |
| **High** | Bug or significant quality issue | **BLOCK** — must fix before merge/Done |
| **Medium** | Maintainability concern | **Advisory** — list, recommend fix, may continue |
| **Low** | Style / minor suggestion | **Advisory** — optional |

> All reviewers (inline + agents) emit findings in these four buckets so synthesis merges without re-bucketing. Any **Critical or High** blocks; Medium/Low are advisory. `post-implementation-review.md` and the review commands defer to this table — they do not redefine the gate.

## Output format

Findings grouped by severity, skip empty categories:

```
### Critical
- `file.cs:42` — issue + suggested fix

### High
- `file.cs:15` — issue + suggested fix

### Medium / Low
- `file.cs:8` — issue
```

## When to review

- After writing or modifying code; before committing to shared branches
- When security-sensitive code changes (auth, payments, user data) → the security lane is mandatory, never size-skipped
- When architectural changes are made

(PR-specific pre-merge checks — CI green, no conflicts, branch up to date — are enforced by `/tas-review-pr`, not here.)

## Integration with other rules

- `security.md` — canonical security checklist (Security lane)
- `testing.md` / `tdd.md` — test coverage requirements
- `post-implementation-review.md` — the isolated-agent review flow that applies these lanes + gate