From d6ab294569ccdd7fc504dd8ec403095e2aa54225 Mon Sep 17 00:00:00 2001 From: Rohit Behera <126186063+r0h1tb@users.noreply.github.com> Date: Tue, 25 Aug 2026 02:07:03 +0530 Subject: [PATCH] fix(cli): keep stdout parseable in JSON mode JSON is the default output for `query`, `goto` and `callers` -- `--humanize` is the opt-out -- but every failure path bypassed the formatter and printed a Rich line to stdout instead. `json.loads(subprocess.check_output(...))` worked while the symbol existed and raised JSONDecodeError the moment it did not. `_warn_if_ambiguous` broke it from the other side: its docstring says the note is "for JSON consumers", but it printed the note to stdout ahead of the document, wrapped mid-sentence by Rich. Diagnostics now go to a stderr console, empty results go through the formatter so stdout is still a document, and the ambiguity note is carried in the payload as `ambiguous` rather than only as a coloured line. Exit codes are unchanged: 0 and 1 still separate the cases. --- ast_rag/cli.py | 30 ++-- ast_rag/utils/output.py | 56 +++++-- tests/test_json_output_is_parseable.py | 197 +++++++++++++++++++++++++ 3 files changed, 264 insertions(+), 19 deletions(-) create mode 100644 tests/test_json_output_is_parseable.py diff --git a/ast_rag/cli.py b/ast_rag/cli.py index 6089b2e..b53aa70 100644 --- a/ast_rag/cli.py +++ b/ast_rag/cli.py @@ -52,7 +52,7 @@ ) from ast_rag.services.embedding_manager import EmbeddingManager from ast_rag.api import ASTRagAPI -from ast_rag.utils.output import get_formatter +from ast_rag.utils.output import err_console, get_formatter from ast_rag.services.summarizer_service import SummarizerService app = typer.Typer( @@ -203,10 +203,16 @@ def _describe_ambiguity(name: str, defs: list) -> Optional[str]: def _warn_if_ambiguous(name: str, defs: list) -> Optional[str]: - """Print the ambiguity note (if any) and return it for JSON consumers.""" + """Print the ambiguity note (if any) and return it for JSON consumers. + + The note goes to stderr: stdout is a JSON document by default, and a + Rich-wrapped line printed into the middle of it is not parseable. + Callers that use a formatter pass the return value on so the note + survives inside the document too. + """ note = _describe_ambiguity(name, defs) if note: - console.print(f"[yellow]{note}[/yellow]") + err_console.print(f"[yellow]{note}[/yellow]") return note @@ -539,7 +545,8 @@ def query( ) if not results: - console.print("[yellow]No results found.[/yellow]") + err_console.print("[yellow]No results found.[/yellow]") + formatter.format_search_results([], text) raise typer.Exit(0) formatter.format_search_results(results, text) @@ -567,7 +574,8 @@ def goto( nodes = api.find_definition(qualified_name, kind=kind, lang=lang) if not nodes: - console.print(f"[yellow]Definition not found for: {qualified_name}[/yellow]") + err_console.print(f"[yellow]Definition not found for: {qualified_name}[/yellow]") + formatter.format_definitions([], api=api, snippet=snippet) raise typer.Exit(1) formatter.format_definitions(nodes, api=api, snippet=snippet) @@ -593,21 +601,23 @@ def callers( defs = api.find_definition(qualified_name, lang=lang) if not defs: - console.print(f"[yellow]Symbol not found: {qualified_name}[/yellow]") + err_console.print(f"[yellow]Symbol not found: {qualified_name}[/yellow]") + formatter.format_callers(qualified_name, []) raise typer.Exit(1) - _warn_if_ambiguous(qualified_name, defs) + note = _warn_if_ambiguous(qualified_name, defs) target = defs[0] if humanize: - console.print(f"Finding callers of [bold]{target.qualified_name}[/bold]...") + err_console.print(f"Finding callers of [bold]{target.qualified_name}[/bold]...") caller_nodes = api.find_callers(target.id, max_depth=depth) if not caller_nodes: - console.print("[yellow]No callers found.[/yellow]") + err_console.print("[yellow]No callers found.[/yellow]") + formatter.format_callers(target.qualified_name, [], note=note) raise typer.Exit(0) - formatter.format_callers(target.qualified_name, caller_nodes) + formatter.format_callers(target.qualified_name, caller_nodes, note=note) # --------------------------------------------------------------------------- diff --git a/ast_rag/utils/output.py b/ast_rag/utils/output.py index cfa7273..d6e3145 100644 --- a/ast_rag/utils/output.py +++ b/ast_rag/utils/output.py @@ -4,6 +4,11 @@ Abstraction layer for formatting CLI output. Supports: - JSON (AI-friendly, default) - Human-readable (Rich tables, with --humanize flag) + +JSON is the default, so a formatter must emit exactly one JSON document on +stdout and nothing else -- including for empty results. Diagnostics belong on +stderr (``err_console``), and anything the caller must know to interpret the +result, such as an ambiguity note, is carried inside the document. """ from __future__ import annotations @@ -21,24 +26,35 @@ console = Console() +# Diagnostics go here so they never land in the middle of a JSON document. +err_console = Console(stderr=True) + class OutputFormatter(ABC): """Abstract base class for output formatters.""" @abstractmethod - def format_search_results(self, results: list[SearchResult], query: str) -> None: + def format_search_results( + self, results: list[SearchResult], query: str, note: Optional[str] = None + ) -> None: """Format semantic search results.""" pass @abstractmethod def format_definitions( - self, nodes: list[ASTNode], api: Optional[Any] = None, snippet: bool = False + self, + nodes: list[ASTNode], + api: Optional[Any] = None, + snippet: bool = False, + note: Optional[str] = None, ) -> None: """Format definition lookup results.""" pass @abstractmethod - def format_callers(self, qualified_name: str, nodes: list[ASTNode]) -> None: + def format_callers( + self, qualified_name: str, nodes: list[ASTNode], note: Optional[str] = None + ) -> None: """Format callers results.""" pass @@ -46,7 +62,9 @@ def format_callers(self, qualified_name: str, nodes: list[ASTNode]) -> None: class JSONFormatter(OutputFormatter): """AI-friendly JSON output (default).""" - def format_search_results(self, results: list[SearchResult], query: str) -> None: + def format_search_results( + self, results: list[SearchResult], query: str, note: Optional[str] = None + ) -> None: output = { "query": query, "count": len(results), @@ -62,10 +80,16 @@ def format_search_results(self, results: list[SearchResult], query: str) -> None for r in results ], } + if note: + output["ambiguous"] = note print(json.dumps(output, indent=2)) def format_definitions( - self, nodes: list[ASTNode], api: Optional[Any] = None, snippet: bool = False + self, + nodes: list[ASTNode], + api: Optional[Any] = None, + snippet: bool = False, + note: Optional[str] = None, ) -> None: output = { "count": len(nodes), @@ -82,9 +106,13 @@ def format_definitions( for n in nodes ], } + if note: + output["ambiguous"] = note print(json.dumps(output, indent=2)) - def format_callers(self, qualified_name: str, nodes: list[ASTNode]) -> None: + def format_callers( + self, qualified_name: str, nodes: list[ASTNode], note: Optional[str] = None + ) -> None: output = { "target": qualified_name, "count": len(nodes), @@ -99,6 +127,8 @@ def format_callers(self, qualified_name: str, nodes: list[ASTNode]) -> None: for n in nodes ], } + if note: + output["ambiguous"] = note print(json.dumps(output, indent=2)) @@ -120,7 +150,9 @@ def _colorize_kind(self, kind: str) -> str: return kind return f"[{color}]{kind}[/{color}]" - def format_search_results(self, results: list[SearchResult], query: str) -> None: + def format_search_results( + self, results: list[SearchResult], query: str, note: Optional[str] = None + ) -> None: table = Table( "Score", "Kind", @@ -143,7 +175,11 @@ def format_search_results(self, results: list[SearchResult], query: str) -> None console.print(table) def format_definitions( - self, nodes: list[ASTNode], api: Optional[Any] = None, snippet: bool = False + self, + nodes: list[ASTNode], + api: Optional[Any] = None, + snippet: bool = False, + note: Optional[str] = None, ) -> None: for node in nodes: console.print( @@ -174,7 +210,9 @@ def format_definitions( ) ) - def format_callers(self, qualified_name: str, nodes: list[ASTNode]) -> None: + def format_callers( + self, qualified_name: str, nodes: list[ASTNode], note: Optional[str] = None + ) -> None: table = Table( "Kind", "Lang", diff --git a/tests/test_json_output_is_parseable.py b/tests/test_json_output_is_parseable.py new file mode 100644 index 0000000..1ca1163 --- /dev/null +++ b/tests/test_json_output_is_parseable.py @@ -0,0 +1,197 @@ +"""JSON is the default CLI output, so stdout must always be one JSON document. + +``get_formatter(humanize=False)`` returns ``JSONFormatter``, so ``query``, +``goto`` and ``callers`` are machine-readable by default -- the point of a +tool whose README opens with "for AI agents". + +All three abandon that contract on their failure paths: they ``console.print`` +a Rich-coloured line to stdout and exit before the formatter is reached. So + + json.loads(subprocess.check_output(["ast-rag", "goto", name])) + +works while the symbol exists and raises ``JSONDecodeError`` the moment it +does not. ``_warn_if_ambiguous`` breaks it from the other side -- its +docstring says the note is returned "for JSON consumers", but it prints the +note as well, so an ambiguous name emits a bare line *before* the document. + +Exit codes are not the problem and are left alone: 0 and 1 still separate the +cases. What has to hold is that stdout parses, and that anything the user +needs to know survives into the JSON rather than being lost with the +Rich line. +""" + +from __future__ import annotations + +import json +from unittest.mock import MagicMock + +import pytest +from typer.testing import CliRunner + +from ast_rag import cli +from ast_rag.cli import app +from ast_rag.dto import ASTNode, SearchResult +from ast_rag.dto.enums import Language, NodeKind + +runner = CliRunner() + + +def _node(qualified_name: str, *, name: str | None = None, line: int = 1) -> ASTNode: + return ASTNode( + kind=NodeKind.FUNCTION, + name=name or qualified_name.rsplit(".", 1)[-1], + qualified_name=qualified_name, + lang=Language.PYTHON, + file_path=f"src/{qualified_name.split('.')[0]}.py", + start_line=line, + end_line=line + 5, + start_byte=0, + end_byte=100, + ) + + +@pytest.fixture +def api(monkeypatch): + """Replace config loading and the API so no service is needed.""" + fake = MagicMock() + monkeypatch.setattr(cli, "_load_config", lambda *_a, **_k: MagicMock()) + monkeypatch.setattr(cli, "_build_api", lambda *_a, **_k: fake) + return fake + + +def _stdout_json(result, argv: list[str]) -> dict: + """Parse stdout, failing with the offending text rather than a traceback.""" + assert result.stdout.strip(), ( + f"`ast-rag {' '.join(argv)}` wrote nothing to stdout; " + f"an agent parsing it gets an empty string (exit {result.exit_code})" + ) + try: + return json.loads(result.stdout) + except json.JSONDecodeError as exc: + pytest.fail( + f"`ast-rag {' '.join(argv)}` is in JSON mode but stdout does not parse " + f"({exc}):\n{result.stdout!r}" + ) + + +def _run(argv: list[str]): + return runner.invoke(app, argv) + + +# -------------------------------------------------------------------------- +# Empty results must still be a JSON document, not a coloured line +# -------------------------------------------------------------------------- + + +def test_goto_not_found_still_emits_json(api): + api.find_definition.return_value = [] + argv = ["goto", "NoSuchSymbol"] + + payload = _stdout_json(_run(argv), argv) + + assert payload["count"] == 0 + assert payload["definitions"] == [] + + +def test_query_no_results_still_emits_json(api): + api.search_semantic.return_value = [] + argv = ["query", "nothing matches this"] + + payload = _stdout_json(_run(argv), argv) + + assert payload["count"] == 0 + assert payload["results"] == [] + + +def test_callers_symbol_not_found_still_emits_json(api): + api.find_definition.return_value = [] + argv = ["callers", "NoSuchSymbol"] + + payload = _stdout_json(_run(argv), argv) + + assert payload["count"] == 0 + assert payload["callers"] == [] + + +def test_callers_with_no_callers_still_emits_json(api): + api.find_definition.return_value = [_node("pkg.orphan")] + api.find_callers.return_value = [] + argv = ["callers", "pkg.orphan"] + + payload = _stdout_json(_run(argv), argv) + + assert payload["count"] == 0 + assert payload["callers"] == [] + + +# -------------------------------------------------------------------------- +# The ambiguity note must travel inside the document, not before it +# -------------------------------------------------------------------------- + + +def test_ambiguous_name_keeps_stdout_parseable(api): + api.find_definition.return_value = [_node("a.main"), _node("b.main")] + api.find_callers.return_value = [_node("c.caller")] + argv = ["callers", "main"] + + payload = _stdout_json(_run(argv), argv) + + assert payload["count"] == 1 + + +def test_ambiguity_note_survives_into_the_json(api): + """#64's warning is useless to an agent if it only exists as a Rich line.""" + api.find_definition.return_value = [_node("a.main"), _node("b.main")] + api.find_callers.return_value = [_node("c.caller")] + argv = ["callers", "main"] + + payload = _stdout_json(_run(argv), argv) + + note = payload.get("ambiguous") + assert note, f"ambiguity was not reported in the JSON payload: {payload}" + assert "a.main" in note, note + + +# -------------------------------------------------------------------------- +# Guards: the working paths and human mode must not regress +# -------------------------------------------------------------------------- + + +def test_goto_found_emits_json(api): + api.find_definition.return_value = [_node("pkg.handler", line=42)] + argv = ["goto", "pkg.handler"] + + payload = _stdout_json(_run(argv), argv) + + assert payload["count"] == 1 + assert payload["definitions"][0]["qualified_name"] == "pkg.handler" + assert payload["definitions"][0]["start_line"] == 42 + + +def test_query_with_results_emits_json(api): + api.search_semantic.return_value = [SearchResult(node=_node("pkg.cache"), score=0.91)] + argv = ["query", "cache with expiration"] + + payload = _stdout_json(_run(argv), argv) + + assert payload["count"] == 1 + assert payload["results"][0]["qualified_name"] == "pkg.cache" + + +def test_humanize_still_produces_text_not_json(api): + """--humanize is the opt-out; it must keep producing Rich output.""" + api.find_definition.return_value = [_node("pkg.handler")] + result = _run(["goto", "--humanize", "pkg.handler"]) + + with pytest.raises(json.JSONDecodeError): + json.loads(result.stdout) + assert "pkg.handler" in result.stdout + + +def test_humanize_empty_result_is_reported_to_the_user(api): + """The human path must not go silent when the JSON path stops printing.""" + api.find_definition.return_value = [] + result = _run(["goto", "--humanize", "NoSuchSymbol"]) + + combined = result.stdout + result.stderr + assert "NoSuchSymbol" in combined, f"nothing told the user the lookup failed:\n{combined!r}"