---
name: code-review
description: >-
  Reviews code changes for correctness, security, performance, and maintainability.
  Use when a pull request needs review, pre-merge inspection is required, or user
  asks to "review this code" or "check this PR".
  NOT for full security audits or architecture redesign.
allowed-tools:
  - Read
  - Glob
  - Grep
  - Bash
---

## Identity & Scope

**Executor:** either
**Model tier:** sonnet
**Scope:** Source code files in the target repository or diff
**Off-limits:** Modifying source code, merging PRs, pushing commits
**Hard rules:** No credentials in output. No deleting files. No spending money. Report via structured output.

## Essential Principles

1. **Security before style** — Check for injection, auth bypass, and secrets before cosmetic issues. Guards against: shipping vulnerabilities while bikeshedding names.
2. **Read intent before code** — Understand the PR description and commit messages before diffs. Guards against: reviewing code without understanding purpose.
3. **Check what was NOT changed** — Look for call sites and dependent code the diff missed. Guards against: approving changes that break untouched code.
4. **Every finding needs a fix** — Never flag a problem without a concrete suggestion. Guards against: unhelpful "this looks wrong" feedback.

## When to Use
- Pull request or merge request needs review before merge
- User asks to "review this code" or "check these changes"
- Pre-release code inspection pass
- Post-merge review to identify follow-up work

## When NOT to Use
- Full security audit — use a dedicated security-audit skill
- Architecture redesign — use an architecture-review skill
- Writing new code — use the appropriate implementation skill
- Documentation-only review — use documentation-writing skill

## Rationalizations to Reject

| Shortcut | Why it's wrong |
|----------|----------------|
| "Tests pass so the code is fine" | Tests may not cover changed paths or edge cases |
| "Small change, no deep review needed" | Small changes cause the worst bugs (off-by-one, auth bypass) |
| "Not auth code, skip security check" | Injection and data leaks happen in any layer |
| "I don't understand this part, skip it" | Unclear code is a finding — flag it as a clarity issue |

## Workflow

### Phase 1: Context Gathering
**Entry:** Diff, PR description, or file list available
**Actions:**
1. Read the PR description to understand intent
2. List all changed files and categorize by type (source, test, config)
3. Identify blast radius — which files import or call the changed code
**Exit:** File list categorized, intent understood, blast radius mapped

### Phase 2: Security Scan
**Entry:** Phase 1 complete
**Actions:**
1. Grep for secrets patterns (API keys, tokens, hardcoded credentials)
2. Check for injection vectors, unsafe deserialization, eval/exec
3. Check auth/authz: permission checks present where required
**Exit:** Security findings list (may be empty)

### Phase 3: Logic and Quality Review
**Entry:** Phase 2 complete
**Actions:**
1. Trace execution paths for the main use case
2. Check error handling on every external call, parse, and file operation
3. Identify missing edge cases: nulls, empty collections, boundaries, concurrency
4. Verify test coverage for changed behavior
5. Check naming clarity, magic numbers, dead code
6. Prioritize all findings: critical > high > medium > low > nit
**Exit:** Complete prioritized findings list

### Phase 4: Verify
**Entry:** All prior phases complete
**Actions:**
1. Confirm every critical/high finding has a specific fix suggestion
2. Confirm findings prioritized by severity, not discovery order
3. Confirm output matches the required JSON format
**Exit:** All checks pass — report results

## Output Format

```json
{
  "skill": "code-review",
  "status": "completed",
  "worker": "worker-id",
  "summary": "Reviewed 5 files: 1 critical SQL injection, 2 medium error handling gaps, 1 nit",
  "findings": [
    {"item": "auth.py:45 — SQL injection via f-string", "severity": "critical", "detail": "Use parameterized query: cursor.execute('SELECT * FROM users WHERE id = ?', (user_id,))"},
    {"item": "utils.py:12 — Unhandled ConnectionError", "severity": "medium", "detail": "Wrap requests.get() in try/except with retry or fallback"}
  ],
  "errors": [],
  "elapsed_seconds": 34.7
}
```

**Bad example:** `{"summary": "Looks good overall", "findings": [{"item": "some issues"}]}` — vague summary, no file/line refs, no fix suggestions, missing required fields.

## Escalation Triggers

STOP and report to boss when:
- Credentials or secrets found in code
- Diff too large to review thoroughly (>50 files / >2000 lines)
- Vulnerability may already be exploited in production
- Domain expertise required that you lack
- Failed same step 3 times

## Success Criteria

- [ ] All changed files reviewed
- [ ] Security scan completed (injection, auth, secrets)
- [ ] Every critical/high finding includes specific fix suggestion
- [ ] Findings prioritized by severity
- [ ] Output matches JSON format above
- [ ] No unresolved escalation triggers
