Conversation
kaxil
force-pushed
the
sandbox-attach
branch
3 times, most recently
from
September 23, 2026 19:22
ef064e2 to
5bc7550
Compare
A task can now create a sandbox, hand an agent only its handle through SandboxToolset(attach_to=...), and read the agent's files out after the run before destroying it. Credentials go in from the provisioning task in ordinary Python at run time, a file the agent built comes out without crossing the model's context, and HITL review works with such an agent because both runs find the same files. A handle alone is never enough: the sandbox has to carry the owner the toolset presents, by default the Dag run, and one agent run holds it at a time. Modal supports this through AttachableSandboxBackend; sbx refuses it.
kaxil
force-pushed
the
sandbox-attach
branch
from
September 23, 2026 21:58
5bc7550 to
77f91a5
Compare
kaxil
marked this pull request as ready for review
September 23, 2026 23:16
Lee-W
reviewed
Sep 24, 2026
Lee-W
left a comment
Member
There was a problem hiding this comment.
will need some more time to finish a full round
| owner: str | None = None | ||
|
|
||
|
|
||
| def dag_run_owner(context: Mapping[str, Any]) -> str: |
Member
There was a problem hiding this comment.
Suggested change
| def dag_run_owner(context: Mapping[str, Any]) -> str: | |
| def extract_dag_run_owner(context: Mapping[str, Any]) -> str: |
I kinda feel i saw it somewhere. if this is the convention, then let's keep it
| def decode_network_policy(value: str | None) -> SandboxSpec | None: | ||
| """Read a :data:`NETWORK_TAG` value back into a spec carrying only the network fields, or ``None``.""" | ||
| if not value: | ||
| return None |
Member
There was a problem hiding this comment.
Should we raise instead of returning none? handling the exception like
try:
decode...
except ...:
...make more sense to me
Lee-W
reviewed
Sep 24, 2026
| clock = f"About {minutes} {unit} of its lifetime remained when this run began." | ||
| return f"{whose} {policy} {clock}" | ||
|
|
||
| def _attachable_backend(self) -> AttachableSandboxBackend: |
Member
There was a problem hiding this comment.
should we make it a property? same for other cases
Lee-W
reviewed
Sep 24, 2026
| raise RuntimeError("attach mode on a backend that cannot attach") | ||
| return self._attachable | ||
|
|
||
| def _identity(self) -> tuple[str, str]: |
Member
There was a problem hiding this comment.
should we make the return type a named tuple?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SandboxToolsetprovisions its own sandbox on the model's first tool call and destroys it when the run ends. Three things cannot be done inside that shape, and they have come up separately: a credential cannot come from a connection (the spec is fixed at parse time), a file the agent built cannot leave (only the model's text-only, capped context crosses the boundary), and HITL regeneration starts a second run whose history describes files that no longer exist, which is why #73529 refusesenable_hitl_reviewwith a sandbox.This PR gives them one answer. A
@taskcreates the sandbox and hands the agent only its handle:The toolset uses that sandbox for the run, leaves it standing at the end, and the task that created it reads the artifact out through the backend and destroys it.
enable_hitl_reviewis now accepted with an attached toolset, since the regenerated run finds the first run's files.durable=Truestays refused: a replayed tool result is not re-executed, so the workspace would not move with the transcript.Live run on Modal with a real model (Opus 4.8 through a gateway).
provisionstages the input,analyseattaches and writes the report,collectreads it out after the agent run ended and destroys the sandbox. The two red tasks are agents that were refused before running a single tool: one presented the wrong owner, one a handle that does not exist.Design rationale
A handle alone is never enough. An upstream XCom can be written by an agent, and a valid id for someone else's sandbox in the same Modal workspace is a valid id. So the provisioning task stamps an owner (
SandboxSpec.owner), and the toolset refuses to attach unless the sandbox carries the owner it presents. By default that is the Dag run, so two tasks in one run need no shared secret;owner=on the toolset covers a sandbox provisioned under another name. The docs are explicit about what the check is: the tags are written with the same vendor credential the attaching task holds, so it stops a run reaching the wrong sandbox by mistake and gives attribution. It is not a boundary between authors.One agent run holds a sandbox at a time. Attaching marks the sandbox with the task instance (Dag, run, task, map index, without the try number), and the run's end clears it. A different task is refused while the mark is there; the same task attaching again is allowed, so a retry finds the files of an attempt that died without releasing. Modal's tags have no conditional write, so the claim is a read-then-write that is read back once. That catches the sequential mistakes; two runs starting in the same instant can both pass, and the docs say so rather than promising a lock.
The lifetime and the network policy travel with the sandbox.
createstamps the expiry and the network policy as tags. On attach the toolset shortens commands to what is left, whatever backend the sandbox is on, and therun_commanddescription tells the model whose sandbox it is, what it can reach, and how much time remained when the run began. The note is fixed at attach rather than recomputed each step, so tool definitions stay stable for provider prompt caching. A backend that provisions a sandbox with an owner refuses anidle_timeout, since the first gap between tasks would reclaim it.Why the ownership rules live on a base class.
AttachableSandboxBackendadds two vendor primitives,read_tagsandwrite_tags, and the owner, holder and expiry rules are written once on top of them. Modal implements the two primitives;sbxruns a microVM on the worker that created it and cannot be reached from another task, so it stays a plainSandboxBackend, refusesSandboxSpec.owner, and the toolset refusesattach_tofor it at construction.How the handle is templated.
attach_toopts into the per-task-instance toolset rendering that #73578 added for connection IDs (agent_template_fields), so it is rendered on a copy wherever the toolset sits, inside wrappers, combined toolsets andToolsetcapabilities, and the toolset object in the Dag file is never mutated. A handle that renders to nothing (the provisioning task pushed no XCom, or the agent ran first) fails the run instead of falling back to a sandbox of the toolset's own.Gotchas
sandbox_timeout; size it for the review, or sethitl_timeoutbelow what will be left.ALL_DONEalso runs when provisioning itself failed and the handle isNone; the example checks that first, and the Modal backend now refuses a non-string handle with a message naming the cause instead of failing inside the SDK.modalextra's floor moves from 1.5.0 to 1.5.2: reading tags back is gated to V1 sandboxes in 1.5.0 and 1.5.1, so attaching would fail on a V2 sandbox there.The system test gains a task that provisions, attaches two agent runs in turn, refuses a run with the wrong owner, reads the file out through the backend and destroys the sandbox; it passed live against Modal.