fix(#605): retry sandbox cleanup with chmod on permission denied

internal/cli/run.go

@@ -3,8 +3,10 @@ package cli
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"net/http"
 	"os"
 	"os/exec"
@@ -887,8 +889,12 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep
 
 		// 9d. Extract target repo back to host. SafeDownload removes dangerous
 		// symlinks (absolute or repo-escaping) and .git/hooks/ to prevent sandbox escape.
-		if clearErr := os.RemoveAll(hostRepositoryDir); clearErr != nil {
-			return fmt.Errorf("clearing local repo %s before extraction: %w", hostRepositoryDir, clearErr)
+		// Use forceRemoveAll to handle files created by sandboxed processes with
+		// restrictive permissions (#605). Warn on failure instead of aborting so
+		// the post-script still runs (review results are more valuable than a
+		// clean temp directory).
+		if clearErr := forceRemoveAll(hostRepositoryDir); clearErr != nil {
+			printer.StepWarn(fmt.Sprintf("Could not fully clear %s before extraction (continuing): %v", hostRepositoryDir, clearErr))
 		}
 		repoExtractStart := time.Now()
 		printer.StepStart("Extracting target repo")
@@ -1296,6 +1302,25 @@ func escapeForDoubleQuotes(s string) string {
 	return s
 }
 
+// forceRemoveAll removes a directory tree. If the initial removal fails with a
+// permission error (e.g. files created by a sandboxed process with a different
+// UID), it makes all entries owner-writable and retries. See #605.
+func forceRemoveAll(path string) error {
+	err := os.RemoveAll(path)
+	if err == nil || !errors.Is(err, fs.ErrPermission) {
+		return err
+	}
+	// Best-effort chmod: make every entry owner-rwx so the retry can unlink.
+	_ = filepath.WalkDir(path, func(p string, _ fs.DirEntry, walkErr error) error {
+		if walkErr != nil {
+			return nil // keep walking
+		}
+		_ = os.Chmod(p, 0o700)
+		return nil
+	})
+	return os.RemoveAll(path)
+}
+
 // validationFailMessage returns a human-readable message for a validation
 // script failure. When the script produces output, that output is used;
 // otherwise it falls back to the exec error string (e.g. ENOENT / EACCES).

internal/cli/run_test.go

@@ -1423,6 +1423,43 @@ func TestEffectiveMaxRuntimeFetches_MatchesFetchsvcDefault(t *testing.T) {
 	}
 }
 
+func TestForceRemoveAll(t *testing.T) {
+	t.Run("removes normal directory", func(t *testing.T) {
+		dir := t.TempDir()
+		target := filepath.Join(dir, "target")
+		require.NoError(t, os.MkdirAll(filepath.Join(target, "sub"), 0o755))
+		require.NoError(t, os.WriteFile(filepath.Join(target, "sub", "file"), []byte("x"), 0o644))
+
+		require.NoError(t, forceRemoveAll(target))
+		_, err := os.Stat(target)
+		assert.True(t, os.IsNotExist(err))
+	})
+
+	t.Run("handles restrictive directory permissions", func(t *testing.T) {
+		if os.Getuid() == 0 {
+			t.Skip("test requires non-root to simulate permission errors")
+		}
+		dir := t.TempDir()
+		target := filepath.Join(dir, "target")
+		sub := filepath.Join(target, "sub")
+		require.NoError(t, os.MkdirAll(sub, 0o755))
+		require.NoError(t, os.WriteFile(filepath.Join(sub, "file"), []byte("x"), 0o644))
+		// Remove write from the subdirectory so os.RemoveAll fails.
+		require.NoError(t, os.Chmod(sub, 0o500))
+
+		// Confirm plain RemoveAll fails.
+		require.Error(t, os.RemoveAll(target))
+
+		require.NoError(t, forceRemoveAll(target))
+		_, err := os.Stat(target)
+		assert.True(t, os.IsNotExist(err))
+	})
+
+	t.Run("nonexistent path is no-op", func(t *testing.T) {
+		require.NoError(t, forceRemoveAll(filepath.Join(t.TempDir(), "nope")))
+	})
+}
+
 type mockForgeClient struct {
 	forge.Client
 }