Repository navigation
feat: add @e2b/acp and e2b-acp for running ACP agents in a sandbox - #1992
max-sudolabs wants to merge 26 commits into
Conversation
Add an internal Commands.startStreaming() that starts a background command whose stdout/stderr only reach the callbacks and are not accumulated on the CommandHandle, so long-lived processes do not grow memory without bound.
Add an internal retain_output flag to Commands._start and both command handles, so long-lived processes can stream stdout/stderr through callbacks or iteration without accumulating it in memory. Mirrors the JS Commands.startStreaming().
New package for running Agent Client Protocol agents in E2B sandboxes. This first part has the newline-delimited JSON framing with a validated per-message size limit, and the rules that keep the agent's tools in the sandbox: initialize (also inside a batch) turns off the client fs and terminal capabilities, and agent fs/* and terminal/* requests are answered with method-not-found.
connectAcpAgent(sandbox, { command, client }) starts the agent over a
streaming command and returns an AcpAgent with the ACP
ClientSideConnection, close(), wait() and kill().
Writes, transport replies and EOF share one queue. close() bounds the
whole shutdown from the call, kills an agent whose output stream failed
without a known exit, and stops delivering later output. Closing the
writable is a graceful EOF that still reports a failed exit and a
failure to close stdin.
A small Python agent in a sandbox exercises the capability rewrite, rejected fs/terminal calls, ordering, UTF-8 split across bytes, the parse-error reply, the size limit, EOF, kill on close, failed exits, a failing stderr callback and cancellation, without any model or key.
Python counterpart of @e2b/acp: newline-delimited JSON framing with a validated per-message size limit, the initialize capability rewrite (also inside a batch) that turns off client fs and terminal, and method-not-found replies for agent fs/* and terminal/* requests.
connect_acp_agent(sandbox, command, client) starts the agent in an AsyncSandbox and returns an AcpAgent with the agent-client-protocol connection, close(), wait() and kill(). Async only: the ACP Python SDK is asynchronous. Messages reach receive() through a deque, so a cancelled receive never loses one. Writes, replies and EOF share one lock. close() bounds the whole shutdown from the call, kills an agent whose output stream failed without a known exit, keeps buffered messages and drops later ones, also when the reader waited for room in the queue.
Run the @e2b/acp and e2b-acp suites when their packages or the base SDKs change, and include them in the release preflight, tests and publish.
🦋 Changeset detectedLatest commit: 6c1a1a4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
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. |
|
Related PRs:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
TASTE.md review of the SDK-facing surface this PR changes: @e2b/acp (packages/acp-js/src), e2b-acp (packages/acp-python/e2b_acp), and the retainOutput/startStreaming changes in packages/js-sdk and packages/python-sdk.
Checked: parity (T-1, T-2), API shape and keyword-only kwargs (T-3, T-3a), handles and streaming (T-26, T-27, T-28, T-30), timeouts and cancellation (T-44 to T-47), config (T-49, T-50), envd version gating (T-55), packaging and exports (T-54, T-56), errors (T-57 to T-59, T-62), docs (T-69 to T-71).
7 inline comments covering 6 rules:
- T-3 and T-3a: required args are positional in JS, and Python optionals need a bare
* - T-30:
close()should bestop()(JS and Python) - T-47: the 5 s default appears as a literal in both SDKs
- T-46:
signalis dropped fromAcpStartOpts - T-1 / T-24:
startStreaming(public in JS) vs. private_start(retain_output=…)in Python
Not tied to a single line:
- T-1 / T-2 (parity):
e2b-acponly acceptsAsyncSandbox, and there is no syncSandboxpath. T-28 explains why a background-pumped transport can't be sync, so this may be intentional. If so, say so in the README/docstring. Otherwise a sync variant should mirror the async one. - T-26 (handles):
AcpAgentis the handle for an open-ended process (pid,wait,kill). The@hiddenconstructor follows the convention, but Python'sAcpAgent.__init__is not marked internal. Consider whether the name should follow the*Handleconvention. - T-62 / T-69 (docs): the
connectAcpAgent/connect_acp_agentdocs don't say when they throw. Examples: start failure,InvalidArgumentErrorfor a badmaxMessageBytes, and theSandboxErrorthe stream fails with on a non-zero agent exit or an oversized message. Add@throws/:raises:.
Compliant: DEFAULT_MAX_MESSAGE_BYTES constant, InvalidArgumentError for argument validation (T-59), Username type in Python (T-11), @dataclass result AcpAgentExit (T-16), kill() returning bool (T-32), ENVD_ENVD_CLOSE gating (T-55), and on_stderr/onStderr accepting sync or async callbacks and awaiting them (T-27).
| export async function connectAcpAgent( | ||
| sandbox: Sandbox, | ||
| opts: ConnectAcpAgentOpts | ||
| ): Promise<AcpAgent> { | ||
| const { command, client, ...startOpts } = opts | ||
| const transport = await startAcpTransport(sandbox, command, startOpts) |
There was a problem hiding this comment.
T-3 — required arguments are positional, optional parameters go in a trailing options object. command and client are both required, yet they live inside ConnectAcpAgentOpts next to the optional cwd/envs/maxMessageBytes. This also breaks T-1 parity: Python already takes them positionally (connect_acp_agent(sandbox, command, client, ...)).
Take them positionally, make the options object optional, and remove command/client from ConnectAcpAgentOpts (that leaves it as AcpStartOpts):
| export async function connectAcpAgent( | |
| sandbox: Sandbox, | |
| opts: ConnectAcpAgentOpts | |
| ): Promise<AcpAgent> { | |
| const { command, client, ...startOpts } = opts | |
| const transport = await startAcpTransport(sandbox, command, startOpts) | |
| export async function connectAcpAgent( | |
| sandbox: Sandbox, | |
| command: string, | |
| client: Client | ((agent: Agent) => Client), | |
| opts?: ConnectAcpAgentOpts | |
| ): Promise<AcpAgent> { | |
| const transport = await startAcpTransport(sandbox, command, opts) |
There was a problem hiding this comment.
Fixed in 833990b: connectAcpAgent(sandbox, command, client, opts?); the options object holds only optional settings (AcpStartOpts).
| client: acp.Client, | ||
| cwd: Optional[str] = None, |
There was a problem hiding this comment.
T-3a — Python optionals have to be keyword-only, enforced by a bare *. Without it, cwd, envs, user, on_stderr, max_message_bytes and request_timeout can all be passed by position, so reordering or adding an option later is a breaking change. This is a new function, so it should have the * from the start.
| client: acp.Client, | |
| cwd: Optional[str] = None, | |
| client: acp.Client, | |
| *, | |
| cwd: Optional[str] = None, |
(T-1: JS also accepts a factory, Client | ((agent: Agent) => Client), but here only acp.Client is accepted. Either support the same input shape or drop the factory form from JS.)
There was a problem hiding this comment.
Fixed in 6c1a1a4. The factory form stays JS-only on purpose: it is the native shape of each ACP SDK (new ClientSideConnection(toClient, stream) in TypeScript takes a factory, acp.connect_to_agent(client, transport) in Python takes an instance), and the package forwards it.
| * | ||
| * @param opts options for closing the agent. | ||
| */ | ||
| async close(opts?: AcpCloseOpts): Promise<void> { |
There was a problem hiding this comment.
T-30 — what you start, you stop: stop(), never close(). connectAcpAgent starts a process in the sandbox, and this method ends that process (EOF, then SIGKILL once the timeout passes). close() reads as releasing a connection. The same rename applies to AcpAgent.close in Python and to AcpCloseOpts → AcpStopOpts. The internal AcpTransport.close can keep its name where the ACP library's transport interface requires it.
| async close(opts?: AcpCloseOpts): Promise<void> { | |
| async stop(opts?: AcpStopOpts): Promise<void> { |
| """ | ||
| return await self._transport.wait() | ||
|
|
||
| async def close(self, timeout: float = 5) -> None: |
There was a problem hiding this comment.
T-30 (stop(), never close() for something you started), T-3a (an optional parameter needs a bare * to be keyword-only), and T-47 (defaults belong in a named module constant, not a magic 5). Also, timeout in this package means a per-call grace period, not the sandbox lifetime. A distinct name avoids conflating the two clocks (T-44/T-45). Something like:
| async def close(self, timeout: float = 5) -> None: | |
| async def stop(self, *, timeout: float = DEFAULT_STOP_TIMEOUT) -> None: |
(define DEFAULT_STOP_TIMEOUT = 5 next to DEFAULT_MAX_MESSAGE_BYTES and reuse it in AcpTransport.close, which repeats the literal at transport.py:104)
There was a problem hiding this comment.
Fixed in 6c1a1a4: stop(self, *, timeout: float = DEFAULT_STOP_TIMEOUT), and AcpTransport.close reuses the constant. Kept the name timeout: it is in seconds next to the sandbox's timeout, but documented as the time for the agent to exit, and request_timeout already covers the start request.
| () => false | ||
| ), | ||
| new Promise<boolean>((resolve) => { | ||
| timer = setTimeout(() => resolve(false), opts?.timeoutMs ?? 5_000) |
There was a problem hiding this comment.
T-47 — defaults live in named module constants, not as magic numbers at call sites. The 5_000 here only shows up in the @default JSDoc on AcpCloseOpts.timeoutMs, and Python has its own copy of it. Export a constant (e.g. DEFAULT_STOP_TIMEOUT_MS) next to DEFAULT_MAX_MESSAGE_BYTES and point the @default at it.
| timer = setTimeout(() => resolve(false), opts?.timeoutMs ?? 5_000) | |
| timer = setTimeout(() => resolve(false), opts?.timeoutMs ?? DEFAULT_STOP_TIMEOUT_MS) |
There was a problem hiding this comment.
Fixed in 833990b: exported DEFAULT_STOP_TIMEOUT_MS, referenced from the @default.
| */ | ||
| export interface AcpStartOpts extends Pick< | ||
| CommandStartOpts, | ||
| 'cwd' | 'envs' | 'user' | 'requestTimeoutMs' |
There was a problem hiding this comment.
T-46 — every networked JS method takes an AbortSignal. CommandStartOpts already has signal (via CommandRequestOpts), but this Pick drops it, so callers can't cancel a hung agent start. Add it here and pass signal: opts?.signal through to commands.startStreaming(...) at line 287.
| 'cwd' | 'envs' | 'user' | 'requestTimeoutMs' | |
| 'cwd' | 'envs' | 'user' | 'requestTimeoutMs' | 'signal' |
There was a problem hiding this comment.
Fixed in 833990b: signal is in AcpStartOpts and forwarded to commands.startStreaming().
| async startStreaming( | ||
| cmd: string, | ||
| opts?: Omit<CommandStartOpts, 'background'> | ||
| ): Promise<CommandHandle> { |
There was a problem hiding this comment.
T-1 / T-24 — the three surfaces mirror each other. In JS, the output-retention switch is a new sibling method on the public Commands class (startStreaming, hidden only through JSDoc). In Python it is a private retain_output parameter on _start, and e2b_acp calls sandbox.commands._start(...) and sandbox.commands._envd_version directly. That leaves the two SDKs with different contracts for the same capability, and the Python package depends on private SDK internals.
Use one mechanism in all three surfaces. Either keep it private everywhere, or expose it the same way in all three. If it is exposed, prefer an option on the existing method (e.g. run(cmd, { background: true, retainOutput: false }) ↔ run(cmd, background=True, retain_output=False)) over a startStreaming sibling, in line with T-24's one-method-plus-option shape. Note that retainOutput: false is a boolean behavior switch. T-14 prefers a string-literal option if a third mode is plausible.
There was a problem hiding this comment.
Both are internal and not part of the public surface: JS startStreaming is @hidden @internal, Python _start(retain_output=False) is private. JS has a separate method only because a TypeScript-private start can't be called from another package. If you'd rather make it public, I'd add retainOutput: false / retain_output=False to run(..., background: true) in all three SDKs, but that's a public API change I didn't want to make without your call.
| async kill(): Promise<boolean> { | ||
| return this.handle.kill() | ||
| } |
There was a problem hiding this comment.
🔴 Killed agents can hang during wait
When agent output fills the read queue, kill() leaves the stdout callback blocked. The command handle cannot process the exit event until that callback returns, so wait() hangs in both SDKs.
Learn more
The stdout callback pauses when its 64-message read queue fills: deliver waits for a reader, and the Python counterpart waits in _deliver. The command handle consumes output and the process exit on the same event loop; it cannot finish wait while the callback remains blocked. close() wakes that callback before killing, but direct kill() in both SDKs does not. A killed process can therefore leave its stream and wait() pending indefinitely.
Example: An agent emits 65 session updates while its client pauses processing. Calling agent.kill() kills its process, but agent.wait() never resolves because the 64th update blocked the output callback.
Recommended fix: Before killing, release the read backpressure in both AcpTransport.kill() implementations and prevent further output delivery. Preserve buffered messages or terminate the readable with a defined result; test kill while the queue is full and then await wait().
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed and fixed in cb4bd67 (JS) and 431618e (Python): kill() releases output waiting on the full read queue before killing, so the handle sees the exit. The offline fake handle now dispatches the exit only after output callbacks return, like the SDK handle, and the new tests fail on the old code.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe004fef4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| push(chunk: string): string[] { | ||
| const parts = (this.pending + chunk).split('\n') |
There was a problem hiding this comment.
When a single JSON-RPC message arrives in normal small stdout chunks (for example, a multi-megabyte content block), this rebuilds and splits the entire accumulated prefix on every chunk; after the prefix exceeds one quarter of the limit, the loop below also UTF-8-encodes the whole prefix on every chunk. The work is quadratic (a 4 MiB line in 4 KiB chunks already takes seconds) even though the documented default permits 32 MiB, so a valid large agent message can stall all protocol reads; keep chunks and byte counts incrementally and join only at a newline, in this implementation and the Python counterpart.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| let transport: AcpTransport | undefined | ||
| const closeTransport = () => transport?.close() ?? Promise.resolve() |
There was a problem hiding this comment.
When a fast agent emits an over-limit first stdout chunk while commands.startStreaming() is still resolving, CommandHandle can already be dispatching onStdout from the event loop started in its constructor. This helper then runs before transport is assigned, returns an already-resolved promise, and the line-limit handler merely errors the readable while leaving the agent process running; queue the close or kill against handleReady (as the Python path defers it) instead of making pre-initialization cleanup a no-op.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed for JS and fixed in cb4bd67: cleanup waits for handleReady and then stops the agent; covered by an offline test that fails on the old code. Python can't hit it: the handle's event task can't run before _start() returns to start(), which sets the exit task in the same step.
kill() now releases agent output that waits for the client, so the command handle can see the exit and wait() resolves. Cleanup after an oversized first message waits for the transport instead of doing nothing when the output arrives before startStreaming() returns.
The line splitter kept the unfinished line as one string and split it again on every chunk, which made a long message quadratic to read. It now keeps the chunks and their byte count and joins them once.
kill() now releases agent output that waits for room in the read queue, so the command handle can see the exit and wait() returns.
The line splitter kept the unfinished line as one string and split it again on every chunk, which made a long message quadratic to read. It now keeps the chunks and their byte count and joins them once.
…) to stop() connectAcpAgent(sandbox, command, client, opts?) matches the Python signature and keeps only optional settings in the options object. stop() ends the agent process that connectAcpAgent started, its default timeout is the exported DEFAULT_STOP_TIMEOUT_MS, and AcpStartOpts forwards an AbortSignal to the start request.
…op() connect_acp_agent() takes its optional settings as keyword arguments only. stop() ends the agent process that connect_acp_agent() started, and its default timeout is the exported DEFAULT_STOP_TIMEOUT.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| this.pending.push(text) | ||
| this.pendingBytes += encoder.encode(text).length | ||
| this.check(this.pendingBytes) |
There was a problem hiding this comment.
🟡 Split emoji falsely exceeds message limit
When an emoji spans two chunks, pendingBytes counts its surrogate halves as six bytes instead of four. A valid message at the byte limit fails and closes the ACP connection.
Learn more
The command SDK decodes stdout as a stream and can deliver the two UTF-16 halves of a decoded character in separate output chunks. TextEncoder.encode replaces a lone surrogate with a three-byte replacement character, while the combined pair encodes as four bytes. Encoding and summing the chunks separately therefore overcounts a line, although the completed line fits the byte limit. The Python splitter does not have this mismatch because Python strings contain whole Unicode code points after streaming decode.
Example: With maxMessageBytes: 4, push('\uD83D') followed by push('\uDE00\n') rejects the four-byte emoji as a six-byte line; push('\uD83D\uDE00\n') succeeds.
Recommended fix: Track a trailing high surrogate between append calls and account for the completed pair together, or use a stateful UTF-8 byte counter matching encoding of the assembled line. Add a test that splits an emoji across chunks at the exact limit.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Seems false positive. The splitter never sees a lone surrogate at a chunk boundary: CommandHandle decodes stdout with TextDecoder.decode(chunk, { stream: true }), which holds incomplete UTF-8 sequences until the code point is complete, so a supplementary character always arrives with both halves in one chunk. The case is only reachable by calling push() directly with a split pair.
|
@max-sudolabs can I ask you to split the PR into JS and Python? would make it easier to review |
|
As requested, this is now split into two PRs, both rebased on current
They are independent and can merge in either order. The package code is the same as here; each PR has its own changeset and CI entries, and the lockfiles are refreshed for the new base. I'll close this one once you're happy with the split. |
Summary
Adds two packages for running Agent Client Protocol agents (goose, OpenCode, Claude Code through its ACP adapter) inside an E2B sandbox and driving them with the official ACP client:
@e2b/acp(packages/acp-js):connectAcpAgent(sandbox, command, client), built on@agentclientprotocol/sdk(peer dependency).e2b-acp(packages/acp-python): asyncconnect_acp_agent(sandbox, command, client), built onagent-client-protocol.Both start the agent with stdin open, speak newline-delimited JSON-RPC over its stdin/stdout, and return the ACP
ClientSideConnectionpluspid,wait(),stop()andkill(). The client filesystem and terminal capabilities are turned off ininitialize, and agent requests forfs/*/terminal/*are rejected in the transport, so the agent edits files and runs commands with its own tools inside the sandbox. Network access and MCP servers passed to the agent are not restricted.Core change (patch, internal):
Commands.startStreamingin JS andCommands._start(retain_output=False)in Python start a background command whose output goes only to the callbacks and is not kept in memory, so a long-running agent does not grow the handle's buffers. Both are marked internal and only the ACP packages use them.Usage
Review guide
The diff is +4.35k lines, but most of it is not logic:
uv.lock(1.3k, generated), tests (1.1k), CI, manifests and READMEs.Worth reading (~1.3k lines):
packages/acp-js/src/transport.tspackages/acp-python/e2b_acp/transport.pyTransport(send/receive/close)packages/acp-js/src/utils.ts,packages/acp-python/e2b_acp/utils.pyinitializerewrite,fs/*/terminal/*rejectionpackages/acp-js/src/index.ts,packages/acp-python/e2b_acp/__init__.pyconnectAcpAgent/connect_acp_agentandAcpAgentpackages/js-sdk/src/sandbox/commands/*,packages/python-sdk/e2b/sandbox_{async,sync}/commands/*Skim:
.github/workflows/acp_*_tests.ymland the ACP entries insdk_tests.yml/release.yml(mirror code-interpreter), package manifests, READMEs,.changeset/acp-package.md.For automated reviewers
Behaviour the code is meant to guarantee, and the test that fails if it breaks. Offline tests use a fake command handle and need no API key; live tests start a scripted Python agent in a sandbox.
initializeforces them off (also inside a batch), agentfs/*/terminal/*requests get-32601, notifications are droppedrestrictClientCapabilities,filterAgentMessage/ same inutils.pyutils.test.ts,test_utils.py; end to end intransport.test.ts,test_transport.pyenqueueWrite/_write_lockstop(timeout): EOF and exit are bounded bytimeoutfrom the call; then kill unless an exit status is known; rejects/raises if the kill failsAcpTransport.closekill()releases output waiting on a full read queue, so the handle sees the exit andwait()returnsAcpTransport.killstop(), messages already queued stay readable and later ones are dropped, including later lines of a chunk that was waiting on backpressureclosingchecks inonStdoutanddeliver(JS),_handle_stdoutand_deliver(Python). They cover different cases and are not duplicatesreceive()never loses a message (Python)CountQueuingStrategy+resumeReading/ deque +_readable/_spaceeventsLineSplitterConnection closed, so callers usewait()andon_stderr_watch_exitstartStreaming()returns still stops the agentconnectAcpAgent,closeTransportOut of scope: restricting network or MCP, batch semantics beyond the boundary check, a sync Python API, reconnecting to a running agent.
Testing
@e2b/acp: 32 tests (unit, offline, live).e2b-acp: 34 tests. Core command tests: JS 36, Python 47.claude-agent-acp0.90.0) wrote and ran a file in the sandbox in both languages. The client received nofs/*orterminal/*calls, and the agent process was gone after it was stopped.Release notes for maintainers
pnpm run versionsetse2b-acp'se2brequirement to the core version being released, because the packages need the new core hook. On this release that ise2b>=2.56.1.@e2b/acpgets the same throughworkspace:^.@e2b/acpand PyPIe2b-acpneed trusted publishing configured before the first release.pkg_artifacts.ymlpreview builds (an ACP preview needs the matching preview core) and staging jobs insdk_tests.yml. Happy to add either if you want them.