Skip to content

Commit b6cb836

Browse files
wongkclaude
andcommitted
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
1 parent a63505f commit b6cb836

6 files changed

Lines changed: 39 additions & 23 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
1212
- `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.
1313

1414
#### 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.
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.
1617

1718
### code-review v3.7.1
1819

plugins/code-review/skills/spawn-reviewers/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ Context-heavy operations that cause "Prompt is too long" failures:
5353
- **`code-review:code-review-worker`** (default; `tools: Read, Write, Grep, Glob`) — use for EVERY reviewer EXCEPT the four code-intelligence-aware roles below. This includes Bug Hunter A, Unified Auditor, Domain Critics, the **verifier fleet** (stage_23), and the **PLN-725 singletons** (stage_11 / stage_15). Its explicit allowlist is what keeps these roles at exactly four tools — they inherit NOTHING from the session, keeping the trust boundary tight for the adversarial verifier and the singleton prompts that never load the code-intelligence protocol.
5454
- **`code-review:code-review-worker-graph`** (no `tools:` allowlist — inherits the session's tools, minus a `disallowedTools` denylist for Bash/Edit/NotebookEdit) — use ONLY for the code-intelligence-aware roles: **Bug Hunter B**, the **Impact Analyzer**, the **Design Critic**, and the **Fast Path** reviewer (which runs a BHB pass). These are the only roles whose prompts load the "OPTIONAL — CODE INTELLIGENCE" protocol. (BHB / Impact / fast-path use the cross-file capabilities C1–C3; the Design Critic also uses the structural capability C4.)
5555

56-
Both end up with the core `Read, Write, Grep, Glob` tools, so file-access permissions and the write-denied fallback work identically; the inheriting variant additionally holds whatever else the operator's session has connected.
56+
The two differ in what they can rely on, and the prompts account for it. `code-review-worker`'s allowlist *guarantees* the core four regardless of what the spawning session holds. The inheriting worker gets whatever that session has — which is usually the core four plus the session's MCP servers, but is NOT guaranteed: a session that supplies its own search tooling instead of `Grep`/`Glob` yields a reviewer without them. That is why the shared prompt states text search as a capability rather than a tool name and tells the reviewer to fall back to targeted `Read` calls, and why `grep_query_used` must describe a query actually executed. `Write` is inherited in practice, and the write-denied fallback in `shared_prompt.txt` (emit `<findings_json>` inline, report `file=WRITE_DENIED`) still covers the case where it is refused.
5757

5858
**Code-intelligence gate (do once, before spawning the code-intelligence-aware roles).** The plugin does not require, name, or probe any particular MCP server. Discovery is the reviewer's job — it holds the tool schemas, so it is the only party that can bind a capability to a real call. The orchestrator decides exactly one thing: whether an external index may be trusted for this run at all.
5959

plugins/code-review/tools/prompts/impact_analyzer_prompt.txt

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,19 @@ Every finding you emit MUST:
4747
that breaks (syntactically, semantically, or behaviorally) under
4848
the new symbol signature
4949
- populate `grep_query_used` with the exact grep query that found
50-
the external usages (the verifier will replay it)
50+
the external usages (the verifier will replay it) — UNLESS you hold
51+
no text-search tool at all, in which case it stays null; see the
52+
text-search rule under Inputs
5153
- populate `reasoning_certificate` with `kind: "impact"` and the
5254
full fields listed below
5355

54-
Findings without a populated `external_impact[]`, without a
55-
`grep_query_used`, or with a certificate whose `kind` does not equal
56-
`"impact"`, will be rejected by the verifier as malformed.
56+
Findings without a populated `external_impact[]`, or with a certificate
57+
whose `kind` does not equal `"impact"`, will be rejected by the verifier
58+
as malformed. A missing `grep_query_used` is malformed ONLY when at
59+
least one entry is `discovery: "grep"`; an all-`"graph"` finding
60+
legitimately carries a null query and is verified per-entry instead.
61+
Never invent a query to satisfy this list — a `grep_query_used` you did
62+
not execute is fabricated evidence and is treated as such.
5763

5864
## Inputs
5965

@@ -164,9 +170,12 @@ For each candidate symbol:
164170
Prefer unambiguous identifiers (`FooBar`, `parseCacheKey`) over
165171
common words. For ambiguous names (`get`, `update`), narrow with
166172
surrounding context: `\.parseCacheKey\(` or `import.*\bUser\b`.
167-
- Record the **exact** query string in `grep_query_used`. The
173+
- Record the **exact** query string in `grep_query_used` — the query
174+
you actually ran, never a reconstruction. The
168175
verifier replays it; if your query and the replay disagree by more
169-
than a small margin, the finding is REJECTED.
176+
than a small margin, the finding is REJECTED. If you hold no
177+
text-search tool, there is no query to record: leave it null and
178+
tag the entries `discovery: "graph"` per the Inputs section.
170179
- Use `output_mode: "files_with_matches"` first to enumerate files;
171180
then `output_mode: "content"` with `-n` and `-B 5 -A 10` to read
172181
context around each hit.

plugins/code-review/tools/prompts/shared_prompt.txt

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -304,9 +304,10 @@ pipeline; you may emit them when relevant, otherwise omit):
304304
rule for these informational attachments.
305305
``discovery`` records HOW the callsite was found: ``"grep"`` (the
306306
default — reproducible by replaying ``grep_query_used``) or
307-
``"graph"`` (found via a code-intelligence substrate — whichever MCP
308-
server the session provides; e.g. an alias,
309-
re-export, or dynamic-dispatch caller that grep cannot surface). Both
307+
``"graph"`` (found on a path the replay cannot reproduce — either a
308+
code-intelligence substrate surfaced it, e.g. an alias, re-export or
309+
dynamic-dispatch caller grep cannot reach, or no text-search tool was
310+
available at all so no replayable query exists). Both
310311
are verified the same way — the verifier reads the cited file:line and
311312
content-matches ``callsite_snippet`` — but ``"graph"`` entries are exempt
312313
from the grep-replay completeness check (see ``grep_query_used``).

plugins/code-review/tools/prompts/verifier_prompt.txt

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -256,9 +256,10 @@ whether the reviewer found it via grep or via a code-intelligence
256256
substrate
257257
(`external_impact[i].discovery` ∈ {`"grep"`, `"graph"`}, default
258258
`"grep"`). Reading the cited file:line and content-matching the snippet
259-
is the canonical proof — a `discovery: "graph"` entry (an alias,
260-
re-export, or dynamic-dispatch caller grep cannot surface) is verified
261-
HERE, not in the grep replay below.
259+
is the canonical proof — a `discovery: "graph"` entry (a caller the
260+
replay cannot reproduce: an alias, re-export or dynamic-dispatch caller
261+
grep cannot surface, or any caller found in a session with no
262+
text-search tool) is verified HERE, not in the grep replay below.
262263

263264
For EACH entry in `finding.external_impact[]`:
264265

plugins/code-review/tools/python/code_review_schema.py

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -263,11 +263,14 @@
263263
# Provenance values for ``external_impact[].discovery`` (FEA-1401 graph
264264
# integration). ``grep`` (default) entries are reproducible via the
265265
# verifier's grep-replay of ``grep_query_used``; ``graph`` entries were
266-
# found only via a code-intelligence substrate — whichever indexing MCP
267-
# server the operator's session provides — and are verified per-entry by
268-
# file-read + snippet-hash, exempt from the grep-replay completeness gate.
269-
# The value records HOW a callsite was found, not which product found it;
270-
# it stays stable across substrates by design.
266+
# found on a path the grep replay cannot reproduce, and are verified
267+
# per-entry by file-read + snippet-hash, exempt from the grep-replay
268+
# completeness gate. Two branches set it: a code-intelligence substrate
269+
# surfaced a caller grep cannot reach (alias, re-export, dynamic
270+
# dispatch), or the session held no text-search tool at all, so no
271+
# replayable query exists for any entry. The value records HOW a
272+
# callsite was found, not which product found it; it stays stable
273+
# across substrates by design.
271274
EXTERNAL_IMPACT_DISCOVERY: frozenset[str] = frozenset({
272275
"grep",
273276
"graph",
@@ -831,10 +834,11 @@ class ExternalImpact:
831834
confidence: float
832835
# Provenance of how the callsite was found (FEA-1401 graph integration).
833836
# "grep" (default) → reproducible by replaying grep_query_used.
834-
# "graph" → found only via a code-intelligence substrate, whichever
835-
# indexing MCP server the session provides (alias/re-export/dynamic
836-
# dispatch grep cannot surface); verified by per-entry file-read +
837-
# content match, exempt from the verifier's grep-replay completeness check.
837+
# "graph" → found on a path the grep replay cannot reproduce: either a
838+
# code-intelligence substrate surfaced it (alias/re-export/dynamic
839+
# dispatch grep cannot surface), or the session had no text-search
840+
# tool at all so no replayable query exists; verified by per-entry
841+
# file-read + content match, exempt from the grep-replay check.
838842
discovery: str = "grep"
839843

840844

0 commit comments

Comments
 (0)