feat: TUI-triggered fix via background worktrees (#279)

CLAUDE.md

@@ -40,6 +40,7 @@ CLI (roborev) → HTTP API → Daemon (roborev daemon run) → Worker Pool → A
 | `internal/storage/` | SQLite operations |
 | `internal/agent/` | Agent interface + implementations |
 | `internal/config/config.go` | Config loading, agent resolution |
+| `internal/worktree/` | Git worktree helpers (create, patch capture/apply) |
 
 ## Conventions
 
@@ -93,7 +94,7 @@ CLI reads this to find the daemon. If port 7373 is busy, daemon auto-increments.
 
 ## Design Constraints
 
-- **Daemon tasks must not modify the git working tree.** Background jobs (reviews, CI polling, synthesis) are read-only with respect to the user's repo checkout. They read source files and write results to the database only. CLI commands like `roborev fix` run synchronously in the foreground and may modify files, but nothing enqueued to the worker pool should touch the working tree. If we need background tasks that produce file changes in the future, they should operate in isolated git worktrees — that is a separate initiative.
+- **Daemon tasks must not modify the git working tree.** Background jobs (reviews, CI polling, synthesis) are read-only with respect to the user's repo checkout. They read source files and write results to the database only. CLI commands like `roborev fix` run synchronously in the foreground and may modify files. Background `fix` jobs run agents in isolated git worktrees (via `internal/worktree`) and store resulting patches in the database — patches are only applied to the working tree when the user explicitly confirms in the TUI.
 
 ## Style Preferences
 

cmd/roborev/daemon_integration_test.go

@@ -245,6 +245,10 @@ func TestDaemonShutdownBySignal(t *testing.T) {
 }
 
 func TestDaemonSignalCleanup(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("skipping daemon signal test on Windows due to file locking differences")
+	}
+
 	// Verify that signal.Stop is called when shutdown
 	// is triggered by a signal.
 	var cleanupCalled bool

cmd/roborev/refine.go

@@ -1,18 +1,12 @@
 package main
 
 import (
-	"bufio"
-	"bytes"
 	"context"
 	"errors"
 	"fmt"
 	"io"
-	"io/fs"
 	"os"
-	"os/exec"
-	"path/filepath"
 	"sort"
-	"strconv"
 	"strings"
 	"time"
 
@@ -23,6 +17,7 @@ import (
 	"github.com/roborev-dev/roborev/internal/git"
 	"github.com/roborev-dev/roborev/internal/prompt"
 	"github.com/roborev-dev/roborev/internal/storage"
+	"github.com/roborev-dev/roborev/internal/worktree"
 	"github.com/spf13/cobra"
 )
 
@@ -513,13 +508,14 @@ func runRefine(ctx RunContext, opts refineOptions) error {
 		branchBefore := git.GetCurrentBranch(repoPath)
 
 		// Create temp worktree to isolate agent from user's working tree
-		worktreePath, cleanupWorktree, err := createTempWorktree(repoPath)
+		wt, err := worktree.Create(repoPath)
 		if err != nil {
 			return fmt.Errorf("create worktree: %w", err)
 		}
+		worktreePath := wt.Dir
 		// NOTE: not using defer here because we're inside a loop;
 		// defer wouldn't run until runRefine returns, leaking worktrees.
-		// Instead, cleanupWorktree() is called explicitly before every exit point.
+		// Instead, wt.Close() is called explicitly before every exit point.
 
 		// Determine output writer
 		var agentOutput io.Writer
@@ -554,31 +550,31 @@ func runRefine(ctx RunContext, opts refineOptions) error {
 
 		// Safety checks on main repo (before applying any changes)
 		if wasCleanBefore && !git.IsWorkingTreeClean(repoPath) {
-			cleanupWorktree()
+			wt.Close()
 			return fmt.Errorf("working tree changed during refine - aborting to prevent data loss")
 		}
 		headAfterAgent, resolveErr := git.ResolveSHA(repoPath, "HEAD")
 		if resolveErr != nil {
-			cleanupWorktree()
+			wt.Close()
 			return fmt.Errorf("cannot determine HEAD after agent run: %w", resolveErr)
 		}
 		branchAfterAgent := git.GetCurrentBranch(repoPath)
 		if headAfterAgent != headBefore || branchAfterAgent != branchBefore {
-			cleanupWorktree()
+			wt.Close()
 			return fmt.Errorf("HEAD changed during refine (was %s on %s, now %s on %s) - aborting to prevent applying patch to wrong commit",
 				shortSHA(headBefore), branchBefore, shortSHA(headAfterAgent), branchAfterAgent)
 		}
 
 		if agentErr != nil {
-			cleanupWorktree()
+			wt.Close()
 			fmt.Printf("Agent error: %v\n", agentErr)
 			fmt.Println("Will retry in next iteration")
 			continue
 		}
 
 		// Check if agent made changes in worktree
 		if git.IsWorkingTreeClean(worktreePath) {
-			cleanupWorktree()
+			wt.Close()
 			fmt.Println("Agent made no changes - skipping this review")
 			if err := client.AddComment(currentFailedReview.JobID, "roborev-refine", "Agent could not determine how to address findings"); err != nil {
 				fmt.Printf("Warning: failed to add comment to job %d: %v\n", currentFailedReview.JobID, err)
@@ -588,12 +584,17 @@ func runRefine(ctx RunContext, opts refineOptions) error {
 			continue
 		}
 
-		// Apply worktree changes to main repo and commit
-		if err := applyWorktreeChanges(repoPath, worktreePath); err != nil {
-			cleanupWorktree()
-			return fmt.Errorf("apply worktree changes: %w", err)
+		// Capture patch from worktree and apply to main repo
+		patch, err := wt.CapturePatch()
+		if err != nil {
+			wt.Close()
+			return fmt.Errorf("capture worktree patch: %w", err)
+		}
+		if err := worktree.ApplyPatch(repoPath, patch); err != nil {
+			wt.Close()
+			return fmt.Errorf("apply worktree patch: %w", err)
 		}
-		cleanupWorktree()
+		wt.Close()
 
 		commitMsg := fmt.Sprintf("Address review findings (job %d)\n\n%s", currentFailedReview.JobID, summarizeAgentOutput(output))
 		newCommit, err := commitWithHookRetry(repoPath, commitMsg, addressAgent, opts.quiet)
@@ -1024,167 +1025,7 @@ func summarizeAgentOutput(output string) string {
 	return strings.Join(summary, "\n")
 }
 
-// createTempWorktree creates a temporary git worktree for isolated agent work
-func createTempWorktree(repoPath string) (string, func(), error) {
-	worktreeDir, err := os.MkdirTemp("", "roborev-refine-")
-	if err != nil {
-		return "", nil, err
-	}
-
-	// Create the worktree (without --recurse-submodules for compatibility with older git).
-	// Suppress hooks via core.hooksPath=<null> — user hooks shouldn't run in internal worktrees.
-	cmd := exec.Command("git", "-C", repoPath, "-c", "core.hooksPath="+os.DevNull, "worktree", "add", "--detach", worktreeDir, "HEAD")
-	if out, err := cmd.CombinedOutput(); err != nil {
-		_ = os.RemoveAll(worktreeDir)
-		return "", nil, fmt.Errorf("git worktree add: %w: %s", err, out)
-	}
-
-	// Initialize and update submodules in the worktree
-	initArgs := []string{"-C", worktreeDir}
-	if submoduleRequiresFileProtocol(worktreeDir) {
-		initArgs = append(initArgs, "-c", "protocol.file.allow=always")
-	}
-	initArgs = append(initArgs, "submodule", "update", "--init")
-	cmd = exec.Command("git", initArgs...)
-	if out, err := cmd.CombinedOutput(); err != nil {
-		_ = exec.Command("git", "-C", repoPath, "worktree", "remove", "--force", worktreeDir).Run()
-		_ = os.RemoveAll(worktreeDir)
-		return "", nil, fmt.Errorf("git submodule update: %w: %s", err, out)
-	}
-
-	updateArgs := []string{"-C", worktreeDir}
-	if submoduleRequiresFileProtocol(worktreeDir) {
-		updateArgs = append(updateArgs, "-c", "protocol.file.allow=always")
-	}
-	updateArgs = append(updateArgs, "submodule", "update", "--init", "--recursive")
-	cmd = exec.Command("git", updateArgs...)
-	if out, err := cmd.CombinedOutput(); err != nil {
-		_ = exec.Command("git", "-C", repoPath, "worktree", "remove", "--force", worktreeDir).Run()
-		_ = os.RemoveAll(worktreeDir)
-		return "", nil, fmt.Errorf("git submodule update: %w: %s", err, out)
-	}
-
-	lfsCmd := exec.Command("git", "-C", worktreeDir, "lfs", "env")
-	if err := lfsCmd.Run(); err == nil {
-		cmd = exec.Command("git", "-C", worktreeDir, "lfs", "pull")
-		_ = cmd.Run()
-	}
-
-	cleanup := func() {
-		_ = exec.Command("git", "-C", repoPath, "worktree", "remove", "--force", worktreeDir).Run()
-		_ = os.RemoveAll(worktreeDir)
-	}
-
-	return worktreeDir, cleanup, nil
-}
-
-func submoduleRequiresFileProtocol(repoPath string) bool {
-	gitmodulesPaths := findGitmodulesPaths(repoPath)
-	if len(gitmodulesPaths) == 0 {
-		return false
-	}
-	for _, gitmodulesPath := range gitmodulesPaths {
-		file, err := os.Open(gitmodulesPath)
-		if err != nil {
-			continue
-		}
-		scanner := bufio.NewScanner(file)
-		for scanner.Scan() {
-			line := strings.TrimSpace(scanner.Text())
-			if line == "" || strings.HasPrefix(line, "#") || strings.HasPrefix(line, ";") {
-				continue
-			}
-			parts := strings.SplitN(line, "=", 2)
-			if len(parts) != 2 {
-				continue
-			}
-			if !strings.EqualFold(strings.TrimSpace(parts[0]), "url") {
-				continue
-			}
-			url := strings.TrimSpace(parts[1])
-			if unquoted, err := strconv.Unquote(url); err == nil {
-				url = unquoted
-			}
-			if isFileProtocolURL(url) {
-				file.Close()
-				return true
-			}
-		}
-		file.Close()
-	}
-	return false
-}
-
-func findGitmodulesPaths(repoPath string) []string {
-	var paths []string
-	err := filepath.WalkDir(repoPath, func(path string, d fs.DirEntry, err error) error {
-		if err != nil {
-			return nil
-		}
-		if d.IsDir() && d.Name() == ".git" {
-			return filepath.SkipDir
-		}
-		if d.Name() == ".gitmodules" {
-			paths = append(paths, path)
-		}
-		return nil
-	})
-	if err != nil {
-		return nil
-	}
-	return paths
-}
-
-func isFileProtocolURL(url string) bool {
-	lower := strings.ToLower(url)
-	if strings.HasPrefix(lower, "file:") {
-		return true
-	}
-	if strings.HasPrefix(url, "/") || strings.HasPrefix(url, "./") || strings.HasPrefix(url, "../") {
-		return true
-	}
-	if len(url) >= 2 && isAlpha(url[0]) && url[1] == ':' {
-		return true
-	}
-	if strings.HasPrefix(url, `\\`) {
-		return true
-	}
-	return false
-}
-
-func isAlpha(b byte) bool {
-	return (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z')
-}
-
-// applyWorktreeChanges applies changes from worktree to main repo via patch
-func applyWorktreeChanges(repoPath, worktreePath string) error {
-	// Stage all changes in worktree
-	cmd := exec.Command("git", "-C", worktreePath, "add", "-A")
-	if out, err := cmd.CombinedOutput(); err != nil {
-		return fmt.Errorf("git add in worktree: %w: %s", err, out)
-	}
-
-	// Get diff as patch
-	diffCmd := exec.Command("git", "-C", worktreePath, "diff", "--cached", "--binary")
-	diff, err := diffCmd.Output()
-	if err != nil {
-		return fmt.Errorf("git diff in worktree: %w", err)
-	}
-	if len(diff) == 0 {
-		return nil // No changes
-	}
-
-	// Apply patch to main repo
-	applyCmd := exec.Command("git", "-C", repoPath, "apply", "--binary", "-")
-	applyCmd.Stdin = bytes.NewReader(diff)
-	var stderr bytes.Buffer
-	applyCmd.Stderr = &stderr
-	if err := applyCmd.Run(); err != nil {
-		return fmt.Errorf("git apply: %w: %s", err, stderr.String())
-	}
-
-	return nil
-}
+// Worktree creation and patch operations are in internal/worktree package.
 
 // commitWithHookRetry attempts git.CreateCommit and, on failure,
 // runs the agent to fix whatever the hook complained about. Only

cmd/roborev/refine_integration_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	"github.com/roborev-dev/roborev/internal/testutil"
+	"github.com/roborev-dev/roborev/internal/worktree"
 )
 
 func TestValidateRefineContext(t *testing.T) {
@@ -169,9 +170,9 @@ func TestWorktreeCleanupBetweenIterations(t *testing.T) {
 	// before the next iteration. Verify the directory is removed each time.
 	var prevPath string
 	for i := range 3 {
-		worktreePath, cleanup, err := createTempWorktree(repo.Root)
+		wt, err := worktree.Create(repo.Root)
 		if err != nil {
-			t.Fatalf("iteration %d: createTempWorktree failed: %v", i, err)
+			t.Fatalf("iteration %d: worktree.Create failed: %v", i, err)
 		}
 
 		// Verify previous worktree was cleaned up
@@ -182,13 +183,13 @@ func TestWorktreeCleanupBetweenIterations(t *testing.T) {
 		}
 
 		// Verify current worktree exists
-		if _, err := os.Stat(worktreePath); err != nil {
-			t.Fatalf("iteration %d: worktree %s should exist: %v", i, worktreePath, err)
+		if _, err := os.Stat(wt.Dir); err != nil {
+			t.Fatalf("iteration %d: worktree %s should exist: %v", i, wt.Dir, err)
 		}
 
 		// Simulate the explicit cleanup call (as done on error/no-change paths)
-		cleanup()
-		prevPath = worktreePath
+		wt.Close()
+		prevPath = wt.Dir
 	}
 
 	// Verify the last worktree was also cleaned up
@@ -221,19 +222,19 @@ func TestCreateTempWorktreeIgnoresHooks(t *testing.T) {
 		_ = out
 	}
 
-	// createTempWorktree should succeed because it suppresses hooks
-	worktreePath, cleanup, err := createTempWorktree(repo.Root)
+	// worktree.Create should succeed because it suppresses hooks
+	wt, err := worktree.Create(repo.Root)
 	if err != nil {
-		t.Fatalf("createTempWorktree should succeed with failing hook: %v", err)
+		t.Fatalf("worktree.Create should succeed with failing hook: %v", err)
 	}
-	defer cleanup()
+	defer wt.Close()
 
 	// Verify the worktree directory exists and has the file from the repo
-	if _, err := os.Stat(worktreePath); err != nil {
+	if _, err := os.Stat(wt.Dir); err != nil {
 		t.Fatalf("worktree directory should exist: %v", err)
 	}
 
-	baseFile := filepath.Join(worktreePath, "base.txt")
+	baseFile := filepath.Join(wt.Dir, "base.txt")
 	content, err := os.ReadFile(baseFile)
 	if err != nil {
 		t.Fatalf("expected base.txt in worktree: %v", err)
@@ -280,13 +281,13 @@ func TestCreateTempWorktreeInitializesSubmodules(t *testing.T) {
 	runMainGit("-c", "protocol.file.allow=always", "submodule", "add", submoduleRepo, "deps/sub")
 	runMainGit("commit", "-m", "add submodule")
 
-	worktreePath, cleanup, err := createTempWorktree(mainRepo)
+	wt, err := worktree.Create(mainRepo)
 	if err != nil {
-		t.Fatalf("createTempWorktree failed: %v", err)
+		t.Fatalf("worktree.Create failed: %v", err)
 	}
-	defer cleanup()
+	defer wt.Close()
 
-	if _, err := os.Stat(filepath.Join(worktreePath, "deps", "sub", "sub.txt")); err != nil {
+	if _, err := os.Stat(filepath.Join(wt.Dir, "deps", "sub", "sub.txt")); err != nil {
 		t.Fatalf("expected submodule file in worktree: %v", err)
 	}
 }