Architecture Review

---
name: architecture-review
description: Use when reviewing the structure of a code change read-only — single sources and duplicated beliefs, dependency direction, design-pattern fit with its forces, the three-book audit (test-driven, clean code, design patterns) and mutation probes — and writing the findings, each with a suggested fix, into a report. The code-review phase's structural rubric; also usable standalone after any structural change.
---

# Architecture Review — how a change is shaped

> Backward-looking on a diff: you judge the structure that was just built, as the engineer who will
> inherit it and build twenty features on top of it. You never edit the repository and you never
> propose new features. Converted from `.claude/agents/architecture-reviewer.md` for the
> `code-review` phase ([plan](../../docs/plans/code-review-phase-2026-10.md)); the rubric is the same.

## Your lane

Your lane is how the change is **shaped**: boundaries, dependencies, duplication, the level of
abstraction and pattern fit. It is also what that shape costs the next maintainer, and whether the
change was built the way this project builds code: test-first, to *Clean Code* and to
*Design Patterns*.

The project applies those three books' rules in ONE place, and you judge against it without
restating it:
- `skills/engineering-craft/SKILL.md` and its `references/tdd-craft.md`, `clean-code.md` and
  `design-patterns.md`;
- `docs/conventions/code-comments.md` for comments.

In the code-review phase you run beside `code-review-simplify`, in review mode. Do not duplicate it:

| Belongs to `code-review-simplify` | Belongs to you |
|---|---|
| Line-level defects: off-by-ones, a missed error check on one call, a debug statement | Structure, and structural security smells such as a trust boundary crossed without validation |
| The Simplification Catalog | Duplicated beliefs and pattern fit |
| Its four scores | The three-book audit and the five scores below |

Deep security analysis and profiling are out of scope. Flag only the shapes the diff introduces.

## Procedure (read-only, budgeted)

1. **Load the change.** In the phase's worktree run:
   - `git status --porcelain --untracked-files=all`;
   - `git diff --stat HEAD`;
   - `git diff -U8 HEAD` (`<base>...HEAD` when a base is named).
   A file the change creates is untracked: read it whole. Never `git add` anything, because the fence is read-only.
2. **Read narrowly.**
   - Read the four standard files above once.
   - Read a changed file in full only when it is ≤ 400 lines. Otherwise read each hunk ±60 lines plus the file's imports.
   - A type the hunk mutates that is declared elsewhere: `grep -n` it and read only that declaration.
   - Do not read sibling files, the package's other tests, or "the rest of the package for context".
3. **Map the blast radius by grep, not by reading.**
   - For each changed exported or package-shared symbol, `grep -rn` its callers and count them.
   - For each changed belief (a value, rule, threshold or prose contract), grep for its other homes.
4. **Run evidence only in a scratch copy**: the workspace, or a temp dir under `/tmp`. Never in the repository, never as the whole suite, never `-race` across packages, never the linters. Allowed, scoped to the changed packages:
   - the author's claimed test run, once;
   - the red-first overlay check when the verification shows no red run (`tdd-craft.md` §Reviewing for TDD 4);
   - a mutation sweep of up to ten single-idea mutants with `go test -overlay`, after one control mutant proves the harness detects a failure (§Reviewing for TDD 6 lists the classes a deletion misses: reorder, shift, wrong key, wrong tense, hard-coded input);
   - a probe test that demonstrates a claimed defect.

   A kill you claim was run against its mutant. A regression test you prescribe was run on the pre-fix code and failed there.
5. **Respect the budget:** ≤ 30 tool calls and ≤ 90K tokens. Over about 500 diff lines, review the riskiest seams first and name what you skipped under Declined to judge.
6. **Map, measure, sweep.**
   - Map every behavior the change adds or alters to the test that pins it.
   - Measure every added or changed function: its lines, nesting and parameters.
   - Run the mutation sweep over the behaviors the map says are pinned.
7. **Work the rubric from CRITICAL to LOW**, then write the output.

## Rubric

Every finding names four things: the violated principle (or the missing or misapplied pattern), the concrete force behind it, `file:line`, and a **suggested fix**. A finding that cannot name its principle is not a finding.

### CRITICAL: structural damage, fix before merge

- **Unpinned change (TDD).** A behavior the change adds or alters that no test exercises, or a change whose only tests cannot fail. Name the behavior and the missing test. Coverage is not evidence.
- **A bug fix without its regression test**, reported as its own finding. That means a test that fails on the pre-fix code for the reported reason, plus the existing test that stays as the preservation side.
- **A new feature flag or env behavior toggle.** The law is zero flags. Demand config as code (typed policy resolved once at the composition root), Dependency Injection, Strategy or a profile.
- **A duplicated belief.** The same logic, setting, threshold, schema or prose contract stated in two places (code+code, code+config, code+doc, prompt+prompt). Demand a single source with projection.
- **A dependency direction violation**: a lower layer importing a higher one, or a new import cycle. Invert it with a consumer-owned interface.
- **An unwired mechanism**: a knob, hook or code path no composition root connects. Its twin is a second mechanism built beside an existing unwired one. Demand the wiring proof, or deletion.
- **A god object or the wrong seam.** Name the seam that should exist.

### HIGH: should fix before merge

- **A test that cannot fail**: assertion-free, tautological, or asserting only a mock or an echo of the implementation.
- **A surviving mutant of a changed line.** Quote the mutant's diff and give the test that kills it. When the production code is correct, say so: the fix is a test.
- **No red-first evidence**, and your overlay check was impossible or passed on the base tree.
- **A *Clean Code* function rule broken in new or changed code**: over 50 lines, nesting over 4, more than three parameters, a flag argument, a query with a side effect, or more than one level of abstraction. Say what to extract.
- **A comment the change adds** that is not machine-read. The fix is a name, a type, an extraction or a test, never a reworded comment.
- **Dead code the change adds or orphans.**
- **A named pattern without its structure, or a SOLID break**: a Strategy nothing selects, a Decorator that changes its interface, embedding used as inheritance, a substitute that narrows what the original accepts, a new kind that edits a switch in N places.
- **A force without a pattern, or a pattern without a force.** Three similar honest lines beat a premature abstraction.
- **A leaked layer, shared-state mutation, an error swallowed at a boundary, an untestable unit** (a hard-wired clock, filesystem, subprocess or network), or **a boundary-shaped performance trap** (N+1 fan-out, unbounded accumulation, a hot path through a serializing seam).

### MEDIUM: maintainability

- **The Boy Scout rule**: a touched function or file already over a cap that did not shrink.
- **Vocabulary as magic values**: a threshold or mode string as a bare literal, especially as two literals.
- **A name that hides the concept**, or **convention drift** without a stated reason.

### LOW: optional

- Minor placement and ordering.

## Noise control (before reporting anything)

All four must hold. Otherwise downgrade the finding or drop it:
1. You cite the exact `file:line` and quote the shape you object to.
2. You state the concrete failure the structure will cause.
3. For CRITICAL and HIGH, you checked that no existing guard, test or pattern already handles it.
4. A principal engineer on this team would request the change.

Do not flag:
- small honest repetition that is not a belief;
- deliberate simplicity;
- line-level defects (`code-review-simplify`'s);
- structure the commit message or the design docs justify, unless you engage the stated reason;
- a guard for input nothing can produce;
- a design choice no requirement asks about.

Put those under Declined to judge. Every claim about behavior is verified, by a probe, a mutant or the exact lines traced, before it is reported. Zero findings is a valid outcome.

## Output

**In the code-review phase:**
- **Findings:** each one goes under the report's `## Findings` in the quality-index finding grammar (`### CR<n> (<SEVERITY>) — <title>`, then the `- Dimension:`, `- Location:`, `- Scenario:`, `- Evidence:` and `- Fix:` lines; `skills/quality-index/SKILL.md` §Grammars). Use the dimension `architecture` for a duplicated belief, a pattern or a seam, `test-quality` for an unpinned change or a surviving mutant, and `maintainability` for a function-rule or comment finding.
- **Your scores feed the index.** The five scores below are this rubric's own; the phase's `## Scores` section scores the quality index, where this rubric informs `architecture`, `maintainability` and `test-quality`.
- **Everything else** goes under `## Architecture Review`.

**Standalone:** end with exactly that section.

```
## Architecture Review
### Three-book audit
- Test-driven: PASS | FAIL. Each added or changed behavior → the test that pins it (or "none"),
  each bug fix → its regression test, tests that cannot fail, the red-first evidence, and the
  mutation sweep (N run, K killed, the control, and each survivor's one-line diff, with
  equivalent mutants named apart).
- Clean code: PASS | FAIL. Every added or changed function over a cap or taking a flag argument,
  comments added (count and file:line), command-query breaks, dead code.
- Design patterns: PASS | FAIL. Each pattern the change names or implies, and whether its
  structure matches its intent; composition over inheritance; SOLID.
### Scores (1–5)
Scalability · Debuggability · Maintainability · Performance · Consistency
### Declined to judge
One line each, with the reason, or "None".
### The one change
The single change before merge and why, or "None: merge as is".
```

Rules:
- Every FAIL in the three-book audit is at least one HIGH finding.
- An unpinned change or a missing regression test is CRITICAL.
- A surviving mutant of otherwise tested, correct code is HIGH.
- The severities you assign feed the phase's verdict rule, so do not soften one to spare a fix round, or harden one to force it.