feat(performance): add the measurement-first optimization plugin - #3561
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. |
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".
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 has reviewed this PR 5 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
|
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
|
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
…count The hygiene lane's machine-specific-paths check failed on a comment in harness-lib.sh that named this machine's actual home directory while explaining the 8.3 short-name mismatch. The explanation is load-bearing -- a string prefix test between the two spellings finds nothing, so a temp-rooted shim dir sails through the check meant to catch it -- but it does not need a real username to make the point. Also drops "200 assertions across the nine" from the CHANGELOG. The suites grew after that sentence was written, and a plugin whose whole premise is refusing to report unsupported numbers should not ship a count that has already drifted. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
…t p50 The disagreement flag discarded ratio_of_p95 at the call site (`p95_cell, _ = ...`), so it never inspected the one statistic it printed without checking. A tail spike in one arm produced median_paired_ratio=1.00x, ratio_of_p50=1.00x and ratio_of_p95=5.95x on the same line, with nothing saying the three disagree. A reader quotes whichever number is printed. The check now takes the widest spread across all three and names which statistic diverged. Adds the p95-only divergence case plus its negative arm, since a flag that fires on every run flags nothing. Found by the fresh-context verifier, which asked the question the fix for the earlier ungated-ratio defect did not: gating the subordinates stopped them printing below the floor, but said nothing about whether they agree above it. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
20e9433 to
7dc4b5b
Compare
|
Claude finished @kyle-sexton's task in 2m 11s —— View job
|
|
Claude finished @kyle-sexton's task in 1m 57s —— View job
|
… machine path Three CI failures, all real: plugin-gate rejected `workflow-stage: discovery` on the target skill. That value is not in the cheat-sheet generator's enum; the stage for candidate identification is `explore`. Regenerated docs/SKILL-CHEAT-SHEET.md, which the same gate compares against the manifests, so all four skills now appear in it. machine-specific-paths failed a second time on a comment in harness-lib.test.sh that the first pass missed: the same 8.3 short-name explanation, duplicated in the test beside the lib. The explanation is load-bearing and stays, since the spelling-independence of the temp-root rejection is the only thing that case proves, but it no longer names a real home directory. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
|
Claude finished @kyle-sexton's task in 1m 11s —— View job
|
|
Claude finished @kyle-sexton's task in 43s —— View job
|
Both reviewers converged on the same three, independently. All real. **Timeout now outranks bimodality in is_measurable.** A timed-out sample is recorded at the timeout ceiling, not as a measurement, so max_ms is censored. Reporting the bimodal reason first handed the reader a spread computed from that ceiling and explained it as an observed slow mode, which reads as a finite measurement of a tail that is actually unbounded. The refusal now names the timeout count and says max_ms is censored. Covered by a case where both findings co-occur, which is the only place the precedence is observable. **The documented import example did not run.** `from spawn_noise import ...` raises ModuleNotFoundError unless the caller is already in lib/, because the lib is plugin-bundled rather than installed. The example now anchors sys.path to the plugin root and says how to count the parents for a caller sitting elsewhere. A documented example that fails as written is the same defect class this plugin exists to prevent, in its own instructions. **The /verification:measure dependency is presence-gated.** It was written as an unconditional dependency with no fallback and no manifest declaration. It is now a preference for reuse: reuse it when the verification plugin is installed, otherwise capture into the same memory-tier path directly and say in the report that the capture was unassisted. This plugin's own gates work either way, and refusing to measure because a sibling plugin is missing would fail worse than the duplication it avoids. The README says the same for every neighbour it names, and records that neither plugin imports the other at runtime since plugins install independently. Refs #3530 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDkZJ87i7h8J9KzDNruvuS
|
Claude finished @kyle-sexton's task in 56s —— View job
|
|
All three review findings fixed in 1. Timeout now outranks bimodality in
|

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.