From da97bf84234657e6fdde8fabd8720eceb685d4f4 Mon Sep 17 00:00:00 2001 From: Aliaksandr Date: Sat, 25 Apr 2026 02:23:25 +0300 Subject: [PATCH 1/4] ci/github-script/merge: skip auto-merge when CI has already failed The merge bot falls back to GitHub Auto Merge whenever the merge queue won't accept a PR yet. That is the right move while CI is still running, but pointless once CI has already failed: Auto Merge can never trigger, and fixing CI requires a new push, which invalidates the merge command anyway (the bot only acts on comments after the latest push). Fetch the `no PR failures` commit status and, when the merge-queue enqueue fails, branch on it. If CI has already failed (`error`/`failure`), skip Auto Merge and reply that a fresh `@NixOS/nixpkgs-merge-bot merge` comment is needed once CI is green again. Otherwise (pending or missing status) enable Auto Merge as before. `merge()` now returns `{ reaction, messages }` so the CI-failure path can leave a thumbs-down reaction rather than the rocket used for an actual merge. Closes #512554. Assisted-by: claude-code with claude-opus-4-8[1m]-high --- ci/README.md | 9 +++++ ci/github-script/merge.js | 69 ++++++++++++++++++++++++++++++--------- 2 files changed, 63 insertions(+), 15 deletions(-) diff --git a/ci/README.md b/ci/README.md index be0e8e4c4fca..814299ed8182 100644 --- a/ci/README.md +++ b/ci/README.md @@ -52,6 +52,14 @@ To ensure security and a focused utility, the bot adheres to specific limitation - The user attempting to merge is a member of [@NixOS/nixpkgs-maintainers]. - The user attempting to merge is a maintainer of all packages touched by the PR. +Once these constraints are met, the bot picks a merge strategy based on the `no PR failures` commit status: + +- CI passing: the PR is added to the merge queue. +- CI unfinished (pending or missing status): the bot enables [Auto Merge], which queues the PR once required checks succeed. + Note that if CI later fails, nothing happens until it is fixed and passes. +- CI already failing (`error`/`failure` status): the bot does not enable Auto Merge, because it would never trigger, and fixing CI requires a new push that invalidates the merge command. + A fresh `@NixOS/nixpkgs-merge-bot merge` comment is needed once CI is green again. + ### Approving merge bot changes Changes to the bot can usually be approved by the [@NixOS/nixpkgs-ci] team, as with other CI changes. @@ -104,3 +112,4 @@ This script can also be run locally to print basic test cases. [@NixOS/nixpkgs-ci]: https://github.com/orgs/NixOS/teams/nixpkgs-ci [@NixOS/nixpkgs-core]: https://github.com/orgs/NixOS/teams/nixpkgs-core [RFC 172]: https://github.com/NixOS/rfcs/pull/172 +[Auto Merge]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index ee861e21187d..e7f0cac8cc83 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -57,6 +57,9 @@ function runChecklist({ pull_request.user.login === 'r-ryantm', }, 'PR is not a draft': !pull_request.draft, + // CI state is intentionally *not* a checklist item: auto-merge exists precisely to + // cover unfinished CI, and an already-failed CI is reported via the merge message + // (see merge() below) rather than a blanket refusal. } if (user) { @@ -148,6 +151,14 @@ async function handleMerge({ // including an early exit when the first non-by-name file is found. if (files.length >= 100) return false + const noPrFailuresState = ( + await github.rest.repos.listCommitStatusesForRef({ + ...context.repo, + ref: pull_request.head.sha, + per_page: 100, + }) + ).data.find(({ context }) => context === 'no PR failures')?.state + // Only look through comments *after* the latest (force) push. const lastPush = events.findLastIndex( ({ event, sha, commit_id }) => @@ -173,10 +184,12 @@ async function handleMerge({ )), ) + // Returns `{ reaction, messages }`: the reaction to leave on the merge comment and the + // lines to append to the bot's reply. Throws only on an unexpected API error. async function merge() { if (dry) { core.info(`Merging #${pull_number}... (dry)`) - return ['Merge completed (dry)'] + return { reaction: 'ROCKET', messages: ['Merge completed (dry)'] } } // Using GraphQL mutations instead of the REST /merge endpoint, because the latter @@ -197,16 +210,37 @@ async function handleMerge({ { node_id: pull_request.node_id, sha: pull_request.head.sha }, ) log('merge', 'Queued for merge') - return [ - `:heavy_check_mark: [Queued](${resp.enqueuePullRequest.mergeQueueEntry.mergeQueue.url}) for merge (#306934)`, - ] + return { + reaction: 'ROCKET', + messages: [ + `:heavy_check_mark: [Queued](${resp.enqueuePullRequest.mergeQueueEntry.mergeQueue.url}) for merge (#306934)`, + ], + } } catch (e) { log('Enqueuing 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. + // Enqueuing fails when the required status checks are not satisfied, yet. If CI has + // already failed, enabling auto-merge would be pointless: it would never fire, and + // fixing CI requires a new push, which invalidates this merge command anyway (we only + // act on comments after the latest push). So we don't enable auto-merge and instead + // ask for a fresh command once CI is green again. + if (['error', 'failure'].includes(noPrFailuresState)) { + log('merge', 'CI has failed, not enabling auto-merge') + return { + reaction: 'THUMBS_DOWN', + messages: [ + ':x: Pull Request could not be merged: CI has failed (#305350).', + '', + '> [!TIP]', + '> PRs cannot be merged while CI is failing.', + '> Once CI is passing, comment `@NixOS/nixpkgs-merge-bot merge` again.', + ], + } + } + + // CI has not finished yet, so we 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) { @@ -219,12 +253,15 @@ async function handleMerge({ { node_id: pull_request.node_id, sha: pull_request.head.sha }, ) log('merge', 'Auto-merge enabled') - return [ - `:heavy_check_mark: Enabled Auto Merge (#306934)`, - '', - '> [!TIP]', - '> Sometimes GitHub gets stuck after enabling Auto Merge. In this case, leaving another approval should trigger the merge.', - ] + return { + reaction: 'ROCKET', + messages: [ + `:heavy_check_mark: Enabled Auto Merge (#306934)`, + '', + '> [!TIP]', + '> Sometimes GitHub gets stuck after enabling Auto Merge. In this case, leaving another approval should trigger the merge.', + ], + } } catch (e) { log('Auto Merge failed', e.response.errors[0].message) throw new Error(e.response.errors[0].message) @@ -308,10 +345,12 @@ async function handleMerge({ } if (result) { - await react('ROCKET') try { - body.push(...(await merge())) + const { reaction, messages } = await merge() + await react(reaction) + body.push(...messages) } catch (e) { + await react('THUMBS_DOWN') // Remove the HTML comment with node_id reference to allow retrying this merge on the next run. body.shift() body.push(`:x: Merge failed with: ${e} (#371492)`) From b94b44d3f9e0d7d021dc4d550d2dd0185da47489 Mon Sep 17 00:00:00 2001 From: Aliaksandr Date: Sat, 25 Apr 2026 03:12:03 +0300 Subject: [PATCH 2/4] ci/github-script/merge: refuse merge when a committer has requested changes A "changes requested" review from a committer blocks both the merge queue and auto-merge, but unlike approvals it isn't auto-dismissed when new commits are pushed. Surface it as a checklist item so the bot's reply explains the block instead of silently enabling auto-merge that will never trigger. Implementation pulls every review for the PR via `listReviews` and, for each committer, takes the latest review whose state is `APPROVED`/`CHANGES_REQUESTED`. The check fails if any committer's latest stance is `CHANGES_REQUESTED`. Other review states fall out naturally: - A dismissed review surfaces as `DISMISSED`, so a committer dismissing their own changes-requested review unblocks the PR. - A comment-only follow-up surfaces as `COMMENTED`, so it doesn't override an earlier actionable review - the prior stance still stands until the committer explicitly approves or requests changes again. Assisted-by: claude-code with claude-opus-4-8[1m]-high --- ci/README.md | 2 ++ ci/github-script/merge.js | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/ci/README.md b/ci/README.md index 814299ed8182..6fbdca39800c 100644 --- a/ci/README.md +++ b/ci/README.md @@ -51,6 +51,8 @@ To ensure security and a focused utility, the bot adheres to specific limitation - opened by [@r-ryantm](https://nix-community.github.io/nixpkgs-update/r-ryantm/). - The user attempting to merge is a member of [@NixOS/nixpkgs-maintainers]. - The user attempting to merge is a maintainer of all packages touched by the PR. +- No [committer][@NixOS/nixpkgs-committers] has an outstanding "changes requested" review. + These block both the merge queue and auto-merge, so the bot refuses to merge until the review is addressed or dismissed. Once these constraints are met, the bot picks a merge strategy based on the `no PR failures` commit status: diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index e7f0cac8cc83..1d218b27b0ae 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -7,6 +7,7 @@ function runChecklist({ pull_request, log, maintainers, + reviews, user, userIsMaintainer, }) { @@ -41,6 +42,27 @@ function runChecklist({ .filter(Boolean), ) + // A "changes requested" review from a committer blocks both the merge queue and + // auto-merge, even if it was made on an older commit (unlike approvals, GitHub does + // not auto-dismiss changes-requested reviews on push). For each committer, take their + // latest actionable review; if it's CHANGES_REQUESTED, they're blocking the PR. + // Dismissed reviews surface as DISMISSED and comment-only follow-ups as COMMENTED, so + // both are skipped naturally — the prior actionable review still stands until the + // committer explicitly approves or requests changes again. + const committerReviewState = new Map() + for (const { user, state } of reviews) { + if ( + user && + committers.has(user.id) && + ['APPROVED', 'CHANGES_REQUESTED'].includes(state) + ) { + committerReviewState.set(user.id, state) + } + } + const noBlockingReviews = !Array.from(committerReviewState.values()).includes( + 'CHANGES_REQUESTED', + ) + const checklist = { 'PR targets a [development branch](https://github.com/NixOS/nixpkgs/blob/-/ci/README.md#branch-classification).': classify(pull_request.base.ref).type.includes('development'), @@ -60,6 +82,8 @@ function runChecklist({ // CI state is intentionally *not* a checklist item: auto-merge exists precisely to // cover unfinished CI, and an already-failed CI is reported via the merge message // (see merge() below) rather than a blanket refusal. + 'PR is not blocked by a "changes requested" review from a [committer](https://github.com/orgs/NixOS/teams/nixpkgs-committers).': + noBlockingReviews, } if (user) { @@ -159,6 +183,15 @@ async function handleMerge({ }) ).data.find(({ context }) => context === 'no PR failures')?.state + // Reviews are returned in chronological order; the latest review per user wins below. + // Dismissed reviews surface with state DISMISSED and comment-only follow-ups as + // COMMENTED, so both naturally drop out of the committer-blocking check. + const reviews = await github.paginate(github.rest.pulls.listReviews, { + ...context.repo, + pull_number, + per_page: 100, + }) + // Only look through comments *after* the latest (force) push. const lastPush = events.findLastIndex( ({ event, sha, commit_id }) => @@ -309,6 +342,7 @@ async function handleMerge({ pull_request, log, maintainers, + reviews, user: comment.user, userIsMaintainer: await isMaintainer(comment.user.login), }) @@ -380,6 +414,7 @@ async function handleMerge({ pull_request, log, maintainers, + reviews, }) // Returns a boolean, which indicates whether the PR is merge-bot eligible in principle. From 6f9325fb5dbf965b2078cba8a5ca4ad285e8f0af Mon Sep 17 00:00:00 2001 From: Aliaksandr Date: Sat, 9 May 2026 15:04:29 +0300 Subject: [PATCH 3/4] ci/github-script/merge: clarify Auto Merge follow-up tip The previous tip ("Sometimes GitHub gets stuck after enabling Auto Merge") didn't explain why nothing happens immediately after the bot's "Enabled Auto Merge" reply, leaving maintainers unsure whether to wait or intervene. Spell out that Auto Merge waits for required CI before queueing, that a later CI failure leaves the PR un-queued until it is fixed, and link to GitHub's documentation; keep the existing "leave another approval" workaround for the rare cases where Auto Merge stalls after CI completes. Assisted-by: claude-code with claude-opus-4-8[1m]-high --- ci/github-script/merge.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 1d218b27b0ae..963d2c1e61bf 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -292,7 +292,9 @@ async function handleMerge({ `:heavy_check_mark: Enabled Auto Merge (#306934)`, '', '> [!TIP]', - '> Sometimes GitHub gets stuck after enabling Auto Merge. In this case, leaving another approval should trigger the merge.', + '> [Auto Merge](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request) will queue this PR once required CI checks succeed.', + '> If CI fails instead, fixing it needs a new push, which disables Auto Merge and invalidates this command — comment `@NixOS/nixpkgs-merge-bot merge` again once CI is green.', + '> If GitHub gets stuck even though CI passed (it sometimes does), leaving another approval should kick off the merge.', ], } } catch (e) { From 0c1c3d48139703764cb5415b585313ef8b15d0fe Mon Sep 17 00:00:00 2001 From: Aliaksandr Date: Sat, 9 May 2026 15:15:27 +0300 Subject: [PATCH 4/4] ci/github-script/merge: share reviews fetch with bot.js, drop events `bot.js` already pulls reviews via GraphQL for the approval-count labels. Move that fetch above the `handleMerge` call, add `commit { oid }` to the query, and pass the result through. `handleMerge` no longer issues its own `listReviews` REST call. While here, the `approvals` set is now derived from the same reviews data. Two upsides: - A reviewer who approved and was later dismissed no longer counts towards the approval check; their review now surfaces with state DISMISSED instead of leaving a stale `reviewed` event behind. - `events` is no longer needed by `runChecklist` (it was only feeding the approvals filter); the parameter is dropped from both call sites. `handleMerge` still uses `events` for tracking the latest push and merge-command comments. Also drop the redundant `user` truthy checks (and the stale "some users have been deleted" comment) in `runChecklist`. The GraphQL query uses `author { ... on User { login id } }` and bot.js then filters via `r.user?.login`, so by the time reviews reach `runChecklist` every entry already has a populated `user`. Verified by querying real PRs: bots surface as `{__typename: "Bot"}` (no `login`/`id`, filtered out by bot.js), and deleted accounts surface as the "ghost" user (login `"ghost"`, id `10137`), which passes the filter but matches no committer - harmless for both checks. Assisted-by: claude-code with claude-opus-4-8[1m]-high --- ci/github-script/bot.js | 29 ++++++++++++++++------------- ci/github-script/merge.js | 27 ++++++--------------------- 2 files changed, 22 insertions(+), 34 deletions(-) diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index c952499cec54..8e0043d5505c 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -206,20 +206,8 @@ module.exports = async ({ github, context, core, dry }) => { const maintainers = await getMaintainerMap(pull_request.base.ref) - const merge_bot_eligible = await handleMerge({ - github, - context, - core, - log, - dry, - pull_request, - events, - maintainers, - getTeamMembers, - getUser, - }) - // Check for any human reviews other than the PR author, GitHub actions and other GitHub apps. + // `commit { oid }` is needed by handleMerge to verify approvals are against the current head. const reviews = ( await github.graphql( `query($owner: String!, $repo: String!, $pr: Int!) { @@ -231,6 +219,7 @@ module.exports = async ({ github, context, core, dry }) => { reviews(first: 100) { nodes { state + commit { oid } user: author { # Only get users, no bots ... on User { @@ -266,6 +255,20 @@ module.exports = async ({ github, context, core, dry }) => { r.user.id !== pull_request.user?.id, ) + const merge_bot_eligible = await handleMerge({ + github, + context, + core, + log, + dry, + pull_request, + events, + reviews, + maintainers, + getTeamMembers, + getUser, + }) + const approvals = new Set( reviews .filter((review) => review.state === 'APPROVED') diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 963d2c1e61bf..180a12b0fe4a 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -2,7 +2,6 @@ const { classify } = require('../supportedBranches.js') function runChecklist({ committers, - events, files, pull_request, log, @@ -28,18 +27,15 @@ function runChecklist({ .reduce((acc, cur) => acc?.intersection(cur) ?? cur) const approvals = new Set( - events + reviews .filter( - ({ event, state, commit_id }) => - event === 'reviewed' && - state === 'approved' && + ({ state, commit }) => + state === 'APPROVED' && // Only approvals for the current head SHA count, otherwise authors could push // bad code between the approval and the merge. - commit_id === pull_request.head.sha, + commit?.oid === pull_request.head.sha, ) - .map(({ user }) => user?.id) - // Some users have been deleted, so filter these out. - .filter(Boolean), + .map(({ user }) => user.id), ) // A "changes requested" review from a committer blocks both the merge queue and @@ -52,7 +48,6 @@ function runChecklist({ const committerReviewState = new Map() for (const { user, state } of reviews) { if ( - user && committers.has(user.id) && ['APPROVED', 'CHANGES_REQUESTED'].includes(state) ) { @@ -150,6 +145,7 @@ async function handleMerge({ dry, pull_request, events, + reviews, maintainers, getTeamMembers, getUser, @@ -183,15 +179,6 @@ async function handleMerge({ }) ).data.find(({ context }) => context === 'no PR failures')?.state - // Reviews are returned in chronological order; the latest review per user wins below. - // Dismissed reviews surface with state DISMISSED and comment-only follow-ups as - // COMMENTED, so both naturally drop out of the committer-blocking check. - const reviews = await github.paginate(github.rest.pulls.listReviews, { - ...context.repo, - pull_number, - per_page: 100, - }) - // Only look through comments *after* the latest (force) push. const lastPush = events.findLastIndex( ({ event, sha, commit_id }) => @@ -339,7 +326,6 @@ async function handleMerge({ const { result, eligible, checklist } = runChecklist({ committers, - events, files, pull_request, log, @@ -411,7 +397,6 @@ async function handleMerge({ const { result } = runChecklist({ committers, - events, files, pull_request, log,