From 0fc513568cf422aad62c2d3069457f310b20eb4f Mon Sep 17 00:00:00 2001 From: Jason Robert Date: Thu, 21 May 2026 14:37:02 -0400 Subject: [PATCH 1/2] fix(cli): make _verbose_console silent-aware and gate replay prints (#209) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #211 (follow-up to #203) gated three foreground dashboard URL prints behind `is_verbose()` but left the remaining stderr leaks in place, violating the `--silent` contract ("No progress output. Only JSON result on stdout."). Specifically: * `app.py:_run_replay` still printed 'Press Ctrl+C to exit' and 'Replay stopped' unconditionally. * `run.py` had ~12 `_verbose_console.print` call sites that bypassed `is_verbose()` — warnings (dashboard failed to start, log-file open failure, workflow-hash mismatch), 'Press Esc to interrupt', 'Event log written to...', 'Log written to...', and `_print_resume_instructions` (printed on failure). Fix at the source: subclass `Console` for `_verbose_console` so every `.print(...)` no-ops when `is_verbose()` is False. This aligns the implementation with the long-misleading name, removes the per-call-site audit burden, and is a no-op for the already-gated helpers. The app-wide `console` in app.py is intentionally NOT made silent-aware because it carries real error messages; the two remaining replay prints are gated per-call instead. Acceptance criterion verified: `conductor --silent replay ` produces 0 bytes on stderr (was leaking 'Press Ctrl+C to exit'). Regression tests: * `TestSilentAwareConsole` (3 tests) — verifies the subclass mechanism and that the module-level instance uses it. * `TestReplaySilentCompliance` (2 tests) — verifies the replay command produces no stderr under `--silent` and still prints when verbose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/conductor/cli/app.py | 5 +- src/conductor/cli/run.py | 24 +++++++- tests/test_cli/test_logging.py | 58 +++++++++++++++++++ tests/test_cli/test_replay_command.py | 80 ++++++++++++++++++++++++++- 4 files changed, 162 insertions(+), 5 deletions(-) diff --git a/src/conductor/cli/app.py b/src/conductor/cli/app.py index ef7be97b..93f5bc60 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 e626d090..01a126f5 100644 --- a/src/conductor/cli/run.py +++ b/src/conductor/cli/run.py @@ -30,8 +30,28 @@ 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 ``_verbose_console.print`` +# call in this module 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``. +class _SilentAwareConsole(Console): + """``Console`` that honors ``--silent`` at the print level.""" + + def print(self, *args: Any, **kwargs: Any) -> None: + # Lazy import to avoid the cli.run -> cli.app import cycle at module + # load time, and to pick up the current ContextVar value per-call. + from conductor.cli.app import is_verbose + + if is_verbose(): + super().print(*args, **kwargs) + + +_verbose_console = _SilentAwareConsole(stderr=True, 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 deb689aa..eb3cdb67 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 cb68e347..5f302f76 100644 --- a/tests/test_cli/test_replay_command.py +++ b/tests/test_cli/test_replay_command.py @@ -12,7 +12,7 @@ import json import re from pathlib import Path -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock, MagicMock, patch from typer.testing import CliRunner @@ -81,3 +81,81 @@ 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."). + """ + + def test_silent_replay_suppresses_all_stderr(self, tmp_path: Path) -> None: + """``conductor --silent replay `` must not write anything to stderr. + + Mocks the dashboard so ``start``/``stop`` are cheap, and patches + ``asyncio.Event`` so the ``await ... .wait()`` raises + ``CancelledError`` immediately (otherwise the test would hang + on the infinite wait the replay command uses to keep the + dashboard alive). + """ + import asyncio + + log_path = _write_log_file(tmp_path) + + mock_dashboard = MagicMock() + mock_dashboard.url = "http://127.0.0.1:9999" + mock_dashboard.start = AsyncMock() + mock_dashboard.stop = AsyncMock() + + mock_event = MagicMock() + mock_event.wait = AsyncMock(side_effect=asyncio.CancelledError()) + + with ( + patch( + "conductor.web.replay.ReplayDashboard", return_value=mock_dashboard + ) as mock_dashboard_cls, + patch("asyncio.Event", return_value=mock_event), + ): + result = runner.invoke(app, ["--silent", "replay", str(log_path)]) + + assert result.exit_code == 0, result.output + assert mock_dashboard_cls.called + clean = _ANSI_RE.sub("", result.output) + # All three messages from `_run_replay` must be suppressed. + assert "Replay dashboard" not in clean + assert "Press Ctrl+C" not in clean + assert "http://127.0.0.1:9999" not in clean + + def test_verbose_replay_still_prints_dashboard_info(self, tmp_path: Path) -> None: + """Sanity check: without ``--silent`` the messages still appear. + + Guards against an over-eager gate that would also suppress the + prints when the user actually wants them. + """ + import asyncio + + log_path = _write_log_file(tmp_path) + + mock_dashboard = MagicMock() + mock_dashboard.url = "http://127.0.0.1:9999" + mock_dashboard.start = AsyncMock() + mock_dashboard.stop = AsyncMock() + + mock_event = MagicMock() + mock_event.wait = AsyncMock(side_effect=asyncio.CancelledError()) + + with ( + patch("conductor.web.replay.ReplayDashboard", return_value=mock_dashboard), + patch("asyncio.Event", return_value=mock_event), + ): + result = runner.invoke(app, ["replay", str(log_path)]) + + assert result.exit_code == 0, result.output + clean = _ANSI_RE.sub("", result.output) + assert "Replay dashboard" in clean + assert "http://127.0.0.1:9999" in clean + assert "Press Ctrl+C" in clean From bcc29a5f498181724a046751e2805fb4d32a1560 Mon Sep 17 00:00:00 2001 From: Jason Robert Date: Thu, 21 May 2026 14:46:15 -0400 Subject: [PATCH 2/2] fix(cli): address PR #223 review findings (#209) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comprehensive PR review surfaced one critical coverage gap and a few defensive improvements. All addressed in this commit: * **Critical (pr-test-analyzer)**: The `KeyboardInterrupt` branch at `app.py:1007-1009` (`Replay stopped`) had zero behavioral test coverage — the previous `CancelledError`-via-`asyncio.Event` path never reaches the outer `except KeyboardInterrupt`. Added `test_silent_replay_suppresses_keyboardinterrupt_message` and its verbose counterpart that drive that branch via `patch("asyncio.run", side_effect=KeyboardInterrupt())`. This is also more robust than the inner-path mocking: a future refactor of `_run_replay`'s wait primitive cannot silently bypass it. * **Important (type-design-analyzer)**: `_SilentAwareConsole.__init__` now locks `stderr=True`. A future caller constructing an instance with `stderr=False` would have silently routed gated output to stdout and corrupted the `--silent` JSON contract. * **Important (comment-analyzer)**: Tests now assert on `result.stderr` (not the combined `result.output`), matching the test names and catching a hypothetical regression that moved the prints to stdout. * **Suggestion (pr-test-analyzer)**: Added `test_quiet_replay_prints_dashboard_messages` to lock in the contract at `app.py:204` that `--quiet` (MINIMAL) keeps `verbose_mode=True` — MINIMAL means "limited progress", not zero. * **Suggestion (multiple agents)**: Class-level comment on `_SilentAwareConsole` warns that only `.print()` is gated; `.rule()`/`.log()`/`.status()`/`.print_json()` would silently bypass `--silent`. All current call sites use only `.print`. Verified: `conductor --silent replay ` still produces 0 bytes on stderr; `conductor --quiet replay ` still prints the dashboard URL and Ctrl+C hint as before. 400 `tests/test_cli` tests pass (was 397; +3 net new). Lint/format/typecheck clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/conductor/cli/run.py | 27 +++++- tests/test_cli/test_replay_command.py | 135 +++++++++++++++++++------- 2 files changed, 121 insertions(+), 41 deletions(-) diff --git a/src/conductor/cli/run.py b/src/conductor/cli/run.py index 01a126f5..e8e0e74d 100644 --- a/src/conductor/cli/run.py +++ b/src/conductor/cli/run.py @@ -34,24 +34,41 @@ # Verbose console for logging (stderr). # # This subclass enforces the global --silent contract ("No progress output. -# Only JSON result on stdout.") at the source: every ``_verbose_console.print`` -# call in this module becomes a no-op when ``is_verbose()`` is False, so +# 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.""" + """``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, and to pick up the current ContextVar value per-call. + # load time. from conductor.cli.app import is_verbose if is_verbose(): super().print(*args, **kwargs) -_verbose_console = _SilentAwareConsole(stderr=True, highlight=False) +_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_replay_command.py b/tests/test_cli/test_replay_command.py index 5f302f76..b84a1f50 100644 --- a/tests/test_cli/test_replay_command.py +++ b/tests/test_cli/test_replay_command.py @@ -12,6 +12,7 @@ import json import re from pathlib import Path +from typing import Any from unittest.mock import AsyncMock, MagicMock, patch from typer.testing import CliRunner @@ -91,71 +92,133 @@ class TestReplaySilentCompliance: ``Replay stopped`` prints. Under ``--silent`` those messages still leaked to stderr, violating the ``--silent`` contract ("No progress output. Only JSON result on stdout."). - """ - def test_silent_replay_suppresses_all_stderr(self, tmp_path: Path) -> None: - """``conductor --silent replay `` must not write anything to stderr. + Two pairs of tests are needed because there are two stderr-leak + paths in the ``replay`` command: - Mocks the dashboard so ``start``/``stop`` are cheap, and patches - ``asyncio.Event`` so the ``await ... .wait()`` raises - ``CancelledError`` immediately (otherwise the test would hang - on the infinite wait the replay command uses to keep the - dashboard alive). - """ - import asyncio + * **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``. - log_path = _write_log_file(tmp_path) + 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, - patch("asyncio.Event", return_value=mock_event), + 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 - clean = _ANSI_RE.sub("", result.output) - # All three messages from `_run_replay` must be suppressed. - assert "Replay dashboard" not in clean - assert "Press Ctrl+C" not in clean - assert "http://127.0.0.1:9999" not in clean - - def test_verbose_replay_still_prints_dashboard_info(self, tmp_path: Path) -> None: - """Sanity check: without ``--silent`` the messages still appear. - - Guards against an over-eager gate that would also suppress the - prints when the user actually wants them. - """ - import asyncio + 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() - mock_dashboard = MagicMock() - mock_dashboard.url = "http://127.0.0.1:9999" - mock_dashboard.start = AsyncMock() - mock_dashboard.stop = AsyncMock() + with ( + patch("conductor.web.replay.ReplayDashboard", return_value=mock_dashboard), + self._patch_inner_wait_to_cancel(), + ): + result = runner.invoke(app, ["replay", str(log_path)]) - mock_event = MagicMock() - mock_event.wait = AsyncMock(side_effect=asyncio.CancelledError()) + 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), - patch("asyncio.Event", return_value=mock_event), + 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 - clean = _ANSI_RE.sub("", result.output) - assert "Replay dashboard" in clean - assert "http://127.0.0.1:9999" in clean - assert "Press Ctrl+C" in clean + stderr = _ANSI_RE.sub("", result.stderr) + assert "Replay stopped" in stderr