From 67eed6e2e0f15167cdd20a13befa56ad4a7fbf9f Mon Sep 17 00:00:00 2001 From: arteeh Date: Sun, 25 Oct 2026 20:57:05 +0000 Subject: [PATCH 1/7] =?UTF-8?q?=F0=9F=90=9B=20fix:=20prevent=20contributor?= =?UTF-8?q?=20adoption=20of=20maintainer=20PRs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: hivecommons/hive#14301 Hive-Plan: maintainer-adoption-guard Hive-Spec: issue-14301#acceptance-criteria Signed-off-by: arteeh --- changelog.d/fixed-maintainer-adoption.md | 1 + .../automerge/automerge_contributor_adopt.go | 82 +++++++++ .../automerge_contributor_adopt_test.go | 171 +++++++++++++++++- .../automerge/automerge_stage_labels.go | 9 + src/pkg/github/automerge/automerge_sweep.go | 3 + src/pkg/stagelabels/store.go | 15 ++ 6 files changed, 272 insertions(+), 9 deletions(-) create mode 100644 changelog.d/fixed-maintainer-adoption.md diff --git a/changelog.d/fixed-maintainer-adoption.md b/changelog.d/fixed-maintainer-adoption.md new file mode 100644 index 0000000000..ff93b1587c --- /dev/null +++ b/changelog.d/fixed-maintainer-adoption.md @@ -0,0 +1 @@ +- Contributor adoption now leaves repository admins and maintainers in control of their PRs, releases existing adoptions, and defers takeover when author permissions cannot be read ([#14301](https://github.com/hivecommons/hive/issues/14301)). diff --git a/src/pkg/github/automerge/automerge_contributor_adopt.go b/src/pkg/github/automerge/automerge_contributor_adopt.go index 94467ee364..a238d3cb01 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt.go @@ -539,6 +539,76 @@ func shortSHA(sha string) string { return sha } +// AdoptionMaintainerMarker identifies the once-per-head refusal to take over a +// repository maintainer's PR, independently of author kind or relay presence. +const AdoptionMaintainerMarker = "\n@%s the hive will not take over a repository maintainer's PR. Any live adoption has been released; this PR stays with you for fixes and merge.", AdoptionMaintainerMarker, obs.HeadSHA, obs.Author) + if _, _, err := c.gh.Issues.CreateComment(ctx, obs.Owner, obs.Repo, obs.Number, &gh.IssueComment{Body: gh.Ptr(body)}); err != nil { + return true, 0, err + } + store.NoteAdoptionMaintainer(key, obs.HeadSHA) + return true, 1, nil +} + // adoptContributorPR runs the stage-074 check for one clanker PR in stage // 072 and, when adoption is due, performs it: in place (the hive's fix lane // pushes to the PR's own branch) when GitHub permits and the policy allows, @@ -564,6 +634,9 @@ func (c *Engine) adoptContributorPR(ctx context.Context, store *stagelabels.Stor c.info("stage labels: adoption due but blocked by label", "repo", obs.Display, "pr", obs.Number, "label", l) return 0, nil } + if blocked, n, err := c.guardAdoption(ctx, store, key, obs, now); blocked { + return n, err + } fence, err := c.adoptionDiffRefusal(ctx, obs) if fence != "" && (err != nil || adoptionFenceDefers(fence)) { // The fence could not judge this head (it moved, or its file list @@ -639,6 +712,9 @@ func delegatedAdoptionAttempted(before, after stagelabels.Record) bool { } func (c *Engine) supersedeAdoptedPR(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, rec stagelabels.Record, policy StageLabelPolicy, reason string, grace time.Duration, online bool, dcoRehome bool, now time.Time) (int, error) { + if blocked, n, err := c.guardAdoption(ctx, store, key, obs, now); blocked { + return n, err + } owner, repo, number := obs.Owner, obs.Repo, obs.Number rec.AdoptReason = reason if stored, ok := store.Get(key); ok && stored.AdoptDiffFence != "" { @@ -729,6 +805,9 @@ func (c *Engine) supersedeAdoptedPR(ctx context.Context, store *stagelabels.Stor // most every adoptCheckInterval): merged → close the original with a note; // closed unmerged → forget the adoption so the original returns to 072. func (c *Engine) settleAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, policy StageLabelPolicy, now time.Time) (int, error) { + if blocked, n, err := c.guardAdoption(ctx, store, key, obs, now); blocked { + return n, err + } rec, ok := store.Get(key) if !ok { return 0, nil @@ -1113,6 +1192,9 @@ func adoptionStalledReason(obs stageObservation, rec stagelabels.Record, policy // blocker clears (hivecommons/hive#13040). Returns the decision and the // number of comments posted. func (c *Engine) escapeStalledAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, policy StageLabelPolicy, now time.Time) (adoptionEscape, int, error) { + if blocked, n, err := c.guardAdoption(ctx, store, key, obs, now); blocked { + return adoptionEscape{}, n, err + } rec, ok := store.Get(key) if !ok { return adoptionEscape{}, 0, nil diff --git a/src/pkg/github/automerge/automerge_contributor_adopt_test.go b/src/pkg/github/automerge/automerge_contributor_adopt_test.go index e02241f9aa..501259c7e0 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt_test.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt_test.go @@ -18,14 +18,18 @@ import ( // adoptAPI layers the git-ref / pull-request routes the adoption needs on // top of stageAPI and records what the hive did. type adoptAPI struct { - mu sync.Mutex - refs map[string]string // refs/heads/x → sha - refUpdates []string - pulls []map[string]any - nextPR int - superseders map[int]map[string]any // number → GET payload - prEdits []string - commits map[string]adoptCommit + permission string + roleName string + permissionStatus int + permissionReads int + mu sync.Mutex + refs map[string]string // refs/heads/x → sha + refUpdates []string + pulls []map[string]any + nextPR int + superseders map[int]map[string]any // number → GET payload + prEdits []string + commits map[string]adoptCommit // failRefs makes POST git/refs answer 500; refAttempts counts them. failRefs bool refAttempts int @@ -62,6 +66,18 @@ func (a *adoptAPI) handle(w http.ResponseWriter, r *http.Request) bool { defer a.mu.Unlock() path := r.URL.Path switch { + case r.Method == http.MethodGet && strings.Contains(path, "/collaborators/") && strings.HasSuffix(path, "/permission"): + a.permissionReads++ + if a.permissionStatus != 0 { + http.Error(w, "permission lookup failed", a.permissionStatus) + return true + } + permission := a.permission + if permission == "" { + permission = "write" + } + _ = json.NewEncoder(w).Encode(map[string]string{"permission": permission, "role_name": a.roleName}) + return true case r.Method == http.MethodGet && strings.HasPrefix(path, "/repos/acme/widget/pulls/") && strings.HasSuffix(path, "/files"): if a.failPRFiles { w.WriteHeader(http.StatusInternalServerError) @@ -1840,7 +1856,7 @@ func TestSupersedeRevalidatesBeforeRefMutation(t *testing.T) { h.adopt.heads[7] = "aaa" h.adopt.refs["refs/heads/hive/adopt-7"] = "old" tc.prepare(h.adopt) - obs := stageObservation{Owner: "acme", Repo: "widget", Display: "acme/widget", Number: 7, HeadSHA: "aaa", BaseRef: "v5"} + obs := stageObservation{Owner: "acme", Repo: "widget", Display: "acme/widget", Author: "mallory", Number: 7, HeadSHA: "aaa", BaseRef: "v5"} n, _ := h.e.supersedeAdoptedPR(context.Background(), h.e.stageStoreOrShared(), stageKey("acme", "widget", 7), obs, stagelabels.Record{}, adoptPolicy(), AdoptReasonDCORehome, 0, false, true, h.clock.at) if n != 0 || h.adopt.refAttempts != 0 || len(h.adopt.refUpdates) != 0 || h.adopt.refs["refs/heads/hive/adopt-7"] != "old" || len(h.adopt.pulls) != 0 || len(h.adopt.prEdits) != 0 { t.Fatalf("unsafe supersession mutated repository: n=%d refs=%v updates=%v pulls=%v edits=%v", n, h.adopt.refs, h.adopt.refUpdates, h.adopt.pulls, h.adopt.prEdits) @@ -1848,3 +1864,140 @@ func TestSupersedeRevalidatesBeforeRefMutation(t *testing.T) { }) } } + +func TestAdoptionAuthorPermission(t *testing.T) { + for _, tc := range []struct { + permission, role string + status int + maintainer, wantErr bool + }{ + {permission: "admin", maintainer: true}, + {permission: "maintain", maintainer: true}, + {permission: "write", role: "maintain", maintainer: true}, + {permission: "write", role: "admin", maintainer: true}, + {permission: "write"}, + {permission: "read"}, + {permission: "none"}, + {status: http.StatusForbidden, wantErr: true}, + } { + t.Run(tc.permission+tc.role+http.StatusText(tc.status), func(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permission, h.adopt.roleName, h.adopt.permissionStatus = tc.permission, tc.role, tc.status + obs := stageObservation{Owner: "acme", Repo: "widget", Author: "mallory", Number: 7} + for i := 0; i < 2; i++ { + got, err := h.e.adoptionAuthorPermission(context.Background(), obs, h.clock.at) + if got != tc.maintainer || (err != nil) != tc.wantErr { + t.Fatalf("maintainer=%v err=%v", got, err) + } + obs.Number++ // A second PR by the same author uses the cached read. + } + if h.adopt.permissionReads != 1 { + t.Fatalf("reads=%d", h.adopt.permissionReads) + } + h.clock.advance(adoptCheckInterval) + _, _ = h.e.adoptionAuthorPermission(context.Background(), obs, h.clock.at) + if h.adopt.permissionReads != 2 { + t.Fatalf("expired cache reads=%d", h.adopt.permissionReads) + } + obs.Author = "other" + _, _ = h.e.adoptionAuthorPermission(context.Background(), obs, h.clock.at) + obs.Repo = "other" + _, _ = h.e.adoptionAuthorPermission(context.Background(), obs, h.clock.at) + if h.adopt.permissionReads != 4 { + t.Fatalf("cache failed to distinguish repo/author: reads=%d", h.adopt.permissionReads) + } + }) + } +} + +func TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption(t *testing.T) { + for _, mode := range []string{"fresh", "in-place", "supersede"} { + for _, permission := range []string{"admin", "maintain"} { + t.Run(permission+"/"+mode, func(t *testing.T) { + p := adoptPolicy() + p.AdoptInPlace = true + p.DCORequired = func(string) (bool, bool) { return true, true } + h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, p) + defer done() + h.adopt.permission = permission + h.adopt.allowMaintainerEdits(7, true) + h.api.commits = map[int][]string{7: {"fix: unsigned contributor commit"}} + h.observe(adoptPR(7, "aaa"), "check-failing:DCO") + key := stageKey("acme", "widget", 7) + labels := []string{"stage/072-returned-to-contributor", "role/contributor", "author/clanker", stagelabels.NoDCOLabel} + if mode != "fresh" { + h.e.stageStoreOrShared().MarkAdoptedInPlace(key, "aaa", AdoptReasonOffline, h.clock.at) + if mode == "supersede" { + h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) + h.adopt.setSuperseder(90, "closed", true) + } + labels[0] = "stage/074-adopted-by-hive" + } + h.clock.advance(2 * time.Hour) + for i := 0; i < 2; i++ { + h.api.reset() + h.observe(adoptPR(7, "aaa", labels...), "check-failing:DCO") + rec, _ := h.e.stageStoreOrShared().Get(key) + if rec.Stage != stagelabels.ReturnedToContributor || rec.AdoptionLive("aaa") || rec.AdoptedHead != "" { + t.Fatalf("maintainer adopted: %+v", rec) + } + notes := 0 + for _, comment := range h.api.comments { + if strings.HasPrefix(comment, AdoptionMaintainerMarker) { + notes++ + } + } + if notes != 1-i { + t.Fatalf("pass %d: refusal notes=%d comments=%v", i, notes, h.api.comments) + } + if len(adoptionComments(h.api)) != 0 || len(h.adopt.refs) != 0 || len(h.adopt.pulls) != 0 || len(h.adopt.prEdits) != 0 { + t.Fatalf("maintainer takeover: comments=%v refs=%v pulls=%v edits=%v", h.api.comments, h.adopt.refs, h.adopt.pulls, h.adopt.prEdits) + } + } + }) + } + } +} + +func TestAdoptionPermissionFailureDefersWithoutAttempt(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, adoptPolicy()) + defer done() + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + h.adopt.permissionStatus = http.StatusForbidden + h.clock.advance(2 * time.Hour) + h.e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", adoptPR(7, "aaa", "author/clanker"), "check-failing:unit", false, nil) + if _, err := h.e.ApplyStageLabels(context.Background()); err == nil { + t.Fatal("permission failure was not surfaced") + } + rec, _ := h.e.stageStoreOrShared().Get(stageKey("acme", "widget", 7)) + if rec.Stage != stagelabels.ReturnedToContributor || rec.AdoptedHead != "" || len(h.adopt.pulls) != 0 || h.adopt.refAttempts != 0 || len(adoptionComments(h.api)) != 0 { + t.Fatalf("adopted without permission evidence: %+v", rec) + } +} + +func TestSupersedeMaintainerRefusesBeforeMutation(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permission = "admin" + obs := stageObservation{Owner: "acme", Repo: "widget", Author: "mallory", Number: 7, HeadSHA: "aaa", BaseRef: "v5"} + _, err := h.e.supersedeAdoptedPR(context.Background(), h.e.stageStoreOrShared(), stageKey("acme", "widget", 7), obs, stagelabels.Record{}, adoptPolicy(), AdoptReasonDCORehome, 0, false, true, h.clock.at) + if err != nil { + t.Fatal(err) + } + if h.adopt.refAttempts != 0 || len(h.adopt.pulls) != 0 || len(h.adopt.prEdits) != 0 { + t.Fatal("maintainer supersession mutated repository") + } +} + +func TestDelegatedSupersedeCannotTakeOverMaintainer(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permission, h.adopt.roleName = "write", "maintain" + labels := []string{"stage/085-needs-decision", delegatedecision.LabelAdoptSupersede} + h.observe(adoptPR(7, "aaa", labels...), contributorLaneReasonNoReviewer) + rec, _ := h.e.stageStoreOrShared().Get(stageKey("acme", "widget", 7)) + if rec.AdoptedHead != "" || h.adopt.refAttempts != 0 || len(h.adopt.pulls) != 0 { + t.Fatalf("delegated request adopted maintainer: %+v", rec) + } +} diff --git a/src/pkg/github/automerge/automerge_stage_labels.go b/src/pkg/github/automerge/automerge_stage_labels.go index b9e21d2990..16757ea19d 100644 --- a/src/pkg/github/automerge/automerge_stage_labels.go +++ b/src/pkg/github/automerge/automerge_stage_labels.go @@ -744,6 +744,15 @@ func (c *Engine) ApplyStageLabels(ctx context.Context) (StageLabelResult, error) obs.Reason = verdict.Reason } prevRec, hadPrev := store.Get(key) + if hadPrev && (prevRec.AdoptedInPlace() || prevRec.AdoptedHead != "") { + _, n, err := c.guardAdoption(ctx, store, key, obs, now) + result.Comments += n + if err != nil && firstErr == nil { + firstErr = err + } + prevRec, _ = store.Get(key) + } + // pass is what this pass learns past resolveStage; it rides on the // Facts so the engine's DerivePR reproduces the stage written here. pass := &stageengine.PRPassFacts{AuthorIsApp: normalizeBotLogin(obs.Author) == normalizeBotLogin(c.transport.AppBotLogin())} diff --git a/src/pkg/github/automerge/automerge_sweep.go b/src/pkg/github/automerge/automerge_sweep.go index 602ce05e51..11dcf03bee 100644 --- a/src/pkg/github/automerge/automerge_sweep.go +++ b/src/pkg/github/automerge/automerge_sweep.go @@ -174,6 +174,9 @@ type Engine struct { gh *gh.Client logger *slog.Logger + adoptionPermissionsMu sync.Mutex + adoptionPermissions map[string]adoptionPermission + selfSkipWarnedMu sync.Mutex selfSkipWarned map[string]struct{} diff --git a/src/pkg/stagelabels/store.go b/src/pkg/stagelabels/store.go index d21625e929..2927a526cd 100644 --- a/src/pkg/stagelabels/store.go +++ b/src/pkg/stagelabels/store.go @@ -133,6 +133,8 @@ type Record struct { // adoption on (the author pushed again); the yield note is posted once // per head. YieldedHead string `json:"yielded_head,omitempty"` + // AdoptionMaintainerNotedHead deduplicates the refusal note across sweeps and restarts. + AdoptionMaintainerNotedHead string `json:"adoption_maintainer_noted_head,omitempty"` // HivePushedHeads are heads the hive recorded pushing to this adopted PR. // They are trusted local evidence for in-place adoption attribution when // the later GitHub commit object cannot link the commit to the App. @@ -1068,6 +1070,19 @@ func (s *Store) ClearAdoption(key string) { s.dirty = true } +// NoteAdoptionMaintainer records a successfully posted maintainer refusal note. +func (s *Store) NoteAdoptionMaintainer(key, head string) { + s.mu.Lock() + defer s.mu.Unlock() + r, ok := s.items[key] + if !ok || head == "" || r.AdoptionMaintainerNotedHead == head { + return + } + r.AdoptionMaintainerNotedHead = head + s.items[key] = r + s.dirty = true +} + // NoteAdoptDiffFence records the diff-fence reason the adopted head tripped // ("" when the diff read clean) on an existing record. func (s *Store) NoteAdoptDiffFence(key, reason string) { From c8c4005417563116d6b8b19ee13fc2d5e857dca0 Mon Sep 17 00:00:00 2001 From: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Sun, 25 Oct 2026 19:19:12 -0400 Subject: [PATCH 2/7] =?UTF-8?q?=F0=9F=90=9B=20fix(adoption):=20keep=20a=20?= =?UTF-8?q?live=20supersede=20adoption=20when=20the=20author=20permission?= =?UTF-8?q?=20read=20fails?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../automerge/automerge_contributor_adopt.go | 7 +++++-- .../automerge_contributor_adopt_test.go | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/pkg/github/automerge/automerge_contributor_adopt.go b/src/pkg/github/automerge/automerge_contributor_adopt.go index a238d3cb01..bcec1b5a4d 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt.go @@ -586,17 +586,20 @@ func (c *Engine) adoptionAuthorPermission(ctx context.Context, obs stageObservat } // guardAdoption releases ownership before any further adoption side effects. -// Unreadable permission also releases ownership: no takeover is safe without it. +// Unreadable permission blocks the pass without changing the record: a fresh +// takeover is not safe without evidence, and releasing a live adoption on +// none would orphan its superseding PR (AdoptedBy) and let the original be +// adopted a second time once the error ages out of the cache. func (c *Engine) guardAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, now time.Time) (bool, int, error) { maintainer, err := c.adoptionAuthorPermission(ctx, obs, now) if !maintainer && err == nil { return false, 0, nil } - store.ClearAdoption(key) if err != nil { c.warn("stage labels: adoption deferred, author permission unreadable", "repo", obs.Display, "pr", obs.Number, "error", err) return true, 0, err } + store.ClearAdoption(key) rec, ok := store.Get(key) if !ok || obs.HeadSHA == "" || rec.AdoptionMaintainerNotedHead == obs.HeadSHA { return true, 0, nil diff --git a/src/pkg/github/automerge/automerge_contributor_adopt_test.go b/src/pkg/github/automerge/automerge_contributor_adopt_test.go index 501259c7e0..5b0dc9ced7 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt_test.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt_test.go @@ -1976,6 +1976,27 @@ func TestAdoptionPermissionFailureDefersWithoutAttempt(t *testing.T) { } } +func TestAdoptionPermissionFailureKeepsLiveSupersede(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + key := stageKey("acme", "widget", 7) + h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) + h.adopt.permissionStatus = http.StatusInternalServerError + h.clock.advance(adoptCheckInterval + time.Minute) + h.e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", adoptPR(7, "aaa", "stage/074-adopted-by-hive", "author/clanker"), "check-failing:unit", false, nil) + if _, err := h.e.ApplyStageLabels(context.Background()); err == nil { + t.Fatal("permission failure was not surfaced") + } + rec, _ := h.e.stageStoreOrShared().Get(key) + if rec.AdoptedHead != "aaa" || rec.AdoptedBy != 90 || rec.AdoptMode != stagelabels.AdoptModeSupersede { + t.Fatalf("transient permission failure released a live supersede adoption: %+v", rec) + } + if len(h.adopt.pulls) != 0 || h.adopt.refAttempts != 0 || len(adoptionComments(h.api)) != 0 { + t.Fatalf("permission failure mutated the repository: pulls=%v refs=%d", h.adopt.pulls, h.adopt.refAttempts) + } +} + func TestSupersedeMaintainerRefusesBeforeMutation(t *testing.T) { h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) defer done() From 54bbcd50b1d25011fe2c68441c55dcc9ae3d88ec Mon Sep 17 00:00:00 2001 From: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Sun, 25 Oct 2026 19:34:59 -0400 Subject: [PATCH 3/7] =?UTF-8?q?=F0=9F=90=9B=20fix(adoption):=20skip=20main?= =?UTF-8?q?tainer=20refusal=20on=20terminal=20PRs=20and=20close=20open=20s?= =?UTF-8?q?uperseders=20on=20release?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- changelog.d/fixed-maintainer-adoption.md | 2 +- .../automerge/automerge_contributor_adopt.go | 58 ++++++++++++- .../automerge_contributor_adopt_test.go | 86 +++++++++++++++++-- .../automerge/automerge_stage_labels.go | 4 +- 4 files changed, 138 insertions(+), 12 deletions(-) diff --git a/changelog.d/fixed-maintainer-adoption.md b/changelog.d/fixed-maintainer-adoption.md index ff93b1587c..cbf5598a15 100644 --- a/changelog.d/fixed-maintainer-adoption.md +++ b/changelog.d/fixed-maintainer-adoption.md @@ -1 +1 @@ -- Contributor adoption now leaves repository admins and maintainers in control of their PRs, releases existing adoptions, and defers takeover when author permissions cannot be read ([#14301](https://github.com/hivecommons/hive/issues/14301)). +- Contributor adoption now leaves repository admins and maintainers in control of their PRs, releases existing adoptions (closing any still-open hive superseder with a note), and defers takeover when author permissions cannot be read ([#14301](https://github.com/hivecommons/hive/issues/14301)). diff --git a/src/pkg/github/automerge/automerge_contributor_adopt.go b/src/pkg/github/automerge/automerge_contributor_adopt.go index bcec1b5a4d..a660f627c6 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt.go @@ -585,11 +585,17 @@ func (c *Engine) adoptionAuthorPermission(ctx context.Context, obs stageObservat return entry.maintainer, entry.err } +// AdoptionMaintainerSupersederMarker is stamped on the note that closes the +// hive's superseding PR when the original turns out to be a maintainer's. +const AdoptionMaintainerSupersederMarker = "" + // guardAdoption releases ownership before any further adoption side effects. // Unreadable permission blocks the pass without changing the record: a fresh // takeover is not safe without evidence, and releasing a live adoption on // none would orphan its superseding PR (AdoptedBy) and let the original be -// adopted a second time once the error ages out of the cache. +// adopted a second time once the error ages out of the cache. A maintainer's +// live supersede adoption closes its still-open superseder first, so the +// release never leaves a hive duplicate of the maintainer's PR mergeable. func (c *Engine) guardAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, now time.Time) (bool, int, error) { maintainer, err := c.adoptionAuthorPermission(ctx, obs, now) if !maintainer && err == nil { @@ -599,16 +605,60 @@ func (c *Engine) guardAdoption(ctx context.Context, store *stagelabels.Store, ke c.warn("stage labels: adoption deferred, author permission unreadable", "repo", obs.Display, "pr", obs.Number, "error", err) return true, 0, err } + comments := 0 + closedSuperseder := 0 + if rec, ok := store.Get(key); ok && rec.AdoptedBy > 0 { + closed, n, err := c.closeMaintainerSuperseder(ctx, obs, rec.AdoptedBy) + comments += n + if err != nil { + // Keep the record: the superseder is still tracked and the + // close retries on the next sweep instead of orphaning it. + return true, comments, err + } + if closed { + closedSuperseder = rec.AdoptedBy + } + } store.ClearAdoption(key) rec, ok := store.Get(key) if !ok || obs.HeadSHA == "" || rec.AdoptionMaintainerNotedHead == obs.HeadSHA { - return true, 0, nil + return true, comments, nil } - body := fmt.Sprintf("%s head=%s -->\n@%s the hive will not take over a repository maintainer's PR. Any live adoption has been released; this PR stays with you for fixes and merge.", AdoptionMaintainerMarker, obs.HeadSHA, obs.Author) + released := "Any live adoption has been released" + if closedSuperseder > 0 { + released = fmt.Sprintf("The hive's superseding PR #%d has been closed and the adoption released", closedSuperseder) + } + body := fmt.Sprintf("%s head=%s -->\n@%s the hive will not take over a repository maintainer's PR. %s; this PR stays with you for fixes and merge.", AdoptionMaintainerMarker, obs.HeadSHA, obs.Author, released) if _, _, err := c.gh.Issues.CreateComment(ctx, obs.Owner, obs.Repo, obs.Number, &gh.IssueComment{Body: gh.Ptr(body)}); err != nil { - return true, 0, err + return true, comments, err } store.NoteAdoptionMaintainer(key, obs.HeadSHA) + return true, comments + 1, nil +} + +// closeMaintainerSuperseder closes the hive's still-open superseding PR with +// a note pointing back at the maintainer's original. Reports whether it +// closed the PR and how many comments it posted; a merged or already-closed +// superseder is left alone. +func (c *Engine) closeMaintainerSuperseder(ctx context.Context, obs stageObservation, superseding int) (bool, int, error) { + sup, _, err := c.gh.PullRequests.Get(ctx, obs.Owner, obs.Repo, superseding) + if err != nil { + c.warn("stage labels: maintainer adoption superseder lookup failed", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding, "error", err) + return false, 0, err + } + if sup.GetMerged() || strings.EqualFold(sup.GetState(), "closed") { + return false, 0, nil + } + body := fmt.Sprintf("%s\nClosing: #%d is a repository maintainer's PR, which the hive does not take over. Fixes and merge stay with @%s there.", AdoptionMaintainerSupersederMarker, obs.Number, obs.Author) + if _, _, err := c.gh.Issues.CreateComment(ctx, obs.Owner, obs.Repo, superseding, &gh.IssueComment{Body: gh.Ptr(body)}); err != nil { + c.warn("stage labels: maintainer adoption superseder note failed", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding, "error", err) + return false, 0, err + } + if _, _, err := c.gh.PullRequests.Edit(ctx, obs.Owner, obs.Repo, superseding, &gh.PullRequest{State: gh.Ptr("closed")}); err != nil { + c.warn("stage labels: closing maintainer adoption superseder failed", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding, "error", err) + return false, 1, err + } + c.info("stage labels: closed superseder of maintainer PR", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding) return true, 1, nil } diff --git a/src/pkg/github/automerge/automerge_contributor_adopt_test.go b/src/pkg/github/automerge/automerge_contributor_adopt_test.go index 5b0dc9ced7..3d29c87d53 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt_test.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt_test.go @@ -1912,7 +1912,7 @@ func TestAdoptionAuthorPermission(t *testing.T) { } func TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption(t *testing.T) { - for _, mode := range []string{"fresh", "in-place", "supersede"} { + for _, mode := range []string{"fresh", "in-place", "supersede", "supersede-open"} { for _, permission := range []string{"admin", "maintain"} { t.Run(permission+"/"+mode, func(t *testing.T) { p := adoptPolicy() @@ -1928,9 +1928,13 @@ func TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption(t *testing.T) { labels := []string{"stage/072-returned-to-contributor", "role/contributor", "author/clanker", stagelabels.NoDCOLabel} if mode != "fresh" { h.e.stageStoreOrShared().MarkAdoptedInPlace(key, "aaa", AdoptReasonOffline, h.clock.at) - if mode == "supersede" { + switch mode { + case "supersede": h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) h.adopt.setSuperseder(90, "closed", true) + case "supersede-open": + h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) + h.adopt.setSuperseder(90, "open", false) } labels[0] = "stage/074-adopted-by-hive" } @@ -1939,20 +1943,37 @@ func TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption(t *testing.T) { h.api.reset() h.observe(adoptPR(7, "aaa", labels...), "check-failing:DCO") rec, _ := h.e.stageStoreOrShared().Get(key) - if rec.Stage != stagelabels.ReturnedToContributor || rec.AdoptionLive("aaa") || rec.AdoptedHead != "" { + if rec.Stage != stagelabels.ReturnedToContributor || rec.AdoptionLive("aaa") || rec.AdoptedHead != "" || rec.AdoptedBy != 0 { t.Fatalf("maintainer adopted: %+v", rec) } - notes := 0 + notes, supersederNotes := 0, 0 for _, comment := range h.api.comments { if strings.HasPrefix(comment, AdoptionMaintainerMarker) { notes++ } + if strings.HasPrefix(comment, AdoptionMaintainerSupersederMarker) { + supersederNotes++ + } } if notes != 1-i { t.Fatalf("pass %d: refusal notes=%d comments=%v", i, notes, h.api.comments) } - if len(adoptionComments(h.api)) != 0 || len(h.adopt.refs) != 0 || len(h.adopt.pulls) != 0 || len(h.adopt.prEdits) != 0 { - t.Fatalf("maintainer takeover: comments=%v refs=%v pulls=%v edits=%v", h.api.comments, h.adopt.refs, h.adopt.pulls, h.adopt.prEdits) + // An open hive superseder is closed with a note on the + // first pass only; the original PR is never edited. + wantEdits, wantSuperseder := []string(nil), 0 + if mode == "supersede-open" { + wantSuperseder = 1 + wantEdits = []string{"90:closed"} + if notes == 1 && !strings.Contains(h.api.comments[len(h.api.comments)-1], "#90 has been closed") { + t.Fatalf("refusal note does not name the closed superseder: %v", h.api.comments) + } + } + if supersederNotes != wantSuperseder || !slices.Equal(h.adopt.prEdits, wantEdits) { + t.Fatalf("pass %d: superseder notes=%d edits=%v comments=%v", i, supersederNotes, h.adopt.prEdits, h.api.comments) + } + h.adopt.prEdits = nil + if len(adoptionComments(h.api)) != 0 || len(h.adopt.refs) != 0 || len(h.adopt.pulls) != 0 { + t.Fatalf("maintainer takeover: comments=%v refs=%v pulls=%v", h.api.comments, h.adopt.refs, h.adopt.pulls) } } }) @@ -1960,6 +1981,59 @@ func TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption(t *testing.T) { } } +func TestMaintainerSupersederCloseFailureKeepsAdoption(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permission = "admin" + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + key := stageKey("acme", "widget", 7) + h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) + // No scripted superseder: GET pulls/90 is 404, so the release must wait. + h.clock.advance(adoptCheckInterval + time.Minute) + h.e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", adoptPR(7, "aaa", "stage/074-adopted-by-hive", "author/clanker"), "check-failing:unit", false, nil) + if _, err := h.e.ApplyStageLabels(context.Background()); err == nil { + t.Fatal("superseder lookup failure was not surfaced") + } + rec, _ := h.e.stageStoreOrShared().Get(key) + if rec.AdoptedHead != "aaa" || rec.AdoptedBy != 90 { + t.Fatalf("release orphaned the superseder: %+v", rec) + } + for _, comment := range h.api.comments { + if strings.HasPrefix(comment, AdoptionMaintainerMarker) { + t.Fatalf("refusal note posted while the superseder is still tracked: %v", h.api.comments) + } + } +} + +func TestMaintainerTerminalObservationPostsNoRefusal(t *testing.T) { + for _, tc := range []struct { + name string + merged bool + reason string + }{{name: "merged", merged: true}, {name: "closed", reason: "closed"}} { + t.Run(tc.name, func(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permission = "admin" + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + key := stageKey("acme", "widget", 7) + h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) + h.adopt.setSuperseder(90, "open", false) + h.api.reset() + h.e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", adoptPR(7, "aaa", "stage/074-adopted-by-hive", "author/clanker"), tc.reason, tc.merged, nil) + if _, err := h.e.ApplyStageLabels(context.Background()); err != nil { + t.Fatal(err) + } + if _, ok := h.e.stageStoreOrShared().Get(key); ok { + t.Fatal("terminal item still tracked") + } + if len(h.api.comments) != 0 || len(h.adopt.prEdits) != 0 || h.adopt.permissionReads != 0 { + t.Fatalf("terminal observation touched the maintainer's PR: comments=%v edits=%v permission reads=%d", h.api.comments, h.adopt.prEdits, h.adopt.permissionReads) + } + }) + } +} + func TestAdoptionPermissionFailureDefersWithoutAttempt(t *testing.T) { h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, adoptPolicy()) defer done() diff --git a/src/pkg/github/automerge/automerge_stage_labels.go b/src/pkg/github/automerge/automerge_stage_labels.go index 16757ea19d..7804522e8a 100644 --- a/src/pkg/github/automerge/automerge_stage_labels.go +++ b/src/pkg/github/automerge/automerge_stage_labels.go @@ -744,7 +744,9 @@ func (c *Engine) ApplyStageLabels(ctx context.Context) (StageLabelResult, error) obs.Reason = verdict.Reason } prevRec, hadPrev := store.Get(key) - if hadPrev && (prevRec.AdoptedInPlace() || prevRec.AdoptedHead != "") { + // A merged/closed/gone PR is forgotten below; a maintainer who + // finished their own adopted PR must not get the refusal note. + if hadPrev && !verdict.Terminal && (prevRec.AdoptedInPlace() || prevRec.AdoptedHead != "") { _, n, err := c.guardAdoption(ctx, store, key, obs, now) result.Comments += n if err != nil && firstErr == nil { From 5f41d11aa7dcbdb00a16fc3fcc63fe449ab8c7b7 Mon Sep 17 00:00:00 2001 From: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Sun, 25 Oct 2026 19:47:25 -0400 Subject: [PATCH 4/7] =?UTF-8?q?=F0=9F=90=9B=20fix(adoption):=20make=20the?= =?UTF-8?q?=20maintainer=20refusal=20marker=20distinct=20from=20the=20supe?= =?UTF-8?q?rseder=20note?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- src/pkg/github/automerge/automerge_contributor_adopt.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/pkg/github/automerge/automerge_contributor_adopt.go b/src/pkg/github/automerge/automerge_contributor_adopt.go index a660f627c6..4ed52c67cd 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt.go @@ -541,7 +541,9 @@ func shortSHA(sha string) string { // AdoptionMaintainerMarker identifies the once-per-head refusal to take over a // repository maintainer's PR, independently of author kind or relay presence. -const AdoptionMaintainerMarker = "` comment with the lane's last skip reason | 30m | | `stage/085-needs-decision` | PR | **PR-only** since the owner decision of 2026-10-17 (issues park at `stage/045-needs-decision`; before that this stage carried both, [#12683](https://github.com/hivecommons/hive/issues/12683)). A person owes a **decision**, not a merge: a `requires_human` swarm verdict (reason `reviewer-verdict:requires_human`), `sentinel-alert`, `hold` / `do-not-merge*` / `needs-decision` modifier, no reviewer configured, GitHub workflow approval is required for a trusted same-repo bot/App/clanker PR (`workflow-approval-required:`); Hive has posted `/ok-to-test` and is waiting for the repo-local `hive-ok-to-test` workflow or a maintainer, or a human-only lane rule unrelated to trust (workflow-path edits, tests removed, human-merge path, reporter-trust hold). A trusted human's normal GitHub **Approve** review on the exact current head clears a `requires_human` verdict for any author kind/lane and records `reviewer-verdict:requires_human:human-approved@`; a new head invalidates it. The same approval also clears the merge-lane review gate over any non-approved swarm outcome, a perspective's `changes_requested` included (the `review_evaluated` audit line records `human_approver` and `overrode`); without it, `changes_requested` / `reject` still require fixes or manual merge. Under `review.self_authored_decision_policy: resolve`, a Hive-authored PR's reviewer decision question parks here only when it matches a named risk class (`review.DecisionRiskClasses`: permissions/credentials, security, sensitive paths, data loss/irreversible, merge/trust policy, external side effects, licensing/legal, unverifiable) or carries ``; any other question is resolved by policy and recorded as an advisory reason on the approving aggregate. Hive-authored non-risk incomplete-fix reviews with concrete remediation are normalized to `changes_requested` at verdict load/aggregation time and by the stage-label detail selector, even when the finding was marked high severity; duplicate summary/changelog intent-alignment cleanup with a concrete trim/reword fix is included. Those PRs go to `stage/070-fixing`, not here. On a hive-authored PR the trusted human can also answer with a **changes requested** review on the head, or `/hive decision `, which moves the PR to `stage/070-fixing` and kicks the author agent with the text verbatim ([#13041](https://github.com/hivecommons/hive/issues/13041)); `/hive decision` on a PR the hive did not open gets one reply naming the review-side equivalents. One `` comment on entry names what is owed, and for a hive-authored `requires_human` park names the two actions: approve the head to merge as-is, or request changes and the hive pushes the fix | none | | `stage/090-ready-for-human-merge` | PR | the hive would merge this itself but trust/role blocks it: reviewer verdict approve at the **current** head, required checks green (tide excluded), DCO yes, no sentinel alert, no conflicts, soak elapsed, no hold — and the only remaining lane reason is the author's tier/role. A maintainer's `/lgtm` `/approve` is the only missing step. One `` comment on entry carries the evidence. Always read with the `role/*` and `author/*` labels. (Was `stage/090-needs-human` before [#12528](https://github.com/hivecommons/hive/issues/12528); the engine renames the GitHub label in place) | none | diff --git a/src/pkg/github/automerge/automerge_adopt_maintainer_test.go b/src/pkg/github/automerge/automerge_adopt_maintainer_test.go new file mode 100644 index 0000000000..1f0a55c971 --- /dev/null +++ b/src/pkg/github/automerge/automerge_adopt_maintainer_test.go @@ -0,0 +1,333 @@ +package automerge + +import ( + "context" + "slices" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/hivecommons/hive/pkg/delegatedecision" + "github.com/hivecommons/hive/pkg/stagelabels" +) + +// projectbluefin/utah-packages#619: the author was a repository admin. The +// hive nudged the PR at 072, adopted it as contributor-offline 16 minutes +// later and then re-homed it, closing the admin's own PR. Adoption stands in +// for a quiet contributor relay, never for a maintainer. + +func TestRepoMaintainerLevel(t *testing.T) { + for _, tc := range []struct { + permission, role string + want bool + }{ + {"admin", "admin", true}, + {"write", "maintain", true}, + {"ADMIN", "", true}, + {"write", "write", false}, + {"write", "triage", false}, + {"read", "read", false}, + {"none", "", false}, + } { + if got := repoMaintainerLevel(tc.permission, tc.role); got != tc.want { + t.Errorf("repoMaintainerLevel(%q, %q) = %v, want %v", tc.permission, tc.role, got, tc.want) + } + } +} + +func TestAdoptionSkipsRepositoryMaintainer(t *testing.T) { + for _, role := range []string{"admin", "maintain"} { + t.Run(role, func(t *testing.T) { + p := adoptPolicy() + p.AdoptInPlace = true + h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, p) + defer done() + h.adopt.permissions = map[string]string{"mallory": role} + h.adopt.allowMaintainerEdits(7, true) + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + labels := []string{"stage/072-returned-to-contributor", "role/contributor", "author/clanker"} + // Past the grace, then two more sweeps inside one cache window. + for _, step := range []time.Duration{2 * time.Hour, 3 * time.Minute, 3 * time.Minute} { + h.clock.advance(step) + h.observe(adoptPR(7, "aaa", labels...), "check-failing:unit") + } + if n := adoptionComments(h.api); len(n) != 0 || len(h.adopt.pulls) != 0 || len(h.adopt.refs) != 0 { + t.Fatalf("maintainer's PR adopted: comments=%v pulls=%v refs=%v", n, h.adopt.pulls, h.adopt.refs) + } + rec, _ := h.e.stageStoreOrShared().Get(stageKey("acme", "widget", 7)) + if rec.Stage != stagelabels.ReturnedToContributor || rec.AdoptedHead != "" { + t.Fatalf("record: %+v", rec) + } + if h.adopt.permissionReads != 1 { + t.Fatalf("permission read %d times, want 1 (cached)", h.adopt.permissionReads) + } + }) + } +} + +func TestAdoptionFailsClosedOnPermissionError(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, adoptPolicy()) + defer done() + h.adopt.failPermission = true + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + h.clock.advance(2 * time.Hour) + h.observe(adoptPR(7, "aaa", "stage/072-returned-to-contributor", "role/contributor", "author/clanker"), "check-failing:unit") + if len(adoptionComments(h.api)) != 0 || len(h.adopt.pulls) != 0 || h.adopt.refAttempts != 0 { + t.Fatalf("adopted although the permission read failed: %v", h.api.comments) + } + // The error is not cached: once the read works, a write-level author + // is adopted as before. + h.adopt.failPermission = false + h.clock.advance(time.Minute) + h.observe(adoptPR(7, "aaa", "stage/072-returned-to-contributor", "role/contributor", "author/clanker"), "check-failing:unit") + if len(adoptionComments(h.api)) != 1 { + t.Fatalf("write-level author not adopted after the read recovered: %v", h.api.comments) + } +} + +// An in-place adoption taken before the author was known to maintain the +// repository is released on the next pass and never re-homed. +func TestInPlaceAdoptionOfMaintainerIsReleased(t *testing.T) { + p := adoptPolicy() + p.AdoptInPlace = true + h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, p) + defer done() + h.adopt.allowMaintainerEdits(7, true) + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + labels := []string{"stage/072-returned-to-contributor", "role/contributor", "author/clanker"} + h.clock.advance(2 * time.Hour) + h.observe(adoptPR(7, "aaa", labels...), "check-failing:unit") + if rec, _ := h.e.stageStoreOrShared().Get(stageKey("acme", "widget", 7)); !rec.AdoptedInPlace() { + t.Fatalf("setup: not adopted in place: %+v", rec) + } + + // The author is (now) an admin; the cached write-level answer expires. + h.adopt.permissions = map[string]string{"mallory": "admin"} + h.clock.advance(maintainerPermTTL + time.Minute) + h.api.reset() + h.observe(adoptPR(7, "aaa", "stage/074-adopted-by-hive", "role/contributor", "author/clanker"), "check-failing:unit") + rec, _ := h.e.stageStoreOrShared().Get(stageKey("acme", "widget", 7)) + if rec.AdoptedHead != "" || rec.Stage == stagelabels.AdoptedByHive { + t.Fatalf("maintainer's in-place adoption kept: %+v", rec) + } + if len(h.adopt.pulls) != 0 || len(h.adopt.refs) != 0 { + t.Fatalf("maintainer's PR re-homed: pulls=%v refs=%v", h.adopt.pulls, h.adopt.refs) + } +} + +// The permission cache is keyed by repository and author, expires after +// maintainerPermTTL, and never caches a failed read. +func TestMaintainerPermissionCache(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permissions = map[string]string{"mallory": "admin"} + ctx := context.Background() + obs := stageObservation{Owner: "acme", Repo: "widget", Author: "mallory", Number: 7} + for i := 0; i < 2; i++ { + if got, err := h.e.authorMaintainsRepo(ctx, obs, h.clock.at); !got || err != nil { + t.Fatalf("maintainer=%v err=%v", got, err) + } + obs.Number++ // A second PR by the same author uses the cached read. + } + if h.adopt.permissionReads != 1 { + t.Fatalf("reads=%d, want 1", h.adopt.permissionReads) + } + h.clock.advance(maintainerPermTTL) + _, _ = h.e.authorMaintainsRepo(ctx, obs, h.clock.at) + if h.adopt.permissionReads != 2 { + t.Fatalf("expired cache reads=%d, want 2", h.adopt.permissionReads) + } + other := obs + other.Author = "other" + if got, _ := h.e.authorMaintainsRepo(ctx, other, h.clock.at); got { + t.Fatal("a write-level author shares the maintainer's cache entry") + } + other = obs + other.Repo = "gadget" + _, _ = h.e.authorMaintainsRepo(ctx, other, h.clock.at) + if h.adopt.permissionReads != 4 { + t.Fatalf("cache failed to distinguish repo/author: reads=%d", h.adopt.permissionReads) + } + h.adopt.failPermission = true + other.Author = "eve" + for i := 0; i < 2; i++ { + if got, err := h.e.authorMaintainsRepo(ctx, other, h.clock.at); got || err == nil { + t.Fatalf("failed read: maintainer=%v err=%v", got, err) + } + } + if h.adopt.permissionReads != 6 { + t.Fatalf("failed read cached: reads=%d, want 6", h.adopt.permissionReads) + } +} + +// hivecommons/hive#14313: the cache mutex guards map access only; it is not +// held while GetPermissionLevel is in flight. +func TestMaintainerPermissionLockNotHeldAcrossAPICall(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + var held atomic.Bool + h.adopt.onPermission = func() { + if !h.e.maintainerPermMu.TryLock() { + held.Store(true) + return + } + h.e.maintainerPermMu.Unlock() + } + obs := stageObservation{Owner: "acme", Repo: "widget", Author: "mallory", Number: 7} + if _, err := h.e.authorMaintainsRepo(context.Background(), obs, h.clock.at); err != nil { + t.Fatal(err) + } + if h.adopt.permissionReads != 1 || held.Load() { + t.Fatalf("reads=%d lock held across the API call=%v", h.adopt.permissionReads, held.Load()) + } +} + +// An unreadable permission defers: a live adoption, in place or superseded, +// is neither released nor re-homed until the read succeeds. +func TestPermissionUnreadableKeepsLiveAdoption(t *testing.T) { + t.Run("in-place", func(t *testing.T) { + p := adoptPolicy() + p.AdoptInPlace = true + h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, p) + defer done() + h.adopt.allowMaintainerEdits(7, true) + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + h.clock.advance(2 * time.Hour) + h.observe(adoptPR(7, "aaa", "stage/072-returned-to-contributor", "role/contributor", "author/clanker"), "check-failing:unit") + key := stageKey("acme", "widget", 7) + before, _ := h.e.stageStoreOrShared().Get(key) + if !before.AdoptedInPlace() { + t.Fatalf("setup: not adopted in place: %+v", before) + } + h.adopt.failPermission = true + h.clock.advance(maintainerPermTTL + time.Minute) + h.observe(adoptPR(7, "aaa", "stage/074-adopted-by-hive", "role/contributor", "author/clanker"), "check-failing:unit") + rec, _ := h.e.stageStoreOrShared().Get(key) + if !rec.AdoptedInPlace() || rec.AdoptedHead != before.AdoptedHead || rec.AdoptedBy != 0 { + t.Fatalf("unreadable permission changed the adoption: %+v", rec) + } + if len(h.adopt.pulls) != 0 || h.adopt.refAttempts != 0 { + t.Fatalf("unreadable permission re-homed: pulls=%v refs=%d", h.adopt.pulls, h.adopt.refAttempts) + } + }) + t.Run("supersede", func(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + key := stageKey("acme", "widget", 7) + h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) + h.adopt.setSuperseder(90, "open", false) + h.adopt.failPermission = true + h.clock.advance(adoptCheckInterval + time.Minute) + h.observe(adoptPR(7, "aaa", "stage/074-adopted-by-hive", "author/clanker"), "check-failing:unit") + rec, _ := h.e.stageStoreOrShared().Get(key) + if rec.AdoptedHead != "aaa" || rec.AdoptedBy != 90 || rec.AdoptMode != stagelabels.AdoptModeSupersede { + t.Fatalf("unreadable permission released a live supersede adoption: %+v", rec) + } + if len(h.adopt.prEdits) != 0 || len(h.adopt.pulls) != 0 || h.adopt.refAttempts != 0 { + t.Fatalf("unreadable permission mutated the repository: edits=%v pulls=%v refs=%d", h.adopt.prEdits, h.adopt.pulls, h.adopt.refAttempts) + } + }) +} + +// hivecommons/hive#14311: releasing a maintainer's supersede adoption closes +// a still-open hive superseder with a note first; a merged or closed one is +// left alone. The maintainer's own PR is never edited. +func TestMaintainerAdoptionReleaseClosesOpenSuperseder(t *testing.T) { + for _, tc := range []struct { + name string + state string + merged bool + wantEdits []string + wantSupNotes int + }{ + {name: "open", state: "open", wantEdits: []string{"90:closed"}, wantSupNotes: 1}, + {name: "closed", state: "closed"}, + {name: "merged", state: "closed", merged: true}, + } { + t.Run(tc.name, func(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permissions = map[string]string{"mallory": "admin"} + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + key := stageKey("acme", "widget", 7) + h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) + h.adopt.setSuperseder(90, tc.state, tc.merged) + labels := []string{"stage/074-adopted-by-hive", "role/contributor", "author/clanker"} + for pass := 0; pass < 2; pass++ { + h.api.reset() + h.adopt.prEdits = nil + h.clock.advance(time.Minute) + h.observe(adoptPR(7, "aaa", labels...), "check-failing:unit") + supNotes := 0 + for _, c := range h.api.comments { + if strings.HasPrefix(c, AdoptionMaintainerSupersederMarker) { + supNotes++ + } + } + wantEdits, wantNotes := tc.wantEdits, tc.wantSupNotes + if pass > 0 { + wantEdits, wantNotes = nil, 0 + } + if supNotes != wantNotes || !slices.Equal(h.adopt.prEdits, wantEdits) { + t.Fatalf("pass %d: superseder notes=%d edits=%v comments=%v", pass, supNotes, h.adopt.prEdits, h.api.comments) + } + rec, _ := h.e.stageStoreOrShared().Get(key) + if rec.AdoptedHead != "" || rec.AdoptedBy != 0 { + t.Fatalf("pass %d: maintainer adoption kept: %+v", pass, rec) + } + if pass > 0 && rec.Stage == stagelabels.AdoptedByHive { + t.Fatalf("maintainer's PR still at 074: %+v", rec) + } + if len(adoptionComments(h.api)) != 0 || len(h.adopt.pulls) != 0 || len(h.adopt.refs) != 0 { + t.Fatalf("maintainer takeover: comments=%v pulls=%v refs=%v", h.api.comments, h.adopt.pulls, h.adopt.refs) + } + } + }) + } +} + +func TestMaintainerSupersederCloseFailureKeepsAdoption(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permissions = map[string]string{"mallory": "admin"} + h.observe(adoptPR(7, "aaa"), "check-failing:unit") + key := stageKey("acme", "widget", 7) + h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) + // No scripted superseder: GET pulls/90 is 404, so the release must wait. + h.clock.advance(adoptCheckInterval + time.Minute) + h.e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", adoptPR(7, "aaa", "stage/074-adopted-by-hive", "author/clanker"), "check-failing:unit", false, nil) + if _, err := h.e.ApplyStageLabels(context.Background()); err == nil { + t.Fatal("superseder lookup failure was not surfaced") + } + rec, _ := h.e.stageStoreOrShared().Get(key) + if rec.AdoptedHead != "aaa" || rec.AdoptedBy != 90 { + t.Fatalf("release orphaned the superseder: %+v", rec) + } +} + +func TestSupersedeMaintainerRefusesBeforeMutation(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permissions = map[string]string{"mallory": "admin"} + obs := stageObservation{Owner: "acme", Repo: "widget", Author: "mallory", Number: 7, HeadSHA: "aaa", BaseRef: "v5"} + if _, err := h.e.supersedeAdoptedPR(context.Background(), h.e.stageStoreOrShared(), stageKey("acme", "widget", 7), obs, stagelabels.Record{}, adoptPolicy(), AdoptReasonDCORehome, 0, false, true, h.clock.at); err != nil { + t.Fatal(err) + } + if h.adopt.refAttempts != 0 || len(h.adopt.pulls) != 0 || len(h.adopt.prEdits) != 0 { + t.Fatal("maintainer supersession mutated repository") + } +} + +func TestDelegatedSupersedeCannotTakeOverMaintainer(t *testing.T) { + h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) + defer done() + h.adopt.permissions = map[string]string{"mallory": "maintain"} + labels := []string{"stage/085-needs-decision", delegatedecision.LabelAdoptSupersede} + h.observe(adoptPR(7, "aaa", labels...), contributorLaneReasonNoReviewer) + rec, _ := h.e.stageStoreOrShared().Get(stageKey("acme", "widget", 7)) + if rec.AdoptedHead != "" || h.adopt.refAttempts != 0 || len(h.adopt.pulls) != 0 { + t.Fatalf("delegated request adopted maintainer: %+v", rec) + } +} diff --git a/src/pkg/github/automerge/automerge_contributor_adopt.go b/src/pkg/github/automerge/automerge_contributor_adopt.go index 4ed52c67cd..24bb5eabd9 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt.go @@ -539,131 +539,6 @@ func shortSHA(sha string) string { return sha } -// AdoptionMaintainerMarker identifies the once-per-head refusal to take over a -// repository maintainer's PR, independently of author kind or relay presence. -// It is deliberately not a prefix of AdoptionMaintainerSupersederMarker so a -// prefix match on either never counts the other note. -const AdoptionMaintainerMarker = "" - -// guardAdoption releases ownership before any further adoption side effects. -// Unreadable permission blocks the pass without changing the record: a fresh -// takeover is not safe without evidence, and releasing a live adoption on -// none would orphan its superseding PR (AdoptedBy) and let the original be -// adopted a second time once the error ages out of the cache. A maintainer's -// live supersede adoption closes its still-open superseder first, so the -// release never leaves a hive duplicate of the maintainer's PR mergeable. -func (c *Engine) guardAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, now time.Time) (bool, int, error) { - maintainer, err := c.adoptionAuthorPermission(ctx, obs, now) - if !maintainer && err == nil { - return false, 0, nil - } - if err != nil { - c.warn("stage labels: adoption deferred, author permission unreadable", "repo", obs.Display, "pr", obs.Number, "error", err) - return true, 0, err - } - comments := 0 - closedSuperseder := 0 - if rec, ok := store.Get(key); ok && rec.AdoptedBy > 0 { - closed, n, err := c.closeMaintainerSuperseder(ctx, obs, rec.AdoptedBy) - comments += n - if err != nil { - // Keep the record: the superseder is still tracked and the - // close retries on the next sweep instead of orphaning it. - return true, comments, err - } - if closed { - closedSuperseder = rec.AdoptedBy - } - } - store.ClearAdoption(key) - rec, ok := store.Get(key) - if !ok || obs.HeadSHA == "" || rec.AdoptionMaintainerNotedHead == obs.HeadSHA { - return true, comments, nil - } - released := "Any live adoption has been released" - if closedSuperseder > 0 { - released = fmt.Sprintf("The hive's superseding PR #%d has been closed and the adoption released", closedSuperseder) - } - body := fmt.Sprintf("%s head=%s -->\n@%s the hive will not take over a repository maintainer's PR. %s; this PR stays with you for fixes and merge.", AdoptionMaintainerMarker, obs.HeadSHA, obs.Author, released) - if _, _, err := c.gh.Issues.CreateComment(ctx, obs.Owner, obs.Repo, obs.Number, &gh.IssueComment{Body: gh.Ptr(body)}); err != nil { - return true, comments, err - } - store.NoteAdoptionMaintainer(key, obs.HeadSHA) - return true, comments + 1, nil -} - -// closeMaintainerSuperseder closes the hive's still-open superseding PR with -// a note pointing back at the maintainer's original. Reports whether it -// closed the PR and how many comments it posted; a merged or already-closed -// superseder is left alone. -func (c *Engine) closeMaintainerSuperseder(ctx context.Context, obs stageObservation, superseding int) (bool, int, error) { - sup, _, err := c.gh.PullRequests.Get(ctx, obs.Owner, obs.Repo, superseding) - if err != nil { - c.warn("stage labels: maintainer adoption superseder lookup failed", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding, "error", err) - return false, 0, err - } - if sup.GetMerged() || strings.EqualFold(sup.GetState(), "closed") { - return false, 0, nil - } - body := fmt.Sprintf("%s\nClosing: #%d is a repository maintainer's PR, which the hive does not take over. Fixes and merge stay with @%s there.", AdoptionMaintainerSupersederMarker, obs.Number, obs.Author) - if _, _, err := c.gh.Issues.CreateComment(ctx, obs.Owner, obs.Repo, superseding, &gh.IssueComment{Body: gh.Ptr(body)}); err != nil { - c.warn("stage labels: maintainer adoption superseder note failed", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding, "error", err) - return false, 0, err - } - if _, _, err := c.gh.PullRequests.Edit(ctx, obs.Owner, obs.Repo, superseding, &gh.PullRequest{State: gh.Ptr("closed")}); err != nil { - c.warn("stage labels: closing maintainer adoption superseder failed", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding, "error", err) - return false, 1, err - } - c.info("stage labels: closed superseder of maintainer PR", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding) - return true, 1, nil -} - // adoptContributorPR runs the stage-074 check for one clanker PR in stage // 072 and, when adoption is due, performs it: in place (the hive's fix lane // pushes to the PR's own branch) when GitHub permits and the policy allows, @@ -689,8 +564,9 @@ func (c *Engine) adoptContributorPR(ctx context.Context, store *stagelabels.Stor c.info("stage labels: adoption due but blocked by label", "repo", obs.Display, "pr", obs.Number, "label", l) return 0, nil } - if blocked, n, err := c.guardAdoption(ctx, store, key, obs, now); blocked { - return n, err + if maintainer, err := c.authorMaintainsRepo(ctx, obs, now); maintainer || err != nil { + c.info("stage labels: adoption due but the author maintains the repository; not adopting", "repo", obs.Display, "pr", obs.Number, "author", obs.Author, "deferred", err != nil) + return 0, nil } fence, err := c.adoptionDiffRefusal(ctx, obs) if fence != "" && (err != nil || adoptionFenceDefers(fence)) { @@ -767,8 +643,15 @@ func delegatedAdoptionAttempted(before, after stagelabels.Record) bool { } func (c *Engine) supersedeAdoptedPR(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, rec stagelabels.Record, policy StageLabelPolicy, reason string, grace time.Duration, online bool, dcoRehome bool, now time.Time) (int, error) { - if blocked, n, err := c.guardAdoption(ctx, store, key, obs, now); blocked { - return n, err + maintainer, err := c.authorMaintainsRepo(ctx, obs, now) + if err != nil { + return 0, nil + } + if maintainer { + // Backstop for every re-home path (dco-rehome, in-place-stalled): + // a maintainer's PR is never closed in favour of a hive copy. + c.info("stage labels: not superseding a repository maintainer's PR; adoption released", "repo", obs.Display, "pr", obs.Number, "author", obs.Author, "reason", reason) + return c.releaseMaintainerAdoption(ctx, store, key, obs, rec.AdoptedBy) } owner, repo, number := obs.Owner, obs.Repo, obs.Number rec.AdoptReason = reason @@ -860,13 +743,21 @@ func (c *Engine) supersedeAdoptedPR(ctx context.Context, store *stagelabels.Stor // most every adoptCheckInterval): merged → close the original with a note; // closed unmerged → forget the adoption so the original returns to 072. func (c *Engine) settleAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, policy StageLabelPolicy, now time.Time) (int, error) { - if blocked, n, err := c.guardAdoption(ctx, store, key, obs, now); blocked { - return n, err - } rec, ok := store.Get(key) if !ok { return 0, nil } + maintainer, err := c.authorMaintainsRepo(ctx, obs, now) + if err != nil { + // No evidence either way: neither re-home nor release this pass. + return 0, nil + } + if maintainer { + // Supersede adoption taken before this rule, or before the + // author's permission was raised: hand the PR back. + c.info("stage labels: adoption of a repository maintainer's PR released", "repo", obs.Display, "pr", obs.Number, "author", obs.Author, "superseded_by", rec.AdoptedBy) + return c.releaseMaintainerAdoption(ctx, store, key, obs, rec.AdoptedBy) + } if adoptedHeadDCORed(obs, rec) { canPushInPlace := c.canPushToPRBranch(ctx, obs) if adoptedHeadNeedsDCORehome(obs, rec, canPushInPlace) { @@ -941,6 +832,108 @@ func (c *Engine) settleAdoption(ctx context.Context, store *stagelabels.Store, k return 0, nil } +// maintainerPermTTL is how long authorMaintainsRepo trusts a permission +// read: one GET per (repo, author) per sweep window, not per PR per sweep. +const maintainerPermTTL = 10 * time.Minute + +type maintainerPermEntry struct { + maintainer bool + at time.Time +} + +// repoMaintainerLevel reports whether a collaborator permission answer +// (`permission`, and the finer `role_name`) is admin or maintain. +func repoMaintainerLevel(permission, role string) bool { + permission, role = strings.ToLower(strings.TrimSpace(permission)), strings.ToLower(strings.TrimSpace(role)) + return permission == "admin" || role == "admin" || role == "maintain" +} + +// authorMaintainsRepo reports whether the PR author holds admin or maintain +// on the PR's repository. Adoption stands in for a contributor's relay that +// went quiet; a maintainer owns the branch and merges it themselves, so the +// hive never takes their PR over (projectbluefin/utah-packages#619, adopted +// as contributor-offline 16 minutes after its nudge and re-homed). A failed +// read is not cached and returns its error: callers defer, so the hive +// neither takes a PR over nor releases an existing adoption without +// evidence. The cache lock covers map access only, never the API call. +func (c *Engine) authorMaintainsRepo(ctx context.Context, obs stageObservation, now time.Time) (bool, error) { + if c == nil || c.gh == nil || obs.Author == "" || obs.Owner == "" || obs.Repo == "" { + return false, nil + } + key := strings.ToLower(obs.Owner + "/" + obs.Repo + "#" + obs.Author) + c.maintainerPermMu.Lock() + if e, ok := c.maintainerPerm[key]; ok && now.Sub(e.at) < maintainerPermTTL { + c.maintainerPermMu.Unlock() + return e.maintainer, nil + } + c.maintainerPermMu.Unlock() + level, _, err := c.gh.Repositories.GetPermissionLevel(ctx, obs.Owner, obs.Repo, obs.Author) + if err != nil { + c.warn("stage labels: could not read the author's repository permission; adoption deferred", "repo", obs.Display, "pr", obs.Number, "author", obs.Author, "error", err) + return false, err + } + maintainer := repoMaintainerLevel(level.GetPermission(), level.GetRoleName()) + c.maintainerPermMu.Lock() + if c.maintainerPerm == nil { + c.maintainerPerm = map[string]maintainerPermEntry{} + } + for k, e := range c.maintainerPerm { + if now.Sub(e.at) >= maintainerPermTTL { + delete(c.maintainerPerm, k) + } + } + c.maintainerPerm[key] = maintainerPermEntry{maintainer: maintainer, at: now} + c.maintainerPermMu.Unlock() + return maintainer, nil +} + +// AdoptionMaintainerSupersederMarker is stamped on the note that closes the +// hive's superseding PR when the original turns out to be a maintainer's. +const AdoptionMaintainerSupersederMarker = "" + +// releaseMaintainerAdoption hands a maintainer's PR back. A still-open hive +// superseder is closed with a note first, so the release never leaves an +// untracked hive duplicate of the maintainer's PR mergeable; if that close +// fails the record (and its AdoptedBy) is kept and the next pass retries. +// Returns the number of comments posted. +func (c *Engine) releaseMaintainerAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, superseding int) (int, error) { + comments := 0 + if superseding > 0 { + n, err := c.closeMaintainerSuperseder(ctx, obs, superseding) + comments += n + if err != nil { + return comments, err + } + } + store.ClearAdoption(key) + return comments, nil +} + +// closeMaintainerSuperseder closes the hive's still-open superseding PR with +// a note pointing back at the maintainer's original; a merged or +// already-closed superseder is left alone. Returns the comments posted. +func (c *Engine) closeMaintainerSuperseder(ctx context.Context, obs stageObservation, superseding int) (int, error) { + sup, _, err := c.gh.PullRequests.Get(ctx, obs.Owner, obs.Repo, superseding) + if err != nil { + c.warn("stage labels: maintainer adoption superseder lookup failed", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding, "error", err) + return 0, err + } + if sup.GetMerged() || strings.EqualFold(sup.GetState(), "closed") { + return 0, nil + } + body := fmt.Sprintf("%s\nClosing: #%d is a repository maintainer's PR, which the hive does not take over. Fixes and merge stay with @%s there.", AdoptionMaintainerSupersederMarker, obs.Number, obs.Author) + if _, _, err := c.gh.Issues.CreateComment(ctx, obs.Owner, obs.Repo, superseding, &gh.IssueComment{Body: gh.Ptr(body)}); err != nil { + c.warn("stage labels: maintainer adoption superseder note failed", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding, "error", err) + return 0, err + } + if _, _, err := c.gh.PullRequests.Edit(ctx, obs.Owner, obs.Repo, superseding, &gh.PullRequest{State: gh.Ptr("closed")}); err != nil { + c.warn("stage labels: closing maintainer adoption superseder failed", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding, "error", err) + return 1, err + } + c.info("stage labels: closed superseder of maintainer PR", "repo", obs.Display, "pr", obs.Number, "superseded_by", superseding) + return 1, nil +} + // canPushToPRBranch reports whether the App can push to the PR's head: a // head in the base repo always; a fork head only with "allow edits by // maintainers", which the list payload omits, so one GET resolves it. @@ -1019,6 +1012,19 @@ func (c *Engine) baseSyncTouchesWorkflows(ctx context.Context, obs stageObservat // re-arms. // Returns the number of comments posted. func (c *Engine) settleInPlaceAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, prev stagelabels.Record, policy StageLabelPolicy) (int, error) { + if prev.AdoptedInPlace() { + maintainer, err := c.authorMaintainsRepo(ctx, obs, c.now()) + if err != nil { + // No evidence either way: keep the adoption as it is. + return 0, nil + } + if maintainer { + // Taken over before this rule, or before the author's permission + // was raised: hand the PR back rather than keep or re-home it. + c.info("stage labels: in-place adoption of a repository maintainer's PR released", "repo", obs.Display, "pr", obs.Number, "author", obs.Author) + return c.releaseMaintainerAdoption(ctx, store, key, obs, prev.AdoptedBy) + } + } if !prev.AdoptedInPlace() || obs.HeadSHA == "" || obs.HeadSHA == prev.AdoptedHead { return 0, nil } @@ -1247,9 +1253,6 @@ func adoptionStalledReason(obs stageObservation, rec stagelabels.Record, policy // blocker clears (hivecommons/hive#13040). Returns the decision and the // number of comments posted. func (c *Engine) escapeStalledAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, policy StageLabelPolicy, now time.Time) (adoptionEscape, int, error) { - if blocked, n, err := c.guardAdoption(ctx, store, key, obs, now); blocked { - return adoptionEscape{}, n, err - } rec, ok := store.Get(key) if !ok { return adoptionEscape{}, 0, nil @@ -1277,6 +1280,12 @@ func (c *Engine) escapeStalledAdoption(ctx context.Context, store *stagelabels.S var firstErr error reason := rehomeReason if reason != "" { + // Read the author's permission before the attempt is recorded: an + // unreadable answer leaves the in-place adoption untouched, and a + // maintainer's adoption is released by settleInPlaceAdoption. + if maintainer, err := c.authorMaintainsRepo(ctx, obs, now); maintainer || err != nil { + return adoptionEscape{}, 0, nil + } // Re-read the current diff before copying a head. A concurrent push // or incomplete evidence defers the write; merge fences travel with it. fence, err := c.adoptionDiffRefusal(ctx, obs) diff --git a/src/pkg/github/automerge/automerge_contributor_adopt_test.go b/src/pkg/github/automerge/automerge_contributor_adopt_test.go index 6417fec4a5..0c4513eb5f 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt_test.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt_test.go @@ -18,18 +18,14 @@ import ( // adoptAPI layers the git-ref / pull-request routes the adoption needs on // top of stageAPI and records what the hive did. type adoptAPI struct { - permission string - roleName string - permissionStatus int - permissionReads int - mu sync.Mutex - refs map[string]string // refs/heads/x → sha - refUpdates []string - pulls []map[string]any - nextPR int - superseders map[int]map[string]any // number → GET payload - prEdits []string - commits map[string]adoptCommit + mu sync.Mutex + refs map[string]string // refs/heads/x → sha + refUpdates []string + pulls []map[string]any + nextPR int + superseders map[int]map[string]any // number → GET payload + prEdits []string + commits map[string]adoptCommit // failRefs makes POST git/refs answer 500; refAttempts counts them. failRefs bool refAttempts int @@ -50,6 +46,13 @@ type adoptAPI struct { // changedFiles is the changed_files count GET pulls/n reports for an // observed PR; 0 omits it. changedFiles int + // permissions is the role GET collaborators//permission reports + // per login (default "write"); failPermission makes it answer 500; + // permissionReads counts the calls; onPermission runs inside each read. + permissions map[string]string + failPermission bool + permissionReads int + onPermission func() } type adoptCommit struct { @@ -68,15 +71,21 @@ func (a *adoptAPI) handle(w http.ResponseWriter, r *http.Request) bool { switch { case r.Method == http.MethodGet && strings.Contains(path, "/collaborators/") && strings.HasSuffix(path, "/permission"): a.permissionReads++ - if a.permissionStatus != 0 { - http.Error(w, "permission lookup failed", a.permissionStatus) + if a.onPermission != nil { + a.onPermission() + } + if a.failPermission { + w.WriteHeader(http.StatusInternalServerError) + _ = json.NewEncoder(w).Encode(map[string]string{"message": "scripted failure"}) return true } - permission := a.permission - if permission == "" { - permission = "write" + login := strings.TrimSuffix(path[strings.Index(path, "/collaborators/")+len("/collaborators/"):], "/permission") + role := nonEmpty(a.permissions[login], "write") + permission := role + if role == "maintain" { + permission = "write" // GitHub's legacy field; role_name carries maintain } - _ = json.NewEncoder(w).Encode(map[string]string{"permission": permission, "role_name": a.roleName}) + _ = json.NewEncoder(w).Encode(map[string]any{"permission": permission, "role_name": role, "user": map[string]string{"login": login}}) return true case r.Method == http.MethodGet && strings.HasPrefix(path, "/repos/acme/widget/pulls/") && strings.HasSuffix(path, "/files"): if a.failPRFiles { @@ -1856,7 +1865,7 @@ func TestSupersedeRevalidatesBeforeRefMutation(t *testing.T) { h.adopt.heads[7] = "aaa" h.adopt.refs["refs/heads/hive/adopt-7"] = "old" tc.prepare(h.adopt) - obs := stageObservation{Owner: "acme", Repo: "widget", Display: "acme/widget", Author: "mallory", Number: 7, HeadSHA: "aaa", BaseRef: "v5"} + obs := stageObservation{Owner: "acme", Repo: "widget", Display: "acme/widget", Number: 7, HeadSHA: "aaa", BaseRef: "v5"} n, _ := h.e.supersedeAdoptedPR(context.Background(), h.e.stageStoreOrShared(), stageKey("acme", "widget", 7), obs, stagelabels.Record{}, adoptPolicy(), AdoptReasonDCORehome, 0, false, true, h.clock.at) if n != 0 || h.adopt.refAttempts != 0 || len(h.adopt.refUpdates) != 0 || h.adopt.refs["refs/heads/hive/adopt-7"] != "old" || len(h.adopt.pulls) != 0 || len(h.adopt.prEdits) != 0 { t.Fatalf("unsafe supersession mutated repository: n=%d refs=%v updates=%v pulls=%v edits=%v", n, h.adopt.refs, h.adopt.refUpdates, h.adopt.pulls, h.adopt.prEdits) @@ -1864,235 +1873,3 @@ func TestSupersedeRevalidatesBeforeRefMutation(t *testing.T) { }) } } - -func TestAdoptionAuthorPermission(t *testing.T) { - for _, tc := range []struct { - permission, role string - status int - maintainer, wantErr bool - }{ - {permission: "admin", maintainer: true}, - {permission: "maintain", maintainer: true}, - {permission: "write", role: "maintain", maintainer: true}, - {permission: "write", role: "admin", maintainer: true}, - {permission: "write"}, - {permission: "read"}, - {permission: "none"}, - {status: http.StatusForbidden, wantErr: true}, - } { - t.Run(tc.permission+tc.role+http.StatusText(tc.status), func(t *testing.T) { - h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) - defer done() - h.adopt.permission, h.adopt.roleName, h.adopt.permissionStatus = tc.permission, tc.role, tc.status - obs := stageObservation{Owner: "acme", Repo: "widget", Author: "mallory", Number: 7} - for i := 0; i < 2; i++ { - got, err := h.e.adoptionAuthorPermission(context.Background(), obs, h.clock.at) - if got != tc.maintainer || (err != nil) != tc.wantErr { - t.Fatalf("maintainer=%v err=%v", got, err) - } - obs.Number++ // A second PR by the same author uses the cached read. - } - if h.adopt.permissionReads != 1 { - t.Fatalf("reads=%d", h.adopt.permissionReads) - } - h.clock.advance(adoptCheckInterval) - _, _ = h.e.adoptionAuthorPermission(context.Background(), obs, h.clock.at) - if h.adopt.permissionReads != 2 { - t.Fatalf("expired cache reads=%d", h.adopt.permissionReads) - } - obs.Author = "other" - _, _ = h.e.adoptionAuthorPermission(context.Background(), obs, h.clock.at) - obs.Repo = "other" - _, _ = h.e.adoptionAuthorPermission(context.Background(), obs, h.clock.at) - if h.adopt.permissionReads != 4 { - t.Fatalf("cache failed to distinguish repo/author: reads=%d", h.adopt.permissionReads) - } - }) - } -} - -func TestMaintainerAdoptionStaysReturnedAndReleasesLiveAdoption(t *testing.T) { - for _, mode := range []string{"fresh", "in-place", "supersede", "supersede-open"} { - for _, permission := range []string{"admin", "maintain"} { - t.Run(permission+"/"+mode, func(t *testing.T) { - p := adoptPolicy() - p.AdoptInPlace = true - p.DCORequired = func(string) (bool, bool) { return true, true } - h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, p) - defer done() - h.adopt.permission = permission - h.adopt.allowMaintainerEdits(7, true) - h.api.commits = map[int][]string{7: {"fix: unsigned contributor commit"}} - h.observe(adoptPR(7, "aaa"), "check-failing:DCO") - key := stageKey("acme", "widget", 7) - labels := []string{"stage/072-returned-to-contributor", "role/contributor", "author/clanker", stagelabels.NoDCOLabel} - if mode != "fresh" { - h.e.stageStoreOrShared().MarkAdoptedInPlace(key, "aaa", AdoptReasonOffline, h.clock.at) - switch mode { - case "supersede": - h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) - h.adopt.setSuperseder(90, "closed", true) - case "supersede-open": - h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) - h.adopt.setSuperseder(90, "open", false) - } - labels[0] = "stage/074-adopted-by-hive" - } - h.clock.advance(2 * time.Hour) - for i := 0; i < 2; i++ { - h.api.reset() - h.observe(adoptPR(7, "aaa", labels...), "check-failing:DCO") - rec, _ := h.e.stageStoreOrShared().Get(key) - if rec.Stage != stagelabels.ReturnedToContributor || rec.AdoptionLive("aaa") || rec.AdoptedHead != "" || rec.AdoptedBy != 0 { - t.Fatalf("maintainer adopted: %+v", rec) - } - notes, supersederNotes := 0, 0 - for _, comment := range h.api.comments { - if strings.HasPrefix(comment, AdoptionMaintainerMarker) { - notes++ - } - if strings.HasPrefix(comment, AdoptionMaintainerSupersederMarker) { - supersederNotes++ - } - } - if notes != 1-i { - t.Fatalf("pass %d: refusal notes=%d comments=%v", i, notes, h.api.comments) - } - // An open hive superseder is closed with a note on the - // first pass only; the original PR is never edited. - wantEdits, wantSuperseder := []string(nil), 0 - if mode == "supersede-open" && i == 0 { - wantSuperseder = 1 - wantEdits = []string{"90:closed"} - if !strings.Contains(h.api.comments[len(h.api.comments)-1], "#90 has been closed") { - t.Fatalf("refusal note does not name the closed superseder: %v", h.api.comments) - } - } - if supersederNotes != wantSuperseder || !slices.Equal(h.adopt.prEdits, wantEdits) { - t.Fatalf("pass %d: superseder notes=%d edits=%v comments=%v", i, supersederNotes, h.adopt.prEdits, h.api.comments) - } - h.adopt.prEdits = nil - if len(adoptionComments(h.api)) != 0 || len(h.adopt.refs) != 0 || len(h.adopt.pulls) != 0 { - t.Fatalf("maintainer takeover: comments=%v refs=%v pulls=%v", h.api.comments, h.adopt.refs, h.adopt.pulls) - } - } - }) - } - } -} - -func TestMaintainerSupersederCloseFailureKeepsAdoption(t *testing.T) { - h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) - defer done() - h.adopt.permission = "admin" - h.observe(adoptPR(7, "aaa"), "check-failing:unit") - key := stageKey("acme", "widget", 7) - h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) - // No scripted superseder: GET pulls/90 is 404, so the release must wait. - h.clock.advance(adoptCheckInterval + time.Minute) - h.e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", adoptPR(7, "aaa", "stage/074-adopted-by-hive", "author/clanker"), "check-failing:unit", false, nil) - if _, err := h.e.ApplyStageLabels(context.Background()); err == nil { - t.Fatal("superseder lookup failure was not surfaced") - } - rec, _ := h.e.stageStoreOrShared().Get(key) - if rec.AdoptedHead != "aaa" || rec.AdoptedBy != 90 { - t.Fatalf("release orphaned the superseder: %+v", rec) - } - for _, comment := range h.api.comments { - if strings.HasPrefix(comment, AdoptionMaintainerMarker) { - t.Fatalf("refusal note posted while the superseder is still tracked: %v", h.api.comments) - } - } -} - -func TestMaintainerTerminalObservationPostsNoRefusal(t *testing.T) { - for _, tc := range []struct { - name string - merged bool - reason string - }{{name: "merged", merged: true}, {name: "closed", reason: "closed"}} { - t.Run(tc.name, func(t *testing.T) { - h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) - defer done() - h.adopt.permission = "admin" - h.observe(adoptPR(7, "aaa"), "check-failing:unit") - key := stageKey("acme", "widget", 7) - h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) - h.adopt.setSuperseder(90, "open", false) - h.api.reset() - h.e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", adoptPR(7, "aaa", "stage/074-adopted-by-hive", "author/clanker"), tc.reason, tc.merged, nil) - if _, err := h.e.ApplyStageLabels(context.Background()); err != nil { - t.Fatal(err) - } - if _, ok := h.e.stageStoreOrShared().Get(key); ok { - t.Fatal("terminal item still tracked") - } - if len(h.api.comments) != 0 || len(h.adopt.prEdits) != 0 || h.adopt.permissionReads != 0 { - t.Fatalf("terminal observation touched the maintainer's PR: comments=%v edits=%v permission reads=%d", h.api.comments, h.adopt.prEdits, h.adopt.permissionReads) - } - }) - } -} - -func TestAdoptionPermissionFailureDefersWithoutAttempt(t *testing.T) { - h, done := newAdoptHarness(t, ContributorNudgeOutcome{Mode: "comment"}, adoptPolicy()) - defer done() - h.observe(adoptPR(7, "aaa"), "check-failing:unit") - h.adopt.permissionStatus = http.StatusForbidden - h.clock.advance(2 * time.Hour) - h.e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", adoptPR(7, "aaa", "author/clanker"), "check-failing:unit", false, nil) - if _, err := h.e.ApplyStageLabels(context.Background()); err == nil { - t.Fatal("permission failure was not surfaced") - } - rec, _ := h.e.stageStoreOrShared().Get(stageKey("acme", "widget", 7)) - if rec.Stage != stagelabels.ReturnedToContributor || rec.AdoptedHead != "" || len(h.adopt.pulls) != 0 || h.adopt.refAttempts != 0 || len(adoptionComments(h.api)) != 0 { - t.Fatalf("adopted without permission evidence: %+v", rec) - } -} - -func TestAdoptionPermissionFailureKeepsLiveSupersede(t *testing.T) { - h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) - defer done() - h.observe(adoptPR(7, "aaa"), "check-failing:unit") - key := stageKey("acme", "widget", 7) - h.e.stageStoreOrShared().MarkAdopted(key, "aaa", 90, h.clock.at) - h.adopt.permissionStatus = http.StatusInternalServerError - h.clock.advance(adoptCheckInterval + time.Minute) - h.e.recordStage(stageLaneContributor, "acme/widget", "acme", "widget", adoptPR(7, "aaa", "stage/074-adopted-by-hive", "author/clanker"), "check-failing:unit", false, nil) - if _, err := h.e.ApplyStageLabels(context.Background()); err == nil { - t.Fatal("permission failure was not surfaced") - } - rec, _ := h.e.stageStoreOrShared().Get(key) - if rec.AdoptedHead != "aaa" || rec.AdoptedBy != 90 || rec.AdoptMode != stagelabels.AdoptModeSupersede { - t.Fatalf("transient permission failure released a live supersede adoption: %+v", rec) - } - if len(h.adopt.pulls) != 0 || h.adopt.refAttempts != 0 || len(adoptionComments(h.api)) != 0 { - t.Fatalf("permission failure mutated the repository: pulls=%v refs=%d", h.adopt.pulls, h.adopt.refAttempts) - } -} - -func TestSupersedeMaintainerRefusesBeforeMutation(t *testing.T) { - h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) - defer done() - h.adopt.permission = "admin" - obs := stageObservation{Owner: "acme", Repo: "widget", Author: "mallory", Number: 7, HeadSHA: "aaa", BaseRef: "v5"} - _, err := h.e.supersedeAdoptedPR(context.Background(), h.e.stageStoreOrShared(), stageKey("acme", "widget", 7), obs, stagelabels.Record{}, adoptPolicy(), AdoptReasonDCORehome, 0, false, true, h.clock.at) - if err != nil { - t.Fatal(err) - } - if h.adopt.refAttempts != 0 || len(h.adopt.pulls) != 0 || len(h.adopt.prEdits) != 0 { - t.Fatal("maintainer supersession mutated repository") - } -} - -func TestDelegatedSupersedeCannotTakeOverMaintainer(t *testing.T) { - h, done := newAdoptHarness(t, ContributorNudgeOutcome{}, adoptPolicy()) - defer done() - h.adopt.permission, h.adopt.roleName = "write", "maintain" - labels := []string{"stage/085-needs-decision", delegatedecision.LabelAdoptSupersede} - h.observe(adoptPR(7, "aaa", labels...), contributorLaneReasonNoReviewer) - rec, _ := h.e.stageStoreOrShared().Get(stageKey("acme", "widget", 7)) - if rec.AdoptedHead != "" || h.adopt.refAttempts != 0 || len(h.adopt.pulls) != 0 { - t.Fatalf("delegated request adopted maintainer: %+v", rec) - } -} diff --git a/src/pkg/github/automerge/automerge_stage_labels.go b/src/pkg/github/automerge/automerge_stage_labels.go index 7804522e8a..b9e21d2990 100644 --- a/src/pkg/github/automerge/automerge_stage_labels.go +++ b/src/pkg/github/automerge/automerge_stage_labels.go @@ -744,17 +744,6 @@ func (c *Engine) ApplyStageLabels(ctx context.Context) (StageLabelResult, error) obs.Reason = verdict.Reason } prevRec, hadPrev := store.Get(key) - // A merged/closed/gone PR is forgotten below; a maintainer who - // finished their own adopted PR must not get the refusal note. - if hadPrev && !verdict.Terminal && (prevRec.AdoptedInPlace() || prevRec.AdoptedHead != "") { - _, n, err := c.guardAdoption(ctx, store, key, obs, now) - result.Comments += n - if err != nil && firstErr == nil { - firstErr = err - } - prevRec, _ = store.Get(key) - } - // pass is what this pass learns past resolveStage; it rides on the // Facts so the engine's DerivePR reproduces the stage written here. pass := &stageengine.PRPassFacts{AuthorIsApp: normalizeBotLogin(obs.Author) == normalizeBotLogin(c.transport.AppBotLogin())} diff --git a/src/pkg/github/automerge/automerge_sweep.go b/src/pkg/github/automerge/automerge_sweep.go index 11dcf03bee..7aaa4cf1ae 100644 --- a/src/pkg/github/automerge/automerge_sweep.go +++ b/src/pkg/github/automerge/automerge_sweep.go @@ -174,15 +174,18 @@ type Engine struct { gh *gh.Client logger *slog.Logger - adoptionPermissionsMu sync.Mutex - adoptionPermissions map[string]adoptionPermission - selfSkipWarnedMu sync.Mutex selfSkipWarned map[string]struct{} mergerAuthzMu sync.RWMutex mergerAuthz MergerAuthorizer + // maintainerPerm caches whether a PR author holds admin or maintain on + // the PR's repository (authorMaintainsRepo): adoption never takes over + // a maintainer's own PR. + maintainerPermMu sync.Mutex + maintainerPerm map[string]maintainerPermEntry + requiredChecksMu sync.RWMutex requiredChecks map[string]bool requiredChecksForRepo func(repo string) (map[string]bool, bool) diff --git a/src/pkg/stagelabels/store.go b/src/pkg/stagelabels/store.go index 2927a526cd..d21625e929 100644 --- a/src/pkg/stagelabels/store.go +++ b/src/pkg/stagelabels/store.go @@ -133,8 +133,6 @@ type Record struct { // adoption on (the author pushed again); the yield note is posted once // per head. YieldedHead string `json:"yielded_head,omitempty"` - // AdoptionMaintainerNotedHead deduplicates the refusal note across sweeps and restarts. - AdoptionMaintainerNotedHead string `json:"adoption_maintainer_noted_head,omitempty"` // HivePushedHeads are heads the hive recorded pushing to this adopted PR. // They are trusted local evidence for in-place adoption attribution when // the later GitHub commit object cannot link the commit to the App. @@ -1070,19 +1068,6 @@ func (s *Store) ClearAdoption(key string) { s.dirty = true } -// NoteAdoptionMaintainer records a successfully posted maintainer refusal note. -func (s *Store) NoteAdoptionMaintainer(key, head string) { - s.mu.Lock() - defer s.mu.Unlock() - r, ok := s.items[key] - if !ok || head == "" || r.AdoptionMaintainerNotedHead == head { - return - } - r.AdoptionMaintainerNotedHead = head - s.items[key] = r - s.dirty = true -} - // NoteAdoptDiffFence records the diff-fence reason the adopted head tripped // ("" when the diff read clean) on an existing record. func (s *Store) NoteAdoptDiffFence(key, reason string) { From db659df6dc178c6eac06943eee8cdef835a46440 Mon Sep 17 00:00:00 2001 From: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Sun, 25 Oct 2026 20:25:34 -0400 Subject: [PATCH 7/7] fix: thread sweep clock into settleInPlaceAdoption 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> --- src/pkg/github/automerge/automerge_contributor_adopt.go | 4 ++-- src/pkg/github/automerge/automerge_stage_labels.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pkg/github/automerge/automerge_contributor_adopt.go b/src/pkg/github/automerge/automerge_contributor_adopt.go index 24bb5eabd9..13a5b74c0d 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt.go @@ -1011,9 +1011,9 @@ func (c *Engine) baseSyncTouchesWorkflows(ctx context.Context, obs stageObservat // the contributor is back, so the hive yields once and the ordinary 072 flow // re-arms. // Returns the number of comments posted. -func (c *Engine) settleInPlaceAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, prev stagelabels.Record, policy StageLabelPolicy) (int, error) { +func (c *Engine) settleInPlaceAdoption(ctx context.Context, store *stagelabels.Store, key string, obs stageObservation, prev stagelabels.Record, policy StageLabelPolicy, now time.Time) (int, error) { if prev.AdoptedInPlace() { - maintainer, err := c.authorMaintainsRepo(ctx, obs, c.now()) + maintainer, err := c.authorMaintainsRepo(ctx, obs, now) if err != nil { // No evidence either way: keep the adoption as it is. return 0, nil diff --git a/src/pkg/github/automerge/automerge_stage_labels.go b/src/pkg/github/automerge/automerge_stage_labels.go index b9e21d2990..1f7c6cbfae 100644 --- a/src/pkg/github/automerge/automerge_stage_labels.go +++ b/src/pkg/github/automerge/automerge_stage_labels.go @@ -858,7 +858,7 @@ func (c *Engine) ApplyStageLabels(ctx context.Context) (StageLabelResult, error) // adoption; a head the contributor pushed yields it. // Settled on every head change whatever the lane sees — // the hive's fix going green must not hide the yield. - n, err := c.settleInPlaceAdoption(ctx, store, key, obs, prevRec, policy) + n, err := c.settleInPlaceAdoption(ctx, store, key, obs, prevRec, policy, now) result.Comments += n if err != nil && firstErr == nil { firstErr = err