-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: handle planning phase crash and resume recovery (#1562) #1844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 2 commits
30f5784
acf719c
62e8723
b3b5c56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 # Best effort - don't mask the original error when logging fails | ||
| raise | ||
|
Comment on lines
+241
to
+262
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Use Lines 246–251 replicate exactly what ♻️ Proposed fix 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
+ self._emit_planning_failed(f"Spec creation crashed: {e}")
try:
task_logger.end_phase(
LogPhase.PLANNING,🤖 Prompt for AI Agents |
||
|
|
||
| async def _run_phases( | ||
| self, | ||
| interactive: bool, | ||
| auto_approve: bool, | ||
| task_logger, | ||
| ui, | ||
| ) -> bool: | ||
|
Comment on lines
+264
to
+270
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Missing type hints on
Proposed fix+ from types import ModuleType
+ from task_logger import TaskLogger
+
async def _run_phases(
self,
interactive: bool,
auto_approve: bool,
- task_logger,
- ui,
+ task_logger: TaskLogger,
+ ui: ModuleType,
) -> bool:🤖 Prompt for AI Agents |
||
| """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,17 @@ 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 | ||
| 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) | ||
|
Comment on lines
+357
to
+360
This comment was marked as outdated.
Sorry, something went wrong.
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| # Update task description from requirements | ||
| req = requirements.load_requirements(self.spec_dir) | ||
|
|
@@ -338,6 +380,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 +444,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 +684,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 | ||
|
Comment on lines
+690
to
+707
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Clean helper; consider logging the swallowed exception for debuggability. The bare Suggested improvement except Exception:
- pass # Best effort - don't mask the original failure
+ # Best effort - don't mask the original failure.
+ # Logging here could itself fail, so keep it minimal.
+ import sys
+ try:
+ sys.stderr.write("[orchestrator] _emit_planning_failed failed\n")
+ except Exception:
+ pass🤖 Prompt for AI Agents |
||
|
|
||
| def _run_review_checkpoint(self, auto_approve: bool) -> bool: | ||
| """Run the human review checkpoint. | ||
|
|
||
|
|
@@ -698,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() | ||
|
||
Uh oh!
There was an error while loading. Please reload this page.