feat(code-review): add graph-aware Design Critic reviewer (#170)

* feat(code-review): add graph-aware Design Critic reviewer

Add the Design Critic — an always-on, deep-tier conditional core reviewer
that evaluates software-design craftsmanship (module depth, information
hiding, SOLID, dependency direction, project structure), drawing on
A Philosophy of Software Design, SOLID, and Clean Architecture. It is
source:"core" (exempt from DOMAIN_CRITIC_CAP), runs on Sonnet, and emits
category:"Code Quality" findings scoped to flaws this change introduces.

- Cap domain critics uniformly at 3 (was 5) on both standard and deep;
  deep's extra breadth now comes from the Design Critic + Impact Analyzer.
- Make the Design Critic graph-aware: spawn it on code-review-worker-graph
  and add get_architecture + query_graph to that worker so it can read the
  real module layout and dependency edges (falls back to grep when the
  graph is unavailable or review_root is set).
- Remove the unused callsite_snippet_hash / snippet_hash_matched fields
  (LLMs cannot compute sha256; the field had no working consumer) — the
  verifier now content-matches the verbatim callsite snippet.
- Render both conditional core reviewers (Impact Analyzer + Design Critic)
  on the fleet "Reviewers:" line by deriving the set from _SPAWN_CORE_ROLES
  instead of a hand-maintained tuple.
- Comment/docstring accuracy fixes surfaced by a self-review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs(code-review): update changelog and README for v3.1.0

Add the v3.1.0 CHANGELOG entry (Design Critic reviewer, uniform
domain-critic cap of 3, graph-aware Design Critic, callsite_snippet_hash
removal) and refresh the README: depth-tier table (cap 5→3, Design Critic
row, Impact Analyzer now shipping), /deep description, the graph worker's
consumers/tools, and the design_critic_suffix.txt prompt listing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: wongk

CHANGELOG.md

@@ -4,6 +4,19 @@ All notable changes to the claude-plugins project will be documented in this fil
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Entries are listed newest-first; each plugin section is treated as released when merged to `main`.
 
+### code-review v3.1.0
+
+#### Added
+- New **Design Critic** reviewer — an always-on, deep-tier conditional core reviewer that evaluates software-design craftsmanship (module depth and information hiding, SOLID, dependency direction and layer boundaries, project structure), drawing on *A Philosophy of Software Design*, SOLID, and *Clean Architecture*. It is a `source: "core"` reviewer (exempt from the domain-critic cap), runs on Sonnet, and emits `category: "Code Quality"` findings scoped to design flaws a change introduces or worsens.
+- Both conditional core reviewers (the Design Critic and the Impact Analyzer) now appear on the operator-facing "Reviewers:" fleet-summary line. The non-partitioned core set is derived from `_SPAWN_CORE_ROLES` so future core reviewers are listed automatically.
+
+#### Changed
+- The domain-critic cap is now a uniform 3 across both standard and deep reviews (previously 5). Deep-tier breadth comes from the always-on Design Critic and the signal-gated Impact Analyzer rather than a wider domain-critic allowance.
+- The Design Critic is graph-aware: it runs on the graph-enabled review worker and uses the `codebase-memory-mcp` knowledge graph (`get_architecture` for module/layer layout, `query_graph` for dependency direction and import cycles) when the repository is indexed, falling back to grep otherwise. The graph worker's tool set gained `get_architecture` and `query_graph`.
+
+#### Removed
+- Removed the unused `callsite_snippet_hash` field from `external_impact[]` entries and the coupled `snippet_hash_matched` evidence-check field. Impact-analysis callsites are now validated by reading the cited file and content-matching the verbatim `callsite_snippet`.
+
 ### code-review v3.0.0
 
 #### Removed

plugins/code-review/.claude-plugin/plugin.json

@@ -1,7 +1,7 @@
 {
   "name": "code-review",
   "description": "Code review plugin",
-  "version": "3.0.0",
+  "version": "3.1.0",
   "author": {
     "name": "ClosedLoop",
     "email": "support@closedloop.ai"

plugins/code-review/README.md

@@ -21,14 +21,15 @@ plugins/code-review/
   SCHEMA.md                          Canonical Finding + ResultEnvelope schema (PLN-719); §12 documents the golden fixture harness
   agents/
     code-review-worker.md            Background worker agent used by every reviewer fleet spawn (Read, Write, Grep, Glob; permissions-stable across sessions)
-    code-review-worker-graph.md      Graph-aware variant for the cross-file reviewers (Impact Analyzer, Bug Hunter B, fast-path); adds read-only codebase-memory-mcp tools for cross-file usage discovery
+    code-review-worker-graph.md      Graph-aware variant for the cross-file and design reviewers (Impact Analyzer, Bug Hunter B, fast-path, Design Critic); adds read-only codebase-memory-mcp tools — cross-file usage discovery for the cross-file roles, project-structure/dependency-graph analysis (get_architecture, query_graph) for the Design Critic
   commands/
     start.md                         Main /start command (orchestrator)
   prompts/
     github-review.md                 GitHub-mode constraints and output steps (loaded conditionally)
   tools/
     prompts/shared_prompt.txt        Shared reviewer constraints injected into every agent prompt
     prompts/bha_suffix.txt           Bug Hunter A reviewer persona and focus areas
+    prompts/design_critic_suffix.txt Design Critic reviewer role (software-design craftsmanship; always-on at deep tier)
     python/code_review_schema.py     Canonical Finding + ResultEnvelope schema + validators (PLN-719)
     python/test_code_review_schema.py  Schema tests + round-trips
     python/code_review_helpers.py    Deterministic helper CLI (parse-diff, hygiene, partition, route, validate, cache, finalize-result, arbitrate-budget, prepare-run, etc.)
@@ -125,7 +126,7 @@ Runs a comprehensive code review. Invokes the full pipeline: diff parsing, hygie
 
 ### `/shallow` and `/deep`
 
-Thin command-file wrappers around `/start` with `--depth` pre-bound. `/shallow` invokes the built-in fleet only (BHA + BHB + unified_auditor + verifier; no `critic-gates.json` entries, no signal extraction). `/deep` invokes the standard fleet plus any reviewer tagged `min_depth: deep` in `stages.json` (reserved for the FEA-1401 Impact Analyzer slot — today equivalent to standard).
+Thin command-file wrappers around `/start` with `--depth` pre-bound. `/shallow` invokes the built-in fleet only (BHA + BHB + unified_auditor + verifier; no `critic-gates.json` entries, no signal extraction). `/deep` invokes the standard fleet plus the deep-tier reviewers: the **Design Critic** (always-on at deep — no signal trigger), and the FEA-1401 **Impact Analyzer** when signal extraction detects an exported-symbol change or symbol deletion.
 
 ### `/cost`
 
@@ -161,12 +162,13 @@ Three tiers select which reviewer fleet runs:
 | `bug_hunter_a` (partitioned at >5000 LOC) | ✓ | ✓ | ✓ |
 | `bug_hunter_b` | ✓ | ✓ | ✓ |
 | `unified_auditor` | ✓ | ✓ | ✓ |
-| `critic-gates.json` domain critics | ✗ | ✓ (≤3 total) | ✓ (≤5 total) |
+| `critic-gates.json` domain critics | ✗ | ✓ (≤3 total) | ✓ (≤3 total) |
 | Verifier | ✓ | ✓ | ✓ |
 | `fast_path_reviewer` (auto on tiny PRs) | ✓ (auto) | ✓ (auto) | ✓ (auto) |
-| `impact_analyzer` (future FEA-1401) | ✗ | ✗ | ✓ |
+| `design_critic` (always-on at deep) | ✗ | ✗ | ✓ |
+| `impact_analyzer` (FEA-1401, on exported-symbol change/deletion) | ✗ | ✗ | ✓ (on signal) |
 
-**Standard-mode budget arithmetic.** With PLN-807 Phase 4, `arbitrate-budget` reserves BHA partitions FIRST (from `_max_bha_partitions_by_loc`) and then allocates the remaining budget to critics and best-effort. The total domain-critic count across both required and best-effort buckets is capped depth-aware: standard runs keep the top `STANDARD_DOMAIN_CRITIC_CAP = 3` (by priority asc, reviewer asc), while deep runs keep the full `DOMAIN_CRITIC_CAP = 5`. Required-bucket critics dropped by the cap emit coverage-gap findings; cap-deferred entries carry `defer_reason: "domain_critic_cap"` in `deferred_for_budget`. PRs with sparse critic-gates rosters see identical fleet to pre-PLN-807.
+**Standard-mode budget arithmetic.** With PLN-807 Phase 4, `arbitrate-budget` reserves BHA partitions FIRST (from `_max_bha_partitions_by_loc`) and then allocates the remaining budget to critics and best-effort. The total domain-critic count across both required and best-effort buckets is capped uniformly at `DOMAIN_CRITIC_CAP = 3` (by priority asc, reviewer asc) for both standard and deep tiers. Required-bucket critics dropped by the cap emit coverage-gap findings; cap-deferred entries carry `defer_reason: "domain_critic_cap"` in `deferred_for_budget`. PRs with sparse critic-gates rosters see identical fleet to pre-PLN-807.
 
 **Tier-mismatch nudge.** Shallow runs emit a single LOW system-scoped finding (`system_marker: "tier_mismatch_nudge"`) when the diff would benefit from a higher tier. Heuristics: diff > 3000 LOC; schema/migration paths (`/migrations/`, `/schemas/`, `/models/`); public API surface (`plugin.json`, `index.ts`, `__init__.py`, etc.).
 

plugins/code-review/SCHEMA.md

@@ -83,8 +83,7 @@ shape. Producers may emit dicts directly; the Python convenience type lives in
       "line": <int>,
       "impact_type": "signature_mismatch | type_incompatibility | semantic_drift | deleted_reference | stale_string_reference | behavioral_change | guard_needed",
       "description": "<one sentence>",
-      "callsite_snippet": "<verbatim>",
-      "callsite_snippet_hash": "<sha256>",
+      "callsite_snippet": "<verbatim source line at file:line>",
       "discovery": "grep | graph",
       "confidence": 0.0..1.0
     }
@@ -102,8 +101,7 @@ shape. Producers may emit dicts directly; the Python convenience type lives in
     {
       "claim": "<verbatim from finding.evidence[]>",
       "verified": <bool>,
-      "actual_read": "<what verifier read at the cited location>",
-      "snippet_hash_matched": <bool>
+      "actual_read": "<what verifier read at the cited location>"
     }
   ],
   "rejection_class": "evidence_not_found | guard_exists | unreachable | out_of_scope | severity_overstated | null",

plugins/code-review/agents/code-review-worker-graph.md

@@ -1,17 +1,18 @@
 ---
 name: code-review-worker-graph
-description: Graph-aware code review worker for the cross-file reviewers (Impact Analyzer, Bug Hunter B, fast-path). Identical to code-review-worker but adds read-only codebase-memory-mcp tools for precise cross-file usage discovery. Use only for reviewers whose role prompt loads the codebase knowledge graph protocol.
-tools: Read, Write, Grep, Glob, mcp__codebase-memory-mcp__search_graph, mcp__codebase-memory-mcp__trace_path, mcp__codebase-memory-mcp__get_code_snippet, mcp__codebase-memory-mcp__search_code
+description: Graph-aware code review worker for the cross-file and design reviewers (Impact Analyzer, Bug Hunter B, fast-path, Design Critic). Identical to code-review-worker but adds read-only codebase-memory-mcp tools for precise cross-file usage discovery and project-structure / dependency-graph analysis. Use only for reviewers whose role prompt loads the codebase knowledge graph protocol.
+tools: Read, Write, Grep, Glob, mcp__codebase-memory-mcp__search_graph, mcp__codebase-memory-mcp__trace_path, mcp__codebase-memory-mcp__get_code_snippet, mcp__codebase-memory-mcp__search_code, mcp__codebase-memory-mcp__get_architecture, mcp__codebase-memory-mcp__query_graph
 effort: high  # pinned so a lowered session effort can't cut reviewer reasoning depth (no per-Task override; frontmatter is the only lever). Not redundant with the default — do not remove. Rationale: start.md "Orchestrator model (cost)".
 ---
 
 # Code Review Worker (graph-aware)
 
-You are a code review worker agent for the cross-file reviewers. Your job is the
-same as the generic `code-review-worker` — read pre-extracted patch files, analyze
-changed code, and write structured findings to a JSON file on disk — but you also
-have read-only access to the `codebase-memory-mcp` knowledge graph for precise
-cross-file usage discovery.
+You are a code review worker agent for the cross-file and design reviewers. Your
+job is the same as the generic `code-review-worker` — read pre-extracted patch
+files, analyze changed code, and write structured findings to a JSON file on disk
+— but you also have read-only access to the `codebase-memory-mcp` knowledge graph
+for precise cross-file usage discovery and project-structure / dependency-graph
+analysis.
 
 ## Workflow
 
@@ -25,8 +26,11 @@ cross-file usage discovery.
 
 - **Read / Write / Grep / Glob**: same as the generic worker.
 - **Graph tools** (`search_graph`, `trace_path`, `get_code_snippet`,
-  `search_code` — each prefixed `mcp__codebase-memory-mcp__` in the allowlist):
-  read-only context aids. Use them ONLY per the "Optional: codebase knowledge
+  `search_code`, `get_architecture`, `query_graph` — each prefixed
+  `mcp__codebase-memory-mcp__` in the allowlist): read-only context aids.
+  `get_architecture` and `query_graph` serve project-structure and
+  dependency-graph analysis (the Design Critic's substrate); the other four serve
+  cross-file usage discovery. Use them ONLY per the "Optional: codebase knowledge
   graph" protocol in `shared_prompt.txt`:
   - They are usable ONLY when your task prompt provides a non-empty
     `GRAPH_PROJECT` value (the orchestrator resolved it to THIS repo's indexed

plugins/code-review/agents/code-review-worker.md

@@ -26,8 +26,9 @@ You are a code review worker agent. Your job is to read pre-extracted patch file
 
 Do NOT use Bash. All data you need is available via Read.
 
-> Graph-aware roles (Impact Analyzer, Bug Hunter B, and the fast-path reviewer)
-> run as the separate `code-review-worker-graph` agent, which adds read-only `codebase-memory-mcp`
-> tools. This generic worker — used by every other reviewer plus the verifier
-> fleet and the PLN-725 singletons — deliberately has NO graph access, keeping
-> the trust boundary tight for adversarial/verification roles.
+> Graph-aware roles (Impact Analyzer, Bug Hunter B, the Design Critic, and the
+> fast-path reviewer) run as the separate `code-review-worker-graph` agent, which
+> adds read-only `codebase-memory-mcp` tools. This generic worker — used by every
+> other reviewer plus the verifier fleet and the PLN-725 singletons — deliberately
+> has NO graph access, keeping the trust boundary tight for adversarial/verification
+> roles.

plugins/code-review/commands/deep.md

@@ -1,5 +1,5 @@
 ---
-description: Deep code review — standard fleet plus Impact Analyzer (FEA-1401) when changed exported symbols are detected
+description: Deep code review — standard fleet plus the always-on Design Critic and the Impact Analyzer (FEA-1401) when changed exported symbols are detected
 argument-hint: "[scope] [--github] [--base <ref>] [--since-last-review] [--full-review]"
 ---
 
@@ -11,14 +11,22 @@ This command is shorthand for `/start --depth deep`. Follow every instruction in
 
 ## What deep does
 
-Deep produces the standard fleet plus the **Impact Analyzer** (FEA-1401) when signal extraction detects `exported_symbol_change` or `symbol_deletion` in the diff. The analyzer identifies changed exported symbols (function signatures, type definitions, exported constants, class API, schema fields, deletions), greps the codebase for external usages outside the diff, and emits findings whose `external_impact[]` array lists every callsite that breaks under the new signature.
+Deep produces the standard fleet plus two deep-only conditional core reviewers: the always-on **Design Critic** and the signal-gated **Impact Analyzer** (FEA-1401).
+
+The **Design Critic** runs on **every** deep review (no trigger required). It evaluates the change for software-design craftsmanship — module depth and information hiding, SOLID adherence, dependency direction and layer boundaries, and project/package structure — drawing on *A Philosophy of Software Design*, the SOLID principles, and *Clean Architecture*. It flags only design flaws this change introduces or demonstrably worsens (a new shallow module, a wrong-direction dependency, a god-class this PR grew, a type-switch it extended), runs on Sonnet, and is **exempt from the domain-critic cap** (it is a `source: "core"` reviewer, not a project-specific critic). Like the Impact Analyzer it is graph-aware: when the repo is indexed it queries the `codebase-memory-mcp` knowledge graph (`get_architecture` for module/layer layout, `query_graph` for dependency direction and import cycles), falling back to grep otherwise. Findings carry `category: "Code Quality"`.
+
+The **Impact Analyzer** (FEA-1401) spawns when signal extraction detects `exported_symbol_change` or `symbol_deletion` in the diff. It identifies changed exported symbols (function signatures, type definitions, exported constants, class API, schema fields, deletions), greps the codebase for external usages outside the diff, and emits findings whose `external_impact[]` array lists every callsite that breaks under the new signature.
 
 | Component | shallow | standard | deep |
 |---|---|---|---|
 | All standard reviewers | (subset) | ✓ | ✓ |
+| Domain critics (from `critic-gates.json`) | ✗ | ✓ (≤3) | ✓ (≤3) |
+| Design Critic | ✗ | ✗ | ✓ (always) |
 | Impact Analyzer (FEA-1401) | ✗ | ✗ | ✓ (when triggered) |
 
-The Impact Analyzer is **conditional**: it only spawns when at least one trigger signal fires above the recommended confidence floor (`exported_symbol_change ≥ 0.8`, `symbol_deletion ≥ 0.85`). A deep run on a docs-only diff or an internal refactor that exposes no new external surface will skip the analyzer entirely. Findings carry `category: "ImpactAnalysis"` and are verifier-audited per callsite (snippet-hash check, grep query replayed for the first 5 findings per batch). ≥2 verified BLOCKING/HIGH Impact findings escalate the verdict to `NEEDS_ATTENTION` (Rule 6).
+The per-source **domain-critic cap is 3** on both standard and deep — deep's extra breadth now comes from the always-on Design Critic and the Impact Analyzer rather than from a wider domain-critic allowance.
+
+The Impact Analyzer is **conditional**: it only spawns when at least one trigger signal fires above the recommended confidence floor (`exported_symbol_change ≥ 0.8`, `symbol_deletion ≥ 0.85`). A deep run on a docs-only diff or an internal refactor that exposes no new external surface will skip the analyzer entirely. Findings carry `category: "ImpactAnalysis"` and are verifier-audited per callsite (cited callsite read and content-matched, grep query replayed for the first 5 findings per batch). ≥2 verified BLOCKING/HIGH Impact findings escalate the verdict to `NEEDS_ATTENTION` (Rule 6).
 
 Cost containment: 30 symbols × 50 callsites per symbol hard cap, 5-minute wall budget, 100 grep ops (soft), 250 read ops (soft). Deferred symbols beyond cap surface in the Coverage Plan footer so operators see what was sampled vs analyzed.