feat: expand default sandbox tool permissions and sanitize container environment variables

pkg/agent/sandbox/container.go

@@ -103,6 +103,10 @@ func NewContainerSandbox(cfg ContainerSandboxConfig) *ContainerSandbox {
 	if cfg.Env == nil {
 		cfg.Env = map[string]string{"LANG": "C.UTF-8"}
 	}
+	cfg.Env = sanitizeEnvVars(cfg.Env)
+	if len(cfg.Env) == 0 {
+		cfg.Env = map[string]string{"LANG": "C.UTF-8"}
+	}
 	cfg.WorkspaceAccess = string(normalizeWorkspaceAccess(config.WorkspaceAccess(cfg.WorkspaceAccess)))
 	cfg.WorkspaceRoot = strings.TrimSpace(cfg.WorkspaceRoot)
 	sb := &ContainerSandbox{cfg: cfg}

pkg/agent/sandbox/container_test.go

@@ -536,3 +536,25 @@ func TestContainerSandbox_StopWithoutClient(t *testing.T) {
 		t.Fatalf("Prune() error: %v", err)
 	}
 }
+
+func TestNewContainerSandbox_SanitizesEnv(t *testing.T) {
+	sb := NewContainerSandbox(ContainerSandboxConfig{
+		Env: map[string]string{
+			"LANG":           "C.UTF-8",
+			"OPENAI_API_KEY": "secret",
+			"SAFE_NAME":      "ok",
+		},
+	})
+
+	got := sb.containerEnv()
+	joined := strings.Join(got, "\n")
+	if strings.Contains(joined, "OPENAI_API_KEY=") {
+		t.Fatalf("sensitive env key should be filtered, got: %v", got)
+	}
+	if !strings.Contains(joined, "SAFE_NAME=ok") {
+		t.Fatalf("safe env key should be preserved, got: %v", got)
+	}
+	if !strings.Contains(joined, "LANG=C.UTF-8") {
+		t.Fatalf("LANG should be preserved or defaulted, got: %v", got)
+	}
+}

pkg/agent/sandbox/host.go

@@ -13,6 +13,8 @@ import (
 	"strings"
 	"sync"
 	"time"
+
+	"github.com/sipeed/picoclaw/pkg/fileutil"
 )
 
 type HostSandbox struct {
@@ -239,22 +241,19 @@ func (h *hostFS) WriteFile(ctx context.Context, path string, data []byte, mkdir
 		if err != nil {
 			return err
 		}
-		if mkdir {
+
-			if err := os.MkdirAll(filepath.Dir(resolved), 0o755); err != nil {
+		parent := filepath.Dir(resolved)
+		if !mkdir {
+			parentInfo, err := os.Stat(parent)
+			if err != nil {
 				return err
 			}
+			if !parentInfo.IsDir() {
+				return fmt.Errorf("parent path is not a directory: %s", parent)
 			}
-		// 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 fileutil.WriteFileAtomic(resolved, data, 0o644)
-			return fmt.Errorf("failed to replace original file: %w", err)
-		}
-		return nil
 	}
 
 	relPath, err := h.getSafeRelPath(path)
@@ -268,16 +267,54 @@ func (h *hostFS) WriteFile(ctx context.Context, path string, data []byte, mkdir
 			return err
 		}
 	}
-	// Atomic write within os.Root: write to temp file then rename.
+	return writeFileAtomicInRoot(h.root, relPath, data)
-	tmpRelPath := fmt.Sprintf("%s.%d.tmp", relPath, time.Now().UnixNano())
+}
-	if err := h.root.WriteFile(tmpRelPath, data, 0o644); err != nil {
+
-		h.root.Remove(tmpRelPath)
+func writeFileAtomicInRoot(root *os.Root, relPath string, data []byte) error {
+	dir := filepath.Dir(relPath)
+	tmpName := fmt.Sprintf(".tmp-%d-%d", os.Getpid(), time.Now().UnixNano())
+	tmpRelPath := tmpName
+	if dir != "." && dir != "/" {
+		tmpRelPath = filepath.Join(dir, tmpName)
+	}
+
+	tmpFile, err := root.OpenFile(tmpRelPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644)
+	if err != nil {
+		_ = root.Remove(tmpRelPath)
+		return fmt.Errorf("failed to open temp file: %w", err)
+	}
+
+	if _, err := tmpFile.Write(data); err != nil {
+		_ = tmpFile.Close()
+		_ = 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)
+	if err := tmpFile.Sync(); err != nil {
+		_ = tmpFile.Close()
+		_ = root.Remove(tmpRelPath)
+		return fmt.Errorf("failed to sync temp file: %w", err)
+	}
+
+	if err := tmpFile.Close(); err != nil {
+		_ = root.Remove(tmpRelPath)
+		return fmt.Errorf("failed to close 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)
 	}
+
+	syncDir := "."
+	if dir != "" && dir != "/" {
+		syncDir = dir
+	}
+	if dirFile, err := root.Open(syncDir); err == nil {
+		_ = dirFile.Sync()
+		_ = dirFile.Close()
+	}
+
 	return nil
 }
 

pkg/agent/sandbox/host_test.go

@@ -233,6 +233,16 @@ func TestHostFS_ReadFileWriteFile_Unrestricted(t *testing.T) {
 	}
 }
 
+func TestHostFS_WriteFile_Unrestricted_NoMkdirMissingParent(t *testing.T) {
+	root := t.TempDir()
+	sb := NewHostSandbox(root, false)
+
+	err := sb.Fs().WriteFile(context.Background(), "missing/parent/file.txt", []byte("x"), false)
+	if err == nil {
+		t.Fatalf("expected WriteFile to fail when parent directory is missing and mkdir=false")
+	}
+}
+
 func TestHostFS_WriteFileMKdir(t *testing.T) {
 	root := t.TempDir()
 	sb := NewHostSandbox(root, true)
@@ -352,6 +362,24 @@ func TestValidatePathErrors(t *testing.T) {
 		t.Fatalf("expected err for empty workspace with restrict=true")
 	}
 
+	absTarget := filepath.Join(t.TempDir(), "abs.txt")
+	got, err := ValidatePath(absTarget, "", false)
+	if err != nil {
+		t.Fatalf("expected unrestricted empty-workspace absolute path to pass, got: %v", err)
+	}
+	if got != absTarget {
+		t.Fatalf("ValidatePath(abs, empty, false) = %q, want %q", got, absTarget)
+	}
+
+	relTarget := "rel.txt"
+	got, err = ValidatePath(relTarget, "", false)
+	if err != nil {
+		t.Fatalf("expected unrestricted empty-workspace relative path to pass, got: %v", err)
+	}
+	if !filepath.IsAbs(got) {
+		t.Fatalf("ValidatePath(rel, empty, false) should return abs path, got %q", got)
+	}
+
 	root := t.TempDir()
 
 	// target parent is file, evalSymlinks should fail

pkg/agent/sandbox/manager.go

@@ -287,7 +287,7 @@ func (m *scopedSandboxManager) pruneOnce(ctx context.Context) error {
 	if m.pruneIdleHours <= 0 && m.pruneMaxAgeDays <= 0 {
 		return nil
 	}
-	regPath := filepath.Join(infra.ResolveHomeDir(), "sandbox", defaultSandboxRegistryFile)
+	regPath := filepath.Join(infra.ResolveHomeDir(), "sandboxes", defaultSandboxRegistryFile)
 	registryMu.Lock()
 	data, err := loadRegistry(regPath)
 	registryMu.Unlock()

pkg/agent/sandbox/manager_test.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/sipeed/picoclaw/pkg/config"
+	"github.com/sipeed/picoclaw/pkg/routing"
 )
 
 func TestNormalizeWorkspaceAccess(t *testing.T) {
@@ -94,7 +95,7 @@ func TestScopedSandboxManager_PruneLoopLifecycle(t *testing.T) {
 func TestScopedSandboxManager_PruneOnceLoadRegistryError(t *testing.T) {
 	home := t.TempDir()
 	t.Setenv("HOME", home)
-	stateDir := filepath.Join(home, ".picoclaw", "sandbox")
+	stateDir := filepath.Join(home, ".picoclaw", "sandboxes")
 	if err := os.MkdirAll(stateDir, 0o755); err != nil {
 		t.Fatalf("mkdir state dir: %v", err)
 	}
@@ -113,3 +114,28 @@ func TestScopedSandboxManager_PruneOnceLoadRegistryError(t *testing.T) {
 		t.Fatal("expected pruneOnce() to return registry load error")
 	}
 }
+
+func TestScopedSandboxManager_ShouldSandbox_NonMain(t *testing.T) {
+	m := &scopedSandboxManager{
+		mode:    config.SandboxModeNonMain,
+		agentID: "default",
+	}
+
+	if m.shouldSandbox(context.Background()) {
+		t.Fatal("expected background context to map to main session (host path)")
+	}
+
+	if m.shouldSandbox(WithSessionKey(context.Background(), "main")) {
+		t.Fatal("expected explicit main alias to remain host path")
+	}
+
+	mainKey := routing.BuildAgentMainSessionKey("default")
+	if m.shouldSandbox(WithSessionKey(context.Background(), mainKey)) {
+		t.Fatal("expected agent main session key to remain host path")
+	}
+
+	nonMainKey := "agent:default:direct:user-1"
+	if !m.shouldSandbox(WithSessionKey(context.Background(), nonMainKey)) {
+		t.Fatal("expected non-main session to use sandbox path")
+	}
+}

pkg/agent/sandbox/path.go

@@ -9,8 +9,18 @@ import (
 // ValidatePath ensures the given path is within the workspace if restrict is true.
 func ValidatePath(path, workspace string, restrict bool) (string, error) {
 	if workspace == "" {
+		if restrict {
 			return "", fmt.Errorf("workspace is not defined")
 		}
+		if filepath.IsAbs(path) {
+			return filepath.Clean(path), nil
+		}
+		absPath, err := filepath.Abs(path)
+		if err != nil {
+			return "", fmt.Errorf("failed to resolve file path: %w", err)
+		}
+		return absPath, nil
+	}
 
 	absWorkspace, err := filepath.Abs(workspace)
 	if err != nil {

pkg/agent/sandbox/registry.go

@@ -10,6 +10,8 @@ import (
 	"path/filepath"
 	"sync"
 	"time"
+
+	"github.com/sipeed/picoclaw/pkg/fileutil"
 )
 
 type registryEntry struct {
@@ -89,22 +91,11 @@ func loadRegistry(path string) (*registryData, error) {
 }
 
 func saveRegistry(path string, data *registryData) error {
-	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
-		return err
-	}
 	raw, err := json.MarshalIndent(data, "", "  ")
 	if err != nil {
 		return err
 	}
-	tmp := fmt.Sprintf("%s.%d.tmp", path, time.Now().UnixNano())
+	return fileutil.WriteFileAtomic(path, append(raw, '\n'), 0o644)
-	if err := os.WriteFile(tmp, append(raw, '\n'), 0o644); err != nil {
-		return err
-	}
-	if err := os.Rename(tmp, path); err != nil {
-		_ = os.Remove(tmp)
-		return err
-	}
-	return nil
 }
 
 func upsertRegistryEntry(path string, entry registryEntry) error {

pkg/agent/sandbox/tool_policy.go

@@ -6,7 +6,7 @@ import (
 	"github.com/sipeed/picoclaw/pkg/config"
 )
 
-var defaultSandboxAllow = []string{"exec", "read_file", "write_file"}
+var defaultSandboxAllow = []string{"exec", "read_file", "write_file", "list_dir", "edit_file", "append_file"}
 
 func IsToolSandboxEnabled(cfg *config.Config, tool string) bool {
 	name := strings.ToLower(strings.TrimSpace(tool))
@@ -14,26 +14,19 @@ func IsToolSandboxEnabled(cfg *config.Config, tool string) bool {
 		return false
 	}
 
-	allow, hasAllow := defaultSandboxAllow, false
+	allow := defaultSandboxAllow
 	deny := []string{}
 	if cfg != nil {
 		allow = cfg.Tools.Sandbox.Tools.Allow
 		deny = cfg.Tools.Sandbox.Tools.Deny
-		hasAllow = cfg.Tools.Sandbox.Tools.Allow != nil
 	}
 
 	if containsTool(deny, name) {
 		return false
 	}
-	if !hasAllow {
-		// No allow list configured: use the built-in default set.
-		return containsTool(defaultSandboxAllow, name)
-	}
 	if len(allow) == 0 {
-		// Explicit empty allow list means "deny all" — no tool gets
+		// Empty allow list falls back to built-in defaults.
-		// sandbox routing. This is the intuitive interpretation: an empty
+		allow = defaultSandboxAllow
-		// allowlist blocks everything (principle of least privilege).
-		return false
 	}
 	return containsTool(allow, name)
 }

pkg/agent/sandbox/tool_policy_test.go

@@ -10,8 +10,8 @@ func TestIsToolSandboxEnabled_Default(t *testing.T) {
 	if !IsToolSandboxEnabled(nil, "exec") {
 		t.Fatal("expected exec to be sandbox-enabled by default")
 	}
-	if IsToolSandboxEnabled(nil, "list_dir") {
+	if !IsToolSandboxEnabled(nil, "list_dir") {
-		t.Fatal("expected list_dir to be host by default")
+		t.Fatal("expected list_dir to be sandbox-enabled by default")
 	}
 }
 
@@ -31,21 +31,19 @@ func TestIsToolSandboxEnabled_AllowDeny(t *testing.T) {
 	}
 }
 
-// TestIsToolSandboxEnabled_EmptyAllowDeniesAll verifies BOUNDARY-1 fix:
+// TestIsToolSandboxEnabled_EmptyAllowUsesDefault verifies that an explicitly
-// an explicitly empty allow list now means "deny all tools" (principle of least privilege).
+// empty allow list falls back to built-in defaults.
-func TestIsToolSandboxEnabled_EmptyAllowDeniesAll(t *testing.T) {
+func TestIsToolSandboxEnabled_EmptyAllowUsesDefault(t *testing.T) {
 	cfg := config.DefaultConfig()
 	cfg.Tools.Sandbox.Tools.Allow = []string{}
 	cfg.Tools.Sandbox.Tools.Deny = []string{"cron"}
 
-	// Empty explicit allow should now deny everything (including read_file which was previously allowed)
+	for _, tool := range []string{"exec", "read_file", "write_file", "list_dir", "edit_file", "append_file"} {
-	if IsToolSandboxEnabled(cfg, "read_file") {
+		if !IsToolSandboxEnabled(cfg, tool) {
-		t.Fatal("expected read_file to be DISABLED when allow list is explicitly empty (deny all)")
+			t.Fatalf("expected %s to use default allow list when allow is empty", tool)
 		}
-	if IsToolSandboxEnabled(cfg, "exec") {
-		t.Fatal("expected exec to be DISABLED when allow list is explicitly empty (deny all)")
 	}
-	// Deny list still applies (as a belt-and-suspenders check)
+
 	if IsToolSandboxEnabled(cfg, "cron") {
 		t.Fatal("expected denied tool to be disabled")
 	}

pkg/config/config_test.go

@@ -377,6 +377,18 @@ func TestDefaultConfig_SandboxTools(t *testing.T) {
 	if len(cfg.Tools.Sandbox.Tools.Allow) == 0 {
 		t.Fatal("Expected sandbox allow tools to be configured")
 	}
+	for _, tool := range []string{"exec", "read_file", "write_file", "list_dir", "edit_file", "append_file"} {
+		found := false
+		for _, v := range cfg.Tools.Sandbox.Tools.Allow {
+			if v == tool {
+				found = true
+				break
+			}
+		}
+		if !found {
+			t.Fatalf("Expected sandbox allow tools to include %q, got %v", tool, cfg.Tools.Sandbox.Tools.Allow)
+		}
+	}
 	if cfg.Agents.Defaults.Sandbox.Prune.IdleHours == nil || *cfg.Agents.Defaults.Sandbox.Prune.IdleHours <= 0 {
 		t.Fatal("Expected sandbox prune idle hours > 0")
 	}

pkg/config/defaults.go

@@ -370,7 +370,7 @@ func DefaultConfig() *Config {
 			},
 			Sandbox: SandboxToolsConfig{
 				Tools: SandboxToolPolicyConfig{
-					Allow: []string{"exec", "read_file", "write_file"},
+					Allow: []string{"exec", "read_file", "write_file", "list_dir", "edit_file", "append_file"},
 					Deny:  []string{"cron"},
 				},
 			},

pkg/session/manager.go

@@ -8,6 +8,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/sipeed/picoclaw/pkg/fileutil"
 	"github.com/sipeed/picoclaw/pkg/providers"
 )
 
@@ -197,40 +198,7 @@ func (sm *SessionManager) Save(key string) error {
 	}
 
 	sessionPath := filepath.Join(sm.storage, filename+".json")
-	tmpFile, err := os.CreateTemp(sm.storage, "session-*.tmp")
+	return fileutil.WriteFileAtomic(sessionPath, data, 0o644)
-	if err != nil {
-		return err
-	}
-
-	tmpPath := tmpFile.Name()
-	cleanup := true
-	defer func() {
-		if cleanup {
-			_ = os.Remove(tmpPath)
-		}
-	}()
-
-	if _, err := tmpFile.Write(data); err != nil {
-		_ = tmpFile.Close()
-		return err
-	}
-	if err := tmpFile.Chmod(0o644); err != nil {
-		_ = tmpFile.Close()
-		return err
-	}
-	if err := tmpFile.Sync(); err != nil {
-		_ = tmpFile.Close()
-		return err
-	}
-	if err := tmpFile.Close(); err != nil {
-		return err
-	}
-
-	if err := os.Rename(tmpPath, sessionPath); err != nil {
-		return err
-	}
-	cleanup = false
-	return nil
 }
 
 func (sm *SessionManager) loadSessions() error {

pkg/tools/filesystem_test.go

@@ -318,7 +318,7 @@ func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) {
 	os.WriteFile(secretFile, []byte("secret data"), 0o600)
 
 	result := tool.Execute(sandbox.WithSandbox(context.Background(), &stubSandbox{
-		err: fmt.Errorf("workspace is not defined"),
+		fs: sandbox.NewHostSandbox("", true).Fs(),
 	}), map[string]any{
 		"path": secretFile,
 	})
@@ -330,6 +330,25 @@ func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) {
 	assert.Contains(t, result.ForLLM, "workspace is not defined", "Expected 'workspace is not defined' error")
 }
 
+func TestFilesystemTool_EmptyWorkspace_UnrestrictedAllowed(t *testing.T) {
+	tool := NewReadFileTool("", false) // restrict=false and workspace=""
+
+	tmpDir := t.TempDir()
+	secretFile := filepath.Join(tmpDir, "public.txt")
+	if err := os.WriteFile(secretFile, []byte("public data"), 0o644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	result := tool.Execute(sandbox.WithSandbox(context.Background(), &stubSandbox{
+		fs: sandbox.NewHostSandbox("", false).Fs(),
+	}), map[string]any{
+		"path": secretFile,
+	})
+
+	assert.False(t, result.IsError, "Expected unrestricted empty-workspace read to succeed, got: %s", result.ForLLM)
+	assert.Contains(t, result.ForLLM, "public data")
+}
+
 // TestRootMkdirAll verifies that root.MkdirAll (used by atomicWriteFileInRoot) handles all cases:
 // single dir, deeply nested dirs, already-existing dirs, and a file blocking a directory path.
 func TestRootMkdirAll(t *testing.T) {