Distrust origin/<branch> 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/<branch> existence and containment proofs on it; a failed fetch removes the worktree and keeps the branch. Local-object proofs and the merged head SHA are unaffected.
This commit is contained in:
@@ -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/<branch>`, otherwise
|
||||
the worktree goes and the branch stays.
|
||||
the worktree goes and the branch stays. `origin/<branch>` 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.
|
||||
|
||||
@@ -126,8 +126,8 @@ agentws token
|
||||
| branch has an open PR | keep |
|
||||
| tip contained in `origin/<default>` | remove worktree + local branch |
|
||||
| every commit patch-equivalent to one in `origin/<default>`'s history | remove worktree + local branch |
|
||||
| PR merged **and** HEAD contained in the PR's head commit (or in `origin/<branch>`) | remove worktree + local branch |
|
||||
| PR closed **and** HEAD contained in `origin/<branch>` | remove worktree + local branch |
|
||||
| PR merged **and** HEAD contained in the PR's head commit (or in a verified `origin/<branch>`) | remove worktree + local branch |
|
||||
| PR closed **and** HEAD contained in a verified `origin/<branch>` | 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/<branch>` 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/<default>`, 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
|
||||
|
||||
+15
-4
@@ -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/<branch> 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:
|
||||
|
||||
@@ -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/<branch> 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/<branch> 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) {
|
||||
|
||||
Reference in New Issue
Block a user