Repository navigation
Point the executable-doc guard at the README as well - #214
Conversation
Three documented-command defects have shipped in this lineage and each was found by running the documentation: a reset naming a `delete` subcommand dl does not have (twice), `-n 5` beside a median over three runs, and `python scripts/bench_launch.py` on a host whose python is pixi's. The guard built after the first two reads the script's own epilog and drives the reset through main(). The third defect shipped in the README, which it never opened (#192). The README keeps its inline commands rather than deferring to --help: the epilog writes a bare shape sketch, the README writes the host invocation a reader copies -- which is where the interpreter lives, and the interpreter is the whole of the third defect. The points script's command is not in that epilog at all. So the same recipe is pointed at the second document. Every fenced invocation under "Measuring launch time" is extracted and handed to whatever would have caught each defect: the interpreter to the two routes this repo can vouch for, the flags to the script's own parser, the `pixi run` shortcuts beside them to the project manifest, and any `--before` reset to the same nothing-to-delete harness the epilog's reset goes through -- which moves to module scope so both documents share it. While the section documents no reset, the guard holds it to the pointer that stands in for one. The extraction is asserted non-empty in its own test, because pytest skips a parametrization with no cases and a guard whose document was rewritten out from under it reads like a guard that passed. bench_launch.py's parser gains allow_abbrev=False, the way the points script was always built (rider from #202's review). argparse accepts any unambiguous prefix of a long flag, so renaming --record to something it is a prefix of leaves every document saying --record, the parser still accepting it, and the rename shipping behind a green guard. A test abbreviates each documented flag by one letter to keep both parsers refusing. No documented or scripted invocation used an abbreviation.
Reviewer's GuideExtend the executable-doc guard to validate README’s bench commands alongside the script epilog, introduce a shared reset harness, and harden bench_launch.py’s CLI by disabling argparse abbreviations and testing them via README-derived cases. Flow diagram for README executable-doc guard validationsflowchart TD
A["README 'Measuring launch time' fenced bench commands"] --> B[Extract invocations for each bench script]
B --> C[Check interpreter route is python3 or pixi run]
B --> D[Parse flags with bench_launch.build_parser]
B --> E[Validate pixi run shortcut against project manifest]
B --> F[Run --before reset through shared reset harness]
F --> G[Call dl.main via reset harness]
B --> H[Assert at least one invocation extracted so guard cannot silently skip]
D --> I[Parser rejects abbreviated long flags]
I --> J[Tests fail if any documented flag is accepted one letter short]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
=======================================
Coverage 92.80% 92.80%
=======================================
Files 24 24
Lines 3654 3654
=======================================
Hits 3391 3391
Misses 263 263
🚀 New features to boost your workflow:
|
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review of #214 against ticket #192 at merge-base a68aab8. Preflight: all checks green, non-empty diff (4 files, +392/−21), MERGEABLE.
Standards
Nothing blocking. The file passes (17 tests, 0.08s), the naming matches house style (a_document/a_reset_…), and the cross-test-module import is established convention (test_timing already imports test_devpod_spawn_counts; test/conftest.py puts test/ on sys.path).
- The abbreviation guard ignores the
--boundary (non-blocking, sharpest).DOCUMENTED_FLAGScollects every--wordin the whole command line, including flags belonging to the benched command after--. Those reachcommand(nargs=REMAINDER), which accepts anything — verified:parse_args(['-n','5','--','dl-next','owner/repo','rm','--forc'])returns cleanly, sopytest.raises(SystemExit)would fail. No such flag is documented today, buttest_the_cold_recipes_reset_…explicitly invites publishing the cold recipe here, whose reset is... rm --force. Cut at the first bare--before collecting flags. VOUCHED_INTERPRETERSblesses a form another guard in the same file rejects (non-blocking).pixi run python scripts/bench_launch.py …passes the interpreter test, butDOCUMENTED_SHORTCUTSwould then yieldpythonand assert it is a pixi task. Two guards, opposite verdicts, on one documented line.- One harness, two extractions (non-blocking). The run harness is genuinely shared; extraction is not —
test_timing.pystill regexes--before '([^']+)'out of the epilog, precisely whatdocumented_resets()'s docstring calls "a second parser to be wrong in a second way". The house pattern for shared non-test code is a helper module. has_bench_section()guards absence but not duplication (nit). A duplicated heading raises during collection — the session-abort the docstring was written to prevent.- Primitive Obsession (nit). A command travels as a raw string and is
shlex.splitfour times; splitting once into a small record collapses it and makes the--fix one line.script_module()re-execs per parametrized case, uncached. - Proportionality: fine (observation). 320 lines against siblings at 448/202; the module-scope move's blast radius is small.
Spec
Verified: the two suites → 94 passed. Both required mutations reproduced and restored.
- The ticket's open decision, settled as "keep and guard" — sound. Spec: "Deleting the duplication beats guarding it, if the README's inline example earns nothing." It earns something: the epilog writes bare
bench_launch.py …with no interpreter, and the interpreter is the whole third defect;scripts/bench_points.pyappears in no epilog. Deletion would remove the defect site itself. - Rider from #202 landed on the real parser and is pinned.
scripts/bench_launch.py:268allow_abbrev=False; removing it turns the--record/--shapeabbreviation tests red. No documented invocation used an abbreviation. - Interpreter defect covered.
python3→pythonin the warm invocation goes red. Minor: the allowlist vouches for any first wordpixi, notpixi run <known task>; the shortcut guard covers task names separately. - Reset defect covered, arm live both directions.
--before 'dl-next owner/repo delete'→ red;rm --force→ green. - The third named defect is unguarded (non-blocking). Spec: "the
-n 5vs 3-runs mismatch" — nothing couples README's-n 5to the five runs quoted beside it. Scoped out as "hand-copied numbers", defensible under the ticket's bullet-2 guard list, but it is the one lineage defect a re-run would not catch. - Harness assumes a reset's first word is
dl(non-blocking).--before "scripts/bench_cold_reset.sh owner/repo"— the exact shapebench.yml:158uses — fails with "which dl refuses". Loud, not silent; the message is wrong. - Deviations 1, 2, 4 verified/sound — with one count correction: renaming the heading gives 2 failed, 5 skipped, not "4 clean failures"; collection does not abort and the vacuity test is the loud failure.
- CHANGELOG under
[Unreleased] → Changed, claims check out.
Verdict
Approve — no blocking findings on either axis. Riders for the next toucher of test_bench_doc.py: cut flag collection at the first bare --; reconcile VOUCHED_INTERPRETERS with DOCUMENTED_SHORTCUTS on the pixi run python form; share the --before extraction; teach the harness the bench_cold_reset.sh reset shape. GitHub refuses --approve from the PR author's account, so this is posted as a comment; this written verdict governs. Merging with a merge commit per repo rules.
Closes #192.
What shipped
The executable-doc guard read
bench_launch.py's epilog and never the README — and the README is where the third defect in this lineage actually shipped (python scripts/bench_launch.py, on a host with nopythonoutside pixi, fixed by hand in 5510b1e after publication). The guard now reads both documents.The ticket's open decision, settled: the README keeps its inline commands and the guard learns to read them. The epilog writes a bare
bench_launch.py ...shape sketch; the README writes the host invocation a reader copies, which is where the interpreter lives — and the interpreter is the whole of the third defect. The trend section's command is not in that epilog at all (it belongs to the points script), and the README's copy carries the flag combination in the context that explains it. Deleting the duplication would take both with it.test/test_bench_doc.pyextracts every fenced invocation of a bench script under "Measuring launch time" and hands it to whatever would have caught each defect:python3, orpixi run, whose task brings its own) — no parser will ever see that word;pixi runshortcuts the prose offers beside them to the project manifest, since a renamed task leaves the sentence confidently wrong;--beforereset to the same nothing-to-delete harness the epilog's reset goes through, which moves to module scope intest_timing.pyso both documents share one starting state.The section documents no reset today, so that arm holds it to the pointer that stands in for one instead — publish a reset and it is run, publish neither and the page has stopped telling a reader how to bench a cold launch. One further test asserts the extraction is non-empty, because pytest skips a parametrization with no cases: a guard whose document was rewritten out from under it otherwise reads exactly like a guard that passed. (A missing section is deliberately not asserted at import time — that would abort collection for the whole suite instead of failing four tests.)
The rider from #202's review
bench_launch.py's parser is now built withallow_abbrev=False, the waybench_points.pyalways was. argparse accepts any unambiguous prefix of a long flag, so renaming--recordto something it is a prefix of leaves every document saying--record, the parser still accepting it, and the rename shipping behind a green guard.test_no_documented_flag_would_be_accepted_one_letter_shortabbreviates each documented flag by one letter and requires the refusal, covering both scripts. No documented or scripted invocation — README, epilog,bench.yml,pixitasks — used an abbreviation, so nothing that runs today changes.Mutation checks
Every test was verified to go red against the defect it claims to catch, and the tree restored between each:
python3→python(the defect that shipped)--record→--record-filepixi run bench-points→pixi run bench-pts`bench_launch.py --help`pointer deleted--before 'dl-next owner/repo delete'(the historical defect)main()--before 'dl-next owner/repo rm --force'(correct)allow_abbrev=Falsereverted--record,--shapedeleteChecks
pixi run ciexits 0 locally: 1703 passed, 23 deselected in 155s; pylint 10.00/10; ruff andtyclean.python3 scripts/bench_launch.py -n 2 -- truestill runs after the parser change.🤖 Generated with Claude Code
Summary by Sourcery
Extend documentation-based guardrails for benchmarking commands to cover README content and tighten CLI flag validation for bench scripts.
New Features:
Bug Fixes:
--beforereset commands are either executed against an empty devpod state or the README still points readers to script help for a valid reset.Enhancements:
bench_launch.pyand add coverage that abbreviated documented flags are rejected, aligning behavior withbench_points.py.Documentation:
bench_launch.py.Tests:
test_bench_doc.pyto systematically exercise README-based benchmarking commands, pixi tasks, and reset recipes, including failure-on-empty extraction and abbreviation guards.