Repository navigation
Conversation
790743f to
beccdc8
Compare
chcodex
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Step 3 of the envd split: E2bSandbox now implements SandboxFileTransfer and routes both workspace archive directions through the REST helpers added in #3343 — persist writes the tar to a sandbox temp file and downloads it, hydrate uploads it and extracts — replacing the base64-chunk + python3 bootstrap. That removes an undeclared in-image dependency and the per-argument command-line ceiling, so the refactor is welcome; unit coverage for the new routing is solid (E2bSandboxFileTransferTest, 238 lines).
What gives me pause is the path gate. supportsFileTransfer accepts every absolute path, which is materially wider than the confinement the Docker and Kubernetes backends apply to the same interface, and the harness relies on the backend to decline paths it should not handle natively. I would resolve that before merge. The archive now also crosses two buffers (sandbox /tmp file plus a host byte[]) with no size ceiling anywhere in the chain, which is worth a decision even if #3344 is not where you want to fix it.
State at review time: CLA signed, Check License, Check Module Sync, build (ubuntu-latest) and codecov pass, build (windows-latest) still running. This branch also carries #3341's stream-parsing commit, whose four review findings (flag-order validation, truncated-stream acceptance, unbounded capture, and the envdAccessToken logged in the shared integration test) are still open — none are re-listed here.
Findings
- [Warning]
E2bSandbox.java:166— native transfer is unconfined: any absolute path is admitted, unlikeDockerSandbox.requireTransferPath/KubernetesSandbox.toFileApiRelative. - [Warning]
E2bSandbox.java:155— persist now needs the archive to fit in sandbox/tmpand in host heap, with no bound in either place; partialtaroutput is not cleaned up. - [Warning]
E2bSandbox.java:342— best-effort temp-file cleanup swallows failures at debug level, leaving a full workspace copy in the sandbox. - [Info]
E2bSandbox.java:207— extract now applies the bind-mount exclude args where the old path extracted everything: restore semantics changed, no test pins it. - [Info]
E2bSandbox.java:41—runShellBinaryStdoutloses its last caller in the module; worth deleting or keeping with a stated reason.
Not covered
No live E2B sandbox was used; behaviour was read from the diff and the mocked-transport tests.
Automated review by github-manager-bot
|
|
||
| @Override | ||
| public boolean supportsFileTransfer(String absolutePath) { | ||
| return absolutePath != null && absolutePath.startsWith("/"); |
There was a problem hiding this comment.
[Warning] supportsFileTransfer admits every absolute path, which makes E2B the only backend that puts no boundary on the native transfer channel. The two existing implementations both confine it:
DockerSandbox.requireTransferPathnormalizes the workspace root and throws"Path is outside the sandbox workspace"(and rejects./../ empty segments) —supportsFileTransferis just that check wrapped in a boolean;KubernetesSandbox.toFileApiRelativeadmits only paths undergetFileApiBaseDir()and returnsnullfor everything else, so the caller declines and the confined exec path handles it.
This matters because the harness delegates that decision to the backend: SandboxBackedFilesystem.resolveTransferPath validates and normalizes, then returns an absolute path unchanged (if (normalized.startsWith("/")) return normalized;) before probing supportsFileTransfer; downloadFiles probes the raw path with no resolution at all. So with this implementation, an absolute path anywhere envd's run user can reach (/home/user/.ssh/*, /etc/profile.d/*, another agent's scratch) is written and read natively rather than declined, and a declined path still has the archive fallback — so nothing is lost by confining.
Suggest resolving against e2bState.getWorkspaceRoot() and rejecting traversal segments the way Docker does, returning false instead of throwing. The temp-tar flow in this class is not affected: it calls envd().uploadFile/downloadFile directly, so it keeps working whatever the public probe decides.
Related history in this repo: #3292 (namespace boundary for absolute paths in ROOTED mode), #3314 (skill staging traversal) — a per-backend divergence here tends to get read as the intended contract.
| .append(" -C ") | ||
| .append(shellSingleQuote(root)) | ||
| .append(" ."); | ||
| envd().runShell(e2bState, root, script.toString(), TAR_TIMEOUT_SECONDS); |
There was a problem hiding this comment.
[Warning] Persist now materializes the archive twice: tar writes it into the sandbox filesystem, then downloadFile(tarPath) (#3343) reads it back as one byte[] before wrapping it in a ByteArrayInputStream. Two consequences worth documenting or bounding:
- Sandbox-side.
/tmpis tmpfs in many E2B templates, so the temp tar is charged against sandbox RAM and peaks at roughly workspace + archive. The only signal is a non-zerotarexit viaExecException, which will read as a tool failure rather than a capacity problem. - Host-side. Combined with the missing ceiling in feat(e2b): transfer files via envd Filesystem REST API [2/3] #3343's
res.body().bytes(), a large workspace becomes a full-archive heap allocation per concurrent persist. TherunShellBinaryStdoutpath this replaces at least sat behind the 64 MiB frame cap.
Also a small cleanup hole: the try starts after runShell, so if tar itself fails part-way the partial /tmp/agentscope-ws-*.tar is never removed.
| try { | ||
| envd().runShell(e2bState, root, "rm -f " + tarPath, 30); | ||
| } catch (Exception e) { | ||
| log.debug("[sandbox-e2b] remove temp tar best-effort: {}", e.getMessage()); |
There was a problem hiding this comment.
[Warning] A failed rm leaves a complete copy of the workspace in the sandbox /tmp — archives routinely contain .env files, tokens and model output — and the only trace is one debug line carrying e.getMessage(). Since this is the point at which the sandbox is most likely to be snapshotted or kept alive, please log at warn with the path and note in the javadoc that cleanup is best-effort, so a leftover tar is distinguishable from a leak when someone is debugging.
| WorkspaceMountSupport.tarExcludeArgsForBindMounts(e2bState.getWorkspaceSpec())) { | ||
| script.append(ex).append(' '); | ||
| } | ||
| script.append("xf ").append(tarPath).append(" -C ").append(shellSingleQuote(root)); |
There was a problem hiding this comment.
[Info] Two things this refactor improves, worth recording so they are not regressed later: the python3 dependency inside the sandbox image is gone, and the B64_CHUNK loop disappears along with the ~128 KiB per-argument ceiling the SandboxFileTransfer javadoc calls out.
One behaviour delta to confirm as intended: the extract command now carries tarExcludeArgsForBindMounts(...), whereas the replaced subprocess.run(['tar','xf','-','-C',d]) extracted everything. On re-hydrate, files under a bind-mounted subtree are therefore silently skipped rather than overwritten. Probably what you want, but it is a restore-semantics change rather than a pure transport swap, and E2bSandboxFileTransferTest covers upload/download routing and the archive fallback — not exclude-on-extract.
| * snapshot API and {@link E2bSnapshotRefs} marker bytes in the Harness snapshot stream. | ||
| */ | ||
| public class E2bSandbox extends AbstractBaseSandbox { | ||
| public class E2bSandbox extends AbstractBaseSandbox implements SandboxFileTransfer { |
There was a problem hiding this comment.
[Info] Replacing the last call site at line 153 leaves E2bEnvdProcessClient.runShellBinaryStdout (and the binary-stdout capture behind it) with no caller in this module. Either delete it on this branch or add a line saying why it stays: #3341 changes the stream decoder that this method depends on, and keeping an unused transport means the parse-hardening has to be tested against a path nothing exercises.
Also fine to see AbstractBaseSandbox + SandboxFileTransfer composition here matching the Docker/K8s shape — the interface javadoc's "declined paths fall back to exec" contract is what makes the backend-specific confinement discussion above actionable.
Implement SandboxFileTransfer on E2bSandbox so SandboxBackedFilesystem prefers the envd Filesystem REST API over base64-over-exec automatically. Only absolute paths are accepted; anything else falls back to the exec strategy. Workspace persist/hydrate use the REST API for tar transfer.
beccdc8 to
0de0d0b
Compare
This branch has not been deployed
2 participants
Stacked PR 3/3
Split from #2196. Merge order: #3341 → #3343 → #3344.
Base is
main, so this diff currently also carries #3341 and #3343. Once both merge, this diff shrinks to 2 files / +304 —E2bSandbox implements SandboxFileTransferplus its tests.Review only the top commit (
feat(e2b): route native file transfer through SandboxFileTransfer); that is the only change this PR owns.Note: this replaces the original #2196 design, which introduced
E2bSandboxFilesystem/E2bFilesystemSpecand acreateFilesystem()factory onSandboxFilesystemSpec. Implementing the existing optionalSandboxFileTransferinterface instead keeps the change inside the e2b module and adds no new abstraction —SandboxBackedFilesystemalready prefers it over base64-over-exec.Fixes #3340
Split from #2196 (stacked on #3339's scope: only the top commit is new).
What
E2bSandbox implements SandboxFileTransfer(supportsFileTransferaccepts absolute paths only; relative paths fall back to the exec strategy).tar xf/ single tar + download), replacing shell+base64.SandboxBackedFilesystemis used.Tests
E2bSandboxFileTransferTest: 10/10 pass (native dispatch, failure reporting, exec fallback, persist/hydrate tar paths incl. cleanup-on-failure).E2bEnvdProcessClientTest: 35/35 pass, no regression.spotless:check: pass.