From d17fe508b540e8594d683b33dc863317d3a9b9f3 Mon Sep 17 00:00:00 2001 From: Karibhou Date: Tue, 27 Oct 2026 09:47:15 +0000 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=90=9B=20fix(holdguard):=20advance=20?= =?UTF-8?q?the=20held=20baseline=20only=20on=20a=20proven=20merge-from-bas?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After an accepted update-branch, maintainHeldPRMergeability refetched the PR's commits and advanced the hold baseline to whatever head it found. A contributor push landing between the 202 and the refetch was therefore adopted as hive hygiene, moving the reviewed baseline past unreviewed commits. The advance now requires the new head to be a two-parent commit whose parents are the head the hive asked to update and the base SHA (holdguard.IsHygieneMerge). Any other head leaves the baseline pinned, so the lift check reads the move as drift and the PR goes back to a human. ListPRCommits now carries the parent SHAs to make that proof possible. Hive-Run: hivecommons/hive#14534 Hive-Plan: review-backlog security finding (PR #14401 out-of-scope) Hive-Spec: holdguard#hygiene-baseline-advance Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HRH6kjKSaVf8mEsXFx1uAd Signed-off-by: Karibhou --- ...security-hold-guard-hygiene-merge-proof.md | 1 + src/cmd/hive/hold_guard_mergeability_test.go | 65 +++++++++++++++++-- src/cmd/hive/main_helpers.go | 19 +++++- src/pkg/github/pr_commits.go | 9 +++ src/pkg/holdguard/holdguard.go | 17 +++++ src/pkg/holdguard/hygiene_merge_test.go | 32 +++++++++ 6 files changed, 136 insertions(+), 7 deletions(-) create mode 100644 changelog.d/security-hold-guard-hygiene-merge-proof.md create mode 100644 src/pkg/holdguard/hygiene_merge_test.go diff --git a/changelog.d/security-hold-guard-hygiene-merge-proof.md b/changelog.d/security-hold-guard-hygiene-merge-proof.md new file mode 100644 index 0000000000..adcdc756d0 --- /dev/null +++ b/changelog.d/security-hold-guard-hygiene-merge-proof.md @@ -0,0 +1 @@ +- The hold guard only advances a held PR's reviewed baseline after a merge-from-base when the new head is provably that merge (parents are the updated head and the base), so a contributor push racing the update can no longer be adopted as reviewed hygiene ([#14534](https://github.com/hivecommons/hive/issues/14534)). diff --git a/src/cmd/hive/hold_guard_mergeability_test.go b/src/cmd/hive/hold_guard_mergeability_test.go index 431753228a..867bc1b73b 100644 --- a/src/cmd/hive/hold_guard_mergeability_test.go +++ b/src/cmd/hive/hold_guard_mergeability_test.go @@ -22,7 +22,7 @@ func heldPRsActionable(prs ...github.PullRequest) *github.ActionableResult { func TestMaintainHeldPRMergeabilityUpdatesBehindBranchAndAdvancesBaseline(t *testing.T) { store := newTestHoldGuardStore(t) fake := &holdGuardServer{commits: map[int][]map[string]any{ - 7: {ghCommit("c1", "strategist-bot", "docs: plan"), ghCommit("c2", "hive-bot", "Merge branch 'main' into feature")}, + 7: {ghCommit("c1", "strategist-bot", "docs: plan"), ghMergeCommit("c2", "hive-bot", "Merge branch 'main' into feature", "c1", "b1")}, }} client := newHoldGuardClient(t, fake) cfg := escalationTestConfig() @@ -31,7 +31,7 @@ func TestMaintainHeldPRMergeabilityUpdatesBehindBranchAndAdvancesBaseline(t *tes t.Fatal("test setup: snapshot must record") } - held := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", MergeableState: "behind"}) + held := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b1", MergeableState: "behind"}) if got := enforceHoldGuard(context.Background(), cfg, client, client, held, discardLogger()); len(got) != 0 { t.Fatalf("keeping a held PR mergeable must not flag it for re-review, got %v", got) } @@ -51,6 +51,63 @@ func TestMaintainHeldPRMergeabilityUpdatesBehindBranchAndAdvancesBaseline(t *tes } } +// ghMergeCommit is ghCommit with explicit parent SHAs, first parent first, so +// a test can serve the merge-from-base GitHub's update-branch produces. +func ghMergeCommit(sha, login, msg string, parents ...string) map[string]any { + c := ghCommit(sha, login, msg) + ps := make([]map[string]string, 0, len(parents)) + for _, p := range parents { + ps = append(ps, map[string]string{"sha": p}) + } + c["parents"] = ps + return c +} + +// #14534: a head that moved after the update request but is not the merge of +// the base into the requested head (a contributor push racing the refetch, or +// a merge of some other branch) must not become the baseline. +func TestMaintainHeldPRMergeabilityKeepsBaselineWhenNewHeadIsNotTheHygieneMerge(t *testing.T) { + tests := []struct { + name string + head map[string]any + }{ + {"contributor push on top of the held head", ghMergeCommit("c2", "contributor", "feat: sneak in", "c1")}, + {"merge of a non-base commit into the held head", ghMergeCommit("c2", "contributor", "Merge branch 'evil'", "c1", "x9")}, + {"merge into a head other than the requested one", ghMergeCommit("c2", "contributor", "Merge branch 'main'", "x8", "b1")}, + {"head with no parent information", ghCommit("c2", "contributor", "Merge branch 'main'")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := newTestHoldGuardStore(t) + fake := &holdGuardServer{commits: map[int][]map[string]any{ + 7: {ghCommit("c1", "strategist-bot", "docs: plan"), tt.head}, + }} + client := newHoldGuardClient(t, fake) + cfg := escalationTestConfig() + if !store.Snapshot("acme/widgets", 7, "c1", []holdguard.Commit{{SHA: "c1", Author: "strategist-bot"}}) { + t.Fatal("test setup: snapshot must record") + } + + held := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b1", MergeableState: "behind"}) + _ = enforceHoldGuard(context.Background(), cfg, client, client, held, discardLogger()) + + fake.mu.Lock() + calls := fake.updateBranchCalls[7] + fake.mu.Unlock() + if calls != 1 { + t.Fatalf("update-branch calls for PR 7 = %d, want 1", calls) + } + rec, ok := store.Recorded("acme/widgets", 7) + if !ok || rec.HeadSHA != "c1" { + t.Fatalf("baseline = %+v ok=%v, want it left pinned at c1: c2 is not proven to be the hive's merge", rec, ok) + } + if drift := holdguard.Diff(rec, "c2", nil); !drift.Moved { + t.Fatal("the unproven head must still read as drift at lift time") + } + }) + } +} + func TestMaintainHeldPRMergeabilityNotesUnresolvableConflictOnce(t *testing.T) { store := newTestHoldGuardStore(t) fake := &holdGuardServer{ @@ -85,9 +142,9 @@ func TestMaintainHeldPRMergeabilityNotesUnresolvableConflictOnce(t *testing.T) { // 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.commits[7] = append(fake.commits[7], ghMergeCommit("c2", "hive-bot", "Merge branch 'main' into feature", "c1", "b1")) 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: "b1", MergeableState: "behind"}) _ = enforceHoldGuard(context.Background(), cfg, client, client, heldResolved, discardLogger()) rec, ok = store.Recorded("acme/widgets", 7) if !ok || rec.ConflictNoted { diff --git a/src/cmd/hive/main_helpers.go b/src/cmd/hive/main_helpers.go index 5d4a562f0e..4a352cafca 100644 --- a/src/cmd/hive/main_helpers.go +++ b/src/cmd/hive/main_helpers.go @@ -95,7 +95,7 @@ func enforceHoldGuard( } commits := make([]holdguard.Commit, 0, len(got)) for _, c := range got { - commits = append(commits, holdguard.Commit{SHA: c.SHA, Author: c.Author, Title: c.Title}) + commits = append(commits, holdguard.Commit{SHA: c.SHA, Author: c.Author, Title: c.Title, Parents: c.Parents}) } return commits } @@ -246,8 +246,11 @@ func enforceHoldGuard( // 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 +// push as drift. The advance requires proof that the new head IS that merge +// (holdguard.IsHygieneMerge: parents are the requested head and the base); a +// head that moved any other way stays unadopted and reads 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. // @@ -311,6 +314,16 @@ func maintainHeldPRMergeability( "repo", repo, "pr", pr.Number) continue } + if !holdguard.IsHygieneMerge(commits[len(commits)-1], pr.HeadSHA, pr.BaseSHA) { + // The head moved, but not provably to the hive's own merge of the + // base into the head it asked to update: a contributor push may + // have landed between the 202 and the refetch. Advancing would + // adopt unreviewed commits as the baseline, so leave it pinned and + // let the lift check read the move as drift (hivecommons/hive#14534). + logger.Warn("hold guard: new head after keeping held PR mergeable is not a merge of the base into the held head; baseline left pinned", + "repo", repo, "pr", pr.Number, "requested_head", pr.HeadSHA, "base", pr.BaseSHA, "new_head", newHead) + continue + } store.AdvanceHygiene(repo, pr.Number, newHead, commits) logger.Info("hold guard: merged base into held PR to keep it mergeable", "repo", repo, "pr", pr.Number, "mergeable_state", state, "new_head", newHead) diff --git a/src/pkg/github/pr_commits.go b/src/pkg/github/pr_commits.go index ab749ffc9e..fddb29fd32 100644 --- a/src/pkg/github/pr_commits.go +++ b/src/pkg/github/pr_commits.go @@ -35,6 +35,10 @@ type PRCommit struct { // one identifies merge-from-base maintenance commits that do not, by // themselves, address a human review. ParentCount int + // Parents are the parent commit SHAs in git order (first parent first). + // The hold guard uses them to prove a post-update head is the hive's own + // merge-from-base rather than a contributor push (hivecommons/hive#14534). + Parents []string } func prCommitTime(rc *gh.RepositoryCommit) time.Time { @@ -72,6 +76,10 @@ func (c *Client) ListPRCommits(ctx context.Context, repo string, number int) ([] if i := strings.IndexByte(title, '\n'); i >= 0 { title = title[:i] } + parents := make([]string, 0, len(rc.Parents)) + for _, p := range rc.Parents { + parents = append(parents, p.GetSHA()) + } out = append(out, PRCommit{ SHA: rc.GetSHA(), Author: author, @@ -79,6 +87,7 @@ func (c *Client) ListPRCommits(ctx context.Context, repo string, number int) ([] Message: message, AuthoredAt: prCommitTime(rc), ParentCount: len(rc.Parents), + Parents: parents, }) } return out, nil diff --git a/src/pkg/holdguard/holdguard.go b/src/pkg/holdguard/holdguard.go index 27477bad2d..70323a5879 100644 --- a/src/pkg/holdguard/holdguard.go +++ b/src/pkg/holdguard/holdguard.go @@ -92,6 +92,10 @@ type Commit struct { Author string `json:"author,omitempty"` // Title is the first line of the commit message. Title string `json:"title,omitempty"` + // Parents are the commit's parent SHAs, first parent first. Never + // persisted in an Entry; carried only so the mergeability upkeep can + // prove a new head is its own merge-from-base (IsHygieneMerge). + Parents []string `json:"parents,omitempty"` } // Entry is the persisted per-PR snapshot taken while the PR sits hold-gated. @@ -278,6 +282,19 @@ func (s *Store) ReArm(repo string, number int, headSHA string, commits []Commit) s.saveLocked() } +// IsHygieneMerge reports whether head is provably the merge-from-base the hive +// asked for: a two-parent commit whose first parent is requestedHead (the head +// the hive updated) and whose second parent is baseSHA (the base it merged +// in). Anything else — a contributor push landing between the update request +// and the refetch, a merge of some other branch, an unknown head or base — +// is not proven hygiene and must not become the baseline (hivecommons/hive#14534). +func IsHygieneMerge(head Commit, requestedHead, baseSHA string) bool { + if head.SHA == "" || requestedHead == "" || baseSHA == "" || len(head.Parents) != 2 { + return false + } + return head.Parents[0] == requestedHead && head.Parents[1] == baseSHA +} + // AdvanceHygiene moves a held PR's baseline forward to a head the hive itself // produced by merging the base into the held branch to keep it mergeable // (#10437). Unlike ReArm, this is NOT a response to drift: the hive's own diff --git a/src/pkg/holdguard/hygiene_merge_test.go b/src/pkg/holdguard/hygiene_merge_test.go new file mode 100644 index 0000000000..6c16411ae9 --- /dev/null +++ b/src/pkg/holdguard/hygiene_merge_test.go @@ -0,0 +1,32 @@ +package holdguard + +import "testing" + +// #14534: only the merge of the base into the requested head is hygiene. +func TestIsHygieneMerge(t *testing.T) { + tests := []struct { + name string + head Commit + requested string + base string + want bool + }{ + {"merge of base into requested head", Commit{SHA: "m", Parents: []string{"h", "b"}}, "h", "b", true}, + {"single-parent push on the requested head", Commit{SHA: "m", Parents: []string{"h"}}, "h", "b", false}, + {"merge of a non-base commit", Commit{SHA: "m", Parents: []string{"h", "x"}}, "h", "b", false}, + {"merge into another head", Commit{SHA: "m", Parents: []string{"x", "b"}}, "h", "b", false}, + {"parents swapped", Commit{SHA: "m", Parents: []string{"b", "h"}}, "h", "b", false}, + {"octopus merge", Commit{SHA: "m", Parents: []string{"h", "b", "x"}}, "h", "b", false}, + {"no parent information", Commit{SHA: "m"}, "h", "b", false}, + {"unknown base", Commit{SHA: "m", Parents: []string{"h", ""}}, "h", "", false}, + {"unknown requested head", Commit{SHA: "m", Parents: []string{"", "b"}}, "", "b", false}, + {"empty head SHA", Commit{Parents: []string{"h", "b"}}, "h", "b", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsHygieneMerge(tt.head, tt.requested, tt.base); got != tt.want { + t.Fatalf("IsHygieneMerge(%+v, %q, %q) = %v, want %v", tt.head, tt.requested, tt.base, got, tt.want) + } + }) + } +} From 640dd3218c4db1baad3097bd992bddbc05b9fcf7 Mon Sep 17 00:00:00 2001 From: Karibhou Date: Tue, 27 Oct 2026 10:08:23 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=90=9B=20fix(holdguard):=20prove=20th?= =?UTF-8?q?e=20hygiene=20merge's=20base=20parent=20against=20the=20live=20?= =?UTF-8?q?base?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #14535: the second-parent check compared against pr.BaseSHA, which is empty for every held PR on the polling path (fetchPRsWithDetails never set it) and, when set, is the enumeration-time tip rather than the tip update-branch actually merges. Either way the hive's own merge failed the proof and read as drift at lift, regressing #10437. - Populate BaseSHA for held PRs and refresh it from the enrichPRCI detail. - IsHygieneMerge keeps the strict first-parent proof but takes an onBase membership predicate for the second parent: BaseSHA is the fast path, otherwise Client.CommitOnBranch asks GitHub's compare whether the base branch contains the merged parent. Errors fail closed. - Document that BaseSHA now feeds the hold guard (fail-closed). - Test the base-moved and BaseSHA-unknown cases. Closes #14534 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Karibhou --- ...security-hold-guard-hygiene-merge-proof.md | 2 +- src/cmd/hive/hold_guard_mergeability_test.go | 50 ++++++++++++++++++- src/cmd/hive/hold_guard_test.go | 15 ++++++ src/cmd/hive/main_helpers.go | 28 +++++++++-- src/pkg/github/client.go | 28 ++++++++++- src/pkg/holdguard/holdguard.go | 20 +++++--- src/pkg/holdguard/hygiene_merge_test.go | 31 ++++++------ 7 files changed, 145 insertions(+), 29 deletions(-) diff --git a/changelog.d/security-hold-guard-hygiene-merge-proof.md b/changelog.d/security-hold-guard-hygiene-merge-proof.md index adcdc756d0..7ba0d9f4e6 100644 --- a/changelog.d/security-hold-guard-hygiene-merge-proof.md +++ b/changelog.d/security-hold-guard-hygiene-merge-proof.md @@ -1 +1 @@ -- The hold guard only advances a held PR's reviewed baseline after a merge-from-base when the new head is provably that merge (parents are the updated head and the base), so a contributor push racing the update can no longer be adopted as reviewed hygiene ([#14534](https://github.com/hivecommons/hive/issues/14534)). +- The hold guard only advances a held PR's reviewed baseline after a merge-from-base when the new head is a two-parent merge of a base-branch commit into the head the hive asked to update, so a contributor push racing the update is no longer adopted as reviewed hygiene; the base parent is matched against the live base branch, so a base that moved before the update does not turn the hive's own merge into drift ([#14534](https://github.com/hivecommons/hive/issues/14534)). diff --git a/src/cmd/hive/hold_guard_mergeability_test.go b/src/cmd/hive/hold_guard_mergeability_test.go index 867bc1b73b..0a93d37755 100644 --- a/src/cmd/hive/hold_guard_mergeability_test.go +++ b/src/cmd/hive/hold_guard_mergeability_test.go @@ -88,7 +88,7 @@ func TestMaintainHeldPRMergeabilityKeepsBaselineWhenNewHeadIsNotTheHygieneMerge( t.Fatal("test setup: snapshot must record") } - held := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b1", MergeableState: "behind"}) + held := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: "b1", BaseRef: "main", MergeableState: "behind"}) _ = enforceHoldGuard(context.Background(), cfg, client, client, held, discardLogger()) fake.mu.Lock() @@ -108,6 +108,54 @@ func TestMaintainHeldPRMergeabilityKeepsBaselineWhenNewHeadIsNotTheHygieneMerge( } } +// #14534 review: update-branch merges the base tip at call time, which may be +// newer than the enumerated BaseSHA, and the polling enumeration may not carry +// a BaseSHA at all. The hive's own merge must still advance the baseline when +// the live base branch contains the merged parent. +func TestMaintainHeldPRMergeabilityAdvancesWhenMergedBaseIsNewerThanEnumerated(t *testing.T) { + tests := []struct { + name string + baseSHA string + }{ + {"base moved after enumeration", "b1"}, + {"base SHA not enumerated", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := newTestHoldGuardStore(t) + fake := &holdGuardServer{ + commits: map[int][]map[string]any{ + 7: {ghCommit("c1", "strategist-bot", "docs: plan"), ghMergeCommit("c2", "hive-bot", "Merge branch 'main' into feature", "c1", "b2")}, + }, + baseContains: map[string]bool{"b1": true, "b2": true}, + } + client := newHoldGuardClient(t, fake) + cfg := escalationTestConfig() + if !store.Snapshot("acme/widgets", 7, "c1", []holdguard.Commit{{SHA: "c1", Author: "strategist-bot"}}) { + t.Fatal("test setup: snapshot must record") + } + + held := heldPRsActionable(github.PullRequest{Repo: "widgets", Number: 7, HeadSHA: "c1", BaseSHA: tt.baseSHA, BaseRef: "main", MergeableState: "behind"}) + if got := enforceHoldGuard(context.Background(), cfg, client, client, held, discardLogger()); len(got) != 0 { + t.Fatalf("keeping a held PR mergeable must not flag it for re-review, got %v", got) + } + rec, ok := store.Recorded("acme/widgets", 7) + if !ok || rec.HeadSHA != "c2" { + t.Fatalf("baseline = %+v ok=%v, want it advanced to the hive's merge c2", rec, ok) + } + fake.mu.Lock() + compares := fake.compareCalls + fake.mu.Unlock() + if compares != 1 { + t.Fatalf("compare calls = %d, want 1 live base-membership check", compares) + } + if len(fake.comments) != 0 { + t.Fatalf("a successful hygiene push must not comment, saw %v", fake.comments) + } + }) + } +} + func TestMaintainHeldPRMergeabilityNotesUnresolvableConflictOnce(t *testing.T) { store := newTestHoldGuardStore(t) fake := &holdGuardServer{ diff --git a/src/cmd/hive/hold_guard_test.go b/src/cmd/hive/hold_guard_test.go index 18dd48baba..921ed9bb7f 100644 --- a/src/cmd/hive/hold_guard_test.go +++ b/src/cmd/hive/hold_guard_test.go @@ -64,6 +64,12 @@ type holdGuardServer struct { // (#10437: keeping a held PR mergeable). updateBranchCalls map[int]int failUpdateBranch bool + + // baseContains lists the SHAs the fake base branch contains; the compare + // endpoint reports them "behind" the base and anything else "diverged" + // (#14534: proving the merged parent of a hygiene merge is a base commit). + baseContains map[string]bool + compareCalls int } func (s *holdGuardServer) handler(t *testing.T) http.Handler { @@ -72,6 +78,15 @@ func (s *holdGuardServer) handler(t *testing.T) http.Handler { s.mu.Lock() defer s.mu.Unlock() switch { + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/compare/"): + s.compareCalls++ + spec := r.URL.Path[strings.Index(r.URL.Path, "/compare/")+len("/compare/"):] + _, head, _ := strings.Cut(spec, "...") + status := "diverged" + if s.baseContains[head] { + status = "behind" + } + _ = json.NewEncoder(w).Encode(map[string]any{"status": status}) case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/commits"): if s.failCommits { http.Error(w, `{"message":"boom"}`, http.StatusInternalServerError) diff --git a/src/cmd/hive/main_helpers.go b/src/cmd/hive/main_helpers.go index 4a352cafca..1cf9bacfd7 100644 --- a/src/cmd/hive/main_helpers.go +++ b/src/cmd/hive/main_helpers.go @@ -247,8 +247,10 @@ func enforceHoldGuard( // 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. The advance requires proof that the new head IS that merge -// (holdguard.IsHygieneMerge: parents are the requested head and the base); a -// head that moved any other way stays unadopted and reads as drift. On +// (holdguard.IsHygieneMerge: first parent is the requested head, second is a +// commit on the base branch — the enumerated BaseSHA, or one the live base +// contains when the base moved before the update ran); a head that moved any +// other way stays unadopted and reads 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 @@ -314,14 +316,32 @@ func maintainHeldPRMergeability( "repo", repo, "pr", pr.Number) continue } - if !holdguard.IsHygieneMerge(commits[len(commits)-1], pr.HeadSHA, pr.BaseSHA) { + onBase := func(sha string) bool { + if sha == pr.BaseSHA { + return true + } + // update-branch merges the base tip at call time, which may be + // newer than the enumerated BaseSHA (or BaseSHA may be unknown): + // ask GitHub whether the base branch contains the merged parent. + if pr.BaseRef == "" { + return false + } + on, err := ghClient.CommitOnBranch(ctx, pr.Repo, pr.BaseRef, sha) + if err != nil { + logger.Warn("hold guard: could not confirm merged parent is on the base branch", + "repo", repo, "pr", pr.Number, "base_ref", pr.BaseRef, "parent", sha, "error", err) + return false + } + return on + } + if !holdguard.IsHygieneMerge(commits[len(commits)-1], pr.HeadSHA, onBase) { // The head moved, but not provably to the hive's own merge of the // base into the head it asked to update: a contributor push may // have landed between the 202 and the refetch. Advancing would // adopt unreviewed commits as the baseline, so leave it pinned and // let the lift check read the move as drift (hivecommons/hive#14534). logger.Warn("hold guard: new head after keeping held PR mergeable is not a merge of the base into the held head; baseline left pinned", - "repo", repo, "pr", pr.Number, "requested_head", pr.HeadSHA, "base", pr.BaseSHA, "new_head", newHead) + "repo", repo, "pr", pr.Number, "requested_head", pr.HeadSHA, "base_ref", pr.BaseRef, "base", pr.BaseSHA, "new_head", newHead) continue } store.AdvanceHygiene(repo, pr.Number, newHead, commits) diff --git a/src/pkg/github/client.go b/src/pkg/github/client.go index 7bc95de54b..5d6b08db8f 100644 --- a/src/pkg/github/client.go +++ b/src/pkg/github/client.go @@ -932,8 +932,11 @@ type PullRequest struct { // true merge-base of an ancestry walk. For an up-to-date PR the two are // the same commit, and for a stale one the base tip is the more useful // tree to read anyway (it is what the change will actually land on). It is - // an ADVISORY grounding anchor, never a merge-safety input — nothing gates - // a merge on this field. + // an advisory grounding anchor for reviews; nothing gates a merge on it. + // The hold guard uses it only as a fast path when proving a post-update + // head is its own merge-from-base (holdguard.IsHygieneMerge, #14534): a + // stale or empty value falls back to a live base-branch membership check + // and, failing that, keeps the held baseline pinned, so it fails closed. BaseSHA string `json:"base_sha,omitempty"` // ReviewClass is the presentational triage class (fix / refactor-docs / // tests) the queue snapshot is ordered by — see review_priority.go and @@ -2010,6 +2013,7 @@ func (c *Client) fetchPRsWithDetails(ctx context.Context, repo string, clankerBu ScopeContract: scopeContract, ReporterTrustReason: reporterTrustReason, HeadSHA: prHeadSHA(pr), + BaseSHA: pr.GetBase().GetSHA(), HeadRef: headRef, HeadRepo: headRepo, FromFork: fromFork, @@ -2383,6 +2387,9 @@ func (c *Client) enrichPRCI(ctx context.Context, pr *PullRequest) map[string]boo pr.Mergeable = mergeableFromState(full.GetMergeableState(), full.Mergeable) pr.MergeableState = full.GetMergeableState() pr.MaintainerCanModify = full.GetMaintainerCanModify() + if base := full.GetBase().GetSHA(); base != "" { + pr.BaseSHA = base + } } var allCheckRuns []*gh.CheckRun @@ -3780,6 +3787,23 @@ func (c *Client) CompareAheadBy(ctx context.Context, owner, repo, base, head str return cmp.GetAheadBy(), nil } +// CommitOnBranch reports whether sha is reachable from the tip of branch, i.e. +// GitHub's compare of branch...sha is "identical" or "behind". The hold guard +// uses it to confirm the second parent of an update-branch merge is a base +// commit even when the base moved after enumeration (#14534). +func (c *Client) CommitOnBranch(ctx context.Context, repo, branch, sha string) (bool, error) { + if c == nil || c.client == nil { + return false, ErrNoGitHubClient + } + owner, repoName := c.splitRepo(repo) + cmp, _, err := c.client.Repositories.CompareCommits(WithRESTCaller(ctx, "hive:hold_guard_base_parent"), owner, repoName, branch, sha, &gh.ListOptions{PerPage: 1}) + if err != nil { + return false, fmt.Errorf("comparing %s %s...%s: %w", repo, branch, sha, err) + } + st := cmp.GetStatus() + return st == "identical" || st == "behind", nil +} + // CommitMessage returns the first line of the commit message for the given SHA. func (c *Client) CommitMessage(ctx context.Context, owner, repo, sha string) (string, error) { commit, _, err := c.client.Repositories.GetCommit(ctx, owner, repo, sha, nil) diff --git a/src/pkg/holdguard/holdguard.go b/src/pkg/holdguard/holdguard.go index 70323a5879..94ee8bac03 100644 --- a/src/pkg/holdguard/holdguard.go +++ b/src/pkg/holdguard/holdguard.go @@ -284,15 +284,21 @@ func (s *Store) ReArm(repo string, number int, headSHA string, commits []Commit) // IsHygieneMerge reports whether head is provably the merge-from-base the hive // asked for: a two-parent commit whose first parent is requestedHead (the head -// the hive updated) and whose second parent is baseSHA (the base it merged -// in). Anything else — a contributor push landing between the update request -// and the refetch, a merge of some other branch, an unknown head or base — -// is not proven hygiene and must not become the baseline (hivecommons/hive#14534). -func IsHygieneMerge(head Commit, requestedHead, baseSHA string) bool { - if head.SHA == "" || requestedHead == "" || baseSHA == "" || len(head.Parents) != 2 { +// the hive updated) and whose second parent is a commit on the base branch +// (onBase). The second parent is checked for membership rather than equality +// with one cached base SHA because update-branch merges the base tip at call +// time, which may be newer than the tip seen at enumeration. Anything else — a +// contributor push landing between the update request and the refetch, a merge +// of some other branch, an unknown head — is not proven hygiene and must not +// become the baseline (hivecommons/hive#14534). +func IsHygieneMerge(head Commit, requestedHead string, onBase func(sha string) bool) bool { + if head.SHA == "" || requestedHead == "" || onBase == nil || len(head.Parents) != 2 { return false } - return head.Parents[0] == requestedHead && head.Parents[1] == baseSHA + if head.Parents[0] != requestedHead || head.Parents[1] == "" { + return false + } + return onBase(head.Parents[1]) } // AdvanceHygiene moves a held PR's baseline forward to a head the hive itself diff --git a/src/pkg/holdguard/hygiene_merge_test.go b/src/pkg/holdguard/hygiene_merge_test.go index 6c16411ae9..f71b265b60 100644 --- a/src/pkg/holdguard/hygiene_merge_test.go +++ b/src/pkg/holdguard/hygiene_merge_test.go @@ -2,30 +2,33 @@ package holdguard import "testing" -// #14534: only the merge of the base into the requested head is hygiene. +// #14534: only the merge of a base commit into the requested head is hygiene. func TestIsHygieneMerge(t *testing.T) { + onBase := func(sha string) bool { return sha == "b" || sha == "b2" } tests := []struct { name string head Commit requested string - base string + onBase func(string) bool want bool }{ - {"merge of base into requested head", Commit{SHA: "m", Parents: []string{"h", "b"}}, "h", "b", true}, - {"single-parent push on the requested head", Commit{SHA: "m", Parents: []string{"h"}}, "h", "b", false}, - {"merge of a non-base commit", Commit{SHA: "m", Parents: []string{"h", "x"}}, "h", "b", false}, - {"merge into another head", Commit{SHA: "m", Parents: []string{"x", "b"}}, "h", "b", false}, - {"parents swapped", Commit{SHA: "m", Parents: []string{"b", "h"}}, "h", "b", false}, - {"octopus merge", Commit{SHA: "m", Parents: []string{"h", "b", "x"}}, "h", "b", false}, - {"no parent information", Commit{SHA: "m"}, "h", "b", false}, - {"unknown base", Commit{SHA: "m", Parents: []string{"h", ""}}, "h", "", false}, - {"unknown requested head", Commit{SHA: "m", Parents: []string{"", "b"}}, "", "b", false}, - {"empty head SHA", Commit{Parents: []string{"h", "b"}}, "h", "b", false}, + {"merge of base into requested head", Commit{SHA: "m", Parents: []string{"h", "b"}}, "h", onBase, true}, + {"merge of a newer base tip into requested head", Commit{SHA: "m", Parents: []string{"h", "b2"}}, "h", onBase, true}, + {"single-parent push on the requested head", Commit{SHA: "m", Parents: []string{"h"}}, "h", onBase, false}, + {"merge of a non-base commit", Commit{SHA: "m", Parents: []string{"h", "x"}}, "h", onBase, false}, + {"merge into another head", Commit{SHA: "m", Parents: []string{"x", "b"}}, "h", onBase, false}, + {"parents swapped", Commit{SHA: "m", Parents: []string{"b", "h"}}, "h", onBase, false}, + {"octopus merge", Commit{SHA: "m", Parents: []string{"h", "b", "x"}}, "h", onBase, false}, + {"no parent information", Commit{SHA: "m"}, "h", onBase, false}, + {"empty second parent", Commit{SHA: "m", Parents: []string{"h", ""}}, "h", func(string) bool { return true }, false}, + {"unknown requested head", Commit{SHA: "m", Parents: []string{"", "b"}}, "", onBase, false}, + {"empty head SHA", Commit{Parents: []string{"h", "b"}}, "h", onBase, false}, + {"no base membership check", Commit{SHA: "m", Parents: []string{"h", "b"}}, "h", nil, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := IsHygieneMerge(tt.head, tt.requested, tt.base); got != tt.want { - t.Fatalf("IsHygieneMerge(%+v, %q, %q) = %v, want %v", tt.head, tt.requested, tt.base, got, tt.want) + if got := IsHygieneMerge(tt.head, tt.requested, tt.onBase); got != tt.want { + t.Fatalf("IsHygieneMerge(%+v, %q) = %v, want %v", tt.head, tt.requested, got, tt.want) } }) } From 319a90ea6911b760d77c9afc0351f9b4e0677723 Mon Sep 17 00:00:00 2001 From: Karibhou Date: Tue, 27 Oct 2026 10:11:30 +0000 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=93=9D=20docs(scale-envelope):=20re-a?= =?UTF-8?q?nchor=20RankActionableIssues=20citation=20after=20client.go=20s?= =?UTF-8?q?hift?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 Signed-off-by: Karibhou --- src/docs/scale-envelope.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/docs/scale-envelope.md b/src/docs/scale-envelope.md index 7dbad23b04..004f74c68f 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:4637-4651`, descending `AgeMinutes`). + (`RankActionableIssues`, `src/pkg/github/client.go:4662-4676`, 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).