fix(orchestrator): do not misclassify branch-checked-out-elsewhere as a missing branch (#2782)

Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>

apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go

@@ -87,7 +87,14 @@ func (p *LocalPreparer) Prepare(ctx context.Context, req *EnvPrepareRequest, onP
 	if req.WorkspacePath == "" && req.RepositoryPath == "" {
 		completeStepError(&step, "no workspace or repository path provided")
 		steps = append(steps, step)
-		return &EnvPrepareResult{Success: false, Steps: steps, ErrorMessage: step.Error, Duration: time.Since(start)}, fmt.Errorf("no workspace path")
+		prepErr := fmt.Errorf("no workspace path")
+		return &EnvPrepareResult{
+			Success:      false,
+			Steps:        steps,
+			ErrorMessage: step.Error,
+			Duration:     time.Since(start),
+			Error:        prepErr,
+		}, prepErr
 	}
 	completeStepSuccess(&step)
 	steps = append(steps, step)
@@ -124,7 +131,14 @@ func (p *LocalPreparer) Prepare(ctx context.Context, req *EnvPrepareRequest, onP
 				completeStepError(&step, errMsg)
 				steps = append(steps, step)
 				reportProgress(onProgress, step, stepIdx, totalSteps)
-				return &EnvPrepareResult{Success: false, Steps: steps, ErrorMessage: errMsg, Duration: time.Since(start)}, fmt.Errorf("checkout branch: %w", err)
+				prepErr := fmt.Errorf("checkout branch: %w", err)
+				return &EnvPrepareResult{
+					Success:      false,
+					Steps:        steps,
+					ErrorMessage: errMsg,
+					Duration:     time.Since(start),
+					Error:        prepErr,
+				}, prepErr
 			}
 			step.Output = output
 			completeStepSuccess(&step)

apps/backend/internal/agent/runtime/lifecycle/env_preparer_local_test.go

@@ -2,6 +2,9 @@ package lifecycle
 
 import (
 	"context"
+	"crypto/sha1"
+	"encoding/hex"
+	"errors"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -9,6 +12,7 @@ import (
 	"testing"
 
 	"github.com/kandev/kandev/internal/common/logger"
+	"github.com/kandev/kandev/internal/worktree"
 )
 
 func newTestLocalLogger() *logger.Logger {
@@ -456,3 +460,150 @@ func TestLocalPreparer_RunsSetupScriptWithoutCheckout(t *testing.T) {
 		t.Fatalf("local preparer modified branch: was %q, now %q", startBranch, got)
 	}
 }
+
+// TestLocalPreparer_BranchCheckedOutElsewhereFailsClosed is the regression for
+// the bug where a second task's environment creation fails when its target
+// branch is already checked out in another worktree AND the remote no longer
+// carries that ref. The local preparer must fail closed with a typed
+// ErrBranchCheckedOut error so the orchestrator does not misclassify the
+// failure as a missing-branch fetch failure and post PR-recovery guidance for
+// a branch that is, in fact, present locally.
+//
+// Before the fix, checkoutBranch concatenated the fetch stderr ("couldn't find
+// remote ref") with the checkout stderr ("is already checked out at ..."); the
+// combined message made the orchestrator's substring-based
+// isMissingBranchError match the "couldn't find remote ref" fragment and emit
+// branch_fetch_failed guidance — wrong because the local branch exists, just
+// not in this working tree.
+//
+// The shared checkout's HEAD and index must also be untouched: switching to
+// the target branch was never possible here, and partial state changes (e.g.
+// the fetch updating FETCH_HEAD or the index) would corrupt another task's
+// workspace.
+func TestLocalPreparer_BranchCheckedOutElsewhereFailsClosed(t *testing.T) {
+	isolateGitEnv(t)
+	repoDir := initGitRepo(t)
+	env := newIsolatedGitEnv()
+
+	for _, args := range [][]string{
+		{"checkout", "-b", "feature/shared"},
+		{"commit", "--allow-empty", "-m", "shared work"},
+		{"checkout", "main"},
+	} {
+		cmd := exec.Command("git", args...)
+		cmd.Dir = repoDir
+		cmd.Env = env
+		if out, err := cmd.CombinedOutput(); err != nil {
+			t.Fatalf("git %v failed: %s", args, out)
+		}
+	}
+
+	// Use a sibling git worktree so the real git binary (used for setup)
+	// knows the branch is checked out elsewhere. This mirrors what the user
+	// hit: another task's worktree holds the branch.
+	sibling := filepath.Join(t.TempDir(), "sibling")
+	siblingCmd := exec.Command("git", "worktree", "add", sibling, "feature/shared")
+	siblingCmd.Dir = repoDir
+	siblingCmd.Env = env
+	if out, err := siblingCmd.CombinedOutput(); err != nil {
+		t.Fatalf("git worktree add failed: %s", out)
+	}
+	t.Cleanup(func() {
+		cleanupCmd := exec.Command("git", "worktree", "remove", "--force", sibling)
+		cleanupCmd.Dir = repoDir
+		cleanupCmd.Env = env
+		_ = cleanupCmd.Run()
+	})
+
+	// Snapshot HEAD + index before the failing prepare so we can prove the
+	// shared checkout was untouched.
+	headBefore := readHeadSHA(t, repoDir)
+	indexBefore := readIndexSHA(t, repoDir)
+	branchBefore := currentBranch(t, repoDir)
+
+	// Fake only fetch: it supplies the missing-remote-ref text that triggers
+	// the substring match, then lets the real checkout detect the sibling
+	// worktree and emit its "already checked out at" diagnostic. Pass through
+	// everything else (symbolic-ref, rev-parse, ls-files, worktree, etc.) by
+	// exec'ing the real git at its absolute path — re-resolving via PATH would
+	// loop back to this fake.
+	realGit, err := exec.LookPath("git")
+	if err != nil {
+		t.Fatalf("locate real git: %v", err)
+	}
+	fakeBin := t.TempDir()
+	fakeGit := filepath.Join(fakeBin, "git")
+	script := `#!/bin/sh
+case "$1" in
+fetch)
+  echo "fatal: couldn't find remote ref feature/shared" >&2
+  exit 128
+  ;;
+esac
+exec "` + realGit + `" "$@"
+`
+	if err := os.WriteFile(fakeGit, []byte(script), 0o755); err != nil {
+		t.Fatalf("write fake git: %v", err)
+	}
+	t.Setenv("PATH", fakeBin+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	result, err := NewLocalPreparer(newTestLocalLogger()).Prepare(context.Background(), &EnvPrepareRequest{
+		TaskID:         "task-2",
+		RepositoryPath: repoDir,
+		BaseBranch:     "feature/shared",
+	}, nil)
+	if err == nil {
+		t.Fatal("expected Prepare() to fail when branch is checked out elsewhere")
+	}
+	if result.Success {
+		t.Fatalf("expected result.Success = false; error=%v", err)
+	}
+	if !errors.Is(err, worktree.ErrBranchCheckedOut) {
+		t.Fatalf("expected error to wrap worktree.ErrBranchCheckedOut, got %v", err)
+	}
+	// Combined fetch+checkout stderr must remain visible (the operator still
+	// needs to see what happened) — this is the same string content that
+	// previously fooled isMissingBranchError into a false match. The typed
+	// sentinel is what prevents the misclassification.
+	if !strings.Contains(err.Error(), "couldn't find remote ref") {
+		t.Fatalf("expected fetch stderr to be preserved in error chain, got %v", err)
+	}
+	if !strings.Contains(err.Error(), "already checked out at") &&
+		!strings.Contains(err.Error(), "already used by worktree at") {
+		t.Fatalf("expected real checkout stderr to be preserved in error chain, got %v", err)
+	}
+
+	if got := currentBranch(t, repoDir); got != branchBefore {
+		t.Fatalf("local preparer changed branch: was %q, now %q", branchBefore, got)
+	}
+	if got := readHeadSHA(t, repoDir); got != headBefore {
+		t.Fatalf("local preparer changed HEAD: was %s, now %s", headBefore, got)
+	}
+	if got := readIndexSHA(t, repoDir); got != indexBefore {
+		t.Fatalf("local preparer changed index: was %s, now %s", indexBefore, got)
+	}
+}
+
+func readHeadSHA(t *testing.T, repoDir string) string {
+	t.Helper()
+	cmd := exec.Command("git", "rev-parse", "HEAD")
+	cmd.Dir = repoDir
+	out, err := cmd.Output()
+	if err != nil {
+		t.Fatalf("git rev-parse HEAD failed: %v", err)
+	}
+	return strings.TrimSpace(string(out))
+}
+
+func readIndexSHA(t *testing.T, repoDir string) string {
+	t.Helper()
+	cmd := exec.Command("git", "ls-files", "--stage")
+	cmd.Dir = repoDir
+	out, err := cmd.Output()
+	if err != nil {
+		t.Fatalf("git ls-files failed: %v", err)
+	}
+	hash := sha1.New()
+	hash.Write(out)
+	return hex.EncodeToString(hash.Sum(nil))
+}

apps/backend/internal/orchestrator/service.go

@@ -40,6 +40,7 @@ import (
 	taskservice "github.com/kandev/kandev/internal/task/service"
 	"github.com/kandev/kandev/internal/workflow/engine"
 	wfmodels "github.com/kandev/kandev/internal/workflow/models"
+	"github.com/kandev/kandev/internal/worktree"
 	v1 "github.com/kandev/kandev/pkg/api/v1"
 )
 
@@ -2603,6 +2604,15 @@ func canResumeRunning(running *models.ExecutorRunning) bool {
 }
 
 func isMissingBranchError(err error) bool {
+	// A checked-out-elsewhere error chain can still mention "couldn't find
+	// remote ref" inside its concatenated fetch+checkout stderr, so the
+	// substring match below would misclassify it as a missing branch and
+	// post PR-recovery guidance for a branch that is in fact present
+	// locally. The local preparer wraps worktree.ErrBranchCheckedOut for
+	// that case; honor the typed sentinel first.
+	if errors.Is(err, worktree.ErrBranchCheckedOut) {
+		return false
+	}
 	for current := err; current != nil; current = errors.Unwrap(current) {
 		if isMissingBranchMessage(current.Error()) {
 			return true

apps/backend/internal/orchestrator/task_launch_failure_test.go

@@ -3,12 +3,14 @@ package orchestrator
 import (
 	"context"
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/kandev/kandev/internal/github"
 	"github.com/kandev/kandev/internal/task/models"
+	"github.com/kandev/kandev/internal/worktree"
 	v1 "github.com/kandev/kandev/pkg/api/v1"
 )
 
@@ -664,3 +666,120 @@ func TestHandleSessionLaunchFailed_IgnoresPathspecAfterFetchFailure(t *testing.T
 		t.Fatalf("expected no guidance message after fetch failure, got %d", len(mc.sessionMessages))
 	}
 }
+
+// branchCheckedOutElsewhereCombinedError reproduces the error chain that the
+// local environment preparer produces when a target branch is already checked
+// out in another worktree and the remote no longer carries the ref. Both
+// stderr fragments end up concatenated in a single error string:
+//
+//   - "fetch branch failed: ... couldn't find remote ref ..." (from the fetch
+//     step)
+//   - "checkout branch failed: ... is already checked out at ..." (from the
+//     checkout step that ClassifyGitError maps to ErrBranchCheckedOut)
+//
+// The wrapping layers are "checkout branch: <classified>" and finally
+// "environment preparation failed: <err>" added by the launch manager.
+func branchCheckedOutElsewhereCombinedError(branch, siblingPath string) error {
+	combined := fmt.Sprintf(
+		"fetch branch failed: exit status 128: fatal: couldn't find remote ref %s\n"+
+			"checkout branch failed: fatal: '%s' is already checked out at '%s'",
+		branch, branch, siblingPath,
+	)
+	return fmt.Errorf("environment preparation failed: checkout branch: %w: %s",
+		fmt.Errorf("%w: %s", worktree.ErrBranchCheckedOut, combined),
+		combined,
+	)
+}
+
+// TestIsMissingBranchError_DoesNotMatchCheckedOutElsewhere is the
+// orchestrator-level regression for the local-preparer misclassification.
+// The combined fetch+checkout error contains "couldn't find remote ref"
+// inside its message, but the chain wraps worktree.ErrBranchCheckedOut —
+// isMissingBranchError must not misclassify it as a missing-branch failure.
+func TestIsMissingBranchError_DoesNotMatchCheckedOutElsewhere(t *testing.T) {
+	err := branchCheckedOutElsewhereCombinedError("feature/shared", "/tmp/sibling")
+	if !errors.Is(err, worktree.ErrBranchCheckedOut) {
+		t.Fatalf("expected chain to wrap worktree.ErrBranchCheckedOut, got %v", err)
+	}
+	if !strings.Contains(err.Error(), "couldn't find remote ref") {
+		t.Fatalf("expected combined message to contain fetch stderr, got %v", err)
+	}
+	if isMissingBranchError(err) {
+		t.Fatalf("isMissingBranchError misclassified the checked-out-elsewhere error as a missing branch: %v", err)
+	}
+}
+
+// TestExtractMissingBranchName_ExtractsCombinedDiagnostic documents that the
+// extractor still returns the branch from a combined fetch and checkout
+// diagnostic. The caller must classify the typed sentinel before using it.
+func TestExtractMissingBranchName_ExtractsCombinedDiagnostic(t *testing.T) {
+	err := branchCheckedOutElsewhereCombinedError("feature/shared", "/tmp/sibling")
+	if got := extractMissingBranchName(err); got != "feature/shared" {
+		t.Fatalf("extractMissingBranchName() = %q, want %q", got, "feature/shared")
+	}
+}
+
+// TestHandleSessionLaunchFailed_BranchCheckedOutElsewhereDoesNotCreateFetchGuidance
+// is the end-to-end orchestrator regression. When the launch manager surfaces
+// a checked-out-elsewhere error, the orchestrator must not post a
+// branch_fetch_failed / missing-branch guidance message — the branch is not
+// missing, only checked out in another worktree.
+func TestHandleSessionLaunchFailed_BranchCheckedOutElsewhereDoesNotCreateFetchGuidance(t *testing.T) {
+	ctx := context.Background()
+	repo := setupTestRepo(t)
+	seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateFailed)
+
+	mc := &mockMessageCreator{}
+	svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo())
+	svc.messageCreator = mc
+	svc.SetGitHubService(&mockGitHubService{})
+
+	err := branchCheckedOutElsewhereCombinedError("feature/shared", "/tmp/sibling")
+	svc.handleSessionLaunchFailed(ctx, "task1", "session1", "repo-a", err)
+
+	if len(mc.sessionMessages) != 0 {
+		var kinds []string
+		for _, m := range mc.sessionMessages {
+			if k, ok := m.metadata["failure_kind"].(string); ok {
+				kinds = append(kinds, k)
+			}
+		}
+		t.Fatalf("expected no session guidance for checked-out-elsewhere error, got %d messages (failure_kinds=%v)",
+			len(mc.sessionMessages), kinds)
+	}
+	if _, ok := svc.suppressToast.Load("session1"); ok {
+		t.Fatal("expected no missing-branch toast suppression for a checked-out-elsewhere error")
+	}
+}
+
+// TestHandleSessionLaunchFailed_StillCreatesFetchGuidanceForRealMissingBranch
+// pins the positive direction: a genuine missing-branch error (no
+// ErrBranchCheckedOut in the chain) must still produce the
+// branch_fetch_failed guidance. The orchestrator fix must not over-correct.
+func TestHandleSessionLaunchFailed_StillCreatesFetchGuidanceForRealMissingBranch(t *testing.T) {
+	ctx := context.Background()
+	repo := setupTestRepo(t)
+	seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateFailed)
+
+	mc := &mockMessageCreator{}
+	svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo())
+	svc.messageCreator = mc
+	svc.SetGitHubService(&mockGitHubService{})
+
+	// Plain "couldn't find remote ref" without any ErrBranchCheckedOut — the
+	// remote branch really is gone and the local branch does not exist
+	// either, so the guidance must still fire.
+	err := errors.New("environment preparation failed: branch \"feature/deleted\" not found locally or on remote: fatal: couldn't find remote ref feature/deleted")
+	if !isMissingBranchError(err) {
+		t.Fatalf("expected genuine missing-branch error to still be classified, got %v", err)
+	}
+
+	svc.handleSessionLaunchFailed(ctx, "task1", "session1", "repo-a", err)
+
+	if len(mc.sessionMessages) != 1 {
+		t.Fatalf("expected 1 guidance message for genuine missing branch, got %d", len(mc.sessionMessages))
+	}
+	if kind, ok := mc.sessionMessages[0].metadata["failure_kind"].(string); !ok || kind != "branch_fetch_failed" {
+		t.Fatalf("expected failure_kind=branch_fetch_failed, got %#v", mc.sessionMessages[0].metadata["failure_kind"])
+	}
+}