feat: add Docker sandbox backend with --sandbox docker flag#101
feat: add Docker sandbox backend with --sandbox docker flag#101devin-ai-integration[bot] wants to merge 1 commit intomainfrom
Conversation
- Add global --sandbox flag (worktree default, docker optional) - Add DockerService with image build, container lifecycle, exec support - Add DockerSandboxServiceLayer implementing SandboxService for Docker - Add dockerSandboxLayer runtime wiring for Docker backend - Restructure layer composition to avoid leaking backend-specific services - Add Docker path constants and package exports - All existing tests pass, typecheck and lint clean Co-Authored-By: Balint Orosz <orosz.balint.97@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const args = ["exec", "-w", cwd]; | ||
| if (options?.interactive) { | ||
| args.push("-it"); | ||
| } | ||
| args.push(name, "sh", "-c", command); |
There was a problem hiding this comment.
🔴 Missing shell quoting in execInContainer causes sh -c to receive only the first word, and shell operators to execute on the HOST
In runDocker (packages/core/src/common/docker.ts:105), all args are joined with spaces into a single string and executed with shell: true. When execInContainer at line 230 does args.push(name, "sh", "-c", command), the command string is not shell-quoted. After joining, a command like git checkout -b BRANCH 2>/dev/null || git checkout BRANCH becomes:
docker exec -w /workspace containerName sh -c git checkout -b BRANCH 2>/dev/null || git checkout BRANCH
The HOST shell interprets || as a conditional operator and 2>/dev/null as a redirection. So sh -c inside the container only receives git (without arguments), and the fallback git checkout BRANCH runs on the host machine, not in the container. This directly affects the caller at packages/core/src/workspace/adapter/docker-sandbox.service.ts:180-184.
Affected caller in docker-sandbox.service.ts
docker.execInContainer(
branchContainerName,
`git checkout -b ${project.branch} 2>/dev/null || git checkout ${project.branch}`,
)| const args = ["exec", "-w", cwd]; | |
| if (options?.interactive) { | |
| args.push("-it"); | |
| } | |
| args.push(name, "sh", "-c", command); | |
| const cwd = options?.cwd ?? DOCKER_WORKSPACE_DIR; | |
| const args = ["exec", "-w", cwd]; | |
| if (options?.interactive) { | |
| args.push("-it"); | |
| } | |
| args.push(name, "sh", "-c", `'${command.replace(/'/g, "'\\''")}'`); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| yield* Stream.runForEach(handle.stdout, (chunk) => { | ||
| stdout += chunk.toString(); | ||
| return options?.silent ? Effect.void : Console.log(chunk.toString()); | ||
| }); | ||
|
|
||
| yield* Stream.runForEach(handle.stderr, (chunk) => { | ||
| stderr += chunk.toString(); | ||
| return Effect.void; | ||
| }); |
There was a problem hiding this comment.
🔴 Sequential stdout then stderr reading in runDocker can deadlock when subprocess produces significant stderr output
runDocker reads stdout to completion (line 118) before starting to read stderr (line 123). With stdout: "pipe" and stderr: "pipe", these are backed by OS pipe buffers (~64KB on Linux). If the child process fills the stderr pipe buffer before stdout is fully consumed, the child blocks on its stderr write. Since the child is blocked, it cannot write more to stdout or close it, so the stdout Stream.runForEach never completes — deadlock. This is particularly risky for docker build (called from ensureImage at line 155), which sends build progress to stderr. Note the existing work-tree-sandbox.service.ts:31 avoids this by reading the combined handle.all stream instead.
Prompt for agents
In packages/core/src/common/docker.ts, the runDocker function (lines 101-137) reads stdout and stderr sequentially, which can deadlock. Replace the sequential Stream.runForEach calls on lines 118-126 with concurrent consumption. Options include: (1) Use Effect.all or Effect.fork to read both streams concurrently, e.g. yield* Effect.all([Stream.runForEach(handle.stdout, ...), Stream.runForEach(handle.stderr, ...)], { concurrency: 2 }); or (2) Use handle.all (the merged stream) like work-tree-sandbox.service.ts does, though this loses the ability to separate stdout from stderr for the return value.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const fs = yield* Effect.sync(async () => { | ||
| const { promises } = await import("node:fs"); | ||
| return promises; | ||
| }); | ||
|
|
||
| const dockerfilePath = `${dataRoot}/Dockerfile`; | ||
| yield* Effect.tryPromise({ | ||
| try: async () => { | ||
| await (await fs).mkdir(dataRoot, { recursive: true }); | ||
| await (await fs).writeFile(dockerfilePath, DOCKERFILE_CONTENT); | ||
| }, | ||
| catch: (error) => | ||
| new DockerError({ | ||
| message: `Failed to write Dockerfile: ${error instanceof Error ? error.message : String(error)}`, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🔴 AGENTS.md violation: Uses node:fs promises.writeFile instead of Bun APIs
The buildDockerfile function uses import("node:fs") and fs.promises.writeFile to write the Dockerfile content. AGENTS.md mandates: "Prefer Bun.file over node:fs's readFile/writeFile" and "Default to using Bun instead of Node.js." This should use await Bun.write(dockerfilePath, DOCKERFILE_CONTENT) and for mkdir, the Effect FileSystem service or import("node:fs").promises.mkdir (since Bun has no direct mkdir equivalent).
| const fs = yield* Effect.sync(async () => { | |
| const { promises } = await import("node:fs"); | |
| return promises; | |
| }); | |
| const dockerfilePath = `${dataRoot}/Dockerfile`; | |
| yield* Effect.tryPromise({ | |
| try: async () => { | |
| await (await fs).mkdir(dataRoot, { recursive: true }); | |
| await (await fs).writeFile(dockerfilePath, DOCKERFILE_CONTENT); | |
| }, | |
| catch: (error) => | |
| new DockerError({ | |
| message: `Failed to write Dockerfile: ${error instanceof Error ? error.message : String(error)}`, | |
| }), | |
| }); | |
| const dockerfilePath = `${dataRoot}/Dockerfile`; | |
| yield* Effect.tryPromise({ | |
| try: async () => { | |
| const { mkdir } = await import("node:fs/promises"); | |
| await mkdir(dataRoot, { recursive: true }); | |
| await Bun.write(dockerfilePath, DOCKERFILE_CONTENT); | |
| }, | |
| catch: (error) => | |
| new DockerError({ | |
| message: `Failed to write Dockerfile: ${error instanceof Error ? error.message : String(error)}`, | |
| }), | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const exists = yield* Effect.try({ | ||
| try: () => { | ||
| require("node:fs").accessSync(mountPath); | ||
| return true; | ||
| }, | ||
| catch: () => new DockerError({ message: `Path ${mountPath} not accessible` }), | ||
| }).pipe(Effect.catchTag("DockerError", () => Effect.succeed(false))); |
There was a problem hiding this comment.
🔴 AGENTS.md violation: Uses require("node:fs").accessSync instead of Bun APIs
The createContainer function uses require("node:fs").accessSync(mountPath) to check if a path exists. AGENTS.md mandates "Default to using Bun instead of Node.js." This should use await Bun.file(mountPath).exists() (or Bun.file(mountPath).size to check existence) wrapped in an appropriate Effect combinator.
| const exists = yield* Effect.try({ | |
| try: () => { | |
| require("node:fs").accessSync(mountPath); | |
| return true; | |
| }, | |
| catch: () => new DockerError({ message: `Path ${mountPath} not accessible` }), | |
| }).pipe(Effect.catchTag("DockerError", () => Effect.succeed(false))); | |
| const exists = yield* Effect.tryPromise({ | |
| try: () => Bun.file(mountPath).exists(), | |
| catch: () => new DockerError({ message: `Path ${mountPath} not accessible` }), | |
| }).pipe(Effect.catchTag("DockerError", () => Effect.succeed(false))); |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Adds a Docker-based sandbox backend as an alternative to the existing worktree backend. The Docker backend is opt-in via
--sandbox docker; worktree remains the default.New files:
packages/core/src/common/docker.ts—DockerServicefor image builds, container lifecycle, exec, listingpackages/core/src/workspace/adapter/docker-sandbox.service.ts—DockerSandboxServiceLayerimplementingSandboxServiceportpackages/core/src/runtime/docker.runtime.ts— Effect layer wiring for the Docker backendModified files:
packages/cli/src/common/global-flags.ts— adds--sandboxglobal flag +getSandboxBackendFromArgv()helperpackages/cli/src/index.ts— selects runtime layer based on--sandboxargvpackages/core/src/runtime/local-work-tree.runtime.ts— refactored to useLayer.provide(instead ofLayer.provideMerge) forTmuxService/InteractiveCommandServiceso backend-specific services don't leak into the layer output type. This is required so both runtimes produce the same output type for the ternary inindex.ts.Review & Testing Checklist for Human
buildDockerfileusesEffect.syncwrapping an async import (docker.ts:82-85).Effect.syncexpects synchronous code but the callback returns a Promise. Verify this doesn't cause a runtime failure when the image is first built. Consider replacing withEffect.promiseor a top-level import.getSandboxBackendFromArgv()manually parsesprocess.argvbefore Effect CLI processes flags. It doesn't handle--sandbox=docker(equals syntax) and could be confused by positional args. Verify the flag is parsed correctly for all subcommands.docker.ts:113-121). Branch names with shell metacharacters (e.g.feat/foo;rm -rf /) could cause injection. Verify the shell command construction is safe, or consider using array-based spawning withoutshell: true.resolveContainerFromCwdreverse-engineers repo/branch from host filesystem paths. If path conventions inDEFAULT_REPOSITORY_ROOTorDEFAULT_WORK_TREE_ROOTchange, this will silently break. Consider whether the project model should be passed directly instead.sk clone <repo> --sandbox docker— creates host checkout + main containersk workspace create --repository <repo> --branch feat/x --sandbox docker— creates branch containersk workspace run --repository <repo> --branch feat/x --command 'pwd' --sandbox docker— runs in containersk workspace attach --repository <repo> --branch feat/x --sandbox docker— interactive shellsk workspace remove --repository <repo> --branch feat/x --sandbox docker— cleans up--sandbox)Notes
localWorkTreeLayerrefactor (switching fromLayer.provideMergetoLayer.providefor sandbox infra) is a structural change needed to unify the output types of both runtimes. Existing tests pass, but watch for any runtime regressions in the worktree path.detachis intentionally a no-op per spec.initrequires the host checkout to already exist — clone handles creating it, theninitadds the container on top.Link to Devin session: https://app.devin.ai/sessions/26bba5549cdd4281adef04dedcb6420e
Requested by: @blntrsz