Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions codeframe/cli/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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}")
Expand Down
5 changes: 4 additions & 1 deletion codeframe/core/conductor.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions codeframe/core/sandbox/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
ExecutionContext,
IsolationLevel,
create_execution_context,
validate_isolation,
)
from codeframe.core.sandbox.worktree import (
MergeResult,
Expand All @@ -20,6 +21,7 @@
"ExecutionContext",
"IsolationLevel",
"create_execution_context",
"validate_isolation",
"MergeResult",
"TaskWorktree",
"WorktreeRegistry",
Expand Down
59 changes: 36 additions & 23 deletions codeframe/core/sandbox/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
"""

Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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}")
42 changes: 42 additions & 0 deletions tests/cli/test_work_exit_codes.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
56 changes: 25 additions & 31 deletions tests/core/test_sandbox_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Loading