Repository navigation
π fix(adoption): re-sign inherited commits when re-homing a dco-missing contributor PR - #14688
Conversation
β¦ contributor PR Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com> Co-authored-by: clubanderson <407614+clubanderson@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 |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
The cached ownership split spares the DCO decision a commit read, but the new re-sign replay must read each commit's tree, author and message, which the cached counts do not hold. The one listing is genuinely required, so the test now expects exactly one read (decision still uncached-free). Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
hive/awaiting-ci: CI pending β sweep will merge when green. π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Read supersedeAdoptedPR and the new resignAdoptionCommits at this head against the DCO counter in automerge_stage_dco.go. The replay itself looks correct to me: it uses the same signedOffByRE (automerge_stage_dco.go:31) that produced DCOUnsigned, so it re-signs exactly the commits the DCO decision called unsigned; it bails to the contributor head on any merge commit or a listing that does not end at obs.HeadSHA; and tree/author/message are carried per commit so content is unchanged. escalation.Observation.Labels (escalation.go:258) and stagelabels.NoDCOLabel (stagelabels.go:673) exist, and TestDCORehomeBranchCarriesResignedCommits pins the ref to the new tip.
One finding, medium β the superseding PR's text is now out of step with what the code does:
src/pkg/github/automerge/automerge_contributor_adopt.go:349-353βAdoptionPRBodystill writes "This branch is #N's head<obs.HeadSHA>as pushed by @author, unchanged". After this change the branch points at the re-signed tip (automerge_contributor_adopt.go:680-686), so the SHA named in the body is no longer on the branch and "unchanged" is false whenever the replay succeeds.automerge_contributor_adopt.go:372-381β thedcoRehomeblock still says "DCO repair required: β¦ the hive cannot repair them β¦ cherry-pick each original commit withgit cherry-pick -sβ¦ open/keep one superseding PR β¦ then comment on and close #N". That is the manual procedure this PR automates; the fix agent reading that body would redo the re-home (and the original is already closed at:743). Only the fallback case (resignAdoptionCommitsreturned""or errored, logged at:683) still needs it.
tip is local to the CreateRef closure, so the body cannot see it today. Hoisting tip out of the closure and passing a resigned bool (or the tip SHA) into AdoptionPRBody lets the body say "re-signed as <tip>" and emit the manual block only on fallback. The redPRFixKick text in main.go:8740-8743 already describes the fallback repair correctly, so that side is fine.
Nothing else from the correctness, security or style perspectives.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
After a successful re-sign the branch no longer points at the contributor's head, so the body must name the re-signed tip and must not tell the fix agent to redo the manual cherry-pick re-home; the manual block now appears only on fallback (review on #14688). Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
Addressed the review: AdoptionPRBody now names the re-signed tip and only emits the manual DCO-repair block on fallback to the unsigned contributor head; tests cover both paths. hive/awaiting-ci: CI pending β sweep will merge when green. π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
|
βΈοΈ CI is red on the base branch (relative links and anchors resolve); holding until it recovers β this is not a failure in this PR, so no fix attempt is counted and no one needs to act here. |
There was a problem hiding this comment.
Read the tree, not just the diff. This looks correct to me and does what #14666 asked for in its first "Expected" bullet (the hive owns hive/adopt-N, so it re-signs itself).
What I checked (v5 @ c7a75a6):
- Replay preserves content.
resignAdoptionCommits(automerge_contributor_adopt.go:766-809) requires every PR commit to have exactly one parent and the listing's last SHA to equalobs.HeadSHA, then re-creates each commit with the same tree, author and message on a chain rooted atall[0].Parents[0]. The final tree is therefore identical to the contributor head, so the superseding PR's diff is unchanged. Commits that already carrySigned-off-byare left untouched (signedOffByRE,automerge_stage_dco.go:31). - Fallback is the old behaviour. Any error or a non-linear history logs a warning and branches from the unsigned head with
resignedTip == "", andAdoptionPRBodythen still emits the manual "DCO repair required" steps (:378). Nothing regresses on the path that used to work. - Fence unaffected.
carriedFenceGate(automerge_adoption_fence.go:91) reads a reason marker from the PR body, not a head SHA, so a re-signed tip can't trip it. - Branch update path. Both
CreateRefand the 422βUpdateReffallback now point attipandProvenancerecords the pushed SHA, so the effects ledger stays accurate. - Fix kick.
redPRFixKick(main.go:8740) puts the re-sign step first only whenno-dcois ino.Labels; the genericcommit -sline is unchanged for everyone else.redpr_fix_kick_no_ci_wait_test.gocovers it.
One open question, not a defect: the new commits are authored by the contributor and committed/signed-off by the App bot (BotSignedOffByTrailer(c.transport.AppBotLogin())). That matches what the body already told fix agents to do by hand (cherry-pick -s), and the test asserts the trailer, but I did not verify that this repo's probot DCO check accepts a sign-off matching the committer rather than the author. If #14618 is re-adopted after this merges and DCO still shows action_required, that's where to look.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Closes #14666
Why: a
dco-rehomeadoption pointedhive/adopt-<N>at the contributor's head unchanged, so the unsigned commit was inherited and the superseding PR failed DCO too (#14618 superseding #14592). The fix kick for such a PR only cited CI, so nothing told the fixer to re-sign.supersedeAdoptedPR: for a DCO re-home, the contributor's commits are replayed as a linear chain via the Git Data API (same tree, original author and message) with the App'sSigned-off-byadded where missing, and the branch points at that tip. Merge commits or an unexpected head fall back to the old behaviour.redPRFixKick: ano-dcoPR now gets a required first step,git rebase --signoffthengit push --force-with-lease, before the CI repair.Tests:
TestDCORehomeBranchCarriesResignedCommits,TestRedPRFixKickNamesDCORehomeFirstForNoDCO. Not run locally; CI is the verdict.β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 requested_by=@clubanderson