From 5900ba013d865d5609ace03ef78be424cde00ce8 Mon Sep 17 00:00:00 2001 From: Matthew Newhook Date: Thu, 29 Jan 2026 22:19:55 -0500 Subject: [PATCH 1/4] Extract interfaces for testability in control and orchestration packages - Create feedback.Processor interface in internal/feedback/interface.go with DefaultProcessor implementation wrapping ProcessPRFeedbackQuiet - Create ControlPlane struct in internal/control/plane.go with: - git.Operations, worktree.Operations, zellij.SessionManager - mise.Operations factory, feedback.Processor - OrchestratorSpawner and WorkDestroyer interfaces - NewControlPlane() with defaults, NewControlPlaneWithDeps() for testing - Update loop.go to use ControlPlane with RunControlPlaneLoopWithControlPlane and ProcessAllDueTasksWithControlPlane for testing This enables unit testing of control plane tasks without external dependencies (CLI tools, services, file system operations). Closes: ac-a7av, ac-a7av.1, ac-a7av.2 Co-Authored-By: Claude Opus 4.5 --- internal/control/loop.go | 35 ++-- internal/control/plane.go | 362 +++++++++++++++++++++++++++++++++ internal/feedback/interface.go | 32 +++ 3 files changed, 413 insertions(+), 16 deletions(-) create mode 100644 internal/control/plane.go create mode 100644 internal/feedback/interface.go diff --git a/internal/control/loop.go b/internal/control/loop.go index 8d7c89e..07eb16f 100644 --- a/internal/control/loop.go +++ b/internal/control/loop.go @@ -13,8 +13,15 @@ import ( trackingwatcher "github.com/newhook/co/internal/tracking/watcher" ) -// RunControlPlaneLoop runs the main control plane event loop +// RunControlPlaneLoop runs the main control plane event loop with default dependencies. func RunControlPlaneLoop(ctx context.Context, proj *project.Project, procManager *procmon.Manager) error { + cp := NewControlPlane() + return RunControlPlaneLoopWithControlPlane(ctx, proj, procManager, cp) +} + +// RunControlPlaneLoopWithControlPlane runs the main control plane event loop with provided dependencies. +// This allows testing with mock dependencies. +func RunControlPlaneLoopWithControlPlane(ctx context.Context, proj *project.Project, procManager *procmon.Manager, cp *ControlPlane) error { // Initialize tracking database watcher trackingDBPath := filepath.Join(proj.Root, ".co", "tracking.db") watcher, err := trackingwatcher.New(trackingwatcher.DefaultConfig(trackingDBPath)) @@ -59,13 +66,13 @@ func RunControlPlaneLoop(ctx context.Context, proj *project.Project, procManager // Handle database change event if event.Payload.Type == trackingwatcher.DBChanged { logging.Debug("Database changed, checking scheduled tasks") - ProcessAllDueTasks(ctx, proj) + ProcessAllDueTasksWithControlPlane(ctx, proj, cp) } case <-checkTimer.C: // Periodic check as a safety net logging.Debug("Control plane periodic check") - ProcessAllDueTasks(ctx, proj) + ProcessAllDueTasksWithControlPlane(ctx, proj, cp) checkTimer.Reset(checkInterval) case <-cleanupTimer.C: @@ -82,21 +89,17 @@ func RunControlPlaneLoop(ctx context.Context, proj *project.Project, procManager // TaskHandler is the signature for all scheduled task handlers. type TaskHandler func(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error -// taskHandlers maps task types to their handler functions. -var taskHandlers = map[string]TaskHandler{ - db.TaskTypeCreateWorktree: HandleCreateWorktreeTask, - db.TaskTypeImportPR: HandleImportPRTask, - db.TaskTypeSpawnOrchestrator: HandleSpawnOrchestratorTask, - db.TaskTypePRFeedback: HandlePRFeedbackTask, - db.TaskTypeCommentResolution: HandleCommentResolutionTask, - db.TaskTypeGitPush: HandleGitPushTask, - db.TaskTypeGitHubComment: HandleGitHubCommentTask, - db.TaskTypeGitHubResolveThread: HandleGitHubResolveThreadTask, - db.TaskTypeDestroyWorktree: HandleDestroyWorktreeTask, +// ProcessAllDueTasks checks for and executes any scheduled tasks that are due across all works. +// This uses the default ControlPlane with production dependencies. +func ProcessAllDueTasks(ctx context.Context, proj *project.Project) { + cp := NewControlPlane() + ProcessAllDueTasksWithControlPlane(ctx, proj, cp) } -// ProcessAllDueTasks checks for and executes any scheduled tasks that are due across all works -func ProcessAllDueTasks(ctx context.Context, proj *project.Project) { +// ProcessAllDueTasksWithControlPlane checks for and executes any scheduled tasks with provided dependencies. +func ProcessAllDueTasksWithControlPlane(ctx context.Context, proj *project.Project, cp *ControlPlane) { + taskHandlers := cp.GetTaskHandlers() + // Get the next due task globally (not work-specific) for { task, err := proj.DB.GetNextScheduledTask(ctx) diff --git a/internal/control/plane.go b/internal/control/plane.go new file mode 100644 index 0000000..a05e0cd --- /dev/null +++ b/internal/control/plane.go @@ -0,0 +1,362 @@ +package control + +import ( + "context" + "fmt" + "io" + "os" + "path/filepath" + "time" + + "github.com/newhook/co/internal/claude" + "github.com/newhook/co/internal/db" + "github.com/newhook/co/internal/feedback" + "github.com/newhook/co/internal/git" + "github.com/newhook/co/internal/logging" + "github.com/newhook/co/internal/mise" + "github.com/newhook/co/internal/project" + "github.com/newhook/co/internal/work" + "github.com/newhook/co/internal/worktree" + "github.com/newhook/co/internal/zellij" +) + +// OrchestratorSpawner defines the interface for spawning work orchestrators. +// This abstraction enables testing without actual zellij operations. +type OrchestratorSpawner interface { + SpawnWorkOrchestrator(ctx context.Context, workID, projectName, workDir, friendlyName string, w io.Writer) error +} + +// WorkDestroyer defines the interface for destroying work units. +// This abstraction enables testing without actual file system operations. +type WorkDestroyer interface { + DestroyWork(ctx context.Context, proj *project.Project, workID string, w io.Writer) error +} + +// DefaultOrchestratorSpawner implements OrchestratorSpawner using the claude package. +type DefaultOrchestratorSpawner struct{} + +// SpawnWorkOrchestrator implements OrchestratorSpawner. +func (d *DefaultOrchestratorSpawner) SpawnWorkOrchestrator(ctx context.Context, workID, projectName, workDir, friendlyName string, w io.Writer) error { + return claude.SpawnWorkOrchestrator(ctx, workID, projectName, workDir, friendlyName, w) +} + +// DefaultWorkDestroyer implements WorkDestroyer using the work package. +type DefaultWorkDestroyer struct{} + +// DestroyWork implements WorkDestroyer. +func (d *DefaultWorkDestroyer) DestroyWork(ctx context.Context, proj *project.Project, workID string, w io.Writer) error { + return work.DestroyWork(ctx, proj, workID, w) +} + +// ControlPlane manages the execution of scheduled tasks with injectable dependencies. +// It allows for testing without actual CLI tools, services, or file system operations. +type ControlPlane struct { + Git git.Operations + Worktree worktree.Operations + Zellij zellij.SessionManager + Mise func(dir string) mise.Operations + FeedbackProcessor feedback.Processor + OrchestratorSpawner OrchestratorSpawner + WorkDestroyer WorkDestroyer +} + +// NewControlPlane creates a new ControlPlane with default production dependencies. +func NewControlPlane() *ControlPlane { + return &ControlPlane{ + Git: git.NewOperations(), + Worktree: worktree.NewOperations(), + Zellij: zellij.New(), + Mise: mise.NewOperations, + FeedbackProcessor: feedback.NewProcessor(), + OrchestratorSpawner: &DefaultOrchestratorSpawner{}, + WorkDestroyer: &DefaultWorkDestroyer{}, + } +} + +// NewControlPlaneWithDeps creates a new ControlPlane with provided dependencies for testing. +func NewControlPlaneWithDeps( + gitOps git.Operations, + wtOps worktree.Operations, + zellijMgr zellij.SessionManager, + miseOps func(dir string) mise.Operations, + feedbackProc feedback.Processor, + orchestratorSpawner OrchestratorSpawner, + workDestroyer WorkDestroyer, +) *ControlPlane { + return &ControlPlane{ + Git: gitOps, + Worktree: wtOps, + Zellij: zellijMgr, + Mise: miseOps, + FeedbackProcessor: feedbackProc, + OrchestratorSpawner: orchestratorSpawner, + WorkDestroyer: workDestroyer, + } +} + +// HandleCreateWorktreeTask handles a scheduled worktree creation task. +func (cp *ControlPlane) HandleCreateWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + branchName := task.Metadata["branch"] + baseBranch := task.Metadata["base_branch"] + workerName := task.Metadata["worker_name"] + useExisting := task.Metadata["use_existing"] == "true" + + if baseBranch == "" { + baseBranch = proj.Config.Repo.GetBaseBranch() + } + + logging.Info("Creating worktree for work", + "work_id", workID, + "branch", branchName, + "base_branch", baseBranch, + "use_existing", useExisting, + "attempt", task.AttemptCount+1) + + // Get work details + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil { + return fmt.Errorf("failed to get work: %w", err) + } + if workRecord == nil { + // Work was deleted - nothing to do + logging.Info("Work not found, task will be marked completed", "work_id", workID) + return nil + } + + mainRepoPath := proj.MainRepoPath() + var branchExistsOnRemote bool + + // For existing branches, check if branch exists on remote + if useExisting { + _, branchExistsOnRemote, _ = cp.Git.ValidateExistingBranch(ctx, mainRepoPath, branchName) + } + + // If worktree path is already set and exists, skip creation + if workRecord.WorktreePath != "" { + // Worktree already created - just need to ensure git push + logging.Info("Worktree already exists, skipping creation", "work_id", workID, "path", workRecord.WorktreePath) + } else { + // Create the worktree + workDir := filepath.Join(proj.Root, workID) + worktreePath := filepath.Join(workDir, "tree") + + // Create work directory + if err := os.Mkdir(workDir, 0750); err != nil && !os.IsExist(err) { + return fmt.Errorf("failed to create work directory: %w", err) + } + + if useExisting { + // For existing branches, fetch the branch first + if err := cp.Git.FetchBranch(ctx, mainRepoPath, branchName); err != nil { + // Ignore fetch errors - branch might only exist locally + logging.Debug("Could not fetch branch from origin (may only exist locally)", "branch", branchName) + } + + // Create worktree from existing branch + if err := cp.Worktree.CreateFromExisting(ctx, mainRepoPath, worktreePath, branchName); err != nil { + _ = os.RemoveAll(workDir) + return fmt.Errorf("failed to create worktree from existing branch: %w", err) + } + } else { + // Fetch latest from origin for the base branch + if err := cp.Git.FetchBranch(ctx, mainRepoPath, baseBranch); err != nil { + _ = os.RemoveAll(workDir) + return fmt.Errorf("failed to fetch base branch: %w", err) + } + + // Create git worktree with new branch based on origin/ + if err := cp.Worktree.Create(ctx, mainRepoPath, worktreePath, branchName, "origin/"+baseBranch); err != nil { + _ = os.RemoveAll(workDir) + return fmt.Errorf("failed to create worktree: %w", err) + } + } + + // Initialize mise if configured + miseOps := cp.Mise(worktreePath) + if err := miseOps.InitializeWithOutput(io.Discard); err != nil { + logging.Warn("mise initialization failed", "error", err) + // Non-fatal, continue + } + + // Update work with worktree path + if err := proj.DB.UpdateWorkWorktreePath(ctx, workID, worktreePath); err != nil { + return fmt.Errorf("failed to update work worktree path: %w", err) + } + } + + // Attempt git push (skip for existing branches that already exist on remote) + workRecord, _ = proj.DB.GetWork(ctx, workID) // Refresh work + if workRecord != nil && workRecord.WorktreePath != "" { + if useExisting && branchExistsOnRemote { + logging.Info("Skipping git push - branch already exists on remote", "branch", branchName) + } else { + if err := cp.Git.PushSetUpstream(ctx, branchName, workRecord.WorktreePath); err != nil { + return fmt.Errorf("git push failed: %w", err) + } + } + } + + logging.Info("Worktree created and pushed successfully", "work_id", workID) + + // Schedule orchestrator spawn task + _, err = proj.DB.ScheduleTask(ctx, workID, db.TaskTypeSpawnOrchestrator, time.Now(), map[string]string{ + "worker_name": workerName, + }) + if err != nil { + logging.Warn("failed to schedule orchestrator spawn", "error", err, "work_id", workID) + } + + return nil +} + +// HandleSpawnOrchestratorTask handles a scheduled orchestrator spawn task. +func (cp *ControlPlane) HandleSpawnOrchestratorTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + workerName := task.Metadata["worker_name"] + + logging.Info("Spawning orchestrator for work", + "work_id", workID, + "attempt", task.AttemptCount+1) + + // Get work details + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil { + return fmt.Errorf("failed to get work: %w", err) + } + if workRecord == nil { + // Work was deleted - nothing to do + logging.Info("Work not found, task will be marked completed", "work_id", workID) + return nil + } + + if workRecord.WorktreePath == "" { + return fmt.Errorf("work %s has no worktree path", workID) + } + + // Spawn the orchestrator + if err := cp.OrchestratorSpawner.SpawnWorkOrchestrator(ctx, workID, proj.Config.Project.Name, workRecord.WorktreePath, workerName, io.Discard); err != nil { + return fmt.Errorf("failed to spawn orchestrator: %w", err) + } + + logging.Info("Orchestrator spawned successfully", "work_id", workID) + + return nil +} + +// HandleDestroyWorktreeTask handles a scheduled worktree destruction task. +func (cp *ControlPlane) HandleDestroyWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + + logging.Info("Destroying worktree for work", + "work_id", workID, + "attempt", task.AttemptCount+1) + + // Check if work still exists + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil { + return err + } + if workRecord == nil { + // Work was already deleted - nothing to do + logging.Info("Work not found, task will be marked completed", "work_id", workID) + return nil + } + + // Delegate to the work destroyer + if err := cp.WorkDestroyer.DestroyWork(ctx, proj, workID, io.Discard); err != nil { + return err + } + + logging.Info("Worktree destroyed successfully", "work_id", workID) + + return nil +} + +// HandleGitPushTask handles a scheduled git push task with retry support. +func (cp *ControlPlane) HandleGitPushTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + // Get branch and directory from metadata + branch := task.Metadata["branch"] + dir := task.Metadata["dir"] + + if branch == "" { + // Try to get from work + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil || workRecord == nil { + return fmt.Errorf("failed to get work for git push: work not found") + } + branch = workRecord.BranchName + dir = workRecord.WorktreePath + } + + if branch == "" || dir == "" { + return fmt.Errorf("git push task missing branch or dir metadata") + } + + logging.Info("Executing git push", "branch", branch, "dir", dir, "attempt", task.AttemptCount+1) + + if err := cp.Git.PushSetUpstream(ctx, branch, dir); err != nil { + return err + } + + logging.Info("Git push succeeded", "branch", branch, "work_id", workID) + + return nil +} + +// HandlePRFeedbackTask handles a scheduled PR feedback check. +func (cp *ControlPlane) HandlePRFeedbackTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + logging.Debug("Starting PR feedback check task", "task_id", task.ID, "work_id", workID) + + // Get work details + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil || workRecord == nil || workRecord.PRURL == "" { + logging.Debug("No PR URL for work, not rescheduling", "work_id", workID, "has_pr", workRecord != nil && workRecord.PRURL != "") + // Don't reschedule - scheduling happens when PR is created + return nil + } + + logging.Debug("Checking PR feedback", "pr_url", workRecord.PRURL, "work_id", workID) + + // Process PR feedback - creates beads but doesn't add them to work + createdCount, err := cp.FeedbackProcessor.ProcessPRFeedback(ctx, proj, proj.DB, workID) + if err != nil { + return fmt.Errorf("failed to check PR feedback: %w", err) + } + + if createdCount > 0 { + logging.Info("Created beads from PR feedback", "count", createdCount, "work_id", workID) + } else { + logging.Debug("No new PR feedback found", "work_id", workID) + } + + // Schedule next check using configured interval + nextInterval := proj.Config.Scheduler.GetPRFeedbackInterval() + nextCheck := time.Now().Add(nextInterval) + _, err = proj.DB.ScheduleOrUpdateTask(ctx, workID, db.TaskTypePRFeedback, nextCheck) + if err != nil { + logging.Warn("failed to schedule next PR feedback check", "error", err, "work_id", workID) + } else { + logging.Info("Scheduled next PR feedback check", "work_id", workID, "next_check", nextCheck.Format(time.RFC3339), "interval", nextInterval) + } + + return nil +} + +// GetTaskHandlers returns the task handler map for the control plane. +func (cp *ControlPlane) GetTaskHandlers() map[string]TaskHandler { + return map[string]TaskHandler{ + db.TaskTypeCreateWorktree: cp.HandleCreateWorktreeTask, + db.TaskTypeSpawnOrchestrator: cp.HandleSpawnOrchestratorTask, + db.TaskTypePRFeedback: cp.HandlePRFeedbackTask, + db.TaskTypeGitPush: cp.HandleGitPushTask, + db.TaskTypeDestroyWorktree: cp.HandleDestroyWorktreeTask, + // These handlers don't need ControlPlane dependencies - keep as standalone functions + db.TaskTypeImportPR: HandleImportPRTask, + db.TaskTypeCommentResolution: HandleCommentResolutionTask, + db.TaskTypeGitHubComment: HandleGitHubCommentTask, + db.TaskTypeGitHubResolveThread: HandleGitHubResolveThreadTask, + } +} diff --git a/internal/feedback/interface.go b/internal/feedback/interface.go new file mode 100644 index 0000000..9909b35 --- /dev/null +++ b/internal/feedback/interface.go @@ -0,0 +1,32 @@ +package feedback + +import ( + "context" + + "github.com/newhook/co/internal/db" + "github.com/newhook/co/internal/project" +) + +// Processor defines the interface for processing PR feedback. +// This abstraction enables testing without actual GitHub API calls. +type Processor interface { + // ProcessPRFeedback processes PR feedback for a work and creates beads. + // Returns the number of beads created and any error. + ProcessPRFeedback(ctx context.Context, proj *project.Project, database *db.DB, workID string) (int, error) +} + +// DefaultProcessor implements Processor using the actual feedback processing logic. +type DefaultProcessor struct{} + +// Compile-time check that DefaultProcessor implements Processor. +var _ Processor = (*DefaultProcessor)(nil) + +// NewProcessor creates a new default Processor implementation. +func NewProcessor() Processor { + return &DefaultProcessor{} +} + +// ProcessPRFeedback implements Processor.ProcessPRFeedback. +func (p *DefaultProcessor) ProcessPRFeedback(ctx context.Context, proj *project.Project, database *db.DB, workID string) (int, error) { + return ProcessPRFeedbackQuiet(ctx, proj, database, workID) +} From bef00f09428d6b1114bf110434e27d91f4b7d108 Mon Sep 17 00:00:00 2001 From: Matthew Newhook Date: Thu, 29 Jan 2026 22:24:05 -0500 Subject: [PATCH 2/4] Remove orphaned handler files from control package Delete standalone handler functions that are now implemented as methods on ControlPlane in plane.go: - handler_create_worktree.go - handler_spawn_orchestrator.go - handler_pr_feedback.go - handler_git_push.go - handler_destroy_worktree.go Co-Authored-By: Claude Opus 4.5 --- internal/control/handler_create_worktree.go | 134 ------------------ internal/control/handler_destroy_worktree.go | 40 ------ internal/control/handler_git_push.go | 44 ------ internal/control/handler_pr_feedback.go | 53 ------- .../control/handler_spawn_orchestrator.go | 46 ------ 5 files changed, 317 deletions(-) delete mode 100644 internal/control/handler_create_worktree.go delete mode 100644 internal/control/handler_destroy_worktree.go delete mode 100644 internal/control/handler_git_push.go delete mode 100644 internal/control/handler_pr_feedback.go delete mode 100644 internal/control/handler_spawn_orchestrator.go diff --git a/internal/control/handler_create_worktree.go b/internal/control/handler_create_worktree.go deleted file mode 100644 index 5226abe..0000000 --- a/internal/control/handler_create_worktree.go +++ /dev/null @@ -1,134 +0,0 @@ -package control - -import ( - "context" - "fmt" - "io" - "os" - "path/filepath" - "time" - - "github.com/newhook/co/internal/db" - "github.com/newhook/co/internal/git" - "github.com/newhook/co/internal/logging" - "github.com/newhook/co/internal/mise" - "github.com/newhook/co/internal/project" - "github.com/newhook/co/internal/worktree" -) - -// HandleCreateWorktreeTask handles a scheduled worktree creation task -func HandleCreateWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - branchName := task.Metadata["branch"] - baseBranch := task.Metadata["base_branch"] - workerName := task.Metadata["worker_name"] - useExisting := task.Metadata["use_existing"] == "true" - - if baseBranch == "" { - baseBranch = proj.Config.Repo.GetBaseBranch() - } - - logging.Info("Creating worktree for work", - "work_id", workID, - "branch", branchName, - "base_branch", baseBranch, - "use_existing", useExisting, - "attempt", task.AttemptCount+1) - - // Get work details - work, err := proj.DB.GetWork(ctx, workID) - if err != nil { - return fmt.Errorf("failed to get work: %w", err) - } - if work == nil { - // Work was deleted - nothing to do - logging.Info("Work not found, task will be marked completed", "work_id", workID) - return nil - } - - mainRepoPath := proj.MainRepoPath() - gitOps := git.NewOperations() - wtOps := worktree.NewOperations() - var branchExistsOnRemote bool - - // For existing branches, check if branch exists on remote - if useExisting { - _, branchExistsOnRemote, _ = gitOps.ValidateExistingBranch(ctx, mainRepoPath, branchName) - } - - // If worktree path is already set and exists, skip creation - if work.WorktreePath != "" { - // Worktree already created - just need to ensure git push - logging.Info("Worktree already exists, skipping creation", "work_id", workID, "path", work.WorktreePath) - } else { - // Create the worktree - workDir := filepath.Join(proj.Root, workID) - worktreePath := filepath.Join(workDir, "tree") - - // Create work directory - if err := os.Mkdir(workDir, 0750); err != nil && !os.IsExist(err) { - return fmt.Errorf("failed to create work directory: %w", err) - } - - if useExisting { - // For existing branches, fetch the branch first - if err := gitOps.FetchBranch(ctx, mainRepoPath, branchName); err != nil { - // Ignore fetch errors - branch might only exist locally - logging.Debug("Could not fetch branch from origin (may only exist locally)", "branch", branchName) - } - - // Create worktree from existing branch - if err := wtOps.CreateFromExisting(ctx, mainRepoPath, worktreePath, branchName); err != nil { - _ = os.RemoveAll(workDir) - return fmt.Errorf("failed to create worktree from existing branch: %w", err) - } - } else { - // Fetch latest from origin for the base branch - if err := gitOps.FetchBranch(ctx, mainRepoPath, baseBranch); err != nil { - _ = os.RemoveAll(workDir) - return fmt.Errorf("failed to fetch base branch: %w", err) - } - - // Create git worktree with new branch based on origin/ - if err := wtOps.Create(ctx, mainRepoPath, worktreePath, branchName, "origin/"+baseBranch); err != nil { - _ = os.RemoveAll(workDir) - return fmt.Errorf("failed to create worktree: %w", err) - } - } - - // Initialize mise if configured - if err := mise.InitializeWithOutput(worktreePath, io.Discard); err != nil { - logging.Warn("mise initialization failed", "error", err) - // Non-fatal, continue - } - - // Update work with worktree path - if err := proj.DB.UpdateWorkWorktreePath(ctx, workID, worktreePath); err != nil { - return fmt.Errorf("failed to update work worktree path: %w", err) - } - } - - // Attempt git push (skip for existing branches that already exist on remote) - work, _ = proj.DB.GetWork(ctx, workID) // Refresh work - if work != nil && work.WorktreePath != "" { - if useExisting && branchExistsOnRemote { - logging.Info("Skipping git push - branch already exists on remote", "branch", branchName) - } else { - if err := gitOps.PushSetUpstream(ctx, branchName, work.WorktreePath); err != nil { - return fmt.Errorf("git push failed: %w", err) - } - } - } - - logging.Info("Worktree created and pushed successfully", "work_id", workID) - - // Schedule orchestrator spawn task - _, err = proj.DB.ScheduleTask(ctx, workID, db.TaskTypeSpawnOrchestrator, time.Now(), map[string]string{ - "worker_name": workerName, - }) - if err != nil { - logging.Warn("failed to schedule orchestrator spawn", "error", err, "work_id", workID) - } - - return nil -} diff --git a/internal/control/handler_destroy_worktree.go b/internal/control/handler_destroy_worktree.go deleted file mode 100644 index fa045a3..0000000 --- a/internal/control/handler_destroy_worktree.go +++ /dev/null @@ -1,40 +0,0 @@ -package control - -import ( - "context" - "io" - - "github.com/newhook/co/internal/db" - "github.com/newhook/co/internal/logging" - "github.com/newhook/co/internal/project" - "github.com/newhook/co/internal/work" -) - -// HandleDestroyWorktreeTask handles a scheduled worktree destruction task -func HandleDestroyWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - - logging.Info("Destroying worktree for work", - "work_id", workID, - "attempt", task.AttemptCount+1) - - // Check if work still exists - workRecord, err := proj.DB.GetWork(ctx, workID) - if err != nil { - return err - } - if workRecord == nil { - // Work was already deleted - nothing to do - logging.Info("Work not found, task will be marked completed", "work_id", workID) - return nil - } - - // Delegate to the shared DestroyWork function - if err := work.DestroyWork(ctx, proj, workID, io.Discard); err != nil { - return err - } - - logging.Info("Worktree destroyed successfully", "work_id", workID) - - return nil -} diff --git a/internal/control/handler_git_push.go b/internal/control/handler_git_push.go deleted file mode 100644 index 1d18279..0000000 --- a/internal/control/handler_git_push.go +++ /dev/null @@ -1,44 +0,0 @@ -package control - -import ( - "context" - "fmt" - - "github.com/newhook/co/internal/db" - "github.com/newhook/co/internal/git" - "github.com/newhook/co/internal/logging" - "github.com/newhook/co/internal/project" -) - -// HandleGitPushTask handles a scheduled git push task with retry support. -// Returns nil on success, error on failure (caller handles retry/completion). -func HandleGitPushTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - // Get branch and directory from metadata - branch := task.Metadata["branch"] - dir := task.Metadata["dir"] - - if branch == "" { - // Try to get from work - work, err := proj.DB.GetWork(ctx, workID) - if err != nil || work == nil { - return fmt.Errorf("failed to get work for git push: work not found") - } - branch = work.BranchName - dir = work.WorktreePath - } - - if branch == "" || dir == "" { - return fmt.Errorf("git push task missing branch or dir metadata") - } - - logging.Info("Executing git push", "branch", branch, "dir", dir, "attempt", task.AttemptCount+1) - - if err := git.NewOperations().PushSetUpstream(ctx, branch, dir); err != nil { - return err - } - - logging.Info("Git push succeeded", "branch", branch, "work_id", workID) - - return nil -} diff --git a/internal/control/handler_pr_feedback.go b/internal/control/handler_pr_feedback.go deleted file mode 100644 index 7284597..0000000 --- a/internal/control/handler_pr_feedback.go +++ /dev/null @@ -1,53 +0,0 @@ -package control - -import ( - "context" - "fmt" - "time" - - "github.com/newhook/co/internal/db" - "github.com/newhook/co/internal/feedback" - "github.com/newhook/co/internal/logging" - "github.com/newhook/co/internal/project" -) - -// HandlePRFeedbackTask handles a scheduled PR feedback check. -// Returns nil on success, error on failure (caller handles retry/completion). -func HandlePRFeedbackTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - logging.Debug("Starting PR feedback check task", "task_id", task.ID, "work_id", workID) - - // Get work details - work, err := proj.DB.GetWork(ctx, workID) - if err != nil || work == nil || work.PRURL == "" { - logging.Debug("No PR URL for work, not rescheduling", "work_id", workID, "has_pr", work != nil && work.PRURL != "") - // Don't reschedule - scheduling happens when PR is created - return nil - } - - logging.Debug("Checking PR feedback", "pr_url", work.PRURL, "work_id", workID) - - // Process PR feedback - creates beads but doesn't add them to work - createdCount, err := feedback.ProcessPRFeedbackQuiet(ctx, proj, proj.DB, workID) - if err != nil { - return fmt.Errorf("failed to check PR feedback: %w", err) - } - - if createdCount > 0 { - logging.Info("Created beads from PR feedback", "count", createdCount, "work_id", workID) - } else { - logging.Debug("No new PR feedback found", "work_id", workID) - } - - // Schedule next check using configured interval - nextInterval := proj.Config.Scheduler.GetPRFeedbackInterval() - nextCheck := time.Now().Add(nextInterval) - _, err = proj.DB.ScheduleOrUpdateTask(ctx, workID, db.TaskTypePRFeedback, nextCheck) - if err != nil { - logging.Warn("failed to schedule next PR feedback check", "error", err, "work_id", workID) - } else { - logging.Info("Scheduled next PR feedback check", "work_id", workID, "next_check", nextCheck.Format(time.RFC3339), "interval", nextInterval) - } - - return nil -} diff --git a/internal/control/handler_spawn_orchestrator.go b/internal/control/handler_spawn_orchestrator.go deleted file mode 100644 index 5fb649a..0000000 --- a/internal/control/handler_spawn_orchestrator.go +++ /dev/null @@ -1,46 +0,0 @@ -package control - -import ( - "context" - "fmt" - "io" - - "github.com/newhook/co/internal/claude" - "github.com/newhook/co/internal/db" - "github.com/newhook/co/internal/logging" - "github.com/newhook/co/internal/project" -) - -// HandleSpawnOrchestratorTask handles a scheduled orchestrator spawn task -func HandleSpawnOrchestratorTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - workerName := task.Metadata["worker_name"] - - logging.Info("Spawning orchestrator for work", - "work_id", workID, - "attempt", task.AttemptCount+1) - - // Get work details - work, err := proj.DB.GetWork(ctx, workID) - if err != nil { - return fmt.Errorf("failed to get work: %w", err) - } - if work == nil { - // Work was deleted - nothing to do - logging.Info("Work not found, task will be marked completed", "work_id", workID) - return nil - } - - if work.WorktreePath == "" { - return fmt.Errorf("work %s has no worktree path", workID) - } - - // Spawn the orchestrator - if err := claude.SpawnWorkOrchestrator(ctx, workID, proj.Config.Project.Name, work.WorktreePath, workerName, io.Discard); err != nil { - return fmt.Errorf("failed to spawn orchestrator: %w", err) - } - - logging.Info("Orchestrator spawned successfully", "work_id", workID) - - return nil -} From 2a5d14e24aa4f3235f01ad7697faa1c79dc3110c Mon Sep 17 00:00:00 2001 From: Matthew Newhook Date: Thu, 29 Jan 2026 23:48:40 -0500 Subject: [PATCH 3/4] Restore handler files as methods on ControlPlane Keep handlers in their original files to minimize PR diff churn. The handlers are now methods on ControlPlane using injected dependencies instead of standalone functions creating instances. Also make processPRFeedbackQuiet internal since it's only used within the feedback package. Co-Authored-By: Claude Opus 4.5 --- internal/control/handler_create_worktree.go | 130 ++++++++ internal/control/handler_destroy_worktree.go | 39 +++ internal/control/handler_git_push.go | 42 +++ internal/control/handler_pr_feedback.go | 51 +++ .../control/handler_spawn_orchestrator.go | 45 +++ internal/control/plane.go | 298 ++---------------- internal/feedback/feedback.go | 4 +- internal/feedback/interface.go | 2 +- 8 files changed, 331 insertions(+), 280 deletions(-) create mode 100644 internal/control/handler_create_worktree.go create mode 100644 internal/control/handler_destroy_worktree.go create mode 100644 internal/control/handler_git_push.go create mode 100644 internal/control/handler_pr_feedback.go create mode 100644 internal/control/handler_spawn_orchestrator.go diff --git a/internal/control/handler_create_worktree.go b/internal/control/handler_create_worktree.go new file mode 100644 index 0000000..03acd52 --- /dev/null +++ b/internal/control/handler_create_worktree.go @@ -0,0 +1,130 @@ +package control + +import ( + "context" + "fmt" + "io" + "os" + "path/filepath" + "time" + + "github.com/newhook/co/internal/db" + "github.com/newhook/co/internal/logging" + "github.com/newhook/co/internal/project" +) + +// HandleCreateWorktreeTask handles a scheduled worktree creation task. +func (cp *ControlPlane) HandleCreateWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + branchName := task.Metadata["branch"] + baseBranch := task.Metadata["base_branch"] + workerName := task.Metadata["worker_name"] + useExisting := task.Metadata["use_existing"] == "true" + + if baseBranch == "" { + baseBranch = proj.Config.Repo.GetBaseBranch() + } + + logging.Info("Creating worktree for work", + "work_id", workID, + "branch", branchName, + "base_branch", baseBranch, + "use_existing", useExisting, + "attempt", task.AttemptCount+1) + + // Get work details + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil { + return fmt.Errorf("failed to get work: %w", err) + } + if workRecord == nil { + // Work was deleted - nothing to do + logging.Info("Work not found, task will be marked completed", "work_id", workID) + return nil + } + + mainRepoPath := proj.MainRepoPath() + var branchExistsOnRemote bool + + // For existing branches, check if branch exists on remote + if useExisting { + _, branchExistsOnRemote, _ = cp.Git.ValidateExistingBranch(ctx, mainRepoPath, branchName) + } + + // If worktree path is already set and exists, skip creation + if workRecord.WorktreePath != "" { + // Worktree already created - just need to ensure git push + logging.Info("Worktree already exists, skipping creation", "work_id", workID, "path", workRecord.WorktreePath) + } else { + // Create the worktree + workDir := filepath.Join(proj.Root, workID) + worktreePath := filepath.Join(workDir, "tree") + + // Create work directory + if err := os.Mkdir(workDir, 0750); err != nil && !os.IsExist(err) { + return fmt.Errorf("failed to create work directory: %w", err) + } + + if useExisting { + // For existing branches, fetch the branch first + if err := cp.Git.FetchBranch(ctx, mainRepoPath, branchName); err != nil { + // Ignore fetch errors - branch might only exist locally + logging.Debug("Could not fetch branch from origin (may only exist locally)", "branch", branchName) + } + + // Create worktree from existing branch + if err := cp.Worktree.CreateFromExisting(ctx, mainRepoPath, worktreePath, branchName); err != nil { + _ = os.RemoveAll(workDir) + return fmt.Errorf("failed to create worktree from existing branch: %w", err) + } + } else { + // Fetch latest from origin for the base branch + if err := cp.Git.FetchBranch(ctx, mainRepoPath, baseBranch); err != nil { + _ = os.RemoveAll(workDir) + return fmt.Errorf("failed to fetch base branch: %w", err) + } + + // Create git worktree with new branch based on origin/ + if err := cp.Worktree.Create(ctx, mainRepoPath, worktreePath, branchName, "origin/"+baseBranch); err != nil { + _ = os.RemoveAll(workDir) + return fmt.Errorf("failed to create worktree: %w", err) + } + } + + // Initialize mise if configured + miseOps := cp.Mise(worktreePath) + if err := miseOps.InitializeWithOutput(io.Discard); err != nil { + logging.Warn("mise initialization failed", "error", err) + // Non-fatal, continue + } + + // Update work with worktree path + if err := proj.DB.UpdateWorkWorktreePath(ctx, workID, worktreePath); err != nil { + return fmt.Errorf("failed to update work worktree path: %w", err) + } + } + + // Attempt git push (skip for existing branches that already exist on remote) + workRecord, _ = proj.DB.GetWork(ctx, workID) // Refresh work + if workRecord != nil && workRecord.WorktreePath != "" { + if useExisting && branchExistsOnRemote { + logging.Info("Skipping git push - branch already exists on remote", "branch", branchName) + } else { + if err := cp.Git.PushSetUpstream(ctx, branchName, workRecord.WorktreePath); err != nil { + return fmt.Errorf("git push failed: %w", err) + } + } + } + + logging.Info("Worktree created and pushed successfully", "work_id", workID) + + // Schedule orchestrator spawn task + _, err = proj.DB.ScheduleTask(ctx, workID, db.TaskTypeSpawnOrchestrator, time.Now(), map[string]string{ + "worker_name": workerName, + }) + if err != nil { + logging.Warn("failed to schedule orchestrator spawn", "error", err, "work_id", workID) + } + + return nil +} diff --git a/internal/control/handler_destroy_worktree.go b/internal/control/handler_destroy_worktree.go new file mode 100644 index 0000000..ea6b0cf --- /dev/null +++ b/internal/control/handler_destroy_worktree.go @@ -0,0 +1,39 @@ +package control + +import ( + "context" + "io" + + "github.com/newhook/co/internal/db" + "github.com/newhook/co/internal/logging" + "github.com/newhook/co/internal/project" +) + +// HandleDestroyWorktreeTask handles a scheduled worktree destruction task. +func (cp *ControlPlane) HandleDestroyWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + + logging.Info("Destroying worktree for work", + "work_id", workID, + "attempt", task.AttemptCount+1) + + // Check if work still exists + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil { + return err + } + if workRecord == nil { + // Work was already deleted - nothing to do + logging.Info("Work not found, task will be marked completed", "work_id", workID) + return nil + } + + // Delegate to the work destroyer + if err := cp.WorkDestroyer.DestroyWork(ctx, proj, workID, io.Discard); err != nil { + return err + } + + logging.Info("Worktree destroyed successfully", "work_id", workID) + + return nil +} diff --git a/internal/control/handler_git_push.go b/internal/control/handler_git_push.go new file mode 100644 index 0000000..872089d --- /dev/null +++ b/internal/control/handler_git_push.go @@ -0,0 +1,42 @@ +package control + +import ( + "context" + "fmt" + + "github.com/newhook/co/internal/db" + "github.com/newhook/co/internal/logging" + "github.com/newhook/co/internal/project" +) + +// HandleGitPushTask handles a scheduled git push task with retry support. +func (cp *ControlPlane) HandleGitPushTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + // Get branch and directory from metadata + branch := task.Metadata["branch"] + dir := task.Metadata["dir"] + + if branch == "" { + // Try to get from work + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil || workRecord == nil { + return fmt.Errorf("failed to get work for git push: work not found") + } + branch = workRecord.BranchName + dir = workRecord.WorktreePath + } + + if branch == "" || dir == "" { + return fmt.Errorf("git push task missing branch or dir metadata") + } + + logging.Info("Executing git push", "branch", branch, "dir", dir, "attempt", task.AttemptCount+1) + + if err := cp.Git.PushSetUpstream(ctx, branch, dir); err != nil { + return err + } + + logging.Info("Git push succeeded", "branch", branch, "work_id", workID) + + return nil +} diff --git a/internal/control/handler_pr_feedback.go b/internal/control/handler_pr_feedback.go new file mode 100644 index 0000000..50a2b11 --- /dev/null +++ b/internal/control/handler_pr_feedback.go @@ -0,0 +1,51 @@ +package control + +import ( + "context" + "fmt" + "time" + + "github.com/newhook/co/internal/db" + "github.com/newhook/co/internal/logging" + "github.com/newhook/co/internal/project" +) + +// HandlePRFeedbackTask handles a scheduled PR feedback check. +func (cp *ControlPlane) HandlePRFeedbackTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + logging.Debug("Starting PR feedback check task", "task_id", task.ID, "work_id", workID) + + // Get work details + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil || workRecord == nil || workRecord.PRURL == "" { + logging.Debug("No PR URL for work, not rescheduling", "work_id", workID, "has_pr", workRecord != nil && workRecord.PRURL != "") + // Don't reschedule - scheduling happens when PR is created + return nil + } + + logging.Debug("Checking PR feedback", "pr_url", workRecord.PRURL, "work_id", workID) + + // Process PR feedback - creates beads but doesn't add them to work + createdCount, err := cp.FeedbackProcessor.ProcessPRFeedback(ctx, proj, proj.DB, workID) + if err != nil { + return fmt.Errorf("failed to check PR feedback: %w", err) + } + + if createdCount > 0 { + logging.Info("Created beads from PR feedback", "count", createdCount, "work_id", workID) + } else { + logging.Debug("No new PR feedback found", "work_id", workID) + } + + // Schedule next check using configured interval + nextInterval := proj.Config.Scheduler.GetPRFeedbackInterval() + nextCheck := time.Now().Add(nextInterval) + _, err = proj.DB.ScheduleOrUpdateTask(ctx, workID, db.TaskTypePRFeedback, nextCheck) + if err != nil { + logging.Warn("failed to schedule next PR feedback check", "error", err, "work_id", workID) + } else { + logging.Info("Scheduled next PR feedback check", "work_id", workID, "next_check", nextCheck.Format(time.RFC3339), "interval", nextInterval) + } + + return nil +} diff --git a/internal/control/handler_spawn_orchestrator.go b/internal/control/handler_spawn_orchestrator.go new file mode 100644 index 0000000..dcd7e9b --- /dev/null +++ b/internal/control/handler_spawn_orchestrator.go @@ -0,0 +1,45 @@ +package control + +import ( + "context" + "fmt" + "io" + + "github.com/newhook/co/internal/db" + "github.com/newhook/co/internal/logging" + "github.com/newhook/co/internal/project" +) + +// HandleSpawnOrchestratorTask handles a scheduled orchestrator spawn task. +func (cp *ControlPlane) HandleSpawnOrchestratorTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { + workID := task.WorkID + workerName := task.Metadata["worker_name"] + + logging.Info("Spawning orchestrator for work", + "work_id", workID, + "attempt", task.AttemptCount+1) + + // Get work details + workRecord, err := proj.DB.GetWork(ctx, workID) + if err != nil { + return fmt.Errorf("failed to get work: %w", err) + } + if workRecord == nil { + // Work was deleted - nothing to do + logging.Info("Work not found, task will be marked completed", "work_id", workID) + return nil + } + + if workRecord.WorktreePath == "" { + return fmt.Errorf("work %s has no worktree path", workID) + } + + // Spawn the orchestrator + if err := cp.OrchestratorSpawner.SpawnWorkOrchestrator(ctx, workID, proj.Config.Project.Name, workRecord.WorktreePath, workerName, io.Discard); err != nil { + return fmt.Errorf("failed to spawn orchestrator: %w", err) + } + + logging.Info("Orchestrator spawned successfully", "work_id", workID) + + return nil +} diff --git a/internal/control/plane.go b/internal/control/plane.go index a05e0cd..aaba8c3 100644 --- a/internal/control/plane.go +++ b/internal/control/plane.go @@ -2,17 +2,12 @@ package control import ( "context" - "fmt" "io" - "os" - "path/filepath" - "time" "github.com/newhook/co/internal/claude" "github.com/newhook/co/internal/db" "github.com/newhook/co/internal/feedback" "github.com/newhook/co/internal/git" - "github.com/newhook/co/internal/logging" "github.com/newhook/co/internal/mise" "github.com/newhook/co/internal/project" "github.com/newhook/co/internal/work" @@ -51,25 +46,25 @@ func (d *DefaultWorkDestroyer) DestroyWork(ctx context.Context, proj *project.Pr // ControlPlane manages the execution of scheduled tasks with injectable dependencies. // It allows for testing without actual CLI tools, services, or file system operations. type ControlPlane struct { - Git git.Operations - Worktree worktree.Operations - Zellij zellij.SessionManager - Mise func(dir string) mise.Operations - FeedbackProcessor feedback.Processor - OrchestratorSpawner OrchestratorSpawner - WorkDestroyer WorkDestroyer + Git git.Operations + Worktree worktree.Operations + Zellij zellij.SessionManager + Mise func(dir string) mise.Operations + FeedbackProcessor feedback.Processor + OrchestratorSpawner OrchestratorSpawner + WorkDestroyer WorkDestroyer } // NewControlPlane creates a new ControlPlane with default production dependencies. func NewControlPlane() *ControlPlane { return &ControlPlane{ - Git: git.NewOperations(), - Worktree: worktree.NewOperations(), - Zellij: zellij.New(), - Mise: mise.NewOperations, - FeedbackProcessor: feedback.NewProcessor(), - OrchestratorSpawner: &DefaultOrchestratorSpawner{}, - WorkDestroyer: &DefaultWorkDestroyer{}, + Git: git.NewOperations(), + Worktree: worktree.NewOperations(), + Zellij: zellij.New(), + Mise: mise.NewOperations, + FeedbackProcessor: feedback.NewProcessor(), + OrchestratorSpawner: &DefaultOrchestratorSpawner{}, + WorkDestroyer: &DefaultWorkDestroyer{}, } } @@ -84,267 +79,16 @@ func NewControlPlaneWithDeps( workDestroyer WorkDestroyer, ) *ControlPlane { return &ControlPlane{ - Git: gitOps, - Worktree: wtOps, - Zellij: zellijMgr, - Mise: miseOps, - FeedbackProcessor: feedbackProc, - OrchestratorSpawner: orchestratorSpawner, - WorkDestroyer: workDestroyer, + Git: gitOps, + Worktree: wtOps, + Zellij: zellijMgr, + Mise: miseOps, + FeedbackProcessor: feedbackProc, + OrchestratorSpawner: orchestratorSpawner, + WorkDestroyer: workDestroyer, } } -// HandleCreateWorktreeTask handles a scheduled worktree creation task. -func (cp *ControlPlane) HandleCreateWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - branchName := task.Metadata["branch"] - baseBranch := task.Metadata["base_branch"] - workerName := task.Metadata["worker_name"] - useExisting := task.Metadata["use_existing"] == "true" - - if baseBranch == "" { - baseBranch = proj.Config.Repo.GetBaseBranch() - } - - logging.Info("Creating worktree for work", - "work_id", workID, - "branch", branchName, - "base_branch", baseBranch, - "use_existing", useExisting, - "attempt", task.AttemptCount+1) - - // Get work details - workRecord, err := proj.DB.GetWork(ctx, workID) - if err != nil { - return fmt.Errorf("failed to get work: %w", err) - } - if workRecord == nil { - // Work was deleted - nothing to do - logging.Info("Work not found, task will be marked completed", "work_id", workID) - return nil - } - - mainRepoPath := proj.MainRepoPath() - var branchExistsOnRemote bool - - // For existing branches, check if branch exists on remote - if useExisting { - _, branchExistsOnRemote, _ = cp.Git.ValidateExistingBranch(ctx, mainRepoPath, branchName) - } - - // If worktree path is already set and exists, skip creation - if workRecord.WorktreePath != "" { - // Worktree already created - just need to ensure git push - logging.Info("Worktree already exists, skipping creation", "work_id", workID, "path", workRecord.WorktreePath) - } else { - // Create the worktree - workDir := filepath.Join(proj.Root, workID) - worktreePath := filepath.Join(workDir, "tree") - - // Create work directory - if err := os.Mkdir(workDir, 0750); err != nil && !os.IsExist(err) { - return fmt.Errorf("failed to create work directory: %w", err) - } - - if useExisting { - // For existing branches, fetch the branch first - if err := cp.Git.FetchBranch(ctx, mainRepoPath, branchName); err != nil { - // Ignore fetch errors - branch might only exist locally - logging.Debug("Could not fetch branch from origin (may only exist locally)", "branch", branchName) - } - - // Create worktree from existing branch - if err := cp.Worktree.CreateFromExisting(ctx, mainRepoPath, worktreePath, branchName); err != nil { - _ = os.RemoveAll(workDir) - return fmt.Errorf("failed to create worktree from existing branch: %w", err) - } - } else { - // Fetch latest from origin for the base branch - if err := cp.Git.FetchBranch(ctx, mainRepoPath, baseBranch); err != nil { - _ = os.RemoveAll(workDir) - return fmt.Errorf("failed to fetch base branch: %w", err) - } - - // Create git worktree with new branch based on origin/ - if err := cp.Worktree.Create(ctx, mainRepoPath, worktreePath, branchName, "origin/"+baseBranch); err != nil { - _ = os.RemoveAll(workDir) - return fmt.Errorf("failed to create worktree: %w", err) - } - } - - // Initialize mise if configured - miseOps := cp.Mise(worktreePath) - if err := miseOps.InitializeWithOutput(io.Discard); err != nil { - logging.Warn("mise initialization failed", "error", err) - // Non-fatal, continue - } - - // Update work with worktree path - if err := proj.DB.UpdateWorkWorktreePath(ctx, workID, worktreePath); err != nil { - return fmt.Errorf("failed to update work worktree path: %w", err) - } - } - - // Attempt git push (skip for existing branches that already exist on remote) - workRecord, _ = proj.DB.GetWork(ctx, workID) // Refresh work - if workRecord != nil && workRecord.WorktreePath != "" { - if useExisting && branchExistsOnRemote { - logging.Info("Skipping git push - branch already exists on remote", "branch", branchName) - } else { - if err := cp.Git.PushSetUpstream(ctx, branchName, workRecord.WorktreePath); err != nil { - return fmt.Errorf("git push failed: %w", err) - } - } - } - - logging.Info("Worktree created and pushed successfully", "work_id", workID) - - // Schedule orchestrator spawn task - _, err = proj.DB.ScheduleTask(ctx, workID, db.TaskTypeSpawnOrchestrator, time.Now(), map[string]string{ - "worker_name": workerName, - }) - if err != nil { - logging.Warn("failed to schedule orchestrator spawn", "error", err, "work_id", workID) - } - - return nil -} - -// HandleSpawnOrchestratorTask handles a scheduled orchestrator spawn task. -func (cp *ControlPlane) HandleSpawnOrchestratorTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - workerName := task.Metadata["worker_name"] - - logging.Info("Spawning orchestrator for work", - "work_id", workID, - "attempt", task.AttemptCount+1) - - // Get work details - workRecord, err := proj.DB.GetWork(ctx, workID) - if err != nil { - return fmt.Errorf("failed to get work: %w", err) - } - if workRecord == nil { - // Work was deleted - nothing to do - logging.Info("Work not found, task will be marked completed", "work_id", workID) - return nil - } - - if workRecord.WorktreePath == "" { - return fmt.Errorf("work %s has no worktree path", workID) - } - - // Spawn the orchestrator - if err := cp.OrchestratorSpawner.SpawnWorkOrchestrator(ctx, workID, proj.Config.Project.Name, workRecord.WorktreePath, workerName, io.Discard); err != nil { - return fmt.Errorf("failed to spawn orchestrator: %w", err) - } - - logging.Info("Orchestrator spawned successfully", "work_id", workID) - - return nil -} - -// HandleDestroyWorktreeTask handles a scheduled worktree destruction task. -func (cp *ControlPlane) HandleDestroyWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - - logging.Info("Destroying worktree for work", - "work_id", workID, - "attempt", task.AttemptCount+1) - - // Check if work still exists - workRecord, err := proj.DB.GetWork(ctx, workID) - if err != nil { - return err - } - if workRecord == nil { - // Work was already deleted - nothing to do - logging.Info("Work not found, task will be marked completed", "work_id", workID) - return nil - } - - // Delegate to the work destroyer - if err := cp.WorkDestroyer.DestroyWork(ctx, proj, workID, io.Discard); err != nil { - return err - } - - logging.Info("Worktree destroyed successfully", "work_id", workID) - - return nil -} - -// HandleGitPushTask handles a scheduled git push task with retry support. -func (cp *ControlPlane) HandleGitPushTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - // Get branch and directory from metadata - branch := task.Metadata["branch"] - dir := task.Metadata["dir"] - - if branch == "" { - // Try to get from work - workRecord, err := proj.DB.GetWork(ctx, workID) - if err != nil || workRecord == nil { - return fmt.Errorf("failed to get work for git push: work not found") - } - branch = workRecord.BranchName - dir = workRecord.WorktreePath - } - - if branch == "" || dir == "" { - return fmt.Errorf("git push task missing branch or dir metadata") - } - - logging.Info("Executing git push", "branch", branch, "dir", dir, "attempt", task.AttemptCount+1) - - if err := cp.Git.PushSetUpstream(ctx, branch, dir); err != nil { - return err - } - - logging.Info("Git push succeeded", "branch", branch, "work_id", workID) - - return nil -} - -// HandlePRFeedbackTask handles a scheduled PR feedback check. -func (cp *ControlPlane) HandlePRFeedbackTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { - workID := task.WorkID - logging.Debug("Starting PR feedback check task", "task_id", task.ID, "work_id", workID) - - // Get work details - workRecord, err := proj.DB.GetWork(ctx, workID) - if err != nil || workRecord == nil || workRecord.PRURL == "" { - logging.Debug("No PR URL for work, not rescheduling", "work_id", workID, "has_pr", workRecord != nil && workRecord.PRURL != "") - // Don't reschedule - scheduling happens when PR is created - return nil - } - - logging.Debug("Checking PR feedback", "pr_url", workRecord.PRURL, "work_id", workID) - - // Process PR feedback - creates beads but doesn't add them to work - createdCount, err := cp.FeedbackProcessor.ProcessPRFeedback(ctx, proj, proj.DB, workID) - if err != nil { - return fmt.Errorf("failed to check PR feedback: %w", err) - } - - if createdCount > 0 { - logging.Info("Created beads from PR feedback", "count", createdCount, "work_id", workID) - } else { - logging.Debug("No new PR feedback found", "work_id", workID) - } - - // Schedule next check using configured interval - nextInterval := proj.Config.Scheduler.GetPRFeedbackInterval() - nextCheck := time.Now().Add(nextInterval) - _, err = proj.DB.ScheduleOrUpdateTask(ctx, workID, db.TaskTypePRFeedback, nextCheck) - if err != nil { - logging.Warn("failed to schedule next PR feedback check", "error", err, "work_id", workID) - } else { - logging.Info("Scheduled next PR feedback check", "work_id", workID, "next_check", nextCheck.Format(time.RFC3339), "interval", nextInterval) - } - - return nil -} - // GetTaskHandlers returns the task handler map for the control plane. func (cp *ControlPlane) GetTaskHandlers() map[string]TaskHandler { return map[string]TaskHandler{ diff --git a/internal/feedback/feedback.go b/internal/feedback/feedback.go index 34a3c25..e4c772b 100644 --- a/internal/feedback/feedback.go +++ b/internal/feedback/feedback.go @@ -11,10 +11,10 @@ import ( "github.com/newhook/co/internal/project" ) -// ProcessPRFeedbackQuiet processes PR feedback without outputting to stdout. +// processPRFeedbackQuiet processes PR feedback without outputting to stdout. // This is used by the scheduler to avoid interfering with the TUI. // Returns the number of beads created and any error. -func ProcessPRFeedbackQuiet(ctx context.Context, proj *project.Project, database *db.DB, workID string) (int, error) { +func processPRFeedbackQuiet(ctx context.Context, proj *project.Project, database *db.DB, workID string) (int, error) { return processPRFeedbackInternal(ctx, proj, database, workID, true) } diff --git a/internal/feedback/interface.go b/internal/feedback/interface.go index 9909b35..d93c9c7 100644 --- a/internal/feedback/interface.go +++ b/internal/feedback/interface.go @@ -28,5 +28,5 @@ func NewProcessor() Processor { // ProcessPRFeedback implements Processor.ProcessPRFeedback. func (p *DefaultProcessor) ProcessPRFeedback(ctx context.Context, proj *project.Project, database *db.DB, workID string) (int, error) { - return ProcessPRFeedbackQuiet(ctx, proj, database, workID) + return processPRFeedbackQuiet(ctx, proj, database, workID) } From 3064df9f2b5d7135499f6486c41ffdb45cc79faf Mon Sep 17 00:00:00 2001 From: Matthew Newhook Date: Thu, 29 Jan 2026 23:51:17 -0500 Subject: [PATCH 4/4] Minimize diff churn in handler files Revert unnecessary variable renames and comment changes to keep the diff focused on the actual interface extraction changes. Co-Authored-By: Claude Opus 4.5 --- internal/control/handler_create_worktree.go | 16 ++++++++-------- internal/control/handler_destroy_worktree.go | 4 ++-- internal/control/handler_git_push.go | 9 +++++---- internal/control/handler_pr_feedback.go | 9 +++++---- internal/control/handler_spawn_orchestrator.go | 10 +++++----- 5 files changed, 25 insertions(+), 23 deletions(-) diff --git a/internal/control/handler_create_worktree.go b/internal/control/handler_create_worktree.go index 03acd52..2ed39e2 100644 --- a/internal/control/handler_create_worktree.go +++ b/internal/control/handler_create_worktree.go @@ -13,7 +13,7 @@ import ( "github.com/newhook/co/internal/project" ) -// HandleCreateWorktreeTask handles a scheduled worktree creation task. +// HandleCreateWorktreeTask handles a scheduled worktree creation task func (cp *ControlPlane) HandleCreateWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { workID := task.WorkID branchName := task.Metadata["branch"] @@ -33,11 +33,11 @@ func (cp *ControlPlane) HandleCreateWorktreeTask(ctx context.Context, proj *proj "attempt", task.AttemptCount+1) // Get work details - workRecord, err := proj.DB.GetWork(ctx, workID) + work, err := proj.DB.GetWork(ctx, workID) if err != nil { return fmt.Errorf("failed to get work: %w", err) } - if workRecord == nil { + if work == nil { // Work was deleted - nothing to do logging.Info("Work not found, task will be marked completed", "work_id", workID) return nil @@ -52,9 +52,9 @@ func (cp *ControlPlane) HandleCreateWorktreeTask(ctx context.Context, proj *proj } // If worktree path is already set and exists, skip creation - if workRecord.WorktreePath != "" { + if work.WorktreePath != "" { // Worktree already created - just need to ensure git push - logging.Info("Worktree already exists, skipping creation", "work_id", workID, "path", workRecord.WorktreePath) + logging.Info("Worktree already exists, skipping creation", "work_id", workID, "path", work.WorktreePath) } else { // Create the worktree workDir := filepath.Join(proj.Root, workID) @@ -105,12 +105,12 @@ func (cp *ControlPlane) HandleCreateWorktreeTask(ctx context.Context, proj *proj } // Attempt git push (skip for existing branches that already exist on remote) - workRecord, _ = proj.DB.GetWork(ctx, workID) // Refresh work - if workRecord != nil && workRecord.WorktreePath != "" { + work, _ = proj.DB.GetWork(ctx, workID) // Refresh work + if work != nil && work.WorktreePath != "" { if useExisting && branchExistsOnRemote { logging.Info("Skipping git push - branch already exists on remote", "branch", branchName) } else { - if err := cp.Git.PushSetUpstream(ctx, branchName, workRecord.WorktreePath); err != nil { + if err := cp.Git.PushSetUpstream(ctx, branchName, work.WorktreePath); err != nil { return fmt.Errorf("git push failed: %w", err) } } diff --git a/internal/control/handler_destroy_worktree.go b/internal/control/handler_destroy_worktree.go index ea6b0cf..6eb0501 100644 --- a/internal/control/handler_destroy_worktree.go +++ b/internal/control/handler_destroy_worktree.go @@ -9,7 +9,7 @@ import ( "github.com/newhook/co/internal/project" ) -// HandleDestroyWorktreeTask handles a scheduled worktree destruction task. +// HandleDestroyWorktreeTask handles a scheduled worktree destruction task func (cp *ControlPlane) HandleDestroyWorktreeTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { workID := task.WorkID @@ -28,7 +28,7 @@ func (cp *ControlPlane) HandleDestroyWorktreeTask(ctx context.Context, proj *pro return nil } - // Delegate to the work destroyer + // Delegate to the shared DestroyWork function if err := cp.WorkDestroyer.DestroyWork(ctx, proj, workID, io.Discard); err != nil { return err } diff --git a/internal/control/handler_git_push.go b/internal/control/handler_git_push.go index 872089d..f33cefb 100644 --- a/internal/control/handler_git_push.go +++ b/internal/control/handler_git_push.go @@ -10,6 +10,7 @@ import ( ) // HandleGitPushTask handles a scheduled git push task with retry support. +// Returns nil on success, error on failure (caller handles retry/completion). func (cp *ControlPlane) HandleGitPushTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { workID := task.WorkID // Get branch and directory from metadata @@ -18,12 +19,12 @@ func (cp *ControlPlane) HandleGitPushTask(ctx context.Context, proj *project.Pro if branch == "" { // Try to get from work - workRecord, err := proj.DB.GetWork(ctx, workID) - if err != nil || workRecord == nil { + work, err := proj.DB.GetWork(ctx, workID) + if err != nil || work == nil { return fmt.Errorf("failed to get work for git push: work not found") } - branch = workRecord.BranchName - dir = workRecord.WorktreePath + branch = work.BranchName + dir = work.WorktreePath } if branch == "" || dir == "" { diff --git a/internal/control/handler_pr_feedback.go b/internal/control/handler_pr_feedback.go index 50a2b11..c8c8376 100644 --- a/internal/control/handler_pr_feedback.go +++ b/internal/control/handler_pr_feedback.go @@ -11,19 +11,20 @@ import ( ) // HandlePRFeedbackTask handles a scheduled PR feedback check. +// Returns nil on success, error on failure (caller handles retry/completion). func (cp *ControlPlane) HandlePRFeedbackTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { workID := task.WorkID logging.Debug("Starting PR feedback check task", "task_id", task.ID, "work_id", workID) // Get work details - workRecord, err := proj.DB.GetWork(ctx, workID) - if err != nil || workRecord == nil || workRecord.PRURL == "" { - logging.Debug("No PR URL for work, not rescheduling", "work_id", workID, "has_pr", workRecord != nil && workRecord.PRURL != "") + work, err := proj.DB.GetWork(ctx, workID) + if err != nil || work == nil || work.PRURL == "" { + logging.Debug("No PR URL for work, not rescheduling", "work_id", workID, "has_pr", work != nil && work.PRURL != "") // Don't reschedule - scheduling happens when PR is created return nil } - logging.Debug("Checking PR feedback", "pr_url", workRecord.PRURL, "work_id", workID) + logging.Debug("Checking PR feedback", "pr_url", work.PRURL, "work_id", workID) // Process PR feedback - creates beads but doesn't add them to work createdCount, err := cp.FeedbackProcessor.ProcessPRFeedback(ctx, proj, proj.DB, workID) diff --git a/internal/control/handler_spawn_orchestrator.go b/internal/control/handler_spawn_orchestrator.go index dcd7e9b..bd643ab 100644 --- a/internal/control/handler_spawn_orchestrator.go +++ b/internal/control/handler_spawn_orchestrator.go @@ -10,7 +10,7 @@ import ( "github.com/newhook/co/internal/project" ) -// HandleSpawnOrchestratorTask handles a scheduled orchestrator spawn task. +// HandleSpawnOrchestratorTask handles a scheduled orchestrator spawn task func (cp *ControlPlane) HandleSpawnOrchestratorTask(ctx context.Context, proj *project.Project, task *db.ScheduledTask) error { workID := task.WorkID workerName := task.Metadata["worker_name"] @@ -20,22 +20,22 @@ func (cp *ControlPlane) HandleSpawnOrchestratorTask(ctx context.Context, proj *p "attempt", task.AttemptCount+1) // Get work details - workRecord, err := proj.DB.GetWork(ctx, workID) + work, err := proj.DB.GetWork(ctx, workID) if err != nil { return fmt.Errorf("failed to get work: %w", err) } - if workRecord == nil { + if work == nil { // Work was deleted - nothing to do logging.Info("Work not found, task will be marked completed", "work_id", workID) return nil } - if workRecord.WorktreePath == "" { + if work.WorktreePath == "" { return fmt.Errorf("work %s has no worktree path", workID) } // Spawn the orchestrator - if err := cp.OrchestratorSpawner.SpawnWorkOrchestrator(ctx, workID, proj.Config.Project.Name, workRecord.WorktreePath, workerName, io.Discard); err != nil { + if err := cp.OrchestratorSpawner.SpawnWorkOrchestrator(ctx, workID, proj.Config.Project.Name, work.WorktreePath, workerName, io.Discard); err != nil { return fmt.Errorf("failed to spawn orchestrator: %w", err) }