ci: make reviews sticky; ci/github-script/check-target-branch: do not "Request changes", add exemptions (#483828)

2026-06-05 21:03:40 +00:00 · 2026-01-26 23:17:38 +00:00
parent d126634798 c9225057c9
commit c693894f8a
4 changed files with 255 additions and 58 deletions
--- a/ci/github-script/check-target-branch.js
+++ b/ci/github-script/check-target-branch.js
@@ -6,8 +6,9 @@

 const { classify, split } = require('../supportedBranches.js')
 const { readFile } = require('node:fs/promises')
-const { postReview } = require('./reviews.js')
+const { postReview, dismissReviews } = require('./reviews.js')

+const reviewKey = 'check-target-branch'
 /**
 * @param {{
 *  github: InstanceType<import('@actions/github/lib/utils').GitHub>,
@@ -17,6 +18,19 @@ const { postReview } = require('./reviews.js')
 * }} CheckTargetBranchProps
 */
 async function checkTargetBranch({ github, context, core, dry }) {
+  /**
+   * @type {{
+   *  attrdiff: {
+   *   added: string[],
+   *   changed: string[],
+   *   removed: string[],
+   *  },
+   *  labels: Record<string, boolean>,
+   *  rebuildCountByKernel: Record<string, number>,
+   *  rebuildsByKernel: Record<string, string[]>,
+   *  rebuildsByPlatform: Record<string, string[]>,
+   * }}
+   */
  const changed = JSON.parse(
    await readFile('comparison/changed-paths.json', 'utf-8'),
  )
@@ -43,6 +57,15 @@ async function checkTargetBranch({ github, context, core, dry }) {
    core.info(
      `Skipping checkTargetBranch: PR is from a development branch (${head})`,
    )
+
+    await dismissReviews({
+      github,
+      context,
+      core,
+      dry,
+      reviewKey,
+    })
+
    return
  }
  // Don't run on PRs against staging branches, wip branches, haskell-updates, etc.
@@ -50,6 +73,15 @@ async function checkTargetBranch({ github, context, core, dry }) {
    core.info(
      `Skipping checkTargetBranch: PR is against a non-primary base branch (${base})`,
    )
+
+    await dismissReviews({
+      github,
+      context,
+      core,
+      dry,
+      reviewKey,
+    })
+
    return
  }

@@ -57,12 +89,40 @@ async function checkTargetBranch({ github, context, core, dry }) {
    ...Object.values(changed.rebuildCountByKernel),
  )
  const rebuildsAllTests =
-    changed.attrdiff.changed.includes('nixosTests.simple') ?? false
+    changed.attrdiff.changed.includes('nixosTests.simple')
+
+  // https://github.com/NixOS/nixpkgs/pull/481205#issuecomment-3790123921
+  // These should go to staging-nixos instead of master,
+  // but release-xx.xx (not staging-xx.xx) when backported
+  let isExemptKernelUpdate = false
+  if (prInfo.changed_files === 1 && base.startsWith('release-')) {
+    const changedFiles = (
+      await github.rest.pulls.listFiles({
+        ...context.repo,
+        pull_number,
+      })
+    ).data
+    isExemptKernelUpdate =
+      changedFiles.length === 1 &&
+      changedFiles[0].filename ===
+        'pkgs/os-specific/linux/kernel/kernels-org.json'
+  }
+
+  // https://github.com/NixOS/nixpkgs/pull/483194#issuecomment-3793393218
+  const isExemptHomeAssistantUpdate =
+    maxRebuildCount <= 1500 && head === 'wip-home-assistant'
+
  core.info(
-    `checkTargetBranch: PR causes ${maxRebuildCount} rebuilds and ${rebuildsAllTests ? 'does' : 'does not'} rebuild all NixOS tests.`,
+    [
+      `checkTargetBranch: this PR:`,
+      `  * causes ${maxRebuildCount} rebuilds`,
+      `  * ${rebuildsAllTests ? 'rebuilds' : 'does not rebuild'} all NixOS tests`,
+      `  * ${isExemptKernelUpdate ? 'is' : 'is not'} an exempt kernel update`,
+      `  * ${isExemptHomeAssistantUpdate ? 'is' : 'is not'} an exempt home-assistant update`,
+    ].join('\n'),
  )

-  if (maxRebuildCount >= 1000) {
+  if (maxRebuildCount >= 1000 && !isExemptHomeAssistantUpdate) {
    const desiredBranch =
      base === 'master' ? 'staging' : `staging-${split(base).version}`
    const body = [
@@ -77,11 +137,12 @@ async function checkTargetBranch({ github, context, core, dry }) {
      core,
      dry,
      body,
-      event: 'REQUEST_CHANGES',
+      event: 'COMMENT',
+      reviewKey,
    })

    throw new Error('This PR is against the wrong branch.')
-  } else if (rebuildsAllTests) {
+  } else if (rebuildsAllTests && !isExemptKernelUpdate) {
    let branchText
    if (base === 'master' && maxRebuildCount >= 500) {
      branchText = '(probably either `staging-nixos` or `staging`)'
@@ -104,11 +165,16 @@ async function checkTargetBranch({ github, context, core, dry }) {
      core,
      dry,
      body,
-      event: 'REQUEST_CHANGES',
+      event: 'COMMENT',
+      reviewKey,
    })

    throw new Error('This PR is against the wrong branch.')
-  } else if (maxRebuildCount >= 500) {
+  } else if (
+    maxRebuildCount >= 500 &&
+    !isExemptKernelUpdate &&
+    !isExemptHomeAssistantUpdate
+  ) {
    const stagingBranch =
      base === 'master' ? 'staging' : `staging-${split(base).version}`
    const body = [
@@ -125,10 +191,18 @@ async function checkTargetBranch({ github, context, core, dry }) {
      dry,
      body,
      event: 'COMMENT',
+      reviewKey,
    })
  } else {
-    // Any existing reviews were dismissed by commits.js
    core.info('checkTargetBranch: this PR is against an appropriate branch.')
+
+    await dismissReviews({
+      github,
+      context,
+      core,
+      dry,
+      reviewKey,
+    })
  }
 }

--- a/ci/github-script/commits.js
+++ b/ci/github-script/commits.js
@@ -3,6 +3,7 @@ module.exports = async ({ github, context, core, dry, cherryPicks }) => {
  const { classify } = require('../supportedBranches.js')
  const withRateLimit = require('./withRateLimit.js')
  const { dismissReviews, postReview } = require('./reviews.js')
+  const reviewKey = 'check-commits'

  await withRateLimit({ github, core }, async (stats) => {
    stats.prs = 1
@@ -193,7 +194,7 @@ module.exports = async ({ github, context, core, dry, cherryPicks }) => {
    // An empty results array will always trigger this condition, which is helpful
    // to clean up reviews created by the prepare step when on the wrong branch.
    if (results.every(({ severity }) => severity === 'info')) {
-      await dismissReviews({ github, context, dry })
+      await dismissReviews({ github, context, dry, reviewKey })
      return
    }

@@ -316,6 +317,6 @@ module.exports = async ({ github, context, core, dry, cherryPicks }) => {
    // Posting a review could fail for very long comments. This can only happen with
    // multiple commits all hitting the truncation limit for the diff. If you ever hit
    // this case, consider just splitting up those commits into multiple PRs.
-    await postReview({ github, context, core, dry, body })
+    await postReview({ github, context, core, dry, body, reviewKey })
  })
 }
--- a/ci/github-script/prepare.js
+++ b/ci/github-script/prepare.js
@@ -1,5 +1,6 @@
 const { classify } = require('../supportedBranches.js')
-const { postReview } = require('./reviews.js')
+const { postReview, dismissReviews } = require('./reviews.js')
+const reviewKey = 'prepare'

 module.exports = async ({ github, context, core, dry }) => {
  const pull_number = context.payload.pull_request.number
@@ -46,7 +47,7 @@ module.exports = async ({ github, context, core, dry }) => {
        `Please target \`${correctBranch}\` instead.`,
      ].join('\n')

-      await postReview({ github, context, core, dry, body })
+      await postReview({ github, context, core, dry, body, reviewKey })

      throw new Error('The PR targets a channel branch.')
    }
@@ -170,12 +171,14 @@ module.exports = async ({ github, context, core, dry }) => {
          '  ```',
        ].join('\n')

-        await postReview({ github, context, core, dry, body })
+        await postReview({ github, context, core, dry, body, reviewKey })

        throw new Error(`The PR contains commits from a different base.`)
      }
    }

+    await dismissReviews({ github, context, core, dry, reviewKey })
+
    let mergedSha, targetSha

    if (prInfo.mergeable) {
--- a/ci/github-script/reviews.js
+++ b/ci/github-script/reviews.js
@@ -1,3 +1,5 @@
+// @ts-check
+
 const eventToState = {
  COMMENT: 'COMMENTED',
  REQUEST_CHANGES: 'CHANGES_REQUESTED',
@@ -6,49 +8,122 @@ const eventToState = {
 /**
 * @param {{
 *  github: InstanceType<import('@actions/github/lib/utils').GitHub>,
- *  context: import('@actions/github/lib/context').Context
- *  core: import('@actions/core')
- *  dry: boolean
- * }} CheckTargetBranchProps
+ *  context: import('@actions/github/lib/context').Context,
+ *  core: import('@actions/core'),
+ *  dry: boolean,
+ *  reviewKey?: string,
+ * }} DismissReviewsProps
 */
-async function dismissReviews({ github, context, dry }) {
-  const pull_number = context.payload.pull_request.number
+async function dismissReviews({ github, context, core, dry, reviewKey }) {
+  const pull_number = context.payload.pull_request?.number
+  if (!pull_number) {
+    core.warning('dismissReviews called outside of pull_request context')
+    return
+  }

  if (dry) {
    return
  }

-  await Promise.all(
-    (
-      await github.paginate(github.rest.pulls.listReviews, {
+  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 changesRequestedReviews = reviews.filter(
+    (review) => review.state === 'CHANGES_REQUESTED',
+  )
+
+  const commentRegex = new RegExp(
+    /<!-- nixpkgs review key: (.*)(?:; resolved: .*)? -->/,
+  )
+  const reviewKeyRegex = new RegExp(
+    `<!-- (nixpkgs review key: ${reviewKey})(?:; resolved: .*)? -->`,
+  )
+  const commentResolvedRegex = new RegExp(
+    /<!-- nixpkgs review key: .*; resolved: true -->/,
+  )
+
+  let reviewsToMinimize = reviews
+  let /** @type {typeof reviews} */ reviewsToDismiss = []
+  let /** @type {typeof reviews} */ reviewsToResolve = []
+
+  if (reviewKey && reviews.every((review) => commentRegex.test(review.body))) {
+    reviewsToMinimize = reviews.filter((review) =>
+      reviewKeyRegex.test(review.body),
+    )
+  }
+
+  // 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)),
+    )
+  ) {
+    reviewsToDismiss = changesRequestedReviews
+  } else if (reviewsToMinimize.length) {
+    reviewsToResolve = reviewsToMinimize.filter(
+      (review) =>
+        review.state === 'CHANGES_REQUESTED' &&
+        !commentResolvedRegex.test(review.body),
+    )
+  }
+
+  await Promise.all([
+    ...reviewsToMinimize.map(async (review) =>
+      github.graphql(
+        `mutation($node_id:ID!) {
+              minimizeComment(input: {
+                classifier: OUTDATED,
+                subjectId: $node_id
+              })
+              { clientMutationId }
+            }`,
+        { node_id: review.node_id },
+      ),
+    ),
+    ...reviewsToDismiss.map(async (review) =>
+      github.rest.pulls.dismissReview({
        ...context.repo,
        pull_number,
-      })
-    )
-      .filter((review) => review.user?.login === 'github-actions[bot]')
-      .map(async (review) => {
-        if (review.state === 'CHANGES_REQUESTED') {
-          await github.rest.pulls.dismissReview({
-            ...context.repo,
-            pull_number,
-            review_id: review.id,
-            message: 'Review dismissed automatically',
-          })
-        }
-        await github.graphql(
-          `mutation($node_id:ID!) {
-            minimizeComment(input: {
-              classifier: OUTDATED,
-              subjectId: $node_id
-            })
-            { clientMutationId }
-          }`,
-          { node_id: review.node_id },
-        )
+        review_id: review.id,
+        message: 'Review dismissed automatically',
      }),
-  )
+    ),
+    ...reviewsToResolve.map(async (review) =>
+      github.rest.pulls.updateReview({
+        ...context.repo,
+        pull_number,
+        review_id: review.id,
+        body: review.body.replace(
+          reviewKeyRegex,
+          `<!-- nixpkgs review key: ${reviewKey}; resolved: true -->`,
+        ),
+      }),
+    ),
+  ])
 }

+/**
+ * @param {{
+ *  github: InstanceType<import('@actions/github/lib/utils').GitHub>,
+ *  context: import('@actions/github/lib/context').Context
+ *  core: import('@actions/core'),
+ *  dry: boolean,
+ *  body: string,
+ *  event: keyof eventToState,
+ *  reviewKey: string,
+ * }} PostReviewProps
+ */
 async function postReview({
  github,
  context,
@@ -56,22 +131,55 @@ async function postReview({
  dry,
  body,
  event = 'REQUEST_CHANGES',
+  reviewKey,
 }) {
-  const pull_number = context.payload.pull_request.number
+  const pull_number = context.payload.pull_request?.number
+  if (!pull_number) {
+    core.warning('postReview called outside of pull_request context')
+    return
+  }

-  const pendingReview = (
+  const reviewKeyRegex = new RegExp(
+    `<!-- (nixpkgs review key: ${reviewKey})(?:; resolved: .*)? -->`,
+  )
+  const reviewKeyComment = `<!-- nixpkgs review key: ${reviewKey}; resolved: false -->`
+  body = body + '\n\n' + reviewKeyComment
+
+  const reviews = (
    await github.paginate(github.rest.pulls.listReviews, {
      ...context.repo,
      pull_number,
    })
-  ).find(
+  ).filter(
    (review) =>
      review.user?.login === 'github-actions[bot]' &&
-      // If a review is still pending, we can just update this instead
-      // of posting a new one.
-      review.state === eventToState[event],
+      review.state !== 'DISMISSED',
  )

+  /** @type {null | typeof reviews[number]} */
+  let pendingReview
+  const matchingReviews = reviews.filter((review) =>
+    reviewKeyRegex.test(review.body),
+  )
+
+  if (matchingReviews.length === 0) {
+    pendingReview = null
+  } else if (
+    matchingReviews.length === 1 &&
+    matchingReviews[0].state === eventToState[event]
+  ) {
+    pendingReview = matchingReviews[0]
+  } else {
+    await dismissReviews({
+      github,
+      context,
+      core,
+      dry,
+      reviewKey,
+    })
+    pendingReview = null
+  }
+
  if (dry) {
    if (pendingReview)
      core.info(`pending review found: ${pendingReview.html_url}`)
@@ -79,12 +187,23 @@ async function postReview({
    core.info(body)
  } else {
    if (pendingReview) {
-      await github.rest.pulls.updateReview({
-        ...context.repo,
-        pull_number,
-        review_id: pendingReview.id,
-        body,
-      })
+      await Promise.all([
+        github.rest.pulls.updateReview({
+          ...context.repo,
+          pull_number,
+          review_id: pendingReview.id,
+          body,
+        }),
+        github.graphql(
+          `mutation($node_id:ID!) {
+              unminimizeComment(input: {
+                subjectId: $node_id
+              })
+              { clientMutationId }
+            }`,
+          { node_id: pendingReview.node_id },
+        ),
+      ])
    } else {
      await github.rest.pulls.createReview({
        ...context.repo,