From 8d5c711e85453dc1da6c9336ff1843ad04214bee Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 31 Aug 2026 22:10:35 -0400 Subject: [PATCH 01/11] docs(performance): lock the brief for the new performance plugin Records the outcome of the /planning:interview that issue #3530 requires before implementation. Twelve questions registered, nine answered by the user, three deferred to planning with arbiter tags. Two decisions diverge from #3530's own text and say so: - Phase 4's "suppress the paired ratio under concurrency" is corrected. Duet Benchmarking (Bulej et al., ICPE 2020) measured 5.03x and 37.4x accuracy improvements from running arms in parallel on shared machines, because both arms absorb the same interference. Sequential interleaving keeps the suppression rule; simultaneous paired arms do not. - The unmeasurable-host refusal ships as a house rule, not as field consensus. No benchmarking tool surveyed refuses above a variance threshold; they warn and print anyway. The brief also records that the plugin's headline metric, a process-spawn count, rests on a rationale the literature grounds only for instruction counts. That gap is labelled rather than smoothed over. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS --- docs/topics/performance-plugin/PLAN.md | 114 +++++++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 docs/topics/performance-plugin/PLAN.md diff --git a/docs/topics/performance-plugin/PLAN.md b/docs/topics/performance-plugin/PLAN.md new file mode 100644 index 000000000..5591e4cde --- /dev/null +++ b/docs/topics/performance-plugin/PLAN.md @@ -0,0 +1,114 @@ +# performance-plugin + +Source issue: [#3530](https://github.com/melodic-software/claude-code-plugins/issues/3530). +Interview: round 1, Q1-Q9, all answered 2026-08-31. Ledger (memory tier, not committed): +`.work/performance-plugin/interview-checklist.md`. +Research slice (memory tier, not committed): `.work/performance-plugin-methodology/RESEARCH.md`. + +## Brief + +### TLDR + +- A new `performance` plugin whose skills run a measurement-first optimization workflow: identify a + target, construct a goal with realistic and ideal tiers plus a computed floor, snapshot a + baseline, verify, and report. +- Its headline metric is a **drift-immune counter**, not a duration. It ships exactly one built-in + counter (process spawns); every other metric is user-declared per domain. +- It **refuses** to report a wall-clock claim from a host whose noise it has characterized as + pathological, and says which counter it can report instead. +- It owns measurement, goal construction, and verification. It delegates the code change to + `/implementation:implement` and depends on `/verification:measure` for baseline/compare mechanics. +- Gates hard-block. Every gate ships with a discrimination check proving it fails when its condition + is unmet. + +### Goal + +Performance optimization in this fleet becomes a repeatable, measured discipline rather than a +per-session improvisation that produces confident, unverifiable numbers. The plugin exists because a +competent operator with a strong prompt still produced five verification harnesses in one session +that each returned a **confident wrong answer** rather than an error. The workflow's value is not +that it measures; it is that it refuses to report what it cannot support, and that every gate it +enforces has itself been proven to discriminate. + +### Constraints + +- **No skill may report a duration without a noise characterization.** Violating this reproduces the + exact failure the plugin exists to prevent. +- **Every gate must be verified to discriminate.** A check that passes whether or not its condition + holds is worse than no check, because it reports success. Each gate ships with a two-arm test: a + positive arm where it must fire and a negative arm where it must not, and the arms must be shown + to differ. +- **`/verification:measure` is not reimplemented.** It already owns two-phase baseline/compare, + machine-bound baseline storage, and the no-baseline refusal. Duplicating it is the silent second + way `/discipline:reuse-or-replace` prohibits. +- **The noise-characterization threshold has exactly one home.** No copy of + `BIMODAL_SPREAD_RATIO` may exist in two plugins. +- New plugin follows repo conventions: `.claude-plugin/plugin.json`, `CHANGELOG.md`, `README.md`, + marketplace entry with a category drawn from `docs/CATALOG-TAXONOMY.md` (read + `.claude/rules/catalog-taxonomy.md` first), changelog-parity and plugin-schema CI gates green. +- Validate with `scripts/affected-tests.sh --run`, never a hand-picked suite. +- Prose follows the repo's house style; `plugins/*/skills/*/vendor/**` formatting is not a model. + +### Acceptance criteria + +- `plugins/performance/` exists with a manifest that validates — the `plugin-schema` CI gate goes + green on the new directory, and `changelog-parity` passes. +- `/skill-quality:check` reports PASS for each new skill. +- The workflow refuses to report a wall-clock claim from a host characterized as too noisy to + measure — asserted by a test feeding it a high-variance baseline (refusal) AND a low-variance + baseline (normal report), with the two arms shown to produce different outcomes. +- A drift-immune counter is reported alongside, and ranked above, any duration — verified by reading + the emitted report format. +- Every normative claim in a skill body carries a citation to a source fetched during the research + pass, or is explicitly labelled as a house rule with no field consensus behind it. +- `claude-ops:audit-performance` consumes the promoted shared noise-characterization lib and its + existing tests still pass. + +### Decisions locked in the interview + +| Q | Decision | +|---|---| +| Q1 | **Narrow metrics, broad targets.** Any target reducible to one repeatable command. Process-spawn count is the ONLY built-in drift-immune counter; all others are user-declared per domain. | +| Q2 | **Depend + route.** `performance` owns the discipline and depends on `/verification:measure` for baseline/compare. `measure` stays and gains one routing line pointing here for wall-clock claims on a drifting host. | +| Q3 | **Measurement + goal + verification only.** The code change is delegated to `/implementation:implement`. | +| Q4 | **Gates hard-block**, with a named per-gate override that is recorded in the emitted report. | +| Q5 | **Reuse `BIMODAL_SPREAD_RATIO`** as the unmeasurable-host threshold rather than inventing a second number. Q4's recorded override applies. The refusal message must name the counter it can still report. | +| Q6 | **Baselines live in the memory tier**, `.work//baselines/`, machine-bound, never committed. Matches `/verification:measure` exactly. | +| Q7 | **Promote the noise-characterization algorithm into a shared lib** with one home for the threshold, and refactor `claude-ops:audit-performance` to consume it. No reaching into its private script directory; no copy-and-drift. | +| Q8 | **Phases 2-6 may run unattended. Phase 1 (goal construction) is human-gated always.** The loop is opt-in, may open PRs, may never merge. Mirrors the repo's existing loop-lane topology. | +| Q9 | **Both pairing modes.** Sequential interleaving suppresses the paired ratio under concurrent load; simultaneous duet-style paired arms report it. #3530's Phase 4 text is corrected, not followed. | + +### Captured assumptions + +- The plugin is used primarily on this host and hosts like it (Windows, MSYS/native mix, bimodal + process-creation cost) — revisit if it is aimed at contributors whose hosts are always noisy, which + would make the Q5 refusal posture unusable rather than protective. +- Promoting the shared lib will not break `claude-ops:audit-performance`'s existing tests — revisit + if that refactor turns out to touch its reporting contract rather than just its internals. +- Skill decomposition (one workflow skill with phases, versus one skill per phase) is a planning + decision — revisit if it turns out to change what the acceptance criteria can assert. + +### Out-of-scope + +- Owning the code change. Phase 3 delegates to `/implementation:implement`. +- Reimplementing baseline/compare mechanics that `/verification:measure` already provides. +- Built-in counters beyond process spawns. Syscall, query, and allocation counters are user-declared + in V1 and only become built-ins once validated against a real target. +- Merging its own PRs, under any autonomy setting. +- Superseding or removing `/verification:measure`. + +### Deferred questions + +- Q10 — Where exactly does the shared noise-characterization lib live, and does the + `claude-ops:audit-performance` refactor land in this PR or a follow-up? — defer until planning; + **arbiter: /planning:plan** +- Q11 — Skill decomposition: one workflow skill with six phases, or one skill per phase? — defer + until planning; **arbiter: /planning:plan** +- Q12 — Sample count and percentile choice. #3530 says "p50 and p95 over >=20 samples", but the + research found no community-grounded sample count and the SRE Book names 99th/99.9th rather than + p95. Whatever ships is a house choice and must be labelled as one. — defer until skill authoring; + **arbiter: USER-RESERVED** + +## Plan + + From 3728a0f67465157ce627026154f05ff895dbb57e Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 31 Aug 2026 22:19:48 -0400 Subject: [PATCH 02/11] docs(performance): plan the build, resolving Q10 and Q11 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Q11 — four skills, no router: target, goal, snapshot, verify, each naming its successor the way the planning pipeline already chains. Named `snapshot` rather than `measure` because Q2 locked "depend + route" on /verification:measure and two skills called `measure` is that routing line failing to route. Harness integrity ships as a shared reference plus a script rather than a fifth skill; it is a discipline applied inside the other skills, not a standalone invocation. Q10 — a cross-plugin runtime import is not available, since plugins install independently. The interview's "shared lib" answer is implemented through the mechanism this repo already uses for six other clusters: canonical source at lib/, byte-identical plugin copies, a dedicated sync-*.sh gate, a registry entry, and a CI job. One home for the threshold, loud drift, no runtime coupling. Recorded while resolving it: the noise threshold is a two-part predicate (spread ratio >= 3.0 AND max >= the slow-spawn floor), not a bare ratio. A cold-then-warm spawn pair clears 3x while every sample is still fast, so a consumer that re-derives a verdict from the ratio alone would report contention on a healthy host. Split into two PRs: the lib promotion and claude-ops refactor first, the new plugin second. The refactor is test-invisible — audit-performance re-exports the promoted names, so its six existing cases prove it. Q12 (sample count and percentile choice) stays USER-RESERVED and is surfaced at the approval gate, not resolved here. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS --- docs/topics/performance-plugin/PLAN.md | 142 ++++++++++++++++++++++++- 1 file changed, 141 insertions(+), 1 deletion(-) diff --git a/docs/topics/performance-plugin/PLAN.md b/docs/topics/performance-plugin/PLAN.md index 5591e4cde..68b4e8650 100644 --- a/docs/topics/performance-plugin/PLAN.md +++ b/docs/topics/performance-plugin/PLAN.md @@ -111,4 +111,144 @@ enforces has itself been proven to discriminate. ## Plan - +Written 2026-08-31 against `feat/performance-plugin` (branched from `origin/main` at `79f1c29`). +Resolves Q10 and Q11 (arbiter `/planning:plan`). Q12 stays USER-RESERVED and is surfaced at the +approval gate below, not resolved here. + +### Q11 resolved — skill decomposition + +**Four skills, no router.** Each names its successor, the way the repo's own planning pipeline +chains (`interview` -> `explore` -> `plan` -> `implement`) rather than routing through a hub. + +| Skill | Phases | Owns | +|---|---|---| +| `/performance:target` | 0 | Identify and rank candidate targets by **evidence quality**, not suspicion. When nothing is measured, the top recommendation is "instrument this first". Entry point. | +| `/performance:goal` | 1 | Human-gated always. Metric + the exact command producing it, a **realistic** target, an **ideal** target, and the computed **floor**. Refuses to accept a target below the measured floor without the user deciding. | +| `/performance:snapshot` | 2, 4 | Host qualification, snapshot capture, interleaved and duet A/B, the drift-immune counter, and the unmeasurable-host refusal. | +| `/performance:verify` | 5, 6 | Fresh-context adversarial re-derivation that does not inherit the implementer's numbers, plus the report. | + +**Naming.** `snapshot`, not `measure`. Q2 locked "depend + route" on `/verification:measure`; two +skills named `measure` in two plugins is the routing line failing to route. `snapshot` is also +#3530's own vocabulary ("baseline snapshot", "post snapshot"). + +**Harness-integrity is a shared reference plus a script, not a fifth skill.** The five +confident-wrong harnesses are the plugin's most important content, but they are a discipline applied +*inside* `snapshot` and `verify`, not something invoked standalone. Ships as +`reference/harness-integrity.md` plus `scripts/discriminate.py`, both consumed by the two skills that +need them. Promoting it to a fifth skill is the obvious V2 move if users start asking "does my +harness actually discriminate?" as a standalone question; deferring keeps the shared skill-listing +budget lower for V1. + +### Q10 resolved — the shared lib, and the PR split + +**A cross-plugin runtime import is not available.** Plugins install independently, so +`performance` cannot import from `claude-ops` at runtime. The interview's accepted answer ("promote +into a shared lib") is implemented through the repo's established mechanism for exactly this, not +through an import. + +**The established pattern**, already carrying six clusters (`scripts/cross-plugin-source-registry.txt`): + +- canonical source at repo root `lib/`; +- byte-identical copies at `plugins//lib/` in each carrying plugin; +- a dedicated `scripts/sync-.sh` built on `scripts/lib/sync-cluster.sh`, giving `--check`, + `--check-bump `, and `--print-manifest`; +- registered in `scripts/cross-plugin-source-registry.txt` with its check named; +- a CI job. + +This satisfies the brief's constraint that the threshold has exactly one home: `lib/` is the home, +and every copy that drifts fails CI loudly. + +**Applied here:** + +- Canonical `lib/spawn-noise.py`, holding `summarize_spawn_samples`, `spawn_probe`, + `BIMODAL_SPREAD_RATIO`, `SLOW_SPAWN_FLOOR_MS`, `SPAWN_SAMPLES`, `NOOP_SPAWN`. +- Copies at `plugins/claude-ops/lib/spawn-noise.py` and (in PR 2) + `plugins/performance/lib/spawn-noise.py`. +- `scripts/sync-spawn-noise.sh` + registry entry + CI job `spawn-noise-sync`. +- `audit_performance.py` imports it with the `sys.path.insert(_LIB_DIR)` shape already used by + `plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py:55`, and **re-exports the names** + so `test_audit_performance.py`'s `engine.summarize_spawn_samples` keeps resolving. The promotion is + test-invisible; the six existing cases are the proof. + +**The threshold is a two-part predicate, not a constant.** `bimodal-spawn-latency` fires on +`spread_ratio >= BIMODAL_SPREAD_RATIO (3.0)` **AND** `max >= SLOW_SPAWN_FLOOR_MS`. A wide ratio alone +is not the contention signature: a cold first spawn against a warm second clears 3x while every +sample is still fast. `performance` must consume the predicate, never re-derive a verdict from the +ratio alone. + +**Two PRs.** + +- **PR 1** — lib promotion, sync gate, registry entry, CI job, `claude-ops` refactor + CHANGELOG + + version bump. Self-contained, test-invisible, independently reviewable. +- **PR 2** — the `performance` plugin itself, consuming the lib, plus the one routing line into + `/verification:measure`. + +Rationale: PR 2 is already large (four skills, harness scripts, evals, marketplace entry). Folding a +cross-plugin refactor of a third plugin into it makes review materially worse. The split is +reversible: if PR 1 reviews trivially, PR 2 can be opened before it merges and rebased. + +### Category + +`verification`. Checked against the taxonomy's Assignment principle rather than assumed: the subject +is arbitrary code, not one of the special subjects (Claude Code, the workstation, music, personal), +so the plugin files by lifecycle activity. `verification`'s scope line is "Prove a change achieved +its intended outcome against baseline and intent", which is this plugin's whole shape. +`codebase-health` is filed `quality` because it audits artifacts on an absolute axis; this plugin is +before/after proof against a baseline, which is the distinguishing trait. + +### Approach, in order + +1. **PR 1.** Extract `lib/spawn-noise.py`; add `scripts/sync-spawn-noise.sh` + its `.test.sh`; + register the cluster; add the CI job; refactor `audit_performance.py` to import and re-export; + bump `claude-ops` version + CHANGELOG. Gate: the six existing `summarize_spawn_samples` cases pass + unchanged. +2. **Scaffold `plugins/performance/`** — `.claude-plugin/plugin.json`, `CHANGELOG.md`, `README.md`, + `lib/spawn-noise.py` (synced copy), marketplace entry, regenerated `docs/CATALOG.md`. Gate: + `plugin-schema` and `changelog-parity` green. +3. **Author `reference/harness-integrity.md`** first, before any skill body. It is the content the + other four depend on, and it is the plugin's reason for existing. +4. **Author the four skills**, each citing the research slice. Parallelizable across workers once + step 3 lands, since each is a separate file with no shared edit surface. +5. **Port the harnesses** from `D:/worktrees/bench-dh/` into `scripts/`, with precondition assertions + built in. They live on local disk only and are not durable. +6. **Add the routing line** to `plugins/verification/skills/measure/SKILL.md` + CHANGELOG + version + bump. +7. **Evals** per skill; `/skill-quality:check` per skill. +8. **PR 2** per the repo's body template. + +### Test strategy + +- **The refusal criterion is the highest-risk one.** Per the source session, four of five + discrimination checks failed by exiting identically in *both* arms and reporting "not + discriminating". So the refusal test asserts three things, not two: the high-variance arm refuses, + the low-variance arm reports normally, and **the two arms produced different output** as a + first-class assertion. Annotated `# discriminating-skip-required:` so + `scripts/check-discriminating-test-skips.sh` forbids skipping it. +- Contract tests as `plugins/performance/**/*.test.sh`, modelled on + `plugins/disk-hygiene/hooks/run-python-hook.test.sh` (32 assertions, full cache-invalidation + matrix) — tests that assert their own preconditions. +- Python unit tests for `lib/spawn-noise.py`. +- Validate with `scripts/affected-tests.sh --run`, never a hand-picked suite. +- Lint against LF content (`tr -d '\r'`): `core.autocrlf=true` on this machine makes shellcheck + report SC1017 on every line and bury real findings. + +### Blast radius + +| Surface | Change | +|---|---| +| `lib/spawn-noise.py`, `scripts/sync-spawn-noise.sh` (+ test), CI workflow, registry | New (PR 1) | +| `plugins/claude-ops/**` | Refactor + CHANGELOG + version bump (PR 1) | +| `plugins/performance/**` | New (PR 2) | +| `.claude-plugin/marketplace.json`, `docs/CATALOG.md` | New entry + regeneration (PR 2) | +| `plugins/verification/skills/measure/SKILL.md` + CHANGELOG + version | One routing line (PR 2) | + +Three plugins are touched in total. Two of them (`claude-ops`, `verification`) are existing, +installed, and working; both changes are additive and version-bumped. + +### Surfaced at the approval gate + +**Q12 (USER-RESERVED) — sample count and percentile choice.** #3530 specifies "p50 and p95 over >=20 +samples". The research grounds none of it: no benchmarking-community sample count for a meaningful +percentile exists beyond the derivable `1/(1-p)` floor, and the SRE Book names the 99th and 99.9th +percentiles rather than p95. Whatever ships is a house choice that must be labelled as one in the +skill body. This needs the user at the approval gate, because it changes what the skills assert. From 6f7737a7a0cd0dca0722329fc9ee7a6a35986a90 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 1 Sep 2026 00:11:49 -0400 Subject: [PATCH 03/11] wip(performance): scaffold manifest, changelog, and README Partial scaffold. Skills, lib copy, sync gate, and marketplace entry still to come. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS --- .../performance/.claude-plugin/plugin.json | 22 +++++ plugins/performance/CHANGELOG.md | 34 +++++++ plugins/performance/README.md | 92 +++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 plugins/performance/.claude-plugin/plugin.json create mode 100644 plugins/performance/CHANGELOG.md create mode 100644 plugins/performance/README.md diff --git a/plugins/performance/.claude-plugin/plugin.json b/plugins/performance/.claude-plugin/plugin.json new file mode 100644 index 000000000..adb7ed0bf --- /dev/null +++ b/plugins/performance/.claude-plugin/plugin.json @@ -0,0 +1,22 @@ +{ + "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", + "name": "performance", + "version": "0.1.0", + "description": "Measurement-first optimization workflow for an arbitrary target, built around refusing to report what the data does not support. Four skills: target (identify and rank optimization candidates by evidence quality rather than suspicion, so an unmeasured target makes \"instrument this first\" the recommendation instead of a guess), goal (human-gated goal construction that holds a realistic target and an ideal target separately and computes the irreducible floor BEFORE any work, so a target below the floor is surfaced as unreachable-by-any-code-change up front rather than discovered as a failed goal at the end), snapshot (baseline and post capture with the host qualified first: repeated no-op spawns characterize the machine's own noise, a drift-immune counter is reported alongside and ranked above any duration, before/after arms are interleaved within one run rather than compared across two passes, and a wall-clock claim is REFUSED outright from a host whose spread carries the bimodal contention signature, naming the counter it can still report instead), and verify (fresh-context adversarial re-derivation that does not inherit the implementer's numbers, plus a report that states a target as met or not met and never rounds a miss into a win). Gates hard-block, with a named override recorded in the report. Every gate ships with a discrimination check proving it fails when its condition is unmet, because a check that passes whether or not the condition holds is worse than no check: it reports success. Normative claims carry a source tier, and the ones the benchmarking literature does not ground (sample counts, the p95 convention, counts-over-time for anything but instruction counts) are labelled as house rules rather than dressed as consensus.", + "author": { + "name": "Melodic Software", + "email": "info@melodicsoftware.com" + }, + "license": "MIT", + "keywords": [ + "performance", + "benchmarking", + "baseline", + "measurement", + "optimization", + "noise-characterization", + "drift-immune-counter", + "interleaved-ab", + "skill" + ] +} diff --git a/plugins/performance/CHANGELOG.md b/plugins/performance/CHANGELOG.md new file mode 100644 index 000000000..0f1f69113 --- /dev/null +++ b/plugins/performance/CHANGELOG.md @@ -0,0 +1,34 @@ +# Changelog + +All notable changes to the `performance` plugin are documented here. Format follows +[Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. + +## [0.1.0] + +### Added + +- **Initial release.** A measurement-first optimization workflow for an arbitrary target, built + around refusing to report what the data does not support. Generalized from one end-to-end run of + the workflow by hand against the `disk-hygiene` destructive-guard hook (#3523), including the five + verification harnesses in that session that each produced a confident WRONG answer rather than an + error. Settled by the `/planning:interview` #3530 required; see + `docs/topics/performance-plugin/PLAN.md`. +- **`target`** — identify and rank optimization candidates by evidence quality rather than + suspicion. An unmeasured target makes "instrument this first" the recommendation, not a guess. +- **`goal`** — human-gated goal construction. Holds a realistic and an ideal target separately and + computes the irreducible floor before any work, so a target below the floor is surfaced as + unreachable-by-any-code-change up front. The source run asked for p50 <= 250 ms on a host charging + 0.3-2.8 s per process spawn, and only discovered the goal was unreachable at the end. +- **`snapshot`** — baseline and post capture with the host qualified first. Repeated no-op spawns + characterize the machine's own noise (via the `spawn_noise` lib shared with `claude-ops`), a + drift-immune counter is reported alongside and ranked above any duration, arms are interleaved + within one run rather than compared across two passes, and a wall-clock claim is refused from a + host carrying the bimodal contention signature. +- **`verify`** — fresh-context adversarial re-derivation that does not inherit the implementer's + numbers, plus a report that never rounds a miss into a win. +- **`reference/harness-integrity.md`** — the discipline the other skills apply: a harness must prove + it is not measuring itself, a probe must assert its own precondition and fail rather than silently + degrade, and a discrimination check must verify its own patch applied and restore from saved bytes + rather than from version control. +- **`lib/spawn_noise.py`** — byte-identical copy of the canonical `claude-ops` lib, registered as a + cross-plugin cluster with a dedicated sync gate so the bimodal threshold has exactly one home. diff --git a/plugins/performance/README.md b/plugins/performance/README.md new file mode 100644 index 000000000..220c0d944 --- /dev/null +++ b/plugins/performance/README.md @@ -0,0 +1,92 @@ +# performance + +Measurement-first optimization for an arbitrary target, built around refusing to report what the +data does not support. + +## Why this exists + +This plugin was generalized from one end-to-end optimization run done by hand against the +`disk-hygiene` destructive-guard hook (#3523). That session had a competent operator and a strong +initial prompt. It still produced **five verification harnesses that each returned a confident wrong +answer rather than an error**, and every one was caught only because something explicitly re-checked +it: + +| Harness | Reported | Actually did | +|---|---|---| +| spawn census via a PATH shim | "no improvement" | `mktemp -d` put a fresh path on `PATH` every run; the subject cached on `PATH`, so every run was a forced cache miss. It measured its own randomization. | +| hard-link identity probe | "0 divergences" | `os.link` failed cross-volume on Windows and fell back to `shutil.copyfile`. A copy is a different file, so the probe never exercised the case it reported on. | +| discrimination check (shell) | "NOT DISCRIMINATING" | A `D:/...` path handed to bash resolves nowhere under MSYS, so both arms exited 127 and the grep found nothing in either. | +| discrimination check (repeat) | "NOT DISCRIMINATING" | Same trap, second harness. | +| discrimination check (python) | "NOT DISCRIMINATING" | Restored via `git checkout --` while the fix under test was uncommitted. The restore silently reverted the fix, so the "with fix" arm ran without it, and the work was destroyed. | + +None of those are knowledge gaps. They are all "the measurement was wrong in a way that looked +right", which is what enforced gates prevent and a checklist does not. Four of the five were in +checks written specifically to avoid being fooled: the meta-checks were less reliable than the thing +they were checking. + +A workflow that measures without enforcing these rules mostly generates confident numbers, which is +worse than generating none. + +## Skills + +| Skill | Owns | +|---|---| +| `/performance:target` | Identify and rank candidates by **evidence quality**, not suspicion. Nothing measured yet means the top recommendation is "instrument this first". | +| `/performance:goal` | Human-gated. The metric and the exact command producing it, a **realistic** target and an **ideal** target held separately, and the **floor** computed before any work. | +| `/performance:snapshot` | Host qualification, baseline and post capture, interleaved and duet A/B, the drift-immune counter, and the unmeasurable-host refusal. | +| `/performance:verify` | Fresh-context re-derivation that does not inherit the implementer's numbers, plus the report. | + +Each names its successor. There is no router skill. + +## What it refuses to do + +- **Report a wall-clock claim from a host it has characterized as unmeasurable.** The host this was + built on spread 15.7x across identical no-op spawns. A percentile from such a host is not so much + wrong as meaningless in isolation, which is why the durable result in the source PR was a + deterministic spawn count (4 -> 1) and not a duration. The refusal always names the counter it can + still report. +- **Rank a duration above a drift-immune counter** when one exists. +- **Compare two separate passes on a drifting host.** A bare `bash -c true` measured 1825 ms and + 283 ms in the same hour at ~10% CPU; any two-pass comparison attributes that 6x to the change. +- **Fold a behavior change into a performance claim.** A correctness regression outranks any + speedup and is stated separately. +- **Own the fix.** It measures, sets the goal, and verifies. The change itself is delegated to the + implementation lane. + +## Honest about its own grounding + +The methodology is sourced (see the source tiers in each skill body), and where the literature does +not support a rule, the skill says so rather than dressing a house choice as consensus: + +- **No benchmarking-community sample count for a meaningful percentile exists** beyond the derivable + `1/(1-p)` floor. The p50/p95-over-20-samples default here is a house rule, and the derivable floor + is the part that is actually enforced. +- **p95 specifically is convention.** The pattern "median plus a high-order percentile" is grounded + (Google SRE Book, ch. 4), but the percentiles that chapter names are the 99th and 99.9th. +- **No surveyed tool refuses above a variance threshold.** pyperf, Criterion, JMH and benchstat all + warn and print anyway. The refusal here is deliberately stricter than the field. +- **The counts-over-wall-clock rationale is grounded only for instruction counts.** Extending it to + syscalls, queries, and process spawns is this plugin's own generalization, and process-spawn count + is its headline metric. Valgrind's manual argues both halves itself: execution time "is what users + perceive", and its simulations are "unlikely to reflect the behaviour of a modern machine". +- **Warmup does not establish steady state.** Barrett et al. (OOPSLA 2017) found at most 43.5% of + VM/benchmark pairs consistently reach one. Discarding warmup iterations is fine; claiming steady + state is not. + +## Relationship to neighbouring plugins + +- **`/verification:measure`** owns two-phase baseline/compare and machine-bound baseline storage. + This plugin **depends on it** rather than reimplementing it, and adds what it does not cover: + interleaved A/B, drift-immune counters, host-unmeasurability refusal, precondition-asserting + probes, and goal tiers. +- **`/claude-ops:audit-performance`** diagnoses a slow *Claude Code installation*. This plugin + optimizes an *arbitrary target*. They share the noise characterization through + `lib/spawn_noise.py`, carried as a registered cross-plugin cluster so the bimodal threshold has + exactly one home. +- **`/implementation:implement`** owns the change. This plugin does not. + +## Baselines + +Baselines live in the topic's memory tier (`.work//baselines/`), are machine-bound, and +are **never committed** — matching `/verification:measure`. A committed baseline is a number that +outlives the conditions that made it true. From 99af8fdd775f2ed20fa5f8f39e20534a97048b1f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 1 Sep 2026 06:47:27 -0400 Subject: [PATCH 04/11] feat(performance): add the measurement-first optimization plugin Implements #3530 to the contract the required /planning:interview settled (docs/topics/performance-plugin/PLAN.md). Four skills, each naming its successor rather than routing through a hub: target ranks candidates by evidence quality (an unmeasured system yields "instrument this first", not a guess); goal is human-gated and computes the irreducible floor BEFORE the target is agreed; snapshot qualifies the host before measuring anything; verify re-derives the result in fresh context and reports it without rounding a miss into a win. Named `snapshot`, not `measure`. Q2 of the interview locked "depend + route" on /verification:measure, and two skills called `measure` is that routing line failing to route. That skill keeps baseline capture, storage, and the compare mechanics; this plugin adds what it does not cover and gains a gotcha pointing here for hosts a noise-floor warning cannot describe. The lib gains is_measurable() and percentile_floor(), and performance now carries lib/spawn_noise.py as a registered cross-plugin cluster with a dedicated sync gate and CI lane, so the bimodal threshold keeps one home. Plugins install independently, so a runtime import across the boundary was never available; the byte-identical-copy mechanism this repo already uses for six clusters is how the constraint is actually met. Two places where the research contradicted the issue, and the code follows the evidence: - #3530 says to suppress the paired ratio under concurrency. Duet Benchmarking (Bulej et al., ICPE 2020) measured 5.03x and 37.4x accuracy gains from running arms in PARALLEL on shared machines, because both arms absorb the same interference. Both modes ship; the suppression rule is scoped to the sequential form, and that reconciliation is labelled as this plugin's reading rather than a sourced claim. - The unmeasurable-host refusal ships as a house rule. No surveyed tool refuses above a variance threshold; pyperf, Criterion, JMH and benchstat all warn and print anyway. Three claims the literature does not ground are labelled rather than dressed as consensus: the p50/p95-over-20 sample default (only the derivable 1/(1-p) floor is real, and only that floor is enforced), p95 itself (the SRE Book names the 99th and 99.9th), and counts-over-wall-clock for anything but instruction counts, which is load-bearing here because process-spawn count is the headline metric and Valgrind does not run on Windows. Verification: all four skills PASS check-skill.sh with zero warnings; the sync gate's own suite proves --check DISCRIMINATES by asserting the clean and drifted arms return DIFFERENT verdicts, not merely that each printed its expected string; ruff clean; markdownlint clean; no em dashes in any new surface. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS --- .claude-plugin/marketplace.json | 6 + .github/workflows/ci.yml | 21 ++ docs/CATALOG.md | 1 + plugins/claude-ops/.claude-plugin/plugin.json | 2 +- plugins/claude-ops/CHANGELOG.md | 17 ++ plugins/claude-ops/lib/spawn_noise.py | 64 ++++++ plugins/claude-ops/lib/test_spawn_noise.py | 52 +++++ plugins/performance/CHANGELOG.md | 12 +- plugins/performance/README.md | 2 +- plugins/performance/lib/spawn_noise.py | 184 ++++++++++++++++++ .../reference/harness-integrity.md | 121 ++++++++++++ plugins/performance/skills/goal/SKILL.md | 130 +++++++++++++ plugins/performance/skills/snapshot/SKILL.md | 148 ++++++++++++++ .../skills/snapshot/evals/evals.json | 67 +++++++ plugins/performance/skills/target/SKILL.md | 110 +++++++++++ plugins/performance/skills/verify/SKILL.md | 98 ++++++++++ .../verification/.claude-plugin/plugin.json | 2 +- plugins/verification/CHANGELOG.md | 12 ++ plugins/verification/skills/measure/SKILL.md | 1 + scripts/cross-plugin-source-registry.txt | 3 + scripts/sync-spawn-noise.sh | 43 ++++ scripts/sync-spawn-noise.test.sh | 160 +++++++++++++++ 22 files changed, 1247 insertions(+), 9 deletions(-) create mode 100644 plugins/performance/lib/spawn_noise.py create mode 100644 plugins/performance/reference/harness-integrity.md create mode 100644 plugins/performance/skills/goal/SKILL.md create mode 100644 plugins/performance/skills/snapshot/SKILL.md create mode 100644 plugins/performance/skills/snapshot/evals/evals.json create mode 100644 plugins/performance/skills/target/SKILL.md create mode 100644 plugins/performance/skills/verify/SKILL.md create mode 100755 scripts/sync-spawn-noise.sh create mode 100755 scripts/sync-spawn-noise.test.sh diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index daced1bea..21057533f 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -391,6 +391,12 @@ "category": "verification", "tags": ["verification", "outcome", "baseline", "measure", "verify", "skill"] }, + { + "name": "performance", + "source": "./plugins/performance", + "category": "verification", + "tags": ["performance", "benchmarking", "baseline", "measurement", "optimization", "noise-characterization", "drift-immune-counter", "interleaved-ab", "skill"] + }, { "name": "kindle-dedrm", "displayName": "Kindle DeDRM", diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 25b9c6dfa..1ed5965cd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -609,6 +609,26 @@ jobs: BASE_REF: ${{ github.base_ref }} run: scripts/sync-state-key.sh --check-bump "origin/$BASE_REF" + spawn-noise-sync: + runs-on: ubuntu-24.04 + timeout-minutes: 15 + steps: + - name: Check out + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - name: Fetch base + uses: ./.github/actions/checkout-with-base + - name: Verify spawn-noise cluster matches canonical + run: scripts/sync-spawn-noise.sh --check + - name: Run spawn-noise tests + run: bash plugins/claude-ops/lib/spawn_noise.test.sh + - name: Verify carrying plugins bumped when canonical changed + if: github.event_name == 'pull_request' + env: + BASE_REF: ${{ github.base_ref }} + run: scripts/sync-spawn-noise.sh --check-bump "origin/$BASE_REF" + resolve-convention-pattern-sync: runs-on: ubuntu-24.04 timeout-minutes: 15 @@ -1729,6 +1749,7 @@ jobs: - parse-concern-value-sync - managed-scope-sync - state-key-sync + - spawn-noise-sync - resolve-convention-pattern-sync - index-regen-sync - standards-contract-sync diff --git a/docs/CATALOG.md b/docs/CATALOG.md index bae18462a..a67e29471 100644 --- a/docs/CATALOG.md +++ b/docs/CATALOG.md @@ -52,6 +52,7 @@ plugin manifests and kept in sync by CI — never hand-edit it; the category voc ## Verification - [`verification`](../plugins/verification) — Outcome-verification stage: prove a change achieved its intended outcome (`/verification:confirm` — a mechanical build/test/lint prerequisite gate, then intent-match + evidence + verdict with the criterion auto-detected by change type), and verify measurable-improvement claims against a planning-time baseline (`/verification:measure`), never fabricating numbers. +- [`performance`](../plugins/performance) — Measurement-first optimization workflow for an arbitrary target, built around refusing to report what the data does not support. Four skills: target (identify and rank optimization candidates by evidence quality rather than suspicion, so an unmeasured target makes "instrument this first" the recommendation instead of a guess), goal (human-gated goal construction that holds a realistic target and an ideal target separately and computes the irreducible floor BEFORE any work, so a target below the floor is surfaced as unreachable-by-any-code-change up front rather than discovered as a failed goal at the end), snapshot (baseline and post capture with the host qualified first: repeated no-op spawns characterize the machine's own noise, a drift-immune counter is reported alongside and ranked above any duration, before/after arms are interleaved within one run rather than compared across two passes, and a wall-clock claim is REFUSED outright from a host whose spread carries the bimodal contention signature, naming the counter it can still report instead), and verify (fresh-context adversarial re-derivation that does not inherit the implementer's numbers, plus a report that states a target as met or not met and never rounds a miss into a win). Gates hard-block, with a named override recorded in the report. Every gate ships with a discrimination check proving it fails when its condition is unmet, because a check that passes whether or not the condition holds is worse than no check: it reports success. Normative claims carry a source tier, and the ones the benchmarking literature does not ground (sample counts, the p95 convention, counts-over-time for anything but instruction counts) are labelled as house rules rather than dressed as consensus. ## Quality diff --git a/plugins/claude-ops/.claude-plugin/plugin.json b/plugins/claude-ops/.claude-plugin/plugin.json index bb7006ae7..06fb82462 100644 --- a/plugins/claude-ops/.claude-plugin/plugin.json +++ b/plugins/claude-ops/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "claude-ops", - "version": "0.41.0", + "version": "0.41.1", "description": "Claude Code operations toolkit. Twelve skills: audit-skill-visibility (audit whether each installed skill is actually VISIBLE to the model, and diagnose why most of a fleet never gets used \u2014 a skill is invisible when its description is dropped by Claude Code's skill-listing context budget, which sheds descriptions lowest-score-first so an unused skill loses the keywords that would let it be matched, from skills genuinely not wanted, from skills the run cannot observe at all; computes whether the listing overflows from documented settings, and withholds every cold verdict the data cannot support rather than reporting absence of data as absence of use), inventory (read-only enumeration of the complete invocable surface \u2014 every built-in CLI command with aliases and hidden/gated status, every bundled skill, and every component of every installed plugin across all marketplaces; reads the shipped binary because upstream publishes no built-in command list, and carries an integrity verdict so a drifted build reports counts as floors rather than silently short totals), audit-install-state (read-only audit of the machine-scope ~/.claude installation directory and ~/.claude.json \u2014 full inventory split into an authored surface and rolled-up bulk trees, product-managed retention vs genuinely unmanaged state, filename-scheme resolution before any process-liveness check, and deliberate/mid-experiment detection; reports, never deletes), audit-performance (read-only slowness-diagnostic capture run at the moment the machine or a session feels slow: CLI version, retention-sweep health including the silent unparsable-settings pause, a timed census walk of the install tree as a sweep-cost proxy, active-session and plugin-fleet counts, a process census, and the fan-out layer, which covers a load-labelled no-op spawn baseline, every hook that will fire bucketed per-tool-call versus per-turn with its invocation shape, the configured statusline, subagent concurrency and spawn-depth ceilings against documented defaults, whether running sessions predate the settings file they are judged by, and orphan attribution by parent liveness rather than age; read against a bundled known-performance-issues reference that also records the causes tested and cleared; separates the four documented suspects of accumulated state, version regression, component bloat, and per-spawn fan-out cost, and routes remediation out; reports, never mutates, and never executes a discovered hook or statusline command), audit-native-overlap (map native Claude Code surfaces \u2014 built-in CLI commands, bundled skills, plugin-backed built-ins, session-provided skills \u2014 against the current repo's plugin skills and agents, so a custom component never silently duplicates what Claude Code itself ships; bare invocation is a read-only overlap report carrying the extraction's integrity floors and a shared-listing-budget exposure section, verdicts are human-gated in a committed store rendered into a generated registry whose every row carries an observable recheck trigger, and only an explicit apply step bakes presence-gated native references into descriptions and Boundary sections), observability (read locally captured telemetry \u2014 OTEL store, collector, hook-event JSONL, ccusage \u2014 with trend reports and store pruning), known-issues (search known Claude product GitHub bugs, check service health, maintain a persistent tracked-issue registry), changelog (ingest Claude Code changelog entries and integrate them into the current repo), plugins (bring a machine's plugin fleet current on demand \u2014 marketplace refresh, effective-scope updates including in-repo project/local installs, new-plugin install per policy, scope-divergence detection and explicit convergence), morning-brief (read-only gh-based operator morning view \u2014 queue-label counts, merge-ready PRs, parked decisions with their RECOMMENDED lines, and loop-lane telemetry freshness), lanes (start/restart/stop/status loop lanes as named background Claude Code sessions seeded from canonical prompt files, with per-lane model/effort, a repo-pull + marketplace-refresh launch step, and a consume-restarts action \u2014 an OS-schedulable reader that relaunches stopped lanes whose telemetry carries a restart_request), and a re-runnable setup action that settles where the known-issues registry lives. Plus a family of eight advisory *-audit hooks (API errors, config changes, instruction loads, permission denials, pre-compaction, skill usage, tool failures, and unsurfaced hook failures \u2014 the last also warns the user via systemMessage, since a hook that fails to launch enforces nothing and Claude Code surfaces the failure to nobody) that emit the shared hook-telemetry envelope, and a reference sink that maps envelopes into the hook-events.jsonl the observability skill reads.", "author": { "name": "Melodic Software", diff --git a/plugins/claude-ops/CHANGELOG.md b/plugins/claude-ops/CHANGELOG.md index a83b64282..dbff7919b 100644 --- a/plugins/claude-ops/CHANGELOG.md +++ b/plugins/claude-ops/CHANGELOG.md @@ -3,6 +3,23 @@ All notable changes to the `claude-ops` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.41.1] + +### Added + +- **`lib/spawn_noise.py` gains `is_measurable()` and `percentile_floor()`.** `is_measurable()` turns + a spawn-noise summary into a verdict on whether a WALL-CLOCK claim may be reported from this host, + reading the `findings` list rather than `spread_ratio` alone so the two-part bimodal predicate is + preserved. `percentile_floor()` returns `1/(1-p)`, the only sample-count constraint the + benchmarking literature actually grounds. `claude-ops` calls neither; both exist for the + `performance` plugin, which now carries this file as a registered cross-plugin cluster + (`scripts/sync-spawn-noise.sh`, CI lane `spawn-noise-sync`) so the bimodal threshold keeps exactly + one home. Adding them here rather than only in the consumer is what byte-identical cluster copies + require. +- **Lib tests for both.** The measurability test asserts the quiet-host and contended-host arms + produce DIFFERENT verdicts as a first-class check, not merely that each produced its expected + value: a refusal that fires on every host refuses nothing. + ## [0.41.0] ### Added diff --git a/plugins/claude-ops/lib/spawn_noise.py b/plugins/claude-ops/lib/spawn_noise.py index 11c31b8ad..dfa608186 100644 --- a/plugins/claude-ops/lib/spawn_noise.py +++ b/plugins/claude-ops/lib/spawn_noise.py @@ -25,6 +25,7 @@ from __future__ import annotations +import math import statistics import subprocess import sys @@ -118,3 +119,66 @@ def spawn_probe(samples: int = SPAWN_SAMPLES, timeout_s: float = SPAWN_TIMEOUT_S summary = summarize_spawn_samples(durations, timeouts, load) summary["command"] = " ".join(command) return summary + + +def is_measurable(summary: dict) -> tuple[bool, str]: + """Decide whether a WALL-CLOCK claim may be reported from this host. + + Returns `(measurable, reason)`. This function states a verdict and its basis; + it never suppresses anything itself, and the caller decides what a False means. + `performance` treats it as a hard refusal; `claude-ops` does not call it. + + The verdict rides on the `findings` list rather than on `spread_ratio`, + because `bimodal-spawn-latency` is the two-part predicate documented at module + level. A bare ratio comparison would fire on a healthy cold-then-warm host. + + A False is never the end of the road: a drift-immune counter (process spawns, + syscalls, queries) is still reportable from a host too noisy for a duration, + and that is what the source run of this workflow ultimately shipped -- a + deterministic 4 -> 1 spawn count, not a millisecond figure that no independent + verifier could reproduce an hour later on the same machine. + + Refusing above a variance threshold is stricter than the benchmarking field: + pyperf, Criterion, JMH and benchstat all WARN and print the number anyway. This + is a deliberate house rule, not a consensus practice, and callers must present + it as one. + """ + findings = summary.get("findings", []) + if "no-spawn-samples-captured" in findings: + return False, "no spawn samples were captured, so the host was never characterized" + if "bimodal-spawn-latency" in findings: + return False, ( + f"spawn cost spread {summary.get('spread_ratio')}x across identical no-op spawns " + f"(min {summary.get('min_ms')} ms, max {summary.get('max_ms')} ms), with the slow mode " + f"above the {SLOW_SPAWN_FLOOR_MS} ms floor: the bimodal contention signature" + ) + if "spawn-probe-timed-out" in findings: + return False, "at least one no-op spawn hit the probe timeout, so the tail is unbounded" + if "slow-spawn-floor" in findings: + return False, ( + f"even the fastest no-op spawn cost {summary.get('min_ms')} ms, above the " + f"{SLOW_SPAWN_FLOOR_MS} ms floor: the host is contended before any work starts" + ) + return True, ( + f"spawn cost spread {summary.get('spread_ratio')}x with a {summary.get('min_ms')} ms " + "floor: within the measurable band" + ) + + +def percentile_floor(percentile: float) -> int: + """Minimum samples for a percentile to be arithmetically expressible: `1/(1-p)`. + + This is the ONLY grounded constraint on sample count that this codebase is + aware of, and it is grounded because it is derivable, not because a source + states it. The research pass behind #3530 found no benchmarking-community + consensus figure for "enough samples to make a percentile meaningful": the + familiar folklore numbers appear in no first-party or peer-reviewed source. + + So `performance` enforces this floor and labels its p50/p95-over-20 default as + a house convention. p95 needs 20 samples; p99 needs 100. Below the floor the + percentile is not merely imprecise, it cannot be computed from the data at all + -- the requested tail does not exist in the sample. + """ + if not 0.0 < percentile < 1.0: + raise ValueError(f"percentile must be in (0, 1), got {percentile!r}") + return math.ceil(1.0 / (1.0 - percentile)) diff --git a/plugins/claude-ops/lib/test_spawn_noise.py b/plugins/claude-ops/lib/test_spawn_noise.py index 1337fd92c..7bc3f5458 100755 --- a/plugins/claude-ops/lib/test_spawn_noise.py +++ b/plugins/claude-ops/lib/test_spawn_noise.py @@ -83,5 +83,57 @@ def test_the_arms_actually_discriminate(self): self.assertIn("bimodal-spawn-latency", slow) +class TestMeasurabilityVerdict(unittest.TestCase): + """The refusal must discriminate, and must never be a dead end.""" + + QUIET = [120.0, 130.0, 125.0] + CONTENDED = [180.0, 1200.0, 1400.0] + + def test_a_quiet_host_is_measurable_and_a_contended_one_is_not(self): + # discriminating-skip-required: a refusal that fires on every host refuses nothing. + quiet_ok, quiet_why = spawn_noise.is_measurable( + spawn_noise.summarize_spawn_samples(self.QUIET, 0, 200) + ) + loud_ok, loud_why = spawn_noise.is_measurable( + spawn_noise.summarize_spawn_samples(self.CONTENDED, 0, 900) + ) + self.assertNotEqual( + quiet_ok, + loud_ok, + "both hosts produced the same verdict, so this test would pass whether or not " + "is_measurable looks at the samples at all", + ) + self.assertTrue(quiet_ok, quiet_why) + self.assertFalse(loud_ok, loud_why) + + def test_a_refusal_states_the_numbers_that_caused_it(self): + _, why = spawn_noise.is_measurable( + spawn_noise.summarize_spawn_samples(self.CONTENDED, 0, 900) + ) + # An unexplained refusal gets overridden reflexively, so the reason has to + # carry the evidence rather than just naming the finding. + self.assertIn("180.0", why) + self.assertIn("1400.0", why) + + def test_an_uncharacterized_host_is_refused_rather_than_assumed_fine(self): + ok, why = spawn_noise.is_measurable(spawn_noise.summarize_spawn_samples([], 0, None)) + self.assertFalse(ok) + self.assertIn("never characterized", why) + + +class TestPercentileFloor(unittest.TestCase): + """`1/(1-p)` is the only sample-count rule this repo can actually ground.""" + + def test_the_documented_floors(self): + self.assertEqual(spawn_noise.percentile_floor(0.5), 2) + self.assertEqual(spawn_noise.percentile_floor(0.95), 20) + self.assertEqual(spawn_noise.percentile_floor(0.99), 100) + + def test_a_percentile_outside_the_open_unit_interval_raises(self): + for bad in (0.0, 1.0, -0.1, 1.5): + with self.subTest(percentile=bad), self.assertRaises(ValueError): + spawn_noise.percentile_floor(bad) + + if __name__ == "__main__": unittest.main() diff --git a/plugins/performance/CHANGELOG.md b/plugins/performance/CHANGELOG.md index 0f1f69113..add6194f4 100644 --- a/plugins/performance/CHANGELOG.md +++ b/plugins/performance/CHANGELOG.md @@ -13,22 +13,22 @@ All notable changes to the `performance` plugin are documented here. Format foll verification harnesses in that session that each produced a confident WRONG answer rather than an error. Settled by the `/planning:interview` #3530 required; see `docs/topics/performance-plugin/PLAN.md`. -- **`target`** — identify and rank optimization candidates by evidence quality rather than +- **`target`**: identify and rank optimization candidates by evidence quality rather than suspicion. An unmeasured target makes "instrument this first" the recommendation, not a guess. -- **`goal`** — human-gated goal construction. Holds a realistic and an ideal target separately and +- **`goal`**: human-gated goal construction. Holds a realistic and an ideal target separately and computes the irreducible floor before any work, so a target below the floor is surfaced as unreachable-by-any-code-change up front. The source run asked for p50 <= 250 ms on a host charging 0.3-2.8 s per process spawn, and only discovered the goal was unreachable at the end. -- **`snapshot`** — baseline and post capture with the host qualified first. Repeated no-op spawns +- **`snapshot`**: baseline and post capture with the host qualified first. Repeated no-op spawns characterize the machine's own noise (via the `spawn_noise` lib shared with `claude-ops`), a drift-immune counter is reported alongside and ranked above any duration, arms are interleaved within one run rather than compared across two passes, and a wall-clock claim is refused from a host carrying the bimodal contention signature. -- **`verify`** — fresh-context adversarial re-derivation that does not inherit the implementer's +- **`verify`**: fresh-context adversarial re-derivation that does not inherit the implementer's numbers, plus a report that never rounds a miss into a win. -- **`reference/harness-integrity.md`** — the discipline the other skills apply: a harness must prove +- **`reference/harness-integrity.md`**: the discipline the other skills apply. A harness must prove it is not measuring itself, a probe must assert its own precondition and fail rather than silently degrade, and a discrimination check must verify its own patch applied and restore from saved bytes rather than from version control. -- **`lib/spawn_noise.py`** — byte-identical copy of the canonical `claude-ops` lib, registered as a +- **`lib/spawn_noise.py`**: a byte-identical copy of the canonical `claude-ops` lib, registered as a cross-plugin cluster with a dedicated sync gate so the bimodal threshold has exactly one home. diff --git a/plugins/performance/README.md b/plugins/performance/README.md index 220c0d944..8d7e4aa44 100644 --- a/plugins/performance/README.md +++ b/plugins/performance/README.md @@ -88,5 +88,5 @@ not support a rule, the skill says so rather than dressing a house choice as con ## Baselines Baselines live in the topic's memory tier (`.work//baselines/`), are machine-bound, and -are **never committed** — matching `/verification:measure`. A committed baseline is a number that +are **never committed**, matching `/verification:measure`. A committed baseline is a number that outlives the conditions that made it true. diff --git a/plugins/performance/lib/spawn_noise.py b/plugins/performance/lib/spawn_noise.py new file mode 100644 index 000000000..dfa608186 --- /dev/null +++ b/plugins/performance/lib/spawn_noise.py @@ -0,0 +1,184 @@ +"""Process-spawn noise characterization: is this host measurable at all? + +A single spawn timing is misleading because the floor itself moves with machine +load. The same no-op that costs ~120 ms on a drained box costs ~1,100 ms under +contention, and one Windows host was measured at min 180.5 ms / median 1107.7 ms +/ max 2841.3 ms across seven identical no-op spawns. A percentile taken from such +a host is not so much wrong as meaningless in isolation. + +So this module reduces repeated no-op spawns to a spread plus a set of findings, +and every reading it emits is labelled with the process count observed at sample +time. A reader cannot mistake a storm-state number for a baseline. + +`bimodal-spawn-latency` is the contention signature, and it is a TWO-PART +predicate on purpose: a wide spread whose slow mode is ALSO slow in absolute +terms. A wide ratio alone is not the signature, because on a healthy machine a +cold first spawn against a warm second one clears 3x while every sample is still +fast. Any consumer that re-derives a verdict from `spread_ratio` alone will +report contention on a healthy host. + +Python 3.11+, standard library only. Reports; never mutates. Never runs a +discovered hook, statusline command, or MCP server: those are third-party +commands with arbitrary side effects, so timing one by executing it would make +a caller a mutator. Only the no-op baseline below is ever spawned. +""" + +from __future__ import annotations + +import math +import statistics +import subprocess +import sys +import time + +#: Trivial no-op spawns, one per platform. Never a discovered hook or statusline command. +NOOP_SPAWN = { + "win32": ["cmd", "/c", "exit"], + "posix": ["/bin/sh", "-c", "exit 0"], +} +SPAWN_SAMPLES = 7 +#: A no-op spawn floored above this is already contended before any hook runs. +SLOW_SPAWN_FLOOR_MS = 500.0 +#: max/min at or above this across identical no-op spawns is the bimodal contention signature. +BIMODAL_SPREAD_RATIO = 3.0 +#: Default ceiling for one no-op spawn. A caller with its own budget passes it explicitly. +SPAWN_TIMEOUT_S = 20 + + +def summarize_spawn_samples( + durations_ms: list[float], timeouts: int, concurrent_processes: int | None +) -> dict: + """Reduce repeated no-op spawn timings to min/median/max plus a load label. + + A single spawn number is misleading because the floor itself moves with + machine load: the same no-op that costs ~120 ms on a drained box costs + ~1,100 ms under contention. Every reading here is labelled with the process + count observed at sample time so a reader cannot mistake a storm-state + number for a baseline. + """ + result: dict = { + "samples": len(durations_ms), + "timeouts": timeouts, + "concurrent_processes_at_sample": concurrent_processes, + "state_label": "as-sampled", + "findings": [], + } + if not durations_ms: + result["findings"].append("no-spawn-samples-captured") + return result + low, high = min(durations_ms), max(durations_ms) + result["min_ms"] = round(low, 1) + result["median_ms"] = round(statistics.median(durations_ms), 1) + result["max_ms"] = round(high, 1) + result["spread_ratio"] = round(high / low, 2) if low > 0 else None + if low > SLOW_SPAWN_FLOOR_MS: + result["findings"].append("slow-spawn-floor") + # A wide RATIO alone is not the contention signature: on a healthy machine a cold + # first spawn against a warm second one clears 3x while every sample is still fast. + # The signature is a wide spread whose slow mode is itself slow, so the absolute + # ceiling has to clear the floor threshold too. + if ( + result["spread_ratio"] is not None + and result["spread_ratio"] >= BIMODAL_SPREAD_RATIO + and high >= SLOW_SPAWN_FLOOR_MS + ): + result["findings"].append("bimodal-spawn-latency") + if timeouts: + result["findings"].append("spawn-probe-timed-out") + result["note"] = ( + "The floor moves with load, so compare these numbers only against another capture " + "carrying a similar concurrent_processes_at_sample. A bimodal spread across identical " + "no-op spawns IS the contention diagnosis." + ) + return result + + +def spawn_probe(samples: int = SPAWN_SAMPLES, timeout_s: float = SPAWN_TIMEOUT_S, + load_probe=None) -> dict: + """Time a trivial no-op spawn repeatedly. Never runs a discovered hook. + + This is the baseline every hook, statusline render, and subagent pays before + it does any work of its own. Timing an actual hook would mean executing a + third-party command with arbitrary side effects, which this engine will not do. + """ + command = NOOP_SPAWN["win32"] if sys.platform == "win32" else NOOP_SPAWN["posix"] + durations: list[float] = [] + timeouts = 0 + for _ in range(samples): + started = time.perf_counter() + try: + subprocess.run(command, capture_output=True, timeout=timeout_s) + except subprocess.TimeoutExpired: + timeouts += 1 + durations.append(timeout_s * 1000.0) + continue + except OSError: + break + durations.append((time.perf_counter() - started) * 1000.0) + load = load_probe() if load_probe else None + summary = summarize_spawn_samples(durations, timeouts, load) + summary["command"] = " ".join(command) + return summary + + +def is_measurable(summary: dict) -> tuple[bool, str]: + """Decide whether a WALL-CLOCK claim may be reported from this host. + + Returns `(measurable, reason)`. This function states a verdict and its basis; + it never suppresses anything itself, and the caller decides what a False means. + `performance` treats it as a hard refusal; `claude-ops` does not call it. + + The verdict rides on the `findings` list rather than on `spread_ratio`, + because `bimodal-spawn-latency` is the two-part predicate documented at module + level. A bare ratio comparison would fire on a healthy cold-then-warm host. + + A False is never the end of the road: a drift-immune counter (process spawns, + syscalls, queries) is still reportable from a host too noisy for a duration, + and that is what the source run of this workflow ultimately shipped -- a + deterministic 4 -> 1 spawn count, not a millisecond figure that no independent + verifier could reproduce an hour later on the same machine. + + Refusing above a variance threshold is stricter than the benchmarking field: + pyperf, Criterion, JMH and benchstat all WARN and print the number anyway. This + is a deliberate house rule, not a consensus practice, and callers must present + it as one. + """ + findings = summary.get("findings", []) + if "no-spawn-samples-captured" in findings: + return False, "no spawn samples were captured, so the host was never characterized" + if "bimodal-spawn-latency" in findings: + return False, ( + f"spawn cost spread {summary.get('spread_ratio')}x across identical no-op spawns " + f"(min {summary.get('min_ms')} ms, max {summary.get('max_ms')} ms), with the slow mode " + f"above the {SLOW_SPAWN_FLOOR_MS} ms floor: the bimodal contention signature" + ) + if "spawn-probe-timed-out" in findings: + return False, "at least one no-op spawn hit the probe timeout, so the tail is unbounded" + if "slow-spawn-floor" in findings: + return False, ( + f"even the fastest no-op spawn cost {summary.get('min_ms')} ms, above the " + f"{SLOW_SPAWN_FLOOR_MS} ms floor: the host is contended before any work starts" + ) + return True, ( + f"spawn cost spread {summary.get('spread_ratio')}x with a {summary.get('min_ms')} ms " + "floor: within the measurable band" + ) + + +def percentile_floor(percentile: float) -> int: + """Minimum samples for a percentile to be arithmetically expressible: `1/(1-p)`. + + This is the ONLY grounded constraint on sample count that this codebase is + aware of, and it is grounded because it is derivable, not because a source + states it. The research pass behind #3530 found no benchmarking-community + consensus figure for "enough samples to make a percentile meaningful": the + familiar folklore numbers appear in no first-party or peer-reviewed source. + + So `performance` enforces this floor and labels its p50/p95-over-20 default as + a house convention. p95 needs 20 samples; p99 needs 100. Below the floor the + percentile is not merely imprecise, it cannot be computed from the data at all + -- the requested tail does not exist in the sample. + """ + if not 0.0 < percentile < 1.0: + raise ValueError(f"percentile must be in (0, 1), got {percentile!r}") + return math.ceil(1.0 / (1.0 - percentile)) diff --git a/plugins/performance/reference/harness-integrity.md b/plugins/performance/reference/harness-integrity.md new file mode 100644 index 000000000..0a8306235 --- /dev/null +++ b/plugins/performance/reference/harness-integrity.md @@ -0,0 +1,121 @@ +# Harness integrity + +The rules a measurement harness must satisfy before any number it produces may be reported. + +Read this before writing a benchmark, a probe, or a check that a gate works. `/performance:snapshot` +and `/performance:verify` both apply it; it is not a standalone skill because it is a discipline +applied inside measurement, not a thing you invoke on its own. + +## Why this file is the first thing in this plugin + +One session, optimizing a single hook, produced **five verification harnesses that each returned a +confident wrong answer rather than an error**. Every one was caught only because something explicitly +re-checked it. Four of the five were checks written *specifically to avoid being fooled*: the +meta-checks were less reliable than the thing they were checking. + +| # | Harness | Reported | Actually did | +|---|---|---|---| +| 1 | spawn census via a `PATH` shim | "no improvement" | `mktemp -d` put a fresh directory on `PATH` every run, and the subject cached keyed on `PATH`. Every run was a forced cache miss. It measured its own randomization. | +| 2 | hard-link identity probe | "0 divergences" | `os.link` failed cross-volume on Windows and fell back to `shutil.copyfile`. A copy is a different file, so the probe reported a green result for a case it never exercised. | +| 3 | discrimination check (shell) | "NOT DISCRIMINATING" | A `D:/...` path handed to bash resolves nowhere under MSYS. Both arms exited 127 and the grep found nothing in either. | +| 4 | discrimination check (repeat) | "NOT DISCRIMINATING" | Same trap, a second harness. Fixing the path form immediately showed FAIL-without / PASS-with. | +| 5 | discrimination check (python) | "NOT DISCRIMINATING" | Restored via `git checkout --` while the fix under test was **uncommitted**. The restore silently reverted the fix, so the "with fix" arm ran without it, and the work was destroyed. | + +None of these are knowledge gaps. They are all "the measurement was wrong in a way that looked +right". A workflow that measures without enforcing the rules below mostly generates confident +numbers, which is worse than generating none. + +## The rules + +### 1. A harness must prove it is not measuring itself + +Anything the harness injects into the environment under test (`PATH` entries, temp directories, +environment variables, working directory) is either **fixed across runs** or **provably irrelevant +to the subject's behavior**. + +Cases 1 and 3/4 above are the same root cause seen twice: the harness changed `PATH` in a way that +mattered to the subject. + +Check it by running the harness twice against an **unchanged** subject. If the two runs disagree +beyond the host's characterized noise, the harness is a variable, not an instrument. + +### 2. A probe must assert its own precondition + +If a test depends on a hard link, a symlink, a particular filesystem, a permission, or a binary being +present, it must **FAIL when that precondition is unmet**. It must never silently degrade into a +weaker test that passes. + +Case 2 is the canonical shape: a `try: os.link / except: shutil.copyfile` fallback turned a +link-identity probe into a probe of something else entirely, and reported success. + +A skip is acceptable **only** when the skipped branch is not the point of the test. A skip that +vacates the only discriminating assertion is a false green. This repo gates that shape mechanically +in `scripts/check-discriminating-test-skips.sh`; annotate a load-bearing branch with +`# discriminating-skip-required:` so a later `skip_case` there is refused. + +### 3. A discrimination check must verify its own patch applied + +A check that a gate works has two arms: **without** the fix it must fail, **with** the fix it must +pass. Both arms failing identically is the most common failure mode, and it reads as "not +discriminating" when it actually means "the harness never ran". + +So assert three things, not two: + +1. the negative arm produces the failing outcome, +2. the positive arm produces the passing outcome, and +3. **the two arms produced different output.** + +Point 3 is the one that catches cases 3, 4 and 5. Without it, a harness where both arms exit 127 +reports a clean, confident, wrong verdict. + +Assert that the patch changed something before running the arm. A patch that silently applied +nothing is arm 3's failure mode. + +### 4. Restore from saved bytes, not from version control + +Take an in-memory or on-disk copy of the file **before** editing it, and restore from that copy. + +`git checkout --` is correct only when the code under test is already committed, and is **actively +destructive** otherwise. Case 5 destroyed the work it was verifying. + +Verify the restore rather than assuming it: an empty `git diff` against the commit, or a byte +comparison against the saved copy. "I restored it" is not evidence. + +### 5. Commit before you verify + +This makes the restore path safe and makes an accidental clobber recoverable. It is the cheapest +mitigation for rule 4 and costs nothing. + +### 6. Windows drive-letter paths are a first-class hazard + +Three of the five cases involve `D:/...` or backslash handling, and an independent verifier hit the +same trap on its own harness. + +- A `D:/...` path handed to `bash` under MSYS resolves nowhere. Use the POSIX form (`/d/...`). +- Windows `PATH` entries in drive-letter form break bash's colon-separated parsing, because the colon + after the drive letter is read as a separator. +- `os.link` fails across volumes; `shutil.copyfile` does not, which is exactly what makes the + fallback dangerous. + +On a mixed MSYS/native host this is not an edge case. It is the default hazard. + +## Two more traps, from the same session + +- **`$(...)` command substitution is a process spawn on MSYS.** A "builtins-only" hot path that + reports via stdout still costs a full process. A spawn-count harness that ignores its own + substitutions undercounts. +- **`${var: -N}` returns the empty string when the string is shorter than N** in bash. This silently + collapsed a per-plugin cache key onto one shared file, which a harness would read as a cache that + works. + +## Checklist + +Before reporting any number: + +- [ ] The harness injects nothing into the subject's environment that varies between runs. +- [ ] Two runs against an unchanged subject agree within the host's characterized noise. +- [ ] Every precondition the probe depends on is asserted, and fails rather than degrading. +- [ ] Any discrimination check asserts that its two arms **differ**. +- [ ] The code under test was committed before the check ran. +- [ ] Restores came from saved bytes, and the restore was verified. +- [ ] Every path handed to a shell is in that shell's own path form. diff --git a/plugins/performance/skills/goal/SKILL.md b/plugins/performance/skills/goal/SKILL.md new file mode 100644 index 000000000..2fce042bf --- /dev/null +++ b/plugins/performance/skills/goal/SKILL.md @@ -0,0 +1,130 @@ +--- +description: "Construct a performance goal the data can actually settle: the metric and the exact command that produces it, a REALISTIC target and an IDEAL target held separately, and the irreducible FLOOR computed BEFORE any work. Surfaces 'your target is below the measured floor, no code change can reach it' up front and makes the human decide, instead of silently failing the goal at the end. Human-gated always: this is the one phase that may never run unattended. Use when: 'set a performance target', 'how fast should this be', 'what is a realistic goal', 'is this target achievable', 'define done for this optimization', 'what is the floor here'. Runs after /performance:target and before /performance:snapshot. Skip when the work is exploratory with no claim to defend, or when the target is a correctness fix that happens to also be faster." +user-invocable: true +argument-hint: "[] (e.g. /performance:goal the destructive-guard PreToolUse hook)" +disable-model-invocation: false +metadata: + workflow-stage: plan + summary: Build a goal with realistic and ideal targets plus a computed floor +--- + +## Purpose + +Answers **"what would count as done, and is it reachable at all?"** + +The failure this prevents: the source run behind this plugin set a goal of p50 <= 250 ms for a hook +on a host that charged 0.3-2.8 s for a single irreducible process spawn. The goal was **unreachable +by any code change**, and that was discovered at the end, after the work. Knowing it up front would +have reframed the entire task from "make it fast" to "remove the spawn or accept the floor". + +## Human-gated, always + +This phase requires the user. `/performance:snapshot` and `/performance:verify` may run unattended; +this one may not, under any autonomy setting. + +The reason is specific: computing the floor routinely produces a verdict the user has to act on +("this target is unreachable"), and choosing between a reframed goal, a different target, and +accepting the floor is a judgment about what the work is for. An agent resolving that on the user's +behalf converts a surfaced constraint back into a silent one. + +If the user is unavailable, **stop and say what is blocked**. Do not pick a target and proceed. + +## What a goal must contain + +### 1. The metric, and the exact command that produces it + +Not "latency". The literal command, its arguments, and the field of its output that is the number. +A metric nobody can re-run is not a metric. + +Name the **drift-immune counter** alongside it (spawns, syscalls, queries, allocations, round trips) +and rank the counter above the duration. On a host that cannot support a wall-clock claim, the +counter is what survives; in the source run the durable result was a spawn census of 4 -> 1, and the +milliseconds were not reproducible by an independent verifier on the same machine an hour later. + +### 2. The floor, computed before any work + +The irreducible cost this target cannot go below whatever the code does. Compute it by measuring the +cheapest possible version of the operation: the empty hook, the no-op spawn, the single round trip, +the query returning one row. + +`lib/spawn_noise.py`'s `spawn_probe()` gives the process-spawn floor for this host directly. + +Then compare: + +- **target > floor**: proceed. +- **target close to floor**: the goal is reachable only by removing the irreducible operation, not + by making it faster. Say that explicitly; it is a different piece of work. +- **target < floor**: **STOP and surface it.** No code change reaches this target. The user + decides: reframe the goal, change the target, remove the operation, or accept the floor. + +### 3. Two targets, held separately + +- **Realistic**: what this change is expected to achieve, given the floor and the measured baseline. +- **Ideal**: what the operation would cost with no incidental overhead at all. + +Both are recorded. A single target collapses "did we succeed" and "how much is left" into one number +and loses the second. + +### 4. What counts as done + +Including whether merge is in scope, and whether a behavior change disqualifies the result. A +correctness regression outranks any speedup and is reported separately, never folded into the +performance claim. + +## Percentiles and sample count + +Default: **p50 and p95 over at least 20 samples**, alongside the counter. + +State plainly that this is a **house convention, not field consensus**: + +- The pattern "a median plus a high-order percentile" is grounded. Google's SRE Book (ch. 4) frames + the high-order percentile as the "plausible worst case" and the median as the "typical case". But + the percentiles that chapter names are the **99th and 99.9th**; p95 is convention. +- **No benchmarking-community sample count exists** for what makes a percentile meaningful. The only + real constraint is arithmetic: a percentile `p` needs at least `1/(1-p)` samples to be expressible + at all. p95 needs 20; p99 needs 100. `percentile_floor()` in `lib/spawn_noise.py` computes it, and + that floor **is** enforced. +- Do not cite coordinated omission (Gil Tene) to justify percentiles here unless the harness is a + load generator. It is a load-generator problem; citing it for a synchronous harness that measures + every operation miscites the field's best-known source. + +If the user wants p99, say what it costs: 100 samples on a host where one spawn can take 2.8 s. + +## Output + +Write the goal into the topic's `PLAN.md`, and keep baselines in the memory tier +(`.work//baselines/`, machine-bound, never committed) per `/verification:measure`. + +```text +Metric: -> +Counter: [ranked above the duration] +Floor: (measured by: ) +Realistic: Ideal: +Percentiles: p50, p95 over N>=20 [house convention; floor 1/(1-p) enforced] +Done when: +Evidence tier of the target: +``` + +## Boundary + +- **Does not measure the baseline.** That is `/performance:snapshot`. This phase measures only the + floor, because the floor is an input to the goal rather than a result of it. +- **Does not implement.** The change is `/implementation:implement`. +- **Does not store baselines.** `/verification:measure` owns baseline capture and storage; this + plugin depends on it rather than reimplementing it. + +## Next + +`/performance:snapshot baseline`. + +## Gotchas + +- **Compute the floor before agreeing the target, not after.** This is the entire point. A goal + agreed first and floored second is the failure that produced this skill. +- **The floor is a property of the host, not of the code.** Re-measure it on a different machine; + never carry a floor across hosts. +- **"Faster" is not a metric.** If the user cannot name the command, the goal is not yet a goal. +- **An ideal target is not a stretch goal.** It is the no-incidental-overhead cost, used to say how + much room is left after a realistic win. +- **A goal built on an E3/E4 candidate must record that.** Optimizing an unmeasured target can + succeed against its own metric and change nothing a user perceives. diff --git a/plugins/performance/skills/snapshot/SKILL.md b/plugins/performance/skills/snapshot/SKILL.md new file mode 100644 index 000000000..5a1da5574 --- /dev/null +++ b/plugins/performance/skills/snapshot/SKILL.md @@ -0,0 +1,148 @@ +--- +description: "Capture a baseline or post-change performance snapshot with the HOST QUALIFIED FIRST: repeated no-op spawns characterize the machine's own noise, and a wall-clock claim is REFUSED outright from a host carrying the bimodal contention signature, naming the drift-immune counter it can still report instead. Interleaves before/after arms within one run rather than comparing two passes, since a host that drifts 6x in an hour attributes its own drift to the change. Reports a counter alongside and ranked above any duration. Use when: 'capture a baseline', 'measure this before I change it', 'take a post snapshot', 'is it actually faster', 'run the A/B', 'benchmark this change', 'how noisy is this machine', 'can I even measure here'. Runs after /performance:goal; hands off to /performance:verify. Skip when no goal with a computed floor exists yet (run /performance:goal), or when the claim is about code shape rather than runtime (that is /verification:measure metrics)." +user-invocable: true +argument-hint: "[baseline|post] [] (e.g. /performance:snapshot baseline, /performance:snapshot post)" +disable-model-invocation: false +metadata: + workflow-stage: verify + summary: Capture a snapshot only from a host proven measurable +--- + +## Purpose + +Answers **"what does this cost, and is this machine even able to tell me?"** + +Read [`${CLAUDE_PLUGIN_ROOT}/reference/harness-integrity.md`](${CLAUDE_PLUGIN_ROOT}/reference/harness-integrity.md) before writing any harness +here. It is not optional background: five harnesses in the source run returned confident wrong +answers, and four of the five were checks written specifically to avoid being fooled. + +## Phase order, and why it is this order + +### 1. Qualify the host, before measuring anything + +```python +from spawn_noise import spawn_probe, is_measurable +summary = spawn_probe() +measurable, why = is_measurable(summary) +``` + +`is_measurable()` returns a verdict and its basis. A `False` is a **hard refusal to report a +wall-clock number**, subject only to the recorded override below. + +The refusal names what it can still report. That matters: an unexplained refusal gets overridden +reflexively. This host spread 15.7x across identical no-op spawns, and the durable result from the +source run was a deterministic spawn count of 4 -> 1, not a duration. + +**Say plainly that this refusal is a house rule.** No surveyed benchmarking tool refuses above a +variance threshold: pyperf, Criterion, JMH and benchstat all warn and print the number anyway. +pyperf's own thresholds (stdev >= 10% of the mean, min/max >= 50% from the mean, shortest value +< 1 ms) are warnings. Presenting this refusal as consensus would be a miscitation. + +### 2. Capture the drift-immune counter first + +Spawns, syscalls, queries, allocations, round trips. The counter is the headline; the duration is +context. A counter also catches harness bugs immediately, because a counter that does not move when +it should is an unambiguous signal, while a duration that does not move is ambiguous. + +Re-measure the counter after **every** change. Both self-inflicted harness bugs in the source run +surfaced first as a counter that failed to move. + +### 3. Capture durations, only if step 1 allowed it + +p50 and p95 over at least 20 samples, per the goal. Enforce the arithmetic floor: a percentile `p` +needs `1/(1-p)` samples to be expressible at all (`percentile_floor()` in `lib/spawn_noise.py`). +Report **no** percentile the sample count cannot support; report the raw samples instead. + +Never a single sample. Never a bare mean. + +## Comparing before and after + +**Never compare two separate passes on a drifting host.** A bare `bash -c true` measured 1825 ms and +283 ms in the same hour at ~10% CPU. Any two-pass comparison attributes that 6x to the change. + +Two valid modes: + +### Sequential interleaving (default) + +Alternate arms within one run, flipping the order each iteration. Report the median of per-pair +ratios alongside per-arm percentiles. + +Grounded, Tier 1, `benchstat`'s own documentation: *"The best way to do this is to interleave before +and after runs, rather than running, say, 10 iterations of the before benchmark, and then 10 +iterations of the after benchmark."* + +**Under uncontrolled concurrent load, suppress the paired ratio** and report per-arm percentiles +only. The arms are no longer load-matched, and pairing by index compares samples that never shared +conditions. + +Do not describe this as "paired statistics, per benchstat". `benchstat` recommends interleaved +*collection* and then analyzes with the **Mann-Whitney U test**, which is an independent two-sample +test. Its `-delta-test` flag no longer exists. + +### Simultaneous duet (for a genuinely shared machine) + +Run both arms **at the same time** and report only their relative performance. + +Grounded, Tier 2: Bulej, Horký, Tůma, Farquet & Prokopec, ["Duet Benchmarking: Improving Measurement +Accuracy in the Cloud"](https://arxiv.org/abs/2001.05811) (ICPE 2020) measured accuracy improvements +of **5.03x** (ScalaBench/DaCapo) and **37.4x** (SPEC CPU 2017) on shared machines from running arms +in parallel, because both arms absorb the same interference. + +This is the reverse of the sequential rule and it is not a contradiction: sequential interleaving is +vulnerable to bursty load precisely because the arms run at different moments. **The reconciliation +"only the sequential form is vulnerable" is this plugin's reading, not a sourced claim.** Duet costs +2x the resources and needs the arms to be genuinely independent. + +## Warmup + +Discard N warmup iterations if the target has a warm path. Do **not** claim this establishes steady +state: Barrett et al. (OOPSLA 2017) found *"at most 43.5% of ⟨VM, benchmark⟩ pairs consistently +reach a steady state of peak performance."* No source justifies any particular N. + +## The override + +Gates here hard-block. A named per-gate override exists, and using it **records itself in the +report**: + +```text +OVERRIDE: unmeasurable-host reason: gate: is_measurable +``` + +An override without a recorded reason is not available. A report carrying an override says so at the +top, not in a footnote. + +## Storage + +Baselines live in the memory tier, `.work//baselines/`, machine-bound, **never +committed**, matching `/verification:measure`, which owns baseline capture and storage mechanics. +This skill depends on it rather than reimplementing it. + +A committed baseline is a number that outlives the conditions that made it true. No source states +"a stored baseline is invalid on another machine" outright, but four independent Tier 1/2 strands +converge on it; the practical rule is to always re-measure both arms rather than compare against a +stored one. + +## Boundary + +- **Does not own baseline/compare mechanics.** `/verification:measure` does. This adds host + qualification, interleaving, counters, and the refusal. +- **Does not implement the change.** `/implementation:implement` does. +- **Does not decide whether the goal was met.** That is `/performance:verify`. + +## Next + +`/performance:verify`. + +## Gotchas + +- **The refusal must always name the counter it can still report.** A dead-end refusal gets + overridden reflexively and teaches nothing. +- **`is_measurable()` reads the findings list, never `spread_ratio` alone.** The bimodal predicate is + two-part: a wide spread whose slow mode is *also* slow. A bare ratio check fires on a healthy + cold-then-warm host. +- **`$(...)` is a process spawn on MSYS.** A "builtins-only" hot path that reports through stdout + still costs a full process, and a spawn census that ignores its own substitutions undercounts. +- **A `PATH` shim directory from `mktemp -d` invalidates a `PATH`-keyed cache every run.** That + harness measured its own randomization and reported "no improvement". +- **Report the counter even when the duration is allowed.** The counter is what an independent + verifier can reproduce tomorrow. diff --git a/plugins/performance/skills/snapshot/evals/evals.json b/plugins/performance/skills/snapshot/evals/evals.json new file mode 100644 index 000000000..66ef873dd --- /dev/null +++ b/plugins/performance/skills/snapshot/evals/evals.json @@ -0,0 +1,67 @@ +{ + "skill_name": "snapshot", + "evals": [ + { + "id": 1, + "name": "refuses-a-wall-clock-claim-from-an-unmeasurable-host", + "prompt": "/performance:snapshot post — the host qualification returned min 180.5 ms, median 1107.7 ms, max 2841.3 ms across seven identical no-op spawns, spread ratio 15.74. Give me the before/after latency numbers.", + "expected_output": "REFUSES to report a wall-clock claim. Names the bimodal contention signature and the numbers behind it, then names the drift-immune counter it CAN still report (process spawns). Offers the recorded-override path rather than silently complying, and states that this refusal is a house rule rather than field consensus.", + "files": [], + "expectations": [ + "Does NOT produce before/after millisecond figures for this host", + "Names the drift-immune counter it can still report instead — the refusal is not a dead end", + "States the refusal is stricter than the benchmarking field, where tools warn and print anyway", + "Any override is explicit, human-supplied, and recorded in the report rather than assumed" + ] + }, + { + "id": 2, + "name": "reports-normally-from-a-quiet-host", + "prompt": "/performance:snapshot post — the host qualification returned min 120 ms, median 125 ms, max 130 ms across seven identical no-op spawns. Give me the before/after numbers.", + "expected_output": "Proceeds and reports. Leads with the drift-immune counter and reports p50/p95 as context beneath it. Does not refuse: this host carries no contention signature. The negative arm of the refusal gate — a skill that refused here would be refusing on every host and therefore refusing nothing.", + "files": [], + "expectations": [ + "Does NOT refuse; this host is within the measurable band", + "Ranks the drift-immune counter above the duration in the report", + "Reports p50 and p95 rather than a single sample or a bare mean" + ] + }, + { + "id": 3, + "name": "refuses-a-percentile-the-sample-count-cannot-support", + "prompt": "/performance:snapshot post — I have 12 samples and I want the p99.", + "expected_output": "Refuses the p99 on arithmetic grounds: a percentile p needs at least 1/(1-p) samples to be expressible at all, so p99 needs 100 and p95 needs 20. Twelve samples support neither. Offers the raw samples or more sampling, and distinguishes this enforced arithmetic floor from the p50/p95-over-20 default, which is a labelled house convention with no community grounding.", + "files": [], + "expectations": [ + "Refuses p99 at 12 samples and cites the 1/(1-p) floor as the reason", + "Separates the ENFORCED arithmetic floor from the UNGROUNDED house convention for sample count", + "Offers raw samples or additional sampling rather than reporting an unsupportable percentile" + ] + }, + { + "id": 4, + "name": "interleaves-rather-than-comparing-two-passes", + "prompt": "/performance:snapshot post — I measured the old version 20 times this morning and the new version 20 times just now. Compare them.", + "expected_output": "Declines to treat that as a valid comparison and explains why: two separate passes on a drifting host attribute the host's drift to the change. Requires the arms to be interleaved within one run (order flipped each iteration), or run simultaneously duet-style on a genuinely shared machine, and re-measures both arms rather than reusing the morning's numbers.", + "files": [], + "expectations": [ + "Rejects the two-pass comparison and names host drift as the reason", + "Requires interleaving within one run, or simultaneous duet execution", + "Re-measures BOTH arms rather than comparing new measurements against stale ones", + "Does not claim benchstat uses paired statistics — it recommends interleaved collection but analyzes with an independent two-sample test" + ] + }, + { + "id": 5, + "name": "catches-a-harness-that-measures-itself", + "prompt": "/performance:snapshot baseline — my spawn census puts a shim directory from `mktemp -d` on PATH and the subject caches its interpreter lookup keyed on PATH. The census says there was no improvement.", + "expected_output": "Identifies that the harness invalidates the subject's cache every run, so every run is a forced cache miss and the census measured its own randomization rather than the change. Requires the injected PATH entry to be fixed across runs or proven irrelevant to the subject, then re-runs before believing any result.", + "files": [], + "expectations": [ + "Identifies the mktemp shim directory as the cause, not the change under test", + "States the rule: anything the harness injects into the environment is fixed across runs or provably irrelevant", + "Does not accept the 'no improvement' verdict at face value" + ] + } + ] +} diff --git a/plugins/performance/skills/target/SKILL.md b/plugins/performance/skills/target/SKILL.md new file mode 100644 index 000000000..daa446cae --- /dev/null +++ b/plugins/performance/skills/target/SKILL.md @@ -0,0 +1,110 @@ +--- +description: "Identify and rank optimization targets by EVIDENCE QUALITY rather than by suspicion, so an unmeasured system yields 'instrument this first' instead of a guess. Accepts targets from the current session's own pain, a named path or component, a telemetry store, or an open-ended 'what is slow here'. Ranks each candidate by how well its cost is actually attributed, names the drift-immune counter that would settle it, and refuses to rank an unmeasured candidate above a measured one however plausible its mechanism. Use when: 'what should we optimize', 'what is slow here', 'find the bottleneck', 'where is the time going', 'this feels slow', 'pick a performance target', 'is X worth optimizing'. Entry point for the measurement-first optimization workflow; hands off to /performance:goal. Skip when the target is already chosen and measured (go straight to /performance:goal), or when a specific failure needs root-causing rather than a candidate ranking (that is debugging)." +user-invocable: true +argument-hint: "[] (e.g. /performance:target plugins/disk-hygiene/hooks)" +disable-model-invocation: false +metadata: + workflow-stage: discovery + summary: Rank optimization candidates by evidence quality, not suspicion +--- + +## Purpose + +Answers **"what should we optimize, and how much do we actually know about it?"** + +The failure this prevents is picking a target because its mechanism sounds expensive. In the source +run behind this plugin, a parallel session diagnosed WDAC code-integrity enforcement as the cause of +slow process spawns. The mechanism was real and the policy was genuinely enabled. It was still the +wrong answer, and had to be retracted: a spread of min 180.5 ms / median 1107.7 ms / max 2841.3 ms +across *identical* no-op spawns is a contention signature, because a fixed policy check cannot +produce a 15x spread. **The bimodality was the diagnosis; the plausible mechanism was a +distraction.** + +So this skill ranks by evidence, and says so when there is none. + +## Inputs it accepts + +| Source | What to do | +|---|---| +| The current session's own pain | Name the operation that felt slow and what was observed. Anecdote is a valid *candidate source* and an invalid *ranking basis*. | +| A named path or component | Enumerate the layers it spans before choosing one (see "Measure the layers first"). | +| A telemetry store | `/claude-ops:observability` for Claude Code's own; otherwise the project's. Prefer it over every other source. | +| Open-ended "what is slow here" | Widest scope, weakest evidence. Expect the output to be "instrument this first". | + +## Evidence tiers + +Rank every candidate into exactly one. **A lower tier never outranks a higher one**, regardless of +how compelling the mechanism sounds. + +| Tier | Means | Example | +|---|---|---| +| **E1, attributed measurement** | A measurement that isolates this component's cost from its neighbours' | A spawn census showing this hook costs 4 of the 7 spawns per tool call | +| **E2, aggregate measurement** | A real measurement that includes this component but does not isolate it | "The whole pre-tool path takes 1.2 s" | +| **E3, structural inference** | No measurement; a documented cost model predicts expense | "This is a 125-line shell wrapper that runs per tool call" | +| **E4, suspicion** | A plausible mechanism and nothing else | "WDAC is probably slowing spawns" | + +**Nothing above E3 exists means the top recommendation is "instrument this first"**, naming the +cheapest instrument that would reach E2. That IS the answer; do not substitute a ranked guess. + +## Measure the layers before choosing one + +When a candidate spans layers (a shell wrapper around a Python program; a route through an ORM +through a driver), attribute cost *across the layers* before picking one to optimize. + +The source run's first instinct was to optimize a 1903-line Python guard body. Measurement showed +the 125-line shell wrapper around it was roughly 88% of the cost and the Python was not the +bottleneck. Layer attribution is cheap and reorders the candidate list. + +## Name the counter, not just the duration + +For each ranked candidate, name the **drift-immune counter** that would settle it: process spawns, +syscalls, queries, allocations, bytes, round trips. A counter is reproducible on a host whose wall +clock is not. + +If no counter exists for a candidate, say so explicitly. That is a real property of the target and +it changes what `/performance:goal` can promise. + +Grounding: the counts-over-wall-clock rationale is stated in the literature for **instruction +counts** specifically (Valgrind's Cachegrind manual; Iai). Extending it to spawns, syscalls and +queries is this plugin's own generalization, not a sourced claim, and Valgrind's own manual argues +the other side too: execution time "is a better metric than instruction counts because it's what +users perceive". Present a counter as *reproducible*, never as *more truthful*. + +## Output + +A ranked table, highest evidence tier first: + +| Rank | Candidate | Tier | What is known | Counter that would settle it | Cheapest next instrument | +|---|---|---|---|---|---| + +Then one line naming the recommended target and the tier it rests on. If that tier is E3 or E4, the +recommendation is to instrument, not to optimize. + +## Boundary + +- **Does not set a goal or a target number.** That is `/performance:goal`, which is human-gated + because it needs the floor computed and the realistic/ideal split agreed. +- **Does not measure.** It ranks what is known and names what is missing; + `/performance:snapshot` captures. +- **Does not root-cause an observed failure.** A specific broken or slow behavior with a + reproduction is a debugging task, not a candidate ranking. +- **Does not diagnose a slow Claude Code installation.** That is + `/claude-ops:audit-performance`, which this skill consumes as a telemetry source rather than + duplicating. + +## Next + +`/performance:goal `. Carry the evidence tier forward: a goal built on an E3 +candidate must say so. + +## Gotchas + +- **A plausible mechanism is not evidence.** The retracted WDAC diagnosis above is the worked + example. Ask what the mechanism predicts, then check whether the data shows it. A fixed cost cannot + produce a variable spread. +- **Anecdote from this session is a candidate source, not a ranking basis.** "It felt slow" gets a + candidate onto the list at E4 and no higher. +- **The biggest file is not the bottleneck.** Line count is not a cost model. Attribute across the + layers before believing size. +- **A target with no drift-immune counter is a harder target**, not an equal one. Say so here rather + than discovering it in `/performance:snapshot` when a wall-clock claim gets refused. diff --git a/plugins/performance/skills/verify/SKILL.md b/plugins/performance/skills/verify/SKILL.md new file mode 100644 index 000000000..fc3a3d63a --- /dev/null +++ b/plugins/performance/skills/verify/SKILL.md @@ -0,0 +1,98 @@ +--- +description: "Re-derive a performance result in a FRESH CONTEXT that does not inherit the implementer's numbers, then report the target as met or not met without rounding a miss into a win. Dispatches a verifier told to distrust the reported figures and reproduce them from the trees, checks that the change did not alter behavior via a differential covering every MODE the subject runs in, and states any behavior change separately from and above the performance claim. Use when: 'verify this speedup', 'did the optimization actually work', 'check my benchmark numbers', 'independent verification', 'is this result real', 'write up the performance result', 'double-check before I claim this'. Final phase; runs after /performance:snapshot post. Skip when no baseline exists (there is nothing to verify), or when reviewing a diff for general quality rather than checking a measured claim." +user-invocable: true +argument-hint: "[] (e.g. /performance:verify the 4-to-1 spawn reduction)" +disable-model-invocation: false +metadata: + workflow-stage: verify + summary: Re-derive the result in fresh context and report it honestly +--- + +## Purpose + +Answers **"is this result real, and what does it actually say?"** + +In the source run behind this plugin, this phase caught **two blocking correctness defects that the +implementer, the implementer's own 143-test suite, and a full green CI run had all missed.** That is +why it is a separate phase from measurement and not a step inside it. + +Read [`${CLAUDE_PLUGIN_ROOT}/reference/harness-integrity.md`](${CLAUDE_PLUGIN_ROOT}/reference/harness-integrity.md) first. + +## 1. Fresh context, and adversarial by construction + +Dispatch a verifier that **does not inherit the implementer's numbers**. Give it the trees and the +claim; withhold the reasoning that produced the figures. A verifier shown the expected answer +verifies the answer, not the work. + +The brief should say, in substance: distrust the reported numbers, re-derive them yourself, and +report what you actually observe including the ways you could not reproduce it. + +Two independent verifiers found different defects in the source run. One is the floor, not the +target. + +## 2. Prove behavior did not change, with a differential + +**A passing test suite is not a behavior proof.** It proves nothing asserted broke. It does not prove +behavior is unchanged, because it only checks what someone thought to assert. + +Run a differential: the pre-change and post-change subject over a harvested corpus of real inputs, +requiring **byte-identical output**. + +**Cover every MODE the subject runs in.** The source run's differential covered one of two modes and +missed a real deny -> ask downgrade in the other. Enumerate the modes first and record which the +differential actually exercised; an unexercised mode is an unverified mode, and it is reported as +such rather than assumed fine. + +## 3. Check the harness before believing the result + +Every gate in the harness-integrity checklist. In particular, for any discrimination +check involved, confirm it asserts that its **two arms differ**, not merely that each produced its +expected string. Four of five harnesses in the source run failed by exiting identically in both arms +and reporting a confident verdict. + +## 4. Report + +State the target as **met** or **not met**, with the measurement that explains why. + +```text +Target: / Floor: +Counter: -> [headline] +Duration: -> [or: REFUSED, ] +Verdict: MET | NOT MET | UNMEASURABLE +Behavior: UNCHANGED (differential: N inputs, modes covered: ) + | CHANGED: [ranked above the performance claim] +Overrides: +Reproduced by an independent verifier: yes/no, and what diverged +``` + +Rules that bind the report: + +- **Never round a miss into a win.** A target missed by 8% is not met. +- **A correctness regression outranks any speedup** and is stated separately, above the performance + claim, never folded into it. +- **The counter is the headline; the duration is context.** On a host that failed + `is_measurable()`, there is no duration line at all, only the refusal and its reason. +- **An unexercised mode is reported, not omitted.** +- **Say which claims rest on the plugin's own conventions** rather than on sourced practice: the + p50/p95-over-20 default, the refusal threshold, and counts-over-wall-clock for anything other than + instruction counts. + +## Boundary + +- **Does not measure.** `/performance:snapshot` captures; this re-derives and reports. +- **Does not review the diff for general quality.** That is the review lane. This checks one measured + claim. +- **Does not merge.** Under any autonomy setting this plugin may open a PR and never merge one. + +## Gotchas + +- **A green CI run is not verification.** It was green in the source run while two blocking defects + were live. +- **A verifier that inherits the numbers is not independent.** Withhold the reasoning, not just the + conclusion. +- **`git checkout --` is not a restore mechanism** when the code under test is uncommitted. It + silently reverts the fix and destroys the work. Restore from saved bytes, and verify the restore. +- **"The tests pass" answers a different question than "behavior is unchanged".** Only a differential + over real inputs answers the second, and only for the modes it ran. +- **UNMEASURABLE is a legitimate, complete verdict.** It is not a failure of the work; reporting a + number the host cannot support would be. diff --git a/plugins/verification/.claude-plugin/plugin.json b/plugins/verification/.claude-plugin/plugin.json index 2fca01741..653248e04 100644 --- a/plugins/verification/.claude-plugin/plugin.json +++ b/plugins/verification/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "verification", - "version": "0.6.0", + "version": "0.6.1", "description": "Outcome-verification stage: prove a change achieved its intended outcome (`/verification:confirm` — a mechanical build/test/lint prerequisite gate, then intent-match + evidence + verdict with the criterion auto-detected by change type), and verify measurable-improvement claims against a planning-time baseline (`/verification:measure`), never fabricating numbers.", "author": { "name": "Melodic Software", diff --git a/plugins/verification/CHANGELOG.md b/plugins/verification/CHANGELOG.md index 6b4606939..6deb48abc 100644 --- a/plugins/verification/CHANGELOG.md +++ b/plugins/verification/CHANGELOG.md @@ -3,6 +3,18 @@ All notable changes to the `verification` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.6.1] + +### Added + +- **`measure`: a routing line to `/performance:snapshot` for hosts a noise-floor warning cannot + cover.** When the machine's own spread makes a wall-clock comparison meaningless, this skill warns + and reports anyway; the new `performance` plugin qualifies the host first, interleaves the arms + within one run, ranks a drift-immune counter above any duration, and refuses the claim outright. + The gotcha now names that boundary. `measure` stays SSOT for baseline capture, storage, and the + compare mechanics `performance` depends on rather than reimplements. Settled as Q2 of the #3530 + interview. + ## [0.6.0] ### Added diff --git a/plugins/verification/skills/measure/SKILL.md b/plugins/verification/skills/measure/SKILL.md index 7bd00fc6c..3b403c941 100644 --- a/plugins/verification/skills/measure/SKILL.md +++ b/plugins/verification/skills/measure/SKILL.md @@ -64,4 +64,5 @@ Measuring broken code is meaningless, and a baseline captured on a broken tree p - **Baseline BEFORE the change, compared under the SAME conditions after.** Condition drift invalidates the comparison, the run/warm-up/conditions methodology is owned by [context/performance.md](context/performance.md). - **Noise floor first.** If the projected saving sits within run-to-run variance, the change is unmeasurable. Surface that before the work, not after (detail: [context/performance.md](context/performance.md)). +- **A drifting host needs more than a noise-floor warning.** When the machine's own spread makes a wall-clock comparison meaningless (a bimodal no-op-spawn signature, arms that cannot be run in one pass, no drift-immune counter agreed), route to `/performance:snapshot` when the `performance` plugin is installed: it qualifies the host before measuring, interleaves the arms within one run, ranks a drift-immune counter above any duration, and refuses a wall-clock claim this skill would still report with a warning. This skill remains SSOT for baseline capture, storage, and the compare mechanics that plugin builds on. - **Never fabricate numbers.** No baseline, high variance, or differing conditions → INCONCLUSIVE / NOT CONFIRMED, stated plainly. diff --git a/scripts/cross-plugin-source-registry.txt b/scripts/cross-plugin-source-registry.txt index a751888f6..e905e61bf 100644 --- a/scripts/cross-plugin-source-registry.txt +++ b/scripts/cross-plugin-source-registry.txt @@ -35,3 +35,6 @@ lib/state-key.sh # single-plugin path fails as stale), so uncomment the line below when a # second plugin consumes it. # scripts/index-regen.sh + +# Dedicated check: scripts/sync-spawn-noise.sh --check (CI: spawn-noise-sync) +lib/spawn_noise.py diff --git a/scripts/sync-spawn-noise.sh b/scripts/sync-spawn-noise.sh new file mode 100755 index 000000000..4c92cfe99 --- /dev/null +++ b/scripts/sync-spawn-noise.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +# Sync or verify the cross-plugin lib/spawn_noise.py cluster. +# +# scripts/sync-spawn-noise.sh copy the canonical file into each carrier +# scripts/sync-spawn-noise.sh --check fail if any carrier differs from canonical +# scripts/sync-spawn-noise.sh --check-bump fail if the canonical changed vs but a +# carrying plugin's manifest version did not +# scripts/sync-spawn-noise.sh --print-manifest emit src and copies as data (for affected-tests) +# +# Canonical copy: plugins/claude-ops/lib/spawn_noise.py (see +# scripts/cross-plugin-source-registry.txt). Tests live beside the canonical copy only. +# +# What the cluster buys: `claude-ops:audit-performance` and `performance` must not +# disagree about what counts as an unmeasurable host. Plugins install independently, +# so neither can import the other at runtime; a byte-identical copy plus this gate is +# how the bimodal threshold keeps exactly one home. Two plugins quietly holding +# different values for BIMODAL_SPREAD_RATIO is worse than either value. +# +# The three modes live in scripts/lib/sync-cluster.sh, shared with the sibling +# sync-*.sh gates; this file supplies the spawn-noise cluster's parameters. +set -euo pipefail + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +cd "$script_dir/.." +# shellcheck source=lib/sync-cluster.sh +. "$script_dir/lib/sync-cluster.sh" + +sync_cluster_script="sync-spawn-noise.sh" +src="plugins/claude-ops/lib/spawn_noise.py" +copies=(plugins/performance/lib/spawn_noise.py) +sync_cluster_manifest_strip='/lib/*' +sync_cluster_noun="Canonical" +sync_cluster_carrier="carrying" +sync_cluster_sync_summary=0 + +mode="${1:-sync}" +base="" +# Raised here, not in the shared engine: bash prefixes a ${var:?} diagnostic with +# the path and line of the expansion, so the message has to come from the script +# the user actually ran. +[[ "$mode" == "--check-bump" ]] && base="${2:?usage: sync-spawn-noise.sh --check-bump }" + +sync_cluster::run "$mode" "$base" diff --git a/scripts/sync-spawn-noise.test.sh b/scripts/sync-spawn-noise.test.sh new file mode 100755 index 000000000..1ca22e8f2 --- /dev/null +++ b/scripts/sync-spawn-noise.test.sh @@ -0,0 +1,160 @@ +#!/usr/bin/env bash +# Unit tests for sync-spawn-noise.sh. Builds a tiny synthetic repo tree per +# scenario in a temp dir and invokes the script against it directly -- the +# script's own `cd "$(dirname "$0")/.."` makes this work unmodified: copy it to +# /scripts/ and it operates on the fixture tree. +# +# The load-bearing case is "--check discriminates": a drift gate that reports +# clean whether or not the copies match is worse than no gate, because it +# reports success. Both arms run and the suite asserts they DIFFER. +set -uo pipefail + +SELF_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SCRIPT="$SELF_DIR/sync-spawn-noise.sh" +. "$SELF_DIR/test-git-helpers.sh" + +# shellcheck source=lib/test-harness.sh +. "$SELF_DIR/lib/test-harness.sh" + +CANONICAL="plugins/claude-ops/lib/spawn_noise.py" +COPY="plugins/performance/lib/spawn_noise.py" + +canonical_v1() { + printf 'BIMODAL_SPREAD_RATIO = 3.0\nSLOW_SPAWN_FLOOR_MS = 500.0\n' +} +canonical_v2() { + printf 'BIMODAL_SPREAD_RATIO = 3.0\nSLOW_SPAWN_FLOOR_MS = 500.0\n\ndef is_measurable(s):\n return True\n' +} + +# new_fixture -> fresh tree with the script and its shared engine copied in. +new_fixture() { + local dir + dir="$(mktemp -d)" + mkdir -p "$dir/scripts/lib" \ + "$dir/plugins/claude-ops/lib" "$dir/plugins/claude-ops/.claude-plugin" \ + "$dir/plugins/performance/lib" "$dir/plugins/performance/.claude-plugin" + cp "$SCRIPT" "$dir/scripts/sync-spawn-noise.sh" + cp "$SELF_DIR/lib/sync-cluster.sh" "$dir/scripts/lib/sync-cluster.sh" + chmod +x "$dir/scripts/sync-spawn-noise.sh" + printf '%s' "$dir" +} + +# manifest +manifest() { + printf '{"name":"%s","version":"%s"}\n' "$2" "$3" >"$1/plugins/$2/.claude-plugin/plugin.json" +} + +# base_fixture -> canonical + a matching copy, both plugins at 0.1.0. +base_fixture() { + local dir + dir="$(new_fixture)" + canonical_v1 >"$dir/$CANONICAL" + canonical_v1 >"$dir/$COPY" + manifest "$dir" claude-ops 0.1.0 + manifest "$dir" performance 0.1.0 + printf '%s' "$dir" +} + +git_fixture() { + local fixture="$1" + # On refusal (e.g. TMPDIR inside the checkout) stop before add/commit can + # resolve to the enclosing real repository. + git_init_test_repo "$fixture" || return 1 + git -C "$fixture" add -A + git -C "$fixture" commit -qm base + git -C "$fixture" rev-parse HEAD +} + +run_mode() ( + local fixture="$1" + shift + cd "$fixture" && bash scripts/sync-spawn-noise.sh "$@" +) + +# --- sync copies the canonical into the carrying plugin --------------------- +f="$(new_fixture)" +canonical_v1 >"$f/$CANONICAL" +printf 'BIMODAL_SPREAD_RATIO = 9.0\n' >"$f/$COPY" +manifest "$f" claude-ops 0.1.0 +manifest "$f" performance 0.1.0 +if out="$(run_mode "$f" 2>&1)" && cmp -s "$f/$CANONICAL" "$f/$COPY"; then + ok "sync makes the carrying copy byte-identical to the canonical" +else + fail "sync should copy the canonical into performance, got: $out" +fi +rm -rf "$f" + +# --- --check DISCRIMINATES -------------------------------------------------- +# discriminating-skip-required: a gate whose arms agree proves nothing. +# The threshold is the whole point of this cluster: claude-ops and performance +# disagreeing about what counts as an unmeasurable host is the failure it +# prevents. So drift a copy and assert the verdict FLIPS, rather than asserting +# only that each arm printed its own expected string. +f="$(base_fixture)" +if run_mode "$f" --check >/dev/null 2>&1; then + clean_verdict=pass +else + clean_verdict=fail +fi +printf 'BIMODAL_SPREAD_RATIO = 9.0\n' >"$f/$COPY" +if run_mode "$f" --check >/dev/null 2>&1; then + drifted_verdict=pass +else + drifted_verdict=fail +fi +if [[ "$clean_verdict" != "$drifted_verdict" ]]; then + ok "--check discriminates: matching copies '$clean_verdict', drifted copies '$drifted_verdict'" +else + fail "--check returned '$clean_verdict' for BOTH matching and drifted copies — it would report clean whether or not the threshold agrees" +fi +if [[ "$clean_verdict" == pass && "$drifted_verdict" == fail ]]; then + ok "--check passes on a matching cluster and fails on a drifted one" +else + fail "expected clean=pass drifted=fail, got clean=$clean_verdict drifted=$drifted_verdict" +fi +rm -rf "$f" + +# --- a drift message names the file to fix ---------------------------------- +f="$(base_fixture)" +printf 'BIMODAL_SPREAD_RATIO = 9.0\n' >"$f/$COPY" +out="$(run_mode "$f" --check 2>&1)" || true +if [[ "$out" == *"$COPY"* && "$out" == *"$CANONICAL"* ]]; then + ok "the drift message names both the drifted copy and the canonical" +else + fail "drift message should name both paths, got: $out" +fi +rm -rf "$f" + +# --- --print-manifest publishes src and copies ------------------------------ +f="$(base_fixture)" +out="$(run_mode "$f" --print-manifest 2>&1)" +if [[ "$out" == *"src"*"$CANONICAL"* && "$out" == *"copy"*"$COPY"* ]]; then + ok "--print-manifest publishes src and copy, so affected-tests can derive the fan-out" +else + fail "--print-manifest should publish src and copy, got: $out" +fi +rm -rf "$f" + +# --- --check-bump requires a carrier version bump when canonical changed ----- +f="$(base_fixture)" +if base="$(git_fixture "$f")"; then + canonical_v2 >"$f/$CANONICAL" + canonical_v2 >"$f/$COPY" + if run_mode "$f" --check-bump "$base" >/dev/null 2>&1; then + fail "--check-bump should fail when the canonical changed but no carrier version moved" + else + ok "--check-bump fails when the canonical changed but no carrier version moved" + fi + manifest "$f" performance 0.2.0 + manifest "$f" claude-ops 0.2.0 + if run_mode "$f" --check-bump "$base" >/dev/null 2>&1; then + ok "--check-bump passes once the carrying plugins bumped" + else + fail "--check-bump should pass after both carriers bumped" + fi +else + fail "could not init a git fixture; --check-bump arms did not run" +fi +rm -rf "$f" + +test_harness::report From 6229c61bba76ac3fbeb41fd1bf72c28f1a72aef4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:22:08 -0400 Subject: [PATCH 05/11] chore(performance): enable the plugin and prune its contract slice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CI gates, both real: plugin-catalog-enablement required an enabledPlugins key. .claude/cloud-bootstrap.sh computes what it installs from that file, so a catalogued plugin with no key never loads in a session here. contract-slice-prune required docs/topics/performance-plugin/ to go. That tree is contract tier: committed on a task branch, pruned before merge. Its durable outcomes graduated to issue #3530 first (comment 5501823163) — the Q10/Q11/Q12 resolutions, the two-part bimodal predicate, and why a cross-plugin runtime import was never available. The interview ledger was already linked there. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS --- .claude/settings.json | 1 + docs/topics/performance-plugin/PLAN.md | 254 ------------------------- 2 files changed, 1 insertion(+), 254 deletions(-) delete mode 100644 docs/topics/performance-plugin/PLAN.md diff --git a/.claude/settings.json b/.claude/settings.json index b9c567669..f63a91048 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -69,6 +69,7 @@ "mutation-testing@melodic-software": true, "naming@melodic-software": true, "overengineering@melodic-software": true, + "performance@melodic-software": true, "planning@melodic-software": true, "playbooks@melodic-software": true, "playgrounds@melodic-software": false, diff --git a/docs/topics/performance-plugin/PLAN.md b/docs/topics/performance-plugin/PLAN.md deleted file mode 100644 index 68b4e8650..000000000 --- a/docs/topics/performance-plugin/PLAN.md +++ /dev/null @@ -1,254 +0,0 @@ -# performance-plugin - -Source issue: [#3530](https://github.com/melodic-software/claude-code-plugins/issues/3530). -Interview: round 1, Q1-Q9, all answered 2026-08-31. Ledger (memory tier, not committed): -`.work/performance-plugin/interview-checklist.md`. -Research slice (memory tier, not committed): `.work/performance-plugin-methodology/RESEARCH.md`. - -## Brief - -### TLDR - -- A new `performance` plugin whose skills run a measurement-first optimization workflow: identify a - target, construct a goal with realistic and ideal tiers plus a computed floor, snapshot a - baseline, verify, and report. -- Its headline metric is a **drift-immune counter**, not a duration. It ships exactly one built-in - counter (process spawns); every other metric is user-declared per domain. -- It **refuses** to report a wall-clock claim from a host whose noise it has characterized as - pathological, and says which counter it can report instead. -- It owns measurement, goal construction, and verification. It delegates the code change to - `/implementation:implement` and depends on `/verification:measure` for baseline/compare mechanics. -- Gates hard-block. Every gate ships with a discrimination check proving it fails when its condition - is unmet. - -### Goal - -Performance optimization in this fleet becomes a repeatable, measured discipline rather than a -per-session improvisation that produces confident, unverifiable numbers. The plugin exists because a -competent operator with a strong prompt still produced five verification harnesses in one session -that each returned a **confident wrong answer** rather than an error. The workflow's value is not -that it measures; it is that it refuses to report what it cannot support, and that every gate it -enforces has itself been proven to discriminate. - -### Constraints - -- **No skill may report a duration without a noise characterization.** Violating this reproduces the - exact failure the plugin exists to prevent. -- **Every gate must be verified to discriminate.** A check that passes whether or not its condition - holds is worse than no check, because it reports success. Each gate ships with a two-arm test: a - positive arm where it must fire and a negative arm where it must not, and the arms must be shown - to differ. -- **`/verification:measure` is not reimplemented.** It already owns two-phase baseline/compare, - machine-bound baseline storage, and the no-baseline refusal. Duplicating it is the silent second - way `/discipline:reuse-or-replace` prohibits. -- **The noise-characterization threshold has exactly one home.** No copy of - `BIMODAL_SPREAD_RATIO` may exist in two plugins. -- New plugin follows repo conventions: `.claude-plugin/plugin.json`, `CHANGELOG.md`, `README.md`, - marketplace entry with a category drawn from `docs/CATALOG-TAXONOMY.md` (read - `.claude/rules/catalog-taxonomy.md` first), changelog-parity and plugin-schema CI gates green. -- Validate with `scripts/affected-tests.sh --run`, never a hand-picked suite. -- Prose follows the repo's house style; `plugins/*/skills/*/vendor/**` formatting is not a model. - -### Acceptance criteria - -- `plugins/performance/` exists with a manifest that validates — the `plugin-schema` CI gate goes - green on the new directory, and `changelog-parity` passes. -- `/skill-quality:check` reports PASS for each new skill. -- The workflow refuses to report a wall-clock claim from a host characterized as too noisy to - measure — asserted by a test feeding it a high-variance baseline (refusal) AND a low-variance - baseline (normal report), with the two arms shown to produce different outcomes. -- A drift-immune counter is reported alongside, and ranked above, any duration — verified by reading - the emitted report format. -- Every normative claim in a skill body carries a citation to a source fetched during the research - pass, or is explicitly labelled as a house rule with no field consensus behind it. -- `claude-ops:audit-performance` consumes the promoted shared noise-characterization lib and its - existing tests still pass. - -### Decisions locked in the interview - -| Q | Decision | -|---|---| -| Q1 | **Narrow metrics, broad targets.** Any target reducible to one repeatable command. Process-spawn count is the ONLY built-in drift-immune counter; all others are user-declared per domain. | -| Q2 | **Depend + route.** `performance` owns the discipline and depends on `/verification:measure` for baseline/compare. `measure` stays and gains one routing line pointing here for wall-clock claims on a drifting host. | -| Q3 | **Measurement + goal + verification only.** The code change is delegated to `/implementation:implement`. | -| Q4 | **Gates hard-block**, with a named per-gate override that is recorded in the emitted report. | -| Q5 | **Reuse `BIMODAL_SPREAD_RATIO`** as the unmeasurable-host threshold rather than inventing a second number. Q4's recorded override applies. The refusal message must name the counter it can still report. | -| Q6 | **Baselines live in the memory tier**, `.work//baselines/`, machine-bound, never committed. Matches `/verification:measure` exactly. | -| Q7 | **Promote the noise-characterization algorithm into a shared lib** with one home for the threshold, and refactor `claude-ops:audit-performance` to consume it. No reaching into its private script directory; no copy-and-drift. | -| Q8 | **Phases 2-6 may run unattended. Phase 1 (goal construction) is human-gated always.** The loop is opt-in, may open PRs, may never merge. Mirrors the repo's existing loop-lane topology. | -| Q9 | **Both pairing modes.** Sequential interleaving suppresses the paired ratio under concurrent load; simultaneous duet-style paired arms report it. #3530's Phase 4 text is corrected, not followed. | - -### Captured assumptions - -- The plugin is used primarily on this host and hosts like it (Windows, MSYS/native mix, bimodal - process-creation cost) — revisit if it is aimed at contributors whose hosts are always noisy, which - would make the Q5 refusal posture unusable rather than protective. -- Promoting the shared lib will not break `claude-ops:audit-performance`'s existing tests — revisit - if that refactor turns out to touch its reporting contract rather than just its internals. -- Skill decomposition (one workflow skill with phases, versus one skill per phase) is a planning - decision — revisit if it turns out to change what the acceptance criteria can assert. - -### Out-of-scope - -- Owning the code change. Phase 3 delegates to `/implementation:implement`. -- Reimplementing baseline/compare mechanics that `/verification:measure` already provides. -- Built-in counters beyond process spawns. Syscall, query, and allocation counters are user-declared - in V1 and only become built-ins once validated against a real target. -- Merging its own PRs, under any autonomy setting. -- Superseding or removing `/verification:measure`. - -### Deferred questions - -- Q10 — Where exactly does the shared noise-characterization lib live, and does the - `claude-ops:audit-performance` refactor land in this PR or a follow-up? — defer until planning; - **arbiter: /planning:plan** -- Q11 — Skill decomposition: one workflow skill with six phases, or one skill per phase? — defer - until planning; **arbiter: /planning:plan** -- Q12 — Sample count and percentile choice. #3530 says "p50 and p95 over >=20 samples", but the - research found no community-grounded sample count and the SRE Book names 99th/99.9th rather than - p95. Whatever ships is a house choice and must be labelled as one. — defer until skill authoring; - **arbiter: USER-RESERVED** - -## Plan - -Written 2026-08-31 against `feat/performance-plugin` (branched from `origin/main` at `79f1c29`). -Resolves Q10 and Q11 (arbiter `/planning:plan`). Q12 stays USER-RESERVED and is surfaced at the -approval gate below, not resolved here. - -### Q11 resolved — skill decomposition - -**Four skills, no router.** Each names its successor, the way the repo's own planning pipeline -chains (`interview` -> `explore` -> `plan` -> `implement`) rather than routing through a hub. - -| Skill | Phases | Owns | -|---|---|---| -| `/performance:target` | 0 | Identify and rank candidate targets by **evidence quality**, not suspicion. When nothing is measured, the top recommendation is "instrument this first". Entry point. | -| `/performance:goal` | 1 | Human-gated always. Metric + the exact command producing it, a **realistic** target, an **ideal** target, and the computed **floor**. Refuses to accept a target below the measured floor without the user deciding. | -| `/performance:snapshot` | 2, 4 | Host qualification, snapshot capture, interleaved and duet A/B, the drift-immune counter, and the unmeasurable-host refusal. | -| `/performance:verify` | 5, 6 | Fresh-context adversarial re-derivation that does not inherit the implementer's numbers, plus the report. | - -**Naming.** `snapshot`, not `measure`. Q2 locked "depend + route" on `/verification:measure`; two -skills named `measure` in two plugins is the routing line failing to route. `snapshot` is also -#3530's own vocabulary ("baseline snapshot", "post snapshot"). - -**Harness-integrity is a shared reference plus a script, not a fifth skill.** The five -confident-wrong harnesses are the plugin's most important content, but they are a discipline applied -*inside* `snapshot` and `verify`, not something invoked standalone. Ships as -`reference/harness-integrity.md` plus `scripts/discriminate.py`, both consumed by the two skills that -need them. Promoting it to a fifth skill is the obvious V2 move if users start asking "does my -harness actually discriminate?" as a standalone question; deferring keeps the shared skill-listing -budget lower for V1. - -### Q10 resolved — the shared lib, and the PR split - -**A cross-plugin runtime import is not available.** Plugins install independently, so -`performance` cannot import from `claude-ops` at runtime. The interview's accepted answer ("promote -into a shared lib") is implemented through the repo's established mechanism for exactly this, not -through an import. - -**The established pattern**, already carrying six clusters (`scripts/cross-plugin-source-registry.txt`): - -- canonical source at repo root `lib/`; -- byte-identical copies at `plugins//lib/` in each carrying plugin; -- a dedicated `scripts/sync-.sh` built on `scripts/lib/sync-cluster.sh`, giving `--check`, - `--check-bump `, and `--print-manifest`; -- registered in `scripts/cross-plugin-source-registry.txt` with its check named; -- a CI job. - -This satisfies the brief's constraint that the threshold has exactly one home: `lib/` is the home, -and every copy that drifts fails CI loudly. - -**Applied here:** - -- Canonical `lib/spawn-noise.py`, holding `summarize_spawn_samples`, `spawn_probe`, - `BIMODAL_SPREAD_RATIO`, `SLOW_SPAWN_FLOOR_MS`, `SPAWN_SAMPLES`, `NOOP_SPAWN`. -- Copies at `plugins/claude-ops/lib/spawn-noise.py` and (in PR 2) - `plugins/performance/lib/spawn-noise.py`. -- `scripts/sync-spawn-noise.sh` + registry entry + CI job `spawn-noise-sync`. -- `audit_performance.py` imports it with the `sys.path.insert(_LIB_DIR)` shape already used by - `plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py:55`, and **re-exports the names** - so `test_audit_performance.py`'s `engine.summarize_spawn_samples` keeps resolving. The promotion is - test-invisible; the six existing cases are the proof. - -**The threshold is a two-part predicate, not a constant.** `bimodal-spawn-latency` fires on -`spread_ratio >= BIMODAL_SPREAD_RATIO (3.0)` **AND** `max >= SLOW_SPAWN_FLOOR_MS`. A wide ratio alone -is not the contention signature: a cold first spawn against a warm second clears 3x while every -sample is still fast. `performance` must consume the predicate, never re-derive a verdict from the -ratio alone. - -**Two PRs.** - -- **PR 1** — lib promotion, sync gate, registry entry, CI job, `claude-ops` refactor + CHANGELOG + - version bump. Self-contained, test-invisible, independently reviewable. -- **PR 2** — the `performance` plugin itself, consuming the lib, plus the one routing line into - `/verification:measure`. - -Rationale: PR 2 is already large (four skills, harness scripts, evals, marketplace entry). Folding a -cross-plugin refactor of a third plugin into it makes review materially worse. The split is -reversible: if PR 1 reviews trivially, PR 2 can be opened before it merges and rebased. - -### Category - -`verification`. Checked against the taxonomy's Assignment principle rather than assumed: the subject -is arbitrary code, not one of the special subjects (Claude Code, the workstation, music, personal), -so the plugin files by lifecycle activity. `verification`'s scope line is "Prove a change achieved -its intended outcome against baseline and intent", which is this plugin's whole shape. -`codebase-health` is filed `quality` because it audits artifacts on an absolute axis; this plugin is -before/after proof against a baseline, which is the distinguishing trait. - -### Approach, in order - -1. **PR 1.** Extract `lib/spawn-noise.py`; add `scripts/sync-spawn-noise.sh` + its `.test.sh`; - register the cluster; add the CI job; refactor `audit_performance.py` to import and re-export; - bump `claude-ops` version + CHANGELOG. Gate: the six existing `summarize_spawn_samples` cases pass - unchanged. -2. **Scaffold `plugins/performance/`** — `.claude-plugin/plugin.json`, `CHANGELOG.md`, `README.md`, - `lib/spawn-noise.py` (synced copy), marketplace entry, regenerated `docs/CATALOG.md`. Gate: - `plugin-schema` and `changelog-parity` green. -3. **Author `reference/harness-integrity.md`** first, before any skill body. It is the content the - other four depend on, and it is the plugin's reason for existing. -4. **Author the four skills**, each citing the research slice. Parallelizable across workers once - step 3 lands, since each is a separate file with no shared edit surface. -5. **Port the harnesses** from `D:/worktrees/bench-dh/` into `scripts/`, with precondition assertions - built in. They live on local disk only and are not durable. -6. **Add the routing line** to `plugins/verification/skills/measure/SKILL.md` + CHANGELOG + version - bump. -7. **Evals** per skill; `/skill-quality:check` per skill. -8. **PR 2** per the repo's body template. - -### Test strategy - -- **The refusal criterion is the highest-risk one.** Per the source session, four of five - discrimination checks failed by exiting identically in *both* arms and reporting "not - discriminating". So the refusal test asserts three things, not two: the high-variance arm refuses, - the low-variance arm reports normally, and **the two arms produced different output** as a - first-class assertion. Annotated `# discriminating-skip-required:` so - `scripts/check-discriminating-test-skips.sh` forbids skipping it. -- Contract tests as `plugins/performance/**/*.test.sh`, modelled on - `plugins/disk-hygiene/hooks/run-python-hook.test.sh` (32 assertions, full cache-invalidation - matrix) — tests that assert their own preconditions. -- Python unit tests for `lib/spawn-noise.py`. -- Validate with `scripts/affected-tests.sh --run`, never a hand-picked suite. -- Lint against LF content (`tr -d '\r'`): `core.autocrlf=true` on this machine makes shellcheck - report SC1017 on every line and bury real findings. - -### Blast radius - -| Surface | Change | -|---|---| -| `lib/spawn-noise.py`, `scripts/sync-spawn-noise.sh` (+ test), CI workflow, registry | New (PR 1) | -| `plugins/claude-ops/**` | Refactor + CHANGELOG + version bump (PR 1) | -| `plugins/performance/**` | New (PR 2) | -| `.claude-plugin/marketplace.json`, `docs/CATALOG.md` | New entry + regeneration (PR 2) | -| `plugins/verification/skills/measure/SKILL.md` + CHANGELOG + version | One routing line (PR 2) | - -Three plugins are touched in total. Two of them (`claude-ops`, `verification`) are existing, -installed, and working; both changes are additive and version-bumped. - -### Surfaced at the approval gate - -**Q12 (USER-RESERVED) — sample count and percentile choice.** #3530 specifies "p50 and p95 over >=20 -samples". The research grounds none of it: no benchmarking-community sample count for a meaningful -percentile exists beyond the derivable `1/(1-p)` floor, and the SRE Book names the 99th and 99.9th -percentiles rather than p95. Whatever ships is a house choice that must be labelled as one in the -skill body. This needs the user at the approval gate, because it changes what the skills assert. From 04ac9a00801fec49891a6c60d64b4d80363bf2bb Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:57:40 -0400 Subject: [PATCH 06/11] feat(performance): port the measurement harnesses and add the eval suites Completes the plugin. Two parallel workers, each gated by its own fresh-context verifier that executed rather than read. ## Harnesses (plugins/performance/scripts/) Nine scripts, each with a co-located test suite, 200 assertions total. Ported from the source run's scratch tree, which lived on local disk only and would have died with that directory. spawn-census.sh / run-spawn-census.sh use a STABLE shim dir, closing the defect where a mktemp -d shim put a fresh path on PATH every run against a PATH-keyed cache, so the census measured its own randomization and reported "no improvement". ab.sh + summarize.py + ratio.py interleave the arms and flip order per iteration, suppressing the paired ratio under concurrency. differential.py proves behavior over an argv matrix. discriminate.py consolidates five variants, four of which were broken. The verifiers found seven real defects between them, all fixed: - discriminate.py scored a check that never ran. Any shared non-zero exit read as NOT DISCRIMINATING with an affirmatively false explanation. It now splits identical-failing (HARNESS BROKEN, exit 2) from identical-passing (NOT DISCRIMINATING, exit 1), so the four original harness failures that exited 127 in both arms would now be caught rather than reported clean. - ratio.py printed a headline ratio with no sample floor: two identical arms measured 0.78x to 17.12x at five pairs. - The floor then guarded only the headline. Two identical `true` arms gave median_paired_ratio=1.06x beside ratio_of_p50=12.08x, so a reader could quote a 12x speedup between `true` and `true`. All three statistics are gated now. - spawn-census.sh censused a 126 subject as spawns=0, exit 0. - ratio.py accepted a spliced row summarize.py rejects. - printf | subject under pipefail fabricated exit 141 intermittently on a pipe-buffer race whenever the subject did not drain stdin. - discriminate.py referenced os with no import, on a line no test reached. A line no test executes is the exact shape that harness exists to detect. Rule 4 is proven behaviorally, not asserted: the target is committed, the fix applied and left uncommitted, and the fix is still present after the run. A git checkout restore would have destroyed it, which is what defect #5 in the source catalogue actually did. ## Evals target (5 cases), goal (6), verify (7), joining snapshot (5). Each case pins a specific gate whose removal would reintroduce a real failure: E4 suspicion never outranking E1 measurement, STOP when the target is below the floor, the differential covering every mode, a miss never rounded into a win. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS --- plugins/performance/CHANGELOG.md | 9 + plugins/performance/scripts/README.md | 73 +++ plugins/performance/scripts/ab.sh | 279 +++++++++++ plugins/performance/scripts/ab.test.sh | 180 +++++++ plugins/performance/scripts/differential.py | 393 +++++++++++++++ .../performance/scripts/differential.test.sh | 214 +++++++++ plugins/performance/scripts/discriminate.py | 446 ++++++++++++++++++ .../performance/scripts/discriminate.test.sh | 320 +++++++++++++ plugins/performance/scripts/harness-lib.sh | 187 ++++++++ .../performance/scripts/harness-lib.test.sh | 116 +++++ plugins/performance/scripts/pathfix.py | 165 +++++++ plugins/performance/scripts/pathfix.test.sh | 87 ++++ plugins/performance/scripts/ratio.py | 273 +++++++++++ plugins/performance/scripts/ratio.test.sh | 191 ++++++++ .../performance/scripts/run-spawn-census.sh | 188 ++++++++ .../scripts/run-spawn-census.test.sh | 103 ++++ plugins/performance/scripts/spawn-census.sh | 245 ++++++++++ .../performance/scripts/spawn-census.test.sh | 166 +++++++ plugins/performance/scripts/summarize.py | 143 ++++++ plugins/performance/scripts/summarize.test.sh | 103 ++++ .../performance/skills/goal/evals/evals.json | 80 ++++ .../skills/snapshot/evals/evals.json | 16 +- .../skills/target/evals/evals.json | 69 +++ .../skills/verify/evals/evals.json | 90 ++++ 24 files changed, 4128 insertions(+), 8 deletions(-) create mode 100644 plugins/performance/scripts/README.md create mode 100755 plugins/performance/scripts/ab.sh create mode 100755 plugins/performance/scripts/ab.test.sh create mode 100644 plugins/performance/scripts/differential.py create mode 100755 plugins/performance/scripts/differential.test.sh create mode 100644 plugins/performance/scripts/discriminate.py create mode 100755 plugins/performance/scripts/discriminate.test.sh create mode 100755 plugins/performance/scripts/harness-lib.sh create mode 100755 plugins/performance/scripts/harness-lib.test.sh create mode 100644 plugins/performance/scripts/pathfix.py create mode 100755 plugins/performance/scripts/pathfix.test.sh create mode 100644 plugins/performance/scripts/ratio.py create mode 100755 plugins/performance/scripts/ratio.test.sh create mode 100755 plugins/performance/scripts/run-spawn-census.sh create mode 100755 plugins/performance/scripts/run-spawn-census.test.sh create mode 100755 plugins/performance/scripts/spawn-census.sh create mode 100755 plugins/performance/scripts/spawn-census.test.sh create mode 100644 plugins/performance/scripts/summarize.py create mode 100755 plugins/performance/scripts/summarize.test.sh create mode 100644 plugins/performance/skills/goal/evals/evals.json create mode 100644 plugins/performance/skills/target/evals/evals.json create mode 100644 plugins/performance/skills/verify/evals/evals.json diff --git a/plugins/performance/CHANGELOG.md b/plugins/performance/CHANGELOG.md index add6194f4..53bd8dbfa 100644 --- a/plugins/performance/CHANGELOG.md +++ b/plugins/performance/CHANGELOG.md @@ -30,5 +30,14 @@ All notable changes to the `performance` plugin are documented here. Format foll it is not measuring itself, a probe must assert its own precondition and fail rather than silently degrade, and a discrimination check must verify its own patch applied and restore from saved bytes rather than from version control. +- **`scripts/`**: nine harnesses ported from the source run's scratch tree, which lived on local disk + only and was not durable. `spawn-census.sh` and `run-spawn-census.sh` (spawn census via a + **stable** shim dir, closing the defect where a `mktemp -d` shim invalidated the subject's + `PATH`-keyed cache every run and the census measured its own randomization), `ab.sh` + + `summarize.py` + `ratio.py` (interleaved A/B, order flipped per iteration, ratio suppressed under + concurrency and floored at 20 pairs), `differential.py` (byte-identical pre/post behavior over an + argv matrix), `discriminate.py` (consolidated does-this-check-actually-fail harness), plus + `harness-lib.sh` and `pathfix.py` for the shared preconditions. Each ships a co-located test suite; + 200 assertions across the nine. - **`lib/spawn_noise.py`**: a byte-identical copy of the canonical `claude-ops` lib, registered as a cross-plugin cluster with a dedicated sync gate so the bimodal threshold has exactly one home. diff --git a/plugins/performance/scripts/README.md b/plugins/performance/scripts/README.md new file mode 100644 index 000000000..53e2fb47f --- /dev/null +++ b/plugins/performance/scripts/README.md @@ -0,0 +1,73 @@ +# Measurement harnesses + +The reference implementations `/performance:snapshot` and `/performance:verify` run. Read +[`../reference/harness-integrity.md`](../reference/harness-integrity.md) first: these scripts exist +to ENFORCE the rules in it, and every refusal below is a defect that shipped in the source run. + +| Script | Owns | +|---|---| +| `harness-lib.sh` | Shared preconditions: drive-letter path refusal, shim-directory stability, the injected-PATH ledger, interpreter discovery. | +| `pathfix.py` | MSYS versus native-Windows path spelling, for the Python harnesses. | +| `spawn-census.sh` | One process-spawn census of one subject command, via a stable PATH shim directory. | +| `run-spawn-census.sh` | Before and after censuses, with the rule 1 warm-agreement proof. | +| `ab.sh` | Interleaved A/B timing with order flipping, order-flipped per iteration. | +| `summarize.py` | Per-arm p50 and p95, refusing any percentile the sample count cannot express. | +| `ratio.py` | Paired ratio, suppressed under concurrency. | +| `differential.py` | Pre-change versus post-change behavior over an argv matrix: byte-identical stdout and exit code, with any stderr difference disclosed as outside that bar. | +| `discriminate.py` | Does this check actually fail without the fix. | + +Every script carries a co-located `.test.sh`. Run one with `bash .test.sh`. + +## What these refuse to do + +- **Invent a shim directory.** `--shim-dir` is required and a temporary root is rejected. The source + census used `mktemp -d`, changed `PATH` every run, forced a permanent cache miss in a subject that + cached keyed on `PATH`, and reported "no improvement" while measuring its own randomization. +- **Skip an unresolvable tool.** The source census did, which undercounts silently. +- **Fall back to `date(1)` for timing.** A process spawn per sample measures the instrument. +- **Report a percentile the sample count cannot express.** `p95` needs 20 samples; below that the + printed value is the maximum wearing a percentile's name. +- **Report a headline paired ratio from a handful of pairs.** Two IDENTICAL arms measured here spread + 0.78x to 17.12x at five pairs, and once read 17.12x. The default floor is 20 pairs; below it the + raw per-pair ratios are printed instead. +- **Pair samples that were not load-matched.** Concurrency suppresses the paired ratio outright. +- **Count a subject that never ran.** Exit 126 as well as 127: "found but not executable" produces + the same tidy `spawns=0` line as "not found". +- **Time on a clock it cannot read.** A comma-decimal locale renders `EPOCHREALTIME` as + `1788283754,274241`. That is refused by name rather than corrected, because changing the locale + would change the environment the SUBJECT runs in. The refusal names the variable that actually + governs: POSIX precedence is `LC_ALL` over `LC_NUMERIC` over `LANG`, so suggesting `LC_NUMERIC=C` + while `LC_ALL` is set would be advice that silently does nothing. +- **Call two arms that both failed "parity" or "not discriminating".** Those are harness failures and + are reported as such, with a distinct exit status. Two arms that both FAILED identically are + indistinguishable from two arms that never ran, so that case is refused rather than scored; two + arms that both PASSED are a knowable finding and are reported as a check that cannot fail. +- **Restore from `git checkout --`.** The pre-patch bytes go to a sidecar file first, the restore + comes from that, and the restore is verified by byte comparison. + +## The path hazard, in both directions + +This is the trap that produced three of the five source-run failures, and it is not symmetric: + +- **bash needs the MSYS spelling.** A `D:/...` path handed to bash resolves nowhere. The shell + harnesses refuse one outright unless `--allow-windows-paths` is passed. +- **A native Windows interpreter needs the native spelling.** The `python3` on this kind of host is + a native build, so `/d/worktrees/repo/x.py` resolves to `D:\d\worktrees\repo\x.py`, which is + nowhere. `pathfix.py` resolves that, loudly. +- **MSYS rewrites argv, but not file contents.** A POSIX path on the command line of a native + executable arrives already converted, so the same spelling works in argv and fails inside a JSON + config. That asymmetry gets debugged in the wrong place. +- **`/tmp` and `/usr/bin` are MOUNTS**, with no drive letter to fold. Only `cygpath -w` knows the + mount table, which is why `pathfix.py` asks it as a last resort. + +`discriminate.py` deliberately does NOT rewrite `check.argv`: a bash check needs MSYS paths in its +arguments and a native check needs native ones, so rewriting would break whichever the caller meant. +Name the check relative to `check.cwd`, which the harness does resolve. + +## Exit statuses + +`0` the harness ran and the result is as expected. `1` the subject or the comparison came out +negative: a mismatch, a check that does not discriminate. `2` the harness could not run, or could not +have measured what it claims. The `1` and `2` split is load-bearing rather than tidy: conflating "the +check does not discriminate" with "the check never ran" is what produced four confident wrong +verdicts in the source run. diff --git a/plugins/performance/scripts/ab.sh b/plugins/performance/scripts/ab.sh new file mode 100755 index 000000000..096209f1b --- /dev/null +++ b/plugins/performance/scripts/ab.sh @@ -0,0 +1,279 @@ +#!/usr/bin/env bash +# INTERLEAVED before/after timing for two arbitrary commands. +# +# Never compare two separate passes on a drifting host. A bare `bash -c true` +# measured 1825ms and 283ms in the same hour at roughly 10% CPU on the box this +# plugin was built from; any two-pass comparison attributes that 6x to the +# change. Alternating the arms within a single run, and flipping the ORDER each +# iteration, puts both arms under the same instantaneous load, so the paired +# ratio survives drift the absolute numbers do not. +# +# Grounded, Tier 1, benchstat's own documentation: "The best way to do this is +# to interleave before and after runs, rather than running, say, 10 iterations +# of the before benchmark, and then 10 iterations of the after benchmark." +# +# Under CONCURRENCY the paired ratio is SUPPRESSED, not merely caveated. The +# arms interleave arbitrarily once they overlap, so pairing by index compares +# samples that never shared conditions. ratio.py enforces that refusal itself, +# so a caller who invokes it directly cannot lose the suppression. +# +# Usage: +# ab.sh --a --b --iterations [options] +# +# --a REQUIRED. Baseline arm, a shell command string. +# --b REQUIRED. Comparison arm, a shell command string. +# --iterations REQUIRED. Paired iterations, at least 1. +# --concurrency Parallel writers. Default 1. Above 1 suppresses the ratio. +# --label-a Default: A (baseline) +# --label-b Default: B (candidate) +# --warmup Discard this many runs per arm first. Default 1. +# --min-pairs Pairs required before a ratio is reported. Default 20. +# --stdin Feed to both arms on stdin. +# --allow-windows-paths Permit drive-letter paths in the arm commands. +# +# Exit: 0 the run completed; 1 a sample-count assertion failed; 2 a precondition +# failed. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +# shellcheck source=harness-lib.sh +source "$SCRIPT_DIR/harness-lib.sh" + +usage() { + cat <<'USAGE' +ab.sh --a --b --iterations [options] + + --a REQUIRED. Baseline arm, a shell command string. + --b REQUIRED. Comparison arm, a shell command string. + --iterations REQUIRED. Paired iterations, at least 1. + --concurrency Parallel writers. Default 1. Above 1 suppresses the ratio. + --label-a Default: A (baseline) + --label-b Default: B (candidate) + --warmup Discard this many runs per arm first. Default 1. + --min-pairs Pairs required before a ratio is reported. Default 20. + --stdin Feed to both arms on stdin. + --allow-windows-paths Permit drive-letter paths in the arm commands. + +Exit: 0 completed; 1 a sample-count assertion failed; 2 a precondition failed. +USAGE +} + +CMD_A="" +CMD_B="" +ITERS="" +CONC=1 +LABEL_A="A (baseline)" +LABEL_B="B (candidate)" +WARMUP=1 +MIN_PAIRS="20" +STDIN_TEXT="" +ALLOW_WINDOWS_PATHS=0 + +while (($# > 0)); do + case "$1" in + --a) + CMD_A="${2:-}" + shift 2 + ;; + --b) + CMD_B="${2:-}" + shift 2 + ;; + --iterations) + ITERS="${2:-}" + shift 2 + ;; + --concurrency) + CONC="${2:-}" + shift 2 + ;; + --label-a) + LABEL_A="${2:-}" + shift 2 + ;; + --label-b) + LABEL_B="${2:-}" + shift 2 + ;; + --warmup) + WARMUP="${2:-}" + shift 2 + ;; + --min-pairs) + MIN_PAIRS="${2:-}" + shift 2 + ;; + --stdin) + STDIN_TEXT="${2:-}" + shift 2 + ;; + --allow-windows-paths) + ALLOW_WINDOWS_PATHS=1 + shift + ;; + -h | --help) + usage + exit 0 + ;; + *) + harness_die "unknown argument: $1 (see --help)" + ;; + esac +done + +# A missing high-resolution clock must FAIL, never fall back to `date`. A +# per-sample date(1) call is a process spawn, so the fallback would measure the +# instrument at roughly the same magnitude as the subject and report it as the +# subject's cost. harness-integrity.md rule 2: assert the precondition, do not +# degrade into a weaker measurement that passes. +# +# PERF_AB_SIMULATE_NO_CLOCK exists so the suite can exercise this branch on a +# host that does have the clock. It can only FORCE the failure, never suppress +# it, so it cannot turn a broken host green. +if [[ -n "${PERF_AB_SIMULATE_NO_CLOCK:-}" || -z "${EPOCHREALTIME:-}" ]]; then + harness_die "EPOCHREALTIME is unavailable, so this shell has no spawn-free high-resolution clock (bash 5.0 or later provides it). Refusing to fall back to date(1): a process spawn per sample would measure the harness alongside the subject." +fi + +# EPOCHREALTIME honors LC_NUMERIC, so a comma-decimal locale renders it as +# `1788283754,274241`. The microsecond split below then feeds a non-numeric +# string to base-10 arithmetic. That failure is caught downstream by the +# sample-count assertion, but it is caught with the wrong diagnosis, and the +# operator is left reading "arm a holds 1 samples" with no hint about locale. +# Refused here, by name. Deliberately NOT auto-corrected: forcing LC_NUMERIC +# would change the environment the SUBJECT runs in, which is the one thing rule 1 +# says a harness must not do quietly. Re-run with LC_NUMERIC=C if that is what +# you want, and record it. +# +# PERF_AB_SIMULATE_COMMA_CLOCK, like the seam above, can only FORCE this failure, +# never suppress it, so it cannot turn a broken host green. +if [[ -n "${PERF_AB_SIMULATE_COMMA_CLOCK:-}" ]]; then + # The simulated branch must NOT assert the real observation, because the real + # observation is fine. A forced failure that claims a locale defect the host + # does not have sends the next reader chasing a working locale. + harness_die "SIMULATED: PERF_AB_SIMULATE_COMMA_CLOCK is set, so this run forces the comma-decimal-separator refusal. No real defect was observed; EPOCHREALTIME actually reads '${EPOCHREALTIME}'. Unset the variable to measure normally." +fi +if [[ "$EPOCHREALTIME" != *.* ]]; then + # Name the variable that ACTUALLY governs. POSIX precedence is + # LC_ALL > LC_NUMERIC > LANG, so telling someone to set LC_NUMERIC=C while + # LC_ALL is set is advice that silently does nothing: verified here, with + # LC_ALL=de_DE.UTF-8 the clock still reads a comma no matter what LC_NUMERIC + # says. A refusal whose suggested fix does not work is worse than no fix. + if [[ -n "${LC_ALL:-}" ]]; then + remedy="LC_ALL is set to '${LC_ALL}', and it OVERRIDES LC_NUMERIC, so setting LC_NUMERIC=C alone will not help. Re-run with LC_ALL=C, or unset LC_ALL and set LC_NUMERIC=C" + else + remedy="Re-run with LC_NUMERIC=C (currently '${LC_NUMERIC:-${LANG:-unset}}')" + fi + harness_die "EPOCHREALTIME reads '${EPOCHREALTIME}', which has no '.' decimal separator, so the microsecond arithmetic here would operate on a non-numeric string. ${remedy}, and record in the report that you changed the locale. This harness will not change it for you: that would alter the environment the SUBJECT runs in." +fi + +[[ -n "$CMD_A" ]] || harness_die "--a is required: a shell command string for the baseline arm." +[[ -n "$CMD_B" ]] || harness_die "--b is required: a shell command string for the comparison arm." +[[ "$ITERS" =~ ^[0-9]+$ ]] || harness_die "--iterations must be a non-negative integer, got '${ITERS:-}'." +((ITERS >= 1)) || harness_die "--iterations must be at least 1; there is no percentile to report from zero samples." +if [[ ! "$CONC" =~ ^[0-9]+$ ]] || ((CONC < 1)); then + harness_die "--concurrency must be an integer of at least 1, got '$CONC'." +fi +[[ "$WARMUP" =~ ^[0-9]+$ ]] || harness_die "--warmup must be a non-negative integer, got '$WARMUP'." + +if ((ALLOW_WINDOWS_PATHS == 0)); then + harness_require_posix_path "the --a command" "$CMD_A" + harness_require_posix_path "the --b command" "$CMD_B" +fi + +harness_require_python + +OUT="$(mktemp -d)" +readonly OUT +trap 'rm -rf "$OUT"' EXIT + +: >"$OUT/a" +: >"$OUT/b" + +# Stdin is materialized ONCE and redirected, never piped per sample. A +# `printf ... | bash -c` pipeline under `pipefail` reports 141 whenever the arm +# exits without draining stdin, because printf takes EPIPE and is then the only +# non-zero element: the exit-code census would carry a code the arm never +# returned, intermittently, on a pipe-buffer race. The pipeline also forks a +# subshell per sample, and a process spawn costs roughly 140ms on this class of +# host, so it would inflate every absolute number this script reports. +STDIN_PATH="$OUT/stdin" +printf '%s' "$STDIN_TEXT" >"$STDIN_PATH" + +# One timed invocation. Deliberately arithmetic-only after the clock reads: +# every command substitution is itself a process spawn under MSYS, so a +# converter that shelled out would add the very cost being measured. +one() { + local command="$1" sink="$2" + local t0 t1 rc + t0=$EPOCHREALTIME + bash -c "$command" <"$STDIN_PATH" >/dev/null 2>&1 + rc=$? + t1=$EPOCHREALTIME + local s0="${t0%%.*}" u0="${t0##*.}" s1="${t1%%.*}" u1="${t1##*.}" + printf '%s %s\n' "$(((s1 - s0) * 1000 + (10#$u1 - 10#$u0) / 1000))" "$rc" >>"$sink" +} + +# Probe both arms before measuring anything. Two arms that both exit 127 produce +# a tidy, symmetric, entirely meaningless comparison, which is the shape four of +# the five source-run harnesses shipped. +probe() { + local label="$1" command="$2" + bash -c "$command" <"$STDIN_PATH" >/dev/null 2>&1 + local rc=$? + if ((rc == 127)); then + harness_die "arm $label exited 127 (command not found): $command. Both arms failing identically is the classic false green; refusing to time a command that never runs." + fi +} + +probe "$LABEL_A" "$CMD_A" +probe "$LABEL_B" "$CMD_B" + +for ((w = 0; w < WARMUP; w++)); do + one "$CMD_A" /dev/null + one "$CMD_B" /dev/null +done + +if [[ "$CONC" == "1" ]]; then + for ((i = 0; i < ITERS; i++)); do + # Flip the ORDER each iteration so neither arm systematically lands in the + # warmer or colder half of a drift cycle. + if ((i % 2 == 0)); then + one "$CMD_A" "$OUT/a" + one "$CMD_B" "$OUT/b" + else + one "$CMD_B" "$OUT/b" + one "$CMD_A" "$OUT/a" + fi + done +else + # One sink file PER ITERATION. Parallel appends to a single file are not + # atomic on MSYS, and a spliced line makes the summarizer raise on a value + # that was never measured. Concatenate after every writer has exited. + mkdir -p "$OUT/parts" + for ((i = 0; i < ITERS; i++)); do + { one "$CMD_A" "$OUT/parts/a-$i"; } & + { one "$CMD_B" "$OUT/parts/b-$i"; } & + while (($(jobs -rp | wc -l) >= CONC)); do wait -n; done + done + wait + cat "$OUT/parts"/a-* >"$OUT/a" + cat "$OUT/parts"/b-* >"$OUT/b" +fi + +for arm in a b; do + lines="$(grep -c . "$OUT/$arm" || true)" + if [[ "$lines" != "$ITERS" ]]; then + printf 'FATAL: arm %s holds %s samples, expected %s. Refusing to summarize a sample set that lost or gained rows.\n' \ + "$arm" "$lines" "$ITERS" >&2 + exit 1 + fi +done + +BENCH_LABEL="$LABEL_A" BENCH_CONC="$CONC" BENCH_TIMES="$OUT/a" \ + "$HARNESS_PYTHON" "$SCRIPT_DIR/summarize.py" || exit $? +BENCH_LABEL="$LABEL_B" BENCH_CONC="$CONC" BENCH_TIMES="$OUT/b" \ + "$HARNESS_PYTHON" "$SCRIPT_DIR/summarize.py" || exit $? + +BENCH_OLD="$OUT/a" BENCH_NEW="$OUT/b" BENCH_CONC="$CONC" BENCH_MIN_PAIRS="$MIN_PAIRS" \ + "$HARNESS_PYTHON" "$SCRIPT_DIR/ratio.py" || exit $? diff --git a/plugins/performance/scripts/ab.test.sh b/plugins/performance/scripts/ab.test.sh new file mode 100755 index 000000000..bcc4fb0a7 --- /dev/null +++ b/plugins/performance/scripts/ab.test.sh @@ -0,0 +1,180 @@ +#!/usr/bin/env bash +# Tests for ab.sh, the interleaved A/B timer. +# +# The refusals are the point. A missing high-resolution clock must fail rather +# than fall back to date(1), because a process spawn per sample would measure +# the instrument alongside the subject; two arms that both exit 127 must be +# refused before timing, because a symmetric comparison of two commands that +# never ran is the classic false green; and concurrency must suppress the paired +# ratio, because the arms stop being load-matched the moment they overlap. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +AB="$SCRIPT_DIR/ab.sh" +readonly AB + +# Inline test helpers: self-contained, no external test lib (ships with the plugin). +FAILED=0 +CASE_NUM=0 +pass() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'PASS: [%d] %s\n' "$CASE_NUM" "$1" +} +fail() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'FAIL: [%d] %s - expected %q got %q\n' "$CASE_NUM" "$1" "$2" "$3" >&2 + FAILED=$((FAILED + 1)) +} +assert_eq() { if [[ "$3" == "$2" ]]; then pass "$1"; else fail "$1" "$2" "$3"; fi; } +assert_contains() { + if [[ "$3" == *"$2"* ]]; then pass "$1"; else fail "$1" "*$2*" "$3"; fi +} +assert_not_contains() { + if [[ "$3" != *"$2"* ]]; then pass "$1"; else fail "$1" "no *$2*" "$3"; fi +} + +RUN_OUT="" +RUN_RC=0 +run_ab() { + RUN_OUT="$(bash "$AB" "$@" 2>&1)" + RUN_RC=$? +} + +NOOP="printf ok" + +# --- 1. a serial run reports both arms and the paired ratio --- +run_ab --a "$NOOP" --b "$NOOP" --iterations 4 --warmup 1 --min-pairs 4 \ + --label-a "A (baseline)" --label-b "B (candidate)" +assert_eq "a serial run exits 0" "0" "$RUN_RC" +assert_contains "the baseline arm is summarized" "A (baseline)" "$RUN_OUT" +assert_contains "the comparison arm is summarized" "B (candidate)" "$RUN_OUT" +assert_contains "every sample is accounted for" "n=4" "$RUN_OUT" +assert_contains "the paired ratio is reported for a serial run" "median_paired_ratio" "$RUN_OUT" + +# --- 1b. the default refuses a headline ratio from too few pairs --- +# discriminating-skip-required: this is the only end-to-end proof that ab.sh +# carries the minimum-pairs floor through rather than reporting a ratio drawn +# from a handful of samples. +run_ab --a "$NOOP" --b "$NOOP" --iterations 4 --warmup 0 +assert_eq "the default-floor run exits 0" "0" "$RUN_RC" +assert_contains "the headline ratio is refused below the floor" \ + "median_paired_ratio=REFUSED(pairs=4<20)" "$RUN_OUT" + +# --- 2. concurrency suppresses the paired ratio --- +# discriminating-skip-required: this case is the only end-to-end proof that +# ab.sh propagates the concurrency suppression rather than reporting a ratio +# over arms that never shared conditions. +run_ab --a "$NOOP" --b "$NOOP" --iterations 4 --warmup 0 --concurrency 2 +assert_eq "a concurrent run exits 0" "0" "$RUN_RC" +assert_contains "the ratio is suppressed" "SUPPRESSED: concurrency=2" "$RUN_OUT" +assert_not_contains "no paired ratio under concurrency" "median_paired_ratio" "$RUN_OUT" +assert_contains "the per-arm percentiles are still reported" "n=4" "$RUN_OUT" + +# --- 3. stdin reaches both arms, and no exit code is fabricated --- +# A `printf | bash -c` pipeline under pipefail reports 141 whenever an arm exits +# without draining stdin, because printf takes EPIPE. The exit-code census would +# then carry a code no arm returned, intermittently. +# discriminating-skip-required: the rc census is the only place a fabricated +# exit code would surface, so this assertion is the whole proof. +# shellcheck disable=SC2016 # $line belongs to the inner `bash -c`, not to this shell +run_ab --a 'read -r line; [[ "$line" == "payload" ]]' --b 'exit 0' \ + --iterations 4 --warmup 0 --stdin 'payload' +assert_eq "a run with stdin exits 0" "0" "$RUN_RC" +assert_contains "the reading arm reports its own exit code" "rc={0: 4}" "$RUN_OUT" +assert_not_contains "an undrained stdin does not fabricate rc 141" "141" "$RUN_OUT" + +# --- 4. an arm that cannot run is refused before anything is timed --- +run_ab --a "$NOOP" --b "/nonexistent/definitely-not-here" --iterations 2 --warmup 0 +assert_eq "an arm exiting 127 is refused" "2" "$RUN_RC" +assert_contains "the refusal names the false-green shape" "classic false green" "$RUN_OUT" + +# --- 4. a missing high-resolution clock FAILS, it never falls back to date --- +# PERF_AB_SIMULATE_NO_CLOCK can only force the failure, never suppress it, so +# it cannot turn a genuinely broken host green. +RUN_OUT="$(PERF_AB_SIMULATE_NO_CLOCK=1 bash "$AB" --a "$NOOP" --b "$NOOP" --iterations 2 2>&1)" +RUN_RC=$? +assert_eq "a missing EPOCHREALTIME is refused" "2" "$RUN_RC" +assert_contains "the refusal rejects a date(1) fallback" "Refusing to fall back" "$RUN_OUT" + +# A comma-decimal locale renders EPOCHREALTIME as `1788283754,274241`, and the +# microsecond arithmetic then operates on a non-numeric string. Left unguarded +# the run still fails, but downstream and with the wrong diagnosis: the operator +# reads "arm a holds 1 samples" and nothing mentions locale. +# discriminating-skip-required: this case is the only cover for the decimal +# separator, and the simulate seam can only force the failure, never hide one. +RUN_OUT="$(PERF_AB_SIMULATE_COMMA_CLOCK=1 bash "$AB" --a "$NOOP" --b "$NOOP" --iterations 2 2>&1)" +RUN_RC=$? +assert_eq "the simulated comma clock is refused" "2" "$RUN_RC" +# The forced branch must say it is SIMULATING, not assert a locale defect the +# host does not have. A false observation in a test log sends the next reader +# chasing a locale that is working correctly. +assert_contains "the simulated branch declares itself" "SIMULATED:" "$RUN_OUT" +assert_contains "the simulated branch denies a real observation" "No real defect was observed" "$RUN_OUT" +assert_not_contains "the simulated branch does not blame LC_NUMERIC" \ + "is rendering it with a comma" "$RUN_OUT" + +# The REAL branch, exercised under an actual comma-decimal locale rather than +# through the seam, so the operator-facing wording is covered by something other +# than the simulation that deliberately does not produce it. +COMMA_LOCALE="" +for candidate in de_DE.UTF-8 de_DE.utf8 de_DE fr_FR.UTF-8 fr_FR.utf8 fr_FR; do + if [[ "$(LC_ALL="$candidate" bash -c 'printf %s "$EPOCHREALTIME"' 2>/dev/null)" == *,* ]]; then + COMMA_LOCALE="$candidate" + break + fi +done +if [[ -n "$COMMA_LOCALE" ]]; then + RUN_OUT="$(LC_ALL="$COMMA_LOCALE" bash "$AB" --a "$NOOP" --b "$NOOP" --iterations 2 2>&1)" + RUN_RC=$? + assert_eq "a real comma-decimal locale is refused" "2" "$RUN_RC" + assert_contains "the real refusal declines to change the subject's environment" \ + "alter the environment the SUBJECT runs in" "$RUN_OUT" + # POSIX precedence is LC_ALL > LC_NUMERIC > LANG. Suggesting LC_NUMERIC=C + # while LC_ALL is set is advice that silently does nothing, so the refusal + # must name the variable that actually governs THIS invocation. + # discriminating-skip-required: a refusal whose suggested remedy does not work + # is worse than no remedy, and only this case proves the remedy is right. + assert_contains "the refusal names LC_ALL as the overriding variable" \ + "it OVERRIDES LC_NUMERIC" "$RUN_OUT" + RUN_OUT="$(LC_ALL=C bash "$AB" --a "$NOOP" --b "$NOOP" \ + --iterations 2 --warmup 0 --min-pairs 2 2>&1)" + RUN_RC=$? + assert_eq "the remedy the refusal names actually runs" "0" "$RUN_RC" + + # And with only LANG set, LC_NUMERIC=C IS the right remedy, so the other + # branch of the advice must be exercised too. + RUN_OUT="$(env -u LC_ALL LANG="$COMMA_LOCALE" bash "$AB" --a "$NOOP" --b "$NOOP" \ + --iterations 2 2>&1)" + RUN_RC=$? + if [[ "$RUN_RC" == "2" ]]; then + assert_contains "with LANG only, the refusal names LC_NUMERIC" "LC_NUMERIC=C" "$RUN_OUT" + RUN_OUT="$(env -u LC_ALL LANG="$COMMA_LOCALE" LC_NUMERIC=C bash "$AB" --a "$NOOP" \ + --b "$NOOP" --iterations 2 --warmup 0 --min-pairs 2 2>&1)" + RUN_RC=$? + assert_eq "the LANG-only remedy actually runs" "0" "$RUN_RC" + else + printf 'SKIP: LANG alone did not produce a comma clock on this host\n' >&2 + fi +else + printf 'SKIP: no comma-decimal locale on this host; the real-locale arm of the clock check did not run\n' >&2 +fi + +# --- 5. argument preconditions --- +run_ab --a "$NOOP" --b "$NOOP" --iterations 0 +assert_eq "zero iterations is refused" "2" "$RUN_RC" +assert_contains "the refusal explains the empty sample set" "no percentile to report" "$RUN_OUT" + +run_ab --a "$NOOP" --b "$NOOP" --iterations many +assert_eq "a non-numeric iteration count is refused" "2" "$RUN_RC" + +run_ab --b "$NOOP" --iterations 2 +assert_eq "a missing --a is refused" "2" "$RUN_RC" + +run_ab --a "bash D:/repo/hook.sh" --b "$NOOP" --iterations 2 +assert_eq "a drive-letter path inside an arm command is refused" "2" "$RUN_RC" +assert_contains "the refusal names the MSYS trap" "resolves nowhere" "$RUN_OUT" + +[[ "${FAILED:-0}" -eq 0 ]] || exit 1 +echo "OK: ab interleaving and refusals" +exit 0 diff --git a/plugins/performance/scripts/differential.py b/plugins/performance/scripts/differential.py new file mode 100644 index 000000000..dda661f4f --- /dev/null +++ b/plugins/performance/scripts/differential.py @@ -0,0 +1,393 @@ +"""Differential: prove behavior did not change, rather than asserting it. + +A passing test suite is not a behavior proof. It proves nothing ASSERTED broke, +which is a different claim. This runs a PRE-CHANGE subject and a POST-CHANGE +subject over the same corpus, with the same argv and the same stdin, across +every combination of a caller-declared matrix, and requires BYTE-IDENTICAL +stdout and identical exit codes. + +Cover every MODE the subject runs in. The source run's differential covered one +of two modes and missed a real behavior downgrade in the other, so the matrix is +a first-class input here rather than a loop the caller writes by hand. + +Three refusals are enforced, each closing a way this could report a confident +wrong answer: + +* Two arms that resolve to the SAME file. The differential would compare a file + with itself and report parity for a comparison it never made. +* An arm that DISCLOSES ITS OWN PATH in stdout. Byte-exactness is only the right + bar when the two arms cannot differ merely by living at different paths; a + subject that prints a `__file__`-derived path fails that, and the mismatch + would be an artifact of the harness's own layout. The check is on the observed + output rather than on a same-directory rule, so two worktrees are a perfectly + legal input as long as neither arm leaks where it lives. +* A run in which NEITHER arm ever produced output and both failed identically. + That is not parity, it is a harness that never exercised the subject, and it + is the exact shape that reads as a clean, confident, wrong verdict. + +Usage: + differential.py --baseline --candidate --config + [--harvest-from ] [--limit ] + +Exit: 0 parity; 1 mismatches; 2 a precondition or harness failure. + +Config (JSON): + { + "argv": ["{{python}}", "{{subject}}", ["--mode", "{{mode}}"], "--root", "{{var:root}}"], + "matrix": {"mode": ["engine-gate", "belt", null], "tool": ["Bash", "PowerShell"]}, + "vars": {"root": "/d/worktrees/repo"}, + "corpus": ["git status --porcelain", "rm -rf /"], + "stdin_json": {"tool_name": "{{tool}}", "tool_input": {"command": "{{corpus}}"}}, + "harvest": {"helpers": ["run_guard"], "arg_index": 0}, + "timeout": 120 + } + + Tokens are `{{python}}` (this interpreter), `{{subject}}` (the arm's path), + `{{corpus}}` (the current corpus item), `{{var:NAME}}` (from `vars`), and + `{{NAME}}` for each matrix dimension. + + A NESTED LIST inside `argv` is an optional group: it is dropped entirely when + any token in it resolves to null, and spliced in otherwise. That is how a + mode of `null` means "pass no --mode at all", which is the shape a caller + gets by default and therefore the one a differential most needs to cover. +""" + +from __future__ import annotations + +import argparse +import ast +import itertools +import json +import pathlib +import re +import subprocess +import sys +from collections import Counter + +import pathfix + +TOKEN = re.compile(r"\{\{([A-Za-z0-9_:]+)\}\}") +WHOLE_TOKEN = re.compile(r"^\{\{([A-Za-z0-9_:]+)\}\}$") + + +class HarnessError(Exception): + """A precondition failed, or the harness could not have measured anything.""" + + +def resolve_token(name: str, bindings: dict[str, object]) -> object: + if name.startswith("var:"): + variables = bindings.get("__vars__") + assert isinstance(variables, dict) + if name[4:] not in variables: + raise HarnessError(f"config references {{{{{name}}}}} but `vars` has no {name[4:]!r}") + return variables[name[4:]] + if name not in bindings: + raise HarnessError( + f"config references {{{{{name}}}}}, which is neither a matrix dimension " + f"nor a built-in token. Known: {sorted(k for k in bindings if k != '__vars__')}" + ) + return bindings[name] + + +def substitute(value: object, bindings: dict[str, object]) -> object: + """Replace tokens. A string that is EXACTLY one token keeps the raw type.""" + if isinstance(value, str): + whole = WHOLE_TOKEN.match(value) + if whole: + return resolve_token(whole.group(1), bindings) + + def replace(match: re.Match[str]) -> str: + resolved = resolve_token(match.group(1), bindings) + if resolved is None: + raise HarnessError( + f"{{{{{match.group(1)}}}}} resolved to null inside the larger string " + f"{value!r}; null is only meaningful as a whole argv element or an " + f"optional argv group." + ) + return str(resolved) + + return TOKEN.sub(replace, value) + if isinstance(value, dict): + return {key: substitute(item, bindings) for key, item in value.items()} + if isinstance(value, list): + return [substitute(item, bindings) for item in value] + return value + + +def build_argv(template: list[object], bindings: dict[str, object]) -> list[str]: + argv: list[str] = [] + for element in template: + if isinstance(element, list): + group = [substitute(item, bindings) for item in element] + if any(item is None for item in group): + continue + argv.extend(str(item) for item in group) + continue + resolved = substitute(element, bindings) + if resolved is None: + raise HarnessError( + f"argv element {element!r} resolved to null. Wrap it and its flag in a " + f"nested list to make it an optional group, for example " + f'["--mode", "{{{{mode}}}}"].' + ) + argv.append(str(resolved)) + return argv + + +def harvest_from_suite(path: pathlib.Path, helpers: set[str], arg_index: int) -> list[str]: + """Every string literal the suite hands positionally to a named helper. + + Harvesting from the suite means every shape the authors thought worth testing + is covered. f-strings and concatenations are skipped deliberately: their + runtime value depends on per-test fixtures this harness does not reproduce, + and a wrong reconstruction would compare two arms on an input neither test + ever ran. + """ + tree = ast.parse(path.read_text(encoding="utf-8")) + found: list[str] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + if isinstance(func, ast.Attribute): + name = func.attr + elif isinstance(func, ast.Name): + name = func.id + else: + continue + if name not in helpers or len(node.args) <= arg_index: + continue + argument = node.args[arg_index] + if isinstance(argument, ast.Constant) and isinstance(argument.value, str): + found.append(argument.value) + return found + + +def path_spellings(path: pathlib.Path) -> set[str]: + """Every spelling of `path` a subject could plausibly print. + + Both the MSYS and the native forms are included: an arm invoked through a + native Windows interpreter discloses `D:\\...` even when this harness was + handed `/d/...`, and a leak the harness cannot recognize is a leak it will + report as a legitimate mismatch. + """ + literal = {str(path), path.as_posix(), str(path.resolve()), path.resolve().as_posix()} + folded = {pathfix.native_to_msys(item) for item in literal} + folded |= {pathfix.msys_to_native(item) for item in literal} + return {item for item in literal | folded if item} + + +def run_one(argv: list[str], stdin: str | None, timeout: int) -> tuple[int, str, str]: + completed = subprocess.run( # noqa: S603 - argv is caller-declared, never a shell string + argv, + input=stdin if stdin is not None else "", + capture_output=True, + text=True, + timeout=timeout, + check=False, + ) + return completed.returncode, completed.stdout, completed.stderr + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--baseline", required=True) + parser.add_argument("--candidate", required=True) + parser.add_argument("--config", required=True) + parser.add_argument("--harvest-from") + parser.add_argument("--limit", type=int, default=0) + args = parser.parse_args() + + baseline, baseline_note = pathfix.resolve_existing(args.baseline) + candidate, candidate_note = pathfix.resolve_existing(args.candidate) + for note in (baseline_note, candidate_note): + if note: + print(f"NOTE: {note}", file=sys.stderr) + for label, path, given in ( + ("baseline", baseline, args.baseline), + ("candidate", candidate, args.candidate), + ): + if not path.is_file(): + raise HarnessError(pathfix.spellings_message(f"the {label}", given)) + if baseline.resolve() == candidate.resolve(): + raise HarnessError( + f"the baseline and the candidate resolve to the same file ({baseline.resolve()}). " + f"The differential would compare a file with itself and report parity for a " + f"comparison it never made." + ) + + config_path, config_note = pathfix.resolve_existing(args.config) + if config_note: + print(f"NOTE: {config_note}", file=sys.stderr) + if not config_path.is_file(): + raise HarnessError(pathfix.spellings_message("the config", args.config)) + try: + config = json.loads(config_path.read_text(encoding="utf-8")) + except json.JSONDecodeError as error: + raise HarnessError(f"{config_path} is not valid JSON: {error}") from None + argv_template = config.get("argv") + if not isinstance(argv_template, list) or not argv_template: + raise HarnessError("config `argv` must be a non-empty list") + matrix: dict[str, list[object]] = config.get("matrix") or {} + for dimension, values in matrix.items(): + if not isinstance(values, list) or not values: + raise HarnessError(f"matrix dimension {dimension!r} must be a non-empty list") + variables = config.get("vars") or {} + stdin_template = config.get("stdin_json") + timeout = int(config.get("timeout", 120)) + + corpus: list[str] = [] + seen: set[str] = set() + if args.harvest_from: + harvest = config.get("harvest") or {} + helpers = set(harvest.get("helpers") or []) + if not helpers: + raise HarnessError( + "--harvest-from was given but config `harvest.helpers` is empty, so the " + "harvest would silently contribute nothing." + ) + suite_path, suite_note = pathfix.resolve_existing(args.harvest_from) + if suite_note: + print(f"NOTE: {suite_note}", file=sys.stderr) + if not suite_path.is_file(): + raise HarnessError(pathfix.spellings_message("the harvest suite", args.harvest_from)) + harvested = harvest_from_suite( + suite_path, helpers, int(harvest.get("arg_index", 0)) + ) + if not harvested: + raise HarnessError( + f"--harvest-from {args.harvest_from} yielded no literals for helpers " + f"{sorted(helpers)}. An empty harvest that passed would claim coverage " + f"the corpus does not have." + ) + corpus.extend(harvested) + corpus.extend(config.get("corpus") or []) + corpus = [item for item in corpus if not (item in seen or seen.add(item))] + if not corpus: + raise HarnessError("the corpus is empty; there is nothing to compare") + if args.limit > 0: + corpus = corpus[: args.limit] + + dimensions = sorted(matrix) + combinations = [ + dict(zip(dimensions, combo)) + for combo in itertools.product(*(matrix[name] for name in dimensions)) + ] or [{}] + + baseline_leaks = path_spellings(baseline) + candidate_leaks = path_spellings(candidate) + if baseline.resolve().parent != candidate.resolve().parent: + baseline_leaks |= path_spellings(baseline.resolve().parent) + candidate_leaks |= path_spellings(candidate.resolve().parent) + + mismatches: list[str] = [] + disclosures: list[str] = [] + results: dict[str, list[tuple[int, str]]] = {"baseline": [], "candidate": []} + checked = 0 + stderr_differences = 0 + + for combo in combinations: + for command in corpus: + bindings: dict[str, object] = dict(combo) + bindings["__vars__"] = variables + bindings["corpus"] = command + bindings["python"] = sys.executable + + outcome: dict[str, tuple[int, str, str]] = {} + for label, subject, leaks in ( + ("baseline", baseline, baseline_leaks), + ("candidate", candidate, candidate_leaks), + ): + bindings["subject"] = str(subject) + argv = build_argv(argv_template, bindings) + stdin = ( + json.dumps(substitute(stdin_template, bindings)) + if stdin_template is not None + else None + ) + code, out, err = run_one(argv, stdin, timeout) + if code == 127: + raise HarnessError( + f"the {label} arm exited 127 (command not found) on argv {argv!r}. " + f"An arm that never ran cannot disprove a behavior change, and two " + f"arms failing this way identically read as parity." + ) + leaked = sorted(spelling for spelling in leaks if spelling and spelling in out) + if leaked: + disclosures.append( + f" {label} arm disclosed its own location {leaked[0]!r} in stdout " + f"for {command!r} with {combo}" + ) + outcome[label] = (code, out, err) + results[label].append((code, out)) + + checked += 1 + base_code, base_out, base_err = outcome["baseline"] + cand_code, cand_out, cand_err = outcome["candidate"] + # stderr is COUNTED but not compared for parity. Diagnostics carry + # timings, paths and warnings that legitimately differ between two + # copies, so failing on them would drown the signal. Reporting the + # count is what keeps the verdict honest: "byte-identical stdout and + # exit code" is a narrower claim than "behavior did not change", and a + # reader cannot tell the difference if the gap is invisible. + if base_err != cand_err: + stderr_differences += 1 + if base_code != cand_code or base_out != cand_out: + mismatches.append( + f" {combo} command={command!r}\n" + f" baseline rc={base_code} out={base_out.strip()!r}\n" + f" candidate rc={cand_code} out={cand_out.strip()!r}" + ) + + distinct = {label: len(set(rows)) for label, rows in results.items()} + codes = {label: dict(Counter(code for code, _ in rows)) for label, rows in results.items()} + + print(f"corpus items : {len(corpus)}") + print(f"matrix combinations : {len(combinations)}") + print(f"invocations compared : {checked} (x2 arms = {checked * 2} runs)") + print(f"distinct results : baseline={distinct['baseline']} candidate={distinct['candidate']}") + print(f"exit codes : baseline={codes['baseline']} candidate={codes['candidate']}") + + never_exercised = all( + not out and code != 0 for rows in results.values() for code, out in rows + ) and distinct == {"baseline": 1, "candidate": 1} + if never_exercised: + raise HarnessError( + "every invocation in BOTH arms produced empty stdout and the same non-zero exit " + "code. That is not parity, it is a harness that never exercised the subject, and " + "it is the shape that reads as a clean, confident, wrong verdict." + ) + + if disclosures: + print(f"\nSELF-DISCLOSURE: {len(disclosures)}") + for entry in disclosures[:10]: + print(entry) + raise HarnessError( + "an arm printed its own path, so byte-identical stdout is not a valid bar here: " + "the arms can differ purely by living at different paths. Place the two copies in " + "one directory, or normalize the disclosed path, then re-run." + ) + + if mismatches: + print(f"\nMISMATCHES: {len(mismatches)}") + for entry in mismatches: + print(entry) + return 1 + + print("\nPARITY: byte-identical stdout and exit code on every invocation.") + if stderr_differences: + print( + f"SCOPE: stderr differed on {stderr_differences} of {checked} invocations and is " + f"NOT part of the parity bar. Diagnostics carry timings and paths that two copies " + f"legitimately differ on. This verdict covers stdout and the exit code; if stderr " + f"is behavior for this subject, it is UNVERIFIED here." + ) + return 0 + + +if __name__ == "__main__": + try: + raise SystemExit(main()) + except HarnessError as failure: + print(f"HARNESS FAIL: {failure}", file=sys.stderr) + raise SystemExit(2) from None diff --git a/plugins/performance/scripts/differential.test.sh b/plugins/performance/scripts/differential.test.sh new file mode 100755 index 000000000..b0879121c --- /dev/null +++ b/plugins/performance/scripts/differential.test.sh @@ -0,0 +1,214 @@ +#!/usr/bin/env bash +# Tests for differential.py, the pre-change versus post-change behavior proof. +# +# Two cases carry the weight, and neither is about detecting a real difference: +# +# * NEVER EXERCISED. Two arms that both produced nothing and both failed the +# same way are not "parity", they are a harness that never ran the subject. +# Reported as parity, that is a confident wrong verdict about behavior. +# * SELF-DISCLOSURE. Byte-identical stdout is only a valid bar when the arms +# cannot differ merely by living at different paths, so an arm that prints +# its own location invalidates the comparison rather than failing it. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +# shellcheck source=harness-lib.sh +source "$SCRIPT_DIR/harness-lib.sh" +harness_require_python +DIFFERENTIAL="$SCRIPT_DIR/differential.py" +readonly DIFFERENTIAL + +# Inline test helpers: self-contained, no external test lib (ships with the plugin). +FAILED=0 +CASE_NUM=0 +pass() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'PASS: [%d] %s\n' "$CASE_NUM" "$1" +} +fail() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'FAIL: [%d] %s - expected %q got %q\n' "$CASE_NUM" "$1" "$2" "$3" >&2 + FAILED=$((FAILED + 1)) +} +assert_eq() { if [[ "$3" == "$2" ]]; then pass "$1"; else fail "$1" "$2" "$3"; fi; } +assert_contains() { + if [[ "$3" == *"$2"* ]]; then pass "$1"; else fail "$1" "*$2*" "$3"; fi +} +assert_not_contains() { + if [[ "$3" != *"$2"* ]]; then pass "$1"; else fail "$1" "no *$2*" "$3"; fi +} + +WORK="$(mktemp -d)" +readonly WORK +trap 'rm -rf "$WORK"' EXIT +mkdir -p "$WORK/base" "$WORK/cand" + +RUN_OUT="" +RUN_RC=0 +run_differential() { + RUN_OUT="$("$HARNESS_PYTHON" "$DIFFERENTIAL" "$@" 2>&1)" + RUN_RC=$? +} + +# A subject whose verdict depends on the mode it was given and the command it +# was handed on stdin. The optional-group form in the config is what lets the +# matrix cover "no --mode at all", the shape a caller gets by default. +cat >"$WORK/base/subject.py" <<'SUBJECT' +import json +import sys + +mode = sys.argv[2] if len(sys.argv) > 2 else "none" +command = json.load(sys.stdin)["tool_input"]["command"] +verdict = "DENY" if "rm -rf" in command else "ALLOW" +print(f"{mode}:{verdict}") +sys.exit(2 if verdict == "DENY" else 0) +SUBJECT +cp "$WORK/base/subject.py" "$WORK/cand/subject.py" + +cat >"$WORK/config.json" <<'CONFIG' +{ + "argv": ["{{python}}", "{{subject}}", ["--mode", "{{mode}}"]], + "matrix": {"mode": [null, "belt"]}, + "corpus": ["git status --porcelain", "rm -rf /"], + "stdin_json": {"tool_input": {"command": "{{corpus}}"}} +} +CONFIG + +# --- 1. identical arms report parity, and the matrix really ran --- +run_differential --baseline "$WORK/base/subject.py" --candidate "$WORK/cand/subject.py" \ + --config "$WORK/config.json" +assert_eq "identical arms exit 0" "0" "$RUN_RC" +assert_contains "parity is stated in full" "PARITY: byte-identical stdout and exit code" "$RUN_OUT" +assert_contains "both matrix combinations ran" "matrix combinations : 2" "$RUN_OUT" +assert_contains "all four invocations were compared" "invocations compared : 4" "$RUN_OUT" +# Four distinct results prove the null-mode optional group and the corpus both +# discriminate; one distinct result would mean the matrix changed nothing. +assert_contains "the corpus and matrix produced distinct results" "baseline=4 candidate=4" "$RUN_OUT" + +# --- 1b. a stderr-only difference is DISCLOSED, not silently outside the bar --- +# stderr is deliberately not part of the parity bar, because diagnostics carry +# timings and paths two copies legitimately differ on. What must not happen is +# the gap being invisible: "byte-identical stdout and exit code" is a narrower +# claim than "behavior did not change", and a reader cannot tell them apart +# unless the difference is stated. +# discriminating-skip-required: this case is the only thing standing between a +# narrow verdict and a reader who believes it was a broad one. +mkdir -p "$WORK/noisy" +cat >"$WORK/base/noisy.py" <<'QUIET' +import sys + +print("same stdout") +QUIET +cat >"$WORK/cand/noisy.py" <<'NOISY' +import sys + +print("same stdout") +print("a warning only the candidate emits", file=sys.stderr) +NOISY +cat >"$WORK/noisy-config.json" <<'NOISYCONFIG' +{ + "argv": ["{{python}}", "{{subject}}"], + "corpus": ["only-one"] +} +NOISYCONFIG +run_differential --baseline "$WORK/base/noisy.py" --candidate "$WORK/cand/noisy.py" \ + --config "$WORK/noisy-config.json" +assert_eq "a stderr-only difference still reports parity" "0" "$RUN_RC" +assert_contains "the parity verdict is still stated" "PARITY:" "$RUN_OUT" +assert_contains "the stderr gap is disclosed with a count" \ + "stderr differed on 1 of 1 invocations" "$RUN_OUT" +assert_contains "the disclosure names what is unverified" "it is UNVERIFIED here" "$RUN_OUT" + +# --- 2. a real behavior change is reported as a mismatch, not smoothed over --- +cat >"$WORK/cand/subject.py" <<'CHANGED' +import json +import sys + +mode = sys.argv[2] if len(sys.argv) > 2 else "none" +command = json.load(sys.stdin)["tool_input"]["command"] +verdict = "ASK" if "rm -rf" in command else "ALLOW" +print(f"{mode}:{verdict}") +sys.exit(1 if verdict == "ASK" else 0) +CHANGED +run_differential --baseline "$WORK/base/subject.py" --candidate "$WORK/cand/subject.py" \ + --config "$WORK/config.json" +assert_eq "a changed arm exits 1" "1" "$RUN_RC" +assert_contains "the mismatch count is reported" "MISMATCHES: 2" "$RUN_OUT" +assert_contains "the mismatch shows both verdicts" "DENY" "$RUN_OUT" +assert_not_contains "a mismatching run never claims parity" "PARITY:" "$RUN_OUT" + +# --- 3. two arms that never ran are refused, NOT reported as parity --- +# discriminating-skip-required: this case is the only proof that a symmetric +# failure of both arms is distinguished from genuine parity. +cat >"$WORK/base/dead.py" <<'DEAD' +import sys + +sys.exit(3) +DEAD +cp "$WORK/base/dead.py" "$WORK/cand/dead.py" +run_differential --baseline "$WORK/base/dead.py" --candidate "$WORK/cand/dead.py" \ + --config "$WORK/config.json" +assert_eq "two identically-failing silent arms are refused" "2" "$RUN_RC" +assert_contains "the refusal denies it is parity" "That is not parity" "$RUN_OUT" +assert_contains "the refusal names the confident-wrong-verdict shape" "confident, wrong verdict" "$RUN_OUT" +assert_not_contains "a never-exercised run never claims parity" "PARITY:" "$RUN_OUT" + +# --- 4. an arm that discloses its own path invalidates byte-exactness --- +cat >"$WORK/base/leaky.py" <<'LEAKY' +import sys + +print(__file__) +LEAKY +cp "$WORK/base/leaky.py" "$WORK/cand/leaky.py" +cat >"$WORK/leaky-config.json" <<'LEAKYCONFIG' +{ + "argv": ["{{python}}", "{{subject}}"], + "corpus": ["only-one"] +} +LEAKYCONFIG +run_differential --baseline "$WORK/base/leaky.py" --candidate "$WORK/cand/leaky.py" \ + --config "$WORK/leaky-config.json" +assert_eq "an arm disclosing its own path is refused" "2" "$RUN_RC" +assert_contains "the disclosure is reported" "SELF-DISCLOSURE:" "$RUN_OUT" +assert_contains "the refusal explains why byte-exactness is invalid here" \ + "differ purely by living at different paths" "$RUN_OUT" + +# --- 5. preconditions --- +run_differential --baseline "$WORK/base/subject.py" --candidate "$WORK/base/subject.py" \ + --config "$WORK/config.json" +assert_eq "comparing a file with itself is refused" "2" "$RUN_RC" +assert_contains "the refusal names the empty comparison" "compare a file with itself" "$RUN_OUT" + +cat >"$WORK/empty-config.json" <<'EMPTY' +{"argv": ["{{python}}", "{{subject}}"], "corpus": []} +EMPTY +run_differential --baseline "$WORK/base/subject.py" --candidate "$WORK/cand/subject.py" \ + --config "$WORK/empty-config.json" +assert_eq "an empty corpus is refused" "2" "$RUN_RC" + +run_differential --baseline "$WORK/base/does-not-exist.py" --candidate "$WORK/cand/subject.py" \ + --config "$WORK/config.json" +assert_eq "a missing baseline is refused" "2" "$RUN_RC" +assert_contains "the refusal lists the spellings tried" "any spelling this harness tried" "$RUN_OUT" + +# --- 6. a harvest that finds nothing is refused, never silently empty --- +cat >"$WORK/suite.py" <<'SUITE' +def test_nothing(): + assert True +SUITE +cat >"$WORK/harvest-config.json" <<'HARVEST' +{ + "argv": ["{{python}}", "{{subject}}"], + "corpus": ["fallback"], + "harvest": {"helpers": ["run_guard"], "arg_index": 0} +} +HARVEST +run_differential --baseline "$WORK/base/subject.py" --candidate "$WORK/cand/subject.py" \ + --config "$WORK/harvest-config.json" --harvest-from "$WORK/suite.py" +assert_eq "an empty harvest is refused" "2" "$RUN_RC" +assert_contains "the refusal names the false coverage claim" "claim coverage" "$RUN_OUT" + +[[ "${FAILED:-0}" -eq 0 ]] || exit 1 +echo "OK: differential parity and refusals" +exit 0 diff --git a/plugins/performance/scripts/discriminate.py b/plugins/performance/scripts/discriminate.py new file mode 100644 index 000000000..03870d281 --- /dev/null +++ b/plugins/performance/scripts/discriminate.py @@ -0,0 +1,446 @@ +"""Does this check actually FAIL without the fix? + +A test that passes both with and without the fix proves nothing. This patches a +target file to disable the fix, runs the check, restores the file, runs the +check again, and reports whether the two arms genuinely differ. + +Consolidated from five source-run variants, four of which were broken. The +fixed semantics are these, and each one is a defect that shipped: + +1. THE ARMS MUST DIFFER, on an extracted SIGNAL rather than on raw output. + Raw stdout differs almost every run on noise alone (`Ran 1 test in 0.003s`), + so a raw-output comparison would always pass and would silently defeat the + whole check. Four of five source failures were both arms exiting 127 + identically and reporting a confident "NOT DISCRIMINATING". + +2. THE SIGNAL MUST APPEAR IN AT LEAST ONE ARM. A pattern that matches in + neither arm is not a negative result, it is a check that never ran. This is + the assertion that catches source failures 3 and 4, where a `D:/...` path + handed to bash resolved nowhere and the grep found nothing in either arm. + +3. THE PATCH MUST HAVE APPLIED. The anchor must occur exactly once, the patched + bytes must differ from the original, and the patched bytes must be what is + actually on disk before the arm runs. + +4. RESTORE FROM SAVED BYTES, NEVER FROM VERSION CONTROL. This script never + invokes git to restore. `git checkout --` is correct only when the code under + test is already committed and is actively destructive otherwise: source + failure 5 reverted the uncommitted fix it was verifying, so the "with fix" + arm ran without it and the work was destroyed. The pre-patch bytes are + written to a SIDECAR FILE ON DISK before anything is mutated, so a kill + between patch and restore leaves a recoverable copy rather than nothing. The + restore is then VERIFIED by byte comparison; "I restored it" is not evidence. + +An uncommitted target is a loud WARNING rather than a failure. Committing first +is the doctrine's own cheap mitigation for restoring badly, and the sidecar plus +the verified restore is the enforcement. + +Usage: + discriminate.py --config [--keep-backup] + +Exit: 0 DISCRIMINATING; 1 NOT DISCRIMINATING; 2 HARNESS BROKEN or a precondition +failed. The 1/2 split is the point: "the check does not discriminate" and "the +harness never ran" are different findings, and conflating them is what produced +four confident wrong verdicts. + +Config (JSON): + { + "target": "/d/worktrees/repo/hooks/run-hook.sh", + "anchor": " interp_base=\"${interp_base##*\\\\}\"", + "replacement": " : # split disabled for the discrimination check", + "check": { + "argv": ["bash", "hooks/run-hook.test.sh"], + "cwd": "/d/worktrees/repo", + "timeout": 900 + }, + "signal": {"regex": "^(PASS|FAIL): a native Windows interpreter path.*$"}, + "expect": {"negative_contains": "FAIL:", "positive_contains": "PASS:"} + } + + `anchor_file` and `replacement_file` may be used instead, for content awkward + to embed. With no `signal`, the signal is the check's exit code and the + expectation is non-zero without the fix and zero with it. + + `argv` is handed straight to the operating system and is deliberately NOT + rewritten: a bash check needs MSYS paths in its arguments and a native check + needs native ones, so rewriting would break whichever the caller meant. Name + the check RELATIVE to `cwd`, which this harness does resolve, rather than + embedding an absolute path in either spelling. +""" + +from __future__ import annotations + +import argparse +import json +import os +import pathlib +import re +import subprocess +import sys + +import pathfix + +BACKUP_SUFFIX = ".discriminate-backup" + + +def note(message: str | None) -> None: + if message: + print(f"NOTE: {message}", file=sys.stderr) + + +class HarnessError(Exception): + """The harness could not run, or could not have measured what it claims.""" + + +def load_bytes(config: dict[str, object], key: str, base: pathlib.Path) -> bytes: + inline = config.get(key) + from_file = config.get(f"{key}_file") + if inline is not None and from_file is not None: + raise HarnessError(f"config sets both {key!r} and {key}_file; pick one") + if inline is not None: + if not isinstance(inline, str): + raise HarnessError(f"config {key!r} must be a string") + return inline.encode("utf-8") + if from_file is None: + raise HarnessError(f"config must set either {key!r} or {key}_file") + path, conversion = pathfix.resolve_existing(str(from_file)) + note(conversion) + if not path.is_absolute(): + path = base / path + if not path.is_file(): + raise HarnessError(pathfix.spellings_message(f"{key}_file", str(from_file))) + return path.read_bytes() + + +def run_check(check: dict[str, object]) -> subprocess.CompletedProcess[str]: + argv = check.get("argv") + if not isinstance(argv, list) or not argv: + raise HarnessError("config `check.argv` must be a non-empty list") + # `cwd` is handed to the OS process launcher, so on Windows it needs the + # NATIVE spelling. `argv` is deliberately left untouched: argv[0] is usually + # bash, and bash needs the MSYS spelling for its own arguments. Converting + # both would break one of them, and this is the one place the two rules meet. + cwd = check.get("cwd") + resolved_cwd: str | None = None + if cwd: + cwd_path, conversion = pathfix.resolve_existing(str(cwd)) + note(conversion) + if not cwd_path.is_dir(): + raise HarnessError(pathfix.spellings_message("check.cwd", str(cwd))) + resolved_cwd = str(cwd_path) + try: + return subprocess.run( # noqa: S603 - argv is caller-declared, never a shell string + [str(item) for item in argv], + cwd=resolved_cwd, + capture_output=True, + text=True, + timeout=int(check.get("timeout", 900)), + check=False, + ) + except FileNotFoundError as error: + # A check that could not be LAUNCHED is a broken harness, not a failing + # arm. Letting the OSError escape as a traceback would strand the caller + # with no verdict at all, and the restore still runs either way because + # the caller wraps this in a finally. + raise HarnessError( + f"the check command could not be launched: {argv[0]!r} ({error}). Nothing was " + f"measured. On a mixed MSYS and native-Windows host, remember that argv is " + f"handed straight to the OS: a bash check needs MSYS paths in its arguments, " + f"and a native check needs native ones. This harness deliberately does not " + f"rewrite argv, because rewriting it would break whichever of the two the " + f"caller actually meant." + ) from None + except subprocess.TimeoutExpired as error: + raise HarnessError( + f"the check command timed out after {error.timeout}s. A timeout in one arm and " + f"not the other would look like discrimination, so this is refused rather than " + f"scored." + ) from None + + +def combined(result: subprocess.CompletedProcess[str]) -> str: + return result.stdout + result.stderr + + +def extract_signal( + pattern: re.Pattern[str] | None, result: subprocess.CompletedProcess[str] +) -> str | None: + if pattern is None: + return f"rc={result.returncode}" + found = pattern.search(combined(result)) + return found.group(0) if found else None + + +def last_line(text: str) -> str: + lines = text.strip().splitlines() + return lines[-1] if lines else "(no output)" + + +def warn_uncommitted(target: pathlib.Path) -> None: + """Rule 5, reported rather than enforced: committing first makes an + accidental clobber recoverable, and it costs nothing. The enforcement that + actually protects the file is the sidecar and the verified restore.""" + # `-C` changes directory, it does not isolate. An inherited absolute GIT_DIR + # overrides repository discovery and GIT_CONFIG replaces the file git reads, + # so an ambient environment would make this warning describe a different + # repository than the one holding the target. + environment = { + key: value + for key, value in os.environ.items() + if key not in {"GIT_DIR", "GIT_WORK_TREE", "GIT_CONFIG"} + } + try: + result = subprocess.run( + ["git", "-C", str(target.parent), "status", "--porcelain", "--", target.name], + capture_output=True, + text=True, + timeout=60, + check=False, + env=environment, + ) + except (OSError, subprocess.SubprocessError) as error: + print(f"HARNESS WARNING: cannot ask git about {target}: {error}", file=sys.stderr) + return + if result.returncode != 0: + print( + f"HARNESS WARNING: {target} is not inside a git working tree, so this run " + f"cannot confirm the code under test is committed.", + file=sys.stderr, + ) + return + if result.stdout.strip(): + print( + f"HARNESS WARNING: {target} has uncommitted changes. Committing before " + f"verifying is the cheapest way to make an accidental clobber recoverable. " + f"This run restores from saved bytes and never from git, so the uncommitted " + f"work is safe here, but commit it anyway.", + file=sys.stderr, + ) + + +def verdict(code: int, headline: str, detail: str) -> int: + print(f"\nVERDICT: {headline}") + print(detail) + return code + + +def main() -> int: + parser = argparse.ArgumentParser(description="Prove a check fails without the fix.") + parser.add_argument("--config", required=True) + parser.add_argument( + "--keep-backup", + action="store_true", + help="keep the pre-patch sidecar even after the restore verifies", + ) + args = parser.parse_args() + + config_path, conversion = pathfix.resolve_existing(args.config) + note(conversion) + if not config_path.is_file(): + raise HarnessError(pathfix.spellings_message("the config", args.config)) + config_path = config_path.resolve() + try: + config = json.loads(config_path.read_text(encoding="utf-8")) + except json.JSONDecodeError as error: + raise HarnessError(f"{config_path} is not valid JSON: {error}") from None + base = config_path.parent + + given_target = str(config.get("target", "")) + target, conversion = pathfix.resolve_existing(given_target) + note(conversion) + if not target.is_absolute(): + target = base / target + if not target.is_file(): + raise HarnessError(pathfix.spellings_message("target", given_target)) + + check = config.get("check") + if not isinstance(check, dict): + raise HarnessError("config `check` must be an object with an `argv` list") + + signal_config = config.get("signal") + pattern: re.Pattern[str] | None = None + if signal_config is not None: + if not isinstance(signal_config, dict) or "regex" not in signal_config: + raise HarnessError("config `signal` must be an object with a `regex`") + pattern = re.compile(str(signal_config["regex"]), re.MULTILINE) + + expect = config.get("expect") or {} + + original = target.read_bytes() + anchor = load_bytes(config, "anchor", base) + replacement = load_bytes(config, "replacement", base) + + occurrences = original.count(anchor) + if occurrences != 1: + hint = "" + if occurrences == 0 and b"\r\n" in original and b"\r\n" not in anchor: + hint = ( + " The target has CRLF line endings and the anchor does not. On a checkout " + "with core.autocrlf=true that alone is enough to miss." + ) + raise HarnessError( + f"the anchor occurs {occurrences} times in {target}; it must occur exactly " + f"once, or the patch is ambiguous and the arms would not test what you " + f"think.{hint}" + ) + + patched = original.replace(anchor, replacement) + if patched == original: + raise HarnessError( + "the patch changed nothing: the replacement is byte-identical to the anchor. " + "A patch that silently applied nothing makes both arms the same run, which is " + "how a harness reports a confident verdict about a check it never varied." + ) + + if config.get("backup_dir"): + backup_dir, conversion = pathfix.resolve_existing(str(config["backup_dir"])) + note(conversion) + else: + backup_dir = target.parent + backup = backup_dir / (target.name + BACKUP_SUFFIX) + backup.parent.mkdir(parents=True, exist_ok=True) + backup.write_bytes(original) + if backup.read_bytes() != original: + raise HarnessError( + f"the pre-patch sidecar at {backup} does not match the target's bytes. " + f"Refusing to mutate {target} without a verified copy to restore from." + ) + + warn_uncommitted(target) + + negative: subprocess.CompletedProcess[str] | None = None + restore_failure: str | None = None + try: + target.write_bytes(patched) + if target.read_bytes() != patched: + raise HarnessError( + f"the patched bytes did not reach disk at {target}; the negative arm would " + f"have run against the unpatched file and reported a false 'not " + f"discriminating'." + ) + negative = run_check(check) + finally: + target.write_bytes(original) + if target.read_bytes() != original: + restore_failure = ( + f"RESTORE FAILED. {target} does not match its pre-patch bytes. The saved " + f"copy is at {backup.resolve()}; restore it by hand before doing anything " + f"else in this tree." + ) + # Printed here as well as raised below, because an exception already in + # flight would otherwise swallow the one message that names the sidecar. + print(f"HARNESS FAIL: {restore_failure}", file=sys.stderr) + elif not args.keep_backup: + # Removed HERE rather than after the try, so a check that could not be + # launched does not strand a *.discriminate-backup beside a source file + # in a live worktree, where the next `git add -A` would sweep it up. The + # copy is kept for exactly the one case that needs it: a restore that + # did not verify. + backup.unlink(missing_ok=True) + + if restore_failure is not None: + raise HarnessError(restore_failure) + + assert negative is not None + positive = run_check(check) + + negative_signal = extract_signal(pattern, negative) + positive_signal = extract_signal(pattern, positive) + + print(f"WITHOUT the fix (patched) : rc={negative.returncode} " + f"signal={negative_signal!r} last={last_line(combined(negative))!r}") + print(f"WITH the fix (restored) : rc={positive.returncode} " + f"signal={positive_signal!r} last={last_line(combined(positive))!r}") + print(f"restore verified byte-identical: {target.read_bytes() == original}") + + if 127 in (negative.returncode, positive.returncode): + arms = [ + name + for name, result in (("negative", negative), ("positive", positive)) + if result.returncode == 127 + ] + return verdict( + 2, + "HARNESS BROKEN", + f"the check command exited 127 (command not found) in the {', '.join(arms)} arm(s). " + f"Under MSYS that is what a D:/... path handed to bash produces, and it is how " + f"four of the five source-run harnesses reported a confident verdict for a check " + f"that never ran. Hand bash a /d/... path.", + ) + + if negative_signal is None and positive_signal is None: + return verdict( + 2, + "HARNESS BROKEN", + "the signal pattern matched in NEITHER arm, so this run observed nothing. An " + "absent signal in both arms is not a negative result, it is a check that never " + "ran. Verify the check command actually executes and that the pattern matches " + "its real output.", + ) + + negative_ok = ( + expect["negative_contains"] in (negative_signal or "") + if "negative_contains" in expect + else negative.returncode != 0 + ) + positive_ok = ( + expect["positive_contains"] in (positive_signal or "") + if "positive_contains" in expect + else positive.returncode == 0 + ) + + if negative_signal == positive_signal: + # Identical arms split on WHETHER THE SHARED OUTCOME IS A FAILURE, and the + # split is the whole point. Two arms that both FAILED the same way are + # indistinguishable from two arms that never ran: 127 is only the most + # visible spelling of that, and with no `signal` regex the signal is the + # exit code, so any shared non-zero code lands here. Scoring it as a + # discrimination verdict would be a confident claim about a check that may + # never have executed. Two arms that both PASSED are a different and + # genuinely knowable finding: the check cannot fail. + if negative_ok: + return verdict( + 2, + "HARNESS BROKEN", + f"both arms produced the IDENTICAL FAILING signal {negative_signal!r}. This " + f"harness cannot tell a check that fails the same way with and without the " + f"patch from a check that never ran at all, so it refuses to score either. " + f"The patch itself WAS verified applied on disk, so that is not the cause. " + f"Confirm the check command executes and that it exercises the patched code " + f"path, then re-run.", + ) + return verdict( + 1, + "NOT DISCRIMINATING (the arms did not differ)", + f"both arms produced the IDENTICAL PASSING signal {negative_signal!r}. The check " + f"passes whether the fix is present or not, so it proves nothing about it. The " + f"patch WAS verified applied on disk, so this is not a patch that failed to land.", + ) + + if negative_ok and positive_ok: + return verdict( + 0, + "DISCRIMINATING", + "the check failed without the fix, passed with it, and the two arms differ.", + ) + if not negative_ok: + return verdict( + 1, + "NOT DISCRIMINATING", + "the check did NOT fail with the fix disabled, so it passes whether the fix is " + "present or not and proves nothing about it.", + ) + return verdict( + 1, + "NOT DISCRIMINATING", + "the check failed WITH the fix present, so the check itself is broken independently " + "of the fix under test.", + ) + + +if __name__ == "__main__": + try: + raise SystemExit(main()) + except HarnessError as failure: + print(f"HARNESS FAIL: {failure}", file=sys.stderr) + raise SystemExit(2) from None diff --git a/plugins/performance/scripts/discriminate.test.sh b/plugins/performance/scripts/discriminate.test.sh new file mode 100755 index 000000000..72ba1b148 --- /dev/null +++ b/plugins/performance/scripts/discriminate.test.sh @@ -0,0 +1,320 @@ +#!/usr/bin/env bash +# Tests for discriminate.py, the does-this-check-actually-fail harness. +# +# This is the harness consolidated from five source-run variants, four of which +# returned a confident wrong answer. Every one of those defects has a case here, +# and each asserts the FIXED behavior: +# +# * both arms exiting 127 must report HARNESS BROKEN, not a verdict; +# * a signal absent from both arms must report HARNESS BROKEN, not a verdict; +# * a check that cannot even be launched must report HARNESS BROKEN, not a +# traceback and not a verdict; +# * a patch that changed nothing must be refused before either arm runs; +# * an UNCOMMITTED change to the target must survive the run, because the +# restore comes from saved bytes and never from `git checkout --`. +# +# The last one is a BEHAVIORAL proof rather than an inspection of the source: +# the target is committed WITHOUT the fix, the fix is applied and left +# uncommitted, and the case asserts the fix is still there afterwards. A +# `git checkout --` restore would have destroyed it, which is exactly what +# source failure 5 did. +# +# The fixtures are Python rather than shell so that `check.argv` needs no path +# spelling at all: the check is named RELATIVE to `check.cwd`, which the harness +# resolves. That is deliberate, not incidental. argv is handed straight to the +# operating system with no MSYS rewriting, so a bash check needs MSYS paths in +# its arguments while a native check needs native ones, and a fixture that +# hardcoded either spelling would test the host rather than the harness. +set -uo pipefail + +# This suite BUILDS a git fixture. `git -C` changes directory, it does not +# isolate: an inherited absolute GIT_DIR overrides repository discovery, and +# GIT_CONFIG replaces the file `git config` writes, so the fixture's identity +# would land in the CALLER's repository. Clear the environment. +unset GIT_DIR GIT_WORK_TREE GIT_CONFIG + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +# shellcheck source=harness-lib.sh +source "$SCRIPT_DIR/harness-lib.sh" +harness_require_python +DISCRIMINATE="$SCRIPT_DIR/discriminate.py" +readonly DISCRIMINATE + +# Inline test helpers: self-contained, no external test lib (ships with the plugin). +FAILED=0 +CASE_NUM=0 +pass() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'PASS: [%d] %s\n' "$CASE_NUM" "$1" +} +fail() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'FAIL: [%d] %s - expected %q got %q\n' "$CASE_NUM" "$1" "$2" "$3" >&2 + FAILED=$((FAILED + 1)) +} +assert_eq() { if [[ "$3" == "$2" ]]; then pass "$1"; else fail "$1" "$2" "$3"; fi; } +assert_contains() { + if [[ "$3" == *"$2"* ]]; then pass "$1"; else fail "$1" "*$2*" "$3"; fi +} +assert_not_contains() { + if [[ "$3" != *"$2"* ]]; then pass "$1"; else fail "$1" "no *$2*" "$3"; fi +} + +# Not under the system temporary root. /tmp is an MSYS MOUNT with no drive +# letter, so a native Windows interpreter reaches it only through cygpath, and +# threading that through every fixture would test the mount table rather than +# the harness. +WORK="${PERF_HARNESS_TEST_ROOT:-$HOME/.cache/performance-harness-tests}/discriminate.$$" +mkdir -p "$WORK" +trap 'rm -rf "$WORK"' EXIT + +RUN_OUT="" +RUN_RC=0 +run_discriminate() { + RUN_OUT="$("$HARNESS_PYTHON" "$DISCRIMINATE" "$@" 2>&1)" + RUN_RC=$? +} + +# Single-quoted in Python so the anchor embeds into a JSON config verbatim. A +# double quote here would need JSON escaping, and getting that wrong is a +# fixture bug that reads as a harness bug. +FIX_LINE="value = value.removesuffix('-suffix')" + +write_subject() { + cat >"$1/subject.py" <"$1/check.py" <<'CHECK' +import pathlib +import subprocess +import sys + +here = pathlib.Path(__file__).resolve().parent +done = subprocess.run( + [sys.executable, str(here / "subject.py"), "abc-suffix"], + capture_output=True, + text=True, + check=False, +) +out = done.stdout.strip() +if out == "abc": + print("PASS: the suffix is stripped") + sys.exit(0) +print(f"FAIL: the suffix is not stripped (got {out!r})") +sys.exit(1) +CHECK +} + +write_config() { + local dir="$1" name="$2" argv="$3" + cat >"$dir/$name" </dev/null 2>&1 +git -C "$WORK/git" config user.email harness@example.invalid +git -C "$WORK/git" config user.name "Harness Test" +write_check "$WORK/git" +# Commit the subject WITHOUT the fix, so the committed blob is the broken one. +cat >"$WORK/git/subject.py" <<'BROKEN' +import sys + +value = sys.argv[1] +print(value) +BROKEN +git -C "$WORK/git" add -A >/dev/null 2>&1 +git -C "$WORK/git" commit --quiet -m "subject without the fix" >/dev/null 2>&1 +# Now apply the fix and leave it UNCOMMITTED. This is the exact situation that +# destroyed the work in the source run. +write_subject "$WORK/git" +write_config "$WORK/git" config.json "$CHECK_ARGV" + +run_discriminate --config "$WORK/git/config.json" +assert_eq "the run over an uncommitted fix exits 0" "0" "$RUN_RC" +assert_contains "the uncommitted target is warned about" "has uncommitted changes" "$RUN_OUT" +if grep -q 'removesuffix' "$WORK/git/subject.py"; then + pass "the UNCOMMITTED fix survived the run" +else + fail "the UNCOMMITTED fix survived the run" "the fix line still present" "$(cat "$WORK/git/subject.py")" +fi + +# --- 3. both arms exiting 127 report HARNESS BROKEN, not a verdict --- +# This is source failures 3 and 4 verbatim: a path that resolved nowhere made +# both arms exit 127, and the harness reported a confident "NOT DISCRIMINATING". +# discriminating-skip-required: without this case nothing proves the harness +# distinguishes "the check never ran" from "the check does not discriminate". +mkdir -p "$WORK/dead127" +write_subject "$WORK/dead127" +cat >"$WORK/dead127/check.py" <<'DEAD127' +import sys + +sys.exit(127) +DEAD127 +write_config "$WORK/dead127" config.json "$CHECK_ARGV" +run_discriminate --config "$WORK/dead127/config.json" +assert_eq "two arms exiting 127 are refused" "2" "$RUN_RC" +assert_contains "the refusal says the harness is broken" "HARNESS BROKEN" "$RUN_OUT" +assert_contains "the refusal names the 127 shape" "exited 127" "$RUN_OUT" +assert_not_contains "it does not report a discrimination verdict" "VERDICT: NOT DISCRIMINATING" "$RUN_OUT" +assert_not_contains "it does not report success" "VERDICT: DISCRIMINATING" "$RUN_OUT" + +# --- 4. a check that cannot be LAUNCHED reports HARNESS BROKEN, not a traceback --- +mkdir -p "$WORK/unlaunchable" +write_subject "$WORK/unlaunchable" +write_config "$WORK/unlaunchable" config.json '["definitely-not-a-real-binary-xyz"]' +run_discriminate --config "$WORK/unlaunchable/config.json" +assert_eq "an unlaunchable check is refused" "2" "$RUN_RC" +assert_contains "the refusal states nothing was measured" "Nothing was measured" "$RUN_OUT" +assert_not_contains "no traceback reaches the operator" "Traceback (most recent call last)" "$RUN_OUT" +assert_eq "the target survives an unlaunchable check" \ + "$(cat "$WORK/ok/subject.py")" "$(cat "$WORK/unlaunchable/subject.py")" + +# --- 5. a signal absent from BOTH arms reports HARNESS BROKEN --- +mkdir -p "$WORK/nosignal" +write_subject "$WORK/nosignal" +write_check "$WORK/nosignal" +cat >"$WORK/nosignal/config.json" <"$WORK/rcsignal/config.json" <"$WORK/weak/check.py" <<'WEAK' +print("PASS: the suffix is ignored by this check") +WEAK +write_config "$WORK/weak" config.json "$CHECK_ARGV" +run_discriminate --config "$WORK/weak/config.json" +assert_eq "a check that cannot fail exits 1" "1" "$RUN_RC" +assert_contains "the identical PASSING arms are named" "IDENTICAL PASSING signal" "$RUN_OUT" +assert_contains "the report rules out a patch that failed to land" "verified applied on disk" "$RUN_OUT" + +# --- 8. patch preconditions --- +mkdir -p "$WORK/anchor" +write_subject "$WORK/anchor" +write_check "$WORK/anchor" + +cat >"$WORK/anchor/missing.json" <"$WORK/anchor/noop.json" <"$WORK/anchor/twice.py" +cat >"$WORK/anchor/twice.json" < +# +# Terminate the CALLING script. Exit status 2 is reserved across this directory +# for "the harness could not run", kept distinct from a subject's own failure so +# a caller can tell "the thing under test is broken" from "the instrument is". +# +# Never call this from inside $( ), where `exit` would leave only the +# substitution's subshell and the caller would sail on with an empty string. +harness_die() { + printf 'HARNESS FAIL: %s\n' "$*" >&2 + exit 2 +} + +# harness_warn +harness_warn() { + printf 'HARNESS WARNING: %s\n' "$*" >&2 +} + +# harness_posix_form +# +# Print the MSYS spelling of a path: backslashes folded to forward slashes and a +# drive letter rewritten as a root segment, so `D:\a\b` and `D:/a/b` both become +# `/d/a/b`. Pure string work, no filesystem access, so it is safe to call on a +# path that does not exist (a Windows TEMP value bash cannot cd into, say). +harness_posix_form() { + local value="$1" + value="${value//\\//}" + if [[ "$value" =~ ^([A-Za-z]):(/.*)?$ ]]; then + local drive rest + drive="${BASH_REMATCH[1]}" + rest="${BASH_REMATCH[2]}" + [[ -n "$rest" ]] || rest="/" + printf '/%s%s\n' "${drive,,}" "$rest" + return 0 + fi + printf '%s\n' "$value" +} + +# harness_require_posix_path +# +# Refuse a Windows drive-letter path anywhere in . Matching is not +# anchored, so an embedded one inside a `bash -c` command string is caught too: +# that is the shape the source-run harnesses actually shipped. +harness_require_posix_path() { + local what="$1" value="$2" + local drive_letter_re='(^|[^A-Za-z0-9])[A-Za-z]:[/\]' + if [[ "$value" =~ $drive_letter_re ]]; then + harness_die "$what carries a Windows drive-letter path: '$value'. Under MSYS a D:/... path handed to bash resolves nowhere, so both arms of a check fail identically and the harness reports a confident wrong verdict (harness-integrity.md rule 6, source failures 3 and 4). Use the POSIX spelling: /d/... rather than D:/... . Pass --allow-windows-paths only if the subject is a native Windows program that genuinely needs the native form." + fi +} + +# harness_real_dir +# +# Print the PHYSICAL path of when it exists, and its POSIX spelling +# otherwise. Resolving through the filesystem rather than comparing strings is +# load-bearing on Windows: this host's TEMP is the 8.3 short form +# `C:\Users\KYLESE~1\AppData\Local\Temp`, while the same directory reached +# through `/tmp` reports `/c/Users/KyleSexton/AppData/Local/Temp`. A string +# prefix test between those two spellings finds nothing, and a temp-rooted shim +# directory would sail straight through the check meant to catch it. +harness_real_dir() { + local posix + posix="$(harness_posix_form "$1")" + (cd "$posix" 2>/dev/null && pwd -P) || printf '%s\n' "$posix" +} + +# harness_resolve_shim_dir +# +# Sets the global HARNESS_SHIM_DIR to a resolved, stability-checked shim +# directory. Call it directly, never inside $( ), so harness_die can terminate. +# +# The shim directory goes on PATH for the subject, so under rule 1 it must be +# FIXED across runs. The canonical defect this refuses is `mktemp -d`: a fresh +# directory each run changed PATH each run, the subject cached keyed on PATH, +# every run was a forced cache miss, and the census reported "no improvement" +# while measuring nothing but its own randomization. +harness_resolve_shim_dir() { + local dir="$1" + if [[ -z "$dir" ]]; then + harness_die "a shim directory is required and deliberately has no default. It is prepended to the subject's PATH, so under harness-integrity.md rule 1 it must be the SAME directory on every run. Pass --shim-dir with a path that persists." + fi + harness_require_posix_path "the shim directory" "$dir" + + local parent base resolved_parent + parent="$(dirname "$dir")" + base="$(basename "$dir")" + [[ -d "$parent" ]] || harness_die "the shim directory's parent does not exist: $parent" + resolved_parent="$(cd "$parent" && pwd -P)" || harness_die "cannot resolve the shim directory's parent: $parent" + local resolved="$resolved_parent/$base" + + # Reject the system temporary roots by RESOLVED PREFIX, not by substring: a + # legitimate directory whose name merely contains "tmp" must not misfire. + local normalized root normalized_root + normalized="$(harness_real_dir "$resolved_parent")/$base" + normalized="${normalized,,}" + for root in "${TMPDIR:-}" "${TMP:-}" "${TEMP:-}" /tmp /var/tmp; do + [[ -n "$root" ]] || continue + normalized_root="$(harness_real_dir "$root")" + normalized_root="${normalized_root,,}" + normalized_root="${normalized_root%/}" + [[ -n "$normalized_root" ]] || continue + if [[ "$normalized" == "$normalized_root" || "$normalized" == "$normalized_root"/* ]]; then + harness_die "the shim directory '$dir' resolves under the temporary root '$root'. A per-run temporary directory is the exact defect this check exists to stop: it changes PATH every run, and a subject that caches keyed on PATH then reports a permanent cache miss as 'no improvement' (harness-integrity.md rule 1, source failure 1). Choose a directory that persists across runs." + fi + done + + # shellcheck disable=SC2034 # this is the function's OUTPUT, read by every sourcing script + HARNESS_SHIM_DIR="$resolved" +} + +# harness_ledger_path +# +# Sets HARNESS_LEDGER_FILE for . The ledger deliberately does NOT live +# inside the shim directory: a ledger keyed on the thing whose stability it is +# proving is reset by the very change it must detect. +harness_ledger_path() { + local key="$1" dir base + dir="${PERF_HARNESS_LEDGER_DIR:-}" + if [[ -z "$dir" ]]; then + base="${XDG_CACHE_HOME:-}" + if [[ -z "$base" ]]; then + if [[ -z "${HOME:-}" ]]; then + harness_die "none of PERF_HARNESS_LEDGER_DIR, XDG_CACHE_HOME or HOME is set, so the harness cannot record the PATH entry it injects and cannot prove that entry stayed fixed. Set PERF_HARNESS_LEDGER_DIR, or pass --no-ledger and prove stability another way." + fi + base="$HOME/.cache" + fi + dir="$base/performance-harness/ledger" + fi + mkdir -p "$dir" || harness_die "cannot create the ledger directory: $dir" + key="${key//[^A-Za-z0-9._-]/-}" + HARNESS_LEDGER_FILE="$dir/$key.path" +} + +# harness_ledger_check +# +# Rule 1, enforced by the standalone script rather than only by its driver. A +# census is invoked on its own far more often than through a before/after +# driver, so the driver's two-runs-agree proof cannot be the only enforcement. +# The first run for a key records and passes; a later run whose injected PATH +# entry differs fails and names both values. +harness_ledger_check() { + local key="$1" value="$2" previous + harness_ledger_path "$key" + if [[ -f "$HARNESS_LEDGER_FILE" ]]; then + previous="$(<"$HARNESS_LEDGER_FILE")" + if [[ "$previous" != "$value" ]]; then + harness_die "the PATH entry this harness injects CHANGED since the previous run recorded under ledger key '$key'. previously: '$previous'; now: '$value'. harness-integrity.md rule 1 requires the injected entry to be fixed across runs, because a subject that caches keyed on PATH sees every run as a cache miss and the census then measures the harness rather than the subject. Re-use the previous directory, or pass --ledger-reset once if you moved it deliberately." + fi + fi + printf '%s\n' "$value" >"$HARNESS_LEDGER_FILE" +} + +# harness_ledger_reset +harness_ledger_reset() { + harness_ledger_path "$1" + rm -f "$HARNESS_LEDGER_FILE" +} + +# harness_require_python +# +# Sets HARNESS_PYTHON. Fails rather than skipping: the summarizers are where the +# refusal rules live, so a run without them is not a weaker run, it is no run. +harness_require_python() { + local candidate + for candidate in python3 python; do + if command -v "$candidate" >/dev/null 2>&1; then + # shellcheck disable=SC2034 # this is the function's OUTPUT, read by every sourcing script + HARNESS_PYTHON="$candidate" + return 0 + fi + done + harness_die "neither python3 nor python is on PATH. The percentile and ratio refusals live in the Python summarizers, so a run without them would report numbers no gate had checked." +} diff --git a/plugins/performance/scripts/harness-lib.test.sh b/plugins/performance/scripts/harness-lib.test.sh new file mode 100755 index 000000000..4fce84f11 --- /dev/null +++ b/plugins/performance/scripts/harness-lib.test.sh @@ -0,0 +1,116 @@ +#!/usr/bin/env bash +# Tests for harness-lib.sh, the shared precondition helpers. +# +# Every case here asserts that a precondition FAILS when it is unmet. That is +# the point of the library: harness-integrity.md rule 2 forbids degrading into a +# weaker check that passes, so "it refused" is the behavior under test, not an +# error path to tolerate. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +# shellcheck source=harness-lib.sh +source "$SCRIPT_DIR/harness-lib.sh" + +# Inline test helpers: self-contained, no external test lib (ships with the plugin). +FAILED=0 +CASE_NUM=0 +pass() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'PASS: [%d] %s\n' "$CASE_NUM" "$1" +} +fail() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'FAIL: [%d] %s - expected %q got %q\n' "$CASE_NUM" "$1" "$2" "$3" >&2 + FAILED=$((FAILED + 1)) +} +assert_eq() { if [[ "$3" == "$2" ]]; then pass "$1"; else fail "$1" "$2" "$3"; fi; } +assert_contains() { + if [[ "$3" == *"$2"* ]]; then pass "$1"; else fail "$1" "*$2*" "$3"; fi +} + +RUN_OUT="" +RUN_RC=0 +# Run a library function in a subshell so its `exit` terminates only that +# subshell, and capture what it said along with how it exited. +capture() { + RUN_OUT="$("$@" 2>&1)" + RUN_RC=$? +} + +WORK="${PERF_HARNESS_TEST_ROOT:-$HOME/.cache/performance-harness-tests}/harness-lib.$$" +mkdir -p "$WORK" +trap 'rm -rf "$WORK"' EXIT +export PERF_HARNESS_LEDGER_DIR="$WORK/ledger" + +# --- harness_posix_form --- +# portability-ok: 'D:\a\b' is literal Windows path DATA under test, not a regex; the +# backslash pairs are the input this conversion exists to fold, not GNU escapes. +assert_eq "backslash drive path folds to POSIX" "/d/a/b" "$(harness_posix_form 'D:\a\b')" +assert_eq "forward-slash drive path folds to POSIX" "/d/a/b" "$(harness_posix_form 'D:/a/b')" +assert_eq "drive root folds to POSIX" "/c/" "$(harness_posix_form 'C:/')" +assert_eq "an already-POSIX path is unchanged" "/d/a/b" "$(harness_posix_form '/d/a/b')" +assert_eq "a non-path string is unchanged" "before" "$(harness_posix_form 'before')" + +# --- harness_require_posix_path --- +capture harness_require_posix_path "the subject" 'D:/worktrees/repo/hook.sh' +assert_eq "a drive-letter path is refused" "2" "$RUN_RC" +assert_contains "the refusal names the POSIX spelling" "/d/... rather than D:/..." "$RUN_OUT" + +capture harness_require_posix_path "the subject" 'bash /d/ok/x.sh && cat C:/leaked/y' +assert_eq "an EMBEDDED drive-letter path is refused" "2" "$RUN_RC" + +capture harness_require_posix_path "the subject" '/d/worktrees/repo/hook.sh' +assert_eq "a POSIX path is accepted" "0" "$RUN_RC" + +capture harness_require_posix_path "the subject" 'bash -c "printf %s hello"' +assert_eq "an ordinary command string is accepted" "0" "$RUN_RC" + +# --- harness_resolve_shim_dir --- +capture harness_resolve_shim_dir "" +assert_eq "an empty shim directory is refused" "2" "$RUN_RC" +assert_contains "the refusal explains why there is no default" "must be the SAME directory" "$RUN_OUT" + +capture harness_resolve_shim_dir "${TMPDIR:-/tmp}/performance-harness-shim-probe" +assert_eq "a shim directory under the temporary root is refused" "2" "$RUN_RC" +assert_contains "the refusal names the mktemp defect" "temporary root" "$RUN_OUT" + +# The same physical directory reached by a DIFFERENT SPELLING must still be +# refused. On this Windows host TEMP is the 8.3 short form +# (C:\Users\KYLESE~1\...) while /tmp resolves to the long form, and a string +# prefix test between the two finds nothing: the rejection would silently stop +# working on exactly the platform the source failures came from. +# discriminating-skip-required: the spelling-independence of the temp-root +# rejection is the only thing this case proves. +TMPDIR_ALIAS="$(cd /tmp && pwd -P)" +capture env TMPDIR=/tmp bash -c \ + "source '$SCRIPT_DIR/harness-lib.sh'; harness_resolve_shim_dir '$TMPDIR_ALIAS/perf-shim-probe'" +assert_eq "a temp root reached by another spelling is refused" "2" "$RUN_RC" + +# A directory whose NAME merely contains "tmp" is not under the temporary root +# and must be accepted; a substring match here would misfire on real paths. +mkdir -p "$WORK/tmp-but-not-temp" +harness_resolve_shim_dir "$WORK/tmp-but-not-temp" +assert_contains "a stable directory named tmp-* is accepted" "tmp-but-not-temp" "$HARNESS_SHIM_DIR" + +# --- harness_ledger_check --- +harness_ledger_check "case-ledger" "/d/stable/shim" +capture harness_ledger_check "case-ledger" "/d/stable/shim" +assert_eq "an unchanged injected PATH entry passes" "0" "$RUN_RC" + +capture harness_ledger_check "case-ledger" "/d/moved/shim" +assert_eq "a CHANGED injected PATH entry is refused" "2" "$RUN_RC" +assert_contains "the refusal names both values" "/d/stable/shim" "$RUN_OUT" +assert_contains "the refusal cites rule 1" "rule 1" "$RUN_OUT" + +harness_ledger_reset "case-ledger" +capture harness_ledger_check "case-ledger" "/d/moved/shim" +assert_eq "a reset ledger accepts a new entry" "0" "$RUN_RC" + +capture env -u PERF_HARNESS_LEDGER_DIR -u XDG_CACHE_HOME -u HOME \ + bash -c "source '$SCRIPT_DIR/harness-lib.sh'; harness_ledger_path k" +assert_eq "no writable ledger location is refused, not skipped" "2" "$RUN_RC" + +[[ "${FAILED:-0}" -eq 0 ]] || exit 1 +echo "OK: harness-lib preconditions" +exit 0 diff --git a/plugins/performance/scripts/pathfix.py b/plugins/performance/scripts/pathfix.py new file mode 100644 index 000000000..6126dbbe5 --- /dev/null +++ b/plugins/performance/scripts/pathfix.py @@ -0,0 +1,165 @@ +"""Path spelling between an MSYS shell and a native Windows interpreter. + +harness-integrity.md rule 6 says a `D:/...` path handed to bash resolves +nowhere. The MIRROR of that hazard is what this file exists for, and it is just +as live on a mixed host: the `python3` on PATH here is a NATIVE Windows build, +so an MSYS `/d/worktrees/repo/x.py` handed to it is read as a path relative to +the current drive root and resolves to `D:\\d\\worktrees\\repo\\x.py`. That is +nowhere, exactly as the bash case is nowhere. + +Left alone, a bash-driven harness passing MSYS paths to a Python probe reports +"target is not a file" for a file that plainly exists, and the next person edits +the harness rather than the spelling. + +Worse, the hazard is INTERMITTENT in a way that reads as "it works". MSYS +rewrites POSIX-looking paths in the ARGV it hands a native Windows executable, +so `python x.py --target /d/repo/a.sh` arrives already converted and everything +looks fine. A path carried inside a CONFIG FILE gets no such treatment, so the +identical spelling fails there. A harness whose command line works and whose +config does not is exactly the shape that gets debugged in the wrong place. + +Not every MSYS path has a drive letter to fold, either: `/tmp` and `/usr/bin` +are MOUNTS, and no string rule reaches them. `cygpath -w` is asked as the last +resort, because it is the only thing that knows the mount table. + +So this resolves the spelling EXPLICITLY and LOUDLY: the literal path is tried +first, the converted spelling only if the literal one does not exist, and the +conversion is reported as a note rather than performed silently. When neither +spelling exists, the caller gets both spellings it tried, so the failure names +the real problem instead of a phantom missing file. + +This is not a degradation under rule 2. It resolves two spellings of the SAME +file; it never substitutes a different subject, and it never converts a path +that already resolves. + +As a command, for debugging a spelling by hand: + + python pathfix.py +""" + +from __future__ import annotations + +import os +import pathlib +import re +import shutil +import subprocess +import sys + +MSYS_ROOTED = re.compile(r"^/([A-Za-z])(/.*)?$") +DRIVE_LETTER = re.compile(r"^([A-Za-z]):[/\\](.*)$", re.DOTALL) + +ON_WINDOWS = os.name == "nt" + + +def msys_to_native(value: str) -> str: + """`/d/a/b` -> `D:\\a\\b`. Returns the input unchanged when it is not MSYS-rooted.""" + match = MSYS_ROOTED.match(value) + if not match: + return value + drive = match.group(1).upper() + rest = (match.group(2) or "/").lstrip("/") + return f"{drive}:\\{rest.replace('/', os.sep if ON_WINDOWS else '/')}" + + +def native_to_msys(value: str) -> str: + """`D:\\a\\b` -> `/d/a/b`. Returns the input unchanged when it has no drive letter.""" + match = DRIVE_LETTER.match(value) + if not match: + return value + drive = match.group(1).lower() + rest = match.group(2).replace("\\", "/") + return f"/{drive}/{rest}" + + +def cygpath_native(value: str) -> str | None: + """Ask cygpath for the native spelling, or None when it cannot help. + + This is the only branch that can resolve an MSYS MOUNT such as `/tmp` or + `/usr/bin`, because the mount table is not derivable from the string. It is + a last resort rather than the first move: a subprocess per path is real cost, + and it is only ever reached once the cheaper spellings have all failed. + """ + if not ON_WINDOWS: + return None + tool = shutil.which("cygpath") + if tool is None: + return None + try: + result = subprocess.run( # noqa: S603 - argv, and the tool came from which() + [tool, "-w", value], + capture_output=True, + text=True, + timeout=30, + check=False, + ) + except (OSError, subprocess.SubprocessError): + return None + if result.returncode != 0: + return None + return result.stdout.strip() or None + + +def tried(value: str) -> list[str]: + """Every spelling resolve_existing would attempt, in order, deduplicated.""" + candidates = [value] + if ON_WINDOWS: + candidates.append(msys_to_native(value)) + converted = cygpath_native(value) + if converted: + candidates.append(converted) + else: + candidates.append(native_to_msys(value)) + seen: set[str] = set() + return [c for c in candidates if c and not (c in seen or seen.add(c))] + + +def resolve_existing(value: str) -> tuple[pathlib.Path, str | None]: + """Return (path, note). + + The literal spelling wins whenever it exists, so a path that already works is + never rewritten, and the expensive cygpath probe is never reached. A + conversion is reported in the note so it appears in the harness output rather + than happening behind the operator's back. When nothing resolves, the literal + spelling comes back with no note and the CALLER raises its own error, which + is where the domain-specific message belongs. + """ + literal = pathlib.Path(value) + if literal.exists(): + return literal, None + for candidate in tried(value)[1:]: + path = pathlib.Path(candidate) + if path.exists(): + return path, ( + f"path spelling converted: {value!r} does not resolve for this " + f"interpreter, {candidate!r} does. A native Windows interpreter cannot " + f"read an MSYS /d/... path, which is the mirror of the rule 6 hazard." + ) + return literal, None + + +def spellings_message(label: str, value: str) -> str: + return ( + f"{label} does not exist under any spelling this harness tried: " + f"{tried(value)}. On a mixed MSYS and native-Windows host the same file has " + f"two spellings and only one works per interpreter; check which one you meant." + ) + + +def main(argv: list[str]) -> int: + if len(argv) != 2: + print("usage: python pathfix.py ", file=sys.stderr) + return 2 + path, note = resolve_existing(argv[1]) + print(f"on_windows={ON_WINDOWS}") + print(f"tried={tried(argv[1])}") + print(f"resolved={path}") + print(f"note={note}") + if not path.exists(): + print(spellings_message("the path", argv[1]), file=sys.stderr) + return 1 + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv)) diff --git a/plugins/performance/scripts/pathfix.test.sh b/plugins/performance/scripts/pathfix.test.sh new file mode 100755 index 000000000..0714c67bc --- /dev/null +++ b/plugins/performance/scripts/pathfix.test.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +# Tests for pathfix.py, the MSYS-versus-native path spelling resolver. +# +# The case that carries the weight is a REAL file addressed by the MSYS spelling +# on a host whose Python is a native Windows build. That combination is the +# mirror of harness-integrity.md rule 6, it is the default on the host this +# plugin was built from, and without it the Python harnesses report a phantom +# missing file for a file that plainly exists. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +# shellcheck source=harness-lib.sh +source "$SCRIPT_DIR/harness-lib.sh" +harness_require_python +PATHFIX="$SCRIPT_DIR/pathfix.py" +readonly PATHFIX + +# Inline test helpers: self-contained, no external test lib (ships with the plugin). +FAILED=0 +CASE_NUM=0 +pass() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'PASS: [%d] %s\n' "$CASE_NUM" "$1" +} +fail() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'FAIL: [%d] %s - expected %q got %q\n' "$CASE_NUM" "$1" "$2" "$3" >&2 + FAILED=$((FAILED + 1)) +} +assert_eq() { if [[ "$3" == "$2" ]]; then pass "$1"; else fail "$1" "$2" "$3"; fi; } +assert_contains() { + if [[ "$3" == *"$2"* ]]; then pass "$1"; else fail "$1" "*$2*" "$3"; fi +} + +RUN_OUT="" +RUN_RC=0 +run_pathfix() { + RUN_OUT="$("$HARNESS_PYTHON" "$PATHFIX" "$1" 2>&1)" + RUN_RC=$? +} + +WORK="$(mktemp -d)" +readonly WORK +trap 'rm -rf "$WORK"' EXIT +printf 'content\n' >"$WORK/real-file.txt" + +# --- 1. a real file addressed in this shell's own spelling always resolves --- +# $WORK is the MSYS spelling under Git Bash and an ordinary POSIX path +# elsewhere, so this case is meaningful on both. +run_pathfix "$WORK/real-file.txt" +assert_eq "an existing file resolves" "0" "$RUN_RC" +assert_contains "the resolution is reported" "resolved=" "$RUN_OUT" + +# --- 2. a path that resolves for the interpreter is NEVER rewritten --- +# discriminating-skip-required: silent rewriting of a working path is the +# failure mode this whole file has to avoid, and this is the case that proves +# it does not happen. +run_pathfix "$WORK/real-file.txt" +if [[ "$RUN_OUT" == *"note=None"* ]]; then + pass "a working spelling is left alone, with no conversion note" +else + fail "a working spelling is left alone" "note=None" "$RUN_OUT" +fi + +# --- 3. a nonexistent path fails and names every spelling it tried --- +run_pathfix "/d/definitely/not/here/at/all.txt" +assert_eq "a nonexistent path fails" "1" "$RUN_RC" +assert_contains "the failure lists the spellings tried" "tried=" "$RUN_OUT" +assert_contains "the failure explains the two-spelling hazard" "two spellings" "$RUN_OUT" + +# --- 4. the conversion itself is correct in both directions --- +conversions="$(cd "$SCRIPT_DIR" && "$HARNESS_PYTHON" - 2>&1 <<'PY' +import pathfix + +# portability-ok: Python string data inside a heredoc, not a shell regex; the +# backslashes are the native path spelling under test. +print(pathfix.native_to_msys("D:\\worktrees\\repo")) +print(pathfix.msys_to_native("/d/worktrees/repo").replace("\\", "/")) +PY +)" +assert_contains "a native path folds to the MSYS spelling" "/d/worktrees/repo" "$conversions" +assert_contains "an MSYS path folds to the native spelling" "D:/worktrees/repo" "$conversions" + +[[ "${FAILED:-0}" -eq 0 ]] || exit 1 +echo "OK: pathfix spelling resolution" +exit 0 diff --git a/plugins/performance/scripts/ratio.py b/plugins/performance/scripts/ratio.py new file mode 100644 index 000000000..9cb90835e --- /dev/null +++ b/plugins/performance/scripts/ratio.py @@ -0,0 +1,273 @@ +"""Paired-sample ratio for an interleaved A/B run. + +The absolute numbers on a drifting host move several-fold within an hour, so the +per-iteration PAIRED ratio is the statistic that survives: each pair was taken +back to back under the same instantaneous load. The median of the per-pair +ratios is reported alongside the ratio of medians, because a single drift spike +moves the latter and not the former. + +Two refusals are ENFORCED here, in this file, rather than left to the caller: + +1. Under concurrency the paired ratio is SUPPRESSED. Once the arms overlap they + interleave arbitrarily, so pairing by index compares samples that never + shared conditions. Per-arm percentiles are the honest statistic there. + BENCH_CONC is read with no default precisely so a caller who forgets it gets + an error rather than a silently re-enabled ratio. + +2. Arms of unequal length are refused outright. Pairing by index across unequal + arms silently drops the tail of the longer arm and reports a ratio over a + sample set nobody chose. + +3. EVERY ratio on the line is refused below a minimum pair count, the same way + summarize.py refuses a percentile the sample count cannot express. Measured on + the host this was built for, six repeats of two IDENTICAL arms at five + iterations produced median paired ratios of 1.00, 0.96, 0.85, 1.00, 0.78 and + 0.98, and one run reported 17.12x. BENCH_MIN_PAIRS overrides the default of + 20, which is the plugin's house sample count rather than a derived floor, and + a lowered floor prints itself on the line. + + The gate covers `ratio_of_p50` and `ratio_of_p95` too, not just the headline. + Gating only the headline left two quotable numbers sitting beside an honest + one: two IDENTICAL `true` arms at twenty iterations reported + `median_paired_ratio=1.06x ratio_of_p50=12.08x ratio_of_p95=0.35x`, and a + reader quotes whichever number is printed. + +4. A ZERO DENOMINATOR is refused rather than clamped. Dividing by + `max(percentile(new, p), 1.0)` turned an arm the clock could not resolve into + a 1ms denominator and manufactured a ratio out of the clamp. That, not drift, + is where the 12.08x above came from. + +5. When the paired median and the ratio of medians DISAGREE beyond a stated + factor, the line says so. One of them is drift, and nothing else in the output + tells the reader which. + +Do not describe this output as "paired statistics, per benchstat". benchstat +recommends interleaved COLLECTION and then analyses with the Mann-Whitney U +test, which is an independent two-sample test. + +Environment: + BENCH_OLD sample file for the baseline arm + BENCH_NEW sample file for the comparison arm + BENCH_CONC concurrency the samples were collected at + BENCH_MIN_PAIRS optional; minimum pairs before a ratio is reported (default 20) + +Exit: 0 reported or suppressed; 2 a precondition failed. +""" + +from __future__ import annotations + +import math +import os +import statistics +import sys + +import pathfix + +LABEL_WIDTH = 28 +DEFAULT_MIN_PAIRS = 20 +# Two ratios of the same two arms disagreeing by this much means one of them is +# drift rather than signal. A house threshold, not a derived one, and it is +# stated as such in the output it produces. +DISAGREEMENT_FACTOR = 2.0 + + +def fail(message: str) -> None: + print(f"HARNESS FAIL: {message}", file=sys.stderr) + raise SystemExit(2) + + +def env(name: str) -> str: + value = os.environ.get(name) + if value is None: + fail( + f"{name} is not set. It is read with no default on purpose: defaulting " + f"BENCH_CONC to 1 would re-enable the paired ratio under concurrency for " + f"any caller who forgot to pass it." + ) + return value or "" + + +def load(path: str) -> list[int]: + # Resolved rather than opened literally, for the reason summarize.py records: + # whether MSYS converts a POSIX path carried in an environment variable is a + # heuristic, not a guarantee. + resolved, note = pathfix.resolve_existing(path) + if note: + print(f"NOTE: {note}", file=sys.stderr) + try: + with open(resolved, encoding="utf-8") as handle: + raw = handle.read() + except OSError as error: + fail(f"cannot read the sample file {path}: {error}") + raise # unreachable + values: list[int] = [] + for number, line in enumerate(raw.splitlines(), start=1): + if not line.strip(): + continue + # The SAME row shape summarize.py enforces, deliberately duplicated rather + # than relaxed. This file documents that a caller invoking it directly + # cannot lose the concurrency suppression; a row guard that only + # summarize.py applied would be a hole in exactly that promise, and a + # spliced row from concurrent appends parses as a plausible number. + fields = line.split() + if len(fields) != 2: + fail( + f"{path} line {number} is not a ` ` row: " + f"{line!r}. A spliced row is what concurrent appends to one sink " + f"produce, and pairing it would report a ratio over a value nothing " + f"measured." + ) + try: + values.append(int(fields[0])) + except ValueError: + fail(f"{path} line {number} has no leading integer milliseconds: {line!r}") + return values + + +def percentile_floor(p: float) -> int: + return math.ceil(1.0 / (1.0 - p / 100.0)) + + +def percentile(values: list[int], p: float) -> float: + ordered = sorted(values) + k = (len(ordered) - 1) * p / 100.0 + lower, upper = math.floor(k), math.ceil(k) + if lower == upper: + return float(ordered[lower]) + return ordered[lower] + (ordered[upper] - ordered[lower]) * (k - lower) + + +def ratio_of_percentile( + old: list[int], new: list[int], p: float, pairs_count: int, minimum: int +) -> tuple[str, float | None]: + """Return (cell, value). `value` is None whenever the ratio was refused. + + THREE gates, and every one of them was a way to print a number the data does + not support: + + 1. The MINIMUM PAIR COUNT, the same gate the headline ratio carries. A ratio + of two per-arm percentiles compounds the drift in both, so it is if + anything less stable than the paired median, and gating only the headline + left two quotable numbers beside an honest one. Measured here, two + IDENTICAL `true` arms at twenty iterations reported ratio_of_p50=12.08x. + 2. The percentile ARITHMETIC floor, 1/(1-p). + 3. A ZERO DENOMINATOR. This is where that 12.08x actually came from: the + previous code divided by `max(percentile(new, p), 1.0)`, so an arm whose + percentile was 0ms silently became 1ms and manufactured a ratio out of a + clamp. A sub-millisecond denominator does not mean "one millisecond", it + means the clock cannot resolve this arm, and the honest output is a + refusal naming that. + """ + label = f"ratio_of_p{p:.0f}" + if pairs_count < minimum: + return f"{label}=REFUSED(pairs={pairs_count}<{minimum})", None + floor = percentile_floor(p) + if len(old) < floor: + return f"{label}=REFUSED(n={len(old)}<{floor})", None + denominator = percentile(new, p) + if denominator <= 0: + return f"{label}=REFUSED(comparison p{p:.0f}=0ms, below clock resolution)", None + value = percentile(old, p) / denominator + return f"{label}={value:.2f}x", value + + +def main() -> int: + concurrency = env("BENCH_CONC") + if concurrency != "1": + print( + f"{'SPEEDUP':<{LABEL_WIDTH}} SUPPRESSED: concurrency={concurrency}. " + f"Under uncontrolled concurrent load the arms are no longer load-matched, " + f"so pairing by index compares samples that never shared conditions. " + f"Read the per-arm percentiles above instead." + ) + return 0 + + old = load(env("BENCH_OLD")) + new = load(env("BENCH_NEW")) + + if not old or not new: + fail("at least one arm holds no samples; there is no ratio to report.") + if len(old) != len(new): + fail( + f"the arms hold {len(old)} and {len(new)} samples. A paired ratio pairs by " + f"index, so unequal arms would silently drop the longer arm's tail and " + f"report a ratio over a sample set nobody chose." + ) + + pairs = [(o, n) for o, n in zip(old, new) if n > 0] + dropped = len(old) - len(pairs) + if not pairs: + fail( + "every comparison-arm sample was zero milliseconds, so no per-pair ratio " + "is defined. The clock resolution is coarser than the subject; measure a " + "larger unit of work." + ) + + ratios = [o / n for o, n in pairs] + note = f" dropped={dropped}(zero-ms comparison samples)" if dropped else "" + + minimum_raw = os.environ.get("BENCH_MIN_PAIRS", str(DEFAULT_MIN_PAIRS)) + try: + minimum = int(minimum_raw) + except ValueError: + fail(f"BENCH_MIN_PAIRS must be an integer, got {minimum_raw!r}") + raise # unreachable + + # A LOWERED floor records itself on the reported line. A gate that can be + # quietly relaxed is not a gate: without this, BENCH_MIN_PAIRS=1 prints a + # headline ratio indistinguishable from one drawn from a full sample set, and + # the reader has no way to know the floor moved. + override = "" + if minimum < DEFAULT_MIN_PAIRS: + override = f" OVERRIDE: min_pairs={minimum} (default {DEFAULT_MIN_PAIRS})" + + p50_cell, p50_value = ratio_of_percentile(old, new, 50.0, len(pairs), minimum) + p95_cell, _ = ratio_of_percentile(old, new, 95.0, len(pairs), minimum) + + if len(pairs) < minimum: + # EVERY ratio on the line is refused below the floor, not just the + # headline. A reader quotes whichever number is printed, so leaving the + # subordinate two ungated beside a refused headline is the same defect + # wearing a smaller label. + print( + f"{'SPEEDUP':<{LABEL_WIDTH}} " + f"median_paired_ratio=REFUSED(pairs={len(pairs)}<{minimum}) " + f"{p50_cell} {p95_cell}{note}" + ) + print( + f"{'':<{LABEL_WIDTH}} raw per-pair ratios: " + f"{' '.join(f'{value:.2f}' for value in ratios)}" + ) + print( + f"{'':<{LABEL_WIDTH}} two IDENTICAL arms measured here spread 0.78x to 17.12x " + f"at five pairs. Raise the iteration count, or lower BENCH_MIN_PAIRS " + f"deliberately and say so in the report." + ) + return 0 + + median_paired = statistics.median(ratios) + + # The docstring predicts that a drift spike moves the ratio of medians and + # not the paired median. Predicting it is not enough: when the two disagree + # this far, one of them is noise, and the reader has no way to tell which + # unless the line says so. + disagreement = "" + if p50_value is not None and min(median_paired, p50_value) > 0: + spread = max(median_paired, p50_value) / min(median_paired, p50_value) + if spread >= DISAGREEMENT_FACTOR: + disagreement = ( + f" DISAGREEMENT: paired median and ratio-of-p50 differ by {spread:.1f}x. " + f"They measure the same thing, so one is drift. Trust the paired median; " + f"it is the only one whose samples shared conditions." + ) + + print( + f"{'SPEEDUP':<{LABEL_WIDTH}} pairs={len(pairs):<4} " + f"median_paired_ratio={median_paired:.2f}x " + f"{p50_cell} {p95_cell}{note}{override}{disagreement}" + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/plugins/performance/scripts/ratio.test.sh b/plugins/performance/scripts/ratio.test.sh new file mode 100755 index 000000000..f331849a1 --- /dev/null +++ b/plugins/performance/scripts/ratio.test.sh @@ -0,0 +1,191 @@ +#!/usr/bin/env bash +# Tests for ratio.py, the paired-sample ratio. +# +# The two behaviors under test are refusals, not calculations: the paired ratio +# is SUPPRESSED under concurrency (the arms are no longer load-matched, so +# pairing by index compares samples that never shared conditions), and arms of +# unequal length are rejected outright rather than silently truncated. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +# shellcheck source=harness-lib.sh +source "$SCRIPT_DIR/harness-lib.sh" +harness_require_python +RATIO="$SCRIPT_DIR/ratio.py" +readonly RATIO + +# Inline test helpers: self-contained, no external test lib (ships with the plugin). +FAILED=0 +CASE_NUM=0 +pass() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'PASS: [%d] %s\n' "$CASE_NUM" "$1" +} +fail() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'FAIL: [%d] %s - expected %q got %q\n' "$CASE_NUM" "$1" "$2" "$3" >&2 + FAILED=$((FAILED + 1)) +} +assert_eq() { if [[ "$3" == "$2" ]]; then pass "$1"; else fail "$1" "$2" "$3"; fi; } +assert_contains() { + if [[ "$3" == *"$2"* ]]; then pass "$1"; else fail "$1" "*$2*" "$3"; fi +} +assert_not_contains() { + if [[ "$3" != *"$2"* ]]; then pass "$1"; else fail "$1" "no *$2*" "$3"; fi +} + +WORK="$(mktemp -d)" +readonly WORK +trap 'rm -rf "$WORK"' EXIT + +write_samples() { + local path="$1" + shift + : >"$path" + local value + for value in "$@"; do + printf '%s 0\n' "$value" >>"$path" + done +} + +RUN_OUT="" +RUN_RC=0 +run_ratio() { + RUN_OUT="$(env "$@" "$HARNESS_PYTHON" "$RATIO" 2>&1)" + RUN_RC=$? +} + +write_samples "$WORK/old" 40 40 40 40 +write_samples "$WORK/new" 10 10 10 10 +write_samples "$WORK/short" 10 10 + +# --- 1. serial run reports the paired ratio, given enough pairs --- +run_ratio BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/new" BENCH_CONC=1 BENCH_MIN_PAIRS=4 +assert_eq "a serial run exits 0" "0" "$RUN_RC" +assert_contains "the paired ratio is reported" "median_paired_ratio=4.00x" "$RUN_OUT" +assert_contains "p95 is refused below its floor" "ratio_of_p95=REFUSED(n=4<20)" "$RUN_OUT" +# A gate that can be quietly relaxed is not a gate. +# discriminating-skip-required: without this assertion a lowered floor is +# indistinguishable in the report from a ratio drawn from a full sample set. +assert_contains "a lowered floor records itself on the reported line" \ + "OVERRIDE: min_pairs=4 (default 20)" "$RUN_OUT" + +# --- 1b. the headline ratio has a floor of its own --- +# Two IDENTICAL arms measured on this class of host spread 0.78x to 17.12x at +# five pairs. A median of a handful of ratios is a number the data does not +# support, and this line already refuses two weaker statistics. +# discriminating-skip-required: the minimum-pairs refusal is the only thing this +# case proves. +run_ratio BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/new" BENCH_CONC=1 +assert_eq "too few pairs still exits 0" "0" "$RUN_RC" +assert_contains "the headline ratio is refused by name" \ + "median_paired_ratio=REFUSED(pairs=4<20)" "$RUN_OUT" +assert_not_contains "no ratio is printed below the floor" "median_paired_ratio=4.00x" "$RUN_OUT" +assert_contains "the raw per-pair ratios replace it" "raw per-pair ratios:" "$RUN_OUT" +assert_contains "the refusal cites the measured spread" "0.78x to 17.12x" "$RUN_OUT" +# The SUBORDINATE ratios are gated by the same floor. Gating only the headline +# left two quotable numbers beside an honest one, which is the same defect +# wearing a smaller label. +# discriminating-skip-required: without these two assertions an ungated +# ratio_of_p50 can still be quoted off a line whose headline is refused. +assert_contains "ratio_of_p50 is refused below the floor too" \ + "ratio_of_p50=REFUSED(pairs=4<20)" "$RUN_OUT" +assert_contains "ratio_of_p95 is refused below the floor too" \ + "ratio_of_p95=REFUSED(pairs=4<20)" "$RUN_OUT" +if [[ "$RUN_OUT" =~ ratio_of_p[0-9]+=[0-9] ]]; then + fail "no numeric subordinate ratio prints below the floor" "all REFUSED" "$RUN_OUT" +else + pass "no numeric subordinate ratio prints below the floor" +fi + +# --- 1c. a zero denominator is refused, never clamped to 1ms --- +# `max(percentile(new, p), 1.0)` turned an arm the clock could not resolve into +# a 1ms denominator and manufactured a ratio out of the clamp. That is where a +# 12.08x between two identical arms actually came from. +# discriminating-skip-required: this case is the only cover for the clamp. +write_samples "$WORK/subms" 0 0 0 0 +run_ratio BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/subms" BENCH_CONC=1 BENCH_MIN_PAIRS=1 +assert_eq "an unresolvable denominator still exits 0" "2" "$RUN_RC" +assert_contains "the all-zero arm is refused before any ratio prints" \ + "clock resolution" "$RUN_OUT" + +# A denominator whose p50 is 0 but which has SOME non-zero samples reaches the +# percentile gate rather than the earlier all-zero refusal. +write_samples "$WORK/mostlyzero" 0 0 0 90 +run_ratio BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/mostlyzero" BENCH_CONC=1 BENCH_MIN_PAIRS=1 +assert_eq "a zero-p50 denominator exits 0" "0" "$RUN_RC" +assert_contains "a zero denominator percentile is refused, not clamped" \ + "ratio_of_p50=REFUSED(comparison p50=0ms, below clock resolution)" "$RUN_OUT" +if [[ "$RUN_OUT" =~ ratio_of_p50=[0-9] ]]; then + fail "a clamped ratio is never printed" "REFUSED" "$RUN_OUT" +else + pass "a clamped ratio is never printed" +fi + +# --- 1d. paired median and ratio-of-p50 disagreeing is flagged --- +# The docstring predicts a drift spike moves one and not the other. Predicting +# it is not enough: the reader cannot tell which number is the noise. +# discriminating-skip-required: the disagreement flag has no other cover. +# The two statistics only diverge when the PAIRING carries information the +# per-arm percentiles throw away. Here each arm has the identical distribution +# (so ratio_of_p50 is 1.00x) while every pair is lopsided in alternating +# directions (so the paired median is ~50x). That is the drift shape the +# docstring predicts, reproduced deterministically. +write_samples "$WORK/dis_old" 100 1 100 1 +write_samples "$WORK/dis_new" 1 100 1 100 +run_ratio BENCH_OLD="$WORK/dis_old" BENCH_NEW="$WORK/dis_new" BENCH_CONC=1 BENCH_MIN_PAIRS=1 +assert_eq "a disagreeing run exits 0" "0" "$RUN_RC" +assert_contains "the disagreement is flagged" "DISAGREEMENT:" "$RUN_OUT" +assert_contains "the flag says which number to trust" "Trust the paired median" "$RUN_OUT" + +run_ratio BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/new" BENCH_CONC=1 BENCH_MIN_PAIRS=4 +assert_not_contains "agreeing ratios are not flagged" "DISAGREEMENT:" "$RUN_OUT" + +# --- 2. concurrency SUPPRESSES the ratio --- +# discriminating-skip-required: this case is the whole proof that the +# concurrency suppression exists. +run_ratio BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/new" BENCH_CONC=4 +assert_eq "a concurrent run still exits 0" "0" "$RUN_RC" +assert_contains "the ratio is suppressed by name" "SUPPRESSED: concurrency=4" "$RUN_OUT" +assert_not_contains "no paired ratio is printed under concurrency" "median_paired_ratio" "$RUN_OUT" +assert_contains "the suppression states the reason" "never shared conditions" "$RUN_OUT" + +# --- 3. an unset BENCH_CONC FAILS rather than defaulting to serial --- +# Defaulting would silently re-enable the paired ratio under concurrency for +# any caller who forgot to pass it. +RUN_OUT="$(env -u BENCH_CONC BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/new" \ + "$HARNESS_PYTHON" "$RATIO" 2>&1)" +RUN_RC=$? +assert_eq "an unset BENCH_CONC is refused" "2" "$RUN_RC" +assert_contains "the refusal explains the danger of defaulting" "re-enable the paired ratio" "$RUN_OUT" + +# --- 4. unequal arms are refused, not truncated --- +run_ratio BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/short" BENCH_CONC=1 +assert_eq "unequal arms are refused" "2" "$RUN_RC" +assert_contains "the refusal names the silent truncation" "silently drop" "$RUN_OUT" + +# --- 5. an all-zero comparison arm is refused --- +write_samples "$WORK/zeros" 0 0 0 0 +run_ratio BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/zeros" BENCH_CONC=1 +assert_eq "an all-zero comparison arm is refused" "2" "$RUN_RC" +assert_contains "the refusal names the clock resolution" "clock resolution" "$RUN_OUT" + +# --- 6. a spliced row is refused HERE, not only in summarize.py --- +# This file promises that a caller invoking it directly cannot lose the +# concurrency suppression. A row guard living only in its sibling would be a +# hole in exactly that promise, and a spliced row parses as a plausible number. +# discriminating-skip-required: nothing else here proves the row shape is +# enforced by this file rather than inherited from its caller. +printf '120\n130 0\n' >"$WORK/spliced" +run_ratio BENCH_OLD="$WORK/spliced" BENCH_NEW="$WORK/short" BENCH_CONC=1 +assert_eq "a spliced row is refused" "2" "$RUN_RC" +assert_contains "the refusal names the concurrent-append cause" "concurrent appends" "$RUN_OUT" + +# --- 7. a non-integer BENCH_MIN_PAIRS is refused, never silently defaulted --- +run_ratio BENCH_OLD="$WORK/old" BENCH_NEW="$WORK/new" BENCH_CONC=1 BENCH_MIN_PAIRS=lots +assert_eq "a non-integer BENCH_MIN_PAIRS is refused" "2" "$RUN_RC" + +[[ "${FAILED:-0}" -eq 0 ]] || exit 1 +echo "OK: ratio suppression and pairing" +exit 0 diff --git a/plugins/performance/scripts/run-spawn-census.sh b/plugins/performance/scripts/run-spawn-census.sh new file mode 100755 index 000000000..4bc2a44ee --- /dev/null +++ b/plugins/performance/scripts/run-spawn-census.sh @@ -0,0 +1,188 @@ +#!/usr/bin/env bash +# Before/after spawn census, with the rule-1 self-proof built in. +# +# harness-integrity.md rule 1: run the harness twice against an UNCHANGED +# subject; if the two runs disagree beyond characterized noise, the harness is a +# variable rather than an instrument. A spawn count has no noise band, so the +# runs must agree exactly, and a disagreement is a hard failure here rather than +# a footnote in the report. +# +# The comparison is between WARM runs only. Run 1 of each arm is discarded as +# cold and reported separately: a cold cache legitimately changes a spawn count, +# and a gate that fired on that would be loosened by the next person to hit it. +# +# Usage: +# run-spawn-census.sh --shim-dir --before --after [options] +# +# --shim-dir REQUIRED. Passed through to spawn-census.sh. +# --before REQUIRED. Shell command string for the baseline arm. +# --after REQUIRED. Shell command string for the changed arm. +# --before-label Default: before +# --after-label Default: after +# --warm Warm runs per arm, all of which must agree. Default 2. +# --reset-command Run once up front, for a targeted cache reset. +# --tool Passed through. Repeatable. +# --stdin Passed through. +# --stdin-file Passed through. +# --allow-windows-paths Passed through. +# +# Exit: 0 both arms were stable; 2 a precondition failed, including an arm whose +# warm runs disagreed. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +# shellcheck source=harness-lib.sh +source "$SCRIPT_DIR/harness-lib.sh" + +CENSUS="$SCRIPT_DIR/spawn-census.sh" + +usage() { + cat <<'USAGE' +run-spawn-census.sh --shim-dir --before --after [options] + + --shim-dir REQUIRED. Passed through to spawn-census.sh. + --before REQUIRED. Shell command string for the baseline arm. + --after REQUIRED. Shell command string for the changed arm. + --before-label Default: before + --after-label Default: after + --warm Warm runs per arm, all of which must agree. Default 2. + --reset-command Run once up front, for a targeted cache reset. + --tool Passed through. Repeatable. + --stdin Passed through. + --stdin-file Passed through. + --allow-windows-paths Passed through. + +Exit: 0 both arms stable; 2 a precondition failed (including disagreeing warm runs). +USAGE +} + +SHIM_DIR="" +BEFORE_COMMAND="" +AFTER_COMMAND="" +BEFORE_LABEL="before" +AFTER_LABEL="after" +WARM=2 +RESET_COMMAND="" +PASSTHROUGH=() +HAVE_BEFORE=0 +HAVE_AFTER=0 + +while (($# > 0)); do + case "$1" in + --shim-dir) + SHIM_DIR="${2:-}" + shift 2 + ;; + --before) + BEFORE_COMMAND="${2:-}" + HAVE_BEFORE=1 + shift 2 + ;; + --after) + AFTER_COMMAND="${2:-}" + HAVE_AFTER=1 + shift 2 + ;; + --before-label) + BEFORE_LABEL="${2:-}" + shift 2 + ;; + --after-label) + AFTER_LABEL="${2:-}" + shift 2 + ;; + --warm) + WARM="${2:-}" + shift 2 + ;; + --reset-command) + RESET_COMMAND="${2:-}" + shift 2 + ;; + --tool) + PASSTHROUGH+=(--tool "${2:-}") + shift 2 + ;; + --stdin) + PASSTHROUGH+=(--stdin "${2:-}") + shift 2 + ;; + --stdin-file) + PASSTHROUGH+=(--stdin-file "${2:-}") + shift 2 + ;; + --allow-windows-paths) + PASSTHROUGH+=(--allow-windows-paths) + shift + ;; + -h | --help) + usage + exit 0 + ;; + *) + harness_die "unknown argument: $1 (see --help)" + ;; + esac +done + +((HAVE_BEFORE == 1)) || harness_die "--before is required: a shell command string for the baseline arm." +((HAVE_AFTER == 1)) || harness_die "--after is required: a shell command string for the changed arm." +[[ -x "$CENSUS" || -f "$CENSUS" ]] || harness_die "spawn-census.sh not found beside this script: $CENSUS" + +if [[ ! "$WARM" =~ ^[0-9]+$ ]] || ((WARM < 2)); then + harness_die "--warm must be an integer of at least 2. The whole point is comparing two warm runs against an unchanged subject; one run can agree with nothing." +fi + +if [[ -n "$RESET_COMMAND" ]]; then + bash -c "$RESET_COMMAND" || harness_die "the reset command failed: $RESET_COMMAND" +fi + +CENSUS_COUNT="" + +# Sets CENSUS_COUNT. Not a $( ) helper, so a precondition +# failure inside spawn-census.sh can terminate this script rather than a +# subshell nobody is checking. +census_run() { + local label="$1" command="$2" rc=0 line="" + set +e + line="$(bash "$CENSUS" --shim-dir "$SHIM_DIR" --label "$label" --ledger-key "$label" \ + "${PASSTHROUGH[@]}" -- bash -c "$command" 2>&1)" + rc=$? + set -e + ((rc == 0)) || harness_die "the census failed for arm '$label' (exit $rc). Its output was: $line" + [[ "$line" == *spawns=* ]] || harness_die "the census produced no spawns= field for arm '$label'. Its output was: $line" + CENSUS_COUNT="${line#*spawns=}" + CENSUS_COUNT="${CENSUS_COUNT%%[[:space:]]*}" +} + +ARM_WARM="" + +run_arm() { + local label="$1" command="$2" + local cold="" warm_first="" i + census_run "$label" "$command" + cold="$CENSUS_COUNT" + printf '%-14s cold spawns=%s\n' "$label" "$cold" + + for ((i = 1; i <= WARM; i++)); do + census_run "$label" "$command" + printf '%-14s warm%-2s spawns=%s\n' "$label" "$i" "$CENSUS_COUNT" + if [[ -z "$warm_first" ]]; then + warm_first="$CENSUS_COUNT" + elif [[ "$CENSUS_COUNT" != "$warm_first" ]]; then + harness_die "arm '$label' counted $warm_first and $CENSUS_COUNT spawns on two WARM runs against an UNCHANGED subject. harness-integrity.md rule 1: two runs against an unchanged subject must agree, or the harness is a variable rather than an instrument. Run 1 was discarded as cold, so this is not a cold-cache artifact. Do not report a before/after delta from this harness until it is stable." + fi + done + ARM_WARM="$warm_first" +} + +run_arm "$BEFORE_LABEL" "$BEFORE_COMMAND" +before_warm="$ARM_WARM" +run_arm "$AFTER_LABEL" "$AFTER_COMMAND" +after_warm="$ARM_WARM" + +printf 'COUNTER %s=%s -> %s=%s delta=%s\n' \ + "$BEFORE_LABEL" "$before_warm" "$AFTER_LABEL" "$after_warm" \ + "$((after_warm - before_warm))" +printf 'STABILITY both arms agreed across %s warm runs each (harness-integrity.md rule 1)\n' "$WARM" diff --git a/plugins/performance/scripts/run-spawn-census.test.sh b/plugins/performance/scripts/run-spawn-census.test.sh new file mode 100755 index 000000000..0112ed613 --- /dev/null +++ b/plugins/performance/scripts/run-spawn-census.test.sh @@ -0,0 +1,103 @@ +#!/usr/bin/env bash +# Tests for run-spawn-census.sh, the before/after driver. +# +# The case that matters most is the UNSTABLE one: a driver that reports a +# before/after delta from a harness whose own repeated runs disagree is +# reporting its own variance as the change. harness-integrity.md rule 1 makes +# that a hard failure, and this suite proves the failure actually fires. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +DRIVER="$SCRIPT_DIR/run-spawn-census.sh" +readonly DRIVER + +# Inline test helpers: self-contained, no external test lib (ships with the plugin). +FAILED=0 +CASE_NUM=0 +pass() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'PASS: [%d] %s\n' "$CASE_NUM" "$1" +} +fail() { + CASE_NUM=$((CASE_NUM + 1)) + printf 'FAIL: [%d] %s - expected %q got %q\n' "$CASE_NUM" "$1" "$2" "$3" >&2 + FAILED=$((FAILED + 1)) +} +assert_eq() { if [[ "$3" == "$2" ]]; then pass "$1"; else fail "$1" "$2" "$3"; fi; } +assert_contains() { + if [[ "$3" == *"$2"* ]]; then pass "$1"; else fail "$1" "*$2*" "$3"; fi +} + +RUN_OUT="" +RUN_RC=0 +run_driver() { + RUN_OUT="$(bash "$DRIVER" "$@" 2>&1)" + RUN_RC=$? +} + +# Not under the system temporary root: the shim directory rejection would +# otherwise make every case unrunnable. +WORK="${PERF_HARNESS_TEST_ROOT:-$HOME/.cache/performance-harness-tests}/run-spawn-census.$$" +mkdir -p "$WORK" +trap 'rm -rf "$WORK"' EXIT +export PERF_HARNESS_LEDGER_DIR="$WORK/ledger" +SHIM="$WORK/shim" + +# A stable subject: exactly $1 sed spawns, every run. +cat >"$WORK/stable.sh" <<'STABLE' +#!/usr/bin/env bash +for ((i = 0; i < $1; i++)); do + printf 'x\n' | sed 's/x/y/' >/dev/null +done +STABLE + +# A subject whose spawn count CLIMBS on every run. Only builtins touch the +# counter (read and printf), so the drift shows up purely as sed spawns. +cat >"$WORK/climbing.sh" <<'CLIMBING' +#!/usr/bin/env bash +n=0 +if [[ -f "$1" ]]; then read -r n <"$1"; fi +n=$((n + 1)) +printf '%s\n' "$n" >"$1" +for ((i = 0; i < n; i++)); do + printf 'x\n' | sed 's/x/y/' >/dev/null +done +CLIMBING + +# --- 1. a stable pair reports a delta and says the arms agreed --- +run_driver --shim-dir "$SHIM" --tool sed \ + --before-label stable-before --after-label stable-after \ + --before "bash '$WORK/stable.sh' 3" --after "bash '$WORK/stable.sh' 1" +assert_eq "a stable before/after pair exits 0" "0" "$RUN_RC" +assert_contains "the counter delta is reported" "delta=-2" "$RUN_OUT" +assert_contains "the cold run is labelled separately" "cold spawns=" "$RUN_OUT" +assert_contains "stability is stated, not assumed" "both arms agreed" "$RUN_OUT" + +# --- 2. a subject whose WARM runs disagree is a hard failure --- +# Run 1 is discarded as cold, so this cannot be a cold-cache artifact: the +# climbing subject counts 2 then 3 on the two warm runs. +# discriminating-skip-required: this case is the only proof that the rule 1 +# agreement gate fires at all. +run_driver --shim-dir "$SHIM" --tool sed \ + --before-label climbing --after-label unused \ + --before "bash '$WORK/climbing.sh' '$WORK/counter'" --after "bash '$WORK/stable.sh' 1" +assert_eq "an arm whose warm runs disagree is refused" "2" "$RUN_RC" +assert_contains "the refusal cites rule 1" "rule 1" "$RUN_OUT" +assert_contains "the refusal rules out a cold-cache artifact" "discarded as cold" "$RUN_OUT" +assert_contains "the refusal forbids reporting a delta" "Do not report a before/after delta" "$RUN_OUT" + +# --- 3. preconditions --- +run_driver --shim-dir "$SHIM" --after "bash -c 'printf ok'" +assert_eq "a missing --before is refused" "2" "$RUN_RC" + +run_driver --shim-dir "$SHIM" --warm 1 --before "bash -c 'printf ok'" --after "bash -c 'printf ok'" +assert_eq "--warm 1 is refused" "2" "$RUN_RC" +assert_contains "the refusal explains why one run agrees with nothing" "agree with nothing" "$RUN_OUT" + +run_driver --shim-dir "$SHIM" --before "bash D:/repo/hook.sh" --after "bash -c 'printf ok'" +assert_eq "a drive-letter path inside an arm command is refused" "2" "$RUN_RC" + +[[ "${FAILED:-0}" -eq 0 ]] || exit 1 +echo "OK: run-spawn-census" +exit 0 diff --git a/plugins/performance/scripts/spawn-census.sh b/plugins/performance/scripts/spawn-census.sh new file mode 100755 index 000000000..77d6c1e68 --- /dev/null +++ b/plugins/performance/scripts/spawn-census.sh @@ -0,0 +1,245 @@ +#!/usr/bin/env bash +# Count the process spawns a subject command performs. +# +# A spawn count is the drift-immune counter this plugin puts ahead of any +# duration: it does not move when the machine is loaded, so it stays the honest +# before/after metric on a box whose wall-clock timings swing several-fold +# within an hour. A counter that fails to move is also an unambiguous signal, +# where a duration that fails to move is ambiguous. +# +# Method: prepend a shim directory to the subject's PATH. Each shim logs its own +# name and then execs the real tool. Builtins never reach a shim, which is +# precisely the distinction being measured. +# +# What this refuses to do, and why (reference/harness-integrity.md): +# +# * Invent a shim directory. The directory goes on PATH, so under rule 1 it +# must be fixed across runs. The source harness used `mktemp -d`, changed +# PATH every run, forced a permanent cache miss in a subject that cached +# keyed on PATH, and reported "no improvement" while measuring its own +# randomization. --shim-dir is required, a temporary root is rejected, and +# a ledger keyed on the label catches a directory that moved between runs. +# * Skip a tool it cannot resolve. The source harness did (`|| continue`), +# which undercounts silently. Rule 2: fail, never degrade. +# * Accept a Windows drive-letter path without being told to. Rule 6. +# +# Usage: +# spawn-census.sh --shim-dir --label