diff --git a/ci/README.md b/ci/README.md index be0e8e4c4fca..6fbdca39800c 100644 --- a/ci/README.md +++ b/ci/README.md @@ -51,6 +51,16 @@ 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: + +- 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 @@ -104,3 +114,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/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 ee861e21187d..180a12b0fe4a 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -2,11 +2,11 @@ const { classify } = require('../supportedBranches.js') function runChecklist({ committers, - events, files, pull_request, log, maintainers, + reviews, user, userIsMaintainer, }) { @@ -27,18 +27,35 @@ 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 + // 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 ( + 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 = { @@ -57,6 +74,11 @@ 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. + 'PR is not blocked by a "changes requested" review from a [committer](https://github.com/orgs/NixOS/teams/nixpkgs-committers).': + noBlockingReviews, } if (user) { @@ -123,6 +145,7 @@ async function handleMerge({ dry, pull_request, events, + reviews, maintainers, getTeamMembers, getUser, @@ -148,6 +171,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 +204,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 +230,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 +273,17 @@ 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]', + '> [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) { log('Auto Merge failed', e.response.errors[0].message) throw new Error(e.response.errors[0].message) @@ -267,11 +326,11 @@ async function handleMerge({ const { result, eligible, checklist } = runChecklist({ committers, - events, files, pull_request, log, maintainers, + reviews, user: comment.user, userIsMaintainer: await isMaintainer(comment.user.login), }) @@ -308,10 +367,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)`) @@ -336,11 +397,11 @@ async function handleMerge({ const { result } = runChecklist({ committers, - events, files, pull_request, log, maintainers, + reviews, }) // Returns a boolean, which indicates whether the PR is merge-bot eligible in principle.