From 9ce7b6ea549f38b24281be933f9515892db1c462 Mon Sep 17 00:00:00 2001 From: Andrew Young Date: Sat, 22 Aug 2026 19:05:10 -0500 Subject: [PATCH] fix: give catalog subcommands the same output vocabulary as everything else Found by cold-installing the repo into a fresh virtualenv and typing the commands a new user would type, in order. The ninth one failed. No catalog subcommand accepted `-o` at all, `--quiet` reached only `ingest`, and `--markdown` reached only the others -- while `-o` works on all ten analyzers. The earlier uniformity check only covered analyzers, so the split went unseen. Applied once, after every subparser is built, rather than inside each one. Adding them per-subcommand is precisely how they drifted apart. The first attempt at this fix added the flags to the parsers and stopped there, so `-o` parsed cleanly and wrote nothing -- the dead-flag bug this project keeps producing, and worse than not offering the flag, because the caller believes a file was written. Every payload now goes through one `_emit` helper. The test asserts behaviour rather than help text: that `-o` writes a non-empty file AND leaves stdout empty, that the default is JSON, and that `--markdown` is not. Unwiring `-o` again fails it. Co-Authored-By: Claude Opus 5 (1M context) --- drydock/catalog/cli.py | 54 ++++++++++++++++++++++++++++--------- tests/test_cli.py | 61 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 12 deletions(-) diff --git a/drydock/catalog/cli.py b/drydock/catalog/cli.py index ee2fb33..305e8bd 100644 --- a/drydock/catalog/cli.py +++ b/drydock/catalog/cli.py @@ -211,6 +211,23 @@ def _build_parser() -> argparse.ArgumentParser: help="Catalog root (default: ~/.drydock; can also use DRYDOCK_CATALOG env var)", ) + # One vocabulary, applied after every subparser is built rather than + # inside each one. Adding these per-subcommand is exactly how they drifted: + # no catalog subcommand accepted `-o` at all, and `--quiet` reached only + # `ingest` while `--markdown` reached only the others. A user meets that in + # their first minute, because `-o` works on all ten analyzers. + for sub in subparsers.choices.values(): + have = {a.dest for a in sub._actions} + if "output" not in have: + sub.add_argument("--output", "-o", metavar="FILE", + help="Write to FILE instead of stdout") + if "markdown" not in have: + sub.add_argument("--markdown", "-m", action="store_true", + help="Render for humans (default: JSON)") + if "quiet" not in have: + sub.add_argument("--quiet", "-q", action="store_true", + help="Suppress progress output on stderr") + return parser @@ -297,6 +314,19 @@ def _format_search(hits: list) -> str: return "\n".join(lines) + +def _emit(args, text: str) -> None: + """Send a payload to --output if given, otherwise stdout. + + Every handler goes through here. Adding `--output` to the parsers without + this is the dead-flag bug in miniature: the flag parsed, was accepted, and + did nothing -- which is worse than not offering it, because the caller + believes the file was written. + """ + write_output(text, getattr(args, "output", None), + Reporter(quiet=getattr(args, "quiet", False))) + + def _cmd_ingest(args) -> int: """Execute the ingest subcommand.""" catalog = Catalog(args.catalog or None) @@ -334,7 +364,7 @@ def _cmd_ingest(args) -> int: for phase, error in result.failures.items(): reporter.warn(f"{phase}: {error}") - print(dumps({ + _emit(args, dumps({ "repo_id": result.repo_id, "components_recorded": result.components_recorded, "components_stored": result.components_stored, @@ -355,7 +385,7 @@ def _cmd_list(args) -> int: try: repos = catalog.repos() if not args.markdown: - print(dumps([{ + _emit(args, dumps([{ "id": r.id, "name": r.name, "source": r.source, @@ -365,7 +395,7 @@ def _cmd_list(args) -> int: "file_count": r.file_count, } for r in repos])) else: - print(_format_list(repos)) + _emit(args, _format_list(repos)) return EXIT_OK except Exception as exc: print(f"error: {exc}", file=sys.stderr) @@ -393,7 +423,7 @@ def _cmd_show(args) -> int: facts = catalog.facts(repo_id=repo.id) if not args.markdown: - print(dumps({ + _emit(args, dumps({ "repo": { "id": repo.id, "name": repo.name, @@ -465,7 +495,7 @@ def _cmd_components(args) -> int: ) if not args.markdown: - print(dumps([{ + _emit(args, dumps([{ "id": c.id, "repo_id": c.repo_id, "name": c.name, @@ -478,7 +508,7 @@ def _cmd_components(args) -> int: "summary": c.summary, } for c in components])) else: - print(_format_components(components, catalog.root)) + _emit(args, _format_components(components, catalog.root)) return EXIT_OK except Exception as exc: print(f"error: {exc}", file=sys.stderr) @@ -498,7 +528,7 @@ def _cmd_search(args) -> int: ) if not args.markdown: - print(dumps([{ + _emit(args, dumps([{ "subject_id": h.subject_id, "subject_kind": h.subject_kind, "repo_id": h.repo_id, @@ -507,7 +537,7 @@ def _cmd_search(args) -> int: "rank": h.rank, } for h in hits])) else: - print(_format_search(hits)) + _emit(args, _format_search(hits)) return EXIT_OK except Exception as exc: print(f"error: {exc}", file=sys.stderr) @@ -546,7 +576,7 @@ def _cmd_diagram(args) -> int: ) if not args.markdown: - print(dumps({ + _emit(args, dumps({ "repo_id": repo.id, "diagram_kind": args.kind, "mermaid": mermaid_text, @@ -568,9 +598,9 @@ def _cmd_stats(args) -> int: try: stats = catalog.stats() if not args.markdown: - print(dumps(stats)) + _emit(args, dumps(stats)) else: - print(_format_stats(stats)) + _emit(args, _format_stats(stats)) return EXIT_OK except Exception as exc: print(f"error: {exc}", file=sys.stderr) @@ -613,7 +643,7 @@ def _cmd_forget(args) -> int: result = catalog.forget(repo.id, delete_content=delete_content) if not args.markdown: - print(dumps({ + _emit(args, dumps({ "repo_id": repo.id, "repo_name": repo.name, "components_removed": result["components"], diff --git a/tests/test_cli.py b/tests/test_cli.py index a7f55cd..98b698f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -346,3 +346,64 @@ def test_no_tests_filters_by_role(self, simple_python_project: Path) -> None: num_with_tests = len(files_with_tests) assert num_no_tests <= num_with_tests + + +# ============================================================================ +# The shared output vocabulary must reach EVERY command, catalog included. +# ============================================================================ + +def test_catalog_subcommands_share_the_output_vocabulary(tmp_path, monkeypatch): + """Every catalog subcommand accepts -o, --markdown and --quiet, and honours them. + + These had drifted: no catalog subcommand accepted `-o` at all, `--quiet` + reached only `ingest`, and `--markdown` reached only the others. A user + meets that immediately, because `-o` works on all ten analyzers. + + Behaviour is asserted, not the help text. Accepting a flag and honouring it + are different things -- the first attempt at this fix added the flags to + the parsers without wiring them, so `-o` parsed cleanly and wrote nothing, + which is worse than not offering it. + """ + import json + import subprocess + import sys + + monkeypatch.setenv("DRYDOCK_CATALOG", str(tmp_path / "cat")) + project = tmp_path / "proj" + project.mkdir() + (project / "m.py").write_text("def f():\n return 1\n") + + subprocess.run([sys.executable, "-m", "drydock.cli", "catalog", "ingest", + str(project)], capture_output=True, timeout=180) + + for sub in (["list"], ["stats"], ["components"], ["search", "f"]): + name = " ".join(sub) + + plain = subprocess.run([sys.executable, "-m", "drydock.cli", "catalog", *sub], + capture_output=True, text=True, timeout=180) + assert plain.returncode == 0, f"{name}: {plain.stderr}" + json.loads(plain.stdout) # JSON by default, like every other command + + target = tmp_path / f"{sub[0]}.json" + piped = subprocess.run( + [sys.executable, "-m", "drydock.cli", "catalog", *sub, "-o", str(target)], + capture_output=True, text=True, timeout=180) + assert piped.returncode == 0, f"{name}: {piped.stderr}" + assert target.exists() and target.stat().st_size > 0, ( + f"{name}: -o was accepted but wrote nothing" + ) + assert not piped.stdout.strip(), ( + f"{name}: payload went to stdout as well as the file" + ) + json.loads(target.read_text()) + + rendered = subprocess.run( + [sys.executable, "-m", "drydock.cli", "catalog", *sub, "--markdown"], + capture_output=True, text=True, timeout=180) + assert rendered.returncode == 0, f"{name}: {rendered.stderr}" + try: + json.loads(rendered.stdout) + except json.JSONDecodeError: + pass # correct: markdown is not JSON + else: + raise AssertionError(f"{name}: --markdown still produced JSON")