Repository navigation
🐛 fix(automerge): an owner's approval carries over the hive's base merge when main only moved the PR's hunks - #17374
Conversation
…ed the PR's hunks 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. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1 Signed-off-by: Danathar <Danathar@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Decision needed: @clubanderson should decide whether the workflow-fence approval may carry over an App base merge when a non-.github file the PR edits was also changed by main outside the PR's hunks (hunks byte-identical, only line numbers moved), or whether the carry must keep requiring identical resulting blobs for every file.
Code read at 3abd102 against v6; no defect found. This is a merge/trust policy loosening, so I am routing rather than judging it alone.
What the code does (verified):
sameChangedFiles(pr_decision_base_refresh.go:289-299) now tolerates a blob mismatch only viasameHunksAtShiftedLines, after name/status/rename and additions/deletions/changes already matched. Thecontinueskips the raw-patch equality, butsameHunksAtShiftedLineshas already required both patches present, non-empty, and equal after normalising hunk headers (:309-324), so nothing is skipped.patchWithoutHunkStarts(:335-359) only rewrites lines prefixed@@; unified-diff body lines start with,+,-or\, so content is never touched. A header that fails the regex, or a patch with no hunks (GitHub omits patches for large files), returnsfalse— conservative..github/at any depth is excluded (underDotGitHub,:329-332), covered by the "nested action file" test case.
The policy question: the carry now accepts a head whose resulting content in a non-.github/ file differs from what the approver saw, as long as the PR's own hunks and their three-line context are unchanged. The extra content came from main, so it was already trusted on the base branch, which is the argument for carrying. The counter-argument is that files outside .github/ (scripts a workflow runs, Dockerfiles, Makefiles) can also change what the approved lines do. The PR body argues for the first reading; the .github/ carve-out is the only line drawn. Whether that line is the right one is a maintainer call, not a code call.
I have not approved or changed anything.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
|
Needs a decision, not a merge:
Head: verdict requires_human @c1eca1a, 44/53 checks green, 9 pending, DCO yes, soak 0s of 10m0s. How to release it:
Clear the cause and the hive re-evaluates on its next pass; the stage moves to Updated 2026-11-09 12:20 UTC. |
|
Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor. Review findings from The code does what it says; I am routing this because it is a deliberate loosening of the approval-carry gate on the contributor workflow-path fence, which is merge/trust policy and not a reviewer's call. What I verified (read against
The question for a maintainer: the fence exists so a person re-approves when a workflow-path PR's head changes. This PR argues that a base merge which moves a non-workflow hunk's line numbers is the same approved change; the counter-argument is that a base edit elsewhere in the same file can change what the approved lines do (the PR already accepts that argument for No defects found. I have not approved or changed anything. Review findings from The implementation does what it says and fails closed on every ambiguous case I could construct; the remaining question is whether the merge-trust gate should be relaxed this way at all, which is a maintainer call. Verified (
Why I am routing rather than confirming This widens what counts as "the same change the person approved" for both the workflow-fence approval carry and the |
There was a problem hiding this comment.
Read sameChangedFiles and the two callers (HeadIsBaseRefreshOf at pr_decision_base_refresh.go:174, BaseMergeParent at :207). The relaxation is well-fenced and looks correct to me:
- The structural guards that make the carry safe are untouched: the new head's merge-base with the base ref must equal the base-side parent (
:167), so anything in the merge not already on the base would show up in the three-dot diff as a changed or extra hunk, andpatchWithoutHunkStartswould then return a different string. sameHunksAtShiftedLines(:309-324) refuses anything that is notmodified, any path under.github/at any depth, and a nil/empty patch (GitHub omits patches for large and binary files), so those fall back to the strict blob equality. Additions/deletions/changes counts are still compared before the SHA branch (:281).- In a unified diff every content line carries a
' ',+,-or\prefix, so a line beginning@@can only be a hunk header; the regex rewrite at:342-353cannot touch content, and an unparseable header or a hunk-less patch isfalse. - Hunk lengths and section headings remain part of the comparison, so a base edit that changes the enclosing heading or hunk extent is still treated as a different diff. The test table covers each of those near-misses.
Author is an OWNERS approver and the PR body states the scope and the .github/ carve-out explicitly, so no open decision from my side.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
Summary
When the hive merges
maininto a PR that edits workflow files, an owner's earlier approval is supposed to carry over as long as the PR's diff is unchanged. The check compared each file's resulting blob, so ifmainhad also edited a file the PR touches (even far from the PR's lines), the blob changed, the approval did not carry, and the PR sat atstage/085-needs-decisionuntil someone approved the exact same change again. This PR lets that case carry when every hunk is byte-for-byte the same and only its line numbers moved. Files under.github/still have to match exactly.What went wrong
/hive decision approvereleased the fence on1417928(audit marker posted 11-07 17:06Z). The only commit after that is the hive's own verified merge ofmain(11e168e, 17:06:16Z). The PR went back to 085 withcontributor-lane:workflow-pathand stayed there until Danathar approved again on 11-09.docs/skills/upstream-version-bumps.mdwent from blob3604514to4da6d8abecausemainedited that doc above the PR's hunks. Its patch is the same except the hunk headers (@@ -145,32 +145,82 @@became@@ -160,32 +160,82 @@).sameChangedFilesrequired equal blobs, soHeadIsBaseRefreshOfsaid no and nothing carried.What changes
sameChangedFiles(the check behindHeadIsBaseRefreshOfandBaseMergeParent) now accepts a different blob for one file when all of these hold:.github/directory;hive-merge.md) and code comments now describe the new rule.Why this is safe
The merge has to be a GitHub-verified App merge whose other parent is the merge-base with the base branch (unchanged). Anything added in that merge that is not on the base would appear in the PR's diff against that merge-base, so it would show up as a changed or extra hunk. The relaxed rule only accepts edits that
mainalready made, away from the approved lines. Workflow and action files keep the strict rule, because a base edit elsewhere in such a file (its triggers or permissions) can change what the approved lines do.Files
src/pkg/github/pr_decision_base_refresh.go:sameHunksAtShiftedLines,patchWithoutHunkStarts,underDotGitHubsrc/pkg/github/automerge/automerge_contributor_workflow_approval.go: comment onlysrc/docs/hive-merge.md,changelog.d/fixed-approval-carry-shifted-hunks.mdTests
TestSameChangedFilesToleratesShiftedHunksOutsideDotGitHub: the feat: add OpenAPI spec and Redoc API docs #924 case compares equal. Eight near-misses still compare different: changed context, changed hunk length, changed section heading, omitted patch, rename, a workflow file, a nested.github/actionsfile, and a workflow file with an identical patch but a different blob.TestContributorLaneWorkflowFenceApprovalCarriesOverHiveBaseRefreshhas three new rows. The fence carries across an App base merge that shifted a doc's hunks. It re-arms when the doc hunk's context changed (the fix(canary): carry a fork's Canary proof across clean merges of main that change no pipeline path projectbluefin/utah-packages#900 shape), and it re-arms when the workflow file itself drifted.Refs #16858
🤖 Generated with Claude Code
https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1