fix: worktree management - #223

internal/core/run/parallel/orchestrator_test.go (1)

241-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

This regression guard is tautological and can never fail.

Prune was removed from the WorktreeLifecycle interface (orchestrator.go Lines 83-98), so the orchestrator only sees interface methods and can never invoke fakeWorktreeLifecycle.Prune. wasPruned() will therefore always return false regardless of orchestrator logic, so this scenario cannot detect a repo-wide-prune regression.

Consider asserting the actual scoped behavior instead (e.g. that only per-path Remove was called for the expected worktrees, plus DiscardIntegrationBranch), which fails if scoped cleanup logic changes. The now-unreachable Prune/pruned/wasPruned scaffolding (Lines 1074-1079, 1111-1115) can be dropped once the assertion is behavior-based.

As per path instructions: "Verify tests can fail when business logic changes."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/parallel/orchestrator_test.go` around lines 241 - 262, The
regression test in
runParallelExecutionOrchestratorDoesNotPruneAfterScopedCleanup is tautological
because WorktreeLifecycle no longer exposes Prune, so
fakeWorktreeLifecycle.wasPruned() can never reflect orchestrator behavior.
Replace this assertion with checks on observable scoped cleanup behavior in
NewParallelExecutionOrchestrator.Run, such as verifying the expected per-path
Remove calls and DiscardIntegrationBranch invocation, using the existing
fakeWorktreeLifecycle and launcher helpers. Once the test asserts real behavior,
remove the unreachable Prune/pruned/wasPruned scaffolding from the fake
lifecycle.

Source: Path instructions

internal/core/gitenv/gitenv.go (1)

1-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a shared Command constructor to eliminate duplicated subprocess-setup logic.

review_watch_git.go and task_multi_worktree.go both duplicate an identical pattern: build cmd := exec.CommandContext(ctx, "git"), then manually overwrite cmd.Args with append([]string{"git"}, cmdArgs...) and set cmd.Env = gitenv.SanitizedEnv(). Since this package already centralizes environment sanitization, it could also expose a small helper (e.g., gitenv.Command(ctx, dir string, args ...string) *exec.Cmd) that wires Env automatically, so future call sites can't forget to sanitize and the three near-identical implementations converge to one.

Also worth adding a dedicated gitenv_test.go exercising SanitizedEnv/IsRepositoryEnvName directly, since coverage currently only exists indirectly via the daemon/worktree re-exec tests.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/gitenv/gitenv.go` around lines 1 - 42, The git subprocess setup
is duplicated in multiple call sites, so factor it into a shared helper in
gitenv to keep Env sanitization and argument wiring consistent. Add a small
constructor like gitenv.Command that wraps exec.CommandContext, sets Args/Dir,
and always applies gitenv.SanitizedEnv, then update review_watch_git.go and
task_multi_worktree.go to use it. Keep SanitizedEnv and IsRepositoryEnvName as
the core sanitization logic, and add gitenv_test.go to directly cover both
functions so the behavior is validated without relying only on indirect
daemon/worktree tests.

internal/cli/daemon_commands.go (2)

992-1011: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate git subprocess runner.

runTaskRunGitPreflight reimplements the same "-C <dir> + sanitized env + stdout/stderr capture" pattern already used by runTaskMultiWorktreeGitCommand in internal/daemon/task_multi_worktree.go (and, per the PR summary, the same shape was also just applied to internal/core/worktree/snapshot.go and internal/daemon/review_watch_git.go). The gitenv package was introduced specifically to centralize the sanitization contract; leaving the surrounding exec/capture boilerplate duplicated across four call sites risks drift (e.g., one site forgetting gitenv.SanitizedEnv() in a future edit).

Consider hoisting a shared RunGit(ctx, dir string, args ...string) (string, error) helper (e.g., into gitenv or a small sibling package) that all four call sites use.

♻️ Sketch of a shared helper

// internal/core/gitenv/exec.go
func RunGit(ctx context.Context, dir string, args ...string) (string, error) {
	cmd := exec.CommandContext(ctx, "git")
	cmd.Args = append([]string{"git", "-C", strings.TrimSpace(dir)}, args...)
	cmd.Env = SanitizedEnv()
	var stdout, stderr strings.Builder
	cmd.Stdout = &stdout
	cmd.Stderr = &stderr
	if err := cmd.Run(); err != nil {
		message := strings.TrimSpace(stderr.String())
		if message == "" {
			message = strings.TrimSpace(stdout.String())
		}
		if message != "" {
			return "", fmt.Errorf("%w: %s", err, message)
		}
		return "", err
	}
	return strings.TrimSpace(stdout.String()), nil
}

-func runTaskRunGitPreflight(ctx context.Context, workspaceRoot string, args ...string) (string, error) {
-	cmdArgs := append([]string{"-C", strings.TrimSpace(workspaceRoot)}, args...)
-	cmd := exec.CommandContext(ctx, "git")
-	cmd.Args = append([]string{"git"}, cmdArgs...)
-	cmd.Env = gitenv.SanitizedEnv()
-	var stdout, stderr strings.Builder
-	cmd.Stdout = &stdout
-	cmd.Stderr = &stderr
-	if err := cmd.Run(); err != nil {
-		message := strings.TrimSpace(stderr.String())
-		if message == "" {
-			message = strings.TrimSpace(stdout.String())
-		}
-		if message != "" {
-			return "", fmt.Errorf("%w: %s", err, message)
-		}
-		return "", err
-	}
-	return strings.TrimSpace(stdout.String()), nil
-}
+func runTaskRunGitPreflight(ctx context.Context, workspaceRoot string, args ...string) (string, error) {
+	return gitenv.RunGit(ctx, workspaceRoot, args...)
+}

Based on learnings from the PR stack context: Git command execution explicitly sets cmd.Env = gitenv.SanitizedEnv() in `internal/daemon/task_multi_worktree.go`, confirming this exact pattern already exists elsewhere.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/daemon_commands.go` around lines 992 - 1011, The git subprocess
logic in runTaskRunGitPreflight duplicates the same command setup, sanitized
environment, and stdout/stderr capture already implemented in
runTaskMultiWorktreeGitCommand and the other git call sites. Extract this
repeated pattern into a shared helper such as RunGit(ctx, dir string, args
...string) in gitenv or a nearby package, and update runTaskRunGitPreflight to
use it so all git execution paths share the same SanitizedEnv and error
handling.

---

1013-1044: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Extract the containment check into one shared helper. cliPathEqualOrInside/cliCleanContainmentPath duplicate pathEqualOrInside/cleanContainmentPath; keeping one implementation avoids drift in this security-sensitive check.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/daemon_commands.go` around lines 1013 - 1044, The containment
logic in cliPathEqualOrInside and cliCleanContainmentPath duplicates the
existing pathEqualOrInside and cleanContainmentPath helpers, so consolidate to
one shared implementation and have the CLI helper reuse it. Update the
containment check path used by the daemon commands to call the shared helper
directly, keeping the behavior identical while avoiding drift in this
security-sensitive validation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/core/gitenv/gitenv_test.go (1)

1-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing test coverage for Run().

SanitizedEnv, IsRepositoryEnvName, and Command are all tested, but Run() — which contains the stderr/stdout fallback and error-wrapping logic reused by review_watch_git.go and task_multi_worktree.go — has no dedicated test. As per path instructions, tests should focus on critical paths including error handling.

func TestRun(t *testing.T) {
	if _, err := exec.LookPath("git"); err != nil {
		t.Skip("git binary not available")
	}
	t.Run("Should return trimmed stdout on success", func(t *testing.T) {
		dir := t.TempDir()
		if _, err := Run(context.Background(), dir, "init", "-q"); err != nil {
			t.Fatalf("init error = %v", err)
		}
		out, err := Run(context.Background(), dir, "rev-parse", "--show-toplevel")
		if err != nil {
			t.Fatalf("Run() error = %v", err)
		}
		if out == "" {
			t.Fatal("Run() returned empty toplevel")
		}
	})
	t.Run("Should wrap error with stderr message on failure", func(t *testing.T) {
		_, err := Run(context.Background(), t.TempDir(), "rev-parse", "--verify", "nope")
		if err == nil {
			t.Fatal("expected error")
		}
	})
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/gitenv/gitenv_test.go` around lines 1 - 89, Add dedicated
coverage for Run() in gitenv_test.go, since it’s currently untested despite
containing the stdout/stderr fallback and error-wrapping behavior used by
review_watch_git.go and task_multi_worktree.go. Extend TestRun to verify the
success path returns trimmed stdout and the failure path wraps the git error
with stderr details; reference the Run function and use the existing
context/background setup alongside exec.LookPath("git") as in the diff.

Source: Path instructions

internal/core/gitenv/gitenv.go (1)

39-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add command context to the no-message error path.

When both stderr and stdout are empty (e.g., binary missing, context canceled before output), Line 53 returns the raw err with no indication of which git command/dir failed, unlike the message branch which is well wrapped. As per coding guidelines, prefer wrapped context via fmt.Errorf("context: %w", err).

♻️ Proposed fix

-		return "", err
+		return "", fmt.Errorf("git %s (dir %s): %w", strings.Join(args, " "), dir, err)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/gitenv/gitenv.go` around lines 39 - 56, The no-message error
path in Run returns a bare error without any git command context. Update the
branch after cmd.Run() so that when both stderr and stdout are empty, it still
wraps err with contextual information from Run/Command, including the git args
and dir, using fmt.Errorf with %w. Keep the existing message-based branch
unchanged, but make the empty-output branch consistent so failures like missing
binary or canceled context are easier to trace.

Source: Coding guidelines

internal/core/gitenv/gitenv_test.go (1)

75-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Independent TestRun subtests could use t.Parallel().

Each subtest allocates its own t.TempDir() and doesn't share mutable state, so they're good candidates for t.Parallel() per the default table-driven pattern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/gitenv/gitenv_test.go` around lines 75 - 135, The TestRun
subtests can run independently and should be marked parallel for better test
execution. Add t.Parallel() at the start of each subtest inside TestRun, and
keep the existing use of t.TempDir(), Run, and the git-based assertions
unchanged since they do not share mutable state.

Source: Coding guidelines

internal/core/gitenv/gitenv.go (1)

50-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rebuild the display command from cmd.Args instead of re-joining args.

cmd.Args already holds ["git", args...]; manually re-deriving it via "git" + strings.Join(args, " ") duplicates that data and could drift if Command's construction changes.

♻️ Proposed simplification

-		command := "git"
-		if len(args) > 0 {
-			command += " " + strings.Join(args, " ")
-		}
+		command := strings.Join(cmd.Args, " ")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/gitenv/gitenv.go` around lines 50 - 57, The error formatting in
gitenv.Command should use the actual executed command from cmd.Args instead of
rebuilding it from args; update the command string construction in the error
path to derive the display text directly from cmd.Args so it stays consistent
with how exec.Command was called. Keep the existing failure message handling in
gitenv.go, but replace the manual "git" plus strings.Join(args, " ")
reconstruction with the command arguments already stored on the Cmd object.