sandbox: dynamic directory mount & snapshot in go + adds detach#246
sandbox: dynamic directory mount & snapshot in go + adds detach#246thomasjpfan wants to merge 35 commits intomainfrom
Conversation
PR SummaryMedium Risk Overview Sandbox directory mounts are implemented via a new Written by Cursor Bugbot for commit 9427661. This will update automatically on new commits. Configure here. |
| "google.golang.org/protobuf/types/known/emptypb" | ||
| ) | ||
|
|
||
| // tlsCredsNoALPN is a TLS credential that skips ALPN enforcement, implementing |
There was a problem hiding this comment.
I think this whole thing should no longer be needing after @saltzm fixed something server side, so might work if you just remove it and use the default?
|
Updated PR with:
|
|
I updated PR:
While implementing this, I made some decisions that are debatable:
|
saltzm
left a comment
There was a problem hiding this comment.
Everything looks good mod the comment about task_command_router_insecure which isn't being used anymore in the python client
modal-go/sandbox.go
Outdated
| func (sb *Sandbox) ensureAttached() error { | ||
| if sb.detached.Load() { | ||
| return SandboxDetached{Exception: "Do not call Detach or Terminate until you are done with your sandbox in this session"} | ||
| return SandboxDetached{Exception: "Do not call Detach until you are done with your sandbox in this session"} |
There was a problem hiding this comment.
I think I might find this error message confusing if I do like sb.Exec() and get this back - I'll be like "why is it telling me not to call Detach, I'm calling Exec".
I'd expect something more like "Unable to perform operation on a detached sandbox" or "Operations on sandbox after Detach are not allowed" or some such thing
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR cherry picks files from the following PRs to just implement the dynamic directory mounts for Go:
This includes the task command router. I think this is a smaller change and does not impact any public API around
sb.exec. Once we have this in, then #242 is easier to review.