feat: Add interactive terminal (PTY) support w/ tests to cua-auto, computer-server, computer, and the cua CLI - #1114
Conversation
…ndlers from computer-server into its own SDK for reuse by our various SDKs
Every cua do action is now automatically recorded to a replayable
trajectory at ~/.cua/trajectories/{machine}/{session}/. Viewing opens
cua.ai/trajectory-viewer via a local CORS-enabled file server.
New files:
- trajectory_recorder.py: session management, turn writing, zip, clean
- trajectory.py: cua trajectory ls/view/stop/clean commands
Modified:
- do.py: --no-record flag, post-action screenshot recording in all handlers,
session reset on switch
- main.py, __init__.py: register trajectory command
- SKILL.md: document trajectory recording
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: ddupont <3820588+ddupont808@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📦 Publishable packages changed
Add |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces cross-platform PTY session management across the codebase, adding a new terminal library (cua-auto), server-side PTY endpoint infrastructure (computer-server), a client-side PTY interface (computer), and CLI shell command enhancements supporting both local and remote interactive terminal sessions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as CLI Client
participant Interface as PtyInterface
participant HTTP as Server (HTTP)
participant Manager as PtyManager
participant Terminal as Terminal<br/>(cua-auto)
participant WS as Server<br/>(WebSocket)
Client->>Interface: create(command)
Interface->>HTTP: POST /pty<br/>(auth headers)
HTTP->>Manager: create()
Manager->>Terminal: create()
Terminal-->>Manager: PtySession{pid}
Manager-->>HTTP: {pid, cols, rows}
HTTP-->>Interface: {pid, cols, rows}
Interface->>WS: WebSocket connect<br/>to /pty/{pid}/ws
WS->>Manager: stream output
Manager->>Terminal: monitor session
Terminal-->>Manager: output bytes
Manager-->>WS: broadcast{type:output, data}
WS-->>Interface: output via WS
Interface->>Client: on_data(bytes)
Client->>Interface: send_stdin(data)
Interface->>WS: queue stdin
WS->>Manager: forward stdin
Manager->>Terminal: send_stdin()
Terminal-->>Manager: exit event{code}
Manager-->>WS: broadcast{type:exit, code}
WS-->>Interface: exit event
Interface->>Client: exit notification
sequenceDiagram
participant User as User
participant CLI as CLI Shell Handler
participant Check as stdin check
participant Provider as Provider
participant Host as cua-auto<br/>Terminal
participant Remote as Computer<br/>(remote)
User->>CLI: cua do shell<br/>[command]
CLI->>Check: isatty(stdin)?
alt Interactive (TTY detected)
Check-->>CLI: yes
alt Provider = "host"
CLI->>Host: terminal.create(cmd)
Host->>User: raw mode shell
User->>Host: input
Host-->>CLI: output
else Provider = cloud/remote
CLI->>Remote: PtyInterface.create(cmd)
Remote-->>Remote: spawn PTY
CLI->>User: raw mode terminal
User->>Remote: stdin via WS/HTTP
Remote-->>CLI: stdout via WS
end
else Non-interactive
Check-->>CLI: no
CLI->>Provider: execute_cmd()
Provider-->>CLI: {stdout, stderr, exit_code}
CLI->>User: print output
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~160 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (5)
libs/python/cua-auto/cua_auto/terminal.py (2)
203-207: Dead code: theelsebranch is unreachable.
commandis typedOptional[str]; aftercmd_str = command or "bash",cmd_stris always astr. Theisinstance(cmd_str, str)check (line 204) will always beTrue, making theelsebranch on line 206-207 unreachable.Simplification
cmd_str = command or "bash" - if isinstance(cmd_str, str): - cmd = ["/bin/sh", "-c", cmd_str] - else: - cmd = cmd_str + cmd = ["/bin/sh", "-c", cmd_str]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/python/cua-auto/cua_auto/terminal.py` around lines 203 - 207, The isinstance check in terminal.py is dead because cmd_str = command or "bash" guarantees a str; remove the unreachable else branch and directly set cmd to the shell invocation using cmd_str (i.e., replace the conditional with cmd = ["/bin/sh", "-c", cmd_str]) in the function where cmd_str is created so the code path is simplified and clear (reference: variable cmd_str and the cmd assignment around the existing isinstance(cmd_str, str) check).
162-182:connect()acquires the lock twice where once would suffice.Lines 176-178 and 180-181 each acquire
self._lockseparately. Between the two acquisitions the session could theoretically be removed. A singlewith self._lockblock would be simpler and more correct.Proposed fix
def connect( self, pid: int, on_data: Callable[[bytes], None], ) -> PtySession: - with self._lock: - holder = self._sessions.get(pid) - if holder is None: - raise KeyError(f"No PTY session with pid {pid}") with self._lock: + holder = self._sessions.get(pid) + if holder is None: + raise KeyError(f"No PTY session with pid {pid}") holder.on_data = [on_data] return PtySession(pid=pid, cols=80, rows=24)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/python/cua-auto/cua_auto/terminal.py` around lines 162 - 182, connect currently acquires self._lock twice which allows the session to be removed between the two critical sections; instead, hold self._lock once while fetching and updating the session: perform holder = self._sessions.get(pid), check if holder is None and raise KeyError while still holding the lock, then set holder.on_data = [on_data] inside the same with self._lock block; return the new PtySession after releasing the lock. Ensure you update the code in the connect method (references: connect, self._lock, self._sessions, holder, holder.on_data).libs/python/cua-cli/cua_cli/commands/do.py (1)
1321-1335: Useasyncio.get_running_loop()instead ofasyncio.get_event_loop().Line 1332 (Windows) and line 1366 (Unix) both use the deprecated
asyncio.get_event_loop(). Since these run insideasyncfunctions,asyncio.get_running_loop()is the correct alternative and avoids the deprecation warning. Same issue noted inpty_manager.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/python/cua-cli/cua_cli/commands/do.py` around lines 1321 - 1335, Replace deprecated asyncio.get_event_loop() with asyncio.get_running_loop() inside the async contexts used to schedule websocket sends: update the call site in the _stdin_loop closure inside async def _run_ws() where asyncio.run_coroutine_threadsafe(..., asyncio.get_event_loop()) is used (and the analogous call in the Unix branch / pty_manager.py). Ensure you call asyncio.get_running_loop() from within the running async function so asyncio.run_coroutine_threadsafe receives the current running loop.libs/python/computer-server/computer_server/pty_manager.py (1)
87-96:_on_dataaccessesself._queuesfrom the Terminal's reader thread without synchronization.
_on_datais invoked from the PTY reader thread (not the event loop thread), yet it readsself._queues(line 92) which is mutated on the event loop thread bysubscribe/unsubscribe. This works under CPython's GIL but is technically a data race per the Python memory model and could break on free-threaded Python (PEP 703). Wrapping the read in a lock or scheduling the entire broadcast vialoop.call_soon_threadsafewould be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/python/computer-server/computer_server/pty_manager.py` around lines 87 - 96, _in_data runs on the PTY reader thread but directly reads self._queues (which is mutated by subscribe/unsubscribe on the event loop), causing a thread-safety/data-race risk; fix by either protecting accesses to self._queues with a dedicated threading.Lock used by _on_data and by subscribe/unsubscribe, or move the broadcast/read into the event loop by wrapping the entire iteration and q.put_nowait calls in a closure submitted via loop.call_soon_threadsafe; locate the logic in the _on_data callback and the subscribe/unsubscribe methods to add the lock or replace the direct iteration with loop.call_soon_threadsafe to ensure safe cross-thread access.libs/python/computer-server/computer_server/main.py (1)
818-820: Bareexcept Exception: passsilently swallows all WebSocket send errors in_send_outputIf
websocket.send_text()fails (e.g., client disconnected, network error), the exception is caught and discarded without logging, cancelling the output task, or unblocking the outer receive loop. At minimum, log the error so failures are observable.♻️ Proposed fix
- except Exception: - pass + except Exception as exc: + logger.debug("PTY WS output task error for pid %d: %s", pid, exc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/python/computer-server/computer_server/main.py` around lines 818 - 820, In _send_output, replace the bare "except Exception: pass" around websocket.send_text() with an "except Exception as e:" that logs the failure (use the module logger or existing logger.exception/logger.error with context like f"websocket.send_text failed in _send_output: {e}"), then stop the send loop by breaking/returning and cancel the associated output task (e.g., call output_task.cancel() or set a flag) so the outer receive loop can unblock/clean up; ensure you reference websocket.send_text and the output_task variable names when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/python/computer-server/computer_server/main.py`:
- Around line 787-803: The pty_ws WebSocket handler currently accepts
connections without auth; before calling await websocket.accept(), extract
credentials (e.g., api_key and container_name) from the connection (preferably
from websocket.query_params or from the first client message) and call await
_require_auth(container_name, api_key) to validate them; if _require_auth raises
or returns an error, close the websocket with an appropriate code and do not
accept the connection. Ensure you reference the existing pty_ws function and use
the same _require_auth(container_name, api_key) helper so behavior matches the
other PTY endpoints.
In `@libs/python/computer-server/computer_server/pty_manager.py`:
- Around line 86-109: The callback _on_data can drop early output because
pid_cell[0] is None until after terminal.create returns and the reader thread
runs; to fix, ensure pid_cell[0] is set before the reader can invoke _on_data —
for example, wrap terminal.create so you synchronously obtain the child PID, set
pid_cell[0] and initialize self._queues[pid] before starting the reader, or
alternatively implement a small buffer in _on_data that stores data when
pid_cell[0] is None and flushes it into the queues after you assign session.pid;
refer to terminal.create, _on_data, pid_cell, session and self._queues when
making the change.
- Line 80: In the async create method of PTYManager (replace the call currently
using asyncio.get_event_loop()), switch to asyncio.get_running_loop() since
create is async and a running loop exists; update the assignment where loop is
captured by the _on_data and _watch_exit closures so they continue to use the
new loop reference (no other behavior changes).
In `@libs/python/computer/computer/computer.py`:
- Around line 1055-1080: The pty property currently constructs a new
PtyInterface on every access which loses per-instance state like
PtyInterface._sessions; change the property (pty) to cache the created instance
on the Computer object (e.g. self._pty_interface) and return that cached
instance when present, creating it only when None and when self._interface is
available; also ensure you invalidate/reset self._pty_interface = None in
lifecycle methods that recreate the connection (e.g. restart() and any
disconnect/reconnect paths) so a fresh PtyInterface is created after
reconnection. Ensure you still compute base_url, api_key and vm_name the same
way when creating the cached instance and raise the same RuntimeError when the
computer is not started.
- Around line 1079-1080: The AttributeError occurs because getattr(self.config,
...) evaluates self.config even when it doesn't exist (when
use_host_computer_server=True); update the vm_name resolution to safely handle a
missing self.config (e.g., use getattr(self, "config", None) and then getattr on
that result, or guard with hasattr(self, "config")) so PtyInterface(...)
receives vm_name or None without raising; change the vm_name assignment near the
return that constructs PtyInterface and/or ensure __init__ always sets
self.config.
In `@libs/python/computer/computer/pty.py`:
- Around line 299-301: The code creates stdin_queue and assigns it into a
temporary dict when pid is missing because sess = self._sessions.get(pid, {}) is
used; as a result self._sessions[pid] is never updated and send_stdin can't find
the queue. Fix by ensuring the session dict is stored back into self._sessions
for that pid (e.g., use self._sessions.setdefault(pid, {}) or check if pid in
self._sessions and assign sess back) so that stdin_queue is placed into the
persistent session entry created by create()/connect() and send_stdin can locate
it.
- Around line 281-287: The _wait method currently returns -1 for unknown pid
which is undocumented and silent; change _wait (in class containing _wait) to
raise a clear exception (e.g., LookupError or a new SessionNotFoundError) when
sess is None (include pid in the message) instead of returning -1, and update
the public API docstring for PtyHandle.wait (or modify callers) to reflect that
waiting on a non-existent session raises that exception; ensure any callers of
_wait/PtyHandle.wait handle or propagate the new exception.
- Around line 248-262: connect() currently returns a PtyHandle with hardcoded
cols=80 and rows=24 (unlike create()), causing incorrect dimensions after
reconnect; update the connect() docstring to document this limitation and that
reconnect cannot fetch resized dimensions, or implement a server endpoint (e.g.,
GET /pty/{pid} returning {"pid": int, "cols": int, "rows": int}) and modify
connect() to call that endpoint and use the returned cols/rows when constructing
the PtyHandle (references: connect(), create(), PtyHandle, _ws_reader,
_sessions, pid).
In `@libs/python/cua-auto/cua_auto/terminal.py`:
- Around line 91-100: send_stdin (and similarly resize) suffers a TOCTOU where
_sessions[pid].master_fd can be closed after releasing self._lock, causing
os.write to raise OSError; fix by performing the write while holding self._lock
or by re-checking and handling a closed fd: acquire self._lock, fetch holder =
self._sessions.get(pid), if holder is None raise KeyError, then if
holder.master_fd is not None attempt os.write inside a try/except OSError
(handle/ignore EBADF or translate to KeyError), else if holder.winpty_pty is not
None call holder.winpty_pty.write; ensure the same pattern is applied in resize
and reference the send_stdin, resize, _sessions, master_fd, winpty_pty, and
_lock symbols when making changes.
In `@libs/python/cua-auto/tests/test_terminal.py`:
- Around line 27-30: Remove the dead helper function _collect: it's never
invoked and returns an empty bytes value while its docstring inaccurately claims
it returns collected output; delete the entire def _collect(...) block
(including its misleading docstring and the always-returning b"") to avoid
confusion and keep the test module focused on the actual on_data closure-based
collection.
- Around line 1-19: Reorder the imports to satisfy isort: ensure from __future__
import annotations stays first, then place standard-library imports sorted
alphabetically (import time before import sys) as a single block, followed by a
blank line and the third-party import (import pytest); update the import lines
(from __future__ import annotations, import time, import sys, import pytest)
accordingly so isort passes.
In `@libs/python/cua-cli/cua_cli/commands/do.py`:
- Around line 1783-1789: The --cols and --rows args are parsed but never used;
update the shell command wiring so args.cols and args.rows are passed into the
PTY helpers and honored: add optional cols, rows parameters to _shell_host_pty
and _shell_remote_pty (or their wrappers), change the shell subparser call sites
to pass args.cols and args.rows, and inside those functions use the provided
cols/rows when not None (fall back to shutil.get_terminal_size() otherwise);
alternatively, if you prefer removal, delete the add_argument lines for "--cols"
and "--rows" and any references to them.
- Around line 1228-1266: The code sets the tty to raw with
tty.setraw(sys.stdin.fileno()) but only restores termios.tcsetattr after
terminal.wait(), which can be skipped by exceptions; wrap the raw-mode setup,
SIGWINCH handler registration, and stdin_thread start in a try/finally block so
that in finally you (1) restore the terminal settings with
termios.tcsetattr(sys.stdin.fileno(), termios.TCSADRAIN, old_settings), (2)
reset SIGWINCH with signal.signal(signal.SIGWINCH, signal.SIG_DFL), and (3)
ensure the stdin_thread is stopped/joined (or at least let it be daemon) to
avoid leaving the terminal in raw mode; apply this change around the code that
defines _resize, calls tty.setraw, starts stdin_thread, and calls
_term.terminal.wait(session.pid).
- Around line 1427-1449: The non-interactive branch in _cmd_shell calls
_cmd_shell_noninteractive without ensuring a target is set, causing
state["provider"] KeyError; fix by invoking the existing _require_target() guard
before the stdin.isatty() check (or alternatively add the same target validation
inside _cmd_shell_noninteractive). Ensure _require_target() is called from
_cmd_shell prior to calling _cmd_shell_noninteractive so that missing target
returns the proper error code/message instead of raising KeyError when
_cmd_shell_noninteractive accesses state["provider"].
---
Nitpick comments:
In `@libs/python/computer-server/computer_server/main.py`:
- Around line 818-820: In _send_output, replace the bare "except Exception:
pass" around websocket.send_text() with an "except Exception as e:" that logs
the failure (use the module logger or existing logger.exception/logger.error
with context like f"websocket.send_text failed in _send_output: {e}"), then stop
the send loop by breaking/returning and cancel the associated output task (e.g.,
call output_task.cancel() or set a flag) so the outer receive loop can
unblock/clean up; ensure you reference websocket.send_text and the output_task
variable names when making the changes.
In `@libs/python/computer-server/computer_server/pty_manager.py`:
- Around line 87-96: _in_data runs on the PTY reader thread but directly reads
self._queues (which is mutated by subscribe/unsubscribe on the event loop),
causing a thread-safety/data-race risk; fix by either protecting accesses to
self._queues with a dedicated threading.Lock used by _on_data and by
subscribe/unsubscribe, or move the broadcast/read into the event loop by
wrapping the entire iteration and q.put_nowait calls in a closure submitted via
loop.call_soon_threadsafe; locate the logic in the _on_data callback and the
subscribe/unsubscribe methods to add the lock or replace the direct iteration
with loop.call_soon_threadsafe to ensure safe cross-thread access.
In `@libs/python/cua-auto/cua_auto/terminal.py`:
- Around line 203-207: The isinstance check in terminal.py is dead because
cmd_str = command or "bash" guarantees a str; remove the unreachable else branch
and directly set cmd to the shell invocation using cmd_str (i.e., replace the
conditional with cmd = ["/bin/sh", "-c", cmd_str]) in the function where cmd_str
is created so the code path is simplified and clear (reference: variable cmd_str
and the cmd assignment around the existing isinstance(cmd_str, str) check).
- Around line 162-182: connect currently acquires self._lock twice which allows
the session to be removed between the two critical sections; instead, hold
self._lock once while fetching and updating the session: perform holder =
self._sessions.get(pid), check if holder is None and raise KeyError while still
holding the lock, then set holder.on_data = [on_data] inside the same with
self._lock block; return the new PtySession after releasing the lock. Ensure you
update the code in the connect method (references: connect, self._lock,
self._sessions, holder, holder.on_data).
In `@libs/python/cua-cli/cua_cli/commands/do.py`:
- Around line 1321-1335: Replace deprecated asyncio.get_event_loop() with
asyncio.get_running_loop() inside the async contexts used to schedule websocket
sends: update the call site in the _stdin_loop closure inside async def
_run_ws() where asyncio.run_coroutine_threadsafe(..., asyncio.get_event_loop())
is used (and the analogous call in the Unix branch / pty_manager.py). Ensure you
call asyncio.get_running_loop() from within the running async function so
asyncio.run_coroutine_threadsafe receives the current running loop.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/ci-test-python.ymllibs/python/computer-server/computer_server/main.pylibs/python/computer-server/computer_server/pty_manager.pylibs/python/computer-server/pyproject.tomllibs/python/computer/computer/__init__.pylibs/python/computer/computer/computer.pylibs/python/computer/computer/pty.pylibs/python/cua-auto/cua_auto/__init__.pylibs/python/cua-auto/cua_auto/terminal.pylibs/python/cua-auto/pyproject.tomllibs/python/cua-auto/tests/__init__.pylibs/python/cua-auto/tests/test_terminal.pylibs/python/cua-cli/cua_cli/commands/do.py
📦 Publishable packages changed
Add |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📦 Publishable packages changed
Add |
📦 Publishable packages changed
Add |
📦 Publishable packages changed
Add |
📦 Publishable packages changed
Add |
📦 Publishable packages changed
Add |
📦 Publishable packages changed
Add |
📦 Publishable packages changed
Add |
📦 Publishable packages changed
Add |
📦 Publishable packages changed
Add |
Adds a full PTY (pseudo-terminal) stack from the local automation layer through the HTTP server and up to the CLI, enabling
cua do shellto open a real interactive terminal (SSH-like) inside any provider VM or local host.cua-auto— newterminal.pyPTY engine (Unixptystdlib / Windowspywinpty)computer-server— new/ptyREST + WebSocket API endpointscomputer— newPtyInterface/PtyHandleclient +computer.ptypropertycua-cli—cua do shellbecomes interactive when stdin is a TTY; falls back to the existingrun_commandpath for non-interactive/piped useci-test-python.ymlChanges by layer
Layer 1 —
libs/python/cua-autocua_auto/terminal.pyTerminalclass +PtySessiondataclass + module-levelterminalsingleton)cua_auto/__init__.pyterminalmodule; adds it to__all__pyproject.tomlptyoptional dep (pywinpty>=2.0.0; sys_platform=='win32');allextra now includesptytests/__init__.pytests/test_terminal.pyTerminalAPI:Layer 2 —
libs/python/computer-servercomputer_server/pty_manager.pyPtyManagerwrapsTerminalwith asyncioQueue-based output broadcastingcomputer_server/main.py_require_auth()helper;/ptyREST endpoints +/pty/{pid}/wsWebSocketpyproject.tomlcua-auto>=0.1.0added as a runtime dep (PTY engine comes via cua-auto)New endpoints:
Layer 3 —
libs/python/computercomputer/pty.pyPtyInterface(async HTTP+WS client) +PtyHandle(per-session handle)computer/computer.pycomputer.ptyproperty returning aPtyInterfacefor the running VMcomputer/__init__.pyPtyHandle,PtyInterfaceUsage:
Layer 4 —
libs/python/cua-clicua_cli/commands/do.py_cmd_shellreplaced with PTY-aware version; non-interactive path preservedBehaviour:
cua do shell [cmd]with a TTY → spawns an interactive PTY session (host usescua_auto.terminaldirectly; remote providers useaiohttpWebSocket to/pty/{pid}/ws)echo ls | cua do shell(no TTY) → unchangedrun_command/ SSE path--cols/--rowsflags added to theshellsub-parser for explicit sizing_default_shell()returnspowershellon Windows,bashelsewhereTests
libs/python/cua-auto/tests/test_terminal.py— 27 tests, all skipped onwin32(CI isubuntu-latest):TestTerminalImportPtySessiondataclassTestEchoBasicecho hello,echo hello world, leading/trailing spaces, multi-word, numbers, special charsTestExitCodesexit 0 / 1 / 42,true,falseTestSendStdinTestKillTestResizeTestConnectKeyErrorTestSingletonTerminalterminalsingleton end-to-end.github/workflows/ci-test-python.yml—cua-autoadded to the package matrix sotest_terminal.pyruns on every PR touchinglibs/python/**.Test plan
cua do switch host && cua do shellopens an interactive shell on the local machinecua do switch docker <name> && cua do shell bashopens interactive bash in the container via WebSocketecho "ls" | cua do shell(piped) runs non-interactively and prints✅ outputcua do shell --cols 120 --rows 40 bashrespects explicit terminal dimensionscurl -X POST localhost:8000/pty -d '{"command":"bash","cols":80,"rows":24}' -H 'Content-Type: application/json'returns{"pid":…}curl -N localhost:8000/pty/<pid>/streamstreams SSE output eventscua-automatrix job passes all 27test_terminal.pytests onubuntu-latestSummary by CodeRabbit
New Features
Chores