Repository navigation
feat: add e2b-acp for running ACP agents in a sandbox - #2026
max-sudolabs wants to merge 19 commits into
Conversation
🦋 Changeset detectedLatest commit: c5734e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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. |
![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 changed code only: the new e2b-acp package (e2b_acp/__init__.py, transport.py, utils.py) and the core Commands._start(retain_output=...) change.
Checked: parity (T-1, T-2), API shape and keyword-only options (T-3/T-3a, T-9–T-11, T-14), types and naming (T-15, T-16, T-22), handles and streaming (T-26–T-30), lifecycle return values (T-32), defaults and timeouts (T-44, T-45, T-47), envd version gating (T-55), exports (T-54), errors (T-57–T-59, T-62, T-63) and docstrings (T-69, T-71, T-72).
6 violations, all inline:
- T-26:
AcpAgentis a handle for a running process but isn't named*Handle - T-22: the
AcpAgentExitresult type has noResult/Infosuffix - T-47/T-69 (×2): the defaults of
stop(timeout=)andmax_message_bytesaren't documented - T-62: the message-size error doesn't point to
max_message_bytes - T-54:
StderrHandlerandDEFAULT_MAX_MESSAGE_BYTESare missing from__all__
Not tied to a line:
- T-1/T-2 parity:
e2b-acpis async-Python only (no syncSandboxvariant), and the JS package is in a separate PR. The reason (the ACP Python SDK is async-only) is stated in the PR and README, so this is a scope decision, not a violation. It does mean that when both PRs land, the option names (cwd,envs,user,on_stderr/onStderr,max_message_bytes/maxMessageBytes,request_timeout/requestTimeoutMs) and the class names above should match 1:1 with the JS PR (T-1a, T-10). If the T-26/T-22 renames are taken, apply them in both PRs. - The core change follows the rules:
retain_outputis private (_start, handle constructors), it is mirrored in sync and async (T-2), and the envd check goes throughENVD_ENVD_CLOSE(T-55). - Things done right: keyword-only options with
*(T-3a),Usernameinstead ofstr(T-11), a sync or asyncon_stderrthat is awaited for backpressure (T-27),stop()rather thanclose()on the public handle (T-30),kill()returningbool(T-32),InvalidArgumentExceptionfor pre-start validation (T-59), and a@dataclassresult (T-16).
| from .utils import DEFAULT_MAX_MESSAGE_BYTES | ||
|
|
||
|
|
||
| class AcpAgent: |
There was a problem hiding this comment.
T-26 — anything open-ended (a running command, a PTY, a watch) returns a dedicated *Handle class whose constructor is internal. AcpAgent is exactly that shape (pid, wait(), stop(), kill() over a running process, obtained only from connect_acp_agent), so name it like CommandHandle / WatchHandle and keep __init__ out of the documented surface. Rename the references in __all__, the return annotation and the docstrings too.
| class AcpAgent: | |
| class AcpAgentHandle: |
There was a problem hiding this comment.
Leaving the name for now, together with the related T-4 comment on #2025 (AcpAgent.start instead of connectAcpAgent). AcpAgent also carries the ACP connection, not only the process.
@mishushakov, if you want AcpAgentHandle and a start factory, I'll rename both packages and the templates, cookbook and docs at once.
|
|
||
|
|
||
| @dataclass | ||
| class AcpAgentExit: |
There was a problem hiding this comment.
T-22 — result types end in Info / Result / Metrics. This dataclass is what wait() returns, the same role as CommandResult, so give it a result suffix (and update __all__ and AcpAgent.wait).
| class AcpAgentExit: | |
| class AcpAgentResult: |
| exited within `timeout` of this call. Messages the agent sends after | ||
| this call are not delivered. | ||
|
|
||
| :param timeout: Time for the agent to exit in **seconds** |
There was a problem hiding this comment.
T-47 / T-69 — defaults live in named constants and are documented in the docstring. DEFAULT_STOP_TIMEOUT exists, but the stop() docstring doesn't say what the default is, so it's missing from the rendered docs. Follow the SDK's existing phrasing (; defaults to ``STREAM_INPUT_TIMEOUT``` in commands`).
| :param timeout: Time for the agent to exit in **seconds** | |
| :param timeout: Time for the agent to exit in **seconds**; defaults to ``DEFAULT_STOP_TIMEOUT`` (5 seconds) |
| :param envs: Environment variables for the agent | ||
| :param user: User to run the agent as | ||
| :param on_stderr: Callback for the agent's stderr output (logs). The agent's stdout is reserved for the protocol | ||
| :param max_message_bytes: Maximum size of a single message from the agent in **bytes** |
There was a problem hiding this comment.
T-47 / T-69 — same here: max_message_bytes defaults to DEFAULT_MAX_MESSAGE_BYTES (32 MiB), but the docstring leaves the default out.
| :param max_message_bytes: Maximum size of a single message from the agent in **bytes** | |
| :param max_message_bytes: Maximum size of a single message from the agent in **bytes**; defaults to ``DEFAULT_MAX_MESSAGE_BYTES`` (32 MiB) |
| raise SandboxException( | ||
| f"ACP message exceeds the limit of {self._max_line_bytes} bytes" | ||
| ) |
There was a problem hiding this comment.
T-62 — error messages should be actionable and say what to do, not only what failed. This message gets to the caller through receive() / wait(), and the user can fix it with max_message_bytes, so name that option (T-63: use the option name the user typed).
| raise SandboxException( | |
| f"ACP message exceeds the limit of {self._max_line_bytes} bytes" | |
| ) | |
| raise SandboxException( | |
| f"ACP message exceeds the limit of {self._max_line_bytes} bytes. " | |
| "Pass a larger `max_message_bytes` to `connect_acp_agent()` if the agent sends larger messages." | |
| ) |
There was a problem hiding this comment.
Done in bb82fe4: the message names max_message_bytes and connect_acp_agent().
| return AcpAgent(transport, acp.connect_to_agent(client, transport)) | ||
|
|
||
|
|
||
| __all__ = ["DEFAULT_STOP_TIMEOUT", "AcpAgent", "AcpAgentExit", "connect_acp_agent"] |
There was a problem hiding this comment.
T-54 — everything public is re-exported from the package entry point and listed in __all__. StderrHandler is the type of the public on_stderr parameter, and DEFAULT_MAX_MESSAGE_BYTES is the public default of max_message_bytes. Both are imported here but not exported, so callers can only reach them through the internal e2b_acp.transport / e2b_acp.utils modules.
| __all__ = ["DEFAULT_STOP_TIMEOUT", "AcpAgent", "AcpAgentExit", "connect_acp_agent"] | |
| __all__ = [ | |
| "DEFAULT_MAX_MESSAGE_BYTES", | |
| "DEFAULT_STOP_TIMEOUT", | |
| "AcpAgent", | |
| "AcpAgentExit", | |
| "StderrHandler", | |
| "connect_acp_agent", | |
| ] |
There was a problem hiding this comment.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| # Stop reading agent output until the client catches up. | ||
| while len(self._pending) >= READ_QUEUE_MESSAGES and not ( | ||
| self._closing or self._ended | ||
| ): | ||
| self._space.clear() | ||
| await self._space.wait() |
There was a problem hiding this comment.
🔴 Agent wait deadlocks on full queue
When the client stops draining 64 queued updates, agent.wait() can hang after the agent exits. _deliver blocks the command handle's output callback, preventing its exit event from reaching _watch_exit.
Learn more
The command handle dispatches stdout through an awaited callback before it processes the process-end event in AsyncCommandHandle._handle_events. When _pending reaches 64 entries, _deliver waits for a reader to pop an entry. If the reader has stopped, the callback never returns, so the command handle never observes the already completed process and AcpTransport.wait never finishes. close() explicitly releases this wait, but wait() does not.
Example: An agent sends 65 updates and exits while the client no longer reads updates. The 65th output callback waits for queue space; await agent.wait() never returns the exit code.
Recommended fix: Make process-exit observation independent of a blocked output callback, or ensure wait() releases the queue backpressure while preserving its queued messages and exit status. Add an offline test for an exited process with a full queue and no receiver.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I don't think this happens with the ACP client. ClientSideConnection._receive_loop calls transport.receive() in a loop and runs every request and notification handler as a separate task (_process_message uses self._tasks.create(...)), so it keeps draining the queue even when a handler is slow. The queue stays full only if the connection is no longer reading, and then stop() and kill() release the blocked callback (covered by offline tests). Making wait() drop the limit would turn the bounded queue into an unbounded one while the agent runs. Separately, aec6126 now kills the agent when its output stream fails.
![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: 25e507324d
ℹ️ 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".
| request_timeout: Optional[float], | ||
| on_stdout: Optional[OutputHandler[Stdout]], | ||
| on_stderr: Optional[OutputHandler[Stderr]], | ||
| retain_output: bool = True, |
There was a problem hiding this comment.
When a JS integration streams a long-running verbose command, this new callback-only path is unavailable: packages/js-sdk/src/sandbox/commands/commandHandle.ts:282-296 still appends every decoded chunk to _stdout/_stderr, and JS Commands has no retainOutput equivalent. That leaves the corresponding JS use case with unbounded handle memory while the sync and async Python paths are fixed; thread the option through the JS start and handle implementations too.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is in the JS PR: #2025 adds the non-retaining path to the JS SDK (startStreaming, with retainOutput threaded through start and CommandHandle) with a test. The JS and Python parts were split into two PRs on request.
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 suite when the package or the Python SDK changes, and include it in the release preflight, tests and publish.
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.
…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.
A failed output stream, for example after on_stderr raises, left the agent running with no working connection. The transport now kills it; receive() and wait() still raise the original error.
…e size error at max_message_bytes Also document the defaults of stop() and max_message_bytes, export DEFAULT_MAX_MESSAGE_BYTES and StderrHandler, and describe the Python SDK streaming hook as internal in the changeset.
…ption e2b-dev#2044 added retain_output to commands.run and commands.connect, so the private Commands._start hook this branch carried is gone. The transport starts the agent with run(background=True, retain_output=False) and the changeset no longer bumps the Python SDK. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bb82fe4 to
c5734e6
Compare
Summary
Adds
e2b-acp(packages/acp-python) for running Agent Client Protocol agents (goose, OpenCode, Claude Code through its ACP adapter) inside an E2B sandbox and driving them with the official Python ACP client,agent-client-protocol.Async
connect_acp_agent(sandbox, command, client, *, ...)starts the agent with stdin open, speaks newline-delimited JSON-RPC over its stdin/stdout through anacpTransport(send/receive/close), and returns the ACPClientSideConnectionpluspid,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. The ACP Python SDK is async-only, so the package takes anAsyncSandbox.The agent is started with
commands.run(..., background=True, retain_output=False)from #2044 (SDK-407), so a long-running agent does not grow the handle's buffers. No core changes. SDK-408 tracks dropping retained output for background commands in the next major.Split out of #1992 together with the JS package (#2025). The two PRs are independent and can merge in either order.
Usage
Review guide
The diff is +2.8k lines, but
uv.lock(1.3k, generated), tests and CI make up most of it.Worth reading (~570 lines):
packages/acp-python/e2b_acp/transport.pypackages/acp-python/e2b_acp/utils.pyinitializerewrite,fs/*/terminal/*rejectionpackages/acp-python/e2b_acp/__init__.pyconnect_acp_agentandAcpAgentSkim:
.github/workflows/acp_python_tests.ymland the ACP Python entries insdk_tests.yml/release.yml(mirror code-interpreter),pyproject.toml,package.json, README,.changeset/acp-python-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 droppedrestrict_client_capabilities,filter_agent_messagetest_utils.py; end to end intest_transport.py_write_lockstop(timeout=…): EOF and exit are bounded bytimeoutfrom the call; then kill unless an exit status is known; 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_closingchecks in_handle_stdoutand_deliverreceive()never loses a message_readable/_spaceeventsLineSplitterConnection closed, so callers usewait()andon_stderr_watch_exiton_stderr) kills the agent and keeps the original error_watch_exitOut of scope: restricting network or MCP, batch semantics beyond the boundary check, a sync API, reconnecting to a running agent.
Testing
e2b-acp: 34 tests (unit, offline, live), ruff and ty on this branch rebased on currentmain(includes feat(sdk): add retainOutput option to command handles #2044).claude-agent-acp0.90.0) wrote and ran a file in the sandbox. 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 package needsretain_outputfrom feat(sdk): add retainOutput option to command handles #2044, which is not released yet.e2b-acpneeds trusted publishing configured before the first release.pkg_artifacts.ymlpreview builds and staging jobs insdk_tests.yml. Happy to add either if you want them.