diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index 90ad2576f6..f6cdf97b0a 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -360,6 +360,7 @@ func (s *Service) backfillRepoProvider(store repoStore, repo *models.Repository) // ensureSessionPRWatch creates a PRWatch (pr_number=0) for a session's branch // so the poller will search for a PR on GitHub. Runs as a background goroutine. func (s *Service) ensureSessionPRWatch(ctx context.Context, taskID, sessionID, branch string) { + branch = s.resolvePRWatchBranch(ctx, taskID, sessionID, branch) if s.githubService == nil || branch == "" { return } @@ -377,6 +378,33 @@ func (s *Service) ensureSessionPRWatch(ctx context.Context, taskID, sessionID, b } } +func (s *Service) resolvePRWatchBranch(ctx context.Context, taskID, sessionID, fallback string) string { + store, ok := s.repo.(repoStore) + if !ok { + return fallback + } + + taskRepo, err := store.GetPrimaryTaskRepository(ctx, taskID) + if err == nil && taskRepo != nil && strings.TrimSpace(taskRepo.CheckoutBranch) != "" { + return strings.TrimSpace(taskRepo.CheckoutBranch) + } + + if fallback != "" { + return fallback + } + + session, err := s.repo.GetTaskSession(ctx, sessionID) + if err != nil || session == nil { + return "" + } + for _, wt := range session.Worktrees { + if wt.WorktreeBranch != "" { + return wt.WorktreeBranch + } + } + return "" +} + // resolveTaskRepo looks up the GitHub owner and repo name for a task's primary repository. // If provider info is missing but a local path exists, it detects from the git remote // and backfills the DB record for future calls. @@ -449,18 +477,7 @@ func (s *Service) CheckSessionPR(ctx context.Context, taskID, sessionID string) return false, nil } - // Get the session's branch from the first worktree with a non-empty branch - session, err := s.repo.GetTaskSession(ctx, sessionID) - if err != nil || session == nil { - return false, nil - } - branch := "" - for _, wt := range session.Worktrees { - if wt.WorktreeBranch != "" { - branch = wt.WorktreeBranch - break - } - } + branch := s.resolvePRWatchBranch(ctx, taskID, sessionID, "") if branch == "" { return false, nil } diff --git a/apps/backend/internal/orchestrator/event_handlers_github_test.go b/apps/backend/internal/orchestrator/event_handlers_github_test.go index f7c2adbb2a..8f161f3b3c 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_test.go @@ -11,27 +11,31 @@ import ( // mockGitHubService implements GitHubService for testing CheckSessionPR. type mockGitHubService struct { - client github.Client - taskPR *github.TaskPR - taskPRErr error - ensureWatchCalls int - createWatchCalls int - associateCalls int + client github.Client + taskPR *github.TaskPR + taskPRErr error + ensureWatchCalls int + createWatchCalls int + associateCalls int + ensureWatchBranch string + createWatchBranch string } func (m *mockGitHubService) Client() github.Client { return m.client } func (m *mockGitHubService) GetTaskPR(_ context.Context, _ string) (*github.TaskPR, error) { return m.taskPR, m.taskPRErr } -func (m *mockGitHubService) EnsurePRWatch(_ context.Context, _, _, _, _, _ string) (*github.PRWatch, error) { +func (m *mockGitHubService) EnsurePRWatch(_ context.Context, _, _, _, _, branch string) (*github.PRWatch, error) { m.ensureWatchCalls++ + m.ensureWatchBranch = branch return &github.PRWatch{}, nil } func (m *mockGitHubService) GetPRWatchBySession(_ context.Context, _ string) (*github.PRWatch, error) { return nil, nil } -func (m *mockGitHubService) CreatePRWatch(_ context.Context, _, _, _, _ string, _ int, _ string) (*github.PRWatch, error) { +func (m *mockGitHubService) CreatePRWatch(_ context.Context, _, _, _, _ string, _ int, branch string) (*github.PRWatch, error) { m.createWatchCalls++ + m.createWatchBranch = branch return &github.PRWatch{}, nil } func (m *mockGitHubService) AssociatePRWithTask(_ context.Context, _ string, _ *github.PR) (*github.TaskPR, error) { @@ -108,7 +112,7 @@ func TestCheckSessionPR(t *testing.T) { // seedWithRepo creates task + session + repository + task-repository + worktree // so that resolveTaskRepo and GetTaskSession succeed. - seedWithRepo := func(t *testing.T, branch string) *Service { + seedWithRepo := func(t *testing.T, branch, checkoutBranch string) *Service { t.Helper() repo := setupTestRepo(t) seedSession(t, repo, "t1", "s1", "step1") @@ -131,12 +135,13 @@ func TestCheckSessionPR(t *testing.T) { // Link task to repository taskRepo := &models.TaskRepository{ - ID: "tr1", - TaskID: "t1", - RepositoryID: "repo1", - Position: 0, - CreatedAt: now, - UpdatedAt: now, + ID: "tr1", + TaskID: "t1", + RepositoryID: "repo1", + CheckoutBranch: checkoutBranch, + Position: 0, + CreatedAt: now, + UpdatedAt: now, } if err := repo.CreateTaskRepository(ctx, taskRepo); err != nil { t.Fatalf("failed to create task repository: %v", err) @@ -210,7 +215,7 @@ func TestCheckSessionPR(t *testing.T) { }) t.Run("returns false when session has no worktree branch", func(t *testing.T) { - svc := seedWithRepo(t, "") // empty branch + svc := seedWithRepo(t, "", "") // empty branch ghSvc := &mockGitHubService{taskPRErr: nil, taskPR: nil} svc.SetGitHubService(ghSvc) @@ -224,7 +229,7 @@ func TestCheckSessionPR(t *testing.T) { }) t.Run("returns false when no PR exists on branch", func(t *testing.T) { - svc := seedWithRepo(t, "feature-branch") + svc := seedWithRepo(t, "feature-branch", "") mockClient := github.NewMockClient() // No PR added to mock client ghSvc := &mockGitHubService{client: mockClient} @@ -240,10 +245,13 @@ func TestCheckSessionPR(t *testing.T) { if ghSvc.ensureWatchCalls != 1 { t.Errorf("expected 1 EnsurePRWatch call, got %d", ghSvc.ensureWatchCalls) } + if ghSvc.ensureWatchBranch != "feature-branch" { + t.Errorf("expected EnsurePRWatch branch %q, got %q", "feature-branch", ghSvc.ensureWatchBranch) + } }) t.Run("finds PR and associates it", func(t *testing.T) { - svc := seedWithRepo(t, "feature-branch") + svc := seedWithRepo(t, "feature-branch", "") mockClient := github.NewMockClient() mockClient.AddPR(testPR) ghSvc := &mockGitHubService{client: mockClient} @@ -262,8 +270,51 @@ func TestCheckSessionPR(t *testing.T) { if ghSvc.createWatchCalls != 1 { t.Errorf("expected 1 CreatePRWatch call (from associatePRFromPush), got %d", ghSvc.createWatchCalls) } + if ghSvc.ensureWatchBranch != "feature-branch" { + t.Errorf("expected EnsurePRWatch branch %q, got %q", "feature-branch", ghSvc.ensureWatchBranch) + } + if ghSvc.createWatchBranch != "feature-branch" { + t.Errorf("expected CreatePRWatch branch %q, got %q", "feature-branch", ghSvc.createWatchBranch) + } if ghSvc.associateCalls != 1 { t.Errorf("expected 1 AssociatePRWithTask call, got %d", ghSvc.associateCalls) } }) + + t.Run("prefers checkout branch over synthetic worktree branch", func(t *testing.T) { + svc := seedWithRepo(t, "kandev/pr-review-abc", "feature-branch") + mockClient := github.NewMockClient() + mockClient.AddPR(testPR) + ghSvc := &mockGitHubService{client: mockClient} + svc.SetGitHubService(ghSvc) + + found, err := svc.CheckSessionPR(ctx, "t1", "s1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !found { + t.Error("expected found=true when PR exists on checkout branch") + } + if ghSvc.ensureWatchBranch != "feature-branch" { + t.Errorf("expected EnsurePRWatch branch %q, got %q", "feature-branch", ghSvc.ensureWatchBranch) + } + if ghSvc.createWatchBranch != "feature-branch" { + t.Errorf("expected CreatePRWatch branch %q, got %q", "feature-branch", ghSvc.createWatchBranch) + } + }) + + t.Run("ensureSessionPRWatch prefers checkout branch", func(t *testing.T) { + svc := seedWithRepo(t, "kandev/pr-review-abc", "feature-branch") + ghSvc := &mockGitHubService{} + svc.SetGitHubService(ghSvc) + + svc.ensureSessionPRWatch(ctx, "t1", "s1", "kandev/pr-review-abc") + + if ghSvc.ensureWatchCalls != 1 { + t.Errorf("expected 1 EnsurePRWatch call, got %d", ghSvc.ensureWatchCalls) + } + if ghSvc.ensureWatchBranch != "feature-branch" { + t.Errorf("expected EnsurePRWatch branch %q, got %q", "feature-branch", ghSvc.ensureWatchBranch) + } + }) } diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 0174799914..c2f9af9d36 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -74,11 +74,10 @@ func (s *Service) EnqueueTask(ctx context.Context, task *v1.Task) error { } // PrepareTaskSession creates a session entry without launching the agent. -// This allows the HTTP handler to return the session ID immediately while the agent setup +// This allows the WS handler to return the session ID immediately while workspace setup // continues in the background. Use StartTaskWithSession to continue with agent launch. -// When launchWorkspace is true, workspace infrastructure (agentctl) is launched synchronously -// so file browsing works immediately. When false, the workspace launch is deferred to -// StartTaskWithSession (useful for remote executors where provisioning takes 30-60s). +// When launchWorkspace is true, workspace infrastructure (agentctl) is launched asynchronously; +// the frontend receives preparation progress via executor.prepare.progress WS events. func (s *Service) PrepareTaskSession(ctx context.Context, taskID string, agentProfileID string, executorID string, executorProfileID string, workflowStepID string, launchWorkspace bool) (string, error) { s.logger.Debug("preparing task session", zap.String("task_id", taskID), @@ -133,17 +132,23 @@ func (s *Service) PrepareTaskSession(ctx context.Context, taskID string, agentPr } if launchWorkspace { - // Launch workspace infrastructure (agentctl) without starting the agent subprocess. - // This enables file browsing, editing, etc. while the session is in CREATED state. - if prepExec, launchErr := s.executor.LaunchPreparedSession(ctx, task, sessionID, executor.LaunchOptions{AgentProfileID: agentProfileID, ExecutorID: executorID, WorkflowStepID: workflowStepID}); launchErr != nil { - s.logger.Warn("failed to launch workspace for prepared session (file browsing may be unavailable)", - zap.String("task_id", taskID), - zap.String("session_id", sessionID), - zap.Error(launchErr)) - // Non-fatal: session is still usable, workspace will be launched when agent starts - } else if prepExec != nil && prepExec.WorktreeBranch != "" { - go s.ensureSessionPRWatch(context.Background(), taskID, prepExec.SessionID, prepExec.WorktreeBranch) - } + // Launch workspace infrastructure (agentctl) in the background so the WS response + // returns the session ID immediately. The frontend navigates to the session page + // and shows preparation progress via executor.prepare.progress WS events. + go func() { + bgCtx := context.Background() + prepExec, launchErr := s.executor.LaunchPreparedSession(bgCtx, task, sessionID, executor.LaunchOptions{AgentProfileID: agentProfileID, ExecutorID: executorID, WorkflowStepID: workflowStepID}) + if launchErr != nil { + s.logger.Warn("failed to launch workspace for prepared session (file browsing may be unavailable)", + zap.String("task_id", taskID), + zap.String("session_id", sessionID), + zap.Error(launchErr)) + return + } + if prepExec != nil && prepExec.WorktreeBranch != "" { + s.ensureSessionPRWatch(bgCtx, taskID, prepExec.SessionID, prepExec.WorktreeBranch) + } + }() } s.logger.Info("task session prepared", @@ -475,9 +480,11 @@ func (s *Service) recordInitialMessage(ctx context.Context, taskID, sessionID, p // buildWorkflowPrompt constructs the effective prompt using workflow step configuration. // If step.Prompt contains {{task_prompt}}, it is replaced with the base prompt. -// Otherwise, step.Prompt is prepended to the base prompt. -// System-injected content is wrapped in tags so it can be stripped when displaying to users. +// Otherwise, step.Prompt fully replaces the base prompt. +// If the step has enable_plan_mode in on_enter events, plan mode prefix is also prepended. +// Only true internal instructions are wrapped in tags so they can be stripped from the visible chat. func (s *Service) buildWorkflowPrompt(basePrompt string, step *wfmodels.WorkflowStep, taskID string, sessionID string) string { + _ = sessionID var parts []string // Build the prompt from step.Prompt template and base prompt @@ -488,9 +495,8 @@ func (s *Service) buildWorkflowPrompt(basePrompt string, step *wfmodels.Workflow combined := strings.Replace(interpolatedPrompt, "{{task_prompt}}", basePrompt, 1) parts = append(parts, combined) } else { - // Prepend step prompt, then base prompt - parts = append(parts, sysprompt.Wrap(interpolatedPrompt)) - parts = append(parts, basePrompt) + // A step prompt without {{task_prompt}} is treated as the full visible prompt. + parts = append(parts, interpolatedPrompt) } } else { // No step prompt, just use base prompt diff --git a/apps/backend/internal/orchestrator/workflow_prompt_test.go b/apps/backend/internal/orchestrator/workflow_prompt_test.go new file mode 100644 index 0000000000..24641f87b8 --- /dev/null +++ b/apps/backend/internal/orchestrator/workflow_prompt_test.go @@ -0,0 +1,74 @@ +package orchestrator + +import ( + "context" + "strings" + "testing" + + wfmodels "github.com/kandev/kandev/internal/workflow/models" +) + +func TestBuildWorkflowPrompt_ReplacesTaskPromptPlaceholder(t *testing.T) { + svc := createTestService(setupTestRepo(t), newMockStepGetter(), newMockTaskRepo()) + step := &wfmodels.WorkflowStep{ + ID: "step-1", + Prompt: "Implement this exactly:\n\n{{task_prompt}}", + } + + got := svc.buildWorkflowPrompt("Migrate Atlantis datasource.", step, "task-1", "session-1") + + want := "Implement this exactly:\n\nMigrate Atlantis datasource." + if got != want { + t.Fatalf("buildWorkflowPrompt() = %q, want %q", got, want) + } +} + +func TestBuildWorkflowPrompt_UsesStepPromptOnlyWithoutTaskPromptPlaceholder(t *testing.T) { + svc := createTestService(setupTestRepo(t), newMockStepGetter(), newMockTaskRepo()) + step := &wfmodels.WorkflowStep{ + ID: "step-1", + Prompt: "Commit the changes, push and create a draft PR.", + } + + got := svc.buildWorkflowPrompt("Migrate Atlantis datasource.", step, "task-1", "session-1") + + want := "Commit the changes, push and create a draft PR." + if got != want { + t.Fatalf("buildWorkflowPrompt() = %q, want %q", got, want) + } +} + +func TestApplyWorkflowAndPlanMode_KeepsWorkflowPromptVisibleWhenStepEnablesPlanMode(t *testing.T) { + repo := setupTestRepo(t) + stepGetter := newMockStepGetter() + stepGetter.steps["step-1"] = &wfmodels.WorkflowStep{ + ID: "step-1", + Prompt: "Commit the changes, push and create a draft PR.", + Events: wfmodels.StepEvents{ + OnEnter: []wfmodels.OnEnterAction{{Type: wfmodels.OnEnterEnablePlanMode}}, + }, + } + svc := createTestService(repo, stepGetter, newMockTaskRepo()) + + got, planModeActive := svc.applyWorkflowAndPlanMode( + context.Background(), + "Migrate Atlantis datasource.", + "task-1", + "session-1", + "step-1", + false, + ) + + if !planModeActive { + t.Fatal("expected plan mode to be active") + } + if !strings.Contains(got, "Commit the changes, push and create a draft PR.") { + t.Fatalf("expected visible workflow prompt in effective prompt, got %q", got) + } + if strings.Contains(got, "Migrate Atlantis datasource.") { + t.Fatalf("expected base prompt to be omitted when step prompt lacks {{task_prompt}}, got %q", got) + } + if strings.Contains(got, "") { + t.Fatalf("expected workflow prompt to remain visible without hidden system wrapping, got %q", got) + } +} diff --git a/apps/backend/internal/task/models/message_test.go b/apps/backend/internal/task/models/message_test.go new file mode 100644 index 0000000000..bcbe5633c6 --- /dev/null +++ b/apps/backend/internal/task/models/message_test.go @@ -0,0 +1,53 @@ +package models + +import ( + "testing" + "time" +) + +func TestMessageToAPI_StripsOnlySystemContent(t *testing.T) { + msg := &Message{ + ID: "msg-1", + TaskSessionID: "session-1", + TaskID: "task-1", + TurnID: "turn-1", + AuthorType: MessageAuthorUser, + Content: "PLAN MODE ACTIVE\n\nCommit the changes, push and create a draft PR.", + Type: MessageTypeMessage, + CreatedAt: time.Now().UTC(), + } + + apiMsg := msg.ToAPI() + + if apiMsg.Content != "Commit the changes, push and create a draft PR." { + t.Fatalf("expected visible content to keep workflow prompt, got %q", apiMsg.Content) + } + if apiMsg.RawContent != msg.Content { + t.Fatalf("expected raw_content to preserve original content, got %q", apiMsg.RawContent) + } + if apiMsg.Metadata["has_hidden_prompts"] != true { + t.Fatalf("expected has_hidden_prompts metadata, got %v", apiMsg.Metadata["has_hidden_prompts"]) + } +} + +func TestMessageToAPI_KeepsWorkflowPromptVisibleWithoutSystemTags(t *testing.T) { + msg := &Message{ + ID: "msg-1", + TaskSessionID: "session-1", + TaskID: "task-1", + TurnID: "turn-1", + AuthorType: MessageAuthorUser, + Content: "Commit the changes, push and create a draft PR.", + Type: MessageTypeMessage, + CreatedAt: time.Now().UTC(), + } + + apiMsg := msg.ToAPI() + + if apiMsg.Content != msg.Content { + t.Fatalf("expected visible content %q, got %q", msg.Content, apiMsg.Content) + } + if apiMsg.RawContent != "" { + t.Fatalf("expected empty raw_content when there is no hidden prompt, got %q", apiMsg.RawContent) + } +} diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index 0b963725d7..5a122513c1 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -141,8 +141,8 @@ type Message struct { } // ToAPI converts internal Message to API type. -// System-injected content (wrapped in tags) is stripped from the content -// so users don't see workflow step prompt modifications in the UI. +// Only true system-injected content (wrapped in tags) is stripped +// from the visible content sent to the UI. func (m *Message) ToAPI() *v1.Message { messageType := string(m.Type) if messageType == "" { diff --git a/apps/backend/internal/task/service/service_events.go b/apps/backend/internal/task/service/service_events.go index 818bebe87c..a95053294c 100644 --- a/apps/backend/internal/task/service/service_events.go +++ b/apps/backend/internal/task/service/service_events.go @@ -214,8 +214,8 @@ func (s *Service) publishEnvironmentEvent(ctx context.Context, eventType string, } // publishMessageEvent publishes message events to the event bus. -// System-injected content (wrapped in tags) is stripped from the content -// so users don't see workflow step prompt modifications in the UI. +// Only true system-injected content (wrapped in tags) is stripped +// from the visible message content delivered to clients. func (s *Service) publishMessageEvent(ctx context.Context, eventType string, message *models.Message) { if s.eventBus == nil { s.logger.Warn("publishMessageEvent: eventBus is nil, skipping") diff --git a/apps/backend/internal/worktree/errors.go b/apps/backend/internal/worktree/errors.go index 6ac5f3e4b6..48a9cc535e 100644 --- a/apps/backend/internal/worktree/errors.go +++ b/apps/backend/internal/worktree/errors.go @@ -54,18 +54,26 @@ func containsAuthFailure(lowerOutput string) bool { strings.Contains(lowerOutput, "askpass") } +// isBranchCheckedOutError checks if git output indicates a branch is already +// checked out in another worktree. Different git versions use different messages: +// "is already checked out at" or "is already used by worktree at". +func isBranchCheckedOutError(output string) bool { + out := strings.ToLower(output) + return strings.Contains(out, "is already checked out at") || + strings.Contains(out, "is already used by worktree at") +} + // ClassifyGitError wraps a raw git error with a user-friendly sentinel error // based on the command output. func ClassifyGitError(output string, _ error) error { - out := strings.ToLower(output) trimmed := strings.TrimSpace(output) switch { - case strings.Contains(out, "is already checked out at"): + case isBranchCheckedOutError(output): return fmt.Errorf("%w: %s", ErrBranchCheckedOut, trimmed) - case containsAuthFailure(out): + case containsAuthFailure(strings.ToLower(output)): return fmt.Errorf("%w: %s", ErrAuthFailed, trimmed) - case strings.Contains(out, "non-fast-forward"): + case strings.Contains(strings.ToLower(output), "non-fast-forward"): return fmt.Errorf("%w: %s", ErrNonFastForward, trimmed) default: return fmt.Errorf("%w: %s", ErrGitCommandFailed, trimmed) diff --git a/apps/backend/internal/worktree/manager.go b/apps/backend/internal/worktree/manager.go index 23dd4f4e15..b92552a744 100644 --- a/apps/backend/internal/worktree/manager.go +++ b/apps/backend/internal/worktree/manager.go @@ -250,9 +250,11 @@ func (m *Manager) Create(ctx context.Context, req CreateRequest) (*Worktree, err // createWorktree performs the actual git worktree creation. func (m *Manager) createWorktree(ctx context.Context, req CreateRequest, baseRef string) (*Worktree, error) { worktreeDirName, branchName := m.buildWorktreeNames(req) + startPoint := baseRef // If a checkout branch is specified (e.g. a PR's head branch), fetch it - // and checkout it directly in the worktree instead of creating a new branch. + // and try to check it out directly. If the branch is already checked out + // in another worktree, fall back to a unique branch with a random suffix. var fetchResult *FetchBranchResult if req.CheckoutBranch != "" { var err error @@ -260,7 +262,7 @@ func (m *Manager) createWorktree(ctx context.Context, req CreateRequest, baseRef if err != nil { return nil, err } - branchName = req.CheckoutBranch + startPoint = req.CheckoutBranch } worktreePath, err := m.config.WorktreePath(worktreeDirName) @@ -268,14 +270,7 @@ func (m *Manager) createWorktree(ctx context.Context, req CreateRequest, baseRef return nil, fmt.Errorf("failed to get worktree path: %w", err) } - var worktreeID string - if req.CheckoutBranch != "" { - // Checkout the existing PR branch directly in the worktree. - worktreeID, err = m.gitAddWorktreeExisting(ctx, req.RepositoryPath, branchName, worktreePath) - } else { - // Create a new branch based on the base ref. - worktreeID, err = m.gitAddWorktree(ctx, req.RepositoryPath, branchName, worktreePath, baseRef) - } + worktreeID, branchName, err := m.addWorktreeForBranch(ctx, req, worktreePath, branchName, startPoint, baseRef) if err != nil { return nil, err } @@ -303,6 +298,30 @@ func (m *Manager) createWorktree(ctx context.Context, req CreateRequest, baseRef return wt, nil } +// addWorktreeForBranch creates the git worktree, trying the checkout branch directly first +// and falling back to a suffixed branch if the checkout branch is already in use. +func (m *Manager) addWorktreeForBranch(ctx context.Context, req CreateRequest, worktreePath, fallbackBranch, startPoint, baseRef string) (string, string, error) { + if req.CheckoutBranch == "" { + id, err := m.gitAddWorktree(ctx, req.RepositoryPath, fallbackBranch, worktreePath, baseRef) + return id, fallbackBranch, err + } + + // Try checking out the PR branch directly (common case: single task per PR). + id, err := m.gitAddWorktreeExisting(ctx, req.RepositoryPath, req.CheckoutBranch, worktreePath) + if err == nil { + return id, req.CheckoutBranch, nil + } + if !errors.Is(err, ErrBranchCheckedOut) { + return "", "", err + } + + // Branch is in use by another worktree — create a unique fallback branch + // using the original branch name with a random suffix. + suffixed := req.CheckoutBranch + "-" + SmallSuffix(3) + id, err = m.gitAddWorktree(ctx, req.RepositoryPath, suffixed, worktreePath, startPoint) + return id, suffixed, err +} + // FetchBranchResult holds the outcome of a fetchBranchToLocal call. type FetchBranchResult struct { Warning string // User-friendly warning (non-empty when fell back to local) @@ -362,25 +381,31 @@ func (m *Manager) gitAddWorktreeExisting(ctx context.Context, repoPath, branchNa } outStr := string(output) - if !strings.Contains(strings.ToLower(outStr), "is already checked out at") { + if !isBranchCheckedOutError(outStr) { m.logger.Error("git worktree add (existing branch) failed", zap.String("output", outStr), zap.Error(err)) return "", ClassifyGitError(outStr, err) } if recoveryErr := m.tryRecoverCheckedOutBranch(ctx, repoPath, branchName, outStr); recoveryErr != nil { - m.logger.Error("git worktree add (existing branch) failed", - zap.String("output", outStr), zap.Error(err)) - return "", ClassifyGitError(outStr, err) + // Branch is genuinely in use by an active worktree — return sentinel error + // so callers can fall back to creating a unique branch. + m.logger.Warn("branch is checked out in active worktree", + zap.String("branch", branchName), zap.Error(recoveryErr)) + return "", ErrBranchCheckedOut } // Retry after pruning stale worktree. retryCmd := exec.CommandContext(ctx, "git", "worktree", "add", worktreePath, branchName) retryCmd.Dir = repoPath if retryOutput, retryErr := retryCmd.CombinedOutput(); retryErr != nil { + retryOutStr := string(retryOutput) + if isBranchCheckedOutError(retryOutStr) { + return "", ErrBranchCheckedOut + } m.logger.Error("git worktree add retry failed", - zap.String("output", string(retryOutput)), zap.Error(retryErr)) - return "", ClassifyGitError(string(retryOutput), retryErr) + zap.String("output", retryOutStr), zap.Error(retryErr)) + return "", ClassifyGitError(retryOutStr, retryErr) } m.logger.Info("recovered from stale worktree checkout", zap.String("branch", branchName)) return worktreeID, nil @@ -420,19 +445,19 @@ func (m *Manager) tryRecoverCheckedOutBranch(ctx context.Context, repoPath, bran return nil } -// parseCheckedOutPath extracts the worktree path from git's -// "fatal: 'branch' is already checked out at '/path'" error message. +// parseCheckedOutPath extracts the worktree path from git's error message. +// Handles both "is already checked out at '/path'" and "is already used by worktree at '/path'". func parseCheckedOutPath(gitOutput string) string { - const marker = "checked out at '" - _, after, found := strings.Cut(gitOutput, marker) - if !found { - return "" + for _, marker := range []string{"checked out at '", "used by worktree at '"} { + _, after, found := strings.Cut(gitOutput, marker) + if found { + path, _, ok := strings.Cut(after, "'") + if ok { + return path + } + } } - path, _, found := strings.Cut(after, "'") - if !found { - return "" - } - return path + return "" } // buildWorktreeNames derives the filesystem directory name and git branch name for a new worktree. diff --git a/apps/backend/internal/worktree/manager_checkout_branch_test.go b/apps/backend/internal/worktree/manager_checkout_branch_test.go new file mode 100644 index 0000000000..d60fd0144c --- /dev/null +++ b/apps/backend/internal/worktree/manager_checkout_branch_test.go @@ -0,0 +1,143 @@ +package worktree + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestCreateWorktree_CheckoutBranchDirectCheckout(t *testing.T) { + cfg := newTestConfig(t) + log := newTestLogger() + store := newMockStore() + + mgr, err := NewManager(cfg, store, log) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + repoPath := initGitRepoForWorktreeTest(t) + + // First worktree with checkout branch should use the branch directly. + wt, err := mgr.Create(context.Background(), CreateRequest{ + TaskID: "task-1", + SessionID: "session-1", + TaskTitle: "PR Review", + RepositoryID: "repo-1", + RepositoryPath: repoPath, + BaseBranch: "main", + CheckoutBranch: "feature/pr-branch", + }) + if err != nil { + t.Fatalf("Create() unexpected error: %v", err) + } + if wt.Branch != "feature/pr-branch" { + t.Fatalf("expected direct checkout branch %q, got %q", "feature/pr-branch", wt.Branch) + } + if wt.FetchWarning == "" { + t.Fatal("expected fetch warning when origin is unavailable and local branch is reused") + } + + gotBranch := strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "--abbrev-ref", "HEAD")) + if gotBranch != "feature/pr-branch" { + t.Fatalf("worktree HEAD branch = %q, want %q", gotBranch, "feature/pr-branch") + } + + prHeadSHA := strings.TrimSpace(runGit(t, repoPath, "rev-parse", "feature/pr-branch")) + worktreeSHA := strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")) + if worktreeSHA != prHeadSHA { + t.Fatalf("worktree HEAD SHA = %q, want %q", worktreeSHA, prHeadSHA) + } +} + +func TestCreateWorktree_CheckoutBranchFallsBackToSuffix(t *testing.T) { + cfg := newTestConfig(t) + log := newTestLogger() + store := newMockStore() + + mgr, err := NewManager(cfg, store, log) + if err != nil { + t.Fatalf("NewManager failed: %v", err) + } + + repoPath := initGitRepoForWorktreeTest(t) + + // First worktree checks out the branch directly. + wt1, err := mgr.Create(context.Background(), CreateRequest{ + TaskID: "task-1", + SessionID: "session-1", + TaskTitle: "PR Review 1", + RepositoryID: "repo-1", + RepositoryPath: repoPath, + BaseBranch: "main", + CheckoutBranch: "feature/pr-branch", + }) + if err != nil { + t.Fatalf("Create() first worktree: %v", err) + } + if wt1.Branch != "feature/pr-branch" { + t.Fatalf("first worktree: expected direct branch, got %q", wt1.Branch) + } + + // Second worktree with the same checkout branch should fall back to a suffixed name. + wt2, err := mgr.Create(context.Background(), CreateRequest{ + TaskID: "task-2", + SessionID: "session-2", + TaskTitle: "PR Review 2", + RepositoryID: "repo-1", + RepositoryPath: repoPath, + BaseBranch: "main", + CheckoutBranch: "feature/pr-branch", + }) + if err != nil { + t.Fatalf("Create() second worktree: %v", err) + } + if wt2.Branch == "feature/pr-branch" { + t.Fatal("second worktree should NOT use the checkout branch directly") + } + if !strings.HasPrefix(wt2.Branch, "feature/pr-branch-") { + t.Fatalf("expected suffixed branch like %q, got %q", "feature/pr-branch-xxx", wt2.Branch) + } + + // Both worktrees should point to the same commit. + sha1 := strings.TrimSpace(runGit(t, wt1.Path, "rev-parse", "HEAD")) + sha2 := strings.TrimSpace(runGit(t, wt2.Path, "rev-parse", "HEAD")) + if sha1 != sha2 { + t.Fatalf("worktree SHAs differ: wt1=%q, wt2=%q", sha1, sha2) + } +} + +func initGitRepoForWorktreeTest(t *testing.T) string { + t.Helper() + + repoPath := t.TempDir() + runGit(t, repoPath, "init", "-b", "main") + runGit(t, repoPath, "config", "user.email", "test@example.com") + runGit(t, repoPath, "config", "user.name", "Test User") + runGit(t, repoPath, "config", "commit.gpgsign", "false") + + filePath := filepath.Join(repoPath, "README.md") + if err := os.WriteFile(filePath, []byte("initial\n"), 0644); err != nil { + t.Fatalf("failed to write README.md: %v", err) + } + runGit(t, repoPath, "add", "README.md") + runGit(t, repoPath, "commit", "-m", "initial commit") + runGit(t, repoPath, "branch", "feature/pr-branch") + + return repoPath +} + +func runGit(t *testing.T, repoPath string, args ...string) string { + t.Helper() + + cmd := exec.Command("git", args...) + cmd.Dir = repoPath + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s failed: %v\n%s", strings.Join(args, " "), err, string(output)) + } + return string(output) +} diff --git a/apps/backend/internal/worktree/worktree.go b/apps/backend/internal/worktree/worktree.go index cd997c8b34..8affc92e18 100644 --- a/apps/backend/internal/worktree/worktree.go +++ b/apps/backend/internal/worktree/worktree.go @@ -97,9 +97,9 @@ type CreateRequest struct { // Typically "main" or "master". BaseBranch string - // CheckoutBranch is a branch to fetch from origin and checkout after worktree creation. - // Used for PR reviews where the worktree is created from the default branch, then - // the PR's head branch is fetched and checked out. Empty means no post-creation checkout. + // CheckoutBranch is a branch to fetch from origin and check out directly in the + // worktree. If the branch is already checked out in another worktree, a unique + // fallback branch is created using the original name with a random suffix. CheckoutBranch string // WorktreeBranchPrefix is the prefix to use for the worktree branch name. diff --git a/apps/web/components/shared/mermaid-block.test.tsx b/apps/web/components/shared/mermaid-block.test.tsx new file mode 100644 index 0000000000..ebe085b3c6 --- /dev/null +++ b/apps/web/components/shared/mermaid-block.test.tsx @@ -0,0 +1,18 @@ +import { describe, expect, it, vi } from "vitest"; +import { renderToStaticMarkup } from "react-dom/server"; +import { MermaidBlock } from "./mermaid-block"; + +vi.mock("next-themes", () => ({ + useTheme: () => ({ resolvedTheme: "dark" }), +})); + +describe("MermaidBlock", () => { + it("renders a constrained outer container with a horizontal scroll viewport", () => { + const html = renderToStaticMarkup(B"} />); + + expect(html).toContain("mermaid-block"); + expect(html).toContain("block w-full max-w-full min-w-0"); + expect(html).toContain("mermaid-scroll-region w-full overflow-x-auto overflow-y-hidden"); + expect(html).not.toContain("inline-block"); + }); +}); diff --git a/apps/web/components/shared/mermaid-block.tsx b/apps/web/components/shared/mermaid-block.tsx index b2ec327c39..e85c3d9cc5 100644 --- a/apps/web/components/shared/mermaid-block.tsx +++ b/apps/web/components/shared/mermaid-block.tsx @@ -82,8 +82,11 @@ export function MermaidBlock({ code }: MermaidBlockProps) { }; return ( -
-
+
+
diff --git a/apps/web/e2e/tests/create-task-github-url.spec.ts b/apps/web/e2e/tests/create-task-github-url.spec.ts index f2d0d2740f..1a44a9efa0 100644 --- a/apps/web/e2e/tests/create-task-github-url.spec.ts +++ b/apps/web/e2e/tests/create-task-github-url.spec.ts @@ -741,6 +741,159 @@ test.describe("Task creation from GitHub URL", () => { await expect(warningBanner).not.toBeVisible(); }); + test("two tasks from the same PR URL create independent worktrees", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + test.setTimeout(120_000); + + const { execSync } = await import("child_process"); + const fs = await import("fs"); + const gitEnv = { + ...process.env, + HOME: backend.tmpDir, + GIT_AUTHOR_NAME: "E2E Test", + GIT_AUTHOR_EMAIL: "e2e@test.local", + GIT_COMMITTER_NAME: "E2E Test", + GIT_COMMITTER_EMAIL: "e2e@test.local", + }; + + // Create a repo with the PR branch locally. + const repoDir = `${backend.tmpDir}/repos/e2e-shared-pr`; + fs.mkdirSync(repoDir, { recursive: true }); + execSync("git init -b main", { cwd: repoDir, env: gitEnv }); + execSync('git commit --allow-empty -m "init"', { cwd: repoDir, env: gitEnv }); + execSync("git checkout -b feature/shared-pr", { cwd: repoDir, env: gitEnv }); + execSync('git commit --allow-empty -m "shared pr commit"', { cwd: repoDir, env: gitEnv }); + execSync("git checkout main", { cwd: repoDir, env: gitEnv }); + + await apiClient.createRepository(seedData.workspaceId, repoDir, "main", { + name: "shared-owner/shared-repo", + provider: "github", + provider_owner: "shared-owner", + provider_name: "shared-repo", + }); + + await apiClient.mockGitHubAddBranches("shared-owner", "shared-repo", [ + { name: "main" }, + { name: "feature/shared-pr" }, + ]); + + await apiClient.mockGitHubAddPRs([ + { + number: 50, + title: "Shared PR", + state: "open", + head_branch: "feature/shared-pr", + base_branch: "main", + author_login: "test-user", + repo_owner: "shared-owner", + repo_name: "shared-repo", + }, + ]); + + // Look up the worktree executor profile + const { executors } = await apiClient.listExecutors(); + const worktreeExec = executors.find((e) => e.type === "worktree"); + const worktreeProfile = worktreeExec?.profiles?.[0]; + if (!worktreeProfile) { + test.skip(true, "No worktree executor profile available"); + return; + } + + const kanban = new KanbanPage(testPage); + await kanban.goto(); + + // Helper: select worktree executor in the create dialog + const selectWorktreeExecutor = async () => { + const selector = testPage.getByTestId("executor-profile-selector"); + // Only click if the selector doesn't already show "Worktree" + const selectorText = await selector.textContent(); + if (!selectorText?.includes("Worktree")) { + await selector.click(); + await testPage.getByRole("option", { name: /Worktree/i }).click(); + } + }; + + // --- Task A: first task from PR URL --- + await kanban.createTaskButton.first().click(); + const dialog = testPage.getByTestId("create-task-dialog"); + await expect(dialog).toBeVisible(); + + await testPage.getByTestId("toggle-github-url").click(); + await testPage + .getByTestId("github-url-input") + .fill("https://github.com/shared-owner/shared-repo/pull/50"); + await expect(dialog.getByText("feature/shared-pr")).toBeVisible({ timeout: 10_000 }); + + await testPage.getByTestId("task-title-input").fill("Shared PR Task A"); + await testPage.getByTestId("task-description-input").fill("/e2e:simple-message"); + + const startBtn = testPage.getByTestId("submit-start-agent"); + await expect(startBtn).toBeEnabled({ timeout: 15_000 }); + await selectWorktreeExecutor(); + + await startBtn.click(); + await expect(dialog).not.toBeVisible({ timeout: 10_000 }); + await expect(kanban.taskCardByTitle("Shared PR Task A")).toBeVisible({ timeout: 10_000 }); + + // Wait for Task A agent to complete before creating Task B + await kanban.taskCardByTitle("Shared PR Task A").click(); + await expect(testPage).toHaveURL(/\/s\//, { timeout: 15_000 }); + const sessionA = new SessionPage(testPage); + await sessionA.waitForLoad(); + await expect(sessionA.chat.getByText("simple mock response", { exact: false })).toBeVisible({ + timeout: 30_000, + }); + await expect(sessionA.idleInput()).toBeVisible({ timeout: 15_000 }); + + // Task A should have the direct PR branch + await expect(sessionA.terminal).toBeVisible({ timeout: 15_000 }); + await sessionA.typeInTerminal("git branch --show-current"); + await sessionA.expectTerminalHasText("feature/shared-pr"); + + // --- Task B: second task from the same PR URL --- + await testPage.goto("/"); + await kanban.board.waitFor({ state: "visible" }); + + await kanban.createTaskButton.first().click(); + await expect(dialog).toBeVisible(); + + await testPage.getByTestId("toggle-github-url").click(); + await testPage + .getByTestId("github-url-input") + .fill("https://github.com/shared-owner/shared-repo/pull/50"); + await expect(dialog.getByText("feature/shared-pr")).toBeVisible({ timeout: 10_000 }); + + await testPage.getByTestId("task-title-input").fill("Shared PR Task B"); + await testPage.getByTestId("task-description-input").fill("/e2e:simple-message"); + + await expect(startBtn).toBeEnabled({ timeout: 15_000 }); + await selectWorktreeExecutor(); + + await startBtn.click(); + await expect(dialog).not.toBeVisible({ timeout: 10_000 }); + + // Task B should also succeed (this would have failed before the fix) + await expect(kanban.taskCardByTitle("Shared PR Task B")).toBeVisible({ timeout: 10_000 }); + + await kanban.taskCardByTitle("Shared PR Task B").click(); + await expect(testPage).toHaveURL(/\/s\//, { timeout: 15_000 }); + const sessionB = new SessionPage(testPage); + await sessionB.waitForLoad(); + await expect(sessionB.chat.getByText("simple mock response", { exact: false })).toBeVisible({ + timeout: 30_000, + }); + + // Task B should have a suffixed branch (not the original PR branch) + await expect(sessionB.terminal).toBeVisible({ timeout: 15_000 }); + await sessionB.typeInTerminal("git branch --show-current"); + // Branch should start with the PR branch name but have a random suffix + await sessionB.expectTerminalHasText("feature/shared-pr-"); + }); + test("can toggle between GitHub URL and repository selector", async ({ testPage }) => { const kanban = new KanbanPage(testPage); await kanban.goto(); diff --git a/apps/web/e2e/tests/workflow-automation.spec.ts b/apps/web/e2e/tests/workflow-automation.spec.ts index ba42bde66c..816c7ecd21 100644 --- a/apps/web/e2e/tests/workflow-automation.spec.ts +++ b/apps/web/e2e/tests/workflow-automation.spec.ts @@ -551,4 +551,9 @@ test.describe("Workflow automation", () => { timeout: 15_000, }); }); + + // Note: "step prompt without {{task_prompt}} replaces task description" is tested + // by backend unit tests (TestBuildWorkflowPrompt_UsesStepPromptOnlyWithoutTaskPromptPlaceholder). + // The auto_start flow for new sessions uses a different prompt resolution path + // that can't be reliably tested e2e without an existing session. });