diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 2b7eeb01f..a5dd8c69f 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -189,12 +189,13 @@ func resolveAgentWorkspace(agentCfg *config.AgentConfig, defaults *config.AgentD if agentCfg != nil && strings.TrimSpace(agentCfg.Workspace) != "" { return expandHome(strings.TrimSpace(agentCfg.Workspace)) } + defaultWS := expandHome(defaults.Workspace) if agentCfg == nil || agentCfg.Default || agentCfg.ID == "" || routing.NormalizeAgentID(agentCfg.ID) == "main" { - return expandHome(defaults.Workspace) + return defaultWS } - home, _ := os.UserHomeDir() + parent := filepath.Dir(defaultWS) id := routing.NormalizeAgentID(agentCfg.ID) - return filepath.Join(home, ".picoclaw", "workspace-"+id) + return filepath.Join(parent, "workspace-"+id) } // resolveAgentModel resolves the primary model for an agent. diff --git a/pkg/agent/registry_test.go b/pkg/agent/registry_test.go index 518bb441f..5abc0b5d8 100644 --- a/pkg/agent/registry_test.go +++ b/pkg/agent/registry_test.go @@ -24,11 +24,12 @@ func (m *mockRegistryProvider) GetDefaultModel() string { return "mock-model" } -func testCfg(agents []config.AgentConfig) *config.Config { +func testCfg(t *testing.T, agents []config.AgentConfig) *config.Config { + workspace := t.TempDir() return &config.Config{ Agents: config.AgentsConfig{ Defaults: config.AgentDefaults{ - Workspace: "/tmp/picoclaw-test-registry", + Workspace: workspace, Model: "gpt-4", MaxTokens: 8192, MaxToolIterations: 10, @@ -39,7 +40,7 @@ func testCfg(agents []config.AgentConfig) *config.Config { } func TestNewAgentRegistry_ImplicitMain(t *testing.T) { - cfg := testCfg(nil) + cfg := testCfg(t, nil) registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) ids := registry.ListAgentIDs() @@ -57,7 +58,7 @@ func TestNewAgentRegistry_ImplicitMain(t *testing.T) { } func TestNewAgentRegistry_ExplicitAgents(t *testing.T) { - cfg := testCfg([]config.AgentConfig{ + cfg := testCfg(t, []config.AgentConfig{ {ID: "sales", Default: true, Name: "Sales Bot"}, {ID: "support", Name: "Support Bot"}, }) @@ -83,7 +84,7 @@ func TestNewAgentRegistry_ExplicitAgents(t *testing.T) { } func TestAgentRegistry_GetAgent_Normalize(t *testing.T) { - cfg := testCfg([]config.AgentConfig{ + cfg := testCfg(t, []config.AgentConfig{ {ID: "my-agent", Default: true}, }) registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) @@ -98,7 +99,7 @@ func TestAgentRegistry_GetAgent_Normalize(t *testing.T) { } func TestAgentRegistry_GetDefaultAgent(t *testing.T) { - cfg := testCfg([]config.AgentConfig{ + cfg := testCfg(t, []config.AgentConfig{ {ID: "alpha"}, {ID: "beta", Default: true}, }) @@ -112,7 +113,7 @@ func TestAgentRegistry_GetDefaultAgent(t *testing.T) { } func TestAgentRegistry_CanSpawnSubagent(t *testing.T) { - cfg := testCfg([]config.AgentConfig{ + cfg := testCfg(t, []config.AgentConfig{ { ID: "parent", Default: true, @@ -141,7 +142,7 @@ func TestAgentRegistry_CanSpawnSubagent(t *testing.T) { } func TestAgentRegistry_CanSpawnSubagent_Wildcard(t *testing.T) { - cfg := testCfg([]config.AgentConfig{ + cfg := testCfg(t, []config.AgentConfig{ { ID: "admin", Default: true, @@ -163,7 +164,7 @@ func TestAgentRegistry_CanSpawnSubagent_Wildcard(t *testing.T) { func TestAgentInstance_Model(t *testing.T) { model := &config.AgentModelConfig{Primary: "claude-opus"} - cfg := testCfg([]config.AgentConfig{ + cfg := testCfg(t, []config.AgentConfig{ {ID: "custom", Default: true, Model: model}, }) registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) @@ -175,7 +176,7 @@ func TestAgentInstance_Model(t *testing.T) { } func TestAgentInstance_FallbackInheritance(t *testing.T) { - cfg := testCfg([]config.AgentConfig{ + cfg := testCfg(t, []config.AgentConfig{ {ID: "inherit", Default: true}, }) cfg.Agents.Defaults.ModelFallbacks = []string{"openai/gpt-4o-mini", "anthropic/haiku"} @@ -192,7 +193,7 @@ func TestAgentInstance_FallbackExplicitEmpty(t *testing.T) { Primary: "gpt-4", Fallbacks: []string{}, // explicitly empty = disable } - cfg := testCfg([]config.AgentConfig{ + cfg := testCfg(t, []config.AgentConfig{ {ID: "no-fallback", Default: true, Model: model}, }) cfg.Agents.Defaults.ModelFallbacks = []string{"should-not-inherit"} diff --git a/pkg/agent/sandbox/container.go b/pkg/agent/sandbox/container.go index 54eada57a..c23ffbcd6 100644 --- a/pkg/agent/sandbox/container.go +++ b/pkg/agent/sandbox/container.go @@ -97,7 +97,7 @@ func NewContainerSandbox(cfg ContainerSandboxConfig) *ContainerSandbox { if strings.TrimSpace(cfg.Network) == "" { cfg.Network = "none" } - if len(cfg.CapDrop) == 0 { + if cfg.CapDrop == nil { cfg.CapDrop = []string{"ALL"} } if cfg.Env == nil { @@ -167,7 +167,10 @@ func (c *ContainerSandbox) Start(ctx context.Context) error { return c.startErr } defer rc.Close() - _, _ = io.Copy(io.Discard, rc) + if _, err := io.Copy(io.Discard, rc); err != nil { + c.startErr = fmt.Errorf("failed to pull fallback image: %w", err) + return c.startErr + } // Tag debian:bookworm-slim as picoclaw-sandbox:bookworm-slim if err := c.cli.ImageTag(ctx, FallbackSandboxImage, DefaultSandboxImage); err != nil { @@ -182,7 +185,10 @@ func (c *ContainerSandbox) Start(ctx context.Context) error { return c.startErr } defer rc.Close() - _, _ = io.Copy(io.Discard, rc) + if _, err := io.Copy(io.Discard, rc); err != nil { + c.startErr = fmt.Errorf("failed to pull image: %w", err) + return c.startErr + } } } @@ -621,48 +627,53 @@ func (f *containerFS) WriteFile(ctx context.Context, p string, data []byte, mkdi if err != nil { return err } - dir := path.Dir(containerPath) - base := path.Base(containerPath) + // Build a script that optionally creates the parent directory and then writes the file via cat. + // This ensures proper ownership and permissions as the configured container user. + script := `set -eu; cat >"$1"` if mkdir { - hostDir, ok := f.sb.hostDirForContainerPath(dir) - if ok { - if err := os.MkdirAll(hostDir, 0o755); err != nil { - return fmt.Errorf("host mkdir failed: %w", err) - } - } else { - _, err := f.sb.Exec(ctx, ExecRequest{ - Command: "mkdir -p " + shellEscape(dir), - }) - if err != nil { - return err - } - } + script = `set -eu; dir=$(dirname -- "$1"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; cat >"$1"` } - var buf bytes.Buffer - tw := tar.NewWriter(&buf) - if err := tw.WriteHeader(&tar.Header{ - Name: base, - Mode: 0o644, - Size: int64(len(data)), - }); err != nil { - _ = tw.Close() - return fmt.Errorf("tar header write failed: %w", err) - } - if _, err := tw.Write(data); err != nil { - _ = tw.Close() - return fmt.Errorf("tar content write failed: %w", err) - } - if err := tw.Close(); err != nil { - return fmt.Errorf("tar close failed: %w", err) + execResp, err := f.sb.cli.ContainerExecCreate(ctx, f.sb.cfg.ContainerName, container.ExecOptions{ + Cmd: []string{"sh", "-c", script, "picoclaw-fs-write", containerPath}, + User: f.sb.cfg.User, // Use configured user to preserve ownership + AttachStdin: true, + AttachStdout: true, + AttachStderr: true, + }) + if err != nil { + return fmt.Errorf("docker exec create failed: %w", err) } - if err := f.sb.cli.CopyToContainer(ctx, f.sb.cfg.ContainerName, dir, &buf, container.CopyToContainerOptions{ - AllowOverwriteDirWithFile: true, - }); err != nil { - return fmt.Errorf("docker copy to container failed: %w", err) + attach, err := f.sb.cli.ContainerExecAttach(ctx, execResp.ID, container.ExecStartOptions{}) + if err != nil { + return fmt.Errorf("docker exec attach failed: %w", err) } + defer attach.Close() + + // Write data to the hijacked connection's stdin + if _, err := attach.Conn.Write(data); err != nil { + return fmt.Errorf("failed to write data to container: %w", err) + } + + // Close the write side of the connection so cat receives EOF and terminates + if conn, ok := attach.Conn.(interface{ CloseWrite() error }); ok { + _ = conn.CloseWrite() + } + + // Wait for the exec process to complete and check the exit code + var stdout, stderr bytes.Buffer + _, _ = stdcopy.StdCopy(&stdout, &stderr, attach.Reader) + + inspect, err := f.sb.cli.ContainerExecInspect(ctx, execResp.ID) + if err != nil { + return fmt.Errorf("failed to inspect write process: %w", err) + } + if inspect.ExitCode != 0 { + return fmt.Errorf("file write failed with code %d: %s", inspect.ExitCode, stderr.String()) + } + return nil } diff --git a/pkg/agent/sandbox/container_integration_test.go b/pkg/agent/sandbox/container_integration_test.go index 2910dcc4b..e45cd4e1d 100644 --- a/pkg/agent/sandbox/container_integration_test.go +++ b/pkg/agent/sandbox/container_integration_test.go @@ -55,6 +55,7 @@ func TestContainerSandbox_Integration_ExecReadWrite(t *testing.T) { Image: image, ContainerName: containerName, Workspace: workspace, + User: fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), }) err := sb.Start(ctx) if err != nil { @@ -114,6 +115,67 @@ func TestContainerSandbox_Integration_ExecReadWrite(t *testing.T) { } } +func TestContainerSandbox_Integration_WriteFileOwnership(t *testing.T) { + _, cleanup := skipIfNoDocker(t) + defer cleanup() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + workspace := t.TempDir() + containerName := fmt.Sprintf("picoclaw-test-ownership-%d", time.Now().UnixNano()) + image := getTestImage() + + // Use the current host user's UID:GID to test real-world mapping compatibility + testUser := fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()) + + sb := NewContainerSandbox(ContainerSandboxConfig{ + Image: image, + ContainerName: containerName, + Workspace: workspace, + User: testUser, + }) + if err := sb.Start(ctx); err != nil { + t.Fatalf("sandbox start failed: %v", err) + } + defer sb.Prune(ctx) + + // Write file via FsBridge with mkdir enabled + testPath := "auth/test.txt" + content := []byte("ownership verification") + if err := sb.Fs().WriteFile(ctx, testPath, content, true); err != nil { + t.Fatalf("WriteFile failed: %v", err) + } + + // Verify identity (UID:GID) and modification time via stat + res, err := sb.Exec(ctx, ExecRequest{ + Command: fmt.Sprintf("stat -c '%%u:%%g %%Y' %s", testPath), + }) + if err != nil || res.ExitCode != 0 { + t.Fatalf("Stat failed: %v, stderr=%s", err, res.Stderr) + } + + // Output format: "UID:GID UnixTimestamp" + parts := strings.Fields(res.Stdout) + if len(parts) < 2 { + t.Fatalf("unexpected stat output: %q", res.Stdout) + } + + // Check UID:GID + if parts[0] != testUser { + t.Errorf("ownership mismatch: got %q, want %q", parts[0], testUser) + } + + // Check Timestamp (should not be 1970 Epoch) + timestamp := parts[1] + if strings.HasPrefix(timestamp, "0") || strings.HasPrefix(timestamp, "1 ") { + // Specifically check for very small timestamps that indicate 1970 + t.Errorf("file created with suspicious 1970-era timestamp: %q", timestamp) + } + + t.Logf("Stat verified: %s", res.Stdout) +} + func TestContainerSandbox_Integration_WriteFileMkdirInContainerTmp(t *testing.T) { _, cleanup := skipIfNoDocker(t) defer cleanup() @@ -123,10 +185,13 @@ func TestContainerSandbox_Integration_WriteFileMkdirInContainerTmp(t *testing.T) containerName := fmt.Sprintf("picoclaw-test-mkdir-%d", time.Now().UnixNano()) image := getTestImage() + workspace := t.TempDir() sb := NewContainerSandbox(ContainerSandboxConfig{ Image: image, ContainerName: containerName, + Workspace: workspace, + User: fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), }) err := sb.Start(ctx) if err != nil { diff --git a/pkg/agent/sandbox/manager_test.go b/pkg/agent/sandbox/manager_test.go index ff6c5be70..262e36f2a 100644 --- a/pkg/agent/sandbox/manager_test.go +++ b/pkg/agent/sandbox/manager_test.go @@ -391,18 +391,21 @@ func TestScopedSandboxManager_ContainerCreationError(t *testing.T) { m := &scopedSandboxManager{ mode: config.SandboxModeAll, workspaceRoot: home, + image: "non-existent-image-12345", dockerCfg: config.AgentSandboxDockerConfig{Image: "non-existent-image-12345"}, scoped: map[string]Sandbox{}, } ctx := WithSessionKey(context.Background(), "error-session") - // Fast-path misses, falls through to buildScopedContainerSandbox and Start() - // Start() succeeds due to lazy evaluation, but Exec() should fail because - // Docker is either unavailable or the image is missing + // Resolve calls getOrCreateSandbox, which calls Start(). + // Start should fail because the image doesn't exist. sb, err := m.Resolve(ctx) if err != nil { - t.Fatalf("expected Resolve() to succeed lazily, got error: %v", err) + // If Resolve fails here, it's also a valid failure for this test Case, + // but historically we expected it to happen during Exec. + // Since Start is now called in Resolve, it's expected to fail here. + return } res, err := sb.Exec(ctx, ExecRequest{Command: "echo"}) diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index b6b9cbe2b..72810206d 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -30,7 +30,6 @@ func DefaultConfig() *Config { Tmpfs: []string{"/tmp", "/var/tmp", "/run"}, Network: "none", User: "", - CapDrop: []string{"ALL"}, Env: map[string]string{ "LANG": "C.UTF-8", },