Repository navigation
π fix: prevent contributor adoption of maintainer PRs - #14320
Conversation
Check cached repository permissions before adoption and supersession, release existing maintainer adoptions, and explain the refusal once per head. Fail closed when permissions cannot be read. Co-authored-by: Danathar <6772335+Danathar@users.noreply.github.com> Hive-Run: #14301 Hive-Plan: maintainer-adoption-guard Hive-Spec: issue-14301#acceptance-criteria Signed-off-by: arteeh <arteeh@users.noreply.github.com>
β¦ssion read fails
guardAdoption cleared the adoption record on every error, so one 5xx on
GET collaborators/{author}/permission during the per-pass re-check wiped
AdoptedBy for a PR superseded days earlier: settleAdoption then never
closed the original when hive/adopt-N merged (or returned it to 072 when
it closed), and after the 10-minute error cache the PR was eligible for a
second adoption and a second superseding PR. A failed read now blocks the
pass without touching the record; only a confirmed admin/maintain author
releases the adoption.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
β¦en superseders on release The adoption pre-guard in ApplyStageLabels now runs only for non-terminal observations: a maintainer who merged or closed their own adopted PR no longer gets the "will not take over" note on a PR that is forgotten in the same pass. Releasing a maintainer's live supersede adoption now closes a still-open hive superseder with a note before clearing the record, so the release never leaves an untracked duplicate of the maintainer's PR mergeable. A failed superseder lookup/close keeps the record so the next sweep retries instead of orphaning it; the refusal note names the closed superseder. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
β¦ superseder note The refusal marker was a prefix of the superseder-close marker, so a prefix match counted the superseder note as a second refusal note (TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption/*/supersede-open). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption/supersede-open expected the hive superseder to be closed on every pass, but closeMaintainerSuperseder only runs while AdoptedBy is set; after the first pass clears adoption the second pass correctly does nothing. Expect the close and its note on pass 0 only, matching production. Hive-Fix-Attempt: 2/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
β¦n (backport of #14305) Rework as the v5 backport of #14305: authorMaintainsRepo and repoMaintainerLevel with the same names and comments, no persisted Record.AdoptionMaintainerNotedHead and no refusal note. Kept beyond v6: a 10-minute permission cache whose failed reads defer (no takeover, no release), closing a still-open hive superseder with a note before a maintainer adoption is released (#14311), and a cache lock that is not held across GetPermissionLevel (#14313). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
Use the now already computed by the stage-label sweep instead of a second c.now() reading, matching the sibling adoption helpers. Hive-Fix-Attempt: 3/6 Co-authored-by: Copilot <223556219+Copilot@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 |
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. π Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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. |
There was a problem hiding this comment.
Reviewed (intent-alignment) β the diff satisfies #14301's acceptance criteria on v5: the permission gate fronts every takeover path β adoptContributorPR (src/pkg/github/automerge/automerge_contributor_adopt.go:567-570), supersedeAdoptedPR (:646-655), settleAdoption (:750-760), settleInPlaceAdoption (:1015-1027) and the stalled re-home in escapeStalledAdoption (:1286-1288); write-or-lower falls through unchanged (repoMaintainerLevel, :846-849); failed reads are not cached and defer (:866-870). Advisory, non-blocking:
- Changelog wording vs. content β
changelog.d/fixed-adoption-skips-repo-maintainers.md:1calls this a "backport of #14305", but it diverges from the merged v6 change: TTL is 10 min here vstime.Hourin #14305,authorMaintainsReporeturns(bool, error)and defers releases on error, and it addsreleaseMaintainerAdoption/closeMaintainerSuperseder(:884-935) which #14305 lacks. The v5βv6 forward-merge will conflict inautomerge_contributor_adopt.go; whoever lands it should reconcile with #14305's version rather than assume a clean merge. - No note on the original PR β #14301 proposed "post one note per head saying the hive will not take over a maintainer's PR". The gate only logs (
:568); the maintainer sees the PR stay at 072 with no explanation. Not in the acceptance criteria (and #14305 omits it too), so advisory. Same applies to an owner-delegateadopt-supersedelabel on a maintainer PR, which is now silently refused (:567precedes therequestedbranch at:573).
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Reviewed (correctness) β gate, fail-closed, cache and release paths check out at db659df; one advisory gap against #14301.
automerge_contributor_adopt.go:567-570refuses with a log line only. #14301 ("Proposed change") asks to "post one note per head saying the hive will not take over a maintainer's PR"; nothing in the diff posts it, and the maintainer sees no explanation why 072 never advances. Not in the acceptance-criteria list, so advisory.- Same silent refusal reaches the delegated path:
automerge_stage_labels.go:1013-1030keepsLabelAdoptSupersedeunless an attempt was recorded, and the maintainer refusal records nothing β so a human-applied adopt label on a maintainer's PR sticks for ever with no comment (TestDelegatedSupersedeCannotTakeOverMaintainerasserts the refusal, not the label/no-note outcome). A one-per-head note (or label removal) would close both.
Verified: read before any mutation in supersedeAdoptedPR (:646-655); error path never caches and defers everywhere (:869-873, 748-751, 1015-1018, 1283-1286); lock not held across the API call (:862-866, 875-884); release closes an open superseder before ClearAdoption (:908-935) and in-place release falls through to the lane stage via the re-read at automerge_stage_labels.go:866.
β hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the diff against v5 plus the callers; this looks correct to me and matches #14301's acceptance criteria.
What I verified in the tree:
automerge_contributor_adopt.go:567β the gate sits beforeadoptionDiffRefusal, andauthorMaintainsRepo(:858) returns(false, err)on a failed read with nothing cached, so a failed read defers (no adoption) exactly as #14301 asks ("fail closed"). The TTL-cache lock is released before theGetPermissionLevelcall (:865-870), so the API round trip is not under the mutex.settleInPlaceAdoptionis called on every pass for an in-place adoption (automerge_stage_labels.go:855-861), not only on head change, so the "release an already-live adoption on the next pass" path actually fires.escapeStalledAdoption(:1286) returnsadoptionEscape{}for a maintainer / unreadable permission; the caller atautomerge_stage_labels.go:878-889only acts onesc.Escalated, so this is a clean no-op.releaseMaintainerAdoptionβstagelabels.Store.ClearAdoption(src/pkg/stagelabels/store.go:1057) zeroesAdoptedBy,AdoptMode,AdoptReasonand the escalation fields, so a released record cannot re-enter the supersede path on the next pass. If closing the superseder fails, the record is kept and retried β matches the comment at:893.repoMaintainerLevelmirrors the existingauthorRepoPermission(automerge_sweep.go:2223) semantics for admin/maintain, so bot-login behaviour of the permission endpoint is already relied on by the trusted-author merge path; nothing new there.go build ./pkg/github/automerge/passes atdb659df.
One gap, informational: #14301's "Proposed change" says to "post one note per head saying the hive will not take over a maintainer's PR". The implementation only logs (automerge_contributor_adopt.go:568) and posts no comment on the maintainer's PR. The acceptance criteria don't require the note, so I'm not calling this a defect β but a maintainer whose PR stays at 072 will see the nudge and then silence.
Not in scope of my read: the no-dco label / cancelled "Sign-off survives the squash" check β that is the DCO re-home work the PR body already assigns to the fix lane.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Reviewed (docs-currency) β head db659df6. Approve; one advisory.
The new sentence in the stage/074-adopted-by-hive row (src/docs/labels-and-control-signals.md:227) and the changelog fragment match the code: 10βminute per (repo, author) cache (automerge_contributor_adopt.go:837 maintainerPermTTL), a failed read is not cached and defers in every caller (:567, :646, :750, :1015, :1286), and a stillβopen superseder is closed with a note before the release (closeMaintainerSuperseder, :918β935).
Advisory (inβscope, low): the rule is documented only in the 074 table row. The operatorβfacing prose that enumerates the neverβadopt exceptions still lists only hold / do-not-merge (src/docs/labels-and-control-signals.md:888β889), the authorβkind table still reads unconditionally "yes β after the grace, the hive adopts" for author/clanker (:615), and the state diagram's s074 --> s072 edge (:306) does not list the new maintainer release. One sentence at :889 ("β¦nor is a PR whose author holds admin/maintain on the repository") plus a clause on :306 would keep those in step; the merged v6 original #14305 made the same rowβonly choice, so this is a followβup for both branches rather than a blocker.
Note: #14301's proposed change asked for "one note per head saying the hive will not take over a maintainer's PR"; this PR logs (:568) but posts no comment on the original PR. The acceptance criteria don't require it and neither the row nor the fragment claims it, so docs are consistent β flagging only so nobody reads the 074 row as promising a comment.
β hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Supersedes #14304 (adopted in place, re-homed for DCO repair; original commits by @arteeh preserved).
Fixes #14301
This branch is #14304's head
db659df6dc17as pushed by @arteeh (forkarteeh/hive), unchanged. Last lane reason on the original:dco-missing.Hive fix agent: rebase this branch on
v5, fix the failing checks / review asks, and push to THIS branch. Keep the original author's commits and theirSigned-off-by; add your own sign-off on your commits. Do not open another PR.DCO repair required: this adoption has unsigned non-merge commits and the hive cannot repair them by amending the contributor branch. Re-home the work onto a hive-owned branch from
v5: cherry-pick each original commit withgit cherry-pick -s(or squash withgit commit --authorpreserving the original author), ensure every replacement commit hasSigned-off-by, push the DCO-clean branch, open/keep one superseding PR with the same title, then comment on and close #14304. Suggested replacement commit message: