diff --git a/changelog.d/fixed-hold-guard-branch-update-retries.md b/changelog.d/fixed-hold-guard-branch-update-retries.md new file mode 100644 index 0000000000..da21180f2b --- /dev/null +++ b/changelog.d/fixed-hold-guard-branch-update-retries.md @@ -0,0 +1 @@ +- Fixed the hold guard retrying a held PR's branch update every pass without ever reaching GitHub after its first update. Each update is now one attempt per head and base. An accepted update is waited on, and a refusal or an update that never lands stops until the head or base moves, with a note telling a person to update the branch. PRs returned to their contributor (072) are no longer updated. diff --git a/src/cmd/hive/hold_guard_mergeability_test.go b/src/cmd/hive/hold_guard_mergeability_test.go index e5ee82477c..a0948d6ebc 100644 --- a/src/cmd/hive/hold_guard_mergeability_test.go +++ b/src/cmd/hive/hold_guard_mergeability_test.go @@ -81,13 +81,21 @@ func TestMaintainHeldPRMergeabilityNotesUnresolvableConflictOnce(t *testing.T) { t.Fatalf("conflict notice must be deduplicated, saw %v", fake.comments) } - // The conflict resolves (a human merges by hand); the next behind push + // The same conflict on the same head and base is not asked again. + fake.mu.Lock() + calls := fake.updateBranchCalls[7] + fake.mu.Unlock() + if calls != 1 { + t.Fatalf("update-branch calls for an unchanged conflicting head and base = %d, want 1", calls) + } + + // The base moves and the conflict goes away; the next behind push // succeeds, and the stale conflict notice must clear. fake.mu.Lock() fake.failUpdateBranch = false fake.commits[7] = append(fake.commits[7], ghCommit("c2", "hive-bot", "Merge branch 'main' into feature")) fake.mu.Unlock() - heldResolved := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", MergeableState: "behind"}) + heldResolved := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b2", MergeableState: "behind"}) _ = enforceHoldGuard(context.Background(), cfg, client, client, heldResolved, discardLogger()) rec, ok = store.Recorded("acme/widgets", 7) if !ok || rec.ConflictNoted { @@ -137,8 +145,9 @@ func TestMaintainHeldPRMergeabilityIgnoresCleanPR(t *testing.T) { // A held PR GitHub refuses to update because the base changed workflow files // and the App lacks the workflows permission (projectbluefin/testsuite#991, -// retried every pass for six hours) gets one attempt per head: the refusal is -// recorded, no conflict notice is posted, and only a new head retries. +// retried every pass for six hours) gets one attempt per head and base: the +// refusal is recorded with its reason, one note tells a person to update the +// branch (not the conflict notice), and only a new head or base retries. func TestMaintainHeldPRMergeabilityStopsOnWorkflowPermissionRefusal(t *testing.T) { for _, state := range []string{"behind", "dirty"} { t.Run(state, func(t *testing.T) { @@ -163,22 +172,41 @@ func TestMaintainHeldPRMergeabilityStopsOnWorkflowPermissionRefusal(t *testing.T if calls != 1 { t.Fatalf("update-branch calls for a refused head = %d, want 1", calls) } - if comments != 0 { - t.Fatalf("a workflows-permission refusal is not a conflict; saw %d comments", comments) + if comments != 1 { + t.Fatalf("want one update notice for a workflows-permission refusal; saw %d comments", comments) + } + fake.mu.Lock() + note := fake.comments[0] + fake.mu.Unlock() + if !strings.Contains(note, "workflows permission") || !strings.Contains(note, "Update branch") || strings.Contains(note, "real conflict") { + t.Fatalf("notice = %q, want the workflows-permission reason and the Update branch step, not the conflict notice", note) } rec, ok := store.Recorded("acme/widgets", 7) - if !ok || rec.WorkflowRefusedHeadSHA != "c1" || rec.ConflictNoted { - t.Fatalf("snapshot = %+v ok=%v, want WorkflowRefusedHeadSHA=c1 and no conflict noted", rec, ok) + if !ok || rec.UpdateStoppedHeadSHA != "c1" || rec.UpdateStoppedReason != holdguard.UpdateStopWorkflowPermission || rec.ConflictNoted { + t.Fatalf("snapshot = %+v ok=%v, want an update stop at c1 for the workflows permission and no conflict noted", rec, ok) + } + + // The base moves; the guard asks again, and the same refusal + // does not post a second note for the same head. + movedBase := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b2", MergeableState: state}) + _ = enforceHoldGuard(context.Background(), cfg, client, client, movedBase, discardLogger()) + _ = enforceHoldGuard(context.Background(), cfg, client, client, movedBase, discardLogger()) + fake.mu.Lock() + calls = fake.updateBranchCalls[7] + comments = len(fake.comments) + fake.mu.Unlock() + if calls != 2 || comments != 1 { + t.Fatalf("after the base moved: update-branch calls = %d, comments = %d; want 2 and 1", calls, comments) } // The head moves (a person pushed); the guard tries again. - movedHead := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c2", MergeableState: state}) + movedHead := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c2", BaseSHA: "b2", MergeableState: state}) _ = enforceHoldGuard(context.Background(), cfg, client, client, movedHead, discardLogger()) fake.mu.Lock() calls = fake.updateBranchCalls[7] fake.mu.Unlock() - if calls != 2 { - t.Fatalf("update-branch calls after the head moved = %d, want 2", calls) + if calls != 3 { + t.Fatalf("update-branch calls after the head moved = %d, want 3", calls) } }) } diff --git a/src/cmd/hive/hold_guard_test.go b/src/cmd/hive/hold_guard_test.go index 2108517276..828d0bb8d0 100644 --- a/src/cmd/hive/hold_guard_test.go +++ b/src/cmd/hive/hold_guard_test.go @@ -90,6 +90,10 @@ type holdGuardServer struct { // workflowRefused answers update-branch with GitHub's 403 for an App // without the workflows permission. workflowRefused bool + // refuseStatus, when set, answers update-branch with that status and + // refuseMessage. + refuseStatus int + refuseMessage string } func (s *holdGuardServer) handler(t *testing.T) http.Handler { @@ -153,6 +157,11 @@ func (s *holdGuardServer) handler(t *testing.T) http.Handler { http.Error(w, `{"message":"refusing to allow a GitHub App to create or update workflow `+"`.github/workflows/docs-validate.yml`"+` without `+"`workflows`"+` permission"}`, http.StatusForbidden) return } + if s.refuseStatus != 0 { + body, _ := json.Marshal(map[string]string{"message": s.refuseMessage}) + http.Error(w, string(body), s.refuseStatus) + return + } if s.failUpdateBranch { http.Error(w, `{"message":"Merge conflict"}`, http.StatusUnprocessableEntity) return diff --git a/src/cmd/hive/hold_guard_update_retry_test.go b/src/cmd/hive/hold_guard_update_retry_test.go new file mode 100644 index 0000000000..e4f582b006 --- /dev/null +++ b/src/cmd/hive/hold_guard_update_retry_test.go @@ -0,0 +1,350 @@ +package main + +// Tests for the hold guard's keep-mergeable update when GitHub does not +// complete it: one attempt per head and base, a wait on an accepted (202) +// update, and a stop with a person-facing note on a refusal the same token +// would get again (projectbluefin/fsdk-containers#474, +// projectbluefin/ghostscript-printer-app#152, projectbluefin/ps-printer-app#136). + +import ( + "context" + "net/http" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/hivecommons/hive/pkg/convergence/mutation" + "github.com/hivecommons/hive/pkg/convergence/proof" + "github.com/hivecommons/hive/pkg/github" + "github.com/hivecommons/hive/pkg/holdguard" +) + +func (s *holdGuardServer) updateCalls(number int) int { + s.mu.Lock() + defer s.mu.Unlock() + return s.updateBranchCalls[number] +} + +func (s *holdGuardServer) commentCount() int { + s.mu.Lock() + defer s.mu.Unlock() + return len(s.comments) +} + +func (s *holdGuardServer) setCommits(number int, commits ...map[string]any) { + s.mu.Lock() + defer s.mu.Unlock() + s.commits[number] = commits +} + +// withJournal installs the mutation journal a hive runs with by default +// (convergence mode shadow), which records every external effect and replays +// an identical one as "already applied". +func withJournal(t *testing.T, c *github.Client) { + t.Helper() + dir := t.TempDir() + ledger, err := mutation.OpenLedger(filepath.Join(dir, "mutations.json"), 1) + if err != nil { + t.Fatal(err) + } + journal, err := mutation.OpenJournal(filepath.Join(dir, "journal.json")) + if err != nil { + t.Fatal(err) + } + c.SetMutationBoundary(&mutation.Boundary{Executor: mutation.Executor{Ledger: ledger, Journal: journal, Mode: proof.ModeShadow}, Holder: "test"}) +} + +// fsdk-containers#474: the first update landed, and every later one for the +// same PR replayed from the journal as "already applied" without reaching +// GitHub, so the branch stayed behind for hours. A new head or base is a new +// attempt and must reach GitHub. +func TestMaintainHeldPRMergeabilityAsksGitHubAgainAfterAJournaledUpdate(t *testing.T) { + store := newTestHoldGuardStore(t) + fake := &holdGuardServer{commits: map[int][]map[string]any{ + 7: {ghCommit("c1", "contributor", "fix: scan"), ghCommit("m1", "hive-bot", "Merge branch 'main' into fix")}, + }} + client := newHoldGuardClient(t, fake) + withJournal(t, client) + cfg := escalationTestConfig() + store.Snapshot("acme/widgets", 7, "c1", []holdguard.Commit{{SHA: "c1", Author: "contributor"}}) + + _ = enforceHoldGuard(context.Background(), cfg, client, client, + heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b1", MergeableState: "behind"}), discardLogger()) + if rec, _ := store.Recorded("acme/widgets", 7); fake.updateCalls(7) != 1 || rec.HeadSHA != "m1" { + t.Fatalf("first update: calls = %d, baseline = %q; want 1 and m1", fake.updateCalls(7), rec.HeadSHA) + } + + // Main moves again; the PR is behind at the hive's own merge commit. + fake.setCommits(7, ghCommit("c1", "contributor", "fix: scan"), ghCommit("m1", "hive-bot", "Merge branch 'main' into fix"), + ghCommit("m2", "hive-bot", "Merge branch 'main' into fix")) + _ = enforceHoldGuard(context.Background(), cfg, client, client, + heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "m1", BaseSHA: "b2", MergeableState: "behind"}), discardLogger()) + if rec, _ := store.Recorded("acme/widgets", 7); fake.updateCalls(7) != 2 || rec.HeadSHA != "m2" { + t.Fatalf("second update: calls = %d, baseline = %q; want GitHub asked again and the baseline at m2", fake.updateCalls(7), rec.HeadSHA) + } + if fake.commentCount() != 0 { + t.Fatalf("successful updates must not comment, saw %v", fake.comments) + } +} + +// ghostscript-printer-app#152 / ps-printer-app#136: GitHub answered 202 and +// pushed the merge about two seconds later, after the guard had already read +// the commit list. An accepted update whose head is not visible yet is waited +// on, not re-sent; one that never lands stops with a note. +func TestMaintainHeldPRMergeabilityWaitsOnAnAcceptedUpdate(t *testing.T) { + store := newTestHoldGuardStore(t) + now := time.Date(2026, 10, 26, 18, 34, 0, 0, time.UTC) + store.SetClock(func() time.Time { return now }) + fake := &holdGuardServer{commits: map[int][]map[string]any{7: {ghCommit("c1", "renovate", "chore(deps): bump")}}} + client := newHoldGuardClient(t, fake) + cfg := escalationTestConfig() + store.Snapshot("acme/widgets", 7, "c1", nil) + behind := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b1", MergeableState: "behind"}) + + _ = enforceHoldGuard(context.Background(), cfg, client, client, behind, discardLogger()) + rec, _ := store.Recorded("acme/widgets", 7) + if fake.updateCalls(7) != 1 || rec.UpdateRequestedHeadSHA != "c1" || rec.HeadSHA != "c1" { + t.Fatalf("accepted update: calls = %d, record = %+v; want 1 call, a request at c1, the baseline unchanged", fake.updateCalls(7), rec) + } + + // Still within the confirmation window: no new request, no note. + now = now.Add(holdguard.UpdateConfirmWindow - time.Minute) + _ = enforceHoldGuard(context.Background(), cfg, client, client, behind, discardLogger()) + if fake.updateCalls(7) != 1 || fake.commentCount() != 0 { + t.Fatalf("while waiting: calls = %d, comments = %d; want 1 and 0", fake.updateCalls(7), fake.commentCount()) + } + + // The window passes with the head unmoved: stop and tell a person once. + now = now.Add(2 * time.Minute) + for i := 0; i < 3; i++ { + _ = enforceHoldGuard(context.Background(), cfg, client, client, behind, discardLogger()) + } + rec, _ = store.Recorded("acme/widgets", 7) + if fake.updateCalls(7) != 1 || rec.UpdateStoppedReason != holdguard.UpdateStopNotLanded || rec.UpdateRequestedHeadSHA != "" { + t.Fatalf("after the window: calls = %d, record = %+v; want no new call and a not-landed stop", fake.updateCalls(7), rec) + } + if fake.commentCount() != 1 || !strings.Contains(fake.comments[0], "had not moved") || !strings.Contains(fake.comments[0], "Update branch") { + t.Fatalf("want one not-landed note naming the Update branch step, saw %v", fake.comments) + } + + // Main moves: a new attempt. + _ = enforceHoldGuard(context.Background(), cfg, client, client, + heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b2", MergeableState: "behind"}), discardLogger()) + if fake.updateCalls(7) != 2 { + t.Fatalf("after the base moved: calls = %d, want 2", fake.updateCalls(7)) + } +} + +// The usual slow case: the merge lands between ticks. The request settles +// once the head moves, and the baseline is not advanced to a head the guard +// cannot prove is its own merge. +func TestMaintainHeldPRMergeabilitySettlesAnUpdateThatLandsLater(t *testing.T) { + store := newTestHoldGuardStore(t) + fake := &holdGuardServer{commits: map[int][]map[string]any{7: {ghCommit("c1", "renovate", "chore(deps): bump")}}} + client := newHoldGuardClient(t, fake) + cfg := escalationTestConfig() + store.Snapshot("acme/widgets", 7, "c1", nil) + + _ = enforceHoldGuard(context.Background(), cfg, client, client, + heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b1", MergeableState: "behind"}), discardLogger()) + if rec, _ := store.Recorded("acme/widgets", 7); rec.UpdateRequestedHeadSHA != "c1" { + t.Fatalf("record = %+v, want a request at c1", rec) + } + + _ = enforceHoldGuard(context.Background(), cfg, client, client, + heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "m1", BaseSHA: "b1", MergeableState: "clean"}), discardLogger()) + rec, _ := store.Recorded("acme/widgets", 7) + if rec.UpdateRequestedHeadSHA != "" || rec.HeadSHA != "c1" { + t.Fatalf("record = %+v, want the request settled and the baseline left at c1", rec) + } + if fake.updateCalls(7) != 1 || fake.commentCount() != 0 { + t.Fatalf("calls = %d, comments = %d; want 1 and 0", fake.updateCalls(7), fake.commentCount()) + } +} + +// A fork PR the App cannot push to: known up front when "allow edits by +// maintainers" is off (no call at all), or from GitHub's refusal when it is +// on but the push is still refused. Either way the guard stops for that head +// and base and tells a person once. +func TestMaintainHeldPRMergeabilityStopsOnAForkItCannotPushTo(t *testing.T) { + cases := []struct { + name string + canModify bool + status int + message string + wantCalls int + }{ + {name: "maintainer edits off", canModify: false, wantCalls: 0}, + {name: "push refused (422)", canModify: true, status: http.StatusUnprocessableEntity, message: "You do not have permission to push to the head repository; maintainer edits may be disabled", wantCalls: 1}, + {name: "push refused (403)", canModify: true, status: http.StatusForbidden, message: "Resource not accessible by integration", wantCalls: 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := newTestHoldGuardStore(t) + fake := &holdGuardServer{ + refuseStatus: tc.status, + refuseMessage: tc.message, + commits: map[int][]map[string]any{7: {ghCommit("c1", "contributor", "fix")}}, + } + client := newHoldGuardClient(t, fake) + cfg := escalationTestConfig() + store.Snapshot("acme/widgets", 7, "c1", nil) + held := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b1", MergeableState: "behind", + FromFork: true, MaintainerCanModify: tc.canModify}) + + for i := 0; i < 3; i++ { + _ = enforceHoldGuard(context.Background(), cfg, client, client, held, discardLogger()) + } + rec, _ := store.Recorded("acme/widgets", 7) + if fake.updateCalls(7) != tc.wantCalls || rec.UpdateStoppedReason != holdguard.UpdateStopForkNotEditable { + t.Fatalf("calls = %d, record = %+v; want %d calls and a fork-not-editable stop", fake.updateCalls(7), rec, tc.wantCalls) + } + if fake.commentCount() != 1 || !strings.Contains(fake.comments[0], "fork") { + t.Fatalf("want one note naming the fork, saw %v", fake.comments) + } + }) + } +} + +// A refused head is not asked again until the head or main moves, and a +// transient failure is retried on the next pass. +func TestMaintainHeldPRMergeabilityRetriesRefusedHeadOnlyAfterHeadOrBaseMoves(t *testing.T) { + store := newTestHoldGuardStore(t) + fake := &holdGuardServer{workflowRefused: true, commits: map[int][]map[string]any{7: {ghCommit("c1", "contributor", "fix")}}} + client := newHoldGuardClient(t, fake) + cfg := escalationTestConfig() + store.Snapshot("acme/widgets", 7, "c1", nil) + pass := func(head, base string) { + _ = enforceHoldGuard(context.Background(), cfg, client, client, + heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: head, BaseSHA: base, MergeableState: "behind"}), discardLogger()) + } + + for i := 0; i < 5; i++ { + pass("c1", "b1") + } + if fake.updateCalls(7) != 1 { + t.Fatalf("calls on an unchanged refused head and base = %d, want 1", fake.updateCalls(7)) + } + pass("c1", "b2") + pass("c1", "b2") + if fake.updateCalls(7) != 2 { + t.Fatalf("calls after main moved = %d, want 2", fake.updateCalls(7)) + } + pass("c2", "b2") + pass("c2", "b2") + if fake.updateCalls(7) != 3 { + t.Fatalf("calls after the head moved = %d, want 3", fake.updateCalls(7)) + } + + // A transient failure is not a stop. + fake.mu.Lock() + fake.workflowRefused = false + fake.refuseStatus, fake.refuseMessage = http.StatusBadGateway, "Server Error" + fake.mu.Unlock() + pass("c3", "b2") + pass("c3", "b2") + if rec, _ := store.Recorded("acme/widgets", 7); fake.updateCalls(7) != 5 || rec.UpdateStoppedHeadSHA == "c3" { + t.Fatalf("transient failure: calls = %d, record = %+v; want a retry each pass and no stop", fake.updateCalls(7), rec) + } +} + +// A record written by a build that only knew the workflows refusal keeps +// stopping on its head. +func TestMaintainHeldPRMergeabilityHonoursLegacyWorkflowRefusal(t *testing.T) { + rec := holdguard.Entry{WorkflowRefusedHeadSHA: "c1"} + if reason, ok := rec.UpdateStoppedFor("c1", "b9"); !ok || reason != holdguard.UpdateStopWorkflowPermission { + t.Fatalf("UpdateStoppedFor(c1) = %q, %v; want the legacy workflows refusal", reason, ok) + } + if _, ok := rec.UpdateStoppedFor("c2", "b9"); ok { + t.Fatal("a moved head must not be stopped by the legacy record") + } +} + +// A PR returned to its contributor (072) is theirs to push to; the guard +// does not merge the base in underneath them. +func TestMaintainHeldPRMergeabilityLeavesAReturnedPRAlone(t *testing.T) { + store := newTestHoldGuardStore(t) + fake := &holdGuardServer{commits: map[int][]map[string]any{7: {ghCommit("c1", "contributor", "fix")}}} + client := newHoldGuardClient(t, fake) + cfg := escalationTestConfig() + store.Snapshot("acme/widgets", 7, "c1", nil) + + _ = enforceHoldGuard(context.Background(), cfg, client, client, + heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b1", MergeableState: "behind", + FromFork: true, MaintainerCanModify: true, Labels: []string{"sentinel-alert", "stage/072-returned-to-contributor"}}), discardLogger()) + if fake.updateCalls(7) != 0 || fake.commentCount() != 0 { + t.Fatalf("calls = %d, comments = %d; want the returned PR left alone", fake.updateCalls(7), fake.commentCount()) + } +} + +// An accepted update that lands between ticks is settled without moving the +// baseline, so the hive's merge commit is not in the snapshot's commit set. +// It still never reads as drift at lift time when the hive saw it while the +// PR was held: the snapshot step records that head before the update +// settles, and a human lift on an observed head is accepted (#13603). Only +// a lift that lands before the hive sees the new head re-holds, which fails +// toward a fresh review. +func TestEnforceHoldGuardSettledUpdateIsNotDriftAtLift(t *testing.T) { + lifted := []map[string]any{{ + "event": "labeled", + "label": map[string]string{"name": "hold"}, + "actor": map[string]string{"login": "kubestellar-hive[bot]", "type": "Bot"}, + "created_at": "2026-10-22T05:00:00Z", + }, { + "event": "committed", + "sha": "m1", + "committer": map[string]string{"name": "GitHub", "date": "2026-10-22T10:00:00Z"}, + }, { + "event": "unlabeled", + "label": map[string]string{"name": "hold"}, + "actor": map[string]string{"login": "clubanderson", "type": "User"}, + "created_at": "2026-10-22T18:19:00Z", + }} + held := func(head, state string) *github.ActionableResult { + a := heldActionable(github.HoldItem{Repo: "widgets", Number: 7, Type: "pr", HeadSHA: head}) + a.PRs.Held = []github.PullRequest{{Repo: "widgets", Number: 7, HeadSHA: head, BaseSHA: "b1", MergeableState: state}} + return a + } + for _, tc := range []struct { + name string + seenWhenHeld bool + }{ + {"merge seen while held", true}, + {"lift before the merge was seen", false}, + } { + t.Run(tc.name, func(t *testing.T) { + store := newTestHoldGuardStore(t) + fake := &holdGuardServer{commits: map[int][]map[string]any{7: {ghCommit("c1", "renovate", "chore(deps): bump")}}} + fake.timeline = map[int][]map[string]any{7: lifted} + client := newHoldGuardClient(t, fake) + client.SetAppBotLogin("kubestellar-hive[bot]") + cfg := escalationTestConfig() + + _ = enforceHoldGuard(context.Background(), cfg, client, client, held("c1", "behind"), discardLogger()) + if rec, _ := store.Recorded("acme/widgets", 7); rec.UpdateRequestedHeadSHA != "c1" { + t.Fatalf("record = %+v, want an accepted update awaited at c1", rec) + } + fake.setCommits(7, ghCommit("c1", "renovate", "chore(deps): bump"), ghCommit("m1", "kubestellar-hive[bot]", "Merge branch 'main' into deps")) + if tc.seenWhenHeld { + _ = enforceHoldGuard(context.Background(), cfg, client, client, held("m1", "clean"), discardLogger()) + if rec, _ := store.Recorded("acme/widgets", 7); rec.UpdateRequestedHeadSHA != "" || rec.HeadSHA != "c1" || rec.ObservedHeldHeadSHA != "m1" { + t.Fatalf("record = %+v, want the update settled, baseline c1, observed head m1", rec) + } + } + + got := enforceHoldGuard(context.Background(), cfg, client, client, + actionableWith(github.PullRequest{Repo: "widgets", Number: 7, Author: "renovate", HeadSHA: "m1"}), discardLogger()) + _, tracked := store.Recorded("acme/widgets", 7) + if tc.seenWhenHeld { + if len(got) != 0 || fake.commentCount() != 0 || len(fake.labels) != 0 || tracked { + t.Fatalf("excluded=%v comments=%v labels=%v tracked=%v; a lift on the merge the hive saw while held must reopen the lanes quietly", got, fake.comments, fake.labels, tracked) + } + return + } + if len(got) == 0 || fake.commentCount() != 1 { + t.Fatalf("excluded=%v comments=%v; a head the hive never saw while held must re-hold for review", got, fake.comments) + } + }) + } +} diff --git a/src/cmd/hive/main_helpers.go b/src/cmd/hive/main_helpers.go index af6629900d..f605d612dd 100644 --- a/src/cmd/hive/main_helpers.go +++ b/src/cmd/hive/main_helpers.go @@ -15,7 +15,7 @@ import ( "github.com/hivecommons/hive/pkg/github/automerge" "github.com/hivecommons/hive/pkg/holdguard" "github.com/hivecommons/hive/pkg/intent" - "github.com/hivecommons/hive/pkg/stageengine/prstage" + "github.com/hivecommons/hive/pkg/stagelabels" ) var ( @@ -248,17 +248,35 @@ func enforceHoldGuard( // conflicts) and sit that way for the entire review wait. // // For each PR the guard is already tracking, a "behind" or "dirty" state gets -// one merge-from-base attempt via UpdateBranch, using the hive's own -// identity. On success the result is hygiene, not reviewable content, so the -// guard's baseline is advanced to the new head (holdguard.AdvanceHygiene) — -// this is what keeps the next lift check from reading the hive's own upkeep -// push as drift. On failure (a real conflict the hive cannot resolve), a -// one-time comment tells the human before they open the review, deduplicated -// through the ledger's ConflictNoted bit exactly like the drift comment -// dedupes through Commented. GitHub's 403 refusing the update because the base -// changed .github/workflows/ and the App lacks the workflows permission is -// neither: it is recorded against the head (WorkflowRefusedHeadSHA) and not -// retried until the head moves, since the same token gets the same answer. +// one merge-from-base attempt per (head, base) via UpdateBranchExpectedHead, +// using the hive's own identity. The head and base are the attempt's +// identity in the mutation journal: an identity of the PR alone made the +// first successful update replay as "already applied" for every later one, +// so the guard never asked GitHub again and logged an unconfirmed head every +// pass (projectbluefin/fsdk-containers#474). +// +// On success the result is hygiene, not reviewable content, so the guard's +// baseline is advanced to the new head (holdguard.AdvanceHygiene) — this is +// what keeps the next lift check from reading the hive's own upkeep push as +// drift. GitHub answers 202 and merges later, so a head not visible yet is +// recorded as requested and waited on, not re-sent; when the head later +// moves the request settles without advancing the baseline (between ticks +// it is not provably the hive's merge, and the head was observed while held +// for the lift check). A request still unmoved after +// holdguard.UpdateConfirmWindow stops. +// +// A refusal the same token would get again stops attempts for that head and +// base until either moves, recorded with its reason: the base changed +// .github/workflows/ and the App lacks the workflows permission, a fork the +// App cannot write to (also known up front when "allow edits by maintainers" +// is off), a real conflict, or GitHub calling the branch current. A person +// gets one note naming the reason, so a maintainer knows to update the +// branch; a conflict keeps its own notice (ConflictNoted) and a stale +// "behind" no note at all. +// +// A PR returned to its contributor (stage 072) is left alone: its author's +// hive is expected to push to that branch, and a merge from base underneath +// it only makes that push fail until they pull. // // PRs the guard has not snapshotted yet are skipped — the snapshot step (1) // runs first every tick, so this only ever sees a PR after its baseline is @@ -277,44 +295,56 @@ func maintainHeldPRMergeability( if ghClient == nil || store == nil { return } + returned := stagelabels.ReturnedToContributor.Label() for _, pr := range held { repo := fullRepoName(pr.Repo, org) - rec, ok := store.Recorded(repo, pr.Number) - if !ok { + if _, ok := store.Recorded(repo, pr.Number); !ok { continue } + head, base := pr.HeadSHA, pr.BaseSHA + if store.SettleUpdateRequest(repo, pr.Number, head) { + logger.Info("hold guard: requested branch update settled; the head moved", + "repo", repo, "pr", pr.Number, "head", head) + } state := strings.ToLower(pr.MergeableState) if state != "behind" && state != "dirty" { continue } - if pr.HeadSHA != "" && rec.WorkflowRefusedHeadSHA == pr.HeadSHA { + if hasLabel(pr.Labels, returned) { continue } - if err := ghClient.UpdateBranch(ctx, pr.Repo, pr.Number); err != nil { - if github.IsWorkflowPermissionRefusal(err) { - store.MarkWorkflowRefused(repo, pr.Number, pr.HeadSHA) - logger.Warn("hold guard: GitHub refused to update held PR from its base; the base changed workflow files and the App lacks the workflows permission, so a person must update the branch", - "repo", repo, "pr", pr.Number, "head", pr.HeadSHA, "mergeable_state", state, - "reason", prstage.AutoMergeReasonWorkflowPermission, "error", err) - continue + rec, _ := store.Recorded(repo, pr.Number) + if reason, stopped := rec.UpdateStoppedFor(head, base); stopped { + noteHeldPRUpdateStopped(ctx, writer, store, repo, pr, reason, logger) + continue + } + if pending, expired := store.UpdatePending(repo, pr.Number, head, base); pending { + if expired { + stopHeldPRUpdate(ctx, writer, store, repo, pr, holdguard.UpdateStopNotLanded, nil, logger) } - if state != "dirty" { - // "behind" is expected to be a trivial fast-forward; a - // failure here is an anomaly (race, API hiccup), not the - // known-unresolvable conflict the comment below is for. + continue + } + if pr.FromFork && !pr.MaintainerCanModify { + stopHeldPRUpdate(ctx, writer, store, repo, pr, holdguard.UpdateStopForkNotEditable, nil, logger) + continue + } + if err := ghClient.UpdateBranchExpectedHead(ctx, pr.Repo, pr.Number, head, base); err != nil { + switch github.ClassifyBranchUpdateError(err, pr.FromFork) { + case github.BranchUpdateWorkflowPermission: + stopHeldPRUpdate(ctx, writer, store, repo, pr, holdguard.UpdateStopWorkflowPermission, err, logger) + case github.BranchUpdateForkNotEditable: + stopHeldPRUpdate(ctx, writer, store, repo, pr, holdguard.UpdateStopForkNotEditable, err, logger) + case github.BranchUpdateUpToDate: + stopHeldPRUpdate(ctx, writer, store, repo, pr, holdguard.UpdateStopUpToDate, err, logger) + case github.BranchUpdateConflict: + stopHeldPRUpdate(ctx, writer, store, repo, pr, holdguard.UpdateStopConflict, err, logger) + default: + // A transient failure (rate limit, 5xx, a fenced or + // unreconciled journal entry) says nothing about the next + // attempt; try again next pass. logger.Warn("hold guard: keeping held PR mergeable failed", "repo", repo, "pr", pr.Number, "mergeable_state", state, "error", err) - continue } - if rec.ConflictNoted || writer == nil { - continue - } - if cerr := writer.CreateIssueComment(ctx, pr.Repo, pr.Number, holdguard.ConflictCommentBody()); cerr != nil { - logger.Warn("hold guard: posting held-PR conflict notice failed; will retry next pass", - "repo", repo, "pr", pr.Number, "error", cerr) - continue - } - store.MarkConflictNoted(repo, pr.Number) continue } commits := fetchCommits(pr.Repo, pr.Number) @@ -322,12 +352,13 @@ func maintainHeldPRMergeability( if len(commits) > 0 { newHead = commits[len(commits)-1].SHA } - if newHead == "" || newHead == pr.HeadSHA { - // UpdateBranch always moves the head; failing to confirm the new - // SHA means the commit list refresh failed — leave the baseline - // alone rather than advance it on stale information. - logger.Warn("hold guard: could not confirm new head after keeping held PR mergeable", - "repo", repo, "pr", pr.Number) + if newHead == "" || newHead == head { + // Accepted, but GitHub merges after answering 202 (or the + // commit list refresh failed): wait for the head to move + // rather than ask again on the next pass. + store.MarkUpdateRequested(repo, pr.Number, head, base) + logger.Info("hold guard: GitHub accepted the branch update for held PR; waiting for the head to move", + "repo", repo, "pr", pr.Number, "head", head, "base", base, "mergeable_state", state) continue } store.AdvanceHygiene(repo, pr.Number, newHead, commits) @@ -336,6 +367,58 @@ func maintainHeldPRMergeability( } } +// stopHeldPRUpdate records that the guard stops updating pr's head on its +// base for reason and tells a person once. err is GitHub's refusal, nil when +// the guard stopped without asking. +func stopHeldPRUpdate(ctx context.Context, writer forge.IssueWriter, store *holdguard.Store, repo string, pr github.PullRequest, reason string, err error, logger *slog.Logger) { + store.MarkUpdateStopped(repo, pr.Number, pr.HeadSHA, pr.BaseSHA, reason) + args := []any{"repo", repo, "pr", pr.Number, "head", pr.HeadSHA, "base", pr.BaseSHA, + "mergeable_state", strings.ToLower(pr.MergeableState), "reason", reason} + if err != nil { + args = append(args, "error", err) + } + logger.Warn("hold guard: stopped keeping held PR mergeable until its head or base moves", args...) + noteHeldPRUpdateStopped(ctx, writer, store, repo, pr, reason, logger) +} + +// noteHeldPRUpdateStopped posts the person-facing note for a stopped update +// once per head and reason, retrying a failed post on later passes. A +// conflict keeps the conflict notice (dirty only, as before); GitHub calling +// a "behind" branch current is a stale state, not something to act on. +func noteHeldPRUpdateStopped(ctx context.Context, writer forge.IssueWriter, store *holdguard.Store, repo string, pr github.PullRequest, reason string, logger *slog.Logger) { + if writer == nil { + return + } + rec, ok := store.Recorded(repo, pr.Number) + if !ok { + return + } + switch reason { + case holdguard.UpdateStopUpToDate: + return + case holdguard.UpdateStopConflict: + if rec.ConflictNoted || strings.ToLower(pr.MergeableState) != "dirty" { + return + } + if err := writer.CreateIssueComment(ctx, pr.Repo, pr.Number, holdguard.ConflictCommentBody()); err != nil { + logger.Warn("hold guard: posting held-PR conflict notice failed; will retry next pass", + "repo", repo, "pr", pr.Number, "error", err) + return + } + store.MarkConflictNoted(repo, pr.Number) + return + } + if rec.UpdateStopNoted(pr.HeadSHA, reason) { + return + } + if err := writer.CreateIssueComment(ctx, pr.Repo, pr.Number, holdguard.UpdateStoppedCommentBody(reason)); err != nil { + logger.Warn("hold guard: posting held-PR update notice failed; will retry next pass", + "repo", repo, "pr", pr.Number, "reason", reason, "error", err) + return + } + store.MarkUpdateStopNoted(repo, pr.Number, pr.HeadSHA, reason) +} + // intentConfigFromCfg maps config.IntentConfig onto the intent package's // classification config. Shared by writeIntentVerdicts (human merge lane) and // the App self-merge sweep's IntentGate (#6258) so both lanes classify with diff --git a/src/docs/scale-envelope.md b/src/docs/scale-envelope.md index af6ee1c21d..067a72b421 100644 --- a/src/docs/scale-envelope.md +++ b/src/docs/scale-envelope.md @@ -34,7 +34,7 @@ ones to: 2. **Truncation must preserve the prioritization signal, not cut an arbitrary slice.** This holds today and is easy to assume backwards: - Issues are sorted by actionable rank tier, then **oldest first** within each tier before any cap is applied - (`RankActionableIssues`, `src/pkg/github/client.go:4707-4717`, descending `AgeMinutes`). + (`RankActionableIssues`, `src/pkg/github/client.go:4712-4722`, descending `AgeMinutes`). - PRs are sorted by **review class then oldest-first within class** (`SortPullRequestsForReview`, `src/pkg/github/review_priority.go:334`; fixes → refactors/docs → tests). diff --git a/src/pkg/github/automerge/automerge_sweep.go b/src/pkg/github/automerge/automerge_sweep.go index 413d051eb8..12fbf3797b 100644 --- a/src/pkg/github/automerge/automerge_sweep.go +++ b/src/pkg/github/automerge/automerge_sweep.go @@ -2458,51 +2458,20 @@ const ( autoMergeReasonWorkflowPermission = prstage.AutoMergeReasonWorkflowPermission ) -type updateBranchFailure int +type updateBranchFailure = hgithub.BranchUpdateFailure const ( - updateBranchOther updateBranchFailure = iota - updateBranchConflict - updateBranchUpToDate - updateBranchForkNotEditable - updateBranchWorkflowPermission + updateBranchOther = hgithub.BranchUpdateOther + updateBranchConflict = hgithub.BranchUpdateConflict + updateBranchUpToDate = hgithub.BranchUpdateUpToDate + updateBranchForkNotEditable = hgithub.BranchUpdateForkNotEditable + updateBranchWorkflowPermission = hgithub.BranchUpdateWorkflowPermission ) -// classifyUpdateBranchError separates the update-branch failures GitHub -// reports with overlapping statuses: 422 covers conflicts and an already -// current branch, while a 422 naming maintainer edits (or a 403 on a fork PR) -// means the app cannot write to the fork head. A 403 refusing to update a -// workflow file means the base changed .github/workflows/ and the App lacks the -// workflows permission, on a fork or not: no retry with the same token can -// succeed, so a person must update the branch. Any other 403 on a same-repo -// branch is a plain permission or rate-limit failure. An unrecognised 422 -// stays a conflict. +// classifyUpdateBranchError is hgithub.ClassifyBranchUpdateError, shared +// with the hold guard's keep-mergeable update. func classifyUpdateBranchError(err error, fork bool) updateBranchFailure { - var ghErr *gh.ErrorResponse - if !errors.As(err, &ghErr) || ghErr.Response == nil { - return updateBranchOther - } - if hgithub.IsWorkflowPermissionRefusal(err) { - return updateBranchWorkflowPermission - } - msg := strings.ToLower(ghErr.Message) - switch ghErr.Response.StatusCode { - case http.StatusForbidden: - if fork { - return updateBranchForkNotEditable - } - case http.StatusUnprocessableEntity: - switch { - case strings.Contains(msg, "maintainer") || strings.Contains(msg, "permission") || strings.Contains(msg, "not authorized"): - return updateBranchForkNotEditable - case strings.Contains(msg, "merge conflict"): - return updateBranchConflict - case strings.Contains(msg, "up to date") || strings.Contains(msg, "up-to-date") || strings.Contains(msg, "no new commits"): - return updateBranchUpToDate - } - return updateBranchConflict - } - return updateBranchOther + return hgithub.ClassifyBranchUpdateError(err, fork) } func (c *Engine) executeUpdateBranchEffect(ctx context.Context, owner, repo string, number int, headSHA, reason string) error { diff --git a/src/pkg/github/branch_update_refusal.go b/src/pkg/github/branch_update_refusal.go new file mode 100644 index 0000000000..03ca61c8b2 --- /dev/null +++ b/src/pkg/github/branch_update_refusal.go @@ -0,0 +1,58 @@ +package github + +import ( + "errors" + "net/http" + "strings" + + gh "github.com/google/go-github/v72/github" +) + +// BranchUpdateFailure is the class of a refused update-branch call. The +// automerge sweep and the hold guard's keep-mergeable update both read it. +type BranchUpdateFailure int + +const ( + BranchUpdateOther BranchUpdateFailure = iota + BranchUpdateConflict + BranchUpdateUpToDate + BranchUpdateForkNotEditable + BranchUpdateWorkflowPermission +) + +// ClassifyBranchUpdateError separates the update-branch failures GitHub +// reports with overlapping statuses: 422 covers conflicts and an already +// current branch, while a 422 naming maintainer edits (or a 403 on a fork PR) +// means the app cannot write to the fork head. A 403 refusing to update a +// workflow file means the base changed .github/workflows/ and the App lacks the +// workflows permission, on a fork or not: no retry with the same token can +// succeed, so a person must update the branch. Any other 403 on a same-repo +// branch is a plain permission or rate-limit failure. An unrecognised 422 +// stays a conflict. +func ClassifyBranchUpdateError(err error, fork bool) BranchUpdateFailure { + var ghErr *gh.ErrorResponse + if !errors.As(err, &ghErr) || ghErr.Response == nil { + return BranchUpdateOther + } + if IsWorkflowPermissionRefusal(err) { + return BranchUpdateWorkflowPermission + } + msg := strings.ToLower(ghErr.Message) + switch ghErr.Response.StatusCode { + case http.StatusForbidden: + if fork { + return BranchUpdateForkNotEditable + } + case http.StatusUnprocessableEntity: + switch { + case strings.Contains(msg, "maintainer") || strings.Contains(msg, "permission") || strings.Contains(msg, "not authorized"): + return BranchUpdateForkNotEditable + case strings.Contains(msg, "merge conflict"): + return BranchUpdateConflict + case strings.Contains(msg, "up to date") || strings.Contains(msg, "up-to-date") || strings.Contains(msg, "no new commits"): + return BranchUpdateUpToDate + } + return BranchUpdateConflict + } + return BranchUpdateOther +} diff --git a/src/pkg/github/client.go b/src/pkg/github/client.go index e1358061db..fa65919705 100644 --- a/src/pkg/github/client.go +++ b/src/pkg/github/client.go @@ -2111,6 +2111,7 @@ func (c *Client) fetchPRsWithDetails(ctx context.Context, repo string, clankerBu FromFork: fromFork, MaintainerCanModify: pr.GetMaintainerCanModify(), BaseRef: prBaseRef(pr), + BaseSHA: pr.GetBase().GetSHA(), RequestedReviewers: reqLogins, RequestedTeams: reqTeams, HoldSources: prHoldSources, diff --git a/src/pkg/holdguard/holdguard.go b/src/pkg/holdguard/holdguard.go index 527e0fb6f9..ad1f68639e 100644 --- a/src/pkg/holdguard/holdguard.go +++ b/src/pkg/holdguard/holdguard.go @@ -45,6 +45,16 @@ // resolve a real conflict this way, a one-time comment (ConflictCommentBody) // tells the human before they open the review, instead of letting them // discover a stale, unmergeable PR mid-review. +// +// Each merge-from-base is one attempt per (head, base) pair. GitHub answers +// update-branch with 202 and merges later, so an accepted attempt whose new +// head is not visible yet is recorded as requested (MarkUpdateRequested) and +// waited on rather than re-sent; one still unmoved after UpdateConfirmWindow +// stops like a refusal. A refusal the same token would get again (workflows +// permission, a fork the App cannot write to, a conflict) or an accepted +// update that never landed stops attempts for that head and base +// (MarkUpdateStopped) until either moves, and a one-time note +// (UpdateStoppedCommentBody) tells a person to update the branch. package holdguard import ( @@ -118,9 +128,28 @@ type Entry struct { ConflictNoted bool `json:"conflict_noted,omitempty"` // WorkflowRefusedHeadSHA is the PR head GitHub refused to update from its // base because the base changed .github/workflows/ and the App lacks the - // workflows permission. The same head gets the same refusal, so the - // guard does not ask again until the head moves. + // workflows permission. Written by builds before UpdateStopped* existed; + // still read (UpdateStoppedFor) so such a record does not retry, never + // written now. WorkflowRefusedHeadSHA string `json:"workflow_refused_head_sha,omitempty"` + // UpdateRequestedHeadSHA and UpdateRequestedBaseSHA are the head and base + // of a merge-from-base GitHub accepted (202) whose new head the guard has + // not seen yet; UpdateRequestedAt is when. Cleared once the head moves. + UpdateRequestedHeadSHA string `json:"update_requested_head_sha,omitempty"` + UpdateRequestedBaseSHA string `json:"update_requested_base_sha,omitempty"` + UpdateRequestedAt time.Time `json:"update_requested_at,omitempty"` + // UpdateStoppedHeadSHA and UpdateStoppedBaseSHA are the head and base the + // guard stopped updating, and UpdateStoppedReason is why (one of the + // UpdateStop* reasons). No attempt is made again until the head or the + // base moves. + UpdateStoppedHeadSHA string `json:"update_stopped_head_sha,omitempty"` + UpdateStoppedBaseSHA string `json:"update_stopped_base_sha,omitempty"` + UpdateStoppedReason string `json:"update_stopped_reason,omitempty"` + // UpdateStopNotedHeadSHA and UpdateStopNotedReason name the head and + // reason the person-facing note was last posted for, so a stop retried + // on a new base with the same answer does not post again. + UpdateStopNotedHeadSHA string `json:"update_stop_noted_head_sha,omitempty"` + UpdateStopNotedReason string `json:"update_stop_noted_reason,omitempty"` // ObservedHeldHeadSHA is the latest head the hive itself observed on the // PR while the hold gate was still on (#13603). A current head equal to // it provably predates the gate lift, so a trusted human's lift reviewed @@ -307,6 +336,7 @@ func (s *Store) AdvanceHygiene(repo string, number int, headSHA string, commits e.ObservedHeldHeadSHA = headSHA e.CommitSHAs, e.Authors = commitSets(commits) e.ConflictNoted = false + e.clearUpdateRequested() e.UpdatedAt = s.now() s.saveLocked() } @@ -324,17 +354,119 @@ func (s *Store) MarkConflictNoted(repo string, number int) { } } -// MarkWorkflowRefused records that GitHub refused to update the PR at headSHA -// from its base for lack of the workflows permission. No-op for an untracked -// PR or an empty head. -func (s *Store) MarkWorkflowRefused(repo string, number int, headSHA string) { +// Reasons the guard stops keeping a held PR mergeable for a head and base. +// The first two match the automerge sweep's reasons for the same refusals. +const ( + UpdateStopWorkflowPermission = "workflow-permission" + UpdateStopForkNotEditable = "fork-not-editable" + UpdateStopConflict = "conflict" + UpdateStopUpToDate = "up-to-date" + UpdateStopNotLanded = "update-not-landed" +) + +// UpdateConfirmWindow is how long an accepted (202) merge-from-base may take +// to move the head before the guard stops waiting. GitHub normally finishes +// in seconds; past this the update is treated as one that will not land. +const UpdateConfirmWindow = 30 * time.Minute + +// UpdateStoppedFor reports whether the guard stopped updating this head on +// this base, and why. A record from a build that only knew the workflows +// refusal (WorkflowRefusedHeadSHA) stops on its head alone. +func (e Entry) UpdateStoppedFor(headSHA, baseSHA string) (string, bool) { + if headSHA == "" { + return "", false + } + if e.UpdateStoppedHeadSHA == headSHA && e.UpdateStoppedBaseSHA == baseSHA && e.UpdateStoppedReason != "" { + return e.UpdateStoppedReason, true + } + if e.WorkflowRefusedHeadSHA == headSHA { + return UpdateStopWorkflowPermission, true + } + return "", false +} + +// UpdateStopNoted reports whether the person-facing note for reason already +// posted for headSHA. +func (e Entry) UpdateStopNoted(headSHA, reason string) bool { + return headSHA != "" && e.UpdateStopNotedHeadSHA == headSHA && e.UpdateStopNotedReason == reason +} + +func (e *Entry) clearUpdateRequested() { + e.UpdateRequestedHeadSHA, e.UpdateRequestedBaseSHA = "", "" + e.UpdateRequestedAt = time.Time{} +} + +// MarkUpdateRequested records that GitHub accepted a merge-from-base for +// headSHA on baseSHA whose new head is not visible yet. No-op for an +// untracked PR or an empty head. +func (s *Store) MarkUpdateRequested(repo string, number int, headSHA, baseSHA string) { + if headSHA == "" { + return + } + s.mu.Lock() + defer s.mu.Unlock() + if e := s.entries[Key(repo, number)]; e != nil { + e.UpdateRequestedHeadSHA, e.UpdateRequestedBaseSHA = headSHA, baseSHA + e.UpdateRequestedAt = s.now() + e.UpdatedAt = s.now() + s.saveLocked() + } +} + +// UpdatePending reports whether an accepted merge-from-base for headSHA on +// baseSHA is still awaited, and whether UpdateConfirmWindow has passed +// without the head moving. +func (s *Store) UpdatePending(repo string, number int, headSHA, baseSHA string) (pending, expired bool) { + s.mu.Lock() + defer s.mu.Unlock() + e := s.entries[Key(repo, number)] + if e == nil || headSHA == "" || e.UpdateRequestedHeadSHA != headSHA || e.UpdateRequestedBaseSHA != baseSHA { + return false, false + } + return true, s.now().Sub(e.UpdateRequestedAt) >= UpdateConfirmWindow +} + +// SettleUpdateRequest forgets an awaited merge-from-base once the PR's head +// is no longer the head it was requested for, and reports whether there was +// one. The baseline is left alone: a head that moved between ticks is not +// proven to be the hive's own merge, so the lift check judges it. +func (s *Store) SettleUpdateRequest(repo string, number int, headSHA string) bool { + s.mu.Lock() + defer s.mu.Unlock() + e := s.entries[Key(repo, number)] + if e == nil || e.UpdateRequestedHeadSHA == "" || headSHA == "" || e.UpdateRequestedHeadSHA == headSHA { + return false + } + e.clearUpdateRequested() + e.UpdatedAt = s.now() + s.saveLocked() + return true +} + +// MarkUpdateStopped records that the guard stops updating headSHA on baseSHA +// for reason, until the head or the base moves. No-op for an untracked PR +// or an empty head. +func (s *Store) MarkUpdateStopped(repo string, number int, headSHA, baseSHA, reason string) { if headSHA == "" { return } s.mu.Lock() defer s.mu.Unlock() if e := s.entries[Key(repo, number)]; e != nil { - e.WorkflowRefusedHeadSHA = headSHA + e.UpdateStoppedHeadSHA, e.UpdateStoppedBaseSHA, e.UpdateStoppedReason = headSHA, baseSHA, reason + e.clearUpdateRequested() + e.UpdatedAt = s.now() + s.saveLocked() + } +} + +// MarkUpdateStopNoted records that the person-facing note for reason posted +// for headSHA. +func (s *Store) MarkUpdateStopNoted(repo string, number int, headSHA, reason string) { + s.mu.Lock() + defer s.mu.Unlock() + if e := s.entries[Key(repo, number)]; e != nil { + e.UpdateStopNotedHeadSHA, e.UpdateStopNotedReason = headSHA, reason e.UpdatedAt = s.now() s.saveLocked() } @@ -587,3 +719,28 @@ func ConflictCommentBody() string { b.WriteString("It will stay held and will NOT merge on its own even once reviewed — removing `" + ReHoldLabel + "` alone will not be enough. Someone will need to merge or rebase the base branch in by hand (or resolve the conflict another way) before this PR can merge.\n") return b.String() } + +// UpdateStoppedCommentBody is the one-time note posted when the guard stops +// keeping a held PR up to date with its base for reason (an UpdateStop* +// reason other than conflict, which has ConflictCommentBody). +func UpdateStoppedCommentBody(reason string) string { + why := "GitHub did not complete the update." + action := "use **Update branch** on this PR, or merge the base branch in by hand." + switch reason { + case UpdateStopWorkflowPermission: + why = "GitHub refused the update because the base branch changed files under `.github/workflows/` and the Hive app does not have the workflows permission." + case UpdateStopForkNotEditable: + why = "The branch is in a fork the Hive app cannot push to (\"Allow edits by maintainers\" is off, or GitHub refused the push)." + action = "ask the author to merge the base branch in, or use **Update branch** on this PR if GitHub offers it." + case UpdateStopNotLanded: + why = fmt.Sprintf("GitHub accepted the update, but the branch had not moved %d minutes later.", int(UpdateConfirmWindow/time.Minute)) + } + var b strings.Builder + b.WriteString("## ⚠️ Hold-gate mergeability: Hive could not update this branch\n\n") + b.WriteString("This PR is hold-gated and has fallen behind its base branch. Hive keeps held PRs up to date while they wait on review, but it could not update this one. ") + b.WriteString(why) + b.WriteString("\n\nHive will not try again until this branch or its base branch changes. To bring it up to date now, ") + b.WriteString(action) + b.WriteString("\n") + return b.String() +} diff --git a/src/pkg/holdguard/update_state_test.go b/src/pkg/holdguard/update_state_test.go new file mode 100644 index 0000000000..54e5d80be7 --- /dev/null +++ b/src/pkg/holdguard/update_state_test.go @@ -0,0 +1,142 @@ +package holdguard + +import ( + "path/filepath" + "strings" + "testing" +) + +// An accepted merge-from-base is awaited for its own head and base only, +// expires after UpdateConfirmWindow, and is forgotten once the head moves. +// Settling leaves the baseline alone: the lift check judges the new head. +func TestUpdateRequestIsPendingUntilTheHeadMoves(t *testing.T) { + s, clock := newTestStore(t) + + s.MarkUpdateRequested("r", 1, "h1", "b1") // untracked: no-op + if pending, _ := s.UpdatePending("r", 1, "h1", "b1"); pending { + t.Fatal("an untracked PR has no awaited update") + } + + s.Snapshot("r", 1, "h1", nil) + s.MarkUpdateRequested("r", 1, "", "b1") // empty head: no-op + if pending, _ := s.UpdatePending("r", 1, "", "b1"); pending { + t.Fatal("an empty head has no awaited update") + } + + s.MarkUpdateRequested("r", 1, "h1", "b1") + if pending, expired := s.UpdatePending("r", 1, "h1", "b1"); !pending || expired { + t.Fatalf("UpdatePending right after the request = (%v, %v), want (true, false)", pending, expired) + } + for _, other := range [][2]string{{"h1", "b2"}, {"h2", "b1"}} { + if pending, _ := s.UpdatePending("r", 1, other[0], other[1]); pending { + t.Fatalf("UpdatePending(%s, %s) = true; the request was for h1 on b1", other[0], other[1]) + } + } + + *clock = clock.Add(UpdateConfirmWindow) + if pending, expired := s.UpdatePending("r", 1, "h1", "b1"); !pending || !expired { + t.Fatalf("UpdatePending after UpdateConfirmWindow = (%v, %v), want (true, true)", pending, expired) + } + + if s.SettleUpdateRequest("r", 1, "h1") || s.SettleUpdateRequest("r", 1, "") || s.SettleUpdateRequest("r", 2, "h2") { + t.Fatal("SettleUpdateRequest settled although the head has not moved, is empty, or the PR is untracked") + } + if !s.SettleUpdateRequest("r", 1, "h2") { + t.Fatal("SettleUpdateRequest must settle once the head moved off the requested head") + } + if pending, _ := s.UpdatePending("r", 1, "h1", "b1"); pending { + t.Fatal("a settled request is still pending") + } + if s.SettleUpdateRequest("r", 1, "h3") { + t.Fatal("a request settles once") + } + if rec, _ := s.Recorded("r", 1); rec.HeadSHA != "h1" { + t.Fatalf("settling moved the baseline to %q; it must stay at h1", rec.HeadSHA) + } +} + +func TestAdvanceHygieneForgetsTheAwaitedUpdate(t *testing.T) { + s, _ := newTestStore(t) + s.Snapshot("r", 1, "h1", nil) + s.MarkUpdateRequested("r", 1, "h1", "b1") + s.AdvanceHygiene("r", 1, "m1", []Commit{{SHA: "m1", Author: "hive"}}) + if pending, _ := s.UpdatePending("r", 1, "h1", "b1"); pending { + t.Fatal("AdvanceHygiene must clear the awaited update it confirmed") + } +} + +// A stop holds for one head on one base and survives a restart; a record +// from a build that only knew the workflows refusal stops on its head alone. +func TestUpdateStoppedForOneHeadAndBase(t *testing.T) { + path := filepath.Join(t.TempDir(), "hold-guard.json") + s := Load(path) + + s.MarkUpdateStopped("r", 1, "h1", "b1", UpdateStopNotLanded) // untracked: no-op + if _, ok := s.Recorded("r", 1); ok { + t.Fatal("MarkUpdateStopped must not create an entry for an untracked PR") + } + + s.Snapshot("r", 1, "h1", nil) + s.MarkUpdateRequested("r", 1, "h1", "b1") + s.MarkUpdateStopped("r", 1, "h1", "b1", UpdateStopNotLanded) + s.MarkUpdateStopped("r", 1, "", "b1", UpdateStopConflict) // empty head: no-op + if pending, _ := s.UpdatePending("r", 1, "h1", "b1"); pending { + t.Fatal("a stopped update is no longer awaited") + } + + rec, _ := Load(path).Recorded("r", 1) + if reason, ok := rec.UpdateStoppedFor("h1", "b1"); !ok || reason != UpdateStopNotLanded { + t.Fatalf("UpdateStoppedFor(h1, b1) after reload = (%q, %v), want (%q, true)", reason, ok, UpdateStopNotLanded) + } + for _, other := range [][2]string{{"h1", "b2"}, {"h2", "b1"}, {"", "b1"}} { + if _, ok := rec.UpdateStoppedFor(other[0], other[1]); ok { + t.Fatalf("UpdateStoppedFor(%q, %q) = true; the stop was for h1 on b1", other[0], other[1]) + } + } + + legacy := Entry{WorkflowRefusedHeadSHA: "h3"} + if reason, ok := legacy.UpdateStoppedFor("h3", "any-base"); !ok || reason != UpdateStopWorkflowPermission { + t.Fatalf("legacy workflows refusal = (%q, %v), want (%q, true)", reason, ok, UpdateStopWorkflowPermission) + } +} + +func TestUpdateStopNotedIsPerHeadAndReason(t *testing.T) { + s, _ := newTestStore(t) + s.MarkUpdateStopNoted("r", 1, "h1", UpdateStopForkNotEditable) // untracked: no-op + if _, ok := s.Recorded("r", 1); ok { + t.Fatal("MarkUpdateStopNoted must not create an entry for an untracked PR") + } + + s.Snapshot("r", 1, "h1", nil) + s.MarkUpdateStopNoted("r", 1, "h1", UpdateStopForkNotEditable) + rec, _ := s.Recorded("r", 1) + if !rec.UpdateStopNoted("h1", UpdateStopForkNotEditable) { + t.Fatal("the note posted for h1 must read as noted") + } + if rec.UpdateStopNoted("h1", UpdateStopNotLanded) || rec.UpdateStopNoted("h2", UpdateStopForkNotEditable) { + t.Fatal("a note covers its own head and reason only") + } + if (Entry{}).UpdateStopNoted("", "") { + t.Fatal("an empty head is never noted") + } +} + +func TestUpdateStoppedCommentBodySaysWhyAndWhatToDo(t *testing.T) { + for _, tc := range []struct { + reason string + want []string + }{ + {UpdateStopWorkflowPermission, []string{"`.github/workflows/`", "use **Update branch**"}}, + {UpdateStopForkNotEditable, []string{"\"Allow edits by maintainers\" is off", "ask the author to merge the base branch in"}}, + {UpdateStopNotLanded, []string{"had not moved 30 minutes later", "use **Update branch**"}}, + {UpdateStopUpToDate, []string{"GitHub did not complete the update.", "use **Update branch**"}}, + } { + body := UpdateStoppedCommentBody(tc.reason) + want := append([]string{"Hive could not update this branch", "will not try again until this branch or its base branch changes"}, tc.want...) + for _, w := range want { + if !strings.Contains(body, w) { + t.Errorf("UpdateStoppedCommentBody(%q) lacks %q:\n%s", tc.reason, w, body) + } + } + } +}