diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index 7259ce03..ee3c6361 100644 --- a/codeframe/cli/app.py +++ b/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}") diff --git a/codeframe/core/conductor.py b/codeframe/core/conductor.py index 98da423d..d27c1422 100644 --- a/codeframe/core/conductor.py +++ b/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 diff --git a/codeframe/core/sandbox/__init__.py b/codeframe/core/sandbox/__init__.py index 5574ca4b..0a7e86c3 100644 --- a/codeframe/core/sandbox/__init__.py +++ b/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", diff --git a/codeframe/core/sandbox/context.py b/codeframe/core/sandbox/context.py index aa0d1651..3092be4f 100644 --- a/codeframe/core/sandbox/context.py +++ b/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}") diff --git a/tests/cli/test_work_exit_codes.py b/tests/cli/test_work_exit_codes.py index fa8edf14..bab43577 100644 --- a/tests/cli/test_work_exit_codes.py +++ b/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 diff --git a/tests/core/test_sandbox_context.py b/tests/core/test_sandbox_context.py index 4e32b231..7d280620 100644 --- a/tests/core/test_sandbox_context.py +++ b/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: