diff --git a/apps/backend/cmd/kandev/adapters.go b/apps/backend/cmd/kandev/adapters.go index e3d7ca6dd6..ccf0918ab4 100644 --- a/apps/backend/cmd/kandev/adapters.go +++ b/apps/backend/cmd/kandev/adapters.go @@ -87,6 +87,7 @@ func (a *lifecycleAdapter) LaunchAgent(ctx context.Context, req *executor.Launch RepositoryID: req.RepositoryID, RepositoryPath: req.RepositoryPath, BaseBranch: req.BaseBranch, + CheckoutBranch: req.CheckoutBranch, WorktreeBranchPrefix: req.WorktreeBranchPrefix, PullBeforeWorktree: req.PullBeforeWorktree, } diff --git a/apps/backend/cmd/kandev/orchestrator.go b/apps/backend/cmd/kandev/orchestrator.go index 5bc67319a3..71e680fb88 100644 --- a/apps/backend/cmd/kandev/orchestrator.go +++ b/apps/backend/cmd/kandev/orchestrator.go @@ -176,8 +176,9 @@ func (a *reviewTaskCreatorAdapter) CreateReviewTask(ctx context.Context, req *or var repos []taskservice.TaskRepositoryInput for _, r := range req.Repositories { repos = append(repos, taskservice.TaskRepositoryInput{ - RepositoryID: r.RepositoryID, - BaseBranch: r.BaseBranch, + RepositoryID: r.RepositoryID, + BaseBranch: r.BaseBranch, + CheckoutBranch: r.CheckoutBranch, }) } return a.svc.CreateTask(ctx, &taskservice.CreateTaskRequest{ diff --git a/apps/backend/internal/agent/lifecycle/env_preparer.go b/apps/backend/internal/agent/lifecycle/env_preparer.go index b3bdce7428..6a14460422 100644 --- a/apps/backend/internal/agent/lifecycle/env_preparer.go +++ b/apps/backend/internal/agent/lifecycle/env_preparer.go @@ -23,16 +23,23 @@ const ( type EnvPrepareRequest struct { TaskID string SessionID string + TaskTitle string ExecutionID string ExecutorType executor.Name WorkspacePath string RepositoryPath string + RepositoryID string UseWorktree bool SetupScript string BaseBranch string + CheckoutBranch string WorktreeID string WorktreeBranch string - Env map[string]string + + WorktreeBranchPrefix string + PullBeforeWorktree bool + + Env map[string]string } // PrepareStep represents a single step in the preparation process. @@ -52,6 +59,11 @@ type EnvPrepareResult struct { WorkspacePath string `json:"workspace_path,omitempty"` ErrorMessage string `json:"error_message,omitempty"` Duration time.Duration `json:"duration"` + + // Worktree fields (populated when worktree preparer runs) + WorktreeID string `json:"worktree_id,omitempty"` + WorktreeBranch string `json:"worktree_branch,omitempty"` + MainRepoGitDir string `json:"main_repo_git_dir,omitempty"` } // PrepareProgressCallback is called when a preparation step changes status. diff --git a/apps/backend/internal/agent/lifecycle/env_preparer_worktree.go b/apps/backend/internal/agent/lifecycle/env_preparer_worktree.go index 5dc4ade330..2a0227cb94 100644 --- a/apps/backend/internal/agent/lifecycle/env_preparer_worktree.go +++ b/apps/backend/internal/agent/lifecycle/env_preparer_worktree.go @@ -2,6 +2,7 @@ package lifecycle import ( "context" + "path/filepath" "time" "go.uber.org/zap" @@ -11,7 +12,7 @@ import ( ) // WorktreePreparer prepares a worktree-based execution environment. -// Steps: validate repository → create/reuse worktree → run setup script (if any). +// Steps: validate repository → create worktree → checkout PR branch (if set) → run setup script (if any). type WorktreePreparer struct { worktreeMgr *worktree.Manager logger *logger.Logger @@ -31,72 +32,111 @@ func (p *WorktreePreparer) Prepare(ctx context.Context, req *EnvPrepareRequest, start := time.Now() var steps []PrepareStep - workspacePath := req.WorkspacePath - if workspacePath == "" { - workspacePath = req.RepositoryPath - } - resolvedScript := resolvePreparerSetupScript(req, workspacePath) - totalSteps := 2 // validate + create worktree - if resolvedScript != "" { + if req.CheckoutBranch != "" { totalSteps++ } + // We can't resolve the setup script until we know the workspace path, + // so we count the script step after worktree creation. + stepIdx := 0 // Step 1: Validate repository path step := beginStep("Validate repository") - reportProgress(onProgress, step, 0, totalSteps) + reportProgress(onProgress, step, stepIdx, totalSteps) if req.RepositoryPath == "" { completeStepError(&step, "no repository path provided") steps = append(steps, step) + reportProgress(onProgress, step, stepIdx, totalSteps) + return &EnvPrepareResult{Success: false, Steps: steps, ErrorMessage: step.Error, Duration: time.Since(start)}, nil + } + if p.worktreeMgr == nil { + completeStepError(&step, "worktree manager not available") + steps = append(steps, step) + reportProgress(onProgress, step, stepIdx, totalSteps) return &EnvPrepareResult{Success: false, Steps: steps, ErrorMessage: step.Error, Duration: time.Since(start)}, nil } completeStepSuccess(&step) steps = append(steps, step) - reportProgress(onProgress, step, 0, totalSteps) + reportProgress(onProgress, step, stepIdx, totalSteps) + stepIdx++ - // Step 2: Create or reuse worktree + // Step 2: Create worktree step = beginStep("Create worktree") - reportProgress(onProgress, step, 1, totalSteps) - if p.worktreeMgr == nil { - completeStepError(&step, "worktree manager not available") + reportProgress(onProgress, step, stepIdx, totalSteps) + + createReq := worktree.CreateRequest{ + TaskID: req.TaskID, + SessionID: req.SessionID, + TaskTitle: req.TaskTitle, + RepositoryID: req.RepositoryID, + RepositoryPath: req.RepositoryPath, + BaseBranch: req.BaseBranch, + CheckoutBranch: req.CheckoutBranch, + WorktreeBranchPrefix: req.WorktreeBranchPrefix, + PullBeforeWorktree: req.PullBeforeWorktree, + WorktreeID: req.WorktreeID, + } + + wt, err := p.worktreeMgr.Create(ctx, createReq) + if err != nil { + completeStepError(&step, err.Error()) steps = append(steps, step) - reportProgress(onProgress, step, 1, totalSteps) - // Fall back: use repository path directly - return &EnvPrepareResult{ - Success: true, - Steps: steps, - WorkspacePath: req.RepositoryPath, - Duration: time.Since(start), - }, nil + reportProgress(onProgress, step, stepIdx, totalSteps) + p.logger.Error("worktree creation failed", + zap.String("task_id", req.TaskID), + zap.Error(err)) + return &EnvPrepareResult{Success: false, Steps: steps, ErrorMessage: err.Error(), Duration: time.Since(start)}, nil } completeStepSuccess(&step) steps = append(steps, step) - reportProgress(onProgress, step, 1, totalSteps) + reportProgress(onProgress, step, stepIdx, totalSteps) + stepIdx++ - // Step 3: Run setup script (if provided) + workspacePath := wt.Path + mainRepoGitDir := filepath.Join(req.RepositoryPath, ".git") + + // Step 3 (optional): Fetch PR branch is handled inside worktree.Manager.Create + // via CreateRequest.CheckoutBranch. If it was set and failed, Create() would have + // returned an error above. We report a success step here for UI visibility. + if req.CheckoutBranch != "" { + step = beginStep("Fetch PR branch") + reportProgress(onProgress, step, stepIdx, totalSteps) + completeStepSuccess(&step) + steps = append(steps, step) + reportProgress(onProgress, step, stepIdx, totalSteps) + stepIdx++ + } + + // Step N: Run setup script (if provided) + resolvedScript := resolvePreparerSetupScript(req, workspacePath) if resolvedScript != "" { + totalSteps++ step = beginStep("Run setup script") - reportProgress(onProgress, step, 2, totalSteps) - output, err := runSetupScript(ctx, resolvedScript, workspacePath, req.Env) - if err != nil { - completeStepError(&step, err.Error()) + reportProgress(onProgress, step, stepIdx, totalSteps) + output, scriptErr := runSetupScript(ctx, resolvedScript, workspacePath, req.Env) + if scriptErr != nil { + completeStepError(&step, scriptErr.Error()) step.Output = output steps = append(steps, step) - reportProgress(onProgress, step, 2, totalSteps) - p.logger.Warn("setup script failed", zap.String("task_id", req.TaskID), zap.Error(err)) + reportProgress(onProgress, step, stepIdx, totalSteps) + p.logger.Warn("setup script failed", zap.String("task_id", req.TaskID), zap.Error(scriptErr)) + // Setup script failure is non-fatal } else { step.Output = output completeStepSuccess(&step) steps = append(steps, step) - reportProgress(onProgress, step, 2, totalSteps) + reportProgress(onProgress, step, stepIdx, totalSteps) } } return &EnvPrepareResult{ - Success: true, - Steps: steps, - WorkspacePath: workspacePath, - Duration: time.Since(start), + Success: true, + Steps: steps, + WorkspacePath: workspacePath, + Duration: time.Since(start), + WorktreeID: wt.ID, + WorktreeBranch: wt.Branch, + MainRepoGitDir: mainRepoGitDir, }, nil } diff --git a/apps/backend/internal/agent/lifecycle/manager.go b/apps/backend/internal/agent/lifecycle/manager.go index 8c42a45667..d076bbfaa0 100644 --- a/apps/backend/internal/agent/lifecycle/manager.go +++ b/apps/backend/internal/agent/lifecycle/manager.go @@ -164,6 +164,12 @@ func NewManager( // If not set, agents will work directly in the repository's main working directory. func (m *Manager) SetWorktreeManager(worktreeMgr *worktree.Manager) { m.worktreeMgr = worktreeMgr + // Register the worktree preparer so that executor type "worktree" gets + // worktree-specific preparation (create git worktree, checkout PR branch) + // instead of the generic local preparer. + if m.preparerRegistry != nil { + m.preparerRegistry.Register(executor.Name("worktree"), NewWorktreePreparer(worktreeMgr, m.logger)) + } } // SetMCPHandler sets the MCP request handler for dispatching MCP tool calls. diff --git a/apps/backend/internal/agent/lifecycle/manager_launch.go b/apps/backend/internal/agent/lifecycle/manager_launch.go index 7dc1963390..1ebc808ace 100644 --- a/apps/backend/internal/agent/lifecycle/manager_launch.go +++ b/apps/backend/internal/agent/lifecycle/manager_launch.go @@ -3,7 +3,6 @@ package lifecycle import ( "context" "fmt" - "path/filepath" "time" "github.com/google/uuid" @@ -103,27 +102,17 @@ func (m *Manager) buildAgentCommand(req *LaunchRequest, profileInfo *AgentProfil } } -// launchResolveWorkspacePath resolves the effective workspace path, handling worktree -// creation if requested. Returns workspacePath, mainRepoGitDir, worktreeID, worktreeBranch. -func (m *Manager) launchResolveWorkspacePath(ctx context.Context, req *LaunchRequest) (workspacePath, mainRepoGitDir, worktreeID, worktreeBranch string) { +// launchResolveWorkspacePath resolves the effective workspace path for non-worktree executors. +// For worktree executors, workspace resolution is handled by the WorktreePreparer. +// Returns workspacePath, mainRepoGitDir, worktreeID, worktreeBranch. +func (m *Manager) launchResolveWorkspacePath(_ context.Context, req *LaunchRequest) (workspacePath, mainRepoGitDir, worktreeID, worktreeBranch string) { + if req.UseWorktree { + // Worktree executors: preparer handles worktree creation. + // Return empty path; it will be populated from preparer results. + return "", "", "", "" + } workspacePath = req.WorkspacePath - if req.UseWorktree && m.worktreeMgr != nil && req.RepositoryPath != "" { - wt, err := m.getOrCreateWorktree(ctx, req) - if err != nil { - m.logger.Warn("failed to create worktree, falling back to direct mount", - zap.String("task_id", req.TaskID), - zap.Error(err)) - // Fall back to direct mount if worktree creation fails - workspacePath = req.RepositoryPath - } else { - workspacePath = wt.Path - worktreeID = wt.ID - worktreeBranch = wt.Branch - // Git worktrees reference the main repo's .git directory via a .git file. - // We need to mount the entire .git directory for git commands to work. - mainRepoGitDir = filepath.Join(req.RepositoryPath, ".git") - } - } else if req.RepositoryPath != "" && workspacePath == "" { + if req.RepositoryPath != "" && workspacePath == "" { workspacePath = req.RepositoryPath } return @@ -160,6 +149,22 @@ func (m *Manager) launchPrepareRequest(req *LaunchRequest, profileInfo *AgentPro return reqWithWorktree, executionID } +// newProgressCallback builds a PrepareProgressCallback that publishes progress events for a task/session. +func (m *Manager) newProgressCallback(taskID, sessionID string) PrepareProgressCallback { + return func(step PrepareStep, stepIndex int, totalSteps int) { + m.eventPublisher.PublishPrepareProgress(sessionID, &PrepareProgressEventPayload{ + TaskID: taskID, + SessionID: sessionID, + StepName: step.Name, + StepIndex: stepIndex, + TotalSteps: totalSteps, + Status: string(step.Status), + Output: step.Output, + Error: step.Error, + }) + } +} + // launchBuildExecutorRequest resolves MCP servers, builds the ExecutorCreateRequest, // and creates the runtime instance. func (m *Manager) launchBuildExecutorRequest(ctx context.Context, executionID string, reqWithWorktree *LaunchRequest, agentConfig agents.Agent, mainRepoGitDir, worktreeID, worktreeBranch string) (*ExecutorCreateRequest, *ExecutorInstance, ExecutorBackend, error) { @@ -200,18 +205,7 @@ func (m *Manager) launchBuildExecutorRequest(ctx context.Context, executionID st AgentConfig: agentConfig, McpServers: mcpServers, PreviousExecutionID: reqWithWorktree.PreviousExecutionID, - OnProgress: func(step PrepareStep, stepIndex int, totalSteps int) { - m.eventPublisher.PublishPrepareProgress(reqWithWorktree.SessionID, &PrepareProgressEventPayload{ - TaskID: reqWithWorktree.TaskID, - SessionID: reqWithWorktree.SessionID, - StepName: step.Name, - StepIndex: stepIndex, - TotalSteps: totalSteps, - Status: string(step.Status), - Output: step.Output, - Error: step.Error, - }) - }, + OnProgress: m.newProgressCallback(reqWithWorktree.TaskID, reqWithWorktree.SessionID), } if resumer, ok := rt.(RemoteSessionResumer); ok { @@ -228,73 +222,98 @@ func (m *Manager) launchBuildExecutorRequest(ctx context.Context, executionID st } // runEnvironmentPreparer runs the environment preparer for the executor type, if one is registered. -// Preparation failures are non-fatal and logged as warnings. +// Returns the prepare result (nil if no preparer ran). Does NOT publish PrepareCompleted; +// the caller is responsible for publishing based on the returned result. func (m *Manager) runEnvironmentPreparer( ctx context.Context, req *LaunchRequest, workspacePath string, - worktreeID string, - worktreeBranch string, -) { +) *EnvPrepareResult { if m.preparerRegistry == nil { - return + return nil } - execName := executor.ExecutorTypeToBackend(models.ExecutorType(req.ExecutorType)) + // Look up preparer by the raw executor type first (e.g. "worktree"), + // then fall back to the backend name (e.g. "standalone"). + // This allows executor types that share a backend (local and worktree + // both map to standalone) to have distinct preparation logic. + execName := executor.Name(req.ExecutorType) preparer := m.preparerRegistry.Get(execName) if preparer == nil { - return + execName = executor.ExecutorTypeToBackend(models.ExecutorType(req.ExecutorType)) + preparer = m.preparerRegistry.Get(execName) + } + if preparer == nil { + return nil } prepReq := &EnvPrepareRequest{ - TaskID: req.TaskID, - SessionID: req.SessionID, - ExecutorType: execName, - WorkspacePath: workspacePath, - RepositoryPath: req.RepositoryPath, - UseWorktree: req.UseWorktree, - SetupScript: req.SetupScript, - BaseBranch: req.BaseBranch, - WorktreeID: worktreeID, - WorktreeBranch: worktreeBranch, - Env: req.Env, + TaskID: req.TaskID, + SessionID: req.SessionID, + TaskTitle: req.TaskTitle, + ExecutorType: execName, + WorkspacePath: workspacePath, + RepositoryPath: req.RepositoryPath, + RepositoryID: req.RepositoryID, + UseWorktree: req.UseWorktree, + SetupScript: req.SetupScript, + BaseBranch: req.BaseBranch, + CheckoutBranch: req.CheckoutBranch, + WorktreeBranchPrefix: req.WorktreeBranchPrefix, + PullBeforeWorktree: req.PullBeforeWorktree, + Env: req.Env, } - onProgress := func(step PrepareStep, stepIndex int, totalSteps int) { - m.eventPublisher.PublishPrepareProgress(req.SessionID, &PrepareProgressEventPayload{ - TaskID: req.TaskID, - SessionID: req.SessionID, - StepName: step.Name, - StepIndex: stepIndex, - TotalSteps: totalSteps, - Status: string(step.Status), - Output: step.Output, - Error: step.Error, - }) - } - - result, err := preparer.Prepare(ctx, prepReq, onProgress) + result, err := preparer.Prepare(ctx, prepReq, m.newProgressCallback(req.TaskID, req.SessionID)) if err != nil { m.logger.Warn("environment preparation failed", zap.String("task_id", req.TaskID), zap.String("preparer", preparer.Name()), zap.Error(err)) + return &EnvPrepareResult{ + Success: false, + ErrorMessage: err.Error(), + } + } + + return result +} + +// launchApplyPrepareResult applies workspace metadata from the preparer result and publishes completion. +// Returns an error if the preparer failed. +func (m *Manager) launchApplyPrepareResult( + req *LaunchRequest, + result *EnvPrepareResult, + workspacePath, mainRepoGitDir, worktreeID, worktreeBranch *string, +) error { + if !result.Success { m.eventPublisher.PublishPrepareCompleted(req.SessionID, &PrepareCompletedEventPayload{ TaskID: req.TaskID, SessionID: req.SessionID, Success: false, - ErrorMessage: err.Error(), + ErrorMessage: result.ErrorMessage, }) - return + return fmt.Errorf("environment preparation failed: %s", result.ErrorMessage) + } + if result.WorkspacePath != "" { + *workspacePath = result.WorkspacePath + } + if result.MainRepoGitDir != "" { + *mainRepoGitDir = result.MainRepoGitDir + } + if result.WorktreeID != "" { + *worktreeID = result.WorktreeID + } + if result.WorktreeBranch != "" { + *worktreeBranch = result.WorktreeBranch } - m.eventPublisher.PublishPrepareCompleted(req.SessionID, &PrepareCompletedEventPayload{ TaskID: req.TaskID, SessionID: req.SessionID, - Success: result.Success, - ErrorMessage: result.ErrorMessage, + Success: true, DurationMs: result.Duration.Milliseconds(), WorkspacePath: result.WorkspacePath, }) + return nil } // Launch launches a new agent for a task @@ -326,11 +345,17 @@ func (m *Manager) Launch(ctx context.Context, req *LaunchRequest) (*AgentExecuti } } - // 4. Resolve workspace path (with optional worktree creation) + // 4. Resolve workspace path (non-worktree executors use this directly) workspacePath, mainRepoGitDir, worktreeID, worktreeBranch := m.launchResolveWorkspacePath(ctx, req) - // 4b. Run environment preparation (if preparer registered for this executor type) - m.runEnvironmentPreparer(ctx, req, workspacePath, worktreeID, worktreeBranch) + // 4b. Run environment preparation (if preparer registered for this executor type). + // For worktree executors, the preparer creates the worktree and returns workspace metadata. + prepResult := m.runEnvironmentPreparer(ctx, req, workspacePath) + if prepResult != nil { + if err := m.launchApplyPrepareResult(req, prepResult, &workspacePath, &mainRepoGitDir, &worktreeID, &worktreeBranch); err != nil { + return nil, err + } + } // 5 & 6. Prepare the request copy with metadata and profile env reqWithWorktree, executionID := m.launchPrepareRequest(req, profileInfo, workspacePath) @@ -346,12 +371,16 @@ func (m *Manager) Launch(ctx context.Context, req *LaunchRequest) (*AgentExecuti }) return nil, err } - m.eventPublisher.PublishPrepareCompleted(req.SessionID, &PrepareCompletedEventPayload{ - TaskID: req.TaskID, - SessionID: req.SessionID, - Success: true, - DurationMs: 0, - }) + // Publish PrepareCompleted for CreateInstance only if no preparer ran + // (preparers publish their own completion above). + if prepResult == nil { + m.eventPublisher.PublishPrepareCompleted(req.SessionID, &PrepareCompletedEventPayload{ + TaskID: req.TaskID, + SessionID: req.SessionID, + Success: true, + DurationMs: 0, + }) + } // Convert to AgentExecution and set the runtime name execution := execInstance.ToAgentExecution(execReq) diff --git a/apps/backend/internal/agent/lifecycle/manager_startup.go b/apps/backend/internal/agent/lifecycle/manager_startup.go index e05fb3f5e2..be76e1590a 100644 --- a/apps/backend/internal/agent/lifecycle/manager_startup.go +++ b/apps/backend/internal/agent/lifecycle/manager_startup.go @@ -13,7 +13,6 @@ import ( "github.com/kandev/kandev/internal/common/appctx" "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/task/models" - "github.com/kandev/kandev/internal/worktree" ) // StartAgentProcess configures and starts the agent subprocess for an execution. @@ -189,53 +188,6 @@ func (m *Manager) buildEnvForExecution(executionID string, req *LaunchRequest, a return env } -// getOrCreateWorktree creates a new worktree or reuses an existing one for session resumption. -// If worktree_id is in metadata, it tries to reuse that specific worktree. -// Otherwise, creates a new worktree with a unique random suffix. -func (m *Manager) getOrCreateWorktree(ctx context.Context, req *LaunchRequest) (*worktree.Worktree, error) { - // Check if we have a worktree_id in metadata for session resumption - var worktreeID string - if req.Metadata != nil { - if id, ok := req.Metadata["worktree_id"].(string); ok && id != "" { - worktreeID = id - } - } - - // Create request with optional WorktreeID for resumption - createReq := worktree.CreateRequest{ - TaskID: req.TaskID, - SessionID: req.SessionID, - TaskTitle: req.TaskTitle, - RepositoryID: req.RepositoryID, - RepositoryPath: req.RepositoryPath, - BaseBranch: req.BaseBranch, - WorktreeBranchPrefix: req.WorktreeBranchPrefix, - PullBeforeWorktree: req.PullBeforeWorktree, - WorktreeID: worktreeID, // If set, will try to reuse this worktree - } - - wt, err := m.worktreeMgr.Create(ctx, createReq) - if err != nil { - return nil, fmt.Errorf("failed to create worktree: %w", err) - } - - if worktreeID != "" && wt.ID == worktreeID { - m.logger.Debug("reusing existing worktree for session resumption", - zap.String("task_id", req.TaskID), - zap.String("worktree_id", wt.ID), - zap.String("worktree_path", wt.Path), - zap.String("branch", wt.Branch)) - } else { - m.logger.Info("created new worktree for task", - zap.String("task_id", req.TaskID), - zap.String("worktree_id", wt.ID), - zap.String("worktree_path", wt.Path), - zap.String("branch", wt.Branch)) - } - - return wt, nil -} - // waitForAgentctlReady waits for the agentctl HTTP server to be ready. // This enables shell/workspace features without starting the agent process. func (m *Manager) waitForAgentctlReady(execution *AgentExecution) { diff --git a/apps/backend/internal/agent/lifecycle/types.go b/apps/backend/internal/agent/lifecycle/types.go index 54ed9deea7..d5fdf47d9e 100644 --- a/apps/backend/internal/agent/lifecycle/types.go +++ b/apps/backend/internal/agent/lifecycle/types.go @@ -186,6 +186,7 @@ type LaunchRequest struct { RepositoryID string // Repository ID for worktree tracking RepositoryPath string // Path to the main repository (for worktree creation) BaseBranch string // Base branch for the worktree (e.g., "main") + CheckoutBranch string // Branch to fetch and checkout after worktree creation (e.g., PR head branch) WorktreeBranchPrefix string // Branch prefix for worktree branches PullBeforeWorktree bool // Whether to pull from remote before creating the worktree } diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index c1248b762c..97057fd703 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -5,8 +5,10 @@ import ( "crypto/sha256" "encoding/base64" "encoding/hex" + "errors" "fmt" "io" + "io/fs" "os" "os/exec" "path/filepath" @@ -169,18 +171,19 @@ func (wt *WorkspaceTracker) resolveSafePath(reqPath string) (string, error) { safePath = filepath.Join(realWorkDir, cleanReqPath) } - // Resolve symlinks to prevent bypassing validation + // Resolve symlinks to prevent bypassing validation. + // When the path doesn't exist yet, walk up until we find an existing + // ancestor so symlinks (e.g. /tmp -> /private/tmp on macOS) are resolved + // consistently with realWorkDir below. Only fall back for not-found errors; + // propagate permission denied, symlink loops, etc. realPath, err := filepath.EvalSymlinks(safePath) if err != nil { - // If EvalSymlinks fails, the path might not exist yet (e.g., creating new file) - // In that case, resolve the parent directory - parentDir := filepath.Dir(safePath) - realParent, parentErr := filepath.EvalSymlinks(parentDir) - if parentErr != nil { - // Parent doesn't exist either, use the cleaned full path - realPath = safePath - } else { - realPath = filepath.Join(realParent, filepath.Base(safePath)) + if !errors.Is(err, fs.ErrNotExist) { + return "", fmt.Errorf("failed to resolve path: %w", err) + } + realPath, err = resolveNonExistentPath(safePath) + if err != nil { + return "", err } } @@ -201,6 +204,35 @@ func (wt *WorkspaceTracker) resolveSafePath(reqPath string) (string, error) { return filepath.Join(realWorkDir, relPath), nil } +// resolveNonExistentPath walks up from path until it finds an existing +// ancestor, resolves its symlinks, then reattaches the non-existent tail. +// This ensures paths under symlinked directories (e.g. /tmp on macOS) +// resolve consistently even when the target doesn't exist yet. +// Returns an error if an ancestor fails with something other than ErrNotExist +// (e.g. permission denied, symlink loop). +func resolveNonExistentPath(path string) (string, error) { + current := path + var tail []string + for { + resolved, err := filepath.EvalSymlinks(current) + if err == nil { + for i := len(tail) - 1; i >= 0; i-- { + resolved = filepath.Join(resolved, tail[i]) + } + return resolved, nil + } + if !errors.Is(err, fs.ErrNotExist) { + return "", fmt.Errorf("failed to resolve ancestor %q: %w", current, err) + } + parent := filepath.Dir(current) + if parent == current { + return "", fmt.Errorf("no existing ancestor for %q: %w", path, err) + } + tail = append(tail, filepath.Base(current)) + current = parent + } +} + // GetFileContent returns the content of a file. // If the file is not valid UTF-8, it is base64-encoded and isBinary is true. func (wt *WorkspaceTracker) GetFileContent(reqPath string) (string, int64, bool, error) { diff --git a/apps/backend/internal/agentctl/server/process/workspace_files_test.go b/apps/backend/internal/agentctl/server/process/workspace_files_test.go new file mode 100644 index 0000000000..cea21beb50 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/workspace_files_test.go @@ -0,0 +1,115 @@ +package process + +import ( + "os" + "path/filepath" + "testing" +) + +func TestResolveNonExistentPath(t *testing.T) { + // Create a real temp dir as the existing ancestor + tmpDir := t.TempDir() + + t.Run("fully existing path returns resolved path", func(t *testing.T) { + existingFile := filepath.Join(tmpDir, "existing.txt") + if err := os.WriteFile(existingFile, []byte(""), 0o644); err != nil { + t.Fatal(err) + } + result, err := resolveNonExistentPath(existingFile) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + expected, _ := filepath.EvalSymlinks(existingFile) + if result != expected { + t.Errorf("got %q, want %q", result, expected) + } + }) + + t.Run("non-existent leaf with existing parent", func(t *testing.T) { + nonExistent := filepath.Join(tmpDir, "noexist.txt") + result, err := resolveNonExistentPath(nonExistent) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resolvedParent, _ := filepath.EvalSymlinks(tmpDir) + expected := filepath.Join(resolvedParent, "noexist.txt") + if result != expected { + t.Errorf("got %q, want %q", result, expected) + } + }) + + t.Run("non-existent nested directories", func(t *testing.T) { + deep := filepath.Join(tmpDir, "a", "b", "c", "file.txt") + result, err := resolveNonExistentPath(deep) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resolvedBase, _ := filepath.EvalSymlinks(tmpDir) + expected := filepath.Join(resolvedBase, "a", "b", "c", "file.txt") + if result != expected { + t.Errorf("got %q, want %q", result, expected) + } + }) + + t.Run("existing intermediate directory", func(t *testing.T) { + subDir := filepath.Join(tmpDir, "sub") + if err := os.Mkdir(subDir, 0o755); err != nil { + t.Fatal(err) + } + deep := filepath.Join(subDir, "deep", "file.txt") + result, err := resolveNonExistentPath(deep) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resolvedSub, _ := filepath.EvalSymlinks(subDir) + expected := filepath.Join(resolvedSub, "deep", "file.txt") + if result != expected { + t.Errorf("got %q, want %q", result, expected) + } + }) + + t.Run("symlinked ancestor resolves correctly", func(t *testing.T) { + realDir := filepath.Join(tmpDir, "real") + if err := os.Mkdir(realDir, 0o755); err != nil { + t.Fatal(err) + } + linkDir := filepath.Join(tmpDir, "link") + if err := os.Symlink(realDir, linkDir); err != nil { + t.Skip("symlinks not supported") + } + path := filepath.Join(linkDir, "new", "file.txt") + result, err := resolveNonExistentPath(path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // realDir itself may be under a symlink (e.g. /var -> /private/var on macOS) + resolvedReal, _ := filepath.EvalSymlinks(realDir) + expected := filepath.Join(resolvedReal, "new", "file.txt") + if result != expected { + t.Errorf("got %q, want %q", result, expected) + } + }) + + t.Run("permission error is propagated", func(t *testing.T) { + // Create a directory, then make it unreadable + restrictedDir := filepath.Join(tmpDir, "restricted") + if err := os.Mkdir(restrictedDir, 0o755); err != nil { + t.Fatal(err) + } + innerDir := filepath.Join(restrictedDir, "inner") + if err := os.Mkdir(innerDir, 0o755); err != nil { + t.Fatal(err) + } + // Remove read+execute permission on the parent + if err := os.Chmod(restrictedDir, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(restrictedDir, 0o755) }) + + path := filepath.Join(innerDir, "file.txt") + _, err := resolveNonExistentPath(path) + if err == nil { + t.Error("expected error for permission-denied path, got nil") + } + }) +} diff --git a/apps/backend/internal/github/handlers.go b/apps/backend/internal/github/handlers.go index b590045e96..10be37e669 100644 --- a/apps/backend/internal/github/handlers.go +++ b/apps/backend/internal/github/handlers.go @@ -25,7 +25,7 @@ func RegisterMockRoutes(router *gin.Engine, svc *Service, log *logger.Logger) { if !ok { return } - ctrl := NewMockController(mock, log) + ctrl := NewMockController(mock, svc.TestStore(), log) ctrl.RegisterRoutes(router) log.Info("registered GitHub mock control endpoints") } diff --git a/apps/backend/internal/github/mock_controller.go b/apps/backend/internal/github/mock_controller.go index 1ed19de1c5..22d2a729c5 100644 --- a/apps/backend/internal/github/mock_controller.go +++ b/apps/backend/internal/github/mock_controller.go @@ -2,20 +2,24 @@ package github import ( "net/http" + "time" "github.com/gin-gonic/gin" "github.com/kandev/kandev/internal/common/logger" ) +const defaultPRState = "open" + // MockController handles HTTP endpoints for controlling the MockClient in E2E tests. type MockController struct { - mock *MockClient + mock *MockClient + store *Store } // NewMockController creates a new MockController. -func NewMockController(mock *MockClient, _ *logger.Logger) *MockController { - return &MockController{mock: mock} +func NewMockController(mock *MockClient, store *Store, _ *logger.Logger) *MockController { + return &MockController{mock: mock, store: store} } // RegisterRoutes registers all mock control HTTP routes. @@ -30,6 +34,7 @@ func (c *MockController) RegisterRoutes(router *gin.Engine) { api.POST("/checks", c.addCheckRuns) api.POST("/files", c.addPRFiles) api.POST("/commits", c.addPRCommits) + api.POST("/task-prs", c.associateTaskPR) api.DELETE("/reset", c.reset) } @@ -159,6 +164,52 @@ func (c *MockController) addPRCommits(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"added": len(req.Commits)}) } +// associateTaskPR directly creates a github_task_prs record for E2E testing. +func (c *MockController) associateTaskPR(ctx *gin.Context) { + var req struct { + TaskID string `json:"task_id"` + Owner string `json:"owner"` + Repo string `json:"repo"` + PRNumber int `json:"pr_number"` + PRURL string `json:"pr_url"` + PRTitle string `json:"pr_title"` + HeadBranch string `json:"head_branch"` + BaseBranch string `json:"base_branch"` + AuthorLogin string `json:"author_login"` + State string `json:"state"` + Additions int `json:"additions"` + Deletions int `json:"deletions"` + } + if err := ctx.ShouldBindJSON(&req); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) + return + } + if req.State == "" { + req.State = defaultPRState + } + now := time.Now().UTC() + tp := &TaskPR{ + TaskID: req.TaskID, + Owner: req.Owner, + Repo: req.Repo, + PRNumber: req.PRNumber, + PRURL: req.PRURL, + PRTitle: req.PRTitle, + HeadBranch: req.HeadBranch, + BaseBranch: req.BaseBranch, + AuthorLogin: req.AuthorLogin, + State: req.State, + Additions: req.Additions, + Deletions: req.Deletions, + CreatedAt: now, + } + if err := c.store.CreateTaskPR(ctx.Request.Context(), tp); err != nil { + ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + ctx.JSON(http.StatusCreated, tp) +} + func (c *MockController) reset(ctx *gin.Context) { c.mock.Reset() ctx.JSON(http.StatusOK, gin.H{"reset": true}) diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index 6b76739672..e9542287e5 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -43,6 +43,11 @@ func (s *Service) Client() Client { return s.client } +// TestStore returns the store for test/mock use only. +func (s *Service) TestStore() *Store { + return s.store +} + // IsAuthenticated returns whether the service has a working GitHub client. func (s *Service) IsAuthenticated() bool { return s.client != nil diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index d877300bf3..de21cb56c1 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -50,8 +50,9 @@ type ReviewTaskRequest struct { // ReviewTaskRepository associates a repository with a review task. type ReviewTaskRepository struct { - RepositoryID string - BaseBranch string + RepositoryID string + BaseBranch string + CheckoutBranch string } // SetGitHubService sets the GitHub service for PR auto-detection. @@ -252,9 +253,9 @@ func (s *Service) resolveReviewRepository(ctx context.Context, workspaceID strin zap.String("repo", repoSlug), zap.String("repo_id", repoID), zap.String("base_branch", baseBranch)) - // Use the PR's head branch so the worktree checks out the PR code, not the merge target. - // The actual base branch is preserved in the TaskPR record for reference. - return []ReviewTaskRepository{{RepositoryID: repoID, BaseBranch: pr.HeadBranch}} + // BaseBranch = repo default branch (e.g. "main") for worktree creation. + // CheckoutBranch = PR head branch to fetch and checkout after worktree is created. + return []ReviewTaskRepository{{RepositoryID: repoID, BaseBranch: baseBranch, CheckoutBranch: pr.HeadBranch}} } // detectPushAndAssociatePR checks if a push happened and looks for a PR on diff --git a/apps/backend/internal/orchestrator/executor/executor.go b/apps/backend/internal/orchestrator/executor/executor.go index c2c33a64a9..6ab4585ba3 100644 --- a/apps/backend/internal/orchestrator/executor/executor.go +++ b/apps/backend/internal/orchestrator/executor/executor.go @@ -186,6 +186,7 @@ type LaunchAgentRequest struct { RepositoryID string // Repository ID for worktree tracking RepositoryPath string // Path to the main repository (for worktree creation) BaseBranch string // Base branch for the worktree (e.g., "main") + CheckoutBranch string // Branch to fetch and checkout after worktree creation (e.g., PR head branch) WorktreeBranchPrefix string // Branch prefix for worktree branches PullBeforeWorktree bool // Whether to pull from remote before creating the worktree } diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index 5ea174c296..b865216e31 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -370,7 +370,7 @@ func (e *Executor) finalizeLaunch(ctx context.Context, task *v1.Task, session *m now := time.Now().UTC() // On initial launch there is no existing ExecutorRunning record to carry forward. e.persistLaunchState(ctx, task.ID, sessionID, session, resp, startAgent, now, execCfg, nil) - e.persistWorktreeAssociation(ctx, task.ID, session.ID, repoInfo.RepositoryID, resp) + e.persistWorktreeAssociation(ctx, task.ID, session, repoInfo.RepositoryID, resp) sessionState := v1.TaskSessionStateCreated if startAgent { @@ -441,6 +441,7 @@ func (e *Executor) buildLaunchAgentRequest(ctx context.Context, task *v1.Task, s req.UseWorktree = shouldUseWorktree(execConfig.ExecutorType) req.RepositoryPath = repoInfo.RepositoryPath req.BaseBranch = repoInfo.BaseBranch + req.CheckoutBranch = repoInfo.CheckoutBranch req.WorktreeBranchPrefix = repoInfo.WorktreeBranchPrefix req.PullBeforeWorktree = repoInfo.PullBeforeWorktree if repoInfo.Repository != nil && repoInfo.Repository.SetupScript != "" { diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index 3db5d8e5dc..c0f36baf1d 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -24,6 +24,7 @@ type repoInfo struct { RepositoryID string RepositoryPath string BaseBranch string + CheckoutBranch string WorktreeBranchPrefix string PullBeforeWorktree bool Repository *models.Repository @@ -44,6 +45,7 @@ func (e *Executor) resolvePrimaryRepoInfo(ctx context.Context, taskID string) (* } info.RepositoryID = primaryTaskRepo.RepositoryID info.BaseBranch = primaryTaskRepo.BaseBranch + info.CheckoutBranch = primaryTaskRepo.CheckoutBranch if info.RepositoryID == "" { return info, nil } @@ -64,24 +66,9 @@ func (e *Executor) resolvePrimaryRepoInfo(ctx context.Context, taskID string) (* return info, nil } -// persistLaunchState updates the session and executor running records after a successful agent launch. -// If existingRunning is provided, resume token and message UUID are carried forward without a DB read. -func (e *Executor) persistLaunchState(ctx context.Context, taskID, sessionID string, session *models.TaskSession, resp *LaunchAgentResponse, startAgent bool, now time.Time, execCfg executorConfig, existingRunning *models.ExecutorRunning) { - session.AgentExecutionID = resp.AgentExecutionID - session.ContainerID = resp.ContainerID - if startAgent { - session.State = models.TaskSessionStateStarting - } - session.ErrorMessage = "" - session.UpdatedAt = now - - if err := e.repo.UpdateTaskSession(ctx, session); err != nil { - e.logger.Error("failed to update agent session after launch", - zap.String("task_id", taskID), - zap.String("session_id", sessionID), - zap.Error(err)) - } - +// buildExecutorRunning constructs an ExecutorRunning record from the launch/resume response, +// carrying forward resume token and metadata from the previous running record if available. +func buildExecutorRunning(session *models.TaskSession, taskID string, resp *LaunchAgentResponse, execCfg executorConfig, existingRunning *models.ExecutorRunning) *models.ExecutorRunning { running := &models.ExecutorRunning{ ID: session.ID, SessionID: session.ID, @@ -104,6 +91,28 @@ func (e *Executor) persistLaunchState(ctx context.Context, taskID, sessionID str running.Metadata = lifecycle.FilterPersistentMetadata(existingRunning.Metadata) } } + return running +} + +// persistLaunchState updates the session and executor running records after a successful agent launch. +// If existingRunning is provided, resume token and message UUID are carried forward without a DB read. +func (e *Executor) persistLaunchState(ctx context.Context, taskID, sessionID string, session *models.TaskSession, resp *LaunchAgentResponse, startAgent bool, now time.Time, execCfg executorConfig, existingRunning *models.ExecutorRunning) { + session.AgentExecutionID = resp.AgentExecutionID + session.ContainerID = resp.ContainerID + if startAgent { + session.State = models.TaskSessionStateStarting + } + session.ErrorMessage = "" + session.UpdatedAt = now + + if err := e.repo.UpdateTaskSession(ctx, session); err != nil { + e.logger.Error("failed to update agent session after launch", + zap.String("task_id", taskID), + zap.String("session_id", sessionID), + zap.Error(err)) + } + + running := buildExecutorRunning(session, taskID, resp, execCfg, existingRunning) if err := e.repo.UpsertExecutorRunning(ctx, running); err != nil { e.logger.Warn("failed to persist executor runtime after launch", zap.String("task_id", taskID), @@ -112,13 +121,19 @@ func (e *Executor) persistLaunchState(ctx context.Context, taskID, sessionID str } } -// persistWorktreeAssociation creates a TaskSessionWorktree record if the response contains a worktree. -func (e *Executor) persistWorktreeAssociation(ctx context.Context, taskID, sessionID, repositoryID string, resp *LaunchAgentResponse) { +// persistWorktreeAssociation creates a TaskSessionWorktree record if the response contains +// a new worktree not already tracked by the session. +func (e *Executor) persistWorktreeAssociation(ctx context.Context, taskID string, session *models.TaskSession, repositoryID string, resp *LaunchAgentResponse) { if resp.WorktreeID == "" { return } + for _, wt := range session.Worktrees { + if wt.WorktreeID == resp.WorktreeID { + return + } + } sessionWorktree := &models.TaskSessionWorktree{ - SessionID: sessionID, + SessionID: session.ID, WorktreeID: resp.WorktreeID, RepositoryID: repositoryID, Position: 0, @@ -128,7 +143,7 @@ func (e *Executor) persistWorktreeAssociation(ctx context.Context, taskID, sessi if err := e.repo.CreateTaskSessionWorktree(ctx, sessionWorktree); err != nil { e.logger.Error("failed to persist session worktree association", zap.String("task_id", taskID), - zap.String("session_id", sessionID), + zap.String("session_id", session.ID), zap.String("worktree_id", resp.WorktreeID), zap.Error(err)) } @@ -183,7 +198,7 @@ func (e *Executor) ResumeSession(ctx context.Context, session *models.TaskSessio } e.persistResumeState(ctx, task.ID, session, resp, startAgent, execCfg, existingRunning) - e.persistResumeWorktree(ctx, task.ID, session, repositoryID, resp) + e.persistWorktreeAssociation(ctx, task.ID, session, repositoryID, resp) worktreePath := resp.WorktreePath worktreeBranch := resp.WorktreeBranch @@ -434,6 +449,11 @@ func (e *Executor) applyResumeRepoConfig(ctx context.Context, task *v1.Task, ses } else { req.BaseBranch = defaultBaseBranch } + // Carry forward CheckoutBranch from task repository (e.g. PR head branch) + primaryTaskRepo, _ := e.repo.GetPrimaryTaskRepository(ctx, task.ID) + if primaryTaskRepo != nil && primaryTaskRepo.CheckoutBranch != "" { + req.CheckoutBranch = primaryTaskRepo.CheckoutBranch + } req.WorktreeBranchPrefix = repository.WorktreeBranchPrefix req.PullBeforeWorktree = repository.PullBeforeWorktree } @@ -458,28 +478,7 @@ func (e *Executor) persistResumeState(ctx context.Context, taskID string, sessio zap.Error(err)) } - running := &models.ExecutorRunning{ - ID: session.ID, - SessionID: session.ID, - TaskID: taskID, - ExecutorID: session.ExecutorID, - Runtime: execCfg.RuntimeName, - Status: "starting", - Resumable: execCfg.Resumable, - AgentExecutionID: resp.AgentExecutionID, - ContainerID: resp.ContainerID, - WorktreeID: resp.WorktreeID, - WorktreePath: resp.WorktreePath, - WorktreeBranch: resp.WorktreeBranch, - Metadata: resp.Metadata, - } - if existingRunning != nil { - running.ResumeToken = existingRunning.ResumeToken - running.LastMessageUUID = existingRunning.LastMessageUUID - if running.Metadata == nil { - running.Metadata = lifecycle.FilterPersistentMetadata(existingRunning.Metadata) - } - } + running := buildExecutorRunning(session, taskID, resp, execCfg, existingRunning) if err := e.repo.UpsertExecutorRunning(ctx, running); err != nil { e.logger.Warn("failed to persist executor runtime after resume", zap.String("task_id", taskID), @@ -488,33 +487,6 @@ func (e *Executor) persistResumeState(ctx context.Context, taskID string, sessio } } -// persistResumeWorktree creates a worktree association if a new worktree was allocated during resume. -func (e *Executor) persistResumeWorktree(ctx context.Context, taskID string, session *models.TaskSession, repositoryID string, resp *LaunchAgentResponse) { - if resp.WorktreeID == "" { - return - } - for _, wt := range session.Worktrees { - if wt.WorktreeID == resp.WorktreeID { - return - } - } - sessionWorktree := &models.TaskSessionWorktree{ - SessionID: session.ID, - WorktreeID: resp.WorktreeID, - RepositoryID: repositoryID, - Position: 0, - WorktreePath: resp.WorktreePath, - WorktreeBranch: resp.WorktreeBranch, - } - if err := e.repo.CreateTaskSessionWorktree(ctx, sessionWorktree); err != nil { - e.logger.Error("failed to persist session worktree association on resume", - zap.String("task_id", taskID), - zap.String("session_id", session.ID), - zap.String("worktree_id", resp.WorktreeID), - zap.Error(err)) - } -} - // startAgentProcessOnResume starts the agent process asynchronously after a session resume, // syncing the task state (REVIEW or unchanged) on success. func (e *Executor) startAgentProcessOnResume(ctx context.Context, taskID string, session *models.TaskSession, agentExecutionID string) { diff --git a/apps/backend/internal/orchestrator/executor/executor_resume_test.go b/apps/backend/internal/orchestrator/executor/executor_resume_test.go new file mode 100644 index 0000000000..c5c006b558 --- /dev/null +++ b/apps/backend/internal/orchestrator/executor/executor_resume_test.go @@ -0,0 +1,152 @@ +package executor + +import ( + "testing" + + "github.com/kandev/kandev/internal/agent/lifecycle" + "github.com/kandev/kandev/internal/task/models" +) + +func TestBuildExecutorRunning(t *testing.T) { + t.Run("basic field mapping", func(t *testing.T) { + session := &models.TaskSession{ + ID: "sess-1", + ExecutorID: "exec-1", + } + resp := &LaunchAgentResponse{ + AgentExecutionID: "agent-exec-1", + ContainerID: "container-1", + WorktreeID: "wt-1", + WorktreePath: "/worktrees/wt-1", + WorktreeBranch: "kandev/feature", + Metadata: map[string]interface{}{"key": "val"}, + } + cfg := executorConfig{ + RuntimeName: "standalone", + Resumable: true, + } + + running := buildExecutorRunning(session, "task-1", resp, cfg, nil) + + if running.ID != "sess-1" { + t.Errorf("ID = %q, want %q", running.ID, "sess-1") + } + if running.SessionID != "sess-1" { + t.Errorf("SessionID = %q, want %q", running.SessionID, "sess-1") + } + if running.TaskID != "task-1" { + t.Errorf("TaskID = %q, want %q", running.TaskID, "task-1") + } + if running.ExecutorID != "exec-1" { + t.Errorf("ExecutorID = %q, want %q", running.ExecutorID, "exec-1") + } + if running.Runtime != "standalone" { + t.Errorf("Runtime = %q, want %q", running.Runtime, "standalone") + } + if running.Status != "starting" { + t.Errorf("Status = %q, want %q", running.Status, "starting") + } + if !running.Resumable { + t.Error("Resumable = false, want true") + } + if running.AgentExecutionID != "agent-exec-1" { + t.Errorf("AgentExecutionID = %q, want %q", running.AgentExecutionID, "agent-exec-1") + } + if running.ContainerID != "container-1" { + t.Errorf("ContainerID = %q, want %q", running.ContainerID, "container-1") + } + if running.WorktreeID != "wt-1" { + t.Errorf("WorktreeID = %q, want %q", running.WorktreeID, "wt-1") + } + if running.WorktreePath != "/worktrees/wt-1" { + t.Errorf("WorktreePath = %q, want %q", running.WorktreePath, "/worktrees/wt-1") + } + if running.WorktreeBranch != "kandev/feature" { + t.Errorf("WorktreeBranch = %q, want %q", running.WorktreeBranch, "kandev/feature") + } + if running.Metadata["key"] != "val" { + t.Errorf("Metadata[key] = %v, want %q", running.Metadata["key"], "val") + } + // No existing running: resume token should be empty + if running.ResumeToken != "" { + t.Errorf("ResumeToken = %q, want empty", running.ResumeToken) + } + if running.LastMessageUUID != "" { + t.Errorf("LastMessageUUID = %q, want empty", running.LastMessageUUID) + } + }) + + t.Run("carries forward resume token from existing running", func(t *testing.T) { + session := &models.TaskSession{ID: "sess-1", ExecutorID: "exec-1"} + resp := &LaunchAgentResponse{ + AgentExecutionID: "agent-exec-2", + Metadata: map[string]interface{}{"new_key": "new_val"}, + } + cfg := executorConfig{RuntimeName: "docker", Resumable: true} + existing := &models.ExecutorRunning{ + ResumeToken: "resume-abc", + LastMessageUUID: "msg-uuid-123", + Metadata: map[string]interface{}{ + lifecycle.MetadataKeyMainRepoGitDir: "/repo/.git", + "ephemeral_key": "should_not_carry", + }, + } + + running := buildExecutorRunning(session, "task-1", resp, cfg, existing) + + if running.ResumeToken != "resume-abc" { + t.Errorf("ResumeToken = %q, want %q", running.ResumeToken, "resume-abc") + } + if running.LastMessageUUID != "msg-uuid-123" { + t.Errorf("LastMessageUUID = %q, want %q", running.LastMessageUUID, "msg-uuid-123") + } + // Response had metadata, so existing metadata should NOT be used as fallback + if running.Metadata["new_key"] != "new_val" { + t.Errorf("Metadata should come from response, got %v", running.Metadata) + } + }) + + t.Run("falls back to filtered existing metadata when response has none", func(t *testing.T) { + session := &models.TaskSession{ID: "sess-1", ExecutorID: "exec-1"} + resp := &LaunchAgentResponse{ + AgentExecutionID: "agent-exec-3", + Metadata: nil, // No metadata from response + } + cfg := executorConfig{RuntimeName: "standalone"} + existing := &models.ExecutorRunning{ + ResumeToken: "token-xyz", + Metadata: map[string]interface{}{ + // Use a key that's in the persistent set (sprite_name is persistent) + "sprite_name": "my-sprite", + lifecycle.MetadataKeyMainRepoGitDir: "/repo/.git", // not persistent, should be filtered + }, + } + + running := buildExecutorRunning(session, "task-1", resp, cfg, existing) + + // When response metadata is nil, should fall back to filtered existing metadata + if running.Metadata == nil { + t.Fatal("expected fallback metadata from existing running, got nil") + } + if running.Metadata["sprite_name"] != "my-sprite" { + t.Errorf("expected persistent key 'sprite_name' to be carried forward, got %v", running.Metadata) + } + // Non-persistent keys should be filtered out + if _, ok := running.Metadata[lifecycle.MetadataKeyMainRepoGitDir]; ok { + t.Error("expected non-persistent key to be filtered out") + } + }) + + t.Run("nil existing running does not panic", func(t *testing.T) { + session := &models.TaskSession{ID: "sess-1"} + resp := &LaunchAgentResponse{AgentExecutionID: "agent-exec-4"} + cfg := executorConfig{} + + running := buildExecutorRunning(session, "task-1", resp, cfg, nil) + + // buildExecutorRunning always returns non-nil; verify resume fields are empty + if running.ResumeToken != "" { + t.Errorf("ResumeToken = %q, want empty", running.ResumeToken) + } + }) +} diff --git a/apps/backend/internal/orchestrator/executor/executor_state.go b/apps/backend/internal/orchestrator/executor/executor_state.go index 07675de59d..000b62f591 100644 --- a/apps/backend/internal/orchestrator/executor/executor_state.go +++ b/apps/backend/internal/orchestrator/executor/executor_state.go @@ -2,6 +2,7 @@ package executor import ( "context" + "maps" "strings" "time" @@ -121,8 +122,18 @@ func (e *Executor) resolveExecutorConfig(ctx context.Context, executorID, worksp metadata = make(map[string]interface{}) } + // When no executor ID is resolved, check if the metadata carries an + // executor profile. The profile references a specific executor, so we + // can derive the full config from it (critical for review-watch tasks + // where the executor comes solely from the profile). if resolved == "" { - return executorConfig{Metadata: metadata} + profileID, _ := metadata["executor_profile_id"].(string) + if profileID == "" { + return executorConfig{Metadata: metadata} + } + cfg := executorConfig{Metadata: metadata} + e.applyProfile(ctx, profileID, &cfg, metadata) + return cfg } metadata["executor_id"] = resolved @@ -165,6 +176,23 @@ func (e *Executor) applyProfile(ctx context.Context, profileID string, cfg *exec if profile == nil { return } + + // The profile is tied to a specific executor. If it differs from the + // currently resolved executor (e.g. the workspace default), override the + // config so the correct executor type is used. This is critical for + // worktree executors selected via review watches or executor profiles. + if profile.ExecutorID != "" && profile.ExecutorID != cfg.ExecutorID { + exec, execErr := e.repo.GetExecutor(ctx, profile.ExecutorID) + if execErr == nil && exec != nil { + cfg.ExecutorID = profile.ExecutorID + cfg.ExecutorType = string(exec.Type) + cfg.ExecutorCfg = exec.Config + cfg.Resumable = exec.Resumable + cfg.RuntimeName = string(exec.Type) + metadata["executor_id"] = profile.ExecutorID + } + } + cfg.SetupScript = profile.PrepareScript cfg.CleanupScript = profile.CleanupScript cfg.ProfileEnv = e.resolveProfileEnvVars(ctx, profile.EnvVars) @@ -230,8 +258,6 @@ func cloneMetadata(src map[string]interface{}) map[string]interface{} { return nil } out := make(map[string]interface{}, len(src)) - for key, value := range src { - out[key] = value - } + maps.Copy(out, src) return out } diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index 2d277f5101..1e4f349e42 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -60,14 +60,15 @@ type Workspace struct { // TaskRepository represents a repository associated with a task type TaskRepository struct { - ID string `json:"id"` - TaskID string `json:"task_id"` - RepositoryID string `json:"repository_id"` - BaseBranch string `json:"base_branch"` - Position int `json:"position"` - Metadata map[string]interface{} `json:"metadata,omitempty"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` + ID string `json:"id"` + TaskID string `json:"task_id"` + RepositoryID string `json:"repository_id"` + BaseBranch string `json:"base_branch"` + CheckoutBranch string `json:"checkout_branch,omitempty"` + Position int `json:"position"` + Metadata map[string]interface{} `json:"metadata,omitempty"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` } // MessageAuthorType represents who authored a message diff --git a/apps/backend/internal/task/repository/sqlite/base.go b/apps/backend/internal/task/repository/sqlite/base.go index a60decf0d4..3226fb7ca3 100644 --- a/apps/backend/internal/task/repository/sqlite/base.go +++ b/apps/backend/internal/task/repository/sqlite/base.go @@ -130,6 +130,8 @@ func (r *Repository) runMigrations() error { _, _ = r.db.Exec(`ALTER TABLE executors_running ADD COLUMN last_message_uuid TEXT DEFAULT ''`) // Add metadata column to executors_running (ignore error if already exists) _, _ = r.db.Exec(`ALTER TABLE executors_running ADD COLUMN metadata TEXT DEFAULT '{}'`) + // Add checkout_branch column to task_repositories (ignore error if already exists) + _, _ = r.db.Exec(`ALTER TABLE task_repositories ADD COLUMN checkout_branch TEXT DEFAULT ''`) return nil } diff --git a/apps/backend/internal/task/repository/sqlite/task_repository.go b/apps/backend/internal/task/repository/sqlite/task_repository.go index 4a4cec1998..0e1752c332 100644 --- a/apps/backend/internal/task/repository/sqlite/task_repository.go +++ b/apps/backend/internal/task/repository/sqlite/task_repository.go @@ -29,9 +29,9 @@ func (r *Repository) CreateTaskRepository(ctx context.Context, taskRepo *models. _, err = r.db.ExecContext(ctx, r.db.Rebind(` INSERT INTO task_repositories ( - id, task_id, repository_id, base_branch, position, metadata, created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?) - `), taskRepo.ID, taskRepo.TaskID, taskRepo.RepositoryID, taskRepo.BaseBranch, taskRepo.Position, string(metadataJSON), taskRepo.CreatedAt, taskRepo.UpdatedAt) + id, task_id, repository_id, base_branch, checkout_branch, position, metadata, created_at, updated_at + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + `), taskRepo.ID, taskRepo.TaskID, taskRepo.RepositoryID, taskRepo.BaseBranch, taskRepo.CheckoutBranch, taskRepo.Position, string(metadataJSON), taskRepo.CreatedAt, taskRepo.UpdatedAt) return err } @@ -41,13 +41,14 @@ func (r *Repository) GetTaskRepository(ctx context.Context, id string) (*models. var metadataJSON string err := r.ro.QueryRowContext(ctx, r.ro.Rebind(` - SELECT id, task_id, repository_id, base_branch, position, metadata, created_at, updated_at + SELECT id, task_id, repository_id, base_branch, checkout_branch, position, metadata, created_at, updated_at FROM task_repositories WHERE id = ? `), id).Scan( &taskRepo.ID, &taskRepo.TaskID, &taskRepo.RepositoryID, &taskRepo.BaseBranch, + &taskRepo.CheckoutBranch, &taskRepo.Position, &metadataJSON, &taskRepo.CreatedAt, @@ -70,7 +71,7 @@ func (r *Repository) GetTaskRepository(ctx context.Context, id string) (*models. // ListTaskRepositories returns all repository links for a task func (r *Repository) ListTaskRepositories(ctx context.Context, taskID string) ([]*models.TaskRepository, error) { rows, err := r.ro.QueryContext(ctx, r.ro.Rebind(` - SELECT id, task_id, repository_id, base_branch, position, metadata, created_at, updated_at + SELECT id, task_id, repository_id, base_branch, checkout_branch, position, metadata, created_at, updated_at FROM task_repositories WHERE task_id = ? ORDER BY position ASC, created_at ASC @@ -89,6 +90,7 @@ func (r *Repository) ListTaskRepositories(ctx context.Context, taskID string) ([ &taskRepo.TaskID, &taskRepo.RepositoryID, &taskRepo.BaseBranch, + &taskRepo.CheckoutBranch, &taskRepo.Position, &metadataJSON, &taskRepo.CreatedAt, @@ -117,9 +119,9 @@ func (r *Repository) UpdateTaskRepository(ctx context.Context, taskRepo *models. result, err := r.db.ExecContext(ctx, r.db.Rebind(` UPDATE task_repositories SET - task_id = ?, repository_id = ?, base_branch = ?, position = ?, metadata = ?, updated_at = ? + task_id = ?, repository_id = ?, base_branch = ?, checkout_branch = ?, position = ?, metadata = ?, updated_at = ? WHERE id = ? - `), taskRepo.TaskID, taskRepo.RepositoryID, taskRepo.BaseBranch, taskRepo.Position, string(metadataJSON), taskRepo.UpdatedAt, taskRepo.ID) + `), taskRepo.TaskID, taskRepo.RepositoryID, taskRepo.BaseBranch, taskRepo.CheckoutBranch, taskRepo.Position, string(metadataJSON), taskRepo.UpdatedAt, taskRepo.ID) if err != nil { return err } @@ -159,7 +161,7 @@ func (r *Repository) ListTaskRepositoriesByTaskIDs(ctx context.Context, taskIDs } query := fmt.Sprintf(` - SELECT id, task_id, repository_id, base_branch, position, metadata, created_at, updated_at + SELECT id, task_id, repository_id, base_branch, checkout_branch, position, metadata, created_at, updated_at FROM task_repositories WHERE task_id IN (%s) ORDER BY position ASC, created_at ASC @@ -179,6 +181,7 @@ func (r *Repository) ListTaskRepositoriesByTaskIDs(ctx context.Context, taskIDs &taskRepo.TaskID, &taskRepo.RepositoryID, &taskRepo.BaseBranch, + &taskRepo.CheckoutBranch, &taskRepo.Position, &metadataJSON, &taskRepo.CreatedAt, diff --git a/apps/backend/internal/task/service/service_requests.go b/apps/backend/internal/task/service/service_requests.go index 66b7f3bb0f..3e8e07f7e4 100644 --- a/apps/backend/internal/task/service/service_requests.go +++ b/apps/backend/internal/task/service/service_requests.go @@ -10,11 +10,12 @@ import ( // TaskRepositoryInput for creating/updating task repositories type TaskRepositoryInput struct { - RepositoryID string `json:"repository_id"` - BaseBranch string `json:"base_branch"` - LocalPath string `json:"local_path,omitempty"` - Name string `json:"name,omitempty"` - DefaultBranch string `json:"default_branch,omitempty"` + RepositoryID string `json:"repository_id"` + BaseBranch string `json:"base_branch"` + CheckoutBranch string `json:"checkout_branch,omitempty"` + LocalPath string `json:"local_path,omitempty"` + Name string `json:"name,omitempty"` + DefaultBranch string `json:"default_branch,omitempty"` } // CreateTaskRequest contains the data for creating a new task diff --git a/apps/backend/internal/task/service/service_tasks.go b/apps/backend/internal/task/service/service_tasks.go index 36aeaaf492..99cd28477d 100644 --- a/apps/backend/internal/task/service/service_tasks.go +++ b/apps/backend/internal/task/service/service_tasks.go @@ -114,11 +114,12 @@ func (s *Service) createTaskRepositories(ctx context.Context, taskID, workspaceI return fmt.Errorf("repository_id is required") } taskRepo := &models.TaskRepository{ - TaskID: taskID, - RepositoryID: repositoryID, - BaseBranch: baseBranch, - Position: i, - Metadata: make(map[string]interface{}), + TaskID: taskID, + RepositoryID: repositoryID, + BaseBranch: baseBranch, + CheckoutBranch: repoInput.CheckoutBranch, + Position: i, + Metadata: make(map[string]interface{}), } if err := s.taskRepos.CreateTaskRepository(ctx, taskRepo); err != nil { s.logger.Error("failed to create task repository", zap.Error(err)) diff --git a/apps/backend/internal/worktree/manager.go b/apps/backend/internal/worktree/manager.go index 5d87278c3c..475cc1bc26 100644 --- a/apps/backend/internal/worktree/manager.go +++ b/apps/backend/internal/worktree/manager.go @@ -239,6 +239,17 @@ 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) { + // If a checkout branch is specified (e.g. a PR's head branch), fetch it + // first and use it as the base so the new worktree branch is created on + // top of the PR branch rather than on top of the default branch. + if req.CheckoutBranch != "" { + fetchedRef, err := m.fetchBranchRef(ctx, req.RepositoryPath, req.CheckoutBranch) + if err != nil { + return nil, err + } + baseRef = fetchedRef + } + worktreeDirName, branchName := m.buildWorktreeNames(req) worktreePath, err := m.config.WorktreePath(worktreeDirName) @@ -265,15 +276,34 @@ func (m *Manager) createWorktree(ctx context.Context, req CreateRequest, baseRef zap.String("session_id", req.SessionID), zap.String("task_id", req.TaskID), zap.String("path", worktreePath), - zap.String("branch", branchName)) + zap.String("branch", wt.Branch)) return wt, nil } +// fetchBranchRef fetches a branch from origin in the given repository and returns +// the FETCH_HEAD ref that can be used as the base for a new worktree branch. +// This is used for PR reviews so the new worktree branch is created on top of +// the PR's head branch. +func (m *Manager) fetchBranchRef(ctx context.Context, repoPath, branch string) (string, error) { + m.logger.Info("fetching branch for worktree base", + zap.String("branch", branch), + zap.String("repo_path", repoPath)) + + fetchCtx, cancelFetch := context.WithTimeout(ctx, 30*time.Second) + defer cancelFetch() + + fetchCmd := m.newNonInteractiveGitCmd(fetchCtx, repoPath, "fetch", "origin", branch) + if output, err := fetchCmd.CombinedOutput(); err != nil { + return "", fmt.Errorf("failed to fetch branch %q: %s", branch, string(output)) + } + + return "FETCH_HEAD", nil +} + // buildWorktreeNames derives the filesystem directory name and git branch name for a new worktree. func (m *Manager) buildWorktreeNames(req CreateRequest) (dirName, branchName string) { - worktreeID := uuid.New().String() - dirSuffix := worktreeID[:8] // Use first 8 chars of UUID for worktree dir uniqueness + dirSuffix := uuid.New().String()[:8] // Use first 8 chars of UUID for worktree dir uniqueness branchSuffix := SmallSuffix(3) prefix := NormalizeBranchPrefix(req.WorktreeBranchPrefix) diff --git a/apps/backend/internal/worktree/manager_names_test.go b/apps/backend/internal/worktree/manager_names_test.go new file mode 100644 index 0000000000..e2e5075f25 --- /dev/null +++ b/apps/backend/internal/worktree/manager_names_test.go @@ -0,0 +1,101 @@ +package worktree + +import ( + "strings" + "testing" +) + +func TestBuildWorktreeNames(t *testing.T) { + cfg := newTestConfig(t) + log := newTestLogger() + store := newMockStore() + mgr, err := NewManager(cfg, store, log) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + + t.Run("semantic naming with task title", func(t *testing.T) { + req := CreateRequest{ + TaskID: "task-abc", + TaskTitle: "Fix login bug", + WorktreeBranchPrefix: "kandev/", + } + dirName, branchName := mgr.buildWorktreeNames(req) + + // Dir should contain sanitized title and a UUID suffix + if !strings.HasPrefix(dirName, "fix-login-bug_") { + t.Errorf("dirName = %q, want prefix %q", dirName, "fix-login-bug_") + } + // UUID suffix should be 8 chars + parts := strings.SplitN(dirName, "_", 2) + if len(parts) != 2 || len(parts[1]) != 8 { + t.Errorf("dirName suffix should be 8 chars, got %q", dirName) + } + + // Branch should have prefix + sanitized title + suffix + if !strings.HasPrefix(branchName, "kandev/fix-login-bug-") { + t.Errorf("branchName = %q, want prefix %q", branchName, "kandev/fix-login-bug-") + } + }) + + t.Run("fallback to task ID when title is empty", func(t *testing.T) { + req := CreateRequest{ + TaskID: "task-xyz-123", + TaskTitle: "", + WorktreeBranchPrefix: "kandev/", + } + dirName, branchName := mgr.buildWorktreeNames(req) + + if !strings.HasPrefix(dirName, "task-xyz-123_") { + t.Errorf("dirName = %q, want prefix %q", dirName, "task-xyz-123_") + } + if !strings.HasPrefix(branchName, "kandev/task-xyz-123-") { + t.Errorf("branchName = %q, want prefix %q", branchName, "kandev/task-xyz-123-") + } + }) + + t.Run("empty branch prefix", func(t *testing.T) { + req := CreateRequest{ + TaskID: "task-1", + TaskTitle: "Add feature", + WorktreeBranchPrefix: "", + } + _, branchName := mgr.buildWorktreeNames(req) + + // Default prefix from NormalizeBranchPrefix("") + defaultPrefix := NormalizeBranchPrefix("") + if !strings.HasPrefix(branchName, defaultPrefix+"add-feature-") { + t.Errorf("branchName = %q, want prefix %q", branchName, defaultPrefix+"add-feature-") + } + }) + + t.Run("title with only special chars falls back to task ID for branch", func(t *testing.T) { + req := CreateRequest{ + TaskID: "task-fallback", + TaskTitle: "!@#$%", + WorktreeBranchPrefix: "kandev/", + } + _, branchName := mgr.buildWorktreeNames(req) + + // SanitizeForBranch("!@#$%") returns "", so should fall back to task ID + if !strings.HasPrefix(branchName, "kandev/task-fallback-") { + t.Errorf("branchName = %q, want prefix %q", branchName, "kandev/task-fallback-") + } + }) + + t.Run("unique names on successive calls", func(t *testing.T) { + req := CreateRequest{ + TaskID: "task-1", + TaskTitle: "Same title", + } + dir1, branch1 := mgr.buildWorktreeNames(req) + dir2, branch2 := mgr.buildWorktreeNames(req) + + if dir1 == dir2 { + t.Errorf("expected unique dir names, both are %q", dir1) + } + if branch1 == branch2 { + t.Errorf("expected unique branch names, both are %q", branch1) + } + }) +} diff --git a/apps/backend/internal/worktree/worktree.go b/apps/backend/internal/worktree/worktree.go index 3740fbbb92..eff86b04d6 100644 --- a/apps/backend/internal/worktree/worktree.go +++ b/apps/backend/internal/worktree/worktree.go @@ -70,6 +70,11 @@ 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 string + // WorktreeBranchPrefix is the prefix to use for the worktree branch name. // If empty, the default prefix is used. WorktreeBranchPrefix string diff --git a/apps/web/components/github/pr-topbar-button.tsx b/apps/web/components/github/pr-topbar-button.tsx index 959a8fd414..99f7a5588e 100644 --- a/apps/web/components/github/pr-topbar-button.tsx +++ b/apps/web/components/github/pr-topbar-button.tsx @@ -49,6 +49,7 @@ export const PRTopbarButton = memo(function PRTopbarButton() {