bugfix

internal/acp/sandbox/sandbox.go

@@ -0,0 +1,115 @@
+// Package sandbox provides a pluggable per-session execution sandbox that sits
+// between acp.session_service.Create and acp.Supervisor.Spawn. It confines an
+// agent child process to a low-privilege UID, bind-mounts only that session's
+// worktree + the per-user agent home, applies OS resource limits, and routes
+// egress through an allowlist proxy — all WITHOUT touching proc.Group's
+// Setpgid-based tree-kill (Apply only ADDS to cmd.SysProcAttr, never replaces).
+//
+// Three modes, config-gated via acp.sandbox.mode:
+//   - none      : no-op (default; Windows dev + CI + any non-Linux box)
+//   - uid       : drop to a low-priv UID + setrlimit (+ optional namespace/bind
+//     mounts) — Linux only
+//   - container : run the agent inside a rootless runc/bwrap or docker container
+//     on an egress-restricted network — Linux only
+//
+// The Linux-specific mechanics live in build-tagged files; this file holds the
+// cross-platform contract and the factory so app.go and supervisor.go compile
+// on every platform.
+package sandbox
+
+import "os/exec"
+
+// Mode selects the sandbox strategy.
+type Mode string
+
+const (
+	// ModeNone disables sandboxing (no-op Apply). Default everywhere except
+	// production Linux.
+	ModeNone Mode = "none"
+	// ModeUID drops to a low-priv UID + setrlimit (+ optional bind mounts).
+	ModeUID Mode = "uid"
+	// ModeContainer runs the child inside a rootless container.
+	ModeContainer Mode = "container"
+)
+
+// Limits captures the OS resource limits applied to the child (and its tree).
+// A zero field means "do not set this limit" (inherit the parent's).
+type Limits struct {
+	AddressSpaceBytes uint64 // RLIMIT_AS — caps total virtual memory (anti big-alloc)
+	NProc             uint64 // RLIMIT_NPROC — caps process/thread count (anti fork-bomb)
+	CPUSeconds        uint64 // RLIMIT_CPU — caps CPU seconds
+	PIDs              uint64 // cgroup pids.max (container mode); mirrors NProc for uid
+	DiskBytes         uint64 // RLIMIT_FSIZE — caps max file size the child can write
+}
+
+// Spec is the per-spawn sandbox request, derived from the session + agent kind.
+type Spec struct {
+	Mode        Mode
+	UID         int    // target low-priv uid (uid mode); base+offset computed by caller
+	GID         int    // target low-priv gid
+	HomeDir     string // per-user agent home (writable, bind-mounted in)
+	WorktreeDir string // this session's worktree (writable, bind-mounted in)
+	DataRoot    string // /data root to mask everything else under
+	Rlimits     Limits
+	ProxyURL    string // HTTP(S)_PROXY value injected into the child env (egress allowlist)
+	NoProxy     string // NO_PROXY value (e.g. localhost,127.0.0.1)
+}
+
+// Sandbox applies a Spec to an exec.Cmd before it is started.
+type Sandbox interface {
+	// Apply mutates cmd.SysProcAttr (Credential, namespace/chroot flags) and
+	// cmd.Env (HTTP(S)_PROXY/NO_PROXY) BEFORE proc.Group.Prepare and cmd.Start.
+	// It returns a cleanup closure (unmount binds, remove ephemeral dirs, stop
+	// container) that the supervisor calls from monitorProcess AFTER
+	// group.Close(). cleanup is always non-nil (a no-op when there is nothing to
+	// clean) and must be safe to call exactly once. Apply MUST NOT set Setpgid —
+	// that is proc.Group's job; on Linux it ADDS to the existing SysProcAttr.
+	Apply(cmd *exec.Cmd, spec Spec) (cleanup func(), err error)
+}
+
+// New returns the Sandbox implementation for the given mode. On non-Linux
+// platforms (or mode=none) it always returns the no-op sandbox. The actual
+// uid/container constructors are provided by build-tagged files; newPlatform
+// returns nil when the mode is unsupported on this OS, in which case we fall
+// back to no-op so dev/CI never break.
+func New(mode Mode) Sandbox {
+	switch mode {
+	case ModeUID, ModeContainer:
+		if sb := newPlatform(mode); sb != nil {
+			return sb
+		}
+		// Unsupported on this platform → safe no-op fallback.
+		return noopSandbox{}
+	default:
+		return noopSandbox{}
+	}
+}
+
+// noopSandbox makes no changes and returns a no-op cleanup. It is the default
+// and the fallback for unsupported modes/platforms.
+type noopSandbox struct{}
+
+func (noopSandbox) Apply(_ *exec.Cmd, _ Spec) (func(), error) {
+	return func() {}, nil
+}
+
+// injectProxyEnv appends egress-proxy env vars to cmd.Env when a proxy URL is
+// configured. Shared by uid + container modes (both honor HTTP(S)_PROXY). Kept
+// here so the cross-platform contract is testable without build tags.
+func injectProxyEnv(cmd *exec.Cmd, spec Spec) {
+	if spec.ProxyURL == "" {
+		return
+	}
+	noProxy := spec.NoProxy
+	if noProxy == "" {
+		noProxy = "localhost,127.0.0.1,::1"
+	}
+	cmd.Env = append(cmd.Env,
+		"HTTP_PROXY="+spec.ProxyURL,
+		"HTTPS_PROXY="+spec.ProxyURL,
+		"http_proxy="+spec.ProxyURL,
+		"https_proxy="+spec.ProxyURL,
+		"NO_PROXY="+noProxy,
+		"no_proxy="+noProxy,
+	)
+}

internal/acp/sandbox/sandbox_container_linux.go

@@ -0,0 +1,118 @@
+//go:build linux
+
+package sandbox
+
+import (
+	"fmt"
+	"os/exec"
+	"strconv"
+	"time"
+
+	"github.com/google/uuid"
+)
+
+// containerSandbox runs the agent binary inside a rootless container with an
+// egress-restricted network and the two writable paths bind-mounted in. This is
+// the hard isolation boundary (UID + filesystem mask + network policy + cgroup
+// limits) — unlike uid mode, a non-cooperating binary cannot bypass the egress
+// allowlist because the container network only routes through the proxy/network
+// policy.
+//
+// It rewrites cmd to `docker run --rm --name <n> --user uid:gid --read-only
+// --network <egress-net> --pids-limit --memory --cpus --mount <home> --mount
+// <worktree> -w <worktree> <image> <binary> <args>` where the image is the same
+// runtime image (so the agent CLI is present). The cleanup closure runs
+// `docker stop <n>` so proc.Group's tree-kill (which signals the `docker run`
+// client's pgid) is reinforced by stopping the actual container — otherwise
+// killing the client could orphan the container.
+//
+// Tree-kill invariant: Apply does NOT set Setpgid (proc.Group owns it). docker
+// run becomes the group leader; killing -pgid signals it, and cleanup() issues
+// docker stop for the container it spawned. The mapping client->container is via
+// the deterministic --name we assign.
+//
+// The container runtime + egress network are provisioned by the deployment
+// (Dockerfile installs the runtime; docker-compose defines the egress-net + the
+// forward-proxy sidecar). Image/network/runtime are read from the spec's
+// DataRoot convention or sensible defaults; here we keep them parameterized via
+// package-level defaults so app.go can override without a code change later.
+type containerSandbox struct{}
+
+// container runtime defaults; overridable by deployment via env in a later pass.
+const (
+	defaultRuntime    = "docker"
+	defaultImage      = "agent-coding-workflow-sandbox:latest"
+	defaultEgressNet  = "egress-net"
+	containerStopWait = 5 * time.Second
+)
+
+func (s *containerSandbox) Apply(cmd *exec.Cmd, spec Spec) (func(), error) {
+	if spec.WorktreeDir == "" {
+		return func() {}, fmt.Errorf("sandbox container: worktree dir required")
+	}
+	runtime, err := exec.LookPath(defaultRuntime)
+	if err != nil {
+		// Runtime missing: fall back to no-op cleanup but surface the error so
+		// the supervisor can decide (it currently logs and proceeds unsandboxed
+		// only if configured to; otherwise spawn fails).
+		return func() {}, fmt.Errorf("sandbox container: %s not found: %w", defaultRuntime, err)
+	}
+
+	name := "acw-sbx-" + uuid.NewString()
+	orig := append([]string{cmd.Path}, cmd.Args[1:]...)
+
+	runArgs := []string{
+		"run", "--rm", "--name", name,
+		"--read-only",
+		"--network", defaultEgressNet,
+		"-w", spec.WorktreeDir,
+		"--mount", "type=bind,source=" + spec.WorktreeDir + ",target=" + spec.WorktreeDir,
+	}
+	if spec.HomeDir != "" {
+		runArgs = append(runArgs,
+			"--mount", "type=bind,source="+spec.HomeDir+",target="+spec.HomeDir,
+			"-e", "HOME="+spec.HomeDir)
+	}
+	if spec.UID > 0 {
+		gid := spec.GID
+		if gid <= 0 {
+			gid = spec.UID
+		}
+		runArgs = append(runArgs, "--user", strconv.Itoa(spec.UID)+":"+strconv.Itoa(gid))
+	}
+	if spec.Rlimits.AddressSpaceBytes > 0 {
+		runArgs = append(runArgs, "--memory", strconv.FormatUint(spec.Rlimits.AddressSpaceBytes, 10))
+	}
+	if spec.Rlimits.PIDs > 0 {
+		runArgs = append(runArgs, "--pids-limit", strconv.FormatUint(spec.Rlimits.PIDs, 10))
+	} else if spec.Rlimits.NProc > 0 {
+		runArgs = append(runArgs, "--pids-limit", strconv.FormatUint(spec.Rlimits.NProc, 10))
+	}
+	if spec.Rlimits.CPUSeconds > 0 {
+		// docker has no direct CPU-seconds cap; approximate with --cpus=1 and
+		// rely on the wall-clock/budget reaper for hard CPU-time bounds.
+		runArgs = append(runArgs, "--cpus", "1")
+	}
+	// Egress proxy env (the proxy itself enforces the allowlist on egress-net).
+	if spec.ProxyURL != "" {
+		noProxy := spec.NoProxy
+		if noProxy == "" {
+			noProxy = "localhost,127.0.0.1,::1"
+		}
+		runArgs = append(runArgs,
+			"-e", "HTTP_PROXY="+spec.ProxyURL,
+			"-e", "HTTPS_PROXY="+spec.ProxyURL,
+			"-e", "NO_PROXY="+noProxy)
+	}
+	runArgs = append(runArgs, defaultImage)
+	runArgs = append(runArgs, orig...)
+
+	cmd.Path = runtime
+	cmd.Args = append([]string{runtime}, runArgs...)
+
+	cleanup := func() {
+		stop := exec.Command(runtime, "stop", "-t", strconv.Itoa(int(containerStopWait.Seconds())), name)
+		_ = stop.Run() // best-effort; --rm removes it after stop
+	}
+	return cleanup, nil
+}

internal/acp/sandbox/sandbox_linux.go

@@ -0,0 +1,15 @@
+//go:build linux
+
+package sandbox
+
+// newPlatform dispatches the Linux sandbox modes to their concrete impls.
+func newPlatform(mode Mode) Sandbox {
+	switch mode {
+	case ModeUID:
+		return &uidSandbox{}
+	case ModeContainer:
+		return &containerSandbox{}
+	default:
+		return nil
+	}
+}

internal/acp/sandbox/sandbox_other.go

@@ -0,0 +1,9 @@
+//go:build !linux
+
+package sandbox
+
+// newPlatform returns nil on non-Linux platforms: UID drop, setrlimit and
+// bind-mount namespaces are Linux-only, so Windows dev and macOS always fall
+// back to the no-op sandbox (see New). This is the documented contract that
+// mode=none is the only supported mode off Linux.
+func newPlatform(_ Mode) Sandbox { return nil }

internal/acp/sandbox/sandbox_test.go

@@ -0,0 +1,59 @@
+package sandbox
+
+import (
+	"os/exec"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestNew_NoneAlwaysNoop(t *testing.T) {
+	sb := New(ModeNone)
+	require.IsType(t, noopSandbox{}, sb)
+}
+
+func TestNew_UnsupportedPlatformFallsBackToNoop(t *testing.T) {
+	if runtime.GOOS == "linux" {
+		t.Skip("uid/container are supported on linux; this asserts the non-linux fallback")
+	}
+	// On non-linux, uid/container must fall back to no-op so dev/CI never break.
+	require.IsType(t, noopSandbox{}, New(ModeUID))
+	require.IsType(t, noopSandbox{}, New(ModeContainer))
+}
+
+func TestNoopSandbox_MutatesNothing(t *testing.T) {
+	cmd := exec.Command("echo", "hi")
+	before := cmd.SysProcAttr
+	cleanup, err := noopSandbox{}.Apply(cmd, Spec{Mode: ModeNone, ProxyURL: "http://proxy:8080"})
+	require.NoError(t, err)
+	require.NotNil(t, cleanup)
+	// no-op must NOT inject proxy env nor touch SysProcAttr.
+	require.Equal(t, before, cmd.SysProcAttr)
+	require.Empty(t, cmd.Env)
+	// cleanup is safe to call.
+	cleanup()
+}
+
+func TestInjectProxyEnv(t *testing.T) {
+	cmd := exec.Command("echo", "hi")
+	cmd.Env = []string{"PATH=/usr/bin"}
+	injectProxyEnv(cmd, Spec{ProxyURL: "http://proxy:3128"})
+	require.Contains(t, cmd.Env, "HTTP_PROXY=http://proxy:3128")
+	require.Contains(t, cmd.Env, "HTTPS_PROXY=http://proxy:3128")
+	require.Contains(t, cmd.Env, "NO_PROXY=localhost,127.0.0.1,::1")
+	// original env preserved.
+	require.Contains(t, cmd.Env, "PATH=/usr/bin")
+}
+
+func TestInjectProxyEnv_NoProxyURLIsNoop(t *testing.T) {
+	cmd := exec.Command("echo", "hi")
+	injectProxyEnv(cmd, Spec{})
+	require.Empty(t, cmd.Env)
+}
+
+func TestInjectProxyEnv_CustomNoProxy(t *testing.T) {
+	cmd := exec.Command("echo", "hi")
+	injectProxyEnv(cmd, Spec{ProxyURL: "http://p", NoProxy: "internal.local"})
+	require.Contains(t, cmd.Env, "NO_PROXY=internal.local")
+}

internal/acp/sandbox/sandbox_uid_linux.go

@@ -0,0 +1,92 @@
+//go:build linux
+
+package sandbox
+
+import (
+	"fmt"
+	"os/exec"
+	"strconv"
+	"syscall"
+)
+
+// uidSandbox confines the agent child to a low-privilege UID/GID and applies OS
+// resource limits, while leaving proc.Group's Setpgid (tree-kill) intact.
+//
+// Tree-kill invariant: we ONLY add Credential to the existing SysProcAttr. We do
+// NOT touch Setpgid (proc.Group sets it after Apply via group.Prepare). The
+// negative-pgid SIGTERM/SIGKILL path keeps working because the new process group
+// leader is the same child — it simply runs as the low-priv uid now.
+//
+// Resource limits are applied by wrapping the real binary in `prlimit(1)`:
+// `prlimit --as=N --nproc=N --cpu=N --fsize=N -- <binary> <args...>`. prlimit
+// sets the limits on the child it execs, so they apply to the agent and (since
+// limits are inherited across fork) its whole subtree — exactly the tree the
+// pgid covers. Wrapping is additive to Credential/Setpgid and does not change
+// the process-group topology (prlimit execs the target in place, becoming the
+// group leader). If prlimit is unavailable, UID drop still applies and we log no
+// rlimits (best-effort; container mode is the hard boundary).
+//
+// Bind-mount masking (CLONE_NEWNS + remount /data with only HomeDir+WorktreeDir
+// visible) is intentionally NOT auto-enabled here: it requires the server to
+// hold CAP_SYS_ADMIN / a user namespace, which the rootless production container
+// (USER app) does not have by default. The robust isolation path is
+// ModeContainer. uidSandbox therefore provides UID drop + rlimits + egress proxy
+// as the minimum viable confinement, and documents that filesystem masking is a
+// container-mode guarantee.
+type uidSandbox struct{}
+
+func (s *uidSandbox) Apply(cmd *exec.Cmd, spec Spec) (func(), error) {
+	if spec.UID <= 0 {
+		return func() {}, fmt.Errorf("sandbox uid: invalid target uid %d", spec.UID)
+	}
+
+	// UID/GID drop — ADDITIVE to whatever proc.Group will set (Setpgid).
+	if cmd.SysProcAttr == nil {
+		cmd.SysProcAttr = &syscall.SysProcAttr{}
+	}
+	gid := spec.GID
+	if gid <= 0 {
+		gid = spec.UID
+	}
+	cmd.SysProcAttr.Credential = &syscall.Credential{
+		Uid: uint32(spec.UID),
+		Gid: uint32(gid),
+	}
+
+	// Resource limits via prlimit wrapper (if available).
+	if prlimit, err := exec.LookPath("prlimit"); err == nil {
+		args := prlimitArgs(spec.Rlimits)
+		if len(args) > 0 {
+			// Re-point the command through prlimit, preserving the original
+			// binary + args after the `--` separator.
+			orig := append([]string{cmd.Path}, cmd.Args[1:]...)
+			cmd.Path = prlimit
+			cmd.Args = append(append([]string{prlimit}, args...), append([]string{"--"}, orig...)...)
+		}
+	}
+
+	// Egress allowlist: inject HTTP(S)_PROXY/NO_PROXY into the stripped child env.
+	injectProxyEnv(cmd, spec)
+
+	// No ephemeral resources to tear down in uid mode (no bind mounts here);
+	// return a no-op cleanup.
+	return func() {}, nil
+}
+
+// prlimitArgs builds the prlimit flags for the non-zero limits in l.
+func prlimitArgs(l Limits) []string {
+	var args []string
+	if l.AddressSpaceBytes > 0 {
+		args = append(args, "--as="+strconv.FormatUint(l.AddressSpaceBytes, 10))
+	}
+	if l.NProc > 0 {
+		args = append(args, "--nproc="+strconv.FormatUint(l.NProc, 10))
+	}
+	if l.CPUSeconds > 0 {
+		args = append(args, "--cpu="+strconv.FormatUint(l.CPUSeconds, 10))
+	}
+	if l.DiskBytes > 0 {
+		args = append(args, "--fsize="+strconv.FormatUint(l.DiskBytes, 10))
+	}
+	return args
+}