Reuse the process codec for the trampoline

apps/sandboxio/cmd/oac-sandbox-io/main.go

@@ -34,7 +34,7 @@ func main() {
 	// Reap is the process's only wait. As a child subreaper it also reaps
 	// the orphaned descendants of operations.
 	if err := unix.Prctl(unix.PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0); err != nil {
-		fail(&sandboxio.StartupError{Step: sandboxio.StepSubreaper, Err: err})
+		fail(fmt.Errorf("become a child subreaper: %w", err))
 	}
 	go processservice.Reap(context.Background())
 

apps/sandboxio/internal/processservice/launch.go

@@ -21,8 +21,8 @@ import (
 // launch spawns the process and emits Started or StartFailed, then starts
 // the output readers. The leader's exit follows the output it left buffered;
 // see markLocked.
-func (op *operation) launch(spec sp.ProcessSpec) {
-	l, f := op.spawn(spec)
+func (op *operation) launch(req sp.StartRequest) {
+	l, f := op.spawn(req)
 	op.mu.Lock()
 	if f != nil {
 		op.state, op.startFailure = sp.StateStartFailed, f
@@ -59,9 +59,9 @@ type launched struct {
 	streams []*stream
 }
 
-// spawn starts the trampoline in a new session and waits until it has
-// exec'd the target or reported why it could not.
-func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) {
+// spawn starts the trampoline in a new session, hands it the request and
+// waits until it has exec'd the target or reported why it could not.
+func (op *operation) spawn(req sp.StartRequest) (l launched, f *sp.Failure) {
 	ioFail := func(what string, err error) *sp.Failure {
 		return sp.Fail(sp.CodeIO, sandboxwire.EffectNone, "%s: %v", what, err)
 	}
@@ -94,8 +94,8 @@ func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) {
 	if files[statusFD], statusR, f = pipe("status pipe", false); f != nil {
 		return l, f
 	}
-	if spec.PTY != nil {
-		master, tty, err := openPTY(*spec.PTY)
+	if req.Spec.PTY != nil {
+		master, tty, err := openPTY(*req.Spec.PTY)
 		if err != nil {
 			return l, ioFail("open terminal", err)
 		}
@@ -125,7 +125,7 @@ func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) {
 		Env:   []string{trampolineEnv},
 		Files: files,
 		// Setctty makes the child's descriptor 0, the terminal, its controlling terminal.
-		Sys: &syscall.SysProcAttr{Setsid: true, Setctty: spec.PTY != nil},
+		Sys: &syscall.SysProcAttr{Setsid: true, Setctty: req.Spec.PTY != nil},
 	}
 	reaping.RLock()
 	p, err := os.StartProcess("/proc/self/exe", []string{trampolineArg0}, attr)
@@ -157,7 +157,7 @@ func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) {
 	closeAll(child)
 	child = nil
 
-	_, werr := launchW.Write(encodeLaunch(spec))
+	_, werr := launchW.Write(sp.Encode(req))
 	launchW.Close()
 	status, rerr := io.ReadAll(statusR)
 	statusR.Close()

apps/sandboxio/internal/processservice/service.go

@@ -187,7 +187,7 @@ func (s *Service) Start(_ context.Context, conn *sp.Conn, req sp.StartRequest) (
 	op.observeLocked(conn, 0)
 	op.mu.Unlock()
 	s.mu.Unlock()
-	op.launch(req.Spec)
+	op.launch(req)
 	return sp.StartResponse{Disposition: sp.StartCreated}, nil
 }
 

apps/sandboxio/internal/processservice/service_test.go

@@ -642,7 +642,7 @@ func TestRevokeWhileStarting(t *testing.T) {
 	h.svc.active.Add(1)
 	h.svc.mu.Unlock()
 	h.svc.AttachmentRevoked(h.att)
-	op.launch(spec)
+	op.launch(sp.StartRequest{OperationRef: sp.OperationRef{ServerInstanceID: h.svc.instance, OperationID: key.operation}, Spec: spec})
 	for deadline := time.Now().Add(10 * time.Second); op.inspect().State != sp.StateExited; time.Sleep(10 * time.Millisecond) {
 		if time.Now().After(deadline) {
 			t.Fatal("the launched operation was not cancelled")

apps/sandboxio/internal/processservice/trampoline.go

@@ -3,9 +3,9 @@
 package processservice
 
 import (
-	"bytes"
 	"encoding/binary"
 	"errors"
+	"io"
 	"os"
 	"os/signal"
 	"runtime"
@@ -15,19 +15,20 @@ import (
 
 	"golang.org/x/sys/unix"
 
-	"github.com/MiniMax-AI/OpenAgentCore/internal/sandboxprocess"
-	"github.com/MiniMax-AI/OpenAgentCore/internal/sandboxwire"
+	sp "github.com/MiniMax-AI/OpenAgentCore/internal/sandboxprocess"
 )
 
 // os/exec cannot set a child's umask, so every launch re-executes the service
-// binary as a trampoline. It reads the launch from an inherited descriptor,
-// applies the umask and working directory, and execs the target. The marker
-// is its whole environment and argv; it is not a command-line interface.
+// binary as a trampoline. It reads the operation's Start request from an
+// inherited descriptor, applies the umask and working directory, and execs the
+// target. The marker is its whole environment and argv; it is not a
+// command-line interface.
 const (
 	trampolineArg0 = "oac-process-trampoline"
 	trampolineEnv  = "OAC_PROCESS_TRAMPOLINE=1"
-	// Descriptors the trampoline inherits: the launch, and a close-on-exec
-	// status pipe that reaches EOF without data exactly when exec succeeds.
+	// Descriptors the trampoline inherits: the Start request, and a
+	// close-on-exec status pipe that reaches EOF without data exactly when
+	// exec succeeds.
 	launchFD = 3
 	statusFD = 4
 )
@@ -64,39 +65,30 @@ func errnoOf(err error) syscall.Errno {
 
 func trampoline() (uint16, error) {
 	unix.CloseOnExec(statusFD)
-	var buf bytes.Buffer
-	chunk := make([]byte, 64<<10)
-	for {
-		n, err := unix.Read(launchFD, chunk)
-		if err == unix.EINTR {
-			continue
-		}
-		if err != nil {
-			return stageLaunch, err
-		}
-		if n == 0 {
-			break
-		}
-		buf.Write(chunk[:n])
+	f := os.NewFile(launchFD, "launch")
+	b, err := io.ReadAll(f)
+	f.Close()
+	if err != nil {
+		return stageLaunch, err
 	}
-	unix.Close(launchFD)
-	l, err := decodeLaunch(buf.Bytes())
+	m, err := sp.Decode(sp.OpStart, b)
 	if err != nil {
 		return stageLaunch, syscall.EINVAL
 	}
+	spec := m.(sp.StartRequest).Spec
 	if err := closeInherited(); err != nil {
 		return stageDescriptors, err
 	}
-	unix.Umask(int(l.umask))
-	if err := unix.Chdir(l.cwd); err != nil {
+	unix.Umask(int(spec.Umask))
+	if err := unix.Chdir(string(spec.Cwd)); err != nil {
 		return stageChdir, err
 	}
 	// The Go runtime keeps SIGHUP and SIGINT ignored when it starts with them
 	// ignored. Handling them resets both to the default at exec.
 	signal.Notify(make(chan os.Signal, 1), syscall.SIGHUP, syscall.SIGINT)
 	var none unix.Sigset_t
 	unix.PthreadSigmask(unix.SIG_SETMASK, &none, nil)
-	return stageExec, execTarget(l)
+	return stageExec, execTarget(spec)
 }
 
 // closeInherited makes exec close every descriptor from 3 up, so the target
@@ -116,21 +108,34 @@ func closeInherited() error {
 	return nil
 }
 
-// execTarget execs the executable, searching PATH for a name without '/' the
-// way execvpe does.
-func execTarget(l launch) error {
-	if strings.Contains(l.executable, "/") {
-		return unix.Exec(l.executable, l.argv, l.env)
+// execTarget execs the spec with TERM added for a terminal, searching PATH
+// for a name without '/' the way execvpe does. Decode guarantees PATH then.
+func execTarget(spec sp.ProcessSpec) error {
+	argv := make([]string, len(spec.Argv))
+	for i, a := range spec.Argv {
+		argv[i] = string(a)
+	}
+	var env []string
+	var path string
+	for _, v := range spec.Env {
+		env = append(env, string(v.Name)+"="+string(v.Value))
+		if string(v.Name) == "PATH" {
+			path = string(v.Value)
+		}
+	}
+	if spec.PTY != nil {
+		env = append(env, "TERM="+string(spec.PTY.Term))
 	}
-	if !l.hasPath {
-		return unix.ENOENT
+	exe := string(spec.Executable)
+	if strings.Contains(exe, "/") {
+		return unix.Exec(exe, argv, env)
 	}
 	denied := false
-	for _, dir := range strings.Split(l.path, ":") {
+	for _, dir := range strings.Split(path, ":") {
 		if dir == "" {
 			dir = "."
 		}
-		err := unix.Exec(dir+"/"+l.executable, l.argv, l.env)
+		err := unix.Exec(dir+"/"+exe, argv, env)
 		switch err {
 		case unix.EACCES:
 			denied = true
@@ -144,86 +149,3 @@ func execTarget(l launch) error {
 	}
 	return unix.ENOENT
 }
-
-// launch is what the trampoline needs from a ProcessSpec.
-type launch struct {
-	executable string
-	argv       []string
-	env        []string
-	cwd        string
-	umask      uint32
-	hasPath    bool
-	path       string
-}
-
-func encodeLaunch(spec sandboxprocess.ProcessSpec) []byte {
-	var e sandboxwire.Encoder
-	e.Bytes(spec.Executable)
-	e.Count(len(spec.Argv))
-	for _, a := range spec.Argv {
-		e.Bytes(a)
-	}
-	env := make([][]byte, 0, len(spec.Env)+1)
-	var path []byte
-	for _, v := range spec.Env {
-		env = append(env, bytes.Join([][]byte{v.Name, v.Value}, []byte("=")))
-		if string(v.Name) == "PATH" {
-			path = v.Value
-		}
-	}
-	if spec.PTY != nil {
-		env = append(env, append([]byte("TERM="), spec.PTY.Term...))
-	}
-	e.Count(len(env))
-	for _, v := range env {
-		e.Bytes(v)
-	}
-	e.Bytes(spec.Cwd)
-	e.U32(spec.Umask)
-	e.Present(path != nil)
-	if path != nil {
-		e.Bytes(path)
-	}
-	return e.Payload()
-}
-
-func decodeLaunch(b []byte) (launch, error) {
-	d := sandboxwire.NewDecoder(b)
-	var l launch
-	var err error
-	str := func() string {
-		var v []byte
-		if err == nil {
-			v, err = d.Bytes()
-		}
-		return string(v)
-	}
-	list := func() []string {
-		var n int
-		if err == nil {
-			n, err = d.Count(sandboxwire.MaxPayload)
-		}
-		out := make([]string, n)
-		for i := range out {
-			out[i] = str()
-		}
-		return out
-	}
-	l.executable = str()
-	l.argv = list()
-	l.env = list()
-	l.cwd = str()
-	if err == nil {
-		l.umask, err = d.U32()
-	}
-	if err == nil {
-		l.hasPath, err = d.Present()
-	}
-	if l.hasPath {
-		l.path = str()
-	}
-	if err == nil {
-		err = d.Finish()
-	}
-	return l, err
-}

apps/sandboxio/internal/sandboxio/sandboxio.go

@@ -10,6 +10,7 @@ package sandboxio
 import (
 	"context"
 	"crypto/tls"
+	"fmt"
 	"log"
 	"sync/atomic"
 	"time"
@@ -26,35 +27,16 @@ import (
 	"github.com/MiniMax-AI/OpenAgentCore/internal/sandboxwire"
 )
 
-// Step names the startup step a StartupError failed in.
-type Step string
-
-const (
-	StepSubreaper      Step = "become a child subreaper"
-	StepBootstrap      Step = "read the bootstrap file"
-	StepProcessService Step = "start the process service"
-	StepFileService    Step = "start the file service"
-)
-
-// StartupError is a failure before the service serves. Its message names the
-// step and never includes the credential.
-type StartupError struct {
-	Step Step
-	Err  error
-}
-
-func (e *StartupError) Error() string { return string(e.Step) + ": " + e.Err.Error() }
-func (e *StartupError) Unwrap() error { return e.Err }
-
 // shutdownMargin is how long Shutdown waits past the grace limit for killed
 // processes to be observed gone.
 const shutdownMargin = 5 * time.Second
 
 // Run serves the sandbox the bootstrap file at bootstrapPath names, with the
 // export world rooted at "/", until ctx ends or the relay refuses the link
 // for good. The caller has made the process a child subreaper running
-// processservice.Reap. Run returns nil when ctx ended, a *StartupError when
-// it could not start, and otherwise the relay's *sandboxlink.Error.
+// processservice.Reap. Run returns nil when ctx ended, an error naming the
+// step and never the credential when it could not start, and otherwise the
+// relay's *sandboxlink.Error.
 func Run(ctx context.Context, bootstrapPath string) error {
 	return run(ctx, bootstrapPath, options{root: "/"})
 }
@@ -69,20 +51,20 @@ type options struct {
 func run(ctx context.Context, bootstrapPath string, opt options) error {
 	raw, err := runtimefs.ReadPrivatePath(bootstrapPath, sandboxbootstrap.MaxBytes)
 	if err != nil {
-		return &StartupError{StepBootstrap, err}
+		return fmt.Errorf("read the bootstrap file: %w", err)
 	}
 	in, err := sandboxbootstrap.Decode(raw)
 	if err != nil {
-		return &StartupError{StepBootstrap, err}
+		return fmt.Errorf("read the bootstrap file: %w", err)
 	}
 	procCfg := processservice.DefaultConfig()
 	procs, err := processservice.New(procCfg)
 	if err != nil {
-		return &StartupError{StepProcessService, err}
+		return fmt.Errorf("start the process service: %w", err)
 	}
 	files, err := fileservice.New(opt.root)
 	if err != nil {
-		return &StartupError{StepFileService, err}
+		return fmt.Errorf("start the file service: %w", err)
 	}
 	defer files.Close()
 	fileServer := sandboxfs.NewServer(files)

apps/sandboxio/internal/sandboxio/sandboxio_test.go

@@ -263,3 +263,22 @@ func TestRefusedCredentialEndsTheService(t *testing.T) {
 		t.Fatalf("run: %v, want AuthenticationFailed without the credential", err)
 	}
 }
+
+// A failure before serving names its step, and the message never carries the
+// credential.
+func TestStartupFailureNamesTheStep(t *testing.T) {
+	valid, _ := writeBootstrap(t, "wss://relay.invalid/link", "serve-credential")
+	invalid := filepath.Join(t.TempDir(), "bootstrap.json")
+	if err := os.WriteFile(invalid, []byte(`{"version":1,"credential":"serve-credential"}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	for _, c := range []struct{ bootstrap, root, want string }{
+		{invalid, t.TempDir(), "read the bootstrap file: "},
+		{valid, filepath.Join(t.TempDir(), "missing"), "start the file service: "},
+	} {
+		err := run(context.Background(), c.bootstrap, options{root: c.root})
+		if err == nil || !strings.HasPrefix(err.Error(), c.want) || strings.Contains(err.Error(), "serve-credential") {
+			t.Errorf("run: %v, want %q without the credential", err, c.want)
+		}
+	}
+}