feat(performance): add the measurement-first optimization plugin - #3561
feat(performance): add the measurement-first optimization plugin#3561kyle-sexton wants to merge 7 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeceb808ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| committed**, matching `/verification:measure`, which owns baseline capture and storage mechanics. | ||
| This skill depends on it rather than reimplementing it. |
There was a problem hiding this comment.
When a user installs performance by itself, /verification:measure is unavailable, yet this skill delegates the baseline capture and storage mechanics to it and provides no fallback. The new plugin.json declares no dependencies, so the primary target → goal → snapshot workflow cannot complete in a standalone installation; either add the native dependency or presence-gate this invocation and provide local baseline mechanics.
Useful? React with 👍 / 👎.
| ```python | ||
| from spawn_noise import spawn_probe, is_measurable | ||
| summary = spawn_probe() | ||
| measurable, why = is_measurable(summary) |
There was a problem hiding this comment.
When this snippet runs from the consuming project's normal working directory, spawn_noise is neither an installed module nor on sys.path, because it lives under ${CLAUDE_PLUGIN_ROOT}/lib; executing the documented code from the repository root raises ModuleNotFoundError. Since host qualification is the mandatory first snapshot step, the skill should explicitly load the module from the plugin's lib directory rather than relying on the current directory.
Useful? React with 👍 / 👎.
| 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" |
There was a problem hiding this comment.
When a probe has both successful samples and at least one timeout, summarize_spawn_samples() commonly adds both bimodal-spawn-latency and spawn-probe-timed-out, but this earlier branch wins. The resulting reason presents the timeout ceiling as a measured finite maximum and omits that the tail is actually unbounded, undermining the evidence attached to the refusal; handle the timeout finding before constructing the bimodal min/max explanation.
Useful? React with 👍 / 👎.
eeceb80 to
4ca4a09
Compare
|
Claude finished @kyle-sexton's task in 3m 53s —— View job
|
|
Claude finished @kyle-sexton's task in 1m 24s —— View job Claude finished the security review
CI security review (
|
|
Last security-reviewed head: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| Baselines live in the memory tier, `.work/<topic-slug>/baselines/`, machine-bound, **never | ||
| committed**, matching `/verification:measure`, which owns baseline capture and storage mechanics. | ||
| This skill depends on it rather than reimplementing it. |
There was a problem hiding this comment.
Correctness/architecture: unconditional dependency on /verification:measure with no gate or fallback, and no manifest declaration.
plugins/performance/.claude-plugin/plugin.json declares no dependencies. This line and goal/SKILL.md's boundary section (This plugin depends on it rather than reimplementing it., line ~114) state the dependency as unconditional, with no "if installed" check and no manual fallback.
That's inconsistent with how this repo already handles the same coupling elsewhere:
verification/skills/measure/SKILL.md:67routes to/performance:snapshotgated with "when theperformanceplugin is installed".planning/skills/plan/SKILL.md:126invokes/verification:measure ... via the Skill tool if installed. ... Or measure the pre-change state manually.
performance doesn't reciprocate either pattern: if someone installs performance standalone (a real path — the plugin's own README lists it as usable on its own), target → goal → snapshot has no way to complete the baseline-storage step, and no fallback is offered.
Confirmed via grep -rn '"dependencies"' plugins/*/.claude-plugin/plugin.json — no plugin.json in this repo actually uses a manifest dependency field (the one hit was a keyword string, not a real field), so a soft "if installed, else fallback" gate — matching the two examples above — is the house convention to follow here, not a new manifest field.
| ```python | ||
| from spawn_noise import spawn_probe, is_measurable | ||
| summary = spawn_probe() | ||
| measurable, why = is_measurable(summary) | ||
| ``` |
There was a problem hiding this comment.
Correctness: this import example has no path anchoring, so it fails as documented.
from spawn_noise import spawn_probe, is_measurablespawn_noise lives at ${CLAUDE_PLUGIN_ROOT}/lib/spawn_noise.py. It's neither installed nor on sys.path when run from a consuming project's working directory (the normal execution context for skill-invoked code in this repo). Every other cross-plugin script reference in this codebase anchors to ${CLAUDE_PLUGIN_ROOT} explicitly — e.g. plugins/claude-ops/skills/*/SKILL.md invoke scripts as python3 "${CLAUDE_PLUGIN_ROOT}/skills/.../scripts/x.py". This snippet should do the equivalent, e.g.:
import sys
sys.path.insert(0, f"{plugin_root}/lib")
from spawn_noise import spawn_probe, is_measurableor invoke it as a subprocess against the anchored path. As written, following this step literally (host qualification, which the skill calls mandatory and first) raises ModuleNotFoundError.
| findings = summary.get("findings", []) | ||
| if "no-spawn-samples-captured" in findings: | ||
| return False, "no spawn samples were captured, so the host was never characterized" | ||
| if "bimodal-spawn-latency" in findings: | ||
| return False, ( | ||
| f"spawn cost spread {summary.get('spread_ratio')}x across identical no-op spawns " | ||
| f"(min {summary.get('min_ms')} ms, max {summary.get('max_ms')} ms), with the slow mode " | ||
| f"above the {SLOW_SPAWN_FLOOR_MS} ms floor: the bimodal contention signature" | ||
| ) | ||
| if "spawn-probe-timed-out" in findings: | ||
| return False, "at least one no-op spawn hit the probe timeout, so the tail is unbounded" |
There was a problem hiding this comment.
Correctness: bimodal-spawn-latency is checked before spawn-probe-timed-out, so a co-occurring timeout is silently absorbed into a finite-max explanation.
summarize_spawn_samples can append both findings for the same probe: a timed-out sample is recorded as timeout_s * 1000.0 in durations_ms (line ~113), which can itself trip the bimodal spread/floor predicate, and the timeouts counter is checked independently and unconditionally appends spawn-probe-timed-out too. When both are present, this branch order means is_measurable() returns the bimodal reason — reporting max {max_ms} ms as a real measured ceiling — and never mentions that the tail is actually unbounded (a timeout, not a real sample).
The verdict (False) doesn't change either way, but the reason does, and this module's whole design point (per the module docstring and harness-integrity.md) is that the refusal's stated basis is what a reader acts on. Swapping the order so the timeout check runs first would fix it:
if "spawn-probe-timed-out" in findings:
return False, "at least one no-op spawn hit the probe timeout, so the tail is unbounded"
if "bimodal-spawn-latency" in findings:
...test_spawn_noise.py's new TestMeasurabilityVerdict cases don't exercise a combined bimodal+timeout sample set, so this ordering wasn't caught by the added tests either.
| - **Does not store baselines.** `/verification:measure` owns baseline capture and storage; this | ||
| plugin depends on it rather than reimplementing it. |
There was a problem hiding this comment.
Same unconditional-dependency issue as snapshot/SKILL.md:118 (see that comment for the full analysis): "/verification:measure owns baseline capture and storage; this plugin depends on it rather than reimplementing it." No "if installed" gate, no fallback, and plugin.json declares no dependency — inconsistent with how verification:measure and planning:plan gate this same coupling elsewhere in the repo.
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
524fa6c to
94f9db7
Compare
|
Claude finished @kyle-sexton's task in 3m 15s —— View job
|
|
Claude finished @kyle-sexton's task in 2m 6s —— View job
|
Records the outcome of the /planning:interview that issue #3530 requires before implementation. Twelve questions registered, nine answered by the user, three deferred to planning with arbiter tags. Two decisions diverge from #3530's own text and say so: - Phase 4's "suppress the paired ratio under concurrency" is corrected. Duet Benchmarking (Bulej et al., ICPE 2020) measured 5.03x and 37.4x accuracy improvements from running arms in parallel on shared machines, because both arms absorb the same interference. Sequential interleaving keeps the suppression rule; simultaneous paired arms do not. - The unmeasurable-host refusal ships as a house rule, not as field consensus. No benchmarking tool surveyed refuses above a variance threshold; they warn and print anyway. The brief also records that the plugin's headline metric, a process-spawn count, rests on a rationale the literature grounds only for instruction counts. That gap is labelled rather than smoothed over. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
Q11 — four skills, no router: target, goal, snapshot, verify, each naming its successor the way the planning pipeline already chains. Named `snapshot` rather than `measure` because Q2 locked "depend + route" on /verification:measure and two skills called `measure` is that routing line failing to route. Harness integrity ships as a shared reference plus a script rather than a fifth skill; it is a discipline applied inside the other skills, not a standalone invocation. Q10 — a cross-plugin runtime import is not available, since plugins install independently. The interview's "shared lib" answer is implemented through the mechanism this repo already uses for six other clusters: canonical source at lib/, byte-identical plugin copies, a dedicated sync-*.sh gate, a registry entry, and a CI job. One home for the threshold, loud drift, no runtime coupling. Recorded while resolving it: the noise threshold is a two-part predicate (spread ratio >= 3.0 AND max >= the slow-spawn floor), not a bare ratio. A cold-then-warm spawn pair clears 3x while every sample is still fast, so a consumer that re-derives a verdict from the ratio alone would report contention on a healthy host. Split into two PRs: the lib promotion and claude-ops refactor first, the new plugin second. The refactor is test-invisible — audit-performance re-exports the promoted names, so its six existing cases prove it. Q12 (sample count and percentile choice) stays USER-RESERVED and is surfaced at the approval gate, not resolved here. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
Partial scaffold. Skills, lib copy, sync gate, and marketplace entry still to come. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
Implements #3530 to the contract the required /planning:interview settled (docs/topics/performance-plugin/PLAN.md). Four skills, each naming its successor rather than routing through a hub: target ranks candidates by evidence quality (an unmeasured system yields "instrument this first", not a guess); goal is human-gated and computes the irreducible floor BEFORE the target is agreed; snapshot qualifies the host before measuring anything; verify re-derives the result in fresh context and reports it without rounding a miss into a win. Named `snapshot`, not `measure`. Q2 of the interview locked "depend + route" on /verification:measure, and two skills called `measure` is that routing line failing to route. That skill keeps baseline capture, storage, and the compare mechanics; this plugin adds what it does not cover and gains a gotcha pointing here for hosts a noise-floor warning cannot describe. The lib gains is_measurable() and percentile_floor(), and performance now carries lib/spawn_noise.py as a registered cross-plugin cluster with a dedicated sync gate and CI lane, so the bimodal threshold keeps one home. Plugins install independently, so a runtime import across the boundary was never available; the byte-identical-copy mechanism this repo already uses for six clusters is how the constraint is actually met. Two places where the research contradicted the issue, and the code follows the evidence: - #3530 says to suppress the paired ratio under concurrency. Duet Benchmarking (Bulej et al., ICPE 2020) measured 5.03x and 37.4x accuracy gains from running arms in PARALLEL on shared machines, because both arms absorb the same interference. Both modes ship; the suppression rule is scoped to the sequential form, and that reconciliation is labelled as this plugin's reading rather than a sourced claim. - The unmeasurable-host refusal ships as a house rule. No surveyed tool refuses above a variance threshold; pyperf, Criterion, JMH and benchstat all warn and print anyway. Three claims the literature does not ground are labelled rather than dressed as consensus: the p50/p95-over-20 sample default (only the derivable 1/(1-p) floor is real, and only that floor is enforced), p95 itself (the SRE Book names the 99th and 99.9th), and counts-over-wall-clock for anything but instruction counts, which is load-bearing here because process-spawn count is the headline metric and Valgrind does not run on Windows. Verification: all four skills PASS check-skill.sh with zero warnings; the sync gate's own suite proves --check DISCRIMINATES by asserting the clean and drifted arms return DIFFERENT verdicts, not merely that each printed its expected string; ruff clean; markdownlint clean; no em dashes in any new surface. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
Two CI gates, both real: plugin-catalog-enablement required an enabledPlugins key. .claude/cloud-bootstrap.sh computes what it installs from that file, so a catalogued plugin with no key never loads in a session here. contract-slice-prune required docs/topics/performance-plugin/ to go. That tree is contract tier: committed on a task branch, pruned before merge. Its durable outcomes graduated to issue #3530 first (comment 5501823163) — the Q10/Q11/Q12 resolutions, the two-part bimodal predicate, and why a cross-plugin runtime import was never available. The interview ledger was already linked there. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
…ites Completes the plugin. Two parallel workers, each gated by its own fresh-context verifier that executed rather than read. ## Harnesses (plugins/performance/scripts/) Nine scripts, each with a co-located test suite, 200 assertions total. Ported from the source run's scratch tree, which lived on local disk only and would have died with that directory. spawn-census.sh / run-spawn-census.sh use a STABLE shim dir, closing the defect where a mktemp -d shim put a fresh path on PATH every run against a PATH-keyed cache, so the census measured its own randomization and reported "no improvement". ab.sh + summarize.py + ratio.py interleave the arms and flip order per iteration, suppressing the paired ratio under concurrency. differential.py proves behavior over an argv matrix. discriminate.py consolidates five variants, four of which were broken. The verifiers found seven real defects between them, all fixed: - discriminate.py scored a check that never ran. Any shared non-zero exit read as NOT DISCRIMINATING with an affirmatively false explanation. It now splits identical-failing (HARNESS BROKEN, exit 2) from identical-passing (NOT DISCRIMINATING, exit 1), so the four original harness failures that exited 127 in both arms would now be caught rather than reported clean. - ratio.py printed a headline ratio with no sample floor: two identical arms measured 0.78x to 17.12x at five pairs. - The floor then guarded only the headline. Two identical `true` arms gave median_paired_ratio=1.06x beside ratio_of_p50=12.08x, so a reader could quote a 12x speedup between `true` and `true`. All three statistics are gated now. - spawn-census.sh censused a 126 subject as spawns=0, exit 0. - ratio.py accepted a spliced row summarize.py rejects. - printf | subject under pipefail fabricated exit 141 intermittently on a pipe-buffer race whenever the subject did not drain stdin. - discriminate.py referenced os with no import, on a line no test reached. A line no test executes is the exact shape that harness exists to detect. Rule 4 is proven behaviorally, not asserted: the target is committed, the fix applied and left uncommitted, and the fix is still present after the run. A git checkout restore would have destroyed it, which is what defect #5 in the source catalogue actually did. ## Evals target (5 cases), goal (6), verify (7), joining snapshot (5). Each case pins a specific gate whose removal would reintroduce a real failure: E4 suspicion never outranking E1 measurement, STOP when the target is below the floor, the differential covering every mode, a miss never rounded into a win. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
The undrained-stdin assertion searched the whole ab.sh output for the string "141" to prove no fabricated exit code. That collides with timing data: a legitimate 141ms sample prints min=141ms and fails the 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 was meant. Adds a positive companion asserting both arms report a clean rc census, since not-containing a failure string is weaker evidence than containing the success one. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
94f9db7 to
20e9433
Compare
|
Claude finished @kyle-sexton's task in 3m 16s —— View job
|
|
Claude finished @kyle-sexton's task in 2m 11s —— View job
|
1 participant
Summary
Adds the
performanceplugin: 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 that workflow done by hand against the
disk-hygienedestructive-guard hook (#3523). That session had a competent operator and a strong prompt, and it
still produced five verification harnesses that each returned a confident wrong answer rather than
an error. Four of the five were checks written specifically to avoid being fooled. That
disproportion is the plugin's whole reason for existing: a workflow that measures without enforcing
harness-integrity rules mostly generates confident numbers, which is worse than generating none.
The design was settled by the
/planning:interview#3530 requires. Nine questions answered by theuser, three deferred to planning, all recorded in
docs/topics/performance-plugin/PLAN.mdandsummarized on the issue.
Fix
Four skills, each naming its successor rather than routing through a hub, the way the planning
pipeline already chains:
targetgoalsnapshotverifyNamed
snapshot, notmeasure: Q2 locked "depend + route" on/verification:measure, and twoskills called
measureis that routing line failing to route.measurekeeps baseline capture,storage and compare mechanics, and gains one gotcha pointing here for hosts a noise-floor warning
cannot describe.
The shared lib becomes a registered cluster. A cross-plugin runtime import was never available,
since plugins install independently. So
lib/spawn_noise.pyis carried as a byte-identical copy withscripts/sync-spawn-noise.sh, a registry entry, and thespawn-noise-syncCI lane, the mechanismthis repo already uses for six clusters. The canonical gains
is_measurable()(the refusal verdict)and
percentile_floor()(the1/(1-p)sample floor).Two places the research contradicted the issue, and the code follows the evidence
Farquet & Prokopec, "Duet Benchmarking" (ICPE 2020) measured
accuracy gains of 5.03x (ScalaBench/DaCapo) and 37.4x (SPEC CPU 2017) from running arms in
parallel on shared machines, because both arms absorb the same interference. Both modes ship, the
suppression rule is scoped to the sequential form, and the reconciliation is labelled as this
plugin's reading rather than a sourced claim.
above a variance threshold: pyperf, Criterion, JMH and benchstat all warn and print anyway.
Three claims the literature does not ground, labelled rather than dressed up
1/(1-p)floor. Thep50/p95-over-20 default is a labelled house convention; only the arithmetic floor is enforced.
percentiles that chapter names are the 99th and 99.9th.
spawns is this plugin's own generalization, and it is load-bearing here because spawn count is the
headline metric and Valgrind does not run on Windows.
Two citation traps the skill bodies avoid on purpose:
benchstatis unpaired (it recommendsinterleaved collection but analyzes with Mann-Whitney U), and coordinated omission is a
load-generator problem, so citing Tene for a synchronous harness would miscite the field's
best-known source.
Verification
check-skill.shwith 0 errors and 0 warnings.scripts/sync-spawn-noise.test.sh— 7 assertions, passing.plugins/claude-ops/lib/spawn_noise.test.sh— 9 assertions, passing.audit_performance.test.sh— 45 tests, passing, unmodified.scripts/check-lane-coverage.sh --check— all 50 lanes reachable fromci-status.needs, includingthe new one.
scripts/check-cross-plugin-source-drift.sh --check— no unregistered or drifted clusters.run-ruff.shclean on both lib copies; markdownlint clean; no em dashes in any new surface.The gates were proven to discriminate, not assumed to. This is the plugin's own doctrine applied
to its own code, and it matters because four of the five catalogued harness failures were checks that
exited identically in both arms and reported a confident verdict:
BIMODAL_SPREAD_RATIOand asserts the clean anddrifted arms return different verdicts, not merely that each printed its expected string.
high >= SLOW_SPAWN_FLOOR_MSclause by hand; thesuite failed with the assertion it was written to produce; restored; confirmed with an empty
git diffrather than trusting the restore. Done after committing, because harness defect feat(hook-telemetry): marketplace-wide telemetry contract + markdown-formatter producer #5in the catalogue was a
git checkout --restore over uncommitted work that destroyed it.test_a_quiet_host_is_measurable_and_a_contended_one_is_notruns both alow-variance and a high-variance host and asserts the verdicts differ, because a refusal that fires
on every host refuses nothing. The
snapshoteval suite carries the same positive/negative pair.The harnesses
Nine scripts under
plugins/performance/scripts/, each with a co-located suite, 200 assertionstotal. Ported from the source run's scratch tree, which lived on local disk only and would have died
with that directory.
spawn-census.sh/run-spawn-census.shuse a stable shim dir, closing the defect where amktemp -dshim put a fresh path onPATHevery run against aPATH-keyed cache, so the censusmeasured its own randomization and reported "no improvement".
ab.sh+summarize.py+ratio.pyinterleave the arms and flip order per iteration.
differential.pyproves behavior over an argvmatrix.
discriminate.pyconsolidates five variants, four of which were broken.The verifiers found seven real defects between them, all fixed. The two that matter most:
discriminate.pyscored a check that never ran. With nosignalconfigured the signal is theexit code, so any shared non-zero rc reported
NOT DISCRIMINATINGwith an affirmatively falseexplanation. It now splits identical-failing (
HARNESS BROKEN, exit 2) from identical-passing(
NOT DISCRIMINATING, exit 1). The four original harness failures that exited 127 in both armswould now be caught rather than reported clean.
truearms producedmedian_paired_ratio=1.06xbesideratio_of_p50=12.08x, so a reader could quote a 12x speedupbetween
trueandtrue. All three are gated now.Also fixed: a
126subject censused asspawns=0exit 0; a spliced rowratio.pyaccepted thatsummarize.pyrejects;printf | subjectunderpipefailfabricating exit 141 intermittently on apipe-buffer race; and an
osreference with no import on a line no test reached, which is exactlythe "check that never ran" shape that harness exists to detect.
Rule 4 is proven behaviorally, not asserted: the target is committed, the fix applied and left
uncommitted, and the fix is still present after the run. A
git checkout --restore would havedestroyed it, which is what defect #5 in the source catalogue actually did.
Related
Closes #3530. Depends on #3553 (merged), which promoted the lib.
Every acceptance criterion on that issue is met: the manifest validates, all four skills pass
check-skill.shwith zero warnings, the refusal is asserted with both a high-variance and alow-variance arm shown to differ, the drift-immune counter is ranked above any duration in the
emitted report, the design questions were answered by the user in a
/planning:interviewlinked from the issue,
and every normative claim carries a source tier with the ungrounded ones labelled as house rules.