From b33d910ff424352a043177387895b82e98df3c47 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Fri, 17 Jul 2026 22:02:26 +0300 Subject: [PATCH] refactor(benchmarks): subcommand-scoped CLI flags via subparsers The flat parser hung every flag off one positional command, so `run --markdown` (and other cross-command flag misuse) parsed and silently no-oped. Split into `run`/`check` subparsers owning their own flags, so a misplaced flag is an argparse error. Extract _build_parser() and give main(argv=None) an optional argv so the grammar is unit-testable without Postgres. Co-Authored-By: Claude Opus 4.8 (1M context) --- benchmarks/__main__.py | 32 ++++++++---- ...2026-07-17.04-benchmarks-cli-subparsers.md | 49 +++++++++++++++++++ tests/test_benchmarks.py | 32 ++++++++++++ 3 files changed, 104 insertions(+), 9 deletions(-) create mode 100644 planning/changes/2026-07-17.04-benchmarks-cli-subparsers.md diff --git a/benchmarks/__main__.py b/benchmarks/__main__.py index 7c67b8e..34a629a 100644 --- a/benchmarks/__main__.py +++ b/benchmarks/__main__.py @@ -102,16 +102,30 @@ def _run(dsn: str, messages: int, repeats: int, *, write_baseline: bool) -> int: return 0 -def main() -> int: +def _build_parser() -> argparse.ArgumentParser: + """Two subcommands with their own flags, so a misplaced flag errors at parse. + + A flat parser accepted every flag with either command (`run --markdown` parsed + and silently no-oped); subparsers scope each flag to the command that reads it. + """ parser = argparse.ArgumentParser(prog="benchmarks") - parser.add_argument("command", choices=("run", "check")) - parser.add_argument("--messages", type=int, default=5_000) - # Wall-clock is IO-noisy, so `run` repeats each point and reports the median. `check` - # uses 1: the gated counters are deterministic, so repeating them only burns CI time. - parser.add_argument("--repeats", type=int, default=3) - parser.add_argument("--write-baseline", action="store_true") - parser.add_argument("--markdown", action="store_true") - args = parser.parse_args() + subparsers = parser.add_subparsers(dest="command", required=True) + + run = subparsers.add_parser("run", help="run the sweep and print the table") + run.add_argument("--messages", type=int, default=5_000) + # Wall-clock is IO-noisy, so `run` repeats each point and reports the median. + run.add_argument("--repeats", type=int, default=3) + run.add_argument("--write-baseline", action="store_true") + + # `check` uses repeats=1: the gated counters are deterministic, so repeating them + # only burns CI time -- it takes no --repeats/--messages (pinned to the baseline). + check = subparsers.add_parser("check", help="gate the counters against baseline.json") + check.add_argument("--markdown", action="store_true") + return parser + + +def main(argv: list[str] | None = None) -> int: + args = _build_parser().parse_args(argv) dsn = os.environ.get("POSTGRES_DSN", DEFAULT_DSN) diff --git a/planning/changes/2026-07-17.04-benchmarks-cli-subparsers.md b/planning/changes/2026-07-17.04-benchmarks-cli-subparsers.md new file mode 100644 index 0000000..d18fbb2 --- /dev/null +++ b/planning/changes/2026-07-17.04-benchmarks-cli-subparsers.md @@ -0,0 +1,49 @@ +--- +summary: Convert the `benchmarks` CLI from a single positional `command` with all flags on one flat parser to real argparse subparsers, so `run --markdown` (and other cross-command flag misuse) is a hard error instead of a silent no-op. +--- + +# Change: Subcommand-scoped flags for the benchmarks CLI + +**Lane:** lightweight — ≲30 LOC net, 2 files, no new file, dev-only CLI, a +focused grammar test. + +## Goal + +`benchmarks/__main__.py` registers `command` as one positional with `choices` +and hangs every flag (`--messages`, `--repeats`, `--write-baseline`, +`--markdown`) off the single top-level parser. argparse therefore accepts any +flag with either command; `--markdown` is read only in the `check` branch and +`--messages`/`--repeats`/`--write-baseline` only in `run`, so e.g. +`benchmarks run --markdown` parses and silently does nothing. Give each +subcommand its own flags so nonsensical combinations fail at parse time. + +## Approach + +Replace the positional `command` + flat flags with +`parser.add_subparsers(dest="command", required=True)`: a `run` subparser owning +`--messages`/`--repeats`/`--write-baseline`, a `check` subparser owning +`--markdown`. Dispatch stays `if args.command == "check"`. Extract the parser +into `_build_parser()` so the grammar is unit-testable without running the +workload (which needs Postgres), and give `main(argv=None)` an optional argv for +the same reason. No behavior change on the happy paths the `just` recipes use +(`run …`, `check`, `check --markdown`); the only new behavior is that misplaced +flags now error. + +## Files + +- `benchmarks/__main__.py` — `_build_parser()` with two subparsers; + `main(argv=None)` delegates to it. +- `tests/test_benchmarks.py` — grammar tests: `run --markdown` and + `check --write-baseline` raise `SystemExit`; `check --markdown` and + `run --messages … --write-baseline` parse to the right namespace. + +## Verification + +- [ ] Failing test first — `run --markdown` should `SystemExit`; before the + change it parses cleanly (test fails on the missing raise). +- [ ] Apply the change. +- [ ] `uv run pytest tests/test_benchmarks.py -v --no-cov` passes. +- [ ] `python -m benchmarks run --markdown` exits non-zero with an argparse + "unrecognized arguments" error; `python -m benchmarks --help` lists the + `run`/`check` subcommands. +- [ ] `just lint` — clean. diff --git a/tests/test_benchmarks.py b/tests/test_benchmarks.py index 73a3b08..981c520 100644 --- a/tests/test_benchmarks.py +++ b/tests/test_benchmarks.py @@ -9,6 +9,7 @@ import pytest +from benchmarks.__main__ import _build_parser from benchmarks.config import RunConfig from benchmarks.probes import ProbeResult from benchmarks.report import EXACT_KEYS, compare, format_markdown, format_table, normalize, to_baseline @@ -212,3 +213,34 @@ def test_format_markdown_renders_failing_verdict_and_bullets() -> None: body = format_markdown([_result()], failures) assert "❌ gate FAILED" in body assert "- consumer/w1/b100: delete_calls changed" in body + + +def test_run_rejects_the_markdown_flag() -> None: + # --markdown belongs to `check`; subparsers make it a hard error under `run` + # instead of the old silent no-op. + with pytest.raises(SystemExit): + _build_parser().parse_args(["run", "--markdown"]) + + +def test_check_rejects_run_only_flags() -> None: + # Symmetrically, run-only flags are not accepted by `check`. + with pytest.raises(SystemExit): + _build_parser().parse_args(["check", "--write-baseline"]) + + +def test_check_accepts_markdown() -> None: + args = _build_parser().parse_args(["check", "--markdown"]) + assert args.command == "check" + assert args.markdown is True + + +def test_run_accepts_its_flags() -> None: + args = _build_parser().parse_args(["run", "--messages", "200", "--write-baseline"]) + assert args.command == "run" + assert args.messages == 200 + assert args.write_baseline is True + + +def test_a_subcommand_is_required() -> None: + with pytest.raises(SystemExit): + _build_parser().parse_args([])