Find every stale worktree, not just the managed ones #14

Merged

benvin merged 3 commits from benvin/worktree-gc into main 2026-09-12 00:46:40 +10:00

Conversation 1 Commits 3 Files Changed 7

+1182 -68

unkin-agent commented 2026-09-12 00:16:47 +10:00

Member

Copy Link

Copy Source

agentws prune only looked under the worktree root, so the hand-made worktrees in ~/src/prodenv/.worktrees were invisible to it, and a checkout mid-rebase, on a detached HEAD or backed by a vanished repo could be classified removable.

• discover worktrees from git worktree list on each source checkout as well as the worktree root
• normalise each candidate to its main checkout, so a linked worktree cannot offer up the real one
• keep locked, mid-rebase and detached-with-unique-commits worktrees
• prune a registration whose working tree is gone; delete a directory only once stat proves its repo absent
• keep anything git fails to answer for, naming the error
• add --no-fetch, --json, --include-unmanaged and --include-keep

`agentws prune` only looked under the worktree root, so the hand-made worktrees in `~/src/prodenv/.worktrees` were invisible to it, and a checkout mid-rebase, on a detached HEAD or backed by a vanished repo could be classified removable. - discover worktrees from `git worktree list` on each source checkout as well as the worktree root - normalise each candidate to its main checkout, so a linked worktree cannot offer up the real one - keep locked, mid-rebase and detached-with-unique-commits worktrees - prune a registration whose working tree is gone; delete a directory only once stat proves its repo absent - keep anything git fails to answer for, naming the error - add `--no-fetch`, `--json`, `--include-unmanaged` and `--include-keep`

unkin-agent added 1 commit 2026-09-12 00:16:47 +10:00

Find every stale worktree, not just the managed ones ... 6380270ac6

- discover worktrees from `git worktree list` on each source checkout, not only
  the worktree root, so hand-made ones, stale registrations and orphaned
  directories are classified too
- normalise each candidate to its main checkout, so a linked worktree in the
  source root cannot offer up the repo's real checkout
- keep locked, mid-rebase and detached-with-unique-commits worktrees, whose
  removal would destroy state nothing else holds
- name the retained branch in every unproven verdict
- add --no-fetch, --json, --include-unmanaged and --include-keep

unkin-agent commented 2026-09-12 00:29:52 +10:00

Author

Member

Copy Link

Copy Source

Reviewed against the stated why (discover worktrees git worktree list finds outside the worktree root; stop classifying mid-rebase/detached checkouts as removable). Build/vet/lint/tests all clean.

Verdict: issues found — one high-severity classification bug. Not merge-ready as-is.

[HIGH] Orphan classification conflates "backing repo confirmed gone" with "any git error" — bypasses every safety check and hard-deletes.
cmd/agentws/main.go:291-296:

branch, branchErr := agent.GitCurrentBranch(path)
srcDir, srcErr := agent.SourceRepoDir(path)
if branchErr != nil || srcErr != nil {
    out = append(out, managedWt{repo: repoFromDirName(e.Name()), path: path, managed: true, orphan: true})
    continue
}

Any failure of rev-parse --abbrev-ref HEAD or rev-parse --git-common-dir — not just "the backing repo directory was actually deleted" — is read as orphan. classify() (prune.go:145-147) short-circuits orphan straight to verdictRemove, skipping dirty/locked/in-progress-op entirely, and removeOrphanDir (main.go:539-552) does a bare os.RemoveAll(wt.path) guarded only by path containment — no dirty check, no lock check, no confirmation the repo is actually gone vs. a transient error (NFS hiccup, permission blip, git under load, momentarily-unmounted srcDir). Before this PR the same error just made the entry silently invisible to managedWorktrees() (continue // skip) — safe by omission. This PR turns "can't read it" into "delete it," reachable on a plain agentws prune --yes with no --include-* flags, for a tool whose source root plausibly lives on network/removable storage under agent automation.
Fix: only classify orphan after independently confirming the backing directory doesn't exist (e.g. stat the gitdir target the .git file names) instead of trusting any non-nil error from two git subcommands; on an ambiguous error, keep + report "inspection failed", same as classify() already does elsewhere (prune.go:169).

[MEDIUM] --include-keep help text undersells what it can destroy.
prune.go:91: "Dangerous: also remove worktrees classified keep (needs --yes; never deletes their branch)". Verified live: git worktree remove --force succeeds silently on a worktree with an interactive rebase paused mid-conflict, deleting the sequencer state (rebase-merge dir, todo list) with zero warning and zero git-level refusal. So this flag doesn't just override a keep you disagree with — for the locked/mid-rebase/dirty keep reasons it can permanently lose uncommitted or in-progress work that no branch ever captured. "Never deletes their branch" reads more reassuring than the actual risk. Suggest either stronger wording or restricting --include-keep to the "PR open" keep reason only, leaving locked/dirty/in-progress-op as non-overridable.

Scope/atomicity: PR body's 5 bullets don't mention the orphan-directory deletion or stale-registration pruning (wt.missing, prune.go:377-379) that the diff also adds — a distinct "handle a vanished backing repo/dir" concern, not "discover hand-made worktrees." Not blocking given how interlinked the discovery/safety changes are, but worth a body update or a split-out PR.

Classifier ordering: no bugs found. missing → locked → in-progress-op → dirty → open-PR → upstream-contained/cherry-equivalent → detached-with-unique-commits → PR-merged/closed switch correctly puts hard-stop guards ahead of "provably safe" checks.

Squash-merge handling: correct and pre-existing (unchanged by this PR) — git cherry for patch-equivalence, PR matching via head.label (not head.ref, which Gitea rewrites to refs/pull/N/head post-merge).

Unpushed-commits question (hard keep only for detached HEAD; branch-backed worktrees still get removed, branch retained): verified safe. Every verdictRemoveBranch assignment (prune.go:414,423,440,442,448) sets proven=true in the same statement — no path deletes a branch without it. deleteLocalBranch (main.go:556-562) only force-deletes (-D) when proven; unproven cases go through plain git branch -d, which git itself refuses if unmerged. For the plain remove outcome I confirmed live that git worktree remove --force never touches refs/heads/* — an unpushed commit's branch and object both survive worktree removal intact in the shared source checkout. So the design is sound for branch-backed worktrees; the one hole is the orphan path above, which never reaches this logic at all.

Tests: solid, not rubber-stamped — real fixtures for locked, mid-rebase (git rebase --exec false), mid-cherry-pick, detached+unique-commits, detached+contained, unmanaged-worktree gating, missing-registration, main-checkout-never-offered, --no-fetch distrust, --json shape, --include-keep branch-retention. Gap: no test for the orphan false-positive above (only the "confirmed gone" case is exercised), and no test combining --include-unmanaged with a keep verdict on an unmanaged worktree.

Conventions: body 646 chars / 7 lines, why + present-tense bullets, no names/AI references — compliant. Comments are one-liners, no blocks. go build, go vet, golangci-lint run, go test ./... all clean.

Reviewed against the stated why (discover worktrees `git worktree list` finds outside the worktree root; stop classifying mid-rebase/detached checkouts as removable). Build/vet/lint/tests all clean. **Verdict: issues found — one high-severity classification bug. Not merge-ready as-is.** **[HIGH] Orphan classification conflates "backing repo confirmed gone" with "any git error" — bypasses every safety check and hard-deletes.** `cmd/agentws/main.go:291-296`: ```go branch, branchErr := agent.GitCurrentBranch(path) srcDir, srcErr := agent.SourceRepoDir(path) if branchErr != nil || srcErr != nil { out = append(out, managedWt{repo: repoFromDirName(e.Name()), path: path, managed: true, orphan: true}) continue } ``` Any failure of `rev-parse --abbrev-ref HEAD` or `rev-parse --git-common-dir` — not just "the backing repo directory was actually deleted" — is read as orphan. `classify()` (`prune.go:145-147`) short-circuits orphan straight to `verdictRemove`, skipping dirty/locked/in-progress-op entirely, and `removeOrphanDir` (`main.go:539-552`) does a bare `os.RemoveAll(wt.path)` guarded only by path containment — no dirty check, no lock check, no confirmation the repo is actually gone vs. a transient error (NFS hiccup, permission blip, git under load, momentarily-unmounted srcDir). Before this PR the same error just made the entry silently invisible to `managedWorktrees()` (`continue // skip`) — safe by omission. This PR turns "can't read it" into "delete it," reachable on a plain `agentws prune --yes` with no `--include-*` flags, for a tool whose source root plausibly lives on network/removable storage under agent automation. Fix: only classify orphan after independently confirming the backing directory doesn't exist (e.g. stat the gitdir target the `.git` file names) instead of trusting any non-nil error from two git subcommands; on an ambiguous error, keep + report "inspection failed", same as `classify()` already does elsewhere (`prune.go:169`). **[MEDIUM] `--include-keep` help text undersells what it can destroy.** `prune.go:91`: `"Dangerous: also remove worktrees classified keep (needs --yes; never deletes their branch)"`. Verified live: `git worktree remove --force` succeeds silently on a worktree with an interactive rebase paused mid-conflict, deleting the sequencer state (rebase-merge dir, todo list) with zero warning and zero git-level refusal. So this flag doesn't just override a keep you disagree with — for the locked/mid-rebase/dirty keep reasons it can permanently lose uncommitted or in-progress work that no branch ever captured. "Never deletes their branch" reads more reassuring than the actual risk. Suggest either stronger wording or restricting `--include-keep` to the "PR open" keep reason only, leaving locked/dirty/in-progress-op as non-overridable. **Scope/atomicity:** PR body's 5 bullets don't mention the orphan-directory deletion or stale-registration pruning (`wt.missing`, `prune.go:377-379`) that the diff also adds — a distinct "handle a vanished backing repo/dir" concern, not "discover hand-made worktrees." Not blocking given how interlinked the discovery/safety changes are, but worth a body update or a split-out PR. **Classifier ordering:** no bugs found. `missing → locked → in-progress-op → dirty → open-PR → upstream-contained/cherry-equivalent → detached-with-unique-commits → PR-merged/closed switch` correctly puts hard-stop guards ahead of "provably safe" checks. **Squash-merge handling:** correct and pre-existing (unchanged by this PR) — `git cherry` for patch-equivalence, PR matching via `head.label` (not `head.ref`, which Gitea rewrites to `refs/pull/N/head` post-merge). **Unpushed-commits question (hard keep only for detached HEAD; branch-backed worktrees still get removed, branch retained):** verified safe. Every `verdictRemoveBranch` assignment (`prune.go:414,423,440,442,448`) sets `proven=true` in the same statement — no path deletes a branch without it. `deleteLocalBranch` (`main.go:556-562`) only force-deletes (`-D`) when `proven`; unproven cases go through plain `git branch -d`, which git itself refuses if unmerged. For the plain `remove` outcome I confirmed live that `git worktree remove --force` never touches `refs/heads/*` — an unpushed commit's branch and object both survive worktree removal intact in the shared source checkout. So the design is sound for branch-backed worktrees; the one hole is the orphan path above, which never reaches this logic at all. **Tests:** solid, not rubber-stamped — real fixtures for locked, mid-rebase (`git rebase --exec false`), mid-cherry-pick, detached+unique-commits, detached+contained, unmanaged-worktree gating, missing-registration, main-checkout-never-offered, `--no-fetch` distrust, `--json` shape, `--include-keep` branch-retention. Gap: no test for the orphan false-positive above (only the "confirmed gone" case is exercised), and no test combining `--include-unmanaged` with a `keep` verdict on an unmanaged worktree. **Conventions:** body 646 chars / 7 lines, why + present-tense bullets, no names/AI references — compliant. Comments are one-liners, no blocks. `go build`, `go vet`, `golangci-lint run`, `go test ./...` all clean.

unkin-agent added 1 commit 2026-09-12 00:37:12 +10:00

Keep worktrees git could not read, never delete them ... 62aeaf063b

Any git error on a directory under the worktree root was classified orphan,
and orphan deletes the directory outright, so a transient failure reading the
source root became data loss on a plain `agentws prune --yes`.

- prove a backing repo gone by stat before calling a directory an orphan
- classify an unexplained git failure as keep, naming the error
- refuse to remove a worktree whose git state is unknown, even with --include-keep
- spell out that --include-keep discards uncommitted and in-progress work

unkin-agent added 1 commit 2026-09-12 00:40:13 +10:00

Cover the keep gate for unmanaged worktrees ... 72a8923c3d

--include-unmanaged lifts only the location gate, and nothing proved the keep
gate still held behind it.

- assert a dirty hand-made worktree survives --include-unmanaged alone

benvin merged commit cdced6536e into main 2026-09-12 00:46:40 +10:00

benvin deleted branch benvin/worktree-gc 2026-09-12 00:46:40 +10:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/agent-tools#14