Add agentws prune #13

Merged

benvin merged 3 commits from benvin/agentws-prune into main 2026-09-10 21:50:20 +10:00

Conversation 2 Commits 3 Files Changed 11

+1504 -6

unkin-agent commented 2026-09-09 23:41:24 +10:00

Member

Copy Link

Copy Source

Agents leave their managed worktrees behind, and agentws rm takes one path at a time with no idea whether a branch's work is safely upstream, so clearing an accumulation by hand risks destroying unmerged commits.

• classify every managed worktree: dirty, PR open, upstream, or unproven
• delete the local branch only when the work is provably upstream
• prove "upstream" with merge-base and git cherry, so squash merges count
• match a PR by head.label, which survives the branch deletion a merge does
• dry run by default; --yes applies, --keep-branches spares every branch

Agents leave their managed worktrees behind, and `agentws rm` takes one path at a time with no idea whether a branch's work is safely upstream, so clearing an accumulation by hand risks destroying unmerged commits. - classify every managed worktree: dirty, PR open, upstream, or unproven - delete the local branch only when the work is provably upstream - prove "upstream" with merge-base and git cherry, so squash merges count - match a PR by head.label, which survives the branch deletion a merge does - dry run by default; `--yes` applies, `--keep-branches` spares every branch

unkin-agent added 1 commit 2026-09-09 23:41:24 +10:00

add agentws prune ... 4bbeaae8f0

Agents leave their managed worktrees behind, and `agentws rm` takes one path at
a time with no idea whether a branch's work is safely upstream, so clearing an
accumulation by hand risks destroying unmerged commits.

- classify every managed worktree: dirty, PR open, upstream, or unproven
- remove only what is safe; delete the local branch only when work is upstream
- prove "upstream" with merge-base and git cherry, so squash merges count
- match a PR by head.label, which survives the branch deletion a merge does
- dry run by default; --yes applies, --keep-branches spares every branch
- read the Gitea path from origin's URL rather than assuming the owner

unkin-agent commented 2026-09-09 23:49:19 +10:00

Author

Member

Copy Link

Copy Source

Reviewed the classification logic in detail, including git-level experiments for the "merge commit only" and "reverted upstream" scenarios named in the review brief. Two safety-relevant findings; rest looks solid (precedence order, dry-run gating, label-vs-ref PR matching, Gitea-unreachable degrade, and repo-path derivation all check out under adversarial cases I tried).

1. classify (cmd/agentws/prune.go:232-243) can delete a branch carrying commits that never reached the remote — no SHA proof backs the "PR merged"/"branch on origin" verdicts

Both fallback branches are reached only once line 228 (unmerged == 0) has already failed, i.e. git itself has just proven there are local commits with no patch-equivalent upstream. From there:

• Line 232-234 trusts Gitea's pr.Merged flag alone and deletes the branch. If the agent added commits on the branch after the PR's head was captured (common if a worktree sits around post-merge for a quick follow-up before agentws rm), those commits are destroyed. PullRequest.Head.Sha (internal/agent/gitea.go) is already fetched but never used to check the local tip is actually contained in what merged.
• Line 236-239 (GitRemoteBranchExists) only checks a ref by that name exists on origin, not that it points at (or is an ancestor of) local HEAD. A closed-unmerged branch with local-only follow-up commits beyond the last push classifies as remove+branch "branch on origin" and loses them.

GitDeleteBranch is invoked with force=true (-D, cmd/agentws/main.go:327) unconditionally, so git's own "not merged" guard is bypassed too — these two branches are the only thing standing between a stale-but-present remote/PR and permanent loss of local-only commits.

Suggest gating both on GitIsAncestor(wt.path, "HEAD", "origin/"+wt.branch) (or against pr.Head.Sha) before trusting Gitea's state as "safe to delete."

2. git cherry "cherry-clean" doesn't mean the code is still present upstream (internal/agent/git.go GitUnmergedCommits, prune.go:228-231)

Verified experimentally: if a squash lands upstream and is later reverted with a new commit, git cherry still reports the original branch commit as - (matched), so unmerged == 0 and prune reports "cherry-clean" / removes the branch — even though the tip of the default branch no longer contains that change. The historical commit is still reachable in origin's history (not literally destroyed), but the "provably upstream" label is misleading for this case. Worth a doc caveat at minimum; a stronger fix would check the matched upstream commit is itself an ancestor of the current origin/<default> tip.

Minor, non-blocking:

• --keep-branches still prints remove+branch as the verdict even though the branch survives (cmd/agentws/prune.go:210) — cosmetically misleading, not unsafe.
• ListPRs pagination caps at maxPRPages*prPageSize = 1000 PRs (internal/agent/gitea.go) with no warning if truncated; a repo past that could silently under-report PR state for old branches.

Everything else matches the stated design: dirty and open-PR keep-precedence is correctly enforced and tested (TestPruneNeverTouchesDirtyWorktree, TestPruneKeepsOpenPR), head.label vs refs/pull/<n>/head matching is correct and tested both ways, dry-run performs no mutation, Gitea-unreachable degrades to git-only signals with a warning rather than a silent input, and an unresolvable default branch keeps the whole repo's worktrees. GitFetchPrune is tightly scoped to this command's needs and fine to ship in the same PR.

Reviewed the classification logic in detail, including git-level experiments for the "merge commit only" and "reverted upstream" scenarios named in the review brief. Two safety-relevant findings; rest looks solid (precedence order, dry-run gating, label-vs-ref PR matching, Gitea-unreachable degrade, and repo-path derivation all check out under adversarial cases I tried). **1. `classify` (cmd/agentws/prune.go:232-243) can delete a branch carrying commits that never reached the remote — no SHA proof backs the "PR merged"/"branch on origin" verdicts** Both fallback branches are reached only once line 228 (`unmerged == 0`) has already failed, i.e. git itself has just proven there *are* local commits with no patch-equivalent upstream. From there: - Line 232-234 trusts Gitea's `pr.Merged` flag alone and deletes the branch. If the agent added commits on the branch *after* the PR's head was captured (common if a worktree sits around post-merge for a quick follow-up before `agentws rm`), those commits are destroyed. `PullRequest.Head.Sha` (internal/agent/gitea.go) is already fetched but never used to check the local tip is actually contained in what merged. - Line 236-239 (`GitRemoteBranchExists`) only checks a ref by that name exists on origin, not that it points at (or is an ancestor of) local HEAD. A closed-unmerged branch with local-only follow-up commits beyond the last push classifies as `remove+branch` "branch on origin" and loses them. `GitDeleteBranch` is invoked with `force=true` (`-D`, cmd/agentws/main.go:327) unconditionally, so git's own "not merged" guard is bypassed too — these two branches are the only thing standing between a stale-but-present remote/PR and permanent loss of local-only commits. Suggest gating both on `GitIsAncestor(wt.path, "HEAD", "origin/"+wt.branch)` (or against `pr.Head.Sha`) before trusting Gitea's state as "safe to delete." **2. `git cherry` "cherry-clean" doesn't mean the code is still present upstream (internal/agent/git.go GitUnmergedCommits, prune.go:228-231)** Verified experimentally: if a squash lands upstream and is later reverted with a new commit, `git cherry` still reports the original branch commit as `-` (matched), so `unmerged == 0` and prune reports "cherry-clean" / removes the branch — even though the tip of the default branch no longer contains that change. The historical commit is still reachable in origin's history (not literally destroyed), but the "provably upstream" label is misleading for this case. Worth a doc caveat at minimum; a stronger fix would check the matched upstream commit is itself an ancestor of the current `origin/<default>` tip. Minor, non-blocking: - `--keep-branches` still prints `remove+branch` as the verdict even though the branch survives (cmd/agentws/prune.go:210) — cosmetically misleading, not unsafe. - `ListPRs` pagination caps at `maxPRPages*prPageSize` = 1000 PRs (internal/agent/gitea.go) with no warning if truncated; a repo past that could silently under-report PR state for old branches. Everything else matches the stated design: dirty and open-PR keep-precedence is correctly enforced and tested (`TestPruneNeverTouchesDirtyWorktree`, `TestPruneKeepsOpenPR`), `head.label` vs `refs/pull/<n>/head` matching is correct and tested both ways, dry-run performs no mutation, Gitea-unreachable degrades to git-only signals with a warning rather than a silent input, and an unresolvable default branch keeps the whole repo's worktrees. `GitFetchPrune` is tightly scoped to this command's needs and fine to ship in the same PR.

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

Require git proof before prune deletes a branch ... c1c02c01cf

A merged or closed PR no longer authorises a delete on its own: HEAD must
be contained in the PR's head commit or in origin/<branch>, otherwise the
worktree goes and the branch stays. Branch deletion runs `git branch -d`
first and falls back to -D only for a proven branch.

Reword the cherry check to say patches reached the default branch's
history, print the verdict --keep-branches will actually perform, and warn
when a PR listing hits the pagination cap instead of reading it as "no PR".

unkin-agent commented 2026-09-10 00:10:38 +10:00

Author

Member

Copy Link

Copy Source

Reviewed the containment-proof logic in prune.go against the stated goals. The proven flag, the -d-then--D fallback, keep-precedence, the truncation sentinel, dry-run, and reason-string wording all check out — every path that sets verdictRemoveBranch also sets proven = true, headContainedIn and GitIsAncestor read an unresolvable ref as not-contained (verified by TestGitIsAncestor's bogus-ref case), the truncated-listing path leaves prsKnown false so an unlisted branch falls to "PR state unknown" (kept), and git branch -d is always tried before any -D.

One gap: newRepoCtx (prune.go:143-171) tolerates a failed git fetch --prune — it prints a warning and continues on stale local refs, with no flag recording that the fetch didn't happen this run. GitRemoteBranchExists is documented as "accurate only after a pruning fetch" (git.go), but classify's onOrigin check (prune.go:264) and the two headContainedIn(wt.path, remote) containment proofs it feeds (prune.go:268, 272-273, the merged-PR-via-origin and closed-PR paths) call it unconditionally, regardless of whether this run's fetch actually succeeded.

Concretely: if origin/<branch> was deleted upstream (PR abandoned/rejected without a Gitea "merged" record, or manually deleted) and this run's fetch fails while a stale refs/remotes/origin/<branch> survives from an earlier successful fetch, the closed-PR path can mark the branch delete "proven" against a ref this run never actually verified. The local branch we then delete was the only durable copy; the stale remote-tracking ref that "proved" containment is itself due to disappear on the next successful --prune fetch.

Suggest: have newRepoCtx record whether the fetch succeeded, and refuse the onOrigin/remote-branch containment proofs (falling through to the unproven verdictRemove case, worktree-only) when it didn't. The origin/<default> ancestor/cherry checks are lower risk since a default branch normally only grows, but the same staleness caveat applies to them in principle.

Everything else looked solid — nice test coverage of the "commits after merge" / "commits beyond origin" over-claim cases and the Gitea-unreachable degradation path.

Reviewed the containment-proof logic in `prune.go` against the stated goals. The `proven` flag, the `-d`-then-`-D` fallback, keep-precedence, the truncation sentinel, dry-run, and reason-string wording all check out — every path that sets `verdictRemoveBranch` also sets `proven = true`, `headContainedIn` and `GitIsAncestor` read an unresolvable ref as not-contained (verified by `TestGitIsAncestor`'s bogus-ref case), the truncated-listing path leaves `prsKnown` false so an unlisted branch falls to "PR state unknown" (kept), and `git branch -d` is always tried before any `-D`. One gap: `newRepoCtx` (prune.go:143-171) tolerates a failed `git fetch --prune` — it prints a warning and continues on stale local refs, with no flag recording that the fetch didn't happen this run. `GitRemoteBranchExists` is documented as "accurate only after a pruning fetch" (git.go), but `classify`'s `onOrigin` check (prune.go:264) and the two `headContainedIn(wt.path, remote)` containment proofs it feeds (prune.go:268, 272-273, the merged-PR-via-origin and closed-PR paths) call it unconditionally, regardless of whether this run's fetch actually succeeded. Concretely: if `origin/<branch>` was deleted upstream (PR abandoned/rejected without a Gitea "merged" record, or manually deleted) and this run's fetch fails while a stale `refs/remotes/origin/<branch>` survives from an earlier successful fetch, the closed-PR path can mark the branch delete "proven" against a ref this run never actually verified. The local branch we then delete was the only durable copy; the stale remote-tracking ref that "proved" containment is itself due to disappear on the next successful `--prune` fetch. Suggest: have `newRepoCtx` record whether the fetch succeeded, and refuse the `onOrigin`/remote-branch containment proofs (falling through to the unproven `verdictRemove` case, worktree-only) when it didn't. The `origin/<default>` ancestor/cherry checks are lower risk since a default branch normally only grows, but the same staleness caveat applies to them in principle. Everything else looked solid — nice test coverage of the "commits after merge" / "commits beyond origin" over-claim cases and the Gitea-unreachable degradation path.

unkin-agent added 1 commit 2026-09-10 00:19:01 +10:00

Distrust origin/<branch> when prune's fetch fails ... 387653a3c0

A stale remote-tracking ref survives a failed fetch and the next successful
--prune deletes it, so it cannot prove a branch's commits survive upstream.
Record whether the pruning fetch succeeded and gate the origin/<branch>
existence and containment proofs on it; a failed fetch removes the worktree and
keeps the branch. Local-object proofs and the merged head SHA are unaffected.

benvin merged commit 6d0e954cce into main 2026-09-10 21:50:20 +10:00

benvin deleted branch benvin/agentws-prune 2026-09-10 21:50:21 +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#13