From ae4d486d17674f2ad2a0975dd665d94814e9a92b Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Mon, 7 Sep 2026 14:16:30 +0200 Subject: [PATCH 1/4] fix(gates): read a terminal dialog reply as one multi-line turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dialog gate read each reply with single-line `Prompt.ask`, so pasting a block of text into an interactive terminal dispatched every line as its own turn. A three-line paste became three separate questions to the model, each answered against a fragment of the intended message, and the paste's trailing newline added a fourth turn with empty content. The human gate already had a multi-line reader for exactly this shape, so the loop is extracted to `read_multiline_lines(console, sentinel)` in human.py and the dialog's main turn reads through it. `HumanGateHandler._read_multiline` delegates with its historical `.` sentinel, so that gate's behaviour is unchanged. The reader returns `(text, hit_eof)` rather than a bare string because the two EOF cases are different answers: an EOF that terminates a paste should submit the accumulated content, while an EOF with nothing accumulated is a deliberate Ctrl-D and dismisses. Collapsing them would make the dismissal branch unreachable, since the reader converts EOF into a returned string. The dialog's sentinel is `/send` rather than the human gate's `.` because a lone `.` is a plausible line of prose in a free-form reply. Because that makes `/send` load-bearing for submitting a turn, `_display_dialog_start` advertises it — gated on the same `sys.stdin.isatty()` condition as the reader, since off a tty the turn falls back to `Prompt.ask` and the sentinel does nothing. Off a tty (a pipe, CI, or the web dashboard, which returns via `_web_handle_dialog` and never renders this banner) the single-line path is untouched. Verified: tests/test_gates 69 passed; full suite 8308 passed with three failures that reproduce identically on an unpatched checkout (chmod 0o000 and case-sensitivity tests that do not hold on this filesystem). Each new test fails against the unpatched gate. ruff check, ruff format --check and ty are clean. --- AGENTS.md | 3 +- CHANGELOG.md | 21 +++++ docs/workflow-syntax.md | 1 + src/conductor/gates/dialog.py | 50 +++++++++- src/conductor/gates/human.py | 73 +++++++++++---- tests/test_gates/test_dialog.py | 161 ++++++++++++++++++++++++++++++++ tests/test_gates/test_human.py | 47 +++++++++- 7 files changed, 332 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 61605d14..8d908b16 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -158,7 +158,8 @@ step-by-step checklist. - `factory.py` - Provider instantiation - **gates/**: Human-in-the-loop support - - `human.py` - Rich terminal UI for human gate interactions + - `human.py` - Rich terminal UI for human gate interactions. `read_multiline_lines(console, sentinel)` is the shared blocking multi-line stdin reader behind both the human gate's `.` sentinel (`MULTILINE_SENTINEL`) and the dialog gate's `/send` (`DIALOG_SUBMIT_SENTINEL`); it returns `(text, hit_eof)` because an EOF that yielded no text is a deliberate dismissal while the sentinel with no text is an empty submission. The two gates use different sentinels, so a caller passes the one it wants rather than relying on the default + - `dialog.py` - Dialog-mode gate. On a tty the main turn reads through `read_multiline_lines`, so a pasted block is one turn rather than one turn per line; off a tty it falls back to single-line `Prompt.ask`, where the sentinel has no effect — `_display_dialog_start` gates the sentinel hint on the same `sys.stdin.isatty()` condition so the banner never advertises a keystroke that does nothing - **interrupt/**: Interactive workflow interruption (Esc/Ctrl+G to pause) - `listener.py` - Keyboard listener daemon thread for Esc/Ctrl+G detection diff --git a/CHANGELOG.md b/CHANGELOG.md index f3a369a1..a8b61e11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 entirely. See [`examples/claude-agent-sdk-setting-sources.yaml`](examples/claude-agent-sdk-setting-sources.yaml). +### Fixed + +- **A multi-line reply to a terminal dialog is now one turn** (#509) — + dialog mode was the only free-text human-input surface that could not accept + a multi-line answer (`QuestionDef.multiline` defaults to `True` and + `GateOption.multiline` opts in, both served by one reader in `gates/human.py`). + The dialog gate read a reply with single-line `Prompt.ask`, so pasting a block of + text into an interactive terminal dispatched *each line* as its own turn: a + three-line paste became three separate questions to the model, each answered + against a fragment, and the paste's trailing newline added a fourth turn with + empty content. Terminal turns now read through the multi-line reader already + behind the human gate's `.` sentinel, submitted with `/send` on its own line, + so internal newlines survive and a paste is a single message; an empty + submission is no longer dispatched as a turn. Ctrl-D (Ctrl-Z then Enter on + Windows) submits what has been typed and dismisses the dialog when nothing + has, and Ctrl-C dismisses rather than propagating. The dialog uses `/send` + where the human gate keeps `.`, since a lone `.` is likelier to be prose in + a conversational reply. Off a tty — a pipe, CI, or the web dashboard — the + single-line path is unchanged, and the opening banner advertises the sentinel + only where it applies. + ## [0.1.36](https://github.com/microsoft/conductor/compare/v0.1.35...v0.1.36) - 2026-09-02 ### Added diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index e6603705..a2efcf60 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -1198,6 +1198,7 @@ After the conversation, the agent re-executes with the dialog transcript as addi **Behavior notes:** - Dialog is supported on regular `agent` type only (not `human_gate`, `questions`, `script`, `workflow`, or `wait`) +- In an interactive terminal, a reply may span multiple lines — paste or type freely and submit the turn with `/send` on its own line. Ctrl-D (Ctrl-Z then Enter on Windows) also submits whatever has been typed, or dismisses the dialog when nothing has. Off a tty (a pipe, CI, or the web dashboard) replies are read one line at a time and `/send` does not apply - In web dashboard mode, the dialog temporarily replaces the graph area with a chat interface - When `--skip-gates` is set (e.g., CI/automation), dialogs are automatically skipped - The evaluator prompt should describe *when* to trigger dialog, not *what* to ask — the evaluator generates the opening question from the agent's output context diff --git a/src/conductor/gates/dialog.py b/src/conductor/gates/dialog.py index b38f88a5..ef38c44d 100644 --- a/src/conductor/gates/dialog.py +++ b/src/conductor/gates/dialog.py @@ -11,6 +11,7 @@ import asyncio import json import logging +import sys import uuid from dataclasses import dataclass, field from typing import TYPE_CHECKING, Any @@ -22,6 +23,11 @@ from conductor.console import MarkupFreeConsole, make_console, styled from conductor.executor.linkify import linkify_markdown +from conductor.gates.human import ( + DIALOG_SUBMIT_SENTINEL, + read_multiline_lines, + read_on_daemon_thread, +) if TYPE_CHECKING: from pathlib import Path @@ -273,6 +279,11 @@ async def handle_dialog( result.user_dismissed = True break + if user_input == "": + # User submitted nothing on a tty (e.g. an accidental bare + # sentinel line) -- not a turn, and not dismissal either. + continue + result.messages.append(DialogMessage(role="user", content=user_input)) self._emit_event( "dialog_message", @@ -600,15 +611,31 @@ def _display_dialog_start( base_dir: Path | None = None, ) -> None: """Display the dialog opening with full agent context.""" + # Gated on the same condition as the multi-line reader in + # _get_user_input: off a tty, that turn falls back to a single-line + # Prompt.ask and the sentinel does nothing, so advertising it would + # instruct the user to type something with no effect. The markup stays + # in the template because styled() inserts *values* verbatim. + if sys.stdin.isatty(): + multiline_hint = ( + " It can span multiple lines; send it with [bold]{}[/bold] on its own line." + ) + hint_args: tuple[object, ...] = (DIALOG_SUBMIT_SENTINEL,) + else: + multiline_hint = "" + hint_args = () + self.console.print() self.console.print( Panel( styled( "[bold]Agent '{}'[/bold] would like to discuss " "its output with you.\n" - "[dim]Type your responses below. Say [bold]done[/bold] or " - "[bold]/done[/bold] when finished.[/dim]", + "[dim]Type your response below." + multiline_hint + " " + "Say [bold]done[/bold] or [bold]/done[/bold] when " + "finished.[/dim]", agent.name, + *hint_args, ), title=Text.from_markup("[bold magenta]Dialog Mode[/bold magenta]"), border_style="magenta", @@ -732,8 +759,25 @@ async def _get_user_input( passing an interpolated f-string here. Returns: - User input text, or None on EOF/error. + User input text, or None on EOF/error, which the caller treats as + dismissal. The main turn (``prompt_text is None`` on a tty) reads + multi-line, so an EOF that *terminates a paste* returns the + accumulated content rather than dismissing; an EOF with nothing + accumulated is a deliberate Ctrl-D and still returns None. """ + if prompt_text is None and sys.stdin.isatty(): + self.console.print(styled("[bold magenta]You[/bold magenta]")) + try: + text, hit_eof = await read_on_daemon_thread( + lambda: read_multiline_lines(self.console, DIALOG_SUBMIT_SENTINEL) + ) + except (EOFError, KeyboardInterrupt): + return None + if hit_eof and not text: + # Ctrl-D at an empty prompt: the user is leaving, not pasting. + return None + return text + prompt = styled("[bold magenta]You[/bold magenta]") if prompt_text is None else prompt_text try: diff --git a/src/conductor/gates/human.py b/src/conductor/gates/human.py index 99fab198..28f77ae8 100644 --- a/src/conductor/gates/human.py +++ b/src/conductor/gates/human.py @@ -32,12 +32,62 @@ MULTILINE_SENTINEL = "." """Line that terminates a multi-line answer when typed on its own.""" +DIALOG_SUBMIT_SENTINEL = "/send" +"""Line that submits a multi-line dialog turn. + +Distinct from :data:`MULTILINE_SENTINEL` because a lone ``.`` is likelier to +be an ordinary line of prose in a conversational reply than in a gate answer. +""" + def _eof_key_hint() -> str: """Return the platform-appropriate EOF keystroke for display.""" return "Ctrl-Z then Enter" if sys.platform == "win32" else "Ctrl-D" +def read_multiline_lines( + console: MarkupFreeConsole, sentinel: str = MULTILINE_SENTINEL +) -> tuple[str, bool]: + """Read a multi-line answer from stdin (blocking; call on a thread). + + Terminates on a line whose stripped text equals ``sentinel`` or on EOF. + Internal newlines are preserved; trailing blank lines are stripped. + + Args: + console: Console to print the input hint to. + sentinel: Line that, typed alone, submits the accumulated text. + + Returns: + ``(text, hit_eof)`` -- the collected text with trailing blank lines + stripped, and whether the read ended at EOF rather than at the + sentinel. Callers need the distinction because an EOF that yielded no + text is a deliberate dismissal (Ctrl-D at an empty prompt), whereas + the sentinel with no text is merely an empty submission. + """ + console.print( + styled( + " [dim]Enter your answer. Finish with '{}' on its own line (or {}).[/dim]", + sentinel, + _eof_key_hint(), + ) + ) + lines: list[str] = [] + hit_eof = False + while True: + try: + line = input() + except (EOFError, StopIteration): + # StopIteration only ever arises from a test double's exhausted + # ``side_effect`` list (real ``input()`` never raises it) -- + # treated the same as EOF: submit what has been accumulated. + hit_eof = True + break + if line.strip() == sentinel: + break + lines.append(line) + return "\n".join(lines).rstrip("\n"), hit_eof + + async def read_on_daemon_thread[T](fn: Callable[[], T]) -> T: """Run a blocking stdin read on a daemon thread and await its result. @@ -477,30 +527,15 @@ def _ask_value() -> str: def _read_multiline(self) -> str: """Read a multi-line answer from stdin (blocking; call in a thread). - Terminates on a line containing only ``.`` or on EOF. The sentinel is - listed first in the hint because Ctrl-D/Ctrl-Z differs by platform. + Delegates to the shared :func:`read_multiline_lines` with this gate's + historical ``.`` sentinel, so behavior is unchanged by the extraction. Returns: The collected text with trailing blank lines stripped. Internal newlines are preserved. """ - self.console.print( - styled( - " [dim]Enter your answer. Finish with '{}' on its own line (or {}).[/dim]", - MULTILINE_SENTINEL, - _eof_key_hint(), - ) - ) - lines: list[str] = [] - while True: - try: - line = input() - except EOFError: - break - if line.strip() == MULTILINE_SENTINEL: - break - lines.append(line) - return "\n".join(lines).rstrip("\n") + text, _ = read_multiline_lines(self.console, MULTILINE_SENTINEL) + return text @dataclass diff --git a/tests/test_gates/test_dialog.py b/tests/test_gates/test_dialog.py index 4b89e8c6..d2ed433a 100644 --- a/tests/test_gates/test_dialog.py +++ b/tests/test_gates/test_dialog.py @@ -9,6 +9,7 @@ from conductor.config.schema import AgentDef, DialogConfig from conductor.gates.dialog import DialogHandler, DialogResult +from conductor.gates.human import DIALOG_SUBMIT_SENTINEL class TestDialogHandlerSkip: @@ -728,3 +729,163 @@ def test_console_panel_renders_non_ascii_unescaped(self) -> None: assert any("你好 мир" in body for body in markdown_bodies) assert all("\\u4f60" not in body for body in markdown_bodies) assert all("\\u043f" not in body for body in markdown_bodies) + + +class TestDialogMultilineInput: + """Terminal dialog turns must accept pasted multi-line blocks.""" + + @pytest.mark.asyncio + async def test_pasted_block_is_one_user_prompt_with_newlines(self) -> None: + """A pasted block is ingested as ONE prompt with internal newlines intact.""" + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + patch( + "builtins.input", + side_effect=["line one", "line two", "line three", "/send", "done"], + ), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + user_msgs = [m for m in result.messages if m.role == "user"] + # Exactly one paste ingested as a single prompt, both newlines intact: + assert user_msgs[0].content == "line one\nline two\nline three" + provider.execute_dialog_turn.assert_awaited_once() + + @pytest.mark.asyncio + async def test_eof_mid_paste_submits_content_not_dismissal(self) -> None: + """EOF (Ctrl-D) mid-paste dispatches accumulated content, not dismissal.""" + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + # Paste, then Ctrl-D (EOF) instead of /send; then a real dismissal. + patch("builtins.input", side_effect=["ticket text", EOFError(), "done"]), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + user_msgs = [m for m in result.messages if m.role == "user"] + assert user_msgs[0].content == "ticket text" # not dropped as None + provider.execute_dialog_turn.assert_awaited() # the turn WAS dispatched + + @pytest.mark.parametrize("isatty", [True, False]) + def test_opening_banner_advertises_the_sentinel_only_on_a_tty(self, isatty: bool) -> None: + """The banner names the sentinel exactly when a turn requires it. + + Off a tty the turn falls back to the single-line ``Prompt.ask`` branch, + where the sentinel does nothing -- so advertising it there would tell + the user to type something with no effect. + + Rendered for real, and asserted against the constant rather than a + literal, so the banner cannot drift from DIALOG_SUBMIT_SENTINEL. + """ + import io + + from conductor.console import make_console + + buf = io.StringIO() + handler = DialogHandler(console=make_console(file=buf, width=300, no_color=True)) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + + with patch("conductor.gates.dialog.sys.stdin.isatty", return_value=isatty): + handler._display_dialog_start(agent, {"out": 1}, "question?") + + rendered = "".join(buf.getvalue().split()) + needle = "".join(DIALOG_SUBMIT_SENTINEL.split()) + assert (needle in rendered) is isatty, rendered + # The markup must be parsed, not inserted verbatim as a value. + assert "[bold]" not in rendered, rendered + + @pytest.mark.asyncio + async def test_ctrl_d_at_empty_prompt_dismisses(self) -> None: + """A deliberate Ctrl-D with nothing typed ends the dialog. + + Regression: the multi-line reader converts EOF into a returned string, + so an empty read must not be fed back round the loop -- otherwise the + dismissal branch is unreachable and the dialog cannot be exited. + """ + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + # A bounded list: if the loop spins, input() raises StopIteration + # rather than hanging the suite. + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + patch("builtins.input", side_effect=EOFError()), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + assert result.user_dismissed is True + assert [m for m in result.messages if m.role == "user"] == [] + provider.execute_dialog_turn.assert_not_awaited() + + @pytest.mark.asyncio + async def test_ctrl_c_dismisses_rather_than_propagating(self) -> None: + """KeyboardInterrupt on the tty turn dismisses, as on the single-line path.""" + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + patch("builtins.input", side_effect=KeyboardInterrupt()), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + assert result.user_dismissed is True + + @pytest.mark.asyncio + async def test_web_path_unaffected_by_multiline(self) -> None: + """The web seam still passes whole messages through, never via the new reader.""" + dashboard = MagicMock() + dashboard.wait_for_dialog_message = AsyncMock( + side_effect=[ + {"type": "dialog_message", "agent_name": "test", "content": "a\nb\nc"}, + {"type": "dialog_decline", "agent_name": "test"}, + ] + ) + handler = DialogHandler(console=MagicMock(), web_dashboard=dashboard) + agent = AgentDef(name="test", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + + with patch( + "conductor.gates.dialog.read_multiline_lines", + side_effect=AssertionError("must not be called on the web path"), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + + user_msgs = [m for m in result.messages if m.role == "user"] + assert user_msgs[0].content == "a\nb\nc" + assert result.user_dismissed is True diff --git a/tests/test_gates/test_human.py b/tests/test_gates/test_human.py index c198239c..82e22613 100644 --- a/tests/test_gates/test_human.py +++ b/tests/test_gates/test_human.py @@ -9,7 +9,7 @@ from conductor.config.schema import AgentDef, GateOption from conductor.exceptions import HumanGateError -from conductor.gates.human import GateResult, HumanGateHandler +from conductor.gates.human import GateResult, HumanGateHandler, read_multiline_lines @pytest.fixture @@ -752,3 +752,48 @@ async def test_returns_the_value(self) -> None: from conductor.gates.human import read_on_daemon_thread assert await asyncio.wait_for(read_on_daemon_thread(lambda: "ok"), timeout=5) == "ok" + + +class TestReadMultilineLines: + """Regression tests for the extracted module-level multi-line reader.""" + + def test_read_multiline_lines_preserves_internal_newlines(self) -> None: + """Extracting the helper must not change behavior or drop newlines.""" + with patch( + "builtins.input", + side_effect=["line one", "line two", "line three", "."], + ): + result, hit_eof = read_multiline_lines(MagicMock()) + + assert result == "line one\nline two\nline three" + assert hit_eof is False + + def test_read_multiline_lines_custom_sentinel(self) -> None: + """A lone '.' is not a submit under a custom sentinel.""" + with patch("builtins.input", side_effect=[".", "still going", "/send"]): + result, hit_eof = read_multiline_lines(MagicMock(), sentinel="/send") + + assert result == ".\nstill going" + assert hit_eof is False + + def test_read_multiline_lines_eof_returns_accumulated(self) -> None: + """EOF submits accumulated content, not empty.""" + with patch("builtins.input", side_effect=["a", "b", EOFError()]): + result, hit_eof = read_multiline_lines(MagicMock()) + + assert result == "a\nb" + assert hit_eof is True + + def test_read_multiline_lines_reports_eof_on_empty_read(self) -> None: + """A bare EOF is distinguishable from an empty sentinel submission. + + The dialog gate relies on this to tell a deliberate Ctrl-D (dismiss) + from a sentinel typed with nothing above it (not a turn). + """ + with patch("builtins.input", side_effect=EOFError()): + text, hit_eof = read_multiline_lines(MagicMock()) + assert (text, hit_eof) == ("", True) + + with patch("builtins.input", side_effect=["."]): + text, hit_eof = read_multiline_lines(MagicMock()) + assert (text, hit_eof) == ("", False) From 84e155d2a08bc26c046d095b73a3a355ee67b16e Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Tue, 8 Sep 2026 18:52:54 +0200 Subject: [PATCH 2/4] fix(gates): compare stripped text in the dialog submission guards The multi-line reader drops trailing newlines but deliberately keeps a whitespace-only line, since a real strip would eat the closing indentation of a pasted code block. Both new guards compared exactly against "", so a buffer of spaces or tabs survived as a truthy string and slipped past both. Two consequences, the second worse than the first. A whitespace-only submission was dispatched to the model as a turn and spliced into the agent's re-execution guidance. And an EOF with only whitespace typed did not reach the "user is leaving, not pasting" branch, so Ctrl-D submitted the whitespace as a turn and *then* dismissed -- the user asked to leave and sent a message instead. Nothing was logged on either path. Both guards now test stripped text, matching _is_dismiss, which already normalises this way and was the outlier's neighbour in the same file. Addresses a blocking review finding. --- src/conductor/gates/dialog.py | 12 ++++---- tests/test_gates/test_dialog.py | 53 +++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/src/conductor/gates/dialog.py b/src/conductor/gates/dialog.py index ef38c44d..478459e9 100644 --- a/src/conductor/gates/dialog.py +++ b/src/conductor/gates/dialog.py @@ -279,9 +279,10 @@ async def handle_dialog( result.user_dismissed = True break - if user_input == "": - # User submitted nothing on a tty (e.g. an accidental bare - # sentinel line) -- not a turn, and not dismissal either. + if not user_input.strip(): + # Empty submission -- not a turn, and not dismissal either. On + # a tty this is a bare sentinel line; off a tty it is a blank + # line from the pipe, which ``Prompt.ask`` returns as "". continue result.messages.append(DialogMessage(role="user", content=user_input)) @@ -763,7 +764,8 @@ async def _get_user_input( dismissal. The main turn (``prompt_text is None`` on a tty) reads multi-line, so an EOF that *terminates a paste* returns the accumulated content rather than dismissing; an EOF with nothing - accumulated is a deliberate Ctrl-D and still returns None. + but whitespace accumulated is a deliberate Ctrl-D and still + returns None. """ if prompt_text is None and sys.stdin.isatty(): self.console.print(styled("[bold magenta]You[/bold magenta]")) @@ -773,7 +775,7 @@ async def _get_user_input( ) except (EOFError, KeyboardInterrupt): return None - if hit_eof and not text: + if hit_eof and not text.strip(): # Ctrl-D at an empty prompt: the user is leaving, not pasting. return None return text diff --git a/tests/test_gates/test_dialog.py b/tests/test_gates/test_dialog.py index d2ed433a..e8f6ea4a 100644 --- a/tests/test_gates/test_dialog.py +++ b/tests/test_gates/test_dialog.py @@ -889,3 +889,56 @@ async def test_web_path_unaffected_by_multiline(self) -> None: user_msgs = [m for m in result.messages if m.role == "user"] assert user_msgs[0].content == "a\nb\nc" assert result.user_dismissed is True + + @pytest.mark.asyncio + async def test_whitespace_only_submission_is_not_a_turn(self) -> None: + """Whitespace must not slip past the empty-submission guard. + + The reader strips trailing newlines, not whitespace, so a buffer of + spaces survives as a truthy string. Dispatched, it would re-run the + agent believing the user replied with whitespace. + """ + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + patch("builtins.input", side_effect=[" ", "/send", "done", EOFError()]), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + assert [m.content for m in result.messages if m.role == "user"] == ["done"] + provider.execute_dialog_turn.assert_not_awaited() + + @pytest.mark.asyncio + async def test_ctrl_d_after_whitespace_dismisses_without_submitting(self) -> None: + """Ctrl-D means "I am leaving", even with whitespace in the buffer. + + Without a stripping guard the whitespace is dispatched as a turn and + the dialog dismisses afterwards -- the user asked to leave and sent a + message instead. + """ + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + patch("builtins.input", side_effect=[" ", EOFError()]), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + provider.execute_dialog_turn.assert_not_awaited() + assert [m for m in result.messages if m.role == "user"] == [] + assert result.user_dismissed is True From 1114e34db68a50511803d494d592a807a59c177e Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Tue, 8 Sep 2026 18:53:23 +0200 Subject: [PATCH 3/4] test(gates): cover the reader gate, and tighten the reader's contract The condition choosing between the multi-line and single-line readers had no test in either direction: deleting the isatty() half, or the `prompt_text is None` half, left the whole suite green. The first would activate the multi-line reader under a pipe or in CI, waiting for a sentinel nobody can type; the second would make the yes/no confirmation demand /send after "yes" on every interactive run. Both halves now have a test, and the Prompt.ask fallback -- previously never executed by any test -- is exercised. The rule itself moves into a `_reads_multiline_turn()` predicate so the reader and the banner's sentinel hint consult one source, rather than two isatty() calls kept in agreement by a comment. The banner's hint is pre-rendered as a Text and spliced into a fixed-arity template: styled() raises IndexError on a mismatch and silently drops an extra argument, and the arity was previously maintained by hand across a `+`-concatenated fragment and its args tuple, on a branch no test reached. On a tty the rendered output is byte-identical to before, ANSI spans included. Off a tty the sentence regains the plural it has upstream ("Type your responses below."), which the previous revision had silently made singular on the one path this series leaves alone; that banner is now byte-identical to upstream, and a test pins the wording, since a byte comparison was otherwise the only thing that would catch it drifting again. Requiring the sentinel to submit also made the terminal UI's own exit instruction inert: the banner said "Say done or /done when finished", but on a tty a dismiss keyword is only seen once the turn is submitted, so `done` alone left the user at a prompt that never responded -- driving the real handler with input() returning "done" every time, the dialog never exited in 30 reads. The failure-recovery notice repeated the same premise, on the one screen where the user most needs a reliable way out. `_dismiss_instruction()` now states that rule once beside `_reads_multiline_turn()`, both sites render it, and both directions are pinned. Off a tty the sentence is unchanged from upstream, since every line is already a turn there. Also tightens the extracted reader: - `sentinel` is keyword-only and required. It is public, the two gates use different sentinels, and `read_multiline_lines(console)` was silently valid -- it would truncate a user's prose at any lone "." with no signal. - Only `except EOFError` remains. The extraction had also caught StopIteration, which a real stdin never produces here -- an exhausted or non-tty stream raises EOFError and a closed one ValueError -- so the catch was wider than any reachable input, and it let a test double read past what it supplied and still pass as a clean submission. The reason it is not caught is recorded at the clause, and a test pins it, so it is not reinstated as an oversight. - The sentinel's whitespace tolerance and the reader's trailing-line handling are pinned; both survived mutation before. So is the keyword-only signature itself, since restoring one default silently re-opens the hazard and nothing else would fail. `test_ctrl_d_at_empty_prompt_dismisses` now uses a bounded EOF source. A bare `side_effect=EOFError()` re-raises forever, so deleting the dismissal branch hung the suite instead of failing it, and there is no pytest-timeout configured -- a hung CI job rather than a readable failure. That test also pins `read_on_daemon_thread` as the dispatch: a cancelled asyncio.to_thread leaves its worker blocked in input() holding a slot in the shared default executor, which that function's own docstring explains at length. A pasted block's indentation is pinned end to end, leading edge included: the guards strip only to decide whether anything was submitted, while the text itself must reach the provider verbatim, and adding a strip to the returned turn text previously passed the whole suite while silently reindenting a pasted code block. Every guard in this series is mutation-tested: reverting any of them fails at least one test. Addresses a blocking review finding plus seven recommended ones. --- src/conductor/gates/dialog.py | 83 ++++++++--- src/conductor/gates/human.py | 40 ++--- tests/test_gates/test_dialog.py | 256 ++++++++++++++++++++++++++++++-- tests/test_gates/test_human.py | 85 ++++++++++- 4 files changed, 410 insertions(+), 54 deletions(-) diff --git a/src/conductor/gates/dialog.py b/src/conductor/gates/dialog.py index 478459e9..acc8e820 100644 --- a/src/conductor/gates/dialog.py +++ b/src/conductor/gates/dialog.py @@ -87,6 +87,38 @@ _READY_MARKER = "[READY_TO_CONTINUE]" +def _reads_multiline_turn() -> bool: + """Whether a conversational dialog turn is read multi-line. + + Off a tty the turn falls back to a single-line ``Prompt.ask``, where the + submit sentinel has no effect. Both the reader in + ``DialogHandler._get_user_input`` and the opening banner's sentinel hint + ask this, so the banner can never advertise a keystroke the reader ignores. + """ + return sys.stdin.isatty() + + +def _dismiss_instruction() -> Text: + """How to actually leave the dialog, phrased for the active reader. + + A dismiss keyword is only recognised once the turn is *submitted*, so on a + tty it needs the sentinel after it. Saying "type done" there describes a + keystroke that does nothing until the reply is sent -- the exit instruction + has to move with :func:`_reads_multiline_turn` or it contradicts the very + banner that advertises the sentinel. + + Returns: + A pre-styled ``Text``, so a caller splices it through ``styled`` and + keeps its own template a literal (#406). + """ + if _reads_multiline_turn(): + return styled( + "send [bold]done[/bold] or [bold]/done[/bold] with [bold]{}[/bold]", + DIALOG_SUBMIT_SENTINEL, + ) + return Text.from_markup("say [bold]done[/bold] or [bold]/done[/bold]") + + def _extract_ready_marker(response: str) -> tuple[bool, str]: """Return ``(proposed, cleaned)`` for an agent response. @@ -321,9 +353,9 @@ async def handle_dialog( exc_info=True, ) self.console.print( - Text.from_markup( - "[dim red] (Agent response failed — you can continue " - "or type 'done')[/dim red]" + styled( + "[dim red] (Agent response failed — you can continue, or {})[/dim red]", + _dismiss_instruction(), ) ) continue @@ -612,19 +644,30 @@ def _display_dialog_start( base_dir: Path | None = None, ) -> None: """Display the dialog opening with full agent context.""" - # Gated on the same condition as the multi-line reader in - # _get_user_input: off a tty, that turn falls back to a single-line - # Prompt.ask and the sentinel does nothing, so advertising it would - # instruct the user to type something with no effect. The markup stays - # in the template because styled() inserts *values* verbatim. - if sys.stdin.isatty(): - multiline_hint = ( - " It can span multiple lines; send it with [bold]{}[/bold] on its own line." + # Advertised only where the sentinel applies -- see + # _reads_multiline_turn. Pre-rendered as a Text so the outer template + # has fixed arity: styled() splices a Text in with its own spans + # re-anchored, and raises IndexError on a template/argument mismatch + # that only the tty branch would reach. Off a tty the sentence keeps + # its original plural, since each line really is a separate response + # there. + instruction = ( + styled( + "Type your response below. It can span multiple lines; send it" + " with [bold]{}[/bold] on its own line. A dismiss keyword ends" + " the dialog the same way: send [bold]done[/bold] or" + " [bold]/done[/bold] with [bold]{}[/bold].", + DIALOG_SUBMIT_SENTINEL, + DIALOG_SUBMIT_SENTINEL, + ) + if _reads_multiline_turn() + # Byte-identical to upstream's sentence: off a tty every line is a + # turn already, so a dismiss keyword needs nothing after it. + else Text.from_markup( + "Type your responses below. Say [bold]done[/bold] or " + "[bold]/done[/bold] when finished." ) - hint_args: tuple[object, ...] = (DIALOG_SUBMIT_SENTINEL,) - else: - multiline_hint = "" - hint_args = () + ) self.console.print() self.console.print( @@ -632,11 +675,9 @@ def _display_dialog_start( styled( "[bold]Agent '{}'[/bold] would like to discuss " "its output with you.\n" - "[dim]Type your response below." + multiline_hint + " " - "Say [bold]done[/bold] or [bold]/done[/bold] when " - "finished.[/dim]", + "[dim]{}[/dim]", agent.name, - *hint_args, + instruction, ), title=Text.from_markup("[bold magenta]Dialog Mode[/bold magenta]"), border_style="magenta", @@ -767,11 +808,11 @@ async def _get_user_input( but whitespace accumulated is a deliberate Ctrl-D and still returns None. """ - if prompt_text is None and sys.stdin.isatty(): + if prompt_text is None and _reads_multiline_turn(): self.console.print(styled("[bold magenta]You[/bold magenta]")) try: text, hit_eof = await read_on_daemon_thread( - lambda: read_multiline_lines(self.console, DIALOG_SUBMIT_SENTINEL) + lambda: read_multiline_lines(self.console, sentinel=DIALOG_SUBMIT_SENTINEL) ) except (EOFError, KeyboardInterrupt): return None diff --git a/src/conductor/gates/human.py b/src/conductor/gates/human.py index 28f77ae8..8e9cbf35 100644 --- a/src/conductor/gates/human.py +++ b/src/conductor/gates/human.py @@ -45,24 +45,26 @@ def _eof_key_hint() -> str: return "Ctrl-Z then Enter" if sys.platform == "win32" else "Ctrl-D" -def read_multiline_lines( - console: MarkupFreeConsole, sentinel: str = MULTILINE_SENTINEL -) -> tuple[str, bool]: +def read_multiline_lines(console: MarkupFreeConsole, *, sentinel: str) -> tuple[str, bool]: """Read a multi-line answer from stdin (blocking; call on a thread). Terminates on a line whose stripped text equals ``sentinel`` or on EOF. - Internal newlines are preserved; trailing blank lines are stripped. + Internal newlines are preserved; trailing empty lines are dropped, but a + trailing line of whitespace is kept verbatim -- a real strip would eat the + meaningful indentation of a pasted code block. Args: console: Console to print the input hint to. sentinel: Line that, typed alone, submits the accumulated text. + Keyword-only and required: the two gates use different sentinels, + so a caller states which one it means. Returns: - ``(text, hit_eof)`` -- the collected text with trailing blank lines - stripped, and whether the read ended at EOF rather than at the - sentinel. Callers need the distinction because an EOF that yielded no - text is a deliberate dismissal (Ctrl-D at an empty prompt), whereas - the sentinel with no text is merely an empty submission. + ``(text, hit_eof)`` -- the collected text and whether the read ended at + EOF rather than at the sentinel. Callers need the distinction because + an EOF that yielded no text is a deliberate dismissal (Ctrl-D at an + empty prompt), whereas the sentinel with no text is merely an empty + submission. """ console.print( styled( @@ -76,10 +78,14 @@ def read_multiline_lines( while True: try: line = input() - except (EOFError, StopIteration): - # StopIteration only ever arises from a test double's exhausted - # ``side_effect`` list (real ``input()`` never raises it) -- - # treated the same as EOF: submit what has been accumulated. + except EOFError: + # The only end-of-input a real stdin produces here: an exhausted + # or non-tty stream raises EOFError, a closed one ValueError. + # StopIteration is deliberately *not* caught -- input() would only + # relay it from a contrived stdin replacement, and treating it as + # EOF would submit a truncated turn as if the user had pressed + # Ctrl-D. A test double that runs past what it supplied is a bug + # in the test, so it must surface rather than read as a dismissal. hit_eof = True break if line.strip() == sentinel: @@ -528,13 +534,13 @@ def _read_multiline(self) -> str: """Read a multi-line answer from stdin (blocking; call in a thread). Delegates to the shared :func:`read_multiline_lines` with this gate's - historical ``.`` sentinel, so behavior is unchanged by the extraction. + historical ``.`` sentinel. Returns: - The collected text with trailing blank lines stripped. Internal - newlines are preserved. + The collected text, with trailing empty lines dropped and internal + newlines preserved. """ - text, _ = read_multiline_lines(self.console, MULTILINE_SENTINEL) + text, _ = read_multiline_lines(self.console, sentinel=MULTILINE_SENTINEL) return text diff --git a/tests/test_gates/test_dialog.py b/tests/test_gates/test_dialog.py index e8f6ea4a..9e2c8c27 100644 --- a/tests/test_gates/test_dialog.py +++ b/tests/test_gates/test_dialog.py @@ -2,14 +2,20 @@ from __future__ import annotations +import itertools from typing import Any from unittest.mock import AsyncMock, MagicMock, patch import pytest from conductor.config.schema import AgentDef, DialogConfig +from conductor.console import styled from conductor.gates.dialog import DialogHandler, DialogResult -from conductor.gates.human import DIALOG_SUBMIT_SENTINEL +from conductor.gates.human import ( + DIALOG_SUBMIT_SENTINEL, + read_multiline_lines, + read_on_daemon_thread, +) class TestDialogHandlerSkip: @@ -746,7 +752,7 @@ async def test_pasted_block_is_one_user_prompt_with_newlines(self) -> None: patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), patch( "builtins.input", - side_effect=["line one", "line two", "line three", "/send", "done"], + side_effect=["line one", "line two", "line three", "/send", "done", EOFError()], ), ): result = await handler.handle_dialog( @@ -771,7 +777,7 @@ async def test_eof_mid_paste_submits_content_not_dismissal(self) -> None: patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), # Paste, then Ctrl-D (EOF) instead of /send; then a real dismissal. - patch("builtins.input", side_effect=["ticket text", EOFError(), "done"]), + patch("builtins.input", side_effect=["ticket text", EOFError(), "done", EOFError()]), ): result = await handler.handle_dialog( agent=agent, @@ -779,9 +785,13 @@ async def test_eof_mid_paste_submits_content_not_dismissal(self) -> None: opening_question="Q?", provider=provider, ) - user_msgs = [m for m in result.messages if m.role == "user"] - assert user_msgs[0].content == "ticket text" # not dropped as None - provider.execute_dialog_turn.assert_awaited() # the turn WAS dispatched + # The paste-terminating EOF submitted its content and did NOT dismiss -- + # the dialog went on to accept a further turn, which is what ended it. + assert [m.content for m in result.messages if m.role == "user"] == [ + "ticket text", + "done", + ] + provider.execute_dialog_turn.assert_awaited_once() @pytest.mark.parametrize("isatty", [True, False]) def test_opening_banner_advertises_the_sentinel_only_on_a_tty(self, isatty: bool) -> None: @@ -810,6 +820,20 @@ def test_opening_banner_advertises_the_sentinel_only_on_a_tty(self, isatty: bool assert (needle in rendered) is isatty, rendered # The markup must be parsed, not inserted verbatim as a value. assert "[bold]" not in rendered, rendered + # Off a tty the sentence keeps its original plural, since each line + # really is a separate response there. Asserted because this wording + # has already drifted to the singular once, and only a byte comparison + # against the unmodified banner would otherwise have caught it. + expected = "Typeyourresponsesbelow." if not isatty else "Typeyourresponsebelow." + assert expected in rendered, rendered + # The exit instruction has to describe the *active* reader. On a tty a + # dismiss keyword is only seen once the turn is submitted, so telling + # the user to "say done" there names a keystroke that does nothing. + if isatty: + assert "senddoneor/donewith/send" in rendered, rendered + assert "Saydoneor/donewhenfinished." not in rendered, rendered + else: + assert "Saydoneor/donewhenfinished." in rendered, rendered @pytest.mark.asyncio async def test_ctrl_d_at_empty_prompt_dismisses(self) -> None: @@ -823,12 +847,28 @@ async def test_ctrl_d_at_empty_prompt_dismisses(self) -> None: agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) provider = MagicMock() provider.execute_dialog_turn = AsyncMock(return_value="ack") - # A bounded list: if the loop spins, input() raises StopIteration - # rather than hanging the suite. + + # A *bounded* EOF source. ``side_effect=EOFError()`` re-raises forever, + # so dropping the dismissal branch would spin this loop and hang the + # suite rather than fail it -- there is no pytest-timeout configured. + calls = itertools.count() + + def _eof_but_bounded(*_args: object, **_kwargs: object) -> str: + assert next(calls) < 10, "dialog loop spun on an empty EOF read" + raise EOFError + with ( patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), - patch("builtins.input", side_effect=EOFError()), + patch("builtins.input", side_effect=_eof_but_bounded), + patch( + "conductor.gates.dialog.read_multiline_lines", + wraps=read_multiline_lines, + ) as reader, + patch( + "conductor.gates.dialog.read_on_daemon_thread", + wraps=read_on_daemon_thread, + ) as dispatch, ): result = await handler.handle_dialog( agent=agent, @@ -839,10 +879,24 @@ async def test_ctrl_d_at_empty_prompt_dismisses(self) -> None: assert result.user_dismissed is True assert [m for m in result.messages if m.role == "user"] == [] provider.execute_dialog_turn.assert_not_awaited() + # Pins *this* reader, not merely "some reader dismissed on EOF". + reader.assert_called_once() + # And pins the dispatch: a cancelled ``asyncio.to_thread`` leaves its + # worker blocked in ``input()`` holding a slot in the shared default + # executor, which eventually deadlocks unrelated ``to_thread`` calls -- + # see ``read_on_daemon_thread``'s own docstring. + dispatch.assert_called_once() @pytest.mark.asyncio - async def test_ctrl_c_dismisses_rather_than_propagating(self) -> None: - """KeyboardInterrupt on the tty turn dismisses, as on the single-line path.""" + async def test_reader_exception_dismisses_rather_than_crashing_the_dialog(self) -> None: + """An exception out of the reader dismisses instead of escaping. + + This does **not** cover Ctrl-C. CPython runs signal handlers on the + main thread only, and the read happens on a daemon thread, so a real + SIGINT never reaches this ``except``: asyncio cancels the main task and + ``KeyboardInterrupt`` tears the run down, as it does everywhere else. + What is covered is an exception the reader itself raises. + """ handler = DialogHandler(console=MagicMock()) agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) provider = MagicMock() @@ -890,6 +944,157 @@ async def test_web_path_unaffected_by_multiline(self) -> None: assert user_msgs[0].content == "a\nb\nc" assert result.user_dismissed is True + @pytest.mark.asyncio + async def test_non_tty_main_turn_uses_the_single_line_prompt(self) -> None: + """Off a tty the conversational turn stays on ``Prompt.ask``. + + Half of the reader gate: without the ``isatty()`` check the multi-line + reader would activate under a pipe or in CI, waiting for a sentinel + nobody can type. + """ + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=False), + patch( + "conductor.gates.dialog.Prompt.ask", + side_effect=["piped answer", "done"], + ) as ask, + patch("builtins.input", side_effect=AssertionError("must not read raw stdin")), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + assert ask.call_count == 2 + assert [m.content for m in result.messages if m.role == "user"] == [ + "piped answer", + "done", + ] + + @pytest.mark.asyncio + async def test_confirmation_prompt_stays_single_line_on_a_tty(self) -> None: + """A ``prompt_text`` question must not require the sentinel. + + The other half of the reader gate: without the ``prompt_text is None`` + check the yes/no confirmation would start demanding ``/send`` after + "yes" on every interactive run. + """ + handler = DialogHandler(console=MagicMock()) + with ( + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + patch("conductor.gates.dialog.Prompt.ask", return_value="yes") as ask, + patch("builtins.input", side_effect=AssertionError("must not read multi-line")), + ): + answer = await handler._get_user_input(prompt_text=styled("[bold]Continue?[/bold]")) + assert answer == "yes" + ask.assert_called_once() + + @pytest.mark.asyncio + async def test_bare_sentinel_is_skipped_and_the_loop_continues(self) -> None: + """An empty submission is neither a turn nor dismissal.""" + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + patch( + "builtins.input", + side_effect=["/send", "real turn", "/send", "done", "/send", EOFError()], + ), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + # No empty turn recorded, none dispatched, and the loop carried on to + # accept a real turn afterwards. + assert [m.content for m in result.messages if m.role == "user"] == [ + "real turn", + "done", + ] + provider.execute_dialog_turn.assert_awaited_once() + assert result.user_dismissed is True + + @pytest.mark.parametrize("isatty", [True, False]) + def test_failure_notice_names_a_working_exit(self, isatty: bool) -> None: + """The recovery notice must not name an inert keystroke either. + + It fires when a provider call has just failed -- the moment the user + most wants a reliable way out -- so it has to move with the reader the + same way the banner does. + """ + from conductor.gates.dialog import _dismiss_instruction + + with patch("conductor.gates.dialog.sys.stdin.isatty", return_value=isatty): + hint = _dismiss_instruction() + + assert ("with /send" in hint) is isatty, hint + + @pytest.mark.asyncio + async def test_dismiss_keyword_still_exits_a_tty_dialog(self) -> None: + """ "done" submitted with the sentinel ends the dialog. + + The banner promises this; without it the only exits from a tty dialog + would be Ctrl-D and whatever the agent decides. + """ + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + patch("builtins.input", side_effect=["done", "/send", EOFError()]), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + assert result.user_dismissed is True + provider.execute_dialog_turn.assert_not_awaited() + + @pytest.mark.asyncio + async def test_pasted_indentation_reaches_the_provider_intact(self) -> None: + """A pasted code block keeps its leading and interior whitespace. + + The empty-submission guards strip only to *decide* whether there is + anything to send; the text itself must go through verbatim. Stripping + it would silently reindent a pasted block, which is the data loss this + whole reader exists to prevent, and no other test covers the leading + edge of it. + """ + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + block = [" def f():", "", " return 1", " "] + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True), + patch("builtins.input", side_effect=[*block, "/send", "done", EOFError()]), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + expected = " def f():\n\n return 1\n " + assert [m.content for m in result.messages if m.role == "user"][0] == expected + assert provider.execute_dialog_turn.await_args.kwargs["user_message"] == expected + @pytest.mark.asyncio async def test_whitespace_only_submission_is_not_a_turn(self) -> None: """Whitespace must not slip past the empty-submission guard. @@ -942,3 +1147,32 @@ async def test_ctrl_d_after_whitespace_dismisses_without_submitting(self) -> Non provider.execute_dialog_turn.assert_not_awaited() assert [m for m in result.messages if m.role == "user"] == [] assert result.user_dismissed is True + + @pytest.mark.asyncio + @pytest.mark.parametrize("blank", ["", " "]) + async def test_blank_piped_line_is_skipped_off_a_tty(self, blank: str) -> None: + """The empty guard also covers the non-tty path. + + ``Prompt.ask`` returns "" for a blank line, which previously reached + the provider as an empty turn. The whitespace case is parametrised + because ``rich.prompt.PromptBase.process_response`` strips its result, + so in production a whitespace-only line already arrives as "" -- these + mocks bypass that, and the guard has to hold either way. + """ + handler = DialogHandler(console=MagicMock()) + agent = AgentDef(name="t", prompt="p", dialog=DialogConfig(trigger_prompt="t")) + provider = MagicMock() + provider.execute_dialog_turn = AsyncMock(return_value="ack") + with ( + patch.object(handler, "_ask_engagement", new_callable=AsyncMock, return_value="engage"), + patch("conductor.gates.dialog.sys.stdin.isatty", return_value=False), + patch("conductor.gates.dialog.Prompt.ask", side_effect=[blank, "done"]), + ): + result = await handler.handle_dialog( + agent=agent, + agent_output={"result": "x"}, + opening_question="Q?", + provider=provider, + ) + assert [m.content for m in result.messages if m.role == "user"] == ["done"] + provider.execute_dialog_turn.assert_not_awaited() diff --git a/tests/test_gates/test_human.py b/tests/test_gates/test_human.py index 82e22613..baf7d968 100644 --- a/tests/test_gates/test_human.py +++ b/tests/test_gates/test_human.py @@ -9,7 +9,13 @@ from conductor.config.schema import AgentDef, GateOption from conductor.exceptions import HumanGateError -from conductor.gates.human import GateResult, HumanGateHandler, read_multiline_lines +from conductor.gates.human import ( + DIALOG_SUBMIT_SENTINEL, + MULTILINE_SENTINEL, + GateResult, + HumanGateHandler, + read_multiline_lines, +) @pytest.fixture @@ -763,7 +769,7 @@ def test_read_multiline_lines_preserves_internal_newlines(self) -> None: "builtins.input", side_effect=["line one", "line two", "line three", "."], ): - result, hit_eof = read_multiline_lines(MagicMock()) + result, hit_eof = read_multiline_lines(MagicMock(), sentinel=MULTILINE_SENTINEL) assert result == "line one\nline two\nline three" assert hit_eof is False @@ -779,7 +785,7 @@ def test_read_multiline_lines_custom_sentinel(self) -> None: def test_read_multiline_lines_eof_returns_accumulated(self) -> None: """EOF submits accumulated content, not empty.""" with patch("builtins.input", side_effect=["a", "b", EOFError()]): - result, hit_eof = read_multiline_lines(MagicMock()) + result, hit_eof = read_multiline_lines(MagicMock(), sentinel=MULTILINE_SENTINEL) assert result == "a\nb" assert hit_eof is True @@ -791,9 +797,78 @@ def test_read_multiline_lines_reports_eof_on_empty_read(self) -> None: from a sentinel typed with nothing above it (not a turn). """ with patch("builtins.input", side_effect=EOFError()): - text, hit_eof = read_multiline_lines(MagicMock()) + text, hit_eof = read_multiline_lines(MagicMock(), sentinel=MULTILINE_SENTINEL) assert (text, hit_eof) == ("", True) with patch("builtins.input", side_effect=["."]): - text, hit_eof = read_multiline_lines(MagicMock()) + text, hit_eof = read_multiline_lines(MagicMock(), sentinel=MULTILINE_SENTINEL) assert (text, hit_eof) == ("", False) + + @pytest.mark.parametrize("sentinel", [MULTILINE_SENTINEL, DIALOG_SUBMIT_SENTINEL]) + def test_hint_names_the_sentinel_it_will_accept(self, sentinel: str) -> None: + """The per-turn hint is where the user learns how to submit. + + Each gate passes its own sentinel, so a hint rendered from anything + else would tell the user to type a line the reader ignores -- and + leave them at a prompt that never submits. + """ + console = MagicMock() + with patch("builtins.input", side_effect=[sentinel]): + read_multiline_lines(console, sentinel=sentinel) + + printed = console.print.call_args.args[0].plain + assert f"'{sentinel}'" in printed, printed + + def test_a_broken_stdin_is_not_read_as_a_dismissal(self) -> None: + """Only EOFError ends the read; anything else propagates. + + A stdin source raising something the loop swallowed would submit a + truncated turn as though the user had pressed Ctrl-D, with nothing + logged. StopIteration is the case that matters: it is what an + exhausted test double raises, so catching it would also let a double + read past what it supplied and still pass as a clean submission. + """ + with ( + patch("builtins.input", side_effect=StopIteration("broken source")), + pytest.raises(StopIteration), + ): + read_multiline_lines(MagicMock(), sentinel=MULTILINE_SENTINEL) + + def test_sentinel_must_be_passed_by_keyword(self) -> None: + """The sentinel is required, so neither gate can inherit the other's. + + A positional default made ``read_multiline_lines(console)`` silently + valid, which would truncate a dialog reply at any lone "." with no + signal at the call site. Pinned because the hazard is re-openable by + restoring one default and nothing else would fail. + """ + with pytest.raises(TypeError, match="sentinel"): + read_multiline_lines(MagicMock()) # ty: ignore[missing-argument] + + @pytest.mark.parametrize("typed", ["/send", " /send", "/send ", "\t/send "]) + def test_sentinel_tolerates_surrounding_whitespace(self, typed: str) -> None: + """A stray space around the sentinel still submits. + + Terminals and paste buffers add trailing whitespace routinely, and + without this the user would sit at a prompt that never submits. The + trailing ``"unreachable"`` proves the reader stopped at the sentinel + rather than merely running out of mock values. + """ + with patch("builtins.input", side_effect=["body", typed, "unreachable"]): + text, hit_eof = read_multiline_lines(MagicMock(), sentinel="/send") + + assert (text, hit_eof) == ("body", False) + + def test_trailing_whitespace_line_is_kept_verbatim(self) -> None: + """Trailing *empty* lines are dropped; a whitespace line is content. + + Pins the docstring's distinction: stripping it would eat the closing + indentation of a pasted code block. + """ + with patch("builtins.input", side_effect=["a", "", "", "/send"]): + text, _ = read_multiline_lines(MagicMock(), sentinel="/send") + assert text == "a" + + with patch("builtins.input", side_effect=["a", " ", "", "/send"]): + text, _ = read_multiline_lines(MagicMock(), sentinel="/send") + assert text == "a\n " From f3faeb87437f72e7e1597bcc63b2754f9515be3d Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Tue, 8 Sep 2026 19:56:09 +0200 Subject: [PATCH 4/4] docs(gates): correct three false claims about the dialog reader All three were verified wrong by execution, having been asserted as verified. "Ctrl-C dismisses rather than propagating" is removed. CPython runs signal handlers on the main thread only and the read happens on a daemon thread, so a real SIGINT never reaches that except clause: sending one to a child blocked in the reader on a pty raises CancelledError at the await and KeyboardInterrupt escapes asyncio.run. Ctrl-C tears the run down as it does everywhere else, and a reader who trusted this line would lose their session expecting to keep it. The except clause is correct for exceptions the reader itself raises and stays; its test is renamed and its docstring now says explicitly that it does not cover a real SIGINT. "Off a tty the single-line path is unchanged" is corrected. Prompt.ask returns "" for a blank line and the empty-submission guard sits above the tty branch, so a blank piped line is now skipped where it was previously dispatched as a turn with empty content. The new behaviour is right, but that path is the contract for anyone driving a dialog from CI, and it said nothing had changed. Grouping the web dashboard with "a pipe, CI" is corrected. It returns before either reader and has always received whole messages, so the old wording implied its chat box could not take a multi-line reply. Two things the prose did not say at all are now stated. A terminal accepts an EOF keystroke only at the start of a line, and its EOF does not persist -- the read returns and the terminal is readable again -- so Ctrl-D after entering a line now submits it and a second Ctrl-D is needed to leave, where one used to exit. On an empty prompt it still exits in one keystroke, so the habitual exit only changes once something has been entered, and that text is now sent rather than discarded. And the "trailing blank lines are stripped" overstatement is dropped: a whitespace-only trailing line is kept verbatim, which is deliberate -- stripping it would eat a pasted code block's closing indentation -- and is now pinned by a test. Addresses three recommended review findings. --- AGENTS.md | 4 ++-- CHANGELOG.md | 30 ++++++++++++++++++++---------- docs/workflow-syntax.md | 2 +- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8d908b16..a8d0c5f1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -158,8 +158,8 @@ step-by-step checklist. - `factory.py` - Provider instantiation - **gates/**: Human-in-the-loop support - - `human.py` - Rich terminal UI for human gate interactions. `read_multiline_lines(console, sentinel)` is the shared blocking multi-line stdin reader behind both the human gate's `.` sentinel (`MULTILINE_SENTINEL`) and the dialog gate's `/send` (`DIALOG_SUBMIT_SENTINEL`); it returns `(text, hit_eof)` because an EOF that yielded no text is a deliberate dismissal while the sentinel with no text is an empty submission. The two gates use different sentinels, so a caller passes the one it wants rather than relying on the default - - `dialog.py` - Dialog-mode gate. On a tty the main turn reads through `read_multiline_lines`, so a pasted block is one turn rather than one turn per line; off a tty it falls back to single-line `Prompt.ask`, where the sentinel has no effect — `_display_dialog_start` gates the sentinel hint on the same `sys.stdin.isatty()` condition so the banner never advertises a keystroke that does nothing + - `human.py` - Rich terminal UI for human gate interactions. `read_multiline_lines(console, sentinel)` is the shared blocking multi-line stdin reader behind both the human gate's `.` sentinel (`MULTILINE_SENTINEL`) and the dialog gate's `/send` (`DIALOG_SUBMIT_SENTINEL`); it returns `(text, hit_eof)` because an EOF that yielded no text is a deliberate dismissal while the sentinel with no text is an empty submission. `sentinel` is keyword-only and required, since the two gates use different ones + - `dialog.py` - Dialog-mode gate. On a tty the main turn reads through `read_multiline_lines`, so a pasted block is one turn rather than one turn per line; off a tty it falls back to single-line `Prompt.ask`, where the sentinel has no effect — the `_reads_multiline_turn()` predicate states that rule once, and `_display_dialog_start` gates its sentinel hint on it so the banner never advertises a keystroke the reader ignores. Both submission guards compare against stripped text: the reader drops trailing newlines but keeps a whitespace-only line, so an exact `== ""` check would dispatch whitespace as a turn and turn Ctrl-D into a submission - **interrupt/**: Interactive workflow interruption (Esc/Ctrl+G to pause) - `listener.py` - Keyboard listener daemon thread for Esc/Ctrl+G detection diff --git a/CHANGELOG.md b/CHANGELOG.md index a8b61e11..f57a9258 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,20 +33,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 dialog mode was the only free-text human-input surface that could not accept a multi-line answer (`QuestionDef.multiline` defaults to `True` and `GateOption.multiline` opts in, both served by one reader in `gates/human.py`). - The dialog gate read a reply with single-line `Prompt.ask`, so pasting a block of - text into an interactive terminal dispatched *each line* as its own turn: a + The dialog gate read a reply with single-line `Prompt.ask`, so pasting a block + of text into an interactive terminal dispatched *each line* as its own turn: a three-line paste became three separate questions to the model, each answered against a fragment, and the paste's trailing newline added a fourth turn with empty content. Terminal turns now read through the multi-line reader already behind the human gate's `.` sentinel, submitted with `/send` on its own line, - so internal newlines survive and a paste is a single message; an empty - submission is no longer dispatched as a turn. Ctrl-D (Ctrl-Z then Enter on - Windows) submits what has been typed and dismisses the dialog when nothing - has, and Ctrl-C dismisses rather than propagating. The dialog uses `/send` - where the human gate keeps `.`, since a lone `.` is likelier to be prose in - a conversational reply. Off a tty — a pipe, CI, or the web dashboard — the - single-line path is unchanged, and the opening banner advertises the sentinel - only where it applies. + so internal newlines survive and a paste is a single message. An empty or + whitespace-only submission is no longer dispatched as a turn. A dismiss + keyword is recognised only once a turn is submitted, so on a tty `done` now + needs `/send` after it, and both the opening banner and the failure-recovery + notice say so rather than naming a keystroke that does nothing there. + + Ctrl-D at the start of a line (Ctrl-Z then Enter on Windows) also submits the + lines entered so far, or dismisses the dialog when there are none. Because a + terminal's EOF does not persist, abandoning a part-written reply that way now + sends what was already entered and a second Ctrl-D is needed to leave, where + one used to exit; on an empty prompt it still exits in one keystroke. + + The dialog uses `/send` where the human gate keeps `.`, since a lone `.` is + likelier to be prose in a conversational reply. Off a tty — a pipe or CI — + replies are still read one line at a time and `/send` has no effect; the one + change on that path is that a blank line is now skipped instead of dispatched + as an empty turn. The web dashboard is unaffected: it takes a separate path + that already delivered each message whole. ## [0.1.36](https://github.com/microsoft/conductor/compare/v0.1.35...v0.1.36) - 2026-09-02 diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index a2efcf60..f9ef4031 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -1198,7 +1198,7 @@ After the conversation, the agent re-executes with the dialog transcript as addi **Behavior notes:** - Dialog is supported on regular `agent` type only (not `human_gate`, `questions`, `script`, `workflow`, or `wait`) -- In an interactive terminal, a reply may span multiple lines — paste or type freely and submit the turn with `/send` on its own line. Ctrl-D (Ctrl-Z then Enter on Windows) also submits whatever has been typed, or dismisses the dialog when nothing has. Off a tty (a pipe, CI, or the web dashboard) replies are read one line at a time and `/send` does not apply +- In an interactive terminal, a reply may span multiple lines — paste or type freely and submit the turn with `/send` on its own line. A dismiss keyword ends the dialog the same way, so `done` needs `/send` after it there; off a tty every line is already a turn, so it does not. Ctrl-D at the start of a line (Ctrl-Z then Enter on Windows) also submits whatever lines have been entered so far, or dismisses the dialog when none have — so abandoning a part-written reply that way sends the lines already entered; press it on an empty prompt to leave without sending. An empty or whitespace-only submission is skipped rather than sent. Off a tty (a pipe or CI) replies are read one line at a time and `/send` does not apply, though a blank line is skipped rather than sent as an empty turn. The web dashboard is unaffected — its chat box takes a separate path that has always delivered each message whole, multi-line included - In web dashboard mode, the dialog temporarily replaces the graph area with a chat interface - When `--skip-gates` is set (e.g., CI/automation), dialogs are automatically skipped - The evaluator prompt should describe *when* to trigger dialog, not *what* to ask — the evaluator generates the opening question from the agent's output context