fix(cli): tasks generate --overwrite must not wipe tasks on gen failure (#725) (#797)

* fix(cli): tasks generate --overwrite must not wipe tasks on gen failure (#725)

--overwrite called delete_all() (committed) and only THEN ran generation. A
transient LLM failure (rate limit / timeout / bad response) fell to a bare
except that just printed the error — the whole task list was already
irreversibly gone, no backup (checkpoints only restore statuses, not deleted
rows).

Generate first (new tasks are appended), then delete the originals only after
success. On failure the originals are untouched and any partially-created new
tasks are rolled back, so a failed generation is a no-op.

Closes #725

* fix/test: guard rollback failure, note O(N^2), cover recursive path (#725 review)

Author: frankbria

codeframe/cli/app.py

@@ -1846,40 +1846,67 @@ def tasks_generate(
             console.print("Add one first: codeframe prd add <file.md>")
             raise typer.Exit(1)
 
-        # Handle --overwrite
-        if overwrite:
-            existing = tasks.list_tasks(workspace)
-            if existing:
-                deleted = tasks.delete_all(workspace)
-                console.print(f"[dim]Cleared {deleted} existing tasks[/dim]")
+        # --overwrite must not lose the existing tasks if generation fails
+        # (#725). Generation persists new tasks alongside the old ones; we only
+        # delete the originals *after* it succeeds. On failure the originals are
+        # untouched and any partially-created new tasks are rolled back, so a
+        # transient LLM error (rate limit / timeout / bad response) is a no-op.
+        original_ids = (
+            {t.id for t in tasks.list_tasks(workspace)} if overwrite else set()
+        )
 
         console.print(f"Generating tasks from PRD: [bold]{prd_record.title}[/bold]")
 
         if not no_llm:
             from codeframe.cli.validators import require_anthropic_api_key
             require_anthropic_api_key()
 
-        if recursive:
-            console.print(f"[dim]Using recursive decomposition (max depth: {max_depth})...[/dim]")
-
-            from codeframe.adapters.llm import get_provider
-            from codeframe.core.task_tree import generate_task_tree, flatten_task_tree
-
-            provider = get_provider()
-            tree = generate_task_tree(
-                provider,
-                prd_record.content,
-                lineage=[],
-                depth=0,
-                max_depth=max_depth,
-            )
-            created = flatten_task_tree(tree, workspace, prd_id=prd_record.id)
-        elif no_llm:
-            console.print("[dim]Using simple extraction (--no-llm)[/dim]")
-            created = tasks.generate_from_prd(workspace, prd_record, use_llm=False)
-        else:
-            console.print("[dim]Using LLM for task generation...[/dim]")
-            created = tasks.generate_from_prd(workspace, prd_record, use_llm=True)
+        try:
+            if recursive:
+                console.print(f"[dim]Using recursive decomposition (max depth: {max_depth})...[/dim]")
+
+                from codeframe.adapters.llm import get_provider
+                from codeframe.core.task_tree import generate_task_tree, flatten_task_tree
+
+                provider = get_provider()
+                tree = generate_task_tree(
+                    provider,
+                    prd_record.content,
+                    lineage=[],
+                    depth=0,
+                    max_depth=max_depth,
+                )
+                created = flatten_task_tree(tree, workspace, prd_id=prd_record.id)
+            elif no_llm:
+                console.print("[dim]Using simple extraction (--no-llm)[/dim]")
+                created = tasks.generate_from_prd(workspace, prd_record, use_llm=False)
+            else:
+                console.print("[dim]Using LLM for task generation...[/dim]")
+                created = tasks.generate_from_prd(workspace, prd_record, use_llm=True)
+        except Exception:
+            # Roll back any partial new tasks; the originals were never deleted.
+            # A failure *during* rollback must not mask the original generation
+            # error, so guard it and always re-raise the original.
+            if overwrite:
+                try:
+                    for t in tasks.list_tasks(workspace):
+                        if t.id not in original_ids:
+                            tasks.delete(workspace, t.id)
+                except Exception:
+                    console.print(
+                        "[yellow]Warning:[/yellow] could not fully roll back "
+                        "partially-generated tasks; run `cf tasks list` to check."
+                    )
+            raise
+
+        # Generation succeeded — now it is safe to clear the old tasks. Each
+        # delete() re-scans the workspace to strip dependents (#724), so this is
+        # O(N^2); fine for typical task counts, revisit with a bulk delete if a
+        # workspace ever holds thousands of tasks.
+        if overwrite and original_ids:
+            for tid in original_ids:
+                tasks.delete(workspace, tid)
+            console.print(f"[dim]Cleared {len(original_ids)} existing tasks[/dim]")
 
         # Emit event
         emit_for_workspace(

tests/cli/test_tasks_generate_overwrite.py

@@ -0,0 +1,89 @@
+"""`cf tasks generate --overwrite` must not lose tasks on a failed gen (#725/P0.14).
+
+Previously --overwrite called delete_all() (committed) and only then ran LLM
+generation. A transient LLM failure irreversibly wiped the task list. The fix
+generates first and deletes the originals only on success.
+"""
+
+from pathlib import Path
+from unittest.mock import patch
+
+import pytest
+from typer.testing import CliRunner
+
+from codeframe.cli.app import app
+from codeframe.core import prd, tasks
+from codeframe.core.workspace import create_or_load_workspace
+
+pytestmark = pytest.mark.v2
+
+runner = CliRunner()
+
+
+@pytest.fixture
+def ws_with_prd_and_tasks(tmp_path: Path):
+    ws = create_or_load_workspace(tmp_path)
+    prd.store(ws, content="# Demo PRD\n\nBuild a thing.", title="Demo")
+    tasks.create(ws, title="Existing 1", description="keep me")
+    tasks.create(ws, title="Existing 2", description="keep me too")
+    return ws, tmp_path
+
+
+def test_overwrite_preserves_tasks_when_generation_fails(ws_with_prd_and_tasks):
+    ws, path = ws_with_prd_and_tasks
+    before = {t.id for t in tasks.list_tasks(ws)}
+    assert len(before) == 2
+
+    # Simulate a transient generation failure (rate limit / timeout / bad response).
+    with patch.object(
+        tasks, "generate_from_prd", side_effect=RuntimeError("LLM timeout")
+    ):
+        result = runner.invoke(
+            app,
+            ["tasks", "generate", "--overwrite", "--no-llm", "-w", str(path)],
+        )
+
+    assert result.exit_code != 0  # the failure surfaces
+    # ...and the pre-existing tasks are intact (not wiped).
+    after = {t.id for t in tasks.list_tasks(ws)}
+    assert after == before
+
+
+def test_overwrite_preserves_tasks_when_recursive_generation_fails(ws_with_prd_and_tasks):
+    """The --recursive path (generate_task_tree) shares the same rollback."""
+    import codeframe.core.task_tree as task_tree
+    import codeframe.cli.validators as validators
+
+    ws, path = ws_with_prd_and_tasks
+    before = {t.id for t in tasks.list_tasks(ws)}
+
+    with patch.object(validators, "require_anthropic_api_key", lambda: None), patch.object(
+        task_tree, "generate_task_tree", side_effect=RuntimeError("LLM timeout")
+    ):
+        result = runner.invoke(
+            app,
+            ["tasks", "generate", "--overwrite", "--recursive", "-w", str(path)],
+        )
+
+    assert result.exit_code != 0
+    assert {t.id for t in tasks.list_tasks(ws)} == before
+
+
+def test_overwrite_replaces_tasks_on_success(ws_with_prd_and_tasks):
+    ws, path = ws_with_prd_and_tasks
+    before = {t.id for t in tasks.list_tasks(ws)}
+
+    def _fake_generate(workspace, prd_record, use_llm=True):
+        return [tasks.create(workspace, title="Fresh task", description="new")]
+
+    with patch.object(tasks, "generate_from_prd", side_effect=_fake_generate):
+        result = runner.invoke(
+            app,
+            ["tasks", "generate", "--overwrite", "--no-llm", "-w", str(path)],
+        )
+
+    assert result.exit_code == 0, result.output
+    titles = {t.title for t in tasks.list_tasks(ws)}
+    # Originals cleared, only the freshly generated task remains.
+    assert titles == {"Fresh task"}
+    assert not (before & {t.id for t in tasks.list_tasks(ws)})