From e43e04273d000dc3afa37700c248aeaf8cd44215 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 05:31:46 +0000 Subject: [PATCH 1/2] Fail a pull request whose benchmark charts drift from their history (#303) Nothing checked that the committed SVGs under docs/benchmarks/ are what scripts/benchmark-history.cs draws from the committed history JSON, so a hand-edited history or a renderer change could leave the README charts wrong without any check failing. verify-charts.yml runs on pull requests that touch the histories, the charts, the renderer or the recorder workflow. It redraws every chart and fails on any difference, naming each drifting file. The render loop moves out of benchmark-history.yml into scripts/render-benchmark-charts.sh, and both workflows run it. The guard reads the subject table from benchmark-history.yml's env, so the guard and the recorder can't drift apart. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW2sHrLz9iC1cJzLLZy --- .github/workflows/benchmark-history.yml | 14 ++---- .github/workflows/verify-charts.yml | 67 +++++++++++++++++++++++++ CLAUDE.md | 4 +- scripts/render-benchmark-charts.sh | 24 +++++++++ 4 files changed, 97 insertions(+), 12 deletions(-) create mode 100644 .github/workflows/verify-charts.yml create mode 100755 scripts/render-benchmark-charts.sh diff --git a/.github/workflows/benchmark-history.yml b/.github/workflows/benchmark-history.yml index 9279b23d..542441e5 100644 --- a/.github/workflows/benchmark-history.yml +++ b/.github/workflows/benchmark-history.yml @@ -60,6 +60,7 @@ env: BENCHMARK_JOB: short # One line per chart: name|history|chart|filter. An environment variable cannot hold an array, # and three steps need the same three triples, so this is the one place they are written. + # verify-charts.yml reads this value from here too, so it stays the only copy. # # The quantities filter names one operation per panel. The strings and paths filters name whole # classes instead, so they measure more than they draw: ingest stores every row and only render @@ -203,19 +204,10 @@ jobs: done <<< "$SUBJECTS" done + # The same script verify-charts.yml runs, so the charts this commits are the ones it checks. - name: Redraw the charts shell: bash - run: | - set -euo pipefail - while IFS='|' read -r subject history chart filter; do - [ -n "$subject" ] || continue - if [ ! -f "$history" ]; then - echo "No history for $subject yet; nothing to draw." - continue - fi - dotnet run scripts/benchmark-history.cs -- render \ - --subject "$subject" --history "$history" --out "$chart" &2 + exit 1 + fi + echo "Every benchmark chart matches its history." diff --git a/CLAUDE.md b/CLAUDE.md index e381cf09..35f57457 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -409,7 +409,9 @@ redraws the chart the README shows. Four things about it are not guessable from version across every package. - **The histories and the SVG files are committed output that a bot pushes.** A local `render` must be diffed before commit rather than assumed. The quantities chart in particular is a regression test: - a change to the renderer that moves a byte in it has broken something. + a change to the renderer that moves a byte in it has broken something. `verify-charts.yml` enforces + this on pull requests: it redraws every chart through `scripts/render-benchmark-charts.sh`, the step + the recorder runs, and fails on any difference. - **Nothing here touches an internal member.** The `InternalsVisibleTo` that would expose one names only the test assembly, and a benchmark built on internals could only ever measure the working copy, never a published package — which would make the release history impossible to backfill. diff --git a/scripts/render-benchmark-charts.sh b/scripts/render-benchmark-charts.sh new file mode 100755 index 00000000..fe351e27 --- /dev/null +++ b/scripts/render-benchmark-charts.sh @@ -0,0 +1,24 @@ +#!/usr/bin/env bash +# Redraws every benchmark chart from its committed history. +# +# The one render invocation both workflows run: benchmark-history.yml after it records a point, +# and verify-charts.yml to check that the committed charts are what the committed histories draw. +# Sharing it is what keeps the guard and the recorder from drifting apart (#303). +# +# Reads the subject table from $SUBJECTS, one `name|history|chart|filter` line per subject, as +# benchmark-history.yml declares it. The filter is not used here. +set -euo pipefail + +: "${SUBJECTS:?SUBJECTS must hold the subject table from benchmark-history.yml}" + +while IFS='|' read -r subject history chart _filter; do + [ -n "$subject" ] || continue + if [ ! -f "$history" ]; then + echo "No history for $subject yet; nothing to draw." + continue + fi + # stdin from /dev/null: a process that drained the here-string would end the loop early and + # still exit zero, silently skipping the remaining subjects. + dotnet run scripts/benchmark-history.cs -- render \ + --subject "$subject" --history "$history" --out "$chart" Date: Thu, 29 Oct 2026 05:45:26 +0000 Subject: [PATCH 2/2] Use [[ ]] in the chart render script Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW2sHrLz9iC1cJzLLZy --- scripts/render-benchmark-charts.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/render-benchmark-charts.sh b/scripts/render-benchmark-charts.sh index fe351e27..e65022d4 100755 --- a/scripts/render-benchmark-charts.sh +++ b/scripts/render-benchmark-charts.sh @@ -12,8 +12,8 @@ set -euo pipefail : "${SUBJECTS:?SUBJECTS must hold the subject table from benchmark-history.yml}" while IFS='|' read -r subject history chart _filter; do - [ -n "$subject" ] || continue - if [ ! -f "$history" ]; then + [[ -n "$subject" ]] || continue + if [[ ! -f "$history" ]]; then echo "No history for $subject yet; nothing to draw." continue fi