Skip to content

πŸ› fix: prevent contributor adoption of maintainer PRs - #14304

Closed
arteeh wants to merge 7 commits into
hivecommons:v5from
arteeh:fix/14301-maintainer-adoption
Closed

arteeh wants to merge 7 commits into
hivecommons:v5from
arteeh:fix/14301-maintainer-adoption

Conversation

@arteeh

@arteeh arteeh commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #14301

Contributor adoption treated repository maintainers as offline contributors and could re-home and close their own PRs. Check the author's repository permission and role before adoption or supersession, exempting admin and maintain regardless of author kind, presence, or delegated supersede requests. Release existing adoptions before the stage-074 pin, closing a still-open hive superseder with a note first so no hive duplicate of the maintainer's PR stays mergeable. The maintainer's own PR gets no comment: the gate only logs and declines, matching the v6 fix in #14305.

Cache successful permission reads and failures per repository/author for ten minutes. An unreadable permission defers adoption and leaves any existing adoption record untouched: the hive neither takes over nor releases ownership without permission evidence, so a live superseder is never orphaned by a transient API error. Existing write-or-lower adoption behavior remains covered by the adoption tests.

Added regression coverage for permission levels and API errors, cache reuse/expiry/key isolation, maintainer clankers remaining at 072, live in-place and superseding adoption release, DCO re-home prevention, and delegated/direct supersession refusal. Added a changelog fragment.

Validation: gofmt -l reports no changes; git diff --check passes. Go tests were not run locally, as required by AGENTS.md; CI provides the test verdict.

β€” hive: backend=codex model=gpt-6.1-sol effort=medium


🐝 Hive Agent: contributor | SHA: 67eed6e2e

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>
@kubestellar-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign danathar for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubestellar-prow kubestellar-prow Bot added dco-signoff: yes Indicates the PR's author has signed the DCO. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 25, 2026
@hivecommons-hive hivecommons-hive Bot added author/clanker A contributor's own hive authored this PR under their login dco All commits are DCO signed-off role/trusted Author is trusted for unattended merge (trusted-author role or registry/claim tier in require_tiers) stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Oct 25, 2026

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the diff against v5 debf70b, plus stagelabels.Store.ClearAdoption and settleAdoption at merge-base. The maintainer gate itself is correct: adoptionAuthorPermission checks both permission and role_name (needed, since GitHub maps maintain β†’ write in permission), it runs before any mutation in adoptContributorPR, supersedeAdoptedPR, settleAdoption and escapeStalledAdoption, and the note is deduplicated per head via AdoptionMaintainerNotedHead. The 10-minute cache keyed on repo/author matches the issue's "once per sweep window".

One finding (medium): a transient permission-read failure releases an existing supersede adoption and orphans the superseding PR.

  • src/pkg/github/automerge/automerge_contributor_adopt.go:595 (PR head) β€” guardAdoption calls store.ClearAdoption(key) whenever err != nil, before distinguishing a maintainer verdict from a failed read.
  • src/pkg/stagelabels/store.go:1064 (v5) β€” ClearAdoption zeroes AdoptedBy, the superseding PR number.
  • automerge_contributor_adopt.go settleAdoption (v5) β€” returns immediately on rec.AdoptedBy <= 0, so once cleared the hive never closes the original when hive/adopt-N merges, and never returns it to 072 when hive/adopt-N closes unmerged.
  • automerge_stage_labels.go:748 (PR head) β€” the new ApplyStageLabels block runs guardAdoption on every pass for every previously adopted PR, so a single 5xx / secondary-rate-limit response on GET .../collaborators/{author}/permission is enough to trigger this for a PR that was adopted days ago. The error is then cached for adoptCheckInterval (10 min), after which the original is eligible for a second adoption β†’ a second superseding PR.

Mechanism: fail-closed is right for starting an adoption, but releasing a live adoption on an unreadable permission is not fail-closed β€” it is a destructive state change on no evidence. #14301 asks only that a failed read "do not adopt"; releasing an existing adoption is specified only for a confirmed admin/maintain author.

Suggested shape: in guardAdoption, when err != nil and the record already carries an adoption (AdoptedHead != ""), return blocked=true without ClearAdoption so the sweep skips this PR for the window but keeps AdoptedBy; clear only on maintainer == true. TestAdoptionPermissionFailureDefersWithoutAttempt covers the fresh-adoption case only; a case with a pre-existing supersede record + 500 would pin the fix.

Everything else I checked reads correctly. Not blocking on anything other than the above.

β€” hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

@github-actions

Copy link
Copy Markdown
Contributor

Open PR overlap check

Base branch: v5

Conflicts are checked with every PR merged onto the current tip of v5, so a PR that only needs a rebase is not reported as conflicting with its siblings.

No conflicting open PRs or same-file overlaps were found.

@hivecommons-hive hivecommons-hive Bot added stage/072-returned-to-contributor Contributor PR: CI red, changes requested or conflicting; diagnosis posted, author must act and removed stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Oct 25, 2026
@hivecommons-hive

Copy link
Copy Markdown
Contributor

@arteeh this PR is back in your court (stage/072-returned-to-contributor). The hive will not merge it until the head changes.

Review asks:

  • the review swarm requested changes

Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path.

Your relay session received this as a fix-pr task and launched a parallel fix session; your current work continues.

@hivecommons-hive

Copy link
Copy Markdown
Contributor

@arteeh the hive has adopted this PR (stage/074-adopted-by-hive): CI stayed red for 2h after the nudge and the relay nudge went unanswered. The hive will rebase this branch on v5 and fix CI here; your commits and sign-off stay. Push a new head yourself at any time and the hive yields it back to you.

@hivecommons-hive hivecommons-hive Bot added stage/074-adopted-by-hive Contributor PR adopted: author unavailable after red CI; the hive is finishing it (takeover path) and removed stage/072-returned-to-contributor Contributor PR: CI red, changes requested or conflicting; diagnosis posted, author must act labels Oct 25, 2026
…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>
@hivecommons-hive

Copy link
Copy Markdown
Contributor

hive/awaiting-ci: addressed the reviewer-2 finding β€” guardAdoption no longer calls ClearAdoption on a failed permission read (only a confirmed admin/maintain author releases a live adoption), with TestAdoptionPermissionFailureKeepsLiveSupersede pinning the pre-existing-supersede + 500 case. CI pending β€” sweep will merge when green.


🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

β€” hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive

Copy link
Copy Markdown
Contributor

1 of 2 commits on c8c400541756 are the hive's, signed off as the hive App, and the rest are @arteeh's with their own sign-off. A squash would land the hive's commits under @arteeh's name with the App's sign-off, so the hive will merge this PR with a merge commit instead (merge-method/merge): every commit lands as it is, each with its own author's sign-off. Nothing is rewritten and nobody needs to act. Maintainers merging by hand: use gh pr merge --merge, not squash.

@hivecommons-hive hivecommons-hive Bot added the merge-method/merge Hive merges this PR with a merge commit (mixed authorship, each commit self-signed); never squash label Oct 25, 2026

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness review (head c8c4005)

  1. Refusal note fires on merged/closed PRs. The new pre-guard at src/pkg/github/automerge/automerge_stage_labels.go:747-754 runs for any record with AdoptedHead != "" before the verdict.Terminal branch at :861 (Forget). ClassifyStage returns Terminal for o.Merged and for reason closed/gone/label-removed (src/pkg/stageengine/derive_pr_classify.go:268,291), so a maintainer who merges or closes their own adopted PR (exactly the #14301 timeline at 19:11Z) gets a permission read plus "@author the hive will not take over a repository maintainer's PR … stays with you for fixes and merge" (automerge_contributor_adopt.go:609) posted on the already-merged PR, then the record is forgotten. Suggest gating the pre-guard on !verdict.Terminal (or moving it inside the non-terminal branch). No test covers a terminal observation with a live adoption.

  2. Releasing a live supersede adoption orphans the open hive PR. guardAdoption calls store.ClearAdoption(key) (automerge_contributor_adopt.go:602), which zeroes AdoptedBy (src/pkg/stagelabels/store.go:1066) without looking at the superseding PR. If that hive-owned PR is still open it stays open, untracked, and can proceed to merge as a duplicate of the maintainer's PR β€” the duplicate the issue complains about β€” while the note claims "Any live adoption has been released". The guard's own comment (:590-593) names orphaning AdoptedBy as the reason not to release on error, but the maintainer path does it anyway. TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption only exercises setSuperseder(90, "closed", true), so the open-superseder case is untested. Suggest closing an open superseder (with a note) before ClearAdoption, or at least naming it in the refusal note.

  3. PR body vs code (low). Body says an unreadable permission "releases existing ownership"; the code deliberately keeps the record (automerge_contributor_adopt.go:589-600, TestAdoptionPermissionFailureKeepsLiveSupersede). The code behaviour is the right one β€” please fix the description.

β€” hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed (security) β€” no in-scope blockers.

All adoption mutations (PullRequests.Create at src/pkg/github/automerge/automerge_contributor_adopt.go:770, PR closes at :799 and :879) are now behind guardAdoption (:593), which is called in adoptContributorPR (:640), supersedeAdoptedPR (:718), settleAdoption (:811), escapeStalledAdoption (:1198) and the live-adoption pre-check in ApplyStageLabels (automerge_stage_labels.go:747). Unreadable permission fails closed without touching the record (:601-604), matching #14301's "if the read fails, do not adopt".

Adjacent, out-of-scope (low): releasing a live supersede adoption via store.ClearAdoption (automerge_contributor_adopt.go:606) drops AdoptedBy while the hive-owned superseding PR may still be open β€” the test only covers a closed superseder (automerge_contributor_adopt_test.go, setSuperseder(90, "closed", true)). The orphaned hive PR is then untracked and could be merged as a duplicate of the maintainer's own PR. Worth a follow-up to close it with a note when releasing.

β€” hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive

Copy link
Copy Markdown
Contributor

Hive filed review findings that do not block this PR as backlog items:

Cap: 10 backlog item(s) per PR per day.

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intent alignment (#14301) β€” reviewed head c8c4005. The diff does implement the issue's gate (admin/maintain via permission and role_name, 10‑min cache, fail‑closed, once‑per‑head note), but two in‑scope problems:

  1. Duplicates a design the maintainer already landed on v6. #14301 was closed by @Danathar as "Superseded by a PR that implements the fix directly: #14305" (merged to v6 21:11Z, after this PR's lease was revoked). This PR targets v5, which is forward‑merged into v6, and uses a different shape from #14305 on every axis: Engine.adoptionPermissions/adoptionPermission (automerge_sweep.go:177-178) vs v6's maintainerPermEntry/maintainerPermTTL = time.Hour (v6 automerge_contributor_adopt.go:783-785), a new persisted Record.AdoptionMaintainerNotedHead + Store.NoteAdoptionMaintainer (store.go:137, store.go:1074) that v6 does not have, and a new AdoptionMaintainerMarker comment (automerge_contributor_adopt.go:544). Landing this on v5 as‑is makes the next v5β†’v6 forward‑merge carry a second, conflicting implementation. Requested change: if v5 needs this fix, port #14305's implementation (authorMaintainsRepo, same test file/names, docs row in labels-and-control-signals.md) so the forward‑merge is a no‑op; otherwise close in favour of #14305.

  2. Releasing a live supersede adoption orphans the hive copy. guardAdoption calls store.ClearAdoption(key) for a confirmed maintainer (automerge_contributor_adopt.go:602), which zeroes AdoptedBy (store.go:1066). For a record in AdoptModeSupersede with an open superseding PR, this drops the only link to it: settleAdoption can no longer close the original on merge or return it on close (automerge_contributor_adopt.go:859-888), and the hive copy stays open as exactly the "duplicate" #14301 complains about. The guard's own comment acknowledges this orphaning for the error path (:589-592) but still does it on the confirmed path. The supersede case in TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption only exercises a superseder that is already closed (h.adopt.setSuperseder(90, "closed", true)), so the open‑superseder case is untested. #14305 avoided this by releasing only in‑place adoptions and using the supersede guard purely as a refusal backstop. Requested change: on supersede‑mode release, either close the open unmerged superseder (with a note) before ClearAdoption, or keep the record and only refuse further re‑homing.

β€” hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

…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>

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed (correctness) β€” no blocking findings; one advisory.

Advisory (low): release path swallows an already-merged superseder. guardAdoption (src/pkg/github/automerge/automerge_contributor_adopt.go:599-640) runs before settleAdoption's merged-superseder branch (:861-863, :912-923). For a maintainer whose record has AdoptedBy > 0 and whose hive superseder has already merged, closeMaintainerSuperseder returns (false, 0, nil) (:651-653), the guard then ClearAdoptions and posts "Any live adoption has been released … this PR stays with you for fixes and merge" (:627-634). The original is never closed with the existing "has merged. Thank you!" note and sits open at 072 while its content is already upstream. Window is small (adoptions live at rollout, or a merge landing between settle polls), but if you want to tighten it: when sup.GetMerged() in the guard, don't block β€” let settleAdoption close the original as before, then release. Not a merge blocker.

Verified: GET …/collaborators/{user}/permission returns 200 with none/read for non-collaborators (maintainβ†’write + role_name=maintain, which :575-577 handles), so fail-closed on error does not regress fork-contributor adoption. Cache mutex is held across the API call (:555-556) β€” serialized, no race. Tests cover admin/maintain/write/read/none/error, cache reuse/expiry/key isolation, fresh/in-place/supersede/open-superseder release, terminal skip, and delegated supersede.

β€” hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed β€” no findings from this perspective.

β€” hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

… 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>
@hivecommons-hive

Copy link
Copy Markdown
Contributor

hive/awaiting-ci: pushed 5f41d11 β€” the refusal marker was a prefix of the superseder-close marker, so the supersede-open case counted two refusal notes. Markers are now distinct. CI pending β€” sweep will merge when green.


🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

β€” hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intent alignment vs #14301 β€” the diff implements every acceptance criterion (admin/maintain gate independent of kind/presence/delegation, 10‑minute cache keyed by repo+author, fail‑closed on unreadable permission, release of live adoptions, one note per head), but CI fails on the PR's own regression test, so the "release" path is not yet proven.

  1. Marker prefix collision breaks the new test (CI test (rest 2/5) fails). AdoptionMaintainerMarker = "<!-- hive:adoption-maintainer" (automerge_contributor_adopt.go:544) is a string prefix of AdoptionMaintainerSupersederMarker = "<!-- hive:adoption-maintainer-superseder -->" (:590). TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption counts refusal notes with strings.HasPrefix(comment, AdoptionMaintainerMarker) (automerge_contributor_adopt_test.go:1951), so the superseder close note is counted too and supersede-open fails with refusal notes=2 (:1958). Production behavior in the log is actually correct (one refusal note, one superseder note, #90 closed), but the two markers are not distinguishable by prefix, which any future marker-scanning dedup would also trip on. Fix: give the superseder note a non‑prefix marker (e.g. <!-- hive:adoption-superseder-released -->) rather than loosening the test.

  2. (low, in‑scope) Release of a legacy supersede adoption whose hive superseder already merged: closeMaintainerSuperseder returns (false, 0, nil) on merged (:649), guardAdoption then ClearAdoptions (:622) and posts "Any live adoption has been released", so the original stays open at 072 instead of being closed with the settle path's "has merged" note (:937 region, now unreachable for maintainers). Consider skipping the guard when the superseder is merged so settleAdoption can close the original as before.

  3. (advisory, out of scope) #14305 already fixed #14301 on v6 with a different implementation (authorMaintainsRepo, separate test file, docs update, no persisted store field). This PR adds adoption_maintainer_noted_head to the store record (stagelabels/store.go) and different markers; the v5β†’v6 forward‑merge will need to pick one implementation.

β€” hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/maintainability review (head 54bbcd5) β€” no in-scope blockers; two advisory items.

  1. Duplicated permission parsing β€” adoptionAuthorPermission (src/pkg/github/automerge/automerge_contributor_adopt.go:557-590) re-implements GetPermissionLevel + lowercase/trim of permission/role_name that already exists in the same package at authorRepoPermission (src/pkg/github/automerge/automerge_sweep.go:2216-2229). Two copies of the admin/maintain matching rule in one package will drift (e.g. if GitHub adds a role). Suggest having the adoption cache call authorRepoPermission and derive maintainer from its normalized Level, keeping only the caching/empty-author handling in the new helper.

  2. Redundant predicate β€” src/pkg/github/automerge/automerge_stage_labels.go:749: prevRec.AdoptedInPlace() || prevRec.AdoptedHead != "". AdoptedInPlace() is AdoptedHead != "" && AdoptMode == AdoptModeInPlace (src/pkg/stagelabels/store.go:796-798), so the first disjunct is implied by the second; prevRec.AdoptedHead != "" alone reads clearer.

Minor: adoptionAuthorPermission holds adoptionPermissionsMu across the network call (automerge_contributor_adopt.go:558-559,573); other Engine caches in this file set only guard map access. Fine while the sweep is single-threaded, but worth narrowing if the lookup ever runs concurrently.

β€” hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness review (read at head 5f41d11)

1. New test fails in CI β€” assertion bug, production behavior is right. TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption/{admin,maintain}/supersede-open fails on pass 1 (automerge_contributor_adopt_test.go:1972: superseder notes=0 edits=[], job 121328708177). The loop at automerge_contributor_adopt_test.go:1943-1975 sets wantSuperseder = 1 / wantEdits = ["90:closed"] for supersede-open on every iteration, but the comment on :1961 and the code under test (guardAdoption β†’ closeMaintainerSuperseder, automerge_contributor_adopt.go:605-619) only close the superseder on the first pass β€” after ClearAdoption the record has AdoptedBy == 0, so pass 1 correctly posts no superseder note and makes no edit. Fix: gate the expectation on i == 0 (e.g. if mode == "supersede-open" && i == 0 { ... }). The coverage job failure is downstream of this.

Advisory (no change requested): because settleAdoption and escapeStalledAdoption also run through guardAdoption (automerge_contributor_adopt.go:863-865, :1250-1252), a cached permission-read failure also delays closing the original after a merged superseder for up to adoptCheckInterval. Consistent with the fail-closed intent in #14301; just noting the blast radius.

Verified: GET …/collaborators/{login}/permission returns 200 permission:none for [bot] logins and non-collaborators, so write-or-lower/fork clankers are not fail-closed by the new gate.

β€” hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed β€” no findings from this perspective.

β€” hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

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>

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intent alignment (#14301) β€” reviewed at 5f41d11.

The diff implements every acceptance criterion of #14301: permission gate before adoptContributorPR (automerge_contributor_adopt.go:692), before every supersession/DCO re-home path (supersedeAdoptedPR :770, settleAdoption :863, escapeStalledAdoption :1250), release of live adoptions on the next pass (automerge_stage_labels.go:749-757), fail-closed on read error (:605-608), 10-minute per-(repo,author) cache (:556-585), and the role_name check that the docs require because permission maps maintain→write. No unrelated scope.

Blocker β€” the PR's own new test fails in CI (test (rest 2/5), job 121328708177): TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption/{admin,maintain}/supersede-open β†’ pass 1: superseder notes=0 edits=[].

That is iteration i=1 (0-based), i.e. the second sweep. Production is right: closeMaintainerSuperseder finds #90 already closed and returns (false, 0, nil) (automerge_contributor_adopt.go:652-654), and guardAdoption only closes when rec.AdoptedBy > 0 (:613), which ClearAdoption zeroed on pass 1 (stagelabels/store.go:1066). The test's comment at automerge_contributor_adopt_test.go:1961-1962 says "on the first pass only", but the expectation at :1964-1965 sets wantSuperseder = 1 / wantEdits = ["90:closed"] for every iteration. Fix: condition that block on i == 0 (e.g. if mode == "supersede-open" && i == 0 {). The coverage job fails for the same reason.

Advisory (no change requested): #14301 says "already-live adoption … should be released on the next pass"; the PR deliberately does not release on an unreadable permission (automerge_contributor_adopt.go:605-608) to avoid orphaning a superseder. That is a sound refinement and is stated in the PR body; noting it only so the deviation is on record.

β€” hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

@clubanderson

Copy link
Copy Markdown
Member

/hive decision Converge on the v6 implementation. #14305 already merged to v6 for the same issue (authorMaintainsRepo/repoMaintainerLevel, no persisted store field). Rework this PR as the v5 backport of #14305: same function names, same comment markers, no Record.AdoptionMaintainerNotedHead in stagelabels/store.go. Keep only what v6 lacks and is worth having on both lines: the 10-minute permission cache with defer-on-unreadable, and closing a still-open hive superseder with a note before releasing a maintainer adoption (#14311). Narrow the cache lock so it is not held across GetPermissionLevel (#14313). Whatever v6 lacks after this lands gets a forward-port PR to v6, so the forward-merge has nothing to choose.

…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>
@kubestellar-prow kubestellar-prow Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 26, 2026
@hivecommons-hive

Copy link
Copy Markdown
Contributor

hive/awaiting-ci: reworked per the /hive decision as the v5 backport of #14305 β€” same authorMaintainsRepo/repoMaintainerLevel, gate sites (adoptContributorPR, supersedeAdoptedPR, settleInPlaceAdoption), v6 test names and changelog fragment name; Record.AdoptionMaintainerNotedHead and the refusal note are gone from stagelabels/store.go. Kept beyond v6: a 10-minute permission cache whose failed reads defer (no takeover, no release, record untouched), and closing a still-open hive superseder with a note before releasing a maintainer adoption (#14311). The cache lock now covers map access only and is not held across GetPermissionLevel (#14313). CI pending β€” sweep will merge when green.


🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

β€” hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intent alignment (reviewed current head e683608a1; the dispatched SHA 7d530fe38 has been superseded by the v6-convergence commit)

Against #14301 the diff satisfies all three acceptance criteria: adoptContributorPR gates on authorMaintainsRepo before the fence (src/pkg/github/automerge/automerge_contributor_adopt.go:567-570), supersedeAdoptedPR refuses and releases for every re-home path including delegated supersede (:646-655), settleAdoption/settleInPlaceAdoption release existing adoptions (:750-760, :1015-1027), reads are cached per repo/author for 10m and a failed read defers (:862-893). Tests cover admin/maintain/write/read/error, 072 stay, in-place and supersede release, delegated supersede (automerge_adopt_maintainer_test.go).

PR body no longer matches the diff (low). The body says the hive will "post one refusal note per head with persistent deduplication" and "skip the refusal on merged/closed observations". At the current head there is no such note: the maintainer gate in adoptContributorPR (automerge_contributor_adopt.go:567-570) only logs and returns 0, nil, and the only comment this PR posts is the superseder-close note (:503, AdoptionMaintainerSupersederMarker). The merged v6 reference #14305 also omits the note and the issue's acceptance criteria do not require it, so the behaviour is fine β€” but the body and the changelog fragment describe the earlier revision. Requested change: trim the PR body (and keep the changelog fragment as is, which already matches the code) so the summary describes the converged behaviour: no refusal comment on the maintainer's PR, only the superseder-close note.

β€” hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style / conventions review (read at head e683608a1, the PR moved past 7d530fe while under review)

1. PR body claims behavior the diff does not implement (in-scope, medium). The body says the fix will "post one refusal note per head with persistent deduplication" and "skip the refusal on merged/closed observations", matching #14301's proposed "post one note per head saying the hive will not take over a maintainer's PR". The diff never posts such a note: src/pkg/github/automerge/automerge_contributor_adopt.go:567-570 (adoptContributorPR) only logs via c.info(...) and returns 0, nil, and the only new marker is AdoptionMaintainerSupersederMarker (:924), which is stamped on the superseder's close note, not on the maintainer's PR. No dedup state or merged/closed check for a refusal exists anywhere in the diff. The merged v6 source #14305 has the same gap, and the issue's acceptance criteria do not require the note, so the concrete fix is to trim those two claims from the PR body (the changelog fragment is already accurate). If the per-head note is still wanted, file it as a follow-up rather than leaving the description asserting it.

2. Clock idiom (in-scope, low, advisory). Every sibling in this file threads now time.Time from the caller (adoptContributorPR:548, supersedeAdoptedPR:645, settleAdoption:745, escapeStalledAdoption:1255), and the caller already has now := c.now() at automerge_stage_labels.go:715. The new block in settleInPlaceAdoption (automerge_contributor_adopt.go:1016) instead calls c.now() directly. Threading now through keeps one clock per sweep and matches the local convention; not blocking.

β€” hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed β€” no findings from this perspective.

β€” hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness review (read at current head e683608a1, not the dispatched 7d530fe38 β€” the PR moved on; the "converge on v6" commit rewrote the implementation).

  1. PR body describes behaviour the head no longer implements (medium, in-scope). The body says the fix "post[s] one refusal note per head with persistent deduplication" and "skip[s] the refusal on merged/closed observations". At e683608a1 there is no refusal comment anywhere on the maintainer's PR: adoptContributorPR just logs and returns (src/pkg/github/automerge/automerge_contributor_adopt.go:567-570), and the convergence commit deleted the marker/dedup state (-11 in automerge_stage_labels.go, -15 in pkg/stagelabels/store.go). The only note posted is on the hive superseder when a live adoption is released (automerge_contributor_adopt.go:912-935). Likewise "release existing adoptions before the stage-074 pin" holds only for in-place adoptions (settleInPlaceAdoption at automerge_stage_labels.go:872, before the pin at :884); a supersede adoption is released in settleAdoption (:1091), after store.Observe, so that PR reads 074 for one more pass. The merged v6 original (#14305) behaves the same way, so the code is fine as a backport β€” please rewrite the body (and the stale SHA: 67eed6e2e line) to match what landed. Issue #14301's acceptance criteria do not require the note, so no code change is needed for that.

  2. Permission-read error now skips the contributor-push yield check (low, in-scope). In settleInPlaceAdoption the maintainer probe runs first and return 0, nil on error (automerge_contributor_adopt.go:1015-1020) β€” before the existing obs.HeadSHA != prev.AdoptedHead / HivePushedHead / GetCommit logic at :1028-1050 that hands the PR back when the contributor pushed. While GET …/permission errors, a contributor push keeps 074 and the hive's red-PR fix kick keeps treating the branch as its own. Suggested fix: on err != nil fall through to the yield check instead of returning (only the release needs permission evidence); on maintainer keep the early release.

Checked and OK: cache lock not held across the API call (:863-870), failures not cached (:871-874), fail-closed in adoptContributorPR, supersedeAdoptedPR (:646-655) and escapeStalledAdoption (:1286-1288), superseder close ordered before ClearAdoption (:898-909). Tests in automerge_adopt_maintainer_test.go cover the issue's table/072/in-place-release plan. go build ./pkg/github/automerge/ passes at head; CI was still pending when I read it.

β€” hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

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>
@kubestellar-prow kubestellar-prow Bot added dco-signoff: no Indicates the PR's author has not signed the DCO. and removed dco-signoff: yes Indicates the PR's author has signed the DCO. labels Oct 26, 2026
@kubestellar-prow

Copy link
Copy Markdown
Contributor

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:

  • db659df fix: thread sweep clock into settleInPlaceAdoption
Details

Instructions 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.

@hivecommons-hive hivecommons-hive Bot added no-dco A commit is missing Signed-off-by β€” run `git rebase --signoff` and removed dco All commits are DCO signed-off labels Oct 26, 2026
@hivecommons-hive

Copy link
Copy Markdown
Contributor

@arteeh this adopted PR still has unsigned commits, so the hive is re-homing it as #14320 on a hive-owned DCO-clean branch. Closing this PR so DCO does not stall the adopted work in place.

@hivecommons-hive hivecommons-hive Bot closed this Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author/clanker A contributor's own hive authored this PR under their login dco-signoff: no Indicates the PR's author has not signed the DCO. merge-method/merge Hive merges this PR with a merge commit (mixed authorship, each commit self-signed); never squash no-dco A commit is missing Signed-off-by β€” run `git rebase --signoff` role/trusted Author is trusted for unattended merge (trusted-author role or registry/claim tier in require_tiers) size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. stage/074-adopted-by-hive Contributor PR adopted: author unavailable after red CI; the hive is finishing it (takeover path)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

πŸ› bug: contributor adoption takes over a repository maintainer's own PR (admin author adopted as contributor-offline, then re-homed)

2 participants