diff --git a/pm-ai-shipping/.claude-plugin/plugin.json b/pm-ai-shipping/.claude-plugin/plugin.json index fb213e2..30896d5 100644 --- a/pm-ai-shipping/.claude-plugin/plugin.json +++ b/pm-ai-shipping/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "pm-ai-shipping", "version": "2.0.0", - "description": "AI Shipping Kit — for PMs and founders accountable for AI-built code. Document a vibe-coded app, audit it for intended-vs-implemented security gaps and performance issues, and produce a reviewer-ready shipping packet.", + "description": "AI Shipping Kit — for PMs and founders accountable for AI-built code. Document a vibe-coded app, review it for correctness, security and performance defects, and produce a reviewer-ready shipping packet.", "author": { "name": "Paweł Huryn", "email": "pawel@productcompass.pm", diff --git a/pm-ai-shipping/README.md b/pm-ai-shipping/README.md index eb288db..9a7477a 100644 --- a/pm-ai-shipping/README.md +++ b/pm-ai-shipping/README.md @@ -1,6 +1,6 @@ # pm-ai-shipping — AI Shipping Kit -For PMs and founders accountable for AI-built code. Document a vibe-coded app, audit it for intended-vs-implemented security gaps and performance issues, and produce a reviewer-ready shipping packet. +For PMs and founders accountable for AI-built code. Document a vibe-coded app, review it for correctness, security and performance defects, and produce a reviewer-ready shipping packet. ## Overview @@ -15,12 +15,12 @@ Install from the [pm-skills marketplace](https://github.com/phuryn/pm-skills) an ## 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. +- **code-review** — The top-level review skill. Correctness is its core; **performance and security are optional sub-cases** of the same engine. 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:ship-check` — Turn a vibe-coded repo into a reviewer-ready shipping packet: document, wire agent context, run security and performance audits, map test coverage, and compile the results. +- `/pm-ai-shipping:ship-check` — Turn a vibe-coded repo into a reviewer-ready shipping packet: document, wire agent context, run correctness, security and performance reviews, add an independent unsteered pass by a second model, map test coverage, and compile the results. - `/pm-ai-shipping:document-app` — Reverse-engineer a codebase into the system documents reviewers and auditors need — a core set (architecture, flows, permissions, variables) plus conditional docs (emails, cron, SEO, automation) when they apply. - `/pm-ai-shipping:derive-tests` — Turn documented intent into a test-coverage map: inventory the tests that exist today, separate them from proposed tests and unverified gaps, mark each unit / guarded-live / manual, and recommend a green-before-merge CI gate. - `/pm-ai-shipping:security-audit-static` — Static security audit: map trust boundaries, cross-reference documented intent, self-refute every finding, and report only evidence-backed risks. diff --git a/pm-ai-shipping/commands/performance-audit-static.md b/pm-ai-shipping/commands/performance-audit-static.md index 6ef86a3..1f6ed17 100644 --- a/pm-ai-shipping/commands/performance-audit-static.md +++ b/pm-ai-shipping/commands/performance-audit-static.md @@ -20,6 +20,12 @@ This is a static review of code and queries, not a load test. Audit **$ARGUMENTS**. If empty, review the whole repository, prioritizing list and dashboard views, frequently hit endpoints, and large tables. +## Model and orchestration + +- **Run every subagent on the strongest model available** — Fable or Mythos when you have access, otherwise Opus 4.8. Match the **effort level of the current session** when the surface exposes it. +- **Flat fan-out for large scopes.** For a big repo, fan out with parallel subagents — one per view/route/table cluster running the three checks below — then rank the merged findings yourself. One level is the target; nest a second only when a cluster is too big for one agent's context. Don't reach for a self-generating workflow. +- **Reroutes are unlikely here, but report them if they happen.** Unlike the security audit, performance work rarely trips Fable's safety classifiers. If a cluster does get rerouted to Opus 4.8, note it in the report so the reader knows the model mix. + ## The audit ### 1. Over-fetch in view payloads @@ -56,4 +62,5 @@ End with what's already efficient (say it explicitly) and what needs runtime pro - Rank by impact-per-effort — one missing index on a hot table usually beats ten micro-optimizations. - Don't flag theoretical inefficiency with no growth path; flag what breaks as rows or traffic scale. - This command covers performance only. For authorization, injection, and data-exposure risks, use `/security-audit-static`. +- This is the data-backed-application specialisation of the **code-review** skill's performance sub-case. For logic and state defects, or for a review across several dimensions at once, use `/pm-ai-shipping:code-review`. - For an end-to-end pass with documentation and a shipping packet, use `/ship-check`. diff --git a/pm-ai-shipping/commands/security-audit-static.md b/pm-ai-shipping/commands/security-audit-static.md index dc40d9b..b972e82 100644 --- a/pm-ai-shipping/commands/security-audit-static.md +++ b/pm-ai-shipping/commands/security-audit-static.md @@ -23,6 +23,12 @@ This is a review, not a guarantee: it produces code-review findings, not confirm Audit **$ARGUMENTS**. If empty, audit the whole repository, prioritizing request handlers, auth, data access, background jobs, and anything that renders, fetches, executes, logs, or stores user-controlled data. For non-trivial scopes, fan out with parallel subagents — one per function/module cluster, each running the mapping and inspection (steps 1–3); then merge candidates and run the self-refute (step 4) yourself over the full set. +## Model and orchestration + +- **Run every subagent on the strongest model available** — Fable or Mythos when you have access, otherwise Opus 4.8. Match the **effort level of the current session** when the surface exposes it. This is recall-first work: a missed cross-file flow is the costly failure, so don't let a cluster silently drop to a cheaper model or a lower effort. +- **Expect reroutes, and report them.** A security audit is exactly the content Fable's safety classifiers screen for, so some subagents will be **automatically rerouted to Opus 4.8**. That is fine for this work — but say so. Note in the report which clusters ran on the fallback model, so the reader knows the audit's model mix instead of assuming one model saw everything. +- **Flat fan-out, not a workflow.** One level of parallel subagents (parent → cluster auditors → merge) is the target. Nest a second level **only** when a single cluster is too big for one agent's context. A deep org chart or a self-generating workflow adds coordination cost without improving recall here. + ## The audit (small engine, strong constraint) ### 1. Map entry points to trust boundaries and sinks @@ -45,7 +51,9 @@ For each finding, try to disprove it. Default to **keep** unless you find cited Name the **attacker** and the **victim**: refute if the only victim is the attacker on their own machine/account/tenant/data and no shared system or privilege boundary is crossed; keep if the impact reaches other users, tenants, shared infrastructure, billing, email reputation, secrets, or compliance-sensitive data. **Never apply attacker-equals-victim refutation to SSRF/outbound-network sinks, shared billing or quota sinks, data-exposure findings, cross-tenant or cross-principal flows, or server-side execution/rendering** — those harm someone other than the attacker by definition. Never refute a finding merely because the code is pre-existing — pre-existing bugs are the point. Do not speculate. -### 5. Report only what survives +### 5. Report only what survives — with an OWASP Top 10 backstop + +Before writing the report, map every surviving finding to its OWASP Top 10 category, and flag any category with **zero** findings as an explicit "not covered — double-check" line. This catches the classes this engine underweights: **A02 cryptographic failures** (plaintext or weakly-hashed credentials, tokens, or PII at rest; predictable tokens; missing encryption on sensitive columns), **A06 vulnerable and outdated components** (a dependency with a *reachable* exploit path — not version-drift noise), and **A09 logging and monitoring failures** (auth failures, access-control denials, and privileged actions that leave no trace for detection). The backstop is a coverage check, not a mandate to invent findings — an honest "no evidence found in A02" is a valid result. ## High-miss checklist (technology-shaped, not stack-specific) @@ -55,7 +63,8 @@ Apply these — they're where AI-built apps most often fail: - **Auth-provider drift** — claims from an external identity provider (e.g. Clerk) trusted without verifying how they map to data scope. - **Gate/action field mismatch** — permission checked on one ID, action performed on an independent ID never proven to belong to it. - **Forgeable request signals** — endpoints gated by `?source=cron`, `?bot=1`, guessable headers, or unsigned webhook-like payloads instead of real auth. Raise severity when the endpoint mutates data, sends email, or triggers paid usage. -- **Output encoding vs. input validation** — user data interpolated into HTML, ``, attributes, JSON-LD, SQL, or Markdown must be encoded for *that* sink; input validation doesn't count. (XSS, CSP gaps.) +- **Output encoding vs. input validation, and CSP** — user data interpolated into HTML, `<title>`, attributes, JSON-LD, SQL, or Markdown must be encoded for *that* sink; input validation doesn't count. Check the Content-Security-Policy itself: weak or missing directives, `unsafe-inline`, wildcard sources, inline event handlers — recommend a stricter policy that still supports app features. (XSS, CSP.) +- **Prompt injection and agent abuse (AI apps)** — treat the model as both a sink and a source. Untrusted content (fetched pages, uploaded files, DB rows, tool output) reaching an LLM prompt; attacker text driving a privileged tool call or agent action (confused deputy); system-prompt or secret exfiltration; and unvalidated LLM *output* flowing into a downstream sink (SQL, shell, HTML, a follow-on tool call). - **SSRF / renderer abuse** — attacker-influenced URLs, HTML, SVG, or Markdown reaching an outbound fetch or a renderer (headless browser, PDF/OG-image generator). - **Parser / validator differentials** — the validator accepts a value the consumer interprets differently: unanchored regex, `startsWith`/substring allowlists, URL-parser disagreement, encoding/case/slash/path-normalization mismatch, or validation on one representation and execution on another. - **Fail-open paths** — error, `catch`, timeout, cancellation, cache-miss, stale-cache, feature-flag, or boundary-value branches that default to *allow*. AI code loves a permissive fallback. @@ -83,4 +92,5 @@ End with: the root-cause theme across findings; **what is well-built — say it - Don't report generic hardening with no concrete impact, outdated deps without a reachable path, or test/mock code unless it ships. Logic and authorization bugs with no classic sink still count. - This command covers security only. For over-fetching, indexes, and caching, use `/performance-audit-static`. +- This is the specialised procedure behind the **code-review** skill's security sub-case. For logic and state defects, or for a review across several dimensions at once, use `/pm-ai-shipping:code-review`. - For an end-to-end pass that documents first and produces a shipping packet, use `/ship-check`. diff --git a/pm-ai-shipping/commands/ship-check.md b/pm-ai-shipping/commands/ship-check.md index c380f76..24e9702 100644 --- a/pm-ai-shipping/commands/ship-check.md +++ b/pm-ai-shipping/commands/ship-check.md @@ -1,5 +1,5 @@ --- -description: Turn a vibe-coded repo into a reviewer-ready shipping packet — document the app, wire agent context, run security and performance audits, map test coverage, and compile the results +description: Turn a vibe-coded repo into a reviewer-ready shipping packet — document the app, wire agent context, run correctness, security and performance reviews, add an independent unsteered pass, map test coverage, and compile the results argument-hint: "<repo path or area; defaults to the whole repository>" --- @@ -29,19 +29,39 @@ Ensure the system docs exist and are current (run `/document-app` if they're mis Create or refresh `CLAUDE.md` (and a thin `AGENTS.md` pointing to it) **derived from** the system docs — the operating instructions the next AI coding agent inherits: what the system is, the trust boundaries, what may and may not be touched, where the guardrails are. This is a different artifact from the system docs: instructions, not description. -### Step 3: Security audit +### Step 3: Correctness review -Run the security pass (`/security-audit-static`), applying the **intended-vs-implemented** skill to flag where the code diverges from `permissions.md`, `flows.md`, and `architecture.md`. Summarize surviving findings. +Apply the **code-review** skill with `dimensions=correctness`. This is the pass the other two audits do not perform: logic and state defects that compile clean, pass the suite, and violate an agreement between two places that each look reasonable alone. Run its forced probes rather than reading through — authority reconciliation (a *requested* value still driving state where the authority returned something different) and identity correlation (results joined to their originating entity by an unstable key) are the classes strong agents miss most, and they are missed at the *look*, not at the fix. -### Step 4: Performance audit +Fan out over flows, never over files. Summarize surviving findings. + +### Step 4: Security audit + +Run the security pass (`/security-audit-static`), applying the **intended-vs-implemented** skill to flag where the code diverges from `permissions.md`, `flows.md`, and `architecture.md`. Summarize surviving findings, and **carry through the model mix it reports** — which clusters ran on the strongest model and which were rerouted to the fallback (Opus 4.8) by Fable's classifiers. + +### Step 5: Performance audit Run the performance pass (`/performance-audit-static`) — over-fetching, missing indexes, caching. Summarize findings. -### Step 5: Derive the test-coverage map +### Step 6: Independent unsteered review -Run `/derive-tests` to turn the documented rules — and the gaps the audits just surfaced — into a coverage map (`tests.md`): which rules are pinned by tests that exist *today*, which are only proposed, which are guarded-live or manual, and which have no verification at all. Running this **after** the audits is deliberate: each confirmed finding becomes a concrete regression test to pin, so the same gap can't silently reopen on the next AI edit. This is the operational form of "documented == implemented," and the unverified boundary rules feed straight into the launch-blocker assessment below. +Everything above is *steered*: each pass looks for the classes its own checklist names, which is exactly why each pass is blind in the same places twice. This step is the backstop, and on a real release it is the highest-yield step in this sequence. -### Step 6: Compile the shipping packet +Hand the subject to a **fresh session of a different model** — Codex (`codex exec`) is the usual choice, but any capable second model works — under three rules: + +1. **Fresh, never a resume.** Not the thread that wrote the code, and not one that has seen the earlier findings. A session that already argued the code is correct will argue it again. +2. **No checklist and no pointer to prior findings.** The value here is what an unprimed reader notices. Giving it the audit output converts an independent sample into a confirmation pass. +3. **Define the subject mechanically, not in prose.** Diff against the last release tag or the deployed branch, plus the working tree — e.g. `git log --oneline <last-tag>..HEAD` and `git status`. A described subject drifts; a computed one does not. + +**Verify every finding against the code by hand before it enters the packet.** An unsteered reviewer has no refutation discipline imposed on it, so it will produce confident findings that the code already prevents. Apply the **code-review** skill's keep/drop rule to each one: a finding survives only with a supported obligation, a feasible execution, a concrete contradiction, an observable consequence, and a counterargument you actually checked. + +Distinguish defects the change **introduced** from defects it merely **revealed** — both belong in the packet, but only the first blocks the change itself. On a release pass, repeat the loop until a round surfaces no introduced findings above Low. + +### Step 7: Derive the test-coverage map + +Run `/derive-tests` to turn the documented rules — and the gaps the reviews just surfaced — into a coverage map (`tests.md`): which rules are pinned by tests that exist *today*, which are only proposed, which are guarded-live or manual, and which have no verification at all. Running this **after** the reviews is deliberate: each confirmed finding becomes a concrete regression test to pin, so the same gap can't silently reopen on the next AI edit. This is the operational form of "documented == implemented," and the unverified boundary rules feed straight into the launch-blocker assessment below. + +### Step 8: Compile the shipping packet ``` ## Shipping Packet: [repo / area] @@ -55,12 +75,21 @@ CLAUDE.md / AGENTS.md: [created / updated / already current] ### Test Coverage [Rules pinned by tests that exist today · proposed but not yet written · guarded-live/manual · and the documented rules nothing verifies yet] +### Correctness Summary +[Surviving findings, each: Expectation · Trigger · Defect · Impact · Remedy, citing every participant] + ### Security Summary [Counts by severity + the surviving findings, each: Risk · Attack · Impact · Fix] ### Performance Summary [Findings by view/route/table, each: Recommendation · Effort · Priority] +### Independent Review +[Which model and session ran it, how the subject was computed, how many findings it returned, how many survived hand-verification — and the ones that survived. Note whether the last round was clean.] + +### Audit Provenance +[Which model each audit actually ran on, any clusters Fable's classifiers rerouted to the fallback (Opus 4.8), and the second model used in Step 6 — so the reviewer knows how much of the work saw the strongest model vs. the fallback, and that at least one pass was genuinely independent] + ### Launch Blockers [Unresolved Critical/High items — including any boundary rule that is both unverified and unaudited — that should stop a ship] @@ -73,4 +102,5 @@ CLAUDE.md / AGENTS.md: [created / updated / already current] - This is a handoff compiler: the value is sequencing plus synthesis, not re-deriving each audit. - If documentation is missing, the packet says so loudly — an audit without documented intent is incomplete, and the inventory makes that visible rather than hiding it. - Findings are code-review results, not confirmed exploits; the packet is a basis for human sign-off, not a substitute for it. -- Run the specialist commands directly (`/document-app`, `/derive-tests`, `/security-audit-static`, `/performance-audit-static`) when you only need one stage. +- Step 6 is skippable only when no second model is available — say so in the packet rather than omitting the section, because "not run" and "run clean" are very different signals to a reviewer. +- Run the specialist commands directly (`/document-app`, `/derive-tests`, `/pm-ai-shipping:code-review`, `/security-audit-static`, `/performance-audit-static`) when you only need one stage. diff --git a/pm-ai-shipping/skills/code-review/SKILL.md b/pm-ai-shipping/skills/code-review/SKILL.md index 7b90c52..9298a64 100644 --- a/pm-ai-shipping/skills/code-review/SKILL.md +++ b/pm-ai-shipping/skills/code-review/SKILL.md @@ -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. 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 ab3a8ee..e8621c5 100644 --- a/pm-ai-shipping/skills/code-review/references/correctness-taxonomy.md +++ b/pm-ai-shipping/skills/code-review/references/correctness-taxonomy.md @@ -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. diff --git a/pm-ai-shipping/skills/code-review/references/performance-review.md b/pm-ai-shipping/skills/code-review/references/performance-review.md new file mode 100644 index 0000000..dd6ef63 --- /dev/null +++ b/pm-ai-shipping/skills/code-review/references/performance-review.md @@ -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. diff --git a/pm-ai-shipping/skills/code-review/references/security-review.md b/pm-ai-shipping/skills/code-review/references/security-review.md new file mode 100644 index 0000000..0686526 --- /dev/null +++ b/pm-ai-shipping/skills/code-review/references/security-review.md @@ -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.