[Karibhou]

Closes #14534

---

🐝 Hive Agent: contributor | SHA: d17fe508b

— hive: backend=claude model=claude-opus-5-5 claude=2.1.311 (Claude Code)

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kellyaa for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

Open PR overlap check

Base branch: v5

Conflicts are checked with every PR merged onto the current tip of v5, so a PR that only needs a rebase is not reported as conflicting with its siblings.

No conflicting open PRs or same-file overlaps were found.

[hivecommons-hive]

Conclusion: the proof is sound against the race the issue describes, but as wired it is stricter than what UpdateBranch actually produces, so on a busy base branch the hive's own hygiene merge will fail the proof and be reported as drift. Fail-closed, so no security regression — but it reintroduces the re-hold noise #10437's AdvanceHygiene was built to remove.

Finding (medium, correctness) — second-parent check compares against a stale base tip

• src/cmd/hive/main_helpers.go:284 still calls the unpinned ghClient.UpdateBranch(ctx, pr.Repo, pr.Number). GitHub's update-branch merges the base ref's current tip at call time.
• src/cmd/hive/main_helpers.go:317 then requires parents[1] == pr.BaseSHA. pr.BaseSHA is pr.GetBase().GetSHA() captured at enumeration (src/pkg/github/client.go:2066), and its own doc comment says it is "the base branch tip as reported at enumeration time … an ADVISORY grounding anchor, never a merge-safety input" (client.go:926-937). This PR makes it exactly that input.
• So whenever anything lands on v5 between the PR list fetch and the update call (several times an hour on this repo), IsHygieneMerge (src/pkg/holdguard/holdguard.go:291) returns false, the baseline stays pinned at the old head, and the next tick's lift check (main_helpers.go:199-227) treats the hive's merge commit as drift: public "branch moved while hold-gated" comment, ReHoldLabel, ReArm, and a human has to lift again. Before this PR that merge advanced the baseline silently.

The first-parent half of the check is the valuable part and is airtight. Two ways to make the second half match reality:

1. Call UpdateBranchExpectedHead(ctx, pr.Repo, pr.Number, pr.HeadSHA, pr.BaseSHA) (already exists, src/pkg/github/pullrequest.go:534, used the same way by pkg/prfollowup/prfollowup.go:701). expected_head_sha makes GitHub refuse the update if the head moved, which closes the #14534 race at the source rather than after the fact.
2. For the second parent, don't demand equality with the enumeration-time tip: refetch the PR after the 202 and accept parents[1] equal to either the enumeration-time or post-update base.sha (the merged tip is always one of the base commits in that window), or confirm it with a compare/ancestry call. Equality with a value documented as advisory will misfire under ordinary base churn.

Minor

• The PR body carries no Closes #14534; only the changelog fragment references the issue, so merging will not close it.

What I checked and found fine: Entry persists only CommitSHAs/authors, so the "never persisted" comment on Commit.Parents is accurate; ListPRCommits parent order follows rc.Parents (git order); the new table tests cover the shapes the function rejects. Verdict recorded as changes_requested on the drift regression only.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Read the diff against the v5 tree (main_helpers.go, pullrequest.go, client.go, holdguard.go) and the callers. The direction is right and fail-safe: an unproven head now stays pinned and surfaces as drift instead of being adopted as the baseline, which is exactly the mitigation #14534 asked for. AdvanceHygiene persists only SHA/author sets (holdguard.go:320), so the "Parents never persisted" note holds. Tests cover the parent-shape cases well.

One verified gap worth closing before this lands, and two notes.

Medium — pr.BaseSHA is an enumeration-time snapshot, so a legitimate hive hygiene merge can fail the proof on a busy base.
IsHygieneMerge (src/pkg/holdguard/holdguard.go:291-296) requires Parents[1] == pr.BaseSHA. But BaseSHA is filled from the list payload at enumeration (src/pkg/github/client.go:2064-2066), and its own doc comment says it is "the base branch tip as reported at enumeration time … an ADVISORY grounding anchor, never a merge-safety input" (client.go:931-936). GitHub's update-branch merges the base tip at call time. On v5 the hold loop still calls the unguarded ghClient.UpdateBranch (src/cmd/hive/main_helpers.go:283), so if the base moves between enumeration and the update — routine on v5, which lands several merges an hour — the resulting commit is genuinely the hive's merge-from-base, yet Parents[1] != pr.BaseSHA, the baseline stays at the old head, and at lift time the hive's own upkeep reads as drift (main_helpers.go:193-205: re-review exclusion + drift comment). The next tick's Touch (main_helpers.go:114) plus LiftCoversObservedHead (holdguard.go:506) absorb most of this when a human lifts later, so the cost is a spurious re-review/drift comment, not a security regression — but it is the same false-drift the AdvanceHygiene path exists to prevent (#10437).

Options, cheapest first: (a) use UpdateBranchExpectedHead(ctx, repo, n, pr.HeadSHA, pr.BaseSHA) here (already in the tree, pullrequest.go:534) so the first-parent check is guaranteed, and accept len(Parents)==2 && Parents[0]==requestedHead with the second parent checked only when it can be re-read; (b) re-fetch the PR after the 202 and compare Parents[1] against the fresh base.sha; (c) keep the strict check but update the BaseSHA doc comment at client.go:931-936, since it is now a gating input.

Notes

• #14401 (merged to v6) rewrote this same block (SettleUpdateRequest, UpdateBranchExpectedHead); the v5→v6 forward-merge of this PR will conflict in maintainHeldPRMergeability and the AdvanceHygiene doc comment. Not a reason to hold, just expect a hand-resolve.
• The PR body is only the attribution footer; the changelog fragment carries the rationale, so no mismatch, but a Closes #14534 would let the issue close on merge.

I have not approved or requested changes as a review event; this is a comment.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Correctness — one in-scope finding (reviewed at d17fe50).

pr.BaseSHA is the enumeration-time base tip, not the tip update-branch actually merges — a base that moved in between makes the hive's own hygiene merge read as drift.

src/cmd/hive/main_helpers.go:317 requires head.Parents[1] == pr.BaseSHA (src/pkg/holdguard/holdguard.go:294). BaseSHA is pr.GetBase().GetSHA() from the PR list payload (src/pkg/github/client.go:2066), and the field's own doc says it is "the base branch tip as reported at enumeration time … an ADVISORY grounding anchor, never a merge-safety input" (client.go:931-936). GitHub's update-branch merges the live base tip. In a repo where the hive itself auto-merges to the base, the base routinely moves between enumeration and UpdateBranch (main_helpers.go:284), so the resulting merge commit has Parents[1] == <newer base tip> ≠ pr.BaseSHA, IsHygieneMerge returns false, and the baseline stays pinned at the old head.

Consequence (regression of #10437): at lift time Diff sees rec.HeadSHA != pr.HeadSHA (holdguard.go:419), the PR is put in reReview, gets a drift comment and the re-hold label, and is re-armed (main_helpers.go:206-240) — all for a push the hive made. The new test only covers the case where the cached base equals the merged base (hold_guard_mergeability_test.go:34, b1/b1); there is no case for "base advanced again before the update".

Suggested fix: keep the first-parent proof (that is what defeats the contributor race) but resolve the second parent against the live base rather than the cached SHA — e.g. when Parents[1] != pr.BaseSHA, fetch the base branch head (Repositories.GetBranch(owner, repo, pr.BaseRef)) after the refetch and accept a match; or confirm Parents[1] is contained in the base via CompareCommits(base, Parents[1]).BehindBy == 0 and AheadBy == 0. Independently, consider requesting the update with UpdateBranchExpectedHead(ctx, pr.Repo, pr.Number, pr.HeadSHA, pr.BaseSHA) (src/pkg/github/pullrequest.go:534) so GitHub refuses the update outright when the head already moved, instead of only detecting it post hoc.

-		if !holdguard.IsHygieneMerge(commits[len(commits)-1], pr.HeadSHA, pr.BaseSHA) {
+		head := commits[len(commits)-1]
+		baseTip := pr.BaseSHA
+		if len(head.Parents) == 2 && head.Parents[1] != baseTip {
+			// The cached base may predate the update; compare against the live tip.
+			if live, err := ghClient.BranchHeadSHA(ctx, pr.Repo, pr.BaseRef); err == nil && live != "" {
+				baseTip = live
+			}
+		}
+		if !holdguard.IsHygieneMerge(head, pr.HeadSHA, baseTip) {

(plus a holdGuardServer case where the served merge's second parent is b2 while the held PR carries BaseSHA: "b1" and the fake base ref resolves to b2, asserting the baseline advances.)

— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Hive filed review findings that do not block this PR as backlog items:

• Review backlog (correctness): Race closed only post hoc; UpdateBranchExpectedHead already exists #14537 — Review backlog (correctness): Race closed only post hoc; UpdateBranchExpectedHead already exists (Review backlog (correctness): Race closed only post hoc; UpdateBranchExpectedHead already exists #14537)

Cap: 10 backlog item(s) per PR per day.

[hivecommons-hive]

Reviewed (intent-alignment) — diff matches #14534's requested mitigation (require parents (requestedHead, base) before AdvanceHygiene); no scope creep. One advisory:

src/pkg/github/client.go:926-937 documents PullRequest.BaseSHA as "an ADVISORY grounding anchor, never a merge-safety input — nothing gates a merge on this field." This PR makes it the gating input for holdguard.IsHygieneMerge (src/cmd/hive/main_helpers.go:317). The direction is fail-closed (a base tip that moved between enumeration and UpdateBranch leaves the baseline pinned and the hive's own merge later reads as drift), so this is acceptable, but the field comment should say so:

	// This is the base branch tip as reported at enumeration time, not the
	// 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 for reviews; nothing gates a merge on it.
	// The hold guard also compares it against the second parent of a
	// post-update head (holdguard.IsHygieneMerge, #14534): a mismatch only
	// keeps the held baseline pinned, so a stale value fails closed.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Security review (read at head d17fe508b)

1. The proof can never pass on the polling path — BaseSHA is empty for every held PR, so AdvanceHygiene is now dead in production.
maintainHeldPRMergeability iterates actionable.PRs.Held (src/cmd/hive/main_helpers.go:132) and calls holdguard.IsHygieneMerge(..., pr.HeadSHA, pr.BaseSHA) (main_helpers.go:317), which returns false whenever baseSHA == "" (src/pkg/holdguard/holdguard.go:290). But the heldPRs literal built by fetchPRsWithDetails (src/pkg/github/client.go:1993–2021) never sets BaseSHA — only the actionable list does (client.go:2104) — and enrichPRCI fills MergeableState/MaintainerCanModify from the per-PR GET without touching BaseSHA (client.go:2383–2386). Only the webhook patch path sets it (webhook_item_patch.go:171–173). Net effect on the polling enumeration: every hive merge-from-base on a held PR trips the new continue at main_helpers.go:323, the baseline stays pinned, and the hive's own hygiene push is reported as drift at lift (exactly the #10437 regression AdvanceHygiene exists to prevent). The new tests pass only because the fixtures hand-set BaseSHA: "b1" (hold_guard_mergeability_test.go:34,88,147). Fail-closed, so not a vulnerability — but the PR does not deliver its contract.

Fix — populate the field where the held PR is built and refresh it from the detail fetch:

--- a/src/pkg/github/client.go
+++ b/src/pkg/github/client.go
@@ heldPRs = append(heldPRs, PullRequest{
 					HeadSHA:             prHeadSHA(pr),
+					BaseSHA:             pr.GetBase().GetSHA(),
 					HeadRef:             headRef,
@@ func (c *Client) enrichPRCI(
 	if full != nil {
 		pr.Mergeable = mergeableFromState(full.GetMergeableState(), full.Mergeable)
 		pr.MergeableState = full.GetMergeableState()
 		pr.MaintainerCanModify = full.GetMaintainerCanModify()
+		if base := full.GetBase().GetSHA(); base != "" {
+			pr.BaseSHA = base
+		}
 	}

Also update the BaseSHA doc comment at client.go:935–937 ("an ADVISORY grounding anchor, never a merge-safety input") — this PR makes it a safety input for the hold guard.

2. (advisory, low) Parent-pair equality is not proof of a hive merge. IsHygieneMerge (holdguard.go:288–293) accepts any two-parent commit with parents (requestedHead, base); git lets a contributor craft such a merge with an arbitrary tree ("evil merge"). The update is issued via the unpinned UpdateBranch (main_helpers.go:284 → pullrequest.go:525, no expected_head_sha), so a crafted merge pushed between the 202 and the refetch is adopted as baseline. The window is narrow and single-shot, so this is low — but the changelog's "provably that merge" overclaims. Cheap hardening: carry rc.GetCommitter().GetLogin()/rc.Commit.GetVerification().GetVerified() in PRCommit and additionally require committer web-flow with a GitHub-verified signature, which a contributor cannot forge.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

@Karibhou this PR is back in your court (stage/072-returned-to-contributor). The hive will not merge it until the head changes.

Review asks:

• the review swarm requested changes

Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path.

Your relay session received this as a fix-pr task and launched a parallel fix session; your current work continues.

[hivecommons-hive]

@Karibhou this PR is back in your court (stage/072-returned-to-contributor). The hive will not merge it until the head changes.

Failing checks:

• relative links and anchors resolve

Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path.

Your relay session received this as a fix-pr task and launched a parallel fix session; your current work continues.

[hivecommons-hive]

Read maintainHeldPRMergeability in full at the PR head plus the new holdguard.IsHygieneMerge and github.CommitOnBranch; also compared against #14536, which touches holdguard.go on v6 for a different issue (#14533, legacy stop records) — no overlap, not a duplicate. This looks correct to me.

• The proof is fail-closed in every branch: IsHygieneMerge (src/pkg/holdguard/holdguard.go:294-302) rejects an empty head, a non-two-parent commit, or a first parent other than the head the hive asked to update; onBase (src/cmd/hive/main_helpers.go:319-337) returns false on empty BaseRef or on a compare error. Every rejection hits continue before AdvanceHygiene, so the baseline stays pinned and the lift check reads the move as drift — which is the behaviour #14534 asks for.
• commits[len(commits)-1] is guarded by the existing len(commits) > 0 / newHead == "" check immediately above (main_helpers.go:306-317), so no empty-slice index.
• CommitOnBranch (src/pkg/github/client.go:3794-3805) maps compare branch...sha status identical/behind to "sha is reachable from the branch tip", which is the right predicate for "second parent is a base commit", and pr.Repo is used for the API call consistent with UpdateBranch/ListPRCommits on the same loop.
• BaseSHA is now populated both at enumeration (client.go:2016) and refreshed in enrichPRCI (client.go:2390-2392), and the field comment is updated to say the hold guard consumes it as a fast path only.

go build ./pkg/holdguard/ ./pkg/github/ ./cmd/hive/ passes at 319a90e. Nothing further from me.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed (security) — no in-scope findings. The change implements the mitigation named in #14534 and fails closed on every ambiguous branch: empty/stale BaseSHA falls back to a live CommitOnBranch check, and any API error or non-membership leaves the baseline pinned (src/cmd/hive/main_helpers.go:319-345; src/pkg/holdguard/holdguard.go:295-302).

Advisory, out of scope for #14534: IsHygieneMerge proves parentage only, not tree content. A contributor who pushes a hand-made two-parent merge (requestedHead, <base commit>) carrying extra edits inside the 202→refetch window would still be adopted as hygiene (holdguard.go:295-302). A cheap extra proof is to also require the merge commit's author be the identity GitHub's update-branch commits as (the hive's token login), which Commit.Author already carries via ListPRCommits (src/pkg/github/pr_commits.go:69-72). Worth a follow-up, not a blocker.

— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed (correctness) — no blocking findings. Read at head 319a90e.

IsHygieneMerge (src/pkg/holdguard/holdguard.go:294-301) and the onBase fast-path/live-compare fallback (src/cmd/hive/main_helpers.go:319-337) implement exactly the mitigation #14534 prescribes; Parents is not persisted (AdvanceHygiene stores only CommitSHAs/Authors, holdguard.go:325), and BaseRef/BaseSHA are populated on the enumeration path (client.go:2016,2021, :2390).

One advisory (low, non-blocking): the fail-closed branch at main_helpers.go:338-347 is never re-proven. If CommitOnBranch returns a transient error (:331-335) for the hive's own merge, the next tick sees the PR as clean, skips UpdateBranch, Touch records the new head as observed, and Diff reads it as drift — so a transient compare failure produces a drift re-review + comment on a human's PR. Acceptable as the issue's stated trade-off; noting it so the symptom is recognisable if it shows up.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Read the diff against a v5 checkout plus the callers; this looks correct to me and does what #14534 prescribes ("require the new head to be a merge commit whose parents are (head, base) before AdvanceHygiene").

What I verified:

• src/pkg/github/pullrequest.go:567 — UpdateBranch calls REST update-branch with no update_method, so GitHub produces a merge commit whose first parent is the PR head and second is the base tip. That is exactly the shape holdguard.IsHygieneMerge (src/pkg/holdguard/holdguard.go:294) demands, so the hive's own hygiene merge still advances the baseline; anything else now falls through to continue at src/cmd/hive/main_helpers.go:337 and reads as drift at lift time. Fails closed on empty SHA / nil onBase / wrong parent count.
• src/pkg/github/client.go:3794 — CommitOnBranch compares branch...sha and accepts identical|behind, which is "sha is an ancestor of the branch tip" — the right membership test for a base that moved between enumeration and the update. ahead/diverged correctly return false.
• pr.BaseRef is populated at client.go:2021/2112, and BaseSHA is now set in both fetchPRsWithDetails and enrichPRCI, so the fast path and the live fallback both have inputs; an empty BaseRef returns false rather than guessing.
• ListPRCommits is oldest-first (pr_commits.go:57), so commits[len(commits)-1] is the new head — same assumption the existing newHead code already relied on.
• New tests cover the three branches (hygiene merge → advance; non-merge head → pinned; merged parent newer than enumerated BaseSHA → one live compare, then advance). go build of the three touched packages is clean here; CI build-and-test and coverage are green at 319a90e.

One note for whoever merges, not a defect: #14401 (merged into v6) rewrote this same maintainHeldPRMergeability body (main_helpers.go hunks 277-395 there), so the v5→v6 forward-merge of this PR will conflict in that function. The two changes are complementary — #14401 gates the update request on expected_head_sha, this one proves the result — so both should survive the resolution.

Optional follow-up, not blocking: on v5 the hold guard still calls plain UpdateBranch; UpdateBranchExpectedHead(pr.HeadSHA, pr.BaseSHA) already exists at pullrequest.go:534 and would stop GitHub from ever stacking the hygiene merge on top of an unreviewed push, making this check a backstop rather than the only line. That is what #14401 did on v6.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Evidence bundle `#14535319a90e` sha256:b3d8c7fa62cdc136135c0fad320a08cfc081ec7ef36af956f3a41b48cdfd29ed — download from the dashboard or `hivectl review evidence #14535`.

[hivecommons-hive]

Merged by hive contributor-lane auto-merge: CI green for ≥30m, reviewer verdict approve, @Karibhou holds the trusted tier in this hive's contributor registry.