From 4bbeaae8f0e435b40dc93e1692cdf67cdac1cefa Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Wed, 9 Sep 2026 23:41:09 +1000 Subject: [PATCH 1/3] add agentws prune 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 --- AGENTS.md | 9 +- README.md | 25 ++ cmd/agentws/main.go | 2 + cmd/agentws/prune.go | 250 +++++++++++++++++++ cmd/agentws/prune_test.go | 456 ++++++++++++++++++++++++++++++++++ internal/agent/client_test.go | 71 ++++++ internal/agent/git.go | 67 +++++ internal/agent/git_test.go | 128 ++++++++++ internal/agent/gitea.go | 55 +++- internal/agent/parse.go | 46 ++++ internal/agent/parse_test.go | 49 ++++ 11 files changed, 1156 insertions(+), 2 deletions(-) create mode 100644 cmd/agentws/prune.go create mode 100644 cmd/agentws/prune_test.go diff --git a/AGENTS.md b/AGENTS.md index 581c161..bc47448 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,7 +19,7 @@ like repospawner can run these tools as itself. repos into the source root (`~/src/prodenv/`), creates worktrees under the worktree root (`~/.cache/agentws/__`), and authenticates clone/fetch/push via an ephemeral credential helper. Subcommands: `new`, - `list`, `rm`, `clean`, `token`, `credential`. + `list`, `rm`, `prune`, `clean`, `token`, `credential`. All tools are separate `main` packages under `cmd/` and share the `internal/agent` package (Vault AppRole login, Gitea REST client, PR-ref @@ -31,6 +31,7 @@ parsing, watch-state comparison, git worktree helpers). cmd/agentpr/main.go # agentpr CLI (pr create / pr comment / whoami) cmd/watchpr/main.go # watchpr CLI (poll + meaningful-change exit) cmd/agentws/main.go # agentws CLI (new / list / rm / clean / token / credential) +cmd/agentws/prune.go # agentws prune (classify worktrees, remove the safe ones) cmd/agentvault/main.go # agentvault CLI (seed-outpost / seed-oauth) internal/agent/ # shared plumbing: token.go # env config + in-process Gitea-token cache @@ -186,5 +187,11 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`. `seed-oauth` reports key names only. - `--rotate` regenerates the `client_secret` too, which then no longer matches the IdP provider unless that is rotated alongside. +- `agentws prune` is a dry run unless `--yes`. It matches a branch to its PR on + `head.label`: Gitea rewrites `head.ref` to `refs/pull//head` once the branch + 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. - 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 c90f774..9056b10 100644 --- a/README.md +++ b/README.md @@ -104,6 +104,11 @@ agentws list agentws rm benvin/my-change agentws rm ~/.cache/agentws/argocd-apps__benvin-my-change --delete-branch +# Classify every managed worktree; dry run unless --yes is given +agentws prune +agentws prune --yes +agentws prune --yes --keep-branches + # Remove every managed worktree and prune each source repo agentws clean @@ -111,6 +116,26 @@ agentws clean agentws token ``` +### prune + +`agentws prune` decides, per worktree, whether its work is safely upstream: + +| Signal (first match wins) | Verdict | +|---|---| +| 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 | +| 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. + ### Auth / credential-helper design Gitea tokens minted from Vault are short-lived (~1h), so `agentws` never diff --git a/cmd/agentws/main.go b/cmd/agentws/main.go index c9c6557..c4857cf 100644 --- a/cmd/agentws/main.go +++ b/cmd/agentws/main.go @@ -13,6 +13,7 @@ // agentws new [--branch benvin/] [--from ] // agentws list // agentws rm [--delete-branch] +// agentws prune [--yes] [--keep-branches] // agentws clean // agentws token // agentws credential get # git credential-helper protocol on stdin @@ -54,6 +55,7 @@ func newRootCmd() *cobra.Command { newNewCmd(), newListCmd(), newRmCmd(), + newPruneCmd(), newCleanCmd(), newTokenCmd(), newCredentialCmd(), diff --git a/cmd/agentws/prune.go b/cmd/agentws/prune.go new file mode 100644 index 0000000..76eca29 --- /dev/null +++ b/cmd/agentws/prune.go @@ -0,0 +1,250 @@ +package main + +import ( + "errors" + "fmt" + "io" + "path/filepath" + "sort" + + "git.unkin.net/unkin/agent-tools/internal/agent" + + "github.com/spf13/cobra" +) + +// Verdicts a worktree can be classified into. +const ( + verdictKeep = "keep" + verdictRemove = "remove" + verdictRemoveBranch = "remove+branch" +) + +// prLister is the slice of the Gitea client prune needs, so tests can drive +// classification without a live server. +type prLister interface { + ListPRs(repoPath, state string) ([]agent.PullRequest, error) +} + +type pruneResult struct { + wt managedWt + verdict string + reason string +} + +// repoCtx is the per-repo state classification is decided against. +type repoCtx struct { + srcDir string + defBranch string + prs map[string]agent.PullRequest + prsKnown bool +} + +func newPruneCmd() *cobra.Command { + var apply, keepBranches bool + cmd := &cobra.Command{ + Use: "prune", + Short: "Classify managed worktrees and remove the ones whose work is safely upstream", + Long: "prune inspects every managed worktree, classifies it against git and its Gitea\npull request, and removes the ones whose work is provably upstream. It is a dry\nrun unless --yes is given.", + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { + return runPrune(cmd.OutOrStdout(), pruneClient(), apply, keepBranches) + }, + } + f := cmd.Flags() + f.BoolVar(&apply, "yes", false, "Actually remove worktrees (default is a dry run)") + f.BoolVar(&keepBranches, "keep-branches", false, "Never delete a local branch, whatever the classification") + return cmd +} + +// pruneClient builds a Gitea client, falling back to anonymous access when no +// token can be minted; prune degrades to git-only signals if that fails too. +func pruneClient() prLister { + tok, err := agent.GiteaToken() + if err != nil { + tok = "" + } + return agent.NewGiteaClient(tok) +} + +func runPrune(out io.Writer, prs prLister, apply, keepBranches bool) error { + managed, err := managedWorktrees() + if err != nil { + return err + } + if len(managed) == 0 { + _, _ = fmt.Fprintln(out, "no managed worktrees") + return nil + } + + byRepo := map[string][]managedWt{} + for _, w := range managed { + byRepo[w.srcDir] = append(byRepo[w.srcDir], w) + } + srcDirs := make([]string, 0, len(byRepo)) + for dir := range byRepo { + srcDirs = append(srcDirs, dir) + } + sort.Strings(srcDirs) + + var results []pruneResult + for _, srcDir := range srcDirs { + ctx, err := newRepoCtx(out, prs, srcDir) + if err != nil { + for _, w := range byRepo[srcDir] { + results = append(results, pruneResult{wt: w, verdict: verdictKeep, reason: "repo state unknown: " + err.Error()}) + } + continue + } + for _, w := range byRepo[srcDir] { + res, err := classify(w, ctx) + if err != nil { + res = pruneResult{wt: w, verdict: verdictKeep, reason: "inspection failed: " + err.Error()} + } + results = append(results, res) + } + } + + for _, r := range results { + _, _ = fmt.Fprintf(out, "%-44s %-34s %-14s %s\n", filepath.Base(r.wt.path), r.wt.branch, r.verdict, r.reason) + } + if !apply { + _, _ = fmt.Fprintln(out, "dry run: nothing removed (pass --yes to apply)") + return nil + } + + var errs []error + for _, r := range results { + if r.verdict == verdictKeep { + continue + } + deleteBranch := r.verdict == verdictRemoveBranch && !keepBranches + if err := removeWorktree(out, r.wt, deleteBranch); err != nil { + errs = append(errs, fmt.Errorf("%s: %w", r.wt.path, err)) + } + } + return errors.Join(errs...) +} + +// 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. +func newRepoCtx(out io.Writer, prs prLister, srcDir string) (repoCtx, error) { + ctx := repoCtx{srcDir: srcDir, prs: map[string]agent.PullRequest{}} + repo := filepath.Base(srcDir) + if err := agent.GitFetchPrune(srcDir, "origin", credentialHelperArgs()...); err != nil { + _, _ = fmt.Fprintf(out, "warn: fetch %s: %v (using local refs)\n", repo, err) + } + def, err := agent.GitRemoteDefaultBranch(srcDir, "origin") + if err != nil { + return repoCtx{}, err + } + ctx.defBranch = def + + if prs == nil { + return ctx, nil + } + list, err := prs.ListPRs(repoPath(srcDir, repo), "all") + if err != nil { + _, _ = fmt.Fprintf(out, "warn: list PRs for %s: %v (git signals only)\n", repo, err) + return ctx, nil + } + ctx.prs = prsByBranch(list) + ctx.prsKnown = true + return ctx, nil +} + +// repoPath is the Gitea "owner/repo" for a checkout, read from origin's URL +// because not every managed repo lives under AGENTWS_OWNER. +func repoPath(srcDir, repo string) string { + url, err := agent.GitRemoteURL(srcDir, "origin") + if err == nil && agent.RemoteHost(url) == giteaHost() { + if path, err := agent.RepoPathFromRemoteURL(url); err == nil { + return path + } + } + return owner() + "/" + repo +} + +// prsByBranch indexes PRs by head branch, preferring an open PR and otherwise +// the most recent one when a branch has been used more than once. +func prsByBranch(list []agent.PullRequest) map[string]agent.PullRequest { + out := map[string]agent.PullRequest{} + for _, pr := range list { + branch := agent.PRHeadBranch(pr) + if branch == "" { + continue + } + if cur, ok := out[branch]; ok && !supersedes(pr, cur) { + continue + } + out[branch] = pr + } + return out +} + +func supersedes(a, b agent.PullRequest) bool { + if a.IsOpen() != b.IsOpen() { + return a.IsOpen() + } + if a.Merged != b.Merged { + return a.Merged + } + return a.Number > b.Number +} + +// 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. +func classify(wt managedWt, ctx repoCtx) (pruneResult, error) { + res := pruneResult{wt: wt} + dirty, err := agent.GitIsDirty(wt.path) + if err != nil { + return res, err + } + if dirty { + res.verdict, res.reason = verdictKeep, "dirty" + return res, nil + } + + pr, hasPR := ctx.prs[wt.branch] + if hasPR && pr.IsOpen() { + res.verdict, res.reason = verdictKeep, fmt.Sprintf("PR open #%d", pr.Number) + return res, nil + } + + upstream := "origin/" + ctx.defBranch + contained, err := agent.GitIsAncestor(wt.path, "HEAD", upstream) + if err != nil { + return res, err + } + if contained { + res.verdict, res.reason = verdictRemoveBranch, "contained in "+upstream + return res, nil + } + unmerged, err := agent.GitUnmergedCommits(wt.path, upstream, "HEAD") + if err != nil { + return res, err + } + if unmerged == 0 { + res.verdict, res.reason = verdictRemoveBranch, "cherry-clean against "+upstream + 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 + } + 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" + } + return res, nil +} diff --git a/cmd/agentws/prune_test.go b/cmd/agentws/prune_test.go new file mode 100644 index 0000000..95ce37f --- /dev/null +++ b/cmd/agentws/prune_test.go @@ -0,0 +1,456 @@ +package main + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" + + "git.unkin.net/unkin/agent-tools/internal/agent" +) + +// fixture is a bare origin plus a source checkout named "repo" and a worktree +// root, wired so managedWorktrees() finds the worktrees created here. +type fixture struct { + root string + bare string + srcDir string + wtRoot string +} + +func git(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s (in %s): %v: %s", strings.Join(args, " "), dir, err, out) + } + return strings.TrimSpace(string(out)) +} + +func newFixture(t *testing.T) *fixture { + t.Helper() + root := t.TempDir() + f := &fixture{ + root: root, + bare: filepath.Join(root, "origin.git"), + srcDir: filepath.Join(root, "src", "repo"), + wtRoot: filepath.Join(root, "worktrees"), + } + if err := os.MkdirAll(filepath.Join(root, "src"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(f.wtRoot, 0o755); err != nil { + t.Fatal(err) + } + git(t, root, "init", "--bare", "-b", "main", f.bare) + + seed := filepath.Join(root, "seed") + git(t, root, "init", "-b", "main", seed) + identity(t, seed) + writeCommit(t, seed, "README.md", "hi\n", "init") + git(t, seed, "remote", "add", "origin", f.bare) + git(t, seed, "push", "-u", "origin", "main") + + git(t, filepath.Join(root, "src"), "clone", f.bare, f.srcDir) + identity(t, f.srcDir) + + t.Setenv("AGENTWS_ROOT", f.wtRoot) + t.Setenv("AGENTWS_SRC_ROOT", filepath.Join(root, "src")) + t.Setenv("AGENTWS_OWNER", "unkin") + return f +} + +func identity(t *testing.T, dir string) { + t.Helper() + git(t, dir, "config", "user.email", "test@example.com") + git(t, dir, "config", "user.name", "Test") +} + +func writeCommit(t *testing.T, dir, name, content, msg string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", msg) +} + +// addWorktree creates a managed worktree for branch and returns its path. +func (f *fixture) addWorktree(t *testing.T, branch string) string { + t.Helper() + path := filepath.Join(f.wtRoot, agent.WorktreeDirName("repo", branch)) + git(t, f.srcDir, "worktree", "add", path, "-b", branch, "origin/main") + identity(t, path) + return path +} + +// landUpstream commits content on origin's main, mimicking a squash merge: the +// same patch arrives upstream under a different SHA. +func (f *fixture) landUpstream(t *testing.T, name, content, msg string) { + t.Helper() + seed := filepath.Join(f.root, "seed") + git(t, seed, "pull", "--ff-only", "origin", "main") + writeCommit(t, seed, name, content, msg) + git(t, seed, "push", "origin", "main") +} + +// fakeGitea serves the pulls listing for unkin/repo with the given PR bodies. +func fakeGitea(t *testing.T, prs ...map[string]any) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls", func(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("page") != "1" { + _, _ = w.Write([]byte("[]")) + return + } + body, err := json.Marshal(prs) + if err != nil { + t.Errorf("marshal PRs: %v", err) + } + _, _ = w.Write(body) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv +} + +func client(srv *httptest.Server) prLister { + return &agent.GiteaClient{BaseURL: srv.URL, HTTP: srv.Client()} +} + +// mergedPR mimics Gitea after a merge: the branch is deleted, so head.ref +// becomes refs/pull//head and only head.label still names the branch. +func mergedPR(number int, branch string) map[string]any { + return map[string]any{ + "number": number, + "state": "closed", + "merged": true, + "head": map[string]any{ + "ref": "refs/pull/" + strconv.Itoa(number) + "/head", + "label": branch, + }, + } +} + +func openPR(number int, branch string) map[string]any { + return map[string]any{ + "number": number, + "state": "open", + "merged": false, + "head": map[string]any{"ref": branch, "label": branch}, + } +} + +func closedPR(number int, branch string) map[string]any { + return map[string]any{ + "number": number, + "state": "closed", + "merged": false, + "head": map[string]any{"ref": branch, "label": branch}, + } +} + +func run(t *testing.T, prs prLister, apply, keepBranches bool) string { + t.Helper() + var out bytes.Buffer + if err := runPrune(&out, prs, apply, keepBranches); err != nil { + t.Fatalf("runPrune: %v\n%s", err, out.String()) + } + return out.String() +} + +func lineFor(t *testing.T, out, branch string) string { + t.Helper() + for _, line := range strings.Split(out, "\n") { + if strings.Contains(line, " "+branch+" ") { + return line + } + } + t.Fatalf("no line for branch %q in:\n%s", branch, out) + return "" +} + +func assertVerdict(t *testing.T, out, branch, verdict, reason string) { + t.Helper() + line := lineFor(t, out, branch) + fields := strings.Fields(line) + if len(fields) < 3 || fields[2] != verdict { + t.Errorf("branch %s: verdict line %q, want verdict %q", branch, line, verdict) + } + if reason != "" && !strings.Contains(line, reason) { + t.Errorf("branch %s: line %q, want reason containing %q", branch, line, reason) + } +} + +func exists(path string) bool { + _, err := os.Stat(path) + return err == nil +} + +// A merged PR's branch is deleted on merge, so its head.ref reads +// refs/pull//head; classification must still see the merge (via head.label) +// and remove the branch, not fall through to "no PR". +func TestPruneMergedPRWithDeletedBranch(t *testing.T) { + f := newFixture(t) + wt := f.addWorktree(t, "benvin/merged") + writeCommit(t, wt, "m.txt", "m\n", "work") + + srv := fakeGitea(t, mergedPR(3, "benvin/merged")) + out := run(t, client(srv), true, false) + assertVerdict(t, out, "benvin/merged", verdictRemoveBranch, "PR merged #3") + + if exists(wt) { + t.Errorf("worktree %s should have been removed", wt) + } + if agent.GitBranchExists(f.srcDir, "benvin/merged") { + t.Error("branch of a merged PR should be deleted") + } +} + +// The same PR without head.label: matching falls back to head.ref, which no +// longer names the branch, so prune must not guess it is merged — the worktree +// goes but the branch stays. +func TestPruneMergedPRWithoutLabelKeepsBranch(t *testing.T) { + f := newFixture(t) + wt := f.addWorktree(t, "benvin/merged") + writeCommit(t, wt, "m.txt", "m\n", "work") + + pr := mergedPR(3, "benvin/merged") + pr["head"].(map[string]any)["label"] = "" + srv := fakeGitea(t, pr) + out := run(t, client(srv), true, false) + assertVerdict(t, out, "benvin/merged", verdictRemove, "no PR") + + if !agent.GitBranchExists(f.srcDir, "benvin/merged") { + t.Error("branch must survive when the PR could not be matched") + } +} + +func TestPruneContainedBranch(t *testing.T) { + f := newFixture(t) + wt := f.addWorktree(t, "benvin/contained") + + srv := fakeGitea(t) + out := run(t, client(srv), true, false) + assertVerdict(t, out, "benvin/contained", verdictRemoveBranch, "contained in origin/main") + + if exists(wt) { + t.Error("contained worktree should have been removed") + } + if agent.GitBranchExists(f.srcDir, "benvin/contained") { + t.Error("contained branch should be deleted") + } +} + +// Squash-merged work keeps a local SHA that is not upstream, so only the +// patch-equivalence check proves it landed. +func TestPruneCherryCleanBranch(t *testing.T) { + f := newFixture(t) + wt := f.addWorktree(t, "benvin/squashed") + writeCommit(t, wt, "s.txt", "same\n", "add s") + f.landUpstream(t, "s.txt", "same\n", "squashed s") + + srv := fakeGitea(t) + out := run(t, client(srv), true, false) + assertVerdict(t, out, "benvin/squashed", verdictRemoveBranch, "cherry-clean") + + if agent.GitBranchExists(f.srcDir, "benvin/squashed") { + t.Error("cherry-clean branch should be deleted") + } +} + +// Uncommitted work outranks every other signal, including a branch that is +// otherwise fully contained upstream. +func TestPruneNeverTouchesDirtyWorktree(t *testing.T) { + f := newFixture(t) + wt := f.addWorktree(t, "benvin/dirty") + if err := os.WriteFile(filepath.Join(wt, "wip.txt"), []byte("wip\n"), 0o644); err != nil { + t.Fatal(err) + } + + srv := fakeGitea(t, mergedPR(4, "benvin/dirty")) + out := run(t, client(srv), true, false) + assertVerdict(t, out, "benvin/dirty", verdictKeep, "dirty") + + if !exists(wt) { + t.Error("dirty worktree must not be removed") + } + if !agent.GitBranchExists(f.srcDir, "benvin/dirty") { + t.Error("dirty worktree's branch must survive") + } +} + +// An open PR is kept even when its commits are already upstream. +func TestPruneKeepsOpenPR(t *testing.T) { + f := newFixture(t) + wt := f.addWorktree(t, "benvin/open") + + srv := fakeGitea(t, openPR(5, "benvin/open")) + out := run(t, client(srv), true, false) + assertVerdict(t, out, "benvin/open", verdictKeep, "PR open #5") + + if !exists(wt) { + t.Error("worktree with an open PR must not be removed") + } + if !agent.GitBranchExists(f.srcDir, "benvin/open") { + t.Error("branch with an open PR must not be deleted") + } +} + +// Closed-unmerged with the branch still on origin: the work is not lost, so the +// local branch goes too. +func TestPruneClosedPRWithBranchOnOrigin(t *testing.T) { + f := newFixture(t) + wt := f.addWorktree(t, "benvin/closed") + writeCommit(t, wt, "c.txt", "c\n", "work") + git(t, wt, "push", "origin", "benvin/closed") + + 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") + + if exists(wt) { + t.Error("worktree should have been removed") + } + if agent.GitBranchExists(f.srcDir, "benvin/closed") { + t.Error("branch should be deleted while origin still has it") + } +} + +// Closed-unmerged with nothing on origin: the commits exist only here, so the +// branch is kept and only the worktree goes. +func TestPruneClosedPRWithBranchGone(t *testing.T) { + f := newFixture(t) + wt := f.addWorktree(t, "benvin/orphan") + writeCommit(t, wt, "o.txt", "o\n", "work") + + srv := fakeGitea(t, closedPR(7, "benvin/orphan")) + out := run(t, client(srv), true, false) + assertVerdict(t, out, "benvin/orphan", verdictRemove, "PR closed #7, branch gone") + + if exists(wt) { + t.Error("worktree should have been removed") + } + if !agent.GitBranchExists(f.srcDir, "benvin/orphan") { + t.Error("branch must survive when origin does not have the commits") + } +} + +func TestPruneNoPRKeepsBranch(t *testing.T) { + f := newFixture(t) + wt := f.addWorktree(t, "benvin/unpushed") + writeCommit(t, wt, "u.txt", "u\n", "work") + + srv := fakeGitea(t) + out := run(t, client(srv), true, false) + assertVerdict(t, out, "benvin/unpushed", verdictRemove, "no PR") + + if exists(wt) { + t.Error("worktree should have been removed") + } + if !agent.GitBranchExists(f.srcDir, "benvin/unpushed") { + t.Error("branch with unproven work must survive") + } +} + +// The default run reports and changes nothing. +func TestPruneDryRunChangesNothing(t *testing.T) { + f := newFixture(t) + contained := f.addWorktree(t, "benvin/contained") + merged := f.addWorktree(t, "benvin/merged") + writeCommit(t, merged, "m.txt", "m\n", "work") + + srv := fakeGitea(t, mergedPR(8, "benvin/merged")) + out := run(t, client(srv), false, false) + + if !strings.Contains(out, "dry run") { + t.Errorf("dry-run output should say so:\n%s", out) + } + assertVerdict(t, out, "benvin/contained", verdictRemoveBranch, "contained") + assertVerdict(t, out, "benvin/merged", verdictRemoveBranch, "PR merged #8") + if !exists(contained) || !exists(merged) { + t.Error("dry run must not remove worktrees") + } + if !agent.GitBranchExists(f.srcDir, "benvin/contained") || !agent.GitBranchExists(f.srcDir, "benvin/merged") { + t.Error("dry run must not delete branches") + } +} + +// --keep-branches removes worktrees but leaves every branch alone. +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") + + if exists(wt) { + t.Error("worktree should have been removed") + } + if !agent.GitBranchExists(f.srcDir, "benvin/contained") { + t.Error("--keep-branches must not delete the branch") + } +} + +// With Gitea unreachable prune falls back to the git signals: provably-upstream +// work is still cleaned up, and anything unproven keeps its branch. +func TestPruneDegradesWhenGiteaUnreachable(t *testing.T) { + f := newFixture(t) + contained := f.addWorktree(t, "benvin/contained") + unproven := f.addWorktree(t, "benvin/unproven") + writeCommit(t, unproven, "u.txt", "u\n", "work") + + dead := httptest.NewServer(http.NewServeMux()) + c := &agent.GiteaClient{BaseURL: dead.URL, HTTP: dead.Client()} + dead.Close() + + out := run(t, c, true, false) + if !strings.Contains(out, "git signals only") { + t.Errorf("output should note the Gitea failure:\n%s", out) + } + assertVerdict(t, out, "benvin/contained", verdictRemoveBranch, "contained") + assertVerdict(t, out, "benvin/unproven", verdictRemove, "PR state unknown") + + if exists(contained) || exists(unproven) { + t.Error("both worktrees should have been removed") + } + if agent.GitBranchExists(f.srcDir, "benvin/contained") { + t.Error("contained branch is safe to delete without Gitea") + } + if !agent.GitBranchExists(f.srcDir, "benvin/unproven") { + t.Error("unproven branch must survive an unreachable Gitea") + } +} + +func TestPruneNoWorktrees(t *testing.T) { + newFixture(t) + srv := fakeGitea(t) + if out := run(t, client(srv), true, false); !strings.Contains(out, "no managed worktrees") { + t.Errorf("output = %q", out) + } +} + +// 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) { + t.Setenv("AGENTWS_OWNER", "unkin") + dir := t.TempDir() + git(t, dir, "init", "-b", "main", ".") + git(t, dir, "remote", "add", "origin", "https://git.unkin.net/unkinben/dotfiles.git") + if got := repoPath(dir, "dotfiles"); got != "unkinben/dotfiles" { + t.Errorf("repoPath = %q, want unkinben/dotfiles", got) + } + git(t, dir, "remote", "set-url", "origin", filepath.Join(dir, "origin.git")) + if got := repoPath(dir, "dotfiles"); got != "unkin/dotfiles" { + t.Errorf("repoPath for a local remote = %q, want unkin/dotfiles", got) + } +} diff --git a/internal/agent/client_test.go b/internal/agent/client_test.go index a65030c..bbda10d 100644 --- a/internal/agent/client_test.go +++ b/internal/agent/client_test.go @@ -3,6 +3,7 @@ package agent import ( "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -219,6 +220,76 @@ func TestFetchStateFailsOnNon404StatusError(t *testing.T) { } } +// Gitea rewrites head.ref to "refs/pull//head" once the PR's branch is +// deleted, which merging does in these repos. Matching a branch against +// head.ref alone therefore finds nothing for every merged PR; head.label keeps +// the original name. +func TestPRHeadBranch(t *testing.T) { + tests := []struct { + name string + ref, label string + want string + }{ + {"merged, branch deleted", "refs/pull/12/head", "benvin/merged", "benvin/merged"}, + {"open PR", "benvin/open", "benvin/open", "benvin/open"}, + {"fully qualified ref", "refs/heads/benvin/x", "", "benvin/x"}, + {"no label falls back to ref", "benvin/y", "", "benvin/y"}, + {"cross-repo label", "benvin/z", "someone:benvin/z", "benvin/z"}, + {"nothing usable", "refs/pull/12/head", "", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var pr PullRequest + pr.Head.Ref = tt.ref + pr.Head.Label = tt.label + if got := PRHeadBranch(pr); got != tt.want { + t.Errorf("PRHeadBranch(ref=%q,label=%q) = %q, want %q", tt.ref, tt.label, got, tt.want) + } + }) + } +} + +func TestListPRsPaginates(t *testing.T) { + var pages []string + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/pulls", func(w http.ResponseWriter, r *http.Request) { + q := r.URL.Query() + pages = append(pages, q.Get("page")) + if q.Get("state") != "all" { + t.Errorf("state = %q, want all", q.Get("state")) + } + if q.Get("page") == "1" { + full := make([]string, 0, prPageSize) + for i := 0; i < prPageSize; i++ { + full = append(full, fmt.Sprintf(`{"number":%d,"state":"closed","merged":true,"head":{"ref":"refs/pull/%d/head","label":"benvin/b%d"}}`, i+1, i+1, i+1)) + } + _, _ = io.WriteString(w, "["+strings.Join(full, ",")+"]") + return + } + _, _ = io.WriteString(w, `[{"number":99,"state":"open","head":{"ref":"benvin/last","label":"benvin/last"}}]`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + c := &GiteaClient{BaseURL: srv.URL, HTTP: srv.Client()} + prs, err := c.ListPRs("unkin/repo", "all") + if err != nil { + t.Fatalf("ListPRs: %v", err) + } + if len(prs) != prPageSize+1 { + t.Fatalf("got %d PRs, want %d", len(prs), prPageSize+1) + } + if len(pages) != 2 || pages[0] != "1" || pages[1] != "2" { + t.Errorf("pages requested = %v, want [1 2]", pages) + } + if got := PRHeadBranch(prs[0]); got != "benvin/b1" { + t.Errorf("first PR head branch = %q, want benvin/b1", got) + } + if !prs[len(prs)-1].IsOpen() { + t.Error("last PR should be open") + } +} + 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/git.go b/internal/agent/git.go index 44a9184..e500fbb 100644 --- a/internal/agent/git.go +++ b/internal/agent/git.go @@ -2,6 +2,7 @@ package agent import ( "bytes" + "errors" "fmt" "os" "os/exec" @@ -67,6 +68,14 @@ func GitFetch(repoDir, remote string, globalArgs ...string) error { return err } +// GitFetchPrune runs `git fetch --prune ` in repoDir so remote-tracking +// refs for branches deleted on the remote (e.g. after a merge) disappear. +func GitFetchPrune(repoDir, remote string, globalArgs ...string) error { + args := append(append([]string{}, globalArgs...), "fetch", "--prune", remote) + _, err := runGit(repoDir, args...) + return err +} + // GitRemoteDefaultBranch returns the short name of remote's default branch // (e.g. "main") by resolving refs/remotes//HEAD. func GitRemoteDefaultBranch(repoDir, remote string) (string, error) { @@ -83,6 +92,64 @@ func GitBranchExists(repoDir, branch string) bool { return err == nil } +// GitRemoteURL returns the configured URL for a remote. +func GitRemoteURL(repoDir, remote string) (string, error) { + return runGit(repoDir, "remote", "get-url", remote) +} + +// GitRemoteBranchExists reports whether a remote-tracking ref for branch exists +// (accurate only after a pruning fetch). +func GitRemoteBranchExists(repoDir, remote, branch string) bool { + _, err := runGit(repoDir, "show-ref", "--verify", "--quiet", "refs/remotes/"+remote+"/"+branch) + return err == nil +} + +// GitIsDirty reports whether the checkout at dir has uncommitted or untracked +// changes. +func GitIsDirty(dir string) (bool, error) { + out, err := runGit(dir, "status", "--porcelain") + if err != nil { + return false, err + } + return strings.TrimSpace(out) != "", nil +} + +// GitIsAncestor reports whether ancestor is reachable from descendant. +func GitIsAncestor(repoDir, ancestor, descendant string) (bool, error) { + cmd := exec.Command("git", "merge-base", "--is-ancestor", ancestor, descendant) + cmd.Dir = repoDir + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + // Exit 1 is the documented "not an ancestor" answer; anything else is a + // real failure (bad ref, not a repo). + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + return false, nil + } + return false, fmt.Errorf("git merge-base --is-ancestor %s %s: %w: %s", + ancestor, descendant, err, strings.TrimSpace(stderr.String())) + } + return true, nil +} + +// GitUnmergedCommits counts commits on head whose patch has no equivalent on +// upstream, using `git cherry` so squash- and rebase-merged work is recognised +// despite its rewritten SHAs. +func GitUnmergedCommits(repoDir, upstream, head string) (int, error) { + out, err := runGit(repoDir, "cherry", upstream, head) + if err != nil { + return 0, err + } + n := 0 + for _, line := range strings.Split(out, "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "+") { + n++ + } + } + return n, nil +} + // GitWorktreeAdd adds a worktree at path checked out to branch. When the branch // already exists it is reused; otherwise it is created from startPoint. func GitWorktreeAdd(repoDir, path, branch, startPoint string) error { diff --git a/internal/agent/git_test.go b/internal/agent/git_test.go index ff64596..e63802b 100644 --- a/internal/agent/git_test.go +++ b/internal/agent/git_test.go @@ -185,6 +185,134 @@ func TestGitWorktreeLifecycle(t *testing.T) { } } +// commit writes a file and commits it, returning the new HEAD sha. +func commit(t *testing.T, dir, name, content, msg string) string { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + if _, err := runGit(dir, "add", "."); err != nil { + t.Fatalf("add: %v", err) + } + if _, err := runGit(dir, "commit", "-m", msg); err != nil { + t.Fatalf("commit: %v", err) + } + sha, err := runGit(dir, "rev-parse", "HEAD") + if err != nil { + t.Fatalf("rev-parse: %v", err) + } + return sha +} + +func TestGitIsAncestor(t *testing.T) { + srcDir := newTempRepos(t) + base, err := runGit(srcDir, "rev-parse", "HEAD") + if err != nil { + t.Fatal(err) + } + tip := commit(t, srcDir, "a.txt", "a\n", "add a") + + if ok, err := GitIsAncestor(srcDir, base, tip); err != nil || !ok { + t.Errorf("GitIsAncestor(base, tip) = %v, %v; want true", ok, err) + } + if ok, err := GitIsAncestor(srcDir, tip, base); err != nil || ok { + t.Errorf("GitIsAncestor(tip, base) = %v, %v; want false with no error", ok, err) + } + if _, err := GitIsAncestor(srcDir, "no-such-ref", tip); err == nil { + t.Error("GitIsAncestor with a bogus ref should error, not report false") + } +} + +// These repos squash-merge, so merged work keeps its local SHA while the +// upstream commit is a different one carrying the same patch. `git cherry` must +// see that as merged even though the SHAs differ. +func TestGitUnmergedCommitsIgnoresRewrittenSHAs(t *testing.T) { + srcDir := newTempRepos(t) + + if _, err := runGit(srcDir, "checkout", "-b", "feature"); err != nil { + t.Fatalf("checkout: %v", err) + } + commit(t, srcDir, "f.txt", "hello\n", "add f") + + n, err := GitUnmergedCommits(srcDir, "origin/main", "HEAD") + if err != nil { + t.Fatalf("GitUnmergedCommits: %v", err) + } + if n != 1 { + t.Fatalf("unmerged before upstream landing = %d, want 1", n) + } + + // Land the same patch upstream under a different SHA. + if _, err := runGit(srcDir, "checkout", "main"); err != nil { + t.Fatalf("checkout main: %v", err) + } + commit(t, srcDir, "f.txt", "hello\n", "squashed f") + if _, err := runGit(srcDir, "push", "origin", "main"); err != nil { + t.Fatalf("push: %v", err) + } + if err := GitFetchPrune(srcDir, "origin"); err != nil { + t.Fatalf("GitFetchPrune: %v", err) + } + + if ok, err := GitIsAncestor(srcDir, "feature", "origin/main"); err != nil || ok { + t.Fatalf("squash-merged branch must not be an ancestor: %v, %v", ok, err) + } + n, err = GitUnmergedCommits(srcDir, "origin/main", "feature") + if err != nil { + t.Fatalf("GitUnmergedCommits: %v", err) + } + if n != 0 { + t.Errorf("unmerged after upstream landing = %d, want 0", n) + } +} + +func TestGitIsDirty(t *testing.T) { + srcDir := newTempRepos(t) + if dirty, err := GitIsDirty(srcDir); err != nil || dirty { + t.Fatalf("clean checkout reported dirty=%v, err=%v", dirty, err) + } + if err := os.WriteFile(filepath.Join(srcDir, "scratch.txt"), []byte("wip\n"), 0o644); err != nil { + t.Fatal(err) + } + if dirty, err := GitIsDirty(srcDir); err != nil || !dirty { + t.Errorf("untracked file must count as dirty: dirty=%v, err=%v", dirty, err) + } +} + +func TestGitRemoteBranchExists(t *testing.T) { + srcDir := newTempRepos(t) + if !GitRemoteBranchExists(srcDir, "origin", "main") { + t.Error("origin/main should exist") + } + if GitRemoteBranchExists(srcDir, "origin", "benvin/nope") { + t.Error("origin/benvin/nope should not exist") + } + + if _, err := runGit(srcDir, "checkout", "-b", "benvin/pushed"); err != nil { + t.Fatalf("checkout: %v", err) + } + commit(t, srcDir, "p.txt", "p\n", "add p") + if _, err := runGit(srcDir, "push", "origin", "benvin/pushed"); err != nil { + t.Fatalf("push: %v", err) + } + if err := GitFetchPrune(srcDir, "origin"); err != nil { + t.Fatalf("GitFetchPrune: %v", err) + } + if !GitRemoteBranchExists(srcDir, "origin", "benvin/pushed") { + t.Error("pushed branch should have a remote-tracking ref") + } + + if _, err := runGit(srcDir, "push", "origin", "--delete", "benvin/pushed"); err != nil { + t.Fatalf("delete remote branch: %v", err) + } + if err := GitFetchPrune(srcDir, "origin"); err != nil { + t.Fatalf("GitFetchPrune: %v", err) + } + if GitRemoteBranchExists(srcDir, "origin", "benvin/pushed") { + t.Error("a pruning fetch must drop the tracking ref for a deleted remote branch") + } +} + // resolve canonicalizes a path (temp dirs may live behind symlinks like /var). func resolve(t *testing.T, p string) string { t.Helper() diff --git a/internal/agent/gitea.go b/internal/agent/gitea.go index 840417d..82e8288 100644 --- a/internal/agent/gitea.go +++ b/internal/agent/gitea.go @@ -137,10 +137,63 @@ type PullRequest struct { Mergeable bool `json:"mergeable"` HTMLURL string `json:"html_url"` Head struct { - Sha string `json:"sha"` + Sha string `json:"sha"` + Ref string `json:"ref"` + Label string `json:"label"` } `json:"head"` } +// prPageSize is the per-page limit for the pulls listing; maxPRPages caps how +// far back a listing walks. +const ( + prPageSize = 50 + maxPRPages = 20 +) + +// ListPRs lists a repo's pull requests in the given state ("open", "closed" or +// "all"), following pagination. +func (c *GiteaClient) ListPRs(repoPath, state string) ([]PullRequest, error) { + if state == "" { + state = "all" + } + var all []PullRequest + for page := 1; page <= maxPRPages; page++ { + var batch []PullRequest + path := fmt.Sprintf("/api/v1/repos/%s/pulls?state=%s&limit=%d&page=%d", repoPath, state, prPageSize, page) + if err := c.do(http.MethodGet, path, nil, &batch); err != nil { + return nil, err + } + all = append(all, batch...) + if len(batch) < prPageSize { + break + } + } + return all, nil +} + +// PRHeadBranch returns the branch a PR was opened from. Gitea rewrites head.ref +// to "refs/pull//head" once the branch is deleted (which merging does), so +// head.label — which keeps the original name — is authoritative. +func PRHeadBranch(pr PullRequest) string { + if label := pr.Head.Label; label != "" && !strings.HasPrefix(label, "refs/pull/") { + // Cross-repo PRs label as ":". + if _, branch, ok := strings.Cut(label, ":"); ok { + return branch + } + return label + } + ref := pr.Head.Ref + if strings.HasPrefix(ref, "refs/pull/") { + return "" + } + return strings.TrimPrefix(ref, "refs/heads/") +} + +// IsOpen reports whether a PR is still open (not merged, not closed). +func (pr PullRequest) IsOpen() bool { + return pr.State == "open" && !pr.Merged +} + // CreatePROptions are the fields for opening a PR. type CreatePROptions struct { Base string `json:"base"` diff --git a/internal/agent/parse.go b/internal/agent/parse.go index ac5f379..8f2a3f9 100644 --- a/internal/agent/parse.go +++ b/internal/agent/parse.go @@ -68,6 +68,52 @@ func ParseDurationFlag(flag, value string) (time.Duration, error) { return d, nil } +// RemoteHost returns the host a git remote URL points at, or "" for a local +// path remote. +func RemoteHost(remote string) string { + s := strings.TrimSpace(remote) + if _, after, ok := strings.Cut(s, "://"); ok { + host, _, _ := strings.Cut(after, "/") + if _, bare, ok := strings.Cut(host, "@"); ok { + host = bare + } + return host + } + if strings.HasPrefix(s, "/") || strings.HasPrefix(s, ".") { + return "" + } + host, _, ok := strings.Cut(s, ":") + if !ok { + return "" + } + if _, bare, ok := strings.Cut(host, "@"); ok { + host = bare + } + return host +} + +// RepoPathFromRemoteURL extracts the "owner/repo" API path from a git remote +// URL, accepting both https and scp-style ssh forms. +func RepoPathFromRemoteURL(remote string) (string, error) { + s := strings.TrimSuffix(strings.TrimSuffix(strings.TrimSpace(remote), "/"), ".git") + switch { + case strings.Contains(s, "://"): + _, after, _ := strings.Cut(s, "://") + _, path, ok := strings.Cut(after, "/") + if !ok { + return "", fmt.Errorf("remote URL %q has no repo path", remote) + } + s = path + case strings.Contains(s, ":"): + _, s, _ = strings.Cut(s, ":") + } + parts := strings.Split(strings.Trim(s, "/"), "/") + if len(parts) < 2 || parts[len(parts)-2] == "" || parts[len(parts)-1] == "" { + return "", fmt.Errorf("remote URL %q is not owner/repo shaped", remote) + } + return parts[len(parts)-2] + "/" + parts[len(parts)-1], nil +} + // ParseRepo validates and splits an "owner/repo" string. func ParseRepo(s string) (owner, repo string, err error) { s = strings.TrimSpace(s) diff --git a/internal/agent/parse_test.go b/internal/agent/parse_test.go index d3d8703..56b324d 100644 --- a/internal/agent/parse_test.go +++ b/internal/agent/parse_test.go @@ -132,3 +132,52 @@ func TestParseDurationFlagErrorMessage(t *testing.T) { } } } + +// Not every managed repo lives under the default owner, so the API path comes +// from origin's URL rather than the directory name. +func TestRepoPathFromRemoteURL(t *testing.T) { + tests := []struct { + in string + want string + }{ + {"https://git.unkin.net/unkin/agent-tools.git", "unkin/agent-tools"}, + {"https://git.unkin.net/unkinben/dotfiles.git", "unkinben/dotfiles"}, + {"https://git.unkin.net/unkin/agent-tools", "unkin/agent-tools"}, + {"https://user@git.unkin.net/unkin/agent-tools.git", "unkin/agent-tools"}, + {"ssh://git@git.unkin.net:2222/unkin/agent-tools.git", "unkin/agent-tools"}, + {"git@git.unkin.net:unkin/agent-tools.git", "unkin/agent-tools"}, + } + for _, tt := range tests { + got, err := RepoPathFromRemoteURL(tt.in) + if err != nil { + t.Errorf("RepoPathFromRemoteURL(%q): %v", tt.in, err) + continue + } + if got != tt.want { + t.Errorf("RepoPathFromRemoteURL(%q) = %q, want %q", tt.in, got, tt.want) + } + } + for _, bad := range []string{"", "https://git.unkin.net", "agent-tools"} { + if got, err := RepoPathFromRemoteURL(bad); err == nil { + t.Errorf("RepoPathFromRemoteURL(%q) = %q, want error", bad, got) + } + } +} + +func TestRemoteHost(t *testing.T) { + tests := []struct { + in, want string + }{ + {"https://git.unkin.net/unkin/repo.git", "git.unkin.net"}, + {"https://user@git.unkin.net/unkin/repo.git", "git.unkin.net"}, + {"ssh://git@git.unkin.net:2222/unkin/repo.git", "git.unkin.net:2222"}, + {"git@git.unkin.net:unkin/repo.git", "git.unkin.net"}, + {"/tmp/fixture/origin.git", ""}, + {"../other/origin.git", ""}, + } + for _, tt := range tests { + if got := RemoteHost(tt.in); got != tt.want { + t.Errorf("RemoteHost(%q) = %q, want %q", tt.in, got, tt.want) + } + } +} From c1c02c01cfe5b41c857b3b457f92aad1f121a31f Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Thu, 10 Sep 2026 00:00:22 +1000 Subject: [PATCH 2/3] 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 From 387653a3c0ef84d4a590c692f37a7647466289ac Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Thu, 10 Sep 2026 00:18:54 +1000 Subject: [PATCH 3/3] Distrust origin/ when prune's fetch fails 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/ 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. --- AGENTS.md | 4 +- README.md | 11 +++- cmd/agentws/prune.go | 19 ++++-- cmd/agentws/prune_test.go | 122 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 7 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6f489ce..cf90434 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -194,6 +194,8 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`. offline-safe; an unreachable Gitea only means no branch gets deleted without 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. + the worktree goes and the branch stays. `origin/` is only evidence when + this run's pruning fetch succeeded; a failed fetch leaves stale tracking refs, + so those verdicts fall back to keeping the branch. - 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 889f098..2ddeef2 100644 --- a/README.md +++ b/README.md @@ -126,8 +126,8 @@ agentws token | branch has an open PR | keep | | 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 | +| PR merged **and** HEAD contained in the PR's head commit (or in a verified `origin/`) | remove worktree + local branch | +| PR closed **and** HEAD contained in a verified `origin/` | remove worktree + local branch | | anything else | remove worktree, keep the branch | A branch is deleted only where git proves its commits survive elsewhere. PR @@ -142,6 +142,13 @@ 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. +`origin/` counts as evidence only when this run's `git fetch --prune` +succeeded. A tracking ref left over from an earlier fetch may name a branch that +is already gone upstream and is itself due for deletion, so a failed fetch +downgrades those verdicts to `remove` and keeps the branch. Proofs that read +only local objects — containment in `origin/`, patch equivalence, and +containment in a merged PR's head SHA — stand on their own. + 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 diff --git a/cmd/agentws/prune.go b/cmd/agentws/prune.go index a47461c..659182d 100644 --- a/cmd/agentws/prune.go +++ b/cmd/agentws/prune.go @@ -40,6 +40,9 @@ type repoCtx struct { defBranch string prs map[string]agent.PullRequest prsKnown bool + // fetched records that this run's pruning fetch succeeded; without it an + // origin/ ref may be stale and due for deletion, so it proves nothing. + fetched bool } func newPruneCmd() *cobra.Command { @@ -139,12 +142,15 @@ func plannedVerdict(r pruneResult, keepBranches bool) string { // 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. +// the signals that hold offline still work, and the rest are recorded as +// unverified. func newRepoCtx(out io.Writer, prs prLister, srcDir string) (repoCtx, error) { ctx := repoCtx{srcDir: srcDir, prs: map[string]agent.PullRequest{}} repo := filepath.Base(srcDir) if err := agent.GitFetchPrune(srcDir, "origin", credentialHelperArgs()...); err != nil { - _, _ = fmt.Fprintf(out, "warn: fetch %s: %v (using local refs)\n", repo, err) + _, _ = fmt.Fprintf(out, "warn: fetch %s: %v (remote state unverified)\n", repo, err) + } else { + ctx.fetched = true } def, err := agent.GitRemoteDefaultBranch(srcDir, "origin") if err != nil { @@ -223,7 +229,8 @@ func headContainedIn(dir, ref string) bool { // provably-upstream work loses its branch too, and anything unproven keeps its // 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. +// what origin still holds, and origin's refs only count when this run's pruning +// fetch refreshed them. func classify(wt managedWt, ctx repoCtx) (pruneResult, error) { res := pruneResult{wt: wt} dirty, err := agent.GitIsDirty(wt.path) @@ -261,18 +268,22 @@ func classify(wt managedWt, ctx repoCtx) (pruneResult, error) { } remote := "origin/" + wt.branch - onOrigin := hasPR && agent.GitRemoteBranchExists(ctx.srcDir, "origin", wt.branch) + onOrigin := hasPR && ctx.fetched && 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 && !ctx.fetched: + res.verdict, res.reason = verdictRemove, fmt.Sprintf("PR merged #%d, fetch failed so %s is unverified", pr.Number, remote) 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 && !ctx.fetched: + res.verdict, res.reason = verdictRemove, fmt.Sprintf("PR closed #%d, fetch failed so %s is unverified", pr.Number, remote) case hasPR: res.verdict, res.reason = verdictRemove, fmt.Sprintf("PR closed #%d, branch gone", pr.Number) case ctx.prsKnown: diff --git a/cmd/agentws/prune_test.go b/cmd/agentws/prune_test.go index 9c99709..a8f7254 100644 --- a/cmd/agentws/prune_test.go +++ b/cmd/agentws/prune_test.go @@ -548,6 +548,128 @@ func TestPruneWarnsOnTruncatedPRListing(t *testing.T) { } } +// breakRemote points origin at a path that does not exist, so every fetch fails. +func (f *fixture) breakRemote(t *testing.T) { + t.Helper() + git(t, f.srcDir, "remote", "set-url", "origin", filepath.Join(f.root, "missing.git")) +} + +// applyUnreachable applies the plan against a repo whose remote is unreachable. +// The post-removal refresh fetch fails, so runPrune must report an error; the +// classification and the removals it authorised happen regardless. +func applyUnreachable(t *testing.T, prs prLister) string { + t.Helper() + var out bytes.Buffer + err := runPrune(&out, prs, true, false) + if err == nil { + t.Fatalf("expected the refresh fetch to fail against a missing remote:\n%s", out.String()) + } + return out.String() +} + +// staleFixture builds a repo where origin/ covers HEAD for a closed and +// a merged PR. Deleting the branches on origin makes those tracking refs stale: +// a pruning fetch would drop them, so they only prove anything while a fetch +// this run confirms they are still there. +func staleFixture(t *testing.T, deleteUpstream bool) (*fixture, *httptest.Server, string, string) { + t.Helper() + f := newFixture(t) + closed := f.addWorktree(t, "benvin/stale-closed") + writeCommit(t, closed, "c.txt", "c\n", "work") + git(t, closed, "push", "origin", "benvin/stale-closed") + + merged := f.addWorktree(t, "benvin/stale-merged") + writeCommit(t, merged, "m.txt", "m\n", "work") + git(t, merged, "push", "origin", "benvin/stale-merged") + + if deleteUpstream { + git(t, f.bare, "update-ref", "-d", "refs/heads/benvin/stale-closed") + git(t, f.bare, "update-ref", "-d", "refs/heads/benvin/stale-merged") + } + + // The merged head is an older commit, so only origin/ covers HEAD. + base := git(t, f.srcDir, "rev-parse", "origin/main") + srv := fakeGitea(t, + closedPR(20, "benvin/stale-closed"), + withHeadSha(mergedPR(21, "benvin/stale-merged"), base), + ) + return f, srv, closed, merged +} + +// A tracking ref this run's fetch could not confirm is not evidence: the branch +// may already be gone upstream, and the next successful --prune deletes the ref. +// Both worktrees go, both branches stay. +func TestPruneFailedFetchDistrustsStaleRemoteBranch(t *testing.T) { + f, srv, closed, merged := staleFixture(t, true) + f.breakRemote(t) + + out := run(t, client(srv), false, false) + if !strings.Contains(out, "remote state unverified") { + t.Errorf("output should report the failed fetch:\n%s", out) + } + assertVerdict(t, out, "benvin/stale-closed", verdictRemove, "PR closed #20, fetch failed so origin/benvin/stale-closed is unverified") + assertVerdict(t, out, "benvin/stale-merged", verdictRemove, "PR merged #21, fetch failed so origin/benvin/stale-merged is unverified") + for _, b := range []string{"benvin/stale-closed", "benvin/stale-merged"} { + if !agent.GitRemoteBranchExists(f.srcDir, "origin", b) { + t.Fatalf("fixture: origin/%s should still be present as a stale ref", b) + } + } + + applyUnreachable(t, client(srv)) + if exists(closed) || exists(merged) { + t.Error("both worktrees should have been removed") + } + for _, b := range []string{"benvin/stale-closed", "benvin/stale-merged"} { + if !agent.GitBranchExists(f.srcDir, b) { + t.Errorf("branch %s must survive an unverified origin", b) + } + } +} + +// The control for the case above: with the fetch working and the branches still +// on origin, the same shape still loses both branches. +func TestPruneSuccessfulFetchTrustsRemoteBranch(t *testing.T) { + f, srv, closed, merged := staleFixture(t, false) + + out := run(t, client(srv), true, false) + assertVerdict(t, out, "benvin/stale-closed", verdictRemoveBranch, "PR closed #20, HEAD contained in origin/benvin/stale-closed") + assertVerdict(t, out, "benvin/stale-merged", verdictRemoveBranch, "PR merged #21, HEAD contained in origin/benvin/stale-merged") + + if exists(closed) || exists(merged) { + t.Error("both worktrees should have been removed") + } + for _, b := range []string{"benvin/stale-closed", "benvin/stale-merged"} { + if agent.GitBranchExists(f.srcDir, b) { + t.Errorf("branch %s should be deleted while origin still has it", b) + } + } +} + +// Proofs that read only local objects and the merged head SHA from the API do +// not depend on the fetch, so a failed fetch must not suppress them. +func TestPruneFailedFetchKeepsFetchIndependentProofs(t *testing.T) { + f := newFixture(t) + contained := f.addWorktree(t, "benvin/contained") + landed := f.addWorktree(t, "benvin/landed") + writeCommit(t, landed, "a.txt", "a\n", "work") + head := git(t, landed, "rev-parse", "HEAD") + f.breakRemote(t) + + srv := fakeGitea(t, withHeadSha(mergedPR(22, "benvin/landed"), head)) + out := applyUnreachable(t, client(srv)) + assertVerdict(t, out, "benvin/contained", verdictRemoveBranch, "contained in origin/main") + assertVerdict(t, out, "benvin/landed", verdictRemoveBranch, "PR merged #22, HEAD contained in the merged head") + + if exists(contained) || exists(landed) { + t.Error("both worktrees should have been removed") + } + for _, b := range []string{"benvin/contained", "benvin/landed"} { + if agent.GitBranchExists(f.srcDir, b) { + t.Errorf("branch %s is proven without the fetch and should be deleted", b) + } + } +} + // 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) {