pm-ai-shipping: add the code-review skill (correctness / performance / security, each optional)

NOT RELEASED. No version bump - versions stay at 2.0.0 across all 9 plugins and
marketplace.json, because bumping is the release action and this ships only after
it has been tested. On a branch for the same reason.

WHY A SKILL AND NOT A FOURTH COMMAND. /security-audit-static is already mature -
sink analysis, self-refutation with attacker/victim rules, OWASP backstop, fan-out.
Rebuilding that inside something new would duplicate it. The hole in this plugin is
CORRECTNESS: there is no bug-finding review at all. So this is one skill with three
independently activated dimensions that defers to the existing command for security
and points at intended-vs-implemented for the doc-vs-code axis.

THE ANCHOR IS THE AGREEMENT, NOT THE FILE. The defects reviewers miss are rarely
visible inside one file - they are disagreements between two participants that each
read sensibly alone. Engine: map a flow, identify an obligation, inspect EVERY
participant, construct a violating execution, trace the consequence, refute, report.
Two lenses get a forced probe rather than a checklist mention: authority
reconciliation (a requested value is not an applied value) and identity correlation
(is the key unique, stable and live under overlap and reuse).

Refutation discipline is deliberately stricter than the security command's: a
correctness defect can harm only the person who triggered it and still be serious,
so the attacker/victim test does not transfer, and 'keep unless disproved' is too
permissive. Keep / Drop / Unresolved, with unresolved kept out of the findings list.

Parallelism fans out over complete flows, never over files - partitioning by file is
exactly the split that hides cross-boundary defects. Overlapping reads are allowed
and encouraged.

Coverage reports work performed in four states; zero findings is not 'not covered'.

Co-designed with GPT-6 Astra (Codex CLI). Contains no project-specific content: no
repo names, no paths, no bug identifiers, no defect text - verified by scan.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G42vsxSKL7je39AsHZ5aJm

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:<command>` or its short `/<command>` 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)

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.

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.