From c0b99e5dcbab42356723d1d1a41ed6a7d08b2c01 Mon Sep 17 00:00:00 2001 From: mikeangstadt Date: Sun, 19 Jul 2026 14:15:20 -0500 Subject: [PATCH] feat(code-review): detect 8 issue classes that slipped past review Append compact, high-signal detection guidance to existing reviewer prompts for eight issue classes recently caught only by human "soul review", not by /code-review. Prompt-only; no new agents, no orchestration or tooling change. Each heuristic keeps a "verify X before flagging" clause to avoid false positives. - bha_suffix.txt: migration-prefix contiguity (CI-blocking off-by-one); read-only-bridge vs write-path method/body mismatch - impact_analyzer_prompt.txt: cross-surface behavioral flip in shared desktop+cloud core; new unconditional provider-requiring hook in a shared component (Storybook/consumer breakage) - design_critic_suffix.txt: local<->cloud parity divergence - spawn-reviewers/SKILL.md (BHB): SSOT-drift-by-copy; test asserts scaffolding not changed behavior - spawn-reviewers/SKILL.md (Unified Auditor): PR-body/docstring drift vs the diff - bump code-review plugin to v3.8.0; CHANGELOG entry Testing: pytest plugins/code-review/tools/python/ -> 1330 passed, 3 skipped Risks: None identified; prompt content excluded from golden state snapshots Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 5 +++++ plugins/code-review/.claude-plugin/plugin.json | 2 +- .../code-review/skills/spawn-reviewers/SKILL.md | 5 ++++- plugins/code-review/tools/prompts/bha_suffix.txt | 2 ++ .../tools/prompts/design_critic_suffix.txt | 1 + .../tools/prompts/impact_analyzer_prompt.txt | 16 ++++++++++++++++ 6 files changed, 29 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69dac2dc..d8171ab8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ 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 +- **Detection guidance for eight issue classes that recently slipped past `/code-review` (caught only by human "soul review").** Prompt-only, appended to the single most-relevant existing reviewer prompt/checklist — no new agents, no orchestration/tooling change, each heuristic keeps a "verify X before flagging" clause to protect against false positives. Placements: Bug Hunter A (`tools/prompts/bha_suffix.txt`) — migration-prefix contiguity (CI-blocking off-by-one against the committed-migration-name guard) and read-only-bridge vs write-path mismatch (check the bridge's `ALLOWED_METHODS`/body handling before a write is routed through it). Impact Analyzer (`tools/prompts/impact_analyzer_prompt.txt`) — cross-surface behavioral flip in a shared desktop+cloud core (error-handling semantics change → trace ALL consumers, verify "cloud-only" claims) and a new unconditional provider-requiring hook in a shared component (enumerate mount sites incl. Storybook preview). Design Critic (`tools/prompts/design_critic_suffix.txt`) — local↔cloud parity divergence (same-surface paths populating different fields/predicates). Bug Hunter B (`skills/spawn-reviewers/SKILL.md`) — SSOT-drift-by-copy (redeclared literal/helper where a canonical symbol exists) and tests that assert scaffolding rather than the changed behavior. Unified Auditor (`skills/spawn-reviewers/SKILL.md`) — PR-body/docstring drift vs the diff (wrong migration numbers, "no schema change" claims contradicted by the diff, etc.). + ### 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 684332fd..5d7a7ca6 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/skills/spawn-reviewers/SKILL.md b/plugins/code-review/skills/spawn-reviewers/SKILL.md index 65e3a2e7..a29c26ae 100644 --- a/plugins/code-review/skills/spawn-reviewers/SKILL.md +++ b/plugins/code-review/skills/spawn-reviewers/SKILL.md @@ -186,6 +186,8 @@ Focus areas: Check that parameters match (undefined vs null vs empty string matters). - Pattern consistency: Find existing examples of similar code, verify new code matches. - Import validation: Verify imports resolve to real modules. +- SSOT-drift-by-copy: a string literal or helper re-declared where a canonical const/utility of the same value already exists (especially within the same package). Grep for the value/name; if a canonical symbol exists, flag the redeclaration and name the symbol to import instead. +- Test asserts scaffolding, not behavior: when the PR adds/changes a behavior, open the test and confirm it DRIVES that behavior end-to-end (the mutation, the flag-off case, the over-cap/other branch) — not just the wiring/scope/shape around it. Also flag a regression test that only passes because a fixture contradicts documented system behavior (false-confidence test). For DRY claims, one concrete example of prior art is sufficient (cite file path + function name). @@ -218,8 +220,9 @@ For each changed file, check against: 3. Rules tagged [pattern] — these are MEDIUM severity (verify pattern is followed) 4. Explicit rules in the main CLAUDE.md sections (Architecture, Type Definitions, etc.) 5. Architectural conventions: data access patterns, type locations, service layer responsibilities, code organization +6. PR-body / docstring drift vs the diff: reconcile the PR description and load-bearing docstrings against what the diff actually does. Common real misses: wrong migration numbers; "no migration"/"no schema change" claims contradicted by the diff; "live fix" language for behavior already shipped on main; and described fallbacks/flags/removals that are not present in the diff. Flag the mismatch even when the code is otherwise correct (skip when the PR body was QUARANTINED — see shared_prompt.txt). -For every finding, cite the exact rule text from CLAUDE.md. +For every finding, cite the exact rule text from CLAUDE.md (or the exact PR-body/docstring line for drift findings). Use Grep and Glob to verify claims. Do NOT flag issues without searching first. ``` diff --git a/plugins/code-review/tools/prompts/bha_suffix.txt b/plugins/code-review/tools/prompts/bha_suffix.txt index f8601eb1..d2a9d7e3 100644 --- a/plugins/code-review/tools/prompts/bha_suffix.txt +++ b/plugins/code-review/tools/prompts/bha_suffix.txt @@ -6,6 +6,8 @@ Focus areas: - State management: race conditions, stale closures, double-trigger patterns - Error handling: missing try-catch on async, unhandled promise rejections - Data transformations: off-by-one, incorrect parsing, wrong parameter types +- Migration prefix contiguity (CI-blocking): when the diff adds a directory under a `prisma/migrations/` (or equivalent numbered-migration) tree, list the existing sibling prefixes and compute the next contiguous index; if the added prefix skips or reuses a slot (e.g. `0026_*` added when `0025` is the next free one), a committed-migration-name guard test fails CI — flag the mismatch as BLOCKING. +- Read-only-bridge / write-path mismatch: when a change newly routes a write (POST/PATCH/DELETE, or any request carrying a body) through a transport/bridge/proxy/IPC layer, read that layer's method allowlist (e.g. an `ALLOWED_METHODS` set) and body handling before trusting it; a GET-only bridge silently turns every write into a network/status-0 error that read paths won't reveal. Flag only after confirming the method/body is not permitted. For each potential finding, complete this reasoning certificate in your before including it: diff --git a/plugins/code-review/tools/prompts/design_critic_suffix.txt b/plugins/code-review/tools/prompts/design_critic_suffix.txt index 5916af9d..2182f964 100644 --- a/plugins/code-review/tools/prompts/design_critic_suffix.txt +++ b/plugins/code-review/tools/prompts/design_critic_suffix.txt @@ -59,6 +59,7 @@ A **deep** module has a simple interface over a powerful implementation; a **sha ### Step 7 — Testability & consistency - **Humble Object:** branching/calculation/validation/formatting embedded in a hard-to-test shell (a view, JSX/React component, controller action, template, framework callback, `main()`) instead of an extractable, framework-free Presenter/ViewModel/use case. The tell: the new logic can only be exercised by rendering the component, starting the server, or hitting the DB. - **Consistency:** changed code that diverges from established sibling conventions (a different naming style, a hand-rolled solution where a shared helper exists, a new ad-hoc error type beside an established hierarchy, a parallel API with a divergent return contract, an invariant maintained on one path but dropped on a newly-added parallel one). A lone "improvement" to a convention is itself a cost. +- **Parity divergence across "same-surface" paths:** when two code paths compute the same user-facing surface (e.g. desktop-SQLite vs cloud-Postgres, or a fast-path vs a hydrate-path), diff the fields they populate and the predicates they filter on. Flag a path that zeroes/omits a field the other sums, or filters a population differently — it makes the numbers change for non-data reasons when the user flips surfaces. Cite both paths' file:line before flagging. ## Reasoning per finding (complete in before emitting) diff --git a/plugins/code-review/tools/prompts/impact_analyzer_prompt.txt b/plugins/code-review/tools/prompts/impact_analyzer_prompt.txt index b274c4b0..dab21743 100644 --- a/plugins/code-review/tools/prompts/impact_analyzer_prompt.txt +++ b/plugins/code-review/tools/prompts/impact_analyzer_prompt.txt @@ -128,6 +128,22 @@ internal to a function body. Specifically, flag: renames a column. - **Deletions**: any exported symbol whose declaration disappears. These are the highest-confidence breakages — they always break. + - **Error-handling semantics in a SHARED module**: a parser/harness/core + lib consumed by BOTH a desktop/Electron surface and a cloud surface + where the diff changes throw/catch/return semantics (e.g. swallowing an + error that previously threw). This is a `behavioral_change` — trace ALL + consumers, not just the one the PR body names, and check whether any + downstream quarantine/skip/defer/import path relied on the old behavior + (e.g. a flip from "quarantine/defer" to "import with dropped data"). + Treat "PR body says cloud-only" as a claim to verify against the + consumer list, not a fact. + - **New unconditional data dependency in a shared component**: a + previously-presentational shared component that now calls a hook + requiring a provider (API/query client, context) unconditionally. Every + existing mount site is a consumer — enumerate them (especially Storybook + stories and the global Storybook preview) and flag any that does not + supply the provider; the unconditional hook call throws on mount where + the component used to be pure. Ignore symbols that are clearly **internal** (private/unexported, underscored, inside `__init__` blocks not in `__all__`, etc.). Ignore