From 387653a3c0ef84d4a590c692f37a7647466289ac Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Thu, 10 Sep 2026 00:18:54 +1000 Subject: [PATCH] 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) {