feat(code-review): detect 8 issue classes that slipped past review - #182
Closed
mikeangstadt wants to merge 1 commit into
Closed
feat(code-review): detect 8 issue classes that slipped past review#182mikeangstadt wants to merge 1 commit into
mikeangstadt wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
wongk
reviewed
Jul 20, 2026
| - 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. |
Collaborator
There was a problem hiding this comment.
this line is very specific to our monorepo and belongs in a CLAUDE/AGENTS.md file.
wongk
reviewed
Jul 20, 2026
| - 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. |
wongk
reviewed
Jul 20, 2026
| ### 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. |
Collaborator
There was a problem hiding this comment.
i have no idea what this is even talking about
wongk
reviewed
Jul 20, 2026
| 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 |
Collaborator
There was a problem hiding this comment.
this makes sense but the language is too specific to our repo
wongk
reviewed
Jul 20, 2026
| (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 |
Collaborator
There was a problem hiding this comment.
this is react specific, but that may be okay.
Collaborator
Author
|
Let's promote this to being applied to the repo Claude.md / agents.md -- solid call. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Minimal, prompt-only detection additions to the
code-reviewplugin, derived from real misses in a downstream review where a human "soul review" caught issues that/code-reviewdid not. Each heuristic is appended to the single most-relevant existing reviewer prompt/checklist in the plugin's existing voice/structure — no new agents, no prompt restructuring, no orchestration or tooling change. Every heuristic keeps a "verify X before flagging" clause to protect reviewer credibility against false positives.The 8 issue classes now covered
prisma/migrations/dir whose numeric prefix is off-by-one fails the committed-migration-name guard test. → Bug Hunter AALLOWED_METHODS/body handling silently rejects it. → Bug Hunter AFeature Flags
None.
Breaking changes
None. Prompt content only — no behavioral, schema, or tooling change. These prompt files are explicitly excluded from the golden state snapshots, so the change does not alter pipeline output shape.
Test plan
pytest plugins/code-review/tools/python/→ 1330 passed, 3 skipped (unchanged).bha_suffix.txt,impact_analyzer_prompt.txt,design_critic_suffix.txt,spawn-reviewers/SKILL.md,plugin.json) plus a terse CHANGELOG entry. Plugin bumped 3.7.0 → 3.8.0 (additive detection guidance).🤖 Generated with Claude Code