---
slug: "ia-code-review-x-2"
source_type: "clawhub"
source_url: "https://clawhub.ai/skills/compound-eng-code-review"
repo: ""
source_file: "description"
---
---
name: ia-code-review
class: discipline
description: >-
  Structured code reviews with severity-ranked findings and deep multi-agent
  mode. Use when performing a code review, auditing code quality, or critiquing
  PRs, MRs, or diffs.
---

# Code Review

**Caller contract:** when the invoking task already defines scope, base SHA, or an output contract (subagent protocols, orchestrated reviews), skip Scope Resolution, Review Mode Selection, and Output Format — apply only the review discipline (two-stage check, severity, evidence rules, anti-patterns) within that contract.

## Two-Stage Review

**Stage 1 -- Spec compliance** (do this FIRST): verify the changes implement what was intended — check the PR description, issue, or task spec for missing requirements, unnecessary additions, interpretation gaps. If the implementation is wrong, stop here -- reviewing quality on the wrong feature wastes effort.

**Stage 2 -- Code quality**: only after Stage 1 passes, review for correctness, maintainability, security, and performance.

## Scope Resolution

**Pre-flight**: verify `git rev-parse --git-dir` exists before anything else. If not in a git repo, ask for explicit file paths — ask via AskUserQuestion (Claude Code; load with ToolSearch `select:AskUserQuestion` if not loaded) or request_user_input (Codex); fall back to numbered options in chat. Later asks reuse this channel.

When no specific files are given, resolve scope via this fallback chain:
1. User-specified files/directories (explicit request)
2. Session-modified files (`git diff --name-only`, unstaged + staged)
3. All uncommitted files (`git diff --name-only HEAD`)
4. Untracked files (`git ls-files --others --exclude-standard`) -- often the most review-worthy
5. **Zero files → stop.** Ask what to review (ask channel above).

Exclude: lockfiles, minified/bundled output, vendored/generated code.

### Base-branch resolution for branch reviews

When the review target is a branch (not a working-tree diff), the comparison range is the **merge-base**, not the working-tree delta — resolve it before reading any diff. Fallback chain (PR base → default-branch inference → `origin/*` → `git merge-base` → unshallow retry), stacked-branch detail, and the "never fall back to `git diff HEAD`" rule in [scope-resolution.md](./references/scope-resolution.md). Stacked branches: prefer the platform's `base_sha` (`gh pr diff`) — a local merge-base over-covers.

**Off-scope filter (always, after any branch review): intersect finding paths with the change's `--name-only` set; discard non-intersecting findings.**

## Review Mode Selection

**Run this BEFORE reading the full diff.** Use metadata only (`git diff --stat`, file list from scope resolution) — reading the diff first creates analysis momentum that bypasses mode selection.

**Exceptions first** — these change types stay single-pass regardless of signal count: pure documentation/markdown changes; mechanical refactors (renames, moves) with no logic changes; single-file changes under 50 lines.

**Verification-mechanism carve-out:** even when a change stays single-pass by the exceptions above, if it *is* a verification mechanism (CI/CD gate, merge-block check, coverage/lint gate, build/deploy step, or test infra/mock that could mask a real failure), apply the "can this silently false-pass?" lens during the single-pass review — the mechanism can go green while the thing it guards is red. In deep review this same lens runs as a size-independent red-team trigger (see [deep-review.md](./references/deep-review.md)).

| Signal | Threshold |
|--------|-----------|
| Lines changed (excluding test files) | >300 |
| Files touched (excluding test files) | >8 |
| Top-level directories spanned (non-test) | >3 |
| Security-sensitive paths (auth, crypto, payments, permissions) | any |
| Database migrations | any |
| API surface changes (public endpoints, exported interfaces) | any |

**Test file exclusion:** filter test paths out of the size signals with `git diff --stat -- ':!tests/' ':!*.test.*' ':!*.spec.*' ':!*_test.*'` and report both totals: "450 lines changed (280 excluding tests)."

**3+ signals → deep review.** Inform the user, then dispatch parallel specialist agents per [deep-review.md](./references/deep-review.md). Pass the diff to agents -- do NOT read it first. **Stop here -- skip the Review Process section.**

**2 signals → suggest** (ask channel above): "This touches N files across M modules. Deep review?"

**0-1 signals → standard review.** Proceed to Review Process below.

Override: `deep` forces multi-agent, `quick` forces single-pass.

## Review Process

**Standard reviews only** -- deep review is handled by the dispatched specialists.

1. **Context** — before reading code:
   - **Scope drift**: compare `git diff --stat` against the PR's stated intent. Classify CLEAN / DRIFT DETECTED / REQUIREMENTS MISSING; on drift, ask the author: ship as-is, split, or remove?
   - **Intent**: read the PR description, linked issue, or task spec. Deviation or under-delivery is a finding — the wrong problem solved correctly is still wrong.
   - **Prior discussions**: reconcile existing review comments so resolved issues aren't re-raised. Gate on a presence check; commands in [scope-resolution.md](./references/scope-resolution.md).
   - **Automated gates**: run the project's test/lint suite (canonical commands in CI config).
2. **Structural scan** -- architecture, file organization, API surface; flag breaking changes. Added (`A`) files on a remote branch: use the diff content, not the working tree.
3. **Line-by-line** -- correctness, edge cases, error handling, naming, readability. Category checklists (correctness, maintainability, performance, adversarial, AI-code lens) in [check-categories.md](./references/check-categories.md); load the profile matching the diff's file extensions from [language-profiles.md](./references/language-profiles.md) (TS/React, Python, PHP, Shell/CI, Config, Data, Security, LLM Trust). Prefer questions ("What happens if `input` is empty?") over declarations.
4. **Security** -- input validation, auth checks, secrets exposure, injection vectors (SQL, XSS, CSRF, SSRF, command, path traversal, unsafe deserialization), race conditions (TOCTOU). Grep-able patterns for the common vulnerability classes in [security-patterns.md](./references/security-patterns.md).
5. **Test coverage** -- untested new paths, error paths, and behavioral changes without test updates. Flag implementation-coupled tests (mocked internals, private methods) -- test behavior, not wiring.
6. **Reliability** -- error handling completeness, timeout/retry, resource cleanup on error paths, graceful degradation. Patterns in [reliability-patterns.md](./references/reliability-patterns.md).
7. **Removal candidates** -- dead code, unused imports, cleanup-ready feature flags; safe-to-delete (no references) vs defer-with-plan.
8. **Verify** -- run formatter/lint/tests on touched files; state what was skipped and why. Note doc staleness (README/ARCHITECTURE/CONTRIBUTING) as informational.
9. **Summary** -- findings grouped by severity with verdict: **Ready to merge / Ready with fixes / Not ready**.

**Large diffs:** >500 lines → review by module, not file-by-file. Flag oversized PRs (ideal ~100-300 meaningful lines) and suggest a split — thresholds and the four split strategies in [pr-sizing.md](./references/pr-sizing.md).

## Severity and Confidence

Four severity tiers (Critical / Important / Medium / Minor) order the report; a confidence score (0.0-1.0) per finding decides what lands in it:

**Confidence bands: ≥0.70 report · 0.60-0.69 report-if-actionable · <0.60 suppress (exception: Critical security findings report at ≥0.50).**

Full 5-band rubric, evidence-before-severity ordering, false-positive suppression categories, and the LLM prompt-injection exception in [severity-and-confidence.md](./references/severity-and-confidence.md).

Evidence lives in the `CR-XXX` entry itself — `[file:line]` plus `` `quoted code` ``, not only in surrounding prose. Never fabricate references.

## Action Routing

Classify every finding's fix into one of four tiers:

- `safe_auto` -- deterministic, local, behavior-preserving fix: apply directly
- `gated_auto` -- concrete fix crossing a behavior/contract/permission/API boundary: present it, wait for sign-off
- `manual` -- author judgment, rewrite, or redesign needed: flag with fix intent, don't apply
- `advisory` -- report-only risk signal: record under Residual Risks

Full decision rules and conflict resolution in [action-routing.md](./references/action-routing.md). When in doubt, escalate to `gated_auto` — never promote toward `safe_auto` on disagreement.

## Comment Labels

Prefix inline comments so authors know what requires action: *(no prefix)* = required change (Critical/Important), blocks merge; **Nit:** = style preference, optional; **Consider:** = suggestion worth evaluating, not blocking; **FYI:** = informational, no action expected.

## Anti-Patterns in Reviews

- Nitpicking style when linters exist -- defer to automated tools
- "While you're at it..." scope creep -- open a separate issue
- Blocking on personal preference -- approve with a Minor comment
- Skipping Stage 1 -- never review code quality before verifying spec compliance; rubber-stamping without reading is not a review
- Recommending fix patterns without checking currency -- verify the pattern is current for the project's framework version; prefer newer built-in alternatives
- Fighting documented overrides -- a rationale-backed bypass (`CLAUDE.md`, `AGENTS.md`, inline comment) is owner-blessed: honor it, don't re-raise; if the rationale is missing, suggest documenting one. Plan-mandated defects are not self-justifying — report them labeled "plan-mandated" for the human to adjudicate
- Resting a finding on an unverified absence -- read the region or grep the *exact* symbol expecting zero lines; a subagent's confident negative or a broad-pattern hit is not proof. When the finding rests on *exhaustive* coverage ("this symbol is unused", "nothing else calls this", "safe to change"), grep is the weakest tier, not the top one: prefer symbol-aware search (LSP or an MCP equivalent, which follows renames, re-exports, and barrel files), then structural AST search (`ast-grep`, which skips the string and comment hits regex reports), then text grep -- which stays correct for genuinely lexical checks like config keys and log messages. Fall through without ceremony to whatever the repo actually has. Dynamic dispatch, reflection, DI containers, string-keyed routes or config, generated code, and external consumers hide usages from every tier; when coverage was grep-only or one of those could apply, record the boundary in Residual Risks (`callsite completeness: grep-only`) or step the finding down rather than asserting absence. A finding that does not turn on exhaustive coverage needs no such note.
- Calling a change a regression without a baseline read -- read the pre-change file (`git show <base>:<file>`), not just the hunk; cite the introducing commit when confirmed
- Widening/narrowing a key or guard without checking the mirror bug -- name one concrete opposite-defect case along the now-ignored axis before accepting the fix
- Checking only one projection on a hide/filter/redact change -- enumerate every field surfacing the same entity (list, `*_count`/`*_ids`, raw documents, detail view); require a test per field
- Pre-classifying own findings as weak -- no "INFO only" / "no action required" wording; anchor severity in concrete constants from the code, not hypotheticals

Extended rationale for the last six traps — and the broader trap catalog — in [review-traps-catalog.md](./references/review-traps-catalog.md).

## When to Stop and Ask

- Fixing the issues would require an API redesign beyond the PR's scope
- Intent behind a change is ambiguous -- ask rather than assume
- Missing validation tooling (no linter, no tests) -- flag the gap, don't guess

## Output Format

```
## Review: [brief title]

### Critical
- **CR-001.** [file:line] `quoted code` -- [issue]. Score: [0.0-1.0]. [Impact if not fixed]. Fix: [concrete suggestion].

### Important / ### Medium
- (same shape; Important adds Consider: [alternative approach])

### Minor
- **CR-004.** [file:line] -- [observation].

### What's Working Well
- [specific positive observation with why it's good]

### Residual Risks
- [unresolved assumptions, areas not covered, open questions]

### Verdict
Ready to merge / Ready with fixes / Not ready -- [one-sentence rationale]
```

Number findings `CR-001`, `CR-002`... sequentially across severities for stable IDs. Cap 10 per severity; note any overflow and show the highest-impact ones.

**Markdown safety:** in table cells, escape literal `|` as `\|` — code excerpts with pipes (`a | b`, `string | null`) split rows silently. Bullet output is pipe-safe.

Multi-agent consolidation: apply the merge algorithm in [deep-review.md](./references/deep-review.md) (same-line dedupe, severity conflicts, `NEEDS DECISION`, cross-lens confidence boosts).

**Clean review (no findings):** a valid outcome, not insufficient effort — say so explicitly and summarize what was checked.

## References

References load at their point of use above. Additionally: [security-test-coverage.md](./references/security-test-coverage.md) — security-audit deliverable checklist; [false-positive-suppression.md](./references/false-positive-suppression.md) — framework-idiom and test-specific FP categories; [external-review-subprocess.md](./references/external-review-subprocess.md) — external-CLI reviewer protocol (heartbeat tolerance, run-until-clean, frozen-diff binding).

## Integration

- `ia-receiving-code-review` -- inbound side. Tier map: `safe_auto` ≈ AUTO-FIX, `gated_auto` ≈ ESCALATE-for-approval, `manual` ≈ ESCALATE, `advisory` ≈ FYI
- `ia-kieran-reviewer` agent -- persona-driven Python/TypeScript deep quality review
- `/ia-review` -- full ceremony (worktrees, ultra-thinking); deep review here is lighter: parallel specialists, no worktrees
- `/resolve-pr-parallel` command -- batch-resolve PR comments with parallel agents
- `ia-security-sentinel` agent -- deep security audit; threat-model mode for new trust boundaries