Repository navigation
feat(cli): support non-TTY runner exec sessions - #1174
badayvedat wants to merge 4 commits into
Conversation
58d043c to
1d2cba1
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes concurrency/TTY-handling logic in the shell/exec streaming path, a human look would still be worthwhile.
What was reviewed:
_shell_session's newremote_tty/close-signal logic inrunners.py, confirmingShellRunnerInput.tty/closeandShellRunnerOutput.streamare proto3-optional fields (checked againstisolate_proto/controller_pb2.pyi), matching theHasFieldusage.- The
_exec/_shellTTY-detection paths and the EOF/close signaling for both piped and pty stdin. - The
pyproject.tomlversion bump (isolate-proto>=0.34.3,<1) lines up with the newtty/streamfields the code relies on. - The added unit tests in
test_runners.py, which cover stdout/stderr demuxing, non-tty stdin-close, interactive piped raw bytes, and tty-size messaging.
Extended reasoning...
Overview
The PR bumps isolate-proto to >=0.34.3 and reworks fal shell/fal exec streaming in projects/fal/src/fal/cli/runners.py: it adds a remote_tty parameter so a PTY is only requested when local stdin is an actual terminal, sends an explicit ShellRunnerInput(close=True) when local input reaches EOF (or immediately for non-interactive commands) so non-PTY sessions signal stdin closure, and demultiplexes output by a new stream field so stderr bytes are written to sys.stderr while stdout (and legacy servers that never set stream) goes to sys.stdout. Tests were substantially extended to cover these paths.
Security risks
No new security-sensitive surface is introduced. The command execution capability against remote runners already existed; this change only affects local TTY negotiation and byte routing for that existing capability. I did not find injection, auth-bypass, or data-exposure concerns.
Level of scrutiny
This touches concurrency (background stdin-reader thread, Queue), raw-mode terminal handling, and a gRPC streaming loop — moderate complexity even though the change is well-contained. I traced the tty_size/close/stream field semantics against isolate_proto/controller_pb2.pyi and confirmed they are proto3-optional fields correctly probed via HasField, and that the remote_tty computation (args.interactive and sys.stdin.isatty()) is consistent between _exec and the local is_tty variable. I did not find a bug, but the combination of threading, signal handling, and protocol semantics is the kind of area where a second human pass is valuable even absent found issues.
Other factors
Test coverage is solid and specifically targets the new behavior (stream separation, non-tty close signaling, interactive piped raw bytes, tty-size messaging, legacy shell always-tty behavior), which increases my confidence the changes work as intended.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1d2cba1. Configure here.
| os.close(read_fd) | ||
|
|
||
| assert sent[0].tty is True | ||
| assert all(not msg.close for msg in sent) |
There was a problem hiding this comment.
Shell TTY test fails on Windows
Medium Severity
test_shell_always_requests_tty invokes fal runners shell, which still rejects interactive sessions on Windows and returns 1. Sibling interactive tests skip that platform, so this assertion that the command succeeds fails in Windows unit CI.
Reviewed by Cursor Bugbot for commit 1d2cba1. Configure here.
There was a problem hiding this comment.
shell never worked on windows


Update
fal runners execto use non-TTY runner sessions when no local terminal is attached. This preserves raw stdin bytes, closes remote stdin correctly, and keeps stdout and stderr separate.This is the client-side activation for the
ShellRunnerfields released inisolate-proto 0.34.3and will be rolled out after the corresponding isolate-cloud server support is deployed.Note
Medium Risk
Changes remote runner shell/exec protocol and behavior; correct operation depends on isolate-cloud server support for the new
ShellRunnerfields in isolate-proto 0.34.3.Overview
Runner
exec/shellnow negotiate a remote PTY and stdin/stdout behavior explicitly viaisolate-proto0.34.3 (tty,close, and per-stream output).For
fal runners exec, a remote pseudo-terminal is requested only when-itis set and local stdin is a TTY; piped or non-interactive runs use a raw byte stream, signalcloseso remote stdin gets EOF, and avoid PTY byte mangling.fal runners shellalways requests a remote TTY. RemoteShellRunneroutput is routed to local stdout vs stderr when the server setsstream; legacy responses withoutstreamstill go to stdout.Unit tests cover stream splitting, non-TTY exec, interactive exec with piped stdin, terminal TTY + size, and shell always using a TTY.
Reviewed by Cursor Bugbot for commit f49859b. Bugbot is set up for automated code reviews on this repo. Configure here.