Skip to content

Commit 9e97927

Browse files
wongkclaude
andauthored
refactor(code-review): decouple review agents from codebase-memory-mcp (#195)
* refactor(code-review): decouple review agents from codebase-memory-mcp The graph worker declared an allowlist of six mcp__codebase-memory-mcp__* tools, so only that server could ever reach a reviewer. It now declares no tools: allowlist and inherits the spawning session, with disallowedTools for Bash/Edit/NotebookEdit. The generic worker keeps its four-tool allowlist so the verifier fleet still inherits nothing. The knowledge-graph protocol becomes a capability contract (C1-C4) that reviewers bind to whatever tools they hold; GRAPH_PROJECT collapses to a CODE_INTEL_ALLOWED boolean and the orchestrator makes no graph calls. Also stops the Impact Analyzer emitting a grep_query_used it never ran when the session provides no text-search tool. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011QjUMmrqfGU4QNXDDRLPrN * fix(code-review): reconcile prompt contradictions found in review The impact-analyzer requirements block and Step 2 still stated the grep_query_used requirement categorically, 15 lines before the new no-text-search exception. An analyzer without a search tool read the rejection threat first and the exception second, which pushes it back toward inventing a query. Both statements are now qualified. spawn-reviewers claimed both worker types "end up with the core Read, Write, Grep, Glob". That is false for the inheriting worker and contradicted the permission-inheritance warning five lines above. Widens the documented meaning of discovery: "graph" to cover the no-text-search branch this change introduced. Vocabulary unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011QjUMmrqfGU4QNXDDRLPrN * fix(code-review): make replay fields conditional in the Impact suffix The suffix injected into the Impact Analyzer's task prompt still stated the grep contract unconditionally -- "Always run grep too", "populated external_impact[] and grep_query_used", "If grep returns zero external usages" -- so the no-text-search fallback was contradicted at the point of injection even after the prompt file itself was qualified. Adds coverage for the shape that fallback emits: an all-graph finding with grep_query_used null, and with the field absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011QjUMmrqfGU4QNXDDRLPrN * docs(code-review): state precisely what disallowedTools enforces The changelog said the denylist "keeps the read-only-reviewer boundary" and the agent comment said reviewers "never shell out or mutate source". Both overstate it: disallowedTools matches only whole servers or every MCP tool, so a write-shaped inherited MCP tool is covered by prompt instruction, not by the denylist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011QjUMmrqfGU4QNXDDRLPrN --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a2ba2c1 commit 9e97927

14 files changed

Lines changed: 291 additions & 199 deletions

File tree

CHANGELOG.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,17 @@ All notable changes to the claude-plugins project will be documented in this fil
44

55
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`.
66

7+
### code-review v3.8.0
8+
9+
#### Changed
10+
- The cross-file and design reviewers are no longer bound to a specific code-intelligence MCP server. `code-review-worker-graph` previously declared an allowlist of six `mcp__codebase-memory-mcp__*` tools, which meant only that server could ever reach a reviewer and unresolved entries were silently dropped on any machine without it. The agent now declares no `tools:` allowlist and inherits the tools of the session that spawned it, so whichever indexing server the operator has connected is available. `disallowedTools: Bash, Edit, NotebookEdit` removes those three native tools at the harness level; note that this is narrower than the old allowlist, which made every non-listed tool unreachable by construction — an inherited MCP tool that runs shell commands or edits files is covered by prompt instruction, not by the denylist, because `disallowedTools` matches only whole servers (`mcp__<server>`) or every MCP tool (`mcp__*`), with no pattern for write-shaped tools across servers. `code-review-worker` keeps its explicit four-tool allowlist, so the verifier fleet, Bug Hunter A, the domain critics and the singleton prompts continue to inherit nothing.
11+
- The knowledge-graph protocol in `shared_prompt.txt` is now a substrate-agnostic capability contract (`OPTIONAL — CODE INTELLIGENCE`). Instead of naming tools and their argument shapes, it describes four capabilities — symbol lookup, usage/caller enumeration, snippet read, and structure/dependency analysis — and directs the reviewer to inspect its own tool roster and bind whichever tools answer them, loading deferred MCP schemas with `ToolSearch` first. The same rewrite is applied to `impact_analyzer_prompt.txt`, `design_critic_suffix.txt`, `verifier_prompt.txt`, and the Bug Hunter B / Impact Analyzer / Design Critic / fast-path suffixes in the `spawn-reviewers` skill. The repo-scoping, path-validation, silent-degradation, and untrusted-tool-output rules are retained and generalized to any MCP tool.
12+
- `GRAPH_PROJECT` is replaced by a single orchestrator-computed boolean, `CODE_INTEL_ALLOWED`. The orchestrator no longer calls `list_projects`, resolves a project identifier, or makes any code-intelligence tool call at all; it only decides whether an external index may be trusted for the run, setting `CODE_INTEL_ALLOWED=false` whenever `review_root` is set (an index covers the operator checkout, not the PR head). This removes the prior step that substituted a server-returned project name into the agents' trusted instruction zone.
13+
14+
#### Fixed
15+
- The Impact Analyzer can no longer report a `grep_query_used` it did not execute. Sessions that provide no text-search tool previously still emitted a grep query string, which the verifier replays as its fabrication check. `shared_prompt.txt` now states that any recorded search must describe a query actually run, and `impact_analyzer_prompt.txt` directs the analyzer to leave `grep_query_used` null, leave `external_usages_found` empty, and tag callsites `discovery: "graph"` when it holds no text-search tool — routing those entries to the per-entry file-read and content-match audit, which the verifier already handles as the all-graph case. The requirements block and Step 2 of `impact_analyzer_prompt.txt` previously stated the `grep_query_used` requirement categorically ("findings without a `grep_query_used` will be rejected as malformed"), which pushed an analyzer with no search tool back toward inventing one; both statements are now qualified, and a missing query is malformed only when at least one entry is `discovery: "grep"`.
16+
- The documented meaning of `discovery: "graph"` is widened to match every branch that sets it. It described only the alias / re-export / dynamic-dispatch case a code-intelligence substrate surfaces, but the no-text-search fallback also assigns it to ordinary direct callsites. It now reads as "found on a path the grep replay cannot reproduce", covering both branches, in `code_review_schema.py`, `shared_prompt.txt`, and `verifier_prompt.txt`. The enum vocabulary is unchanged — no schema or fixture change.
17+
718
### code v1.14.11
819

920
#### Changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "code-review",
33
"description": "Code review plugin",
4-
"version": "3.7.1",
4+
"version": "3.8.0",
55
"author": {
66
"name": "ClosedLoop",
77
"email": "support@closedloop.ai"

plugins/code-review/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ plugins/code-review/
2121
SCHEMA.md Canonical Finding + ResultEnvelope schema (PLN-719); §12 documents the golden fixture harness
2222
agents/
2323
code-review-worker.md Background worker agent used by every reviewer fleet spawn (Read, Write, Grep, Glob; permissions-stable across sessions)
24-
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
24+
code-review-worker-graph.md Code-intelligence-aware variant for the cross-file and design reviewers (Impact Analyzer, Bug Hunter B, fast-path, Design Critic); declares no tool allowlist, so it inherits whatever indexing MCP server the operator's session provides — cross-file usage discovery for the cross-file roles, project-structure/dependency analysis for the Design Critic. Degrades to grep when the session has none.
2525
commands/
2626
start.md Main /start command (orchestrator)
2727
shallow.md /shallow wrapper — `/start --depth shallow`
Lines changed: 35 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,50 +1,54 @@
11
---
22
name: code-review-worker-graph
3-
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.
4-
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
3+
description: Code-intelligence-aware review worker for the cross-file and design reviewers (Impact Analyzer, Bug Hunter B, fast-path, Design Critic). Identical to code-review-worker but inherits the parent session's tools, so whatever code-intelligence MCP server the operator has connected is available for cross-file usage discovery and project-structure / dependency-graph analysis. Use only for reviewers whose role prompt loads the code-intelligence protocol.
4+
disallowedTools: Bash, Edit, NotebookEdit # harness-level removal of the three native tools a reviewer must never hold. Does NOT reach write-shaped MCP tools (no cross-server pattern exists); those are covered by the prompt below. MCP inheritance is deliberately untouched — see shared_prompt.txt "OPTIONAL — CODE INTELLIGENCE".
55
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)".
66
---
77

8-
# Code Review Worker (graph-aware)
8+
# Code Review Worker (code-intelligence-aware)
99

1010
You are a code review worker agent for the cross-file and design reviewers. Your
1111
job is the same as the generic `code-review-worker` — read pre-extracted patch
1212
files, analyze changed code, and write structured findings to a JSON file on disk
13-
— but you also have read-only access to the `codebase-memory-mcp` knowledge graph
14-
for precise cross-file usage discovery and project-structure / dependency-graph
15-
analysis.
13+
— but this agent declares no tool allowlist, so you inherit the tools of the
14+
session that spawned you. That session may have a code-intelligence MCP server
15+
connected (one that indexes this repository and answers symbol, caller, and
16+
structure questions). If it does, those tools are yours to use for precise
17+
cross-file usage discovery and project-structure / dependency-graph analysis.
18+
19+
Which server it is — and whether there is one at all — varies by operator. Bind
20+
to what you actually have; never assume a particular server, tool name, or
21+
argument shape.
1622

1723
## Workflow
1824

1925
1. Read the patches file and shared prompt file specified in your task prompt
2026
2. Follow the instructions in the shared prompt exactly (constraints, severity guidelines, output format)
21-
3. Use Read, Grep, and Glob — plus the graph tools below when your task prompt supplies a `GRAPH_PROJECT` — to explore the codebase for context
27+
3. Use Read, Grep, and Glob — plus any code-intelligence tools you hold, per the protocol below — to explore the codebase for context
2228
4. Write your findings JSON to the output file specified in `<output_file>`
2329
5. Respond with a one-line summary: `DONE findings={count} file={path}`
2430

2531
## Tool Usage
2632

27-
- **Read / Write / Grep / Glob**: same as the generic worker.
28-
- **Graph tools** (`search_graph`, `trace_path`, `get_code_snippet`,
29-
`search_code`, `get_architecture`, `query_graph` — each prefixed
30-
`mcp__codebase-memory-mcp__` in the allowlist): read-only context aids.
31-
`get_architecture` and `query_graph` serve project-structure and
32-
dependency-graph analysis (the Design Critic's substrate); the other four serve
33-
cross-file usage discovery. Use them ONLY per the "Optional: codebase knowledge
34-
graph" protocol in `shared_prompt.txt`:
35-
- They are usable ONLY when your task prompt provides a non-empty
36-
`GRAPH_PROJECT` value (the orchestrator resolved it to THIS repo's indexed
37-
project). If `GRAPH_PROJECT` is empty/absent, the graph is unavailable —
38-
fall back to Grep/Glob silently.
39-
- Pass `project=<GRAPH_PROJECT>` on EVERY graph call. Never omit it and never
40-
guess a different project — other indexed repos are out of scope and must
41-
never appear in findings.
42-
- Validate every returned file path: it MUST be openable with Read at its
43-
repo-relative path inside this checkout. Discard (and never cite) any path
44-
that is absolute-outside-cwd or escapes the repo via `..`.
45-
- The graph never replaces evidence: every finding still cites a concrete
46-
file:line you confirmed, and verifier-replay fields (e.g. `grep_query_used`)
47-
stay populated per your role prompt.
48-
49-
Do NOT use Bash. Do NOT call indexing or write graph tools (they are not in your
50-
allowlist). All findings are written with Write exactly as the generic worker does.
33+
- **Read / Write / Grep / Glob**: same as the generic worker. These always work
34+
and are always sufficient — every capability below is an accelerator, never a
35+
prerequisite.
36+
- **Code-intelligence tools**: use them ONLY per the "OPTIONAL — CODE
37+
INTELLIGENCE" protocol in `shared_prompt.txt`, which defines how to discover
38+
what you hold, which capabilities to look for, and the invariants every call
39+
must satisfy. Two mechanics matter before you can call anything:
40+
- **Availability is yours to determine.** Inspect your own tool roster. Your
41+
task prompt carries `CODE_INTEL_ALLOWED`; when it is `false` the orchestrator
42+
has determined an external index cannot be trusted for this run (see the
43+
protocol) and you must use Grep/Glob only, regardless of what you hold.
44+
- **Some MCP tools arrive deferred** — the name is visible but the schema is
45+
not, and calling one cold fails with an input-validation error. Use
46+
`ToolSearch` to load the schemas of the tools you intend to use first.
47+
- **Findings are evidence-bound regardless of substrate.** Every finding cites a
48+
concrete file:line you confirmed by reading it, and verifier-replay fields
49+
(e.g. `grep_query_used`) stay populated per your role prompt.
50+
51+
Do NOT use Bash — everything you need is reachable with Read, Grep, and Glob.
52+
That applies equally to any inherited MCP tool that runs shell commands or edits
53+
files: a reviewer reads and reports, it never executes or mutates. All findings
54+
are written with Write exactly as the generic worker does.

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

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,10 @@ You are a code review worker agent. Your job is to read pre-extracted patch file
2626

2727
Do NOT use Bash. All data you need is available via Read.
2828

29-
> Graph-aware roles (Impact Analyzer, Bug Hunter B, the Design Critic, and the
30-
> fast-path reviewer) run as the separate `code-review-worker-graph` agent, which
31-
> adds read-only `codebase-memory-mcp` tools. This generic worker — used by every
32-
> other reviewer plus the verifier fleet and the PLN-725 singletons — deliberately
33-
> has NO graph access, keeping the trust boundary tight for adversarial/verification
34-
> roles.
29+
> Code-intelligence-aware roles (Impact Analyzer, Bug Hunter B, the Design Critic,
30+
> and the fast-path reviewer) run as the separate `code-review-worker-graph` agent,
31+
> which declares no `tools:` allowlist and so inherits whatever MCP tools the
32+
> operator's session provides. This generic worker — used by every other reviewer
33+
> plus the verifier fleet and the PLN-725 singletons — keeps its explicit
34+
> four-tool allowlist and deliberately inherits NOTHING, keeping the trust boundary
35+
> tight for adversarial/verification roles.

plugins/code-review/commands/deep.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ This command is shorthand for `/start --depth deep`. Follow every instruction in
1313

1414
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).
1515

16-
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"`.
16+
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 code-intelligence-aware: when the operator's session provides an MCP server that indexes the repo, it uses that server for module/layer layout and for dependency direction and import cycles, falling back to grep otherwise. The plugin is substrate-agnostic — it names no particular server and works with whichever one is connected. Findings carry `category: "Code Quality"`.
1717

1818
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.
1919

0 commit comments

Comments
 (0)