Skip to content

Commit d7e634d

Browse files
mikeangstadtclaude
andcommitted
feat(code-review): review non-application files + three TestQuality shapes
Extends the shared reviewer prompt to cover CI/pipeline definitions, guard and meta-test scripts, migrations, and docs/API specs, plus the wiring-unproven, assertion-of-existence, and input-invariant-fixture TestQuality shapes. Prompt-only: no dispatch, roster, or schema change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 5c6b6ab commit d7e634d

3 files changed

Lines changed: 102 additions & 1 deletion

File tree

CHANGELOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,15 @@ 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+
#### Added
10+
- **The shared reviewer prompt now covers non-application files (CI/pipeline definitions, guard and meta-test scripts, migrations, docs/API specs) and three high-frequency `TestQuality` shapes.** Measured against a five-day window of one consuming repo — 2,617 original review comments across 326 PRs, attributed per reviewer agent — the fleet's coverage was categorically absent outside application source: **0 of 101** findings on CI guard/deploy scripts, **0 of 69** on workflow definitions, **0 of 94** on docs/MDX, **0 of 28** on migrations, and **1 of 219** on test files, while producing 68% of all `.tsx` findings and 72% of all prototype findings in the same window. Those file classes were 22% of the corpus and were being carried almost entirely by other reviewer agents. Two additions to `tools/prompts/shared_prompt.txt`, which every reviewer role loads, so no dispatch, roster, or schema change is involved: a `<non_application_files>` block declaring those files in scope and enumerating the generic defect classes per class (privileged jobs running repository lifecycle scripts before minting a write token; workflow-scope write permissions and persisted checkout credentials; the implicit "all dependencies succeeded" condition that is false for a *skipped* dependency; missing job timeouts; cancel-on-new-run over irreversible side effects; trigger filters that do not match what they gate; a shell step whose unguarded command can pre-empt the branch owning its exit code; guards that assert a predicate *appears* rather than executing the decision; allowlist entries broad enough to suppress future real violations; destructive migrations selecting survivors by a proxy for canonicity; idempotency guards that no-op over an invalid object left by a failed run; an enum value added without updating every classifier; docs claims stronger than the code enforces; generated-client specs admitting combinations the runtime rejects), and a `<test_quality>` block naming the three shapes that stay green while proving nothing — wiring-unproven (helper tested, production caller never asserted), assertion-of-existence (source scanned for a predicate instead of executing it), and input-invariant fixture (a handler returning identical results regardless of the params the test claims to cover).
11+
- **Pinned-file pair detection.** Repos routinely hold meta-tests asserting another file's literal content (a workflow's action-version pin, a config snapshot, a required-context manifest, an allowlist). Reviewers are now told to grep for a test referencing the changed path or a distinctive changed literal, and to report a `Correctness` finding citing both file:line pairs when that test exists and is not in the diff. Reported as `Correctness` rather than `CompanionChange` deliberately: it is a provable CI failure with a concrete assertion to cite, and it needs no rule-driven `reasoning_certificate`, which the `CompanionChange` fix template interpolates.
12+
13+
#### Notes
14+
- `shared_prompt.txt` grows from 317 to 409 lines (~17.5 KB → ~24 KB), so each reviewer agent's prompt carries roughly +1.6k input tokens. It is identical across agents and stable across runs, so it caches; measure with `/code-review cost` if the fleet-side delta matters.
15+
716
### code-review v3.7.0
817

918
#### 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.0",
4+
"version": "3.8.0",
55
"author": {
66
"name": "ClosedLoop",
77
"email": "support@closedloop.ai"

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

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,98 @@ Do NOT flag:
102102
- Hypothetical edge cases dependent on specific inputs/state
103103
</constraints>
104104

105+
<non_application_files>
106+
Config, CI, infrastructure, migration, docs, and test files in <files_assigned>
107+
are IN SCOPE and are systematically under-reviewed relative to application
108+
source. When the diff touches them, apply the same evidence standard as for
109+
application code — every finding here still needs a concrete file:line and a
110+
described failure. The recurring defect classes, by file class:
111+
112+
CI / PIPELINE DEFINITIONS (workflow YAML, pipeline configs, composite actions):
113+
- A privileged job (write-scoped token or secrets) that runs repository
114+
lifecycle scripts — dependency install, postinstall/prepare hooks, build —
115+
BEFORE minting or consuming that credential. Lifecycle code from the ref under
116+
test can persist PATH/env changes, shell hooks, or a background process and
117+
intercept later tool invocations once the credential is present.
118+
- Write permissions or persisted checkout credentials scoped to the whole
119+
workflow when only one step needs them.
120+
- A manual-dispatch path exposing secrets or write tokens with no trusted-ref
121+
gate. Checking out a trusted ref INSIDE the job does not change what the
122+
dispatch already exposed.
123+
- A job declaring dependencies plus a condition with no status-check function:
124+
the implicit "all dependencies succeeded" is FALSE for a skipped dependency,
125+
so a skippable dependency silently skips the dependent job.
126+
- A job with no explicit timeout — it inherits a multi-hour platform default, so
127+
a runner that wedges (stops progressing rather than failing) holds its
128+
concurrency group for that whole window.
129+
- Cancel-on-new-run applied to a job with irreversible side effects (publishing,
130+
releasing, deploying an immutable artifact).
131+
- A path/trigger filter that does not actually match files it is meant to gate.
132+
- A shell step where an unguarded command can terminate the step before the
133+
branch that owns its exit code, or before its telemetry/status emission.
134+
135+
GUARD AND META-TEST SCRIPTS (lint rules, source tests, policy checks):
136+
- PINNED-FILE PAIR BROKEN. Repos commonly hold meta-tests that assert another
137+
file's literal content — a workflow's pinned action version, a config
138+
snapshot, a required-context manifest, an allowlist. When the diff edits such
139+
a file, grep for a test referencing that path or a distinctive literal from
140+
the changed lines; if that test exists and is NOT in the diff, the change
141+
breaks it. This is a provable CI failure, so report it as "Correctness" with
142+
both file:line pairs cited (the edited line and the assertion it contradicts),
143+
not as a stylistic concern. It applies in both directions — loosening the
144+
assertion without updating the file it pins is the same defect.
145+
- A guard asserting a predicate APPEARS in a file rather than executing the
146+
decision. A separate permissive branch, or a short-circuit ahead of the
147+
compound condition, keeps every such assertion green while widening behavior.
148+
- A guard anchored to the first matching block rather than the specific
149+
production node, so an unused or disconnected copy satisfies it.
150+
- An allowlist or known-exceptions entry broad enough to suppress future real
151+
violations rather than only the instance it was added for.
152+
153+
MIGRATIONS / SCHEMA:
154+
- A destructive statement (dedupe, drop, backfill) selecting survivors by a
155+
proxy for canonicity and cascading the rest.
156+
- An idempotency guard that no-ops over an invalid or partial object left by a
157+
previous failed run, letting the migration record as successfully applied.
158+
- A new enum/status/discriminator value added without updating every classifier
159+
that switches on it, so stored rows follow the wrong lifecycle.
160+
161+
DOCS, READMEs, AND API SPECS:
162+
- A claim stronger than the code enforces: unconditional where the code is
163+
conditional, product-wide where the behavior is one lane, or collapsing layers
164+
the implementation deliberately keeps separate.
165+
- A behavior change in this diff whose describing document is not updated.
166+
- A generated-client spec admitting field combinations the runtime always
167+
rejects, or omitting inputs the runtime accepts.
168+
169+
TEST FILES: a test file in the diff is reviewable work product, not merely
170+
evidence for findings about other files. See <test_quality> below.
171+
</non_application_files>
172+
173+
<test_quality>
174+
Three TestQuality shapes are high-frequency and easy to miss because the suite
175+
is green. Report them as category "TestQuality" with concrete file:line:
176+
177+
1. WIRING UNPROVEN. A new helper, reducer, or classifier gets direct tests while
178+
nothing asserts that its production caller invokes it — the route handler,
179+
the event/action set, the IPC or transport boundary. Deleting the call site
180+
would leave the suite green. Check that some test drives the production entry
181+
point, not only the unit.
182+
2. ASSERTION OF EXISTENCE, NOT BEHAVIOR. A test that greps, regex-matches, or
183+
AST-inspects source to confirm a predicate is present, instead of executing
184+
that decision against synthetic inputs. It cannot distinguish a correct
185+
implementation from a widened one.
186+
3. INPUT-INVARIANT FIXTURE. A mock, stub, or route handler returning the same
187+
result regardless of the arguments, query params, or filters the test claims
188+
to cover. The test then proves only that the UI changed, not that the request
189+
or predicate did. Look for a handler ignoring its request argument while the
190+
test name references filtering, sorting, or pagination.
191+
192+
Also flag a test asserting only a count or a container class where the state
193+
mapping under test (per-item variant, icon, tone) could be uniformly wrong and
194+
still pass.
195+
</test_quality>
196+
105197
<instructions>
106198
JUSTIFICATION COMMENTS:
107199
Inline justification comments (// Intentionally..., // Required for...) REDUCE your

0 commit comments

Comments
 (0)