Optional Docker sandbox: whole-skvm-in-container via --sandbox#39
Open
lec77 wants to merge 40 commits into
Open
Optional Docker sandbox: whole-skvm-in-container via --sandbox#39lec77 wants to merge 40 commits into
lec77 wants to merge 40 commits into
Conversation
…SandboxConfig tests
…sandbox key injection
… dup-guard test) Add verified path-shaped CLI flags that were absent from PATH_FLAGS: --profile (file, ro) used by aot-compile/pipeline/bench; --skill-list (file, ro) for jit-optimize batch mode; --workdir (dir, rw) for run; --target (dir, rw) for proposals accept; --custom (file, ro) for bench YAML plans; --manifest (dir, ro) for bench judge; --output-dir (dir, rw) for bench compare; --path (dir, ro) for bench import; --report (file, rw) for bench merge-judge. --logs and --failures (comma-separated path lists) are excluded from PATH_FLAGS with a TODO(docker-sandbox) comment near their parsing site. Add a "flag list has no duplicates" test to catch future drift.
… guard runs in production
…le + dispatch guard) Adds assertSandboxCompatible() exported from src/index.ts, which throws a hard error when --sandbox is combined with either a config subcommand (host-state management) or native adapter mode (imports host credentials). Wires the config-command guard in the sandbox dispatch block inside main(), right before runLauncher() is invoked. Per-command native-adapter wiring (after resolveAdapterConfigMode) is a follow-up task.
…xtraMounts plumbing
…e-code + skvm binary) Pinned versions: - opencode v1.4.3 (anomalyco/opencode GitHub release binary, linux-x64) SHA-256: 34d503ebb029853293be6fd4d441bbb2dbb03919bfa4525e88b1ca55d68f3e17 (opencode is not on npm; installed from upstream release tarball) - @anthropic-ai/claude-code@2.1.152 (npm) pi/hermes/openclaw CLIs are deferred to follow-up commits. dist/skvm must be a Linux x86_64 binary built on the host before docker build; cross-compile with `bun run build:all` (CI) or a linux/amd64 bun target. Docker daemon not available locally; build smoke test deferred to reck (Task 21).
… with defaults.sandbox)
…ses assertKnownFlags
…iner skvm reads the mounted config
…e subcommand still guards config
… of stripping (keeps route schema-valid)
…pterConfigMode choke point
…injecting the wrong key
…daemon never blocks launch
8 tasks
--skill / --logs / --failures / --tasks / --test-tasks take comma- separated path lists, but PATH_FLAGS modelled every flag as a single path. Out-of-root list values were resolved as one nonexistent path (or left unrewritten for --logs/--failures, which were absent entirely), so log-source JIT and multi-skill runs broke silently inside the sandbox. Add shape:"csv" and pathLikeOnly metadata to PathFlag; composeMounts now splits csv values, resolves/rewrites/mounts each element independently, and reassembles the arg. pathLikeOnly leaves bare bench task IDs in --tasks/--test-tasks untouched while rewriting path elements.
sandbox.docker.extraMounts bypassed the denylist that --mount-extra enforces, so a config could mount /var/run/docker.sock or / and defeat the sandbox. Route both escape hatches through a shared assertExtraMountsAllowed before composing mounts. Also mkdir -p the cache root before docker bind-mounts it: a missing bind source is created by the daemon as root, leaving the container (run as the host uid) unable to write /skvm-cache and the host with a root-owned ~/.skvm.
--no-auto-probe is stripped from argv on the host and re-expressed as SKVM_AUTO_PROBE=0, but composeEnv's allowlist never forwarded it, so the container re-enabled auto-probe despite the opt-out. Forward the var when the host has it set.
Two mount-composition fixes from review: - Longest-prefix root matching. rewriteUnderFixedRoots matched cwd before skvmCache, so --skvm-cache=./.skvm (cache nested under the workspace) rewrote to /workspace/.skvm. Since the in-container --skvm-cache flag outranks SKVM_CACHE=/skvm-cache, the container then read the raw config via the workspace mount and bypassed the sanitized /skvm-cache overlay, exposing literal API keys. Match the most specific root instead. - Pre-create managed rw mount sources. composeMounts only existence- checked required flags, so an optional output like --out=/tmp/new still produced a dynamic bind mount; Docker creates a missing source as root and the host-uid container cannot write it. composeMounts now reports ensureDirs (cache root + dynamic rw outputs, excluding user extra mounts) and the launcher mkdir -p's them. Subsumes the earlier explicit cache-root pre-create.
Collaborator
Author
|
Pushed follow-up commits addressing the review feedback:
Each fix ships with regression tests; On the intermittent |
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
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.
Linked issue
Closes #30
Summary
Adds an opt-in
--sandboxflag that runs the entire skvm process inside an ephemeral Docker container, instead of containing execution per-adapter. Default behaviour is unchanged — without--sandbox, skvm runs on the host exactly as before. Raising the containment boundary above the adapter layer means adapter code is untouched and the per-surface complexity explored earlier (per-CLI HOME mounts, env-var flips, per-surface network defaults) disappears.Changes
Launcher (new
src/launcher/)index.ts—runLauncher(): composes mounts/env/image, a hardeneddocker runargv, then execs docker. Parses--mount-extra,--debug-sandbox(key-redacted),--docker-image,--docker-network.path-flags.ts— enumerates every path-shaped CLI flag with{kind, mode}, including comma-separated list flags.mounts.ts— three default mounts (/workspace,/skvm-cache,/skvm-data) plus dynamic/extra/<idx>/mounts for out-of-root paths, with path-prefix dedup and longest-prefix root matching.env.ts— proxy passthrough,SKVM_CACHE/HOMEmarkers, per-route key injection (SKVM_ROUTE_<id>_KEY), route-match collision detection.config-sanitize.ts— writes a key-stripped config the container mounts, socat skvm.config.jsoninside the container exposes no secrets.image.ts— hybrid resolve: pullghcr.io/sjtu-ipads/skvm-sandbox:<version>, fall back to a printed localdocker buildcommand.docker-argv.ts—--cap-drop=ALL,no-new-privileges, pids/mem/cpu limits, host uid:gid, labels for stale-reap; rejects env values with newlines.stale-reap.ts— reaps leaked containers and tmp dirs by host-pid label (timeout-bounded, best-effort).Core wiring
src/index.ts—--sandboxflag,SKVM_IN_SANDBOXre-entry guard, launcher dispatch, config-command/native-mode guards, clean error output.src/core/config.ts—getSandboxConfig/getDefaultSandboxMode,resolveRouteApiKeyenv fallback, native-mode-under-sandbox enforcement at theresolveAdapterConfigModechoke point.src/core/types.ts—SandboxConfigSchema(validated memory/cpus/network).src/providers/registry.ts—resolveRouteApiKeymoved tocore/config.ts(re-exported; behaviour preserved).src/cli-config/index.ts—config init/show/doctorsurface the sandbox slice plus docker/image checks.Image + docs
docker/skvm-sandbox.Dockerfile— Ubuntu 24.04 + Bun + opencode + claude-code + baked skvm binary.README.md—--sandboxsemantics, image build, cleanup recipes.Adapter code (
src/adapters/*.ts) is unchanged — the defining property.Test plan
bunx tsc --noEmitbun test— 1013 pass, 0 failVerified end-to-end: image build, in-container tooling, launcher roundtrip, a real LLM run with artifacts persisted back to the host cache, and all guard / redaction paths.
Notes for reviewers
SKVM_ROUTE_<id>_KEY); the design accepts that env-readable keys remain visible to in-container code (network=bridge is required for LLM calls).--debug-sandboxredacts key values.src/index.tsglobal error handler now printserror: <msg>by default (full stack under--verbose/SKVM_DEBUG). This affects all commands, not just sandbox — happy to split it out if you'd prefer the PR stay strictly sandbox-scoped.@anthropic-ai/claude-codenpm install + skvm binary). Reasonable for an agent sandbox but not lean. Multi-stage build, debian-slim base, and dropping thepython3/npm/jqapt set are the obvious slimming levers — left as follow-ups so this PR stays scoped to the launcher mechanism.--mount-extracolon-in-path limitation; stripping non-route host paths (adapters/headlessAgent) from the mounted config; baseUrl-embedded-credentials stripping; pinning Bun in the Dockerfile.Checklist
<scope>: <summary>styleREADME.md/docs/skvm/where behaviour changed