Require git proof before prune deletes a branch

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".
2026-09-10 00:00:22 +10:00
parent 4bbeaae8f0
commit c1c02c01cf
7 changed files with 249 additions and 47 deletions
@@ -296,7 +296,8 @@ func newRmCmd() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			return removeWorktree(cmd.OutOrStdout(), wt, deleteBranch)
+			// Naming one worktree to delete is explicit, so rm keeps the force fallback.
+			return removeWorktree(cmd.OutOrStdout(), wt, deleteBranch, true)
 		},
 	}
 	cmd.Flags().BoolVar(&deleteBranch, "delete-branch", false, "Also delete the local branch after removing the worktree")
@@ -318,13 +319,16 @@ func resolveWorktree(target string) (managedWt, error) {
 	return managedWt{}, fmt.Errorf("no managed worktree matching %q (try `agentws list`)", target)
 }

-func removeWorktree(out io.Writer, wt managedWt, deleteBranch bool) error {
+// removeWorktree removes a managed worktree and, when asked, its local branch.
+// forceBranch overrides git's unmerged-branch guard, so only a caller that
+// proved the commits survive elsewhere may set it.
+func removeWorktree(out io.Writer, wt managedWt, deleteBranch, forceBranch bool) error {
 	if err := agent.GitWorktreeRemove(wt.srcDir, wt.path, true); err != nil {
 		return err
 	}
 	_, _ = fmt.Fprintf(out, "removed worktree %s\n", wt.path)
 	if deleteBranch {
-		if err := agent.GitDeleteBranch(wt.srcDir, wt.branch, true); err != nil {
+		if err := deleteLocalBranch(wt, forceBranch); err != nil {
 			return err
 		}
 		_, _ = fmt.Fprintf(out, "deleted branch %s\n", wt.branch)
@@ -336,6 +340,16 @@ func removeWorktree(out io.Writer, wt managedWt, deleteBranch bool) error {
 	return agent.GitWorktreePrune(wt.srcDir)
 }

+// deleteLocalBranch tries the guarded delete first so git refuses to drop
+// unmerged commits on its own; force is a fallback, never the first attempt.
+func deleteLocalBranch(wt managedWt, force bool) error {
+	err := agent.GitDeleteBranch(wt.srcDir, wt.branch, false)
+	if err == nil || !force {
+		return err
+	}
+	return agent.GitDeleteBranch(wt.srcDir, wt.branch, true)
+}
+
 // --- clean ----------------------------------------------------------------

 func newCleanCmd() *cobra.Command {
@@ -354,7 +368,7 @@ func newCleanCmd() *cobra.Command {
 				return nil
 			}
 			for _, w := range managed {
-				if err := removeWorktree(out, w, false); err != nil {
+				if err := removeWorktree(out, w, false, false); err != nil {
 					return err
 				}
 			}
@@ -29,6 +29,9 @@ type pruneResult struct {
 	wt      managedWt
 	verdict string
 	reason  string
+	// proven records that git itself confirmed the branch's commits survive
+	// elsewhere; only then may a branch delete override git's own guard.
+	proven bool
 }

 // repoCtx is the per-repo state classification is decided against.
@@ -105,7 +108,7 @@ func runPrune(out io.Writer, prs prLister, apply, keepBranches bool) error {
 	}

 	for _, r := range results {
-		_, _ = fmt.Fprintf(out, "%-44s %-34s %-14s %s\n", filepath.Base(r.wt.path), r.wt.branch, r.verdict, r.reason)
+		_, _ = fmt.Fprintf(out, "%-44s %-34s %-14s %s\n", filepath.Base(r.wt.path), r.wt.branch, plannedVerdict(r, keepBranches), r.reason)
 	}
 	if !apply {
 		_, _ = fmt.Fprintln(out, "dry run: nothing removed (pass --yes to apply)")
@@ -118,13 +121,22 @@ func runPrune(out io.Writer, prs prLister, apply, keepBranches bool) error {
 			continue
 		}
 		deleteBranch := r.verdict == verdictRemoveBranch && !keepBranches
-		if err := removeWorktree(out, r.wt, deleteBranch); err != nil {
+		if err := removeWorktree(out, r.wt, deleteBranch, r.proven); err != nil {
 			errs = append(errs, fmt.Errorf("%s: %w", r.wt.path, err))
 		}
 	}
 	return errors.Join(errs...)
 }

+// plannedVerdict is what will actually happen, so --keep-branches does not
+// print a branch deletion it will not perform.
+func plannedVerdict(r pruneResult, keepBranches bool) string {
+	if keepBranches && r.verdict == verdictRemoveBranch {
+		return verdictRemove
+	}
+	return r.verdict
+}
+
 // newRepoCtx refreshes a source repo and collects the signals prune classifies
 // against. A failed fetch or an unreachable Gitea is reported and tolerated:
 // the git-only signals still work offline.
@@ -144,12 +156,17 @@ func newRepoCtx(out io.Writer, prs prLister, srcDir string) (repoCtx, error) {
 		return ctx, nil
 	}
 	list, err := prs.ListPRs(repoPath(srcDir, repo), "all")
-	if err != nil {
+	switch {
+	case errors.Is(err, agent.ErrPRListTruncated):
+		// A branch missing from a partial listing must not read as "no PR".
+		_, _ = fmt.Fprintf(out, "warn: list PRs for %s: %v (older PRs unseen)\n", repo, err)
+		ctx.prs = prsByBranch(list)
+	case err != nil:
 		_, _ = fmt.Fprintf(out, "warn: list PRs for %s: %v (git signals only)\n", repo, err)
-		return ctx, nil
+	default:
+		ctx.prs = prsByBranch(list)
+		ctx.prsKnown = true
 	}
-	ctx.prs = prsByBranch(list)
-	ctx.prsKnown = true
 	return ctx, nil
 }

@@ -192,9 +209,21 @@ func supersedes(a, b agent.PullRequest) bool {
 	return a.Number > b.Number
 }

+// headContainedIn reports whether the worktree's HEAD is reachable from ref. A
+// ref that cannot be resolved proves nothing, so it reads as not contained.
+func headContainedIn(dir, ref string) bool {
+	if ref == "" {
+		return false
+	}
+	ok, err := agent.GitIsAncestor(dir, "HEAD", ref)
+	return err == nil && ok
+}
+
 // classify applies the prune precedence: dirty and open-PR worktrees are kept,
 // provably-upstream work loses its branch too, and anything unproven keeps its
-// branch so no commits become unreachable.
+// branch so no commits become unreachable. A PR's state alone never authorises
+// deleting a branch — git must confirm HEAD is contained in what merged or in
+// what origin still holds.
 func classify(wt managedWt, ctx repoCtx) (pruneResult, error) {
 	res := pruneResult{wt: wt}
 	dirty, err := agent.GitIsDirty(wt.path)
@@ -218,7 +247,7 @@ func classify(wt managedWt, ctx repoCtx) (pruneResult, error) {
 		return res, err
 	}
 	if contained {
-		res.verdict, res.reason = verdictRemoveBranch, "contained in "+upstream
+		res.verdict, res.reason, res.proven = verdictRemoveBranch, "contained in "+upstream, true
 		return res, nil
 	}
 	unmerged, err := agent.GitUnmergedCommits(wt.path, upstream, "HEAD")
@@ -226,25 +255,30 @@ func classify(wt managedWt, ctx repoCtx) (pruneResult, error) {
 		return res, err
 	}
 	if unmerged == 0 {
-		res.verdict, res.reason = verdictRemoveBranch, "cherry-clean against "+upstream
+		// git cherry proves the patches reached that history, not that they stand at its tip.
+		res.verdict, res.reason, res.proven = verdictRemoveBranch, "patch-equivalent commits in "+upstream+" history", true
 		return res, nil
 	}
-	if hasPR && pr.Merged {
-		res.verdict, res.reason = verdictRemoveBranch, fmt.Sprintf("PR merged #%d", pr.Number)
-		return res, nil
-	}
-	if hasPR {
-		if agent.GitRemoteBranchExists(ctx.srcDir, "origin", wt.branch) {
-			res.verdict, res.reason = verdictRemoveBranch, fmt.Sprintf("PR closed #%d, branch on origin", pr.Number)
-			return res, nil
-		}
+
+	remote := "origin/" + wt.branch
+	onOrigin := hasPR && agent.GitRemoteBranchExists(ctx.srcDir, "origin", wt.branch)
+	switch {
+	case hasPR && pr.Merged && headContainedIn(wt.path, pr.Head.Sha):
+		res.verdict, res.reason, res.proven = verdictRemoveBranch, fmt.Sprintf("PR merged #%d, HEAD contained in the merged head", pr.Number), true
+	case hasPR && pr.Merged && onOrigin && headContainedIn(wt.path, remote):
+		res.verdict, res.reason, res.proven = verdictRemoveBranch, fmt.Sprintf("PR merged #%d, HEAD contained in %s", pr.Number, remote), true
+	case hasPR && pr.Merged:
+		res.verdict, res.reason = verdictRemove, fmt.Sprintf("PR merged #%d, local commits not in the merged head", pr.Number)
+	case hasPR && onOrigin && headContainedIn(wt.path, remote):
+		res.verdict, res.reason, res.proven = verdictRemoveBranch, fmt.Sprintf("PR closed #%d, HEAD contained in %s", pr.Number, remote), true
+	case hasPR && onOrigin:
+		res.verdict, res.reason = verdictRemove, fmt.Sprintf("PR closed #%d, local commits not on %s", pr.Number, remote)
+	case hasPR:
 		res.verdict, res.reason = verdictRemove, fmt.Sprintf("PR closed #%d, branch gone", pr.Number)
-		return res, nil
-	}
-	res.verdict = verdictRemove
-	res.reason = "no PR"
-	if !ctx.prsKnown {
-		res.reason = "PR state unknown"
+	case ctx.prsKnown:
+		res.verdict, res.reason = verdictRemove, "no PR"
+	default:
+		res.verdict, res.reason = verdictRemove, "PR state unknown"
 	}
 	return res, nil
 }
@@ -3,6 +3,7 @@ package main
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -140,6 +141,11 @@ func mergedPR(number int, branch string) map[string]any {
 	}
 }

+func withHeadSha(pr map[string]any, sha string) map[string]any {
+	pr["head"].(map[string]any)["sha"] = sha
+	return pr
+}
+
 func openPR(number int, branch string) map[string]any {
 	return map[string]any{
 		"number": number,
@@ -202,8 +208,9 @@ func TestPruneMergedPRWithDeletedBranch(t *testing.T) {
 	f := newFixture(t)
 	wt := f.addWorktree(t, "benvin/merged")
 	writeCommit(t, wt, "m.txt", "m\n", "work")
+	head := git(t, wt, "rev-parse", "HEAD")

-	srv := fakeGitea(t, mergedPR(3, "benvin/merged"))
+	srv := fakeGitea(t, withHeadSha(mergedPR(3, "benvin/merged"), head))
 	out := run(t, client(srv), true, false)
 	assertVerdict(t, out, "benvin/merged", verdictRemoveBranch, "PR merged #3")

@@ -260,7 +267,7 @@ func TestPruneCherryCleanBranch(t *testing.T) {

 	srv := fakeGitea(t)
 	out := run(t, client(srv), true, false)
-	assertVerdict(t, out, "benvin/squashed", verdictRemoveBranch, "cherry-clean")
+	assertVerdict(t, out, "benvin/squashed", verdictRemoveBranch, "patch-equivalent commits in origin/main history")

 	if agent.GitBranchExists(f.srcDir, "benvin/squashed") {
 		t.Error("cherry-clean branch should be deleted")
@@ -315,7 +322,7 @@ func TestPruneClosedPRWithBranchOnOrigin(t *testing.T) {

 	srv := fakeGitea(t, closedPR(6, "benvin/closed"))
 	out := run(t, client(srv), true, false)
-	assertVerdict(t, out, "benvin/closed", verdictRemoveBranch, "PR closed #6, branch on origin")
+	assertVerdict(t, out, "benvin/closed", verdictRemoveBranch, "PR closed #6, HEAD contained in origin/benvin/closed")

 	if exists(wt) {
 		t.Error("worktree should have been removed")
@@ -367,8 +374,9 @@ func TestPruneDryRunChangesNothing(t *testing.T) {
 	contained := f.addWorktree(t, "benvin/contained")
 	merged := f.addWorktree(t, "benvin/merged")
 	writeCommit(t, merged, "m.txt", "m\n", "work")
+	head := git(t, merged, "rev-parse", "HEAD")

-	srv := fakeGitea(t, mergedPR(8, "benvin/merged"))
+	srv := fakeGitea(t, withHeadSha(mergedPR(8, "benvin/merged"), head))
 	out := run(t, client(srv), false, false)

 	if !strings.Contains(out, "dry run") {
@@ -384,14 +392,18 @@ func TestPruneDryRunChangesNothing(t *testing.T) {
 	}
 }

-// --keep-branches removes worktrees but leaves every branch alone.
+// --keep-branches removes worktrees but leaves every branch alone, and the
+// printed verdict says so.
 func TestPruneKeepBranches(t *testing.T) {
 	f := newFixture(t)
 	wt := f.addWorktree(t, "benvin/contained")

 	srv := fakeGitea(t)
 	out := run(t, client(srv), true, true)
-	assertVerdict(t, out, "benvin/contained", verdictRemoveBranch, "contained")
+	assertVerdict(t, out, "benvin/contained", verdictRemove, "contained")
+	if strings.Contains(out, verdictRemoveBranch) {
+		t.Errorf("--keep-branches must not print a branch-deleting verdict:\n%s", out)
+	}

 	if exists(wt) {
 		t.Error("worktree should have been removed")
@@ -439,6 +451,103 @@ func TestPruneNoWorktrees(t *testing.T) {
 	}
 }

+// Commits made after the PR merged exist nowhere else, so a merged PR alone
+// must not authorise deleting the branch.
+func TestPruneMergedPRWithCommitsAfterMerge(t *testing.T) {
+	f := newFixture(t)
+	wt := f.addWorktree(t, "benvin/ahead")
+	writeCommit(t, wt, "a.txt", "a\n", "merged work")
+	merged := git(t, wt, "rev-parse", "HEAD")
+	writeCommit(t, wt, "b.txt", "b\n", "work after the merge")
+
+	srv := fakeGitea(t, withHeadSha(mergedPR(9, "benvin/ahead"), merged))
+	out := run(t, client(srv), true, false)
+	assertVerdict(t, out, "benvin/ahead", verdictRemove, "PR merged #9")
+
+	if exists(wt) {
+		t.Error("worktree should have been removed")
+	}
+	if !agent.GitBranchExists(f.srcDir, "benvin/ahead") {
+		t.Error("branch with commits beyond the merged head must survive")
+	}
+}
+
+// HEAD proven contained in the merged head still loses its branch.
+func TestPruneMergedPRContainedInMergedHead(t *testing.T) {
+	f := newFixture(t)
+	wt := f.addWorktree(t, "benvin/landed")
+	writeCommit(t, wt, "a.txt", "a\n", "work")
+	head := git(t, wt, "rev-parse", "HEAD")
+
+	srv := fakeGitea(t, withHeadSha(mergedPR(10, "benvin/landed"), head))
+	out := run(t, client(srv), true, false)
+	assertVerdict(t, out, "benvin/landed", verdictRemoveBranch, "HEAD contained in the merged head")
+
+	if exists(wt) {
+		t.Error("worktree should have been removed")
+	}
+	if agent.GitBranchExists(f.srcDir, "benvin/landed") {
+		t.Error("branch contained in the merged head should be deleted")
+	}
+}
+
+// A surviving remote branch only covers what was pushed to it; later local
+// commits keep the branch.
+func TestPruneClosedPRWithCommitsBeyondOrigin(t *testing.T) {
+	f := newFixture(t)
+	wt := f.addWorktree(t, "benvin/beyond")
+	writeCommit(t, wt, "c.txt", "c\n", "pushed work")
+	git(t, wt, "push", "origin", "benvin/beyond")
+	writeCommit(t, wt, "d.txt", "d\n", "local only")
+
+	srv := fakeGitea(t, closedPR(11, "benvin/beyond"))
+	out := run(t, client(srv), true, false)
+	assertVerdict(t, out, "benvin/beyond", verdictRemove, "local commits not on origin/benvin/beyond")
+
+	if exists(wt) {
+		t.Error("worktree should have been removed")
+	}
+	if !agent.GitBranchExists(f.srcDir, "benvin/beyond") {
+		t.Error("branch with commits beyond origin must survive")
+	}
+}
+
+// truncatedLister stands in for a repo with more PRs than the listing cap.
+type truncatedLister struct{ prs []agent.PullRequest }
+
+func (l truncatedLister) ListPRs(string, string) ([]agent.PullRequest, error) {
+	return l.prs, fmt.Errorf("unkin/repo: %w after 1000 pull requests", agent.ErrPRListTruncated)
+}
+
+// A truncated listing still classifies the PRs it saw, but a branch missing
+// from it reads as unknown rather than as having no PR.
+func TestPruneWarnsOnTruncatedPRListing(t *testing.T) {
+	f := newFixture(t)
+	listed := f.addWorktree(t, "benvin/listed")
+	writeCommit(t, listed, "a.txt", "a\n", "work")
+	head := git(t, listed, "rev-parse", "HEAD")
+	unlisted := f.addWorktree(t, "benvin/unlisted")
+	writeCommit(t, unlisted, "b.txt", "b\n", "work")
+
+	var pr agent.PullRequest
+	pr.Number, pr.State, pr.Merged = 12, "closed", true
+	pr.Head.Label, pr.Head.Sha = "benvin/listed", head
+
+	out := run(t, truncatedLister{prs: []agent.PullRequest{pr}}, true, false)
+	if !strings.Contains(out, "truncated") || !strings.Contains(out, "older PRs unseen") {
+		t.Errorf("output should warn about the truncated listing:\n%s", out)
+	}
+	assertVerdict(t, out, "benvin/listed", verdictRemoveBranch, "PR merged #12")
+	assertVerdict(t, out, "benvin/unlisted", verdictRemove, "PR state unknown")
+
+	if agent.GitBranchExists(f.srcDir, "benvin/listed") {
+		t.Error("branch of a merged PR seen in the listing should be deleted")
+	}
+	if !agent.GitBranchExists(f.srcDir, "benvin/unlisted") {
+		t.Error("branch missing from a truncated listing must survive")
+	}
+}
+
 // Managed repos are not all under AGENTWS_OWNER, so the Gitea path comes from
 // origin's URL; a non-Gitea remote falls back to the configured owner.
 func TestRepoPathFollowsOrigin(t *testing.T) {