From 0f576ba3a54ee08b1fda099ca26404a86c62a543 Mon Sep 17 00:00:00 2001 From: 0x5487 Date: Sat, 21 Feb 2026 19:11:38 +0800 Subject: [PATCH] refactor: Rename sandbox Stop to Prune, remove DisabledTool, and introduce a centralized sandbox pruning loop. --- pkg/agent/instance.go | 18 +-- pkg/agent/instance_test.go | 21 ++- pkg/agent/sandbox/container.go | 142 +++++++----------- .../sandbox/container_additional_test.go | 50 +----- .../sandbox/container_integration_test.go | 23 ++- pkg/agent/sandbox/container_test.go | 19 ++- pkg/agent/sandbox/factory.go | 125 ++++++++++++++- pkg/agent/sandbox/factory_additional_test.go | 51 ++++++- pkg/agent/sandbox/host.go | 2 +- .../sandbox/host_unavailable_util_test.go | 8 +- pkg/agent/sandbox/sandbox.go | 6 +- pkg/agent/sandbox/unavailable.go | 2 +- pkg/tools/cron_additional_test.go | 2 +- pkg/tools/disabled.go | 46 ------ pkg/tools/shell.go | 21 ++- pkg/tools/shell_test.go | 29 +++- 16 files changed, 323 insertions(+), 242 deletions(-) delete mode 100644 pkg/tools/disabled.go diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 98ac099f5..ab8f8250e 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -74,27 +74,15 @@ func NewAgentInstance( roContainer := isContainerReadOnlySandbox(cfg) toolsRegistry := tools.NewToolRegistry() toolsRegistry.Register(tools.NewReadFileToolWithSandbox(workspace, restrict, readSb)) - if roContainer { - toolsRegistry.Register(tools.NewDisabledTool( - "write_file", - "Write content to a file", - "write_file is disabled when sandbox workspace_access=ro", - )) - } else { + if !roContainer { toolsRegistry.Register(tools.NewWriteFileToolWithSandbox(workspace, restrict, writeSb)) } toolsRegistry.Register(tools.NewListDirTool(workspace, restrict)) toolsRegistry.Register(tools.NewExecToolWithSandbox(workspace, restrict, cfg, execSb)) - if roContainer { - toolsRegistry.Register(tools.NewDisabledTool( - "edit_file", - "Edit a file by replacing old_text with new_text. The old_text must exist exactly in the file.", - "edit_file is disabled when sandbox workspace_access=ro", - )) - } else { + if !roContainer { toolsRegistry.Register(tools.NewEditFileTool(workspace, restrict)) + toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict)) } - toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict)) sessionsDir := filepath.Join(workspace, "sessions") sessionsManager := session.NewSessionManager(sessionsDir) diff --git a/pkg/agent/instance_test.go b/pkg/agent/instance_test.go index e2f89c90d..e8649a82d 100644 --- a/pkg/agent/instance_test.go +++ b/pkg/agent/instance_test.go @@ -96,7 +96,7 @@ func TestNewAgentInstance_DefaultsTemperatureWhenUnset(t *testing.T) { } } -func TestNewAgentInstance_ReadOnlyContainerDisablesWriteAndEdit(t *testing.T) { +func TestNewAgentInstance_ReadOnlyContainerOmitsWriteTools(t *testing.T) { tmpDir, err := os.MkdirTemp("", "agent-instance-test-*") if err != nil { t.Fatalf("Failed to create temp dir: %v", err) @@ -121,20 +121,17 @@ func TestNewAgentInstance_ReadOnlyContainerDisablesWriteAndEdit(t *testing.T) { provider := &mockProvider{} agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider) + for _, name := range []string{"write_file", "edit_file", "append_file"} { + if _, ok := agent.Tools.Get(name); ok { + t.Fatalf("%s should not be registered in ro sandbox", name) + } + } + writeRes := agent.Tools.Execute(context.Background(), "write_file", map[string]interface{}{ "path": "a.txt", "content": "hello", }) - if !writeRes.IsError || !strings.Contains(writeRes.ForLLM, "workspace_access=ro") { - t.Fatalf("write_file should be disabled in ro sandbox, got: %+v", writeRes) - } - - editRes := agent.Tools.Execute(context.Background(), "edit_file", map[string]interface{}{ - "path": "a.txt", - "old_text": "h", - "new_text": "H", - }) - if !editRes.IsError || !strings.Contains(editRes.ForLLM, "workspace_access=ro") { - t.Fatalf("edit_file should be disabled in ro sandbox, got: %+v", editRes) + if !writeRes.IsError || !strings.Contains(writeRes.ForLLM, "not found") { + t.Fatalf("write_file should be absent in ro sandbox, got: %+v", writeRes) } } diff --git a/pkg/agent/sandbox/container.go b/pkg/agent/sandbox/container.go index a068ee23e..022116986 100644 --- a/pkg/agent/sandbox/container.go +++ b/pkg/agent/sandbox/container.go @@ -14,7 +14,6 @@ import ( "sort" "strconv" "strings" - "sync" "time" "github.com/docker/docker/api/types/container" @@ -64,11 +63,10 @@ type ContainerSandbox struct { startErr error fs FsBridge hash string - loopMu sync.Mutex - loopStop context.CancelFunc - loopDone chan struct{} } +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) == "" { @@ -103,7 +101,7 @@ func NewContainerSandbox(cfg ContainerSandboxConfig) *ContainerSandbox { return sb } -// Start initializes docker connectivity, validates config, and starts background prune scheduling. +// Start initializes docker connectivity and validates sandbox runtime requirements. func (c *ContainerSandbox) Start(ctx context.Context) error { if err := validateSandboxSecurity(c.cfg); err != nil { c.startErr = err @@ -148,74 +146,28 @@ func (c *ContainerSandbox) Start(ctx context.Context) error { } c.startErr = nil - _ = c.maybePrune(ctx) - c.ensurePruneLoop() return nil } -// Stop terminates background prune scheduling and stops the managed container if present. -func (c *ContainerSandbox) Stop(ctx context.Context) error { - c.stopPruneLoop(ctx) - if c.cli == nil { +// Prune reclaims container sandbox resources. +// This is the container-specific cleanup boundary where implementations should +// stop and remove this sandbox container. +func (c *ContainerSandbox) Prune(ctx context.Context) error { + containerName := strings.TrimSpace(c.cfg.ContainerName) + if containerName == "" { return nil } - if c.cfg.ContainerName != "" { - _ = c.cli.ContainerStop(ctx, c.cfg.ContainerName, container.StopOptions{}) - } - return nil -} -func (c *ContainerSandbox) ensurePruneLoop() { - if c.cfg.PruneIdleHours <= 0 && c.cfg.PruneMaxAgeDays <= 0 { - return - } - c.loopMu.Lock() - defer c.loopMu.Unlock() - if c.loopStop != nil { - return - } - loopCtx, cancel := context.WithCancel(context.Background()) - done := make(chan struct{}) - c.loopStop = cancel - c.loopDone = done - go func() { - ticker := time.NewTicker(5 * time.Minute) - defer func() { - ticker.Stop() - close(done) - }() - for { - select { - case <-loopCtx.Done(): - return - case <-ticker.C: - _ = c.maybePrune(loopCtx) - } + var firstErr error + if c.cli != nil { + if err := c.stopAndRemoveContainer(ctx, containerName); err != nil { + firstErr = err } - }() -} - -func (c *ContainerSandbox) stopPruneLoop(ctx context.Context) { - if ctx == nil { - ctx = context.Background() } - c.loopMu.Lock() - stop := c.loopStop - done := c.loopDone - c.loopStop = nil - c.loopDone = nil - c.loopMu.Unlock() - if stop == nil { - return - } - stop() - if done == nil { - return - } - select { - case <-done: - case <-ctx.Done(): + if err := removeRegistryEntry(c.registryPath(), containerName); err != nil && firstErr == nil { + firstErr = err } + return firstErr } // Exec ensures the container is ready and runs the requested command inside the sandbox. @@ -306,8 +258,6 @@ func (c *ContainerSandbox) Fs() FsBridge { } func (c *ContainerSandbox) ensureContainer(ctx context.Context) error { - _ = c.maybePrune(ctx) - inspect, err := c.cli.ContainerInspect(ctx, c.cfg.ContainerName) if err != nil { return c.createAndStart(ctx) @@ -421,37 +371,49 @@ func (c *ContainerSandbox) binds() []string { } func (c *ContainerSandbox) registryPath() string { - root := strings.TrimSpace(c.cfg.WorkspaceRoot) - if root == "" { - root = strings.TrimSpace(c.cfg.Workspace) - } - if root == "" { - root = osTempDir() - } - return filepath.Join(root, "state", "registry.json") + return filepath.Join(c.sandboxStateDir(), defaultSandboxRegistryFile) } -func (c *ContainerSandbox) maybePrune(ctx context.Context) error { - if c.cfg.PruneIdleHours <= 0 && c.cfg.PruneMaxAgeDays <= 0 { +func (c *ContainerSandbox) sandboxStateDir() string { + return filepath.Join(resolvePicoClawHomeDir(), "state", "sandbox") +} + +func resolvePicoClawHomeDir() string { + if envHome := strings.TrimSpace(os.Getenv("PICOCLAW_HOME")); envHome != "" { + if abs := resolveAbsPath(expandHomePath(envHome)); strings.TrimSpace(abs) != "" { + return abs + } + } + if home, err := os.UserHomeDir(); err == nil && strings.TrimSpace(home) != "" { + return filepath.Join(home, ".picoclaw") + } + return filepath.Join(osTempDir(), ".picoclaw") +} + +func (c *ContainerSandbox) stopAndRemoveContainer(ctx context.Context, containerName string) error { + timeout := 10 + _ = c.cli.ContainerStop(ctx, containerName, container.StopOptions{Timeout: &timeout}) + if err := c.cli.ContainerRemove(ctx, containerName, container.RemoveOptions{Force: true}); err != nil { + return err + } + return nil +} + +func stopAndRemoveContainerByName(ctx context.Context, containerName string) error { + name := strings.TrimSpace(containerName) + if name == "" { return nil } - if c.cli == nil { - return nil - } - regPath := c.registryPath() - registryMu.Lock() - data, err := loadRegistry(regPath) - registryMu.Unlock() + cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) if err != nil { return err } - now := time.Now().UnixMilli() - for _, e := range data.Entries { - if !shouldPruneEntry(c.cfg, now, e) { - continue - } - _ = c.cli.ContainerRemove(ctx, e.ContainerName, container.RemoveOptions{Force: true}) - _ = removeRegistryEntry(regPath, e.ContainerName) + defer cli.Close() + + timeout := 10 + _ = cli.ContainerStop(ctx, name, container.StopOptions{Timeout: &timeout}) + if err := cli.ContainerRemove(ctx, name, container.RemoveOptions{Force: true}); err != nil { + return err } return nil } diff --git a/pkg/agent/sandbox/container_additional_test.go b/pkg/agent/sandbox/container_additional_test.go index 6c5225d84..ba92d389d 100644 --- a/pkg/agent/sandbox/container_additional_test.go +++ b/pkg/agent/sandbox/container_additional_test.go @@ -9,7 +9,6 @@ import ( "testing" "time" - "github.com/docker/docker/client" "github.com/sipeed/picoclaw/pkg/config" ) @@ -24,7 +23,7 @@ func TestContainerSandbox_StartCreatesWorkspaceBeforeDockerPing(t *testing.T) { err := sb.Start(context.Background()) if err == nil { - _ = sb.Stop(context.Background()) + _ = sb.Prune(context.Background()) t.Skip("docker daemon available in this environment; skip unavailable-path assertion") } if !strings.Contains(err.Error(), "docker daemon unavailable") { @@ -38,50 +37,14 @@ func TestContainerSandbox_StartCreatesWorkspaceBeforeDockerPing(t *testing.T) { } } -func TestContainerSandbox_PruneLoopLifecycleAndNoopPrune(t *testing.T) { +func TestContainerSandbox_NoopPruneWithoutClient(t *testing.T) { sb := NewContainerSandbox(ContainerSandboxConfig{ PruneIdleHours: 1, PruneMaxAgeDays: 0, }) - sb.ensurePruneLoop() - if sb.loopStop == nil || sb.loopDone == nil { - t.Fatal("expected prune loop to start") - } - sb.stopPruneLoop(nil) - if sb.loopStop != nil || sb.loopDone != nil { - t.Fatal("expected prune loop state reset after stop") - } - - if err := sb.maybePrune(context.Background()); err != nil { - t.Fatalf("maybePrune() with nil client should be noop, got: %v", err) - } -} - -func TestContainerSandbox_MaybePruneDisabledAndLoadError(t *testing.T) { - disabled := NewContainerSandbox(ContainerSandboxConfig{}) - if err := disabled.maybePrune(context.Background()); err != nil { - t.Fatalf("maybePrune() should return nil when both prune rules disabled: %v", err) - } - - root := t.TempDir() - stateDir := filepath.Join(root, "state") - if err := os.MkdirAll(stateDir, 0o755); err != nil { - t.Fatalf("mkdir state dir: %v", err) - } - regPath := filepath.Join(stateDir, "registry.json") - if err := os.WriteFile(regPath, []byte("{not-json"), 0o644); err != nil { - t.Fatalf("write invalid registry: %v", err) - } - - sb := NewContainerSandbox(ContainerSandboxConfig{ - WorkspaceRoot: root, - PruneIdleHours: 1, - PruneMaxAgeDays: 0, - }) - sb.cli = &client.Client{} - if err := sb.maybePrune(context.Background()); err == nil { - t.Fatal("expected maybePrune() to return registry load error") + if err := sb.Prune(context.Background()); err != nil { + t.Fatalf("Prune() with nil client should be noop, got: %v", err) } } @@ -215,11 +178,10 @@ func TestContainerSandbox_StopWithoutClient(t *testing.T) { PruneIdleHours: 1, PruneMaxAgeDays: 1, }) - sb.ensurePruneLoop() ctx, cancel := context.WithTimeout(context.Background(), time.Second) defer cancel() - if err := sb.Stop(ctx); err != nil { - t.Fatalf("Stop() error: %v", err) + if err := sb.Prune(ctx); err != nil { + t.Fatalf("Prune() error: %v", err) } } diff --git a/pkg/agent/sandbox/container_integration_test.go b/pkg/agent/sandbox/container_integration_test.go index b2046158d..bee2c9821 100644 --- a/pkg/agent/sandbox/container_integration_test.go +++ b/pkg/agent/sandbox/container_integration_test.go @@ -47,7 +47,7 @@ func TestContainerSandbox_Integration_ExecReadWrite(t *testing.T) { t.Fatalf("sandbox start failed: %v", err) } defer func() { - _ = sb.Stop(context.Background()) + _ = sb.Prune(context.Background()) if sb.cli != nil { _ = sb.cli.ContainerRemove(context.Background(), containerName, container.RemoveOptions{Force: true}) } @@ -134,7 +134,7 @@ func TestContainerSandbox_Integration_WriteFileMkdirInContainerTmp(t *testing.T) t.Fatalf("sandbox start failed: %v", err) } defer func() { - _ = sb.Stop(context.Background()) + _ = sb.Prune(context.Background()) if sb.cli != nil { _ = sb.cli.ContainerRemove(context.Background(), containerName, container.RemoveOptions{Force: true}) } @@ -183,7 +183,7 @@ func TestContainerSandbox_Integration_SetupCommandSuccess(t *testing.T) { t.Fatalf("sandbox start failed: %v", err) } defer func() { - _ = sb.Stop(context.Background()) + _ = sb.Prune(context.Background()) if sb.cli != nil { _ = sb.cli.ContainerRemove(context.Background(), containerName, container.RemoveOptions{Force: true}) } @@ -227,7 +227,7 @@ func TestContainerSandbox_Integration_SetupCommandFailureRemovesContainer(t *tes t.Fatalf("sandbox start failed: %v", err) } defer func() { - _ = sb.Stop(context.Background()) + _ = sb.Prune(context.Background()) if sb.cli != nil { _ = sb.cli.ContainerRemove(context.Background(), containerName, container.RemoveOptions{Force: true}) } @@ -271,7 +271,7 @@ func TestContainerSandbox_Integration_MaybePruneRemovesOldContainer(t *testing.T t.Fatalf("sandbox start failed: %v", err) } defer func() { - _ = sb.Stop(context.Background()) + _ = sb.Prune(context.Background()) if sb.cli != nil { _ = sb.cli.ContainerRemove(context.Background(), containerName, container.RemoveOptions{Force: true}) } @@ -292,8 +292,15 @@ func TestContainerSandbox_Integration_MaybePruneRemovesOldContainer(t *testing.T t.Fatalf("upsert old registry entry failed: %v", err) } - if err := sb.maybePrune(ctx); err != nil { - t.Fatalf("maybePrune failed: %v", err) + manager := &scopedSandboxManager{ + pruneIdleHours: 1, + pruneMaxAgeDays: 0, + scoped: map[string]Sandbox{ + "agent:main": sb, + }, + } + if err := manager.pruneOnce(ctx); err != nil { + t.Fatalf("pruneOnce failed: %v", err) } if _, err := sb.cli.ContainerInspect(ctx, containerName); err == nil { @@ -333,7 +340,7 @@ func TestContainerSandbox_Integration_ExecTimeoutRespectsRequest(t *testing.T) { t.Fatalf("sandbox start failed: %v", err) } defer func() { - _ = sb.Stop(context.Background()) + _ = sb.Prune(context.Background()) if sb.cli != nil { _ = sb.cli.ContainerRemove(context.Background(), containerName, container.RemoveOptions{Force: true}) } diff --git a/pkg/agent/sandbox/container_test.go b/pkg/agent/sandbox/container_test.go index d71e97759..84796be70 100644 --- a/pkg/agent/sandbox/container_test.go +++ b/pkg/agent/sandbox/container_test.go @@ -260,13 +260,26 @@ func TestContainerSandbox_Binds_WorkspaceAccessModes(t *testing.T) { } } -func TestContainerSandbox_RegistryPath_UsesWorkspaceRoot(t *testing.T) { +func TestContainerSandbox_RegistryPath_UsesSandboxStateDir(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) sb := NewContainerSandbox(ContainerSandboxConfig{ Workspace: "/tmp/ws", WorkspaceRoot: "/tmp/sbx", }) - if got := sb.registryPath(); got != "/tmp/sbx/state/registry.json" { - t.Fatalf("registryPath = %q, want %q", got, "/tmp/sbx/state/registry.json") + want := filepath.Join(home, ".picoclaw", "state", "sandbox", "containers.json") + if got := sb.registryPath(); got != want { + t.Fatalf("registryPath = %q, want %q", got, want) + } +} + +func TestContainerSandbox_RegistryPath_UsesPicoClawHomeOverride(t *testing.T) { + picoHome := t.TempDir() + t.Setenv("PICOCLAW_HOME", picoHome) + sb := NewContainerSandbox(ContainerSandboxConfig{}) + want := filepath.Join(picoHome, "state", "sandbox", "containers.json") + if got := sb.registryPath(); got != want { + t.Fatalf("registryPath = %q, want %q", got, want) } } diff --git a/pkg/agent/sandbox/factory.go b/pkg/agent/sandbox/factory.go index 463da934d..1cff0b805 100644 --- a/pkg/agent/sandbox/factory.go +++ b/pkg/agent/sandbox/factory.go @@ -10,6 +10,7 @@ import ( "regexp" "strings" "sync" + "time" "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/routing" @@ -22,7 +23,7 @@ func NewFromConfig(workspace string, restrict bool, cfg *config.Config) Sandbox // NewFromConfigWithAgent builds a sandbox instance with an explicit agent ID context. func NewFromConfigWithAgent(workspace string, restrict bool, cfg *config.Config, agentID string) Sandbox { - mode := "off" + mode := "all" scope := "agent" workspaceAccess := "none" workspaceRoot := "~/.picoclaw/sandboxes" @@ -172,17 +173,26 @@ type scopedSandboxManager struct { mu sync.Mutex scoped map[string]Sandbox fs FsBridge + + loopMu sync.Mutex + loopStop context.CancelFunc + loopDone chan struct{} } func (m *scopedSandboxManager) Start(ctx context.Context) error { if m.mode == "off" { return nil } - _, err := m.getOrCreateSandbox(ctx, m.defaultScopeKey()) - return err + if _, err := m.getOrCreateSandbox(ctx, m.defaultScopeKey()); err != nil { + return err + } + m.ensurePruneLoop() + return nil } -func (m *scopedSandboxManager) Stop(ctx context.Context) error { +func (m *scopedSandboxManager) Prune(ctx context.Context) error { + m.stopPruneLoop(ctx) + m.mu.Lock() scoped := make([]Sandbox, 0, len(m.scoped)) for _, sb := range m.scoped { @@ -192,13 +202,118 @@ func (m *scopedSandboxManager) Stop(ctx context.Context) error { var firstErr error for _, sb := range scoped { - if err := sb.Stop(ctx); err != nil && firstErr == nil { + if err := sb.Prune(ctx); err != nil && firstErr == nil { firstErr = err } } return firstErr } +func (m *scopedSandboxManager) ensurePruneLoop() { + if m.pruneIdleHours <= 0 && m.pruneMaxAgeDays <= 0 { + return + } + m.loopMu.Lock() + defer m.loopMu.Unlock() + if m.loopStop != nil { + return + } + + loopCtx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + m.loopStop = cancel + m.loopDone = done + + go func() { + ticker := time.NewTicker(5 * time.Minute) + defer func() { + ticker.Stop() + close(done) + }() + for { + select { + case <-loopCtx.Done(): + return + case <-ticker.C: + _ = m.pruneOnce(loopCtx) + } + } + }() +} + +func (m *scopedSandboxManager) stopPruneLoop(ctx context.Context) { + if ctx == nil { + ctx = context.Background() + } + m.loopMu.Lock() + stop := m.loopStop + done := m.loopDone + m.loopStop = nil + m.loopDone = nil + m.loopMu.Unlock() + if stop == nil { + return + } + stop() + if done == nil { + return + } + select { + case <-done: + case <-ctx.Done(): + } +} + +func (m *scopedSandboxManager) pruneOnce(ctx context.Context) error { + if m.pruneIdleHours <= 0 && m.pruneMaxAgeDays <= 0 { + return nil + } + + regPath := filepath.Join(resolvePicoClawHomeDir(), "state", "sandbox", defaultSandboxRegistryFile) + registryMu.Lock() + data, err := loadRegistry(regPath) + registryMu.Unlock() + if err != nil { + return err + } + + pruneCfg := ContainerSandboxConfig{ + PruneIdleHours: m.pruneIdleHours, + PruneMaxAgeDays: m.pruneMaxAgeDays, + } + now := time.Now().UnixMilli() + + m.mu.Lock() + byContainer := make(map[string]Sandbox, len(m.scoped)) + for _, sb := range m.scoped { + if containerSb, ok := sb.(*ContainerSandbox); ok { + byContainer[containerSb.cfg.ContainerName] = sb + } + } + m.mu.Unlock() + + var firstErr error + for _, entry := range data.Entries { + if !shouldPruneEntry(pruneCfg, now, entry) { + continue + } + if sb, ok := byContainer[entry.ContainerName]; ok { + if err := sb.Prune(ctx); err != nil && firstErr == nil { + firstErr = err + } + continue + } + if err := stopAndRemoveContainerByName(ctx, entry.ContainerName); err != nil && firstErr == nil { + firstErr = err + } + if err := removeRegistryEntry(regPath, entry.ContainerName); err != nil && firstErr == nil { + firstErr = err + } + } + + return firstErr +} + func (m *scopedSandboxManager) Exec(ctx context.Context, req ExecRequest) (*ExecResult, error) { if !m.shouldSandbox(ctx) { return m.host.Exec(ctx, req) diff --git a/pkg/agent/sandbox/factory_additional_test.go b/pkg/agent/sandbox/factory_additional_test.go index 87d3c1b67..601ce2ec2 100644 --- a/pkg/agent/sandbox/factory_additional_test.go +++ b/pkg/agent/sandbox/factory_additional_test.go @@ -2,7 +2,10 @@ package sandbox import ( "context" + "os" + "path/filepath" "testing" + "time" "github.com/sipeed/picoclaw/pkg/config" ) @@ -30,8 +33,8 @@ func TestNewFromConfig_HostMode(t *testing.T) { if _, ok := sb.(*HostSandbox); !ok { t.Fatalf("expected HostSandbox, got %T", sb) } - if err := sb.Stop(context.Background()); err != nil { - t.Fatalf("Stop() error: %v", err) + if err := sb.Prune(context.Background()); err != nil { + t.Fatalf("Prune() error: %v", err) } } @@ -50,3 +53,47 @@ func TestNewFromConfig_AllModeReturnsUnavailableWhenBlocked(t *testing.T) { t.Fatal("expected unavailable sandbox start error") } } + +func TestScopedSandboxManager_PruneLoopLifecycle(t *testing.T) { + m := &scopedSandboxManager{ + mode: "all", + pruneIdleHours: 1, + pruneMaxAgeDays: 0, + scoped: map[string]Sandbox{}, + } + + m.ensurePruneLoop() + if m.loopStop == nil || m.loopDone == nil { + t.Fatal("expected manager prune loop to start") + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + m.stopPruneLoop(ctx) + if m.loopStop != nil || m.loopDone != nil { + t.Fatal("expected manager prune loop state reset after stop") + } +} + +func TestScopedSandboxManager_PruneOnceLoadRegistryError(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + stateDir := filepath.Join(home, ".picoclaw", "state", "sandbox") + if err := os.MkdirAll(stateDir, 0o755); err != nil { + t.Fatalf("mkdir state dir: %v", err) + } + regPath := filepath.Join(stateDir, "containers.json") + if err := os.WriteFile(regPath, []byte("{not-json"), 0o644); err != nil { + t.Fatalf("write invalid registry: %v", err) + } + + m := &scopedSandboxManager{ + mode: "all", + pruneIdleHours: 1, + pruneMaxAgeDays: 0, + scoped: map[string]Sandbox{}, + } + if err := m.pruneOnce(context.Background()); err == nil { + t.Fatal("expected pruneOnce() to return registry load error") + } +} diff --git a/pkg/agent/sandbox/host.go b/pkg/agent/sandbox/host.go index cfb48d982..96d85d7ad 100644 --- a/pkg/agent/sandbox/host.go +++ b/pkg/agent/sandbox/host.go @@ -31,7 +31,7 @@ func (h *HostSandbox) Start(ctx context.Context) error { return nil } -func (h *HostSandbox) Stop(ctx context.Context) error { +func (h *HostSandbox) Prune(ctx context.Context) error { return nil } diff --git a/pkg/agent/sandbox/host_unavailable_util_test.go b/pkg/agent/sandbox/host_unavailable_util_test.go index 47570b8be..b49e7dd1c 100644 --- a/pkg/agent/sandbox/host_unavailable_util_test.go +++ b/pkg/agent/sandbox/host_unavailable_util_test.go @@ -15,8 +15,8 @@ func TestHostSandbox_StartStopFs(t *testing.T) { if err := sb.Start(context.Background()); err != nil { t.Fatalf("Start() error: %v", err) } - if err := sb.Stop(context.Background()); err != nil { - t.Fatalf("Stop() error: %v", err) + if err := sb.Prune(context.Background()); err != nil { + t.Fatalf("Prune() error: %v", err) } if sb.Fs() == nil { t.Fatal("Fs() returned nil") @@ -110,8 +110,8 @@ func TestUnavailableSandboxAndUtilHelpers(t *testing.T) { if err := sb.Start(context.Background()); err == nil { t.Fatal("expected Start() error") } - if err := sb.Stop(context.Background()); err != nil { - t.Fatalf("Stop() error: %v", err) + if err := sb.Prune(context.Background()); err != nil { + t.Fatalf("Prune() error: %v", err) } if _, err := sb.Exec(context.Background(), ExecRequest{Command: "echo hi"}); err == nil { t.Fatal("expected Exec() error") diff --git a/pkg/agent/sandbox/sandbox.go b/pkg/agent/sandbox/sandbox.go index 79594c61e..7d5cbbc3f 100644 --- a/pkg/agent/sandbox/sandbox.go +++ b/pkg/agent/sandbox/sandbox.go @@ -11,9 +11,11 @@ type Sandbox interface { // Implementations should prepare resources that are expensive to set up lazily // (for example, container client connectivity checks). Start(ctx context.Context) error - // Stop releases runtime resources acquired by Start. + // Prune performs sandbox resource reclamation. + // Implementations should release reclaimable runtime resources and remove + // sandbox artifacts (for example containers) according to their policy. // It should be safe to call multiple times. - Stop(ctx context.Context) error + Prune(ctx context.Context) error // Exec runs a command in sandbox context. // Command/Args semantics follow ExecRequest; a non-zero exit code should be // returned in ExecResult.ExitCode, while transport/runtime failures return error. diff --git a/pkg/agent/sandbox/unavailable.go b/pkg/agent/sandbox/unavailable.go index 8d9917c0e..4465c3a57 100644 --- a/pkg/agent/sandbox/unavailable.go +++ b/pkg/agent/sandbox/unavailable.go @@ -22,7 +22,7 @@ func NewUnavailableSandbox(err error) Sandbox { } func (u *unavailableSandbox) Start(ctx context.Context) error { return u.err } -func (u *unavailableSandbox) Stop(ctx context.Context) error { return nil } +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) { diff --git a/pkg/tools/cron_additional_test.go b/pkg/tools/cron_additional_test.go index d26a978f6..f502a9433 100644 --- a/pkg/tools/cron_additional_test.go +++ b/pkg/tools/cron_additional_test.go @@ -19,7 +19,7 @@ type cronStubSandbox struct { } func (s *cronStubSandbox) Start(ctx context.Context) error { return nil } -func (s *cronStubSandbox) Stop(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) Exec(ctx context.Context, req sandbox.ExecRequest) (*sandbox.ExecResult, error) { return s.ExecStream(ctx, req, nil) diff --git a/pkg/tools/disabled.go b/pkg/tools/disabled.go deleted file mode 100644 index f3a26e678..000000000 --- a/pkg/tools/disabled.go +++ /dev/null @@ -1,46 +0,0 @@ -package tools - -import ( - "context" -) - -// DisabledTool keeps a stable tool surface but always returns an error result. -type DisabledTool struct { - name string - description string - reason string -} - -func NewDisabledTool(name, description, reason string) *DisabledTool { - return &DisabledTool{ - name: name, - description: description, - reason: reason, - } -} - -func (t *DisabledTool) Name() string { - return t.name -} - -func (t *DisabledTool) Description() string { - if t.description != "" { - return t.description - } - return "This tool is disabled in current sandbox policy." -} - -func (t *DisabledTool) Parameters() map[string]interface{} { - return map[string]interface{}{ - "type": "object", - "properties": map[string]interface{}{}, - } -} - -func (t *DisabledTool) Execute(ctx context.Context, args map[string]interface{}) *ToolResult { - msg := t.reason - if msg == "" { - msg = "tool is disabled" - } - return ErrorResult(msg) -} diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index a28be4205..1efae07d4 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "os/exec" + "path" "path/filepath" "regexp" "runtime" @@ -154,9 +155,16 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult if t.restrictToWorkspace && t.workingDir != "" { resolvedWD, err := validatePath(wd, t.workingDir, true) if err != nil { - return ErrorResult("Command blocked by safety guard (" + err.Error() + ")") + // In sandbox mode, allow explicit container workspace paths when + // restrict_to_workspace is enabled. + if t.sandbox != nil && filepath.IsAbs(wd) && isSandboxWorkspaceAbsolutePath(wd) { + cwd = wd + } else { + return ErrorResult("Command blocked by safety guard (" + err.Error() + ")") + } + } else { + cwd = resolvedWD } - cwd = resolvedWD } else { cwd = wd } @@ -390,7 +398,14 @@ func (t *ExecTool) resolveSandboxWorkingDir(cwd string) string { } } } - return "." + // Preserve explicit absolute paths in sandbox mode (e.g. /tmp/logs), + // instead of silently downgrading to ".". + return filepath.ToSlash(trimmed) +} + +func isSandboxWorkspaceAbsolutePath(wd string) bool { + clean := path.Clean(filepath.ToSlash(strings.TrimSpace(wd))) + return clean == "/workspace" || strings.HasPrefix(clean, "/workspace/") } func (t *ExecTool) SetTimeout(timeout time.Duration) { diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 9df6653e5..2be8f21a4 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -19,7 +19,7 @@ type stubSandbox struct { } func (s *stubSandbox) Start(ctx context.Context) error { return nil } -func (s *stubSandbox) Stop(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) Exec(ctx context.Context, req sandbox.ExecRequest) (*sandbox.ExecResult, error) { return sandboxAggregateFromStub(ctx, req, s.ExecStream) @@ -370,7 +370,7 @@ func TestShellTool_SandboxMapsHostWorkingDirToRelative(t *testing.T) { } } -func TestShellTool_SandboxUsesDotForUnmappedAbsoluteDir(t *testing.T) { +func TestShellTool_SandboxAllowsAbsoluteWorkspaceWorkingDir(t *testing.T) { workspace := t.TempDir() sb := &stubSandbox{} tool := NewExecToolWithSandbox(workspace, true, nil, sb) @@ -378,14 +378,33 @@ func TestShellTool_SandboxUsesDotForUnmappedAbsoluteDir(t *testing.T) { ctx := context.Background() args := map[string]interface{}{ "command": "echo test", - "working_dir": "/outside/path", + "working_dir": "/workspace/subdir", } result := tool.Execute(ctx, args) if result.IsError { t.Fatalf("expected success, got error: %s", result.ForLLM) } - if sb.lastReq.WorkingDir != "." { - t.Fatalf("sandbox working_dir = %q, want .", sb.lastReq.WorkingDir) + if sb.lastReq.WorkingDir != "/workspace/subdir" { + t.Fatalf("sandbox working_dir = %q, want /workspace/subdir", sb.lastReq.WorkingDir) + } +} + +func TestShellTool_SandboxBlocksAbsoluteNonWorkspaceWorkingDirWhenRestricted(t *testing.T) { + workspace := t.TempDir() + sb := &stubSandbox{} + tool := NewExecToolWithSandbox(workspace, true, nil, sb) + + ctx := context.Background() + args := map[string]interface{}{ + "command": "echo test", + "working_dir": "/tmp/logs", + } + result := tool.Execute(ctx, args) + if !result.IsError { + t.Fatalf("expected error for /tmp/logs with restrict_to_workspace=true, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "blocked") { + t.Fatalf("expected blocked error, got: %s", result.ForLLM) } }