From a75ff2cf8200273c2059d9193542bb289a197873 Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Fri, 27 Feb 2026 13:11:14 +0800 Subject: [PATCH] refactor(sandbox): Unify host process management and refine tool enablement logic within the sandbox. --- pkg/agent/instance.go | 35 +- pkg/agent/instance_test.go | 33 ++ pkg/agent/loop.go | 19 +- pkg/agent/sandbox/container.go | 110 ++++++- .../sandbox/container_integration_test.go | 61 ++++ pkg/agent/sandbox/container_test.go | 44 +++ pkg/agent/sandbox/host.go | 112 +++---- .../sandbox/host_process_unix.go} | 2 +- .../sandbox/host_process_windows.go} | 2 +- pkg/agent/sandbox/host_test.go | 79 ++++- pkg/agent/sandbox/manager.go | 117 ++++--- pkg/agent/sandbox/path.go | 78 +++++ pkg/agent/sandbox/sandbox.go | 40 ++- pkg/config/config.go | 33 +- pkg/config/defaults.go | 6 +- pkg/tools/edit.go | 129 +++----- pkg/tools/edit_test.go | 63 ++-- pkg/tools/filesystem.go | 308 ++---------------- pkg/tools/filesystem_test.go | 186 +++-------- pkg/tools/shell.go | 126 ++----- pkg/tools/shell_test.go | 50 ++- pkg/tools/shell_timeout_unix_test.go | 16 +- 22 files changed, 832 insertions(+), 817 deletions(-) rename pkg/{tools/shell_process_unix.go => agent/sandbox/host_process_unix.go} (97%) rename pkg/{tools/shell_process_windows.go => agent/sandbox/host_process_windows.go} (96%) create mode 100644 pkg/agent/sandbox/path.go diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 023d9ddea..b68fc107e 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -64,27 +64,35 @@ func NewAgentInstance( toolsRegistry := tools.NewToolRegistry() sandboxManager := sandbox.NewFromConfigWithAgent(workspace, restrict, cfg, agentID) - isSandboxAllowed := func(toolName string) bool { - return sandboxManager == nil || sandbox.IsToolSandboxEnabled(cfg, toolName) + // isToolEnabled determines if a base system tool should be registered for the LLM. + // SandboxManager *always* provides a Sandbox context (HostSandbox if mode="off"). + isToolEnabled := func(toolName string) bool { + // If the user explicitly disables the sandbox mode (Mode = "off"), they are opting out of + // safety isolation. We grant the LLM access to all core tools via the HostSandbox. + if isSandboxModeOff(cfg) { + return true + } + // Otherwise, respect the fine-grained allow/deny policy defined in the Sandbox config. + return sandbox.IsToolSandboxEnabled(cfg, toolName) } - if isSandboxAllowed("read_file") { + if isToolEnabled("read_file") { toolsRegistry.Register(tools.NewReadFileTool(workspace, restrict)) } - if !roContainer && isSandboxAllowed("write_file") { + if !roContainer && isToolEnabled("write_file") { toolsRegistry.Register(tools.NewWriteFileTool(workspace, restrict)) } - if isSandboxAllowed("list_dir") { + if isToolEnabled("list_dir") { toolsRegistry.Register(tools.NewListDirTool(workspace, restrict)) } - if isSandboxAllowed("exec") { + if isToolEnabled("exec") { toolsRegistry.Register(tools.NewExecToolWithConfig(workspace, restrict, cfg)) } if !roContainer { - if isSandboxAllowed("edit_file") { + if isToolEnabled("edit_file") { toolsRegistry.Register(tools.NewEditFileTool(workspace, restrict)) } - if isSandboxAllowed("append_file") { + if isToolEnabled("append_file") { toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict)) } } @@ -170,8 +178,15 @@ func isContainerReadOnlySandbox(cfg *config.Config) bool { if cfg == nil { return false } - return strings.EqualFold(strings.TrimSpace(cfg.Agents.Defaults.Sandbox.Mode), "all") && - strings.EqualFold(strings.TrimSpace(cfg.Agents.Defaults.Sandbox.WorkspaceAccess), "ro") + return cfg.Agents.Defaults.Sandbox.Mode == config.SandboxModeAll && + cfg.Agents.Defaults.Sandbox.WorkspaceAccess == config.WorkspaceAccessRO +} + +func isSandboxModeOff(cfg *config.Config) bool { + if cfg == nil { + return false + } + return cfg.Agents.Defaults.Sandbox.Mode == config.SandboxModeOff } func expandHome(path string) string { diff --git a/pkg/agent/instance_test.go b/pkg/agent/instance_test.go index 3b42d3d9e..f4ac24c87 100644 --- a/pkg/agent/instance_test.go +++ b/pkg/agent/instance_test.go @@ -135,3 +135,36 @@ func TestNewAgentInstance_ReadOnlyContainerOmitsWriteTools(t *testing.T) { t.Fatalf("write_file should be absent in ro sandbox, got: %+v", writeRes) } } + +func TestNewAgentInstance_SandboxModeOffRegistersFullToolSet(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-instance-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 1234, + MaxToolIterations: 5, + Sandbox: config.AgentSandboxConfig{ + Mode: "off", + }, + }, + }, + } + // Keep sandbox allowlist narrow to ensure mode=off bypasses sandbox policy gating. + cfg.Tools.Sandbox.Tools.Allow = []string{"exec"} + + provider := &mockProvider{} + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider) + + for _, name := range []string{"read_file", "write_file", "list_dir", "exec", "edit_file", "append_file"} { + if _, ok := agent.Tools.Get(name); !ok { + t.Fatalf("%s should be registered when sandbox.mode=off", name) + } + } +} diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 1350bd2e0..41c25177f 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -409,16 +409,17 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt // 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) + // Resolve sandbox environment for this run. + // We guarantee SandboxManager is non-nil (fallback to host manager) in NewAgentInstance. + ctx = sandbox.WithManager(ctx, agent.SandboxManager) + + 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 pre-resolved sandbox to context for thread-safe access in tools + ctx = sandbox.WithSandbox(ctx, sb) // 2. Build messages (skip history for heartbeat) var history []providers.Message diff --git a/pkg/agent/sandbox/container.go b/pkg/agent/sandbox/container.go index 6abbddf54..da16afabc 100644 --- a/pkg/agent/sandbox/container.go +++ b/pkg/agent/sandbox/container.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "io" + "io/fs" "math" "os" "path" @@ -101,7 +102,7 @@ func NewContainerSandbox(cfg ContainerSandboxConfig) *ContainerSandbox { if cfg.Env == nil { cfg.Env = map[string]string{"LANG": "C.UTF-8"} } - cfg.WorkspaceAccess = normalizeWorkspaceAccess(cfg.WorkspaceAccess) + cfg.WorkspaceAccess = string(normalizeWorkspaceAccess(config.WorkspaceAccess(cfg.WorkspaceAccess))) cfg.WorkspaceRoot = strings.TrimSpace(cfg.WorkspaceRoot) sb := &ContainerSandbox{cfg: cfg} sb.hash = computeContainerConfigHash(cfg) @@ -125,7 +126,7 @@ func (c *ContainerSandbox) Start(ctx context.Context) error { c.startErr = err return err } - if strings.TrimSpace(c.cfg.Workspace) != "" && c.cfg.WorkspaceAccess == "none" { + if strings.TrimSpace(c.cfg.Workspace) != "" && c.cfg.WorkspaceAccess == string(config.WorkspaceAccessNone) { if err := os.MkdirAll(c.cfg.Workspace, 0o755); err != nil { c.startErr = fmt.Errorf("sandbox workspace init failed: %w", err) return c.startErr @@ -255,6 +256,13 @@ func (c *ContainerSandbox) ExecStream( if err != nil { return nil, fmt.Errorf("docker exec attach failed: %w", err) } + + // Prevent stdcopy.StdCopy from blocking indefinitely if the container hangs. + // We force close the hijacked connection when the context times out. + go func() { + <-execCtx.Done() + attach.Close() + }() defer attach.Close() var stdout, stderr bytes.Buffer @@ -404,16 +412,16 @@ func (c *ContainerSandbox) binds() []string { } if hostDir != "" { - if c.cfg.WorkspaceAccess == "none" { + if c.cfg.WorkspaceAccess == string(config.WorkspaceAccessNone) { // Ensure the isolated directory exists on the host so Docker doesn't create it as root _ = os.MkdirAll(hostDir, 0o755) } // Add :Z flag for SELinux (Podman) to label the content with a private unshared label. // This fixes errors like: "crun: getcwd: Operation not permitted: OCI permission denied" - switch c.cfg.WorkspaceAccess { - case "ro": + switch config.WorkspaceAccess(c.cfg.WorkspaceAccess) { + case config.WorkspaceAccessRO: binds = append(binds, fmt.Sprintf("%s:%s:ro,Z", hostDir, c.cfg.Workdir)) - case "rw", "none": + case config.WorkspaceAccessRW, config.WorkspaceAccessNone: binds = append(binds, fmt.Sprintf("%s:%s:rw,Z", hostDir, c.cfg.Workdir)) default: // Default to no mount for unknown access types @@ -574,7 +582,7 @@ func (f *containerFS) ReadFile(ctx context.Context, p string) ([]byte, error) { return content, nil } } - return nil, fmt.Errorf("file not found in container: %s", containerPath) + return nil, fmt.Errorf("file not found in container %s: %w", containerPath, fs.ErrNotExist) } func (f *containerFS) WriteFile(ctx context.Context, p string, data []byte, mkdir bool) error { @@ -630,8 +638,94 @@ func (f *containerFS) WriteFile(ctx context.Context, p string, data []byte, mkdi return nil } +func (f *containerFS) ReadDir(ctx context.Context, p string) ([]os.DirEntry, error) { + if err := f.sb.ensureContainer(ctx); err != nil { + return nil, err + } + containerPath, err := resolveContainerPathWithRoot(f.sb.cfg.Workdir, p) + if err != nil { + return nil, err + } + + rc, _, err := f.sb.cli.CopyFromContainer(ctx, f.sb.cfg.ContainerName, containerPath) + if err != nil { + return nil, fmt.Errorf("docker copy from container failed: %w", err) + } + defer rc.Close() + + var entries []os.DirEntry + tr := tar.NewReader(rc) + entries, err = parseTopLevelDirEntriesFromTar(tr) + if err != nil { + return nil, err + } + return entries, nil +} + +func parseTopLevelDirEntriesFromTar(tr *tar.Reader) ([]os.DirEntry, error) { + var entries []os.DirEntry + seen := make(map[string]struct{}) + rootName := "" + first := true + + for { + hdr, err := tr.Next() + if err == io.EOF { + break + } + if err != nil { + return nil, fmt.Errorf("tar read failed: %w", err) + } + + cleanName := path.Clean(hdr.Name) + if first { + first = false + rootName = cleanName + continue + } + + relName := cleanName + if rootName != "" && rootName != "." { + if relName == rootName { + continue + } + prefix := rootName + "/" + relName = strings.TrimPrefix(relName, prefix) + } + relName = strings.TrimPrefix(relName, "./") + if relName == "" || relName == "." { + continue + } + if strings.Contains(relName, "/") { + // CopyFromContainer is recursive; keep only immediate children. + continue + } + if _, ok := seen[relName]; ok { + continue + } + seen[relName] = struct{}{} + + entries = append(entries, &containerDirEntry{ + name: relName, + info: hdr.FileInfo(), + }) + } + + return entries, nil +} + +type containerDirEntry struct { + name string + info os.FileInfo +} + +func (d *containerDirEntry) Name() string { return d.name } +func (d *containerDirEntry) IsDir() bool { return d.info.IsDir() } +func (d *containerDirEntry) Type() os.FileMode { return d.info.Mode().Type() } +func (d *containerDirEntry) Info() (os.FileInfo, error) { return d.info, nil } + func (c *ContainerSandbox) hostDirForContainerPath(containerDir string) (string, bool) { - if c.cfg.WorkspaceAccess == "ro" { + if c.cfg.WorkspaceAccess == string(config.WorkspaceAccessRO) { return "", false } workspace := strings.TrimSpace(c.cfg.Workspace) diff --git a/pkg/agent/sandbox/container_integration_test.go b/pkg/agent/sandbox/container_integration_test.go index e6b0722cf..e4a0743e8 100644 --- a/pkg/agent/sandbox/container_integration_test.go +++ b/pkg/agent/sandbox/container_integration_test.go @@ -103,6 +103,22 @@ func TestContainerSandbox_Integration_ExecReadWrite(t *testing.T) { if strings.TrimSpace(pwdRes.Stdout) != "/workspace/it" { t.Fatalf("pwd mismatch: got %q want %q", strings.TrimSpace(pwdRes.Stdout), "/workspace/it") } + + // Test ReadDir + entries, err := sb.Fs().ReadDir(ctx, "it") + if err != nil { + t.Fatalf("ReadDir failed: %v", err) + } + found := false + for _, e := range entries { + if e.Name() == "write.txt" { + found = true + break + } + } + if !found { + t.Errorf("ReadDir result missing 'write.txt'") + } } func TestContainerSandbox_Integration_WriteFileMkdirInContainerTmp(t *testing.T) { @@ -364,3 +380,48 @@ func TestContainerSandbox_Integration_ExecTimeoutRespectsRequest(t *testing.T) { t.Fatalf("expected timeout to trigger early, took %v", time.Since(start)) } } + +func TestContainerSandbox_Integration_ExecTimeoutBreaksStdCopyBlock(t *testing.T) { + if os.Getenv("PICOCLAW_RUN_DOCKER_TESTS") != "1" { + t.Skip("set PICOCLAW_RUN_DOCKER_TESTS=1 to run docker integration tests") + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + containerName := fmt.Sprintf("picoclaw-test-timeout-block-%d", time.Now().UnixNano()) + image := strings.TrimSpace(os.Getenv("PICOCLAW_DOCKER_TEST_IMAGE")) + if image == "" { + image = "debian:bookworm-slim" + } + + sb := NewContainerSandbox(ContainerSandboxConfig{ + Image: image, + ContainerName: containerName, + Workspace: t.TempDir(), + }) + if err := sb.Start(ctx); err != nil { + t.Fatalf("sandbox start failed: %v", err) + } + defer func() { + _ = sb.Prune(context.Background()) + if sb.cli != nil { + _ = sb.cli.ContainerRemove(context.Background(), containerName, container.RemoveOptions{Force: true}) + } + }() + + start := time.Now() + // Run a command that sleeps for a very long time holding the stream open. + // We set a 500ms timeout. If StdCopy isn't broken asynchronously, the Exec call will hang. + _, err := sb.Exec(ctx, ExecRequest{ + Command: "sh -c 'sleep 1000'", + TimeoutMs: 500, + }) + if err == nil { + t.Fatal("expected timeout error for hanging command") + } + elapsed := time.Since(start) + if elapsed > 2*time.Second { + t.Fatalf("expected timeout to trigger within 2s, but it blocked for %v (StdCopy might be hanging)", elapsed) + } +} diff --git a/pkg/agent/sandbox/container_test.go b/pkg/agent/sandbox/container_test.go index affc78909..55a5356ed 100644 --- a/pkg/agent/sandbox/container_test.go +++ b/pkg/agent/sandbox/container_test.go @@ -1,6 +1,8 @@ package sandbox import ( + "archive/tar" + "bytes" "context" "errors" "os" @@ -53,6 +55,48 @@ func TestResolveContainerPath_RejectsAbsoluteOutsideWorkspace(t *testing.T) { } } +func TestParseTopLevelDirEntriesFromTar_KeepImmediateChildren(t *testing.T) { + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + writeHeader := func(hdr *tar.Header, content []byte) { + t.Helper() + if err := tw.WriteHeader(hdr); err != nil { + t.Fatalf("write header failed: %v", err) + } + if len(content) > 0 { + if _, err := tw.Write(content); err != nil { + t.Fatalf("write content failed: %v", err) + } + } + } + + writeHeader(&tar.Header{Name: "it", Typeflag: tar.TypeDir, Mode: 0o755}, nil) + writeHeader(&tar.Header{Name: "it/write.txt", Typeflag: tar.TypeReg, Mode: 0o644, Size: 1}, []byte("x")) + writeHeader(&tar.Header{Name: "it/sub", Typeflag: tar.TypeDir, Mode: 0o755}, nil) + writeHeader(&tar.Header{Name: "it/sub/nested.txt", Typeflag: tar.TypeReg, Mode: 0o644, Size: 1}, []byte("y")) + if err := tw.Close(); err != nil { + t.Fatalf("tar close failed: %v", err) + } + + entries, err := parseTopLevelDirEntriesFromTar(tar.NewReader(&buf)) + if err != nil { + t.Fatalf("parseTopLevelDirEntriesFromTar failed: %v", err) + } + + got := make(map[string]struct{}, len(entries)) + for _, e := range entries { + got[e.Name()] = struct{}{} + } + for _, want := range []string{"write.txt", "sub"} { + if _, ok := got[want]; !ok { + t.Fatalf("missing top-level entry %q in %+v", want, got) + } + } + if _, ok := got["nested.txt"]; ok { + t.Fatalf("nested file should not appear in top-level entries: %+v", got) + } +} + func TestBuildExecCommand_DefaultShell(t *testing.T) { sb := NewContainerSandbox(ContainerSandboxConfig{}) cmd, wd, err := sb.buildExecCommand(ExecRequest{ diff --git a/pkg/agent/sandbox/host.go b/pkg/agent/sandbox/host.go index cb176c2ce..63253a71b 100644 --- a/pkg/agent/sandbox/host.go +++ b/pkg/agent/sandbox/host.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "io" + "io/fs" "os" "os/exec" "path/filepath" @@ -90,7 +91,7 @@ func (h *HostSandbox) ExecStream( } if req.WorkingDir != "" { - dir, err := validatePath(req.WorkingDir, h.workspace, h.restrict) + dir, err := ValidatePath(req.WorkingDir, h.workspace, h.restrict) if err != nil { return nil, err } @@ -105,6 +106,9 @@ func (h *HostSandbox) ExecStream( if err != nil { return nil, fmt.Errorf("stderr pipe setup failed: %w", err) } + + prepareCommandForTermination(cmd) + if err := cmd.Start(); err != nil { return nil, err } @@ -158,9 +162,11 @@ func (h *HostSandbox) ExecStream( waitErr := cmd.Wait() if streamErr != nil { + _ = terminateProcessTree(cmd) return nil, streamErr } if cmdCtx.Err() != nil { + _ = terminateProcessTree(cmd) return nil, cmdCtx.Err() } @@ -205,7 +211,7 @@ func (h *hostFS) getSafeRelPath(path string) (string, error) { func (h *hostFS) ReadFile(ctx context.Context, path string) ([]byte, error) { if !h.restrict || h.workspace == "" || h.root == nil { // Unrestricted mode continues to use traditional resolution - resolved, err := validatePath(path, h.workspace, h.restrict) + resolved, err := ValidatePath(path, h.workspace, h.restrict) if err != nil { return nil, err } @@ -225,7 +231,7 @@ func (h *hostFS) ReadFile(ctx context.Context, path string) ([]byte, error) { func (h *hostFS) WriteFile(ctx context.Context, path string, data []byte, mkdir bool) error { if !h.restrict || h.workspace == "" || h.root == nil { // Unrestricted mode continues to use traditional resolution - resolved, err := validatePath(path, h.workspace, h.restrict) + resolved, err := ValidatePath(path, h.workspace, h.restrict) if err != nil { return err } @@ -234,7 +240,17 @@ func (h *hostFS) WriteFile(ctx context.Context, path string, data []byte, mkdir return err } } - return os.WriteFile(resolved, data, 0o644) + // Atomic write: write to temp file then rename to prevent partial writes. + tmpPath := fmt.Sprintf("%s.%d.tmp", resolved, time.Now().UnixNano()) + if err := os.WriteFile(tmpPath, data, 0o644); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("failed to write temp file: %w", err) + } + if err := os.Rename(tmpPath, resolved); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("failed to replace original file: %w", err) + } + return nil } relPath, err := h.getSafeRelPath(path) @@ -248,80 +264,32 @@ func (h *hostFS) WriteFile(ctx context.Context, path string, data []byte, mkdir return err } } - // Uses OS-level guarantees to restrict the file writing within the root descriptor. - return h.root.WriteFile(relPath, data, 0o644) + // Atomic write within os.Root: write to temp file then rename. + tmpRelPath := fmt.Sprintf("%s.%d.tmp", relPath, time.Now().UnixNano()) + if err := h.root.WriteFile(tmpRelPath, data, 0o644); err != nil { + h.root.Remove(tmpRelPath) + return fmt.Errorf("failed to write temp file: %w", err) + } + if err := h.root.Rename(tmpRelPath, relPath); err != nil { + h.root.Remove(tmpRelPath) + return fmt.Errorf("failed to rename temp file over target: %w", err) + } + return nil } -// validatePath ensures the given path is within the workspace if restrict is true but does not ensure atomic TOCTOU protection. -// It is kept for setting string-based fields like cmd.Dir where os.Root cannot be directly mapped. -// The secure file operations boundary relies on os.Root implemented in FsBridge. -// validatePath ensures the given path is within the workspace if restrict is true. -func validatePath(path, workspace string, restrict bool) (string, error) { - if workspace == "" { - return path, nil - } - - absWorkspace, err := filepath.Abs(workspace) - if err != nil { - return "", fmt.Errorf("failed to resolve workspace path: %w", err) - } - - var absPath string - if filepath.IsAbs(path) { - absPath = filepath.Clean(path) - } else { - absPath, err = filepath.Abs(filepath.Join(absWorkspace, path)) +func (h *hostFS) ReadDir(ctx context.Context, path string) ([]os.DirEntry, error) { + if !h.restrict || h.workspace == "" || h.root == nil { + resolved, err := ValidatePath(path, h.workspace, h.restrict) if err != nil { - return "", fmt.Errorf("failed to resolve file path: %w", err) + return nil, err } + return os.ReadDir(resolved) } - if restrict { - if !isWithinWorkspace(absPath, absWorkspace) { - return "", ErrOutsideWorkspace - } - - var resolved string - workspaceReal := absWorkspace - if resolved, err = filepath.EvalSymlinks(absWorkspace); err == nil { - workspaceReal = resolved - } - - if resolved, err = filepath.EvalSymlinks(absPath); err == nil { - if !isWithinWorkspace(resolved, workspaceReal) { - return "", ErrOutsideWorkspace - } - } else if os.IsNotExist(err) { - var parentResolved string - if parentResolved, err = resolveExistingAncestor(filepath.Dir(absPath)); err == nil { - if !isWithinWorkspace(parentResolved, workspaceReal) { - return "", fmt.Errorf("access denied: symlink resolves outside workspace") - } - } else if !os.IsNotExist(err) { - return "", fmt.Errorf("failed to resolve path: %w", err) - } - } else { - return "", fmt.Errorf("failed to resolve path: %w", err) - } + relPath, err := h.getSafeRelPath(path) + if err != nil { + return nil, err } - return absPath, nil -} - -func resolveExistingAncestor(path string) (string, error) { - for current := filepath.Clean(path); ; current = filepath.Dir(current) { - if resolved, err := filepath.EvalSymlinks(current); err == nil { - return resolved, nil - } else if !os.IsNotExist(err) { - return "", err - } - if filepath.Dir(current) == current { - return "", os.ErrNotExist - } - } -} - -func isWithinWorkspace(candidate, workspace string) bool { - rel, err := filepath.Rel(filepath.Clean(workspace), filepath.Clean(candidate)) - return err == nil && filepath.IsLocal(rel) + return fs.ReadDir(h.root.FS(), relPath) } diff --git a/pkg/tools/shell_process_unix.go b/pkg/agent/sandbox/host_process_unix.go similarity index 97% rename from pkg/tools/shell_process_unix.go rename to pkg/agent/sandbox/host_process_unix.go index 7b29a81bf..8b63f61bd 100644 --- a/pkg/tools/shell_process_unix.go +++ b/pkg/agent/sandbox/host_process_unix.go @@ -1,6 +1,6 @@ //go:build !windows -package tools +package sandbox import ( "os/exec" diff --git a/pkg/tools/shell_process_windows.go b/pkg/agent/sandbox/host_process_windows.go similarity index 96% rename from pkg/tools/shell_process_windows.go rename to pkg/agent/sandbox/host_process_windows.go index fe23b5c96..34da5849f 100644 --- a/pkg/tools/shell_process_windows.go +++ b/pkg/agent/sandbox/host_process_windows.go @@ -1,6 +1,6 @@ //go:build windows -package tools +package sandbox import ( "os/exec" diff --git a/pkg/agent/sandbox/host_test.go b/pkg/agent/sandbox/host_test.go index b00e470b5..b23455d29 100644 --- a/pkg/agent/sandbox/host_test.go +++ b/pkg/agent/sandbox/host_test.go @@ -77,7 +77,7 @@ func TestHostSandbox_ExecAndFs(t *testing.T) { func TestHostSandbox_ResolvePathRestrictions(t *testing.T) { root := t.TempDir() - got, err := validatePath("a/b.txt", root, true) + got, err := ValidatePath("a/b.txt", root, true) if err != nil { t.Fatalf("resolvePath relative error: %v", err) } @@ -86,7 +86,7 @@ func TestHostSandbox_ResolvePathRestrictions(t *testing.T) { t.Fatalf("resolvePath relative got %q, want %q", got, want) } - _, err = validatePath(filepath.Join(root, "..", "outside.txt"), root, true) + _, err = ValidatePath(filepath.Join(root, "..", "outside.txt"), root, true) if err == nil || !errors.Is(err, ErrOutsideWorkspace) { t.Fatalf("expected outside workspace error, got: %v", err) } @@ -97,7 +97,7 @@ func TestHostSandbox_ResolvePathRestrictions(t *testing.T) { } link := filepath.Join(root, "link.txt") if err := os.Symlink(target, link); err == nil { - _, err = validatePath("link.txt", root, true) + _, err = ValidatePath("link.txt", root, true) if err == nil || !errors.Is(err, ErrOutsideWorkspace) { t.Fatalf("expected symlink outside error, got: %v", err) } @@ -158,6 +158,56 @@ func TestHostFS_ReadFileWriteFile_Restricted(t *testing.T) { } } +func TestHostFS_ReadDir(t *testing.T) { + root := t.TempDir() + os.MkdirAll(filepath.Join(root, "a/b"), 0o755) + os.WriteFile(filepath.Join(root, "a/f1.txt"), []byte("1"), 0o644) + os.WriteFile(filepath.Join(root, "a/b/f2.txt"), []byte("2"), 0o644) + + sb := NewHostSandbox(root, true) + if err := sb.Start(context.Background()); err != nil { + t.Fatal(err) + } + defer sb.Prune(context.Background()) + + // Test restricted ReadDir on "a" + entries, err := sb.Fs().ReadDir(context.Background(), "a") + if err != nil { + t.Fatalf("ReadDir failed: %v", err) + } + + foundF1 := false + foundB := false + for _, e := range entries { + if e.Name() == "f1.txt" && !e.IsDir() { + foundF1 = true + } + if e.Name() == "b" && e.IsDir() { + foundB = true + } + } + if !foundF1 || !foundB { + t.Errorf("ReadDir result missing expected entries: foundF1=%v, foundB=%v", foundF1, foundB) + } + + // Test unrestricted ReadDir + sb2 := NewHostSandbox(root, false) + entries2, err := sb2.Fs().ReadDir(context.Background(), root) + if err != nil { + t.Fatalf("ReadDir unrestricted failed: %v", err) + } + foundA := false + for _, e := range entries2 { + if e.Name() == "a" { + foundA = true + break + } + } + if !foundA { + t.Errorf("ReadDir unrestricted missing 'a'") + } +} + func TestHostFS_ReadFileWriteFile_Unrestricted(t *testing.T) { root := t.TempDir() sb := NewHostSandbox(root, false) @@ -264,16 +314,13 @@ func TestHostFS_ReadFileWriteFile_WithoutWorkspaceOrRoot(t *testing.T) { content := []byte("hello empty workspace") target := filepath.Join(root, "empty.txt") - if err := sb.Fs().WriteFile(context.Background(), target, content, true); err != nil { - t.Fatalf("WriteFile failed: %v", err) + if err := sb.Fs().WriteFile(context.Background(), target, content, true); err == nil { + t.Fatalf("expected WriteFile to fail due to empty workspace with restrict=true") } - readContent, err := sb.Fs().ReadFile(context.Background(), target) - if err != nil { - t.Fatalf("ReadFile failed: %v", err) - } - if string(readContent) != string(content) { - t.Fatalf("content mismatch") + _, err := sb.Fs().ReadFile(context.Background(), target) + if err == nil { + t.Fatalf("expected ReadFile to fail due to empty workspace with restrict=true") } // Test case where root is nil explicitly @@ -284,7 +331,7 @@ func TestHostFS_ReadFileWriteFile_WithoutWorkspaceOrRoot(t *testing.T) { t.Fatalf("WriteFile failed: %v", err) } - readContent, err = sb2.Fs().ReadFile(context.Background(), "nil_root_test.txt") + readContent, err := sb2.Fs().ReadFile(context.Background(), "nil_root_test.txt") if err != nil { t.Fatalf("ReadFile failed: %v", err) } @@ -294,9 +341,9 @@ func TestHostFS_ReadFileWriteFile_WithoutWorkspaceOrRoot(t *testing.T) { } func TestValidatePathErrors(t *testing.T) { - _, err := validatePath("/a/b/c", "", true) - if err != nil { - t.Fatalf("expected no err for empty workspace with abs path") + _, err := ValidatePath("/a/b/c", "", true) + if err == nil { + t.Fatalf("expected err for empty workspace with restrict=true") } root := t.TempDir() @@ -306,7 +353,7 @@ func TestValidatePathErrors(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = validatePath("a.txt/b.txt", root, true) + _, err = ValidatePath("a.txt/b.txt", root, true) if err == nil { t.Fatalf("expected error when ancestor is file") } diff --git a/pkg/agent/sandbox/manager.go b/pkg/agent/sandbox/manager.go index 3f2f83e0f..0642ddaef 100644 --- a/pkg/agent/sandbox/manager.go +++ b/pkg/agent/sandbox/manager.go @@ -27,11 +27,11 @@ func NewFromConfig(workspace string, restrict bool, cfg *config.Config) Sandbox } // NewFromConfigWithAgent builds the sandbox Manager for an agent. -// Returns nil when sandboxing is disabled (mode=off), so callers can check manager != nil. +// It always returns a non-nil Manager (falling back to a host manager or error manager if needed). func NewFromConfigWithAgent(workspace string, restrict bool, cfg *config.Config, agentID string) Manager { - mode := "all" - scope := "agent" - workspaceAccess := "none" + mode := config.SandboxModeAll + scope := config.SandboxScopeAgent + workspaceAccess := config.WorkspaceAccessNone workspaceRoot := "~/.picoclaw/sandboxes" image := "picoclaw-sandbox:bookworm-slim" containerPrefix := "picoclaw-sandbox-" @@ -41,14 +41,14 @@ func NewFromConfigWithAgent(workspace string, restrict bool, cfg *config.Config, if cfg != nil { sb := cfg.Agents.Defaults.Sandbox - if strings.TrimSpace(sb.Mode) != "" { - mode = strings.TrimSpace(sb.Mode) + if sb.Mode != "" { + mode = sb.Mode } - if strings.TrimSpace(sb.Scope) != "" { - scope = strings.TrimSpace(sb.Scope) + if sb.Scope != "" { + scope = sb.Scope } - if strings.TrimSpace(sb.WorkspaceAccess) != "" { - workspaceAccess = strings.TrimSpace(sb.WorkspaceAccess) + if sb.WorkspaceAccess != "" { + workspaceAccess = sb.WorkspaceAccess } if strings.TrimSpace(sb.WorkspaceRoot) != "" { workspaceRoot = strings.TrimSpace(sb.WorkspaceRoot) @@ -71,13 +71,14 @@ func NewFromConfigWithAgent(workspace string, restrict bool, cfg *config.Config, agentID = routing.NormalizeAgentID(agentID) resolvedMode := normalizeSandboxMode(mode) - if resolvedMode == "off" { - return nil // sandbox disabled; host-level access is handled directly by tools - } - host := NewHostSandbox(workspace, restrict) _ = host.Start(context.Background()) + // When sandbox is disabled, skip building container infrastructure entirely. + if resolvedMode == config.SandboxModeOff { + return &hostOnlyManager{host: host} + } + resolvedScope := normalizeSandboxScope(scope) normalizedAccess := normalizeWorkspaceAccess(workspaceAccess) workspaceRootAbs := resolveAbsPath(expandHomePath(workspaceRoot)) @@ -105,31 +106,33 @@ func NewFromConfigWithAgent(workspace string, restrict bool, cfg *config.Config, return manager } -func normalizeWorkspaceAccess(access string) string { - v := strings.ToLower(strings.TrimSpace(access)) +func normalizeWorkspaceAccess(access config.WorkspaceAccess) config.WorkspaceAccess { + v := config.WorkspaceAccess(strings.ToLower(strings.TrimSpace(string(access)))) switch v { - case "ro", "rw": + case config.WorkspaceAccessRO, config.WorkspaceAccessRW: return v default: - return "none" + return config.WorkspaceAccessNone } } -func normalizeSandboxMode(mode string) string { - switch strings.ToLower(strings.TrimSpace(mode)) { - case "all", "non-main": - return strings.ToLower(strings.TrimSpace(mode)) +func normalizeSandboxMode(mode config.SandboxMode) config.SandboxMode { + v := config.SandboxMode(strings.ToLower(strings.TrimSpace(string(mode)))) + switch v { + case config.SandboxModeAll, config.SandboxModeNonMain: + return v default: - return "off" + return config.SandboxModeOff } } -func normalizeSandboxScope(scope string) string { - switch strings.ToLower(strings.TrimSpace(scope)) { - case "session", "shared": - return strings.ToLower(strings.TrimSpace(scope)) +func normalizeSandboxScope(scope config.SandboxScope) config.SandboxScope { + v := config.SandboxScope(strings.ToLower(strings.TrimSpace(string(scope)))) + switch v { + case config.SandboxScopeSession, config.SandboxScopeShared: + return v default: - return "agent" + return config.SandboxScopeAgent } } @@ -165,13 +168,13 @@ func resolveAbsPath(p string) string { } type scopedSandboxManager struct { - mode string - scope string + mode config.SandboxMode + scope config.SandboxScope agentID string host Sandbox image string containerPrefix string - workspaceAccess string + workspaceAccess config.WorkspaceAccess workspaceRoot string agentWorkspace string pruneIdleHours int @@ -188,7 +191,7 @@ type scopedSandboxManager struct { } func (m *scopedSandboxManager) Start(ctx context.Context) error { - if m.mode == "off" { + if m.mode == config.SandboxModeOff { return nil } if _, err := m.getOrCreateSandbox(ctx, m.defaultScopeKey()); err != nil { @@ -361,9 +364,9 @@ func (m *scopedSandboxManager) Resolve(ctx context.Context) (Sandbox, error) { func (m *scopedSandboxManager) shouldSandbox(ctx context.Context) bool { switch m.mode { - case "all": + case config.SandboxModeAll: return true - case "non-main": + case config.SandboxModeNonMain: // 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() @@ -397,9 +400,9 @@ func (m *scopedSandboxManager) normalizeSessionKey(raw string) string { func (m *scopedSandboxManager) scopeKeyFromContext(ctx context.Context) string { sessionKey := m.normalizeSessionKey(SessionKeyFromContext(ctx)) switch m.scope { - case "shared": + case config.SandboxScopeShared: return "shared" - case "session": + case config.SandboxScopeSession: return sessionKey default: if parsed := routing.ParseAgentSessionKey(sessionKey); parsed != nil { @@ -434,7 +437,7 @@ func (m *scopedSandboxManager) getOrCreateSandbox(ctx context.Context, scopeKey func (m *scopedSandboxManager) buildScopedContainerSandbox(scopeKey string) Sandbox { workspace := m.agentWorkspace - if m.workspaceAccess == "none" || strings.TrimSpace(workspace) == "" { + if m.workspaceAccess == config.WorkspaceAccessNone || strings.TrimSpace(workspace) == "" { workspace = filepath.Join(m.workspaceRoot, slugScopeKey(scopeKey), "workspace") } return NewContainerSandbox(ContainerSandboxConfig{ @@ -443,7 +446,7 @@ func (m *scopedSandboxManager) buildScopedContainerSandbox(scopeKey string) Sand ContainerPrefix: m.containerPrefix, Workspace: workspace, AgentWorkspace: m.agentWorkspace, - WorkspaceAccess: m.workspaceAccess, + WorkspaceAccess: string(m.workspaceAccess), WorkspaceRoot: m.workspaceRoot, PruneIdleHours: m.pruneIdleHours, PruneMaxAgeDays: m.pruneMaxAgeDays, @@ -494,6 +497,17 @@ func (f *managerFS) WriteFile(ctx context.Context, path string, data []byte, mkd return sb.Fs().WriteFile(ctx, path, data, mkdir) } +func (f *managerFS) ReadDir(ctx context.Context, path string) ([]os.DirEntry, error) { + if !f.m.shouldSandbox(ctx) { + return f.m.host.Fs().ReadDir(ctx, path) + } + sb, err := f.m.getOrCreateSandbox(ctx, f.m.scopeKeyFromContext(ctx)) + if err != nil { + return nil, err + } + return sb.Fs().ReadDir(ctx, path) +} + var nonAlnum = regexp.MustCompile(`[^a-z0-9._-]+`) func slugScopeKey(scopeKey string) string { @@ -513,6 +527,29 @@ func slugScopeKey(scopeKey string) string { return safe + "-" + hex.EncodeToString(sum[:4]) } +// hostOnlyManager is a lightweight Manager used when sandbox mode is "off". +// It delegates all operations directly to the HostSandbox, avoiding +// unnecessary container infrastructure setup. +type hostOnlyManager struct { + host Sandbox +} + +func (h *hostOnlyManager) Start(ctx context.Context) error { return nil } +func (h *hostOnlyManager) Prune(ctx context.Context) error { return h.host.Prune(ctx) } +func (h *hostOnlyManager) Resolve(ctx context.Context) (Sandbox, error) { return h.host, nil } +func (h *hostOnlyManager) Fs() FsBridge { return h.host.Fs() } +func (h *hostOnlyManager) Exec(ctx context.Context, req ExecRequest) (*ExecResult, error) { + return h.host.Exec(ctx, req) +} + +func (h *hostOnlyManager) ExecStream( + ctx context.Context, + req ExecRequest, + onEvent func(ExecEvent) error, +) (*ExecResult, error) { + return h.host.ExecStream(ctx, req, onEvent) +} + type unavailableSandboxManager struct { err error fs FsBridge @@ -562,3 +599,7 @@ func (e *errorFS) ReadFile(ctx context.Context, path string) ([]byte, error) { func (e *errorFS) WriteFile(ctx context.Context, path string, data []byte, mkdir bool) error { return fmt.Errorf("sandbox unavailable: %w", e.err) } + +func (e *errorFS) ReadDir(ctx context.Context, path string) ([]os.DirEntry, error) { + return nil, fmt.Errorf("sandbox unavailable: %w", e.err) +} diff --git a/pkg/agent/sandbox/path.go b/pkg/agent/sandbox/path.go new file mode 100644 index 000000000..0399f6eac --- /dev/null +++ b/pkg/agent/sandbox/path.go @@ -0,0 +1,78 @@ +package sandbox + +import ( + "fmt" + "os" + "path/filepath" +) + +// ValidatePath ensures the given path is within the workspace if restrict is true. +func ValidatePath(path, workspace string, restrict bool) (string, error) { + if workspace == "" { + return "", fmt.Errorf("workspace is not defined") + } + + absWorkspace, err := filepath.Abs(workspace) + if err != nil { + return "", fmt.Errorf("failed to resolve workspace path: %w", err) + } + + var absPath string + if filepath.IsAbs(path) { + absPath = filepath.Clean(path) + } else { + absPath, err = filepath.Abs(filepath.Join(absWorkspace, path)) + if err != nil { + return "", fmt.Errorf("failed to resolve file path: %w", err) + } + } + + if restrict { + if !isWithinWorkspace(absPath, absWorkspace) { + return "", ErrOutsideWorkspace + } + + var resolved string + workspaceReal := absWorkspace + if resolved, err = filepath.EvalSymlinks(absWorkspace); err == nil { + workspaceReal = resolved + } + + if resolved, err = filepath.EvalSymlinks(absPath); err == nil { + if !isWithinWorkspace(resolved, workspaceReal) { + return "", ErrOutsideWorkspace + } + } else if os.IsNotExist(err) { + var parentResolved string + if parentResolved, err = resolveExistingAncestor(filepath.Dir(absPath)); err == nil { + if !isWithinWorkspace(parentResolved, workspaceReal) { + return "", fmt.Errorf("access denied: symlink resolves outside workspace") + } + } else if !os.IsNotExist(err) { + return "", fmt.Errorf("failed to resolve path: %w", err) + } + } else { + return "", fmt.Errorf("failed to resolve path: %w", err) + } + } + + return absPath, nil +} + +func resolveExistingAncestor(path string) (string, error) { + for current := filepath.Clean(path); ; current = filepath.Dir(current) { + if resolved, err := filepath.EvalSymlinks(current); err == nil { + return resolved, nil + } else if !os.IsNotExist(err) { + return "", err + } + if filepath.Dir(current) == current { + return "", os.ErrNotExist + } + } +} + +func isWithinWorkspace(candidate, workspace string) bool { + rel, err := filepath.Rel(filepath.Clean(workspace), filepath.Clean(candidate)) + return err == nil && filepath.IsLocal(rel) +} diff --git a/pkg/agent/sandbox/sandbox.go b/pkg/agent/sandbox/sandbox.go index d1021bd8b..d5d4065b0 100644 --- a/pkg/agent/sandbox/sandbox.go +++ b/pkg/agent/sandbox/sandbox.go @@ -2,7 +2,10 @@ package sandbox import ( "context" + "os" "strings" + + "github.com/sipeed/picoclaw/pkg/logger" ) // Sandbox abstracts command execution and filesystem access. @@ -97,12 +100,41 @@ 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 { +// FromContext returns the sandbox instance attached by WithSandbox. +// If no pre-resolved sandbox exists, it attempts to resolve one via the Manager in context. +func FromContext(ctx context.Context) Sandbox { if ctx == nil { return nil } - v, _ := ctx.Value(sandboxContextKey{}).(Sandbox) + // 1. Try pre-resolved sandbox + if v, ok := ctx.Value(sandboxContextKey{}).(Sandbox); ok && v != nil { + return v + } + // 2. Try on-demand resolution via Manager + if m := managerFromContext(ctx); m != nil { + sb, err := m.Resolve(ctx) + if err != nil { + logger.WarnCF("sandbox", "FromContext: manager.Resolve failed", map[string]any{"error": err.Error()}) + } else if sb != nil { + return sb + } + } + return nil +} + +type managerContextKey struct{} + +// WithManager returns a derived context carrying the sandbox manager. +func WithManager(ctx context.Context, m Manager) context.Context { + return context.WithValue(ctx, managerContextKey{}, m) +} + +// managerFromContext returns the manager attached by WithManager. +func managerFromContext(ctx context.Context) Manager { + if ctx == nil { + return nil + } + v, _ := ctx.Value(managerContextKey{}).(Manager) return v } @@ -113,6 +145,8 @@ type FsBridge interface { // WriteFile writes data to a sandbox-visible path. // When mkdir is true, missing parent directories should be created. WriteFile(ctx context.Context, path string, data []byte, mkdir bool) error + // ReadDir reads the named directory and returns a list of directory entries. + ReadDir(ctx context.Context, path string) ([]os.DirEntry, error) } func aggregateExecStream(execFn func(onEvent func(ExecEvent) error) (*ExecResult, error)) (*ExecResult, error) { diff --git a/pkg/config/config.go b/pkg/config/config.go index 426162fbe..8cff3d642 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -539,10 +539,37 @@ type AgentSandboxDockerConfig struct { Binds []string `json:"binds" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_DOCKER_BINDS"` } +// SandboxMode defines the operational mode of the agent sandbox. +type SandboxMode string + +const ( + SandboxModeOff SandboxMode = "off" // Sandbox disabled (host execution) + SandboxModeNonMain SandboxMode = "non-main" // Sandbox all sessions except main + SandboxModeAll SandboxMode = "all" // Sandbox all sessions +) + +// SandboxScope defines the isolation scope of the sandbox container. +type SandboxScope string + +const ( + SandboxScopeSession SandboxScope = "session" // One container per session + SandboxScopeAgent SandboxScope = "agent" // One container per agent (shared across sessions) + SandboxScopeShared SandboxScope = "shared" // One container shared by all agents +) + +// WorkspaceAccess defines how the agent workspace is exposed to the sandbox. +type WorkspaceAccess string + +const ( + WorkspaceAccessNone WorkspaceAccess = "none" // No workspace access + WorkspaceAccessRO WorkspaceAccess = "ro" // Read-only access + WorkspaceAccessRW WorkspaceAccess = "rw" // Read-write access +) + type AgentSandboxConfig struct { - Mode string `json:"mode" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_MODE"` - Scope string `json:"scope" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_SCOPE"` - WorkspaceAccess string `json:"workspace_access" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_WORKSPACE_ACCESS"` + Mode SandboxMode `json:"mode" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_MODE"` + Scope SandboxScope `json:"scope" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_SCOPE"` + WorkspaceAccess WorkspaceAccess `json:"workspace_access" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_WORKSPACE_ACCESS"` WorkspaceRoot string `json:"workspace_root" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_WORKSPACE_ROOT"` Docker AgentSandboxDockerConfig `json:"docker" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_DOCKER"` Prune AgentSandboxPruneConfig `json:"prune" env:"PICOCLAW_AGENTS_DEFAULTS_SANDBOX_PRUNE"` diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 5c3c93fa8..76042c559 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -18,9 +18,9 @@ func DefaultConfig() *Config { Temperature: nil, // nil means use provider default MaxToolIterations: 20, Sandbox: AgentSandboxConfig{ - Mode: "off", - Scope: "agent", - WorkspaceAccess: "none", + Mode: SandboxModeOff, + Scope: SandboxScopeAgent, + WorkspaceAccess: WorkspaceAccessNone, WorkspaceRoot: "~/.picoclaw/sandboxes", Docker: AgentSandboxDockerConfig{ Image: "picoclaw-sandbox:bookworm-slim", diff --git a/pkg/tools/edit.go b/pkg/tools/edit.go index e0873d047..5f42c432b 100644 --- a/pkg/tools/edit.go +++ b/pkg/tools/edit.go @@ -2,9 +2,8 @@ package tools import ( "context" - "errors" "fmt" - "io/fs" + "os" "strings" "github.com/sipeed/picoclaw/pkg/agent/sandbox" @@ -12,19 +11,13 @@ import ( // EditFileTool edits a file by replacing old_text with new_text. // The old_text must exist exactly in the file. -type EditFileTool struct { - fs fileSystem -} +type EditFileTool struct{} -// NewEditFileTool creates a new EditFileTool with optional directory restriction. +// NewEditFileTool creates a new EditFileTool. +// The workspace and restrict parameters are kept for compatibility with tool registry signatures +// but are no longer used since filesystem access is entirely delegated to the Sandbox context. func NewEditFileTool(workspace string, restrict bool) *EditFileTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} - } - return &EditFileTool{fs: fs} + return &EditFileTool{} } func (t *EditFileTool) Name() string { @@ -72,51 +65,35 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe return ErrorResult("new_text is required") } - sb := sandbox.SandboxFromContext(ctx) - if sb != nil { - content, err := sb.Fs().ReadFile(ctx, path) - if err != nil { - return ErrorResult(fmt.Sprintf("failed to read file: %v", err)) - } - - contentStr := string(content) - if !strings.Contains(contentStr, oldText) { - return ErrorResult("old_text not found in file. Make sure it matches exactly") - } - - count := strings.Count(contentStr, oldText) - if count > 1 { - return ErrorResult( - fmt.Sprintf("old_text appears %d times. Please provide more context to make it unique", count), - ) - } - - newContent := strings.Replace(contentStr, oldText, newText, 1) - err = sb.Fs().WriteFile(ctx, path, []byte(newContent), true) - if err != nil { - return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) - } - return SilentResult(fmt.Sprintf("File edited: %s", path)) + sb := sandbox.FromContext(ctx) + if sb == nil { + return ErrorResult("sandbox environment unavailable") } - if err := editFile(t.fs, path, oldText, newText); err != nil { + content, err := sb.Fs().ReadFile(ctx, path) + if err != nil { + return ErrorResult(fmt.Sprintf("failed to read file: %v", err)) + } + + newContent, err := replaceEditContent(content, oldText, newText) + if err != nil { return ErrorResult(err.Error()) } + + err = sb.Fs().WriteFile(ctx, path, newContent, true) + if err != nil { + return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) + } return SilentResult(fmt.Sprintf("File edited: %s", path)) } -type AppendFileTool struct { - fs fileSystem -} +type AppendFileTool struct{} +// NewAppendFileTool creates a new AppendFileTool. +// The workspace and restrict parameters are kept for compatibility with tool registry signatures +// but are no longer used since filesystem access is entirely delegated to the Sandbox context. func NewAppendFileTool(workspace string, restrict bool) *AppendFileTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} - } - return &AppendFileTool{fs: fs} + return &AppendFileTool{} } func (t *AppendFileTool) Name() string { @@ -155,54 +132,24 @@ func (t *AppendFileTool) Execute(ctx context.Context, args map[string]any) *Tool return ErrorResult("content is required") } - 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 - err = sb.Fs().WriteFile(ctx, path, []byte(newContent), true) - if err != nil { - return ErrorResult(fmt.Sprintf("failed to append (write) to file: %v", err)) - } - return SilentResult(fmt.Sprintf("Appended to %s", path)) + sb := sandbox.FromContext(ctx) + if sb == nil { + return ErrorResult("sandbox environment unavailable") } - if err := appendFile(t.fs, path, content); err != nil { - return ErrorResult(err.Error()) + // Implement Append using Read + Write + oldContent, err := sb.Fs().ReadFile(ctx, path) + if err != nil && !os.IsNotExist(err) && !strings.Contains(err.Error(), "not found") { + return ErrorResult(fmt.Sprintf("failed to read file for append: %v", err)) + } + newContent := append(oldContent, []byte(content)...) + err = sb.Fs().WriteFile(ctx, path, newContent, true) + if err != nil { + return ErrorResult(fmt.Sprintf("failed to append (write) to file: %v", err)) } return SilentResult(fmt.Sprintf("Appended to %s", path)) } -// editFile reads the file via sysFs, performs the replacement, and writes back. -// It uses a fileSystem interface, allowing the same logic for both restricted and unrestricted modes. -func editFile(sysFs fileSystem, path, oldText, newText string) error { - content, err := sysFs.ReadFile(path) - if err != nil { - return err - } - - newContent, err := replaceEditContent(content, oldText, newText) - if err != nil { - return err - } - - return sysFs.WriteFile(path, newContent) -} - -// appendFile reads the existing content (if any) via sysFs, appends new content, and writes back. -func appendFile(sysFs fileSystem, path, appendContent string) error { - content, err := sysFs.ReadFile(path) - if err != nil && !errors.Is(err, fs.ErrNotExist) { - return err - } - - newContent := append(content, []byte(appendContent)...) - return sysFs.WriteFile(path, newContent) -} - // replaceEditContent handles the core logic of finding and replacing a single occurrence of oldText. func replaceEditContent(content []byte, oldText, newText string) ([]byte, error) { contentStr := string(content) diff --git a/pkg/tools/edit_test.go b/pkg/tools/edit_test.go index 83a7e778c..141d23c9e 100644 --- a/pkg/tools/edit_test.go +++ b/pkg/tools/edit_test.go @@ -8,6 +8,8 @@ import ( "testing" "github.com/stretchr/testify/assert" + + "github.com/sipeed/picoclaw/pkg/agent/sandbox" ) // TestEditTool_EditFile_Success verifies successful file editing @@ -17,7 +19,8 @@ func TestEditTool_EditFile_Success(t *testing.T) { os.WriteFile(testFile, []byte("Hello World\nThis is a test"), 0o644) tool := NewEditFileTool(tmpDir, true) - ctx := context.Background() + sb := sandbox.NewHostSandbox(tmpDir, true) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": testFile, "old_text": "World", @@ -61,7 +64,8 @@ func TestEditTool_EditFile_NotFound(t *testing.T) { testFile := filepath.Join(tmpDir, "nonexistent.txt") tool := NewEditFileTool(tmpDir, true) - ctx := context.Background() + sb := sandbox.NewHostSandbox(tmpDir, true) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": testFile, "old_text": "old", @@ -75,10 +79,9 @@ func TestEditTool_EditFile_NotFound(t *testing.T) { t.Errorf("Expected error for non-existent file") } - // Should mention file not found - if !strings.Contains(result.ForLLM, "not found") && !strings.Contains(result.ForUser, "not found") { - t.Errorf("Expected 'file not found' message, got ForLLM: %s", result.ForLLM) - } + // Should mention file not found or no such file + assert.True(t, strings.Contains(result.ForLLM, "not found") || strings.Contains(result.ForLLM, "no such file"), + "Expected 'not found' or 'no such file' message, got ForLLM: %s", result.ForLLM) } // TestEditTool_EditFile_OldTextNotFound verifies error when old_text doesn't exist @@ -88,7 +91,8 @@ func TestEditTool_EditFile_OldTextNotFound(t *testing.T) { os.WriteFile(testFile, []byte("Hello World"), 0o644) tool := NewEditFileTool(tmpDir, true) - ctx := context.Background() + sb := sandbox.NewHostSandbox(tmpDir, true) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": testFile, "old_text": "Goodbye", @@ -115,7 +119,8 @@ func TestEditTool_EditFile_MultipleMatches(t *testing.T) { os.WriteFile(testFile, []byte("test test test"), 0o644) tool := NewEditFileTool(tmpDir, true) - ctx := context.Background() + sb := sandbox.NewHostSandbox(tmpDir, true) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": testFile, "old_text": "test", @@ -143,7 +148,8 @@ func TestEditTool_EditFile_OutsideAllowedDir(t *testing.T) { os.WriteFile(testFile, []byte("content"), 0o644) tool := NewEditFileTool(tmpDir, true) // Restrict to tmpDir - ctx := context.Background() + sb := sandbox.NewHostSandbox(tmpDir, true) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": testFile, "old_text": "content", @@ -169,8 +175,9 @@ func TestEditTool_EditFile_OutsideAllowedDir(t *testing.T) { // TestEditTool_EditFile_MissingPath verifies error handling for missing path func TestEditTool_EditFile_MissingPath(t *testing.T) { - tool := NewEditFileTool("", false) - ctx := context.Background() + tool := NewEditFileTool("/", false) + sb := sandbox.NewHostSandbox("/", false) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "old_text": "old", "new_text": "new", @@ -186,8 +193,9 @@ func TestEditTool_EditFile_MissingPath(t *testing.T) { // TestEditTool_EditFile_MissingOldText verifies error handling for missing old_text func TestEditTool_EditFile_MissingOldText(t *testing.T) { - tool := NewEditFileTool("", false) - ctx := context.Background() + tool := NewEditFileTool("/", false) + sb := sandbox.NewHostSandbox("/", false) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": "/tmp/test.txt", "new_text": "new", @@ -203,8 +211,9 @@ func TestEditTool_EditFile_MissingOldText(t *testing.T) { // TestEditTool_EditFile_MissingNewText verifies error handling for missing new_text func TestEditTool_EditFile_MissingNewText(t *testing.T) { - tool := NewEditFileTool("", false) - ctx := context.Background() + tool := NewEditFileTool("/", false) + sb := sandbox.NewHostSandbox("/", false) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": "/tmp/test.txt", "old_text": "old", @@ -224,8 +233,9 @@ func TestEditTool_AppendFile_Success(t *testing.T) { testFile := filepath.Join(tmpDir, "test.txt") os.WriteFile(testFile, []byte("Initial content"), 0o644) - tool := NewAppendFileTool("", false) - ctx := context.Background() + tool := NewAppendFileTool("/", false) + sb := sandbox.NewHostSandbox("/", false) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": testFile, "content": "\nAppended content", @@ -264,7 +274,7 @@ func TestEditTool_AppendFile_Success(t *testing.T) { // TestEditTool_AppendFile_MissingPath verifies error handling for missing path func TestEditTool_AppendFile_MissingPath(t *testing.T) { - tool := NewAppendFileTool("", false) + tool := NewAppendFileTool("/", false) ctx := context.Background() args := map[string]any{ "content": "test", @@ -280,7 +290,7 @@ func TestEditTool_AppendFile_MissingPath(t *testing.T) { // TestEditTool_AppendFile_MissingContent verifies error handling for missing content func TestEditTool_AppendFile_MissingContent(t *testing.T) { - tool := NewAppendFileTool("", false) + tool := NewAppendFileTool("/", false) ctx := context.Background() args := map[string]any{ "path": "/tmp/test.txt", @@ -349,7 +359,8 @@ func TestReplaceEditContent(t *testing.T) { func TestAppendFileTool_AppendToNonExistent_Restricted(t *testing.T) { workspace := t.TempDir() tool := NewAppendFileTool(workspace, true) - ctx := context.Background() + sb := sandbox.NewHostSandbox(workspace, true) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": "brand_new_file.txt", @@ -379,7 +390,8 @@ func TestAppendFileTool_Restricted_Success(t *testing.T) { assert.NoError(t, err) tool := NewAppendFileTool(workspace, true) - ctx := context.Background() + sb := sandbox.NewHostSandbox(workspace, true) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": testFile, "content": " appended", @@ -403,7 +415,8 @@ func TestEditFileTool_Restricted_InPlaceEdit(t *testing.T) { assert.NoError(t, err) tool := NewEditFileTool(workspace, true) - ctx := context.Background() + sb := sandbox.NewHostSandbox(workspace, true) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": testFile, "old_text": "World", @@ -424,7 +437,8 @@ func TestEditFileTool_Restricted_InPlaceEdit(t *testing.T) { func TestEditFileTool_Restricted_FileNotFound(t *testing.T) { workspace := t.TempDir() tool := NewEditFileTool(workspace, true) - ctx := context.Background() + sb := sandbox.NewHostSandbox(workspace, true) + ctx := sandbox.WithSandbox(context.Background(), sb) args := map[string]any{ "path": "no_such_file.txt", "old_text": "old", @@ -433,5 +447,6 @@ func TestEditFileTool_Restricted_FileNotFound(t *testing.T) { result := tool.Execute(ctx, args) assert.True(t, result.IsError) - assert.Contains(t, result.ForLLM, "not found") + assert.True(t, strings.Contains(result.ForLLM, "not found") || strings.Contains(result.ForLLM, "no such file"), + "Expected 'not found' or 'no such file' message, got ForLLM: %s", result.ForLLM) } diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 8f2db9703..41866c3d1 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -3,98 +3,19 @@ package tools import ( "context" "fmt" - "io/fs" "os" - "path/filepath" "strings" - "time" "github.com/sipeed/picoclaw/pkg/agent/sandbox" ) -// validatePath ensures the given path is within the workspace if restrict is true. -func validatePath(path, workspace string, restrict bool) (string, error) { - if workspace == "" { - return path, fmt.Errorf("workspace is not defined") - } - - absWorkspace, err := filepath.Abs(workspace) - if err != nil { - return "", fmt.Errorf("failed to resolve workspace path: %w", err) - } - - var absPath string - if filepath.IsAbs(path) { - absPath = filepath.Clean(path) - } else { - absPath, err = filepath.Abs(filepath.Join(absWorkspace, path)) - if err != nil { - return "", fmt.Errorf("failed to resolve file path: %w", err) - } - } - - if restrict { - if !isWithinWorkspace(absPath, absWorkspace) { - return "", fmt.Errorf("access denied: path is outside the workspace") - } - - var resolved string - workspaceReal := absWorkspace - if resolved, err = filepath.EvalSymlinks(absWorkspace); err == nil { - workspaceReal = resolved - } - - if resolved, err = filepath.EvalSymlinks(absPath); err == nil { - if !isWithinWorkspace(resolved, workspaceReal) { - return "", fmt.Errorf("access denied: symlink resolves outside workspace") - } - } else if os.IsNotExist(err) { - var parentResolved string - if parentResolved, err = resolveExistingAncestor(filepath.Dir(absPath)); err == nil { - if !isWithinWorkspace(parentResolved, workspaceReal) { - return "", fmt.Errorf("access denied: symlink resolves outside workspace") - } - } else if !os.IsNotExist(err) { - return "", fmt.Errorf("failed to resolve path: %w", err) - } - } else { - return "", fmt.Errorf("failed to resolve path: %w", err) - } - } - - return absPath, nil -} - -func resolveExistingAncestor(path string) (string, error) { - for current := filepath.Clean(path); ; current = filepath.Dir(current) { - if resolved, err := filepath.EvalSymlinks(current); err == nil { - return resolved, nil - } else if !os.IsNotExist(err) { - return "", err - } - if filepath.Dir(current) == current { - return "", os.ErrNotExist - } - } -} - -func isWithinWorkspace(candidate, workspace string) bool { - rel, err := filepath.Rel(filepath.Clean(workspace), filepath.Clean(candidate)) - return err == nil && filepath.IsLocal(rel) -} - -type ReadFileTool struct { - fs fileSystem -} +type ReadFileTool struct{} +// NewReadFileTool creates a new ReadFileTool. +// The workspace and restrict parameters are kept for compatibility with tool registry signatures +// but are no longer used since filesystem access is entirely delegated to the Sandbox context. func NewReadFileTool(workspace string, restrict bool) *ReadFileTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} - } - return &ReadFileTool{fs: fs} + return &ReadFileTool{} } func (t *ReadFileTool) Name() string { @@ -124,34 +45,22 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe return ErrorResult("path is required") } - sb := sandbox.SandboxFromContext(ctx) - if sb != nil { - content, err := sb.Fs().ReadFile(ctx, path) - if err != nil { - return ErrorResult(fmt.Sprintf("failed to read file from sandbox: %v", err)) - } - return NewToolResult(string(content)) + sb := sandbox.FromContext(ctx) + if sb == nil { + return ErrorResult("sandbox environment unavailable") } - content, err := t.fs.ReadFile(path) + content, err := sb.Fs().ReadFile(ctx, path) if err != nil { - return ErrorResult(err.Error()) + return ErrorResult(fmt.Sprintf("failed to read file: %v", err)) } return NewToolResult(string(content)) } -type WriteFileTool struct { - fs fileSystem -} +type WriteFileTool struct{} func NewWriteFileTool(workspace string, restrict bool) *WriteFileTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} - } - return &WriteFileTool{fs: fs} + return &WriteFileTool{} } func (t *WriteFileTool) Name() string { @@ -190,35 +99,21 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *ToolR return ErrorResult("content is required") } - 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 to sandbox: %v", err)) - } - return SilentResult(fmt.Sprintf("File written to sandbox: %s", path)) + sb := sandbox.FromContext(ctx) + if sb == nil { + return ErrorResult("sandbox environment unavailable") } - if err := t.fs.WriteFile(path, []byte(content)); err != nil { - return ErrorResult(err.Error()) + 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)) } -// 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 { - fs fileSystem -} +type ListDirTool struct{} func NewListDirTool(workspace string, restrict bool) *ListDirTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} - } - return &ListDirTool{fs: fs} + return &ListDirTool{} } func (t *ListDirTool) Name() string { @@ -248,7 +143,12 @@ func (t *ListDirTool) Execute(ctx context.Context, args map[string]any) *ToolRes path = "." } - entries, err := t.fs.ReadDir(path) + sb := sandbox.FromContext(ctx) + if sb == nil { + return ErrorResult("sandbox environment unavailable") + } + + entries, err := sb.Fs().ReadDir(ctx, path) if err != nil { return ErrorResult(fmt.Sprintf("failed to read directory: %v", err)) } @@ -266,161 +166,3 @@ func formatDirEntries(entries []os.DirEntry) *ToolResult { } return NewToolResult(result.String()) } - -// fileSystem abstracts reading, writing, and listing files, allowing both -// unrestricted (host filesystem) and sandbox (os.Root) implementations to share the same polymorphic interface. -type fileSystem interface { - ReadFile(path string) ([]byte, error) - WriteFile(path string, data []byte) error - ReadDir(path string) ([]os.DirEntry, error) -} - -// hostFs is an unrestricted fileReadWriter that operates directly on the host filesystem. -type hostFs struct{} - -func (h *hostFs) ReadFile(path string) ([]byte, error) { - content, err := os.ReadFile(path) - if err != nil { - if os.IsNotExist(err) { - return nil, fmt.Errorf("failed to read file: file not found: %w", err) - } - if os.IsPermission(err) { - return nil, fmt.Errorf("failed to read file: access denied: %w", err) - } - return nil, fmt.Errorf("failed to read file: %w", err) - } - return content, nil -} - -func (h *hostFs) ReadDir(path string) ([]os.DirEntry, error) { - return os.ReadDir(path) -} - -func (h *hostFs) WriteFile(path string, data []byte) error { - dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("failed to create parent directories: %w", err) - } - - // We use a "write-then-rename" pattern here to ensure an atomic write. - // This prevents the target file from being left in a truncated or partial state - // if the operation is interrupted, as the rename operation is atomic on Linux. - tmpPath := fmt.Sprintf("%s.%d.tmp", path, time.Now().UnixNano()) - if err := os.WriteFile(tmpPath, data, 0o644); err != nil { - os.Remove(tmpPath) // Ensure cleanup of partial/empty temp file - return fmt.Errorf("failed to write temp file: %w", err) - } - - if err := os.Rename(tmpPath, path); err != nil { - os.Remove(tmpPath) - return fmt.Errorf("failed to replace original file: %w", err) - } - return nil -} - -// sandboxFs is a sandboxed fileSystem that operates within a strictly defined workspace using os.Root. -type sandboxFs struct { - workspace string -} - -func (r *sandboxFs) execute(path string, fn func(root *os.Root, relPath string) error) error { - if r.workspace == "" { - return fmt.Errorf("workspace is not defined") - } - - root, err := os.OpenRoot(r.workspace) - if err != nil { - return fmt.Errorf("failed to open workspace: %w", err) - } - defer root.Close() - - relPath, err := getSafeRelPath(r.workspace, path) - if err != nil { - return err - } - - return fn(root, relPath) -} - -func (r *sandboxFs) ReadFile(path string) ([]byte, error) { - var content []byte - err := r.execute(path, func(root *os.Root, relPath string) error { - fileContent, err := root.ReadFile(relPath) - if err != nil { - if os.IsNotExist(err) { - return fmt.Errorf("failed to read file: file not found: %w", err) - } - // os.Root returns "escapes from parent" for paths outside the root - if os.IsPermission(err) || strings.Contains(err.Error(), "escapes from parent") || - strings.Contains(err.Error(), "permission denied") { - return fmt.Errorf("failed to read file: access denied: %w", err) - } - return fmt.Errorf("failed to read file: %w", err) - } - content = fileContent - return nil - }) - return content, err -} - -func (r *sandboxFs) WriteFile(path string, data []byte) error { - return r.execute(path, func(root *os.Root, relPath string) error { - dir := filepath.Dir(relPath) - if dir != "." && dir != "/" { - if err := root.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("failed to create parent directories: %w", err) - } - } - - // We use a "write-then-rename" pattern here to ensure an atomic write. - // This prevents the target file from being left in a truncated or partial state - // if the operation is interrupted, as the rename operation is atomic on Linux. - tmpRelPath := fmt.Sprintf("%s.%d.tmp", relPath, time.Now().UnixNano()) - - if err := root.WriteFile(tmpRelPath, data, 0o644); err != nil { - root.Remove(tmpRelPath) // Ensure cleanup of partial/empty temp file - return fmt.Errorf("failed to write to temp file: %w", err) - } - - if err := root.Rename(tmpRelPath, relPath); err != nil { - root.Remove(tmpRelPath) - return fmt.Errorf("failed to rename temp file over target: %w", err) - } - return nil - }) -} - -func (r *sandboxFs) ReadDir(path string) ([]os.DirEntry, error) { - var entries []os.DirEntry - err := r.execute(path, func(root *os.Root, relPath string) error { - dirEntries, err := fs.ReadDir(root.FS(), relPath) - if err != nil { - return err - } - entries = dirEntries - return nil - }) - return entries, err -} - -// Helper to get a safe relative path for os.Root usage -func getSafeRelPath(workspace, path string) (string, error) { - if workspace == "" { - return "", fmt.Errorf("workspace is not defined") - } - - rel := filepath.Clean(path) - if filepath.IsAbs(rel) { - var err error - rel, err = filepath.Rel(workspace, rel) - if err != nil { - return "", fmt.Errorf("failed to calculate relative path: %w", err) - } - } - - if !filepath.IsLocal(rel) { - return "", fmt.Errorf("path escapes workspace: %s", path) - } - - return rel, nil -} diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 6f896e22d..ff6a47580 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -2,13 +2,15 @@ package tools import ( "context" - "io" + "fmt" "os" "path/filepath" "strings" "testing" "github.com/stretchr/testify/assert" + + "github.com/sipeed/picoclaw/pkg/agent/sandbox" ) // TestFilesystemTool_ReadFile_Success verifies successful file reading @@ -18,7 +20,11 @@ func TestFilesystemTool_ReadFile_Success(t *testing.T) { os.WriteFile(testFile, []byte("test content"), 0o644) tool := NewReadFileTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + fs: sandbox.NewHostSandbox(tmpDir, false).Fs(), + }) + // We must ensure the mock FsBridge can actually read the TempDir. + // but stubSandbox uses hostFs internally for Fs(). ReadFile so this just works if injected. args := map[string]any{ "path": testFile, } @@ -45,7 +51,9 @@ func TestFilesystemTool_ReadFile_Success(t *testing.T) { // TestFilesystemTool_ReadFile_NotFound verifies error handling for missing file func TestFilesystemTool_ReadFile_NotFound(t *testing.T) { tool := NewReadFileTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + err: fmt.Errorf("failed to read file: file not found"), + }) args := map[string]any{ "path": "/nonexistent_file_12345.txt", } @@ -66,7 +74,7 @@ func TestFilesystemTool_ReadFile_NotFound(t *testing.T) { // TestFilesystemTool_ReadFile_MissingPath verifies error handling for missing path func TestFilesystemTool_ReadFile_MissingPath(t *testing.T) { tool := &ReadFileTool{} - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{}) args := map[string]any{} result := tool.Execute(ctx, args) @@ -88,7 +96,9 @@ func TestFilesystemTool_WriteFile_Success(t *testing.T) { testFile := filepath.Join(tmpDir, "newfile.txt") tool := NewWriteFileTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + fs: sandbox.NewHostSandbox(tmpDir, false).Fs(), + }) args := map[string]any{ "path": testFile, "content": "hello world", @@ -127,7 +137,9 @@ func TestFilesystemTool_WriteFile_CreateDir(t *testing.T) { testFile := filepath.Join(tmpDir, "subdir", "newfile.txt") tool := NewWriteFileTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + fs: sandbox.NewHostSandbox(tmpDir, false).Fs(), + }) args := map[string]any{ "path": testFile, "content": "test", @@ -153,7 +165,7 @@ func TestFilesystemTool_WriteFile_CreateDir(t *testing.T) { // TestFilesystemTool_WriteFile_MissingPath verifies error handling for missing path func TestFilesystemTool_WriteFile_MissingPath(t *testing.T) { tool := NewWriteFileTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{}) args := map[string]any{ "content": "test", } @@ -169,7 +181,7 @@ func TestFilesystemTool_WriteFile_MissingPath(t *testing.T) { // TestFilesystemTool_WriteFile_MissingContent verifies error handling for missing content func TestFilesystemTool_WriteFile_MissingContent(t *testing.T) { tool := NewWriteFileTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{}) args := map[string]any{ "path": "/tmp/test.txt", } @@ -195,8 +207,10 @@ func TestFilesystemTool_ListDir_Success(t *testing.T) { os.WriteFile(filepath.Join(tmpDir, "file2.txt"), []byte("content"), 0o644) os.Mkdir(filepath.Join(tmpDir, "subdir"), 0o755) - tool := NewListDirTool("", false) - ctx := context.Background() + tool := NewListDirTool(tmpDir, false) + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + fs: sandbox.NewHostSandbox(tmpDir, false).Fs(), + }) args := map[string]any{ "path": tmpDir, } @@ -219,8 +233,12 @@ func TestFilesystemTool_ListDir_Success(t *testing.T) { // TestFilesystemTool_ListDir_NotFound verifies error handling for non-existent directory func TestFilesystemTool_ListDir_NotFound(t *testing.T) { - tool := NewListDirTool("", false) - ctx := context.Background() + tmpDir := t.TempDir() + tool := NewListDirTool(tmpDir, false) + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + fs: sandbox.NewHostSandbox(tmpDir, false).Fs(), + err: fmt.Errorf("failed to read directory: file not found"), + }) args := map[string]any{ "path": "/nonexistent_directory_12345", } @@ -240,8 +258,10 @@ func TestFilesystemTool_ListDir_NotFound(t *testing.T) { // TestFilesystemTool_ListDir_DefaultPath verifies default to current directory func TestFilesystemTool_ListDir_DefaultPath(t *testing.T) { - tool := NewListDirTool("", false) - ctx := context.Background() + tool := NewListDirTool(".", false) + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + fs: sandbox.NewHostSandbox(".", false).Fs(), + }) args := map[string]any{} result := tool.Execute(ctx, args) @@ -271,7 +291,9 @@ func TestFilesystemTool_ReadFile_RejectsSymlinkEscape(t *testing.T) { } tool := NewReadFileTool(workspace, true) - result := tool.Execute(context.Background(), map[string]any{ + result := tool.Execute(sandbox.WithSandbox(context.Background(), &stubSandbox{ + fs: sandbox.NewHostSandbox(workspace, true).Fs(), + }), map[string]any{ "path": link, }) @@ -295,7 +317,9 @@ func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) { secretFile := filepath.Join(tmpDir, "shadow") os.WriteFile(secretFile, []byte("secret data"), 0o600) - result := tool.Execute(context.Background(), map[string]any{ + result := tool.Execute(sandbox.WithSandbox(context.Background(), &stubSandbox{ + err: fmt.Errorf("workspace is not defined"), + }), map[string]any{ "path": secretFile, }) @@ -342,7 +366,9 @@ func TestRootMkdirAll(t *testing.T) { func TestFilesystemTool_WriteFile_Restricted_CreateDir(t *testing.T) { workspace := t.TempDir() tool := NewWriteFileTool(workspace, true) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + fs: sandbox.NewHostSandbox(workspace, true).Fs(), + }) testFile := "deep/nested/path/to/file.txt" content := "deep content" @@ -360,129 +386,3 @@ func TestFilesystemTool_WriteFile_Restricted_CreateDir(t *testing.T) { assert.NoError(t, err) assert.Equal(t, content, string(data)) } - -// TestHostRW_Read_PermissionDenied verifies that hostRW.Read surfaces access denied errors. -func TestHostRW_Read_PermissionDenied(t *testing.T) { - if os.Getuid() == 0 { - t.Skip("skipping permission test: running as root") - } - tmpDir := t.TempDir() - protected := filepath.Join(tmpDir, "protected.txt") - err := os.WriteFile(protected, []byte("secret"), 0o000) - assert.NoError(t, err) - defer os.Chmod(protected, 0o644) // ensure cleanup - - _, err = (&hostFs{}).ReadFile(protected) - assert.Error(t, err) - assert.Contains(t, err.Error(), "access denied") -} - -// TestHostRW_Read_Directory verifies that hostRW.Read returns an error when given a directory path. -func TestHostRW_Read_Directory(t *testing.T) { - tmpDir := t.TempDir() - - _, err := (&hostFs{}).ReadFile(tmpDir) - assert.Error(t, err, "expected error when reading a directory as a file") -} - -// TestRootRW_Read_Directory verifies that rootRW.Read returns an error when given a directory. -func TestRootRW_Read_Directory(t *testing.T) { - workspace := t.TempDir() - root, err := os.OpenRoot(workspace) - assert.NoError(t, err) - defer root.Close() - - // Create a subdirectory - err = root.Mkdir("subdir", 0o755) - assert.NoError(t, err) - - _, err = (&sandboxFs{workspace: workspace}).ReadFile("subdir") - assert.Error(t, err, "expected error when reading a directory as a file") -} - -// TestHostRW_Write_ParentDirMissing verifies that hostRW.Write creates parent dirs automatically. -func TestHostRW_Write_ParentDirMissing(t *testing.T) { - tmpDir := t.TempDir() - target := filepath.Join(tmpDir, "a", "b", "c", "file.txt") - - err := (&hostFs{}).WriteFile(target, []byte("hello")) - assert.NoError(t, err) - - data, err := os.ReadFile(target) - assert.NoError(t, err) - assert.Equal(t, "hello", string(data)) -} - -// TestRootRW_Write_ParentDirMissing verifies that rootRW.Write creates -// nested parent directories automatically within the sandbox. -func TestRootRW_Write_ParentDirMissing(t *testing.T) { - workspace := t.TempDir() - - relPath := "x/y/z/file.txt" - err := (&sandboxFs{workspace: workspace}).WriteFile(relPath, []byte("nested")) - assert.NoError(t, err) - - data, err := os.ReadFile(filepath.Join(workspace, relPath)) - assert.NoError(t, err) - assert.Equal(t, "nested", string(data)) -} - -// TestHostRW_Write verifies the hostRW.Write helper function -func TestHostRW_Write(t *testing.T) { - tmpDir := t.TempDir() - testFile := filepath.Join(tmpDir, "atomic_test.txt") - testData := []byte("atomic test content") - - err := (&hostFs{}).WriteFile(testFile, testData) - assert.NoError(t, err) - - content, err := os.ReadFile(testFile) - assert.NoError(t, err) - assert.Equal(t, testData, content) - - // Verify it overwrites correctly - newData := []byte("new atomic content") - err = (&hostFs{}).WriteFile(testFile, newData) - assert.NoError(t, err) - - content, err = os.ReadFile(testFile) - assert.NoError(t, err) - assert.Equal(t, newData, content) -} - -// TestRootRW_Write verifies the rootRW.Write helper function -func TestRootRW_Write(t *testing.T) { - tmpDir := t.TempDir() - - relPath := "atomic_root_test.txt" - testData := []byte("atomic root test content") - - erw := &sandboxFs{workspace: tmpDir} - err := erw.WriteFile(relPath, testData) - assert.NoError(t, err) - - root, err := os.OpenRoot(tmpDir) - assert.NoError(t, err) - defer root.Close() - - f, err := root.Open(relPath) - assert.NoError(t, err) - defer f.Close() - - content, err := io.ReadAll(f) - assert.NoError(t, err) - assert.Equal(t, testData, content) - - // Verify it overwrites correctly - newData := []byte("new root atomic content") - err = erw.WriteFile(relPath, newData) - assert.NoError(t, err) - - f2, err := root.Open(relPath) - assert.NoError(t, err) - defer f2.Close() - - content, err = io.ReadAll(f2) - assert.NoError(t, err) - assert.Equal(t, newData, content) -} diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 12d9af53e..b27bafd55 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -1,16 +1,13 @@ package tools import ( - "bytes" "context" "errors" "fmt" "os" - "os/exec" "path" "path/filepath" "regexp" - "runtime" "strings" "time" @@ -148,11 +145,11 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult cwd := t.workingDir if wd != "" { if t.restrictToWorkspace && t.workingDir != "" { - resolvedWD, err := validatePath(wd, t.workingDir, true) + resolvedWD, err := sandbox.ValidatePath(wd, t.workingDir, true) if err != nil { // In sandbox mode, allow explicit container workspace paths when // restrict_to_workspace is enabled. - sb := sandbox.SandboxFromContext(ctx) + sb := sandbox.FromContext(ctx) if sb != nil && filepath.IsAbs(wd) && isSandboxWorkspaceAbsolutePath(wd) { cwd = wd } else { @@ -176,96 +173,19 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult return ErrorResult(guardError) } - sb := sandbox.SandboxFromContext(ctx) - if sb != nil { - sandboxWD := t.resolveSandboxWorkingDir(cwd) - res, err := sb.Exec(ctx, sandbox.ExecRequest{ - Command: command, - WorkingDir: sandboxWD, - TimeoutMs: t.timeout.Milliseconds(), - }) - if err != nil { - return ErrorResult(fmt.Sprintf("sandbox exec failed: %v", err)) - } - output := res.Stdout - if res.Stderr != "" { - output += "\nSTDERR:\n" + res.Stderr - } - if output == "" { - output = "(no output)" - } - if res.ExitCode != 0 { - output += fmt.Sprintf("\nExit code: %d", res.ExitCode) - return &ToolResult{ - ForLLM: output, - ForUser: output, - IsError: true, - } - } - return &ToolResult{ - ForLLM: output, - ForUser: output, - IsError: false, - } - } - - // timeout == 0 means no timeout - var cmdCtx context.Context - var cancel context.CancelFunc - if t.timeout > 0 { - cmdCtx, cancel = context.WithTimeout(ctx, t.timeout) - } else { - cmdCtx, cancel = context.WithCancel(ctx) - } - defer cancel() - - var cmd *exec.Cmd - if runtime.GOOS == "windows" { - cmd = exec.CommandContext(cmdCtx, "powershell", "-NoProfile", "-NonInteractive", "-Command", command) - } else { - cmd = exec.CommandContext(cmdCtx, "sh", "-c", command) - } - if cwd != "" { - cmd.Dir = cwd - } - - prepareCommandForTermination(cmd) - - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - - if err := cmd.Start(); err != nil { - return ErrorResult(fmt.Sprintf("failed to start command: %v", err)) - } - - done := make(chan error, 1) - go func() { - done <- cmd.Wait() - }() - - var err error - select { - case err = <-done: - case <-cmdCtx.Done(): - terminateProcessTree(cmd) - select { - case err = <-done: - case <-time.After(2 * time.Second): - if cmd.Process != nil { - _ = cmd.Process.Kill() - } - err = <-done - } - } - - output := stdout.String() - if stderr.Len() > 0 { - output += "\nSTDERR:\n" + stderr.String() + sb := sandbox.FromContext(ctx) + if sb == nil { + return ErrorResult("sandbox environment unavailable") } + sandboxWD := t.resolveSandboxWorkingDir(cwd) + res, err := sb.Exec(ctx, sandbox.ExecRequest{ + Command: command, + WorkingDir: sandboxWD, + TimeoutMs: t.timeout.Milliseconds(), + }) if err != nil { - if errors.Is(cmdCtx.Err(), context.DeadlineExceeded) { + if errors.Is(err, context.DeadlineExceeded) || strings.Contains(err.Error(), "context deadline exceeded") { msg := fmt.Sprintf("command timed out after %v", t.timeout) return &ToolResult{ ForLLM: msg, @@ -273,14 +193,12 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult IsError: true, } } - var exitErr *exec.ExitError - if errors.As(err, &exitErr) { - output += fmt.Sprintf("\nExit code: %d", exitErr.ExitCode()) - } else { - output += fmt.Sprintf("\nError: %v", err) - } + return ErrorResult(fmt.Sprintf("sandbox exec failed: %v", err)) + } + output := res.Stdout + if res.Stderr != "" { + output += "\nSTDERR:\n" + res.Stderr } - if output == "" { output = "(no output)" } @@ -290,10 +208,18 @@ 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 res.ExitCode != 0 { + output += fmt.Sprintf("\nExit code: %d", res.ExitCode) + return &ToolResult{ + ForLLM: output, + ForUser: output, + IsError: true, + } + } return &ToolResult{ ForLLM: output, ForUser: output, - IsError: err != nil, + IsError: false, } } diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 096aec112..17a34235d 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -16,6 +16,7 @@ type stubSandbox struct { lastReq sandbox.ExecRequest err error res *sandbox.ExecResult + fs sandbox.FsBridge } func (s *stubSandbox) Start(ctx context.Context) error { return nil } @@ -24,7 +25,14 @@ func (s *stubSandbox) Prune(ctx context.Context) error { 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) Fs() sandbox.FsBridge { + if s.fs != nil { + return s.fs + } + return sandbox.NewHostSandbox("", false).Fs() +} + func (s *stubSandbox) Exec(ctx context.Context, req sandbox.ExecRequest) (*sandbox.ExecResult, error) { return sandboxAggregateFromStub(ctx, req, s.ExecStream) } @@ -103,7 +111,9 @@ func sandboxAggregateFromStub( func TestShellTool_Success(t *testing.T) { tool := NewExecTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + res: &sandbox.ExecResult{Stdout: "hello world", ExitCode: 0}, + }) args := map[string]any{ "command": "echo 'hello world'", } @@ -130,7 +140,13 @@ func TestShellTool_Success(t *testing.T) { func TestShellTool_Failure(t *testing.T) { tool := NewExecTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + res: &sandbox.ExecResult{ + Stdout: "", + Stderr: "ls: cannot access '/nonexistent_directory_12345': No such file or directory", + ExitCode: 2, + }, + }) args := map[string]any{ "command": "ls /nonexistent_directory_12345", } @@ -158,7 +174,9 @@ func TestShellTool_Timeout(t *testing.T) { tool := NewExecTool("", false) tool.SetTimeout(100 * time.Millisecond) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + err: context.DeadlineExceeded, + }) args := map[string]any{ "command": "sleep 10", } @@ -185,7 +203,9 @@ func TestShellTool_WorkingDir(t *testing.T) { tool := NewExecTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + res: &sandbox.ExecResult{Stdout: "test content\n", ExitCode: 0}, + }) args := map[string]any{ "command": "cat test.txt", "working_dir": tmpDir, @@ -206,7 +226,9 @@ func TestShellTool_WorkingDir(t *testing.T) { func TestShellTool_DangerousCommand(t *testing.T) { tool := NewExecTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + res: &sandbox.ExecResult{Stdout: "", Stderr: "", ExitCode: 1}, + }) args := map[string]any{ "command": "rm -rf /", } @@ -227,7 +249,9 @@ func TestShellTool_DangerousCommand(t *testing.T) { func TestShellTool_MissingCommand(t *testing.T) { tool := NewExecTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + res: &sandbox.ExecResult{Stdout: "", Stderr: "", ExitCode: 1}, + }) args := map[string]any{} result := tool.Execute(ctx, args) @@ -242,7 +266,9 @@ func TestShellTool_MissingCommand(t *testing.T) { func TestShellTool_StderrCapture(t *testing.T) { tool := NewExecTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + res: &sandbox.ExecResult{Stdout: "stdout", Stderr: "stderr", ExitCode: 0}, + }) args := map[string]any{ "command": "sh -c 'echo stdout; echo stderr >&2'", } @@ -262,7 +288,9 @@ func TestShellTool_StderrCapture(t *testing.T) { func TestShellTool_OutputTruncation(t *testing.T) { tool := NewExecTool("", false) - ctx := context.Background() + ctx := sandbox.WithSandbox(context.Background(), &stubSandbox{ + res: &sandbox.ExecResult{Stdout: strings.Repeat("x", 20000), ExitCode: 0}, + }) // Generate long output (>10000 chars) args := map[string]any{ "command": "python3 -c \"print('x' * 20000)\" || echo " + strings.Repeat("x", 20000), @@ -289,7 +317,9 @@ func TestShellTool_WorkingDir_OutsideWorkspace(t *testing.T) { } tool := NewExecTool(workspace, true) - result := tool.Execute(context.Background(), map[string]any{ + result := tool.Execute(sandbox.WithSandbox(context.Background(), &stubSandbox{ + res: &sandbox.ExecResult{Stdout: "", Stderr: "", ExitCode: 1}, + }), map[string]any{ "command": "pwd", "working_dir": outsideDir, }) diff --git a/pkg/tools/shell_timeout_unix_test.go b/pkg/tools/shell_timeout_unix_test.go index 04ef8e441..053eda27b 100644 --- a/pkg/tools/shell_timeout_unix_test.go +++ b/pkg/tools/shell_timeout_unix_test.go @@ -11,6 +11,8 @@ import ( "syscall" "testing" "time" + + "github.com/sipeed/picoclaw/pkg/agent/sandbox" ) func processExists(pid int) bool { @@ -22,15 +24,25 @@ func processExists(pid int) bool { } func TestShellTool_TimeoutKillsChildProcess(t *testing.T) { - tool := NewExecTool(t.TempDir(), false) + workspace := t.TempDir() + tool := NewExecTool(workspace, false) tool.SetTimeout(500 * time.Millisecond) + sb := sandbox.NewHostSandbox(workspace, false) + err := sb.Start(context.Background()) + if err != nil { + t.Fatalf("failed to start sandbox: %v", err) + } + defer sb.Prune(context.Background()) + + ctx := sandbox.WithSandbox(context.Background(), sb) + args := map[string]any{ // Spawn a child process that would outlive the shell unless process-group kill is used. "command": "sleep 60 & echo $! > child.pid; wait", } - result := tool.Execute(context.Background(), args) + result := tool.Execute(ctx, args) if !result.IsError { t.Fatalf("expected timeout error, got success: %s", result.ForLLM) }