Repository navigation
Python: Fix stateless shell tools inheriting host stdin - #9469
Patel Namraa (Namraa310806) wants to merge 4 commits into
Conversation
Bicheng (Kenneth) (BichengWang)
left a comment
There was a problem hiding this comment.
Reviewed fd76aaa311819b572c75c145e43db991def72fb8. I found no correctness blocker in the stdin change: both stateless spawn paths now supply EOF, while persistent sessions still create their own PIPE.
One small fix before merge: the pinned Ruff 0.17.0 formatter rejects the new assertion at python/packages/tools/tests/test_docker_shell_tool.py:640. ruff format --check wants the assertion and message on one line. Ruff lint passes on all four changed files, and Pyright 1.1.414 reports zero errors or warnings on both changed source modules.
Offline validation on macOS arm64 / CPython 3.14.8:
- The two affected test files passed: 71 passed, 9 skipped, including existing persistent-session coverage.
- Loading only the two pre-fix implementations from base
44fe223da7ea6853eefb7053e25c2c213a2e5c15in memory made both new regression tests fail: localcattimed out, and the Docker spawn omittedstdin. The checkout remained at the reviewed head. - Seven additional checks used a separate host process with piped stdin. With a closed writer, the base consumed
host-input\n, whereas this head returned empty stdout and left that input for the host. With the writer still open, the base consumed the input and timed out; this head preserved it and exited successfully. An open empty pipe also returned EOF promptly. Explicit shell pipelines and here-documents continued to supply their own input without consuming host input.
The full tools unit run was 157 passed, 10 skipped, 3 xfailed, and one failure: the unchanged test_denylist_pattern_timeout_denies exceeded its five-second wall-clock bound at 6.03 seconds. That test passed alone on rerun in 1.13 seconds; I would not attribute the initial timing failure to this diff.
I checked the existing review and inline comments; there were no technical findings to duplicate. Docker and PowerShell are unavailable here, so Docker validation covers the mocked subprocess boundary, not a running container, and Windows behavior remains untested. I did not run the full workspace or service integrations. Separately, #9468 currently has automated reproduction feedback but no explicit maintainer agreement; that contribution-process prerequisite remains for maintainers to resolve.
Motivation & Context
Stateless shell execution currently allows child processes to inherit the host process's standard input.
This can cause stateless shell commands that read from stdin to unexpectedly consume input intended for the host application. Commands that wait for stdin can also remain blocked until the shell tool timeout is reached.
This affects both
LocalShellToolandDockerShellToolstateless execution paths.This change ensures stateless shell processes do not inherit the host application's stdin, while leaving the dedicated stdin handling of persistent shell sessions unchanged.
Description & Review Guide
What are the major changes?
stdin=asyncio.subprocess.DEVNULLwhen creating stateless subprocesses inLocalShellTool.stdin=asyncio.subprocess.DEVNULLwhen creating the Docker subprocess used byDockerShellToolstateless execution.What is the impact of these changes?
What do you want reviewers to focus on?
DEVNULLis the appropriate isolation behavior.Related Issue
Fixes #9468
AI Assistance
AI assistance details:
AI assistance was used to investigate the reported behavior, review the affected shell execution paths, develop the regression tests, and prepare this PR description. The implementation and tests were reviewed and verified by the contributor.
Contribution Checklist