fix: ensure evaluator triggers when resuming multi-pass planning sessions (#571)

Fixed a critical bug where resuming an ultraplan session in multi-pass mode
would fail to trigger the plan evaluator. The issue occurred because:

1. The resume logic checked CoordinatorID (single-pass field) instead of
   PlanCoordinatorIDs (multi-pass field)
2. When GetInstance returned nil for a planner, it wasn't marked as
   processed, causing false negatives in the all-completed check

The fix:
- Added proper multi-pass handling in resumeMultiPassPlanning that checks
  existing planners, collects completed plans, and triggers the evaluator
- Created multiPassResumeDeps struct for dependency injection to enable
  comprehensive testing without real orchestrator/tmux
- Added 13 unit tests covering all code paths including edge cases

Author: Iron-Ham

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- **Multi-Pass Planning Session Resume** - Fixed a critical bug where resuming an ultraplan session in multi-pass mode (`:ultraplan --multi-pass`) would fail to trigger the plan evaluator. When the TUI was closed while the 3 parallel planners were running, re-attaching to the session would incorrectly check `CoordinatorID` (which is not used in multi-pass mode) and restart planning from scratch, overwriting `PlanCoordinatorIDs` with new instance IDs. The original planners' completion events would then be orphaned, causing the evaluator to never kick off. The fix adds proper multi-pass handling in session resume: it now correctly checks for existing planners in `PlanCoordinatorIDs`, collects any completed plans from worktrees, and triggers the evaluator when all planners have finished. Also fixed an edge case where missing planner instances (GetInstance returning nil) would cause false negatives in the all-processed check, preventing the evaluator from being triggered.
+
 ### Changed
 
 - **Instance Manager Callbacks Required at Construction** - Callbacks (OnStateChange, OnMetrics, OnTimeout, OnBell) are now passed via `ManagerCallbacks` struct in `ManagerOptions` at construction time, rather than being set separately via setter methods. This prevents the "leaky abstraction" bug where `Start()`/`Reconnect()` could be called without callbacks configured. The callback setter methods are now deprecated.

internal/cmd/session/start.go

@@ -514,13 +514,20 @@ func resumeUltraplanSession(orch *orchestrator.Orchestrator, sess *orchestrator.
 	// Resume based on current phase
 	switch ultraSession.Phase {
 	case orchestrator.PhasePlanning:
-		// Check if planning coordinator instance exists and is still running
-		if ultraSession.CoordinatorID != "" && orch.GetInstance(ultraSession.CoordinatorID) != nil {
-			fmt.Println("Planning in progress...")
+		// Handle multi-pass planning mode separately
+		if ultraSession.Config.MultiPass {
+			if err := resumeMultiPassPlanning(orch, coordinator, ultraSession, logger); err != nil {
+				return fmt.Errorf("failed to resume multi-pass planning: %w", err)
+			}
 		} else {
-			fmt.Println("Restarting planning...")
-			if err := coordinator.RunPlanning(); err != nil {
-				return fmt.Errorf("failed to restart planning: %w", err)
+			// Single-pass planning: check if planning coordinator instance exists and is still running
+			if ultraSession.CoordinatorID != "" && orch.GetInstance(ultraSession.CoordinatorID) != nil {
+				fmt.Println("Planning in progress...")
+			} else {
+				fmt.Println("Restarting planning...")
+				if err := coordinator.RunPlanning(); err != nil {
+					return fmt.Errorf("failed to restart planning: %w", err)
+				}
 			}
 		}
 
@@ -592,6 +599,214 @@ func resumeUltraplanSession(orch *orchestrator.Orchestrator, sess *orchestrator.
 	return app.Run()
 }
 
+// multiPassResumeDeps encapsulates dependencies for resumeMultiPassPlanning
+// to enable testing with mocked implementations. This follows Go's dependency
+// injection pattern for testability.
+type multiPassResumeDeps struct {
+	// getInstance returns the instance for the given ID, or nil if not found
+	getInstance func(id string) *orchestrator.Instance
+	// isTmuxRunning returns true if the tmux session for the given instance ID exists
+	isTmuxRunning func(id string) bool
+	// saveSession persists the current session state
+	saveSession func() error
+	// runPlanning starts fresh multi-pass planning
+	runPlanning func() error
+	// runPlanManager starts the plan evaluator
+	runPlanManager func() error
+	// parsePlan parses a plan from the given worktree path
+	parsePlan func(worktreePath, objective string) (*orchestrator.PlanSpec, error)
+	// logWarn logs a warning message
+	logWarn func(msg string, args ...any)
+	// logInfo logs an info message
+	logInfo func(msg string, args ...any)
+}
+
+// resumeMultiPassPlanning handles resuming multi-pass planning mode.
+// In multi-pass mode, 3 parallel planners generate candidate plans which are then
+// evaluated by a plan manager (evaluator). This function handles the case where
+// the TUI was closed while planners were running and needs to:
+// 1. Check if planners are still running
+// 2. Collect any plans from completed planners
+// 3. Trigger the evaluator if all planners have completed
+func resumeMultiPassPlanning(
+	orch *orchestrator.Orchestrator,
+	coordinator *orchestrator.Coordinator,
+	ultraSession *orchestrator.UltraPlanSession,
+	logger *logging.Logger,
+) error {
+	// Create dependencies from concrete types
+	deps := multiPassResumeDeps{
+		getInstance: orch.GetInstance,
+		isTmuxRunning: func(id string) bool {
+			mgr := orch.GetInstanceManager(id)
+			return mgr != nil && mgr.TmuxSessionExists()
+		},
+		saveSession:    orch.SaveSession,
+		runPlanning:    coordinator.RunPlanning,
+		runPlanManager: coordinator.RunPlanManager,
+		parsePlan: func(worktreePath, objective string) (*orchestrator.PlanSpec, error) {
+			planPath := orchestrator.PlanFilePath(worktreePath)
+			return orchestrator.ParsePlanFromFile(planPath, objective)
+		},
+		logWarn: logger.Warn,
+		logInfo: logger.Info,
+	}
+	return resumeMultiPassPlanningInternal(deps, ultraSession)
+}
+
+// resumeMultiPassPlanningInternal is the testable core logic for resuming
+// multi-pass planning. It accepts dependencies explicitly to enable unit testing
+// without requiring real orchestrator, coordinator, or logger instances.
+func resumeMultiPassPlanningInternal(
+	deps multiPassResumeDeps,
+	ultraSession *orchestrator.UltraPlanSession,
+) error {
+	numCoordinators := len(ultraSession.PlanCoordinatorIDs)
+
+	// No planners means we need to start fresh
+	if numCoordinators == 0 {
+		fmt.Println("No existing planners found, starting multi-pass planning...")
+		return deps.runPlanning()
+	}
+
+	// Check the status of each planner
+	var runningPlanners []string
+	var completedPlanners []int
+
+	for i, plannerID := range ultraSession.PlanCoordinatorIDs {
+		inst := deps.getInstance(plannerID)
+		if inst == nil {
+			deps.logWarn("planner instance not found in session",
+				"planner_index", i,
+				"planner_id", plannerID,
+			)
+			// Treat missing instances as completed - they're not running and
+			// we need to mark them as processed to avoid false negatives in
+			// the all-processed check
+			completedPlanners = append(completedPlanners, i)
+			continue
+		}
+
+		if deps.isTmuxRunning(plannerID) {
+			runningPlanners = append(runningPlanners, plannerID)
+			deps.logInfo("planner still running",
+				"planner_index", i,
+				"planner_id", plannerID,
+			)
+		} else {
+			completedPlanners = append(completedPlanners, i)
+			deps.logInfo("planner completed",
+				"planner_index", i,
+				"planner_id", plannerID,
+			)
+		}
+	}
+
+	// If some planners are still running, just monitor them
+	if len(runningPlanners) > 0 {
+		fmt.Printf("Multi-pass planning in progress (%d/%d planners completed)...\n",
+			len(completedPlanners), numCoordinators)
+		return nil
+	}
+
+	// All planners have completed - check if we need to collect plans and trigger evaluator
+	fmt.Printf("All %d multi-pass planners completed. Checking plan collection...\n", numCoordinators)
+
+	// Ensure CandidatePlans is properly sized
+	if len(ultraSession.CandidatePlans) < numCoordinators {
+		newPlans := make([]*orchestrator.PlanSpec, numCoordinators)
+		copy(newPlans, ultraSession.CandidatePlans)
+		ultraSession.CandidatePlans = newPlans
+	}
+
+	// Ensure ProcessedCoordinators is initialized
+	if ultraSession.ProcessedCoordinators == nil {
+		ultraSession.ProcessedCoordinators = make(map[int]bool)
+	}
+
+	// Collect plans from completed planners that we haven't processed yet
+	for _, idx := range completedPlanners {
+		// Skip if already processed
+		if ultraSession.ProcessedCoordinators[idx] {
+			continue
+		}
+
+		plannerID := ultraSession.PlanCoordinatorIDs[idx]
+		inst := deps.getInstance(plannerID)
+		if inst == nil {
+			ultraSession.ProcessedCoordinators[idx] = true
+			continue
+		}
+
+		// Try to parse plan from the planner's worktree
+		plan, err := deps.parsePlan(inst.WorktreePath, ultraSession.Objective)
+		if err != nil {
+			deps.logWarn("failed to parse plan from completed planner",
+				"planner_index", idx,
+				"planner_id", plannerID,
+				"error", err.Error(),
+			)
+			// Mark as processed even if parsing failed - we don't want to retry forever
+			ultraSession.ProcessedCoordinators[idx] = true
+			continue
+		}
+
+		// Store the plan
+		ultraSession.CandidatePlans[idx] = plan
+		ultraSession.ProcessedCoordinators[idx] = true
+		deps.logInfo("collected plan from completed planner",
+			"planner_index", idx,
+			"planner_id", plannerID,
+			"task_count", len(plan.Tasks),
+		)
+	}
+
+	// Count valid plans
+	validPlans := 0
+	for _, p := range ultraSession.CandidatePlans {
+		if p != nil {
+			validPlans++
+		}
+	}
+
+	// Check if all planners have been processed
+	processedCount := len(ultraSession.ProcessedCoordinators)
+	if processedCount < numCoordinators {
+		// Some planners haven't been processed - this shouldn't happen if all tmux sessions are gone
+		deps.logWarn("not all planners processed despite all tmux sessions being gone",
+			"processed", processedCount,
+			"total", numCoordinators,
+		)
+		fmt.Printf("Waiting for plan collection (%d/%d processed)...\n", processedCount, numCoordinators)
+		return nil
+	}
+
+	// All planners processed - check if evaluator already started or needs to be triggered
+	if ultraSession.PlanManagerID != "" {
+		fmt.Println("Plan evaluator already running...")
+		return nil
+	}
+
+	if validPlans == 0 {
+		return fmt.Errorf("all multi-pass planners completed but no valid plans were produced")
+	}
+
+	// Trigger the evaluator
+	fmt.Printf("Starting plan evaluator with %d/%d valid plans...\n", validPlans, numCoordinators)
+	if err := deps.runPlanManager(); err != nil {
+		return fmt.Errorf("failed to start plan evaluator: %w", err)
+	}
+
+	// Save session to persist the updated state
+	if err := deps.saveSession(); err != nil {
+		deps.logWarn("failed to save session after triggering evaluator",
+			"error", err.Error(),
+		)
+	}
+
+	return nil
+}
+
 // CreateLogger creates a logger if logging is enabled in config.
 // Returns a NopLogger if logging is disabled or if creation fails.
 // This function uses NewLoggerWithRotation to respect MaxSizeMB and MaxBackups config.