Skip to content

Commit f43b794

Browse files
frankbriaTest User
andauthored
feat(core): WorktreeRegistry, orphan cleanup, and get_base_branch (#533)
* feat(core): WorktreeRegistry, orphan cleanup, and get_base_branch (#533) Add atomic worktree registration and stale cleanup to complete the worktree-per-task isolation feature for parallel batch execution. Changes: - codeframe/core/worktrees.py: add WorktreeRegistry (register/unregister/ list_stale/cleanup_stale), get_base_branch(), list_worktrees() - codeframe/core/sandbox/context.py: use get_base_branch() instead of hardcoded "main" when creating worktrees - codeframe/core/conductor.py: call WorktreeRegistry.cleanup_stale() at _execute_parallel() start when isolation == worktree - codeframe/cli/env_commands.py: report stale worktrees in cf env doctor - tests/core/test_worktrees.py: 13 new tests for registry, get_base_branch, list_worktrees, and conductor orphan cleanup integration Closes #533 * fix(web-ui): re-enable frontend CI and fix 5 stale tests The frontend-tests CI job was disabled during the Phase 2 CLI-first refactor and never re-enabled when Phase 3 web UI became active. This allowed component changes to silently break tests across multiple PRs. CI change: - Uncomment frontend-tests job in test.yml (runs npm run test:coverage) - Add frontend-tests to test-summary needs and failure check - Exclude e2e/ (Playwright) and test-helpers.ts from Jest via jest.config.js Test fixes (component updated but test not): - QuickActions: rewrite tests to exercise all 5 pipeline states (broke in #493 — context-aware refactor changed button labels) - TaskCard: update dependency title to regex match Radix Tooltip text (broke in #524 — tooltip replaced title attribute) - TaskDetailModal: update dep count assertion to "Dependencies (N)" (broke in #524 — label changed from "2 dependencies") - BlockerCard: update success text to "answer recorded" (broke in #531 — success message reworded) - TaskBoardView: update empty state to "No tasks yet" (single block) (broke in #499 — empty state redesigned with CTA) * fix(core): address code review feedback on WorktreeRegistry (#537) - wire register()/unregister() into create_execution_context() — registry was defined but never populated, making cleanup_stale() a no-op - fix list_stale() to catch PermissionError separately (process alive, different owner is not stale); previously OSError caught both cases - guard list_worktrees() against non-list JSON (returns [] for dicts etc.) - fix get_base_branch() to return "main" in detached HEAD state (git returns literal "HEAD" — now treated as fallback) - fix conductor.py string comparison to use IsolationLevel.WORKTREE.value - replace misleading conductor test with two focused tests that patch WorktreeRegistry.cleanup_stale directly and assert call/no-call - add tests for detached HEAD fallback and non-list JSON guard --------- Co-authored-by: Test User <test@example.com>
1 parent 6483895 commit f43b794

13 files changed

Lines changed: 658 additions & 78 deletions

File tree

.github/workflows/test.yml

Lines changed: 46 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -247,50 +247,48 @@ jobs:
247247
# ============================================
248248
# Frontend Unit Tests (After Code Quality)
249249
# ============================================
250-
# DISABLED: web-ui is legacy during v2 CLI-first refactor
251-
# Re-enable when web-ui package.json is restored
252-
# frontend-tests:
253-
# name: Frontend Unit Tests
254-
# runs-on: ubuntu-latest
255-
# needs: code-quality
256-
#
257-
# steps:
258-
# - name: Checkout code
259-
# uses: actions/checkout@v4
260-
#
261-
# - name: Set up Node.js
262-
# uses: actions/setup-node@v4
263-
# with:
264-
# node-version: ${{ env.NODE_VERSION }}
265-
# cache: 'npm'
266-
# cache-dependency-path: 'web-ui/package-lock.json'
267-
#
268-
# - name: Install dependencies
269-
# working-directory: web-ui
270-
# run: npm ci
271-
#
272-
# - name: Run Jest tests with coverage
273-
# working-directory: web-ui
274-
# run: npm run test:coverage
275-
#
276-
# - name: Check coverage threshold (65%)
277-
# working-directory: web-ui
278-
# run: |
279-
# COVERAGE=$(cat coverage/coverage-summary.json | jq '.total.statements.pct')
280-
# echo "Coverage: ${COVERAGE}%"
281-
# if (( $(echo "$COVERAGE < 65" | bc -l) )); then
282-
# echo "❌ Coverage ${COVERAGE}% is below 65% threshold"
283-
# exit 1
284-
# else
285-
# echo "✅ Coverage ${COVERAGE}% meets 65% threshold"
286-
# fi
287-
#
288-
# - name: Upload coverage reports
289-
# uses: codecov/codecov-action@v4
290-
# with:
291-
# directory: web-ui/coverage
292-
# flags: frontend
293-
# name: frontend-coverage
250+
frontend-tests:
251+
name: Frontend Unit Tests
252+
runs-on: ubuntu-latest
253+
needs: code-quality
254+
255+
steps:
256+
- name: Checkout code
257+
uses: actions/checkout@v4
258+
259+
- name: Set up Node.js
260+
uses: actions/setup-node@v4
261+
with:
262+
node-version: ${{ env.NODE_VERSION }}
263+
cache: 'npm'
264+
cache-dependency-path: 'web-ui/package-lock.json'
265+
266+
- name: Install dependencies
267+
working-directory: web-ui
268+
run: npm ci
269+
270+
- name: Run Jest tests with coverage
271+
working-directory: web-ui
272+
run: npm run test:coverage
273+
274+
- name: Check coverage threshold (65%)
275+
working-directory: web-ui
276+
run: |
277+
COVERAGE=$(cat coverage/coverage-summary.json | jq '.total.statements.pct')
278+
echo "Coverage: ${COVERAGE}%"
279+
if (( $(echo "$COVERAGE < 65" | bc -l) )); then
280+
echo "❌ Coverage ${COVERAGE}% is below 65% threshold"
281+
exit 1
282+
else
283+
echo "✅ Coverage ${COVERAGE}% meets 65% threshold"
284+
fi
285+
286+
- name: Upload coverage reports
287+
uses: codecov/codecov-action@v4
288+
with:
289+
directory: web-ui/coverage
290+
flags: frontend
291+
name: frontend-coverage
294292

295293
# ============================================
296294
# E2E Backend Tests (Pytest)
@@ -791,7 +789,7 @@ jobs:
791789
test-summary:
792790
name: Test Summary
793791
runs-on: ubuntu-latest
794-
needs: [backend-tests, code-quality, check-hardcoded-urls]
792+
needs: [backend-tests, frontend-tests, code-quality, check-hardcoded-urls]
795793
if: always()
796794

797795
steps:
@@ -804,11 +802,12 @@ jobs:
804802
echo "| Code Quality | ${{ needs.code-quality.result }} |" >> $GITHUB_STEP_SUMMARY
805803
echo "| Hardcoded URLs | ${{ needs.check-hardcoded-urls.result }} |" >> $GITHUB_STEP_SUMMARY
806804
echo "| Backend Tests | ${{ needs.backend-tests.result }} |" >> $GITHUB_STEP_SUMMARY
807-
echo "| Frontend Tests | skipped (v2 CLI-first) |" >> $GITHUB_STEP_SUMMARY
805+
echo "| Frontend Tests | ${{ needs.frontend-tests.result }} |" >> $GITHUB_STEP_SUMMARY
808806
809807
if [ "${{ needs.code-quality.result }}" == "failure" ] || \
810808
[ "${{ needs.check-hardcoded-urls.result }}" == "failure" ] || \
811-
[ "${{ needs.backend-tests.result }}" == "failure" ]; then
809+
[ "${{ needs.backend-tests.result }}" == "failure" ] || \
810+
[ "${{ needs.frontend-tests.result }}" == "failure" ]; then
812811
echo "" >> $GITHUB_STEP_SUMMARY
813812
echo "❌ Some checks failed. Please review the logs above." >> $GITHUB_STEP_SUMMARY
814813
exit 1

codeframe/cli/env_commands.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,29 @@ def doctor(
215215
for rec in result.recommendations:
216216
console.print(f" • {rec}")
217217

218+
# Stale worktree check
219+
try:
220+
from codeframe.core.worktrees import WorktreeRegistry
221+
stale = WorktreeRegistry().list_stale(project_path)
222+
if stale:
223+
console.print()
224+
console.print("[bold yellow]Stale Worktrees:[/bold yellow]")
225+
for entry in stale:
226+
console.print(
227+
f" [yellow]⚠[/yellow] task [cyan]{entry['task_id']}[/cyan] "
228+
f"(pid {entry.get('pid', '?')} no longer running)"
229+
)
230+
console.print()
231+
console.print(
232+
"[dim]To clean up, run:[/dim] codeframe work batch run --all-ready "
233+
"[dim](auto-cleans on next run)[/dim]"
234+
)
235+
console.print(
236+
"[dim]Or remove manually:[/dim] rm -rf .codeframe/worktrees/"
237+
)
238+
except Exception:
239+
pass # Worktree registry is optional; never fail doctor over it
240+
218241
console.print()
219242

220243
if not result.is_healthy:

codeframe/core/conductor.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1566,6 +1566,12 @@ def _execute_parallel(
15661566
Raises:
15671567
CycleDetectedError: If circular dependencies are detected
15681568
"""
1569+
# Clean up orphaned worktrees from crashed workers on previous runs
1570+
from codeframe.core.sandbox.context import IsolationLevel as _IL
1571+
if batch.isolation == _IL.WORKTREE.value:
1572+
from codeframe.core.worktrees import WorktreeRegistry
1573+
WorktreeRegistry().cleanup_stale(workspace.repo_path)
1574+
15691575
# Create execution plan based on dependencies
15701576
plan = create_execution_plan(workspace, batch.task_ids)
15711577

codeframe/core/sandbox/context.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,17 @@ def create_execution_context(
7070
)
7171

7272
if isolation == IsolationLevel.WORKTREE:
73-
from codeframe.core.worktrees import TaskWorktree
73+
from codeframe.core.worktrees import TaskWorktree, WorktreeRegistry, get_base_branch
7474

7575
worktree = TaskWorktree()
76-
worktree_path = worktree.create(repo_path, task_id)
76+
registry = WorktreeRegistry()
77+
base_branch = get_base_branch(repo_path)
78+
worktree_path = worktree.create(repo_path, task_id, base_branch=base_branch)
79+
registry.register(repo_path, task_id, batch_id="unknown")
7780

7881
def cleanup() -> None:
7982
worktree.cleanup(repo_path, task_id)
83+
registry.unregister(repo_path, task_id)
8084

8185
return ExecutionContext(
8286
task_id=task_id,

codeframe/core/worktrees.py

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,21 @@
1313

1414
from __future__ import annotations
1515

16+
import json
1617
import logging
18+
import os
1719
import subprocess
20+
import threading
1821
from dataclasses import dataclass
22+
from datetime import datetime, timezone
1923
from pathlib import Path
2024
from typing import Optional
2125

2226
logger = logging.getLogger(__name__)
2327

2428
WORKTREE_DIR = ".codeframe/worktrees"
29+
_REGISTRY_FILE = ".codeframe/worktrees.json"
30+
_registry_lock = threading.Lock()
2531

2632

2733
@dataclass
@@ -173,3 +179,98 @@ def cleanup(
173179
)
174180
except Exception as exc:
175181
logger.warning("Failed to delete branch %s: %s", branch_name, exc)
182+
183+
184+
def get_base_branch(workspace_path: Path) -> str:
185+
"""Return the current HEAD branch name, defaulting to 'main' on failure.
186+
187+
Returns 'main' when git is unavailable, the directory is not a repo,
188+
or HEAD is detached (rev-parse returns 'HEAD' literally).
189+
"""
190+
result = subprocess.run(
191+
["git", "rev-parse", "--abbrev-ref", "HEAD"],
192+
cwd=str(workspace_path),
193+
capture_output=True,
194+
text=True,
195+
)
196+
branch = result.stdout.strip()
197+
if result.returncode != 0 or not branch or branch == "HEAD":
198+
return "main"
199+
return branch
200+
201+
202+
def list_worktrees(workspace_path: Path) -> list[dict]:
203+
"""Return all entries in the worktree registry, or [] if absent/corrupt/malformed."""
204+
registry_file = workspace_path / _REGISTRY_FILE
205+
if not registry_file.exists():
206+
return []
207+
try:
208+
data = json.loads(registry_file.read_text())
209+
return data if isinstance(data, list) else []
210+
except Exception:
211+
return []
212+
213+
214+
class WorktreeRegistry:
215+
"""Atomic registry of active worktrees for orphan detection.
216+
217+
Stores entries in ``.codeframe/worktrees.json``. All mutations are
218+
protected by a module-level threading.Lock and written atomically
219+
(write to .tmp then os.replace).
220+
"""
221+
222+
def register(self, workspace_path: Path, task_id: str, batch_id: str) -> None:
223+
"""Add or refresh a worktree entry for task_id."""
224+
with _registry_lock:
225+
entries = list_worktrees(workspace_path)
226+
# Remove any existing entry for this task_id (idempotent)
227+
entries = [e for e in entries if e["task_id"] != task_id]
228+
entries.append({
229+
"task_id": task_id,
230+
"batch_id": batch_id,
231+
"created_at": datetime.now(timezone.utc).isoformat(),
232+
"pid": os.getpid(),
233+
})
234+
self._write(workspace_path, entries)
235+
236+
def unregister(self, workspace_path: Path, task_id: str) -> None:
237+
"""Remove the worktree entry for task_id. Safe if absent."""
238+
with _registry_lock:
239+
entries = list_worktrees(workspace_path)
240+
entries = [e for e in entries if e["task_id"] != task_id]
241+
self._write(workspace_path, entries)
242+
243+
def list_stale(self, workspace_path: Path) -> list[dict]:
244+
"""Return entries whose registered PID is no longer alive."""
245+
stale = []
246+
for entry in list_worktrees(workspace_path):
247+
pid = entry.get("pid")
248+
if pid is None:
249+
continue
250+
try:
251+
os.kill(pid, 0)
252+
except ProcessLookupError:
253+
stale.append(entry)
254+
except PermissionError:
255+
pass # Process is alive but owned by another user — not stale
256+
return stale
257+
258+
def cleanup_stale(self, workspace_path: Path) -> None:
259+
"""Remove stale worktree directories and unregister their entries."""
260+
stale = self.list_stale(workspace_path)
261+
if not stale:
262+
return
263+
wt = TaskWorktree()
264+
for entry in stale:
265+
task_id = entry["task_id"]
266+
logger.info("Cleaning up orphaned worktree for task %s (pid %s)", task_id, entry.get("pid"))
267+
wt.cleanup(workspace_path, task_id)
268+
self.unregister(workspace_path, task_id)
269+
270+
@staticmethod
271+
def _write(workspace_path: Path, entries: list[dict]) -> None:
272+
registry_file = workspace_path / _REGISTRY_FILE
273+
registry_file.parent.mkdir(parents=True, exist_ok=True)
274+
tmp_file = registry_file.with_suffix(".json.tmp")
275+
tmp_file.write_text(json.dumps(entries, indent=2))
276+
os.replace(tmp_file, registry_file)

0 commit comments

Comments
 (0)