fix(cli): mirror --version printed an error instead of a version
Build and Push Containers / test (push) Successful in 1m20s
Build and Push Containers / build-and-push-app (push) Successful in 9s

`main()` rewrites a bare invocation into `serve`, which is what makes
`mirror --port 8055` work at all — but it did that for every argument that
starts with a dash, so `mirror --version` became `mirror serve --version`, the
subparser has no such flag, and the command exited with "unrecognized
arguments". Found by pulling the image CI published and asking it what it was.

The rewrite rule is now `with_serve()`, extracted so the eight interesting
inputs are asserted directly — including `--port 9999` still becoming a serve
and `serve --version` staying as typed. `serve` also grew its own --version,
because typing it there is just as reasonable and argparse does not inherit
flags from a parent parser.

The extraction is not cosmetic: the first version of this test monkeypatched
`cmd_serve` to prove the rewrite, which did nothing, because argparse binds
`func` at parse time — the test would have started a real uvicorn on 9999 and
hung the suite. Asserting the pure function is both correct and safe.

187 tests pass, 2 skipped; ruff and pyright clean.

LLM-Generated: true
This commit is contained in:
2026-11-01 18:14:45 -05:00
parent 28dfc26500
commit 269b64bace
2 changed files with 51 additions and 4 deletions
+21 -4
View File
@@ -24,6 +24,20 @@ def _positive_int(raw: str) -> int:
return value return value
def with_serve(argv: list[str]) -> list[str]:
"""``mirror``, ``mirror --host x`` etc. all mean ``serve``.
A bare informational flag does not: prepending would route ``--version`` into
the ``serve`` subparser, which has no such flag, and the command would die
with "unrecognized arguments" instead of printing a version. Extracted from
main() so the rule can be asserted without starting anything.
"""
informational = ("-h", "--help", "--version")
if not argv or (argv[0].startswith("-") and argv[0] not in informational):
return ["serve", *argv]
return list(argv)
def build_parser() -> argparse.ArgumentParser: def build_parser() -> argparse.ArgumentParser:
parser = argparse.ArgumentParser( parser = argparse.ArgumentParser(
prog="mirror", prog="mirror",
@@ -64,6 +78,12 @@ def build_parser() -> argparse.ArgumentParser:
choices=["critical", "error", "warning", "info", "debug", "trace"], choices=["critical", "error", "warning", "info", "debug", "trace"],
help="Uvicorn log level (default: %(default)s).", help="Uvicorn log level (default: %(default)s).",
) )
# Repeated from the top level on purpose: `mirror serve --version` is as
# reasonable a thing to type as `mirror --version`, and argparse does not
# inherit flags from the parent parser.
serve.add_argument(
"--version", action="version", version=f"mirror {__version__}", help="Print the version."
)
serve.set_defaults(func=cmd_serve) serve.set_defaults(func=cmd_serve)
return parser return parser
@@ -127,10 +147,7 @@ def cmd_serve(args: argparse.Namespace) -> int:
def main(argv: list[str] | None = None) -> int: def main(argv: list[str] | None = None) -> int:
parser = build_parser() parser = build_parser()
argv = list(sys.argv[1:] if argv is None else argv) argv = with_serve(list(sys.argv[1:] if argv is None else argv))
if not argv or argv[0].startswith("-"):
# `mirror`, `mirror --host x` etc. all mean `serve`.
argv = ["serve", *argv]
args = parser.parse_args(argv) args = parser.parse_args(argv)
return int(args.func(args)) return int(args.func(args))
+30
View File
@@ -114,6 +114,36 @@ class TestCli:
"fd00::/64", "fd00::/64",
] ]
@pytest.mark.parametrize(
("given", "expected"),
[
([], ["serve"]),
(["--port", "9999"], ["serve", "--port", "9999"]),
(["--allow-cidr", "10.2.0.0/22"], ["serve", "--allow-cidr", "10.2.0.0/22"]),
(["serve"], ["serve"]),
(["serve", "--version"], ["serve", "--version"]),
(["--version"], ["--version"]),
(["--help"], ["--help"]),
(["-h"], ["-h"]),
],
)
def test_only_informational_flags_escape_the_implicit_serve(self, given, expected):
assert entry.with_serve(given) == expected
@pytest.mark.parametrize("argv", [["--version"], ["serve", "--version"]])
def test_version_prints_and_exits_cleanly(self, argv, capsys):
with pytest.raises(SystemExit) as caught:
entry.main(argv)
assert caught.value.code == 0
assert f"mirror {entry.__version__}" in capsys.readouterr().out
def test_help_lists_the_subcommand_it_is_not_being_rewritten_into(self, capsys):
with pytest.raises(SystemExit):
entry.main(["--help"])
out = capsys.readouterr().out
assert "usage: mirror" in out
assert "serve" in out
def test_a_bad_allow_cidr_exits_with_a_message_not_a_traceback(self, capsys): def test_a_bad_allow_cidr_exits_with_a_message_not_a_traceback(self, capsys):
# Config is parsed before anything binds or imports, so this cannot stall # Config is parsed before anything binds or imports, so this cannot stall
# on a stray DEBUGPY in the operator's shell. # on a stray DEBUGPY in the operator's shell.