diff --git a/pm-ai-shipping/README.md b/pm-ai-shipping/README.md index 4c3b97b..eb288db 100644 --- a/pm-ai-shipping/README.md +++ b/pm-ai-shipping/README.md @@ -12,9 +12,10 @@ Start with `/ship-check` for the full sequence, or run a single stage with the s Install from the [pm-skills marketplace](https://github.com/phuryn/pm-skills) and enable the `pm-ai-shipping` plugin. Each command can be triggered with `/pm-ai-shipping:` or its short `/` form; skills auto-load when the topic matches. -## Skills (2) +## Skills (3) - **shipping-artifacts** — The durable documentation set that makes an AI-built app reviewable: a core every app needs (architecture, user/permission flows, permissions, variables/secrets, test-coverage map) plus conditional docs added only when they apply (emails, cron, SEO, embedded agents/automation). Defines what each doc must capture and how a reviewer uses it. +- **code-review** — Review code for actionable correctness, performance, or security defects, each dimension independently optional. Anchors on agreements between participants across a boundary — the defects that are invisible file-by-file because each side looks reasonable alone — forces a violating execution, and refutes every candidate before reporting. - **intended-vs-implemented** — The method for finding the gap between what a system is documented to do and what the code actually does, with cited evidence on both sides and without hand-wavy findings. ## Commands (5) diff --git a/pm-ai-shipping/skills/code-review/SKILL.md b/pm-ai-shipping/skills/code-review/SKILL.md new file mode 100644 index 0000000..7b90c52 --- /dev/null +++ b/pm-ai-shipping/skills/code-review/SKILL.md @@ -0,0 +1,210 @@ +--- +name: code-review +description: "Review code for actionable correctness, performance, or security defects — each dimension independently optional. Anchors on agreements between participants across a boundary, forces a violating execution, and refutes every candidate before reporting. Use when asked to review changes, find bugs, audit a codebase, or check whether a fix is safe." +--- + +# Code Review + +## Purpose + +Most review output is noise: a list of things that *look* wrong, unranked, unrefuted, and impossible +to act on. This skill produces the opposite — a small number of findings, each with a required +behaviour, a feasible trigger, a concrete contradiction, an observable consequence, and the strongest +counterargument already checked. + +Its central bet: **the defects reviewers miss are rarely visible inside one file.** They are +disagreements between two participants that each look reasonable alone — a caller and a callee, a +producer and a consumer, a writer and a later reader, two branches that should establish the same +state. A checklist applied file-by-file cannot see those, because the two halves are never in view at +the same time. So the unit of review here is the **agreement**, not the file. + +## Invocation + +``` +/pm-ai-shipping:code-review +/pm-ai-shipping:code-review dimensions=correctness scope=changes +/pm-ai-shipping:code-review dimensions=performance,security +/pm-ai-shipping:code-review dimensions=all +``` + +Claude Code ships its own bundled `/code-review`. Use the plugin-qualified form above when you mean +this one. + +These are instruction arguments, not shell flags. + +- **Default: `correctness`.** It is the dimension generic tooling covers worst. +- An explicit list selects exactly those dimensions; `all` selects three. "Find bugs" means + correctness. Never silently reinterpret an unknown or empty selection — ask. +- **Scope:** use what was asked. Otherwise review working changes if present, else the repository. +- **State the selected dimensions, the scope and the comparison baseline before investigating.** +- Reviewing changes means following dependencies *beyond* the changed lines, and distinguishing + defects the change **introduced** from defects it merely **revealed**. +- Review and report. Apply fixes only when asked. + +## Shared engine + +Every dimension uses one skeleton. Only the anchor and the refutation rules differ. + +**Map a flow → identify an obligation → inspect every participant → construct a violating execution +→ trace the consequence → attempt refutation → report.** + +Build one minimal map first: inputs, major execution flows, who owns which state, external +dependencies, observable effects. Each selected dimension enriches it — do not build three maps, and +do not make a security-only run wait on correctness mapping. + +| Dimension | Anchor | +|---|---| +| **Correctness** | Agreements between participants across a boundary | +| **Performance** | Workload → resource demand → growth or contention → material consequence | +| **Security** | Trust boundaries and sinks (see `/security-audit-static`, which owns the specialised procedure and its attacker/victim refutation rules) | + +Dimensions are independently *activated*, not mutually exclusive. One root cause can carry +correctness and security impact — report it once, with both impacts. + +## Correctness: the agreement engine + +A *boundary* is semantic, not a file split. It separates a caller and a callee, two callbacks, two +executions of the same function, a producer and a consumer, or a value written now and read later. + +For each consequential agreement, hold these in working notes — not in the report: + +``` +Participants: +Value, entity or effect exchanged: +Authority (who decides the real answer): +Identity and lifetime/version: +Required relationship: +Evidence for that relationship: +Relevant transitions or orderings: +Observable consumer or consequence: +``` + +**Establish the obligation without inventing intent.** Evidence comes from specifications, +documented contracts, language or protocol semantics, tests that encode an expectation, or a +necessary producer/consumer relationship. A consumer's implementation alone does not prove the +consumer is right. Where participants disagree, say why the disagreement produces a *wrong outcome* — +sometimes the contradiction is certain while which side should change is genuinely open. Missing +documentation is a limitation, not automatically a finding. + +**Start where agreements are most likely to break:** values transformed or negotiated, identities +reassigned, work becoming asynchronous, state persisted and reloaded, several effects that must +agree. Then do a local pass over ordinary decisions, arithmetic, boundaries and error branches — the +anchor must not become a filter that discards plain bugs. + +### Force a violating execution + +A suspicion is not a finding until you construct the execution that breaks it. Where the +implementation permits: + +- make a **requested** value differ from the **accepted or effective** one; +- keep two operations live at once and vary their completion order; +- change the relevant identity or generation between observation and use; +- compare distinct transitions that should end in equivalent state; +- inject failure between effects, and interruption before completion; +- exercise empty, exact-boundary and adjacent-boundary inputs. + +Establish that each case is actually reachable. Do not assume it. + +### Two lenses that need a forced probe, not a mention + +Across a large evaluation of planted runtime defects in real codebases, two classes were almost never +*even reported* by strong agents — not missed at the fix, missed at the look. Naming them in a +checklist will not help; each needs an explicit probe: + +1. **Authority reconciliation.** Follow a proposed value through validation, normalisation, + negotiation or commit, and find downstream state still derived from the **proposal** where the + authority can return something different. *A requested value is not an applied value.* Probe: + force them apart and ask what still reads the request. +2. **Identity and correlation.** Trace how an operation's result finds its originating entity, then + establish that the key is unique, stable and live for long enough — under overlap, reordering, + 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 +`references/correctness-taxonomy.md`. They are overlapping lenses, not a quota to fill. + +## Refutation: the discipline that makes this worth running + +A candidate becomes a finding only with all five: + +1. **A supported obligation** — what must hold, and on what evidence. +2. **A feasible execution** — inputs, state and ordering the real system permits. +3. **A concrete contradiction** — where the obligation fails. +4. **An observable consequence** — wrong output, state, effect, completion or progress. +5. **An examined counterargument** — the strongest mechanism that would prevent or repair it. + +Actively hunt for the refutation: an enclosing guarantee that makes the execution impossible; +synchronisation excluding the interleaving; reconciliation before any consequential read; an +intentional contract; a precondition excluding the input; a different owner responsible for it. + +| Outcome | Rule | +|---|---| +| **Keep** | Evidence establishes the defect; the counterargument checked does not prevent it. | +| **Drop** | Cited evidence defeats the execution, the obligation or the consequence. | +| **Unresolved** | An essential contract or runtime fact is unknown. List it *separately from findings*. | + +Do not import the security dimension's attacker/victim test. **A correctness defect can harm only the +person who triggered it and still be serious.** Equally, "keep unless disproved" is too permissive +here — an ungrounded suspicion with no constructed execution is not a finding. + +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 +valid result. + +## Parallelism + +Fan out over **complete flows or connected groups of agreements** — never over files, and never one +agent per taxonomy class. Partitioning by file is precisely the split that hides cross-boundary +defects, which are the ones worth finding. + +1. The coordinator builds the initial map and identifies shared state. +2. Each worker gets a bounded flow, its participants, the selected dimensions and open questions. +3. Workers inspect **both sides** of their agreements and may follow dependencies outside their list. +4. Workers return candidates, cited evidence, completed refutations and unresolved relationships. +5. The coordinator reconciles assumptions and any relationship that crosses assignments. +6. Strong candidates get a separate verification pass before they are reported. + +**Allow overlapping reads.** Two workers reading the same authority is far cheaper than either one +holding half its contract. Keep integration capacity in reserve: an unresolved relationship spanning +two assignments stays unexamined until someone closes it. One level of fan-out is the target; if +delegation is unavailable or the scope is small, run the same procedure sequentially. + +## Report + +Lead with supported findings, ordered by impact. Keep severity separate from evidential strength. + +``` +Review scope: +Comparison baseline: +Selected dimensions: + +[Severity] [Dimension] Concrete consequence + Expectation: required behaviour, and the evidence for it + Trigger: feasible preconditions and execution + Defect: the violated relationship + Evidence: source locations for EVERY participant + Impact: observable consequence and affected scope + Refutation: strongest counterargument checked, and why it fails + Remedy: minimal correction to the violated relationship + Verification: what was executed, versus established from source + +Coverage: +Unexamined areas and essential unknowns: +``` + +Cite **both** participants for a cross-boundary defect, and do not group findings only by file — that +hides the relationship the review exists to find. + +**Coverage means work performed, not boxes ticked.** For each selected dimension report: examined +with supported findings · examined, none supported · not applicable, with reason · not examined, with +reason. Zero findings in a category does **not** mean "not covered", and a table of ticks is not +evidence of completeness. Say "no supported findings in the examined scope" — never that the code is +bug-free. + +## Notes + +- Say explicitly what is well built. A review that only accuses is easy to dismiss. +- For trust boundaries, sinks and OWASP coverage use `/security-audit-static`; for the doc-vs-code + axis use the `intended-vs-implemented` skill. This skill does not restate either. +- A static review produces code-review findings, not confirmed exploits or measured regressions. diff --git a/pm-ai-shipping/skills/code-review/references/correctness-taxonomy.md b/pm-ai-shipping/skills/code-review/references/correctness-taxonomy.md new file mode 100644 index 0000000..ab3a8ee --- /dev/null +++ b/pm-ai-shipping/skills/code-review/references/correctness-taxonomy.md @@ -0,0 +1,92 @@ +# Correctness taxonomy — twelve lenses, with detection tells + +Overlapping diagnostic lenses, not a classification scheme and not a quota. Each entry says **how you +detect it**, because a class name alone changes nothing about what a reviewer looks at. + +Lenses 1 and 2 are the ones strong reviewers — human and machine — miss most often, and the only two +that warrant an explicit forced probe rather than a read-through. Both are failures of *looking*, not +of judgement: the code reads sensibly at each participant, and the defect exists only in the +relationship between them. + +--- + +### 1. Authority reconciliation +Follow a proposed value through validation, normalisation, negotiation or commit. Identify downstream +state still derived from the **proposal** when the authority can legitimately return something +different. Check that reconciliation happens *before* the first consequential use. + +> **A requested value is not an applied value.** Anywhere a system can say "I heard you, and here is +> what I actually did", the reply is the authority — and the request is not. + +**Probe:** construct an execution where the effective answer differs from the requested one, then ask +which stored state, which UI, which later call and which log line still carry the request. + +### 2. Identity and correlation +Trace how an operation's result finds its originating entity. Establish the correlation key's +**uniqueness, stability and lifetime** under overlapping operations, reordering, removal and reuse. A +label, an index, a position or arrival order is suspicious exactly when one of those can fail. + +**Probe:** run two operations concurrently and complete them out of order. Then remove one mid-flight +and reuse its slot. + +### 3. Freshness and generations +Mark every value captured before a yield, await, callback, timer or lock release. Determine what can +change before it is used, and whether the operation still targets the intended entity *and version*. +Check what the code actually does with a stale result — ignore, apply, or apply silently. + +### 4. Lifecycle and derived state +Compare every reachable construction, replacement, restoration, reset, failure and termination path. +Look for derived fields or cached decisions correctly re-established on one path and wrongly retained +on another. Two paths that should end in equivalent state are an agreement like any other. + +### 5. Atomicity and partial failure +Split multi-effect operations at each failure and cancellation point. Is partial state permitted, +recoverable and accurately reported? Look for success reported before the required effects are +durable. + +### 6. Replay and effect cardinality +Follow retries, duplicate delivery, repeated callbacks and re-entry into effects. Compare the actual +delivery guarantee against the required effect count — especially where an effect costs money, sends +a message or mutates a shared total. Inspect deduplication scope, lifetime, and behaviour after +partial success. + +### 7. Composition and precedence +Trace independently produced pieces through merge, reduction, ordering and dispatch. Compare the real +overwrite and selection rules against the intended authority or priority — including transformations +applied *after* the merge that quietly re-order or re-key it. + +### 8. Bounds, units and accounting +Follow counts, lengths, offsets, capacities and totals through every transformation. Check empty and +boundary cases, overflow, rounding, and whether measurement and consumption use the same unit and +representation. + +### 9. Framing and incremental processing +Compare logical item boundaries against actual read, write, iterator and callback boundaries. Test +split items, combined items, partial writes and early termination. Inspect buffering, flush and +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. + +### 11. Ownership, completion and progress +Trace who may mutate, release, cancel and complete an operation or resource. Inspect exceptional +exits and competing terminal paths for premature release, missing completion, deadlock, or use after +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. + +--- + +## Using these + +- They overlap on purpose. One defect can be lens 1 and lens 3 at once; report the root cause once. +- Absence of findings under a lens is a valid result. Do not manufacture one to fill the table. +- Finding nothing under lenses 1 and 2 is worth a second look *only* if you never constructed their + probes — a read-through reliably returns nothing here, which is exactly the failure mode. +- None of these is language- or framework-specific. If a lens seems inapplicable, say which property + of the system makes it so.