Repository navigation
✨ feat(automerge): contributor auto-promotion counts clean merges across the hive's repos - #14518
Conversation
…oss the hive's repos The contributor-lane clean-merge ratchet counted an author's merged PRs per repository, so a contributor with nine clean merges in hivecommons/hive was still contributor-lane:claim-tier-untrusted on their first PR to hivecommons/docs (docs#481 sat 17h green and approved). Trust is a property of the person, not the repo, and the satellite repos of a multi-repo hive never reach a per-repo threshold. - auto_merge.contributor_lane.auto_promotion_scope: hive|repo (default hive), validated, wired through the policy func and the Settings auto-merge API/UI. - With scope hive the PR's own repo is walked first; only when short does one search/issues query (is:pr is:merged author:X repo:... chunked to 256 chars) find the other hive repos holding the author's merges, and only those are walked for revert/sentinel evidence. A search failure degrades to the own-repo count with a warning. - Audit marker gains scope= and repos=<per-repo breakdown>; the admission log line carries scope and repos; comment text says "in this hive's repositories" for the hive scope. Fixes #14517 Signed-off-by: Andy Anderson <andy@clubanderson.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
|
@clubanderson is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this repository have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
…llowlist The contributor-lane auto-promotion scope help text now references the key, so the parity test requires the allowlist entry to go. Signed-off-by: Andy Anderson <andy@clubanderson.com>
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
There was a problem hiding this comment.
Security review (head ee74634)
Cross-repo revert/sentinel evidence is order-dependent, not "any repo" — src/pkg/github/automerge/automerge_contributor_lane.go, contributorCleanMergedHistory walk closure (~L720–737): walk returns done=true as soon as history.Checked >= need, and the hiveWide loop (~L742–747) stops at the first done. So with need=5, own repo 2 clean, hive 3 clean, docs holding a merged Revert "…"/sentinel-labelled PR by the author, the author is promoted without docs ever being listed. The PR body says "A revert or sentinel hit in any repo blocks" and the marker text says the merges "have no revert or sentinel-alert evidence"; neither holds for repos after the threshold is reached. The tests only cover the favourable ordering ("hive scope blocks on a revert in another repo" puts the revert in hive, walked before docs). Since contributorMergedRepos already narrows to the k repos that actually hold the author's merges (typically 1–2), walking all of them before deciding costs at most k−1 extra closed-PR lists:
walk := func(ref contributorRepoRef) (done bool, err error) {
- clean, blocked, err := c.contributorCleanMergesInRepo(ctx, ref.Owner, ref.Repo, author, need-history.Checked)
+ clean, blocked, err := c.contributorCleanMergesInRepo(ctx, ref.Owner, ref.Repo, author, need)
...
if blocked {
history.Reason = "revert-or-sentinel"
return true, nil
}
- if history.Checked >= need {
- history.Clean = true
- return true, nil
- }
- return false, nil
+ return false, nil
}
- if done, err := walk(contributorRepoRef{Owner: owner, Repo: repo}); done {
+ if done, err := walk(contributorRepoRef{Owner: owner, Repo: repo}); done || history.Checked >= need {
+ history.Clean = !done
return history, err
}
if hiveWide {
for _, ref := range c.contributorMergedRepos(ctx, owner, repo, author) {
if done, err := walk(ref); done {
return history, err
}
}
+ if history.Checked >= need {
+ history.Clean = true
+ return history, nil
+ }
}(Keeping the own-repo-settles-alone shortcut is a conscious cost trade-off and matches pre-#14517 behaviour; if you keep it, please soften the body/marker wording to "in the repositories counted" rather than "any repo".) Add a test case with the revert in the repo walked last.
No injection surface: author is the PR user's login (admitContributor L602) and found repos are intersected with the operator's activeRepos() candidates before any listing.
— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Reviewed (intent-alignment) — the diff matches #14517 point for point: hive-wide clean-merge count with per-PR revert/sentinel exclusion, auto_promotion_scope: hive|repo (default hive, enum-validated in config.go Validate, select in Settings), scoped marker/log wording with per-repo breakdown, one search/issues query then only matching repos walked. No unrelated scope creep. Approve.
Advisory (non-blocking): contributorCleanMergedHistory walks the PR's own repo first and returns as soon as it alone reaches need (src/pkg/github/automerge/automerge_contributor_lane.go:731-739), so the other repos — and any revert/sentinel evidence in them — are never consulted in that case. #14517's "exclusion evidence … per PR regardless of repo" therefore holds only when the own repo is short. This is identical to today's per-repo behaviour, so it is not a regression, but hive-merge.md:432-455 reads as if a revert "in any repository blocks" unconditionally; consider a short caveat there ("…when those repositories are consulted").
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the engine change against the tree at ee74634 (contributorCleanMergedHistory, contributorCleanMergesInRepo, contributorMergedRepos, the activeRepos() provider in automerge_sweep.go:363, and the config/dashboard wiring). The change does what #14517 asks and I found no defect: the PR's own repo is still walked first and settles the common case with one closed-PR list; a revert/sentinel hit in any walked repo blocks before the threshold check; activeRepos() already excludes paused and other-forge repos so the search never names a repo the hive cannot read; the search-failure path returns nil and degrades to exactly the old per-repo count. Docs (operator-reference.md:436, hive-merge.md:443–516) and the changed- fragment track the behaviour. Looks correct to me.
Two low-severity observations, neither blocking:
-
Satellite-repo list failure is an error, not a degrade —
automerge_contributor_lane.go:721. Thewalkclosure returns thePullRequests.Listerror for any repo, so a 403/404 on a satellite repo the search named surfaces ascontributor-lane:auto-promote-historyand the PR is skipped for the sweep with an error, whereas the body's "degrades … instead of blocking the sweep" only holds for the search call (:840,:847). Consequence is small (the author was short in their own repo anyway, so the alternative isinsufficient-clean-merges), but it is asymmetric with the search path and will show up as sweep errors rather than a debug line if a repo's permissions drift. Consider treating a non-own-repo list error like the search failure: warn, skip that repo, continue. -
Search is a single 100-result page —
:821(PerPage: 100, no pagination). For an author with >100 merged PRs across the hive, sortedupdated desc, a repo whose merges by that author are all older than the newest 100 is never walked. Under-count only (conservative: no promotion), so not a correctness risk, but worth a comment incontributorMergedReposso nobody later reads the repo list as exhaustive.
Also noting for the record: the settings_parity_allowlist.txt removal of auto_merge.contributor_tier.auto_promote_after_merged is forced by the parity test (settings_parity_test.go:38) because the new tooltip mentions that key, not because the threshold gained a control — the UI still only edits the scope. Not a defect in this PR.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at ee74634. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Fixes #14517
Problem
Contributor-lane auto-promotion (
contributorCleanMergedHistory) counted an author's clean merged PRs per repository. On hive's own hive, @thatguywob was auto-promoted inhivecommons/hive(9 clean merges, 0 reverts) while hivecommons/docs#481 — green CI, reviewer-swarmapprove— sat 17h atstage/050-review-pendingwithcontributor-lane:claim-tier-untrusted, because the count indocsis 0. Trust in a contributor is a property of the person, not the repo; the small satellite repos never reach a per-repo threshold.Change
auto_merge.contributor_lane.auto_promotion_scope: hive|repo(defaulthive), enum-validated inContributorLaneAutoMergeConfig.Validate, wired throughcontributorLanePolicyFunc, and exposed inGET/PUT /api/config/auto-mergeplus a select in Settings → Features → Auto Merge → Contributor-lane (mirrorsregistry_tiers_sufficient).automerge_contributor_lane.go):contributorCleanMergedHistorynow returns acontributorMergedHistory(clean, count, scope, per-repo breakdown, reason). With scopehivethe PR's own repo is walked first and settles the common case alone; only when short doescontributorMergedReposrun onesearch/issuesquery (is:pr is:merged author:X repo:… repo:…, chunked to GitHub's 256-charqlimit, PR's own repo excluded) to find which other active hive repos hold the author's merges, and only those repos' closed-PR lists are walked for revert/sentinel evidence. A revert or sentinel hit in any repo blocks. Search failure degrades to the own-repo count with a warning (exactly the old behaviour) instead of blocking the sweep.<!-- hive:contributor-auto-promoted author=X merged=N scope=hive repos=hive:4,docs:1 -->, text says "in this hive's repositories" (or "in this repository" with scoperepo); the admission log line carriesscope=… repos=…; the merge comment wording follows the scope.hive-merge.mdgains the auto-promotion admission path + YAML;operator-reference.mdgains both keys.TestTrySweepContributorLanePRAutoPromotionScope(hive-scope admit on first PR to a satellite repo, cross-repo sum, skipping repos without the author's merges, revert/sentinel in another repo blocks, repo scope, own-repo threshold needs no search, search failure degrades),TestContributorMergedHistoryHelpers, config enum test, dashboard GET/PUT test. Notime.Sleep.API cost
Common case (author already past threshold in the PR's repo): unchanged, one closed-PR list. Short case on a 10-repo hive: +1 search call (30/min budget, separate from core) + k closed-PR lists where k = repos that actually hold the author's merges (typically 1–2), instead of 10 lists.