From 30f5784f851f59d8c0e1f0baaaa3f3745e2dcc2e Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sun, 15 Feb 2026 17:39:55 +0100 Subject: [PATCH 1/4] 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 --- apps/backend/spec/pipeline/orchestrator.py | 76 +++++++++++++++++++ .../ipc-handlers/task/execution-handlers.ts | 58 ++++++++++---- .../src/shared/state-machines/task-machine.ts | 6 +- 3 files changed, 125 insertions(+), 15 deletions(-) diff --git a/apps/backend/spec/pipeline/orchestrator.py b/apps/backend/spec/pipeline/orchestrator.py index e8dc040ef5..bff2cf9264 100644 --- a/apps/backend/spec/pipeline/orchestrator.py +++ b/apps/backend/spec/pipeline/orchestrator.py @@ -238,6 +238,42 @@ async def run(self, interactive: bool = True, auto_approve: bool = False) -> boo 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 + 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 @@ def run_phase(name: str, phase_fn: Callable) -> phases.PhaseResult: 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,28 @@ def run_phase(name: str, phase_fn: Callable) -> phases.PhaseResult: 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 + # IMPORTANT: Update self.spec_dir after rename so subsequent phases use the correct path + old_spec_dir = self.spec_dir rename_spec_dir_from_requirements(self.spec_dir) + if not self.spec_dir.exists() and old_spec_dir.name.endswith("-pending"): + # Directory was renamed - find the new path + parent = old_spec_dir.parent + prefix = old_spec_dir.name[:4] # e.g., "001-" + for candidate in parent.iterdir(): + if ( + candidate.name.startswith(prefix) + and candidate.is_dir() + and "pending" not in candidate.name + ): + self.spec_dir = candidate + self.validator = SpecValidator(self.spec_dir) + break # Update task description from requirements req = requirements.load_requirements(self.spec_dir) @@ -338,6 +391,7 @@ def run_phase(name: str, phase_fn: Callable) -> phases.PhaseResult: 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 +455,9 @@ def run_phase(name: str, phase_fn: Callable) -> phases.PhaseResult: 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 +695,25 @@ def _print_completion_summary( ) ) + 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. diff --git a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts index 95bcb9d8e6..e540a960b0 100644 --- a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts @@ -173,6 +173,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' && task.subtasks.length === 0) { + // FIX (#1562): Task crashed during planning (no subtasks yet). + // Send PLANNING_STARTED to go back to planning state, not coding. + console.warn('[TASK_START] XState: error with 0 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 +191,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' && task.subtasks.length === 0) { + // FIX (#1562): No XState actor, task crashed during planning (no subtasks). + // Send PLANNING_STARTED to restart planning instead of going to coding. + console.warn('[TASK_START] No XState actor, error with 0 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'); @@ -217,12 +227,38 @@ export function registerTaskExecutionHandlers( const specFilePath = path.join(specDir, AUTO_BUILD_PATHS.SPEC_FILE); const hasSpec = existsSync(specFilePath); + // Check if implementation_plan.json has valid subtasks + // This is more reliable than task.subtasks.length which may not be loaded yet + const planFilePath = path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN); + let planHasSubtasks = false; + if (existsSync(planFilePath)) { + try { + const planContent = readFileSync(planFilePath, 'utf-8'); + const plan = JSON.parse(planContent); + const phases = plan?.phases; + if (Array.isArray(phases)) { + const subtaskCount = phases.reduce( + (sum: number, phase: { subtasks?: unknown[] }) => + sum + (Array.isArray(phase.subtasks) ? phase.subtasks.length : 0), + 0 + ); + planHasSubtasks = subtaskCount > 0; + } + } catch { + // Invalid/corrupt plan file - treat as no subtasks + } + } + // 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 +273,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, diff --git a/apps/frontend/src/shared/state-machines/task-machine.ts b/apps/frontend/src/shared/state-machines/task-machine.ts index 52621af3c3..91fb3b4282 100644 --- a/apps/frontend/src/shared/state-machines/task-machine.ts +++ b/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' } }, From acf719cff7525c8ca13ef71aeda538d0a992d2fc Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sun, 15 Feb 2026 22:17:53 +0100 Subject: [PATCH 2/4] 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 --- apps/backend/spec/pipeline/models.py | 16 ++--- apps/backend/spec/pipeline/orchestrator.py | 39 +++-------- .../ipc-handlers/task/execution-handlers.ts | 70 +++++++++---------- 3 files changed, 52 insertions(+), 73 deletions(-) diff --git a/apps/backend/spec/pipeline/models.py b/apps/backend/spec/pipeline/models.py index 69614f88e0..b7cb1febc6 100644 --- a/apps/backend/spec/pipeline/models.py +++ b/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 diff --git a/apps/backend/spec/pipeline/orchestrator.py b/apps/backend/spec/pipeline/orchestrator.py index bff2cf9264..8cf231468f 100644 --- a/apps/backend/spec/pipeline/orchestrator.py +++ b/apps/backend/spec/pipeline/orchestrator.py @@ -258,7 +258,7 @@ async def run(self, interactive: bool = True, auto_approve: bool = False) -> boo message=f"Spec creation crashed: {e}", ) except Exception: - pass + pass # Best effort - don't mask the original error when logging fails raise async def _run_phases( @@ -354,21 +354,10 @@ def run_phase(name: str, phase_fn: Callable) -> phases.PhaseResult: # Rename spec folder with better name from requirements # IMPORTANT: Update self.spec_dir after rename so subsequent phases use the correct path - old_spec_dir = self.spec_dir - rename_spec_dir_from_requirements(self.spec_dir) - if not self.spec_dir.exists() and old_spec_dir.name.endswith("-pending"): - # Directory was renamed - find the new path - parent = old_spec_dir.parent - prefix = old_spec_dir.name[:4] # e.g., "001-" - for candidate in parent.iterdir(): - if ( - candidate.name.startswith(prefix) - and candidate.is_dir() - and "pending" not in candidate.name - ): - self.spec_dir = candidate - self.validator = SpecValidator(self.spec_dir) - break + 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 task description from requirements req = requirements.load_requirements(self.spec_dir) @@ -774,17 +763,7 @@ def _rename_spec_dir_from_requirements(self) -> bool: Returns: True if successful or not needed, False on error """ - 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 + new_spec_dir = rename_spec_dir_from_requirements(self.spec_dir) + if new_spec_dir != self.spec_dir: + self.spec_dir = new_spec_dir + return new_spec_dir.exists() diff --git a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts index e540a960b0..17e4d91125 100644 --- a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts @@ -161,6 +161,34 @@ 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; + if (existsSync(planFilePath)) { + try { + const planContent = readFileSync(planFilePath, 'utf-8'); + const plan = JSON.parse(planContent); + const phases = plan?.phases; + if (Array.isArray(phases)) { + const subtaskCount = phases.reduce( + (sum: number, phase: { subtasks?: unknown[] }) => + sum + (Array.isArray(phase.subtasks) ? phase.subtasks.length : 0), + 0 + ); + planHasSubtasks = subtaskCount > 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,10 +201,10 @@ 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' && task.subtasks.length === 0) { + } else if (currentXState === 'error' && !planHasSubtasks) { // FIX (#1562): Task crashed during planning (no subtasks yet). - // Send PLANNING_STARTED to go back to planning state, not coding. - console.warn('[TASK_START] XState: error with 0 subtasks -> planning via PLANNING_STARTED'); + // 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 @@ -191,10 +219,10 @@ 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' && task.subtasks.length === 0) { + } else if (task.status === 'error' && !planHasSubtasks) { // FIX (#1562): No XState actor, task crashed during planning (no subtasks). - // Send PLANNING_STARTED to restart planning instead of going to coding. - console.warn('[TASK_START] No XState actor, error with 0 subtasks -> planning via PLANNING_STARTED'); + // 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 @@ -214,41 +242,13 @@ 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) const specFilePath = path.join(specDir, AUTO_BUILD_PATHS.SPEC_FILE); const hasSpec = existsSync(specFilePath); - // Check if implementation_plan.json has valid subtasks - // This is more reliable than task.subtasks.length which may not be loaded yet - const planFilePath = path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN); - let planHasSubtasks = false; - if (existsSync(planFilePath)) { - try { - const planContent = readFileSync(planFilePath, 'utf-8'); - const plan = JSON.parse(planContent); - const phases = plan?.phases; - if (Array.isArray(phases)) { - const subtaskCount = phases.reduce( - (sum: number, phase: { subtasks?: unknown[] }) => - sum + (Array.isArray(phase.subtasks) ? phase.subtasks.length : 0), - 0 - ); - planHasSubtasks = subtaskCount > 0; - } - } catch { - // Invalid/corrupt plan file - treat as no subtasks - } - } - // 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; From 62e872308b3fe8dc98819e19869c4da70404a2b4 Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Mon, 16 Feb 2026 08:51:22 +0100 Subject: [PATCH 3/4] fix: update tests for rename_spec_dir_from_requirements return type change (#1562) Co-Authored-By: Claude Opus 4.6 --- apps/backend/spec/pipeline/orchestrator.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/apps/backend/spec/pipeline/orchestrator.py b/apps/backend/spec/pipeline/orchestrator.py index 8cf231468f..3cf9f5fbdd 100644 --- a/apps/backend/spec/pipeline/orchestrator.py +++ b/apps/backend/spec/pipeline/orchestrator.py @@ -761,9 +761,24 @@ def _rename_spec_dir_from_requirements(self) -> bool: 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 """ + # 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 - return new_spec_dir.exists() + return True From b3b5c56f4764563d46721bfab886e59cf4e43733 Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Mon, 16 Feb 2026 20:32:12 +0100 Subject: [PATCH 4/4] 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 --- apps/backend/spec/pipeline/orchestrator.py | 4 +++ .../ipc-handlers/task/execution-handlers.ts | 28 +++++++++++-------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/apps/backend/spec/pipeline/orchestrator.py b/apps/backend/spec/pipeline/orchestrator.py index 3cf9f5fbdd..2abec3ca4a 100644 --- a/apps/backend/spec/pipeline/orchestrator.py +++ b/apps/backend/spec/pipeline/orchestrator.py @@ -358,6 +358,9 @@ def run_phase(name: str, phase_fn: Callable) -> phases.PhaseResult: 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) @@ -781,4 +784,5 @@ def _rename_spec_dir_from_requirements(self) -> bool: 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 diff --git a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts index 17e4d91125..37c34b8105 100644 --- a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts @@ -171,19 +171,11 @@ export function registerTaskExecutionHandlers( ); const planFilePath = path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN); let planHasSubtasks = false; - if (existsSync(planFilePath)) { + const planContent = safeReadFileSync(planFilePath); + if (planContent) { try { - const planContent = readFileSync(planFilePath, 'utf-8'); const plan = JSON.parse(planContent); - const phases = plan?.phases; - if (Array.isArray(phases)) { - const subtaskCount = phases.reduce( - (sum: number, phase: { subtasks?: unknown[] }) => - sum + (Array.isArray(phase.subtasks) ? phase.subtasks.length : 0), - 0 - ); - planHasSubtasks = subtaskCount > 0; - } + planHasSubtasks = checkSubtasksCompletion(plan).totalCount > 0; } catch { // Invalid/corrupt plan file - treat as no subtasks } @@ -746,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);