Repository navigation
feat(sdk): add streamInput / stream_input for commands and pty - #1997
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
🦋 Changeset detectedLatest commit: 5247ed5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Package ArtifactsBuilt from 2bf12d8. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-dockerfile-utils-0.1.2-devin-1792417976-stream-input.0.tgz ./e2b-2.57.1-devin-1792417976-stream-input.0.tgzCLI ( npm install ./e2b-cli-2.22.3-devin-1792417976-stream-input.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.9.1-devin-1792417976-stream-input.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.5.1-devin-1792417976-stream-input.0.tgzPython SDK ( pip install ./e2b_dockerfile_utils-0.1.1+devin.1792417976.stream.input-py3-none-any.whl ./e2b-2.57.0+devin.1792417976.stream.input-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.11.1+devin.1792417976.stream.input-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.7.0+devin.1792417976.stream.input-py3-none-any.whl |
|
align with TASTE pls |
|
Aligned with TASTE in 691f8aa:
The PR description is updated to match. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
packages/js-sdk/src/sandbox/commands/inputHandle.ts— JS callers who reuse a Uint8Array buffer afterawait input.send(buf)can send corrupted or duplicated bytes, because the handle sends the buffer later.send()pushes the caller's array by reference and resolves at once.requests()only reads it when the RPC pulls the next message (inputHandle.ts:395). Fix: whensend()gets a Uint8Array, queue a copy (data.slice()), so later changes by the caller cannot reach the wire; leave strings as they are.Why this was flagged
A caller reads a file, socket or pty source into one reused buffer, e.g.
fs.read(fd, buf, ...)in a loop, and callsawait handle.send(buf.subarray(0, n))for each chunk. That is the pattern this API invites, sincesendresolves once the input is queued (inputHandle.ts:310).send()at inputHandle.ts:325 queues the caller's own Uint8Array.requests()encodes it only when the connect transport pulls the next request (inputHandle.ts:388-409), which is after the caller has already overwritten the buffer for the next chunk. The PTY or stdin then gets the later chunk's bytes, possibly twice, and the earlier chunk is lost. With the existingsendInput/sendStdin, the caller awaits a full request that serializes the bytes before it returns. Python is not affected becausebytescannot be changed.Verification: nit. The push is at line 106:
this.queue.push(data)stores the caller's array by reference, andsendresolves right afternotify()(lines 106-107). The array is read only later, inrequests()(lines 171-177). The existingsendInputwaits for the RPC, so buffer reuse after its await was safe. -
🟡
packages/python-sdk/e2b/sandbox_sync/commands/pty.py— Sync users who pass a singlebytesorstrtostream_input(pid, data)get a confusing failure or one-character messages, not one input chunk.requests()iteratesdatawithout checking its type. Iteratingbytesyields ints, sostream_input_datagets an int payload and serialization fails. Iterating astrsends one message per character. Fix: in both syncstream_inputmethods, treat a barestr/bytes/bytearrayas one chunk (or raiseInvalidArgumentException) before iterating. Same pattern at 2 sites (sandbox_sync/commands/pty.py:1116, sandbox_sync/commands/command.py:1048).Why this was flagged
The trigger is a call like
sandbox.pty.stream_input(pid, b"ls\n")orsandbox.commands.stream_input(pid, "hello\n"). That is the natural sync counterpart of JSinput.send(data)and asynchandle.send(data), both of which take onestr | bytes. Inrequests()(sandbox_sync/commands/pty.py and sandbox_sync/commands/command.py),for item in datawalks a bytes object as ints.stream_input_data(e2b/sandbox/commands/stream_input.py) then passes the int straight through aspayload, because onlystrgets encoded. The error is raised inside the request generator that connectrpc consumes. It then goes throughhandle_rpc_exception_with_healthand comes out as a generic RPC failure rather than a clear argument error. Astrargument silently becomes one StreamInput data message per character. No type guard exists. The annotationIterable[Union[str, bytes]]is not enforced at runtime, and astrsatisfies it anyway. This is a new API, so the base branch has no equivalent call.Verification: In both sync methods (packages/python-sdk/e2b/sandbox_sync/commands/pty.py and sandbox_sync/commands/command.py),
requests()doesfor item in data: yield stream_input_data("pty"|"stdin", item)without checking the type. So iterating abytesvalue hands it an int, which goes intoOneof("pty", payload)and fails during serialization.
|
check the findings above |
|
Checked the findings: all three are valid, and 49ea9df fixes them.
Each fix has a regression test. The targeted JS and Python tests, format, lint and typecheck pass. |
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
49ea9df to
5247ed5
Compare
Summary
Exposes envd's client-streaming
process.Process/StreamInputRPC: one long-lived ordered input stream per process instead of oneSendInputrequest per chunk (sequentialpty.sendInput()was the latency bottleneck in SDK-393).sendenqueues and returns immediately; the RPC drains the queue asstart→data… and sends akeepaliveafterKEEPALIVE_PING_INTERVAL_SEC(50s) idle. Sync Python sends each item as the iterable yields it, without keepalives.STREAM_INPUT_TIMEOUT_MS/STREAM_INPUT_TIMEOUT(60s),0= no limit; Python options are keyword-only. Closing the input stream does not close the process' stdin (usecloseStdin).The fetch API does not support streaming request bodies), sosrc/envd/streamingTransport.tsbuilds a Connect transport (createTransportfrom@connectrpc/connect/protocol-connect) with a smallhttpClientthat sends aReadableStreambody withduplex: 'half'through the same envd RPC fetch (headers, access token, undici dispatcher). All other RPCs stay on connect-web. Browsers need fetch upload streaming (Chromium over HTTP/2).DefaultHeadersInterceptor/LoggingInterceptorgainedintercept_client_stream(_sync); without them sandbox headers were missing and the proxy answeredInvalid host.Link to Devin session: https://app.devin.ai/sessions/40d699b8846f4ceabbdbcfdfec2b2012
Open in Devin Desktop: https://app.devin.ai/desktop/session/40d699b8846f4ceabbdbcfdfec2b2012?variant=devin
Requested by: @mishushakov