diff --git a/src/handler/control/spawn.py b/src/handler/control/spawn.py index 61a056a..21f1573 100644 --- a/src/handler/control/spawn.py +++ b/src/handler/control/spawn.py @@ -119,9 +119,12 @@ def spawn( except reposync.SyncError as exc: raise SpawnError(str(exc)) from exc - working_dir = worktree.resolve_working_dir( - project["root_dir"], name, subdir=subdir, worktree_branch=worktree_branch - ) + try: + working_dir = worktree.resolve_working_dir( + project["root_dir"], name, subdir=subdir, worktree_branch=worktree_branch + ) + except (worktree.WorktreeError, worktree.IsolationError) as exc: + raise SpawnError(str(exc)) from exc # Hard gates before any state is written or process launched: the test task must # exist, and configured credentials must actually resolve — a broken pointer diff --git a/src/handler/control/worktree.py b/src/handler/control/worktree.py index 696026b..c490d5b 100644 --- a/src/handler/control/worktree.py +++ b/src/handler/control/worktree.py @@ -6,6 +6,7 @@ root (README 3.4), never reaching into another project's tree. from __future__ import annotations import os +import re import subprocess @@ -13,12 +14,39 @@ class IsolationError(Exception): """Raised when a requested working dir would escape the project root.""" +class WorktreeError(Exception): + """Raised when ``git worktree add`` fails, carrying git's own stderr.""" + + def _under(root: str, path: str) -> bool: root_abs = os.path.realpath(root) path_abs = os.path.realpath(path) return path_abs == root_abs or path_abs.startswith(root_abs + os.sep) +def _slug(name: str) -> str: + """A filesystem-safe directory name derived from an agent name. + + Agent names can be free-form (a PR title, a branch expression) and land here as a + directory under the project root, so collapse anything that isn't ``[A-Za-z0-9._-]`` + to a single dash. Keeps the repo root free of paths with spaces, colons, and parens. + """ + slug = re.sub(r"[^A-Za-z0-9._-]+", "-", name).strip("-.") + return slug or "agent" + + +def _branch_exists(project_root: str, branch: str) -> bool: + return ( + subprocess.run( + ["git", "-C", project_root, "rev-parse", "--verify", "--quiet", + f"refs/heads/{branch}"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ).returncode + == 0 + ) + + def resolve_working_dir( project_root: str, agent_name: str, @@ -36,13 +64,24 @@ def resolve_working_dir( raise ValueError("pass at most one of subdir / worktree_branch") if worktree_branch: - target = os.path.join(project_root, agent_name) + target = os.path.join(project_root, _slug(agent_name)) if not _under(project_root, target): raise IsolationError(f"{target} escapes project root {project_root}") - subprocess.run( - ["git", "-C", project_root, "worktree", "add", target, worktree_branch], - check=True, - ) + # `git worktree add ` only checks out an *existing* ref; a fresh + # feature branch won't exist yet (spawn starts from the remote's latest state), so + # create it with -b. An existing local branch is checked out as-is; if it exists + # only on a remote, git DWIMs a tracking branch from the bare `add`. + cmd = ["git", "-C", project_root, "worktree", "add"] + if _branch_exists(project_root, worktree_branch): + cmd += [target, worktree_branch] + else: + cmd += ["-b", worktree_branch, target] + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise WorktreeError( + f"git worktree add for branch '{worktree_branch}' failed: " + f"{result.stderr.strip() or result.stdout.strip()}" + ) return target if subdir: diff --git a/tests/test_worktree.py b/tests/test_worktree.py new file mode 100644 index 0000000..0b4df88 --- /dev/null +++ b/tests/test_worktree.py @@ -0,0 +1,110 @@ +"""Worktree resolution: the new-branch creation path, existing-branch checkout, +directory slugification, the isolation guard, and clear errors on git failure. + +Uses real git repos in tmp_path — the whole point is git's actual behaviour (a bare +`worktree add ` only checks out an *existing* ref), which a mock can't +exercise. +""" + +from __future__ import annotations + +import subprocess + +import pytest + +from handler.control import worktree + + +def _git(root, *args): + subprocess.run(["git", "-C", str(root), *args], check=True, capture_output=True) + + +@pytest.fixture +def repo(tmp_path): + root = tmp_path / "proj" + root.mkdir() + _git(root, "init", "-q") + _git(root, "config", "user.email", "t@t.co") + _git(root, "config", "user.name", "t") + _git(root, "config", "commit.gpgsign", "false") # hermetic: ignore any global signing + _git(root, "commit", "-q", "--allow-empty", "-m", "init") + return root + + +def _is_worktree(root, path) -> bool: + out = subprocess.run( + ["git", "-C", str(root), "worktree", "list", "--porcelain"], + check=True, capture_output=True, text=True, + ).stdout + return str(path) in out + + +def test_creates_branch_when_it_does_not_exist(repo): + # The regression: a fresh feature branch doesn't exist yet, so a bare `worktree add + # ` used to fail with "invalid reference" (exit 128). It must create it. + target = worktree.resolve_working_dir( + str(repo), "feat-x", worktree_branch="feat/new-thing" + ) + assert target.endswith("/feat-x") + assert _is_worktree(repo, target) + branches = subprocess.run( + ["git", "-C", str(repo), "branch", "--list", "feat/new-thing"], + check=True, capture_output=True, text=True, + ).stdout + assert "feat/new-thing" in branches + + +def test_checks_out_existing_branch(repo): + _git(repo, "branch", "feat/existing") + target = worktree.resolve_working_dir( + str(repo), "agent", worktree_branch="feat/existing" + ) + assert _is_worktree(repo, target) + head = subprocess.run( + ["git", "-C", target, "rev-parse", "--abbrev-ref", "HEAD"], + check=True, capture_output=True, text=True, + ).stdout.strip() + assert head == "feat/existing" + + +def test_agent_name_is_slugified_for_the_directory(repo): + # A free-form agent name (a PR title) must not put spaces/colons/parens in the path. + name = "feat(claude-management): add claude skills management to webui" + target = worktree.resolve_working_dir( + str(repo), name, worktree_branch="feat/claude-management" + ) + leaf = target.rsplit("/", 1)[-1] + assert leaf == "feat-claude-management-add-claude-skills-management-to-webui" + assert " " not in target and ":" not in leaf and "(" not in leaf + + +def test_worktree_error_carries_git_stderr(repo): + # A path that already exists (an earlier worktree on the same slug) surfaces git's own + # message, not a bare "exit status 128". + worktree.resolve_working_dir(str(repo), "dup", worktree_branch="feat/one") + with pytest.raises(worktree.WorktreeError) as exc: + worktree.resolve_working_dir(str(repo), "dup", worktree_branch="feat/two") + assert "already exists" in str(exc.value) + + +def test_rejects_worktree_and_subdir_together(repo): + with pytest.raises(ValueError): + worktree.resolve_working_dir( + str(repo), "a", subdir="sub", worktree_branch="feat/x" + ) + + +def test_subdir_is_created_under_root(repo): + target = worktree.resolve_working_dir(str(repo), "a", subdir="nested/dir") + assert target.endswith("/nested/dir") + import os + assert os.path.isdir(target) + + +def test_subdir_escaping_root_is_rejected(repo): + with pytest.raises(worktree.IsolationError): + worktree.resolve_working_dir(str(repo), "a", subdir="../escape") + + +def test_no_subdir_no_branch_returns_root(repo): + assert worktree.resolve_working_dir(str(repo), "a") == str(repo)