Skip to content

Commit dfce7ef

Browse files
Danatharclaudehivecommons-hive[bot]
authored
🐛 fix(automerge): an approval carries over a base merge that only moved the PR's hunks (#17374)
The workflow-path fence (and the operator-decision carry, the review-head binding and the base-preparation proof, which share the same check) carried an approval across the hive's own merge of the base branch only when every file in the PR's three-dot diff kept the same resulting blob. When main had also edited a file the PR changes, away from the PR's lines, the blob changed even though every hunk was byte-for-byte the same at shifted line numbers, so the fence re-armed and the PR sat at stage/085 until someone approved the identical change again (projectbluefin/utah-packages#924: hanthor approved 1417928, the hive merged main in as 11e168e, and main had edited docs/skills/upstream-version-bumps.md 15 lines above the PR's hunk). sameChangedFiles now accepts a different blob for a plain modification outside any .github/ directory when GitHub returned a patch for both sides and the patches are equal once each hunk header's two start lines are dropped (context, added and removed lines, hunk lengths and section headings must all match). Workflow and action files still have to read exactly as approved, and a merge that changed the PR's hunks or their context still needs a new approval. Claude-Session: https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1 Signed-off-by: Danathar <Danathar@users.noreply.github.com> Co-authored-by: Danathar <Danathar@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: hivecommons-hive[bot] <280983584+hivecommons-hive[bot]@users.noreply.github.com>
1 parent 5bfc094 commit dfce7ef

6 files changed

Lines changed: 195 additions & 16 deletions
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- An approval or recorded decision now carries over the hive's own merge of the base branch when the base also edited a file the PR changes, as long as the edit is away from the PR's hunks: every hunk is still byte-for-byte what was approved and only its line numbers moved, so the workflow-path fence no longer re-arms and the PR no longer parks at `stage/085-needs-decision` waiting for a fresh approval of the same change (projectbluefin/utah-packages#924). Files under `.github/` still have to read exactly as approved, and a base merge that touched the PR's own hunks or their context lines still needs a new approval.

‎src/docs/hive-merge.md‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,14 @@ a merge commit authored by the App, committed by GitHub (`web-flow`) or the
409409
App and carrying a signature GitHub verified, whose parents are the decided
410410
head and a commit on the base branch, and whose three-dot diff against the
411411
base is file-for-file (name, status, blob, line counts, patch) identical to
412-
the decided head's. An App login GitHub derived from an unsigned commit's
412+
the decided head's. One difference is allowed: when the base branch also
413+
edited a file the PR modifies, away from the PR's hunks, that file's blob
414+
differs but every hunk (context, added and removed lines, lengths, section
415+
heading) is the same and only its line numbers moved; that still counts as
416+
identical
417+
([projectbluefin/utah-packages#924](https://github.com/projectbluefin/utah-packages/pull/924)).
418+
A file under any `.github/` directory gets no such allowance: a workflow or
419+
action file must read exactly as decided. An App login GitHub derived from an unsigned commit's
413420
author email does not count, and a compare that lists GitHub's 300-file cap
414421
cannot be verified, so neither carries
415422
([#16860](https://github.com/hivecommons/hive/issues/16860)). Up to five such refreshes may stack before any gate reads
@@ -1040,8 +1047,12 @@ A PR merges through this tier only when all of the following hold:
10401047
head, or the hive's own audit marker for it; the approver must still be
10411048
trusted. A contributor commit re-arms the fence even when it leaves the
10421049
workflow files alone (the existing head-bound rule), as do a rebase, a
1043-
base merge pushed by anyone but the App, and a base merge that changed a
1044-
file the PR also changes. A typed `/hive decision approve` that
1050+
base merge pushed by anyone but the App, a base merge that changed the
1051+
PR's own hunks, and a base merge that changed anything in a `.github/`
1052+
file the PR also changes. A base merge that edited some other file the PR
1053+
changes only away from the PR's hunks (the hunks identical, their line
1054+
numbers moved) still carries
1055+
([projectbluefin/utah-packages#924](https://github.com/projectbluefin/utah-packages/pull/924)). A typed `/hive decision approve` that
10451056
releases the fence while the swarm's `requires_human` verdict asks for a
10461057
person is also recorded as the answer to that question on the head, as
10471058
the dashboard's approval is, so the owner of their own PR (whom GitHub

‎src/pkg/github/automerge/automerge_contributor_workflow_approval.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,8 +268,12 @@ func (c *Engine) workflowFenceApproval(ctx context.Context, owner, repo string,
268268
// waits for a fresh approval every time the base moves (projectbluefin/utah
269269
// #1111). The approver must still be a trusted merger. Any other head
270270
// change — a contributor commit, even one leaving the workflow files alone;
271-
// a rebase; a base merge by anyone but the App; a base merge that changed a
272-
// file the PR also changes — carries nothing and the fence stays armed.
271+
// a rebase; a base merge by anyone but the App; a base merge that changed
272+
// the PR's own hunks, or changed anything at all in a `.github/` file the PR
273+
// also changes — carries nothing and the fence stays armed. A base merge
274+
// that edited another file the PR changes only where the PR's hunks do not
275+
// reach (the hunks identical, only their line numbers moved) still carries
276+
// (projectbluefin/utah-packages#924).
273277
func (c *Engine) workflowFenceCarriedApproval(ctx context.Context, owner, repo string, pr *gh.PullRequest, number int, headSHA, appLogin string, reviews []*gh.PullRequestReview, comments []*gh.IssueComment) (workflowApproval, bool) {
274278
baseRef := strings.TrimSpace(pr.GetBase().GetRef())
275279
if c.gh == nil || appLogin == "" || baseRef == "" {

‎src/pkg/github/automerge/automerge_contributor_workflow_approval_test.go‎

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -333,15 +333,51 @@ func TestContributorLaneWorkflowFenceApprovalCarriesOverHiveBaseRefresh(t *testi
333333
}
334334
hiveBaseMerge := baseMerge(testHiveAppBotLogin, "web-flow", true)
335335
approvedReview := []map[string]any{ownerApprovedReview("clubanderson", approvedHead)}
336+
// docFile is a non-workflow file the PR also edits. Main editing the same
337+
// doc above the PR's hunk changes the doc's blob and shifts the hunk's
338+
// line numbers; editing next to the hunk changes its context lines too
339+
// (projectbluefin/utah-packages#924 and #900).
340+
docFile := func(blob, patch string) map[string]any {
341+
return map[string]any{"filename": "docs/skills/upstream-version-bumps.md", "status": "modified", "sha": blob, "additions": 1, "deletions": 1, "changes": 2, "patch": patch}
342+
}
343+
const docPatch = "@@ -145,4 +145,4 @@ Verify a fix\n ctx one\n-old line\n+new line\n ctx two"
344+
withDoc := func(diff []map[string]any, doc map[string]any) []map[string]any {
345+
return append(append([]map[string]any{}, diff...), doc)
346+
}
336347
for _, tc := range []struct {
337-
name string
338-
reviews []map[string]any
339-
comments []map[string]any
340-
headCommit map[string]any
341-
currentDiff []map[string]any
342-
wantReason string
343-
wantMarker string
348+
name string
349+
reviews []map[string]any
350+
comments []map[string]any
351+
headCommit map[string]any
352+
approvedDiff []map[string]any
353+
currentDiff []map[string]any
354+
wantReason string
355+
wantMarker string
344356
}{
357+
{
358+
name: "a base merge that also edited another file the PR changes, away from its hunks, carries the approval",
359+
reviews: approvedReview,
360+
headCommit: hiveBaseMerge,
361+
approvedDiff: withDoc(workflowDiff("blobA"), docFile("docOld", docPatch)),
362+
currentDiff: withDoc(workflowDiff("blobA"), docFile("docNew", "@@ -160,4 +160,4 @@ Verify a fix\n ctx one\n-old line\n+new line\n ctx two")),
363+
wantMarker: "<!-- hive:workflow-approval head=sha7 by=clubanderson from=sha6 -->",
364+
},
365+
{
366+
name: "a base merge that changed the context of the PR's hunk in another file re-arms the fence",
367+
reviews: approvedReview,
368+
headCommit: hiveBaseMerge,
369+
approvedDiff: withDoc(workflowDiff("blobA"), docFile("docOld", docPatch)),
370+
currentDiff: withDoc(workflowDiff("blobA"), docFile("docNew", "@@ -160,4 +160,4 @@ Verify a fix\n ctx one\n-old line\n+new line\n main's new line")),
371+
wantReason: contributorLaneReasonWorkflowPath,
372+
},
373+
{
374+
name: "a base merge that edited the workflow file away from the PR's hunks still re-arms the fence",
375+
reviews: approvedReview,
376+
headCommit: hiveBaseMerge,
377+
approvedDiff: workflowDiff("blobA"),
378+
currentDiff: []map[string]any{{"filename": ".github/workflows/ci.yml", "status": "modified", "sha": "blobA2", "additions": 3, "deletions": 1, "changes": 4, "patch": "@@ -9 +9,3 @@\n-old\n+new\n+steps\n+run"}},
379+
wantReason: contributorLaneReasonWorkflowPath,
380+
},
345381
{
346382
name: "review on the approved head carries over the hive's base merge",
347383
reviews: approvedReview,
@@ -417,7 +453,11 @@ func TestContributorLaneWorkflowFenceApprovalCarriesOverHiveBaseRefresh(t *testi
417453
fx.reviews = tc.reviews
418454
fx.prComments = tc.comments
419455
fx.headCommit = tc.headCommit
420-
fx.compareFiles = map[string][]map[string]any{approvedHead: workflowDiff("blobA"), "sha7": tc.currentDiff}
456+
approvedDiff := tc.approvedDiff
457+
if approvedDiff == nil {
458+
approvedDiff = workflowDiff("blobA")
459+
}
460+
fx.compareFiles = map[string][]map[string]any{approvedHead: approvedDiff, "sha7": tc.currentDiff}
421461
fx.compareMergeBase = map[string]string{approvedHead: "base1", "sha7": baseTip}
422462
api := newContributorLaneAPI(t, &fx)
423463
defer api.Close()

‎src/pkg/github/pr_decision_base_refresh.go‎

Lines changed: 89 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"errors"
66
"fmt"
77
"log/slog"
8+
"regexp"
89
"sort"
910
"strconv"
1011
"strings"
@@ -63,8 +64,10 @@ type BaseRefreshChecker struct {
6364
// GitHub-verified merge commit created by the App (appBotLogin, see
6465
// CommitIsVerifiedAppMerge) whose parents are oldHead
6566
// and a commit on baseRef, and whose three-dot diff against baseRef is
66-
// file-for-file identical to oldHead's (a compare at GitHub's 300-file cap
67-
// cannot show that and is false). That is exactly the head the
67+
// file-for-file identical to oldHead's (sameChangedFiles: hunks that only
68+
// moved to other line numbers in a file outside `.github/` still count as
69+
// identical; a compare at GitHub's 300-file cap cannot show any of that and
70+
// is false). That is exactly the head the
6871
// require_current_base gate produces when it merges the base into the
6972
// branch: the PR's content is unchanged, only its base moved. Any other
7073
// head change — a content commit (even one that also merged the base), a
@@ -241,6 +244,17 @@ func compareAllFiles(ctx context.Context, api BaseRefreshRepos, owner, repo, bas
241244
// sameChangedFiles reports whether two three-dot diffs touch the same files
242245
// the same way: name, status, rename source, resulting blob, line counts
243246
// and — when GitHub returned it for both — the patch text itself.
247+
//
248+
// One difference in the resulting blob is tolerated (sameHunksAtShiftedLines):
249+
// the base branch also edited a file the PR modifies, somewhere the PR's
250+
// hunks do not reach, so every hunk is byte-for-byte the same — context
251+
// lines, added and removed lines, hunk lengths — and only its line numbers
252+
// moved. That is still exactly the change the person approved
253+
// (projectbluefin/utah-packages#924: main edited a doc the PR also edits, 15
254+
// lines above the PR's hunks). A file under `.github/` never gets that
255+
// tolerance: a workflow or action file must read exactly as approved,
256+
// because a base edit elsewhere in it (its triggers, its permissions) can
257+
// change what the approved lines do.
244258
func sameChangedFiles(a, b []*gh.CommitFile) bool {
245259
if len(a) != len(b) {
246260
return false
@@ -264,16 +278,88 @@ func sameChangedFiles(a, b []*gh.CommitFile) bool {
264278
if x.GetFilename() != y.GetFilename() || x.GetStatus() != y.GetStatus() || x.GetPreviousFilename() != y.GetPreviousFilename() {
265279
return false
266280
}
267-
if x.GetSHA() != y.GetSHA() || x.GetAdditions() != y.GetAdditions() || x.GetDeletions() != y.GetDeletions() || x.GetChanges() != y.GetChanges() {
281+
if x.GetAdditions() != y.GetAdditions() || x.GetDeletions() != y.GetDeletions() || x.GetChanges() != y.GetChanges() {
268282
return false
269283
}
284+
if x.GetSHA() != y.GetSHA() {
285+
if !sameHunksAtShiftedLines(x, y) {
286+
return false
287+
}
288+
continue
289+
}
270290
if x.Patch != nil && y.Patch != nil && x.GetPatch() != y.GetPatch() {
271291
return false
272292
}
273293
}
274294
return true
275295
}
276296

297+
// baseDriftHunkHeaderRE reads a unified-diff hunk header: the two start
298+
// lines, the two optional lengths, and the trailing section heading.
299+
var baseDriftHunkHeaderRE = regexp.MustCompile(`^@@ -\d+(,\d+)? \+\d+(,\d+)? @@(.*)$`)
300+
301+
// sameHunksAtShiftedLines reports whether two entries for the same file,
302+
// whose resulting blobs differ, carry the same hunks moved to other line
303+
// numbers: both are plain modifications of a path outside any `.github/`
304+
// directory, GitHub returned a patch for both, and the patches are equal
305+
// once each hunk header's two start lines are dropped (hunk lengths and
306+
// section headings still have to match). Anything else — a missing or
307+
// empty patch, a header that does not parse, a rename, a `.github/` path —
308+
// is false.
309+
func sameHunksAtShiftedLines(x, y *gh.CommitFile) bool {
310+
if !strings.EqualFold(x.GetStatus(), "modified") || !strings.EqualFold(y.GetStatus(), "modified") {
311+
return false
312+
}
313+
for _, name := range []string{x.GetFilename(), x.GetPreviousFilename(), y.GetFilename(), y.GetPreviousFilename()} {
314+
if underDotGitHub(name) {
315+
return false
316+
}
317+
}
318+
if x.Patch == nil || y.Patch == nil || x.GetPatch() == "" || y.GetPatch() == "" {
319+
return false
320+
}
321+
px, okx := patchWithoutHunkStarts(x.GetPatch())
322+
py, oky := patchWithoutHunkStarts(y.GetPatch())
323+
return okx && oky && px == py
324+
}
325+
326+
// underDotGitHub reports a path inside a `.github/` directory at any depth.
327+
func underDotGitHub(name string) bool {
328+
name = strings.TrimSpace(name)
329+
return strings.HasPrefix(name, ".github/") || strings.Contains(name, "/.github/")
330+
}
331+
332+
// patchWithoutHunkStarts rewrites each hunk header of a unified-diff patch
333+
// to keep only its lengths and section heading ("@@ -,3 +,5 @@ heading"),
334+
// so two patches of the same hunks at different line numbers read the same.
335+
// false when a header does not parse or there is no hunk at all.
336+
func patchWithoutHunkStarts(patch string) (string, bool) {
337+
lines := strings.Split(patch, "\n")
338+
hunks := 0
339+
for i, line := range lines {
340+
if !strings.HasPrefix(line, "@@") {
341+
continue
342+
}
343+
m := baseDriftHunkHeaderRE.FindStringSubmatch(line)
344+
if m == nil {
345+
return "", false
346+
}
347+
oldLen, newLen := strings.TrimPrefix(m[1], ","), strings.TrimPrefix(m[2], ",")
348+
if oldLen == "" {
349+
oldLen = "1"
350+
}
351+
if newLen == "" {
352+
newLen = "1"
353+
}
354+
lines[i] = "@@ -," + oldLen + " +," + newLen + " @@" + m[3]
355+
hunks++
356+
}
357+
if hunks == 0 {
358+
return "", false
359+
}
360+
return strings.Join(lines, "\n"), true
361+
}
362+
277363
// CommitByGitHubLogin reports whether the commit's GitHub-attributed author
278364
// or committer is login ([bot] suffix tolerated, case-insensitive). GitHub
279365
// resolves those logins from the git author and committer emails, so on its

‎src/pkg/github/pr_decision_base_refresh_test.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"errors"
66
"log/slog"
77
"strconv"
8+
"strings"
89
"sync"
910
"testing"
1011

@@ -410,3 +411,39 @@ func TestHeadIsBaseRefreshOfFailsClosedAtCompareFileCap(t *testing.T) {
410411
}
411412
}
412413
}
414+
415+
// A base merge that also edited a file the PR modifies, away from the PR's
416+
// hunks, leaves every hunk byte-for-byte the same at other line numbers: the
417+
// blob differs but the change is still the one approved
418+
// (projectbluefin/utah-packages#924). Anything more — a changed context
419+
// line or hunk length, a missing patch, a rename, any `.github/` file — is
420+
// still a different diff.
421+
func TestSameChangedFilesToleratesShiftedHunksOutsideDotGitHub(t *testing.T) {
422+
const approved = "@@ -145,32 +145,82 @@ Verify a fix with `rebuild-rpms.yml`\n ctx\n-old\n+new\n ctx\n@@ -207,7 +257,7 @@ way as libcdio.\n ctx\n-a\n+b"
423+
const shifted = "@@ -160,32 +160,82 @@ Verify a fix with `rebuild-rpms.yml`\n ctx\n-old\n+new\n ctx\n@@ -239,7 +289,7 @@ way as libcdio.\n ctx\n-a\n+b"
424+
file := func(name, status, blob, patch string) []*gh.CommitFile {
425+
f := refreshFile(name, blob, patch)
426+
f.Status = gh.Ptr(status)
427+
return []*gh.CommitFile{f}
428+
}
429+
const doc = "docs/skills/upstream-version-bumps.md"
430+
if !sameChangedFiles(file(doc, "modified", "3604514", approved), file(doc, "modified", "4da6d8a", shifted)) {
431+
t.Fatal("the same hunks at shifted line numbers must compare equal outside .github/")
432+
}
433+
noPatch := file(doc, "modified", "4da6d8a", "")
434+
noPatch[0].Patch = nil
435+
for name, tc := range map[string][2][]*gh.CommitFile{
436+
"a context line changed": {file(doc, "modified", "3604514", approved), file(doc, "modified", "4da6d8a", strings.Replace(shifted, " ctx\n@@", " main's line\n@@", 1))},
437+
"a hunk length changed": {file(doc, "modified", "3604514", approved), file(doc, "modified", "4da6d8a", strings.Replace(shifted, "+160,82", "+160,83", 1))},
438+
"a section heading moved": {file(doc, "modified", "3604514", approved), file(doc, "modified", "4da6d8a", strings.Replace(shifted, "way as libcdio.", "way as libcue.", 1))},
439+
"the patch was omitted": {file(doc, "modified", "3604514", approved), noPatch},
440+
"the file was renamed": {file(doc, "renamed", "3604514", approved), file(doc, "renamed", "4da6d8a", shifted)},
441+
"a workflow file": {file(".github/workflows/bump.yml", "modified", "a0bbe66", approved), file(".github/workflows/bump.yml", "modified", "39b9f1e", shifted)},
442+
"a nested action file": {file("tools/.github/actions/x/action.yml", "modified", "a0bbe66", approved), file("tools/.github/actions/x/action.yml", "modified", "39b9f1e", shifted)},
443+
"identical patch text": {file(".github/workflows/bump.yml", "modified", "a0bbe66", approved), file(".github/workflows/bump.yml", "modified", "39b9f1e", approved)},
444+
} {
445+
if sameChangedFiles(tc[0], tc[1]) {
446+
t.Errorf("%s: a different blob must not compare equal here", name)
447+
}
448+
}
449+
}

0 commit comments

Comments
 (0)