From 4b56512a5a397219e39efbcdc05fa5f8e41761fd Mon Sep 17 00:00:00 2001
From: fullsend-code
 <278716306+fullsend-ai-coder[bot]@users.noreply.github.com>
Date: Tue, 28 Jul 2026 10:59:51 +0000
Subject: [PATCH] fix(#605): retry sandbox cleanup with chmod on permission
 denied

When sandboxed processes create files with restrictive permissions
(e.g. different UID), os.RemoveAll fails during repo extraction
cleanup. This caused the entire run to abort and skip the post-script,
losing review results (observed on 4 of 7 review runs for PR #3193).

Add forceRemoveAll helper that retries removal after making all entries
owner-writable (chmod 0700). Change the cleanup at step 9d from a hard
failure to a warning so the post-script always has a chance to run.

Closes #605
---
 internal/cli/run.go      | 29 +++++++++++++++++++++++++++--
 internal/cli/run_test.go | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/internal/cli/run.go b/internal/cli/run.go
index a5ff8cd351..c5d10fb80a 100644
--- a/internal/cli/run.go
+++ b/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).
diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go
index 10fdb2a76c..6278629e66 100644
--- a/internal/cli/run_test.go
+++ b/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
 }