Review

---
description: Peer code review — agent-to-agent or agent-to-human. Confidence-scored, blocks ship on low confidence.
---

# Peer Code Review

Review **implemented code** against requirements, architecture, and quality standards.
This is NOT engineering review (`/eng`) — this reviews actual code that has been written.

**Input:** Branch with commits to review (auto-detected or specified).
**Output:** Review verdict with confidence score. Blocking issues must be resolved before `/ship`.

> Follows `rules/integrity.md` — evidence over confidence (I1), honest completeness (I6 — state what you could not check), verify before asserting (I7), state assumptions Verified/Inferred/Unverified (I8), neutrality check on your own recommendation (I9), question inherited rules (I11). The quality bar: write every finding as if a senior engineer and an AI reviewer will re-read it in 12 months.

---

## When to Use

| Situation | Skill |
|-----------|-------|
| Reviewing a plan before coding | `/eng` |
| Reviewing code after implementation | **`/review`** |
| Testing the running app | `/qa` |
| Pre-landing security/data check | `/ship` (built-in) |

`/review` fills the gap between `/eng` (engineering plan) and `/qa` (behavior).
It catches design drift, quality issues, and architectural violations
that tests won't find and QA can't see.

---

## Presentation Rules

Follow the shared formatting rules in `skills/shared/formatting.md`.

1. **Progress indicator** — every output starts with:

```
/review ═════════════════════════════════════════════════════════

  ▸ Phase 1  Scope the Review
  ○ Phase 2  Review Checklist
  ○ Phase 3  Findings
  ○ Phase 4  Verdict

═════════════════════════════════════════════════════════════════
```

Update `▸` (current), `✓` (done), `○` (pending) as phases progress.
Completed phases show a status note (e.g., `✓ 12 files`, `✓ 3 blocking`).

---

## Phase 1: Scope the Review

### Independence check — delegate to subagent if this thread implemented the code

**Before doing anything else, ask yourself:** *did I (the agent currently
running this skill) write or modify the code being reviewed in this
conversation?*

Detection signals — if **any** are true, this thread is contaminated:

- You ran `/tdd`, `/fix`, `/simplify`, or `/eng` followed by code changes in this conversation.
- You called `Edit`, `Write`, or `NotebookEdit` on files in the diff under review.
- You created the commits being reviewed (`git commit` in this thread, or visible in your tool-call history).
- You authored the spec/eng plan immediately before this review and the diff implements it.

**If contaminated → delegate.** Spawn the reviewer subagent and return
its verdict. Do NOT continue inline:

```
Agent(
  subagent_type: "hashb:reviewer",
  description: "Independent review of {branch} → {base}",
  prompt: "Review the diff on the current branch against {base}. Spec/eng context: {brief summary or path to /spec, /eng artifacts}. Task: #{TASK_ISSUE} in {TODOS file} (include if resolved). Tracker: {TRACKER_TYPE} (include if set). Return verdict per skills/review/SKILL.md Phase 4."
)
```

**Tracker context forwarding:** when delegating, include `Task:` and
`Tracker:` fields in the prompt if they are available in this thread's
context. The subagent has no conversation history and cannot resolve these
on its own. Without them, the subagent's badge label operation silently
skips — which is safe but loses cross-session visibility.

Emit a one-line preface to the user before the Agent call so they see
the delegation: `▸ Implementing context detected — delegating to reviewer subagent for independent verdict.`

When the subagent returns, render its verdict as your Phase 4 output and stop. Do not re-run the checklist on top of it.

**If clean → run inline.** Emit a one-line acknowledgement so the user
knows the check ran: `▸ No implementing context in this thread — running review inline.` Then proceed with the bash block below.

**Subagent path is naturally non-recursive.** When the `reviewer` subagent
itself reads this skill, its conversation has no prior tool calls or
commits — the detection signals all return false, and it runs inline
within its own fresh context. No special-casing needed.

**Tracker integration** — run the Tracker Detection block from
`skills/shared/tracker.md` §Tracker Detection if `TRACKER_TYPE` is not yet
cached. Then run the §Issue Resolution Block to resolve `TASK_ISSUE` if not
already set (from upstream `Task:` handoff, user message, or branch name).
If `TRACKER_TYPE=github-issues` and `TASK_ISSUE` is resolved, swap the
issue's status label to `status:review` using the §Status Swap Block. This
signals the task has entered code review.

### Branch / base detection

```bash
# Branch/base detection — see skills/shared/preflight.md
BRANCH=$(git branch --show-current 2>/dev/null || echo "unknown")
if command -v gh >/dev/null 2>&1; then
  BASE=$(gh pr view --json baseRefName -q .baseRefName 2>/dev/null \
    || gh repo view --json defaultBranchRef -q .defaultBranchRef.name 2>/dev/null \
    || echo "main")
else
  echo "⚠ gh CLI not found — defaulting BASE to 'main'"
  BASE="main"
fi

echo "REVIEWING: $BRANCH → $BASE"
git diff $BASE...HEAD --stat
git log $BASE..HEAD --oneline
```

### Graph-Aware Context (if graphify MCP is available)

If the consumer's Project Profile lists `graphify` as an MCP server,
query it to sharpen the review scope:

1. **Blast radius** — for each changed function/class, query the graph for
   callers, dependents, and affected tests. Review these alongside the diff —
   they're the code most likely to break from this change.
2. **Minimal review set** — use the graph's structural map instead of reading
   entire files. This reduces token usage while improving review quality.
3. **Dependency direction** — the graph can verify that dependency flow matches
   architecture (2A checks) without manual import tracing.

If graphify is not available, fall back to standard file reads.
Don't block the review on graph availability.

**Gather context:**
- Read the original requirement / user request
- Read `/eng` review summary if it exists (check conversation or commit messages)
- Read `specs/{slug}.md` if referenced in the HANDOFF `Spec file:` field
  or discoverable in `specs/`. This is the canonical acceptance criteria
  for §2A design fidelity checks. If `Source: engineering-inferred`, treat
  AC gaps as WARNING not BLOCKING (inferred ACs may be incomplete).
- Read any ADRs or architecture docs referenced
- Check active rules that apply to changed files
- Read Project Profile from consumer's `CLAUDE.md` (if present) — adapt security
  and performance checks to the declared stack and architecture
- Check Claude Code memories for relevant project/feedback memories from prior
  retros — recurring patterns flagged by `/retro` should inform review severity
- Check for rule overrides — if a consumer rule has `overrides:` targeting a
  generic rule, the override's guidance takes precedence for its matched paths

```bash
# Files changed — these are the review scope
git diff $BASE...HEAD --name-only
```

**Scope rule:** Review ONLY the diff. Don't review pre-existing code unless
the diff makes it worse or introduces a dependency on it.

---

## Phase 2: Review Checklist

Work through each category **in order**. The sequence is intentional:
spec/requirement compliance first (2A), then code quality (2B-2F). If the
implementation doesn't match the spec, code quality feedback is wasted effort —
the code may need to be rewritten. Catch design drift before polishing details.

For each finding, classify severity and confidence.

### 2A. Design Fidelity

Does the code match the plan?

- **Architecture compliance** — Does the implementation follow the approved approach
  from `/eng`? Flag deviations.
- **Module boundaries** — Are bounded contexts preserved? No direct cross-module
  DB access, no bypassing public APIs.
- **Dependency direction** — Dependencies flow the right way? No circular imports,
  no upstream depending on downstream internals.
- **Contract adherence** — Do new APIs match their planned signatures? Are events
  published as designed?
- **Design principles** — Check against: Single Responsibility, Explicit
  Dependencies, Composition over Inheritance, Separation of Concerns,
  Immutability by Default, Small Interfaces / Narrow Contracts.
  Adapt checks to the consumer's declared Stack:

  | Stack | What to look for |
  |---|---|
  | C# / .NET | Constructor DI via `IServiceCollection`, interface segregation, records/readonly for immutability, service layer separation |
  | React / Next.js | Component composition + custom hooks (not inheritance), props/context for dependencies, `useState`/`useReducer` (never mutate), small prop surfaces, server/client component separation |
  | TypeScript / Node | Module-level composition, dependency injection via constructors or factory functions, narrow exported interfaces |
  | Python | Parameter injection, protocols/ABCs for contracts, frozen dataclasses/tuples for immutability, module-level separation |

  Flag violations with specific principle and file:line.
- **AC coverage** — Does the implementation satisfy every MUST AC in
  `specs/{slug}.md`? Flag any AC with no corresponding code path.
- **Testability** — Can each new unit be tested in isolation? Are dependencies
  injectable? Are contracts narrow? Flag tightly-coupled code that requires
  integration tests where unit tests should suffice.

### 2B. Code Quality

- **Error handling** — Errors caught and handled meaningfully? No swallowed
  exceptions, no generic catch-all without re-throw.
- **Simplification scan** — Note obvious complexity, duplication, dead code, or
  naming issues as SUGGESTION-severity findings. For anything beyond surface-level,
  recommend `/simplify` as a follow-up rather than blocking the review.
  Do NOT duplicate `/simplify`'s full analysis — flag the smell, defer the fix.

> **Relationship to /simplify:** `/review` detects code quality smells at a
> surface level. `/simplify` performs deep analysis and applies changes.
> If significant complexity or duplication is noted, route through `/simplify`
> before `/ship`.

### 2C. Test Coverage

- **New codepaths tested?** — Every new function/method has at least one test.
- **Edge cases covered?** — Nil, empty, boundary, error paths.
- **Regression test for bugs?** — If this is a bugfix, is there a test that
  would have caught it?
- **Test quality** — Tests assert behavior, not implementation. No brittle
  mocks of internals.
- **Test coverage proportional to risk** — Critical business logic, data
  mutations, auth flows, and payment paths require thorough unit + integration
  tests. Utility functions and simple getters don't need dedicated tests
  unless they have edge cases. Apply industry-standard judgment — the goal
  is trust and confidence, not 100% coverage for its own sake.

### 2D. Security & Data Safety

- **Input validation** — User inputs validated at the boundary?
- **Auth checks** — New endpoints/mutations protected?
- **Secrets** — No hardcoded credentials, tokens, or keys?
- **SQL/XSS/injection** — Parameterized queries, escaped output?
- **Data migrations** — Reversible? Backfill plan for existing data?
- **Dependency versions** — Any new or changed dependencies must target the
  latest stable release. Flag outdated, pre-release (RC, canary, next), or
  abandoned (no release in >12 months) packages. Verify via Context7
  `resolve-library-id` when uncertain.

### 2E. Performance

- **N+1 queries** — Loops that hit the database?
- **Unbounded results** — Missing pagination or limits?
- **Missing indexes** — New queries without supporting indexes?
- **Resource cleanup** — Connections, file handles, subscriptions closed?

### 2F. Rule Compliance

Check the diff against every active rule that matches the changed files:

```bash
# Which rules activate for these files?
# Cross-reference changed files against rules/ paths: frontmatter
```

**Rule override handling:** If a consumer rule has `overrides: <rule-id>` in its
frontmatter, the override rule's guidance takes precedence over the generic rule
for the paths the override matches. Rules without overrides still apply normally.

Flag any rule violations with rule name and specific line.

### 2G. Accessibility

Skip if the diff has no UI changes. For UI changes, check signal-only — don't
re-derive WCAG, surface load-bearing gaps:

- **Keyboard path** — Every interactive action reachable without a pointer?
  Drag-and-drop, custom widgets, and gesture-only flows need a keyboard
  alternative. Missing keyboard path → **BLOCKING**.
- **Focus management** — Focus moves correctly on route change, modal open/close,
  async content insertion? No focus traps in non-modal contexts? Missing focus
  on a new dialog/drawer or a trap with no escape → **BLOCKING**.
- **Color-only signaling** — State (error, success, required) conveyed by more
  than color alone (icon, text, pattern)? Color-only → **WARNING**.
- **Screen-reader announcement** — Async state changes (loading → loaded,
  validation errors, toasts) announced via `aria-live` or equivalent? Missing
  announcement on a state change a sighted user would see → **WARNING**.

Style of finding: name the specific element and the missing affordance, not
the standard. "Drawer at `OrderDrawer.tsx:42` has no Escape-to-close" beats
"violates WCAG 2.1.1".

---

## Phase 3: Findings

For each issue found:

```
⚠ FINDING #{N}
  ─────────────────────────────────────────────────
  Severity      BLOCKING | WARNING | SUGGESTION
  Confidence    HIGH | MEDIUM | LOW
  Category      design | quality | test | security | performance | rule | accessibility
  File          {path}:{line}
  Description   {what's wrong}
  Suggestion    {how to fix — be specific}
```

**Severity definitions:**
- **BLOCKING** — Must fix before ship. Security holes, data loss risk,
  architectural violations, missing tests for critical paths.
- **WARNING** — Should fix. Quality issues, missing edge-case tests,
  performance concerns. Ship at user's discretion.
- **SUGGESTION** — Could improve. Naming, style, minor simplifications.
  Don't block on these.

**Confidence definitions:**
- **HIGH** — Certain this is an issue. Clear rule violation, obvious bug,
  proven pattern.
- **MEDIUM** — Likely an issue but context might justify it. Flag for
  discussion.
- **LOW** — Possible concern, might be wrong. Needs domain knowledge
  the reviewer doesn't have.

---

## Phase 4: Verdict

### Confidence Score

Calculate overall review confidence:

```
REVIEW SCORE
─────────────────────────────────────────────────
  Findings       {N} blocking, {N} warning, {N} suggestion
  Confidence     {weighted average of finding confidence}
  Test coverage  {assessed qualitatively: strong | adequate | weak | missing}
```

### Verdict

| Condition | Verdict | Action |
|-----------|---------|--------|
| 0 blocking, confidence HIGH | **APPROVED** | Proceed to `/ship` |
| 0 blocking, confidence MEDIUM | **APPROVED WITH NOTES** | Proceed, address warnings |
| Any blocking, confidence HIGH | **CHANGES REQUESTED** | Fix blockers, re-review |
| Any blocking, confidence LOW | **ESCALATE** | Need human reviewer — agent unsure |
| Confidence LOW overall | **ESCALATE** | Domain knowledge gap — flag for human |

### Output

```
✓ PEER REVIEW ───────────────────────────────────────────────────

  Branch           {branch} → {base}
  Reviewer         {agent | human}
  Date             {date}
  Files reviewed   {N}

  VERDICT: {APPROVED | APPROVED WITH NOTES | CHANGES REQUESTED | ESCALATE}

  BLOCKING ({N})
  ─────────────────────────────────────────────────
  1.  #{finding} — {one-line summary}

  WARNINGS ({N})
  ─────────────────────────────────────────────────
  1.  #{finding} — {one-line summary}

  SUGGESTIONS ({N})
  ─────────────────────────────────────────────────
  1.  #{finding} — {one-line summary}

  Confidence       {HIGH | MEDIUM | LOW}
  Reason           {why this confidence level}
  Rule compliance  {all rules checked | violations listed}

  Next:            /hashb:{recommended-per-verdict} (recommended — {verdict}){· /hashb:simplify → /hashb:ship (if quality smells noted in receipts) — only when flagged}

─────────────────────────────────────────────────────────────────
```

**Task badge** — if the upstream handoff includes a `Task:` line (from
`/decompose`), append `[review ✓]` to the task's badge line in the TODOS file
on APPROVED or APPROVED WITH NOTES verdicts. Skip on CHANGES REQUESTED or
ESCALATE. If no badge line exists, create one (continuation line, indented
6 spaces). Skip if `[review ✓]` is already present.

**Tracker badge label** — if `TRACKER_TYPE=github-issues` (see
`skills/shared/tracker.md` §Badge Label Block), also add the `hashb:review`
label to the GitHub Issue matching the task number. Skip on CHANGES REQUESTED
or ESCALATE (matching badge skip logic). Skip silently on failure.

### Next Step

| Verdict | Next Skill | Why |
|---------|-----------|-----|
| APPROVED | `/ship` | Review passed — proceed to ship |
| APPROVED WITH NOTES | `/ship` | Proceed with warnings noted |
| APPROVED but significant quality smells noted | `/simplify` then `/ship` | Clean up before shipping |
| CHANGES REQUESTED | Fix blockers → re-invoke `/review` | Max 2 cycles before escalating |
| ESCALATE | Stop — present to user | Agent unsure, needs human judgment |

**After fixing findings:** If WARNING or BLOCKING findings are fixed inline
(same session), re-run a scoped review of just the fixes before proceeding —
verify the fixes are correct and didn't introduce new issues. Output an updated
verdict, then hand off to `/ship`. SUGGESTION fixes don't require re-review.

**Default chain behavior** (downstream, `Verbose: true` absent):
- APPROVED / APPROVED WITH NOTES → auto-proceed to `/ship`
- CHANGES REQUESTED → return blockers to implementing agent, re-review after fixes (max 2 cycles)
- ESCALATE → stop the chain, present findings to user

In verbose mode, emit full finding cards before the verdict. In lean mode
(default), emit the verdict line only — finding cards available via
`receipts review`.

---

## Agent-to-Agent Mode

**Subagent invocation.** Orchestrators call this skill via the Agent tool with `subagent_type: hashb:reviewer` (defined in `agents/reviewer.md` at the plugin root; auto-discovered by Claude Code and namespaced under the plugin name). The subagent file is a thin wrapper — caller contract + "no mid-run prompts" enforcement — and delegates everything below to this skill.

When invoked by `/workflow` or `/swarm` (not directly by user):

**Autonomous rules:**
- **APPROVED** → pass verdict to orchestrator, proceed to next phase
- **APPROVED WITH NOTES** → pass verdict + warnings, proceed
- **CHANGES REQUESTED** → return blockers to implementing agent, request fixes,
  then re-review (max 2 cycles before escalating to user)
- **ESCALATE** → stop workflow, present findings to user for decision

**Review cycle limit:** Max 2 review rounds per implementation phase.
If blockers remain after 2 rounds, escalate to user with full context.

**Fresh context:** When invoked as a subagent, the reviewer gets ONLY:
- The diff
- The requirement / eng summary
- Active rules
- No conversation history from the implementing agent

This prevents confirmation bias — the reviewer forms independent conclusions.

---

## Rules

- **Review the diff, not the codebase.** Pre-existing issues are out of scope
  unless the diff makes them worse.
- **Be specific.** File:line references for every finding. No vague "consider
  improving" without pointing at code.
- **Explain why.** Every finding needs a reason — not just "this is wrong"
  but "this is wrong because X could happen."
- **Don't nitpick in blocking.** Style preferences are SUGGESTION, not BLOCKING.
  Only block on real risks.
- **Confidence is honest.** LOW confidence is valuable — it means "I'm not sure,
  get a human to check." That's better than a false HIGH.
- **No rubber stamps.** "LGTM" without checking every category is not a review.
  Work the checklist.
- **Independent judgment.** Don't defer to the implementing agent's reasoning.
  Review the code as if you've never seen it before.