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..7ba0d9f4e6 --- /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 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 431753228a..0a93d37755 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,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 { 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 5d4a562f0e..1cf9bacfd7 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,13 @@ 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: 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 @@ func maintainHeldPRMergeability( "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) 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). 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/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..94ee8bac03 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,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 diff --git a/src/pkg/holdguard/hygiene_merge_test.go b/src/pkg/holdguard/hygiene_merge_test.go new file mode 100644 index 0000000000..f71b265b60 --- /dev/null +++ b/src/pkg/holdguard/hygiene_merge_test.go @@ -0,0 +1,35 @@ +package holdguard + +import "testing" + +// #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 + onBase func(string) bool + want bool + }{ + {"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.onBase); got != tt.want { + t.Fatalf("IsHygieneMerge(%+v, %q) = %v, want %v", tt.head, tt.requested, got, tt.want) + } + }) + } +}