From 53ee632f86acbd844c058366d1e3175febcc2d5e Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 14:11:37 -0700 Subject: [PATCH 1/9] PR5 S1: AgentAdapter command construction (replaces run.sh) Add AgentAdapter interface + claude/codex/opencode/custom adapters in internal/agent. Command(cfg, taskPath) reproduces BuildRunSh's argv (PATH prepend, entrypoint validation, task dispatch) as a typed command, the single owner of entrypoint construction. Environment() is empty today: sandbox env keeps its single owner (config -> BuildEnvMap). BuildRunSh and run.sh removal happen in S5 when the last caller goes away. Invariant 30. Firewall: internal/agent stays cobra/SDK-free. --- internal/agent/adapter.go | 143 +++++++++++++ internal/agent/adapter_test.go | 362 +++++++++++++++++++++++++++++++++ 2 files changed, 505 insertions(+) create mode 100644 internal/agent/adapter.go create mode 100644 internal/agent/adapter_test.go diff --git a/internal/agent/adapter.go b/internal/agent/adapter.go new file mode 100644 index 0000000..fb19a0c --- /dev/null +++ b/internal/agent/adapter.go @@ -0,0 +1,143 @@ +package agent + +import ( + "strings" +) + +const ( + // SandboxTaskPath is the in-sandbox path to task.md written by the payload uploader. + SandboxTaskPath = "/sandbox/.config/openshell/task.md" + // SandboxPayloadBinDir is the in-sandbox path to the payload bin directory. + SandboxPayloadBinDir = "/sandbox/.config/openshell/bin" +) + +// AgentAdapter owns per-agent-type command construction for sandbox execution. +// Given an agent config and the in-sandbox task path, it returns the argv to +// exec inside the sandbox, replacing the generated run.sh shell script. +type AgentAdapter interface { + // Environment returns agent-type-specific env not already supplied by config. + // Returns empty map for all adapters today (env is config-owned via BuildEnvMap). + Environment(cfg *AgentConfig) map[string]string + + // Command returns the argv to exec inside the sandbox. taskPath is the + // in-sandbox path to task.md ("" when no task). Reproduces BuildRunSh behavior: + // - Prepends PATH with payload bin directory + // - For tasks with headless mode: uses --print (claude/codex) or run (opencode) + // - For tasks with interactive mode: uses -p + // - For no task: just the entrypoint + Command(cfg *AgentConfig, taskPath string) []string +} + +// AdapterFor returns the appropriate AgentAdapter for the given entrypoint. +// Dispatch: "claude" or "" -> claude adapter, "codex" -> codex adapter, +// "opencode" -> opencode adapter, else -> custom adapter. +func AdapterFor(entrypoint string) AgentAdapter { + switch entrypoint { + case "claude", "": + return &claudeAdapter{} + case "codex": + return &codexAdapter{} + case "opencode": + return &opencodeAdapter{} + default: + return &customAdapter{} + } +} + +// claudeAdapter implements AgentAdapter for the claude agent. +type claudeAdapter struct{} + +func (a *claudeAdapter) Environment(cfg *AgentConfig) map[string]string { + return make(map[string]string) +} + +func (a *claudeAdapter) Command(cfg *AgentConfig, taskPath string) []string { + return buildCommand("claude", cfg, taskPath) +} + +// codexAdapter implements AgentAdapter for the codex agent. +type codexAdapter struct{} + +func (a *codexAdapter) Environment(cfg *AgentConfig) map[string]string { + return make(map[string]string) +} + +func (a *codexAdapter) Command(cfg *AgentConfig, taskPath string) []string { + return buildCommand("codex", cfg, taskPath) +} + +// opencodeAdapter implements AgentAdapter for the opencode agent. +type opencodeAdapter struct{} + +func (a *opencodeAdapter) Environment(cfg *AgentConfig) map[string]string { + return make(map[string]string) +} + +func (a *opencodeAdapter) Command(cfg *AgentConfig, taskPath string) []string { + return buildCommand("opencode", cfg, taskPath) +} + +// customAdapter implements AgentAdapter for custom entrypoints. +type customAdapter struct{} + +func (a *customAdapter) Environment(cfg *AgentConfig) map[string]string { + return make(map[string]string) +} + +func (a *customAdapter) Command(cfg *AgentConfig, taskPath string) []string { + entrypoint := cfg.EffectiveEntrypoint() + // For custom entrypoints, treat them as the base agent type but use their + // custom entrypoint instead of a predefined one. + return buildCommand(entrypoint, cfg, taskPath) +} + +// buildCommand constructs the argv for the given base entrypoint (could be +// "claude", "codex", "opencode", or a custom entrypoint). It handles: +// - PATH prepending to /sandbox/.config/openshell/bin +// - Task dispatch (--print for headless, -p for interactive, none for no task) +// - Entrypoint validation via command -v check +// - Wrapping in bash -lc for shell setup +func buildCommand(baseEntrypoint string, cfg *AgentConfig, taskPath string) []string { + epBin := strings.Fields(baseEntrypoint)[0] + + var cmdBuilder strings.Builder + + // Prepend PATH and validate entrypoint + cmdBuilder.WriteString("export PATH=\"") + cmdBuilder.WriteString(SandboxPayloadBinDir) + cmdBuilder.WriteString(":$PATH\"; ") + + cmdBuilder.WriteString("if ! command -v ") + cmdBuilder.WriteString(epBin) + cmdBuilder.WriteString(" >/dev/null 2>&1; then echo \"ERROR: entrypoint ") + cmdBuilder.WriteString(epBin) + cmdBuilder.WriteString(" not found in PATH\" >&2; exit 1; fi; ") + + cmdBuilder.WriteString("exec ") + cmdBuilder.WriteString(baseEntrypoint) + + // Handle task dispatch + if taskPath != "" { + if cfg.NoTTY() { + // Headless mode + switch epBin { + case "opencode": + cmdBuilder.WriteString(" run \"$(cat ") + cmdBuilder.WriteString(taskPath) + cmdBuilder.WriteString(")\"") + default: + // claude, codex, and custom use --print + cmdBuilder.WriteString(" --print \"$(cat ") + cmdBuilder.WriteString(taskPath) + cmdBuilder.WriteString(")\"") + } + } else { + // Interactive mode + cmdBuilder.WriteString(" -p \"$(cat ") + cmdBuilder.WriteString(taskPath) + cmdBuilder.WriteString(")\"") + } + } + + return []string{"bash", "-lc", cmdBuilder.String()} +} diff --git a/internal/agent/adapter_test.go b/internal/agent/adapter_test.go new file mode 100644 index 0000000..23865ce --- /dev/null +++ b/internal/agent/adapter_test.go @@ -0,0 +1,362 @@ +package agent + +import ( + "testing" +) + +// TestAdapterForDispatch verifies the AdapterFor function routes correctly. +func TestAdapterForDispatch(t *testing.T) { + tests := []struct { + name string + entrypoint string + wantType string + }{ + {"claude explicit", "claude", "claudeAdapter"}, + {"claude implicit (empty)", "", "claudeAdapter"}, + {"codex", "codex", "codexAdapter"}, + {"opencode", "opencode", "opencodeAdapter"}, + {"custom", "myagent", "customAdapter"}, + {"custom with args", "myagent --flag", "customAdapter"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + adapter := AdapterFor(tt.entrypoint) + // Verify by checking the type name. + gotType := typeOf(adapter) + if gotType != tt.wantType { + t.Errorf("AdapterFor(%q) returned %s, want %s", tt.entrypoint, gotType, tt.wantType) + } + }) + } +} + +// TestEnvironmentAlwaysEmpty verifies all adapters return empty maps. +func TestEnvironmentAlwaysEmpty(t *testing.T) { + adapters := map[string]AgentAdapter{ + "claude": AdapterFor("claude"), + "codex": AdapterFor("codex"), + "opencode": AdapterFor("opencode"), + "custom": AdapterFor("custom"), + } + + cfg := &AgentConfig{Name: "test"} + + for name, adapter := range adapters { + t.Run(name, func(t *testing.T) { + env := adapter.Environment(cfg) + if env == nil { + t.Error("Environment() returned nil, want empty map") + } + if len(env) != 0 { + t.Errorf("Environment() returned non-empty map: %v, want empty", env) + } + }) + } +} + +// TestClaudeHeadlessWithTask verifies claude adapter emits --print form for headless+task. +func TestClaudeHeadlessWithTask(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "claude", + Task: "task.md", + TTY: boolPtr(false), // headless + } + + adapter := AdapterFor("claude") + cmd := adapter.Command(cfg, SandboxTaskPath) + + expected := []string{ + "bash", + "-lc", + `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v claude >/dev/null 2>&1; then echo "ERROR: entrypoint claude not found in PATH" >&2; exit 1; fi; exec claude --print "$(cat /sandbox/.config/openshell/task.md)"`, + } + + if !cmdEqual(cmd, expected) { + t.Errorf("Command() mismatch\ngot: %v\nwant: %v", cmd, expected) + } +} + +// TestClaudeInteractiveWithTask verifies claude adapter emits -p form for interactive+task. +func TestClaudeInteractiveWithTask(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "claude", + Task: "task.md", + TTY: boolPtr(true), // interactive + } + + adapter := AdapterFor("claude") + cmd := adapter.Command(cfg, SandboxTaskPath) + + expected := []string{ + "bash", + "-lc", + `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v claude >/dev/null 2>&1; then echo "ERROR: entrypoint claude not found in PATH" >&2; exit 1; fi; exec claude -p "$(cat /sandbox/.config/openshell/task.md)"`, + } + + if !cmdEqual(cmd, expected) { + t.Errorf("Command() mismatch\ngot: %v\nwant: %v", cmd, expected) + } +} + +// TestClaudeNoTask verifies claude adapter emits bare entrypoint when no task. +func TestClaudeNoTask(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "claude", + TTY: boolPtr(false), + } + + adapter := AdapterFor("claude") + cmd := adapter.Command(cfg, "") + + expected := []string{ + "bash", + "-lc", + `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v claude >/dev/null 2>&1; then echo "ERROR: entrypoint claude not found in PATH" >&2; exit 1; fi; exec claude`, + } + + if !cmdEqual(cmd, expected) { + t.Errorf("Command() mismatch\ngot: %v\nwant: %v", cmd, expected) + } +} + +// TestClaudeImplicitEntrypoint verifies implicit entrypoint (empty) defaults to claude. +func TestClaudeImplicitEntrypoint(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "", // implicit + Task: "task.md", + TTY: boolPtr(false), + } + + adapter := AdapterFor("") // "" dispatches to claude + cmd := adapter.Command(cfg, SandboxTaskPath) + + expected := []string{ + "bash", + "-lc", + `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v claude >/dev/null 2>&1; then echo "ERROR: entrypoint claude not found in PATH" >&2; exit 1; fi; exec claude --print "$(cat /sandbox/.config/openshell/task.md)"`, + } + + if !cmdEqual(cmd, expected) { + t.Errorf("Command() mismatch\ngot: %v\nwant: %v", cmd, expected) + } +} + +// TestOpenCodeHeadlessWithTask verifies opencode adapter emits run form for headless+task. +func TestOpenCodeHeadlessWithTask(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "opencode", + Task: "task.md", + TTY: boolPtr(false), // headless + } + + adapter := AdapterFor("opencode") + cmd := adapter.Command(cfg, SandboxTaskPath) + + expected := []string{ + "bash", + "-lc", + `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v opencode >/dev/null 2>&1; then echo "ERROR: entrypoint opencode not found in PATH" >&2; exit 1; fi; exec opencode run "$(cat /sandbox/.config/openshell/task.md)"`, + } + + if !cmdEqual(cmd, expected) { + t.Errorf("Command() mismatch\ngot: %v\nwant: %v", cmd, expected) + } +} + +// TestOpenCodeInteractiveWithTask verifies opencode uses -p for interactive mode. +func TestOpenCodeInteractiveWithTask(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "opencode", + Task: "task.md", + TTY: boolPtr(true), // interactive + } + + adapter := AdapterFor("opencode") + cmd := adapter.Command(cfg, SandboxTaskPath) + + expected := []string{ + "bash", + "-lc", + `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v opencode >/dev/null 2>&1; then echo "ERROR: entrypoint opencode not found in PATH" >&2; exit 1; fi; exec opencode -p "$(cat /sandbox/.config/openshell/task.md)"`, + } + + if !cmdEqual(cmd, expected) { + t.Errorf("Command() mismatch\ngot: %v\nwant: %v", cmd, expected) + } +} + +// TestCodexHeadlessWithTask verifies codex adapter emits --print form for headless+task. +func TestCodexHeadlessWithTask(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "codex", + Task: "task.md", + TTY: boolPtr(false), // headless + } + + adapter := AdapterFor("codex") + cmd := adapter.Command(cfg, SandboxTaskPath) + + expected := []string{ + "bash", + "-lc", + `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v codex >/dev/null 2>&1; then echo "ERROR: entrypoint codex not found in PATH" >&2; exit 1; fi; exec codex --print "$(cat /sandbox/.config/openshell/task.md)"`, + } + + if !cmdEqual(cmd, expected) { + t.Errorf("Command() mismatch\ngot: %v\nwant: %v", cmd, expected) + } +} + +// TestCustomHeadlessWithTask verifies custom adapter uses effective entrypoint. +func TestCustomHeadlessWithTask(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "myagent", + Task: "task.md", + TTY: boolPtr(false), // headless + } + + adapter := AdapterFor("myagent") + cmd := adapter.Command(cfg, SandboxTaskPath) + + expected := []string{ + "bash", + "-lc", + `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v myagent >/dev/null 2>&1; then echo "ERROR: entrypoint myagent not found in PATH" >&2; exit 1; fi; exec myagent --print "$(cat /sandbox/.config/openshell/task.md)"`, + } + + if !cmdEqual(cmd, expected) { + t.Errorf("Command() mismatch\ngot: %v\nwant: %v", cmd, expected) + } +} + +// TestCustomWithArgsHeadlessWithTask verifies custom adapter with args in entrypoint. +func TestCustomWithArgsHeadlessWithTask(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "myagent --model=mini", + Task: "task.md", + TTY: boolPtr(false), // headless + } + + adapter := AdapterFor("myagent --model=mini") + cmd := adapter.Command(cfg, SandboxTaskPath) + + expected := []string{ + "bash", + "-lc", + `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v myagent >/dev/null 2>&1; then echo "ERROR: entrypoint myagent not found in PATH" >&2; exit 1; fi; exec myagent --model=mini --print "$(cat /sandbox/.config/openshell/task.md)"`, + } + + if !cmdEqual(cmd, expected) { + t.Errorf("Command() mismatch\ngot: %v\nwant: %v", cmd, expected) + } +} + +// TestTaskPathUsesProvidedConstant verifies the command uses the passed taskPath. +func TestTaskPathUsesProvidedConstant(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "claude", + Task: "task.md", + TTY: boolPtr(false), + } + + adapter := AdapterFor("claude") + cmd := adapter.Command(cfg, SandboxTaskPath) + + // Verify the exact constant is used + if len(cmd) != 3 || cmd[0] != "bash" || cmd[1] != "-lc" { + t.Fatalf("Command structure wrong: %v", cmd) + } + cmdStr := cmd[2] + + if cmdStr != `export PATH="/sandbox/.config/openshell/bin:$PATH"; if ! command -v claude >/dev/null 2>&1; then echo "ERROR: entrypoint claude not found in PATH" >&2; exit 1; fi; exec claude --print "$(cat /sandbox/.config/openshell/task.md)"` { + t.Errorf("Task path or command structure mismatch:\n%s", cmdStr) + } +} + +// TestEnvironmentReturnsNonNilMap verifies Environment() never returns nil. +func TestEnvironmentReturnsNonNilMap(t *testing.T) { + adapters := []AgentAdapter{ + AdapterFor("claude"), + AdapterFor("codex"), + AdapterFor("opencode"), + AdapterFor("custom"), + } + + cfg := &AgentConfig{Name: "test"} + + for i, adapter := range adapters { + env := adapter.Environment(cfg) + if env == nil { + t.Errorf("adapter %d returned nil from Environment(), want non-nil map", i) + } + } +} + +// TestAnthropicBaseURLFromConfig verifies that ANTHROPIC_BASE_URL flows via BuildEnvMap. +// This test confirms the adapter does NOT add it, keeping env single-sourced. +func TestAnthropicBaseURLFromConfig(t *testing.T) { + cfg := &AgentConfig{ + Name: "test", + Entrypoint: "claude", + Env: map[string]string{ + "ANTHROPIC_BASE_URL": "http://inference.local", + }, + } + + // BuildEnvMap should return it (config owned) + envMap := cfg.BuildEnvMap() + if envMap["ANTHROPIC_BASE_URL"] != "http://inference.local" { + t.Errorf("BuildEnvMap() missing ANTHROPIC_BASE_URL, got: %v", envMap) + } + + // Adapter should NOT add it (empty map) + adapter := AdapterFor("claude") + adapterEnv := adapter.Environment(cfg) + if len(adapterEnv) > 0 { + t.Errorf("adapter.Environment() should be empty but got: %v", adapterEnv) + } +} + +// Helper functions + +func boolPtr(b bool) *bool { + return &b +} + +func typeOf(a AgentAdapter) string { + switch a.(type) { + case *claudeAdapter: + return "claudeAdapter" + case *codexAdapter: + return "codexAdapter" + case *opencodeAdapter: + return "opencodeAdapter" + case *customAdapter: + return "customAdapter" + default: + return "unknown" + } +} + +func cmdEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} From 2dc3e3dc3d57f2428f9f3cc7352ba76b5c219c60 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 14:11:37 -0700 Subject: [PATCH 2/9] PR5 S2: sandbox create argv gains policy/gateway/workspace/label/no-auto-providers Extend SandboxCreateOpts and extract the argv into a pure sandboxCreateArgs helper (golden-tested). Adds --policy, --gateway, --workspace, --label (sorted), --no-auto-providers, wiring flags the 0.0.110 CLI already supports. Zero-valued new fields produce byte-identical argv to today, so existing callers are unchanged until S5. gateway stays the single argv owner. Invariant 28. internal/gateway stays exec-only (no cobra/SDK). --- internal/gateway/cli.go | 49 +++-- internal/gateway/cli_test.go | 349 +++++++++++++++++++++++++++++++++++ internal/gateway/gateway.go | 21 ++- 3 files changed, 400 insertions(+), 19 deletions(-) diff --git a/internal/gateway/cli.go b/internal/gateway/cli.go index 37ec112..052242d 100644 --- a/internal/gateway/cli.go +++ b/internal/gateway/cli.go @@ -242,12 +242,16 @@ func (c *CLI) GatewaySelect(name string) error { return c.silent("gateway", "select", name) } -func (c *CLI) SandboxCreate(opts SandboxCreateOpts) error { +func sandboxCreateArgs(opts SandboxCreateOpts) []string { args := []string{"sandbox", "create", "--name", opts.Name} - if opts.TTY { - args = append(args, "--tty") - } else { - args = append(args, "--no-tty") + if opts.Gateway != "" { + args = append(args, "--gateway", opts.Gateway) + } + if opts.Workspace != "" { + args = append(args, "--workspace", opts.Workspace) + } + if opts.Policy != "" { + args = append(args, "--policy", opts.Policy) } if opts.From != "" { args = append(args, "--from", opts.From) @@ -255,8 +259,23 @@ func (c *CLI) SandboxCreate(opts SandboxCreateOpts) error { for _, p := range opts.Providers { args = append(args, "--provider", p) } - if !opts.Keep { - args = append(args, "--no-keep") + if opts.NoAutoProviders { + args = append(args, "--no-auto-providers") + } + if opts.TTY { + args = append(args, "--tty") + } else { + args = append(args, "--no-tty") + } + if len(opts.Env) > 0 { + keys := make([]string, 0, len(opts.Env)) + for k := range opts.Env { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + args = append(args, "--env", k+"="+opts.Env[k]) + } } if len(opts.Uploads) > 0 { for _, u := range opts.Uploads { @@ -264,20 +283,28 @@ func (c *CLI) SandboxCreate(opts SandboxCreateOpts) error { } args = append(args, "--no-git-ignore") } - if len(opts.Env) > 0 { - keys := make([]string, 0, len(opts.Env)) - for k := range opts.Env { + if !opts.Keep { + args = append(args, "--no-keep") + } + if len(opts.Labels) > 0 { + keys := make([]string, 0, len(opts.Labels)) + for k := range opts.Labels { keys = append(keys, k) } sort.Strings(keys) for _, k := range keys { - args = append(args, "--env", k+"="+opts.Env[k]) + args = append(args, "--label", k+"="+opts.Labels[k]) } } if len(opts.Command) > 0 { args = append(args, "--") args = append(args, opts.Command...) } + return args +} + +func (c *CLI) SandboxCreate(opts SandboxCreateOpts) error { + args := sandboxCreateArgs(opts) return c.passthrough(args...) } diff --git a/internal/gateway/cli_test.go b/internal/gateway/cli_test.go index 34cc50f..e246de5 100644 --- a/internal/gateway/cli_test.go +++ b/internal/gateway/cli_test.go @@ -559,3 +559,352 @@ exit 0 t.Errorf("ProviderProfileDelete: %v", err) } } + +// Test sandboxCreateArgs with all new fields set to verify pinned argv order. +func TestSandboxCreateArgs_AllNewFieldsSet(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "my-sandbox", + Gateway: "remote-gw", + Workspace: "dev", + Policy: "/tmp/policy.yaml", + From: "quay.io/test:latest", + Providers: []string{"github", "google-vertex-ai"}, + NoAutoProviders: true, + TTY: true, + Env: map[string]string{ + "KEY_B": "value_b", + "KEY_A": "value_a", + }, + Uploads: []Upload{ + {Src: "/src", Dst: "/dst"}, + }, + Keep: false, + Labels: map[string]string{ + "label_z": "z_val", + "label_a": "a_val", + }, + Command: []string{"bash", "-c", "echo test"}, + } + + args := sandboxCreateArgs(opts) + + // Expected order per spec: + // sandbox create --name + // [--gateway ] + // [--workspace ] + // [--policy

] + // [--from ] + // [--provider

]... + // [--no-auto-providers] + // (--tty | --no-tty) + // [--env k=v]... (sorted) + // [--upload src:dst]... --no-git-ignore + // [--no-keep] + // [--label k=v]... (sorted) + // [-- ...] + + expectedOrder := []string{ + "sandbox", "create", + "--name", "my-sandbox", + "--gateway", "remote-gw", + "--workspace", "dev", + "--policy", "/tmp/policy.yaml", + "--from", "quay.io/test:latest", + "--provider", "github", + "--provider", "google-vertex-ai", + "--no-auto-providers", + "--tty", + "--env", "KEY_A=value_a", + "--env", "KEY_B=value_b", + "--upload", "/src:/dst", + "--no-git-ignore", + "--no-keep", + "--label", "label_a=a_val", + "--label", "label_z=z_val", + "--", + "bash", "-c", "echo test", + } + + if len(args) != len(expectedOrder) { + t.Fatalf("got %d args, want %d. got: %v", len(args), len(expectedOrder), args) + } + + for i, want := range expectedOrder { + if args[i] != want { + t.Errorf("arg[%d]: got %q, want %q", i, args[i], want) + t.Logf("Full args: %v", args) + } + } +} + +// Test sandboxCreateArgs with all new fields zero-valued to ensure +// byte-identical output to existing callers. +func TestSandboxCreateArgs_AllNewFieldsZero(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "test", + From: "quay.io/test:v1", + Providers: []string{"provider1", "provider2"}, + TTY: false, + Keep: true, + Uploads: []Upload{ + {Src: "/src", Dst: "/dst"}, + }, + Env: map[string]string{ + "KEY_Z": "val_z", + "KEY_A": "val_a", + }, + Command: []string{"cmd"}, + // All new fields are zero-valued + Policy: "", + Gateway: "", + Workspace: "", + Labels: nil, + NoAutoProviders: false, + } + + args := sandboxCreateArgs(opts) + + // Should match the old behavior exactly (order per existing code) + expectedOrder := []string{ + "sandbox", "create", + "--name", "test", + "--from", "quay.io/test:v1", + "--provider", "provider1", + "--provider", "provider2", + "--no-tty", + "--env", "KEY_A=val_a", + "--env", "KEY_Z=val_z", + "--upload", "/src:/dst", + "--no-git-ignore", + // No --no-keep (Keep=true) + // No --label (Labels=nil) + // No --policy (Policy="") + // No --gateway (Gateway="") + // No --workspace (Workspace="") + // No --no-auto-providers (NoAutoProviders=false) + "--", + "cmd", + } + + if len(args) != len(expectedOrder) { + t.Fatalf("got %d args, want %d. got: %v", len(args), len(expectedOrder), args) + } + + for i, want := range expectedOrder { + if args[i] != want { + t.Errorf("arg[%d]: got %q, want %q", i, args[i], want) + t.Logf("Full args: %v", args) + } + } +} + +// Test that providers maintain declared order (not sorted). +func TestSandboxCreateArgs_ProvidersPreserveDeclaredOrder(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "test", + Providers: []string{"z-provider", "a-provider", "m-provider"}, + TTY: false, + Keep: true, + } + + args := sandboxCreateArgs(opts) + + // Find the provider flags and verify they appear in declared order + providerArgs := []string{} + for i, arg := range args { + if arg == "--provider" && i+1 < len(args) { + providerArgs = append(providerArgs, args[i+1]) + } + } + + expectedProviders := []string{"z-provider", "a-provider", "m-provider"} + if len(providerArgs) != len(expectedProviders) { + t.Fatalf("got %d providers, want %d: %v", len(providerArgs), len(expectedProviders), providerArgs) + } + + for i, want := range expectedProviders { + if providerArgs[i] != want { + t.Errorf("provider[%d]: got %q, want %q (providers must be in declared order, not sorted)", i, providerArgs[i], want) + } + } +} + +// Test that labels are sorted by key. +func TestSandboxCreateArgs_LabelsSorted(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "test", + TTY: false, + Keep: true, + Labels: map[string]string{ + "zebra": "z_val", + "apple": "a_val", + "mango": "m_val", + }, + } + + args := sandboxCreateArgs(opts) + + // Find the label flags and verify they appear in sorted key order + labelArgs := []string{} + for i, arg := range args { + if arg == "--label" && i+1 < len(args) { + labelArgs = append(labelArgs, args[i+1]) + } + } + + expectedLabels := []string{ + "apple=a_val", + "mango=m_val", + "zebra=z_val", + } + + if len(labelArgs) != len(expectedLabels) { + t.Fatalf("got %d labels, want %d: %v", len(labelArgs), len(expectedLabels), labelArgs) + } + + for i, want := range expectedLabels { + if labelArgs[i] != want { + t.Errorf("label[%d]: got %q, want %q (labels must be sorted by key)", i, labelArgs[i], want) + } + } +} + +// Test that env and label values do not leak from each other. +// Labels should only come from opts.Labels, never from opts.Env. +func TestSandboxCreateArgs_NoSecretLeakFromEnvToLabels(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "test", + TTY: false, + Keep: true, + Env: map[string]string{ + "SECRET_KEY": "super-secret-123", + "ANTHROPIC_API_KEY": "sk-ant-abc123", + "GITHUB_TOKEN": "ghp_secret", + }, + Labels: map[string]string{ + "config-hash": "abc123", + "run-id": "run-789", + }, + } + + args := sandboxCreateArgs(opts) + + // Collect all label values + labelValues := []string{} + for i, arg := range args { + if arg == "--label" && i+1 < len(args) { + parts := strings.Split(args[i+1], "=") + if len(parts) == 2 { + labelValues = append(labelValues, parts[1]) + } + } + } + + // Verify no env secret values appear in labels + for _, labelVal := range labelValues { + if labelVal == "super-secret-123" || labelVal == "sk-ant-abc123" || labelVal == "ghp_secret" { + t.Errorf("env secret value leaked into labels: %q", labelVal) + } + } + + // Verify expected label values are present + if len(labelValues) != 2 { + t.Fatalf("got %d label values, want 2: %v", len(labelValues), labelValues) + } +} + +// Test each new field individually when set (others zero). +func TestSandboxCreateArgs_PolicyField(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "test", + Policy: "/etc/custom-policy.yaml", + TTY: false, + Keep: true, + } + args := sandboxCreateArgs(opts) + found := false + for i, arg := range args { + if arg == "--policy" && i+1 < len(args) && args[i+1] == "/etc/custom-policy.yaml" { + found = true + break + } + } + if !found { + t.Errorf("--policy flag not found or has wrong value in: %v", args) + } +} + +func TestSandboxCreateArgs_GatewayField(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "test", + Gateway: "remote-gateway", + TTY: false, + Keep: true, + } + args := sandboxCreateArgs(opts) + found := false + for i, arg := range args { + if arg == "--gateway" && i+1 < len(args) && args[i+1] == "remote-gateway" { + found = true + break + } + } + if !found { + t.Errorf("--gateway flag not found or has wrong value in: %v", args) + } +} + +func TestSandboxCreateArgs_WorkspaceField(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "test", + Workspace: "development", + TTY: false, + Keep: true, + } + args := sandboxCreateArgs(opts) + found := false + for i, arg := range args { + if arg == "--workspace" && i+1 < len(args) && args[i+1] == "development" { + found = true + break + } + } + if !found { + t.Errorf("--workspace flag not found or has wrong value in: %v", args) + } +} + +func TestSandboxCreateArgs_NoAutoProvidersField(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "test", + NoAutoProviders: true, + TTY: false, + Keep: true, + } + args := sandboxCreateArgs(opts) + found := false + for _, arg := range args { + if arg == "--no-auto-providers" { + found = true + break + } + } + if !found { + t.Errorf("--no-auto-providers flag not found in: %v", args) + } +} + +func TestSandboxCreateArgs_NoAutoProvidersFieldNotEmitted(t *testing.T) { + opts := SandboxCreateOpts{ + Name: "test", + NoAutoProviders: false, + TTY: false, + Keep: true, + } + args := sandboxCreateArgs(opts) + for _, arg := range args { + if arg == "--no-auto-providers" { + t.Errorf("--no-auto-providers should not be present when false, but found in: %v", args) + } + } +} diff --git a/internal/gateway/gateway.go b/internal/gateway/gateway.go index aaa6b5c..71ec2ea 100644 --- a/internal/gateway/gateway.go +++ b/internal/gateway/gateway.go @@ -88,12 +88,17 @@ type Upload struct { } type SandboxCreateOpts struct { - Name string - From string - Providers []string - TTY bool - Keep bool - Uploads []Upload - Command []string - Env map[string]string + Name string + From string + Providers []string + TTY bool + Keep bool + Uploads []Upload + Command []string + Env map[string]string + Policy string // --policy + Gateway string // --gateway + Workspace string // --workspace + Labels map[string]string // --label k=v + NoAutoProviders bool // --no-auto-providers when true } From 0415d0545e2a53bc7f90d602a3359f665748a6e0 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 14:11:37 -0700 Subject: [PATCH 3/9] PR5 S3: policy staging (internal/payload) WriteEffectivePolicy writes the single policy source (kind: policy) to a caller-owned dir and returns the path for --policy at create; returns "" when no policy is configured. No provider-policy merging (none exists) and no PR6 staging-root dependency. Invariant 29/32. New package is cobra/SDK-free. --- internal/payload/policy.go | 25 +++++++ internal/payload/policy_test.go | 112 ++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 internal/payload/policy.go create mode 100644 internal/payload/policy_test.go diff --git a/internal/payload/policy.go b/internal/payload/policy.go new file mode 100644 index 0000000..b17b999 --- /dev/null +++ b/internal/payload/policy.go @@ -0,0 +1,25 @@ +package payload + +import ( + "os" + "path/filepath" +) + +// WriteEffectivePolicy writes policy YAML bytes into dir and returns the file +// path, or "" if policy is empty/nil (caller then omits --policy). dir is a +// caller-owned temp dir (caller creates and removes it). Returns an error only +// on a real write failure. +func WriteEffectivePolicy(dir string, policy []byte) (string, error) { + // If policy is nil or empty, return empty string with no error + if len(policy) == 0 { + return "", nil + } + + // Write policy bytes to policy.yaml in the given directory + filePath := filepath.Join(dir, "policy.yaml") + if err := os.WriteFile(filePath, policy, 0o644); err != nil { + return "", err + } + + return filePath, nil +} diff --git a/internal/payload/policy_test.go b/internal/payload/policy_test.go new file mode 100644 index 0000000..8954aae --- /dev/null +++ b/internal/payload/policy_test.go @@ -0,0 +1,112 @@ +package payload + +import ( + "os" + "path/filepath" + "testing" +) + +func TestWriteEffectivePolicy_NonEmpty(t *testing.T) { + dir := t.TempDir() + policy := []byte("kind: policy\napiVersion: v1\n") + + filePath, err := WriteEffectivePolicy(dir, policy) + + if err != nil { + t.Fatalf("WriteEffectivePolicy failed: %v", err) + } + + if filePath == "" { + t.Fatal("expected non-empty filepath, got empty string") + } + + // Verify the file exists at the returned path + if _, err := os.Stat(filePath); err != nil { + t.Fatalf("file at returned path does not exist: %v", err) + } + + // Verify the file is within the given dir + rel, err := filepath.Rel(dir, filePath) + if err != nil { + t.Fatalf("could not compute relative path: %v", err) + } + if filepath.IsAbs(rel) { + t.Fatal("returned path is not within dir") + } + + // Verify the exact content + content, err := os.ReadFile(filePath) + if err != nil { + t.Fatalf("failed to read file: %v", err) + } + if string(content) != string(policy) { + t.Fatalf("file content mismatch: got %q, want %q", string(content), string(policy)) + } +} + +func TestWriteEffectivePolicy_Nil(t *testing.T) { + dir := t.TempDir() + + filePath, err := WriteEffectivePolicy(dir, nil) + + if err != nil { + t.Fatalf("WriteEffectivePolicy failed: %v", err) + } + + if filePath != "" { + t.Fatalf("expected empty filepath, got %q", filePath) + } + + // Verify no file was created + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("failed to read dir: %v", err) + } + if len(entries) != 0 { + t.Fatalf("expected no files created, but found %d entries", len(entries)) + } +} + +func TestWriteEffectivePolicy_Empty(t *testing.T) { + dir := t.TempDir() + + filePath, err := WriteEffectivePolicy(dir, []byte{}) + + if err != nil { + t.Fatalf("WriteEffectivePolicy failed: %v", err) + } + + if filePath != "" { + t.Fatalf("expected empty filepath, got %q", filePath) + } + + // Verify no file was created + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("failed to read dir: %v", err) + } + if len(entries) != 0 { + t.Fatalf("expected no files created, but found %d entries", len(entries)) + } +} + +func TestWriteEffectivePolicy_FilePermissions(t *testing.T) { + dir := t.TempDir() + policy := []byte("test policy content") + + filePath, err := WriteEffectivePolicy(dir, policy) + + if err != nil { + t.Fatalf("WriteEffectivePolicy failed: %v", err) + } + + // Verify file is readable + info, err := os.Stat(filePath) + if err != nil { + t.Fatalf("failed to stat file: %v", err) + } + // Check that the file has user read permission (affected by umask) + if (info.Mode().Perm() & 0o400) == 0 { + t.Fatalf("expected file to be readable by user, got mode %o", info.Mode().Perm()) + } +} From 6c87779eb5dc16ca3dbc75ecbf286f97b8fac904 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 14:17:23 -0700 Subject: [PATCH 4/9] PR5 S4: RunSandbox lifecycle owner (internal/run) Single owner of sandbox execution (invariant 27): create -> bounded retry with best-effort delete between attempts -> cleanup per Keep. Depends on a narrow SandboxRunner interface (SandboxCreate+SandboxDelete) the real gateway satisfies structurally, so the test fake implements two methods. RetrySleep is a plain time.Duration; the retry pause is context-interruptible. Keep maps to the create flag with no post-success delete. Firewall-clean (invariant 32). --- internal/run/lifecycle.go | 69 ++++++++++++++ internal/run/run.go | 81 ++++++++++++++++ internal/run/run_test.go | 191 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 341 insertions(+) create mode 100644 internal/run/lifecycle.go create mode 100644 internal/run/run.go create mode 100644 internal/run/run_test.go diff --git a/internal/run/lifecycle.go b/internal/run/lifecycle.go new file mode 100644 index 0000000..55b3496 --- /dev/null +++ b/internal/run/lifecycle.go @@ -0,0 +1,69 @@ +package run + +import ( + "context" + "fmt" + "time" + + "github.com/stackrox/harness-openshell/internal/gateway" +) + +const maxRetries = 5 + +// toCreateOpts maps a SandboxRunRequest to gateway.SandboxCreateOpts. +func toCreateOpts(req SandboxRunRequest) gateway.SandboxCreateOpts { + opts := gateway.SandboxCreateOpts{ + Name: req.Name, + From: req.Image, + Providers: req.Providers, + NoAutoProviders: req.NoAutoProviders, + TTY: req.TTY, + Keep: req.Keep, + Uploads: req.Uploads, + Command: req.Command, + Env: req.Env, + Gateway: req.Gateway, + Workspace: req.Workspace, + Labels: req.Labels, + } + if req.PolicyPath != "" { + opts.Policy = req.PolicyPath + } + return opts +} + +// runSandboxWithLifecycle implements the bounded-retry loop with best-effort +// cleanup (invariant 34). It mirrors cmd/sandbox.go's createSandbox exactly. +func runSandboxWithLifecycle(ctx context.Context, gw SandboxRunner, req SandboxRunRequest) error { + for attempt := 1; attempt <= maxRetries; attempt++ { + // Honor context cancellation before each attempt. + if ctx.Err() != nil { + return ctx.Err() + } + + opts := toCreateOpts(req) + err := gw.SandboxCreate(opts) + if err == nil { + // Success; sandbox was created and is retained per Keep. + return nil + } + + // Failed; attempt best-effort cleanup (ignore its error). + gw.SandboxDelete(req.Name) + + // On the last attempt, return the wrapped error. + if attempt == maxRetries { + return fmt.Errorf("sandbox create failed after %d attempts: %w", maxRetries, err) + } + + // Pause before the next attempt, honoring cancellation. + if req.RetrySleep > 0 { + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(req.RetrySleep): + } + } + } + return nil // unreachable: the loop returns on attempt == maxRetries +} diff --git a/internal/run/run.go b/internal/run/run.go new file mode 100644 index 0000000..a35504c --- /dev/null +++ b/internal/run/run.go @@ -0,0 +1,81 @@ +package run + +import ( + "context" + "time" + + "github.com/stackrox/harness-openshell/internal/gateway" +) + +// SandboxRunRequest carries the neutral vocabulary needed to create and run a +// sandbox. All fields are primitives — no agent, cobra, or SDK types — allowing +// this package to remain firewall-clean (invariant 32). +type SandboxRunRequest struct { + // Name is the sandbox name. + Name string + + // Gateway is the gateway context name (empty → active context). + Gateway string + + // Workspace is the workspace name (empty → default). + Workspace string + + // Image is the sandbox image ref or relative Dockerfile dir. + Image string + + // Providers are the registered providers to attach, in declared order. + Providers []string + + // NoAutoProviders, when true, disables auto-discovery of providers. + NoAutoProviders bool + + // Env is the environment variables to inject via --env on sandbox create. + Env map[string]string + + // Command is the argv to execute inside the sandbox (adapter-produced). + Command []string + + // Uploads are additional uploads to stage in the sandbox (caller pre-stages + // payload dir; S5 owns temp dirs). + Uploads []gateway.Upload + + // TTY, when true, preserves native TTY streaming (invariant 31). + TTY bool + + // Keep, when true, retains the sandbox after creation; when false, deletes it. + Keep bool + + // PolicyPath is the staged policy file path ("" → omit --policy). + PolicyPath string + + // Labels are arbitrary key-value labels to attach to the sandbox. + Labels map[string]string + + // RetrySleep is the duration to pause between retry attempts. Zero means no + // pause (tests pass zero; callers pass the real backoff). + RetrySleep time.Duration +} + +// SandboxRunner is the minimal interface required to execute a sandbox lifecycle. +// The real *gateway.CLI and gateway.Gateway satisfy this structurally. +type SandboxRunner interface { + SandboxCreate(opts gateway.SandboxCreateOpts) error + SandboxDelete(name string) error +} + +// RunSandbox creates and runs a sandbox with the given request, mirroring the +// behavior of cmd/sandbox.go's createSandbox. It is the single owner of sandbox +// execution (invariant 27). +// +// Behavior: +// - Maps req → gateway.SandboxCreateOpts field-for-field. +// - Bounded retry: up to 5 attempts. On each failure, attempts best-effort +// SandboxDelete and then retries (sleeping between attempts). +// - After the last failure, returns a wrapped error mentioning the attempt count. +// - Honors context cancellation: if ctx.Err() != nil, returns immediately +// without calling SandboxCreate. +// - Preserves native TTY (does not wrap stdout/stderr). +// - On success, the sandbox is retained or deleted according to Keep. +func RunSandbox(ctx context.Context, gw SandboxRunner, req SandboxRunRequest) error { + return runSandboxWithLifecycle(ctx, gw, req) +} diff --git a/internal/run/run_test.go b/internal/run/run_test.go new file mode 100644 index 0000000..0149567 --- /dev/null +++ b/internal/run/run_test.go @@ -0,0 +1,191 @@ +package run + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/stackrox/harness-openshell/internal/gateway" +) + +// fakeRunner is a fake SandboxRunner that records calls and returns scripted +// SandboxCreate errors (one per call, in order; exhausted => nil). +type fakeRunner struct { + calls []runnerCall + creates []error + nextIdx int +} + +type runnerCall struct { + method string + name string + opts gateway.SandboxCreateOpts +} + +func (f *fakeRunner) SandboxCreate(opts gateway.SandboxCreateOpts) error { + f.calls = append(f.calls, runnerCall{method: "SandboxCreate", name: opts.Name, opts: opts}) + if f.nextIdx >= len(f.creates) { + return nil + } + err := f.creates[f.nextIdx] + f.nextIdx++ + return err +} + +func (f *fakeRunner) SandboxDelete(name string) error { + f.calls = append(f.calls, runnerCall{method: "SandboxDelete", name: name}) + return nil +} + +func (f *fakeRunner) methods() []string { + ms := make([]string, len(f.calls)) + for i, c := range f.calls { + ms[i] = c.method + } + return ms +} + +func TestRunSandboxSuccess(t *testing.T) { + gw := &fakeRunner{} + req := SandboxRunRequest{Name: "test-sandbox", Image: "ubuntu:20.04"} + + if err := RunSandbox(context.Background(), gw, req); err != nil { + t.Fatalf("RunSandbox failed: %v", err) + } + + // Exactly one create, no deletes. + if got, want := gw.methods(), []string{"SandboxCreate"}; !equalStrings(got, want) { + t.Fatalf("calls = %v, want %v", got, want) + } +} + +func TestRunSandboxRetryThenSucceed(t *testing.T) { + gw := &fakeRunner{creates: []error{errors.New("transient 1"), errors.New("transient 2")}} + req := SandboxRunRequest{Name: "test-sandbox", Image: "ubuntu:20.04"} + + if err := RunSandbox(context.Background(), gw, req); err != nil { + t.Fatalf("RunSandbox failed: %v", err) + } + + // 3 creates, a best-effort delete between each failed attempt. + want := []string{"SandboxCreate", "SandboxDelete", "SandboxCreate", "SandboxDelete", "SandboxCreate"} + if got := gw.methods(); !equalStrings(got, want) { + t.Fatalf("calls = %v, want %v", got, want) + } +} + +func TestRunSandboxExhaustRetries(t *testing.T) { + gw := &fakeRunner{creates: []error{ + errors.New("f1"), errors.New("f2"), errors.New("f3"), errors.New("f4"), errors.New("f5"), + }} + req := SandboxRunRequest{Name: "test-sandbox", Image: "ubuntu:20.04"} + + err := RunSandbox(context.Background(), gw, req) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "sandbox create failed after 5 attempts") { + t.Fatalf("error = %q, want it to mention 5 attempts", err) + } + // Wrapped cause is preserved. + if !strings.Contains(err.Error(), "f5") { + t.Fatalf("error = %q, want wrapped last cause f5", err) + } + + // 5 creates + a delete after each failed attempt. + want := []string{ + "SandboxCreate", "SandboxDelete", "SandboxCreate", "SandboxDelete", + "SandboxCreate", "SandboxDelete", "SandboxCreate", "SandboxDelete", + "SandboxCreate", "SandboxDelete", + } + if got := gw.methods(); !equalStrings(got, want) { + t.Fatalf("calls = %v, want %v", got, want) + } +} + +func TestContextCancellation(t *testing.T) { + gw := &fakeRunner{} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := RunSandbox(ctx, gw, SandboxRunRequest{Name: "test-sandbox", Image: "ubuntu:20.04"}) + if !errors.Is(err, context.Canceled) { + t.Fatalf("err = %v, want context.Canceled", err) + } + if len(gw.calls) != 0 { + t.Fatalf("expected 0 calls, got %d", len(gw.calls)) + } +} + +func TestToCreateOpts(t *testing.T) { + t.Run("all fields", func(t *testing.T) { + req := SandboxRunRequest{ + Name: "sandbox", + Gateway: "local", + Workspace: "ws1", + Image: "ubuntu:20.04", + Providers: []string{"p1", "p2"}, + NoAutoProviders: true, + TTY: true, + Keep: true, + Env: map[string]string{"KEY": "val"}, + Command: []string{"bash"}, + Labels: map[string]string{"id": "123"}, + PolicyPath: "/tmp/policy.yaml", + } + opts := toCreateOpts(req) + if opts.Name != "sandbox" || opts.Gateway != "local" || opts.Workspace != "ws1" { + t.Fatalf("name/gateway/workspace mismatch: %+v", opts) + } + if opts.From != "ubuntu:20.04" { + t.Fatalf("Image should map to From, got %q", opts.From) + } + if len(opts.Providers) != 2 || opts.Providers[0] != "p1" || !opts.NoAutoProviders { + t.Fatalf("providers mismatch: %+v", opts) + } + if !opts.TTY || !opts.Keep { + t.Fatalf("tty/keep mismatch: %+v", opts) + } + if opts.Policy != "/tmp/policy.yaml" { + t.Fatalf("PolicyPath should map to Policy, got %q", opts.Policy) + } + }) + + t.Run("empty policy path omits Policy", func(t *testing.T) { + opts := toCreateOpts(SandboxRunRequest{Name: "sandbox", Image: "ubuntu"}) + if opts.Policy != "" { + t.Fatalf("Policy should be empty, got %q", opts.Policy) + } + }) +} + +func TestKeepMapsToCreateFlagNoPostSuccessDelete(t *testing.T) { + for _, keep := range []bool{true, false} { + gw := &fakeRunner{} + req := SandboxRunRequest{Name: "test-sandbox", Image: "ubuntu:20.04", Keep: keep} + + if err := RunSandbox(context.Background(), gw, req); err != nil { + t.Fatalf("RunSandbox failed: %v", err) + } + // Keep flows to the create flag; success never triggers a delete. + if got := gw.methods(); !equalStrings(got, []string{"SandboxCreate"}) { + t.Fatalf("keep=%v: calls = %v, want [SandboxCreate]", keep, got) + } + if gw.calls[0].opts.Keep != keep { + t.Fatalf("keep=%v: opts.Keep = %v", keep, gw.calls[0].opts.Keep) + } + } +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} From 6ca402a5b1d095896802970101a2f8eb8fe8e1fb Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 14:24:43 -0700 Subject: [PATCH 5/9] PR5 S5: cmd calls run.RunSandbox; policy-at-create; delete run.sh + PolicySet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit upLocal is now a thin caller of run.RunSandbox (invariant 27). The in-sandbox command comes from agent.AdapterFor(...).Command (invariant 30) instead of the generated run.sh; headless-with-no-task still runs ["true"]. A configured kind:policy doc is staged via payload.WriteEffectivePolicy and applied AT CREATE via --policy (invariant 29) — the post-create gw.PolicySet path is deleted, and with its last caller gone PolicySet is removed from the Gateway interface + CLI. createSandbox/sandboxOpts (single caller, dead onSuccess) are deleted; their payload staging and Dockerfile-dir resolution move to stagePayloadUpload / resolveSandboxImagePath in cmd/sandbox.go. --gateway is now explicit via ActiveGateway(). BuildRunSh and the run.sh write in RenderPayload are gone. Gates green: build, vet, test, golangci-lint 0, config-suite 33/33, firewall clean. --- cmd/apply.go | 3 +- cmd/executor.go | 84 +++++++++++++++++-------------- cmd/helpers_test.go | 1 - cmd/sandbox.go | 97 ++++++++---------------------------- cmd/status_cmd_test.go | 1 - internal/agent/agent.go | 42 ++-------------- internal/agent/agent_test.go | 46 +++-------------- internal/gateway/cli.go | 4 -- internal/gateway/gateway.go | 3 -- 9 files changed, 81 insertions(+), 200 deletions(-) diff --git a/cmd/apply.go b/cmd/apply.go index 0beacc0..863dcb5 100644 --- a/cmd/apply.go +++ b/cmd/apply.go @@ -56,7 +56,8 @@ then deploy a sandbox. Use --dry-run to validate without deploying, or agentCfg.Entrypoint = entrypoint } if task != "" && !attach { - // Headless task: set TTY=false so BuildRunSh generates --print + // Headless task: set TTY=false so the agent adapter dispatches with + // --print (claude/codex) or run (opencode) instead of interactive -p. f := false agentCfg.TTY = &f } diff --git a/cmd/executor.go b/cmd/executor.go index 3e84210..186a56a 100644 --- a/cmd/executor.go +++ b/cmd/executor.go @@ -15,8 +15,10 @@ import ( "github.com/stackrox/harness-openshell/internal/gateway" "github.com/stackrox/harness-openshell/internal/k8s" "github.com/stackrox/harness-openshell/internal/openshell" + "github.com/stackrox/harness-openshell/internal/payload" "github.com/stackrox/harness-openshell/internal/plan" "github.com/stackrox/harness-openshell/internal/reconcile" + "github.com/stackrox/harness-openshell/internal/run" "github.com/stackrox/harness-openshell/internal/status" ) @@ -123,7 +125,7 @@ func upLocal(opts upLocalOpts) error { // Resolve payload entries into upload pairs. Inline content payloads are // written to temp files that are uploaded individually by their own // sandbox_path, so their source paths must survive until SandboxCreate. - // They MUST NOT live inside payloadDir: createSandbox renames payloadDir + // They MUST NOT live inside payloadDir: stagePayloadUpload renames payloadDir // into a staging directory, which would invalidate any path pointing inside // it and fail every upload with "local path does not exist" (issue #84). var extraUploads []gateway.Upload @@ -148,53 +150,61 @@ func upLocal(opts upLocalOpts) error { } status.Header("Sandbox") + + // Command: the agent adapter owns entrypoint + task dispatch (replaces the + // generated run.sh). Headless with no task starts the sandbox without an agent. + taskPath := "" + if agentCfg.Task != "" { + taskPath = agent.SandboxTaskPath + } var sandboxCmd []string if noTTY && agentCfg.Task == "" { sandboxCmd = []string{"true"} } else { - sandboxCmd = []string{"bash", "/sandbox/.config/openshell/run.sh"} - } - - err = createSandbox(sandboxOpts{ - harnessDir: opts.harnessDir, - gw: gw, - name: sandboxName, - image: sandboxImage, - providers: registered, - noTTY: noTTY, - retrySleep: opts.retrySleep, - sandboxCmd: sandboxCmd, - payloadDir: payloadDir, - uploads: extraUploads, - env: agentCfg.BuildEnvMap(), - }) + sandboxCmd = agent.AdapterFor(agentCfg.EffectiveEntrypoint()).Command(agentCfg, taskPath) + } + + // Stage the rendered payload for upload (--upload lands it at + // /sandbox/.config/openshell/*). + uploadDir, cleanupUpload, err := stagePayloadUpload(payloadDir) if err != nil { return err } + defer cleanupUpload() + + uploads := []gateway.Upload{{Src: uploadDir, Dst: "/sandbox/.config"}} + uploads = append(uploads, extraUploads...) - // Apply custom policy after sandbox creation (kind: policy in harness YAML). - // /etc/openshell/policy.yaml is read-only in the image, so policy changes - // must go through the openshell CLI which hot-reloads the policy. + // Stage the effective policy and apply it AT CREATE via --policy. + // /etc/openshell/policy.yaml is read-only in the image; policy-at-create is + // authoritative (the old post-create hot-reload could be silently dropped). + var policyPath string if opts.harness != nil && opts.harness.Policy != nil { - policyFile, writeErr := os.CreateTemp("", "harness-policy-*.yaml") - if writeErr != nil { - return fmt.Errorf("creating policy temp file: %w", writeErr) + policyDir, mkErr := os.MkdirTemp("", "harness-policy-") + if mkErr != nil { + return fmt.Errorf("creating policy dir: %w", mkErr) } - defer os.Remove(policyFile.Name()) - if _, writeErr := policyFile.Write(opts.harness.Policy); writeErr != nil { - policyFile.Close() - return fmt.Errorf("writing policy: %w", writeErr) + defer os.RemoveAll(policyDir) + p, wErr := payload.WriteEffectivePolicy(policyDir, opts.harness.Policy) + if wErr != nil { + return fmt.Errorf("writing policy: %w", wErr) } - policyFile.Close() - - status.Info("Applying custom policy...") - if err := gw.PolicySet(sandboxName, policyFile.Name()); err != nil { - return fmt.Errorf("applying policy: %w", err) - } - status.OK("Policy applied") - } - - return nil + policyPath = p + } + + return run.RunSandbox(context.Background(), gw, run.SandboxRunRequest{ + Name: sandboxName, + Gateway: gw.ActiveGateway(), + Image: resolveSandboxImagePath(sandboxImage, opts.harnessDir), + Providers: registered, + Env: agentCfg.BuildEnvMap(), + Command: sandboxCmd, + Uploads: uploads, + TTY: !noTTY, + Keep: true, + PolicyPath: policyPath, + RetrySleep: opts.retrySleep, + }) } // cloneRepo clones or updates a cached git repository and returns an Upload diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index 0b598f3..f217064 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -108,4 +108,3 @@ env: `), 0o644) return dir } -func (m *mockGW) PolicySet(string, string) error { return nil } diff --git a/cmd/sandbox.go b/cmd/sandbox.go index e59ae3b..dc797fb 100644 --- a/cmd/sandbox.go +++ b/cmd/sandbox.go @@ -4,87 +4,34 @@ import ( "fmt" "os" "path/filepath" - "time" - - "github.com/stackrox/harness-openshell/internal/gateway" - "github.com/stackrox/harness-openshell/internal/status" ) -// sandboxOpts holds the parameters that vary between callers of -// createSandbox (upLocal vs create). Everything else is derived -// from the agent config passed alongside. -type sandboxOpts struct { - harnessDir string - gw gateway.Gateway - name string // sandbox name - image string // sandbox image ref or relative Dockerfile dir - providers []string // registered providers to attach - noTTY bool // true → TTY=false for the sandbox - retrySleep time.Duration // pause between retry attempts - sandboxCmd []string // command to run inside the sandbox - payloadDir string // pre-rendered payload dir to upload - uploads []gateway.Upload // additional uploads (payloads) - env map[string]string // env vars injected via --env on sandbox create - onSuccess func(name string) // called after successful creation (optional) -} - -// createSandbox resolves the image path, stages the payload directory, -// creates the sandbox with up to 5 retries, and cleans up on failure. -// Both upLocal and create delegate to this function after preparing -// their caller-specific sandboxOpts. -func createSandbox(opts sandboxOpts) error { - // Relative Dockerfile dirs are resolved against harnessDir. - image := opts.image - if image != "" && !filepath.IsAbs(image) { - candidate := filepath.Join(opts.harnessDir, image) - if info, err := os.Stat(candidate); err == nil && info.IsDir() { - image = candidate - } +// resolveSandboxImagePath resolves a relative Dockerfile directory against +// harnessDir. An image ref (or an already-absolute path) is returned unchanged. +func resolveSandboxImagePath(image, harnessDir string) string { + if image == "" || filepath.IsAbs(image) { + return image } + candidate := filepath.Join(harnessDir, image) + if info, err := os.Stat(candidate); err == nil && info.IsDir() { + return candidate + } + return image +} - // Stage upload directory. openshell --upload copies the source directory - // BY NAME into the destination, so we always create a subdirectory called - // "openshell" and upload to /sandbox/.config → /sandbox/.config/openshell/*. +// stagePayloadUpload moves the rendered payload dir into a staging directory +// named "openshell" so that `openshell --upload` copies it BY NAME into +// /sandbox/.config → /sandbox/.config/openshell/*. It returns the staged upload +// source dir and a cleanup func that removes the staging parent. +func stagePayloadUpload(payloadDir string) (uploadDir string, cleanup func(), err error) { tmpParent, err := os.MkdirTemp("", "harness-") if err != nil { - return fmt.Errorf("creating staging dir: %w", err) + return "", nil, fmt.Errorf("creating staging dir: %w", err) } - defer os.RemoveAll(tmpParent) - uploadDir := filepath.Join(tmpParent, "openshell") - - if err := os.Rename(opts.payloadDir, uploadDir); err != nil { - return fmt.Errorf("staging payload: %w", err) - } - - const maxRetries = 5 - for attempt := 1; attempt <= maxRetries; attempt++ { - uploads := []gateway.Upload{{Src: uploadDir, Dst: "/sandbox/.config"}} - uploads = append(uploads, opts.uploads...) - - err := opts.gw.SandboxCreate(gateway.SandboxCreateOpts{ - Name: opts.name, - From: image, - Providers: opts.providers, - TTY: !opts.noTTY, - Keep: true, - Uploads: uploads, - Command: opts.sandboxCmd, - Env: opts.env, - }) - if err == nil { - if opts.onSuccess != nil { - opts.onSuccess(opts.name) - } - return nil - } - - status.Warnf("attempt %d: %v, retrying in %s", attempt, err, opts.retrySleep) - opts.gw.SandboxDelete(opts.name) // best-effort cleanup - - if attempt == maxRetries { - return fmt.Errorf("sandbox create failed after 5 attempts: %w", err) - } - time.Sleep(opts.retrySleep) + uploadDir = filepath.Join(tmpParent, "openshell") + if err := os.Rename(payloadDir, uploadDir); err != nil { + os.RemoveAll(tmpParent) + return "", nil, fmt.Errorf("staging payload: %w", err) } - return nil // unreachable but required by compiler + return uploadDir, func() { os.RemoveAll(tmpParent) }, nil } diff --git a/cmd/status_cmd_test.go b/cmd/status_cmd_test.go index bd9a50d..df2c3ec 100644 --- a/cmd/status_cmd_test.go +++ b/cmd/status_cmd_test.go @@ -50,4 +50,3 @@ func TestRunStatus_NoGateway(t *testing.T) { t.Fatalf("runStatus: %v", err) } } -func (m *statusMockGW) PolicySet(string, string) error { return nil } diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 579985b..c33ddda 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -356,39 +356,10 @@ func (c *AgentConfig) BuildEnvMap() map[string]string { return env } -func (c *AgentConfig) BuildRunSh() string { - var b strings.Builder - b.WriteString("#!/usr/bin/env bash\nset -euo pipefail\n\n") - b.WriteString("PAYLOAD_DIR=\"$(cd \"$(dirname \"$0\")\" && pwd)\"\n\n") - b.WriteString("# Prepend payload bin to PATH\n") - b.WriteString("export PATH=\"$PAYLOAD_DIR/bin:$PATH\"\n\n") - b.WriteString("# Validate entrypoint\n") - entrypoint := c.EffectiveEntrypoint() - epBin := strings.Fields(entrypoint)[0] - fmt.Fprintf(&b, "if ! command -v %q >/dev/null 2>&1; then\n", epBin) - fmt.Fprintf(&b, " echo \"ERROR: entrypoint %q not found in PATH\" >&2\n", epBin) - b.WriteString(" exit 1\n") - b.WriteString("fi\n\n") - b.WriteString("# Execute entrypoint\n") - if c.Task != "" { - b.WriteString("TASK=\"$(cat \"$PAYLOAD_DIR/task.md\")\"\n") - if c.NoTTY() { - // Headless: use --print (claude) or run (opencode) for stdout output - switch epBin { - case "opencode": - fmt.Fprintf(&b, "exec %s run \"$TASK\"\n", entrypoint) - default: - fmt.Fprintf(&b, "exec %s --print \"$TASK\"\n", entrypoint) - } - } else { - fmt.Fprintf(&b, "exec %s -p \"$TASK\"\n", entrypoint) - } - } else { - fmt.Fprintf(&b, "exec %s\n", entrypoint) - } - return b.String() -} - +// RenderPayload writes the payload directory uploaded into the sandbox: the bin/ +// dir (for payload-provided binaries on PATH), task.md when a task is set, and +// any includes. The in-sandbox command is built by the agent adapters (see +// internal/agent/adapter.go), not a generated run.sh. func RenderPayload(cfg *AgentConfig, baseDir, destDir string) error { if err := os.MkdirAll(destDir, 0o755); err != nil { return fmt.Errorf("creating payload dir: %w", err) @@ -398,11 +369,6 @@ func RenderPayload(cfg *AgentConfig, baseDir, destDir string) error { return fmt.Errorf("creating bin dir: %w", err) } - runSh := cfg.BuildRunSh() - if err := os.WriteFile(filepath.Join(destDir, "run.sh"), []byte(runSh), 0o755); err != nil { - return fmt.Errorf("writing run.sh: %w", err) - } - if cfg.Task != "" { taskSrc := cfg.Task if !filepath.IsAbs(taskSrc) { diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 7ca58d5..8d13a57 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -266,37 +266,6 @@ func TestBuildEnvMap_Empty(t *testing.T) { } } -func TestBuildRunSh(t *testing.T) { - cfg := &AgentConfig{ - Entrypoint: "claude", - Task: "tasks/standup.md", - } - runSh := cfg.BuildRunSh() - if !strings.Contains(runSh, "#!/usr/bin/env bash") { - t.Error("missing shebang") - } - if strings.Contains(runSh, "env.sh") { - t.Error("run.sh should not source env.sh — env vars are injected via --env") - } - if !strings.Contains(runSh, `command -v "claude"`) { - t.Error("missing entrypoint validation") - } - if !strings.Contains(runSh, `exec claude -p "$TASK"`) { - t.Errorf("missing task exec with -p in:\n%s", runSh) - } -} - -func TestBuildRunSh_NoTask(t *testing.T) { - cfg := &AgentConfig{Entrypoint: "codex"} - runSh := cfg.BuildRunSh() - if !strings.Contains(runSh, "exec codex\n") { - t.Errorf("expected bare exec, got:\n%s", runSh) - } - if strings.Contains(runSh, "task.md") { - t.Error("should not reference task.md when no task set") - } -} - func TestRenderPayload(t *testing.T) { baseDir := t.TempDir() os.WriteFile(filepath.Join(baseDir, "my-task.md"), []byte("Do the thing: ${USER}"), 0o644) @@ -315,8 +284,10 @@ func TestRenderPayload(t *testing.T) { t.Fatalf("RenderPayload: %v", err) } - if _, err := os.Stat(filepath.Join(destDir, "run.sh")); err != nil { - t.Error("missing run.sh") + // run.sh is no longer generated — the in-sandbox command is built by the + // agent adapters (see adapter_test.go). + if _, err := os.Stat(filepath.Join(destDir, "run.sh")); !os.IsNotExist(err) { + t.Error("run.sh should not be created — command comes from the adapter") } if _, err := os.Stat(filepath.Join(destDir, "task.md")); err != nil { t.Error("missing task.md") @@ -328,11 +299,6 @@ func TestRenderPayload(t *testing.T) { t.Error("env.sh should not be created — env vars are injected via --env") } - runData, _ := os.ReadFile(filepath.Join(destDir, "run.sh")) - if !strings.Contains(string(runData), "exec claude") { - t.Errorf("run.sh missing entrypoint:\n%s", runData) - } - taskData, _ := os.ReadFile(filepath.Join(destDir, "task.md")) if strings.Contains(string(taskData), "${USER}") { t.Error("task.md should have envsubst applied") @@ -353,8 +319,8 @@ func TestRenderPayload_NoEnv(t *testing.T) { if _, err := os.Stat(filepath.Join(destDir, "env.sh")); !os.IsNotExist(err) { t.Error("env.sh should not exist when no config vars") } - if _, err := os.Stat(filepath.Join(destDir, "run.sh")); err != nil { - t.Error("run.sh should always be created") + if _, err := os.Stat(filepath.Join(destDir, "run.sh")); !os.IsNotExist(err) { + t.Error("run.sh should not be created") } } diff --git a/internal/gateway/cli.go b/internal/gateway/cli.go index 052242d..0645798 100644 --- a/internal/gateway/cli.go +++ b/internal/gateway/cli.go @@ -106,10 +106,6 @@ func (c *CLI) CheckMinVersion(minVersion string) error { return nil } -func (c *CLI) PolicySet(name, policyFile string) error { - return c.passthrough("policy", "set", name, "--policy", policyFile, "--wait") -} - func (c *CLI) InferenceGet() error { return c.silent("inference", "get") } diff --git a/internal/gateway/gateway.go b/internal/gateway/gateway.go index 71ec2ea..50e951e 100644 --- a/internal/gateway/gateway.go +++ b/internal/gateway/gateway.go @@ -18,9 +18,6 @@ type Gateway interface { SandboxCreate(opts SandboxCreateOpts) error SandboxDelete(name string) error - // Policy - PolicySet(name, policyFile string) error - // Inference // // The inference route is owned by the SDK reconcile path From a731c2828b2f8c11c6030847c10df464ae4176d3 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 14:51:15 -0700 Subject: [PATCH 6/9] fix(agent): strip kind: policy discriminator at capture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sandbox create --policy rejected the policy YAML with an "unknown field kind" error: ParseHarness stored the kind: policy document verbatim into Harness.Policy, and S5 routes those bytes straight to --policy at create. Three consumers disagreed on whether the body carries kind — the gateway --policy parser rejects it, RenderHarness prepends its own kind header (doubling it), and acp/renderPolicy deleted it defensively. Strip the discriminator once at the parse layer via policyBody: re-marshal the policy mapping without its top-level kind key. RenderHarness's header prepend is now correct (single kind) and acp's delete is a harmless no-op. Found by live S6 acceptance on both the OCP (mTLS) and managed HyperShell (OIDC) gateways; policy-at-create is now verified via 'openshell policy get --full'. --- internal/agent/agent.go | 33 +++++++++++++++++++++++++++-- internal/agent/agent_test.go | 41 ++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index c33ddda..e88739a 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -164,7 +164,7 @@ type Harness struct { Gateways map[string][]byte // name -> raw gateway YAML Providers map[string][]byte // name -> raw provider profile YAML Payloads []PayloadEntry // files to upload to sandbox - Policy []byte // raw policy YAML + Policy []byte // policy body YAML, without the kind: policy discriminator } // kindHeader peeks at the kind and name fields of a YAML document. @@ -264,7 +264,14 @@ func ParseHarness(data []byte) (*Harness, error) { if h.Policy != nil { return nil, fmt.Errorf("multiple policy documents found") } - h.Policy = raw + // Store the bare policy body: the gateway's --policy parser (and the + // ACP exporter) reject the kind discriminator as an unknown field, and + // RenderHarness re-adds the `kind: policy` header on serialize. + body, err := policyBody(&node) + if err != nil { + return nil, fmt.Errorf("re-marshaling policy document %d: %w", docIndex, err) + } + h.Policy = body default: return nil, fmt.Errorf("document %d: unknown kind %q", docIndex, header.Kind) @@ -280,6 +287,28 @@ func ParseHarness(data []byte) (*Harness, error) { return h, nil } +// policyBody re-marshals a kind: policy document without the kind discriminator, +// yielding the bare policy YAML that the gateway's --policy parser accepts (it +// rejects unknown top-level keys such as kind) and that RenderHarness re-wraps +// under a `kind: policy` header. Non-mapping documents fall through unchanged. +func policyBody(doc *yaml.Node) ([]byte, error) { + mapping := doc + if doc.Kind == yaml.DocumentNode && len(doc.Content) == 1 { + mapping = doc.Content[0] + } + if mapping.Kind == yaml.MappingNode { + filtered := make([]*yaml.Node, 0, len(mapping.Content)) + for i := 0; i+1 < len(mapping.Content); i += 2 { + if mapping.Content[i].Value == "kind" { + continue + } + filtered = append(filtered, mapping.Content[i], mapping.Content[i+1]) + } + mapping.Content = filtered + } + return yaml.Marshal(doc) +} + // RenderHarness writes a complete multi-document YAML from a Harness. // builtinProviders are labeled with a comment; custom providers are included as-is. func RenderHarness(h *Harness, builtinProviders map[string][]byte) ([]byte, error) { diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 8d13a57..af8bd49 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -410,6 +410,47 @@ network_policies: if h.Policy == nil { t.Error("missing policy") } + // Policy is stored as the bare body: the kind discriminator is stripped so + // the gateway's --policy parser accepts it, but the policy fields survive. + if strings.Contains(string(h.Policy), "kind:") { + t.Errorf("Policy retained kind discriminator:\n%s", h.Policy) + } + if !strings.Contains(string(h.Policy), "network_policies:") { + t.Errorf("Policy dropped its body:\n%s", h.Policy) + } +} + +// TestParseHarness_PolicyRoundTrip proves ParseHarness → RenderHarness restores a +// single kind: policy header (capture strips it, RenderHarness re-adds it) rather +// than duplicating it. +func TestParseHarness_PolicyRoundTrip(t *testing.T) { + data := []byte(`kind: agent +name: rt +entrypoint: claude +providers: [] +--- +kind: policy +network_policies: + demo: + endpoints: + - host: example.com + port: 443 +`) + h, err := ParseHarness(data) + if err != nil { + t.Fatalf("ParseHarness: %v", err) + } + out, err := RenderHarness(h, nil) + if err != nil { + t.Fatalf("RenderHarness: %v", err) + } + if n := strings.Count(string(out), "kind: policy"); n != 1 { + t.Errorf("kind: policy appears %d times, want 1:\n%s", n, out) + } + // The rendered harness must re-parse (proves it is not malformed). + if _, err := ParseHarness(out); err != nil { + t.Fatalf("re-parsing rendered harness: %v", err) + } } func TestParseHarness_DuplicateAgent(t *testing.T) { From ccc1729c09771cbb0485c3ecc564c7d5c7d1fe59 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 15:05:31 -0700 Subject: [PATCH 7/9] fix(status): redact sensitive --env values in command diagnostics PR5 routes sandbox create through passthrough -> status.Cmd, which echoes the full argv (including --env KEY=VALUE) when --show-commands/verbose is on. formatCmdLine redacted --credential/--material/--from-literal but not --env, so a secret passed via env: (e.g. ANTHROPIC_API_KEY) leaked in plaintext to stdout/stderr. Apply the existing --from-literal sensitivity heuristic to --env: mask the value when the key matches TOKEN/SECRET/PASSWORD/KEY/CREDENTIAL, keep the key visible, and leave benign env (e.g. ANTHROPIC_BASE_URL) readable. Enforces invariant 33 (secrets never reach status.Cmd diagnostics). --- internal/status/status.go | 19 +++++++++++++++++++ internal/status/status_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/internal/status/status.go b/internal/status/status.go index ad18b12..ee687a3 100644 --- a/internal/status/status.go +++ b/internal/status/status.go @@ -27,6 +27,7 @@ func formatCmdLine(name string, args []string) string { b.WriteString("$ ") b.WriteString(name) redactNext := false + redactNextEnvIfSensitive := false for _, a := range args { b.WriteByte(' ') if redactNext { @@ -34,11 +35,29 @@ func formatCmdLine(name string, args []string) string { redactNext = false continue } + if redactNextEnvIfSensitive { + // --env values carry secrets on the acknowledged-plaintext path + // (the gateway also warns the agent can read them). Mask the value + // when the key looks sensitive; keep KEY visible either way, and + // leave benign env (e.g. ANTHROPIC_BASE_URL) readable for debugging. + if isSensitiveLiteral(a) { + b.WriteString(redactValue(a)) + } else { + b.WriteString(a) + } + redactNextEnvIfSensitive = false + continue + } if a == "--credential" || a == "--material" || a == "--secret-material-key" { redactNext = true b.WriteString(a) continue } + if a == "--env" { + redactNextEnvIfSensitive = true + b.WriteString(a) + continue + } if strings.HasPrefix(a, "--from-literal=") && isSensitiveLiteral(a) { b.WriteString(redactFromLiteral(a)) continue diff --git a/internal/status/status_test.go b/internal/status/status_test.go index 112a039..e334a83 100644 --- a/internal/status/status_test.go +++ b/internal/status/status_test.go @@ -80,6 +80,32 @@ func TestCmdDoesNotRedactNonSensitiveLiteral(t *testing.T) { } } +func TestCmdRedactsSensitiveEnv(t *testing.T) { + // sandbox create --env carries secrets on the plaintext path; a sensitive + // key's value must not leak into diagnostics, but the key stays visible. + out := captureCmd("openshell", "sandbox", "create", + "--env", "ANTHROPIC_API_KEY=sk-secret-xyz", + "--env", "ANTHROPIC_BASE_URL=https://inference.local") + if contains(out, "sk-secret-xyz") { + t.Errorf("sensitive env value leaked: %s", out) + } + if !contains(out, "ANTHROPIC_API_KEY=***") { + t.Errorf("expected redacted sensitive env, got: %s", out) + } + // benign env (no sensitive keyword) stays readable for debugging. + if !contains(out, "ANTHROPIC_BASE_URL=https://inference.local") { + t.Errorf("benign env should not be redacted, got: %s", out) + } +} + +func TestCmdEnvKeyOnly(t *testing.T) { + // --env KEY (no =VALUE) passes through unchanged. + out := captureCmd("openshell", "sandbox", "create", "--env", "ANTHROPIC_API_KEY") + if !contains(out, "ANTHROPIC_API_KEY") { + t.Errorf("env key should be preserved: %s", out) + } +} + func TestCmdCredentialKeyOnly(t *testing.T) { // --credential KEY (no =VALUE) should pass through as-is out := captureCmd("openshell", "provider", "create", "github", "--credential", "GITHUB_TOKEN") From e0362209cd27d60f7fba3c3d5ddc97be049ef89c Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 15:20:19 -0700 Subject: [PATCH 8/9] fix(agent): validate entrypoint is shell-safe before bash -lc wrapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two CodeRabbit findings in the new adapter command construction: - buildCommand did strings.Fields(entrypoint)[0], which panics on a whitespace-only entrypoint. EffectiveEntrypoint now trims (whitespace-only -> default "claude"), and buildCommand returns an error on an empty entrypoint instead of indexing an empty slice. - The entrypoint is embedded in a bash -lc script, so shell metacharacters in a custom entrypoint would be interpreted. buildCommand now rejects any entrypoint that isn't shell-safe (^[A-Za-z0-9._/@:=+,\- ]+$ — command path plus flag args allowed; ; | & $ ` < > ( ) etc. rejected). The bash -lc wrapper is kept: it is load-bearing for the PATH prepend + command -v check + exec and was validated live at S6; a structured-argv rebuild would undo it for no benefit. AgentAdapter.Command gains an error return; cmd/executor.go surfaces it before sandbox create. Entrypoint is operator-controlled config, so this is defense-in-depth (invariant 30), not a privilege boundary. --- cmd/executor.go | 6 +- internal/agent/adapter.go | 41 ++++++++++--- internal/agent/adapter_test.go | 103 +++++++++++++++++++++++++++++---- internal/agent/agent.go | 2 +- 4 files changed, 132 insertions(+), 20 deletions(-) diff --git a/cmd/executor.go b/cmd/executor.go index 186a56a..49cfd63 100644 --- a/cmd/executor.go +++ b/cmd/executor.go @@ -161,7 +161,11 @@ func upLocal(opts upLocalOpts) error { if noTTY && agentCfg.Task == "" { sandboxCmd = []string{"true"} } else { - sandboxCmd = agent.AdapterFor(agentCfg.EffectiveEntrypoint()).Command(agentCfg, taskPath) + cmd, cmdErr := agent.AdapterFor(agentCfg.EffectiveEntrypoint()).Command(agentCfg, taskPath) + if cmdErr != nil { + return cmdErr + } + sandboxCmd = cmd } // Stage the rendered payload for upload (--upload lands it at diff --git a/internal/agent/adapter.go b/internal/agent/adapter.go index fb19a0c..bd16b2d 100644 --- a/internal/agent/adapter.go +++ b/internal/agent/adapter.go @@ -1,6 +1,8 @@ package agent import ( + "fmt" + "regexp" "strings" ) @@ -25,7 +27,10 @@ type AgentAdapter interface { // - For tasks with headless mode: uses --print (claude/codex) or run (opencode) // - For tasks with interactive mode: uses -p // - For no task: just the entrypoint - Command(cfg *AgentConfig, taskPath string) []string + // It returns an error when the configured entrypoint is empty or contains + // shell metacharacters: the command is wrapped in bash -lc, so an unsafe + // entrypoint would otherwise be interpreted by the shell. + Command(cfg *AgentConfig, taskPath string) ([]string, error) } // AdapterFor returns the appropriate AgentAdapter for the given entrypoint. @@ -51,7 +56,7 @@ func (a *claudeAdapter) Environment(cfg *AgentConfig) map[string]string { return make(map[string]string) } -func (a *claudeAdapter) Command(cfg *AgentConfig, taskPath string) []string { +func (a *claudeAdapter) Command(cfg *AgentConfig, taskPath string) ([]string, error) { return buildCommand("claude", cfg, taskPath) } @@ -62,7 +67,7 @@ func (a *codexAdapter) Environment(cfg *AgentConfig) map[string]string { return make(map[string]string) } -func (a *codexAdapter) Command(cfg *AgentConfig, taskPath string) []string { +func (a *codexAdapter) Command(cfg *AgentConfig, taskPath string) ([]string, error) { return buildCommand("codex", cfg, taskPath) } @@ -73,7 +78,7 @@ func (a *opencodeAdapter) Environment(cfg *AgentConfig) map[string]string { return make(map[string]string) } -func (a *opencodeAdapter) Command(cfg *AgentConfig, taskPath string) []string { +func (a *opencodeAdapter) Command(cfg *AgentConfig, taskPath string) ([]string, error) { return buildCommand("opencode", cfg, taskPath) } @@ -84,21 +89,41 @@ func (a *customAdapter) Environment(cfg *AgentConfig) map[string]string { return make(map[string]string) } -func (a *customAdapter) Command(cfg *AgentConfig, taskPath string) []string { +func (a *customAdapter) Command(cfg *AgentConfig, taskPath string) ([]string, error) { entrypoint := cfg.EffectiveEntrypoint() // For custom entrypoints, treat them as the base agent type but use their // custom entrypoint instead of a predefined one. return buildCommand(entrypoint, cfg, taskPath) } +// entrypointPattern allows a command (optionally a path) plus flag-style +// arguments: space-separated tokens of letters, digits, and the punctuation a +// command line legitimately needs (./-_@:=+,). It deliberately rejects shell +// metacharacters (; | & $ ` < > ( ) { } [ ] * ? ! " ' \ newline #) because the +// entrypoint is embedded in a bash -lc script — anything the shell would +// interpret must not reach it. +var entrypointPattern = regexp.MustCompile(`^[A-Za-z0-9._/@:=+,\- ]+$`) + // buildCommand constructs the argv for the given base entrypoint (could be // "claude", "codex", "opencode", or a custom entrypoint). It handles: // - PATH prepending to /sandbox/.config/openshell/bin // - Task dispatch (--print for headless, -p for interactive, none for no task) // - Entrypoint validation via command -v check // - Wrapping in bash -lc for shell setup -func buildCommand(baseEntrypoint string, cfg *AgentConfig, taskPath string) []string { - epBin := strings.Fields(baseEntrypoint)[0] +// +// It returns an error when the entrypoint is empty/whitespace-only or contains +// shell metacharacters (see entrypointPattern) — the entrypoint reaches a shell, +// so it must be shell-safe rather than trusted implicitly. +func buildCommand(baseEntrypoint string, cfg *AgentConfig, taskPath string) ([]string, error) { + baseEntrypoint = strings.TrimSpace(baseEntrypoint) + fields := strings.Fields(baseEntrypoint) + if len(fields) == 0 { + return nil, fmt.Errorf("agent entrypoint is empty") + } + if !entrypointPattern.MatchString(baseEntrypoint) { + return nil, fmt.Errorf("agent entrypoint %q contains unsupported characters; shell metacharacters are not allowed", baseEntrypoint) + } + epBin := fields[0] var cmdBuilder strings.Builder @@ -139,5 +164,5 @@ func buildCommand(baseEntrypoint string, cfg *AgentConfig, taskPath string) []st } } - return []string{"bash", "-lc", cmdBuilder.String()} + return []string{"bash", "-lc", cmdBuilder.String()}, nil } diff --git a/internal/agent/adapter_test.go b/internal/agent/adapter_test.go index 23865ce..c2dfe02 100644 --- a/internal/agent/adapter_test.go +++ b/internal/agent/adapter_test.go @@ -1,6 +1,7 @@ package agent import ( + "strings" "testing" ) @@ -65,7 +66,10 @@ func TestClaudeHeadlessWithTask(t *testing.T) { } adapter := AdapterFor("claude") - cmd := adapter.Command(cfg, SandboxTaskPath) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } expected := []string{ "bash", @@ -88,7 +92,10 @@ func TestClaudeInteractiveWithTask(t *testing.T) { } adapter := AdapterFor("claude") - cmd := adapter.Command(cfg, SandboxTaskPath) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } expected := []string{ "bash", @@ -110,7 +117,10 @@ func TestClaudeNoTask(t *testing.T) { } adapter := AdapterFor("claude") - cmd := adapter.Command(cfg, "") + cmd, err := adapter.Command(cfg, "") + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } expected := []string{ "bash", @@ -133,7 +143,10 @@ func TestClaudeImplicitEntrypoint(t *testing.T) { } adapter := AdapterFor("") // "" dispatches to claude - cmd := adapter.Command(cfg, SandboxTaskPath) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } expected := []string{ "bash", @@ -156,7 +169,10 @@ func TestOpenCodeHeadlessWithTask(t *testing.T) { } adapter := AdapterFor("opencode") - cmd := adapter.Command(cfg, SandboxTaskPath) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } expected := []string{ "bash", @@ -179,7 +195,10 @@ func TestOpenCodeInteractiveWithTask(t *testing.T) { } adapter := AdapterFor("opencode") - cmd := adapter.Command(cfg, SandboxTaskPath) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } expected := []string{ "bash", @@ -202,7 +221,10 @@ func TestCodexHeadlessWithTask(t *testing.T) { } adapter := AdapterFor("codex") - cmd := adapter.Command(cfg, SandboxTaskPath) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } expected := []string{ "bash", @@ -225,7 +247,10 @@ func TestCustomHeadlessWithTask(t *testing.T) { } adapter := AdapterFor("myagent") - cmd := adapter.Command(cfg, SandboxTaskPath) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } expected := []string{ "bash", @@ -248,7 +273,10 @@ func TestCustomWithArgsHeadlessWithTask(t *testing.T) { } adapter := AdapterFor("myagent --model=mini") - cmd := adapter.Command(cfg, SandboxTaskPath) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } expected := []string{ "bash", @@ -271,7 +299,10 @@ func TestTaskPathUsesProvidedConstant(t *testing.T) { } adapter := AdapterFor("claude") - cmd := adapter.Command(cfg, SandboxTaskPath) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error: %v", err) + } // Verify the exact constant is used if len(cmd) != 3 || cmd[0] != "bash" || cmd[1] != "-lc" { @@ -328,6 +359,58 @@ func TestAnthropicBaseURLFromConfig(t *testing.T) { } } +// TestCustomEntrypointRejectsShellMetacharacters verifies a custom entrypoint +// carrying shell metacharacters is rejected rather than embedded in the bash -lc +// script (the entrypoint reaches a shell, so it must be shell-safe). +func TestCustomEntrypointRejectsShellMetacharacters(t *testing.T) { + for _, ep := range []string{ + "claude; rm -rf /", + "claude && curl evil", + "claude | tee /tmp/x", + "claude $(whoami)", + "claude `id`", + "claude > /etc/passwd", + } { + cfg := &AgentConfig{Name: "test", Entrypoint: ep, Task: "task.md", TTY: boolPtr(false)} + adapter := AdapterFor(cfg.EffectiveEntrypoint()) + if _, err := adapter.Command(cfg, SandboxTaskPath); err == nil { + t.Errorf("entrypoint %q: expected error, got nil", ep) + } + } +} + +// TestEntrypointEmptyOrWhitespaceRejected verifies buildCommand does not panic +// on a whitespace-only entrypoint and returns an error instead of indexing an +// empty strings.Fields result. +func TestEntrypointEmptyOrWhitespaceRejected(t *testing.T) { + // A whitespace-only Entrypoint resolves to the default via EffectiveEntrypoint. + cfg := &AgentConfig{Name: "test", Entrypoint: " "} + if got := cfg.EffectiveEntrypoint(); got != "claude" { + t.Errorf("EffectiveEntrypoint() = %q, want claude for whitespace-only", got) + } + // buildCommand itself rejects an empty/whitespace entrypoint without panicking. + if _, err := buildCommand(" ", cfg, ""); err == nil { + t.Error("buildCommand(whitespace) = nil error, want error") + } +} + +// TestCustomEntrypointWithArgsAllowed verifies a legitimate command-plus-flags +// entrypoint (spaces and flag punctuation) is accepted. +func TestCustomEntrypointWithArgsAllowed(t *testing.T) { + cfg := &AgentConfig{Name: "test", Entrypoint: "/usr/local/bin/my-agent --model gpt-4o", Task: "task.md", TTY: boolPtr(false)} + adapter := AdapterFor(cfg.EffectiveEntrypoint()) + cmd, err := adapter.Command(cfg, SandboxTaskPath) + if err != nil { + t.Fatalf("Command() returned error for valid entrypoint: %v", err) + } + if len(cmd) != 3 || !strings.Contains(cmd[2], "command -v /usr/local/bin/my-agent") { + t.Errorf("expected entrypoint bin validated in script, got: %v", cmd) + } + if !strings.Contains(cmd[2], "exec /usr/local/bin/my-agent --model gpt-4o --print") { + t.Errorf("expected full entrypoint with args in exec, got: %s", cmd[2]) + } +} + // Helper functions func boolPtr(b bool) *bool { diff --git a/internal/agent/agent.go b/internal/agent/agent.go index e88739a..9d3729f 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -118,7 +118,7 @@ func (c *AgentConfig) NoTTY() bool { } func (c *AgentConfig) EffectiveEntrypoint() string { - if c.Entrypoint == "" { + if strings.TrimSpace(c.Entrypoint) == "" { return "claude" } return c.Entrypoint From cdef80f909a130d7962c2726b4027af4740b3277 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 15:48:02 -0700 Subject: [PATCH 9/9] fix: wrap sandbox command-construction error with apply-stage context Address CodeRabbit review on PR #102: the adapter Command error was returned bare, without apply-stage context. Wrap it with fmt.Errorf("building sandbox command: %w", ...) to match the file's error-handling convention. --- cmd/executor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/executor.go b/cmd/executor.go index 49cfd63..e9c9c7b 100644 --- a/cmd/executor.go +++ b/cmd/executor.go @@ -163,7 +163,7 @@ func upLocal(opts upLocalOpts) error { } else { cmd, cmdErr := agent.AdapterFor(agentCfg.EffectiveEntrypoint()).Command(agentCfg, taskPath) if cmdErr != nil { - return cmdErr + return fmt.Errorf("building sandbox command: %w", cmdErr) } sandboxCmd = cmd }