From c1c02c01cfe5b41c857b3b457f92aad1f121a31f Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Thu, 10 Sep 2026 00:00:22 +1000 Subject: [PATCH] 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/, 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". --- AGENTS.md | 4 +- README.md | 32 ++++++--- cmd/agentws/main.go | 22 +++++-- cmd/agentws/prune.go | 82 ++++++++++++++++------- cmd/agentws/prune_test.go | 121 ++++++++++++++++++++++++++++++++-- internal/agent/client_test.go | 24 +++++++ internal/agent/gitea.go | 11 +++- 7 files changed, 249 insertions(+), 47 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index bc47448..6f489ce 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -192,6 +192,8 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`. is deleted, which merging does, so `head.ref` matching misses every merged PR. Git signals (`merge-base --is-ancestor`, `git cherry`) are authoritative and offline-safe; an unreachable Gitea only means no branch gets deleted without - git proof. + git proof. A PR's state never authorises a branch delete on its own — HEAD + must be contained in the PR's head commit or in `origin/`, otherwise + the worktree goes and the branch stays. - CI "combined status" comes from `/commits/{sha}/status`; an empty head SHA yields an empty state without an API call. diff --git a/README.md b/README.md index 9056b10..889f098 100644 --- a/README.md +++ b/README.md @@ -124,17 +124,31 @@ agentws token |---|---| | uncommitted or untracked changes | keep | | branch has an open PR | keep | -| tip contained in `origin/`, or `git cherry` finds no unmerged patch, or its PR is merged | remove worktree + local branch | -| PR closed unmerged and the branch is still on origin | remove worktree + local branch | +| tip contained in `origin/` | remove worktree + local branch | +| every commit patch-equivalent to one in `origin/`'s history | remove worktree + local branch | +| PR merged **and** HEAD contained in the PR's head commit (or in `origin/`) | remove worktree + local branch | +| PR closed **and** HEAD contained in `origin/` | remove worktree + local branch | | anything else | remove worktree, keep the branch | -The git checks are authoritative and work offline: these repos squash-merge, so -a merged branch's commits carry different SHAs upstream and a plain -`rev-list origin/..HEAD` count proves nothing. Gitea PR state only adds -to the git answer — when it cannot be reached, prune says so and never deletes a -branch it could not prove. Matching a branch to its PR uses `head.label`, since -Gitea rewrites `head.ref` to `refs/pull//head` once the branch is deleted on -merge. +A branch is deleted only where git proves its commits survive elsewhere. PR +state alone never authorises that: a merged or closed PR whose branch picked up +commits since keeps its branch, because those commits exist nowhere but here. +The delete runs `git branch -d` first so git's own unmerged check is a backstop, +falling back to `-D` only for a proven branch — squash merges keep the guard +tripping even once the work has landed. + +Patch equivalence comes from `git cherry`, which these squash-merging repos need +because a merged branch's commits carry different SHAs upstream. It proves the +patches reached the default branch's history at some point — a later revert +still counts — not that they stand at its tip. + +Gitea PR state only adds to the git answer: when it cannot be reached, prune +says so and never deletes a branch it could not prove, and a PR listing that +hits the pagination cap is reported rather than read as "no PR". Matching a +branch to its PR uses `head.label`, since Gitea rewrites `head.ref` to +`refs/pull//head` once the branch is deleted on merge. + +`--keep-branches` removes worktrees only, and its verdicts print as `remove`. ### Auth / credential-helper design diff --git a/cmd/agentws/main.go b/cmd/agentws/main.go index c4857cf..27b3258 100644 --- a/cmd/agentws/main.go +++ b/cmd/agentws/main.go @@ -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 } } diff --git a/cmd/agentws/prune.go b/cmd/agentws/prune.go index 76eca29..a47461c 100644 --- a/cmd/agentws/prune.go +++ b/cmd/agentws/prune.go @@ -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 } diff --git a/cmd/agentws/prune_test.go b/cmd/agentws/prune_test.go index 95ce37f..9c99709 100644 --- a/cmd/agentws/prune_test.go +++ b/cmd/agentws/prune_test.go @@ -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) { diff --git a/internal/agent/client_test.go b/internal/agent/client_test.go index bbda10d..47757b8 100644 --- a/internal/agent/client_test.go +++ b/internal/agent/client_test.go @@ -290,6 +290,30 @@ func TestListPRsPaginates(t *testing.T) { } } +// A listing that fills every page is truncated: the caller must be told rather +// than treating a partial view as the whole repo. +func TestListPRsReportsTruncation(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls", func(w http.ResponseWriter, r *http.Request) { + full := make([]string, 0, prPageSize) + for i := 0; i < prPageSize; i++ { + full = append(full, fmt.Sprintf(`{"number":%s,"state":"open"}`, r.URL.Query().Get("page"))) + } + _, _ = io.WriteString(w, "["+strings.Join(full, ",")+"]") + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + c := &GiteaClient{BaseURL: srv.URL, HTTP: srv.Client()} + prs, err := c.ListPRs("unkin/repo", "all") + if !errors.Is(err, ErrPRListTruncated) { + t.Fatalf("ListPRs err = %v, want ErrPRListTruncated", err) + } + if len(prs) != maxPRPages*prPageSize { + t.Errorf("got %d PRs, want %d", len(prs), maxPRPages*prPageSize) + } +} + func TestGiteaAPIError(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("/api/v1/repos/unkin/repo/pulls", func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/agent/gitea.go b/internal/agent/gitea.go index 82e8288..7a3be19 100644 --- a/internal/agent/gitea.go +++ b/internal/agent/gitea.go @@ -150,8 +150,13 @@ const ( maxPRPages = 20 ) +// ErrPRListTruncated reports that a listing hit the page cap, so the returned +// pull requests are only the most recent ones and older PRs went unseen. +var ErrPRListTruncated = errors.New("pull request listing truncated at the page cap") + // ListPRs lists a repo's pull requests in the given state ("open", "closed" or -// "all"), following pagination. +// "all"), following pagination. A repo with more PRs than the page cap returns +// the PRs it did read alongside ErrPRListTruncated. func (c *GiteaClient) ListPRs(repoPath, state string) ([]PullRequest, error) { if state == "" { state = "all" @@ -165,10 +170,10 @@ func (c *GiteaClient) ListPRs(repoPath, state string) ([]PullRequest, error) { } all = append(all, batch...) if len(batch) < prPageSize { - break + return all, nil } } - return all, nil + return all, fmt.Errorf("%s: %w after %d pull requests", repoPath, ErrPRListTruncated, len(all)) } // PRHeadBranch returns the branch a PR was opened from. Gitea rewrites head.ref