fix: handle planning phase crash and resume recovery (#1562) (#1844)

* fix: handle planning phase crash and resume recovery (#1562)

When spec creation crashes, the task gets stuck in "planning" state
forever because the backend never emits PLANNING_FAILED to the frontend
XState machine. Clicking Resume then also crashes because the resume
logic transitions to "coding" state, but there are no subtasks yet.

Root causes and fixes:

1. Backend orchestrator (orchestrator.py):
   - Wrap run() in try/except to emit PLANNING_FAILED on unhandled exceptions
   - Add _emit_planning_failed() calls at every early return path
   - Fix spec_dir tracking after rename_spec_dir_from_requirements()

2. XState machine (task-machine.ts):
   - Add PLANNING_STARTED transitions from error and human_review states
   - This allows tasks that crashed during planning to resume back to planning

3. Execution handlers (execution-handlers.ts):
   - Detect error state with 0 subtasks and send PLANNING_STARTED (not USER_RESUMED)
   - Check actual implementation_plan.json for subtasks instead of task.subtasks.length
   - Handles both with-actor and without-actor (app restart) code paths

Closes #1562

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address PR review findings - reliable subtask check, spec_dir rename, empty except (#1562)

- Move planHasSubtasks calculation (reads implementation_plan.json) before
  XState handling so the crash-during-planning check uses the reliable
  file-based check instead of task.subtasks.length
- Change rename_spec_dir_from_requirements to return the new Path directly
  instead of a bool, eliminating brittle directory scanning in orchestrator
- Add descriptive comment to empty except clause to satisfy code scanning

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: update tests for rename_spec_dir_from_requirements return type change (#1562)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address PR review findings for planning crash resume

- Update phase_executor.spec_dir and spec_validator after directory rename
  so subsequent phases don't use stale paths (critical bug flagged by
  sentry, coderabbitai, and Auto Claude review)
- Fix TASK_UPDATE_STATUS handler to use file-based plan check instead of
  unreliable task.subtasks.length (same #1562 bug fixed in TASK_START)
- Replace manual subtask counting with existing checkSubtasksCompletion helper
- Use safeReadFileSync instead of existsSync+readFileSync (TOCTOU fix)
- Add self.validator update to backward-compat _rename_spec_dir_from_requirements

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

apps/backend/spec/pipeline/models.py

@@ -203,19 +203,19 @@ def generate_spec_name(task_description: str) -> str:
     return "-".join(name_parts) if name_parts else "spec"
 
 
-def rename_spec_dir_from_requirements(spec_dir: Path) -> bool:
+def rename_spec_dir_from_requirements(spec_dir: Path) -> Path:
     """Rename spec directory based on requirements.json task description.
 
     Args:
         spec_dir: The current spec directory
 
     Returns:
-        Tuple of (success, new_spec_dir). If success is False, new_spec_dir is the original.
+        The new spec directory path (or the original if no rename was needed/possible).
     """
     requirements_file = spec_dir / "requirements.json"
 
     if not requirements_file.exists():
-        return False
+        return spec_dir
 
     try:
         with open(requirements_file, encoding="utf-8") as f:
@@ -223,7 +223,7 @@ def rename_spec_dir_from_requirements(spec_dir: Path) -> bool:
 
         task_desc = req.get("task_description", "")
         if not task_desc:
-            return False
+            return spec_dir
 
         # Generate new name
         new_name = generate_spec_name(task_desc)
@@ -240,11 +240,11 @@ def rename_spec_dir_from_requirements(spec_dir: Path) -> bool:
 
         # Don't rename if it's already a good name (not "pending")
         if "pending" not in current_name:
-            return True
+            return spec_dir
 
         # Don't rename if target already exists
         if new_spec_dir.exists():
-            return True
+            return spec_dir
 
         # Rename the directory
         shutil.move(str(spec_dir), str(new_spec_dir))
@@ -253,11 +253,11 @@ def rename_spec_dir_from_requirements(spec_dir: Path) -> bool:
         update_task_logger_path(new_spec_dir)
 
         print_status(f"Spec folder: {highlight(new_dir_name)}", "success")
-        return True
+        return new_spec_dir
 
     except (json.JSONDecodeError, OSError) as e:
         print_status(f"Could not rename spec folder: {e}", "warning")
-        return False
+        return spec_dir
 
 
 # Phase display configuration

apps/backend/spec/pipeline/orchestrator.py

@@ -238,6 +238,42 @@ class SpecOrchestrator:
         task_logger.start_phase(LogPhase.PLANNING, "Starting spec creation process")
         TaskEventEmitter.from_spec_dir(self.spec_dir).emit("PLANNING_STARTED")
 
+        try:
+            return await self._run_phases(interactive, auto_approve, task_logger, ui)
+        except Exception as e:
+            # Emit PLANNING_FAILED so the frontend XState machine transitions to error state
+            # instead of leaving the task stuck in "planning" forever
+            try:
+                task_emitter = TaskEventEmitter.from_spec_dir(self.spec_dir)
+                task_emitter.emit(
+                    "PLANNING_FAILED",
+                    {"error": str(e), "recoverable": True},
+                )
+            except Exception:
+                pass  # Don't mask the original error
+            try:
+                task_logger.end_phase(
+                    LogPhase.PLANNING,
+                    success=False,
+                    message=f"Spec creation crashed: {e}",
+                )
+            except Exception:
+                pass  # Best effort - don't mask the original error when logging fails
+            raise
+
+    async def _run_phases(
+        self,
+        interactive: bool,
+        auto_approve: bool,
+        task_logger,
+        ui,
+    ) -> bool:
+        """Internal method that runs all spec creation phases.
+
+        Separated from run() so that run() can wrap this in a try/except
+        to emit PLANNING_FAILED on unhandled exceptions.
+        """
+
         print(
             box(
                 f"Spec Directory: {self.spec_dir}\n"
@@ -294,6 +330,7 @@ class SpecOrchestrator:
             task_logger.end_phase(
                 LogPhase.PLANNING, success=False, message="Discovery failed"
             )
+            self._emit_planning_failed("Discovery phase failed")
             return False
         # Store summary for subsequent phases (compaction)
         await self._store_phase_summary("discovery")
@@ -310,12 +347,20 @@ class SpecOrchestrator:
                 success=False,
                 message="Requirements gathering failed",
             )
+            self._emit_planning_failed("Requirements gathering failed")
             return False
         # Store summary for subsequent phases (compaction)
         await self._store_phase_summary("requirements")
 
         # Rename spec folder with better name from requirements
-        rename_spec_dir_from_requirements(self.spec_dir)
+        # IMPORTANT: Update self.spec_dir after rename so subsequent phases use the correct path
+        new_spec_dir = rename_spec_dir_from_requirements(self.spec_dir)
+        if new_spec_dir != self.spec_dir:
+            self.spec_dir = new_spec_dir
+            self.validator = SpecValidator(self.spec_dir)
+            # Update phase executor to use the renamed directory
+            phase_executor.spec_dir = self.spec_dir
+            phase_executor.spec_validator = self.validator
 
         # Update task description from requirements
         req = requirements.load_requirements(self.spec_dir)
@@ -338,6 +383,7 @@ class SpecOrchestrator:
             task_logger.end_phase(
                 LogPhase.PLANNING, success=False, message="Complexity assessment failed"
             )
+            self._emit_planning_failed("Complexity assessment failed")
             return False
 
         # Map of all available phases
@@ -401,6 +447,9 @@ class SpecOrchestrator:
                     success=False,
                     message=f"Phase {phase_name} failed",
                 )
+                self._emit_planning_failed(
+                    f"Phase '{phase_name}' failed: {'; '.join(result.errors)}"
+                )
                 return False
 
         # Summary
@@ -638,6 +687,25 @@ class SpecOrchestrator:
             )
         )
 
+    def _emit_planning_failed(self, error: str) -> None:
+        """Emit PLANNING_FAILED event so the frontend transitions to error state.
+
+        Without this, the task stays stuck in 'planning' / 'in_progress' forever
+        when spec creation fails, because the XState machine never receives a
+        terminal event.
+
+        Args:
+            error: Human-readable error description
+        """
+        try:
+            task_emitter = TaskEventEmitter.from_spec_dir(self.spec_dir)
+            task_emitter.emit(
+                "PLANNING_FAILED",
+                {"error": error, "recoverable": True},
+            )
+        except Exception:
+            pass  # Best effort - don't mask the original failure
+
     def _run_review_checkpoint(self, auto_approve: bool) -> bool:
         """Run the human review checkpoint.
 
@@ -696,19 +764,25 @@ class SpecOrchestrator:
         The functionality has been moved to models.rename_spec_dir_from_requirements.
 
         Returns:
-            True if successful or not needed, False on error
+            True if successful or not needed, False if prerequisites are missing
         """
-        result = rename_spec_dir_from_requirements(self.spec_dir)
-        # Update self.spec_dir if it was renamed
-        if result and self.spec_dir.name.endswith("-pending"):
-            # Find the renamed directory
-            parent = self.spec_dir.parent
-            prefix = self.spec_dir.name[:4]  # e.g., "001-"
-            for candidate in parent.iterdir():
-                if (
-                    candidate.name.startswith(prefix)
-                    and "pending" not in candidate.name
-                ):
-                    self.spec_dir = candidate
-                    break
-        return result
+        # Check prerequisites first
+        requirements_file = self.spec_dir / "requirements.json"
+        if not requirements_file.exists():
+            return False
+
+        try:
+            with open(requirements_file, encoding="utf-8") as f:
+                req = json.load(f)
+            task_desc = req.get("task_description", "")
+            if not task_desc:
+                return False
+        except (json.JSONDecodeError, OSError):
+            return False
+
+        # Attempt rename
+        new_spec_dir = rename_spec_dir_from_requirements(self.spec_dir)
+        if new_spec_dir != self.spec_dir:
+            self.spec_dir = new_spec_dir
+            self.validator = SpecValidator(self.spec_dir)
+        return True

apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts

@@ -161,6 +161,26 @@ export function registerTaskExecutionHandlers(
 
       console.warn('[TASK_START] Found task:', task.specId, 'status:', task.status, 'reviewReason:', task.reviewReason, 'subtasks:', task.subtasks.length);
 
+      // Check if implementation_plan.json has valid subtasks BEFORE XState handling.
+      // This is more reliable than task.subtasks.length which may not be loaded yet.
+      const specsBaseDir = getSpecsDir(project.autoBuildPath);
+      const specDir = path.join(
+        project.path,
+        specsBaseDir,
+        task.specId
+      );
+      const planFilePath = path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN);
+      let planHasSubtasks = false;
+      const planContent = safeReadFileSync(planFilePath);
+      if (planContent) {
+        try {
+          const plan = JSON.parse(planContent);
+          planHasSubtasks = checkSubtasksCompletion(plan).totalCount > 0;
+        } catch {
+          // Invalid/corrupt plan file - treat as no subtasks
+        }
+      }
+
       // Immediately mark as started so the UI moves the card to In Progress.
       // Use XState actor state as source of truth (if actor exists), with task data as fallback.
       // - plan_review: User approved the plan, send PLAN_APPROVED to transition to coding
@@ -173,6 +193,11 @@ export function registerTaskExecutionHandlers(
         // XState says plan_review - send PLAN_APPROVED
         console.warn('[TASK_START] XState: plan_review -> coding via PLAN_APPROVED');
         taskStateManager.handleUiEvent(taskId, { type: 'PLAN_APPROVED' }, task, project);
+      } else if (currentXState === 'error' && !planHasSubtasks) {
+        // FIX (#1562): Task crashed during planning (no subtasks yet).
+        // Uses planHasSubtasks from implementation_plan.json (more reliable than task.subtasks.length).
+        console.warn('[TASK_START] XState: error with no plan subtasks -> planning via PLANNING_STARTED');
+        taskStateManager.handleUiEvent(taskId, { type: 'PLANNING_STARTED' }, task, project);
       } else if (currentXState === 'human_review' || currentXState === 'error') {
         // XState says human_review or error - send USER_RESUMED
         console.warn('[TASK_START] XState:', currentXState, '-> coding via USER_RESUMED');
@@ -186,6 +211,11 @@ export function registerTaskExecutionHandlers(
         // No XState actor - fallback to task data (e.g., after app restart)
         console.warn('[TASK_START] No XState actor, task data: plan_review -> coding via PLAN_APPROVED');
         taskStateManager.handleUiEvent(taskId, { type: 'PLAN_APPROVED' }, task, project);
+      } else if (task.status === 'error' && !planHasSubtasks) {
+        // FIX (#1562): No XState actor, task crashed during planning (no subtasks).
+        // Uses planHasSubtasks from implementation_plan.json (more reliable than task.subtasks.length).
+        console.warn('[TASK_START] No XState actor, error with no plan subtasks -> planning via PLANNING_STARTED');
+        taskStateManager.handleUiEvent(taskId, { type: 'PLANNING_STARTED' }, task, project);
       } else if (task.status === 'human_review' || task.status === 'error') {
         // No XState actor - fallback to task data for resuming
         console.warn('[TASK_START] No XState actor, task data:', task.status, '-> coding via USER_RESUMED');
@@ -204,13 +234,7 @@ export function registerTaskExecutionHandlers(
         console.warn(`[TASK_START] Reset ${resetResult.resetCount} stuck subtask(s) before starting`);
       }
 
-      // Start file watcher for this task
-      const specsBaseDir = getSpecsDir(project.autoBuildPath);
-      const specDir = path.join(
-        project.path,
-        specsBaseDir,
-        task.specId
-      );
+      // Start file watcher for this task (specsBaseDir and specDir already computed above)
       fileWatcher.watch(taskId, specDir);
 
       // Check if spec.md exists (indicates spec creation was already done or in progress)
@@ -220,9 +244,13 @@ export function registerTaskExecutionHandlers(
       // Check if this task needs spec creation first (no spec file = not yet created)
       // OR if it has a spec but no implementation plan subtasks (spec created, needs planning/building)
       const needsSpecCreation = !hasSpec;
-      const needsImplementation = hasSpec && task.subtasks.length === 0;
+      // FIX (#1562): Check actual plan file for subtasks, not just task.subtasks.length.
+      // When a task crashes during planning, it may have spec.md but an empty/missing
+      // implementation_plan.json. Previously, this path would call startTaskExecution
+      // (run.py) which expects subtasks to exist. Now we check the actual plan file.
+      const needsImplementation = hasSpec && !planHasSubtasks;
 
-      console.warn('[TASK_START] hasSpec:', hasSpec, 'needsSpecCreation:', needsSpecCreation, 'needsImplementation:', needsImplementation);
+      console.warn('[TASK_START] hasSpec:', hasSpec, 'planHasSubtasks:', planHasSubtasks, 'needsSpecCreation:', needsSpecCreation, 'needsImplementation:', needsImplementation);
 
       // Get base branch: task-level override takes precedence over project settings
       const baseBranch = task.metadata?.baseBranch || project.settings?.mainBranch;
@@ -237,18 +265,12 @@ export function registerTaskExecutionHandlers(
         // Also pass baseBranch so worktrees are created from the correct branch
         agentManager.startSpecCreation(taskId, project.path, taskDescription, specDir, task.metadata, baseBranch, project.id);
       } else if (needsImplementation) {
-        // Spec exists but no subtasks - run run.py to create implementation plan and execute
-        // Read the spec.md to get the task description
-        const _taskDescription = task.description || task.title;
-        try {
-          readFileSync(specFilePath, 'utf-8');
-        } catch {
-          // Use default description
-        }
-
-        console.warn('[TASK_START] Starting task execution (no subtasks) for:', task.specId);
-        // Start task execution which will create the implementation plan
-        // Note: No parallel mode for planning phase - parallel only makes sense with multiple subtasks
+        // Spec exists but no valid subtasks in implementation plan
+        // FIX (#1562): Use startTaskExecution (run.py) which will create the planner
+        // agent session to generate the implementation plan. run.py handles the case
+        // where implementation_plan.json is missing or has no subtasks - the planner
+        // agent will generate the plan before the coder starts.
+        console.warn('[TASK_START] Starting task execution (no valid subtasks in plan) for:', task.specId);
         agentManager.startTaskExecution(
           taskId,
           project.path,
@@ -716,7 +738,19 @@ export function registerTaskExecutionHandlers(
           const specFilePath = path.join(specDir, AUTO_BUILD_PATHS.SPEC_FILE);
           const hasSpec = existsSync(specFilePath);
           const needsSpecCreation = !hasSpec;
-          const needsImplementation = hasSpec && task.subtasks.length === 0;
+          // FIX (#1562): Check actual plan file for subtasks, not just task.subtasks.length
+          const updatePlanFilePath = path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN);
+          let updatePlanHasSubtasks = false;
+          const updatePlanContent = safeReadFileSync(updatePlanFilePath);
+          if (updatePlanContent) {
+            try {
+              const plan = JSON.parse(updatePlanContent);
+              updatePlanHasSubtasks = checkSubtasksCompletion(plan).totalCount > 0;
+            } catch {
+              // Invalid/corrupt plan file - treat as no subtasks
+            }
+          }
+          const needsImplementation = hasSpec && !updatePlanHasSubtasks;
 
           console.warn('[TASK_UPDATE_STATUS] hasSpec:', hasSpec, 'needsSpecCreation:', needsSpecCreation, 'needsImplementation:', needsImplementation);
 

apps/frontend/src/shared/state-machines/task-machine.ts

@@ -126,12 +126,16 @@ export const taskMachine = createMachine(
         on: {
           CREATE_PR: 'creating_pr',
           MARK_DONE: 'done',
-          USER_RESUMED: { target: 'coding', actions: 'clearReviewReason' }
+          USER_RESUMED: { target: 'coding', actions: 'clearReviewReason' },
+          // Allow restarting planning from human_review (e.g., incomplete task with no subtasks)
+          PLANNING_STARTED: { target: 'planning', actions: 'clearReviewReason' }
         }
       },
       error: {
         on: {
           USER_RESUMED: { target: 'coding', actions: 'clearReviewReason' },
+          // Allow restarting from error back to planning (e.g., spec creation crashed)
+          PLANNING_STARTED: { target: 'planning', actions: 'clearReviewReason' },
           MARK_DONE: 'done'
         }
       },