fix(isolation): hard-reject worktree isolation to stop silent data loss (#714) (#786)

* fix(isolation): hard-reject worktree isolation to stop silent data loss (#714)

Worktree isolation force-deleted the per-task branch + worktree in cleanup()
without ever calling merge_back() (zero call sites) — external-engine work was
written to cf/<task_id>, then destroyed, while the task was marked COMPLETED.

Real merge-back is entangled with #715 (builtin engines ignore the worktree
path) and #716 (verification runs against the wrong tree) and also needs an
auto-commit step (adapters leave changes uncommitted). Per the issue's
sanctioned interim, fail closed until that lands:

- new validate_isolation(); create_execution_context() raises for WORKTREE
  before creating anything (the single chokepoint for CLI/server/conductor)
- cf work start / batch run reject --isolation worktree up front with a clear
  message, so no run is created and no task is stranded IN_PROGRESS
- worktree primitives (TaskWorktree.create/merge_back/cleanup) are untouched
  and still unit-tested; only the wiring is gated off

Closes #714

* docs/api: fix worktree docstring, export validate_isolation, note legacy paths (#714 review)

* test(cli): cover --isolation worktree rejection in work start/batch (#714 review)

* fix(cli): reject worktree isolation before API-key check in batch run (#714)

The batch isolation guard ran after the ANTHROPIC_API_KEY check, so without a
key the run failed on the key error before reaching the isolation reject
(CI failure). Move it ahead of the key check, matching work start.

Author: frankbria

codeframe/cli/app.py

@@ -2421,6 +2421,15 @@ def work_start(
 
         task = matching[0]
 
+        # Reject unsupported isolation first (#714): worktree would silently
+        # discard agent work. Fails closed before any run is created.
+        from codeframe.core.sandbox.context import IsolationLevel, validate_isolation
+        try:
+            validate_isolation(IsolationLevel(isolation))
+        except ValueError as exc:
+            console.print(f"[red]Error:[/red] {exc}")
+            raise typer.Exit(1)
+
         # Validate API key before creating run record (avoids dangling IN_PROGRESS state)
         if execute:
             from codeframe.core.engine_registry import is_external_engine
@@ -3779,6 +3788,15 @@ def batch_run(
             console.print("[red]Error:[/red] Specify task IDs or use --all-ready/--all-blocked")
             raise typer.Exit(1)
 
+        # Reject unsupported isolation up front (#714) — before the API-key
+        # check and before any run is created (worktree would discard work).
+        from codeframe.core.sandbox.context import IsolationLevel, validate_isolation
+        try:
+            validate_isolation(IsolationLevel(isolation))
+        except ValueError as exc:
+            console.print(f"[red]Error:[/red] {exc}")
+            raise typer.Exit(1)
+
         # Show execution plan
         console.print("\n[bold]Batch Execution Plan[/bold]")
         console.print(f"  Strategy: {strategy}")

codeframe/core/conductor.py

@@ -1611,7 +1611,10 @@ def _execute_parallel(
     Raises:
         CycleDetectedError: If circular dependencies are detected
     """
-    # Clean up orphaned worktrees from crashed workers on previous runs
+    # Clean up orphaned worktrees from crashed workers on previous runs.
+    # Legacy-record path only (#714): worktree isolation is disabled, so this
+    # fires just for batches persisted before the fix — harmless best-effort
+    # cleanup of pre-existing debris.
     from codeframe.core.sandbox.context import IsolationLevel as _IL
     if batch.isolation == _IL.WORKTREE.value:
         from codeframe.core.worktrees import WorktreeRegistry

codeframe/core/sandbox/__init__.py

@@ -8,6 +8,7 @@
     ExecutionContext,
     IsolationLevel,
     create_execution_context,
+    validate_isolation,
 )
 from codeframe.core.sandbox.worktree import (
     MergeResult,
@@ -20,6 +21,7 @@
     "ExecutionContext",
     "IsolationLevel",
     "create_execution_context",
+    "validate_isolation",
     "MergeResult",
     "TaskWorktree",
     "WorktreeRegistry",

codeframe/core/sandbox/context.py

@@ -5,7 +5,7 @@
 
 Isolation levels:
   NONE     — shared filesystem, preserves current behavior (default)
-  WORKTREE — git worktree per task, safe for parallel execution
+  WORKTREE — git worktree per task (DISABLED — discarded agent work; see #714)
   CLOUD    — E2B Linux VM per task (reserved, raises NotImplementedError)
 """
 
@@ -42,6 +42,36 @@ class ExecutionContext:
     cleanup: Callable[[], None]
 
 
+# worktree isolation is disabled until real merge-back ships (issue #714).
+# It force-deleted the per-task branch/worktree in cleanup() WITHOUT ever
+# merging the agent's work back to the base branch — silently discarding all
+# changes. Re-enable once merge-back (+ auto-commit of worktree changes) lands;
+# that work is gated behind #715 (builtin engines ignore the worktree path) and
+# #716 (verification runs against the wrong tree).
+_WORKTREE_DISABLED_MSG = (
+    "worktree isolation is temporarily disabled: it discards agent work without "
+    "merging it back to the base branch (silent data loss — see issue #714). "
+    "Use --isolation none (the default) until merge-back ships."
+)
+
+
+def validate_isolation(isolation: IsolationLevel) -> None:
+    """Reject isolation levels that are not currently safe to run.
+
+    Raises:
+        ValueError: If ``isolation`` is WORKTREE (see #714). Callers (CLI,
+            server, conductor) should surface this to the user *before*
+            creating a run so no task is stranded IN_PROGRESS.
+
+    Note:
+        The server path needs no explicit guard today — no ``ui/routers/`` route
+        accepts an ``isolation`` parameter — but ``create_execution_context``
+        calls this, so any future server/programmatic caller is covered too.
+    """
+    if isolation == IsolationLevel.WORKTREE:
+        raise ValueError(_WORKTREE_DISABLED_MSG)
+
+
 def create_execution_context(
     task_id: str,
     isolation: IsolationLevel,
@@ -58,9 +88,12 @@ def create_execution_context(
         ExecutionContext with workspace_path and cleanup configured.
 
     Raises:
+        ValueError: If isolation is WORKTREE (disabled until merge-back — #714).
         NotImplementedError: If isolation is CLOUD (future E2B phase).
-        subprocess.CalledProcessError: If git worktree creation fails.
     """
+    # Fail closed before creating anything: WORKTREE would destroy agent work.
+    validate_isolation(isolation)
+
     if isolation == IsolationLevel.NONE:
         return ExecutionContext(
             task_id=task_id,
@@ -69,30 +102,10 @@ def create_execution_context(
             cleanup=lambda: None,
         )
 
-    if isolation == IsolationLevel.WORKTREE:
-        from codeframe.core.worktrees import TaskWorktree, WorktreeRegistry, get_base_branch
-
-        worktree = TaskWorktree()
-        registry = WorktreeRegistry()
-        base_branch = get_base_branch(repo_path)
-        worktree_path = worktree.create(repo_path, task_id, base_branch=base_branch)
-        registry.register(repo_path, task_id, batch_id="unknown")
-
-        def cleanup() -> None:
-            worktree.cleanup(repo_path, task_id)
-            registry.unregister(repo_path, task_id)
-
-        return ExecutionContext(
-            task_id=task_id,
-            isolation=isolation,
-            workspace_path=worktree_path,
-            cleanup=cleanup,
-        )
-
     if isolation == IsolationLevel.CLOUD:
         raise NotImplementedError(
             "IsolationLevel.CLOUD is reserved for the future E2B agent adapter phase. "
-            "Use 'none' or 'worktree' instead."
+            "Use 'none' instead."
         )
 
     raise ValueError(f"Unknown isolation level: {isolation}")

tests/cli/test_work_exit_codes.py

@@ -208,3 +208,45 @@ def test_batch_with_blocked_returns_exit_one(self, workspace_with_two_ready_task
             )
 
         assert result.exit_code == 1, f"Expected 1 for BLOCKED batch: {result.output}"
+
+
+class TestIsolationRejection:
+    """Issue #714: `--isolation worktree` must be rejected up front (exit 1)
+    before any run/batch is created — worktree isolation would silently discard
+    agent work. Covers the branch CodeRabbit flagged as untested."""
+
+    def _workspace_with_task(self, tmp_path):
+        repo = tmp_path / "repo"
+        repo.mkdir()
+        ws = create_or_load_workspace(repo)
+        task = tasks.create(ws, title="t", description="d", status=TaskStatus.READY)
+        return repo, ws, task
+
+    def test_work_start_rejects_worktree(self, tmp_path):
+        repo, ws, task = self._workspace_with_task(tmp_path)
+        result = runner.invoke(
+            app,
+            ["work", "start", task.id[:8], "--execute", "--isolation", "worktree", "-w", str(repo)],
+        )
+        assert result.exit_code == 1
+        assert "worktree isolation is temporarily disabled" in result.output
+        # No run was created; the task was not moved to IN_PROGRESS.
+        assert tasks.get(ws, task.id).status != TaskStatus.IN_PROGRESS
+
+    def test_work_batch_run_rejects_worktree(self, tmp_path):
+        repo, ws, task = self._workspace_with_task(tmp_path)
+        result = runner.invoke(
+            app,
+            ["work", "batch", "run", task.id[:8], "--isolation", "worktree", "-w", str(repo)],
+        )
+        assert result.exit_code == 1
+        assert "worktree isolation is temporarily disabled" in result.output
+
+    def test_work_start_allows_none(self, tmp_path):
+        """--isolation none must NOT hit the rejection path (sanity)."""
+        repo, ws, task = self._workspace_with_task(tmp_path)
+        result = runner.invoke(
+            app,
+            ["work", "start", task.id[:8], "--isolation", "none", "-w", str(repo)],
+        )
+        assert "worktree isolation is temporarily disabled" not in result.output

tests/core/test_sandbox_context.py

@@ -112,43 +112,37 @@ def test_task_id_stored(self, tmp_path: Path):
 
 
 class TestCreateExecutionContextWorktree:
-    def test_creates_worktree_directory(self, git_repo: Path):
-        ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo)
-        assert ctx.workspace_path.exists()
-        assert ctx.workspace_path.is_dir()
-        ctx.cleanup()
+    """Issue #714 / P0.3: worktree isolation is disabled until merge-back ships
+    because cleanup() force-deleted the branch/worktree without merging agent
+    work back — silent data loss. create_execution_context must fail closed and
+    create NOTHING (no worktree dir, no branch)."""
 
-    def test_workspace_path_differs_from_repo_path(self, git_repo: Path):
-        ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo)
-        assert ctx.workspace_path != git_repo
-        ctx.cleanup()
+    def test_worktree_is_rejected(self, git_repo: Path):
+        with pytest.raises(ValueError, match="worktree isolation is temporarily disabled"):
+            create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo)
 
-    def test_workspace_path_is_inside_worktrees_dir(self, git_repo: Path):
-        ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo)
-        assert ".codeframe/worktrees" in str(ctx.workspace_path)
-        ctx.cleanup()
+    def test_reject_message_points_at_the_issue(self, git_repo: Path):
+        with pytest.raises(ValueError, match="#714"):
+            create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo)
 
-    def test_cleanup_removes_worktree(self, git_repo: Path):
-        ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo)
-        worktree_path = ctx.workspace_path
-        assert worktree_path.exists()
-        ctx.cleanup()
-        assert not worktree_path.exists()
+    def test_no_worktree_created_on_reject(self, git_repo: Path):
+        with pytest.raises(ValueError):
+            create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo)
+        # Nothing was created before the guard fired.
+        assert not (git_repo / ".codeframe" / "worktrees" / "task-abc").exists()
 
-    def test_isolation_level_stored(self, git_repo: Path):
-        ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo)
-        assert ctx.isolation == IsolationLevel.WORKTREE
-        ctx.cleanup()
 
-    def test_task_id_stored(self, git_repo: Path):
-        ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo)
-        assert ctx.task_id == "task-abc"
-        ctx.cleanup()
+class TestValidateIsolation:
+    def test_none_is_allowed(self):
+        from codeframe.core.sandbox.context import validate_isolation
 
-    def test_worktree_contains_repo_files(self, git_repo: Path):
-        ctx = create_execution_context("task-wt", IsolationLevel.WORKTREE, git_repo)
-        assert (ctx.workspace_path / "README.md").exists()
-        ctx.cleanup()
+        validate_isolation(IsolationLevel.NONE)  # no raise
+
+    def test_worktree_raises(self):
+        from codeframe.core.sandbox.context import validate_isolation
+
+        with pytest.raises(ValueError, match="#714"):
+            validate_isolation(IsolationLevel.WORKTREE)
 
 
 class TestCreateExecutionContextCloud: