pm-ai-shipping: code-review becomes the top-level skill; perf + security are sub-cases

Restructure, per the ask that code review be the parent and the other two dimensions its sub-cases: - SKILL.md gains a "one engine, three anchors" section. Correctness is the core and stays inline; performance and security move to their own reference files, loaded only when selected. - references/performance-review.md (new) — a universal, stack-agnostic core (repeated work, growth relationships, retention, copying, contention, amplification) plus the three-part bar for a performance finding. Defers the database/web checklist to /performance-audit-static instead of restating it. - references/security-review.md (new) — trust boundaries and sinks for code with no web surface, and the one rule that INVERTS relative to correctness: attacker-equals-victim refutes a security finding but never a correctness one. Defers the full procedure to /security-audit-static. - Both audit commands now say they are the specialisation behind their sub-case, so the narrow entry points still lead back to the skill. ship-check gains two stages it was missing: - Step 3, correctness review — the pass neither audit performs: logic and state defects that compile clean and pass the suite. - Step 6, independent unsteered review — a fresh session of a second model (Codex or equivalent), given no checklist and no prior findings, with the subject computed from a diff rather than described. Every finding is hand-verified against the code before it enters the packet, since an unsteered reviewer carries no refutation discipline of its own. The packet reports whether it ran clean or did not run at all - those are different signals. Also carries the working-tree edits already in progress: model-and-orchestration guidance on both audits, the OWASP A02/A06/A09 backstop, CSP in the output-encoding bullet, the prompt-injection/agent-abuse bullet, and the Audit Provenance section (now also naming the second model). No version bump - not tested against the benchmark yet. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01G42vsxSKL7je39AsHZ5aJm
2026-09-13 15:15:57 +02:00
parent 2e662ac04d
commit 18032bc9f7
9 changed files with 230 additions and 37 deletions
@@ -1,6 +1,6 @@
 ---
 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."
+description: "Review code for actionable defects. Correctness is the core; performance and security are optional sub-cases of the same engine. 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
@@ -18,6 +18,25 @@ producer and a consumer, a writer and a later reader, two branches that should e
 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.

+## Structure: one engine, three anchors
+
+Code review is the skill. **Correctness is its core** — the dimension generic tooling covers worst,
+and the one described in full below. **Performance and security are sub-cases**: the same engine, the
+same refutation discipline, the same report contract, with a different anchor and one or two extra
+rules each.
+
+| Sub-case | Anchor | Where its rules live |
+|---|---|---|
+| **Correctness** *(core, default)* | Agreements between participants across a boundary | This file + `references/correctness-taxonomy.md` |
+| **Performance** | Workload → resource demand → growth or contention → consequence | `references/performance-review.md` |
+| **Security** | Source → trust boundary → sink, with an attacker controlling the source | `references/security-review.md` |
+
+Read a sub-case's file only when that sub-case is selected. Each is short on purpose: it states what
+*differs*, and the rest of this file still applies.
+
+Sub-cases are independently *activated*, not mutually exclusive. One root cause can carry correctness
+and security impact — report it once, with both impacts.
+
 ## Invocation

 ```
@@ -32,9 +51,9 @@ 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.
+- **Default: `correctness`.** Bare "review this" or "find bugs" means correctness only.
+- An explicit list selects exactly those sub-cases; `all` selects three. 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
@@ -43,24 +62,15 @@ These are instruction arguments, not shell flags.

 ## Shared engine

-Every dimension uses one skeleton. Only the anchor and the refutation rules differ.
+Every sub-case 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
+dependencies, observable effects. Each selected sub-case 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
@@ -143,9 +153,10 @@ intentional contract; a precondition excluding the input; a different owner resp
 | **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.
+Do not import the security sub-case's attacker/victim test into correctness. **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. When
+both sub-cases are active, apply each test only to its own dimension.

 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
@@ -159,7 +170,7 @@ agent per taxonomy class. Partitioning by file is precisely the split that hides
 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.
+2. Each worker gets a bounded flow, its participants, the selected sub-cases 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.
@@ -170,6 +181,12 @@ holding half its contract. Keep integration capacity in reserve: an unresolved r
 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.

+**Run workers on the strongest model available, and match the current session's effort level.** This
+is recall-first work: a missed cross-boundary flow is the costly failure, and a worker that silently
+drops to a cheaper model or a lower effort is the cheapest way to lose one. If any worker is rerouted
+or downgraded, say which in the report — a reader who assumes one model saw everything will
+misjudge the coverage.
+
 ## Report

 Lead with supported findings, ordered by impact. Keep severity separate from evidential strength.
@@ -196,8 +213,8 @@ 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
+**Coverage means work performed, not boxes ticked.** For each selected sub-case 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.
@@ -205,6 +222,9 @@ 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.
+- The two sub-cases have mature commands behind them: `/security-audit-static` (trust boundaries,
+  sinks, OWASP backstop) and `/performance-audit-static` (over-fetching, indexes, caching). Run the
+  command when the sub-case is the whole job; use the reference file when it is one dimension of a
+  broader review. This skill does not restate either.
+- For the doc-vs-code axis use the `intended-vs-implemented` skill.
 - A static review produces code-review findings, not confirmed exploits or measured regressions.
@@ -1,5 +1,8 @@
 # Correctness taxonomy — twelve 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.*
+
 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.

@@ -0,0 +1,55 @@
+# Sub-case: performance review
+
+A specialisation of the parent engine. The anchor changes; the refutation discipline and the report
+contract do not.
+
+**Anchor:** workload → resource demand → growth or contention → material consequence.
+
+The agreement being tested is between what the code *assumes about its workload* and what the
+workload *will actually be*. Code written against seed data agrees with a world that will not exist
+in production. That is the same shape as any other broken agreement: two participants, each
+reasonable alone.
+
+## The universal core
+
+Language- and stack-agnostic. Apply before any technology-specific checklist.
+
+- **Repeated work** — scans, parsing, serialisation, allocation, initialisation or I/O performed
+  again where a single pass, a hoist or a reuse would do.
+- **Growth relationships** — how does resource use scale with input size, with concurrency, and with
+  elapsed time? Superlinear growth in any of the three is the finding; the constant factor is not.
+- **Retention** — queues, buffers, caches and collections that grow without a bound, an eviction
+  policy or backpressure. Unbounded retention is a failure with a delay on it.
+- **Copying and conversion** — data copied or converted between representations on a hot path,
+  especially at a boundary where both sides could have agreed on one representation.
+- **Serialisation and contention** — lock duration and scope, single-threaded chokepoints,
+  head-of-line blocking, and work held inside a critical section that did not need to be.
+- **Amplification** — retries, polling, fan-out and cache misses that multiply one logical request
+  into many real ones. Check the multiplier under failure, not under success.
+
+## Technology specialisations
+
+Apply only where the underlying technology exists — do not report the absence of a database concept
+in a program that has no database. For data-backed applications (over-fetching, `SELECT *`, missing
+pagination, index definitions, caching layers), `/performance-audit-static` holds the detailed
+checklist; use it rather than restating it here.
+
+## What makes a performance finding
+
+All three, or it is not a finding:
+
+1. **A reachable workload** — the input size, rate or concurrency is one the system will actually
+   meet, established from the code and its context rather than assumed.
+2. **A resource cost or growth relationship** — what is consumed, and how it scales.
+3. **A material consequence** — latency a user feels, a cost that is paid, a limit that is hit, or a
+   failure that results.
+
+## Refutation
+
+Refute against real bounds, amortisation, reuse, actual call frequency, and deliberate trade-offs. A
+nested loop over a collection with a hard bound of four is not a finding. A missing cache in code
+called once at startup is not a finding.
+
+**Distinguish measurement from static deduction, and label which you did.** Never invent a timing.
+Never report absent caching, a nested loop, or a missing index as a finding on its own — without a
+workload, those are observations, not defects.
@@ -0,0 +1,68 @@
+# Sub-case: security review
+
+A specialisation of the parent engine. The anchor changes, and one refutation rule is **inverted**
+relative to correctness — read that section before running this sub-case alongside another.
+
+**Anchor:** source → trust boundary → sink, with an attacker who controls the source.
+
+The agreement being tested is between what a component *trusts* and what an attacker can *supply*.
+Where correctness asks "can this happen", security asks "can someone make this happen on purpose" —
+and an adversary will construct the unlikely execution deliberately.
+
+## Where the procedure lives
+
+`/security-audit-static` owns the full specialised procedure — entry-point mapping, the four
+high-value paths, the keep/drop rule with its attacker-and-victim test, the OWASP Top 10 coverage
+backstop, and the high-miss checklist. **Run it rather than restating it.** This file exists to say
+what changes when security is selected as a dimension of a code review, and to supply the part of the
+engine that survives when the application has no web surface at all.
+
+## The universal core
+
+Applies to a CLI, a library, a daemon, a build tool — anything without an HTTP handler in sight.
+
+- **Trust boundaries** — every point where data crosses from a less-trusted origin into a
+  more-trusted context: arguments, environment, config files, stdin, filenames, archive members,
+  network responses, plugin and extension surfaces, deserialised state, and model output.
+- **Sinks** — where a value becomes an instruction rather than data: process execution, dynamic
+  evaluation, query construction, path resolution, template rendering, deserialisation, outbound
+  requests, permission and role writes, and logging.
+- **Injection by representation confusion** — a value interpreted in the syntax of the sink rather
+  than as an opaque datum. Encode for the *sink*, not at the input. This is the same disagreement as
+  correctness lens 10 (representation and information loss), with an adversary steering it.
+- **Validator/consumer differentials** — the check and the use disagree about what the value means:
+  unanchored patterns, prefix allowlists, normalisation applied on one side only, validation on one
+  representation and execution on another.
+- **Fail-open paths** — error, timeout, cancellation, cache-miss and boundary branches that default
+  to *allow*. Correctness lens 12 finds these; security decides what they cost.
+- **Secrets and sensitive data in transit to the wrong place** — logs, traces, error bodies,
+  temporary files, crash dumps, and anything an unprivileged local user can read.
+- **Privilege and identity** — which principal an operation runs as, whether the check and the action
+  name the same object, and what happens when they do not.
+
+## The inverted refutation rule
+
+Under correctness, a defect that harms only the person who triggered it is still a defect. Under
+security it usually is **not** a finding: if the only victim is the attacker, on their own machine,
+account, tenant or data, and no shared system or privilege boundary is crossed, drop it.
+
+The carve-outs where that refutation is **forbidden** — outbound-network sinks, shared billing or
+quota, data exposure, cross-tenant or cross-principal flows, and server-side execution or rendering —
+are listed in `/security-audit-static`. Use its list; do not reinvent one.
+
+**Do not let the two rules leak into each other.** Running both dimensions in one review, keep the
+tests separate per finding: a defect dropped as a security finding may still be a correctness finding
+with a real consequence, and should be reported as one.
+
+## What makes a security finding
+
+The parent skill's five requirements, with the trigger read adversarially:
+
+1. A supported obligation — the trust assumption, and what establishes it.
+2. A feasible execution — **including who the attacker is and what they control.**
+3. A concrete contradiction — the boundary that fails to hold.
+4. An observable consequence — **naming the victim**, who must not be only the attacker.
+5. An examined counterargument — a real check at the sink, an unreachable path, an upstream
+   validator, or a non-dangerous sink.
+
+Findings are code-review results, not confirmed exploits. Say so.