Skip to content

feat(sandbox-adapter): add sandbox providers for agentOS, E2B, and Daytona - #5799

Open
eersnington wants to merge 3 commits into
mainfrom
feat/sandbox-adapter
Open

eersnington wants to merge 3 commits into
mainfrom
feat/sandbox-adapter

Conversation

@eersnington

@eersnington eersnington commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Adds @rivet-dev/sandbox-adapter, where an agent's file and shell tools run.

import { e2bProvider } from "@rivet-dev/sandbox-adapter/e2b";

const agent = pi({ model: "anthropic/claude-opus-5-5", sandbox: e2bProvider() });
  • SandboxProvider creates, connects, suspends, and destroys one sandbox per actor. Sandbox is exec plus file operations.
  • Providers for agentOS, E2B, and Daytona. Each SDK is an optional peer dependency.
  • Any other provider implements SandboxProvider.

This is part 1 of 3 in a stack:

@railway-app

railway-app Bot commented Sep 25, 2026

Copy link
Copy Markdown

This PR was not deployed automatically as @eersnington does not have access to the Railway project.

In order to get automatic PR deploys, please add @eersnington to your workspace on Railway.

@claude

claude Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review: @rivet-dev/sandbox-adapter (part 1 of 3)

Solid design overall: a narrow SandboxProvider/Sandbox interface, structural typing against agentOS to avoid a hard dependency, and optional peer deps for e2b/Daytona. A few issues worth addressing before merge.

Bug: Daytona provider breaks the timeoutMs: 0 contract

src/index.ts documents SandboxExecOptions.timeoutMs:

/** Kills the process after this many milliseconds. Undefined or 0 means no timeout. */
timeoutMs?: number;

e2b.ts and remote-process.ts both honor "0 means no timeout" (options.timeoutMs ?? 0 disables the SDK's own default in e2b; options.timeoutMs ? Date.now() + options.timeoutMs : undefined in runRemoteProcess treats 0 as "no deadline").

daytona.ts, however, only special-cases undefined:

options.timeoutMs === undefined ? undefined : Math.ceil(options.timeoutMs / 1000),

If a caller explicitly passes timeoutMs: 0 (a valid, documented way to request "no timeout"), this forwards 0 to sandbox.process.executeCommand's timeout parameter instead of undefined. Depending on how Daytona's SDK treats a 0 timeout, this could produce an immediate timeout or otherwise diverge from the other two providers' behavior for the same input. Suggest a truthy check instead, matching the other two implementations:

options.timeoutMs ? Math.ceil(options.timeoutMs / 1000) : undefined,

Possible unhandled rejection in e2b.ts

const kill = () => void handle.kill();
options.signal?.addEventListener("abort", kill, { once: true });

handle.kill() returns a promise that is discarded with void, not caught. If it rejects (e.g. the sandbox is already gone), this becomes an unhandled promise rejection outside the surrounding try/catch/finally chain, which can crash the host process under Node's default unhandled-rejection behavior. Worth a .catch(() => {}).

Parity gap: mkdir recursive semantics

The Sandbox.mkdir(path) interface doesn't document recursive-vs-non-recursive semantics. agentOSSandbox explicitly passes { recursive: true }, but e2bSandbox (sandbox.files.makeDir(path)) and daytonaSandbox (sandbox.fs.createFolder(path, "755")) rely on each SDK's own default for whether intermediate directories are created. If the two SDKs differ here, a tool built against one provider could silently break on another. Worth confirming (or documenting) that all three create intermediate directories the same way.

Test coverage

No tests are included. runRemoteProcess's polling/timeout/abort loop is the most bug-prone piece (per the timeoutMs issue above) and doesn't need a real sandbox SDK to test — it only depends on the small RemoteProcess interface (poll/kill), so it could be exercised with a hand-written fake process per this repo's "no vi.mock" testing convention, covering: dedup by sequence number, abort mid-poll, and deadline-triggered kill.

Minor

  • readAllOutput's inner pagination loop (agentos.ts) has no delay or bound between pages while hasMore is true; if the agentOS API were to report hasMore: true for an extended stretch, this would busy-loop tighter than the outer 100ms poll interval. Probably fine in practice, but worth a second look if agentOS ever streams large output.

Nothing else stood out; naming/terminology (agentOS, package scope, optional peer deps) all follow the conventions in CLAUDE.md.

🤖 Generated with Claude Code

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 4 medium-severity findings

Reviewed commit 0dbd4fe.

Comment thread shared/typescript/sandbox-adapter/src/daytona.ts Outdated
Comment on lines +61 to +62
const kill = () => void handle.kill();
options.signal?.addEventListener("abort", kill, { once: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Medium · Handle signals that abort before the listener is attached

AbortSignal.addEventListener does not replay an abort that already happened. If the signal is pre-aborted, or aborts while commands.run is awaiting startup, this listener never kills the handle; exec waits for the command to finish and only then throws. Check signal.aborted before launching and again immediately after acquiring the handle (killing it in the latter case) before installing the listener.

Comment thread shared/typescript/sandbox-adapter/src/agentos.ts
Comment thread shared/typescript/sandbox-adapter/src/remote-process.ts

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant