feat(isolation): re-enable worktree isolation with real merge-back (#787)

[frankbria]

Summary

Re-enables --isolation worktree for the single-run path (cf work start)
with real merge-back, reversing the interim hard-reject from #714 (#786). Closes #787.

Each worktree run now:

• runs the agent + verification gates inside the per-task worktree while task/
  blocker/event state stays in the main repo's .codeframe DB, and
• on success, auto-commits the worktree and merges cf/<task_id> back to
  the base branch; on conflict or failure the branch is preserved for recovery
  (never silently discarded — the [P0.3] Worktree isolation must not silently discard external-engine work (merge_back is dead code) #714 bug).

Scope decision (single-run only)

Batch (cf work batch run --isolation worktree) stays rejected at the CLI.
The batch path spawns a subprocess with cwd=worktree, and .codeframe/ is
gitignored, so a child can't reach the main task DB from a worktree. Solving that
is a separate follow-up; this PR delivers every acceptance criterion in #787,
which are all single-run.

How it works

rebased_workspace(ws, worktree) = dataclasses.replace(ws, repo_path=worktree) —
code I/O + gates follow the worktree; state_dir/db_path stay on main. Merge-back
is driven by runtime.execute_agent from the terminal run status.

Changes

• core/worktrees.py: TaskWorktree.auto_commit() — stage + commit worktree changes.
• core/sandbox/context.py: rebased_workspace(); ExecutionContext gains
  merge_back/preserve; validate_isolation no longer rejects WORKTREE (CLOUD
  still raises); create_execution_context builds a real worktree + wires hooks
  (intentionally not registered in WorktreeRegistry so orphan cleanup can't
  nuke a preserved branch).
• core/adapters/builtin.py ([P0.4] Builtin (react/plan) engines ignore the worktree workspace_path — isolation is a silent no-op #715): react/plan adapters build the agent against
  the worktree via rebased_workspace.
• core/adapters/verification_wrapper.py ([P0.5] VerificationWrapper runs gates/quick-fixes against the main repo, not the worktree the agent modified #716): gates + quick-fixes run against
  the worktree; blockers still land in the main-repo DB.
• core/runtime.py: merge-back on success; conflict → blocker + downgrade to
  BLOCKED + preserve; failure/blocked/exception → preserve.
• cli/app.py: work start accepts worktree; work batch run explicitly rejects
  it with a batch-specific message.

Tests

• test_worktree_isolation.py (new): rebased_workspace invariant, auto_commit,
  and end-to-end execute_agent — file lands on base after success + branch
  cleaned, conflict → blocker + branch preserved, failure → branch preserved;
  plus [P0.4] Builtin (react/plan) engines ignore the worktree workspace_path — isolation is a silent no-op #715/[P0.5] VerificationWrapper runs gates/quick-fixes against the main repo, not the worktree the agent modified #716 adapter-threading unit tests.
• test_sandbox_context.py / test_work_exit_codes.py: updated from the [P0.3] Worktree isolation must not silently discard external-engine work (merge_back is dead code) #714
  disable contract to the [P2.24] Re-enable worktree isolation with real merge-back (+auto-commit, workspace_path, verify) #787 enable contract.

Review hardening (post-review, in this PR)

Two independent reviews (opencode/GLM + CI bots) converged on real defects, all fixed here:

• auto_commit silent data-loss (critical): git add/commit now fail loud —
  a failed stage/commit raises → run preserved, never merged-then-deleted.
• Preserved-branch collision on retry (major): worktree creation moved inside
  execute_agent's try + a clear actionable error, so a leftover cf/<task> yields
  a handled FAILED (branch preserved), not a stranded IN_PROGRESS run.
• engine_stats miscount (major): agent_status re-synced to the final state so a
  merge-back conflict is recorded as BLOCKED, not COMPLETED.
• Concurrent merge-back on the shared main tree (major): serialized with a
  cross-process flock (_main_tree_lock).
• Dirty-tree checkout failure now routes through the merge-back error → blocker
  • preserve (via the try/except around merge_back()), matching the docs.

Known limitations

• Batch worktree isolation still rejected (see scope above).
• Merge-back checks out the base branch in the main working tree. A concurrent
  merge on the same repo is serialized (flock); a dirty/conflicting main tree
  surfaces as a blocker with the branch preserved rather than merging. Clean,
  sequential cf work start runs (the norm) are unaffected.

[github-actions]

🔍 Precision bug-hunting review — 2 defects found

Scoped to concrete defects only (logic/data-loss/error-path). Style, naming, architecture, and test coverage are covered by the general review and intentionally not repeated here.

Note: the inline-comment endpoint returned a transient GitHub 500, so both findings are consolidated here instead of posted as inline review comments.

severity	file:line	finding
critical	codeframe/core/worktrees.py:112 (auto_commit)	git commit (and git add -A) run without check/return-code inspection; a failed commit is reported as success → silent, permanent loss of the agent's completed work on cleanup
major	codeframe/core/sandbox/context.py:155 (_create_worktree_context)	a preserved cf/<task> branch from a prior failed/conflicted run makes worktree.create() raise on retry; the exception escapes execute_agent and strands the run IN_PROGRESS

---

1. auto_commit swallows commit failures → silent data loss (critical)

codeframe/core/worktrees.py:97-119 runs git add -A and git commit with capture_output=True and no check/return-code inspection. If the commit fails, auto_commit still logs "Auto-committed …" and returns True.

Failure scenario: in any environment without a configured git identity (no user.name/user.email — the default in many CI runners, containers, and fresh installs; existing repo history does not supply it), the agent completes and edits files in the worktree. git add -A stages them; git diff --cached --quiet returns 1, so the code proceeds to git commit, which fails with "Author identity unknown". The nonzero return code is swallowed and auto_commit returns True. merge_back then runs git merge cf/<task> which is "Already up to date" (no new commit on the branch) → success=True, so in runtime.execute_agent worktree_merged=True and the finally block calls cleanup() — git worktree remove --force destroys the worktree (with the still-uncommitted index) and git branch -D deletes the branch. Net result: run marked COMPLETED, base branch unchanged, the agent's file edits permanently destroyed. This is the exact silent-data-loss regression #787 exists to prevent. (A failing pre-commit hook produces the same outcome; an git add -A failure has the analogous "returns False ⇒ nothing to merge ⇒ cleanup destroys the worktree" path.)

Suggested fix (add check=True; same for git add -A):

         subprocess.run(
             ["git", "commit", "-m", f"cf: auto-commit worktree changes for {task_id}"],
             cwd=str(worktree_path),
             capture_output=True,
             text=True,
+            check=True,
         )

With check=True, a commit/add failure raises and propagates to execute_agent's outer except Exception (runtime.py:943), which marks the run FAILED and — because worktree_merged stays False — calls preserve(), keeping the worktree and branch for recovery instead of destroying them.

2. Preserved branch makes worktree.create() crash on retry → stranded run (major)

_create_worktree_context (context.py:155) calls worktree.create(), which does git worktree add -b cf/<task_id> … <base> with check=True. A preserved cf/<task_id> from a prior run (the PR's own preserve() path on failure/conflict) makes this raise on the next attempt.

Failure scenario: Run 1 ends via preserve() (agent failure or merge conflict) → the cf/T branch and .codeframe/worktrees/T are intentionally left on disk. The user retries cf work start T --execute --isolation worktree. create_execution_context (runtime.py:715) runs before the try: at runtime.py:723, so _create_worktree_context → worktree.create() → git worktree add -b cf/T … fails with "fatal: a branch named 'cf/T' already exists" (and/or the worktree dir already exists). The CalledProcessError is not caught by runtime's try/except/finally (no fail_run; finally never runs) nor by the CLI's except ValueError (cli/app.py:2572 — CalledProcessError is not a ValueError), so it surfaces as a raw traceback and run T is left IN_PROGRESS. Every subsequent retry fails identically until cf/T and the worktree dir are deleted by hand. This is a regression specific to this PR's preserve() design — previously WORKTREE was hard-rejected before any branch existed.

The fix is not local to one line: either move create_execution_context inside the try (so a collision becomes a handled failure that fails + preserves, rather than an escaped exception), or have _create_worktree_context detect an existing preserved cf/<task_id> and reuse/reset it explicitly instead of letting git worktree add -b crash.

---

Non-issues verified (traced against real code, discarded): rebased_workspace is safe — Workspace.state_dir is an independent field and db_path derives from state_dir (not repo_path), so the main DB stays put. The "dirty main tree" case is handled correctly: git checkout base_branch is a no-op (already on it), and a conflicting dirty tree fails at git merge (returncode ≠ 0), which routes through the conflict → blocker → preserve() path as the PR describes.

Review complete — no files modified, no commits pushed.

View job run

[claude]

Review: #867 — re-enable worktree isolation with real merge-back

Solid design overall — rebased_workspace cleanly separates "code root" from "state root," and the merge/preserve/cleanup wiring in runtime.execute_agent correctly avoids the #714 silent-discard bug for the success/failure/conflict paths that are actually tested (tests/core/test_worktree_isolation.py covers all three well with a real git repo + fake adapter). A few things worth addressing before/soon after merge:

Major — merge-back races on the shared main working tree

TaskWorktree.merge_back() (codeframe/core/worktrees.py:121-188) runs git checkout base_branch then git merge directly in repo_path — the single shared main working directory — with no locking anywhere in this path. Two cf work start --execute --isolation worktree invocations for different tasks run concurrently (e.g. two terminals against the same repo, which is exactly the workflow this feature is meant to make safe) will interleave checkouts/merges in that same directory. That can corrupt the working tree, land a merge on the wrong base commit, or produce spurious conflicts — undermining the isolation this feature is supposed to provide. Worth a lock around merge-back (e.g. a lockfile keyed on repo_path, similar to the existing _registry_lock pattern in the same module) or at least an explicit documented caveat that concurrent single-run worktree executions against the same repo aren't safe yet.

Major — merge conflict downgrades state.status but not agent_status, so metrics/cost stats end up wrong

In codeframe/core/runtime.py (~lines 819-903): agent_status is computed once from result.status before merge-back runs, and is what's later passed to engine_stats.record_run(status=agent_status.value.upper(), ...). When merge-back conflicts, state is correctly reassigned to AgentState(status=AgentStatus.BLOCKED) (and the blocker/run-status transition uses state.status correctly), but agent_status still holds COMPLETED. So a worktree run that conflicts on merge-back gets permanently recorded in engine_stats (and by extension the Costs page, per Phase 5.2) as COMPLETED even though the task is actually BLOCKED. Suggest updating agent_status alongside state.status in the conflict branch (or just deriving the metrics status from state.status at that point).

Moderate — a dirty main tree doesn't actually produce the documented "conflict blocker"

The PR description's "Known limitations" says a dirty main tree "surfaces as a conflict blocker (branch preserved)." But merge_back()'s git checkout base_branch call uses check=True; if the main tree has conflicting local changes, that checkout raises CalledProcessError, which isn't caught by _merge_back() or by the merge-conflict handling in runtime.py (lines ~843-860) — it propagates to the outer except Exception at line ~943, so the run instead fails as a generic FAILED with an "Unhandled error" log rather than a blocker carrying conflict details. No data loss (the finally block still preserves the worktree/branch since worktree_merged stays False), but the user experience doesn't match what's documented. Might be worth catching the checkout failure explicitly and routing it through the same blocker path as a real merge conflict, or correcting the docs.

Minor — stale docstring example

work_batch_run's docstring in cli/app.py still lists codeframe work batch run --all-ready --isolation worktree as an example, even though this exact function now explicitly rejects --isolation worktree with a dedicated error message. This line predates the PR (it was already stale under the old #714 hard-reject), but since this PR touches this function's rejection logic directly, it'd be a good time to fix the example.

Minor — merge-back's checkout can silently switch the main tree's branch

base_branch is captured once at worktree creation via get_base_branch(repo_path). If anything changes the main tree's checked-out branch between worktree creation and merge-back, merge_back() will silently check it back out to base_branch with no restoration afterward. Not destructive on its own (a real conflicting dirty tree fails per the point above), but worth calling out beyond "a dirty tree conflicts" — the main tree shouldn't be touched at all while a worktree run is in flight.

Nits

• No test covers the _create_worktree_context docstring's claim that the worktree is intentionally not registered in WorktreeRegistry (i.e. asserting list_worktrees() stays empty after a worktree run) — worth a small regression test given that's explicitly why orphan cleanup can't nuke a preserved branch.

Test coverage for the three core scenarios (success/conflict/failure) is good. The gaps above (concurrent runs, dirty-main-tree checkout failure, engine-stats mismatch) aren't covered, which is consistent with them slipping through.

[frankbria]

Review response — all Critical/Major findings addressed (commit acc06b4)

Thanks to both reviews (opencode/GLM + CI bots) — they converged on the same real defects.

Finding	Severity	Resolution
auto_commit swallows git failures → silent data loss	critical	Fixed. git add/git commit now inspect return codes and raise on failure; the run then preserves the branch instead of merging-nothing-then-deleting. Regression test TestAutoCommit::test_commit_failure_raises_not_silent.
Preserved cf/<task> branch crashes worktree.create() on retry → stranded IN_PROGRESS	major	Fixed. create_execution_context moved inside execute_agent's try (a failure → handled FAILED + preserve, never stranded), plus a clear actionable ValueError on collision. Test test_preserved_branch_collision_raises_clear_error.
Merge-back races on the shared main working tree	major	Fixed. merge_back's checkout+merge is serialized with a cross-process flock (_main_tree_lock).
Merge conflict downgrades state.status but not agent_status → engine_stats records COMPLETED	major	Fixed. agent_status re-synced to state.status before engine_stats.record_run.
Dirty main tree → generic FAILED, not the documented blocker	moderate	Fixed. The try/except around merge_back() catches the checkout CalledProcessError → blocker + preserve. Docs updated to match.
Stale --isolation worktree example in batch docstring	minor	Removed.
No test that the worktree stays out of WorktreeRegistry	nit	Added list_worktrees(repo) == [] assertion to the success test.

Verified non-issues (agreed): rebased_workspace keeps state on the main DB (db_path derives from the untouched state_dir); no double-apply path.

[frankbria]

Demo — #787 acceptance criteria → outcome evidence

This is a CLI/core feature (no web surface); the evidence is the integration suite
exercising real git worktrees + a real Workspace (fake adapter for determinism),
asserting real filesystem/git state — not "exited 0".

Acceptance criterion	Proving test	Outcome asserted
Success → uncommitted changes auto-committed on cf/<task_id>, then merge_back() merges to base before cleanup	TestExecuteAgentWorktreeMergeBack::test_success_merges_file_to_base_and_cleans_up	adapter's file exists on the base branch + cf/<task> branch & worktree removed + not registered
Merge conflict → blocker; branch/worktree preserved	test_conflict_creates_blocker_and_preserves_branch	run BLOCKED, a blocker exists for the task, cf/<task> branch + worktree dir still present
Run failure/blocked → branch preserved (no branch -D)	test_failed_run_preserves_branch	run FAILED, cf/<task> branch still present
validate_isolation() no longer rejects WORKTREE; CLI re-accepts --isolation worktree	test_work_start_accepts_worktree, TestValidateIsolation::test_worktree_is_allowed	cf work start --isolation worktree exits 0 + task IN_PROGRESS
Integration: adapter file under --isolation worktree exists on base after success	(same as row 1)	file content verified on base branch
Regression: conflicting change → blocker + preserved branch	(same as row 2)	real add/add git conflict → blocker + preserved
#715 builtin adapters thread workspace_path	TestBuiltinAdapterThreadsWorktree::*	agent built with repo_path==worktree, db_path==main
#716 gates run against the worktree	TestVerificationWrapperThreadsWorktree::test_gates_run_against_worktree_path	run_gates workspace repo_path==worktree, db_path==main
Data-loss guard (auto_commit fails loud)	TestAutoCommit::test_commit_failure_raises_not_silent	failing commit raises (→ preserve), never reports success
Preserved-branch collision handled	test_preserved_branch_collision_raises_clear_error	clear ValueError (→ handled FAILED, not stranded)

[claude]

Follow-up review — verified the hardening commit (acc06b4)

I re-read the full base→head diff (codeframe/core/worktrees.py, sandbox/context.py, runtime.py, cli/app.py, adapters, and the new/updated tests) against my prior review and the CI bot's review on this PR. All five Critical/Major/Moderate findings are fixed correctly, not just cosmetically:

• auto_commit silent data loss (critical) — git add/git commit now check returncode and raise RuntimeError (worktrees.py:140-167). A raise from _merge_back() is caught in runtime.py:855-874, turned into a blocker, and routes to exec_ctx.preserve() in the finally (runtime.py:1005-1009) instead of cleanup(). Regression test test_commit_failure_raises_not_silent exercises this with a real rejecting pre-commit hook — good, that's a more realistic repro than a missing git identity.
• Preserved-branch collision stranding IN_PROGRESS (major) — create_execution_context moved inside execute_agent's try (runtime.py:719-726), and _create_worktree_context now pre-checks for a leftover cf/<task_id> branch or worktree dir and raises a clear ValueError (context.py, ~30 lines added) instead of letting git worktree add throw a raw CalledProcessError. Since exec_ctx stays None when this raises, the finally correctly does nothing (nothing was created this attempt) and the outer except Exception marks the run FAILED. Covered by test_preserved_branch_collision_raises_clear_error.
• Concurrent merge-back on the shared main tree (major) — _main_tree_lock (worktrees.py:39-59) wraps checkout+merge+abort in a flock on .git/cf-merge.lock. Confirmed the lock is released correctly on both the success and conflict-abort return paths, and that it uses LOCK_EX on a real fd (flock is per-open-file-description, so this actually serializes across threads and processes, not just processes — better than what the docstring undersells).
• agent_status/engine_stats mismatch (major) — agent_status = state.status re-sync now happens right before engine_stats.record_run (runtime.py:896-899), after any merge-back downgrade to BLOCKED. Confirmed engine_stats.py already accepts BLOCKED as a valid status value, so this is a complete fix, not just a type-compatible one.
• Dirty-tree checkout failure → generic FAILED instead of documented blocker (moderate) — now caught by the same try/except Exception around exec_ctx.merge_back(), producing a blocker + preserve. Matches the "Known limitations" wording in the updated PR description.

The one minor item from my first pass — merge_back()'s git checkout base_branch can silently switch the main tree's branch with no restoration — is still open. That's consistent with the author's response (only Critical/Major/Moderate were in scope for this follow-up), and it's genuinely low severity given the dirty-tree case is now handled via the blocker path. Not blocking.

Two small things worth a look (optional, not blocking)

1. No regression test pins the engine_stats fix. test_conflict_creates_blocker_and_preserves_branch in tests/core/test_worktree_isolation.py verifies the run ends BLOCKED and the branch is preserved, but doesn't assert what engine_stats recorded. Since that table is now user-facing (Costs page, Phase 5.2), a one-line query against engine_stats after the conflict test would lock in the exact bug this commit fixes and prevent a silent regression later.
2. Cosmetic: runtime.py:904-907 logs f"Agent blocked: {result.blocker_question or 'Unknown'}" for all BLOCKED outcomes, including the new merge-conflict path — since result.blocker_question is never set there, the log line always reads "Agent blocked: Unknown" even though the real blocker (with the actual conflict details) was created correctly. Doesn't affect behavior, just makes the log line less useful for this new path.

No new correctness, security, or data-loss issues found in the hardening commit itself — subprocess calls throughout use list-form args (no shell=True), so no injection surface. Test coverage for the three core scenarios (success/conflict/failure) plus the two regressions is solid and uses a real git repo rather than mocks, which is the right call for this kind of state-machine-over-git logic.