Repository navigation
fix(benchmarks): reproduce.sh --only fails closed on a selector that runs nothing - #550
Conversation
…runs nothing --only was compared token for token against longmemeval, locomo, beam and decision-ids, while the script itself prints the artifact names longmemeval-s and beam-100K. Passing one of those, or any typo, matched no benchmark: the script built the package, started and stopped an ephemeral container, wrote MANIFEST.json and START_SNAPSHOT.json, measured nothing and exited 0. Reproduced on main before the change. A sourced library, benchmarks/lib/bench_only.sh, maps the two aliases to the tokens want_bench matches and fails closed on an unknown or empty token with exit 2 and the accepted list, at the end of parse_args, before any uv run or container start. Empty tokens are detected on the whole string first, because read -a drops a trailing empty field. Eleven tests pin the alias table, the rejection cases and the script's end-to-end behaviour, which now needs neither Docker nor a synced environment. README row and CHANGELOG entry added. Co-Authored-By: Claude Fable 5.1 <[email protected]>
|
ZETETIC-REVIEW: APPROVE Verdict on head 5060595. An independent fresh-context reviewer read the library as a hostile shell reviewer and found no bug: the unset-or-empty ONLY check is placed so set -e does not misfire; the comma-wrap check catches leading, trailing and doubled commas before the read -a split, which drops a trailing empty field and is why the whole-string check has to come first; the negated command substitution and the Verified by the reviewer from a detached scratch worktree at this head: shellcheck -S warning clean on the new library; the 11 tests pass; the end-to-end GREEN run exits 2 with no results directory and no container created; RED confirmed on main, where the library does not exist and the test file fails. I had reproduced RED on main earlier the same way the PR body describes: exit 0, container cortex-bench-pg-17377 started and stopped, MANIFEST.json and START_SNAPSHOT.json written, nothing measured. One SHOULD-FIX, non-blocking and disclosed in the PR body: --ablate-on flows through the same alias table and is not validated here; whether the downstream Python rejects an unknown name is a separate question and stays out of this change, which is the installer-style discipline of keeping a delivery to its subject. CI on this head: 24 pass, 4 skipping, zero failures. |
Summary
benchmarks/reproduce.sh --onlycompared its value token for token againstlongmemeval,locomo,beamanddecision-ids, while the script itself prints the artifact nameslongmemeval-sandbeam-100K. Passing one of those, or any typo, matched no benchmark: the script built the package, started and stopped an ephemeral container, wroteMANIFEST.jsonandSTART_SNAPSHOT.json, measured nothing and exited 0.Observed on 2026-09-09 while re-measuring v4.20.0, and reproduced on
mainbefore writing the fix (see the test plan). An empty run that reports success is the silent fallback this repository forbids elsewhere.The fix
A new sourced library,
benchmarks/lib/bench_only.sh, on the pattern ofbenchmarks/lib/bench_regression.sh:bench_only_canonical_tokenmaps the two aliases to the tokenswant_benchmatches, andvalidate_onlyrewritesONLYas canonical comma-separated tokens or fails closed with exit 2 and the accepted list on stderr. It runs at the end ofparse_args, beforecheck_reranker_cell, so before anyuv runor container start. An empty token (leading, trailing or doubled comma) is rejected too;read -awould otherwise drop a trailing empty field silently.benchmarks/README.mddocuments the aliases and the fail-closed behaviour.--ablate-ontakes benchmark names through the same alias table but is not validated here; noted, not folded in.Type of change
Test plan
RED, on
main, before the change:bash benchmarks/reproduce.sh --only bogus --no-ablation --results-dir <tmp>exited 0 after building the package, starting containercortex-bench-pg-17377, and writingMANIFEST.jsonandSTART_SNAPSHOT.json; no error, nothing measured.GREEN, after: the same invocation prints
error: --only 'bogus' selects no benchmark; nothing would run.plus the accepted list, exits 2 in well under a second, and writes nothing.ONLY="longmemeval-s,beam-100K"is rewritten tolongmemeval,beam.tests_py/benchmarks/test_reproduce_only_validation.py, 11 tests: alias normalisation for each alias and a mixed list,decision-idsaccepted, unknown token, unknown token in a list, trailing and leading empty tokens all rejected with the accepted list named, empty--onlymeaning every benchmark, and one end-to-end test of the real script that needs neither Docker nor a synced environment because validation precedes both.Mutations these catch: removing the alias table (the normalisation cases fail), removing the empty-token guard (the trailing-comma case passes silently again), and moving
validate_onlyaftercheck_reranker_cellor the container start (the end-to-end test would need Docker and stop returning in under a second).Gates on the final state:
bash -non both scripts,shellcheck -S warningclean on the new library (the three SC2034 warnings onreproduce.shpre-date this change and CI's shellcheck step covers workflow files only),ruff checkandruff format --checkon the tree,scripts/check_craftsmanship.py,scripts/check_doc_claims.py, andtests_py/benchmarks/test_bench_regression_datasets.pystill green.Audit notes
Self-review plus the RED/GREEN evidence. A fresh-context review is requested before merge.
Reviewer checklist
benchmarks/README.md).🤖 Generated with Claude Code