fix(cli): make _verbose_console silent-aware and gate replay prints (#209)#223
Merged
Conversation
…209) 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 <log>` 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>
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 <log>` still produces 0 bytes on
stderr; `conductor --quiet replay <log>` 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>
Collaborator
Author
|
Addressed PR review feedback (commit bcc29a5): Critical
Important
Suggestions
Verification
Not adopted (with reason)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #211, closes #209.
Problem
PR #211 gated the three foreground dashboard URL prints behind
is_verbose()but left several other stderr leaks in place. Under--silent("No progress output. Only JSON result on stdout."):app.py:_run_replaystill printedPress Ctrl+C to exitandReplay stoppedunconditionally.run.pyhad ~12_verbose_console.printcall sites that bypassedis_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
Fix at the source. Subclass
Consolefor_verbose_consoleso every.print(...)is a no-op whenis_verbose()is False. This:_verbose_consoleis now actually verbose-aware)._verbose_console.print(...)calls inrun.pywill now respect--silentautomatically.verbose_log,verbose_log_agent_*, the four PR fix(cli): gate remaining dashboard URL prints behind is_verbose() for --silent compliance #211 dashboard prints) — their internalis_verbose()checks become belt-and-suspenders.The app-wide
consoleinapp.pyis intentionally NOT made silent-aware because it carries real error messages (print_erroretc.). The two remaining replay prints in_run_replayare gated per-call instead.Acceptance criterion
Verified end-to-end. Pre-fix, the same invocation leaked
Press Ctrl+C to exitto stderr.Tests
TestSilentAwareConsole(3 tests) — verifies the subclass mechanism and that the module-level_verbose_consoleinstance uses it. Thetest_module_verbose_console_is_silent_awaretest guards against a future refactor accidentally swapping back to a plainConsole(the original regression).TestReplaySilentCompliance(2 tests) — verifiesconductor --silent replayproduces no stderr, and that without--silentthe messages still appear (guard against an over-eager gate).Mocks
ReplayDashboard.start/stopand patchesasyncio.Eventto raiseCancelledError; otherwise the tests would hang on theawait asyncio.Event().wait()the replay command uses to keep the dashboard alive.All 397 existing
tests/test_clitests pass.Rubber-duck pass
Consulted before implementation. Key findings adopted:
asyncio.Eventpatch above.Press Esconly fires for non---webinteractive runs, so a--silent run --webtest couldn't exercise it — the silent-aware subclass tests at the unit level cover all 12run.pycall sites at once instead.