🐛 fix(holdguard): advance the held baseline only on a proven merge-from-base

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 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)).

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,111 @@ 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", BaseRef: "main", 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")
+			}
+		})
+	}
+}
+
+// #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{
@@ -85,9 +190,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 {

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)

src/cmd/hive/main_helpers.go

@@ -1,3 +1,3 @@
 package main
 
 import (
@@ -95,7 +95,7 @@
 		}
 		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,13 @@
 // 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: 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
 // dedupes through Commented.
 //
@@ -311,6 +316,34 @@
 				"repo", repo, "pr", pr.Number)
 			continue
 		}
+		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_ref", pr.BaseRef, "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)

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).

src/pkg/github/client.go

@@ -1,3 +1,3 @@
 package github
 
 import (
@@ -932,8 +932,11 @@
 	// 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 @@
 					ScopeContract:       scopeContract,
 					ReporterTrustReason: reporterTrustReason,
 					HeadSHA:             prHeadSHA(pr),
+					BaseSHA:             pr.GetBase().GetSHA(),
 					HeadRef:             headRef,
 					HeadRepo:            headRepo,
 					FromFork:            fromFork,
@@ -2383,6 +2387,9 @@
 		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 @@
 	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)

src/pkg/github/pr_commits.go

@@ -1,3 +1,3 @@
 package github
 
 import (
@@ -35,6 +35,10 @@
 	// 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,13 +76,18 @@
 		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,
 			Title:       strings.TrimSpace(title),
 			Message:     message,
 			AuthoredAt:  prCommitTime(rc),
 			ParentCount: len(rc.Parents),
+			Parents:     parents,
 		})
 	}
 	return out, nil

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,25 @@ 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 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
+	}
+	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
 // 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