Repository navigation
fix(subagent): track branch ownership during worktree cleanup - #4520
Conversation
 [Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryRecords the full branch name in each worktree’s Git admin directory and reads it before cleanup.
wozhenbang2004 explicitly identifies retaining legacy and external branch refs as intentional. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Create worktree and new branch] --> B[Record full branch name]
B --> C[Read ownership before cleanup]
C --> D[Remove working directory]
D --> E{Recorded branch?}
E -->|No| F[Leave branch refs alone]
E -->|Yes| G{Smart cleanup found changes?}
G -->|Yes| H[Keep and return recorded branch]
G -->|No| I[Delete recorded branch]
Reviews (1) · Last reviewed commit: "test(subagent): cover worktree branch ow..." · Reviewed by Greptile |
TimeToBuildBob
left a comment
TimeToBuildBob
left a comment
There was a problem hiding this comment.
Thanks for the clean reproduction and the ownership-record approach. GitHub was holding the first-contributor fork workflows; I approved those runs, so CI is in progress now.
The design looks right:
- Recording the exact created branch in the worktree git admin dir is the correct ownership signal. Directory basename is wrong for slash-separated names; current HEAD is wrong after a worker checks out an existing branch.
- Leaving refs alone when the marker is missing is the right default for legacy/external worktrees.
git branch -D --is the right deletion form.- The four regression tests hit the actual failure modes (slash names vs unrelated refs, smart-cleanup return value, switched HEAD, missing marker).
No requested changes from the diff. If CI fails, that's yours to fix — this branch stays yours. I can look at a follow-up if something comes back red.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Problem
Closes #4519. Cleaning a worktree created on
feature/taskuses the directory basenametaskas the branch to force-delete. This leaves the worktree's branch behind and can delete an unrelatedtaskbranch. Smart cleanup also returns the truncated branch name.Change
Record the exact branch created by gptme in the linked worktree's Git admin directory, then read that record before removing the worktree. The current HEAD is deliberately not used as ownership evidence: a worker can switch to an existing user branch. Legacy/external worktrees with no ownership record retain their branch refs instead of guessing. Working-tree removal behavior otherwise stays the same.
Four real-Git regression cases cover slash-separated names and unrelated refs, the smart-cleanup return value, switching to an existing branch, and worktrees without ownership metadata.
Validation
uv run pytest tests/test_git_worktree.py -q --no-cov --retries 0: 17 passed, independently rerun during review. The two slash-name cases fail on the original source.git diff --checkpass for the change.make test PYTEST_WORKERS=2 NO_COVERAGE=1: 14,954 passed, 41 failed, 285 skipped. This run began before the final ownership-record revision; the final worktree suite was rerun afterward. A follow-up rerun of those 41 cases on unchanged master reproduced 40 failures; one process-memory test passed on that rerun. The baseline run took 24.43 seconds. A full clean test run is not claimed.make lintusing lockfile Ruff 0.17.0 reports the same two existing ISC004 diagnostics intests/test_tools_watch.pyon both base and patch. The separately pinned pre-commit Ruff hook passes.make typecheckreports 37 identical errors on base and patch, involving unavailable optional dependencies and their follow-on types. No full type-check pass is claimed.AI assistance: OpenAI Codex assisted with investigation, implementation, regression tests, and an independent review.