From c69024892c46169ff2519854c7e5160fc00a5546 Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Sun, 22 Feb 2026 21:12:34 +0800 Subject: [PATCH] refactor: reimplement sandbox management with a Manager interface for context-aware resolution and enable non-main sandboxing mode. --- pkg/agent/instance.go | 35 +++++------- pkg/agent/loop.go | 17 +++++- pkg/agent/sandbox/container.go | 23 ++++++-- pkg/agent/sandbox/host_test.go | 2 +- pkg/agent/sandbox/manager.go | 84 ++++++++++++++++++++++++---- pkg/agent/sandbox/manager_test.go | 22 +++++--- pkg/agent/sandbox/runtime_context.go | 16 ++++++ pkg/agent/sandbox/sandbox.go | 10 ++++ pkg/agent/sandbox/unavailable.go | 47 ---------------- pkg/tools/base.go | 4 +- pkg/tools/cron.go | 2 +- pkg/tools/cron_additional_test.go | 6 +- pkg/tools/edit.go | 39 +++++++++++-- pkg/tools/filesystem.go | 39 +++---------- pkg/tools/shell.go | 45 +++++++-------- pkg/tools/shell_test.go | 23 +++++--- 16 files changed, 246 insertions(+), 168 deletions(-) delete mode 100644 pkg/agent/sandbox/unavailable.go diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 80b65d20d..00f061e99 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -29,6 +29,7 @@ type AgentInstance struct { Sessions *session.SessionManager ContextBuilder *ContextBuilder Tools *tools.ToolRegistry + SandboxManager sandbox.Manager Subagents *config.SubagentsConfig SkillsFilter []string Candidates []providers.FallbackCandidate @@ -58,40 +59,31 @@ func NewAgentInstance( } restrict := defaults.RestrictToWorkspace - sb := sandbox.NewFromConfigWithAgent(workspace, restrict, cfg, agentID) - isSandboxOff := true - if cfg != nil { - mode := strings.ToLower(strings.TrimSpace(cfg.Agents.Defaults.Sandbox.Mode)) - if mode == "all" || mode == "non-main" { - isSandboxOff = false - } - } roContainer := isContainerReadOnlySandbox(cfg) - toolsRegistry := tools.NewToolRegistry() - // Helper to check if tool is allowed (either sandbox is off or policy allows it) - isAllowed := func(toolName string) bool { - return isSandboxOff || sandbox.IsToolSandboxEnabled(cfg, toolName) + sandboxManager := sandbox.NewFromConfigWithAgent(workspace, restrict, cfg, agentID) + isSandboxAllowed := func(toolName string) bool { + return sandboxManager == nil || sandbox.IsToolSandboxEnabled(cfg, toolName) } - if isAllowed("read_file") { - toolsRegistry.Register(tools.NewReadFileToolWithSandbox(workspace, restrict, sb)) + if isSandboxAllowed("read_file") { + toolsRegistry.Register(tools.NewReadFileTool(workspace, restrict)) } - if !roContainer && isAllowed("write_file") { - toolsRegistry.Register(tools.NewWriteFileToolWithSandbox(workspace, restrict, sb)) + if !roContainer && isSandboxAllowed("write_file") { + toolsRegistry.Register(tools.NewWriteFileTool(workspace, restrict)) } - if isAllowed("list_dir") { + if isSandboxAllowed("list_dir") { toolsRegistry.Register(tools.NewListDirTool(workspace, restrict)) } - if isAllowed("exec") { - toolsRegistry.Register(tools.NewExecToolWithSandbox(workspace, restrict, cfg, sb)) + if isSandboxAllowed("exec") { + toolsRegistry.Register(tools.NewExecToolWithConfig(workspace, restrict, cfg)) } if !roContainer { - if isAllowed("edit_file") { + if isSandboxAllowed("edit_file") { toolsRegistry.Register(tools.NewEditFileTool(workspace, restrict)) } - if isAllowed("append_file") { + if isSandboxAllowed("append_file") { toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict)) } } @@ -138,6 +130,7 @@ func NewAgentInstance( Sessions: sessionsManager, ContextBuilder: contextBuilder, Tools: toolsRegistry, + SandboxManager: sandboxManager, Subagents: subagents, SkillsFilter: skillsFilter, Candidates: candidates, diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index b062d9bd1..a99b97722 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -400,9 +400,24 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt } } - // 1. Update tool contexts + // 1. Prepare tool contexts and sandbox al.updateToolContexts(agent, opts.Channel, opts.ChatID) + // Inject the routing session key so sandbox.shouldSandbox() can compare + // it against the main session key for non-main mode. + ctx = sandbox.WithSessionKey(ctx, opts.SessionKey) + + // Resolve sandbox environment for this run + if agent.SandboxManager != nil { + sb, err := agent.SandboxManager.Resolve(ctx) + if err != nil { + logger.ErrorCF("agent", "Failed to resolve sandbox", map[string]any{"error": err.Error()}) + return "", fmt.Errorf("failed to resolve sandbox: %w", err) + } + // Add sandbox to context for thread-safe access in tools via registry + ctx = sandbox.WithSandbox(ctx, sb) + } + // 2. Build messages (skip history for heartbeat) var history []providers.Message var summary string diff --git a/pkg/agent/sandbox/container.go b/pkg/agent/sandbox/container.go index 300f8fe25..442b62ab0 100644 --- a/pkg/agent/sandbox/container.go +++ b/pkg/agent/sandbox/container.go @@ -14,6 +14,7 @@ import ( "sort" "strconv" "strings" + "sync" "time" "github.com/docker/docker/api/types/container" @@ -58,6 +59,7 @@ type ContainerSandboxConfig struct { // ContainerSandbox executes commands and filesystem operations inside a managed docker container. type ContainerSandbox struct { + mu sync.Mutex cfg ContainerSandboxConfig cli *client.Client startErr error @@ -70,10 +72,10 @@ const defaultSandboxRegistryFile = "containers.json" // NewContainerSandbox creates a container sandbox with normalized defaults and precomputed config hash. func NewContainerSandbox(cfg ContainerSandboxConfig) *ContainerSandbox { if strings.TrimSpace(cfg.Image) == "" { - cfg.Image = "openclaw-sandbox:bookworm-slim" + cfg.Image = "debian:bookworm-slim" } if strings.TrimSpace(cfg.ContainerPrefix) == "" { - cfg.ContainerPrefix = "picoclaw-sbx-" + cfg.ContainerPrefix = "picoclaw-sandbox-" } if strings.TrimSpace(cfg.ContainerName) == "" { cfg.ContainerName = cfg.ContainerPrefix + "default" @@ -103,11 +105,20 @@ func NewContainerSandbox(cfg ContainerSandboxConfig) *ContainerSandbox { // Start initializes docker connectivity and validates sandbox runtime requirements. func (c *ContainerSandbox) Start(ctx context.Context) error { + c.mu.Lock() + defer c.mu.Unlock() + + if c.startErr != nil { + return c.startErr + } + if c.cli != nil { + return nil + } + if err := validateSandboxSecurity(c.cfg); err != nil { c.startErr = err return err } - c.cfg.Env = sanitizeEnvVars(c.cfg.Env) if strings.TrimSpace(c.cfg.Workspace) != "" && c.cfg.WorkspaceAccess == "none" { if err := os.MkdirAll(c.cfg.Workspace, 0o755); err != nil { c.startErr = fmt.Errorf("sandbox workspace init failed: %w", err) @@ -145,10 +156,14 @@ func (c *ContainerSandbox) Start(ctx context.Context) error { _, _ = io.Copy(io.Discard, rc) } - c.startErr = nil return nil } +// Resolve returns the container sandbox itself. +func (c *ContainerSandbox) Resolve(ctx context.Context) (Sandbox, error) { + return c, nil +} + // Prune reclaims container sandbox resources. // This is the container-specific cleanup boundary where implementations should // stop and remove this sandbox container. diff --git a/pkg/agent/sandbox/host_test.go b/pkg/agent/sandbox/host_test.go index 8a78ea040..e530627bd 100644 --- a/pkg/agent/sandbox/host_test.go +++ b/pkg/agent/sandbox/host_test.go @@ -104,7 +104,7 @@ func TestHostSandbox_ResolvePathRestrictions(t *testing.T) { } func TestUnavailableSandboxAndUtilHelpers(t *testing.T) { - sb := NewUnavailableSandbox(nil) + sb := NewUnavailableSandboxManager(nil) if err := sb.Start(context.Background()); err == nil { t.Fatal("expected Start() error") } diff --git a/pkg/agent/sandbox/manager.go b/pkg/agent/sandbox/manager.go index bff0c354f..2cabdaa58 100644 --- a/pkg/agent/sandbox/manager.go +++ b/pkg/agent/sandbox/manager.go @@ -4,6 +4,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "errors" "fmt" "os" "path/filepath" @@ -16,13 +17,17 @@ import ( "github.com/sipeed/picoclaw/pkg/routing" ) -// NewFromConfig builds a sandbox instance from config and starts it before returning. +// NewFromConfig builds a host sandbox from config for host-level execution (e.g. cron jobs). +// It does not return a Manager; use NewFromConfigWithAgent when sandbox routing is needed. func NewFromConfig(workspace string, restrict bool, cfg *config.Config) Sandbox { - return NewFromConfigWithAgent(workspace, restrict, cfg, routing.DefaultAgentID) + host := NewHostSandbox(workspace, restrict) + _ = host.Start(context.Background()) + return host } -// NewFromConfigWithAgent builds a sandbox instance with an explicit agent ID context. -func NewFromConfigWithAgent(workspace string, restrict bool, cfg *config.Config, agentID string) Sandbox { +// NewFromConfigWithAgent builds the sandbox Manager for an agent. +// Returns nil when sandboxing is disabled (mode=off), so callers can check manager != nil. +func NewFromConfigWithAgent(workspace string, restrict bool, cfg *config.Config, agentID string) Manager { mode := "all" scope := "agent" workspaceAccess := "none" @@ -63,13 +68,15 @@ func NewFromConfigWithAgent(workspace string, restrict bool, cfg *config.Config, } agentID = routing.NormalizeAgentID(agentID) - host := NewHostSandbox(workspace, restrict) - _ = host.Start(context.Background()) resolvedMode := normalizeSandboxMode(mode) if resolvedMode == "off" { - return host + return nil // sandbox disabled; host-level access is handled directly by tools } + + host := NewHostSandbox(workspace, restrict) + _ = host.Start(context.Background()) + resolvedScope := normalizeSandboxScope(scope) normalizedAccess := normalizeWorkspaceAccess(workspaceAccess) workspaceRootAbs := resolveAbsPath(expandHomePath(workspaceRoot)) @@ -92,7 +99,7 @@ func NewFromConfigWithAgent(workspace string, restrict bool, cfg *config.Config, } manager.fs = &managerFS{m: manager} if err := manager.Start(context.Background()); err != nil { - return NewUnavailableSandbox(fmt.Errorf("container sandbox unavailable: %w", err)) + return NewUnavailableSandboxManager(fmt.Errorf("container sandbox unavailable: %w", err)) } return manager } @@ -340,15 +347,22 @@ func (m *scopedSandboxManager) Fs() FsBridge { return m.fs } +// Resolve returns the specific sandbox instance to be used for the given context. +func (m *scopedSandboxManager) Resolve(ctx context.Context) (Sandbox, error) { + if !m.shouldSandbox(ctx) { + return m.host, nil + } + return m.getOrCreateSandbox(ctx, m.scopeKeyFromContext(ctx)) +} + func (m *scopedSandboxManager) shouldSandbox(ctx context.Context) bool { switch m.mode { case "all": return true case "non-main": - // Phase 2 deferred: `non-main` requires stable session-key propagation - // across all tool execution paths. For now, keep behavior disabled until - // the execution context plumbing is finalized. - return false + // Sandbox all sessions except the agent's main session. + // Normalize before comparing to handle aliases like "main" or bare agent keys + return m.normalizeSessionKey(SessionKeyFromContext(ctx)) != m.mainSessionKey() default: return false } @@ -493,3 +507,49 @@ func slugScopeKey(scopeKey string) string { sum := sha256.Sum256([]byte(raw)) return safe + "-" + hex.EncodeToString(sum[:4]) } + +type unavailableSandboxManager struct { + err error + fs FsBridge +} + +func NewUnavailableSandboxManager(err error) Manager { + if err == nil { + err = errors.New("sandbox unavailable") + } + return &unavailableSandboxManager{ + err: err, + fs: &errorFS{err: err}, + } +} + +func (u *unavailableSandboxManager) Start(ctx context.Context) error { return u.err } +func (u *unavailableSandboxManager) Prune(ctx context.Context) error { return nil } + +// Resolve returns an error because the sandbox is unavailable. +func (u *unavailableSandboxManager) Resolve(ctx context.Context) (Sandbox, error) { + return nil, u.err +} + +func (u *unavailableSandboxManager) Fs() FsBridge { return u.fs } +func (u *unavailableSandboxManager) Exec(ctx context.Context, req ExecRequest) (*ExecResult, error) { + return aggregateExecStream(func(onEvent func(ExecEvent) error) (*ExecResult, error) { + return u.ExecStream(ctx, req, onEvent) + }) +} + +func (u *unavailableSandboxManager) ExecStream(ctx context.Context, req ExecRequest, onEvent func(ExecEvent) error) (*ExecResult, error) { + return nil, u.err +} + +type errorFS struct { + err error +} + +func (e *errorFS) ReadFile(ctx context.Context, path string) ([]byte, error) { + return nil, fmt.Errorf("sandbox unavailable: %w", e.err) +} + +func (e *errorFS) WriteFile(ctx context.Context, path string, data []byte, mkdir bool) error { + return fmt.Errorf("sandbox unavailable: %w", e.err) +} diff --git a/pkg/agent/sandbox/manager_test.go b/pkg/agent/sandbox/manager_test.go index bb96ba2b5..4ef0fc465 100644 --- a/pkg/agent/sandbox/manager_test.go +++ b/pkg/agent/sandbox/manager_test.go @@ -38,10 +38,8 @@ func TestExpandHomePath(t *testing.T) { } func TestNewFromConfig_HostMode(t *testing.T) { - cfg := config.DefaultConfig() - cfg.Agents.Defaults.Sandbox.Mode = "off" - - sb := NewFromConfig(t.TempDir(), true, cfg) + // NewFromConfig always returns a HostSandbox regardless of mode config. + sb := NewFromConfig(t.TempDir(), true, nil) if _, ok := sb.(*HostSandbox); !ok { t.Fatalf("expected HostSandbox, got %T", sb) } @@ -57,12 +55,18 @@ func TestNewFromConfig_AllModeReturnsUnavailableWhenBlocked(t *testing.T) { cfg.Agents.Defaults.Sandbox.Prune.IdleHours = 0 cfg.Agents.Defaults.Sandbox.Prune.MaxAgeDays = 0 - sb := NewFromConfig(t.TempDir(), true, cfg) - if _, ok := sb.(*unavailableSandbox); !ok { - t.Fatalf("expected unavailableSandbox, got %T", sb) + // NewFromConfigWithAgent is the manager factory; when Docker is unavailable + // it should return an unavailableSandbox that implements Manager. + mgr := NewFromConfigWithAgent(t.TempDir(), true, cfg, "test") + if mgr == nil { + t.Fatal("expected non-nil Manager when mode=all") } - if err := sb.Start(context.Background()); err == nil { - t.Fatal("expected unavailable sandbox start error") + if _, ok := mgr.(*unavailableSandboxManager); !ok { + t.Fatalf("expected unavailableSandbox, got %T", mgr) + } + // Resolve() must propagate the unavailability error. + if _, err := mgr.Resolve(context.Background()); err == nil { + t.Fatal("expected unavailable sandbox Resolve() to return error") } } diff --git a/pkg/agent/sandbox/runtime_context.go b/pkg/agent/sandbox/runtime_context.go index 6312a12e8..c163aff5a 100644 --- a/pkg/agent/sandbox/runtime_context.go +++ b/pkg/agent/sandbox/runtime_context.go @@ -17,3 +17,19 @@ func SessionKeyFromContext(ctx context.Context) string { v, _ := ctx.Value(sessionContextKey{}).(string) return v } + +type sandboxContextKey struct{} + +// WithSandbox returns a derived context carrying the current sandbox instance. +func WithSandbox(ctx context.Context, sb Sandbox) context.Context { + return context.WithValue(ctx, sandboxContextKey{}, sb) +} + +// SandboxFromContext returns the sandbox instance attached by WithSandbox. +func SandboxFromContext(ctx context.Context) Sandbox { + if ctx == nil { + return nil + } + v, _ := ctx.Value(sandboxContextKey{}).(Sandbox) + return v +} diff --git a/pkg/agent/sandbox/sandbox.go b/pkg/agent/sandbox/sandbox.go index 7d5cbbc3f..d8076987e 100644 --- a/pkg/agent/sandbox/sandbox.go +++ b/pkg/agent/sandbox/sandbox.go @@ -28,6 +28,16 @@ type Sandbox interface { Fs() FsBridge } +// Manager is implemented by the scoped sandbox manager. +// It resolves the specific Sandbox instance to use for the current execution context +// (e.g. based on session key / scope). Only the manager needs this; leaf sandboxes +// (HostSandbox, ContainerSandbox) implement Sandbox directly and are the resolved result. +type Manager interface { + Sandbox + // Resolve returns the concrete Sandbox instance to use for the given context. + Resolve(ctx context.Context) (Sandbox, error) +} + // ExecRequest describes a command execution request for Sandbox.Exec. type ExecRequest struct { // Command is the program or shell command to execute. diff --git a/pkg/agent/sandbox/unavailable.go b/pkg/agent/sandbox/unavailable.go deleted file mode 100644 index 4465c3a57..000000000 --- a/pkg/agent/sandbox/unavailable.go +++ /dev/null @@ -1,47 +0,0 @@ -package sandbox - -import ( - "context" - "errors" - "fmt" -) - -type unavailableSandbox struct { - err error - fs FsBridge -} - -func NewUnavailableSandbox(err error) Sandbox { - if err == nil { - err = errors.New("sandbox unavailable") - } - return &unavailableSandbox{ - err: err, - fs: &errorFS{err: err}, - } -} - -func (u *unavailableSandbox) Start(ctx context.Context) error { return u.err } -func (u *unavailableSandbox) Prune(ctx context.Context) error { return nil } -func (u *unavailableSandbox) Fs() FsBridge { return u.fs } -func (u *unavailableSandbox) Exec(ctx context.Context, req ExecRequest) (*ExecResult, error) { - return aggregateExecStream(func(onEvent func(ExecEvent) error) (*ExecResult, error) { - return u.ExecStream(ctx, req, onEvent) - }) -} - -func (u *unavailableSandbox) ExecStream(ctx context.Context, req ExecRequest, onEvent func(ExecEvent) error) (*ExecResult, error) { - return nil, u.err -} - -type errorFS struct { - err error -} - -func (e *errorFS) ReadFile(ctx context.Context, path string) ([]byte, error) { - return nil, fmt.Errorf("sandbox unavailable: %w", e.err) -} - -func (e *errorFS) WriteFile(ctx context.Context, path string, data []byte, mkdir bool) error { - return fmt.Errorf("sandbox unavailable: %w", e.err) -} diff --git a/pkg/tools/base.go b/pkg/tools/base.go index 770d8cb04..d9c940b3f 100644 --- a/pkg/tools/base.go +++ b/pkg/tools/base.go @@ -1,6 +1,8 @@ package tools -import "context" +import ( + "context" +) // Tool is the interface that all tools must implement. type Tool interface { diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index a8a5a9ea8..c910e4a48 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -35,7 +35,7 @@ type CronTool struct { // execTimeout: 0 means no timeout, >0 sets the timeout duration. func NewCronTool(cronService *cron.CronService, executor JobExecutor, msgBus *bus.MessageBus, workspace string, restrict bool, execTimeout time.Duration, config *config.Config) *CronTool { sb := sandbox.NewFromConfig(workspace, restrict, config) - guard := NewExecToolWithSandbox(workspace, restrict, config, nil) + guard := NewExecToolWithConfig(workspace, restrict, config) return &CronTool{ cronService: cronService, executor: executor, diff --git a/pkg/tools/cron_additional_test.go b/pkg/tools/cron_additional_test.go index f502a9433..fe19f8a1b 100644 --- a/pkg/tools/cron_additional_test.go +++ b/pkg/tools/cron_additional_test.go @@ -20,7 +20,11 @@ type cronStubSandbox struct { func (s *cronStubSandbox) Start(ctx context.Context) error { return nil } func (s *cronStubSandbox) Prune(ctx context.Context) error { return nil } -func (s *cronStubSandbox) Fs() sandbox.FsBridge { return nil } + +func (s *cronStubSandbox) Resolve(ctx context.Context) (sandbox.Sandbox, error) { + return s, nil +} +func (s *cronStubSandbox) Fs() sandbox.FsBridge { return nil } func (s *cronStubSandbox) Exec(ctx context.Context, req sandbox.ExecRequest) (*sandbox.ExecResult, error) { return s.ExecStream(ctx, req, nil) } diff --git a/pkg/tools/edit.go b/pkg/tools/edit.go index c28ca6ca2..68e6fdb5f 100644 --- a/pkg/tools/edit.go +++ b/pkg/tools/edit.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "strings" + + "github.com/sipeed/picoclaw/pkg/agent/sandbox" ) // EditFileTool edits a file by replacing old_text with new_text. @@ -72,12 +74,17 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe return ErrorResult(err.Error()) } - if _, err = os.Stat(resolvedPath); os.IsNotExist(err) { - return ErrorResult(fmt.Sprintf("file not found: %s", path)) + var content []byte + sb := sandbox.SandboxFromContext(ctx) + if sb != nil { + content, err = sb.Fs().ReadFile(ctx, path) + } else { + content, err = os.ReadFile(resolvedPath) } - - content, err := os.ReadFile(resolvedPath) if err != nil { + if os.IsNotExist(err) { + return ErrorResult(fmt.Sprintf("file not found: %s", path)) + } return ErrorResult(fmt.Sprintf("failed to read file: %v", err)) } @@ -96,8 +103,14 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe newContent := strings.Replace(contentStr, oldText, newText, 1) - if err := os.WriteFile(resolvedPath, []byte(newContent), 0o644); err != nil { - return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) + if sb != nil { + if err := sb.Fs().WriteFile(ctx, path, []byte(newContent), true); err != nil { + return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) + } + } else { + if err := os.WriteFile(resolvedPath, []byte(newContent), 0o644); err != nil { + return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) + } } return SilentResult(fmt.Sprintf("File edited: %s", path)) @@ -153,6 +166,20 @@ func (t *AppendFileTool) Execute(ctx context.Context, args map[string]any) *Tool return ErrorResult(err.Error()) } + sb := sandbox.SandboxFromContext(ctx) + if sb != nil { + // Implement Append using Read + Write if no Append in FsBridge + oldContent, err := sb.Fs().ReadFile(ctx, path) + if err != nil && !strings.Contains(err.Error(), "no such file") { + return ErrorResult(fmt.Sprintf("failed to read file for append: %v", err)) + } + newContent := string(oldContent) + content + if err := sb.Fs().WriteFile(ctx, path, []byte(newContent), true); err != nil { + return ErrorResult(fmt.Sprintf("failed to append (write) to file: %v", err)) + } + return SilentResult(fmt.Sprintf("Appended to %s", path)) + } + f, err := os.OpenFile(resolvedPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) if err != nil { return ErrorResult(fmt.Sprintf("failed to open file: %v", err)) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index e80efbe02..7f5ab83eb 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -84,25 +84,12 @@ func isWithinWorkspace(candidate, workspace string) bool { type ReadFileTool struct { workspace string restrict bool - fsBridge sandbox.FsBridge } func NewReadFileTool(workspace string, restrict bool) *ReadFileTool { return &ReadFileTool{workspace: workspace, restrict: restrict} } -func NewReadFileToolWithSandbox(workspace string, restrict bool, sb sandbox.Sandbox) *ReadFileTool { - var fsBridge sandbox.FsBridge - if sb != nil { - fsBridge = sb.Fs() - } - return &ReadFileTool{workspace: workspace, restrict: restrict, fsBridge: fsBridge} -} - -func NewReadFileToolWithFsBridge(workspace string, restrict bool, fsBridge sandbox.FsBridge) *ReadFileTool { - return &ReadFileTool{workspace: workspace, restrict: restrict, fsBridge: fsBridge} -} - func (t *ReadFileTool) Name() string { return "read_file" } @@ -134,8 +121,9 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe content []byte err error ) - if t.fsBridge != nil { - content, err = t.fsBridge.ReadFile(ctx, path) + sb := sandbox.SandboxFromContext(ctx) + if sb != nil { + content, err = sb.Fs().ReadFile(ctx, path) } else { var resolvedPath string resolvedPath, err = validatePath(path, t.workspace, t.restrict) @@ -153,25 +141,12 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe type WriteFileTool struct { workspace string restrict bool - fsBridge sandbox.FsBridge } func NewWriteFileTool(workspace string, restrict bool) *WriteFileTool { return &WriteFileTool{workspace: workspace, restrict: restrict} } -func NewWriteFileToolWithSandbox(workspace string, restrict bool, sb sandbox.Sandbox) *WriteFileTool { - var fsBridge sandbox.FsBridge - if sb != nil { - fsBridge = sb.Fs() - } - return &WriteFileTool{workspace: workspace, restrict: restrict, fsBridge: fsBridge} -} - -func NewWriteFileToolWithFsBridge(workspace string, restrict bool, fsBridge sandbox.FsBridge) *WriteFileTool { - return &WriteFileTool{workspace: workspace, restrict: restrict, fsBridge: fsBridge} -} - func (t *WriteFileTool) Name() string { return "write_file" } @@ -208,8 +183,9 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *ToolR return ErrorResult("content is required") } - if t.fsBridge != nil { - if err := t.fsBridge.WriteFile(ctx, path, []byte(content), true); err != nil { + sb := sandbox.SandboxFromContext(ctx) + if sb != nil { + if err := sb.Fs().WriteFile(ctx, path, []byte(content), true); err != nil { return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) } return SilentResult(fmt.Sprintf("File written: %s", path)) @@ -232,6 +208,8 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *ToolR return SilentResult(fmt.Sprintf("File written: %s", path)) } +// ListDirTool lists files and directories at a given path. +// Phase 1: host-only execution; sandbox routing is deferred to Phase 2 (see sandbox.md ยง6.2). type ListDirTool struct { workspace string restrict bool @@ -274,6 +252,7 @@ func (t *ListDirTool) Execute(ctx context.Context, args map[string]any) *ToolRes } entries, err := os.ReadDir(resolvedPath) + if err != nil { return ErrorResult(fmt.Sprintf("failed to read directory: %v", err)) } diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 1efae07d4..317fd4e06 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -24,7 +24,6 @@ type ExecTool struct { denyPatterns []*regexp.Regexp allowPatterns []*regexp.Regexp restrictToWorkspace bool - sandbox sandbox.Sandbox } var defaultDenyPatterns = []*regexp.Regexp{ @@ -77,10 +76,6 @@ func NewExecTool(workingDir string, restrict bool) *ExecTool { } func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Config) *ExecTool { - return NewExecToolWithSandbox(workingDir, restrict, config, nil) -} - -func NewExecToolWithSandbox(workingDir string, restrict bool, config *config.Config, sb sandbox.Sandbox) *ExecTool { denyPatterns := make([]*regexp.Regexp, 0) enableDenyPatterns := true @@ -115,7 +110,6 @@ func NewExecToolWithSandbox(workingDir string, restrict bool, config *config.Con denyPatterns: denyPatterns, allowPatterns: nil, restrictToWorkspace: restrict, - sandbox: sb, } } @@ -150,14 +144,18 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult return ErrorResult("command is required") } + wd, _ := args["working_dir"].(string) + + // Resolve the working directory cwd := t.workingDir - if wd, ok := args["working_dir"].(string); ok && wd != "" { + if wd != "" { if t.restrictToWorkspace && t.workingDir != "" { resolvedWD, err := validatePath(wd, t.workingDir, true) if err != nil { // In sandbox mode, allow explicit container workspace paths when // restrict_to_workspace is enabled. - if t.sandbox != nil && filepath.IsAbs(wd) && isSandboxWorkspaceAbsolutePath(wd) { + sb := sandbox.SandboxFromContext(ctx) + if sb != nil && filepath.IsAbs(wd) && isSandboxWorkspaceAbsolutePath(wd) { cwd = wd } else { return ErrorResult("Command blocked by safety guard (" + err.Error() + ")") @@ -171,9 +169,8 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult } if cwd == "" { - wd, err := os.Getwd() - if err == nil { - cwd = wd + if dir, err := os.Getwd(); err == nil { + cwd = dir } } @@ -181,9 +178,10 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult return ErrorResult(guardError) } - if t.sandbox != nil { + sb := sandbox.SandboxFromContext(ctx) + if sb != nil { sandboxWD := t.resolveSandboxWorkingDir(cwd) - res, err := t.sandbox.Exec(ctx, sandbox.ExecRequest{ + res, err := sb.Exec(ctx, sandbox.ExecRequest{ Command: command, WorkingDir: sandboxWD, TimeoutMs: t.timeout.Milliseconds(), @@ -252,7 +250,7 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult select { case err = <-done: case <-cmdCtx.Done(): - _ = terminateProcessTree(cmd) + terminateProcessTree(cmd) select { case err = <-done: case <-time.After(2 * time.Second): @@ -270,14 +268,19 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult if err != nil { if errors.Is(cmdCtx.Err(), context.DeadlineExceeded) { - msg := fmt.Sprintf("Command timed out after %v", t.timeout) + msg := fmt.Sprintf("command timed out after %v", t.timeout) return &ToolResult{ ForLLM: msg, ForUser: msg, IsError: true, } } - output += fmt.Sprintf("\nExit code: %v", err) + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + output += fmt.Sprintf("\nExit code: %d", exitErr.ExitCode()) + } else { + output += fmt.Sprintf("\nError: %v", err) + } } if output == "" { @@ -289,18 +292,10 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult output = output[:maxLen] + fmt.Sprintf("\n... (truncated, %d more chars)", len(output)-maxLen) } - if err != nil { - return &ToolResult{ - ForLLM: output, - ForUser: output, - IsError: true, - } - } - return &ToolResult{ ForLLM: output, ForUser: output, - IsError: false, + IsError: err != nil, } } diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 2be8f21a4..693275438 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -20,7 +20,11 @@ type stubSandbox struct { func (s *stubSandbox) Start(ctx context.Context) error { return nil } func (s *stubSandbox) Prune(ctx context.Context) error { return nil } -func (s *stubSandbox) Fs() sandbox.FsBridge { return nil } + +func (s *stubSandbox) Resolve(ctx context.Context) (sandbox.Sandbox, error) { + return s, nil +} +func (s *stubSandbox) Fs() sandbox.FsBridge { return nil } func (s *stubSandbox) Exec(ctx context.Context, req sandbox.ExecRequest) (*sandbox.ExecResult, error) { return sandboxAggregateFromStub(ctx, req, s.ExecStream) } @@ -354,9 +358,9 @@ func TestShellTool_RestrictToWorkspace(t *testing.T) { func TestShellTool_SandboxMapsHostWorkingDirToRelative(t *testing.T) { workspace := t.TempDir() sb := &stubSandbox{} - tool := NewExecToolWithSandbox(workspace, true, nil, sb) + tool := NewExecTool(workspace, true) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]interface{}{ "command": "echo test", "working_dir": filepath.Join(workspace, "subdir"), @@ -373,9 +377,9 @@ func TestShellTool_SandboxMapsHostWorkingDirToRelative(t *testing.T) { func TestShellTool_SandboxAllowsAbsoluteWorkspaceWorkingDir(t *testing.T) { workspace := t.TempDir() sb := &stubSandbox{} - tool := NewExecToolWithSandbox(workspace, true, nil, sb) + tool := NewExecTool(workspace, true) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]interface{}{ "command": "echo test", "working_dir": "/workspace/subdir", @@ -392,9 +396,9 @@ func TestShellTool_SandboxAllowsAbsoluteWorkspaceWorkingDir(t *testing.T) { func TestShellTool_SandboxBlocksAbsoluteNonWorkspaceWorkingDirWhenRestricted(t *testing.T) { workspace := t.TempDir() sb := &stubSandbox{} - tool := NewExecToolWithSandbox(workspace, true, nil, sb) + tool := NewExecTool(workspace, true) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]interface{}{ "command": "echo test", "working_dir": "/tmp/logs", @@ -411,9 +415,10 @@ func TestShellTool_SandboxBlocksAbsoluteNonWorkspaceWorkingDirWhenRestricted(t * func TestShellTool_SandboxExecError(t *testing.T) { workspace := t.TempDir() sb := &stubSandbox{err: fmt.Errorf("sandbox down")} - tool := NewExecToolWithSandbox(workspace, true, nil, sb) + tool := NewExecTool(workspace, true) - result := tool.Execute(context.Background(), map[string]interface{}{"command": "echo test"}) + ctx := sandbox.WithSandbox(context.Background(), sb) + result := tool.Execute(ctx, map[string]interface{}{"command": "echo test"}) if !result.IsError { t.Fatal("expected sandbox error result") }