diff --git a/pm-ai-shipping/skills/code-review/SKILL.md b/pm-ai-shipping/skills/code-review/SKILL.md index 9298a64..5c8eb28 100644 --- a/pm-ai-shipping/skills/code-review/SKILL.md +++ b/pm-ai-shipping/skills/code-review/SKILL.md @@ -130,7 +130,7 @@ checklist will not help; each needs an explicit probe: removal and reuse. A label, a position or arrival order is suspicious exactly when those properties can fail. Probe: run two operations concurrently and complete them out of order. -The full set of twelve diagnostic lenses, each with a detection tell, is in +The full set of thirteen diagnostic lenses, each with a detection tell, is in `references/correctness-taxonomy.md`. They are overlapping lenses, not a quota to fill. ## Refutation: the discipline that makes this worth running @@ -158,6 +158,15 @@ can harm only the person who triggered it and still be serious.** Equally, "keep too permissive here — an ungrounded suspicion with no constructed execution is not a finding. When both sub-cases are active, apply each test only to its own dimension. +**Absorption is not prevention.** The most expensive refutation mistake is finding something +downstream that happens to hide the defect - a cache that usually holds the value, a retry that +usually succeeds, a default that is usually right - and dropping the finding. That is not a +guarantee, it is a coincidence with good odds, and it fails the day the absorber is cold, evicted or +reconfigured. Drop only on a mechanism that makes the execution *impossible*, and say which mechanism +it was. For the same reason, **"it works nearly always" describes a race, not a refutation** - a +timing window that usually resolves correctly is a finding, and the fact that you had to reason about +which side usually wins is the evidence. + Passing tests, unfamiliar code, a suspicious name, a missing test and a sibling difference are evidence to investigate — none of them is proof, and none is refutation. Deduplicate by violated agreement and root cause, never by file. There is no findings quota; zero supported findings is a @@ -187,6 +196,21 @@ drops to a cheaper model or a lower effort is the cheapest way to lose one. If a or downgraded, say which in the report — a reader who assumes one model saw everything will misjudge the coverage. +**One model, unless told otherwise.** Fan-out here is for coverage, not for a second opinion: every +worker runs the same model as the coordinator. Do not bring in a second model to cross-check findings +unless explicitly asked. Mixing models makes the result unattributable — when this skill is being +measured, or compared across models, a single foreign worker invalidates the number. The independent +second-model pass is a separate, explicitly-invoked step (`/ship-check` Step 6), never something this +skill reaches for on its own. + +**Give workers read-only tools.** A review worker needs to read, search and navigate — nothing more. +Withhold file writes, edits and any mutating command; allow read, search and non-mutating inspection. +Three reasons, in order of importance: a worker that can edit will drift from reviewing into +"helpfully" fixing and stop reporting what it silently repaired; the repository under review must end +the run byte-identical to how it started, or the findings cannot be checked against it; and a +read-only worker cannot damage a working tree it misunderstood. If the host cannot restrict tools, +say so in the prompt and verify the tree is unchanged when the run ends. + ## Report Lead with supported findings, ordered by impact. Keep severity separate from evidential strength. diff --git a/pm-ai-shipping/skills/code-review/references/correctness-taxonomy.md b/pm-ai-shipping/skills/code-review/references/correctness-taxonomy.md index e8621c5..0e5a3c1 100644 --- a/pm-ai-shipping/skills/code-review/references/correctness-taxonomy.md +++ b/pm-ai-shipping/skills/code-review/references/correctness-taxonomy.md @@ -1,4 +1,4 @@ -# Correctness taxonomy — twelve lenses, with detection tells +# Correctness taxonomy — thirteen lenses, with detection tells *Reference for the correctness sub-case — the core of the `code-review` skill. The performance and security sub-cases have their own files alongside this one.* @@ -69,9 +69,35 @@ split items, combined items, partial writes and early termination. Inspect buffe finalisation — especially the last item. ### 10. Representation and information loss -Compare the values a producer can emit against the distinctions a consumer relies on: absent versus -empty, zero versus missing, signed versus unsigned, canonical forms, precision, encoding, equality -semantics. Trace round-trips and derived outputs for distinctions that disappear. +Compare the values a producer can emit against the distinctions a consumer relies on. Trace +round-trips and derived outputs for distinctions that disappear. **This is the highest-frequency +class in every corpus examined** - planted and natural - so give it more than one pass. + +Three sub-shapes carry most of it: + +- **The nullish family.** *Pending*, *absent*, *empty*, *zero*, *false* and *failed* are six different + states that collapse into each other with alarming ease. A failed read returning an empty result, a + never-started resource reported as broken, a guard that excludes `null` but not `undefined`, an + explicit null skipped as though the field were missing, a string `"false"` landing in a boolean, a + sentinel like `-1` standing in for "no answer" and then being counted. **Probe:** for each value + that can be missing, enumerate which of the six it can actually be, and check the consumer + distinguishes the ones that matter. +- **Projection and field-set drift.** A producer - a query, a DTO, a serialiser, a mapper - stops + emitting a field, and consumers degrade silently rather than failing. **Probe:** diff the field set + a producer actually selects against every field its consumers read, including nested projections + and the fields a *renderer* touches. Also check for a field read under a name the producer never + emits. +- **Unresolved values stored as resolved ones.** A promise, a future, a lazy handle or a + still-loading state persisted or compared as though it were the settled value. **Probe:** anywhere a + value has a "not ready yet" state, find who reads it without checking. + +Then the ordinary axes: signed versus unsigned, canonical forms, precision, encoding, equality +semantics. + +**Tell:** a cast, an `any`, a non-null assertion or a suppressed warning at a boundary is where +contracts go to die - the annotation exists precisely because the two sides disagreed and someone +silenced the compiler rather than reconciling them. Treat every one of them on a boundary as a +candidate. ### 11. Ownership, completion and progress Trace who may mutate, release, cancel and complete an operation or resource. Inspect exceptional @@ -81,7 +107,30 @@ ownership changed hands. ### 12. Decisions and dispatch Enumerate the meaningful states and inputs for consequential predicates and dispatch tables. Compare branches against supported expectations: overlapping conditions, inverted tests, missing cases, and -what the fallback actually does. +what the fallback actually does. Check the *default* a framework or language supplies when the code +specifies nothing - an unstated default is still a decision. + +**Include reachability, not just correctness.** Ask of each consequential branch whether any input +can reach it, and of each scheduled or registered thing whether anything actually starts it. A +predicate that can never be true, a handler never wired up, a writer whose output can never reach +disk, and a job whose scheduler is never started are all defects that read as perfectly correct code. +They are common in the wild and almost absent from planted corpora, so no checklist trained on +planted bugs will prompt you to look. + +### 13. Verification and observability +Follow what happens when each step *fails*, and ask what would make the failure visible. Look for +checks that cannot fail, oracles that measure something other than the thing they claim to, +exceptions swallowed into a success path, gates that skip their subject, and effects whose absence +nothing would detect. A step that always passes is not a passing step. + +**Probe:** for each guarantee the system claims, name the observation that would break if it stopped +holding. If there is none, the guarantee is decorative. + +> This lens exists because of a gap in the evidence, and the gap is worth stating. A *planted* defect +> is detectable by construction - somebody planted it, so somebody can find it. Silent failure is +> therefore systematically absent from planted corpora and heavily represented in real fix histories, +> where "and nothing noticed" is a recurring phrase. Do not let a benchmark-shaped checklist talk you +> out of looking here. ---