diff --git a/changelog.d/fixed-approval-carry-shifted-hunks.md b/changelog.d/fixed-approval-carry-shifted-hunks.md new file mode 100644 index 0000000000..d3492c9ba0 --- /dev/null +++ b/changelog.d/fixed-approval-carry-shifted-hunks.md @@ -0,0 +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. diff --git a/src/docs/hive-merge.md b/src/docs/hive-merge.md index 1802844852..3461fc4222 100644 --- a/src/docs/hive-merge.md +++ b/src/docs/hive-merge.md @@ -409,7 +409,14 @@ a merge commit authored by the App, committed by GitHub (`web-flow`) or the App and carrying a signature GitHub verified, whose parents are the decided head and a commit on the base branch, and whose three-dot diff against the base is file-for-file (name, status, blob, line counts, patch) identical to -the decided head's. An App login GitHub derived from an unsigned commit's +the decided head's. One difference is allowed: when the base branch also +edited a file the PR modifies, away from the PR's hunks, that file's blob +differs but every hunk (context, added and removed lines, lengths, section +heading) is the same and only its line numbers moved; that still counts as +identical +([projectbluefin/utah-packages#924](https://github.com/projectbluefin/utah-packages/pull/924)). +A file under any `.github/` directory gets no such allowance: a workflow or +action file must read exactly as decided. An App login GitHub derived from an unsigned commit's author email does not count, and a compare that lists GitHub's 300-file cap cannot be verified, so neither carries ([#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: head, or the hive's own audit marker for it; the approver must still be trusted. A contributor commit re-arms the fence even when it leaves the workflow files alone (the existing head-bound rule), as do a rebase, a - base merge pushed by anyone but the App, and a base merge that changed a - file the PR also changes. A typed `/hive decision approve` that + base merge pushed by anyone but the App, a base merge that changed the + PR's own hunks, and a base merge that changed anything in a `.github/` + file the PR also changes. A base merge that edited some other file the PR + changes only away from the PR's hunks (the hunks identical, their line + numbers moved) still carries + ([projectbluefin/utah-packages#924](https://github.com/projectbluefin/utah-packages/pull/924)). A typed `/hive decision approve` that releases the fence while the swarm's `requires_human` verdict asks for a person is also recorded as the answer to that question on the head, as the dashboard's approval is, so the owner of their own PR (whom GitHub diff --git a/src/pkg/github/automerge/automerge_contributor_workflow_approval.go b/src/pkg/github/automerge/automerge_contributor_workflow_approval.go index 6232e3f146..e5a4b61cdd 100644 --- a/src/pkg/github/automerge/automerge_contributor_workflow_approval.go +++ b/src/pkg/github/automerge/automerge_contributor_workflow_approval.go @@ -268,8 +268,12 @@ func (c *Engine) workflowFenceApproval(ctx context.Context, owner, repo string, // waits for a fresh approval every time the base moves (projectbluefin/utah // #1111). The approver must still be a trusted merger. Any other head // change — a contributor commit, even one leaving the workflow files alone; -// a rebase; a base merge by anyone but the App; a base merge that changed a -// file the PR also changes — carries nothing and the fence stays armed. +// a rebase; a base merge by anyone but the App; a base merge that changed +// the PR's own hunks, or changed anything at all in a `.github/` file the PR +// also changes — carries nothing and the fence stays armed. A base merge +// that edited another file the PR changes only where the PR's hunks do not +// reach (the hunks identical, only their line numbers moved) still carries +// (projectbluefin/utah-packages#924). 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) { baseRef := strings.TrimSpace(pr.GetBase().GetRef()) if c.gh == nil || appLogin == "" || baseRef == "" { diff --git a/src/pkg/github/automerge/automerge_contributor_workflow_approval_test.go b/src/pkg/github/automerge/automerge_contributor_workflow_approval_test.go index 0ffc78ef99..4d5ad9fbca 100644 --- a/src/pkg/github/automerge/automerge_contributor_workflow_approval_test.go +++ b/src/pkg/github/automerge/automerge_contributor_workflow_approval_test.go @@ -333,15 +333,51 @@ func TestContributorLaneWorkflowFenceApprovalCarriesOverHiveBaseRefresh(t *testi } hiveBaseMerge := baseMerge(testHiveAppBotLogin, "web-flow", true) approvedReview := []map[string]any{ownerApprovedReview("clubanderson", approvedHead)} + // docFile is a non-workflow file the PR also edits. Main editing the same + // doc above the PR's hunk changes the doc's blob and shifts the hunk's + // line numbers; editing next to the hunk changes its context lines too + // (projectbluefin/utah-packages#924 and #900). + docFile := func(blob, patch string) map[string]any { + return map[string]any{"filename": "docs/skills/upstream-version-bumps.md", "status": "modified", "sha": blob, "additions": 1, "deletions": 1, "changes": 2, "patch": patch} + } + const docPatch = "@@ -145,4 +145,4 @@ Verify a fix\n ctx one\n-old line\n+new line\n ctx two" + withDoc := func(diff []map[string]any, doc map[string]any) []map[string]any { + return append(append([]map[string]any{}, diff...), doc) + } for _, tc := range []struct { - name string - reviews []map[string]any - comments []map[string]any - headCommit map[string]any - currentDiff []map[string]any - wantReason string - wantMarker string + name string + reviews []map[string]any + comments []map[string]any + headCommit map[string]any + approvedDiff []map[string]any + currentDiff []map[string]any + wantReason string + wantMarker string }{ + { + name: "a base merge that also edited another file the PR changes, away from its hunks, carries the approval", + reviews: approvedReview, + headCommit: hiveBaseMerge, + approvedDiff: withDoc(workflowDiff("blobA"), docFile("docOld", docPatch)), + 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")), + wantMarker: "", + }, + { + name: "a base merge that changed the context of the PR's hunk in another file re-arms the fence", + reviews: approvedReview, + headCommit: hiveBaseMerge, + approvedDiff: withDoc(workflowDiff("blobA"), docFile("docOld", docPatch)), + 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")), + wantReason: contributorLaneReasonWorkflowPath, + }, + { + name: "a base merge that edited the workflow file away from the PR's hunks still re-arms the fence", + reviews: approvedReview, + headCommit: hiveBaseMerge, + approvedDiff: workflowDiff("blobA"), + 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"}}, + wantReason: contributorLaneReasonWorkflowPath, + }, { name: "review on the approved head carries over the hive's base merge", reviews: approvedReview, @@ -417,7 +453,11 @@ func TestContributorLaneWorkflowFenceApprovalCarriesOverHiveBaseRefresh(t *testi fx.reviews = tc.reviews fx.prComments = tc.comments fx.headCommit = tc.headCommit - fx.compareFiles = map[string][]map[string]any{approvedHead: workflowDiff("blobA"), "sha7": tc.currentDiff} + approvedDiff := tc.approvedDiff + if approvedDiff == nil { + approvedDiff = workflowDiff("blobA") + } + fx.compareFiles = map[string][]map[string]any{approvedHead: approvedDiff, "sha7": tc.currentDiff} fx.compareMergeBase = map[string]string{approvedHead: "base1", "sha7": baseTip} api := newContributorLaneAPI(t, &fx) defer api.Close() diff --git a/src/pkg/github/pr_decision_base_refresh.go b/src/pkg/github/pr_decision_base_refresh.go index 892c09018f..a38a045c83 100644 --- a/src/pkg/github/pr_decision_base_refresh.go +++ b/src/pkg/github/pr_decision_base_refresh.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "regexp" "sort" "strconv" "strings" @@ -63,8 +64,10 @@ type BaseRefreshChecker struct { // GitHub-verified merge commit created by the App (appBotLogin, see // CommitIsVerifiedAppMerge) whose parents are oldHead // and a commit on baseRef, and whose three-dot diff against baseRef is -// file-for-file identical to oldHead's (a compare at GitHub's 300-file cap -// cannot show that and is false). That is exactly the head the +// file-for-file identical to oldHead's (sameChangedFiles: hunks that only +// moved to other line numbers in a file outside `.github/` still count as +// identical; a compare at GitHub's 300-file cap cannot show any of that and +// is false). That is exactly the head the // require_current_base gate produces when it merges the base into the // branch: the PR's content is unchanged, only its base moved. Any other // 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 // sameChangedFiles reports whether two three-dot diffs touch the same files // the same way: name, status, rename source, resulting blob, line counts // and — when GitHub returned it for both — the patch text itself. +// +// One difference in the resulting blob is tolerated (sameHunksAtShiftedLines): +// the base branch also edited a file the PR modifies, somewhere the PR's +// hunks do not reach, so every hunk is byte-for-byte the same — context +// lines, added and removed lines, hunk lengths — and only its line numbers +// moved. That is still exactly the change the person approved +// (projectbluefin/utah-packages#924: main edited a doc the PR also edits, 15 +// lines above the PR's hunks). A file under `.github/` never gets that +// tolerance: a workflow or action file must read exactly as approved, +// because a base edit elsewhere in it (its triggers, its permissions) can +// change what the approved lines do. func sameChangedFiles(a, b []*gh.CommitFile) bool { if len(a) != len(b) { return false @@ -264,9 +278,15 @@ func sameChangedFiles(a, b []*gh.CommitFile) bool { if x.GetFilename() != y.GetFilename() || x.GetStatus() != y.GetStatus() || x.GetPreviousFilename() != y.GetPreviousFilename() { return false } - if x.GetSHA() != y.GetSHA() || x.GetAdditions() != y.GetAdditions() || x.GetDeletions() != y.GetDeletions() || x.GetChanges() != y.GetChanges() { + if x.GetAdditions() != y.GetAdditions() || x.GetDeletions() != y.GetDeletions() || x.GetChanges() != y.GetChanges() { return false } + if x.GetSHA() != y.GetSHA() { + if !sameHunksAtShiftedLines(x, y) { + return false + } + continue + } if x.Patch != nil && y.Patch != nil && x.GetPatch() != y.GetPatch() { return false } @@ -274,6 +294,72 @@ func sameChangedFiles(a, b []*gh.CommitFile) bool { return true } +// baseDriftHunkHeaderRE reads a unified-diff hunk header: the two start +// lines, the two optional lengths, and the trailing section heading. +var baseDriftHunkHeaderRE = regexp.MustCompile(`^@@ -\d+(,\d+)? \+\d+(,\d+)? @@(.*)$`) + +// sameHunksAtShiftedLines reports whether two entries for the same file, +// whose resulting blobs differ, carry the same hunks moved to other line +// numbers: both are plain modifications of a path outside any `.github/` +// directory, GitHub returned a patch for both, and the patches are equal +// once each hunk header's two start lines are dropped (hunk lengths and +// section headings still have to match). Anything else — a missing or +// empty patch, a header that does not parse, a rename, a `.github/` path — +// is false. +func sameHunksAtShiftedLines(x, y *gh.CommitFile) bool { + if !strings.EqualFold(x.GetStatus(), "modified") || !strings.EqualFold(y.GetStatus(), "modified") { + return false + } + for _, name := range []string{x.GetFilename(), x.GetPreviousFilename(), y.GetFilename(), y.GetPreviousFilename()} { + if underDotGitHub(name) { + return false + } + } + if x.Patch == nil || y.Patch == nil || x.GetPatch() == "" || y.GetPatch() == "" { + return false + } + px, okx := patchWithoutHunkStarts(x.GetPatch()) + py, oky := patchWithoutHunkStarts(y.GetPatch()) + return okx && oky && px == py +} + +// underDotGitHub reports a path inside a `.github/` directory at any depth. +func underDotGitHub(name string) bool { + name = strings.TrimSpace(name) + return strings.HasPrefix(name, ".github/") || strings.Contains(name, "/.github/") +} + +// patchWithoutHunkStarts rewrites each hunk header of a unified-diff patch +// to keep only its lengths and section heading ("@@ -,3 +,5 @@ heading"), +// so two patches of the same hunks at different line numbers read the same. +// false when a header does not parse or there is no hunk at all. +func patchWithoutHunkStarts(patch string) (string, bool) { + lines := strings.Split(patch, "\n") + hunks := 0 + for i, line := range lines { + if !strings.HasPrefix(line, "@@") { + continue + } + m := baseDriftHunkHeaderRE.FindStringSubmatch(line) + if m == nil { + return "", false + } + oldLen, newLen := strings.TrimPrefix(m[1], ","), strings.TrimPrefix(m[2], ",") + if oldLen == "" { + oldLen = "1" + } + if newLen == "" { + newLen = "1" + } + lines[i] = "@@ -," + oldLen + " +," + newLen + " @@" + m[3] + hunks++ + } + if hunks == 0 { + return "", false + } + return strings.Join(lines, "\n"), true +} + // CommitByGitHubLogin reports whether the commit's GitHub-attributed author // or committer is login ([bot] suffix tolerated, case-insensitive). GitHub // resolves those logins from the git author and committer emails, so on its diff --git a/src/pkg/github/pr_decision_base_refresh_test.go b/src/pkg/github/pr_decision_base_refresh_test.go index 074cce341a..f82792904b 100644 --- a/src/pkg/github/pr_decision_base_refresh_test.go +++ b/src/pkg/github/pr_decision_base_refresh_test.go @@ -5,6 +5,7 @@ import ( "errors" "log/slog" "strconv" + "strings" "sync" "testing" @@ -410,3 +411,39 @@ func TestHeadIsBaseRefreshOfFailsClosedAtCompareFileCap(t *testing.T) { } } } + +// A base merge that also edited a file the PR modifies, away from the PR's +// hunks, leaves every hunk byte-for-byte the same at other line numbers: the +// blob differs but the change is still the one approved +// (projectbluefin/utah-packages#924). Anything more — a changed context +// line or hunk length, a missing patch, a rename, any `.github/` file — is +// still a different diff. +func TestSameChangedFilesToleratesShiftedHunksOutsideDotGitHub(t *testing.T) { + 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" + 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" + file := func(name, status, blob, patch string) []*gh.CommitFile { + f := refreshFile(name, blob, patch) + f.Status = gh.Ptr(status) + return []*gh.CommitFile{f} + } + const doc = "docs/skills/upstream-version-bumps.md" + if !sameChangedFiles(file(doc, "modified", "3604514", approved), file(doc, "modified", "4da6d8a", shifted)) { + t.Fatal("the same hunks at shifted line numbers must compare equal outside .github/") + } + noPatch := file(doc, "modified", "4da6d8a", "") + noPatch[0].Patch = nil + for name, tc := range map[string][2][]*gh.CommitFile{ + "a context line changed": {file(doc, "modified", "3604514", approved), file(doc, "modified", "4da6d8a", strings.Replace(shifted, " ctx\n@@", " main's line\n@@", 1))}, + "a hunk length changed": {file(doc, "modified", "3604514", approved), file(doc, "modified", "4da6d8a", strings.Replace(shifted, "+160,82", "+160,83", 1))}, + "a section heading moved": {file(doc, "modified", "3604514", approved), file(doc, "modified", "4da6d8a", strings.Replace(shifted, "way as libcdio.", "way as libcue.", 1))}, + "the patch was omitted": {file(doc, "modified", "3604514", approved), noPatch}, + "the file was renamed": {file(doc, "renamed", "3604514", approved), file(doc, "renamed", "4da6d8a", shifted)}, + "a workflow file": {file(".github/workflows/bump.yml", "modified", "a0bbe66", approved), file(".github/workflows/bump.yml", "modified", "39b9f1e", shifted)}, + "a nested action file": {file("tools/.github/actions/x/action.yml", "modified", "a0bbe66", approved), file("tools/.github/actions/x/action.yml", "modified", "39b9f1e", shifted)}, + "identical patch text": {file(".github/workflows/bump.yml", "modified", "a0bbe66", approved), file(".github/workflows/bump.yml", "modified", "39b9f1e", approved)}, + } { + if sameChangedFiles(tc[0], tc[1]) { + t.Errorf("%s: a different blob must not compare equal here", name) + } + } +}