diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index f4b3d2e1..e93fa3e2 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -2489,12 +2489,12 @@ def work_start( task = matching[0] - # Reject unsupported isolation first (#714): worktree would silently - # discard agent work. Fails closed before any run is created. + # Reject unsupported isolation first (before any run is created). WORKTREE + # is now accepted for this single-run path (#787); CLOUD still raises. from codeframe.core.sandbox.context import IsolationLevel, validate_isolation try: validate_isolation(IsolationLevel(isolation)) - except ValueError as exc: + except (ValueError, NotImplementedError) as exc: console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) @@ -3800,7 +3800,6 @@ def batch_run( codeframe work batch run --all-ready --engine plan codeframe work batch run task1 task2 --dry-run codeframe work batch run task1 task2 --retry 2 - codeframe work batch run --all-ready --isolation worktree """ from codeframe.core.workspace import get_workspace from codeframe.core import tasks as tasks_module, conductor @@ -3859,12 +3858,23 @@ 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). + # Reject unsupported isolation up front — before the API-key check and + # before any batch is created. CLOUD is not implemented; WORKTREE is + # enabled only for the single-run path (`cf work start`, #787) — the + # batch subprocess path can't reach the gitignored .codeframe DB from a + # worktree, so it stays rejected here until that's solved. from codeframe.core.sandbox.context import IsolationLevel, validate_isolation + if IsolationLevel(isolation) == IsolationLevel.WORKTREE: + console.print( + "[red]Error:[/red] worktree isolation is not yet supported for " + "batch runs (subprocess workers can't reach the workspace state " + "DB in a worktree). Use it with a single task: " + "`cf work start --execute --isolation worktree`." + ) + raise typer.Exit(1) try: validate_isolation(IsolationLevel(isolation)) - except ValueError as exc: + except (ValueError, NotImplementedError) as exc: console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) diff --git a/codeframe/core/adapters/builtin.py b/codeframe/core/adapters/builtin.py index 5b759399..cd45721f 100644 --- a/codeframe/core/adapters/builtin.py +++ b/codeframe/core/adapters/builtin.py @@ -74,15 +74,21 @@ def run( ) -> AgentResult: """Run the ReactAgent with stall retry, map AgentStatus to AgentResult.""" from codeframe.core.react_agent import ReactAgent + from codeframe.core.sandbox.context import rebased_workspace from codeframe.core.stall_detector import StallDetectedError + # #715: when isolated in a worktree, run against workspace_path so code + # I/O and gates land in the worktree; task/blocker state stays on the + # main-repo DB (rebased_workspace keeps state_dir unchanged). + run_workspace = rebased_workspace(self._workspace, workspace_path) + def _bridge_event(event_type: str, data: dict) -> None: if on_event: on_event(AgentEvent(type=event_type, data=data)) def _build_agent() -> ReactAgent: kwargs: dict = { - "workspace": self._workspace, + "workspace": run_workspace, "llm_provider": self._llm_provider, "stall_timeout_s": self._stall_timeout_s, "event_publisher": self._event_publisher, @@ -179,6 +185,10 @@ def run( ) -> AgentResult: """Run the plan-based Agent with supervisor retry, map to AgentResult.""" from codeframe.core.agent import Agent, AgentStatus + from codeframe.core.sandbox.context import rebased_workspace + + # #715: run against the worktree path when isolated (state stays on main). + run_workspace = rebased_workspace(self._workspace, workspace_path) def _bridge_event(event_type: str, data: dict) -> None: if on_event: @@ -186,7 +196,7 @@ def _bridge_event(event_type: str, data: dict) -> None: def _build_agent() -> Agent: return Agent( - workspace=self._workspace, + workspace=run_workspace, llm_provider=self._llm_provider, dry_run=self._dry_run, on_event=_bridge_event, diff --git a/codeframe/core/adapters/verification_wrapper.py b/codeframe/core/adapters/verification_wrapper.py index 32808c6a..18ec461e 100644 --- a/codeframe/core/adapters/verification_wrapper.py +++ b/codeframe/core/adapters/verification_wrapper.py @@ -53,6 +53,10 @@ def __init__( ) -> None: self._inner = inner self._workspace = workspace + # Workspace that verification gates + quick fixes run against. Rebased to + # the worktree in run() when isolated (#716); blocker creation still uses + # self._workspace so blockers land in the main-repo state DB. + self._verify_workspace = workspace self._max_correction_rounds = max_correction_rounds self._gate_names = gate_names # None = use default gates self._verbose = verbose @@ -70,6 +74,10 @@ def run( on_event: Callable[[AgentEvent], None] | None = None, ) -> AgentResult: """Run the inner adapter, then verify with gates. Self-correct on failure.""" + from codeframe.core.sandbox.context import rebased_workspace + + # #716: verify against the worktree when isolated (no-op for NONE). + self._verify_workspace = rebased_workspace(self._workspace, workspace_path) # Initial run result = self._inner.run(task_id, prompt, workspace_path, on_event) @@ -90,7 +98,7 @@ def run( )) gate_result = run_gates( - self._workspace, + self._verify_workspace, gates=self._gate_names, verbose=self._verbose, ) @@ -160,7 +168,7 @@ def run( # Final gate check after all correction rounds gate_result = run_gates( - self._workspace, + self._verify_workspace, gates=self._gate_names, verbose=self._verbose, ) @@ -179,11 +187,11 @@ def _try_quick_fix(self, error_summary: str) -> bool: Returns True if a fix was successfully applied. """ - fix = find_quick_fix(error_summary, repo_path=self._workspace.repo_path) + fix = find_quick_fix(error_summary, repo_path=self._verify_workspace.repo_path) if fix is None: return False - success, msg = apply_quick_fix(fix, self._workspace.repo_path) + success, msg = apply_quick_fix(fix, self._verify_workspace.repo_path) if success: self._verbose_print(f"[VerificationWrapper] Quick fix: {msg}") return success diff --git a/codeframe/core/conductor.py b/codeframe/core/conductor.py index a60b1244..e50f03db 100644 --- a/codeframe/core/conductor.py +++ b/codeframe/core/conductor.py @@ -1701,9 +1701,9 @@ def _execute_parallel( CycleDetectedError: If circular dependencies are detected """ # 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. + # Batch worktree isolation stays rejected at the CLI (#787 enables it only + # for the single-run path), so this fires just for legacy batches persisted + # with worktree isolation — 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/runtime.py b/codeframe/core/runtime.py index f4776ca2..b0703f46 100644 --- a/codeframe/core/runtime.py +++ b/codeframe/core/runtime.py @@ -710,14 +710,27 @@ def execute_agent( import time as _time_mod _perf_start_ms = int(_time_mod.monotonic() * 1000) - # Create execution context (handles isolation; NONE is a no-op) - from codeframe.core.sandbox.context import IsolationLevel, create_execution_context - exec_ctx = create_execution_context( - run.task_id, IsolationLevel(isolation), workspace.repo_path + from codeframe.core.sandbox.context import ( + ExecutionContext, + IsolationLevel, + create_execution_context, ) - effective_repo_path = exec_ctx.workspace_path + # Execution context is created INSIDE the try (#787): building a worktree runs + # git, which can fail (e.g. a preserved cf/ branch from a prior run). + # Creating it here would let that exception escape execute_agent and strand the + # run IN_PROGRESS; inside the try it becomes a handled FAILED with preservation. + exec_ctx: "ExecutionContext | None" = None + effective_repo_path = workspace.repo_path + # Worktree isolation (#787): tracks whether agent work was merged back so the + # finally block cleans up (merged) vs preserves the branch (failed/conflict). + worktree_merged = False try: + exec_ctx = create_execution_context( + run.task_id, IsolationLevel(isolation), workspace.repo_path + ) + effective_repo_path = exec_ctx.workspace_path + # Execute before_task hook (aborts on failure) if env_config and hook_ctx: try: @@ -830,6 +843,61 @@ def on_adapter_event(event: AdapterEvent) -> None: ) state.blocker = blocker_obj + # Worktree isolation (#787): on a successful run, auto-commit + merge the + # task branch back to base. A merge conflict (or an auto-commit failure) + # becomes a blocker and the branch is preserved (downgrade COMPLETED → + # BLOCKED so the run reflects it before the status transition below). + # worktree_merged is only set True on a clean merge, so any other outcome + # — including an exception — routes the finally block to preserve(). + # Runs here only when exec_ctx has a worktree (merge_back is None for NONE). + if exec_ctx.merge_back is not None and state.status == AgentStatus.COMPLETED: + from codeframe.core import blockers as blockers_mod + try: + merge_result = exec_ctx.merge_back() + except Exception as merge_exc: + # Auto-commit / merge raised (e.g. commit hook rejection, git + # error). Never proceed to cleanup — that would discard the + # agent's uncommitted work (the #714 class of bug). + block_q = ( + f"Worktree merge-back failed for cf/{run.task_id}: {merge_exc}. " + "The branch and worktree have been preserved for recovery." + ) + blocker_obj = blockers_mod.create( + workspace, task_id=run.task_id, question=block_q, + ) + state = AgentState(status=AgentStatus.BLOCKED) + state.blocker = blocker_obj + run_logger.error( + LogCategory.BLOCKER, + f"Worktree merge-back error for {run.task_id}", + {"error": str(merge_exc)[:500]}, + ) + else: + if not merge_result.success: + conflict_q = ( + f"Worktree merge-back conflicted merging cf/{run.task_id} " + "into the base branch. Resolve manually — the branch and " + "worktree have been preserved for recovery.\n\n" + f"{merge_result.conflict_details[:1000]}" + ) + blocker_obj = blockers_mod.create( + workspace, task_id=run.task_id, question=conflict_q, + ) + state = AgentState(status=AgentStatus.BLOCKED) + state.blocker = blocker_obj + run_logger.warning( + LogCategory.BLOCKER, + f"Worktree merge-back conflict for {run.task_id}", + {"conflict": merge_result.conflict_details[:500]}, + ) + else: + worktree_merged = True + + # Re-sync agent_status to the final state: merge-back may have downgraded + # a COMPLETED run to BLOCKED, and engine_stats.record_run below keys off + # agent_status — without this a conflicted run is miscounted as COMPLETED. + agent_status = state.status + # Log final status if state.status == AgentStatus.COMPLETED: run_logger.info(LogCategory.STATE_CHANGE, "Agent completed successfully") @@ -928,8 +996,17 @@ def on_adapter_event(event: AdapterEvent) -> None: finally: # Always close the output logger to ensure file is properly flushed output_logger.close() - # Clean up execution context (no-op for NONE, removes worktree for WORKTREE) - exec_ctx.cleanup() + # Clean up execution context. For NONE this is a harmless no-op. For a + # WORKTREE run: remove the worktree + branch only when work was merged + # back; otherwise preserve them for recovery (failure/blocked/conflict/ + # exception — never silently discard agent work, the #714 bug). + # exec_ctx is None when create_execution_context itself failed (e.g. a + # preserved branch collision) — nothing to clean up or preserve here. + if exec_ctx is not None: + if exec_ctx.merge_back is not None and not worktree_merged: + exec_ctx.preserve() + else: + exec_ctx.cleanup() def _event_type_to_category(event_type: str): diff --git a/codeframe/core/sandbox/context.py b/codeframe/core/sandbox/context.py index 3092be4f..d4738b2d 100644 --- a/codeframe/core/sandbox/context.py +++ b/codeframe/core/sandbox/context.py @@ -5,16 +5,28 @@ Isolation levels: NONE — shared filesystem, preserves current behavior (default) - WORKTREE — git worktree per task (DISABLED — discarded agent work; see #714) + WORKTREE — git worktree per task with merge-back (single-run path only; #787) CLOUD — E2B Linux VM per task (reserved, raises NotImplementedError) + +Worktree scope (#787): worktree isolation is enabled for the in-process +single-run path (``cf work start --isolation worktree`` → runtime.execute_agent), +which rebases the workspace so code + gates land in the worktree while task/ +blocker/event state stays in the main repo's ``.codeframe`` DB. The batch +subprocess path (conductor) stays rejected at the CLI: a spawned child runs with +``cwd=worktree`` and cannot reach the gitignored ``.codeframe`` DB there. """ from __future__ import annotations -from dataclasses import dataclass +import dataclasses +from dataclasses import dataclass, field from enum import Enum from pathlib import Path -from typing import Callable +from typing import TYPE_CHECKING, Callable, Optional + +if TYPE_CHECKING: + from codeframe.core.workspace import Workspace + from codeframe.core.worktrees import MergeResult class IsolationLevel(str, Enum): @@ -25,6 +37,10 @@ class IsolationLevel(str, Enum): CLOUD = "cloud" +def _noop() -> None: + return None + + @dataclass class ExecutionContext: """Execution environment for a single task run. @@ -33,43 +49,59 @@ class ExecutionContext: task_id: Task being executed. isolation: Isolation strategy in use. workspace_path: Root path the agent should use for all file I/O. - cleanup: Called after task completion to release resources. + cleanup: Full teardown — removes the worktree and deletes its branch + (no-op for NONE). Called only when the run's work has been merged + back (or when there is nothing to preserve). + merge_back: For WORKTREE, auto-commits worktree changes then merges the + task branch into the base branch, returning a MergeResult. ``None`` + when there is no worktree to merge (NONE). + preserve: Leave the worktree + branch intact for recovery (no-op for + NONE). Called instead of ``cleanup`` on failure, block, or merge + conflict so agent work is never silently discarded (the #714 bug). """ task_id: str isolation: IsolationLevel workspace_path: Path cleanup: Callable[[], None] + merge_back: Optional[Callable[[], "MergeResult"]] = None + preserve: Callable[[], None] = field(default=_noop) -# 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 rebased_workspace(workspace: "Workspace", workspace_path: Path) -> "Workspace": + """Return a Workspace whose code root is ``workspace_path``. + + Used by the builtin adapters (#715) and the verification wrapper (#716) so + that code I/O and verification gates run against the worktree, while the + ``state_dir``/``db_path`` (task, blocker, and event state) stay pointed at + the original main-repo ``.codeframe`` directory. Returns the workspace + unchanged when ``workspace_path`` already is its repo root (the NONE case). + """ + if Path(workspace_path) == workspace.repo_path: + return workspace + return dataclasses.replace(workspace, repo_path=Path(workspace_path)) def validate_isolation(isolation: IsolationLevel) -> None: """Reject isolation levels that are not currently safe to run. + WORKTREE is now allowed for the in-process single-run path (#787): it + auto-commits and merges agent work back to the base branch, and preserves + the branch on failure/conflict rather than discarding it (the #714 bug). + 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. + NotImplementedError: If ``isolation`` is CLOUD (reserved for E2B). 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. + The batch subprocess path (conductor) still rejects WORKTREE at the CLI + because a child process cannot reach the gitignored ``.codeframe`` DB in + a worktree. That guard lives in ``cli/app.py`` (batch command), not here. """ - if isolation == IsolationLevel.WORKTREE: - raise ValueError(_WORKTREE_DISABLED_MSG) + if isolation == IsolationLevel.CLOUD: + raise NotImplementedError( + "IsolationLevel.CLOUD is reserved for the future E2B agent adapter phase. " + "Use 'none' instead." + ) def create_execution_context( @@ -85,13 +117,13 @@ def create_execution_context( repo_path: Canonical repository root path. Returns: - ExecutionContext with workspace_path and cleanup configured. + ExecutionContext with workspace_path, merge_back, cleanup, and preserve + configured for the isolation level. Raises: - ValueError: If isolation is WORKTREE (disabled until merge-back — #714). NotImplementedError: If isolation is CLOUD (future E2B phase). + ValueError: If isolation is an unknown level. """ - # Fail closed before creating anything: WORKTREE would destroy agent work. validate_isolation(isolation) if isolation == IsolationLevel.NONE: @@ -99,13 +131,58 @@ def create_execution_context( task_id=task_id, isolation=isolation, workspace_path=repo_path, - cleanup=lambda: None, + cleanup=_noop, ) - if isolation == IsolationLevel.CLOUD: - raise NotImplementedError( - "IsolationLevel.CLOUD is reserved for the future E2B agent adapter phase. " - "Use 'none' instead." - ) + if isolation == IsolationLevel.WORKTREE: + return _create_worktree_context(task_id, repo_path) raise ValueError(f"Unknown isolation level: {isolation}") + + +def _create_worktree_context(task_id: str, repo_path: Path) -> ExecutionContext: + """Create a git worktree and wire its merge-back / cleanup / preserve hooks. + + The worktree is intentionally NOT registered in WorktreeRegistry: orphan + cleanup (keyed on process liveness) would force-delete a preserved branch + once this process exits, defeating the failure/conflict preservation the + acceptance criteria require. + """ + import subprocess + + from codeframe.core.worktrees import WORKTREE_DIR, TaskWorktree, get_base_branch + + # A preserved cf/ branch or worktree dir from a prior failed/conflicted + # run would make `git worktree add -b` fail. Surface an actionable error instead + # of a raw git traceback (runtime creates this inside its try, so this becomes a + # handled failure rather than a stranded IN_PROGRESS run). + branch_name = f"cf/{task_id}" + existing = subprocess.run( + ["git", "branch", "--list", branch_name], + cwd=str(repo_path), capture_output=True, text=True, + ) + worktree_dir = repo_path / WORKTREE_DIR / task_id + if branch_name in existing.stdout or worktree_dir.exists(): + raise ValueError( + f"a worktree or branch '{branch_name}' from a previous run of this task " + "still exists (preserved for recovery). Recover or discard it, then " + f"retry — e.g. `git worktree remove --force {worktree_dir}` and " + f"`git branch -D {branch_name}`." + ) + + base_branch = get_base_branch(repo_path) + worktree = TaskWorktree() + worktree_path = worktree.create(repo_path, task_id, base_branch=base_branch) + + def _merge_back() -> "MergeResult": + worktree.auto_commit(worktree_path, task_id) + return worktree.merge_back(repo_path, task_id, base_branch=base_branch) + + return ExecutionContext( + task_id=task_id, + isolation=IsolationLevel.WORKTREE, + workspace_path=worktree_path, + cleanup=lambda: worktree.cleanup(repo_path, task_id), + merge_back=_merge_back, + preserve=_noop, # leave worktree + branch on disk for recovery + ) diff --git a/codeframe/core/worktrees.py b/codeframe/core/worktrees.py index f9278383..66bf1166 100644 --- a/codeframe/core/worktrees.py +++ b/codeframe/core/worktrees.py @@ -13,6 +13,7 @@ from __future__ import annotations +import contextlib import json import logging import os @@ -21,7 +22,12 @@ from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path -from typing import Optional +from typing import Iterator, Optional + +try: + import fcntl +except ImportError: # pragma: no cover - non-POSIX (e.g. native Windows) + fcntl = None # type: ignore[assignment] logger = logging.getLogger(__name__) @@ -30,6 +36,29 @@ _registry_lock = threading.Lock() +@contextlib.contextmanager +def _main_tree_lock(workspace_path: Path) -> Iterator[None]: + """Serialize operations that mutate the shared main working tree. + + ``merge_back`` runs ``git checkout`` + ``git merge`` directly in the main + repo's single working directory. Two concurrent single-run worktree merges + against the same repo would otherwise interleave in that one directory and + corrupt it. This is a cross-process advisory lock (``flock``) keyed on the + repo. Best-effort: a no-op where ``fcntl`` is unavailable (non-POSIX). + """ + if fcntl is None: + yield + return + lock_path = workspace_path / ".git" / "cf-merge.lock" + lock_path.parent.mkdir(parents=True, exist_ok=True) + with open(lock_path, "w") as handle: + fcntl.flock(handle, fcntl.LOCK_EX) + try: + yield + finally: + fcntl.flock(handle, fcntl.LOCK_UN) + + @dataclass class MergeResult: """Result from merging a worktree branch back to base.""" @@ -77,6 +106,68 @@ def create( logger.info("Created worktree for %s at %s", task_id, worktree_path) return worktree_path + def auto_commit( + self, + worktree_path: Path, + task_id: str, + ) -> bool: + """Stage and commit all changes in the worktree on its branch. + + Agent adapters write files but never commit, so ``git merge cf/`` + would merge nothing. This commits any pending work first. + + Args: + worktree_path: Path to the task's worktree directory + task_id: Task identifier (for the commit message) + + Returns: + True if a commit was created, False if the worktree was clean. + + Raises: + RuntimeError: If staging or committing fails. Failing loudly is + deliberate — a silent failure here would report "clean"/"committed" + while agent work sits uncommitted, and the caller's merge-back + would then merge nothing and cleanup would delete the branch, + discarding that work (the #714 class of bug). On raise, the run's + merge-back aborts and the branch/worktree are preserved. + """ + add = subprocess.run( + ["git", "add", "-A"], + cwd=str(worktree_path), + capture_output=True, + text=True, + ) + if add.returncode != 0: + raise RuntimeError( + f"git add failed in worktree for {task_id}: {add.stderr.strip()}" + ) + + # Nothing staged → genuinely clean; nothing to commit. + staged = subprocess.run( + ["git", "diff", "--cached", "--quiet"], + cwd=str(worktree_path), + capture_output=True, + ) + if staged.returncode == 0: + return False + + commit = subprocess.run( + ["git", "commit", "-m", f"cf: auto-commit worktree changes for {task_id}"], + cwd=str(worktree_path), + capture_output=True, + text=True, + ) + if commit.returncode != 0: + # Staged changes exist but couldn't be committed (hook rejection, + # missing identity, object-DB error). Do NOT report success — that + # would route to cleanup and discard the staged work. + raise RuntimeError( + f"git commit failed in worktree for {task_id}: " + f"{(commit.stderr or commit.stdout).strip()}" + ) + logger.info("Auto-committed worktree changes for %s", task_id) + return True + def merge_back( self, workspace_path: Path, @@ -95,41 +186,44 @@ def merge_back( """ branch_name = f"cf/{task_id}" - # Checkout base branch - subprocess.run( - ["git", "checkout", base_branch], - cwd=str(workspace_path), - capture_output=True, - text=True, - check=True, - ) - - # Attempt merge - result = subprocess.run( - ["git", "merge", branch_name, "--no-ff", "-m", f"Merge {branch_name} into {base_branch}"], - cwd=str(workspace_path), - capture_output=True, - text=True, - ) - - if result.returncode == 0: - # Get merge commit hash - head = subprocess.run( - ["git", "rev-parse", "HEAD"], + # checkout + merge mutate the shared main working tree — serialize against + # concurrent worktree merges on the same repo (see _main_tree_lock). + with _main_tree_lock(workspace_path): + # Checkout base branch + subprocess.run( + ["git", "checkout", base_branch], cwd=str(workspace_path), capture_output=True, text=True, + check=True, ) - merge_commit = head.stdout.strip() if head.returncode == 0 else None - logger.info("Merged %s back to %s", branch_name, base_branch) - return MergeResult( - task_id=task_id, - success=True, - conflict_details="", - merge_commit=merge_commit, + # Attempt merge + result = subprocess.run( + ["git", "merge", branch_name, "--no-ff", "-m", f"Merge {branch_name} into {base_branch}"], + cwd=str(workspace_path), + capture_output=True, + text=True, ) - else: + + if result.returncode == 0: + # Get merge commit hash + head = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=str(workspace_path), + capture_output=True, + text=True, + ) + merge_commit = head.stdout.strip() if head.returncode == 0 else None + + logger.info("Merged %s back to %s", branch_name, base_branch) + return MergeResult( + task_id=task_id, + success=True, + conflict_details="", + merge_commit=merge_commit, + ) + # Merge conflict — abort and report conflict_output = result.stdout + result.stderr subprocess.run( diff --git a/tests/cli/test_work_exit_codes.py b/tests/cli/test_work_exit_codes.py index 32a4b9a6..70877ea8 100644 --- a/tests/cli/test_work_exit_codes.py +++ b/tests/cli/test_work_exit_codes.py @@ -211,9 +211,9 @@ def test_batch_with_blocked_returns_exit_one(self, workspace_with_two_ready_task 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.""" + """Issue #787: `--isolation worktree` is now accepted for the single-run + path (`cf work start`) but still rejected for batch runs (the subprocess + worker can't reach the workspace state DB in a worktree).""" def _workspace_with_task(self, tmp_path): repo = tmp_path / "repo" @@ -222,16 +222,18 @@ def _workspace_with_task(self, tmp_path): task = tasks.create(ws, title="t", description="d", status=TaskStatus.READY) return repo, ws, task - def test_work_start_rejects_worktree(self, tmp_path): + def test_work_start_accepts_worktree(self, tmp_path): + """#787: `cf work start --isolation worktree` passes validation and starts + a run (worktree is created inside execute_agent, so no --execute here).""" repo, ws, task = self._workspace_with_task(tmp_path) result = runner.invoke( app, - ["work", "start", task.id[:8], "--execute", "--isolation", "worktree", "-w", str(repo)], + ["work", "start", task.id[:8], "--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 + assert result.exit_code == 0, result.output + assert "not yet supported for batch" not in result.output + # The run was started (task moved to IN_PROGRESS), not rejected. + 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) @@ -240,16 +242,16 @@ def test_work_batch_run_rejects_worktree(self, tmp_path): ["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 + assert "not yet supported for batch" in result.output def test_work_start_allows_none(self, tmp_path): - """--isolation none must NOT hit the rejection path (sanity).""" + """--isolation none must NOT hit any 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 + assert result.exit_code == 0, result.output def test_work_start_rejects_cloud_at_parse_time(self, tmp_path): """Issue #769: 'cloud' is not a valid --isolation choice — Click rejects diff --git a/tests/core/adapters/test_builtin.py b/tests/core/adapters/test_builtin.py index 12392d84..d8809518 100644 --- a/tests/core/adapters/test_builtin.py +++ b/tests/core/adapters/test_builtin.py @@ -16,7 +16,10 @@ @pytest.fixture def mock_workspace(): ws = MagicMock() - ws.repo_path = Path("/tmp/test-repo") + # Match the path passed to adapter.run() below so the (non-worktree) call has + # workspace_path == repo_path — rebased_workspace then returns this mock + # unchanged instead of attempting dataclasses.replace on a MagicMock (#787). + ws.repo_path = Path("/tmp") return ws diff --git a/tests/core/adapters/test_verification_wrapper.py b/tests/core/adapters/test_verification_wrapper.py index bb1860df..a8290a22 100644 --- a/tests/core/adapters/test_verification_wrapper.py +++ b/tests/core/adapters/test_verification_wrapper.py @@ -13,7 +13,10 @@ @pytest.fixture def mock_workspace(): ws = MagicMock() - ws.repo_path = Path("/tmp/test-repo") + # Match the path passed to wrapper.run() below so the (non-worktree) call has + # workspace_path == repo_path — rebased_workspace then returns this mock + # unchanged instead of attempting dataclasses.replace on a MagicMock (#787). + ws.repo_path = Path("/tmp") return ws diff --git a/tests/core/test_engine_registry_extended.py b/tests/core/test_engine_registry_extended.py index ce28cc99..e1ceb5bd 100644 --- a/tests/core/test_engine_registry_extended.py +++ b/tests/core/test_engine_registry_extended.py @@ -98,7 +98,7 @@ def test_stall_retry_succeeds_on_second_attempt(self): from codeframe.core.stall_detector import StallDetectedError ws = MagicMock() - ws.repo_path = Path("/tmp/test") + ws.repo_path = Path("/tmp") # match adapter.run() path so #787 rebase is a no-op provider = MagicMock() call_count = 0 @@ -128,7 +128,7 @@ def test_stall_retry_exhausted_returns_failed(self): from codeframe.core.stall_detector import StallDetectedError ws = MagicMock() - ws.repo_path = Path("/tmp/test") + ws.repo_path = Path("/tmp") # match adapter.run() path so #787 rebase is a no-op provider = MagicMock() def always_stall(**kwargs): @@ -161,7 +161,7 @@ def test_completed_returns_completed(self): from codeframe.core.agent import AgentStatus ws = MagicMock() - ws.repo_path = Path("/tmp/test") + ws.repo_path = Path("/tmp") # match adapter.run() path so #787 rebase is a no-op provider = MagicMock() with patch("codeframe.core.agent.Agent") as mock_cls: @@ -176,7 +176,7 @@ def test_blocked_triggers_supervisor_unblock(self): from codeframe.core.agent import AgentStatus ws = MagicMock() - ws.repo_path = Path("/tmp/test") + ws.repo_path = Path("/tmp") # match adapter.run() path so #787 rebase is a no-op provider = MagicMock() with patch("codeframe.core.agent.Agent") as mock_cls: @@ -194,7 +194,7 @@ def test_supervisor_exception_returns_original_state(self): from codeframe.core.agent import AgentStatus ws = MagicMock() - ws.repo_path = Path("/tmp/test") + ws.repo_path = Path("/tmp") # match adapter.run() path so #787 rebase is a no-op provider = MagicMock() with patch("codeframe.core.agent.Agent") as mock_cls: @@ -211,7 +211,7 @@ def test_failed_triggers_tactical_recovery(self): from codeframe.core.agent import AgentStatus ws = MagicMock() - ws.repo_path = Path("/tmp/test") + ws.repo_path = Path("/tmp") # match adapter.run() path so #787 rebase is a no-op provider = MagicMock() step_result = MagicMock() diff --git a/tests/core/test_sandbox_context.py b/tests/core/test_sandbox_context.py index 7d280620..01e3682f 100644 --- a/tests/core/test_sandbox_context.py +++ b/tests/core/test_sandbox_context.py @@ -112,24 +112,63 @@ def test_task_id_stored(self, tmp_path: Path): class TestCreateExecutionContextWorktree: - """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_worktree_is_rejected(self, git_repo: Path): - with pytest.raises(ValueError, match="worktree isolation is temporarily disabled"): + """Issue #787: worktree isolation is re-enabled for the single-run path with + real merge-back. create_execution_context creates a git worktree, points + workspace_path at it, and wires merge_back / cleanup / preserve hooks.""" + + def test_creates_worktree_dir(self, git_repo: Path): + ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo) + assert ctx.workspace_path == git_repo / ".codeframe" / "worktrees" / "task-abc" + assert ctx.workspace_path.exists() + + def test_merge_back_is_wired(self, git_repo: Path): + ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo) + assert callable(ctx.merge_back) + + def test_cleanup_removes_worktree_and_branch(self, git_repo: Path): + ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo) + assert ctx.workspace_path.exists() + ctx.cleanup() + assert not ctx.workspace_path.exists() + branches = subprocess.run( + ["git", "branch", "--list", "cf/task-abc"], + cwd=str(git_repo), capture_output=True, text=True, + ) + assert "cf/task-abc" not in branches.stdout + + def test_preserve_keeps_worktree_and_branch(self, git_repo: Path): + ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo) + ctx.preserve() + assert ctx.workspace_path.exists() + branches = subprocess.run( + ["git", "branch", "--list", "cf/task-abc"], + cwd=str(git_repo), capture_output=True, text=True, + ) + assert "cf/task-abc" in branches.stdout + + def test_preserved_branch_collision_raises_clear_error(self, git_repo: Path): + """A preserved cf/ branch from a prior run must raise an actionable + ValueError (caught by runtime → handled FAILED), not a raw git error that + strands the run IN_PROGRESS.""" + # Simulate a preserved branch from a previous failed/conflicted run. + subprocess.run( + ["git", "branch", "cf/task-abc"], + cwd=str(git_repo), check=True, capture_output=True, + ) + with pytest.raises(ValueError, match="from a previous run"): create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo) - 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_merge_back_lands_worktree_file_on_base(self, git_repo: Path): + """The core acceptance mechanic: a file written in the worktree exists on + the base branch after auto-commit + merge_back.""" + ctx = create_execution_context("task-abc", IsolationLevel.WORKTREE, git_repo) + (ctx.workspace_path / "agent_output.txt").write_text("from the agent") - 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() + result = ctx.merge_back() + + assert result is not None and result.success is True + assert (git_repo / "agent_output.txt").exists() + assert (git_repo / "agent_output.txt").read_text() == "from the agent" class TestValidateIsolation: @@ -138,11 +177,17 @@ def test_none_is_allowed(self): validate_isolation(IsolationLevel.NONE) # no raise - def test_worktree_raises(self): + def test_worktree_is_allowed(self): + """#787: worktree no longer rejected (single-run path supports it).""" + from codeframe.core.sandbox.context import validate_isolation + + validate_isolation(IsolationLevel.WORKTREE) # no raise + + def test_cloud_raises(self): from codeframe.core.sandbox.context import validate_isolation - with pytest.raises(ValueError, match="#714"): - validate_isolation(IsolationLevel.WORKTREE) + with pytest.raises(NotImplementedError, match="E2B"): + validate_isolation(IsolationLevel.CLOUD) class TestCreateExecutionContextCloud: diff --git a/tests/core/test_worktree_isolation.py b/tests/core/test_worktree_isolation.py new file mode 100644 index 00000000..2422bf37 --- /dev/null +++ b/tests/core/test_worktree_isolation.py @@ -0,0 +1,293 @@ +"""Worktree isolation end-to-end tests (issue #787). + +Covers the single-run path re-enabling `--isolation worktree` with real +merge-back: + - rebased_workspace keeps state on the main repo, code on the worktree (#715/#716) + - TaskWorktree.auto_commit stages+commits worktree changes + - runtime.execute_agent merges agent work back on success (file lands on base), + turns a merge conflict into a blocker and preserves the branch, and preserves + the branch on a failed run. + +Uses a real git repo + real Workspace (no git mocking); the agent adapter is a +fake so behavior is deterministic without an LLM. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from codeframe.core import tasks as tasks_mod +from codeframe.core import blockers as blockers_mod +from codeframe.core import runtime +from codeframe.core.adapters.agent_adapter import AgentResult +from codeframe.core.agent import AgentStatus +from codeframe.core.sandbox.context import rebased_workspace +from codeframe.core.workspace import create_or_load_workspace, Workspace + +pytestmark = pytest.mark.v2 + + +def _git(cwd: Path, *args: str) -> None: + subprocess.run(["git", "-C", str(cwd), *args], check=True, capture_output=True) + + +@pytest.fixture +def git_workspace(tmp_path: Path) -> Workspace: + """A real git repo (branch 'main', one commit) with a CodeFRAME workspace.""" + subprocess.run(["git", "init", "-b", "main"], cwd=str(tmp_path), check=True, capture_output=True) + _git(tmp_path, "config", "user.email", "test@test.com") + _git(tmp_path, "config", "user.name", "Test") + (tmp_path / "README.md").write_text("seed") + _git(tmp_path, "add", "README.md") + _git(tmp_path, "commit", "-m", "init") + return create_or_load_workspace(tmp_path) + + +def _branch_exists(repo: Path, name: str) -> bool: + out = subprocess.run( + ["git", "-C", str(repo), "branch", "--list", name], + capture_output=True, text=True, + ) + return name in out.stdout + + +# --------------------------------------------------------------------------- +# rebased_workspace (#715 / #716 invariant) +# --------------------------------------------------------------------------- + + +class TestRebasedWorkspace: + def test_repo_path_moves_state_stays(self, git_workspace: Workspace): + worktree = git_workspace.repo_path / ".codeframe" / "worktrees" / "t1" + rebased = rebased_workspace(git_workspace, worktree) + # Code root follows the worktree... + assert rebased.repo_path == worktree + # ...but task/blocker/event state stays on the main-repo DB. + assert rebased.state_dir == git_workspace.state_dir + assert rebased.db_path == git_workspace.db_path + + def test_same_path_returns_same_workspace(self, git_workspace: Workspace): + assert rebased_workspace(git_workspace, git_workspace.repo_path) is git_workspace + + +# --------------------------------------------------------------------------- +# auto_commit +# --------------------------------------------------------------------------- + + +class TestAutoCommit: + def test_commits_dirty_worktree(self, git_workspace: Workspace): + from codeframe.core.worktrees import TaskWorktree + + repo = git_workspace.repo_path + wt = TaskWorktree() + wtp = wt.create(repo, "t1", base_branch="main") + (wtp / "new.txt").write_text("hi") + + assert wt.auto_commit(wtp, "t1") is True + # HEAD of the branch now contains the file + log = subprocess.run( + ["git", "-C", str(wtp), "log", "-1", "--name-only", "--format="], + capture_output=True, text=True, + ) + assert "new.txt" in log.stdout + + def test_clean_worktree_is_noop(self, git_workspace: Workspace): + from codeframe.core.worktrees import TaskWorktree + + wt = TaskWorktree() + wtp = wt.create(git_workspace.repo_path, "t1", base_branch="main") + assert wt.auto_commit(wtp, "t1") is False + + def test_commit_failure_raises_not_silent(self, git_workspace: Workspace): + """Regression: a failed commit must raise, not report success — else the + caller merges nothing and cleanup discards the agent's work (#714 class).""" + from codeframe.core.worktrees import TaskWorktree + + repo = git_workspace.repo_path + # A rejecting pre-commit hook lives in the shared .git/hooks (worktrees + # share GIT_COMMON_DIR), so commits in the worktree fail. + hooks = repo / ".git" / "hooks" + hooks.mkdir(parents=True, exist_ok=True) + hook = hooks / "pre-commit" + hook.write_text("#!/bin/sh\nexit 1\n") + hook.chmod(0o755) + + wt = TaskWorktree() + wtp = wt.create(repo, "t1", base_branch="main") + (wtp / "work.txt").write_text("agent work") + + with pytest.raises(RuntimeError, match="git commit failed"): + wt.auto_commit(wtp, "t1") + + +# --------------------------------------------------------------------------- +# runtime.execute_agent worktree wiring +# --------------------------------------------------------------------------- + + +class _FileWritingAdapter: + """Fake external adapter that writes a file into the worktree it's given.""" + + name = "claude-code" + + def __init__(self, filename: str, content: str): + self._filename = filename + self._content = content + + def run(self, task_id, prompt, workspace_path, on_event=None): + (Path(workspace_path) / self._filename).write_text(self._content) + return AgentResult(status="completed", output="done") + + +class _ConflictingAdapter: + """Writes a divergent commit on base AND a conflicting file in the worktree, + forcing a real add/add merge conflict on merge-back.""" + + name = "claude-code" + + def run(self, task_id, prompt, workspace_path, on_event=None): + worktree = Path(workspace_path) + repo = worktree.parents[2] # /.codeframe/worktrees/ + # Divergent change landing on base while the agent worked. + (repo / "conflict.txt").write_text("base version\n") + _git(repo, "add", "conflict.txt") + _git(repo, "commit", "-m", "base change") + # Conflicting change in the worktree. + (worktree / "conflict.txt").write_text("worktree version\n") + return AgentResult(status="completed", output="done") + + +class _FailingAdapter: + name = "claude-code" + + def run(self, task_id, prompt, workspace_path, on_event=None): + (Path(workspace_path) / "partial.txt").write_text("partial work") + return AgentResult(status="failed", error="boom") + + +def _run_with_adapter(git_workspace, adapter): + """Start a run and execute it with a fake external adapter under worktree + isolation, with gates stubbed to pass (isolates the merge-back wiring).""" + task = tasks_mod.create(git_workspace, title="do a thing") + run = runtime.start_task_run(git_workspace, task.id) + + gate_ok = MagicMock() + gate_ok.passed = True + + with patch( + "codeframe.core.engine_registry.get_external_adapter", + return_value=adapter, + ), patch( + "codeframe.core.adapters.verification_wrapper.run_gates", + return_value=gate_ok, + ), patch( + "codeframe.core.context_packager.TaskContextPackager.build", + return_value=MagicMock(prompt="p", context=MagicMock()), + ): + state = runtime.execute_agent( + git_workspace, run, engine="claude-code", isolation="worktree", + ) + return task, state + + +class TestExecuteAgentWorktreeMergeBack: + def test_success_merges_file_to_base_and_cleans_up(self, git_workspace: Workspace): + task, state = _run_with_adapter( + git_workspace, _FileWritingAdapter("agent_output.txt", "hello from agent"), + ) + repo = git_workspace.repo_path + + assert state.status == AgentStatus.COMPLETED + # Acceptance: file created by the adapter exists on the base branch. + assert (repo / "agent_output.txt").exists() + assert (repo / "agent_output.txt").read_text() == "hello from agent" + # Merged → worktree + branch cleaned up. + assert not _branch_exists(repo, f"cf/{task.id}") + assert not (repo / ".codeframe" / "worktrees" / task.id).exists() + # Single-run worktrees are intentionally never registered (so orphan + # cleanup keyed on process liveness can't force-delete a preserved branch). + from codeframe.core.worktrees import list_worktrees + assert list_worktrees(repo) == [] + + def test_conflict_creates_blocker_and_preserves_branch(self, git_workspace: Workspace): + task, state = _run_with_adapter(git_workspace, _ConflictingAdapter()) + repo = git_workspace.repo_path + + # Acceptance: conflict → blocker, branch preserved. + assert state.status == AgentStatus.BLOCKED + task_blockers = blockers_mod.list_for_task(git_workspace, task.id) + assert len(task_blockers) >= 1 + assert _branch_exists(repo, f"cf/{task.id}") + assert (repo / ".codeframe" / "worktrees" / task.id).exists() + + def test_failed_run_preserves_branch(self, git_workspace: Workspace): + task, state = _run_with_adapter(git_workspace, _FailingAdapter()) + repo = git_workspace.repo_path + + # Acceptance: run failure → branch preserved (no silent discard). + assert state.status == AgentStatus.FAILED + assert _branch_exists(repo, f"cf/{task.id}") + + +# --------------------------------------------------------------------------- +# Adapter threading of workspace_path (#715 / #716) +# --------------------------------------------------------------------------- + + +class TestBuiltinAdapterThreadsWorktree: + """#715: the builtin react adapter builds its agent against workspace_path.""" + + def test_react_agent_built_with_worktree_repo_path(self, git_workspace: Workspace): + from codeframe.core.adapters.builtin import BuiltinReactAdapter + + worktree = git_workspace.repo_path / ".codeframe" / "worktrees" / "t1" + adapter = BuiltinReactAdapter(git_workspace, MagicMock()) + + with patch("codeframe.core.react_agent.ReactAgent") as MockAgent: + MockAgent.return_value.run.return_value = AgentStatus.COMPLETED + adapter.run("t1", "", worktree) + + built_ws = MockAgent.call_args.kwargs["workspace"] + assert built_ws.repo_path == worktree + # State DB stays on the main repo. + assert built_ws.db_path == git_workspace.db_path + + def test_none_isolation_uses_original_workspace(self, git_workspace: Workspace): + from codeframe.core.adapters.builtin import BuiltinReactAdapter + + adapter = BuiltinReactAdapter(git_workspace, MagicMock()) + with patch("codeframe.core.react_agent.ReactAgent") as MockAgent: + MockAgent.return_value.run.return_value = AgentStatus.COMPLETED + adapter.run("t1", "", git_workspace.repo_path) + + assert MockAgent.call_args.kwargs["workspace"] is git_workspace + + +class TestVerificationWrapperThreadsWorktree: + """#716: gates run against the worktree, blockers against the main repo.""" + + def test_gates_run_against_worktree_path(self, git_workspace: Workspace): + from codeframe.core.adapters.verification_wrapper import VerificationWrapper + + worktree = git_workspace.repo_path / ".codeframe" / "worktrees" / "t1" + inner = MagicMock() + inner.name = "fake" + inner.run.return_value = AgentResult(status="completed", output="ok") + wrapper = VerificationWrapper(inner, git_workspace) + + gate_ok = MagicMock() + gate_ok.passed = True + with patch( + "codeframe.core.adapters.verification_wrapper.run_gates", + return_value=gate_ok, + ) as mock_gates: + wrapper.run("t1", "prompt", worktree) + + gate_ws = mock_gates.call_args.args[0] + assert gate_ws.repo_path == worktree + assert gate_ws.db_path == git_workspace.db_path