feat: reimplement sandbox file writing using docker exec to ensure correct ownership and permissions, refine agent workspace resolution, and adjust default container capabilities.

This commit is contained in:
0x5487 2026-03-01 21:33:44 +08:00
parent ac52547f2b
commit 21e9651ca5
6 changed files with 137 additions and 57 deletions

View file

@ -189,12 +189,13 @@ func resolveAgentWorkspace(agentCfg *config.AgentConfig, defaults *config.AgentD
if agentCfg != nil && strings.TrimSpace(agentCfg.Workspace) != "" { if agentCfg != nil && strings.TrimSpace(agentCfg.Workspace) != "" {
return expandHome(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" { 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) 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. // resolveAgentModel resolves the primary model for an agent.

View file

@ -24,11 +24,12 @@ func (m *mockRegistryProvider) GetDefaultModel() string {
return "mock-model" 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{ return &config.Config{
Agents: config.AgentsConfig{ Agents: config.AgentsConfig{
Defaults: config.AgentDefaults{ Defaults: config.AgentDefaults{
Workspace: "/tmp/picoclaw-test-registry", Workspace: workspace,
Model: "gpt-4", Model: "gpt-4",
MaxTokens: 8192, MaxTokens: 8192,
MaxToolIterations: 10, MaxToolIterations: 10,
@ -39,7 +40,7 @@ func testCfg(agents []config.AgentConfig) *config.Config {
} }
func TestNewAgentRegistry_ImplicitMain(t *testing.T) { func TestNewAgentRegistry_ImplicitMain(t *testing.T) {
cfg := testCfg(nil) cfg := testCfg(t, nil)
registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) registry := NewAgentRegistry(cfg, &mockRegistryProvider{})
ids := registry.ListAgentIDs() ids := registry.ListAgentIDs()
@ -57,7 +58,7 @@ func TestNewAgentRegistry_ImplicitMain(t *testing.T) {
} }
func TestNewAgentRegistry_ExplicitAgents(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: "sales", Default: true, Name: "Sales Bot"},
{ID: "support", Name: "Support Bot"}, {ID: "support", Name: "Support Bot"},
}) })
@ -83,7 +84,7 @@ func TestNewAgentRegistry_ExplicitAgents(t *testing.T) {
} }
func TestAgentRegistry_GetAgent_Normalize(t *testing.T) { func TestAgentRegistry_GetAgent_Normalize(t *testing.T) {
cfg := testCfg([]config.AgentConfig{ cfg := testCfg(t, []config.AgentConfig{
{ID: "my-agent", Default: true}, {ID: "my-agent", Default: true},
}) })
registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) registry := NewAgentRegistry(cfg, &mockRegistryProvider{})
@ -98,7 +99,7 @@ func TestAgentRegistry_GetAgent_Normalize(t *testing.T) {
} }
func TestAgentRegistry_GetDefaultAgent(t *testing.T) { func TestAgentRegistry_GetDefaultAgent(t *testing.T) {
cfg := testCfg([]config.AgentConfig{ cfg := testCfg(t, []config.AgentConfig{
{ID: "alpha"}, {ID: "alpha"},
{ID: "beta", Default: true}, {ID: "beta", Default: true},
}) })
@ -112,7 +113,7 @@ func TestAgentRegistry_GetDefaultAgent(t *testing.T) {
} }
func TestAgentRegistry_CanSpawnSubagent(t *testing.T) { func TestAgentRegistry_CanSpawnSubagent(t *testing.T) {
cfg := testCfg([]config.AgentConfig{ cfg := testCfg(t, []config.AgentConfig{
{ {
ID: "parent", ID: "parent",
Default: true, Default: true,
@ -141,7 +142,7 @@ func TestAgentRegistry_CanSpawnSubagent(t *testing.T) {
} }
func TestAgentRegistry_CanSpawnSubagent_Wildcard(t *testing.T) { func TestAgentRegistry_CanSpawnSubagent_Wildcard(t *testing.T) {
cfg := testCfg([]config.AgentConfig{ cfg := testCfg(t, []config.AgentConfig{
{ {
ID: "admin", ID: "admin",
Default: true, Default: true,
@ -163,7 +164,7 @@ func TestAgentRegistry_CanSpawnSubagent_Wildcard(t *testing.T) {
func TestAgentInstance_Model(t *testing.T) { func TestAgentInstance_Model(t *testing.T) {
model := &config.AgentModelConfig{Primary: "claude-opus"} model := &config.AgentModelConfig{Primary: "claude-opus"}
cfg := testCfg([]config.AgentConfig{ cfg := testCfg(t, []config.AgentConfig{
{ID: "custom", Default: true, Model: model}, {ID: "custom", Default: true, Model: model},
}) })
registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) registry := NewAgentRegistry(cfg, &mockRegistryProvider{})
@ -175,7 +176,7 @@ func TestAgentInstance_Model(t *testing.T) {
} }
func TestAgentInstance_FallbackInheritance(t *testing.T) { func TestAgentInstance_FallbackInheritance(t *testing.T) {
cfg := testCfg([]config.AgentConfig{ cfg := testCfg(t, []config.AgentConfig{
{ID: "inherit", Default: true}, {ID: "inherit", Default: true},
}) })
cfg.Agents.Defaults.ModelFallbacks = []string{"openai/gpt-4o-mini", "anthropic/haiku"} cfg.Agents.Defaults.ModelFallbacks = []string{"openai/gpt-4o-mini", "anthropic/haiku"}
@ -192,7 +193,7 @@ func TestAgentInstance_FallbackExplicitEmpty(t *testing.T) {
Primary: "gpt-4", Primary: "gpt-4",
Fallbacks: []string{}, // explicitly empty = disable Fallbacks: []string{}, // explicitly empty = disable
} }
cfg := testCfg([]config.AgentConfig{ cfg := testCfg(t, []config.AgentConfig{
{ID: "no-fallback", Default: true, Model: model}, {ID: "no-fallback", Default: true, Model: model},
}) })
cfg.Agents.Defaults.ModelFallbacks = []string{"should-not-inherit"} cfg.Agents.Defaults.ModelFallbacks = []string{"should-not-inherit"}

View file

@ -97,7 +97,7 @@ func NewContainerSandbox(cfg ContainerSandboxConfig) *ContainerSandbox {
if strings.TrimSpace(cfg.Network) == "" { if strings.TrimSpace(cfg.Network) == "" {
cfg.Network = "none" cfg.Network = "none"
} }
if len(cfg.CapDrop) == 0 { if cfg.CapDrop == nil {
cfg.CapDrop = []string{"ALL"} cfg.CapDrop = []string{"ALL"}
} }
if cfg.Env == nil { if cfg.Env == nil {
@ -167,7 +167,10 @@ func (c *ContainerSandbox) Start(ctx context.Context) error {
return c.startErr return c.startErr
} }
defer rc.Close() 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 // Tag debian:bookworm-slim as picoclaw-sandbox:bookworm-slim
if err := c.cli.ImageTag(ctx, FallbackSandboxImage, DefaultSandboxImage); err != nil { 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 return c.startErr
} }
defer rc.Close() 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 { if err != nil {
return err 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 { if mkdir {
hostDir, ok := f.sb.hostDirForContainerPath(dir) script = `set -eu; dir=$(dirname -- "$1"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; cat >"$1"`
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{ execResp, err := f.sb.cli.ContainerExecCreate(ctx, f.sb.cfg.ContainerName, container.ExecOptions{
Command: "mkdir -p " + shellEscape(dir), 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 { if err != nil {
return err return fmt.Errorf("docker exec create failed: %w", err)
}
}
} }
var buf bytes.Buffer attach, err := f.sb.cli.ContainerExecAttach(ctx, execResp.ID, container.ExecStartOptions{})
tw := tar.NewWriter(&buf) if err != nil {
if err := tw.WriteHeader(&tar.Header{ return fmt.Errorf("docker exec attach failed: %w", err)
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 { defer attach.Close()
_ = tw.Close()
return fmt.Errorf("tar content write failed: %w", err) // Write data to the hijacked connection's stdin
} if _, err := attach.Conn.Write(data); err != nil {
if err := tw.Close(); err != nil { return fmt.Errorf("failed to write data to container: %w", err)
return fmt.Errorf("tar close failed: %w", err)
} }
if err := f.sb.cli.CopyToContainer(ctx, f.sb.cfg.ContainerName, dir, &buf, container.CopyToContainerOptions{ // Close the write side of the connection so cat receives EOF and terminates
AllowOverwriteDirWithFile: true, if conn, ok := attach.Conn.(interface{ CloseWrite() error }); ok {
}); err != nil { _ = conn.CloseWrite()
return fmt.Errorf("docker copy to container failed: %w", err)
} }
// 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 return nil
} }

View file

@ -55,6 +55,7 @@ func TestContainerSandbox_Integration_ExecReadWrite(t *testing.T) {
Image: image, Image: image,
ContainerName: containerName, ContainerName: containerName,
Workspace: workspace, Workspace: workspace,
User: fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()),
}) })
err := sb.Start(ctx) err := sb.Start(ctx)
if err != nil { 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) { func TestContainerSandbox_Integration_WriteFileMkdirInContainerTmp(t *testing.T) {
_, cleanup := skipIfNoDocker(t) _, cleanup := skipIfNoDocker(t)
defer cleanup() defer cleanup()
@ -123,10 +185,13 @@ func TestContainerSandbox_Integration_WriteFileMkdirInContainerTmp(t *testing.T)
containerName := fmt.Sprintf("picoclaw-test-mkdir-%d", time.Now().UnixNano()) containerName := fmt.Sprintf("picoclaw-test-mkdir-%d", time.Now().UnixNano())
image := getTestImage() image := getTestImage()
workspace := t.TempDir()
sb := NewContainerSandbox(ContainerSandboxConfig{ sb := NewContainerSandbox(ContainerSandboxConfig{
Image: image, Image: image,
ContainerName: containerName, ContainerName: containerName,
Workspace: workspace,
User: fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()),
}) })
err := sb.Start(ctx) err := sb.Start(ctx)
if err != nil { if err != nil {

View file

@ -391,18 +391,21 @@ func TestScopedSandboxManager_ContainerCreationError(t *testing.T) {
m := &scopedSandboxManager{ m := &scopedSandboxManager{
mode: config.SandboxModeAll, mode: config.SandboxModeAll,
workspaceRoot: home, workspaceRoot: home,
image: "non-existent-image-12345",
dockerCfg: config.AgentSandboxDockerConfig{Image: "non-existent-image-12345"}, dockerCfg: config.AgentSandboxDockerConfig{Image: "non-existent-image-12345"},
scoped: map[string]Sandbox{}, scoped: map[string]Sandbox{},
} }
ctx := WithSessionKey(context.Background(), "error-session") ctx := WithSessionKey(context.Background(), "error-session")
// Fast-path misses, falls through to buildScopedContainerSandbox and Start() // Resolve calls getOrCreateSandbox, which calls Start().
// Start() succeeds due to lazy evaluation, but Exec() should fail because // Start should fail because the image doesn't exist.
// Docker is either unavailable or the image is missing
sb, err := m.Resolve(ctx) sb, err := m.Resolve(ctx)
if err != nil { 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"}) res, err := sb.Exec(ctx, ExecRequest{Command: "echo"})

View file

@ -30,7 +30,6 @@ func DefaultConfig() *Config {
Tmpfs: []string{"/tmp", "/var/tmp", "/run"}, Tmpfs: []string{"/tmp", "/var/tmp", "/run"},
Network: "none", Network: "none",
User: "", User: "",
CapDrop: []string{"ALL"},
Env: map[string]string{ Env: map[string]string{
"LANG": "C.UTF-8", "LANG": "C.UTF-8",
}, },