diff --git a/CHANGELOG.md b/CHANGELOG.md index ab793713..21adaa95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,18 @@ 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.11.0 + +#### Added +- Config, CI, infrastructure, migration, docs, and test files in `` are declared in scope in `tools/prompts/shared_prompt.txt`, on the same evidence standard as application source. The declaration carries no checklist: the per-class defect lists are routed to the reviewer that owns them, split by whether the defect is decidable from the changed file or needs a file outside the diff, and the shared prompt tells each role not to reconstruct another role's list. +- `tools/prompts/bha_suffix.txt` (Bug Hunter A, diff-local correctness) gains the classes readable in the changed file: CI/pipeline definitions (a privileged job running repository lifecycle scripts before minting or consuming a write-scoped credential; write permissions and persisted checkout credentials scoped to the whole workflow; a manual-dispatch path with no trusted-ref gate; a dependency plus a condition with no status-check function, whose implicit "all dependencies succeeded" is false for a *skipped* dependency; a job with no explicit timeout; cancel-on-new-run over irreversible side effects; a path/trigger filter that does not match what it gates; a shell step whose unguarded command can pre-empt the branch owning its exit code), migrations/schema (a destructive statement selecting survivors by a proxy for canonicity; an idempotency guard that no-ops over an object left by a failed run; a new enum value added without updating every classifier), guard-script shape (a guard asserting a predicate *appears* rather than executing the decision; a guard anchored to the first matching block; an allowlist entry broad enough to suppress future real violations), and two test shapes readable in the test — assertion-of-existence (emitted as `TestQuality`/`weak-assertion`) and input-invariant fixture (`TestQuality`/`mock-faithfulness`). +- The Bug Hunter B suffix in `skills/spawn-reviewers/SKILL.md` (cross-file, unpartitioned; both the standard and fast-path copies) gains the classes that require a file outside the diff: the pinned-file pair, wiring-unproven (`TestQuality`/`missing-coverage` — a helper tested while nothing asserts its production caller runs it), and the docs/API-spec classes (a claim stronger than the code enforces; a behavior change whose describing document is not updated; a generated-client spec admitting combinations the runtime rejects). +- Pinned-file-pair detection, emitted as `Correctness` with subcategory `pinned-file-pair`. 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 — and editing the pinned file breaks them. The contract is narrow in three ways: + - **A path reference is not evidence.** The reviewer must quote the specific assertion, at its own file:line, whose expected value the pinned file no longer satisfies, and name the contradicted value beside the new one. A test that names the changed path only as a fixture input, a synthetic file list, a glob, or a docstring pins nothing and is not a finding. + - **Absence from the diff is not evidence.** A partitioned Bug Hunter A worker receives only its own patch, and a reviewer's `` is a scope rather than a manifest of the PR, so full-PR diff membership is not knowable to a reviewer and is excluded from the argument. The case rests on the contradiction between the assertion read at HEAD and the file as it now stands. + - **The companion assertion is structured data**, carried in the existing `other_locations[]` field (`{file, line, issue}`) rather than in prose. +- `skills/fix/SKILL.md` routes `Correctness`/`pinned-file-pair` to the **manual-surface** bucket with a new `templates/correctness_pinned_file_pair.md`. Without that row the finding fell through to the `Correctness` auto-fix row, whose drift check and edit both anchor on `finding.file` — the changed config or workflow — so `--apply` would have reverted the intended change rather than updating the unchanged assertion. The template renders both sides and leaves the choice of which one is wrong to the operator. + ### code-review v3.10.0 #### Added diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index a2c3e3be..1ffce1e0 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.10.0", + "version": "3.11.0", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/skills/fix/SKILL.md b/plugins/code-review/skills/fix/SKILL.md index 2f619d58..32caa968 100644 --- a/plugins/code-review/skills/fix/SKILL.md +++ b/plugins/code-review/skills/fix/SKILL.md @@ -96,6 +96,7 @@ For each surviving finding, look up its dispatch bucket using the table below. S | Category | Subcategory | Bucket | Notes | |---|---|---|---| +| `Correctness` | `pinned-file-pair` | **manual-surface** | The finding anchors on the CHANGED file (diff-scope validation requires it), but the edit that resolves it belongs in the UNCHANGED companion assertion cited in `other_locations[]`. Auto-fix would edit the anchor and revert the intended config/workflow change. Two files, one of them outside the diff — operator judgment. | | `Correctness` | — | **auto-fix** | Direct code edit at anchor line | | `Code Quality` | — | **auto-fix** | DRY / maintainability — auto-fix at anchor | | `Documentation` | — | **auto-fix** | Edit cited file:line | @@ -199,6 +200,7 @@ For each finding in the manual-surface bucket, look up the template per the rout | `TestQuality/test-deletion` | `templates/testquality_test_deletion.md` | | `TestQuality/*` (other, pre-PLN-723) | `templates/testquality_specialized.md` | | `ImpactAnalysis/*` (pre-PLN-726) | `templates/impact_semantic_change.md` | +| `Correctness/pinned-file-pair` | `templates/correctness_pinned_file_pair.md` | | `CompanionChange/*` | `templates/companion_change.md` | | `Coverage/*` | `templates/coverage_gap.md` | | `InjectionAttempt/*` | `templates/injection_attempt.md` | diff --git a/plugins/code-review/skills/fix/templates/correctness_pinned_file_pair.md b/plugins/code-review/skills/fix/templates/correctness_pinned_file_pair.md new file mode 100644 index 00000000..9f4a7c51 --- /dev/null +++ b/plugins/code-review/skills/fix/templates/correctness_pinned_file_pair.md @@ -0,0 +1,23 @@ +### ⚠️ Correctness / pinned-file-pair — `{file}:{line}` ({severity}) + +**Issue:** {issue} + +**Changed file (finding anchor):** `{file}:{line}` +``` +{code_snippet} +``` + +**Companion assertion(s) this change contradicts (other_locations, one `file:line — issue` per entry):** +{other_locations_rendered} + +**Explanation:** {explanation} + +**Why this is surfaced manually:** This defect spans two files and the fix almost never belongs at the anchor. Diff-scope validation requires the finding to anchor on the changed file, and auto-fix edits that anchor — so applying it would revert the intended config, workflow, or manifest change instead of updating the stale assertion that pins it. Which side is wrong is the operator's call: the change may be correct and the assertion stale, or the assertion may be the contract and the change a mistake. The companion file is also typically outside this review's diff, so `/fix` has not verified its current content. + +**Your options:** +1. **Update the companion assertion** at the `other_locations[]` file:line above to match the changed file, if the change is intended. +2. **Revert or correct the change** at the anchor, if the pinned assertion is the contract being violated. +3. **Dismiss** if the companion test does not actually pin these lines — a test that only names the changed path as fixture input pins nothing, and this finding should not have been emitted. +4. **Re-assert** via `python3 /tools/python/code_review_helpers.py re-assert --cr-dir --cache-dir --finding-ids {id}`. (Resolve `` from `/cache_config.json:cache_dir`.) + +**Original recommendation:** {recommendation} diff --git a/plugins/code-review/skills/spawn-reviewers/SKILL.md b/plugins/code-review/skills/spawn-reviewers/SKILL.md index 8a6b58e5..c27cf8ef 100644 --- a/plugins/code-review/skills/spawn-reviewers/SKILL.md +++ b/plugins/code-review/skills/spawn-reviewers/SKILL.md @@ -189,6 +189,54 @@ Focus areas: For DRY claims, one concrete example of prior art is sufficient (cite file path + function name). +NON-APPLICATION FILES — the cross-file half. Config, CI, migration, docs, and +test files in the diff are reviewable work product on the same evidence standard +as source. The classes below are yours BECAUSE each one needs a file outside the +diff; the shapes decidable from the changed file alone belong to Bug Hunter A and +you should not re-review them. + +- 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 that pins the changed lines. + EVIDENCE BAR — a path reference is NOT evidence. You must quote the specific + assertion, at its own file:line, whose expected value the pinned-head file no + longer satisfies: a literal comparison, an exact-match membership check, a + snapshot equality. A test that merely names the changed path — as a fixture + input, a synthetic `files=[...]` list, a glob it iterates, a docstring — pins + nothing and is not a finding. If you cannot name the contradicted expected + value and the new value beside it, you do not have this finding. + DO NOT infer the defect from the companion test's absence from the diff. You + are not given full-PR diff membership (a partitioned Bug Hunter A worker sees + only its own patch, and your `` is a scope, not a manifest of + every file the PR touches), so "the test is not in the diff" is unknowable and + is never part of the argument. The contradiction between the assertion and the + file as it now stands is the whole case; read the companion test at HEAD and + check whether it was already updated before you report. + EMIT as category "Correctness" with subcategory "pinned-file-pair" — the + subcategory is required, because it is what routes the finding to manual + handling instead of an auto-edit at the changed file. Anchor `file`/`line` on + the edited line as usual, and put the companion assertion in + `other_locations[]` as `{file, line, issue}` with the contradicted expected + value in `issue`. It applies in both directions — loosening the assertion + without updating the file it pins is the same defect, reported the same way. + +- WIRING UNPROVEN (category "TestQuality", subcategory "missing-coverage"). 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. Cite the production caller you read, at file:line, and say what + no test drives. + +- DOCS, READMEs, AND API SPECS (category "Documentation"), each held against the + code you read: + - 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. Cite both sides. + - 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. + CODE INTELLIGENCE (optional): CODE_INTEL_ALLOWED=, CODE_INTEL_REQUIRE_ROOT_ARG=. Follow the "OPTIONAL — CODE INTELLIGENCE" protocol in {CR_DIR}/shared_prompt.txt: inspect your own @@ -422,6 +470,54 @@ Focus areas: For DRY claims, one concrete example of prior art is sufficient (cite file path + function name). +NON-APPLICATION FILES — the cross-file half. Config, CI, migration, docs, and +test files in the diff are reviewable work product on the same evidence standard +as source. The classes below are yours BECAUSE each one needs a file outside the +diff; the shapes decidable from the changed file alone belong to Bug Hunter A and +you should not re-review them. + +- 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 that pins the changed lines. + EVIDENCE BAR — a path reference is NOT evidence. You must quote the specific + assertion, at its own file:line, whose expected value the pinned-head file no + longer satisfies: a literal comparison, an exact-match membership check, a + snapshot equality. A test that merely names the changed path — as a fixture + input, a synthetic `files=[...]` list, a glob it iterates, a docstring — pins + nothing and is not a finding. If you cannot name the contradicted expected + value and the new value beside it, you do not have this finding. + DO NOT infer the defect from the companion test's absence from the diff. You + are not given full-PR diff membership (a partitioned Bug Hunter A worker sees + only its own patch, and your `` is a scope, not a manifest of + every file the PR touches), so "the test is not in the diff" is unknowable and + is never part of the argument. The contradiction between the assertion and the + file as it now stands is the whole case; read the companion test at HEAD and + check whether it was already updated before you report. + EMIT as category "Correctness" with subcategory "pinned-file-pair" — the + subcategory is required, because it is what routes the finding to manual + handling instead of an auto-edit at the changed file. Anchor `file`/`line` on + the edited line as usual, and put the companion assertion in + `other_locations[]` as `{file, line, issue}` with the contradicted expected + value in `issue`. It applies in both directions — loosening the assertion + without updating the file it pins is the same defect, reported the same way. + +- WIRING UNPROVEN (category "TestQuality", subcategory "missing-coverage"). 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. Cite the production caller you read, at file:line, and say what + no test drives. + +- DOCS, READMEs, AND API SPECS (category "Documentation"), each held against the + code you read: + - 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. Cite both sides. + - 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. + CODE INTELLIGENCE (optional): CODE_INTEL_ALLOWED=, CODE_INTEL_REQUIRE_ROOT_ARG=. Follow the "OPTIONAL — CODE INTELLIGENCE" protocol in {CR_DIR}/shared_prompt.txt — when diff --git a/plugins/code-review/tools/prompts/bha_suffix.txt b/plugins/code-review/tools/prompts/bha_suffix.txt index f8601eb1..ed3c59b7 100644 --- a/plugins/code-review/tools/prompts/bha_suffix.txt +++ b/plugins/code-review/tools/prompts/bha_suffix.txt @@ -21,4 +21,72 @@ Apply this certificate and BUG CONFIRMED emission gate only when acting as Bug H Only emit findings where CONCLUSION = BUG CONFIRMED with a completed trace. Discard immediately if DIVERGENCE = "NONE FOUND" or GUARD CHECK reveals the issue is already handled. + +NON-APPLICATION FILES IN YOUR PARTITION: +Config, CI, infrastructure, migration, and test files are yours to review on the +same terms as application source — same certificate, same file:line, same BUG +CONFIRMED gate. The classes below are the ones decidable from the changed file +itself, which is why they are yours and not Bug Hunter B's. Anything needing a +file outside the diff is B's; do not reach for it. + +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. + +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. + +GUARD AND META-TEST SCRIPTS (lint rules, source tests, policy checks) — the +shapes visible in the guard itself: +- 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. +(The paired defect — a meta-test whose pinned assertion this diff contradicts — +needs a file outside your patch and belongs to Bug Hunter B.) + +TEST FILES IN YOUR PARTITION — two shapes readable in the test itself. Emit as +category "TestQuality" with the existing subcategory named: +- ASSERTION OF EXISTENCE, NOT BEHAVIOR (subcategory "weak-assertion"). 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. +- INPUT-INVARIANT FIXTURE (subcategory "mock-faithfulness"). 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. The third shape — a helper tested while nothing asserts its +production caller runs it — needs the caller, so it is Bug Hunter B's. + Use Read, Grep, and Glob for codebase context. Do NOT use Bash. diff --git a/plugins/code-review/tools/prompts/shared_prompt.txt b/plugins/code-review/tools/prompts/shared_prompt.txt index 8f26b1b8..ccea9bbd 100644 --- a/plugins/code-review/tools/prompts/shared_prompt.txt +++ b/plugins/code-review/tools/prompts/shared_prompt.txt @@ -139,6 +139,23 @@ Do NOT flag: - Hypothetical edge cases dependent on specific inputs/state + +Config, CI, infrastructure, migration, docs, and test files in +are IN SCOPE. They are reviewable work product, not merely evidence for findings +about application source, and they carry the SAME evidence standard: every +finding still needs a concrete file:line and a described failure. + +The per-class defect checklists are NOT here. They are routed to the reviewer +that owns them, so read only your own role suffix for them: +- Defects decidable from the changed file itself (CI/pipeline definitions, + migrations/schema, guard-script shape, and the test-file shapes readable in + the test) belong to Bug Hunter A. +- Defects that require reading a file OUTSIDE the diff (a meta-test pinning a + changed file's content, a production caller that proves wiring, a document + whose claim must be held against the code) belong to Bug Hunter B. +Do not reconstruct another role's checklist from this declaration. + + JUSTIFICATION COMMENTS: Inline justification comments (// Intentionally..., // Required for...) REDUCE your