sdk/python: Add PTY-related APIs#579
Conversation
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
chenhengqi
requested review from
fslongjin,
ls-ggg,
tinklone and
up2wing
as code owners
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/u/20101770?s=60&v=4){kind=small}
| def __iter__(self) -> Generator[PtyOutput, None, None]: | ||
| return self._handle_events() |
There was a problem hiding this comment.
Double iteration silently produces no output
__iter__ returns self._handle_events(), which creates a fresh generator each call. But self._events (the underlying Connect-JSON event generator) is single-use. Calling __iter__ a second time — e.g., calling wait() twice, or manually iterating then calling wait() — yields zero chunks because the inner generator is exhausted. The second wait() would raise a confusing "PTY stream ended without an end event" error.
Consider guarding with a consumed flag that raises RuntimeError on reuse, or clearly documenting/explicitly enforcing single-iteration.
| def _client(self): | ||
| if self._sandbox._client is None: | ||
| self._sandbox._client = self._sandbox._build_data_client() | ||
| return self._sandbox._client |
There was a problem hiding this comment.
Missing return type annotation
_client() has no return type annotation. It always returns a non-None httpx.Client (it lazily builds one). The companion Sandbox._build_data_client in sandbox.py is annotated; this one should be for consistency and to help type checkers. Should be -> httpx.Client.
| except BaseException: | ||
| try: | ||
| resp_ctx.__exit__(None, None, None) | ||
| except Exception: | ||
| pass | ||
| raise |
There was a problem hiding this comment.
except BaseException: catches KeyboardInterrupt and SystemExit
The broadest catch clause silently swallows Ctrl-C and interpreter shutdown signals. If a user hits Ctrl-C during PTY creation, the handler blocks clean process exit. Change to except Exception: — or handle GeneratorExit explicitly if the generator cleanup path requires it.
| _raise_connect_end_stream(raw) | ||
| return | ||
|
|
||
| message = json.loads(raw.decode("utf-8")) |
There was a problem hiding this comment.
Unnecessary raw.decode("utf-8") before json.loads
json.loads has accepted bytes directly since Python 3.6, and the Connect-JSON wire format is always UTF-8. Writing json.loads(raw) instead eliminates one intermediate string allocation and one decode pass per event. On busy PTY output streams with thousands of events per second this adds measurable GC pressure.
| def __iter__(self) -> Generator[PtyOutput, None, None]: | ||
| return self._handle_events() |
There was a problem hiding this comment.
Double iteration silently produces no output
__iter__ returns self._handle_events(), which creates a fresh generator each call. But self._events (the underlying Connect-JSON event generator) is single-use. Calling __iter__ a second time — e.g., calling wait() twice, or manually iterating then calling wait() — yields zero chunks because the inner generator is exhausted. The second wait() would raise a confusing "PTY stream ended without an end event" error.
Consider guarding with a consumed flag that raises RuntimeError on reuse, or clearly documenting/explicitly enforcing single-iteration.
| def _client(self): | ||
| if self._sandbox._client is None: | ||
| self._sandbox._client = self._sandbox._build_data_client() | ||
| return self._sandbox._client |
There was a problem hiding this comment.
Missing return type annotation
_client() has no return type annotation. It always returns a non-None httpx.Client (it lazily builds one). The companion Sandbox._build_data_client in sandbox.py is annotated; this one should be for consistency and to help type checkers. Should be -> httpx.Client.
| except BaseException: | ||
| try: | ||
| resp_ctx.__exit__(None, None, None) | ||
| except Exception: | ||
| pass | ||
| raise |
There was a problem hiding this comment.
except BaseException: catches KeyboardInterrupt and SystemExit
The broadest catch clause silently swallows Ctrl-C and interpreter shutdown signals. If a user hits Ctrl-C during PTY creation, the handler prevents clean process exit. Change to except Exception: — or handle GeneratorExit explicitly if cleanup requires it.
| _raise_connect_end_stream(raw) | ||
| return | ||
|
|
||
| message = json.loads(raw.decode("utf-8")) |
There was a problem hiding this comment.
Unnecessary raw.decode("utf-8") before json.loads
json.loads has accepted bytes directly since Python 3.6, and the Connect-JSON wire format is always UTF-8. Writing json.loads(raw) instead eliminates one intermediate string allocation and one decode pass per event. On busy PTY output streams with thousands of events per second this adds measurable GC pressure.
1 participant
No description provided.