feat(core): worktree-per-task isolation for parallel batch execution - #441
Conversation
…ion (#418) Create isolated git worktrees for each task in parallel batch execution, preventing race conditions when multiple agents modify files concurrently. - TaskWorktree class: create(), merge_back(), cleanup() lifecycle - MergeResult dataclass for tracking merge success/conflicts - Worktrees at .codeframe/worktrees/<task-id> on branch cf/<task-id> - Merge conflicts detected and reported (merge --abort on failure) - Cleanup never raises (logged as warnings) - BatchRun.isolate field + start_batch(isolate=) parameter - _execute_task_subprocess accepts worktree_path for cwd override - 11 unit tests including real git worktree create/merge/cleanup/conflict Closes #418
WalkthroughAdds a new worktrees module for per-task git worktrees and updates the conductor to optionally create/use isolated worktrees for batch tasks (BatchRun gains an Changes
Sequence DiagramsequenceDiagram
participant Conductor
participant TaskWorktree
participant GitRepo as "Git Repository"
participant Agent
Conductor->>TaskWorktree: create(workspace_path, task_id)
TaskWorktree->>GitRepo: git worktree add .codeframe/worktrees/<task_id> -b cf/<task_id>
TaskWorktree-->>Conductor: worktree_path
Conductor->>Agent: execute task (cwd=worktree_path)
Agent->>Agent: modify files / run gates
Agent-->>Conductor: task finished
Conductor->>TaskWorktree: merge_back(workspace_path, task_id)
TaskWorktree->>GitRepo: git checkout <base_branch>
TaskWorktree->>GitRepo: git merge --no-ff cf/<task_id>
alt Merge success
TaskWorktree-->>Conductor: MergeResult(success=True, merge_commit)
else Merge conflict
TaskWorktree->>GitRepo: git merge --abort
TaskWorktree-->>Conductor: MergeResult(success=False, conflict_details)
end
Conductor->>TaskWorktree: cleanup(workspace_path, task_id)
TaskWorktree->>GitRepo: git worktree remove .codeframe/worktrees/<task_id>
TaskWorktree->>GitRepo: git branch -D cf/<task_id>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
📝 Coding Plan
Comment |
Code Review: feat(core): worktree-per-task isolation for parallel batch executionThis is a solid foundation for issue #418 — the Critical1.
The standard worktree pattern avoids this entirely: the worktree branch should be merged from within the worktree's directory using # current — dangerous during parallel execution
subprocess.run(["git", "checkout", base_branch], cwd=str(workspace_path), ...)
subprocess.run(["git", "merge", branch_name, ...], cwd=str(workspace_path), ...)2. The diff shows the signature change and the Important3. Both result = subprocess.run(
["git", "rev-parse", "--abbrev-ref", "HEAD"],
cwd=str(workspace_path), capture_output=True, text=True, check=True,
)
base_branch = result.stdout.strip()4. The try:
result = subprocess.run([...], capture_output=True, text=True)
if result.returncode != 0:
logger.warning("Failed to remove worktree for %s: %s", task_id, result.stderr)
except Exception as exc:
logger.warning("Failed to remove worktree for %s: %s", task_id, exc)5.
Architecture / Convention6. Per 7.
Test Quality8. Tests will fail in CI without Every test that calls Add identity configuration to each test's setup: subprocess.run(["git", "-C", str(tmp_path), "config", "user.email", "test@test.com"], capture_output=True)
subprocess.run(["git", "-C", str(tmp_path), "config", "user.name", "Test"], capture_output=True)9. The test confirms Minor / Nitpicks
Summary
The core isolation concept is sound and the module structure follows |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/core/test_worktrees.py (2)
43-55: Consider adding git user config for CI compatibility.The tests use real git operations, but
git commitmay fail in CI environments whereuser.nameanduser.emailare not configured. This could cause silent failures sincecapture_output=Truesuppresses error messages and return codes aren't checked.♻️ Proposed fix to configure git user for commits
def test_creates_worktree(self, tmp_path: Path) -> None: from codeframe.core.worktrees import TaskWorktree # Set up a real git repo subprocess.run(["git", "init", str(tmp_path)], capture_output=True) + subprocess.run(["git", "-C", str(tmp_path), "config", "user.email", "test@test.com"], capture_output=True) + subprocess.run(["git", "-C", str(tmp_path), "config", "user.name", "Test"], capture_output=True) subprocess.run(["git", "-C", str(tmp_path), "commit", "--allow-empty", "-m", "init"], capture_output=True)Alternatively, consider creating a pytest fixture that sets up a configured git repo to avoid repetition across all test methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_worktrees.py` around lines 43 - 55, The test_creates_worktree uses real git commands via subprocess.run which can fail in CI because git user.name/user.email are unset; before running the commit in the test (or in a pytest fixture used by TaskWorktree tests) run git config commands to set a local user.name and user.email for the tmp_path repo and check subprocess.run returncodes (or remove capture_output=True) so failures surface; update the test that calls subprocess.run(["git", "init", str(tmp_path)]), subprocess.run(["git", "-C", str(tmp_path), "commit", ...]) and/or introduce a fixture that initializes and configures the repo used by TaskWorktree to ensure commits succeed in CI.
47-48: Addcheck=Trueor validate return codes for git setup commands.The git setup commands use
capture_output=Truebut don't verify success. Ifgit initorgit commitfails (e.g., due to missing git binary or permission issues), tests would proceed with an invalid state and produce confusing failures.♻️ Example fix for one test
- subprocess.run(["git", "init", str(tmp_path)], capture_output=True) - subprocess.run(["git", "-C", str(tmp_path), "commit", "--allow-empty", "-m", "init"], capture_output=True) + subprocess.run(["git", "init", str(tmp_path)], capture_output=True, check=True) + subprocess.run(["git", "-C", str(tmp_path), "commit", "--allow-empty", "-m", "init"], capture_output=True, check=True)Also applies to: 60-61, 72-73, 93-94, 115-118, 145-146, 165-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_worktrees.py` around lines 47 - 48, The git setup subprocess.run calls that execute ["git", "init", str(tmp_path)] and ["git", "-C", str(tmp_path), "commit", "--allow-empty", "-m", "init"] must enforce success; update each such invocation in tests/core/test_worktrees.py (the subprocess.run calls using capture_output=True with tmp_path) to either pass check=True or explicitly inspect the returned CompletedProcess.returncode and raise/assert on non-zero, and apply the same change to all occurrences (the other similar pairs around the noted ranges) so failures surface immediately instead of letting tests proceed with an invalid repo state.codeframe/core/worktrees.py (1)
156-174: The exception handling doesn't catch git command failures.
subprocess.run()with defaultcheck=Falsedoesn't raise exceptions when the command fails—it only returns a non-zeroreturncode. Thetry/except Exceptionblocks will only catch Python-level exceptions (e.g.,FileNotFoundErrorif git isn't installed), not git operation failures.Since the docstring says "Never raises — cleanup failures are logged as warnings," you should check the return code instead:
♻️ Proposed fix to properly handle failures
# Remove worktree - try: - subprocess.run( - ["git", "worktree", "remove", str(worktree_path), "--force"], - cwd=str(workspace_path), - capture_output=True, - text=True, - ) - except Exception as exc: - logger.warning("Failed to remove worktree for %s: %s", task_id, exc) + result = subprocess.run( + ["git", "worktree", "remove", str(worktree_path), "--force"], + cwd=str(workspace_path), + capture_output=True, + text=True, + ) + if result.returncode != 0: + logger.warning("Failed to remove worktree for %s: %s", task_id, result.stderr) # Delete branch - try: - subprocess.run( - ["git", "branch", "-D", branch_name], - cwd=str(workspace_path), - capture_output=True, - text=True, - ) - except Exception as exc: - logger.warning("Failed to delete branch %s: %s", branch_name, exc) + result = subprocess.run( + ["git", "branch", "-D", branch_name], + cwd=str(workspace_path), + capture_output=True, + text=True, + ) + if result.returncode != 0: + logger.warning("Failed to delete branch %s: %s", branch_name, result.stderr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/worktrees.py` around lines 156 - 174, The current try/except around subprocess.run(...) for the git worktree removal and git branch deletion (the two subprocess.run calls invoking ["git", "worktree", "remove", ...] and ["git", "branch", "-D", ...]) won't catch git failures because subprocess.run defaults to check=False; change these blocks to capture the CompletedProcess result (e.g., result = subprocess.run(..., capture_output=True, text=True)) and then check result.returncode != 0 and log a warning via logger.warning including task_id or branch_name and result.stderr/result.stdout to surface git error details; keep the outer try/except for Python-level exceptions (e.g., FileNotFoundError) but ensure git command failures are handled by inspecting returncode and logging accordingly.
🤖 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`:
- Line 537: The BatchRun.isolate flag is set but never used when running
parallel batches; update the orchestration so that when BatchRun.isolate (and
the start_batch(isolate=) parameter) is True you create per-task worktrees and
pass their paths into _execute_task_subprocess via worktree_path. Specifically:
import TaskWorktree from codeframe.core.worktrees, create a TaskWorktree for
each task in _execute_parallel and _execute_group_parallel when isolate is True,
ensure the created worktree path is passed to _execute_task_subprocess (which
already uses cwd=str(worktree_path) if provided), and clean up or close the
TaskWorktree after the subprocess completes; keep the non-isolate path unchanged
so existing behavior is preserved.
In `@codeframe/core/worktrees.py`:
- Around line 92-99: merge_back() currently runs "git checkout base_branch" in
the main workspace_path which can race with other concurrent tasks; change
merge_back() to avoid mutating the main workspace by creating a temporary git
worktree (use "git worktree add <temp_path> <base_branch>"), perform the merge
there (checkout base_branch in the temp worktree, merge the branch/ref to be
merged, push if needed), then remove the worktree, or alternatively perform a
merge using explicit ref paths without checking out (e.g., fetch refs and run
git merge <remote>/<branch> in a separate repo path). Update the code that calls
subprocess.run around workspace_path to operate on the temp worktree path and
document that if worktrees are not used, merge_back() must be called serially to
avoid interference; reference the merge_back(), workspace_path and base_branch
symbols when applying the change.
- Around line 40-72: The create() method accepts base_branch but never uses it
when creating the worktree; update the subprocess.run call in create
(referencing branch_name and base_branch) so the new branch is created from the
given base_branch by passing base_branch as the start-point to the git worktree
command (i.e., include base_branch as the final argument to the ["git",
"worktree", "add", "-b", branch_name, ...] invocation), ensuring worktree_path
and cwd usage remain unchanged.
---
Nitpick comments:
In `@codeframe/core/worktrees.py`:
- Around line 156-174: The current try/except around subprocess.run(...) for the
git worktree removal and git branch deletion (the two subprocess.run calls
invoking ["git", "worktree", "remove", ...] and ["git", "branch", "-D", ...])
won't catch git failures because subprocess.run defaults to check=False; change
these blocks to capture the CompletedProcess result (e.g., result =
subprocess.run(..., capture_output=True, text=True)) and then check
result.returncode != 0 and log a warning via logger.warning including task_id or
branch_name and result.stderr/result.stdout to surface git error details; keep
the outer try/except for Python-level exceptions (e.g., FileNotFoundError) but
ensure git command failures are handled by inspecting returncode and logging
accordingly.
In `@tests/core/test_worktrees.py`:
- Around line 43-55: The test_creates_worktree uses real git commands via
subprocess.run which can fail in CI because git user.name/user.email are unset;
before running the commit in the test (or in a pytest fixture used by
TaskWorktree tests) run git config commands to set a local user.name and
user.email for the tmp_path repo and check subprocess.run returncodes (or remove
capture_output=True) so failures surface; update the test that calls
subprocess.run(["git", "init", str(tmp_path)]), subprocess.run(["git", "-C",
str(tmp_path), "commit", ...]) and/or introduce a fixture that initializes and
configures the repo used by TaskWorktree to ensure commits succeed in CI.
- Around line 47-48: The git setup subprocess.run calls that execute ["git",
"init", str(tmp_path)] and ["git", "-C", str(tmp_path), "commit",
"--allow-empty", "-m", "init"] must enforce success; update each such invocation
in tests/core/test_worktrees.py (the subprocess.run calls using
capture_output=True with tmp_path) to either pass check=True or explicitly
inspect the returned CompletedProcess.returncode and raise/assert on non-zero,
and apply the same change to all occurrences (the other similar pairs around the
noted ranges) so failures surface immediately instead of letting tests proceed
with an invalid repo state.
🪄 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: c1672e6d-2bba-4590-aa9c-a058428817cf
📒 Files selected for processing (3)
codeframe/core/conductor.pycodeframe/core/worktrees.pytests/core/test_worktrees.py
| stall_timeout_s: int = 300 | ||
| stall_action: str = "blocker" | ||
| concurrency: ConcurrencyConfig = field(default_factory=ConcurrencyConfig) | ||
| isolate: bool = True |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if TaskWorktree is imported or used anywhere in conductor.py
rg -n "TaskWorktree|worktrees" codeframe/core/conductor.py
# Check if worktree_path is ever passed as non-None
rg -n "worktree_path=" codeframe/core/conductor.pyRepository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
# Check imports at top of conductor.py
head -50 codeframe/core/conductor.py
# Check if TaskWorktree exists anywhere in the codebase
rg -l "class TaskWorktree" codeframe/
# Check the signature of _execute_parallel and how it calls _execute_task_subprocess
rg -A 10 "def _execute_parallel" codeframe/core/conductor.py | head -30
# Check _execute_group_parallel similarly
rg -A 10 "def _execute_group_parallel" codeframe/core/conductor.py | head -30Repository: frankbria/codeframe
Length of output: 2523
🏁 Script executed:
# Check if TaskWorktree is defined in worktrees.py
rg -n "class TaskWorktree" codeframe/core/worktrees.py
# Look at the full _execute_parallel implementation
rg -A 50 "def _execute_parallel\(" codeframe/core/conductor.py | head -70
# Look at the full _execute_group_parallel implementation
rg -A 50 "def _execute_group_parallel\(" codeframe/core/conductor.py | head -70
# Check the _execute_task_subprocess signature
rg -B 2 -A 10 "def _execute_task_subprocess\(" codeframe/core/conductor.pyRepository: frankbria/codeframe
Length of output: 4146
🏁 Script executed:
# Check how _execute_task_subprocess is called within _execute_group_parallel
rg -B 5 -A 15 "def execute_task\(" codeframe/core/conductor.py | head -40
# Check the complete execute_task implementation
rg -A 25 "def execute_task\(task_id: str\)" codeframe/core/conductor.py | head -35
# Check if batch.isolate is ever checked or used
rg -n "batch\.isolate\|isolate\s*==" codeframe/core/conductor.py
# Check where TaskWorktree is actually imported or used
rg -n "TaskWorktree\|from.*worktrees" codeframe/core/conductor.pyRepository: frankbria/codeframe
Length of output: 2029
🏁 Script executed:
# Check if isolate flag is stored in BatchRun but never used
rg -n "self.isolate\|batch.isolate" codeframe/core/conductor.py
# Check where BatchRun is defined and what fields it has
rg -B 5 -A 30 "class BatchRun" codeframe/core/conductor.py | head -50
# Check if worktree_path is used inside _execute_task_subprocess
rg -A 30 "def _execute_task_subprocess" codeframe/core/conductor.py | head -40Repository: frankbria/codeframe
Length of output: 2331
🏁 Script executed:
# Check how worktree_path is actually used in _execute_task_subprocess
rg -A 50 "def _execute_task_subprocess" codeframe/core/conductor.py | tail -30
# Check if there's any orchestration code that creates TaskWorktree
rg -n "TaskWorktree(" codeframe/core/
# Check the events to see if there's any worktree-related event emission
rg -n "worktree\|WORKTREE" codeframe/core/events.pyRepository: frankbria/codeframe
Length of output: 1113
🏁 Script executed:
# Check where _execute_task_subprocess is called to see if worktree_path is ever passed
rg "worktree_path\s*=" codeframe/core/conductor.py
# Check if batch.isolate is checked anywhere in the file
rg -n "\.isolate" codeframe/core/conductor.py
# Check if TaskWorktree is instantiated anywhere in the codebase
rg "TaskWorktree\(" codeframe/core/
# Look for any worktree-related logic in start_batch or elsewhere
rg -B 3 -A 3 "isolate" codeframe/core/conductor.pyRepository: frankbria/codeframe
Length of output: 638
🏁 Script executed:
# Check the full start_batch function to see if it creates worktrees
rg -A 60 "def start_batch\(" codeframe/core/conductor.py | head -80
# Check if there's any orchestration logic that creates worktrees before calling _execute_parallel
rg -B 5 -A 5 "_execute_parallel\(workspace" codeframe/core/conductor.py
# Verify TaskWorktree is not imported at all in conductor.py
head -30 codeframe/core/conductor.py | grep -i "import\|from"Repository: frankbria/codeframe
Length of output: 3757
The isolate flag is stored but never used to create worktrees.
The BatchRun.isolate field and start_batch(isolate=) parameter are added, but the parallel execution paths (_execute_parallel, _execute_group_parallel) never check this flag. Additionally, TaskWorktree (defined in codeframe/core/worktrees.py) is not imported or instantiated in conductor.py, and worktree_path is never passed a non-None value when calling _execute_task_subprocess().
The infrastructure exists—_execute_task_subprocess() correctly handles worktree_path via cwd=str(worktree_path) if worktree_path else workspace.repo_path—but the orchestration logic that would create worktrees when isolate=True and pass their paths to subprocesses is missing. Is this deferred to a follow-up PR, or should issue #418 be considered incomplete?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/conductor.py` at line 537, The BatchRun.isolate flag is set
but never used when running parallel batches; update the orchestration so that
when BatchRun.isolate (and the start_batch(isolate=) parameter) is True you
create per-task worktrees and pass their paths into _execute_task_subprocess via
worktree_path. Specifically: import TaskWorktree from codeframe.core.worktrees,
create a TaskWorktree for each task in _execute_parallel and
_execute_group_parallel when isolate is True, ensure the created worktree path
is passed to _execute_task_subprocess (which already uses cwd=str(worktree_path)
if provided), and clean up or close the TaskWorktree after the subprocess
completes; keep the non-isolate path unchanged so existing behavior is
preserved.
CI environments may use 'master' instead of 'main' as the default branch. Use git rev-parse --abbrev-ref HEAD to detect the actual name.
Follow-up ReviewThe latest commit adds the However, the five blocking issues from my previous review are still open in the production code:
The production-side fix for #3 mirrors the test helper: call The two critical issues (#1 and #2) remain the main blockers for a safe merge. The feature is architecturally present but not yet wired into the parallel execution loop, and |
Follow-up Review (commit
|
| # | Issue | Location | Status |
|---|---|---|---|
| Critical 1 | merge_back() runs git checkout <base_branch> on the shared workspace_path while parallel agents may still be executing in that same directory |
worktrees.py:92 |
Still open |
| Critical 2 | worktree_path is added to _execute_task_subprocess but never passed from the parallel ThreadPoolExecutor loop — the feature is a no-op for actual parallel batches |
conductor.py parallel call site |
Still open |
| Important 3 | base_branch defaults to "main" in both create() and merge_back() — repositories using any other default branch will fail; _get_default_branch() helper in tests shows the right pattern for the production fix |
worktrees.py:44, 74 |
Still open |
| Important 4 | cleanup() catches Exception but subprocess.run() without check=True never raises on git errors — non-zero exit codes are silently ignored and worktrees/branches can leak |
worktrees.py:156–174 |
Still open |
The two critical issues remain the blockers for a safe merge. Specifically, Critical 1 means the merge-back step re-introduces the exact race condition that worktrees are designed to prevent. Critical 2 means the isolation layer has no effect during actual parallel execution until the call site is wired up.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/worktrees.py`:
- Around line 156-174: The cleanup currently calls subprocess.run for git
commands without check=True so non-zero git exits are not raised or logged;
update the two subprocess.run invocations (the ["git", "worktree", "remove",
...] call and the ["git", "branch", "-D", ...] call in this module) to detect
failures and log them as warnings: either pass check=True so CalledProcessError
is raised and caught by the existing except block, or capture the
CompletedProcess result and on non-zero returncode call logger.warning with a
descriptive message that includes task_id/branch_name and the process's
stdout/stderr; ensure you still catch and log other Exceptions as before.
🪄 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: 3f301569-300e-4ce6-b9ba-0b662296c405
📒 Files selected for processing (1)
codeframe/core/worktrees.py
| try: | ||
| subprocess.run( | ||
| ["git", "worktree", "remove", str(worktree_path), "--force"], | ||
| cwd=str(workspace_path), | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| except Exception as exc: | ||
| logger.warning("Failed to remove worktree for %s: %s", task_id, exc) | ||
|
|
||
| # Delete branch | ||
| try: | ||
| subprocess.run( | ||
| ["git", "branch", "-D", branch_name], | ||
| cwd=str(workspace_path), | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| except Exception as exc: |
There was a problem hiding this comment.
Git command failures are silently ignored in cleanup.
The docstring promises "cleanup failures are logged as warnings," but subprocess.run without check=True won't raise on non-zero exit codes. The try/except only catches Python-level exceptions (e.g., git not found), not git command failures. If the worktree removal or branch deletion fails, nothing is logged.
🐛 Proposed fix to log git command failures
# Remove worktree
try:
- subprocess.run(
+ result = subprocess.run(
["git", "worktree", "remove", str(worktree_path), "--force"],
cwd=str(workspace_path),
capture_output=True,
text=True,
)
+ if result.returncode != 0:
+ logger.warning("Failed to remove worktree for %s: %s", task_id, result.stderr)
except Exception as exc:
logger.warning("Failed to remove worktree for %s: %s", task_id, exc)
# Delete branch
try:
- subprocess.run(
+ result = subprocess.run(
["git", "branch", "-D", branch_name],
cwd=str(workspace_path),
capture_output=True,
text=True,
)
+ if result.returncode != 0:
+ logger.warning("Failed to delete branch %s: %s", branch_name, result.stderr)
except Exception as exc:
logger.warning("Failed to delete branch %s: %s", branch_name, exc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/worktrees.py` around lines 156 - 174, The cleanup currently
calls subprocess.run for git commands without check=True so non-zero git exits
are not raised or logged; update the two subprocess.run invocations (the ["git",
"worktree", "remove", ...] call and the ["git", "branch", "-D", ...] call in
this module) to detect failures and log them as warnings: either pass check=True
so CalledProcessError is raised and caught by the existing except block, or
capture the CompletedProcess result and on non-zero returncode call
logger.warning with a descriptive message that includes task_id/branch_name and
the process's stdout/stderr; ensure you still catch and log other Exceptions as
before.
Follow-up Review (commit
|
| # | Issue | Location | Status |
|---|---|---|---|
| Critical 1 | merge_back() runs git checkout <base_branch> on the shared workspace_path while parallel agents may still be running — re-introduces the race condition worktrees are designed to prevent |
worktrees.py:92 |
Still open |
| Critical 2 | worktree_path is added to _execute_task_subprocess but never passed from the parallel ThreadPoolExecutor loop — isolation is a no-op for actual parallel batches |
conductor.py parallel call site |
Still open |
| Important 3 | base_branch defaults to "main" in both create() and merge_back() — repositories with any other default branch will fail silently |
worktrees.py:44, 74 |
Still open |
| Important 4 | cleanup() catches Exception but subprocess.run() without check=True never raises on git errors — non-zero exit codes are swallowed and worktrees/branches can leak |
worktrees.py:156-174 |
Still open |
| Test 8 | Tests needed git config user.email and user.name to run in CI |
test_worktrees.py |
Fixed (assumed) |
No new concerns introduced by this commit. The two critical issues remain the blockers for a safe merge: the feature is architecturally present but not wired into the parallel execution path, and merge_back() still mutates shared workspace state mid-parallel-batch.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/test_worktrees.py`:
- Around line 12-18: Tests call git via subprocess.run without check=True and
_get_default_branch falls back to "main"; add a helper _git(repo_path: Path,
*args: str) that wraps subprocess.run(..., check=True, capture_output=True,
text=True) and an _init_repo(tmp_path: Path) that runs git init, sets user.email
and user.name, makes an initial commit, then call _get_default_branch which
should use _git(..., "rev-parse", "--abbrev-ref", "HEAD") and return
result.stdout.strip() (remove the "or 'main'" fallback); replace all bare
subprocess.run(["git", ...]) calls in tests with _git(...) or use
_init_repo(...) for repo setup so git failures fail fast and identity is
configured.
🪄 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: e9476b08-e744-439f-b512-8f2347c22d76
📒 Files selected for processing (1)
tests/core/test_worktrees.py
| def _get_default_branch(repo_path: Path) -> str: | ||
| """Get the default branch name of a git repo.""" | ||
| result = subprocess.run( | ||
| ["git", "-C", str(repo_path), "rev-parse", "--abbrev-ref", "HEAD"], | ||
| capture_output=True, text=True, | ||
| ) | ||
| return result.stdout.strip() or "main" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import ast
from pathlib import Path
p = Path("tests/core/test_worktrees.py")
tree = ast.parse(p.read_text())
for n in ast.walk(tree):
if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute):
if isinstance(n.func.value, ast.Name) and n.func.value.id == "subprocess" and n.func.attr == "run":
is_git = (
n.args
and isinstance(n.args[0], ast.List)
and n.args[0].elts
and isinstance(n.args[0].elts[0], ast.Constant)
and n.args[0].elts[0].value == "git"
)
if not is_git:
continue
check_kw = next((kw for kw in n.keywords if kw.arg == "check"), None)
ok = bool(check_kw and isinstance(check_kw.value, ast.Constant) and check_kw.value.value is True)
if not ok:
print(f"Line {n.lineno}: git subprocess.run without check=True")
PYRepository: frankbria/codeframe
Length of output: 1206
🏁 Script executed:
head -20 tests/core/test_worktrees.pyRepository: frankbria/codeframe
Length of output: 566
🏁 Script executed:
cat -n tests/core/test_worktrees.py | head -150Repository: frankbria/codeframe
Length of output: 7081
🏁 Script executed:
cat -n tests/core/test_worktrees.py | tail -80Repository: frankbria/codeframe
Length of output: 3612
Add check=True to all git subprocess calls; fail fast on command failures.
Lines 14, 55–56, 69–70, 82–83, 90, 104–105, 113–114, 127, 129–130, 138–139, 143–144, 158–159, 170, 179–180: git commands run without check=True, silencing failures. When git commit fails (common in CI without configured user email/name), tests continue with a broken repo state and fail later with misleading assertions. Additionally, line 18's or "main" fallback masks _get_default_branch failures instead of failing fast.
Add a helper function to configure git user identity once, then use subprocess with check=True consistently:
Suggested pattern
def _git(repo_path: Path, *args: str) -> subprocess.CompletedProcess[str]:
"""Run git command with check=True."""
return subprocess.run(
["git", "-C", str(repo_path), *args],
capture_output=True,
text=True,
check=True,
)
def _init_repo(tmp_path: Path) -> str:
"""Initialize test repo with user config."""
subprocess.run(["git", "init", str(tmp_path)], capture_output=True, text=True, check=True)
_git(tmp_path, "config", "user.email", "test@example.com")
_git(tmp_path, "config", "user.name", "Test User")
_git(tmp_path, "commit", "--allow-empty", "-m", "init")
return _get_default_branch(tmp_path)
def _get_default_branch(repo_path: Path) -> str:
"""Get the default branch name of a git repo."""
result = _git(repo_path, "rev-parse", "--abbrev-ref", "HEAD")
return result.stdout.strip()Then replace all test setup subprocess.run(["git", ...]) calls with _git(...) or _init_repo(...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/test_worktrees.py` around lines 12 - 18, Tests call git via
subprocess.run without check=True and _get_default_branch falls back to "main";
add a helper _git(repo_path: Path, *args: str) that wraps subprocess.run(...,
check=True, capture_output=True, text=True) and an _init_repo(tmp_path: Path)
that runs git init, sets user.email and user.name, makes an initial commit, then
call _get_default_branch which should use _git(..., "rev-parse", "--abbrev-ref",
"HEAD") and return result.stdout.strip() (remove the "or 'main'" fallback);
replace all bare subprocess.run(["git", ...]) calls in tests with _git(...) or
use _init_repo(...) for repo setup so git failures fail fast and identity is
configured.
1 participant
Summary
Implements #418: Worktree-Per-Task Isolation for Parallel Batch Execution
Gives each task in parallel batch execution its own git worktree, eliminating race conditions from multiple agents modifying files in the same directory.
create(),merge_back(),cleanup()lifecycle.codeframe/worktrees/<task-id>on branchcf/<task-id>--aborton failure, details preservedstart_batch(isolate=)parameter_execute_task_subprocessacceptsworktree_pathfor cwd overrideAcceptance Criteria
cwdset to worktree pathTest Plan
Closes #418
Summary by CodeRabbit
New Features
Tests