-
Notifications
You must be signed in to change notification settings - Fork 62
fix(gates): read a terminal dialog reply as one multi-line turn #510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ae4d486
84e155d
1114e34
f3faeb8
98b7634
c6bd2fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,12 +32,68 @@ | |
| 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) -> 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 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 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: | ||
| # 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: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RECOMMENDED Mutating A trailing space after Worth flagging the flip side while you're adding coverage: @pytest.mark.parametrize("typed", ["/send", " /send", "/send ", "\t/send "])
def test_sentinel_tolerates_surrounding_whitespace(self, typed: str) -> None:
with patch("builtins.input", side_effect=["body", typed, "unreachable"]):
text, hit_eof = read_multiline_lines(MagicMock(), sentinel="/send")
assert (text, hit_eof) == ("body", False)The |
||
| 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 +533,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. | ||
|
|
||
| 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. | ||
| """ | ||
| 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, sentinel=MULTILINE_SENTINEL) | ||
| return text | ||
|
|
||
|
|
||
| @dataclass | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RECOMMENDED
This line has no test coverage at all —
--cov-report=term-missingflags it as missing, and deleting the wholeif user_input == "": continueblock leaves 138/138 tests passing acrosstests/test_gates,tests/test_engine/test_dialog_integration.py,tests/test_cli/test_markup_injection.py, andtests/test_engine/test_resume.py.So nothing actually pins the behavior the CHANGELOG advertises. Without this guard, a bare
/sendwith nothing typed above it appends an emptyDialogMessage, emits adialog_messageevent, and sends an empty turn to the provider — a real API call with no content. Neither half of what matters is verified: that the empty submission isn't dispatched, and that the loop keeps going so a real turn afterward still works.This fails against the unguarded code (an empty message shows up first and the provider gets awaited twice). Worth pairing with a non-tty variant for the blank-piped-line case from the CHANGELOG finding above.