diff --git a/docs/advanced/configuration.mdx b/docs/advanced/configuration.mdx index 5d56c9a16..499db4a52 100644 --- a/docs/advanced/configuration.mdx +++ b/docs/advanced/configuration.mdx @@ -83,6 +83,10 @@ When remote vars are set, Strix dual-writes telemetry to both local JSONL and th Maximum size (in MB) of a local directory target that Strix will copy into the sandbox file-by-file. Larger targets exit early with a suggestion to use `--mount` instead. Set to `0` to disable the check. + + Whether `exec_command` defaults `tty` to `true` when a call omits it (an explicit `tty: false` is always respected). Defaults to on for native Windows hosts, where Docker Desktop's named-pipe transport can silently return empty output for non-tty execs. Set to `1`/`true`/`t`/`yes`/`y`/`on` to force it on any platform, or `0`/`false`/`f`/`no`/`n`/`off` to disable it (including on Windows). + + ## Sandbox Configuration diff --git a/strix/agents/factory.py b/strix/agents/factory.py index 2b7de85e0..2df6c733f 100644 --- a/strix/agents/factory.py +++ b/strix/agents/factory.py @@ -5,7 +5,9 @@ import inspect import json import logging +import os import re +import sys from typing import TYPE_CHECKING, Any from agents.agent import ToolsToFinalOutputResult @@ -205,10 +207,63 @@ def _format_validation_error(tool_name: str, exc: ValidationError) -> str: return f"{tool_name}: invalid arguments — " + "; ".join(parts) +_EXEC_TTY_TRUE_STRINGS = frozenset({"1", "true", "t", "yes", "y", "on"}) +_EXEC_TTY_FALSE_STRINGS = frozenset({"0", "false", "f", "no", "n", "off"}) + + +def _should_force_exec_tty() -> bool: + """Whether ``exec_command`` should force ``tty: true`` (see ``_force_exec_tty``). + + Defaults to on for native Windows hosts. ``STRIX_EXEC_FORCE_TTY`` + overrides the auto-detection in either direction, for Windows setups + where forcing a PTY causes its own problems (e.g. pager/isatty-sensitive + commands) or for non-Windows setups that want the same workaround. + """ + override = (os.environ.get("STRIX_EXEC_FORCE_TTY") or "").strip().lower() + if override in _EXEC_TTY_TRUE_STRINGS: + return True + if override in _EXEC_TTY_FALSE_STRINGS: + return False + return sys.platform == "win32" + + +def _force_exec_tty(raw_input: str) -> str: + """Default an ``exec_command`` payload's ``tty`` to ``true`` when unset. + + On native Windows, docker-py talks to Docker Desktop over a named pipe + (``NpipeSocket``) rather than a POSIX socket. ``exec_command``'s + non-tty path demultiplexes stdout/stderr with a blocking 8-byte + frame-header read (``docker.utils.socket.frames_iter_no_tty``); over the + named-pipe transport this routinely returns no data within the tool's + yield window, so commands appear to hang with empty output (GH #727). + The tty path (``frames_iter_tty``) is a single raw read and unaffected. + Reproduced directly against the pinned SDK on Windows + Docker Desktop: + ``tty=True`` returns output instantly, ``tty=False`` (the schema + default) returns empty output with no exception. + + Only fills in ``tty`` when the key is absent — an explicit ``tty: false`` + is left alone, since some commands (e.g. ``git log``, ``git diff``) start + a pager or otherwise change behavior under a real TTY and must stay + non-interactive. ``exec_command`` is registered with + ``strict_json_schema=False``, so models routinely omit optional fields + they don't care about; that's the case this covers. + """ + try: + parsed = json.loads(raw_input) + except json.JSONDecodeError: + return raw_input + if not isinstance(parsed, dict): + return raw_input + parsed.setdefault("tty", True) + return json.dumps(parsed) + + def _wrap_exec_command(tool: FunctionTool) -> FunctionTool: invoke_tool = tool.on_invoke_tool async def invoke(ctx: Any, raw_input: str) -> Any: + if _should_force_exec_tty(): + raw_input = _force_exec_tty(raw_input) try: return await invoke_tool(ctx, raw_input) except ValidationError as exc: diff --git a/tests/test_exec_command_windows_tty.py b/tests/test_exec_command_windows_tty.py new file mode 100644 index 000000000..f0bd5dfd8 --- /dev/null +++ b/tests/test_exec_command_windows_tty.py @@ -0,0 +1,159 @@ +"""Tests for forcing tty=true on exec_command on Windows (GH issue #727). + +On native Windows, docker-py talks to Docker Desktop over a named pipe +(``NpipeSocket``). exec_command's non-tty path demultiplexes stdout/stderr +with a blocking 8-byte frame-header read (docker.utils.socket.frames_iter_no_tty); +over the named-pipe transport this routinely returns no data within the +tool's yield window, so commands hang with empty output. The tty path +(frames_iter_tty) is a single raw read and unaffected. Reproduced directly +against the pinned SDK on a real Windows 11 + Docker Desktop host: tty=True +returned real output instantly, tty=False (the exec_command default) +returned empty output with no exception. +""" + +from __future__ import annotations + +import json +import sys +from typing import Any, cast + +import pytest + +from strix.agents import factory + + +@pytest.fixture(autouse=True) +def _clear_force_tty_env(monkeypatch: pytest.MonkeyPatch) -> None: + """Deterministic tests must not inherit a dev machine's env override.""" + monkeypatch.delenv("STRIX_EXEC_FORCE_TTY", raising=False) + + +def _fake_tool(captured: dict[str, str]) -> Any: + async def on_invoke_tool(_ctx: Any, raw_input: str) -> str: + captured["raw_input"] = raw_input + return "ok" + + class _Tool: + name = "exec_command" + + tool = _Tool() + tool.on_invoke_tool = on_invoke_tool # type: ignore[attr-defined] + return tool + + +def test_force_exec_tty_injects_true_when_absent() -> None: + result = factory._force_exec_tty(json.dumps({"cmd": "ls"})) + assert json.loads(result) == {"cmd": "ls", "tty": True} + + +def test_force_exec_tty_respects_explicit_false() -> None: + result = factory._force_exec_tty(json.dumps({"cmd": "ls", "tty": False})) + assert json.loads(result)["tty"] is False + + +def test_force_exec_tty_leaves_malformed_json_untouched() -> None: + assert factory._force_exec_tty("not json") == "not json" + + +def test_force_exec_tty_leaves_non_dict_json_untouched() -> None: + raw = json.dumps(["cmd", "ls"]) + assert factory._force_exec_tty(raw) == raw + + +@pytest.mark.asyncio +async def test_wrap_exec_command_forces_tty_on_windows(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(sys, "platform", "win32") + captured: dict[str, str] = {} + wrapped = factory._wrap_exec_command(_fake_tool(captured)) + + result = await wrapped.on_invoke_tool(cast("Any", None), json.dumps({"cmd": "echo test"})) + + assert result == "ok" + assert json.loads(captured["raw_input"]) == {"cmd": "echo test", "tty": True} + + +@pytest.mark.asyncio +async def test_wrap_exec_command_does_not_force_tty_on_linux( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(sys, "platform", "linux") + captured: dict[str, str] = {} + wrapped = factory._wrap_exec_command(_fake_tool(captured)) + + await wrapped.on_invoke_tool(cast("Any", None), json.dumps({"cmd": "echo test"})) + + assert json.loads(captured["raw_input"]) == {"cmd": "echo test"} + + +@pytest.mark.asyncio +async def test_wrap_exec_command_respects_explicit_tty_false_on_linux( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(sys, "platform", "linux") + captured: dict[str, str] = {} + wrapped = factory._wrap_exec_command(_fake_tool(captured)) + + await wrapped.on_invoke_tool(cast("Any", None), json.dumps({"cmd": "echo test", "tty": False})) + + assert json.loads(captured["raw_input"])["tty"] is False + + +@pytest.mark.asyncio +async def test_wrap_exec_command_respects_explicit_tty_false_on_windows( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A model explicitly opting out of a TTY (e.g. for `git log`/`git diff`, + which start a pager under a real TTY) must not be overridden.""" + monkeypatch.setattr(sys, "platform", "win32") + captured: dict[str, str] = {} + wrapped = factory._wrap_exec_command(_fake_tool(captured)) + + await wrapped.on_invoke_tool(cast("Any", None), json.dumps({"cmd": "git log", "tty": False})) + + assert json.loads(captured["raw_input"])["tty"] is False + + +def test_should_force_exec_tty_defaults_true_on_windows( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(sys, "platform", "win32") + assert factory._should_force_exec_tty() is True + + +def test_should_force_exec_tty_defaults_false_on_linux( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(sys, "platform", "linux") + assert factory._should_force_exec_tty() is False + + +@pytest.mark.parametrize("value", ["1", "true", "TRUE", "t", "T", "yes", "y", "on"]) +def test_should_force_exec_tty_env_var_forces_on_non_windows( + monkeypatch: pytest.MonkeyPatch, value: str +) -> None: + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.setenv("STRIX_EXEC_FORCE_TTY", value) + assert factory._should_force_exec_tty() is True + + +@pytest.mark.parametrize("value", ["0", "false", "FALSE", "f", "F", "no", "n", "off"]) +def test_should_force_exec_tty_env_var_forces_off_on_windows( + monkeypatch: pytest.MonkeyPatch, value: str +) -> None: + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setenv("STRIX_EXEC_FORCE_TTY", value) + assert factory._should_force_exec_tty() is False + + +@pytest.mark.asyncio +async def test_wrap_exec_command_env_var_disables_force_on_windows( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setenv("STRIX_EXEC_FORCE_TTY", "0") + captured: dict[str, str] = {} + wrapped = factory._wrap_exec_command(_fake_tool(captured)) + + await wrapped.on_invoke_tool(cast("Any", None), json.dumps({"cmd": "echo test"})) + + assert json.loads(captured["raw_input"]) == {"cmd": "echo test"}