feat(core): ExecutionContext abstraction for task isolation (#532) - #536
Conversation
Adds IsolationLevel enum and ExecutionContext dataclass in a new codeframe/core/sandbox/ package, enabling agents to run in isolated git worktrees instead of the shared filesystem. - New codeframe/core/sandbox/context.py with IsolationLevel (none|worktree|cloud), ExecutionContext dataclass, and create_execution_context() factory - Uses existing TaskWorktree (worktrees.py) for WORKTREE isolation - --isolation flag wired into cf work start and cf work batch run - conductor.py creates context before each task dispatch, cleans up after - runtime.execute_agent() creates context, passes effective_repo_path to adapter - BatchRun.isolation: str = "none" field with DB schema migration - 16 tests covering context lifecycle, NONE no-op, WORKTREE create/cleanup, CLOUD NotImplementedError - Default none preserves all existing behavior
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughIntroduces an ExecutionContext abstraction and an Changes
Sequence DiagramsequenceDiagram
participant User as User
participant CLI as CLI (app.py)
participant Runtime as Runtime (execute_agent)
participant Sandbox as Sandbox (create_execution_context)
participant Adapter as Adapter (agent)
participant Cleanup as Cleanup
User->>CLI: cf work start --isolation worktree
CLI->>Runtime: execute_agent(isolation="worktree", run)
Runtime->>Sandbox: create_execution_context(run.task_id, WORKTREE, repo_path)
Sandbox-->>Runtime: ExecutionContext(workspace_path=worktree_path, cleanup)
Runtime->>Adapter: adapter.run(workspace_path=worktree_path)
Adapter-->>Runtime: result
Runtime->>Cleanup: exec_ctx.cleanup()
Cleanup-->>Runtime: cleanup complete
Runtime-->>User: agent state/result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Review: ExecutionContext abstraction (#532)The abstraction design is clean and well-structured. A few issues worth addressing before merge: Critical: WORKTREE isolation in batch runs likely brokenThe But the subprocess runs The Inline imports repeated 4x in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/cli/app.py (1)
3490-3566:⚠️ Potential issue | 🟡 Minor
work batch runstill needs the required--verbose/-vflag.This command exposes
--engineand now--isolation, but it still has no supported way to request detailed output for batch executions from the CLI surface itself.As per coding guidelines, "All CLI commands must support the
--engineflag for engine selection (default: react, legacy option: plan) and--verbose/-vflag for detailed output."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/cli/app.py` around lines 3490 - 3566, The batch_run command handler lacks the required verbose flag; update the batch_run function signature to add a verbose: bool parameter exposed as a Typer/Click Option (flags --verbose and -v, default False, help="Enable detailed output") and thread that flag into whatever logging/execution path currently uses engine/isolation (e.g., pass verbose into the batch executor or set process/log level where engine or isolation are used). Ensure the new option name is consistent with existing options and does not conflict with engine or isolation parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/core/conductor.py`:
- Around line 557-559: Validate the isolation parameter before persisting the
batch: in the function that takes isolate: bool and isolation: str -> BatchRun,
check that isolation is one of the allowed values (e.g., "none", "host",
"container", "vm" or your project's canonical set) and raise a clear validation
error if not, before calling create_execution_context(...) or saving/moving the
batch to RUNNING; apply the same preflight check for the similar block around
create_execution_context usage at the other location referenced (the code
between the earlier create_execution_context call and the block at 598-615) so
no batch is persisted when isolation is invalid.
- Around line 2146-2149: The SELECTs that populate rows for _row_to_batch() are
missing the isolation column so loaded batches default to "none"; update
get_batch() and list_batches() (and any other queries that fetch batch_runs rows
used by _row_to_batch(), e.g., resume_batch related queries) to include the
isolation column in the SELECT list in the same order as the INSERT (id,
workspace_id, task_ids, status, strategy, max_parallel, on_failure, started_at,
completed_at, results, engine, isolation) so _row_to_batch() reads isolation
from row[11] correctly.
- Around line 1020-1030: The worktree created by create_execution_context
(ExecutionContext / exec_ctx) is torn down in the finally block via
exec_ctx.cleanup() without harvesting changes, so any edits under
exec_ctx.workspace_path are deleted; before calling exec_ctx.cleanup() (after
_execute_task_subprocess returns success or result_status is available) detect
when exec_ctx.workspace_path != workspace.repo_path and persist the worktree
edits back into the main workspace (e.g., by merging/patching/copying changed
files or invoking a commit/merge method on ExecutionContext); add or call a
clearly named method such as exec_ctx.harvest_changes() or
exec_ctx.merge_back_to(workspace.repo_path) prior to cleanup, and ensure
_execute_task_subprocess and any agent tool functions remain stateless and write
only to the provided workspace_path; apply the same harvest-before-cleanup fix
at the other similar locations referenced (around the other ExecutionContext
usages).
In `@codeframe/core/runtime.py`:
- Around line 679-685: The HookContext and hook invocations are being built with
the shared repo path before the execution context is created, which breaks
isolation; move the create_execution_context(...) call (using
IsolationLevel(isolation) and workspace.repo_path) so exec_ctx =
create_execution_context(...) runs before constructing HookContext and before
any execute_hook(...) calls, then use exec_ctx.workspace_path (or
effective_repo_path) as the repo path when building HookContext and when calling
execute_hook(...) so hooks operate against the execution context workspace
rather than the main checkout; keep the same create_execution_context,
IsolationLevel, HookContext, execute_hook, and exec_ctx symbols when editing.
- Around line 602-603: The isolation/worktree setup (create_execution_context)
must be moved into the guarded execution path so failures don't bypass
fail_run(); change execute_agent so exec_ctx is declared/initialized to None
before the try, then call create_execution_context(...) inside the try block
(e.g., where execution begins) and only perform cleanup if exec_ctx is not None;
apply same pattern for the other occurrences that initialize execution contexts
(the blocks around create_execution_context and cleanup mentioned near lines
679-685 and 877-878), and ensure fail_run() still runs on exceptions.
In `@tests/core/test_sandbox_context.py`:
- Around line 23-48: The git_repo fixture creates a repo that may default to
"master", but TaskWorktree.create defaults to base_branch="main", causing tests
to fail; update the git_repo fixture to rename the current branch to "main"
after the initial commit (e.g., run a git branch -M main or equivalent
subprocess.run call) so the repo has a "main" branch available for
TaskWorktree.create and the WORKTREE tests.
---
Outside diff comments:
In `@codeframe/cli/app.py`:
- Around line 3490-3566: The batch_run command handler lacks the required
verbose flag; update the batch_run function signature to add a verbose: bool
parameter exposed as a Typer/Click Option (flags --verbose and -v, default
False, help="Enable detailed output") and thread that flag into whatever
logging/execution path currently uses engine/isolation (e.g., pass verbose into
the batch executor or set process/log level where engine or isolation are used).
Ensure the new option name is consistent with existing options and does not
conflict with engine or isolation parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d472ceb3-3bf1-44d5-acd4-7a5fb5224bed
📒 Files selected for processing (6)
codeframe/cli/app.pycodeframe/core/conductor.pycodeframe/core/runtime.pycodeframe/core/sandbox/__init__.pycodeframe/core/sandbox/context.pytests/core/test_sandbox_context.py
| isolate: bool = True, | ||
| isolation: str = "none", | ||
| ) -> BatchRun: |
There was a problem hiding this comment.
Validate isolation before the batch is persisted.
isolation is stored as a raw string here and only converted inside the execution loops. If callers pass "cloud" or any invalid value, the first create_execution_context(...) raises after the batch has already been saved and moved to RUNNING, which leaves a half-started batch instead of a clean validation error.
🛠️ Suggested preflight validation
def start_batch(
@@
if not task_ids:
raise ValueError("task_ids cannot be empty")
+
+ from codeframe.core.sandbox.context import IsolationLevel
+
+ try:
+ isolation_level = IsolationLevel(isolation)
+ except ValueError as exc:
+ raise ValueError(f"Unsupported isolation level: {isolation}") from exc
+ if isolation_level is IsolationLevel.CLOUD:
+ raise ValueError(
+ "Isolation level 'cloud' is not implemented yet. Use 'none' or 'worktree'."
+ )
@@
concurrency=concurrency,
isolate=isolate,
- isolation=isolation,
+ isolation=isolation_level.value,
)Also applies to: 598-615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/conductor.py` around lines 557 - 559, Validate the isolation
parameter before persisting the batch: in the function that takes isolate: bool
and isolation: str -> BatchRun, check that isolation is one of the allowed
values (e.g., "none", "host", "container", "vm" or your project's canonical set)
and raise a clear validation error if not, before calling
create_execution_context(...) or saving/moving the batch to RUNNING; apply the
same preflight check for the similar block around create_execution_context usage
at the other location referenced (the code between the earlier
create_execution_context call and the block at 598-615) so no batch is persisted
when isolation is invalid.
| # Execute task via subprocess (with isolation context) | ||
| from codeframe.core.sandbox.context import IsolationLevel, create_execution_context | ||
| exec_ctx = create_execution_context(task_id, IsolationLevel(batch.isolation), workspace.repo_path) | ||
| try: | ||
| result_status = _execute_task_subprocess( | ||
| workspace, task_id, batch.id, engine=batch.engine, | ||
| stall_timeout_s=batch.stall_timeout_s, stall_action=batch.stall_action, | ||
| worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None, | ||
| ) | ||
| finally: | ||
| exec_ctx.cleanup() |
There was a problem hiding this comment.
worktree runs currently delete their own outputs.
These paths tear the ExecutionContext down in finally, but there is no merge/copy/patch step before cleanup. Because task tools write against exec_ctx.workspace_path, a successful WORKTREE run leaves its edits in the task worktree and then deletes them here, so the batch can report success while workspace.repo_path stays unchanged. Harvest the worktree changes before cleanup, or keep the worktree around until a caller has done so.
Based on learnings, "agent tool functions ... must be stateless with signature (input_data: dict, workspace_path: Path, tool_call_id: str) -> ToolResult".
Also applies to: 1197-1207, 1426-1448, 1840-1870, 1960-1976
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/conductor.py` around lines 1020 - 1030, The worktree created
by create_execution_context (ExecutionContext / exec_ctx) is torn down in the
finally block via exec_ctx.cleanup() without harvesting changes, so any edits
under exec_ctx.workspace_path are deleted; before calling exec_ctx.cleanup()
(after _execute_task_subprocess returns success or result_status is available)
detect when exec_ctx.workspace_path != workspace.repo_path and persist the
worktree edits back into the main workspace (e.g., by merging/patching/copying
changed files or invoking a commit/merge method on ExecutionContext); add or
call a clearly named method such as exec_ctx.harvest_changes() or
exec_ctx.merge_back_to(workspace.repo_path) prior to cleanup, and ensure
_execute_task_subprocess and any agent tool functions remain stateless and write
only to the provided workspace_path; apply the same harvest-before-cleanup fix
at the other similar locations referenced (around the other ExecutionContext
usages).
| INSERT OR REPLACE INTO batch_runs | ||
| (id, workspace_id, task_ids, status, strategy, max_parallel, on_failure, | ||
| started_at, completed_at, results, engine) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| started_at, completed_at, results, engine, isolation) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) |
There was a problem hiding this comment.
Loaded batches always fall back to isolation="none".
_row_to_batch() now expects row[11], but get_batch() and list_batches() still select only ... results, engine from batch_runs. Every reloaded batch therefore drops its stored isolation, so resume_batch() re-executes on the shared repo even if the original batch used worktree.
💾 Suggested query fix
SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
- on_failure, started_at, completed_at, results, engine
+ on_failure, started_at, completed_at, results, engine, isolation
FROM batch_runs
@@
SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
- on_failure, started_at, completed_at, results, engine
+ on_failure, started_at, completed_at, results, engine, isolation
FROM batch_runs
@@
SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
- on_failure, started_at, completed_at, results, engine
+ on_failure, started_at, completed_at, results, engine, isolation
FROM batch_runsAlso applies to: 2162-2163, 2171-2185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/conductor.py` around lines 2146 - 2149, The SELECTs that
populate rows for _row_to_batch() are missing the isolation column so loaded
batches default to "none"; update get_batch() and list_batches() (and any other
queries that fetch batch_runs rows used by _row_to_batch(), e.g., resume_batch
related queries) to include the isolation column in the SELECT list in the same
order as the INSERT (id, workspace_id, task_ids, status, strategy, max_parallel,
on_failure, started_at, completed_at, results, engine, isolation) so
_row_to_batch() reads isolation from row[11] correctly.
| isolation: str = "none", | ||
| ) -> "AgentState": |
There was a problem hiding this comment.
Move isolation setup under the guarded execution path.
create_execution_context() runs before the main try, so --isolation cloud (currently NotImplementedError) or any worktree setup failure exits execute_agent() before fail_run() runs. That leaves the run/task stuck active even though execution never started. Initialize exec_ctx first, create the context inside the try, and only clean it up when setup succeeded.
🛠️ Suggested fix
- # 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
- )
- effective_repo_path = exec_ctx.workspace_path
-
- try:
+ # Create execution context (handles isolation; NONE is a no-op)
+ from codeframe.core.sandbox.context import IsolationLevel, create_execution_context
+ exec_ctx = None
+ effective_repo_path = workspace.repo_path
+
+ try:
+ exec_ctx = create_execution_context(
+ run.task_id, IsolationLevel(isolation), workspace.repo_path
+ )
+ effective_repo_path = exec_ctx.workspace_path- exec_ctx.cleanup()
+ if exec_ctx is not None:
+ exec_ctx.cleanup()Also applies to: 679-685, 877-878
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/runtime.py` around lines 602 - 603, The isolation/worktree
setup (create_execution_context) must be moved into the guarded execution path
so failures don't bypass fail_run(); change execute_agent so exec_ctx is
declared/initialized to None before the try, then call
create_execution_context(...) inside the try block (e.g., where execution
begins) and only perform cleanup if exec_ctx is not None; apply same pattern for
the other occurrences that initialize execution contexts (the blocks around
create_execution_context and cleanup mentioned near lines 679-685 and 877-878),
and ensure fail_run() still runs on exceptions.
| # 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 | ||
| ) | ||
| effective_repo_path = exec_ctx.workspace_path | ||
|
|
There was a problem hiding this comment.
Create the context before building hook state.
Because the execution context is created after HookContext is assembled, Line 673 and the execute_hook(..., workspace.repo_path, ...) calls still point at the shared repo. With worktree isolation, before_task / after_task_* hooks can read or mutate the main checkout while the agent runs against the worktree, which breaks task isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/runtime.py` around lines 679 - 685, The HookContext and hook
invocations are being built with the shared repo path before the execution
context is created, which breaks isolation; move the
create_execution_context(...) call (using IsolationLevel(isolation) and
workspace.repo_path) so exec_ctx = create_execution_context(...) runs before
constructing HookContext and before any execute_hook(...) calls, then use
exec_ctx.workspace_path (or effective_repo_path) as the repo path when building
HookContext and when calling execute_hook(...) so hooks operate against the
execution context workspace rather than the main checkout; keep the same
create_execution_context, IsolationLevel, HookContext, execute_hook, and
exec_ctx symbols when editing.
|
Follow-up Review: The latest commit only touches tests/core/test_sandbox_context.py with trivial changes. None of the critical issues from the previous review round have been addressed. Still open: (1) isolation column missing from SELECT queries in conductor.py - _row_to_batch reads row[11] but queries return only 11-element tuples, silently defaulting batch isolation to none on reload. (2) create_execution_context called outside try block in runtime.execute_agent() - a worktree creation failure leaves runs stuck in IN_PROGRESS. (3) No merge_back/harvest before cleanup() - worktree output is silently discarded. (4) Batch subprocess worktree not propagated - _execute_task_subprocess sets cwd=worktree_path but subprocess agent resolves workspace from DB repo_path; --isolation not forwarded in command array. (5) Bare except Exception: pass in DB migration swallows all errors. Suggested fix order matches CodeRabbit item order: SELECT queries first (data loss), then try-block fix, then merge-back, then migration guard, then subprocess forwarding. |
1 participant
Summary
IsolationLevelenum andExecutionContextdataclass in newcodeframe/core/sandbox/package--isolation none|worktree|cloudflag oncf work startandcf work batch runconductor.pycreates context before each task dispatch, callscleanup()in finally blockruntime.execute_agent()creates context, passeseffective_repo_pathto adaptersTaskWorktreefromworktrees.pyfor WORKTREE isolation (reuses tested git logic)Test plan
tests/core/test_sandbox_context.py— 16 tests: IsolationLevel enum, ExecutionContext dataclass, NONE no-op, WORKTREE create/cleanup with real git repo, CLOUD raises NotImplementedErrortests/core/test_conductor.py— 63 existing tests pass (BatchRun backward compat verified)uv run ruff check— all cleancf work start --helpandcf work batch run --helpshow--isolationflagCloses #532
Summary by CodeRabbit
--isolationflag towork startandwork batch runwith modes:none(default),worktree(per-task isolated worktree), andcloud(placeholder for future cloud isolation).