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/.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/.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/docs/SKILL-CHEAT-SHEET.md b/docs/SKILL-CHEAT-SHEET.md index 3ef5f983b..fb2d1d253 100644 --- a/docs/SKILL-CHEAT-SHEET.md +++ b/docs/SKILL-CHEAT-SHEET.md @@ -43,6 +43,7 @@ owned by [docs/CATALOG-TAXONOMY.md](CATALOG-TAXONOMY.md). | [`/discovery:blindspot`](../plugins/discovery/skills/blindspot/SKILL.md) | `discovery` | Surface your unknown-unknowns and sharpen the prompt before unfamiliar work | | [`/discovery:explore`](../plugins/discovery/skills/explore/SKILL.md) | `discovery` | Explore code, history, tests, and config before changing anything | | [`/discovery:trace-intent`](../plugins/discovery/skills/trace-intent/SKILL.md) | `discovery` | Reconstruct why a thing was built this way, from evidence outside the code | +| [`/performance:target`](../plugins/performance/skills/target/SKILL.md) | `performance` | Rank optimization candidates by evidence quality, not suspicion | ## 2. Research @@ -63,6 +64,7 @@ owned by [docs/CATALOG-TAXONOMY.md](CATALOG-TAXONOMY.md). | [`/event-storming:methodology`](../plugins/event-storming/skills/methodology/SKILL.md) | `event-storming` | EventStorming facilitation reference across all three formats | | [`/event-storming:simulation`](../plugins/event-storming/skills/simulation/SKILL.md) | `event-storming` | Multi-persona agentic EventStorming workshop on Miro | | [`/naming:name-it-better`](../plugins/naming/skills/name-it-better/SKILL.md) | `naming` | Generate and evaluate name candidates from blind fresh-context lenses | +| [`/performance:goal`](../plugins/performance/skills/goal/SKILL.md) | `performance` | Build a goal with realistic and ideal targets plus a computed floor | | [`/planning:design`](../plugins/planning/skills/design/SKILL.md) | `planning` | Resolve types, contracts, and module boundaries before planning | | [`/planning:design-handoff`](../plugins/planning/skills/design-handoff/SKILL.md) | `planning` | Gate a finished design and package it for planning | | [`/planning:devils-advocate`](../plugins/planning/skills/devils-advocate/SKILL.md) | `planning` | Stress-test a plan or the incumbent approach adversarially | @@ -118,6 +120,8 @@ owned by [docs/CATALOG-TAXONOMY.md](CATALOG-TAXONOMY.md). | Skill | Plugin | What it does | | --- | --- | --- | +| [`/performance:snapshot`](../plugins/performance/skills/snapshot/SKILL.md) | `performance` | Capture a snapshot only from a host proven measurable | +| [`/performance:verify`](../plugins/performance/skills/verify/SKILL.md) | `performance` | Re-derive the result in fresh context and report it honestly | | [`/toolchain:check`](../plugins/toolchain/skills/check/SKILL.md) | `toolchain` | Build, test, and lint changed files across detected ecosystems | | [`/toolchain:lint`](../plugins/toolchain/skills/lint/SKILL.md) | `toolchain` | Polyglot lint and format checks without a full build | | [`/verification:confirm`](../plugins/verification/skills/confirm/SKILL.md) | `verification` | Prove the change achieved its intended outcome with evidence | 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..3f7faa388 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,76 @@ 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" + # A timeout outranks the bimodal signature, and the order is load-bearing. + # A timed-out sample is recorded as the timeout ceiling, not as a measurement, + # so `max_ms` is a censored value. Reporting the bimodal reason first would + # hand the reader a spread computed from that ceiling and explain it as an + # observed slow mode, which reads as a finite measurement of a tail that is + # actually unbounded. + if "spawn-probe-timed-out" in findings: + return False, ( + f"{summary.get('timeouts')} of {summary.get('samples')} no-op spawns hit the probe " + f"timeout, so the tail is unbounded and max_ms is a censored ceiling rather than a " + f"measurement" + ) + 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 "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..6a622145f 100755 --- a/plugins/claude-ops/lib/test_spawn_noise.py +++ b/plugins/claude-ops/lib/test_spawn_noise.py @@ -83,5 +83,72 @@ 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_a_timeout_outranks_the_bimodal_reason(self): + # A timed-out sample is recorded at the timeout ceiling, so max_ms is + # censored. If bimodality were reported first, the reader would get a + # spread computed from that ceiling described as an observed slow mode, + # which reads as a finite measurement of an unbounded tail. + # discriminating-skip-required: this is the only case where both findings + # co-occur, so without it the precedence can silently invert. + summary = spawn_noise.summarize_spawn_samples([180.0, 1400.0, 20000.0], 1, 900) + self.assertIn("spawn-probe-timed-out", summary["findings"]) + self.assertIn("bimodal-spawn-latency", summary["findings"]) + ok, why = spawn_noise.is_measurable(summary) + self.assertFalse(ok) + self.assertIn("timeout", why) + self.assertNotIn("bimodal contention signature", 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/.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..84e5f1294 --- /dev/null +++ b/plugins/performance/CHANGELOG.md @@ -0,0 +1,42 @@ +# 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. +- **`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. +- **`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 new file mode 100644 index 000000000..74a430114 --- /dev/null +++ b/plugins/performance/README.md @@ -0,0 +1,99 @@ +# 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 + +Every plugin named here is **presence-gated**: this plugin prefers to reuse them, and degrades with a +stated fallback when one is absent. None is a hard install requirement, and none is declared as a +manifest dependency, because a measurement workflow that refuses to run when a sibling plugin is +missing fails worse than the duplication it was avoiding. + +- **`/verification:measure`** owns two-phase baseline/compare and machine-bound baseline storage. + When the `verification` plugin is installed this plugin reuses 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. When it is absent, baselines are captured + into the same memory-tier path directly and the report says the capture was unassisted. +- **`/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`, which each plugin **carries its own byte-identical copy of** as a registered + cross-plugin cluster. Neither imports the other at runtime, since plugins install independently; + the sync gate is what keeps the bimodal threshold at 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. diff --git a/plugins/performance/lib/spawn_noise.py b/plugins/performance/lib/spawn_noise.py new file mode 100644 index 000000000..3f7faa388 --- /dev/null +++ b/plugins/performance/lib/spawn_noise.py @@ -0,0 +1,194 @@ +"""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" + # A timeout outranks the bimodal signature, and the order is load-bearing. + # A timed-out sample is recorded as the timeout ceiling, not as a measurement, + # so `max_ms` is a censored value. Reporting the bimodal reason first would + # hand the reader a spread computed from that ceiling and explain it as an + # observed slow mode, which reads as a finite measurement of a tail that is + # actually unbounded. + if "spawn-probe-timed-out" in findings: + return False, ( + f"{summary.get('timeouts')} of {summary.get('samples')} no-op spawns hit the probe " + f"timeout, so the tail is unbounded and max_ms is a censored ceiling rather than a " + f"measurement" + ) + 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 "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/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..854cbfb28 --- /dev/null +++ b/plugins/performance/scripts/ab.test.sh @@ -0,0 +1,189 @@ +#!/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" +# Match the rc CENSUS, not a bare "141" anywhere in the output. A bare substring +# search collides with timing data: a legitimate 141ms sample printed `min=141ms` +# and failed this assertion for a reason unrelated to what it tests. An exit code +# only ever appears as a dict KEY, so `141:` is the form that means what is meant. +assert_not_contains "an undrained stdin does not fabricate exit code 141" "141:" "$RUN_OUT" +if [[ "$(printf '%s' "$RUN_OUT" | grep -c 'rc={0: 4}')" == "2" ]]; then + pass "both arms report a clean exit-code census" +else + fail "both arms report a clean exit-code census" "two rc={0: 4} lines" "$RUN_OUT" +fi + +# --- 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: TEMP is often the 8.3 short form +# `C:\Users\\AppData\Local\Temp`, while the same directory reached +# through `/tmp` reports `/c/Users//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..a80f37655 --- /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 Windows, TEMP is often the 8.3 short form +# (C:\Users\\...) 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..a94c80213 --- /dev/null +++ b/plugins/performance/scripts/ratio.py @@ -0,0 +1,288 @@ +"""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) + # Capture the p95 VALUE too, not just its rendered cell. Discarding it left + # the disagreement check blind to the one statistic it never inspected, so a + # quotable ratio_of_p95=5.95x could sit beside median_paired_ratio=1.00x and + # ratio_of_p50=1.00x with nothing on the line saying they disagree. + p95_cell, p95_value = 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. + # Every printed ratio is checked, not only ratio-of-p50. A reader quotes + # whichever number is on the line, so a statistic excluded from the check is + # a statistic that can be quoted while the other two silently disagree with + # it. Take the widest spread across all three. + disagreement = "" + candidates = {"ratio-of-p50": p50_value, "ratio-of-p95": p95_value} + worst_spread = 0.0 + worst_name = "" + for name, value in candidates.items(): + if value is None or min(median_paired, value) <= 0: + continue + spread = max(median_paired, value) / min(median_paired, value) + if spread > worst_spread: + worst_spread, worst_name = spread, name + if worst_spread >= DISAGREEMENT_FACTOR: + disagreement = ( + f" DISAGREEMENT: paired median and {worst_name} differ by {worst_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..5ce42ccac --- /dev/null +++ b/plugins/performance/scripts/ratio.test.sh @@ -0,0 +1,214 @@ +#!/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" + +# --- 8. the disagreement check inspects EVERY printed ratio, not just p50 --- +# A tail spike in one arm moves ratio_of_p95 while the paired median and +# ratio_of_p50 both sit at 1.00x. The p95 value was previously discarded at the +# call site, so a quotable 5.95x could sit beside two 1.00x figures with nothing +# saying they disagree. A statistic excluded from the check is a statistic that +# can be quoted while the rest of the line silently contradicts it. +# discriminating-skip-required: this is the only case covering p95 in the +# disagreement check; without it the flag can regress to p50-only and every +# other assertion here still passes. +write_samples "$WORK/spike_old" 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 400 +write_samples "$WORK/spike_new" 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 +run_ratio BENCH_OLD="$WORK/spike_old" BENCH_NEW="$WORK/spike_new" BENCH_CONC=1 +assert_eq "a p95-only divergence still exits 0" "0" "$RUN_RC" +assert_contains "the paired median is unmoved by the tail spike" "median_paired_ratio=1.00x" "$RUN_OUT" +assert_contains "ratio-of-p50 is unmoved too" "ratio_of_p50=1.00x" "$RUN_OUT" +assert_contains "the divergence is flagged" "DISAGREEMENT" "$RUN_OUT" +assert_contains "the flag names the diverging statistic" "ratio-of-p95" "$RUN_OUT" + +# The negative arm. Three agreeing ratios must NOT be flagged, or the check +# fires on every run and flags nothing. +run_ratio BENCH_OLD="$WORK/spike_new" BENCH_NEW="$WORK/spike_new" BENCH_CONC=1 +assert_not_contains "agreeing ratios are not flagged" "DISAGREEMENT" "$RUN_OUT" + +[[ "${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