diff --git a/changelog.d/fixed-adoption-skips-repo-maintainers.md b/changelog.d/fixed-adoption-skips-repo-maintainers.md new file mode 100644 index 0000000000..ec83bb6e75 --- /dev/null +++ b/changelog.d/fixed-adoption-skips-repo-maintainers.md @@ -0,0 +1 @@ +- Contributor adoption (stage `074` and its re-home / DCO re-home) no longer takes over a PR whose author has admin or maintain permission on the repository (backport of [#14305](https://github.com/hivecommons/hive/pull/14305), [#14301](https://github.com/hivecommons/hive/issues/14301)). The permission is read once per repository and author per 10 minutes; a failed read is not cached and defers, neither adopting nor releasing; an existing adoption of such an author is released, closing a still-open hive superseder with a note first. diff --git a/src/docs/labels-and-control-signals.md b/src/docs/labels-and-control-signals.md index 97f735fc30..f70c36b6ad 100644 --- a/src/docs/labels-and-control-signals.md +++ b/src/docs/labels-and-control-signals.md @@ -224,7 +224,7 @@ referenced it) a lone `stage/050-review-pending` | `stage/070-fixing` | PR | hive-owned repair: App/taken-over PR red, changes requested (by the swarm, or by a trusted human's **changes requested** review / `/hive decision ` on the current head — reason `human-changes-requested`, which also kicks the author agent with the text verbatim, [#13041](https://github.com/hivecommons/hive/issues/13041)) or needs-rebase, **or** a non-App PR whose red head was caused by a hive-pushed commit that must be removed/re-homed; a hive agent is reworking it | 2h | | `stage/071-needs-author` | PR | human-authored PR (`author/human`): CI red / changes requested / conflicting; the hive posted its diagnosis, the author must act. No nudge, no adoption — the person pushes or comments | 24h | | `stage/072-returned-to-contributor` | PR | clanker PR (`author/clanker`, relay or external hive): CI red / changes requested / conflicting; the hive posted its diagnosis, the ball is in the contributor hive's court. (Briefly spelled `stage/072-needs-author` by [#12713](https://github.com/hivecommons/hive/pull/12713); the engine renames that label back in place) | 24h | -| `stage/074-adopted-by-hive` | PR | the author (a clanker) was unavailable — relay offline, never registered, or silent past the grace (see below); the hive took the PR over: in place on the PR's own branch when it may push there (the scanner's red-PR fix kick rebases and fixes it), otherwise via `hive/adopt-` and a superseding App PR that keeps the author's commits (the original closes automatically when the superseder merges). Adoption is an **ownership** state, not a CI state: the PR reads 074 for as long as the hive owns it — through its fixes going pending, green, reviewed or red again (074 outranks 050/060/070/071/072) — and leaves 074 only at the merge gate (080/085/090/100), when the adoption is released, or to 085 `adoption-stalled:` when the head sat unchanged past the dwell with CI idle and a real blocker the hive cannot push past — a red optional check alone never counts, and the escalation releases back to 074 as soon as the reason clears (an in-place adoption with a blocking review finding re-homes to `hive/adopt-` first instead; one with a squash-unsafe hive commit elects a merge commit on a `role/trusted` PR — `merge-method/merge` label, lane continues — and otherwise immediately re-homes to an App-authored superseder (`signoff-rehome`), preserving commits and sign-offs while changing squash attribution; loss of push access re-homes immediately, and idle rebase work re-homes after the dwell). For in-place adoption, a changed head must be linked to the App bot by GitHub commit metadata (the stage store only remembers heads that already passed that check); otherwise the hive treats it as a contributor push and hands the PR back (072 if red, else the normal path). Forgeable git author/committer names and emails never keep 074. | 3h | +| `stage/074-adopted-by-hive` | PR | the author (a clanker) was unavailable — relay offline, never registered, or silent past the grace (see below); the hive took the PR over: in place on the PR's own branch when it may push there (the scanner's red-PR fix kick rebases and fixes it), otherwise via `hive/adopt-` and a superseding App PR that keeps the author's commits (the original closes automatically when the superseder merges). Adoption is an **ownership** state, not a CI state: the PR reads 074 for as long as the hive owns it — through its fixes going pending, green, reviewed or red again (074 outranks 050/060/070/071/072) — and leaves 074 only at the merge gate (080/085/090/100), when the adoption is released, or to 085 `adoption-stalled:` when the head sat unchanged past the dwell with CI idle and a real blocker the hive cannot push past — a red optional check alone never counts, and the escalation releases back to 074 as soon as the reason clears (an in-place adoption with a blocking review finding re-homes to `hive/adopt-` first instead; one with a squash-unsafe hive commit elects a merge commit on a `role/trusted` PR — `merge-method/merge` label, lane continues — and otherwise immediately re-homes to an App-authored superseder (`signoff-rehome`), preserving commits and sign-offs while changing squash attribution; loss of push access re-homes immediately, and idle rebase work re-homes after the dwell). For in-place adoption, a changed head must be linked to the App bot by GitHub commit metadata (the stage store only remembers heads that already passed that check); otherwise the hive treats it as a contributor push and hands the PR back (072 if red, else the normal path). Forgeable git author/committer names and emails never keep 074. A PR whose author holds **admin** or **maintain** on the repository is never adopted or re-homed (one cached permission read per repo and author per 10 minutes; a failed read is not cached and defers: no adoption, and an existing adoption is neither released nor re-homed until the read succeeds), and an adoption of such an author is released on the next pass, closing a still-open hive superseder with a note first. | 3h | | `stage/080-awaiting-merge-admission` | PR | nothing left for the author: CI green (no red check on the head, required or not), verdict approve, no modifiers — and the automerge lane has not admitted it yet (prefilter not evaluated for this head, labels not synced, lane tripped). A stall here means a lane problem, so the hive also posts one `` 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 94467ee364..13a5b74c0d 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt.go @@ -564,6 +564,10 @@ 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 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)) { // The fence could not judge this head (it moved, or its file list @@ -639,6 +643,16 @@ 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) { + 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 if stored, ok := store.Get(key); ok && stored.AdoptDiffFence != "" { @@ -733,6 +747,17 @@ func (c *Engine) settleAdoption(ctx context.Context, store *stagelabels.Store, k 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) { @@ -807,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. @@ -884,7 +1011,20 @@ 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, 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 } @@ -1140,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 e02241f9aa..0c4513eb5f 100644 --- a/src/pkg/github/automerge/automerge_contributor_adopt_test.go +++ b/src/pkg/github/automerge/automerge_contributor_adopt_test.go @@ -46,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 { @@ -62,6 +69,24 @@ 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.onPermission != nil { + a.onPermission() + } + if a.failPermission { + w.WriteHeader(http.StatusInternalServerError) + _ = json.NewEncoder(w).Encode(map[string]string{"message": "scripted failure"}) + return true + } + 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]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 { w.WriteHeader(http.StatusInternalServerError) 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 diff --git a/src/pkg/github/automerge/automerge_sweep.go b/src/pkg/github/automerge/automerge_sweep.go index 602ce05e51..7aaa4cf1ae 100644 --- a/src/pkg/github/automerge/automerge_sweep.go +++ b/src/pkg/github/automerge/automerge_sweep.go @@ -180,6 +180,12 @@ type Engine 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)