From 636fc13366919d6441b26a14ed7a4395e835e0b1 Mon Sep 17 00:00:00 2001 From: Michael Daniels Date: Sun, 5 Apr 2026 12:14:17 -0400 Subject: [PATCH] {ci,workflows}: allow multiple blocking reviews --- .github/workflows/check.yml | 37 +++++- .github/workflows/eval.yml | 14 +++ .github/workflows/pull-request-target.yml | 22 ++++ ci/github-script/check-target-branch.js | 10 +- ci/github-script/manual-file-edits.js | 26 ++++- ci/github-script/prepare.js | 16 ++- ci/github-script/reviews.js | 130 +++++++++++++++------- 7 files changed, 192 insertions(+), 63 deletions(-) diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index fa16aec65997..99e83c2c151f 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -16,6 +16,14 @@ on: required: true type: string secrets: + # Can be provided in pull requests because the job it is used in does + # not evaluate untrusted code. + NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY: + required: false + # Can be provided in pull requests because the job it is used in does + # not evaluate untrusted code. + NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY: + required: false # Should only be provided in the merge queue, not in pull requests, # where we're evaluating untrusted code. CACHIX_AUTH_TOKEN_GHA: @@ -45,9 +53,19 @@ jobs: - name: Install dependencies run: npm install bottleneck@2.19.5 + # It's fine to reuse this app in the 'pull-request-target / prepare' job, + # because that job has to run before this one. + - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 + if: github.event_name != 'pull_request' && vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID + id: app-token + with: + client-id: ${{ vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID }} + private-key: ${{ secrets.NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY }} + permission-pull-requests: write + - name: Log current API rate limits env: - GH_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} run: gh api /rate_limit | jq - name: Check commits @@ -56,6 +74,7 @@ jobs: env: TARGETS_STABLE: ${{ fromJSON(inputs.baseBranch).stable && !contains(fromJSON(inputs.headBranch).type, 'development') }} with: + github-token: ${{ steps.app-token.outputs.token || github.token }} script: | const targetsStable = JSON.parse(process.env.TARGETS_STABLE) require('./trusted/ci/github-script/commits.js')({ @@ -68,7 +87,7 @@ jobs: - name: Log current API rate limits env: - GH_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} run: gh api /rate_limit | jq manual-file-edits: @@ -85,25 +104,35 @@ jobs: sparse-checkout: | ci/github-script + - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 + if: github.event_name != 'pull_request' && vars.NIXPKGS_MANUAL_EDIT_CHECK_CLIENT_ID + id: app-token + with: + client-id: ${{ vars.NIXPKGS_MANUAL_EDIT_CHECK_CLIENT_ID }} + private-key: ${{ secrets.NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY }} + permission-pull-requests: write + - name: Log current API rate limits env: - GH_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} run: gh api /rate_limit | jq - name: Discourage manual edits to certain files uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: + github-token: ${{ steps.app-token.outputs.token || github.token }} script: | require('./trusted/ci/github-script/manual-file-edits.js')({ github, context, core, + dry: context.eventName == 'pull_request', repoPath: 'trusted', }) - name: Log current API rate limits env: - GH_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} run: gh api /rate_limit | jq owners: diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index 4939fa5ec329..a8dcaf29456f 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -23,6 +23,10 @@ on: default: false type: boolean secrets: + # Can be provided in pull requests because the job it is used in does + # not evaluate untrusted code. + NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY: + required: false # Should only be provided in the merge queue, not in pull requests, # where we're evaluating untrusted code. CACHIX_AUTH_TOKEN_GHA: @@ -349,10 +353,20 @@ jobs: description, target_url }) + + - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 + if: github.event_name == 'pull_request_target' && vars.NIXPKGS_BRANCH_CHECK_CLIENT_ID + id: app-token + with: + client-id: ${{ vars.NIXPKGS_BRANCH_CHECK_CLIENT_ID }} + private-key: ${{ secrets.NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY }} + permission-pull-requests: write + - name: Request changes if PR is against an inappropriate branch if: ${{ github.event_name == 'pull_request_target' }} uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: + github-token: ${{ steps.app-token.outputs.token || github.token }} script: | require('./nixpkgs/trusted/ci/github-script/check-target-branch.js')({ github, diff --git a/.github/workflows/pull-request-target.yml b/.github/workflows/pull-request-target.yml index 190ce2510a3e..02d07344eee8 100644 --- a/.github/workflows/pull-request-target.yml +++ b/.github/workflows/pull-request-target.yml @@ -10,6 +10,12 @@ on: secrets: NIXPKGS_CI_APP_PRIVATE_KEY: required: true + NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY: + required: false + NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY: + required: false + NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY: + required: false concurrency: group: pr-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }} @@ -36,6 +42,17 @@ jobs: sparse-checkout-cone-mode: true # default, for clarity sparse-checkout: | ci/github-script + + # It's fine to reuse this app in the 'check / commits' job, + # because this job has to run before that one. + - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 + if: vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID + id: app-token + with: + client-id: ${{ vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID }} + private-key: ${{ secrets.NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY }} + permission-pull-requests: write + - id: prepare uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: @@ -60,6 +77,9 @@ jobs: permissions: # cherry-picks pull-requests: write + secrets: + NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY }} + NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY }} with: baseBranch: ${{ needs.prepare.outputs.baseBranch }} headBranch: ${{ needs.prepare.outputs.headBranch }} @@ -82,6 +102,8 @@ jobs: # compare pull-requests: write statuses: write + secrets: + NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY }} with: artifact-prefix: ${{ inputs.artifact-prefix }} mergedSha: ${{ needs.prepare.outputs.mergedSha }} diff --git a/ci/github-script/check-target-branch.js b/ci/github-script/check-target-branch.js index 9b47a946b889..a31520c02fc6 100644 --- a/ci/github-script/check-target-branch.js +++ b/ci/github-script/check-target-branch.js @@ -151,11 +151,9 @@ async function checkTargetBranch({ github, context, core, dry }) { core, dry, body, - event: 'COMMENT', + event: 'REQUEST_CHANGES', reviewKey, }) - - throw new Error('This PR is against the wrong branch.') } else if (rebuildsAllTests && !isExemptKernelUpdate) { let branchText if (base === 'master' && maxRebuildCount >= 500) { @@ -179,11 +177,9 @@ async function checkTargetBranch({ github, context, core, dry }) { core, dry, body, - event: 'COMMENT', + event: 'REQUEST_CHANGES', reviewKey, }) - - throw new Error('This PR is against the wrong branch.') } else if ( maxRebuildCount >= 500 && !isExemptKernelUpdate && @@ -204,7 +200,7 @@ async function checkTargetBranch({ github, context, core, dry }) { core, dry, body, - event: 'COMMENT', + event: 'REQUEST_CHANGES', reviewKey, }) } else { diff --git a/ci/github-script/manual-file-edits.js b/ci/github-script/manual-file-edits.js index e40d6decbb7d..84235d44752c 100644 --- a/ci/github-script/manual-file-edits.js +++ b/ci/github-script/manual-file-edits.js @@ -7,9 +7,13 @@ const { getCommitDetailsForPR } = require('./get-pr-commit-details') * context: import('@actions/github/lib/context').Context, * core: import('@actions/core'), * repoPath?: string, + * dry: boolean, * }} CheckManualFileEditsProps */ -async function checkManualFileEdits({ github, context, core, repoPath }) { +async function checkManualFileEdits({ github, context, core, repoPath, dry }) { + const { dismissReviews, postReview } = require('./reviews.js') + const reviewKey = 'manual-file-edits' + const pull_number = context.payload.pull_request?.number if (!pull_number) { core.info('This is not a pull request. Skipping checks.') @@ -35,8 +39,13 @@ async function checkManualFileEdits({ github, context, core, repoPath }) { changedPaths.includes('maintainers/github-teams.json'), ) ) { - core.setFailed( - [ + postReview({ + github, + context, + core, + dry, + event: 'REQUEST_CHANGES', + body: [ 'maintainers/github-teams.json is supposed to accurately reflect the state of the teams in GitHub.\n', 'Therefore, it should not be edited manually.\n', 'All changes to teams listed in maintainers/github-teams.json should be performed in GitHub by a team maintainer.\n', @@ -48,7 +57,16 @@ async function checkManualFileEdits({ github, context, core, repoPath }) { (prev, curr) => prev + (!prev || prev.endsWith('\n') ? '' : ' ') + curr, '', ), - ) + reviewKey, + }) + } else { + dismissReviews({ + github, + context, + core, + dry, + reviewKey, + }) } } diff --git a/ci/github-script/prepare.js b/ci/github-script/prepare.js index dfe2d93f3d69..d4d69eb71692 100644 --- a/ci/github-script/prepare.js +++ b/ci/github-script/prepare.js @@ -172,14 +172,20 @@ module.exports = async ({ github, context, core, dry }) => { ' ```', ].join('\n') - await postReview({ github, context, core, dry, body, reviewKey }) - - throw new Error(`The PR contains commits from a different base.`) + await postReview({ + github, + context, + core, + dry, + body, + event: 'REQUEST_CHANGES', + reviewKey, + }) } + } else { + await dismissReviews({ github, context, core, dry, reviewKey }) } - await dismissReviews({ github, context, core, dry, reviewKey }) - let mergedSha, targetSha if (prInfo.mergeable) { diff --git a/ci/github-script/reviews.js b/ci/github-script/reviews.js index 7041fdae1f10..80e250cfa7c3 100644 --- a/ci/github-script/reviews.js +++ b/ci/github-script/reviews.js @@ -5,10 +5,28 @@ const eventToState = { REQUEST_CHANGES: 'CHANGES_REQUESTED', } +// Use substring checks in order to allow testing in forks +// Usernames must also end in "[bot]" +const reviewUsers = [ + 'github-actions', + 'nixpkgs-ci', + 'branch-check', + 'commit-check', + 'manual-edit', +] + +/** + * @typedef {InstanceType} GitHub + * @typedef {typeof import('@actions/github').context} Context + * + * @typedef {Awaited>['data'][number]} Review + * @typedef {Review & { user: NonNullable }} ReviewWithNonNullUser + */ + /** * @param {{ - * github: InstanceType, - * context: import('@actions/github/lib/context').Context, + * github: GitHub, + * context: Context, * core: import('@actions/core'), * dry: boolean, * reviewKey?: string, @@ -25,18 +43,32 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) { return } - const reviews = ( - await github.paginate(github.rest.pulls.listReviews, { - ...context.repo, - pull_number, - }) - ).filter( - (review) => - review.user?.login === 'github-actions[bot]' && - review.state !== 'DISMISSED', + const allReviews = await github.paginate(github.rest.pulls.listReviews, { + ...context.repo, + pull_number, + }) + + const reviews = /** @type {ReviewWithNonNullUser[]} */ ( + allReviews.filter( + (review) => + review.user && + review.state !== 'DISMISSED' && + review.user.login.endsWith('[bot]') && + reviewUsers.some((substr) => review.user?.login.includes(substr)), + ) ) - const changesRequestedReviews = reviews.filter( - (review) => review.state === 'CHANGES_REQUESTED', + + const reviewsByUser = reviews.reduce( + (prev, curr) => { + if (!(curr.user.login in prev)) { + prev[curr.user.login] = [] + } + + prev[curr.user.login].push(curr) + + return prev + }, + /** @type {Record } */ ({}), ) const commentRegex = new RegExp( @@ -50,8 +82,8 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) { ) let reviewsToMinimize = reviews - let /** @type {typeof reviews} */ reviewsToDismiss = [] - let /** @type {typeof reviews} */ reviewsToResolve = [] + const /** @type {ReviewWithNonNullUser[]} */ reviewsToDismiss = [] + const /** @type {ReviewWithNonNullUser[]} */ reviewsToResolve = [] if (reviewKey && reviews.every((review) => commentRegex.test(review.body))) { reviewsToMinimize = reviews.filter((review) => @@ -59,29 +91,39 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) { ) } - // If we want to dismiss all reviews with the key reviewKey, - // but there are other requested changes from CI, we can't dismiss, - // because then the other requested changes will be dismissed too. - if ( - changesRequestedReviews.every( - (review) => - commentResolvedRegex.test(review.body) || - (reviewKey && reviewKeyRegex.test(review.body)) || - // If we are called by check-commits and the review body is clearly - // from `commits.js`, then we can safely dismiss the review. - // This helps with pre-existing reviews (before the comments were added). - (reviewKey && - reviewKey === 'check-commits' && - review.body.includes('PR / Check / cherry-pick')), - ) - ) { - reviewsToDismiss = changesRequestedReviews - } else if (reviewsToMinimize.length) { - reviewsToResolve = reviewsToMinimize.filter( - (review) => - review.state === 'CHANGES_REQUESTED' && - !commentResolvedRegex.test(review.body), - ) + for (const reviewsForUser of Object.values(reviewsByUser)) { + // Make sure that we don't dismiss all reviews by a user if they + // have any reviews we don't want to dismiss. + if ( + reviewsForUser.every( + (review) => + commentResolvedRegex.test(review.body) || + (reviewKey && reviewKeyRegex.test(review.body)) || + // If we are called by check-commits and the review body is clearly + // from `commits.js`, then we can safely dismiss the review. + // This helps with pre-existing reviews (before the comments were added). + (reviewKey && + reviewKey === 'check-commits' && + review.body.includes('PR / Check / cherry-pick')), + ) + ) { + reviewsToDismiss.push( + ...reviewsForUser.filter( + (review) => review.state === 'CHANGES_REQUESTED', + ), + ) + } else { + reviewsToResolve.push( + ...reviewsForUser.filter( + (review) => + review.state === 'CHANGES_REQUESTED' && + !commentResolvedRegex.test(review.body) && + reviewsToMinimize.some( + (toMinimize) => toMinimize.node_id === review.node_id, + ), + ), + ) + } } await Promise.all([ @@ -121,8 +163,8 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) { /** * @param {{ - * github: InstanceType, - * context: import('@actions/github/lib/context').Context + * github: GitHub, + * context: Context, * core: import('@actions/core'), * dry: boolean, * body: string, @@ -158,11 +200,13 @@ async function postReview({ }) ).filter( (review) => - review.user?.login === 'github-actions[bot]' && - review.state !== 'DISMISSED', + review.user && + review.state !== 'DISMISSED' && + review.user.login.endsWith('[bot]') && + reviewUsers.some((substr) => review.user?.login.includes(substr)), ) - /** @type {null | typeof reviews[number]} */ + /** @type {null | Review} */ let pendingReview const matchingReviews = reviews.filter((review) => reviewKeyRegex.test(review.body),