feat(code-review): code-intelligence capabilities C5–C7 and broker discovery

[wongk]

Campaign prd-632-20260921 · chunk C1 — Code-intelligence capabilities C5–C7 and broker discovery (prompt-only)

Artifacts: PRD-632 · PLN-2082 · related ISS-10842

What this lands

PLN-2082 steps S1, S2, S3, S4, S5, S6 — the whole plan. Prompt and agent wording only; no Python, schema, hook, or orchestrator change. code-review 3.10.0 → 3.10.1.

The reviewer code-intelligence protocol (shared_prompt.txt) asked for four capabilities (C1 symbol lookup, C2 caller enumeration, C3 snippet read, C4 structure). This adds three, plus one discovery rule, all server-agnostic:

• C5 change impact — a diff → changed symbols → call sites tool seeds the Impact Analyzer's Steps 1–2. Rows are candidates: scoped to assigned files, row-limited, generic/dunder names unverified until confirmed. The diff base is read from base_ref in the run's scope.json; absent, empty, or leading - skips C5.
• C6 index coverage — an empty index answer is no longer a negative one. A finding or dismissal resting on absence needs the paths confirmed indexed and current and the response not cut short, else Grep or don't assert. "A clean coverage result means 'no recorded gap', never proof of completeness."
• C7 duplication — near-duplicate hits are candidates; a tool error or unbuilt data means unavailable, never "no duplicates".
• No completeness claims — no substrate field licenses "all callers" / "only N call sites".
• Broker discovery — one search of a search-and-call broker per unanswered capability, query-only tools, every existing invariant applies.

Commits — read in this order

1. fa84341 feat: C5/C6/C7 + version bump
2. 3e2b26d feat: broker discovery — deliberately isolated; reverts cleanly on its own (verified after every later commit). Its CHANGELOG bullet would need removing by hand.
3. 16757e0 fix: review findings (below)
4. 0f0aea5 docs: CHANGELOG for 3.10.1

⚠️ Broker discovery is only as safe as the broker. The graph worker still inherits any connected server's shell/edit/broker tools (unshipped guard: ISS-10842). If that guard lands as originally designed it would deny brokers outright; ISS-10842 records that its rule must become "deny write-shaped brokered calls".

Acceptance ledger

27 criteria · 21 met · 6 met differently · 0 not met

Met differently:

• S1 C6 wording — reworded to "whether the index is current, whether given paths are in it…" after checking the sentence against a second server whose freshness tool is whole-index only.
• S2 diff base — plan said "as given in the task prompt"; no reviewer prompt carries one, so reviewers Read it from scope.json (campaign decision).
• S6 /update-documentation — run by the campaign after review rather than by the build.
• §5 real runs, §5 transcript checks, §5 cost — operator-owned, see "Owed before merge".

Other deviations: generic-name examples trimmed to __init__, get, open for the +30-line budget (section grew by exactly 30); PLN-2082 is still DRAFT (status change was permission-denied); the PLN-2082 build thread cites pre-rebase SHAs c762cec/515279a for what are now fa84341/3e2b26d.

Review

/code-review:deep --base origin/main at 3e2b26d → NEEDS_ATTENTION: verified 3 · justified 0 · rejected 0 · pending 0 · coverage gaps 0. Fast path (one reviewer, three verifiers).

• Fixed (16757e0) — HIGH: stale capability summary in spawn-reviewers/SKILL.md ("BHB / Impact / fast-path use C1–C3…") replaced with a pointer to the role suffixes and shared protocol.
• Fixed (16757e0) — MEDIUM: Bug Hunter B and fast-path blocks got C5 without the base_ref sourcing and leading-- guard the Impact Analyzer has.
• Pushed back — MEDIUM (verifier TENTATIVE 0.45): "PATCH bump for new capabilities; recommends 3.11.0." Verbatim: The repo rule ties MINOR to new agents/skills/commands and PATCH to bug fixes/prompt wording. This branch adds no agent, skill, or command: every changed file is a modification of an existing prompt, skill, or agent body, i.e. prompt wording. The finding's precedent is factually wrong, as its own verifier established: CODE_INTEL_REQUIRE_ROOT_ARG shipped in code-review v3.9.0, not v3.10.0, and v3.10.0's MINOR bump covered an unrelated feature set (review_result.json fields, reviewed_line footer, render-reviewed-commit). The operator-approved PRD specifies PATCH and the chunk manifest constrains the bump to 3.10.0 -> 3.10.1 exactly once. A feat commit type describes the change to reviewers' behaviour; it does not by itself select the semver level under this repo's rule. Version left at 3.10.1.
• Declined: none.

Re-review (gate fired: resolve touched the HIGH finding's file) — --since-last-review at 16757e0 → APPROVED, 0 findings. Both runs proven against scope.json: review_root = the chunk worktree, review_root_sha = HEAD, base_ref = origin/main, worktree_path empty.

Verification

• uv run pytest plugins/code-review/tools/python/ -q — 1397 passed, 3 skipped, no golden failures (run after build and again after the fix commit)
• uv run ruff check . · uv run pyright — clean
• Vendor-vocabulary grep over the diff — empty (and shown to fire on a scratch string)
• Diff = the six plugin files + CHANGELOG.md
• Capability sentences checked against a second code-intelligence server's tool descriptions

Side effects

• Editing shared_prompt.txt changes the prompt hash: the Bug Hunter A cache and the verifications/ cache are invalidated once for every operator.
• feat(code-review): review non-application files + three TestQuality shapes #183 edits lines adjacent to two blocks touched here in spawn-reviewers/SKILL.md and also touches shared_prompt.txt; whichever merges second rebases.

Owed before merge (operator)

Golden fixtures replay recorded output and cannot exercise reviewer judgement, so PLN-2082 §5 asks for real /code-review:deep runs, each once with a code-intelligence server connected and once without:

Branch under review	Pass condition
Changes an exported symbol with an aliased-import caller	Aliased caller reported discovery: "graph" with matching snippet; verifier confirms
Changes a Python constant referenced only from a prompt .txt	No "unused / no callers / safe to delete" claim on index evidence alone
Adds a function near-identical to an existing helper	Reported citing both locations; with duplicate data unbuilt, no "no duplication" claim and no build attempted
Changes a method named get or __init__	No external_impact[] entry cites an unrelated namesake
Any of the above, no server connected	Findings equivalent to a 3.10.0 run

Plus transcript checks (C5 always path-scoped; ≤1 broker search per capability; no "all callers" wording) and /code-review:cost before/after (soft budget ~15% on Impact Analyzer / Design Critic).

Nothing further is owed by later chunks — this is the campaign's only chunk.

[wongk]

Real-run verification (PLN-2082 §5)

Six headless /code-review:deep --base main runs against one synthetic fixture: a small Python package with a 245-line branch (over the 200-line fast-path threshold, so the full fleet ran — Bug Hunter A/B, Unified Auditor, Impact Analyzer, Design Critic). The branch plants three real breakages, each reproduced by executing the code, plus one rename-only duplicate:

1. compute_total gains a required param; the only un-migrated caller reaches it through a re-export alias (order_total) in a file outside the diff.
2. A constant is renamed; its only other reference is a {PLACEHOLDER} in a .txt prompt template (index state: excluded).
3. make_slug is a rename-only copy of an existing slugify_name (near-duplicate data built; similarity 1.000).
4. CartStore.get gains a required kwarg; its callers sit among unrelated dict.get / os.environ.get calls.

Runs were isolated with --setting-sources project,local --plugin-dir <3.10.0|this branch> --strict-mcp-config (one code-intelligence server, or none), one fresh fixture copy per run, the plugin version confirmed from the prompts each run copied into its review directory.

Run	Plugin	Server	Actual $	Verified	Alias caller	.txt constant	Duplicate	get
A	this branch @ 0f0aea5	connected	10.88	15	✅ graph	✅	✅	✅
B	this branch @ 0f0aea5	none	9.17	11	✅	✅	✅	✅
C	3.10.0	none	8.98	14	✅	✅	❌	✅
D	3.10.0	connected	10.51	16	✅	✅	✅	✅
E	3.10.0 (unintended — see note)	connected	10.30	12	✅	✅	✅	✅
F	this branch @ b5577f9	connected	10.39	16	✅ graph	✅	❌	✅

All six: CHANGES_REQUESTED, 0 rejected / 0 pending / 0 coverage gaps.

Pass conditions

• Aliased-import caller — ✅ checkout.py:9 reported discovery: "graph" with a verbatim snippet and the alias chain traced in the certificate; verifier confirmed.
• Constant referenced only from .txt — ✅ Bug Hunter B checked index coverage on prompts/, got excluded, made no "unused" claim, found the stale placeholder.
• get namesakes — ✅ every run cites only the two real call sites.
• No server ≈ 3.10.0 (B vs C) — ✅ same verdict, same three planted breakages; differences only in incidental MEDIUMs.
• Transcript checks — ✅ every change-impact call on this branch carried a path scope, a limit, base_ref: main and a root argument; no index-backed completeness wording ("all callers" / "only N call sites") in any run on this branch.
• Near-duplicate helper — ⚠️ unproven. Caught in 2 of 3 runs on each version, always by reading. No reviewer on this branch made a duplication query (0 in A, 0 in F). C7's wording is harmless but has no real-run evidence that it changes behaviour.
• Broker discovery (3e2b26d) — ⚠️ not exercised. Zero broker calls in any run; nothing needed it.
• Not run: the "duplicate data unbuilt" variant.

Cost — and the fix it prompted

Run A's reviewers called change impact 3× with limit: 100, depth: 2, whole directories → 29–33 KB each (the changed-symbols half of the response is not bounded by the row limit). b5577f9 tightens the C5 wording to one caller hop, ≤25 rows, only files whose exported surface changed. Run F after it: 1 call, 4 files, depth: 1, limit: 25 → 7.9 KB.

Like-for-like (/code-review:cost yardstick, relative only): 3.10.0 + server $53.14 / $54.75 (D, E) · this branch before fix $58.34 (A) · after fix $52.03 (F). No server: $42.62 (C) vs $44.35 (B). Impact Analyzer alone ranged $5.26–$7.89 between the two identical 3.10.0 runs, so per-role deltas at n=1 are noise; whole-run cost after the fix is inside the 3.10.0 range.

Counter-example worth keeping: run E (3.10.0, no C5 guidance) had a reviewer call change impact with no path scope, no limit, and base_ref: HEAD~1 — the wrong base — for 33 KB. That is the behaviour C5 CALL DISCIPLINE exists to prevent.

Found along the way — not caused by this PR

• ISS-10862 — under Claude Code 2.1.278 the graph review worker holds no Grep/Glob (reproduces on 3.10.0). The Impact Analyzer said "I have no text-search tool in my roster", spawned an Explore subagent, and tagged every call site graph with a null grep_query_used — even with no server connected — so the verifier's grep-replay gate never ran. Precision held via the per-entry read/content-match audit.
• Unscoped tools answered for another repository. The server's daemon was pinned to a different live session's workspace; tools without a root argument answered for that repo. Reviewers detected it and discarded the answers (protocol working), but it shows CODE_INTEL_REQUIRE_ROOT_ARG=false ("an unscoped tool already answers for the session's checkout") is not safe for a shared-daemon server.
• Plugin-root fallback. With CLAUDE_PLUGIN_ROOT empty under --plugin-dir, the orchestrator picks its helper directory by fallback and chose differently between runs (A: the branch; E: the marketplace 3.10.0 copy). Run F set it explicitly.
• The three planted breakages are each reported ~3× by different reviewers in every run — existing cross-reviewer duplication.

Commits added since this PR opened: b5577f9 (C5 bounds), 4e89bd6 (CHANGELOG). 3e2b26d still reverts cleanly on its own.