feat(sandbox): unify worker tool execution with tools.ts via runner injection#83
Closed
feat(sandbox): unify worker tool execution with tools.ts via runner injection#83
Conversation
…njection - Add CommandRunner type and optional runner param to executeTool, defaulting to Bun.spawn-based runCommand for backward compatibility - Export ALLOWED_GIT_COMMANDS constant from tools.ts, derived from the same set as the TypeBox schema - Replace worker's duplicated rg/find/ls/read switch cases with single executeTool(name, args, worktree, sandboxRunner) call - Add makeSandboxRunner() that wraps commands with bwrap + seccomp - Keep git as special case (different bwrap topology for bare repo mount) - Remove duplicated validatePath from worker (handled by tools.ts) - Update Containerfile: install fd, copy tools.ts, install runtime deps - Change docker-compose build context to project root for tools.ts access - 18 new tests: runner injection (mock runner for each tool, path validation before runner), worker parity, ALLOWED_GIT_COMMANDS Closes #78
- Replace string-prefix error dispatch with structured CommandResult type (exitCode + output). CommandRunner now returns CommandResult instead of string. Worker checks tool names against TOOL_NAMES set instead of parsing output prefixes. - Derive TypeBox git schema from ALLOWED_GIT_COMMANDS_LIST constant (single source of truth, was duplicated before) - Add git subcommand allowlist validation in executeGit (was only enforced in sandbox worker, not in local mode) - Replace source-text-based worker tests with behavioral tests - Add .dockerignore to exclude node_modules/test/.git from build context - Reorder Containerfile layers: deps install before source copy for better Docker layer caching
…ror HTTP responses
makeSandboxRunner now returns a SandboxRunner with a lastExitCode
field. handleTool checks this after executeTool returns:
- exitCode !== 0 → { ok: false, error, status: 500 }
- exitCode === 0 → { ok: true, output }
This restores the old behavior where tool failures returned HTTP 500
with ok:false, which the SandboxClient uses for warning logs.
Path validation errors (which don't go through the runner) return
ok:true with the error in output — correct since they're not server
errors, just invalid input the LLM should see and retry.
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
1 participant
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.
Summary
Replaces the sandbox worker's duplicated tool logic with a single call to the shared
executeToolfromsrc/tools.ts, using aCommandRunnerinjection to wrap commands with bwrap + seccomp isolation.This is Phase 2 of the unified tool execution plan. Phase 1 (#77, merged in #80) added fallback command support and switched all tools to use
runCommand.Changes
src/tools.ts:CommandRunnertype:(cmd: string[], cwd: string) => Promise<string>executeToolaccepts optionalrunnerparam (default:Bun.spawn-basedrunCommand)execute*functionsALLOWED_GIT_COMMANDSconstant (same set as TypeBox schema literals)src/sandbox/worker.ts:makeSandboxRunner(worktree)— creates aCommandRunnerthat wraps commands withrunToolIsolated(bwrap + seccomp)handleTool: non-git tools →executeTool(name, args, worktree, sandboxRunner)handleGitTool: extracted git-specific handler using sharedALLOWED_GIT_COMMANDSandisolatedGitToolCommand(different bwrap topology — needs bare repo mount)rg,find,ls,readswitch cases,validatePathfunctionsrc/sandbox/Containerfile:fd(apk add fd)tools.tsinto container at/app/tools.ts@sinclair/typebox,@mariozechner/pi-ai) viabun install --productiondocker-compose.yml:./src/sandboxto.(project root) so Containerfile can accesssrc/tools.tsWhat's fixed
The sandbox worker now automatically gets:
rgparameters (max_count,max_results,word) — previously ignoredfdinstead offind— with full parameter supportreadwith[File: path]header and path validation — previously used rawcat -nrg→grepandfd→findfallback from Phase 1tools.ts— no more dual maintenanceTest Coverage
18 new tests (300 total, 12 files):
CommandRunnerinjection: mock runner verifies correct command arrays for all 5 toolsALLOWED_GIT_COMMANDSmatches TypeBox schema literalsWhy git stays special
Git commands in the sandbox need
isolatedGitToolCommandwhich mounts both the worktree and bare repo read-only — a different bwrap topology from other tools. The sandbox runner only mounts the worktree.Closes #78