diff --git a/src/conductor/cli/app.py b/src/conductor/cli/app.py index ef7be97..93f5bc6 100644 --- a/src/conductor/cli/app.py +++ b/src/conductor/cli/app.py @@ -993,7 +993,7 @@ async def _run_replay() -> None: await dashboard.start() if is_verbose(): console.print(f"\n[bold green]▶ Replay dashboard:[/] {dashboard.url}\n") - console.print("[dim]Press Ctrl+C to exit[/dim]\n") + console.print("[dim]Press Ctrl+C to exit[/dim]\n") try: await asyncio.Event().wait() @@ -1005,7 +1005,8 @@ async def _run_replay() -> None: try: asyncio.run(_run_replay()) except KeyboardInterrupt: - console.print("\n[dim]Replay stopped.[/dim]") + if is_verbose(): + console.print("\n[dim]Replay stopped.[/dim]") @app.command() diff --git a/src/conductor/cli/run.py b/src/conductor/cli/run.py index e626d09..e8e0e74 100644 --- a/src/conductor/cli/run.py +++ b/src/conductor/cli/run.py @@ -30,8 +30,45 @@ if TYPE_CHECKING: from conductor.events import WorkflowEvent -# Verbose console for logging (stderr) -_verbose_console = Console(stderr=True, highlight=False) + +# Verbose console for logging (stderr). +# +# This subclass enforces the global --silent contract ("No progress output. +# Only JSON result on stdout.") at the source: every ``.print()`` call on a +# ``_SilentAwareConsole`` becomes a no-op when ``is_verbose()`` is False, so +# individual call sites do not each have to remember to gate themselves +# (see issue #209, follow-up to #203/#211). File logging is unaffected — +# ``_file_console`` is a separate Console wired up by ``init_file_logging``. +# +# Scope: only ``.print()`` is gated. Other Rich ``Console`` output methods +# (``.log()``, ``.rule()``, ``.status()``, ``.print_json()``, etc.) would +# silently bypass ``--silent`` if used on this instance. All current call +# sites in this module use only ``.print``; if you introduce a new one, +# either route it through ``.print`` or extend this subclass. +class _SilentAwareConsole(Console): + """``Console`` that honors ``--silent`` at the print level. + + The instance is locked to ``stderr=True`` to preserve the contract that + ``--silent`` runs emit JSON on stdout with nothing else; routing gated + output to stdout would corrupt that channel. + """ + + def __init__(self, **kwargs: Any) -> None: + # Lock stderr=True; everything else (highlight, width, etc.) is + # caller-tunable. + kwargs.pop("stderr", None) + super().__init__(stderr=True, **kwargs) + + def print(self, *args: Any, **kwargs: Any) -> None: + # Lazy import to avoid the cli.run -> cli.app import cycle at module + # load time. + from conductor.cli.app import is_verbose + + if is_verbose(): + super().print(*args, **kwargs) + + +_verbose_console = _SilentAwareConsole(highlight=False) # File console for file logging (None when not active) _file_console: Console | None = None diff --git a/tests/test_cli/test_logging.py b/tests/test_cli/test_logging.py index deb689a..eb3cdb6 100644 --- a/tests/test_cli/test_logging.py +++ b/tests/test_cli/test_logging.py @@ -1749,3 +1749,61 @@ def test_agent_timeout_event_triggers_log(self) -> None: assert "timed out" in output_text finally: verbose_mode.reset(token) + + +class TestSilentAwareConsole: + """Tests for the ``_SilentAwareConsole`` subclass (issue #209). + + The verbose console used by ``run`` / ``resume`` is silent-aware at the + source: every ``.print(...)`` call no-ops when ``is_verbose()`` is False. + This guarantees ``--silent`` compliance for *every* call site in + ``conductor.cli.run`` without each site having to remember to gate + itself. + """ + + def test_print_writes_when_verbose(self) -> None: + """Verbose mode (the default) lets ``print`` reach the underlying stream.""" + from io import StringIO + + from conductor.cli.run import _SilentAwareConsole + + sink = StringIO() + console = _SilentAwareConsole(file=sink, force_terminal=True, no_color=True) + + token = verbose_mode.set(True) + try: + console.print("hello world") + finally: + verbose_mode.reset(token) + + assert "hello world" in sink.getvalue() + + def test_print_suppresses_when_silent(self) -> None: + """Silent mode (``verbose_mode=False``) makes ``print`` a no-op.""" + from io import StringIO + + from conductor.cli.run import _SilentAwareConsole + + sink = StringIO() + console = _SilentAwareConsole(file=sink, force_terminal=True, no_color=True) + + token = verbose_mode.set(False) + try: + console.print("hello world") + console.print("[bold yellow]Warning:[/bold yellow] something failed") + console.print() # empty-arg call must also be a no-op + finally: + verbose_mode.reset(token) + + assert sink.getvalue() == "" + + def test_module_verbose_console_is_silent_aware(self) -> None: + """Module-level ``_verbose_console`` is an instance of the silent-aware subclass. + + Guards against a future refactor accidentally swapping it back to a + plain ``rich.console.Console``, which is the regression issue #209 + is about. + """ + from conductor.cli.run import _SilentAwareConsole, _verbose_console + + assert isinstance(_verbose_console, _SilentAwareConsole) diff --git a/tests/test_cli/test_replay_command.py b/tests/test_cli/test_replay_command.py index cb68e34..b84a1f5 100644 --- a/tests/test_cli/test_replay_command.py +++ b/tests/test_cli/test_replay_command.py @@ -12,7 +12,8 @@ import json import re from pathlib import Path -from unittest.mock import AsyncMock, patch +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch from typer.testing import CliRunner @@ -81,3 +82,143 @@ def test_custom_port_option(self) -> None: result = runner.invoke(app, ["replay", "--help"]) clean = _ANSI_RE.sub("", result.output) assert "--web-port" in clean + + +class TestReplaySilentCompliance: + """Regression tests for issue #209. + + PR #211 gated the dashboard URL print in ``_run_replay`` behind + ``is_verbose()`` but missed the ``Press Ctrl+C to exit`` and + ``Replay stopped`` prints. Under ``--silent`` those messages still + leaked to stderr, violating the ``--silent`` contract ("No progress + output. Only JSON result on stdout."). + + Two pairs of tests are needed because there are two stderr-leak + paths in the ``replay`` command: + + * **Inner path** — prints inside ``_run_replay`` after the dashboard + starts (``app.py:994-996``). Exercised by faking + ``asyncio.Event().wait()`` to raise ``CancelledError``. + * **Outer path** — the ``Replay stopped`` print inside the + ``except KeyboardInterrupt`` handler that wraps ``asyncio.run`` + (``app.py:1007-1009``). Exercised by making ``asyncio.run`` itself + raise ``KeyboardInterrupt``. + + Each path also has a verbose counterpart to guard against an + over-eager gate suppressing the message when the user did NOT pass + ``--silent``. + """ + + @staticmethod + def _mock_replay_dashboard() -> MagicMock: + """Build a dashboard mock with async ``start``/``stop`` no-ops.""" + mock_dashboard = MagicMock() + mock_dashboard.url = "http://127.0.0.1:9999" + mock_dashboard.start = AsyncMock() + mock_dashboard.stop = AsyncMock() + return mock_dashboard + + @staticmethod + def _patch_inner_wait_to_cancel() -> Any: + """Patch ``asyncio.Event`` so ``await ... .wait()`` raises ``CancelledError``. + + Without this the replay command hangs forever on the + ``await asyncio.Event().wait()`` it uses to keep the dashboard + alive after start. + """ + import asyncio + + mock_event = MagicMock() + mock_event.wait = AsyncMock(side_effect=asyncio.CancelledError()) + return patch("asyncio.Event", return_value=mock_event) + + # ----- inner path: prints inside _run_replay ----- + + def test_silent_replay_suppresses_dashboard_messages(self, tmp_path: Path) -> None: + """``--silent replay`` must not leak the dashboard URL or hint to stderr.""" + log_path = _write_log_file(tmp_path) + mock_dashboard = self._mock_replay_dashboard() + + with ( + patch( + "conductor.web.replay.ReplayDashboard", return_value=mock_dashboard + ) as mock_dashboard_cls, + self._patch_inner_wait_to_cancel(), + ): + result = runner.invoke(app, ["--silent", "replay", str(log_path)]) + + assert result.exit_code == 0, result.output + assert mock_dashboard_cls.called + stderr = _ANSI_RE.sub("", result.stderr) + assert "Replay dashboard" not in stderr + assert "Press Ctrl+C" not in stderr + assert "http://127.0.0.1:9999" not in stderr + + def test_verbose_replay_prints_dashboard_messages(self, tmp_path: Path) -> None: + """Sanity counterpart: without ``--silent`` the messages still appear.""" + log_path = _write_log_file(tmp_path) + mock_dashboard = self._mock_replay_dashboard() + + with ( + patch("conductor.web.replay.ReplayDashboard", return_value=mock_dashboard), + self._patch_inner_wait_to_cancel(), + ): + result = runner.invoke(app, ["replay", str(log_path)]) + + assert result.exit_code == 0, result.output + stderr = _ANSI_RE.sub("", result.stderr) + assert "Replay dashboard" in stderr + assert "http://127.0.0.1:9999" in stderr + assert "Press Ctrl+C" in stderr + + def test_quiet_replay_prints_dashboard_messages(self, tmp_path: Path) -> None: + """``--quiet`` (MINIMAL) must NOT suppress. + + Locks in the contract from ``app.py:204`` + (``verbose_mode.set(verbosity != ConsoleVerbosity.SILENT)``) — if + a future refactor changes that line to ``== ConsoleVerbosity.FULL``, + MINIMAL mode would start swallowing every progress message, which + is not what ``--quiet`` is supposed to mean. + """ + log_path = _write_log_file(tmp_path) + mock_dashboard = self._mock_replay_dashboard() + + with ( + patch("conductor.web.replay.ReplayDashboard", return_value=mock_dashboard), + self._patch_inner_wait_to_cancel(), + ): + result = runner.invoke(app, ["--quiet", "replay", str(log_path)]) + + assert result.exit_code == 0, result.output + stderr = _ANSI_RE.sub("", result.stderr) + assert "Replay dashboard" in stderr + assert "http://127.0.0.1:9999" in stderr + + # ----- outer path: print inside the KeyboardInterrupt handler ----- + + def test_silent_replay_suppresses_keyboardinterrupt_message(self, tmp_path: Path) -> None: + """``--silent replay`` must not leak ``Replay stopped`` on Ctrl+C. + + Drives the outer ``except KeyboardInterrupt`` branch by making + ``asyncio.run`` itself raise — this is robust against future + refactors of the wait primitive inside ``_run_replay``. + """ + log_path = _write_log_file(tmp_path) + + with patch("asyncio.run", side_effect=KeyboardInterrupt()): + result = runner.invoke(app, ["--silent", "replay", str(log_path)]) + + assert result.exit_code == 0, result.output + stderr = _ANSI_RE.sub("", result.stderr) + assert "Replay stopped" not in stderr + + def test_verbose_replay_prints_keyboardinterrupt_message(self, tmp_path: Path) -> None: + """Sanity counterpart: without ``--silent`` Ctrl+C still prints the hint.""" + log_path = _write_log_file(tmp_path) + + with patch("asyncio.run", side_effect=KeyboardInterrupt()): + result = runner.invoke(app, ["replay", str(log_path)]) + + assert result.exit_code == 0, result.output + stderr = _ANSI_RE.sub("", result.stderr) + assert "Replay stopped" in stderr