review-checklist: enforce one load-balanced reviewer and fix checklist display (#6439)

* review-checklist: show all requested owners per area in checklist

buildBody was using area.owners.find() — picking the first CODEOWNERS-listed
owner in the requested set. This broke reviewer swaps (removing @A and adding
@b would show @C, the next CODEOWNERS entry, not @b) and also failed to
reflect GitHub's CODEOWNERS auto-assignment, which requests all owners.

Switch to area.owners.filter() so every requested owner for an area is
mentioned in that row. Approval logic is unchanged: any owner approval
still checks the box.

* review-checklist: enforce one load-balanced reviewer per area

GitHub's CODEOWNERS auto-assignment requests all owners of touched files
when a PR opens, before the workflow runs. The script was then seeing them
already assigned and skipping its own selection entirely.

On opened/reopened: select one load-balanced reviewer per area (ignoring
GitHub's pre-assigned set), remove any auto-assigned CODEOWNERS not in the
selection, then add the chosen reviewer. Only code owners are removed —
manually-added non-owner reviewers are left untouched.

On synchronize: keep existing assignments (reviewer may have already started).

confirmedRequested now starts empty and is populated only with the script's
selection, so the checklist only mentions owners that were explicitly chosen.

* review-checklist: retry reviewer cleanup to handle GitHub auto-assign race

GitHub's CODEOWNERS auto-assignment can fire after the workflow starts,
re-adding extra reviewers after we remove them. Add a 15-second wait
followed by a second cleanup pass on opened/reopened events.

Extract the removal loop into enforceSelection() so both passes share
the same logic.

* zizmor: suppress template-injection for PR #6356 workflow files

Add zizmor_config.yml to ignore template-injection findings in the
setup-jextract action and maven/ctest workflow files introduced in
PR #6356, where inputs are caller-controlled but not user-controlled.

* review-checklist: don't re-assign reviewers on synchronize

On synchronize, chooseReviewers saw no owner assigned (because the
reviewer was manually removed) and re-added one via requestReviewers,
overriding the manual removal.

Reviewer assignment now only happens on opened/reopened. Synchronize
only updates the checklist display based on whoever is currently
assigned — manual removals are respected.

* zizmor: skip upload-sarif failure on fork PRs

* zizmor: move config out of workflows dir to fix GitHub Actions parse error

GitHub Actions parses all .yml files in .github/workflows/ as workflow
files; zizmor_config.yml there caused "unexpected value 'rules'" because
rules: is not valid workflow syntax. Moved to .github/zizmor.yml and
updated the --config path in zizmor.yml accordingly.

* review-checklist: retry on transient 401 from GitHub API

GitHub's API intermittently returns 401 on write operations
(issues.addAssignees, issues.createComment) even when the token has
Issues: write and PullRequests: write — read-only calls succeed in the
same run. The github-script action excludes 401 from retries by default.
Removing 401 from retry-exempt-status-codes and setting retries: 3
handles these transient failures with exponential backoff.

* review-checklist: fix checklist mention when non-requested owner approves

The mention in each checklist row was derived from the approver (if any),
so when a different area owner happened to approve first, the mention
changed from the assigned reviewer to the approver. Fix by decoupling
sign-off detection from display: signedOff now uses .some() so any
owner's approval checks the box, while the mention always shows the
confirmed-requested reviewer(s) via .filter(). Also shows multiple
reviewers when one is manually added alongside the load-balanced
selection.

* review-checklist: show approver name when signed off, requested reviewer(s) when pending

When an area is signed off, replace the mention with the approver so the
checklist shows who actually reviewed it (which may differ from whoever
was load-balanced as the requested reviewer). When pending, show all
confirmed-requested reviewers — the one load-balanced pick normally, but
two if a reviewer was manually added alongside it.

* zizmor: remove config file and --config flag

The ignored files (ctest.yml, maven-deploy.yml, maven-staging.yml,
setup-jextract/action.yml) had template-injection findings on
inputs.* references, which are not attacker-controllable. Rather than
suppressing them via a config file, let all findings surface in the
Security tab and address them individually if needed.

Author: brtnfld

.github/scripts/review-checklist.js

@@ -159,11 +159,15 @@ function chooseReviewers(touchedAreas, {
 function buildBody(touchedAreas, approvedUsers, confirmedRequested) {
   const rowData = touchedAreas.map(area => {
     const approver  = area.owners.find(o => approvedUsers.has(o));
-    const assigned  = approver ?? area.owners.find(o => confirmedRequested.has(o));
     const signedOff = !!approver;
     const box       = signedOff ? 'x' : ' ';
     const tick      = signedOff ? ' ✅' : '';
-    const mention   = assigned ? ` — @${assigned}` : '';
+    // Signed off: show who approved. Pending: show all confirmed-requested reviewers
+    // (may be > 1 if a reviewer was manually added alongside the load-balanced pick).
+    const requested = area.owners.filter(o => confirmedRequested.has(o));
+    const mention   = approver
+      ? ` — @${approver}`
+      : requested.length > 0 ? ` — ${requested.map(o => `@${o}`).join(', ')}` : '';
     return { text: `- [${box}] **${area.label}**${tick}${mention}`, signedOff };
   });
 
@@ -357,10 +361,10 @@ module.exports = async function run({ github, context, core }) {
     prData.requested_reviewers.map(r => r.login).filter(Boolean)
   );
 
-  // confirmedRequested tracks what was actually successfully requested
-  // (used in buildBody so the checklist never shows an owner whose
-  // requestReviewers call silently failed).
-  let confirmedRequested = new Set(existingRequested);
+  // confirmedRequested tracks who is actually assigned for checklist display.
+  // Starts empty — populated below only with the load-balanced selection so
+  // the checklist never shows owners that were not chosen by the script.
+  let confirmedRequested = new Set();
 
   // ----------------------------------------------------------------
   // 6. Auto-assign reviewers (pull_request events only, not reviews).
@@ -400,30 +404,80 @@ module.exports = async function run({ github, context, core }) {
       core.warning(`Could not fetch open PRs for load balancing; falling back to CODEOWNERS order: ${e.message}`);
     }
 
+    const isNewPR = context.payload.action === 'opened' || context.payload.action === 'reopened';
+
+    // On open/reopen: ignore existing assignments so GitHub's CODEOWNERS
+    // auto-assignment doesn't suppress our load-balanced selection.
+    // On synchronize: respect existing assignments (reviewer may have already started).
     const { selected, log } = chooseReviewers(touchedAreas, {
       prAuthor,
-      existingRequested,
+      existingRequested: isNewPR ? new Set() : existingRequested,
       reviewerLoad,
       LINE_THRESHOLD,
       AREA_THRESHOLDS,
       PUBLIC_HEADER,
     });
     for (const msg of log) core.info(msg);
 
-    // Request one at a time so a single invalid login cannot block the rest.
-    // Only add to confirmedRequested on success — the checklist must not show
-    // an owner whose request call failed.
-    for (const reviewer of selected) {
+    // Helper: remove CODEOWNERS auto-assigned reviewers not in our selection.
+    // Only removes code owners — leaves manually-added non-owner reviewers untouched.
+    async function enforceSelection(currentRequested) {
+      for (const reviewer of currentRequested) {
+        if (allCodeOwners.has(reviewer) && !selected.has(reviewer)) {
+          try {
+            await github.rest.pulls.removeRequestedReviewers({
+              owner, repo, pull_number: pr_number, reviewers: [reviewer],
+            });
+            core.info(`Removed auto-assigned reviewer ${reviewer} (not selected by load balancer)`);
+          } catch (e) {
+            core.warning(`Could not remove reviewer ${reviewer}: ${e.message}`);
+          }
+        }
+      }
+    }
+
+    if (isNewPR) {
+      // First pass: clean up whatever GitHub auto-assigned before the workflow ran.
+      await enforceSelection(existingRequested);
+
+      // Request one at a time so a single invalid login cannot block the rest.
+      // Only add to confirmedRequested on success — the checklist must not show
+      // an owner whose request call failed.
+      for (const reviewer of selected) {
+        try {
+          await github.rest.pulls.requestReviewers({
+            owner, repo, pull_number: pr_number,
+            reviewers: [reviewer],
+          });
+          confirmedRequested.add(reviewer);
+        } catch (e) {
+          core.warning(`Could not request reviewer ${reviewer}: ${e.message}`);
+        }
+      }
+
+      // Second pass: GitHub's auto-assignment can fire after the workflow starts,
+      // so wait briefly then re-check and remove any extras that appeared.
+      await new Promise(resolve => setTimeout(resolve, 15000));
+      let retryPR;
       try {
-        await github.rest.pulls.requestReviewers({
-          owner, repo, pull_number: pr_number,
-          reviewers: [reviewer],
-        });
-        confirmedRequested.add(reviewer);
+        ({ data: retryPR } = await github.rest.pulls.get({ owner, repo, pull_number: pr_number }));
       } catch (e) {
-        core.warning(`Could not request reviewer ${reviewer}: ${e.message}`);
+        core.warning(`Could not re-fetch PR for reviewer cleanup retry: ${e.message}`);
       }
+      if (retryPR) {
+        const retryRequested = new Set(
+          retryPR.requested_reviewers.map(r => r.login).filter(Boolean)
+        );
+        await enforceSelection(retryRequested);
+      }
+    } else {
+      // synchronize/reopened: never re-assign reviewers — respect manual removals.
+      // Just carry forward whoever is currently assigned for checklist display.
+      for (const reviewer of existingRequested) confirmedRequested.add(reviewer);
     }
+  } else {
+    // For workflow_run (review events): show whoever is currently assigned.
+    for (const reviewer of existingRequested) confirmedRequested.add(reviewer);
   }
 
   // ----------------------------------------------------------------

.github/scripts/review-checklist.test.js

@@ -378,6 +378,21 @@ test('buildBody: area with no confirmed reviewer shows no @-mention', () => {
   assert.ok(!body.includes('@alice'));
 });
 
+test('buildBody: mention shows approver when a non-requested owner signs off', () => {
+  // alice was load-balanced as the reviewer; bob (also an owner) approves instead
+  const areas = [makeArea('src', ['alice', 'bob'], 10)];
+  const body  = buildBody(areas, new Set(['bob']), new Set(['alice']));
+  assert.ok(body.includes('- [x] **src** ✅'));
+  assert.ok(body.includes('— @bob'));
+  assert.ok(!body.includes('@alice'));
+});
+
+test('buildBody: shows multiple requested reviewers when more than one is assigned', () => {
+  const areas = [makeArea('src', ['alice', 'bob'], 10)];
+  const body  = buildBody(areas, new Set(), new Set(['alice', 'bob']));
+  assert.ok(body.includes('— @alice, @bob'));
+});
+
 test('buildBody: always contains the marker', () => {
   const areas = [makeArea('src', ['alice'], 10)];
   const body  = buildBody(areas, new Set(), new Set());

.github/workflows/review-checklist.yml

@@ -50,6 +50,8 @@ jobs:
           persist-credentials: false
       - uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0
         with:
+          retries: 3
+          retry-exempt-status-codes: 400,403,404,422
           script: |
             const run = require('./.github/scripts/review-checklist.js');
             await run({ github, context, core });

.github/workflows/zizmor.yml

@@ -33,6 +33,7 @@ jobs:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
       - uses: github/codeql-action/upload-sarif@8aad20d150bbac5944a9f9d289da16a4b0d87c1e # v4.36.2
         if: always()
+        continue-on-error: true
         with:
           sarif_file: zizmor-results.sarif
           category: zizmor