From 6faeff50e6ef8f0d46d7561d2630bce36be8e2d6 Mon Sep 17 00:00:00 2001 From: Greg Holmes Date: Tue, 18 Aug 2026 15:33:00 +0100 Subject: [PATCH 1/4] fix(keys): honor -o json so stdout stays parseable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finishes #98. #97 fixed the -o json pollution across requests, read, models, projects, members, usage and billing, but `keys` was not in that sweep, so it was left as the last command whose stdout still broke pipes: $ dg -o json keys | jq -r '.keys[0].key_id' jq: parse error: Invalid numeric literal at line 1, column 9 "Fetching API keys..." and the Rich table were printed to stdout ahead of the JSON the framework serialises from the result. Applies the same pattern the other seven commands now use: - `status_console = Console(stderr=True)` for progress, errors, the empty-state notice and the delete confirmation — chrome never touches stdout - human rendering (list table, created-key details, key details, and both dry-run summaries) is gated on `get_output_format() == "default"` The created key's secret is unaffected: it already travels in `KeysResult.created_key.key`, so json/yaml/csv callers still receive it — only the human-facing echo is suppressed. One known limitation, unchanged in spirit from #97: in json mode the dry-run paths report status and message but not the would-be scopes/ttl/tags, since those are not modelled on the result. Worth a follow-up if callers need them. Verified live: all eight commands now emit parseable JSON on stdout (`-o yaml`/`-o csv` valid too), chrome still visible on stderr, and human output matches the already-merged `projects` behaviour under identical conditions. 1067 unit tests pass, including 4 new gating tests for keys. --- .../src/deepctl_cmd_keys/command.py | 88 +++++++------ .../tests/unit/test_keys_command.py | 117 ++++++++++++++++-- 2 files changed, 157 insertions(+), 48 deletions(-) diff --git a/packages/deepctl-cmd-keys/src/deepctl_cmd_keys/command.py b/packages/deepctl-cmd-keys/src/deepctl_cmd_keys/command.py index fee0e50..aeae024 100644 --- a/packages/deepctl-cmd-keys/src/deepctl_cmd_keys/command.py +++ b/packages/deepctl-cmd-keys/src/deepctl_cmd_keys/command.py @@ -10,6 +10,7 @@ BaseResult, Config, DeepgramClient, + get_output_format, ) from rich.console import Console from rich.table import Table @@ -17,6 +18,9 @@ from .models import KeyCreatedInfo, KeyInfo, KeysResult console = Console() +# Status/progress chrome must never touch stdout, or it corrupts JSON/CSV +# output that callers pipe into jq and friends. +status_console = Console(stderr=True) class KeysCommand(BaseCommand): @@ -159,7 +163,7 @@ def handle( return self._list_keys(client, project_id, kwargs.get("status")) except Exception as e: - console.print(f"[red]Error:[/red] {e}") + status_console.print(f"[red]Error:[/red] {e}") return BaseResult(status="error", message=str(e)) def _list_keys( @@ -168,12 +172,12 @@ def _list_keys( project_id: str | None, status: str | None, ) -> BaseResult: - console.print("[blue]Fetching API keys...[/blue]") + status_console.print("[blue]Fetching API keys...[/blue]") result = client.list_keys(project_id=project_id, status=status) keys_raw = result.get("api_keys", result.get("keys", [])) if not keys_raw: - console.print("[yellow]No API keys found[/yellow]") + status_console.print("[yellow]No API keys found[/yellow]") return KeysResult(status="info", message="No keys found") key_models: list[KeyInfo] = [] @@ -208,8 +212,12 @@ def _list_keys( info.expiration_date[:10] if info.expiration_date else "never", ) - console.print(table) - console.print(f"\n[dim]{len(key_models)} key(s) found[/dim]") + # Human table only in default mode; json/yaml/csv are emitted by the + # framework from the returned result, so printing here would prepend + # non-parseable text to that output. + if get_output_format() == "default": + console.print(table) + console.print(f"\n[dim]{len(key_models)} key(s) found[/dim]") return KeysResult(status="success", keys=key_models, count=len(key_models)) @@ -231,18 +239,19 @@ def _create_key( tags = [t.strip() for t in tags_str.split(",")] if tags_str else None if dry_run: - console.print("[yellow]Dry run — no changes made[/yellow]") - console.print(f" Would create key: comment='{comment or '(none)'}'") - console.print(f" Scopes: {', '.join(scopes)}") - if expiration: - console.print(f" Expires: {expiration}") - if ttl: - console.print(f" TTL: {ttl}s") - if tags: - console.print(f" Tags: {', '.join(tags)}") + if get_output_format() == "default": + console.print("[yellow]Dry run — no changes made[/yellow]") + console.print(f" Would create key: comment='{comment or '(none)'}'") + console.print(f" Scopes: {', '.join(scopes)}") + if expiration: + console.print(f" Expires: {expiration}") + if ttl: + console.print(f" TTL: {ttl}s") + if tags: + console.print(f" Tags: {', '.join(tags)}") return BaseResult(status="dry_run", message="Dry run: key would be created") - console.print("[blue]Creating API key...[/blue]") + status_console.print("[blue]Creating API key...[/blue]") result = client.create_key( project_id=project_id, @@ -256,13 +265,18 @@ def _create_key( key_id = result.get("api_key_id", "") key_value = result.get("key", "") - console.print("[green]API key created successfully[/green]") - console.print(f" Key ID: {key_id}") - console.print(f" Comment: {comment or '(none)'}") - console.print(f" Scopes: {', '.join(scopes)}") - if key_value: - console.print(f"\n [bold yellow]Key: {key_value}[/bold yellow]") - console.print("[dim] Save this key now — it won't be shown again.[/dim]") + # The created key (secret included) is carried in the result for + # json/yaml/csv, so only render it for humans in default mode. + if get_output_format() == "default": + console.print("[green]API key created successfully[/green]") + console.print(f" Key ID: {key_id}") + console.print(f" Comment: {comment or '(none)'}") + console.print(f" Scopes: {', '.join(scopes)}") + if key_value: + console.print(f"\n [bold yellow]Key: {key_value}[/bold yellow]") + console.print( + "[dim] Save this key now — it won't be shown again.[/dim]" + ) created = KeyCreatedInfo( key_id=key_id, @@ -278,7 +292,7 @@ def _show_key( key_id: str, project_id: str | None, ) -> BaseResult: - console.print(f"[blue]Fetching key details:[/blue] {key_id}") + status_console.print(f"[blue]Fetching key details:[/blue] {key_id}") result = client.get_key(key_id, project_id=project_id) key_data = result.get("api_key", result) if isinstance(result, dict) else result @@ -292,14 +306,17 @@ def _show_key( tags=key_data.get("tags", []), ) - console.print("[green]Key Details:[/green]") - console.print(f" Key ID: {info.key_id}") - console.print(f" Comment: {info.comment or '(none)'}") - console.print(f" Scopes: {', '.join(info.scopes) if info.scopes else '-'}") - console.print(f" Created: {info.created or '-'}") - console.print(f" Expires: {info.expiration_date or 'never'}") - if info.tags: - console.print(f" Tags: {', '.join(info.tags)}") + if get_output_format() == "default": + console.print("[green]Key Details:[/green]") + console.print(f" Key ID: {info.key_id}") + console.print(f" Comment: {info.comment or '(none)'}") + console.print( + f" Scopes: {', '.join(info.scopes) if info.scopes else '-'}" + ) + console.print(f" Created: {info.created or '-'}") + console.print(f" Expires: {info.expiration_date or 'never'}") + if info.tags: + console.print(f" Tags: {', '.join(info.tags)}") return KeysResult(status="success", keys=[info], count=1) @@ -312,8 +329,9 @@ def _delete_key( dry_run: bool = False, ) -> BaseResult: if dry_run: - console.print("[yellow]Dry run — no changes made[/yellow]") - console.print(f" Would delete key: {key_id}") + if get_output_format() == "default": + console.print("[yellow]Dry run — no changes made[/yellow]") + console.print(f" Would delete key: {key_id}") return BaseResult( status="dry_run", message=f"Dry run: key {key_id} would be deleted" ) @@ -323,7 +341,7 @@ def _delete_key( ): return BaseResult(status="cancelled", message="Cancelled by user") - console.print(f"[blue]Deleting API key:[/blue] {key_id}") + status_console.print(f"[blue]Deleting API key:[/blue] {key_id}") client.delete_key(key_id, project_id=project_id) - console.print(f"[green]API key {key_id} deleted[/green]") + status_console.print(f"[green]API key {key_id} deleted[/green]") return BaseResult(status="success", message=f"Key {key_id} deleted") diff --git a/packages/deepctl-cmd-keys/tests/unit/test_keys_command.py b/packages/deepctl-cmd-keys/tests/unit/test_keys_command.py index 437f131..c71bee2 100644 --- a/packages/deepctl-cmd-keys/tests/unit/test_keys_command.py +++ b/packages/deepctl-cmd-keys/tests/unit/test_keys_command.py @@ -1,6 +1,6 @@ """Tests for keys command.""" -from unittest.mock import Mock +from unittest.mock import Mock, patch import pytest from deepctl_cmd_keys.command import KeysCommand @@ -99,9 +99,7 @@ def test_handle_list_keys( assert result.keys[0].key_id == "key1" assert result.keys[0].comment == "test" assert result.keys[0].scopes == ["member"] - mock_client.list_keys.assert_called_once_with( - project_id=None, status=None - ) + mock_client.list_keys.assert_called_once_with(project_id=None, status=None) def test_handle_list_keys_empty( self, command, mock_config, mock_auth_manager, mock_client @@ -184,9 +182,7 @@ def test_handle_show_key( assert result.keys[0].comment == "production key" assert result.keys[0].scopes == ["admin"] assert result.keys[0].tags == ["prod"] - mock_client.get_key.assert_called_once_with( - "key-id", project_id=None - ) + mock_client.get_key.assert_called_once_with("key-id", project_id=None) def test_handle_delete_key( self, command, mock_config, mock_auth_manager, mock_client @@ -204,13 +200,9 @@ def test_handle_delete_key( assert result.status == "success" assert "key-id" in result.message assert "deleted" in result.message.lower() - mock_client.delete_key.assert_called_once_with( - "key-id", project_id=None - ) + mock_client.delete_key.assert_called_once_with("key-id", project_id=None) - def test_handle_error( - self, command, mock_config, mock_auth_manager, mock_client - ): + def test_handle_error(self, command, mock_config, mock_auth_manager, mock_client): """Test client raises exception, returns error.""" mock_client.list_keys.side_effect = Exception("API connection failed") @@ -224,6 +216,105 @@ def test_handle_error( assert "API connection failed" in result.message +class TestKeysOutputFormat: + """`-o json` must leave stdout parseable (#98). + + `keys` was the one account command missed when the other seven were fixed, + so `dg -o json keys | jq` still failed on "Fetching API keys..." arriving + ahead of the JSON. + """ + + @pytest.fixture + def command(self): + return KeysCommand() + + @staticmethod + def _keys_response(): + return { + "api_keys": [ + { + "api_key": { + "api_key_id": "key1", + "comment": "ci-runner", + "scopes": ["member"], + "created": "2026-01-01", + "expiration_date": "", + } + } + ] + } + + @patch("deepctl_cmd_keys.command.get_output_format", return_value="default") + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.console") + def test_default_mode_table_on_stdout_chrome_on_stderr( + self, mock_console, mock_status_console, _fmt, command + ): + client = Mock(spec=DeepgramClient) + client.list_keys.return_value = self._keys_response() + + command._list_keys(client, None, None) + + stderr_text = " ".join( + str(c.args[0]) for c in mock_status_console.print.call_args_list if c.args + ) + # The table rides stdout for humans... + mock_console.print.assert_called() + # ...while progress chrome only ever goes to stderr. + assert "Fetching API keys" in stderr_text + + @patch("deepctl_cmd_keys.command.get_output_format", return_value="json") + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.console") + def test_json_mode_renders_no_human_output( + self, mock_console, mock_status_console, _fmt, command + ): + client = Mock(spec=DeepgramClient) + client.list_keys.return_value = self._keys_response() + + result = command._list_keys(client, None, None) + + # Nothing human-facing on stdout — the framework serialises the result. + mock_console.print.assert_not_called() + assert result.count == 1 + assert result.keys[0].key_id == "key1" + + @patch("deepctl_cmd_keys.command.get_output_format", return_value="json") + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.console") + def test_json_mode_created_key_only_in_result( + self, mock_console, mock_status_console, _fmt, command + ): + """The secret must reach the caller via the result, not stray prints.""" + client = Mock(spec=DeepgramClient) + client.create_key.return_value = { + "api_key_id": "new-id", + "key": "secret-value", + } + + result = command._create_key(client, None, comment="ci", scopes="member") + + mock_console.print.assert_not_called() + assert result.created_key.key == "secret-value" + assert result.created_key.key_id == "new-id" + + @patch("deepctl_cmd_keys.command.get_output_format", return_value="json") + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.console") + def test_json_mode_show_key_renders_nothing( + self, mock_console, mock_status_console, _fmt, command + ): + client = Mock(spec=DeepgramClient) + client.get_key.return_value = { + "api_key": {"api_key_id": "key1", "comment": "c", "scopes": ["member"]} + } + + result = command._show_key(client, "key1", None) + + mock_console.print.assert_not_called() + assert result.keys[0].key_id == "key1" + + class TestKeysModels: """Test cases for keys models.""" From 61381ae6a4ef3f869c62e6faf5b004bdb82a852b Mon Sep 17 00:00:00 2001 From: Greg Holmes Date: Tue, 18 Aug 2026 16:13:09 +0100 Subject: [PATCH 2/4] fix(core): exit non-zero on failure and stop corrupting yaml/csv payloads Review follow-ups on #101. Three defects that made "-o json keeps stdout parseable" true only in the narrow case, plus the shared status console the per-command copies were standing in for. Exit codes. main.py caught SystemExit and dropped the code on the floor, so every command exited 0 no matter what it reported -- including the auth-guard failure that already raised SystemExit(1). A caller writing `if dg -o json keys; then` took the success branch on a failed call, which defeats the point of a parseable stdout. main.py now carries the code through the post-run notifications and re-raises it, and BaseCommand maps a result status to an exit code once, in one table: error -> 1, cancelled -> 130, everything else -> 0. Payload integrity. _output_yaml and _output_csv printed through Rich, which treats square brackets as style markup and deletes them: an API key comment of "[ci] runner" came out as "runner", silently, with no error. Rich also hard-wrapped at the console width, injecting newlines into the middle of a csv field. Both now go through _write_payload with markup, highlighting and wrapping off. Still routed via `console` so --quiet keeps working. The JSON path was already safe -- print_json escapes rather than interprets -- and is left alone. The unknown-format complaint moved to stderr, so it cannot corrupt the JSON we fall back to. get_status_console() gives core one definition of the stderr chrome console and the seven commands fixed in #97 now use it. A per-command Console(stderr=True) silently missed core's agentic no-color settings, and a new command reaching for a bare Console() is exactly how keys regressed in the first place. --- .../src/deepctl_cmd_billing/command.py | 3 +- .../src/deepctl_cmd_members/command.py | 3 +- .../src/deepctl_cmd_models/command.py | 3 +- .../src/deepctl_cmd_projects/command.py | 3 +- .../src/deepctl_cmd_read/command.py | 3 +- .../src/deepctl_cmd_requests/command.py | 3 +- .../src/deepctl_cmd_usage/command.py | 3 +- .../deepctl-core/src/deepctl_core/__init__.py | 2 + .../src/deepctl_core/base_command.py | 54 +++++- .../deepctl-core/src/deepctl_core/output.py | 16 ++ packages/deepctl-core/tests/unit/test_base.py | 165 +++++++++++++++++- src/deepctl/main.py | 15 +- 12 files changed, 253 insertions(+), 20 deletions(-) diff --git a/packages/deepctl-cmd-billing/src/deepctl_cmd_billing/command.py b/packages/deepctl-cmd-billing/src/deepctl_cmd_billing/command.py index 080ede3..c0af07e 100644 --- a/packages/deepctl-cmd-billing/src/deepctl_cmd_billing/command.py +++ b/packages/deepctl-cmd-billing/src/deepctl_cmd_billing/command.py @@ -11,6 +11,7 @@ Config, DeepgramClient, get_output_format, + get_status_console, ) from rich.console import Console from rich.table import Table @@ -20,7 +21,7 @@ console = Console() # Status/progress chrome must never touch stdout, or it corrupts JSON/CSV # output that callers pipe into jq and friends. -status_console = Console(stderr=True) +status_console = get_status_console() class BillingCommand(BaseCommand): diff --git a/packages/deepctl-cmd-members/src/deepctl_cmd_members/command.py b/packages/deepctl-cmd-members/src/deepctl_cmd_members/command.py index 88cc12f..cef8aaf 100644 --- a/packages/deepctl-cmd-members/src/deepctl_cmd_members/command.py +++ b/packages/deepctl-cmd-members/src/deepctl_cmd_members/command.py @@ -11,6 +11,7 @@ Config, DeepgramClient, get_output_format, + get_status_console, ) from rich.console import Console from rich.table import Table @@ -20,7 +21,7 @@ console = Console() # Status/progress chrome must never touch stdout, or it corrupts JSON/CSV # output that callers pipe into jq and friends. -status_console = Console(stderr=True) +status_console = get_status_console() class MembersCommand(BaseCommand): diff --git a/packages/deepctl-cmd-models/src/deepctl_cmd_models/command.py b/packages/deepctl-cmd-models/src/deepctl_cmd_models/command.py index c128b2b..610cbff 100644 --- a/packages/deepctl-cmd-models/src/deepctl_cmd_models/command.py +++ b/packages/deepctl-cmd-models/src/deepctl_cmd_models/command.py @@ -11,6 +11,7 @@ Config, DeepgramClient, get_output_format, + get_status_console, ) from rich.console import Console from rich.table import Table @@ -20,7 +21,7 @@ console = Console() # Status/progress chrome must never touch stdout, or it corrupts JSON/CSV # output that callers pipe into jq and friends. -status_console = Console(stderr=True) +status_console = get_status_console() class ModelsCommand(BaseCommand): diff --git a/packages/deepctl-cmd-projects/src/deepctl_cmd_projects/command.py b/packages/deepctl-cmd-projects/src/deepctl_cmd_projects/command.py index fbdcdf5..780cf62 100644 --- a/packages/deepctl-cmd-projects/src/deepctl_cmd_projects/command.py +++ b/packages/deepctl-cmd-projects/src/deepctl_cmd_projects/command.py @@ -9,6 +9,7 @@ Config, DeepgramClient, get_output_format, + get_status_console, ) from rich.console import Console @@ -17,7 +18,7 @@ console = Console() # Status/progress chrome must never touch stdout, or it corrupts JSON/CSV # output that callers pipe into jq and friends. -status_console = Console(stderr=True) +status_console = get_status_console() class ProjectsCommand(BaseCommand): diff --git a/packages/deepctl-cmd-read/src/deepctl_cmd_read/command.py b/packages/deepctl-cmd-read/src/deepctl_cmd_read/command.py index 613cf28..a51bcb9 100644 --- a/packages/deepctl-cmd-read/src/deepctl_cmd_read/command.py +++ b/packages/deepctl-cmd-read/src/deepctl_cmd_read/command.py @@ -13,6 +13,7 @@ Config, DeepgramClient, get_output_format, + get_status_console, ) from rich.console import Console @@ -21,7 +22,7 @@ console = Console() # Status/progress chrome must never touch stdout, or it corrupts JSON/CSV # output that callers pipe into jq and friends. -status_console = Console(stderr=True) +status_console = get_status_console() class ReadCommand(BaseCommand): diff --git a/packages/deepctl-cmd-requests/src/deepctl_cmd_requests/command.py b/packages/deepctl-cmd-requests/src/deepctl_cmd_requests/command.py index 51c47cc..d3ac77a 100644 --- a/packages/deepctl-cmd-requests/src/deepctl_cmd_requests/command.py +++ b/packages/deepctl-cmd-requests/src/deepctl_cmd_requests/command.py @@ -12,6 +12,7 @@ Config, DeepgramClient, get_output_format, + get_status_console, ) from rich.console import Console from rich.table import Table @@ -21,7 +22,7 @@ console = Console() # Status/progress chrome must never touch stdout, or it corrupts JSON/CSV # output that callers pipe into jq and friends. -status_console = Console(stderr=True) +status_console = get_status_console() class RequestsCommand(BaseCommand): diff --git a/packages/deepctl-cmd-usage/src/deepctl_cmd_usage/command.py b/packages/deepctl-cmd-usage/src/deepctl_cmd_usage/command.py index caf72df..26f2e7b 100644 --- a/packages/deepctl-cmd-usage/src/deepctl_cmd_usage/command.py +++ b/packages/deepctl-cmd-usage/src/deepctl_cmd_usage/command.py @@ -10,6 +10,7 @@ Config, DeepgramClient, get_output_format, + get_status_console, ) from deepctl_shared_utils import validate_date_format from rich.console import Console @@ -19,7 +20,7 @@ console = Console() # Status/progress chrome must never touch stdout, or it corrupts JSON/CSV # output that callers pipe into jq and friends. -status_console = Console(stderr=True) +status_console = get_status_console() class UsageCommand(BaseCommand): diff --git a/packages/deepctl-core/src/deepctl_core/__init__.py b/packages/deepctl-core/src/deepctl_core/__init__.py index dc7663a..6a6f86d 100644 --- a/packages/deepctl-core/src/deepctl_core/__init__.py +++ b/packages/deepctl-core/src/deepctl_core/__init__.py @@ -16,6 +16,7 @@ OutputFormatter, get_console, get_output_format, + get_status_console, is_agentic, print_error, print_info, @@ -51,6 +52,7 @@ "enable_timing", "get_console", "get_output_format", + "get_status_console", "get_timing_summary", "is_agentic", "is_timing_enabled", diff --git a/packages/deepctl-core/src/deepctl_core/base_command.py b/packages/deepctl-core/src/deepctl_core/base_command.py index 1ea6ebb..24def7c 100644 --- a/packages/deepctl-core/src/deepctl_core/base_command.py +++ b/packages/deepctl-core/src/deepctl_core/base_command.py @@ -29,6 +29,15 @@ class BaseCommand(ABC): requires_project: bool = False ci_friendly: bool = True + # Result status -> process exit code. A command that reports failure must + # not exit 0: scripts and CI branch on the exit code, not on the "status" + # field inside the payload. Any status not listed here exits 0 + # (success, succeeded, info, dry_run, warning). + EXIT_CODES: ClassVar[dict[str, int]] = { + "error": 1, + "cancelled": 130, + } + # Agent-oriented metadata examples: ClassVar[list[str]] = [] agent_help: str = "" @@ -147,6 +156,23 @@ def execute(self, ctx: click.Context, **kwargs: Any) -> None: stderr_console.print_exception() raise click.ClickException(str(e)) + # Payload is already on stdout; now make the exit code agree with it. + exit_code = self.exit_code_for(result) + if exit_code: + raise SystemExit(exit_code) + + def exit_code_for(self, result: Any) -> int: + """Map a command result to a process exit code. + + Args: + result: Command result (may be None) + + Returns: + Exit code for this result's status; 0 when it reported no failure + """ + status = str(getattr(result, "status", "") or "") + return self.EXIT_CODES.get(status, 0) + def is_guided(self, ctx: click.Context) -> bool: """True only when the user invoked this command with no input at all. @@ -285,9 +311,25 @@ def output_result(self, result: Any, config: Config) -> None: elif output_format == "csv": self._output_csv(result) else: - console.print(f"[red]Unknown output format:[/red] {output_format}") + # Diagnostics belong on stderr even here, or the complaint about + # the format corrupts the payload we fall back to. + stderr_console.print( + f"[red]Unknown output format:[/red] {output_format}" + ) self._output_json(result) + def _write_payload(self, text: str) -> None: + """Write already-serialised machine-readable text to stdout verbatim. + + Rich's default print would treat square brackets as style markup and + silently delete them, so a key comment of "[ci] runner" would lose the + "[ci]". It would also hard-wrap at the console width, injecting newlines + into the middle of a value. Both corrupt the payload, so markup, + highlighting and wrapping are all off. Routed through `console` rather + than `sys.stdout` so `--quiet` still suppresses output. + """ + console.print(text, markup=False, highlight=False, soft_wrap=True) + def _output_json(self, result: Any) -> None: """Output result as JSON.""" import json @@ -310,9 +352,11 @@ def _output_yaml(self, result: Any) -> None: import yaml if isinstance(result, dict | list): - console.print(yaml.dump(result, default_flow_style=False)) + self._write_payload(yaml.dump(result, default_flow_style=False)) else: - console.print(yaml.dump({"result": str(result)}, default_flow_style=False)) + self._write_payload( + yaml.dump({"result": str(result)}, default_flow_style=False) + ) def _output_table(self, result: Any) -> None: """Output result as table.""" @@ -359,7 +403,7 @@ def _output_csv(self, result: Any) -> None: dict_writer = csv.DictWriter(output, fieldnames=result[0].keys()) dict_writer.writeheader() dict_writer.writerows(result) - console.print(output.getvalue()) + self._write_payload(output.getvalue()) elif isinstance(result, dict): # Dictionary @@ -368,7 +412,7 @@ def _output_csv(self, result: Any) -> None: writer.writerow(["Key", "Value"]) for key, value in result.items(): writer.writerow([key, value]) - console.print(output.getvalue()) + self._write_payload(output.getvalue()) else: # Fallback to JSON diff --git a/packages/deepctl-core/src/deepctl_core/output.py b/packages/deepctl-core/src/deepctl_core/output.py index 685cb72..f7469db 100644 --- a/packages/deepctl-core/src/deepctl_core/output.py +++ b/packages/deepctl-core/src/deepctl_core/output.py @@ -505,3 +505,19 @@ def get_console() -> Console: Console instance """ return console + + +def get_status_console() -> Console: + """Get the console for status/progress chrome. + + Always writes to stderr, so anything printed through it can never corrupt + the machine-readable payload a command writes to stdout. Commands should + use this rather than declaring their own ``Console(stderr=True)`` — a + per-command console silently misses the agentic no-color settings, and a + command that reaches for a bare ``Console()`` reintroduces the stdout + pollution this exists to prevent. + + Returns: + Console instance writing to stderr + """ + return stderr_console diff --git a/packages/deepctl-core/tests/unit/test_base.py b/packages/deepctl-core/tests/unit/test_base.py index f2d3086..d78f802 100644 --- a/packages/deepctl-core/tests/unit/test_base.py +++ b/packages/deepctl-core/tests/unit/test_base.py @@ -6,7 +6,13 @@ import click import pytest from click.testing import CliRunner -from deepctl_core import AuthManager, BaseCommand, Config, DeepgramClient +from deepctl_core import ( + AuthManager, + BaseCommand, + BaseResult, + Config, + DeepgramClient, +) # Mirrors the full key set of deepctl_core.output._output_config so a patched # stand-in can't drift from the real global. Spread with a format override, @@ -607,7 +613,11 @@ def test_output_result_yaml(self, mock_console, mock_command_class): import yaml expected_yaml = yaml.dump(result, default_flow_style=False) - mock_console.print.assert_called_once_with(expected_yaml) + # Machine-readable payloads go out verbatim: Rich must not interpret + # markup, highlight, or wrap them. + mock_console.print.assert_called_once_with( + expected_yaml, markup=False, highlight=False, soft_wrap=True + ) @pytest.mark.unit @patch("deepctl_core.base_command.console") @@ -688,12 +698,15 @@ def test_output_result_csv_dict(self, mock_console, mock_command_class): assert "age,30" in output @pytest.mark.unit + @patch("deepctl_core.base_command.stderr_console") @patch("deepctl_core.base_command.console") @patch( "deepctl_core.output._output_config", {**_OUTPUT_CONFIG_DEFAULTS, "format": "unknown"}, ) - def test_output_result_unknown_format(self, mock_console, mock_command_class): + def test_output_result_unknown_format( + self, mock_console, mock_stderr_console, mock_command_class + ): """Test output_result with unknown format falls back to JSON.""" command = mock_command_class() config = Mock(spec=Config) @@ -703,10 +716,12 @@ def test_output_result_unknown_format(self, mock_console, mock_command_class): with patch.object(command, "_output_json") as mock_output_json: command.output_result(result, config) - # Verify error message and fallback to JSON - mock_console.print.assert_called_once_with( + # The complaint goes to stderr so it cannot corrupt the JSON we fall + # back to on stdout. + mock_stderr_console.print.assert_called_once_with( "[red]Unknown output format:[/red] unknown" ) + mock_console.print.assert_not_called() mock_output_json.assert_called_once_with(result) @pytest.mark.unit @@ -1057,6 +1072,146 @@ def test_execute_sets_guided_true_for_bare_invocation( assert mock_command_class.captured_guided is True +class TestExitCodes: + """A command that reports failure must not exit 0 (#98). + + Scripts and CI branch on the exit code, not on the "status" field buried in + the payload, so `if dg -o json keys; then` took the success branch on a + failed call. + """ + + @pytest.fixture + def command(self): + class MockCommand(BaseCommand): + name = "test" + help = "Test command" + result: Any = None + + def handle(self, *args, **kwargs): + return MockCommand.result + + return MockCommand + + @pytest.mark.unit + @pytest.mark.parametrize( + ("status", "expected"), + [ + ("error", 1), + ("cancelled", 130), + ("success", 0), + ("succeeded", 0), + ("info", 0), + ("dry_run", 0), + ("warning", 0), + ("anything-unrecognised", 0), + ], + ) + def test_exit_code_for_status(self, command, status, expected): + assert command().exit_code_for(BaseResult(status=status)) == expected + + @pytest.mark.unit + def test_exit_code_for_result_without_status(self, command): + assert command().exit_code_for(None) == 0 + assert command().exit_code_for({"not": "a result"}) == 0 + + @pytest.mark.unit + @patch("deepctl_core.base_command.AuthManager") + @patch("deepctl_core.base_command.DeepgramClient") + def test_execute_exits_nonzero_on_error_result( + self, _client_class, _auth_class, command + ): + cmd = command() + command.result = BaseResult(status="error", message="API connection failed") + + with pytest.raises(SystemExit) as exc: + cmd.execute(_exit_code_ctx()) + + assert exc.value.code == 1 + + @pytest.mark.unit + @patch("deepctl_core.base_command.AuthManager") + @patch("deepctl_core.base_command.DeepgramClient") + def test_execute_exits_zero_on_success_result( + self, _client_class, _auth_class, command + ): + cmd = command() + command.result = BaseResult(status="success", message="done") + + # No SystemExit at all — a successful command must fall through. + cmd.execute(_exit_code_ctx()) + + +class TestMachineReadablePayloads: + """yaml/csv payloads must reach stdout byte-for-byte. + + Rich's default print treats square brackets as style markup and deletes + them, and hard-wraps at the console width; both silently corrupt user data + such as an API key comment of "[ci] runner". + """ + + @pytest.fixture + def command(self): + class MockCommand(BaseCommand): + name = "test" + help = "Test command" + + def handle(self, *args, **kwargs): + return None + + return MockCommand() + + @staticmethod + def _capture(command, method, payload): + import io + + from rich.console import Console + + buffer = io.StringIO() + # Deliberately narrow: a wrapping console would break the long value. + with patch( + "deepctl_core.base_command.console", + Console(file=buffer, width=40, no_color=True), + ): + getattr(command, method)(payload) + return buffer.getvalue() + + @pytest.mark.unit + def test_yaml_preserves_brackets_and_does_not_wrap(self, command): + comment = "[bold red]staging[/bold red] key " + "x" * 60 + out = self._capture(command, "_output_yaml", {"comment": comment}) + + assert "[bold red]staging[/bold red] key" in out + assert "x" * 60 in out + + @pytest.mark.unit + def test_csv_preserves_brackets_and_does_not_wrap(self, command): + comment = "[ci] runner " + "y" * 60 + out = self._capture(command, "_output_csv", {"comment": comment}) + + assert "[ci] runner" in out + assert "y" * 60 in out + + @pytest.mark.unit + def test_json_preserves_brackets(self, command): + out = self._capture(command, "_output_json", {"comment": "[ci] runner"}) + + assert "[ci] runner" in out + + +def _exit_code_ctx(): + """A minimal Click context good enough to drive BaseCommand.execute.""" + ctx = Mock(spec=click.Context) + ctx.obj = {"config": Config()} + ctx.command_path = "deepctl test" + ctx.params = {} + ctx.command = Mock() + ctx.command.params = [] + src = Mock() + src.name = "DEFAULT" + ctx.get_parameter_source = lambda _name: src + return ctx + + class TestConfirmPromptGating: """Verify confirm() and prompt() respect _guided alongside _agentic and ci_friendly.""" diff --git a/src/deepctl/main.py b/src/deepctl/main.py index c5828f9..db6aa0e 100644 --- a/src/deepctl/main.py +++ b/src/deepctl/main.py @@ -361,12 +361,18 @@ def main() -> None: # Preprocess arguments to handle hyphenated commands processed_args = preprocess_hyphenated_commands(args) + exit_code = 0 with TimingContext("cli_execution"), _telemetry_transaction(): try: cli(args=processed_args, standalone_mode=False) - except SystemExit: - # Click calls sys.exit() even in non-standalone mode - pass + except SystemExit as exc: + # Click calls sys.exit() even in non-standalone mode, so we + # cannot let this propagate — the notifications and timing + # summary below still need to run. But swallowing the code + # outright made every failure exit 0, which is what broke + # `if dg -o json keys; then ...` in scripts and CI. Carry it + # and re-raise at the end instead. + exit_code = exc.code if isinstance(exc.code, int) else 0 # Print update notifications if available (before timing summary) if print_pending_notification is not None: @@ -380,6 +386,9 @@ def main() -> None: if timing_requested: print_timing_summary(detailed_timing) + if exit_code: + sys.exit(exit_code) + except KeyboardInterrupt: _safe_console_print("\n[yellow]Operation cancelled by user[/yellow]") sys.exit(2) From 36b5f3f85eaec27d01d79164df845c744842bdce Mon Sep 17 00:00:00 2001 From: Greg Holmes Date: Tue, 18 Aug 2026 16:14:04 +0100 Subject: [PATCH 3/4] fix(keys): make --create --dry-run reachable and actually confirm deletes Two defects found reviewing #101, both older than the -o json work but both in code this branch touches. --create --dry-run never ran. handle() read project_id and dry_run off kwargs with .get(), leaving them in the dict, then forwarded **kwargs alongside them to _create_key. Every argument arrived twice, so Python raised "got multiple values for argument 'project_id'" before the function body started; the error was swallowed into an error result and the process still exited 0. The dry-run gating added on this branch was unreachable, and the "known limitation" noted in the PR description described behaviour the path never had. Popping both fixes it -- fixing only dry_run just exposes the project_id collision behind it. --dry-run is the flag a cautious developer reaches for first on a command that mints credentials, so it should not print a private method name at them. --delete without --yes deleted nothing and blamed the user. BaseCommand.confirm returns its default whenever any parameter came from the command line, and --delete KEY_ID is itself such a parameter, so the prompt was unreachable and the command always returned "Cancelled by user" without asking or calling the API. _confirm_delete now prompts on stderr when someone is there to answer -- stderr so -o json stdout stays parseable -- and returns a usage error naming --yes when nobody is, rather than reporting a cancellation that never happened. With the exit-code mapping that error now exits 1 instead of 0. Also switches to core's get_status_console(), and adds tests for both dry-run paths, all four confirmation outcomes, and the error path -- the absence of a --create --dry-run test is why this shipped. --- .../src/deepctl_cmd_keys/command.py | 55 +++++- .../tests/unit/test_keys_command.py | 181 ++++++++++++++++++ 2 files changed, 228 insertions(+), 8 deletions(-) diff --git a/packages/deepctl-cmd-keys/src/deepctl_cmd_keys/command.py b/packages/deepctl-cmd-keys/src/deepctl_cmd_keys/command.py index aeae024..71a10d5 100644 --- a/packages/deepctl-cmd-keys/src/deepctl_cmd_keys/command.py +++ b/packages/deepctl-cmd-keys/src/deepctl_cmd_keys/command.py @@ -2,8 +2,10 @@ from __future__ import annotations +import sys from typing import Any +import click from deepctl_core import ( AuthManager, BaseCommand, @@ -11,6 +13,8 @@ Config, DeepgramClient, get_output_format, + get_status_console, + is_agentic, ) from rich.console import Console from rich.table import Table @@ -20,7 +24,7 @@ console = Console() # Status/progress chrome must never touch stdout, or it corrupts JSON/CSV # output that callers pipe into jq and friends. -status_console = Console(stderr=True) +status_console = get_status_console() class KeysCommand(BaseCommand): @@ -141,9 +145,11 @@ def handle( create_key = kwargs.get("create", False) show_key = kwargs.get("show") delete_key = kwargs.get("delete") - project_id = kwargs.get("project_id") - - dry_run = kwargs.get("dry_run", False) + # Popped, not read: both are forwarded explicitly to _create_key, which + # also receives **kwargs. Leaving them in the dict passes each argument + # twice and raises TypeError before the body runs. + project_id = kwargs.pop("project_id", None) + dry_run = kwargs.pop("dry_run", False) try: if create_key: @@ -336,12 +342,45 @@ def _delete_key( status="dry_run", message=f"Dry run: key {key_id} would be deleted" ) - if not yes and not self.confirm( - f"Delete API key {key_id}? This cannot be undone.", default=False - ): - return BaseResult(status="cancelled", message="Cancelled by user") + if not yes: + aborted = self._confirm_delete(key_id) + if aborted is not None: + return aborted status_console.print(f"[blue]Deleting API key:[/blue] {key_id}") client.delete_key(key_id, project_id=project_id) status_console.print(f"[green]API key {key_id} deleted[/green]") return BaseResult(status="success", message=f"Key {key_id} deleted") + + def _confirm_delete(self, key_id: str) -> BaseResult | None: + """Gate a delete behind confirmation; return a result to abort with. + + `BaseCommand.confirm` cannot be used here. It returns its default + (False) whenever any parameter came from the command line, and + `--delete KEY_ID` is itself such a parameter — so the prompt never + appeared and every `dg keys --delete ID` reported "Cancelled by user" + without asking and without deleting. Prompting on stderr keeps stdout + parseable under `-o json`. + + Returns: + None to proceed with the delete, or the result to return instead + """ + if is_agentic() or not sys.stdin.isatty(): + # No one is there to answer. Refusing is right, but it is a usage + # error rather than something the user chose to cancel. + return BaseResult( + status="error", + message=( + f"Refusing to delete key {key_id} without confirmation. " + "Pass --yes to confirm." + ), + ) + + if not click.confirm( + f"Delete API key {key_id}? This cannot be undone.", + default=False, + err=True, + ): + return BaseResult(status="cancelled", message="Delete cancelled") + + return None diff --git a/packages/deepctl-cmd-keys/tests/unit/test_keys_command.py b/packages/deepctl-cmd-keys/tests/unit/test_keys_command.py index c71bee2..6b8dcc0 100644 --- a/packages/deepctl-cmd-keys/tests/unit/test_keys_command.py +++ b/packages/deepctl-cmd-keys/tests/unit/test_keys_command.py @@ -315,6 +315,187 @@ def test_json_mode_show_key_renders_nothing( assert result.keys[0].key_id == "key1" +class TestKeysDryRun: + """`--dry-run` must reach its own code, not a TypeError. + + `handle` read `project_id` and `dry_run` off kwargs with `.get()` and then + forwarded `**kwargs` alongside them, so every argument arrived twice and + `--create --dry-run` failed with "got multiple values for argument" before + the dry-run body ran. + """ + + @pytest.fixture + def command(self): + return KeysCommand() + + @pytest.fixture + def mock_client(self): + return Mock(spec=DeepgramClient) + + @patch("deepctl_cmd_keys.command.get_output_format", return_value="json") + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.console") + def test_create_dry_run_reports_dry_run_and_calls_nothing( + self, mock_console, _status, _fmt, command, mock_client + ): + result = command.handle( + Mock(spec=Config), + Mock(spec=AuthManager), + mock_client, + create=True, + dry_run=True, + project_id=None, + comment="probe", + scopes="member", + ttl=3600, + tags="a,b", + ) + + assert result.status == "dry_run" + mock_client.create_key.assert_not_called() + mock_console.print.assert_not_called() + + @patch("deepctl_cmd_keys.command.get_output_format", return_value="default") + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.console") + def test_create_dry_run_renders_summary_for_humans( + self, mock_console, _status, _fmt, command, mock_client + ): + result = command.handle( + Mock(spec=Config), + Mock(spec=AuthManager), + mock_client, + create=True, + dry_run=True, + project_id=None, + comment="probe", + scopes="member", + ) + + assert result.status == "dry_run" + printed = " ".join( + str(c.args[0]) for c in mock_console.print.call_args_list if c.args + ) + assert "Dry run" in printed + assert "probe" in printed + + @patch("deepctl_cmd_keys.command.get_output_format", return_value="json") + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.console") + def test_delete_dry_run_reports_dry_run_and_calls_nothing( + self, mock_console, _status, _fmt, command, mock_client + ): + result = command.handle( + Mock(spec=Config), + Mock(spec=AuthManager), + mock_client, + delete="key-id", + dry_run=True, + project_id=None, + ) + + assert result.status == "dry_run" + assert "key-id" in result.message + mock_client.delete_key.assert_not_called() + mock_console.print.assert_not_called() + + +class TestKeysDeleteConfirmation: + """`--delete` without `--yes` must not silently no-op. + + `BaseCommand.confirm` returns its default whenever any parameter came from + the command line, and `--delete KEY_ID` is itself such a parameter — so the + prompt never appeared and the command reported "Cancelled by user" without + asking anyone and without deleting anything. + """ + + @pytest.fixture + def command(self): + return KeysCommand() + + @pytest.fixture + def mock_client(self): + return Mock(spec=DeepgramClient) + + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.is_agentic", return_value=True) + def test_non_interactive_without_yes_errors_and_deletes_nothing( + self, _agentic, _status, command, mock_client + ): + result = command._delete_key(mock_client, "key-id", None, yes=False) + + assert result.status == "error" + assert "--yes" in result.message + mock_client.delete_key.assert_not_called() + + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.is_agentic", return_value=False) + def test_interactive_declined_cancels_and_deletes_nothing( + self, _agentic, _status, command, mock_client + ): + with ( + patch("deepctl_cmd_keys.command.sys.stdin.isatty", return_value=True), + patch("click.confirm", return_value=False) as mock_confirm, + ): + result = command._delete_key(mock_client, "key-id", None, yes=False) + + assert result.status == "cancelled" + mock_client.delete_key.assert_not_called() + # The prompt goes to stderr, so stdout stays parseable under -o json. + assert mock_confirm.call_args.kwargs["err"] is True + + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.is_agentic", return_value=False) + def test_interactive_accepted_deletes( + self, _agentic, _status, command, mock_client + ): + with ( + patch("deepctl_cmd_keys.command.sys.stdin.isatty", return_value=True), + patch("click.confirm", return_value=True), + ): + result = command._delete_key(mock_client, "key-id", None, yes=False) + + assert result.status == "success" + mock_client.delete_key.assert_called_once_with("key-id", project_id=None) + + @patch("deepctl_cmd_keys.command.status_console") + def test_yes_skips_confirmation_entirely(self, _status, command, mock_client): + with patch("click.confirm", side_effect=AssertionError("prompted anyway")): + result = command._delete_key(mock_client, "key-id", None, yes=True) + + assert result.status == "success" + mock_client.delete_key.assert_called_once_with("key-id", project_id=None) + + +class TestKeysErrorPath: + """Errors are chrome, not payload: stderr, and a non-zero exit code.""" + + @pytest.fixture + def command(self): + return KeysCommand() + + @patch("deepctl_cmd_keys.command.get_output_format", return_value="json") + @patch("deepctl_cmd_keys.command.status_console") + @patch("deepctl_cmd_keys.command.console") + def test_error_goes_to_stderr_and_maps_to_exit_1( + self, mock_console, mock_status_console, _fmt, command + ): + client = Mock(spec=DeepgramClient) + client.list_keys.side_effect = Exception("API connection failed") + + result = command.handle( + Mock(spec=Config), Mock(spec=AuthManager), client, project_id=None + ) + + assert result.status == "error" + mock_console.print.assert_not_called() + stderr_text = " ".join( + str(c.args[0]) for c in mock_status_console.print.call_args_list if c.args + ) + assert "API connection failed" in stderr_text + assert command.exit_code_for(result) == 1 + + class TestKeysModels: """Test cases for keys models.""" From 9ea9542735afce75e6fd830558d347abf4370b80 Mon Sep 17 00:00:00 2001 From: Greg Holmes Date: Tue, 18 Aug 2026 16:27:27 +0100 Subject: [PATCH 4/4] fix(core): use exit code 2 for cancelled, per the published contract llms-full.txt documents "Exit codes: 0 = success, 1 = error, 2 = user interrupt". The status->code table landed cancelled on the shell's 130, which contradicts that and contradicts main.py's own KeyboardInterrupt handler, which already exits 2. deepctl-cmd-mcp returns status="cancelled" straight out of its KeyboardInterrupt handler, so Ctrl-C on `dg mcp` exited 130 where the docs promise 2. This also settles the versioning question: with error -> 1 and cancelled -> 2, the exit codes now match what was already published, so this is a bug fix restoring documented behaviour rather than a new contract. No breaking-change footer, no docs update needed. --- .../deepctl-core/src/deepctl_core/base_command.py | 12 ++++++++---- packages/deepctl-core/tests/unit/test_base.py | 5 ++++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/deepctl-core/src/deepctl_core/base_command.py b/packages/deepctl-core/src/deepctl_core/base_command.py index 24def7c..38f8b69 100644 --- a/packages/deepctl-core/src/deepctl_core/base_command.py +++ b/packages/deepctl-core/src/deepctl_core/base_command.py @@ -29,13 +29,17 @@ class BaseCommand(ABC): requires_project: bool = False ci_friendly: bool = True - # Result status -> process exit code. A command that reports failure must - # not exit 0: scripts and CI branch on the exit code, not on the "status" - # field inside the payload. Any status not listed here exits 0 + # Result status -> process exit code, per the contract published in + # llms-full.txt: 0 = success, 1 = error, 2 = user interrupt. A command that + # reports failure must not exit 0 -- scripts and CI branch on the exit code, + # not on the "status" field inside the payload. "cancelled" is 2 rather than + # the shell's 130 to match that contract and main.py's own KeyboardInterrupt + # handler; `dg mcp` in particular returns cancelled straight out of its + # KeyboardInterrupt path. Any status not listed here exits 0 # (success, succeeded, info, dry_run, warning). EXIT_CODES: ClassVar[dict[str, int]] = { "error": 1, - "cancelled": 130, + "cancelled": 2, } # Agent-oriented metadata diff --git a/packages/deepctl-core/tests/unit/test_base.py b/packages/deepctl-core/tests/unit/test_base.py index d78f802..d494e0b 100644 --- a/packages/deepctl-core/tests/unit/test_base.py +++ b/packages/deepctl-core/tests/unit/test_base.py @@ -1097,7 +1097,10 @@ def handle(self, *args, **kwargs): ("status", "expected"), [ ("error", 1), - ("cancelled", 130), + # 2, not the shell's 130: llms-full.txt publishes + # "0 = success, 1 = error, 2 = user interrupt", and main.py's + # KeyboardInterrupt handler already exits 2. + ("cancelled", 2), ("success", 0), ("succeeded", 0), ("info", 0),