From 747d9e2d348b154162b76b4fc24f10f02ad9e55e Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Tue, 4 Nov 2025 10:06:36 +0100 Subject: [PATCH 1/4] ci/github-script/merge: switch order of merge operations We previously used auto-merge first and then enqueued explicitly on the assumption that auto-merge would fail if the PR was actually in mergeable state already. This turned out to be false. Instead, we currently face the problem of auto-merge sometimes getting stuck. This seems to happen when, at the time of enabling auto-merge, the required status checks already passed and the PR would be ready to go - but sometimes GitHub doesn't do it. This *can* be unblocked by approving the PR again, which seems to run the internal "let's check whether we can merge this" procedures on the GitHub side again. However, we can probably also solve this by just explicitly trying to enqueue the PR first - and only if that fails, fall back to auto-merge. I previously argued against that, based on a potential race condition, in which a PR could become ready to merge between these two requests - at which point the auto-merge operation would fail, if the original assumption was true. But since we don't observe this, we might as well switch. --- ci/github-script/merge.js | 48 ++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 5f536579f22c..3ebc86d616c2 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -184,32 +184,9 @@ async function handleMerge({ return 'Merge completed (dry)' } - // Using GraphQL's enablePullRequestAutoMerge mutation instead of the REST - // /merge endpoint, because the latter doesn't work with Merge Queues. - // This mutation works both with and without Merge Queues. - // It doesn't work when there are no required status checks for the target branch. - // All development branches have these enabled, so this is a non-issue. - try { - await github.graphql( - `mutation($node_id: ID!, $sha: GitObjectID) { - enablePullRequestAutoMerge(input: { - expectedHeadOid: $sha, - pullRequestId: $node_id - }) - { clientMutationId } - }`, - { node_id: pull_request.node_id, sha: pull_request.head.sha }, - ) - return 'Enabled Auto Merge' - } catch (e) { - log('Auto Merge failed', e.response.errors[0].message) - } - - // TODO: Observe whether the below is true and whether manual enqueue is actually needed. - // Auto-merge doesn't work if the target branch has already run all CI, in which - // case the PR must be enqueued explicitly. - // We now have merge queues enabled on all development branches, thus don't need a - // fallback after this. + // Using GraphQL mutations instead of the REST /merge endpoint, because the latter + // doesn't work with Merge Queues. We now have merge queues enabled on all development + // branches, so we don't need a fallback for regular merges. try { const resp = await github.graphql( `mutation($node_id: ID!, $sha: GitObjectID) { @@ -227,6 +204,25 @@ async function handleMerge({ return `[Queued](${resp.enqueuePullRequest.mergeQueueEntry.mergeQueue.url}) for merge` } catch (e) { log('Enqueing failed', e.response.errors[0].message) + } + + // If required status checks are not satisfied, yet, the above will fail. In this case + // we can enable auto-merge. We could also only use auto-merge, but this often gets + // stuck for no apparent reason. + try { + await github.graphql( + `mutation($node_id: ID!, $sha: GitObjectID) { + enablePullRequestAutoMerge(input: { + expectedHeadOid: $sha, + pullRequestId: $node_id + }) + { clientMutationId } + }`, + { node_id: pull_request.node_id, sha: pull_request.head.sha }, + ) + return 'Enabled Auto Merge' + } catch (e) { + log('Auto Merge failed', e.response.errors[0].message) throw new Error(e.response.errors[0].message) } } From 2d6602908bdf3824b189bad0f8408e42ba3a3cad Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Tue, 4 Nov 2025 15:38:21 +0100 Subject: [PATCH 2/4] ci/github-script/merge: improve testability By only ignoring already-handled comments when running non-dry, it's much easier to look at existing PRs, for which the merge bot already commented, and iterate on them locally. It's dry mode anyway, so it won't hurt to get a few more merge comments in the console output. --- ci/github-script/merge.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 3ebc86d616c2..02854dbd5ea3 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -168,14 +168,15 @@ async function handleMerge({ // Ignore comments where the user has been deleted already. user && // Ignore comments which had already been responded to by the bot. - !events.some( - ({ event, body }) => - ['commented'].includes(event) && - // We're only testing this hidden reference, but not the author of the comment. - // We'll just assume that nobody creates comments with this marker on purpose. - // Additionally checking the author is quite annoying for local debugging. - body.match(new RegExp(`^$`, 'm')), - ), + (dry || + !events.some( + ({ event, body }) => + ['commented'].includes(event) && + // We're only testing this hidden reference, but not the author of the comment. + // We'll just assume that nobody creates comments with this marker on purpose. + // Additionally checking the author is quite annoying for local debugging. + body.match(new RegExp(`^$`, 'm')), + )), ) async function merge() { From 58a1fe4761b55e98d6652e04fa628d81d3e75a1d Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Tue, 4 Nov 2025 12:14:13 +0100 Subject: [PATCH 3/4] ci/github-script/bot: move getTeamMembers cache into main file This allows re-using this elsewhere with a shared cache. --- ci/github-script/bot.js | 24 ++++++++++++++++++++++++ ci/github-script/merge.js | 30 ++++-------------------------- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index fb5e79e54239..2c3bd45f69e5 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -96,6 +96,29 @@ module.exports = async ({ github, context, core, dry }) => { return maintainerMaps[branch] } + // Caching the list of team members saves API requests when running the bot on the schedule and + // processing many PRs at once. + const members = {} + function getTeamMembers(team_slug) { + if (context.eventName === 'pull_request') { + // We have no chance of getting a token in the pull_request context with the right + // permissions to access the members endpoint below. Thus, we're pretending to have + // no members. This is OK; because this is only for the Test workflow, not for + // real use. + return [] + } + + if (!members[team_slug]) { + members[team_slug] = github.paginate(github.rest.teams.listMembersInOrg, { + org: context.repo.owner, + team_slug, + per_page: 100, + }) + } + + return members[team_slug] + } + async function handlePullRequest({ item, stats, events }) { const log = (k, v) => core.info(`PR #${item.number} - ${k}: ${v}`) @@ -121,6 +144,7 @@ module.exports = async ({ github, context, core, dry }) => { pull_request, events, maintainers, + getTeamMembers, }) // When the same change has already been merged to the target branch, a PR will still be diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 02854dbd5ea3..8a6789a30544 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -109,10 +109,6 @@ async function handleMergeComment({ github, body, node_id, reaction }) { ) } -// Caching the list of team members saves API requests when running the bot on the schedule and -// processing many PRs at once. -const members = {} - async function handleMerge({ github, context, @@ -122,31 +118,13 @@ async function handleMerge({ pull_request, events, maintainers, + getTeamMembers, }) { const pull_number = pull_request.number - function getTeamMembers(team_slug) { - if (context.eventName === 'pull_request') { - // We have no chance of getting a token in the pull_request context with the right - // permissions to access the members endpoint below. Thus, we're pretending to have - // no members. This is OK; because this is only for the Test workflow, not for - // real use. - return new Set() - } - - if (!members[team_slug]) { - members[team_slug] = github - .paginate(github.rest.teams.listMembersInOrg, { - org: context.repo.owner, - team_slug, - per_page: 100, - }) - .then((members) => new Set(members.map(({ id }) => id))) - } - - return members[team_slug] - } - const committers = await getTeamMembers('nixpkgs-committers') + const committers = new Set( + (await getTeamMembers('nixpkgs-committers')).map(({ id }) => id), + ) const files = await github.paginate(github.rest.pulls.listFiles, { ...context.repo, From 1e6124a504ea2d4ba01039968b00ae670fde3b07 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Tue, 4 Nov 2025 11:21:14 +0100 Subject: [PATCH 4/4] ci/github-script/merge: list eligible users in comment When a user tries to merge a PR, but is not allowed to, it is helpful to explicitly list the users who *are* allowed. This helps explaining *why* the merge-eligible label was set. I objected to this proposal before, because it would incur too many API requests. But after we have restructured the checklist, this is not actually true anymore - we can now sensibly run this only when a comment is posted and not whenever we check a PR for eligibility. --- ci/github-script/bot.js | 19 +++++++++++++++++++ ci/github-script/merge.js | 16 +++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index 2c3bd45f69e5..133a6881d235 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -119,6 +119,24 @@ module.exports = async ({ github, context, core, dry }) => { return members[team_slug] } + // Caching users saves API requests when running the bot on the schedule and processing + // many PRs at once. It also helps to encapsulate the special logic we need, because + // actions/github doesn't support that endpoint fully, yet. + const users = {} + function getUser(id) { + if (!users[id]) { + users[id] = github + .request({ + method: 'GET', + url: '/user/{id}', + id, + }) + .then((resp) => resp.data) + } + + return users[id] + } + async function handlePullRequest({ item, stats, events }) { const log = (k, v) => core.info(`PR #${item.number} - ${k}: ${v}`) @@ -145,6 +163,7 @@ module.exports = async ({ github, context, core, dry }) => { events, maintainers, getTeamMembers, + getUser, }) // When the same change has already been merged to the target branch, a PR will still be diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 8a6789a30544..688a7928fe97 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -80,6 +80,7 @@ function runChecklist({ return { checklist, + eligible, result, } } @@ -119,6 +120,7 @@ async function handleMerge({ events, maintainers, getTeamMembers, + getUser, }) { const pull_number = pull_request.number @@ -240,7 +242,7 @@ async function handleMerge({ } } - const { result, checklist } = runChecklist({ + const { result, eligible, checklist } = runChecklist({ committers, events, files, @@ -270,6 +272,18 @@ async function handleMerge({ '', ] + if (eligible.size > 0 && !eligible.has(comment.user.id)) { + const users = await Promise.all( + Array.from(eligible, async (id) => (await getUser(id)).login), + ) + body.push( + '> [!TIP]', + '> Maintainers eligible to merge are:', + ...users.map((login) => `> - ${login}`), + '', + ) + } + if (result) { await react('ROCKET') try {