Skip to content

Commit fd21741

Browse files
wongkclaude
andcommitted
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
1 parent db8a791 commit fd21741

2 files changed

Lines changed: 33 additions & 7 deletions

File tree

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

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -274,8 +274,9 @@ entries can cite any repo file.
274274
275275
Write findings to <output_file> in the JSON shape documented in
276276
shared_prompt.txt (`category: "ImpactAnalysis"`, populated
277-
external_impact[] and grep_query_used). Emit findings only when you
278-
have ≥1 concrete external usage with cited breakage. If grep returns
277+
external_impact[]; `grep_query_used` populated whenever any entry is
278+
`discovery: "grep"`). Emit findings only when you
279+
have ≥1 concrete external usage with cited breakage. If your search finds
279280
zero external usages OR every usage is guarded, do not emit a finding
280281
for that symbol.
281282
@@ -285,17 +286,22 @@ deferred schemas with ToolSearch first) and ALSO use its capability C2 (usage/ca
285286
enumeration) to reach callers grep cannot (aliases,
286287
re-exports, dynamic dispatch); tag those entries `discovery: "graph"` and put
287288
them in the certificate's `graph_discovered_usages` per the Inputs/Step 2
288-
sections of impact_analyzer_prompt.txt. Always run grep too and record a real
289+
sections of impact_analyzer_prompt.txt. Run your text-search tool too whenever you
290+
hold one, and record the real query you ran in
289291
`grep_query_used` for the `discovery: "grep"` entries (the verifier replays it
290-
against `external_usages_found`). Read every callsite to capture its verbatim
292+
against `external_usages_found`). If you hold NO text-search tool at all, leave
293+
`grep_query_used` null and `external_usages_found` empty and tag every entry
294+
`discovery: "graph"` — never write a query you did not execute. Read every callsite
295+
to capture its verbatim
291296
`callsite_snippet` regardless of substrate, and validate substrate-returned paths are
292-
inside this checkout. When CODE_INTEL_ALLOWED is false or nothing answers C2, grep only.
297+
inside this checkout. When CODE_INTEL_ALLOWED is false or nothing answers C2, use
298+
text search alone (or targeted Reads if you hold no search tool).
293299
294300
Respond ONLY with:
295301
DONE findings={count} file={output_file_path}
296302
297-
Use Read, Grep, and Glob — plus whatever code-intelligence MCP tools your
298-
session provides. Do NOT use Bash.
303+
Use Read, plus whatever text-search and code-intelligence tools your session
304+
provides. Do NOT use Bash.
299305
```
300306

301307
**Design Critic** (conditional, deep tier only, `subagent_type: "code-review:code-review-worker-graph"`, model `sonnet`, `AGENT_ID: "design_critic"`):

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -994,6 +994,26 @@ def test_external_impact_discovery_mixed_substrates_valid():
994994
assert validate_finding(f) == []
995995

996996

997+
def test_external_impact_all_graph_with_null_grep_query_valid():
998+
# The no-text-search fallback shape: a session holding no Grep and no
999+
# MCP equivalent cannot produce a replayable query, so the analyzer
1000+
# emits every entry as discovery="graph" with grep_query_used null.
1001+
# The verifier skips the replay gate for this case and audits each
1002+
# entry by file-read + content match instead, so the validator must
1003+
# accept it — rejecting it here would silently drop the fallback's
1004+
# findings and push reviewers back toward inventing a query.
1005+
f = _impact_finding_with_impacts(_impact_entry("graph"))
1006+
f["grep_query_used"] = None
1007+
assert validate_finding(f) == []
1008+
1009+
1010+
def test_external_impact_all_graph_with_omitted_grep_query_valid():
1011+
# Same shape, but the field is absent rather than explicitly null.
1012+
f = _impact_finding_with_impacts(_impact_entry("graph"))
1013+
del f["grep_query_used"]
1014+
assert validate_finding(f) == []
1015+
1016+
9971017
def _impact_entry_with_file(file: str) -> dict:
9981018
entry = _impact_entry("graph")
9991019
entry["file"] = file

0 commit comments

Comments
 (0)