From e88e7af0fbec53768d2695766ce76aad90023fa0 Mon Sep 17 00:00:00 2001 From: WaleWangPW <396218656@qq.com> Date: Wed, 19 Aug 2026 06:25:15 +0800 Subject: [PATCH] fix(orchestrator): do not misclassify branch-checked-out-elsewhere as a missing branch (#2782) Co-authored-by: Carlos Florencio --- .../runtime/lifecycle/env_preparer_local.go | 18 ++- .../lifecycle/env_preparer_local_test.go | 151 ++++++++++++++++++ apps/backend/internal/orchestrator/service.go | 10 ++ .../orchestrator/task_launch_failure_test.go | 119 ++++++++++++++ 4 files changed, 296 insertions(+), 2 deletions(-) diff --git a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go index a6cb5e727d..19298fe2d8 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go +++ b/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) diff --git a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local_test.go b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local_test.go index 30dca34be3..86ac9cf237 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local_test.go +++ b/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)) +} diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index c8e6842074..0c133ca39e 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/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 diff --git a/apps/backend/internal/orchestrator/task_launch_failure_test.go b/apps/backend/internal/orchestrator/task_launch_failure_test.go index 5ca1072975..509a949a91 100644 --- a/apps/backend/internal/orchestrator/task_launch_failure_test.go +++ b/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: " and finally +// "environment preparation failed: " 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"]) + } +}