diff --git a/CHANGELOG.md b/CHANGELOG.md index bf6e90b8..8c6661a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ - **Deep-link straight to a profile with `?profile=`.** Opening the WebUI with a `?profile=NAME` query parameter now switches to that profile on boot. The name is validated against the existing profile allowlist (a nonexistent profile boots normally, path-traversal / cross-profile attempts are rejected), the switch goes through the same access-enforced `switchToProfile`, and boot fails safe on anything invalid. Thanks @rodboev. (#5730, #5682) +- **WebUI-created git worktrees can now be cleaned up (no more orphan accumulation).** The agent locks every worktree it creates, and git refuses to remove a locked worktree with a single `--force`, so WebUI-created worktrees were piling up unremovable. Session-worktree removal now runs a fail-soft `git worktree unlock` right before the remove (mirroring the agent's own cleanup ordering); the existing dirty/uncommitted-changes guard still runs first, so user data stays protected. Thanks @ayxuerui. (#6028, #6023) + ### Fixed - **Gateway provider errors are reported accurately, and your prompt survives a failed turn.** On gateway-backed sessions, a terminal provider error (bad model id, exhausted credentials, etc.) mid-turn is now classified and shown as the real error instead of a generic empty turn, the error card is bound to the correct session, and the in-flight user prompt plus any partial output is persisted so it survives a reload — even when you'd just re-sent an identical prompt (e.g. "continue"). Thanks @rodboev. (#5969, #5940) diff --git a/api/worktrees.py b/api/worktrees.py index d5aeb1e7..761ed3fd 100644 --- a/api/worktrees.py +++ b/api/worktrees.py @@ -265,6 +265,18 @@ def remove_worktree_for_session(session, *, force: bool = False) -> dict: repo_root = getattr(session, "worktree_repo_root", None) if not repo_root: raise ValueError("Session missing worktree_repo_root") + + # Unlock the creation-time bookkeeping lock before removing (fail-soft). + # The agent locks every worktree it creates (cli._setup_worktree), and git + # refuses to remove a locked worktree with a single --force, so without + # this every WebUI-created worktree is unremovable. The dirty/untracked/ + # unpushed/stream/terminal guards above are the real safety layer, not + # git's lock — mirrors the agent's own _cleanup_worktree ordering. + try: + _run_git(["worktree", "unlock", str(worktree_path)], str(repo_root), timeout=5) + except (OSError, subprocess.TimeoutExpired): + pass # already unlocked / never locked — non-fatal + try: remove_args = ["worktree", "remove"] if force: diff --git a/tests/test_worktree_remove.py b/tests/test_worktree_remove.py index 0c658235..e8800697 100644 --- a/tests/test_worktree_remove.py +++ b/tests/test_worktree_remove.py @@ -78,6 +78,80 @@ def test_remove_clean_worktree_succeeds(tmp_path): assert not wt_path.exists() +def test_remove_locked_worktree_succeeds_without_force(tmp_path): + """Issue #6023: WebUI-created worktrees are locked at creation by the + agent (cli._setup_worktree). Removal must unlock first — otherwise git + refuses with \"use 'remove -f -f' to override or unlock first\" and every + WebUI-created worktree is unremovable from the UI.""" + import subprocess + from api.models import Session + + main = _make_minimal_git_repo(tmp_path) + wt_path = tmp_path / "wt_locked" + subprocess.run( + ["git", "-C", str(main), "worktree", "add", str(wt_path), "-b", "hermes/testlocked"], + check=True, capture_output=True, + ) + # Reproduce the agent's creation-time lock. + subprocess.run( + ["git", "-C", str(main), "worktree", "lock", "--reason", "hermes pid=12345", str(wt_path)], + check=True, capture_output=True, + ) + + s = Session( + session_id="testlocked", + title="Locked", + workspace=str(wt_path), + worktree_path=str(wt_path), + worktree_branch="hermes/testlocked", + worktree_repo_root=str(main), + ) + + result = worktrees.remove_worktree_for_session(s, force=False) + assert result["ok"] is True + assert result["removed_path"] == str(wt_path.resolve()) + assert not wt_path.exists() + listed = subprocess.run( + ["git", "-C", str(main), "worktree", "list", "--porcelain"], + check=True, capture_output=True, text=True, + ).stdout + assert str(wt_path.resolve()) not in listed + + +def test_remove_never_locked_worktree_unlock_is_fail_soft(tmp_path): + """The pre-remove unlock must be fail-soft: removing a worktree that was + never locked still succeeds (unlock exits non-zero, _run_git is + check=False, remove proceeds).""" + import subprocess + from api.models import Session + + main = _make_minimal_git_repo(tmp_path) + wt_path = tmp_path / "wt_neverlocked" + subprocess.run( + ["git", "-C", str(main), "worktree", "add", str(wt_path), "-b", "hermes/testneverlocked"], + check=True, capture_output=True, + ) + # Sanity: not locked — `git worktree unlock` on it would fail. + probe = subprocess.run( + ["git", "-C", str(main), "worktree", "unlock", str(wt_path)], + capture_output=True, text=True, + ) + assert probe.returncode != 0 + + s = Session( + session_id="testneverlocked", + title="Never locked", + workspace=str(wt_path), + worktree_path=str(wt_path), + worktree_branch="hermes/testneverlocked", + worktree_repo_root=str(main), + ) + + result = worktrees.remove_worktree_for_session(s, force=False) + assert result["ok"] is True + assert not wt_path.exists() + + def test_remove_clean_worktree_does_not_force(tmp_path, monkeypatch): from api.models import Session @@ -111,7 +185,9 @@ def test_remove_clean_worktree_does_not_force(tmp_path, monkeypatch): result = worktrees.remove_worktree_for_session(s, force=False) assert result["ok"] is True - assert calls[0] == ["worktree", "remove", str(worktree_path.resolve())] + # Unlock (fail-soft) runs first, then the non-force remove. + assert calls[0] == ["worktree", "unlock", str(worktree_path.resolve())] + assert calls[1] == ["worktree", "remove", str(worktree_path.resolve())] def test_remove_dirty_worktree_without_force_is_rejected(tmp_path, monkeypatch): @@ -234,7 +310,9 @@ def test_remove_force_warns_and_uses_git_force(tmp_path, monkeypatch): result = worktrees.remove_worktree_for_session(s, force=True) assert result["ok"] is True - assert calls[0] == ["worktree", "remove", "--force", str(worktree_path.resolve())] + # Unlock (fail-soft) runs first, then the forced remove. + assert calls[0] == ["worktree", "unlock", str(worktree_path.resolve())] + assert calls[1] == ["worktree", "remove", "--force", str(worktree_path.resolve())] assert "untracked file" in " ".join(result["warnings"]) assert "unpushed commit" in " ".join(result["warnings"])