diff --git a/CHANGELOG.md b/CHANGELOG.md index 69dac2d..32ef546 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ All notable changes to the claude-plugins project will be documented in this fil 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`. +### code-review v3.8.0 + +#### Added +- **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 `` 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 `` 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). +- **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. + +#### Notes +- `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. + ### code-review v3.7.0 #### Changed diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 684332f..5d7a7ca 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/plugins/code-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code-review", "description": "Code review plugin", - "version": "3.7.0", + "version": "3.8.0", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/tools/prompts/shared_prompt.txt b/plugins/code-review/tools/prompts/shared_prompt.txt index f3aa532..37e70c7 100644 --- a/plugins/code-review/tools/prompts/shared_prompt.txt +++ b/plugins/code-review/tools/prompts/shared_prompt.txt @@ -102,6 +102,98 @@ Do NOT flag: - Hypothetical edge cases dependent on specific inputs/state + +Config, CI, infrastructure, migration, docs, and test files in +are IN SCOPE and are systematically under-reviewed relative to application +source. When the diff touches them, apply the same evidence standard as for +application code — every finding here still needs a concrete file:line and a +described failure. The recurring defect classes, by file class: + +CI / PIPELINE DEFINITIONS (workflow YAML, pipeline configs, composite actions): +- A privileged job (write-scoped token or secrets) that runs repository + lifecycle scripts — dependency install, postinstall/prepare hooks, build — + BEFORE minting or consuming that credential. Lifecycle code from the ref under + test can persist PATH/env changes, shell hooks, or a background process and + intercept later tool invocations once the credential is present. +- Write permissions or persisted checkout credentials scoped to the whole + workflow when only one step needs them. +- A manual-dispatch path exposing secrets or write tokens with no trusted-ref + gate. Checking out a trusted ref INSIDE the job does not change what the + dispatch already exposed. +- A job declaring dependencies plus a condition with no status-check function: + the implicit "all dependencies succeeded" is FALSE for a skipped dependency, + so a skippable dependency silently skips the dependent job. +- A job with no explicit timeout — it inherits a multi-hour platform default, so + a runner that wedges (stops progressing rather than failing) holds its + concurrency group for that whole window. +- Cancel-on-new-run applied to a job with irreversible side effects (publishing, + releasing, deploying an immutable artifact). +- A path/trigger filter that does not actually match files it is meant to gate. +- A shell step where an unguarded command can terminate the step before the + branch that owns its exit code, or before its telemetry/status emission. + +GUARD AND META-TEST SCRIPTS (lint rules, source tests, policy checks): +- PINNED-FILE PAIR BROKEN. Repos commonly hold meta-tests that assert another + file's literal content — a workflow's pinned action version, a config + snapshot, a required-context manifest, an allowlist. When the diff edits such + a file, grep for a test referencing that path or a distinctive literal from + the changed lines; if that test exists and is NOT in the diff, the change + breaks it. This is a provable CI failure, so report it as "Correctness" with + both file:line pairs cited (the edited line and the assertion it contradicts), + not as a stylistic concern. It applies in both directions — loosening the + assertion without updating the file it pins is the same defect. +- A guard asserting a predicate APPEARS in a file rather than executing the + decision. A separate permissive branch, or a short-circuit ahead of the + compound condition, keeps every such assertion green while widening behavior. +- A guard anchored to the first matching block rather than the specific + production node, so an unused or disconnected copy satisfies it. +- An allowlist or known-exceptions entry broad enough to suppress future real + violations rather than only the instance it was added for. + +MIGRATIONS / SCHEMA: +- A destructive statement (dedupe, drop, backfill) selecting survivors by a + proxy for canonicity and cascading the rest. +- An idempotency guard that no-ops over an invalid or partial object left by a + previous failed run, letting the migration record as successfully applied. +- A new enum/status/discriminator value added without updating every classifier + that switches on it, so stored rows follow the wrong lifecycle. + +DOCS, READMEs, AND API SPECS: +- A claim stronger than the code enforces: unconditional where the code is + conditional, product-wide where the behavior is one lane, or collapsing layers + the implementation deliberately keeps separate. +- A behavior change in this diff whose describing document is not updated. +- A generated-client spec admitting field combinations the runtime always + rejects, or omitting inputs the runtime accepts. + +TEST FILES: a test file in the diff is reviewable work product, not merely +evidence for findings about other files. See below. + + + +Three TestQuality shapes are high-frequency and easy to miss because the suite +is green. Report them as category "TestQuality" with concrete file:line: + +1. WIRING UNPROVEN. A new helper, reducer, or classifier gets direct tests while + nothing asserts that its production caller invokes it — the route handler, + the event/action set, the IPC or transport boundary. Deleting the call site + would leave the suite green. Check that some test drives the production entry + point, not only the unit. +2. ASSERTION OF EXISTENCE, NOT BEHAVIOR. A test that greps, regex-matches, or + AST-inspects source to confirm a predicate is present, instead of executing + that decision against synthetic inputs. It cannot distinguish a correct + implementation from a widened one. +3. INPUT-INVARIANT FIXTURE. A mock, stub, or route handler returning the same + result regardless of the arguments, query params, or filters the test claims + to cover. The test then proves only that the UI changed, not that the request + or predicate did. Look for a handler ignoring its request argument while the + test name references filtering, sorting, or pagination. + +Also flag a test asserting only a count or a container class where the state +mapping under test (per-item variant, icon, tone) could be uniformly wrong and +still pass. + + JUSTIFICATION COMMENTS: Inline justification comments (// Intentionally..., // Required for...) REDUCE your