fix(agents): force tty for exec_command on Windows#739
Conversation
Greptile SummaryThis PR adds a Windows workaround for Docker exec output handling. The main changes are:
Confidence Score: 4/5The Windows exec wrapper needs a fix for commands that explicitly request non-TTY execution.
strix/agents/factory.py Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
strix/agents/factory.py:252
**Explicit Non-TTY Opt-Out Is Lost**
When a Windows caller sends `tty: false` for a command that must stay noninteractive, this line rewrites it to `true` before the SDK sees it. PTY-sensitive commands such as `git log` or `git diff` can then start a pager or change their output mode, leaving the command running instead of returning the expected output.
### Issue 2 of 2
strix/agents/factory.py:210-211
**Boolean Alias Falls Through**
This parser accepts fewer boolean aliases than the existing env-backed boolean settings. If a Windows user sets `STRIX_EXEC_FORCE_TTY=f` or `n` expecting the usual false value, the value is treated as unrecognized and falls back to the Windows default, so TTY forcing stays enabled.
```suggestion
_EXEC_TTY_TRUE_STRINGS = frozenset({"1", "true", "t", "yes", "y", "on"})
_EXEC_TTY_FALSE_STRINGS = frozenset({"0", "false", "f", "no", "n", "off"})
```
Reviews (1): Last reviewed commit: "fix(agents): force tty for exec_command ..." | Re-trigger Greptile |
| return raw_input | ||
| if not isinstance(parsed, dict): | ||
| return raw_input | ||
| parsed["tty"] = True |
There was a problem hiding this comment.
Explicit Non-TTY Opt-Out Is Lost
When a Windows caller sends tty: false for a command that must stay noninteractive, this line rewrites it to true before the SDK sees it. PTY-sensitive commands such as git log or git diff can then start a pager or change their output mode, leaving the command running instead of returning the expected output.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/agents/factory.py
Line: 252
Comment:
**Explicit Non-TTY Opt-Out Is Lost**
When a Windows caller sends `tty: false` for a command that must stay noninteractive, this line rewrites it to `true` before the SDK sees it. PTY-sensitive commands such as `git log` or `git diff` can then start a pager or change their output mode, leaving the command running instead of returning the expected output.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch, thanks. Fixed in d7b5255 — now only fills in tty when the key is absent (setdefault instead of unconditional overwrite), so an explicit tty: false from the model is respected. Added a regression test (git log + tty: false on Windows -> stays false). Confirmed exec_command is registered with strict_json_schema=False, so models do omit optional fields when they don't need them — that's the case the default-fill still covers.
| _EXEC_TTY_TRUE_STRINGS = frozenset({"1", "true", "yes", "on"}) | ||
| _EXEC_TTY_FALSE_STRINGS = frozenset({"0", "false", "no", "off"}) |
There was a problem hiding this comment.
This parser accepts fewer boolean aliases than the existing env-backed boolean settings. If a Windows user sets STRIX_EXEC_FORCE_TTY=f or n expecting the usual false value, the value is treated as unrecognized and falls back to the Windows default, so TTY forcing stays enabled.
| _EXEC_TTY_TRUE_STRINGS = frozenset({"1", "true", "yes", "on"}) | |
| _EXEC_TTY_FALSE_STRINGS = frozenset({"0", "false", "no", "off"}) | |
| _EXEC_TTY_TRUE_STRINGS = frozenset({"1", "true", "t", "yes", "y", "on"}) | |
| _EXEC_TTY_FALSE_STRINGS = frozenset({"0", "false", "f", "no", "n", "off"}) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/agents/factory.py
Line: 210-211
Comment:
**Boolean Alias Falls Through**
This parser accepts fewer boolean aliases than the existing env-backed boolean settings. If a Windows user sets `STRIX_EXEC_FORCE_TTY=f` or `n` expecting the usual false value, the value is treated as unrecognized and falls back to the Windows default, so TTY forcing stays enabled.
```suggestion
_EXEC_TTY_TRUE_STRINGS = frozenset({"1", "true", "t", "yes", "y", "on"})
_EXEC_TTY_FALSE_STRINGS = frozenset({"0", "false", "f", "no", "n", "off"})
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Leaving this one as-is. Checked the rest of the repo for precedent and the only other hand-rolled boolean env parsing (STRIX_DEBUG in strix/telemetry/logging.py) uses the same 4-string set (1/true/yes/on, no single-letter aliases), so I kept STRIX_EXEC_FORCE_TTY consistent with that rather than introducing a new convention.
Only set tty=true when the key is absent from the payload. Overriding an explicit tty:false broke commands that need to stay non-interactive (e.g. git log/git diff would start a pager and hang). exec_command is registered with strict_json_schema=False so models can genuinely omit optional fields, which is the case this still covers. Addresses Greptile review feedback on PR usestrix#739.
d7b5255 to
5e01a7f
Compare
Only set tty=true when the key is absent from the payload. Overriding an explicit tty:false broke commands that need to stay non-interactive (e.g. git log/git diff would start a pager and hang). exec_command is registered with strict_json_schema=False so models can genuinely omit optional fields, which is the case this still covers. Addresses Greptile review feedback on PR usestrix#739.
Matches Greptile review feedback on PR usestrix#739 (single-letter boolean aliases were previously unrecognized and silently fell back to the platform default).
44e87ca to
daf39a2
Compare
Non-tty exec hangs with empty output on Windows Docker Desktop (named-pipe transport issue in the pinned SDK's docker exec socket read). Force tty=true by default on win32; override via STRIX_EXEC_FORCE_TTY.
Only set tty=true when the key is absent from the payload. Overriding an explicit tty:false broke commands that need to stay non-interactive (e.g. git log/git diff would start a pager and hang). exec_command is registered with strict_json_schema=False so models can genuinely omit optional fields, which is the case this still covers. Addresses Greptile review feedback on PR usestrix#739.
Matches Greptile review feedback on PR usestrix#739 (single-letter boolean aliases were previously unrecognized and silently fell back to the platform default).
4a11e42 to
a6ae2da
Compare
Why
exec_command's non-tty path (the tool's default) reads command output through docker-py's frame-header demux (
docker.utils.socket.frames_iter_no_tty), which blocks on an 8-byte header read before any payload surfaces. On native Windows, docker-py talks to Docker Desktop over a named pipe (NpipeSocket) instead of a POSIX socket, and that framed read routinely doesn't resolve inside exec_command's 10s yield window - the tool returns cleanly with empty output, no exception, no timeout.tty=truetakes a different SDK path (frames_iter_tty, a single raw read) and isn't affected.That matches every symptom in #727: a fresh session id on every retry (each exec_command call opens a new pty_exec_start),
tty"fixing itself" but not staying fixed (it's a one-shot param, reverts to default next call), anddocker execfrom the host working fine (that's the CLI, a different code path than the SDK's exec-socket streaming).Checked whether this is already fixed upstream: diffed the pinned SDK (
0.14.6) against latest (0.18.2) -pty_exec_start/frames_iterlogic is identical. Not something a version bump resolves, so this is a Strix-side workaround rather than waiting on upstream.What
_wrap_exec_commandforcestty: trueon every exec_command call whensys.platform == "win32", overriding even an explicittty: falsefrom the model (some models emit the schema default explicitly).STRIX_EXEC_FORCE_TTYenv var overrides the auto-detection either way. No change on Linux/macOS/WSL2.Verified
tty=True-> real output in ~0.17s,tty=False->'', no exception, no timeout.{"cmd": "..."}payload (no tty key, matching what a model sends) - now returns real output.tests/test_exec_command_windows_tty.py- 20 new unit tests, no Docker required.Known limitation
The sandbox image doesn't set
PAGER=cat/GIT_PAGER=cat, so forcing a real tty meansgit log/git diffwithout--no-pagercould invokelessand block on a keypress the agent can't send - a different hang than this one. exec_command already returns early with a session id for anything still running, recoverable viawrite_stdin, but it's a new failure mode worth flagging. Can add those env vars to the Dockerfile here or as a follow-up if maintainers want it addressed now.Closes #727.